Skip to content

Commit 4b2221d

Browse files
committed
Fix CI test failures: proxy deployment, smock corruption, revert assertions, gas values
- Replace upgrades.deployProxy with manual ERC1967Proxy in Allowlist and DualMode tests to avoid shared ProxyAdmin ownership conflict that broke deploy/11_transfer_proxy_admin_ownership.ts - Replace smock.fake<TokenStaking> with real staking operations in Authorization tests to prevent EVM storage corruption that broke Slashing test suite's seize() calls - Fix governance revert assertions from revertedWithCustomError to revertedWith string matching (onlyGovernance uses require, not custom errors) - Update notification reward expectations to 0 (TokenStaking no longer configures notification rewards) - Recalibrate challengeDkgResult gas expectations to match actual contract gas costs after Allowlist integration
1 parent 3fe9696 commit 4b2221d

7 files changed

Lines changed: 194 additions & 133 deletions

solidity/ecdsa/test/Allowlist.test.ts

Lines changed: 46 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -1,12 +1,14 @@
11
/* eslint-disable @typescript-eslint/no-unused-expressions, no-restricted-syntax, no-await-in-loop */
2-
import { ethers, upgrades } from "hardhat"
2+
import { ethers, helpers } from "hardhat"
33
import { smock } from "@defi-wonderland/smock"
44
import { expect } from "chai"
55

66
import type { FakeContract } from "@defi-wonderland/smock"
77
import type { SignerWithAddress } from "@nomiclabs/hardhat-ethers/signers"
88
import type { Allowlist, WalletRegistry } from "../typechain"
99

10+
const { createSnapshot, restoreSnapshot } = helpers.snapshot
11+
1012
const ZERO_ADDRESS = ethers.constants.AddressZero
1113

1214
describe("Allowlist", () => {
@@ -17,6 +19,14 @@ describe("Allowlist", () => {
1719
let stakingProvider2: SignerWithAddress
1820
let thirdParty: SignerWithAddress
1921

22+
before(async () => {
23+
await createSnapshot()
24+
})
25+
26+
after(async () => {
27+
await restoreSnapshot()
28+
})
29+
2030
beforeEach(async () => {
2131
// Create fake WalletRegistry
2232
walletRegistry = await smock.fake<WalletRegistry>("WalletRegistry")
@@ -28,15 +38,34 @@ describe("Allowlist", () => {
2838
stakingProvider2 = sp2
2939
thirdParty = tp
3040

31-
// Deploy Allowlist as upgradeable proxy (has _disableInitializers in constructor)
41+
// Fund the fake WalletRegistry so it can send transactions when
42+
// impersonated via walletRegistry.wallet (needed for
43+
// approveAuthorizationDecrease which checks msg.sender == walletRegistry).
44+
await governance.sendTransaction({
45+
to: walletRegistry.address,
46+
value: ethers.utils.parseEther("10"),
47+
})
48+
49+
// Deploy Allowlist proxy using ERC1967Proxy directly instead of
50+
// @openzeppelin/hardhat-upgrades' upgrades.deployProxy(). Using
51+
// upgrades.deployProxy with kind:"transparent" creates a shared
52+
// ProxyAdmin in the OZ manifest. Because this test file runs first
53+
// (alphabetically), that ProxyAdmin would be owned by signer[0], but
54+
// the deploy scripts expect signer[1] (named "deployer") to own it.
55+
// This mismatch causes deploy/11_transfer_proxy_admin_ownership.ts to
56+
// fail with "Ownable: caller is not the owner" when other test suites
57+
// call deployments.fixture().
3258
const AllowlistFactory = await ethers.getContractFactory("Allowlist")
33-
const proxy = await upgrades.deployProxy(
34-
AllowlistFactory,
35-
[walletRegistry.address],
36-
{ kind: "transparent" }
59+
const impl = await AllowlistFactory.deploy()
60+
await impl.deployed()
61+
const initData = AllowlistFactory.interface.encodeFunctionData(
62+
"initialize",
63+
[walletRegistry.address]
3764
)
65+
const ERC1967ProxyFactory = await ethers.getContractFactory("ERC1967Proxy")
66+
const proxy = await ERC1967ProxyFactory.deploy(impl.address, initData)
3867
await proxy.deployed()
39-
allowlist = proxy as Allowlist
68+
allowlist = AllowlistFactory.attach(proxy.address) as Allowlist
4069
})
4170

4271
describe("initialization", () => {
@@ -56,11 +85,17 @@ describe("Allowlist", () => {
5685

5786
it("should revert if initialized with zero address", async () => {
5887
const AllowlistFactory = await ethers.getContractFactory("Allowlist")
59-
88+
const impl = await AllowlistFactory.deploy()
89+
await impl.deployed()
90+
const initData = AllowlistFactory.interface.encodeFunctionData(
91+
"initialize",
92+
[ZERO_ADDRESS]
93+
)
94+
const ERC1967ProxyFactory = await ethers.getContractFactory(
95+
"ERC1967Proxy"
96+
)
6097
await expect(
61-
upgrades.deployProxy(AllowlistFactory, [ZERO_ADDRESS], {
62-
kind: "transparent",
63-
})
98+
ERC1967ProxyFactory.deploy(impl.address, initData)
6499
).to.be.reverted
65100
})
66101
})

solidity/ecdsa/test/WalletRegistry.Authorization.test.ts

Lines changed: 69 additions & 66 deletions
Original file line numberDiff line numberDiff line change
@@ -75,35 +75,45 @@ const MAX_UINT64 = ethers.BigNumber.from("18446744073709551615") // 2^64 - 1
7575
*/
7676

7777
/**
78-
* Creates a Smock fake for TokenStaking contract with mocked authorization data.
79-
* Used in Pre-Upgrade and Upgrade Flow tests to simulate stake authorization
80-
* without using deprecated TokenStaking.stake() and increaseAuthorization() methods.
78+
* Sets up real TokenStaking authorization for a staking provider using actual
79+
* contract calls instead of smock.fake. This avoids a known smock issue where
80+
* smock.fake({address: existingAddress}) corrupts EVM storage in a way that
81+
* evm_revert cannot restore, breaking subsequent test suites.
8182
*
82-
* TIP-092 Context: Real TokenStaking contract has no write methods for test setup,
83-
* so we mock the read methods (authorizedStake, rolesOf) to return expected values.
84-
*
85-
* @param stakingAddress - Address of the deployed TokenStaking contract to fake
86-
* @param minimumAuthorization - Amount to return from authorizedStake() calls
87-
* @param stakingProvider - Address to return as owner/authorizer in rolesOf()
88-
* @param beneficiary - Address to return as beneficiary in rolesOf()
89-
* @returns Configured FakeContract<TokenStaking>
83+
* @param t - T token contract for minting
84+
* @param staking - TokenStaking contract
85+
* @param walletRegistry - WalletRegistry contract (application to authorize)
86+
* @param deployer - Deployer signer (can mint tokens)
87+
* @param stakingProvider - Staking provider signer
88+
* @param beneficiary - Beneficiary signer
89+
* @param amount - Amount to stake and authorize
9090
*/
91-
async function createTokenStakingFake(
92-
stakingAddress: string,
93-
minimumAuthorization: any,
91+
async function setupRealStaking(
92+
t: T,
93+
staking: TokenStaking,
94+
walletRegistry: WalletRegistry,
95+
deployer: SignerWithAddress,
9496
stakingProvider: SignerWithAddress,
95-
beneficiary: SignerWithAddress
96-
): Promise<FakeContract<TokenStaking>> {
97-
const stakingFake = await smock.fake<TokenStaking>("TokenStaking", {
98-
address: stakingAddress,
99-
})
100-
stakingFake.authorizedStake.returns(minimumAuthorization)
101-
stakingFake.rolesOf.returns([
102-
stakingProvider.address,
103-
beneficiary.address,
104-
stakingProvider.address,
105-
])
106-
return stakingFake
97+
beneficiary: SignerWithAddress,
98+
amount: any
99+
): Promise<void> {
100+
await t.connect(deployer).mint(stakingProvider.address, amount)
101+
await t.connect(stakingProvider).approve(staking.address, amount)
102+
await staking
103+
.connect(stakingProvider)
104+
.stake(
105+
stakingProvider.address,
106+
beneficiary.address,
107+
stakingProvider.address,
108+
amount
109+
)
110+
await staking
111+
.connect(stakingProvider)
112+
.increaseAuthorization(
113+
stakingProvider.address,
114+
walletRegistry.address,
115+
amount
116+
)
107117
}
108118

109119
/**
@@ -3729,11 +3739,16 @@ describe("WalletRegistry - Migration Scenario Tests (TIP-092)", () => {
37293739
const stakedAmount = to1e18(1000000) // 1M T
37303740

37313741
before("load test fixture", async () => {
3732-
await createSnapshot()
3733-
3734-
// Deploy fixture in default TokenStaking mode
3742+
// Deploy fixture BEFORE taking snapshot. The order matters because
3743+
// deployments.fixture() caches an internal EVM snapshot. If we took
3744+
// our snapshot first (lower ID), restoreSnapshot() would call
3745+
// evm_revert(lowerID) which invalidates ALL snapshots with higher IDs
3746+
// — including the deploy cache. This would break deployments.fixture()
3747+
// for all subsequent test suites (e.g., Slashing, WalletCreation).
37353748
await deployments.fixture()
37363749

3750+
await createSnapshot()
3751+
37373752
t = await helpers.contracts.getContract("T")
37383753
walletRegistry = await helpers.contracts.getContract("WalletRegistry")
37393754
sortitionPool = await helpers.contracts.getContract("EcdsaSortitionPool")
@@ -3770,31 +3785,24 @@ describe("WalletRegistry - Migration Scenario Tests (TIP-092)", () => {
37703785
* Coverage: Tests the false branch of ternary operator in _currentAuthorizationSource()
37713786
*/
37723787
describe("Pre-Upgrade Mode (TokenStaking Authorization)", () => {
3773-
let stakingFake: FakeContract<TokenStaking>
3774-
37753788
before(async () => {
37763789
await createSnapshot()
37773790

3778-
// Setup: Mock TokenStaking authorization using Smock fake
3779-
stakingFake = await createTokenStakingFake(
3780-
staking.address,
3781-
minimumAuthorization,
3791+
// Setup: Use real TokenStaking for authorization (avoids smock.fake
3792+
// storage corruption that breaks evm_revert for subsequent tests)
3793+
await setupRealStaking(
3794+
t,
3795+
staking,
3796+
walletRegistry,
3797+
deployer,
37823798
stakingProvider,
3783-
beneficiary
3799+
beneficiary,
3800+
minimumAuthorization
37843801
)
37853802

37863803
// Setup: Deactivate chaosnet to allow operators to join sortition pool
37873804
await deactivateChaosnetMode(sortitionPool)
37883805

3789-
// Setup: Trigger authorization callback for staking provider
3790-
await triggerAuthorizationCallback(
3791-
walletRegistry,
3792-
staking.address,
3793-
stakingProvider.address,
3794-
ethers.BigNumber.from(0),
3795-
minimumAuthorization
3796-
)
3797-
37983806
// Setup: Register operator with authorized stake
37993807
await walletRegistry
38003808
.connect(stakingProvider)
@@ -3979,17 +3987,19 @@ describe("WalletRegistry - Migration Scenario Tests (TIP-092)", () => {
39793987
*/
39803988
describe("NOT MIGRATED Touchpoints", () => {
39813989
let allowlist: FakeContract<IStaking>
3982-
let stakingFake: FakeContract<TokenStaking>
39833990

39843991
before(async () => {
39853992
await createSnapshot()
39863993

3987-
// Setup: Mock TokenStaking for beneficiary lookup (NOT migrated to Allowlist)
3988-
stakingFake = await createTokenStakingFake(
3989-
staking.address,
3990-
minimumAuthorization,
3994+
// Setup: Use real TokenStaking for beneficiary roles (NOT migrated to Allowlist)
3995+
await setupRealStaking(
3996+
t,
3997+
staking,
3998+
walletRegistry,
3999+
deployer,
39914000
stakingProvider,
3992-
beneficiary
4001+
beneficiary,
4002+
minimumAuthorization
39934003
)
39944004

39954005
// Setup: Create allowlist fake and upgrade (but beneficiary still in TokenStaking)
@@ -4043,31 +4053,24 @@ describe("WalletRegistry - Migration Scenario Tests (TIP-092)", () => {
40434053
*/
40444054
describe("Upgrade Flow", () => {
40454055
let allowlist: FakeContract<IStaking>
4046-
let stakingFake: FakeContract<TokenStaking>
40474056

40484057
before(async () => {
40494058
await createSnapshot()
40504059

4051-
// Setup: Mock TokenStaking authorization (pre-upgrade state)
4052-
stakingFake = await createTokenStakingFake(
4053-
staking.address,
4054-
minimumAuthorization,
4060+
// Setup: Use real TokenStaking authorization (pre-upgrade state)
4061+
await setupRealStaking(
4062+
t,
4063+
staking,
4064+
walletRegistry,
4065+
deployer,
40554066
stakingProvider,
4056-
beneficiary
4067+
beneficiary,
4068+
minimumAuthorization
40574069
)
40584070

40594071
// Setup: Deactivate chaosnet to allow operators to join sortition pool
40604072
await deactivateChaosnetMode(sortitionPool)
40614073

4062-
// Setup: Trigger authorization callback for staking provider (pre-upgrade)
4063-
await triggerAuthorizationCallback(
4064-
walletRegistry,
4065-
staking.address,
4066-
stakingProvider.address,
4067-
ethers.BigNumber.from(0),
4068-
minimumAuthorization
4069-
)
4070-
40714074
// Setup: Register operator and join pool (before upgrade)
40724075
await walletRegistry
40734076
.connect(stakingProvider)

solidity/ecdsa/test/WalletRegistry.DualMode.test.ts

Lines changed: 40 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -1,12 +1,14 @@
11
/* eslint-disable @typescript-eslint/no-unused-expressions */
2-
import { ethers, upgrades } from "hardhat"
2+
import { ethers, helpers } from "hardhat"
33
import { smock } from "@defi-wonderland/smock"
44
import { expect } from "chai"
55

66
import type { FakeContract } from "@defi-wonderland/smock"
77
import type { SignerWithAddress } from "@nomiclabs/hardhat-ethers/signers"
88
import type { WalletRegistry, Allowlist, IStaking } from "../typechain"
99

10+
const { createSnapshot, restoreSnapshot } = helpers.snapshot
11+
1012
const ZERO_ADDRESS = ethers.constants.AddressZero
1113

1214
describe("WalletRegistry - Dual-Mode Authorization", () => {
@@ -19,6 +21,14 @@ describe("WalletRegistry - Dual-Mode Authorization", () => {
1921
let operator: SignerWithAddress
2022
let unauthorizedCaller: SignerWithAddress
2123

24+
before(async () => {
25+
await createSnapshot()
26+
})
27+
28+
after(async () => {
29+
await restoreSnapshot()
30+
})
31+
2232
beforeEach(async () => {
2333
const signers = await ethers.getSigners()
2434
;[deployer, governance, stakingProvider, operator, unauthorizedCaller] =
@@ -41,7 +51,16 @@ describe("WalletRegistry - Dual-Mode Authorization", () => {
4151
const randomBeacon = await smock.fake("IRandomBeacon")
4252
const reimbursementPool = await smock.fake("ReimbursementPool")
4353

44-
// Deploy WalletRegistry as upgradeable proxy with library linking
54+
// Deploy WalletRegistry proxy using ERC1967Proxy directly instead of
55+
// @openzeppelin/hardhat-upgrades' upgrades.deployProxy(). Using
56+
// upgrades.deployProxy with kind:"transparent" creates a shared
57+
// ProxyAdmin in the OZ manifest. Because this test file runs before
58+
// tests that use deployments.fixture(), that ProxyAdmin would be owned
59+
// by signer[0], but the deploy scripts expect signer[1] (named
60+
// "deployer") to own it. This mismatch causes
61+
// deploy/11_transfer_proxy_admin_ownership.ts to fail with
62+
// "Ownable: caller is not the owner" when other test suites call
63+
// deployments.fixture().
4564
const WalletRegistryFactory = await ethers.getContractFactory(
4665
"WalletRegistry",
4766
{
@@ -51,15 +70,25 @@ describe("WalletRegistry - Dual-Mode Authorization", () => {
5170
}
5271
)
5372

54-
walletRegistry = (await upgrades.deployProxy(
55-
WalletRegistryFactory,
56-
[dkgValidator.address, randomBeacon.address, reimbursementPool.address],
57-
{
58-
constructorArgs: [sortitionPool.address, stakingContract.address],
59-
unsafeAllow: ["external-library-linking", "state-variable-immutable"],
60-
}
61-
)) as WalletRegistry
62-
await walletRegistry.deployed()
73+
const impl = await WalletRegistryFactory.deploy(
74+
sortitionPool.address,
75+
stakingContract.address
76+
)
77+
await impl.deployed()
78+
79+
const initData = WalletRegistryFactory.interface.encodeFunctionData(
80+
"initialize",
81+
[dkgValidator.address, randomBeacon.address, reimbursementPool.address]
82+
)
83+
84+
const ERC1967ProxyFactory =
85+
await ethers.getContractFactory("ERC1967Proxy")
86+
const proxy = await ERC1967ProxyFactory.deploy(impl.address, initData)
87+
await proxy.deployed()
88+
89+
walletRegistry = WalletRegistryFactory.attach(
90+
proxy.address
91+
) as WalletRegistry
6392
})
6493

6594
describe("initializeV2", () => {

0 commit comments

Comments
 (0)