feat(solidity): improve Sepolia deploy behavior and wallet registry flows#4000
Conversation
cef3185 to
e1bca45
Compare
8e4fea1 to
23b83ce
Compare
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThis PR refactors Threshold's ECDSA and random-beacon deployment infrastructure to decouple verification from deployment success, defaults test fixtures to allowlist authorization, vendors random-beacon export scripts, adds compatibility layers for legacy TokenStaking method variants, and migrates test suites to use legacy helpers while skipping unsupported scenarios. ChangesUnified deployment, verification, and test compatibility changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
40e522f to
e1bca45
Compare
…ety) (#4016) ## Context Follow-up to #4000 addressing valid findings from the multi-agent review. Stacks **on top of** `stack/testnet4-02-solidity-logic`; merge after #4000. ## Findings addressed | # | Fix | Commit | |---|-----|--------| | 3 | Allowlist redeploy-safe networks for `EcdsaDkgValidator` (was: mainnet-only denylist) | \`fix(ecdsa/deploy): allowlist redeploy-safe networks\` | | 5 | README documenting vendored `random-beacon-export` regeneration policy (`05_*.js` diverges intentionally) | \`docs(ecdsa/deploy): document vendored random-beacon-export format policy\` | | 7 | Restore \`HardhatUserConfig\` type annotation (TS 4.5 compatible, no \`satisfies\`) | \`fix(ecdsa/hardhat): restore HardhatUserConfig type annotation\` | | 8 | \`verifyOnTenderlyOrContinue\` helper applied to all 4 deploy scripts (was: only 03 swallowed errors) | \`fix(ecdsa/deploy): apply tenderly verify-or-continue helper everywhere\` | | 9 | Comment explaining sepolia named-account role collapse | \`docs(ecdsa/hardhat): note sepolia named-account role collapse\` | | 10 | Comment marking the second \`WalletRegistry.governance()\` read as a deliberate TOCTOU recheck | \`docs(ecdsa/tasks): explain TOCTOU recheck of WR.governance()\` | | extra | Move \`README.md\` out of \`deploy/\` (hardhat-deploy walks the dir and \`require()\`s every file) | \`fix(ecdsa/deploy): move README out of deploy/ dir\` | ## Findings rejected after review - **PR body wording on `WalletRegistry.sol`** (NatSpec-only) — best handled by editing PR #4000's body directly, not as a code commit here. - **Skip-suite disclosure** — same: belongs in PR #4000's body. - \`#4\` (gate \`00_resolve_*\` on env var) — would break \`deployments.fixture()\` in tests (\`yarn test\` runs the hardhat network which needs the skip). - \`#6\` (drop \`approveApplication\` try/catch) — deliberate runtime backstop for forked/aliased networks where artifact ABI ≠ on-chain. ## Test Plan Verified locally with \`FORKING_URL\` unset: - \`cd solidity/ecdsa && yarn test\` → **644 passing, 44 pending, 0 failing** - \`cd solidity/random-beacon && yarn test\` → **535 passing, 397 pending, 0 failing** The 44 + 397 pending are the pre-existing \`describe.skip(...)\` suites (legacy Keep TokenStaking ABI unavailable in current Threshold build).
## Stack Context This is **PR 1/4** in the reorganization stack. Base: `main` Next: #4000 ## What Changed ### 1) CI + workflow execution model - Added reusable Git/Yarn setup action: `.github/actions/setup-git-for-yarn/action.yml`. - Added local reusable docs workflow: `.github/workflows/reusable-solidity-docs.yml`. - Updated Solidity workflows to use the new setup and Yarn/Corepack-compatible behavior: - `.github/workflows/contracts-ecdsa.yml` - `.github/workflows/contracts-random-beacon.yml` - `.github/workflows/contracts-ecdsa-docs.yml` - `.github/workflows/contracts-random-beacon-docs.yml` - `.github/workflows/npm-ecdsa.yml` - `.github/workflows/npm-random-beacon.yml` ### 2) Solidity workspace tooling alignment - Added Yarn 4 config files: - `solidity/ecdsa/.yarnrc.yml` - `solidity/random-beacon/.yarnrc.yml` - Updated build/tooling ignore and container files: - `solidity/ecdsa/.dockerignore`, `.eslintignore`, `.gitignore`, `.prettierignore`, `Dockerfile` - `solidity/random-beacon/.dockerignore`, `.gitignore`, `.prettierignore`, `Dockerfile` ### 3) Dependency metadata and lockfiles - Updated manifests/config: - `solidity/ecdsa/package.json`, `solidity/ecdsa/tsconfig.json` - `solidity/random-beacon/package.json`, `solidity/random-beacon/tsconfig.json` - Refreshed lockfiles: - `solidity/ecdsa/yarn.lock` - `solidity/random-beacon/yarn.lock` ## Out of Scope - Solidity deploy logic and contract behavior updates. - Generated Sepolia deployment artifacts. - Go/Testnet4 runtime changes. ## Test Plan - [ ] Workflow YAML is valid and jobs trigger. - [ ] Solidity workspaces install with Yarn/Corepack in CI-compatible mode. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added a reusable workflow to generate and optionally publish Solidity API documentation with configurable publish and artifact options. * **Chores** * Introduced shared CI actions to standardize Node/Yarn dependency installation, git handling for yarn, and caching across workflows. * Standardized on Node >=18.15.0 and Yarn 4.8.1; updated build images, package manager metadata, install scripts, and various project config/ignore entries. * **Other** * Switched some dependency upgrade commands to the newer Yarn command. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
…andom-beacon Adjusts hardhat/deploy scripts, WalletRegistry behavior/tests, and external random-beacon export deploy scripts so Sepolia deployment and verification paths match the new stack expectations. Co-authored-by: Cursor <cursoragent@cursor.com>
…ator Invert mainnet-only denylist to explicit allowlist (hardhat, development, sepolia). Future production-like networks added downstream now default to the safe behavior (preserve existing deployment) instead of silently overwriting deployments/<network>/EcdsaDkgValidator.json.
Add README explaining that 05_approve_random_beacon_in_token_staking.js intentionally diverges from its tsc-compiled siblings to carry an ifaceHasFunction precheck for Threshold TokenStaking. A blind regeneration from upstream would silently drop the precheck and reintroduce a hard failure on networks without approveApplication.
The conditional etherscan spread inside the object literal forced removal of the HardhatUserConfig annotation. Assign etherscan via a post-declaration if-block instead so the annotation (and type-checking) is preserved without relying on TS 4.9 'satisfies' (this package is on TS 4.5).
Sepolia maps deployer/governance/chaosnetOwner/esdm all to account index 0 because the testnet deploy uses a single key from ACCOUNTS_PRIVATE_KEYS. Add a header comment so readers don't expect role-separation branches (e.g. initialize-wallet-owner.ts's owner-vs-governance fork) to fire on Sepolia.
The second WalletRegistry.governance() read immediately before execute() is a deliberate recheck against concurrent transferGovernance on shared networks (sepolia/mainnet); annotate so it isn't mistaken for dead code.
Add verifyOnTenderlyOrContinue mirroring verifyOnEtherscanOrContinue and wrap tenderly.verify in all four deploy scripts (01, 02, 03, 09). Before this, only 03 swallowed Tenderly errors; a Tenderly outage during deploy would fail 01/02/09 while 03 kept going. Now all four behave the same.
hardhat-deploy walks deploy/ and requires() every file; a Markdown sibling there crashes deployments.fixture() in tests. Move the regeneration-policy README one level up to external/random-beacon-export/README.md (still discoverable, no longer in the require path).
3f5e3c9 to
0d1ebff
Compare
The post-declaration etherscan assignment used a leading-semicolon cast expression that tripped @typescript-eslint/no-extra-semi while prettier re-added the semicolon, so eslint --fix could not converge. Assign via an intermediate typed variable instead: no leading semicolon, satisfies both prettier and no-extra-semi, and preserves the HardhatUserConfig annotation. Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
solidity/ecdsa/test/fixtures/index.ts (1)
216-235:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winCritical: JSDoc contradicts actual default behavior.
Lines 216 and 230 state that
TokenStakingis the default authorization mode, but the code at line 105 setsuseAllowlist ?? true, makingAllowlistthe default. This documentation bug will mislead developers who rely on the JSDoc.Additionally, the examples are now backwards:
- Line 224 shows the default call but documents it as using
staking(legacy)- Line 227 shows explicit
useAllowlist: falsebut documents it as usingallowlist📝 Corrected JSDoc
/** * Creates and loads a WalletRegistry test fixture with dual-mode authorization support. * * This is the main entry point for test files to load the WalletRegistry fixture. - * It supports TokenStaking (default) and Allowlist authorization modes for + * It supports Allowlist (default) and TokenStaking authorization modes for * comprehensive testing of the dual-mode authorization routing implementation. * * `@param` options - Configuration options for the fixture * `@param` options.useAllowlist - If true (default), Allowlist mode; if false, legacy stake path * `@returns` Promise resolving to fixture with all deployed contracts, signers, and test operators * * `@example` - * const { walletRegistry, operators, allowlist } = await walletRegistryFixture() + * const { walletRegistry, operators, allowlist } = await walletRegistryFixture() // Default: Allowlist mode * * `@example` - * const { walletRegistry, operators, staking } = await walletRegistryFixture({ useAllowlist: false }) + * const { walletRegistry, operators, staking } = await walletRegistryFixture({ useAllowlist: false }) // Legacy TokenStaking mode * * `@remarks` - * - Default uses TokenStaking; use `{ useAllowlist: true }` for dual-mode / Allowlist-only tests + * - Default uses Allowlist; use `{ useAllowlist: false }` for legacy TokenStaking tests * - Allowlist mode calls walletRegistry.initializeV2() to enable dual-mode routing * - In Allowlist mode, TokenStaking notification rewards are NOT configured (not needed) * - Fixture uses hardhat-deploy's snapshot/restore for efficient test isolation - * - Performance: ~5 seconds (TokenStaking mode), ~7 seconds (Allowlist mode) + * - Performance: ~7 seconds (Allowlist mode), ~5 seconds (TokenStaking mode) */🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@solidity/ecdsa/test/fixtures/index.ts` around lines 216 - 235, The JSDoc incorrectly states TokenStaking is the default while the code sets useAllowlist ?? true; update the doc for walletRegistryFixture to state Allowlist is the default, adjust the two examples so the default call returns allowlist (and the explicit { useAllowlist: false } example returns staking), and correct remarks that currently claim TokenStaking is default; ensure references to useAllowlist, walletRegistry.initializeV2(), TokenStaking, and Allowlist match the actual behavior.
♻️ Duplicate comments (1)
solidity/ecdsa/external/random-beacon-export/deploy/07_deploy_random_beacon_governance.js (1)
61-76:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftVerification failures will block deployment flow.
Same issue as in
04_deploy_random_beacon.js: lines 61-67 callhelpers.etherscan.verifydirectly without the error-suppressing wrapper, and lines 70-76 callhre.tenderly.verifydirectly. This creates inconsistency with the ECDSA deployment scripts and contradicts the PR's goal of preventing verification failures from blocking deployments.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@solidity/ecdsa/external/random-beacon-export/deploy/07_deploy_random_beacon_governance.js` around lines 61 - 76, The verification calls for RandomBeaconGovernance currently call helpers.etherscan.verify and hre.tenderly.verify directly (in the RandomBeaconGovernance deployment block) which can throw and abort deployment; change these to the same error-suppressing pattern used in the other ECDSA deploy scripts by wrapping each call in a try/catch that logs the verification error (including the error message) but does not rethrow, i.e., replace direct calls to helpers.etherscan.verify(RandomBeaconGovernance) and hre.tenderly.verify({...}) with try { await helpers.etherscan.verify(RandomBeaconGovernance); } catch (err) { console.warn("Etherscan verify failed for RandomBeaconGovernance:", err); } and similarly for hre.tenderly.verify, referencing RandomBeaconGovernance.address/name in the log.
🧹 Nitpick comments (5)
solidity/random-beacon/types/TokenStaking.extensions.d.ts (1)
10-39: ⚡ Quick winApply transaction overrides to all write signatures here as well.
These are write methods, so
CallOverridesis not the right override type and may block valid tx override fields in tests.Suggested typing fix
import type { BigNumber, BigNumberish, ContractTransaction } from "ethers" -import type { CallOverrides } from "`@ethersproject/contracts`" +import type { CallOverrides, Overrides } from "`@ethersproject/contracts`" @@ - overrides?: CallOverrides + overrides?: Overrides @@ - overrides?: CallOverrides + overrides?: Overrides @@ - overrides?: CallOverrides + overrides?: Overrides @@ - overrides?: CallOverrides + overrides?: Overrides @@ - overrides?: CallOverrides + overrides?: Overrides @@ - overrides?: CallOverrides + overrides?: OverridesAlso applies to: 50-55
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@solidity/random-beacon/types/TokenStaking.extensions.d.ts` around lines 10 - 39, The listed functions (stake, approveApplication, increaseAuthorization, topUp, processSlashing and the other write signatures in the same file) incorrectly use CallOverrides (read/call-only) — change their overrides parameter to the appropriate transaction override type (Overrides from ethers for non-payable writes, or PayableOverrides if the function is payable) so tests and tx options (gas, nonce, value, etc.) are accepted; update the signature for each write method (e.g., stake(..., overrides?: Overrides), topUp(..., overrides?: Overrides|PayableOverrides as applicable), approveApplication(..., overrides?: Overrides), increaseAuthorization(..., overrides?: Overrides), processSlashing(..., overrides?: Overrides)) and adjust imports if necessary.solidity/ecdsa/test/WalletRegistry.Authorization.test.ts (1)
3780-3808: ⚡ Quick winAvoid unconditional skips for migration-path suites.
These
describe.skipblocks remove coverage for pre-upgrade routing and upgrade transition behavior. Prefer conditional execution (skip only when required capabilities are absent) so these assertions still run in environments that support legacy flows.Also applies to: 3981-4038, 4047-4158
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@solidity/ecdsa/test/WalletRegistry.Authorization.test.ts` around lines 3780 - 3808, Replace the unconditional describe.skip with a normal describe and make the before hook a non-arrow function so you can call this.skip() if the environment lacks legacy token-staking authorization; e.g., change describe.skip("Pre-Upgrade Mode...") to describe("Pre-Upgrade Mode...") and convert the before(async () => { ... }) to before(async function() { if (!(await supportsTokenStaking())) return this.skip(); await createSnapshot(); await setupRealStaking(...); await deactivateChaosnetMode(...); await walletRegistry.connect(...).registerOperator(...); }); similarly add or reuse a small helper like supportsTokenStaking or capability check to detect when token-staking/legacy flows are unavailable so the suite runs in capable environments but is skipped gracefully otherwise (same pattern for the other skipped suites).solidity/ecdsa/test/WalletRegistry.WalletCreation.test.ts (1)
2403-2466: ⚡ Quick winPreserve slashing-path coverage with capability-based skipping instead of unconditional skips.
These blocks are fully disabled, so challenge/slashing regressions won’t be caught in this suite. Gate them conditionally (based on fixture/runtime capability) rather than permanently skipping.
Also applies to: 2484-2544, 2587-2663, 2946-3000, 3028-3082, 3122-3188
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@solidity/ecdsa/test/WalletRegistry.WalletCreation.test.ts` around lines 2403 - 2466, The test contexts are unconditionally skipped via context.skip which removes coverage for the challenge/slashing paths; change each context.skip to a conditional wrapper that checks runtime capability (e.g., whether processSlashing exists) and only skips when unsupported. Concretely, introduce a boolean like supportsProcessSlashing = typeof legacyTokenStakingAt(staking, thirdParty).processSlashing === "function" (or await staking.callStatic.processSlashing? catch) at the top of the suite or in a before hook, then replace context.skip(...) with supportsProcessSlashing ? context(...) : context.skip(...), keeping the inner tests that call walletRegistry.challengeDkgResult, legacyTokenStakingAt(...).processSlashing, assertGasUsed, etc., so the tests run when processSlashing is available and are skipped otherwise.solidity/ecdsa/deploy/07_approve_wallet_registry.ts (1)
22-46: ⚡ Quick winConsider consolidating redundant checks or documenting the dual-check rationale.
The script performs two separate checks for
approveApplicationavailability:
- ABI interface check (lines 22-28)
- Try-catch with error message parsing (lines 37-44)
If the ABI check is reliable, the try-catch is redundant. If both are needed (e.g., to handle deployment ABI/on-chain ABI mismatches), document why. The error message parsing at line 39 is fragile and will break if the error format changes.
♻️ Simplified approach relying on ABI check
If the ABI check is sufficient, remove the try-catch:
if (!ifaceHasFunction(iface, "approveApplication")) { hre.deployments.log( "TokenStaking does not have approveApplication (Threshold TokenStaking); skipping WalletRegistry approval" ) return } - try { - await execute( - "TokenStaking", - { from: deployer, log: true, waitConfirmations: 1 }, - "approveApplication", - WalletRegistry.address - ) - } catch (e: unknown) { - const msg = e instanceof Error ? e.message : String(e) - if (msg.includes("No method named") && msg.includes("approveApplication")) { - hre.deployments.log( - "TokenStaking has no approveApplication callable on this network; skipping WalletRegistry approval" - ) - return - } - throw e - } + await execute( + "TokenStaking", + { from: deployer, log: true, waitConfirmations: 1 }, + "approveApplication", + WalletRegistry.address + )If both checks are needed, add a comment explaining the deployment/runtime ABI divergence scenario.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@solidity/ecdsa/deploy/07_approve_wallet_registry.ts` around lines 22 - 46, The script redundantly checks for approveApplication twice (via ifaceHasFunction on TokenStaking ABI and again by catching runtime errors around execute("approveApplication", ...)); either remove the try-catch and rely on the ABI check (delete the catch block and its special-case string parsing) or keep the runtime guard but add a clear comment explaining why both checks are required (e.g., deployment ABI vs on-chain implementation mismatch) and replace fragile string parsing in the catch with a safer detection (inspect error type/metadata instead of message substrings). Ensure changes reference ifaceHasFunction, ethers.utils.Interface/TokenStaking.abi, execute("approveApplication", WalletRegistry.address) and the current try-catch so reviewers can locate the logic to modify.solidity/ecdsa/deploy/03_deploy_wallet_registry.ts (1)
64-88: 💤 Low valueConsider applying DISABLE_HARDHAT_VERIFY to Tenderly verification for consistency.
The Etherscan verification is gated by both
hre.network.tags.etherscanandprocess.env.DISABLE_HARDHAT_VERIFY !== "true"(line 65-66), while Tenderly verification only checkshre.network.tags.tenderly(line 81).If
DISABLE_HARDHAT_VERIFYis intended to disable all verification mechanisms, apply it to Tenderly as well. If the asymmetry is intentional (Tenderly uses a separate plugin), consider documenting this design decision.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@solidity/ecdsa/deploy/03_deploy_wallet_registry.ts` around lines 64 - 88, The Tenderly verification block currently only checks hre.network.tags.tenderly; to make DISABLE_HARDHAT_VERIFY apply consistently, guard the verifyOnTenderlyOrContinue call with the same environment check (process.env.DISABLE_HARDHAT_VERIFY !== "true"), e.g. wrap the existing hre.network.tags.tenderly condition or add it to the if condition that invokes verifyOnTenderlyOrContinue (reference: verifyOnTenderlyOrContinue, hre.network.tags.tenderly, DISABLE_HARDHAT_VERIFY) so Tenderly verification honors the same disable flag as verifyOnEtherscanOrContinue.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@solidity/ecdsa/external/random-beacon-export/deploy/01_deploy_reimbursement_pool.js`:
- Around line 60-75: The vendored deploy script calls
helpers.etherscan.verify(...) and hre.tenderly.verify(...) directly (e.g.,
surrounding ReimbursementPool verification) which can throw and halt deployment;
change these calls to use the same non-blocking wrapper logic as
verifyOnEtherscanOrContinue and respect DISABLE_HARDHAT_VERIFY (or create a
small wrapper that catches errors and logs them and/or skips when
DISABLE_HARDHAT_VERIFY is set), and apply that wrapper to the ReimbursementPool
verification calls so failures do not abort the deployment flow.
In
`@solidity/ecdsa/external/random-beacon-export/deploy/04_deploy_random_beacon.js`:
- Around line 110-137: The verification calls in 04_deploy_random_beacon.js are
unprotected and will break the deploy on any rejection; replace direct
helpers.etherscan.verify(...) invocations for BLS, BeaconAuthorization,
BeaconDkg, BeaconInactivity, and RandomBeacon with the existing helper
verifyOnEtherscanOrContinue (passing the same contract symbols) and replace the
hre.tenderly.verify(...) call for RandomBeacon with verifyOnTenderlyOrContinue
(passing name "RandomBeacon" and RandomBeacon.address or the helper's expected
args) so each verification is wrapped to catch/skip failures and allow the
deploy to continue.
In
`@solidity/ecdsa/external/random-beacon-export/deploy/09_deploy_random_beacon_chaosnet.js`:
- Around line 67-83: Replace the direct calls to helpers.etherscan.verify and
hre.tenderly.verify for RandomBeaconChaosnet with the graceful wrappers
(verifyOnEtherscanOrContinue and verifyOnTenderlyOrContinue) or, if you prefer
not to convert this .js file to .ts, wrap each verification in a try/catch that
logs the error and continues; specifically, for hre.network.tags.etherscan wait
for RandomBeaconChaosnet.transactionHash then call the etherscan wrapper (or try
{ await helpers.etherscan.verify(RandomBeaconChaosnet) } catch (e) {
console.log(...) }) and for hre.network.tags.tenderly call the tenderly wrapper
(or try { await hre.tenderly.verify({ name: "RandomBeaconChaosnet", address:
RandomBeaconChaosnet.address }) } catch (e) { console.log(...) }), ensuring
failures do not throw and block deployment.
In `@solidity/ecdsa/test/WalletRegistry.WalletCreation.test.ts`:
- Around line 2707-2709: Update the test description string in the two
Jest/Mocha test cases using it(...) so it accurately reflects the asserted gas
target (currently asserting assertGasUsed(tx, 221_000, 60_000)); change the
title from "should use close to 275 000 gas" to a wording that matches ~221k
(e.g., "should use close to 221 000 gas") in both locations (the it(...)
containing assertGasUsed and the second occurrence noted), leaving the
assertGasUsed(tx, 221_000, 60_000) call unchanged.
In `@solidity/ecdsa/types/chai.d.ts`:
- Line 7: The revertedWithCustomError matcher is currently typed to return
Promise<void>, preventing chainable .withArgs(...). Update
solidity/ecdsa/types/chai.d.ts by adding an AsyncAssertion that extends
Assertion & Promise<void> and a CustomErrorAssertion that extends AsyncAssertion
and exposes withArgs(...args: any[]): AsyncAssertion, then change the
Declaration of revertedWithCustomError(contract: unknown, errorName: string) to
return CustomErrorAssertion so callers can chain .withArgs(...).
In `@solidity/random-beacon/hardhat.config.ts`:
- Around line 111-114: Validate process.env.GAS_PRICE_GWEI before using parseInt
to set gasPrice: check that GAS_PRICE_GWEI exists and that
Number.isFinite(Number(GAS_PRICE_GWEI)) (or a similar numeric check) yields
true; if valid, compute gasPrice = Math.floor(Number(GAS_PRICE_GWEI) * 1e9) and
include the override, otherwise skip adding the gasPrice override (or throw/log
a clear error) so gasPrice is never set to NaN; update the code around the
GAS_PRICE_GWEI usage and parseInt call accordingly to use this validated numeric
conversion and explicit handling.
In `@solidity/random-beacon/test/RandomBeacon.Authorization.test.ts`:
- Around line 1556-1559: The test currently calls legacyTokenStakingAt(...)[
"requestAuthorizationDecrease(address,address,uint96)"](stakingProvider.address,
randomBeacon.address, deauthorizingBy) without awaiting the returned promise,
which can race with later assertions; update both occurrences (the one shown and
the similar block at lines referenced) to await the call so the
authorization-decrease setup transactions complete before proceeding, i.e. add
await before legacyTokenStakingAt(...)[
"requestAuthorizationDecrease(address,address,uint96)"](...) for each instance.
---
Outside diff comments:
In `@solidity/ecdsa/test/fixtures/index.ts`:
- Around line 216-235: The JSDoc incorrectly states TokenStaking is the default
while the code sets useAllowlist ?? true; update the doc for
walletRegistryFixture to state Allowlist is the default, adjust the two examples
so the default call returns allowlist (and the explicit { useAllowlist: false }
example returns staking), and correct remarks that currently claim TokenStaking
is default; ensure references to useAllowlist, walletRegistry.initializeV2(),
TokenStaking, and Allowlist match the actual behavior.
---
Duplicate comments:
In
`@solidity/ecdsa/external/random-beacon-export/deploy/07_deploy_random_beacon_governance.js`:
- Around line 61-76: The verification calls for RandomBeaconGovernance currently
call helpers.etherscan.verify and hre.tenderly.verify directly (in the
RandomBeaconGovernance deployment block) which can throw and abort deployment;
change these to the same error-suppressing pattern used in the other ECDSA
deploy scripts by wrapping each call in a try/catch that logs the verification
error (including the error message) but does not rethrow, i.e., replace direct
calls to helpers.etherscan.verify(RandomBeaconGovernance) and
hre.tenderly.verify({...}) with try { await
helpers.etherscan.verify(RandomBeaconGovernance); } catch (err) {
console.warn("Etherscan verify failed for RandomBeaconGovernance:", err); } and
similarly for hre.tenderly.verify, referencing
RandomBeaconGovernance.address/name in the log.
---
Nitpick comments:
In `@solidity/ecdsa/deploy/03_deploy_wallet_registry.ts`:
- Around line 64-88: The Tenderly verification block currently only checks
hre.network.tags.tenderly; to make DISABLE_HARDHAT_VERIFY apply consistently,
guard the verifyOnTenderlyOrContinue call with the same environment check
(process.env.DISABLE_HARDHAT_VERIFY !== "true"), e.g. wrap the existing
hre.network.tags.tenderly condition or add it to the if condition that invokes
verifyOnTenderlyOrContinue (reference: verifyOnTenderlyOrContinue,
hre.network.tags.tenderly, DISABLE_HARDHAT_VERIFY) so Tenderly verification
honors the same disable flag as verifyOnEtherscanOrContinue.
In `@solidity/ecdsa/deploy/07_approve_wallet_registry.ts`:
- Around line 22-46: The script redundantly checks for approveApplication twice
(via ifaceHasFunction on TokenStaking ABI and again by catching runtime errors
around execute("approveApplication", ...)); either remove the try-catch and rely
on the ABI check (delete the catch block and its special-case string parsing) or
keep the runtime guard but add a clear comment explaining why both checks are
required (e.g., deployment ABI vs on-chain implementation mismatch) and replace
fragile string parsing in the catch with a safer detection (inspect error
type/metadata instead of message substrings). Ensure changes reference
ifaceHasFunction, ethers.utils.Interface/TokenStaking.abi,
execute("approveApplication", WalletRegistry.address) and the current try-catch
so reviewers can locate the logic to modify.
In `@solidity/ecdsa/test/WalletRegistry.Authorization.test.ts`:
- Around line 3780-3808: Replace the unconditional describe.skip with a normal
describe and make the before hook a non-arrow function so you can call
this.skip() if the environment lacks legacy token-staking authorization; e.g.,
change describe.skip("Pre-Upgrade Mode...") to describe("Pre-Upgrade Mode...")
and convert the before(async () => { ... }) to before(async function() { if
(!(await supportsTokenStaking())) return this.skip(); await createSnapshot();
await setupRealStaking(...); await deactivateChaosnetMode(...); await
walletRegistry.connect(...).registerOperator(...); }); similarly add or reuse a
small helper like supportsTokenStaking or capability check to detect when
token-staking/legacy flows are unavailable so the suite runs in capable
environments but is skipped gracefully otherwise (same pattern for the other
skipped suites).
In `@solidity/ecdsa/test/WalletRegistry.WalletCreation.test.ts`:
- Around line 2403-2466: The test contexts are unconditionally skipped via
context.skip which removes coverage for the challenge/slashing paths; change
each context.skip to a conditional wrapper that checks runtime capability (e.g.,
whether processSlashing exists) and only skips when unsupported. Concretely,
introduce a boolean like supportsProcessSlashing = typeof
legacyTokenStakingAt(staking, thirdParty).processSlashing === "function" (or
await staking.callStatic.processSlashing? catch) at the top of the suite or in a
before hook, then replace context.skip(...) with supportsProcessSlashing ?
context(...) : context.skip(...), keeping the inner tests that call
walletRegistry.challengeDkgResult, legacyTokenStakingAt(...).processSlashing,
assertGasUsed, etc., so the tests run when processSlashing is available and are
skipped otherwise.
In `@solidity/random-beacon/types/TokenStaking.extensions.d.ts`:
- Around line 10-39: The listed functions (stake, approveApplication,
increaseAuthorization, topUp, processSlashing and the other write signatures in
the same file) incorrectly use CallOverrides (read/call-only) — change their
overrides parameter to the appropriate transaction override type (Overrides from
ethers for non-payable writes, or PayableOverrides if the function is payable)
so tests and tx options (gas, nonce, value, etc.) are accepted; update the
signature for each write method (e.g., stake(..., overrides?: Overrides),
topUp(..., overrides?: Overrides|PayableOverrides as applicable),
approveApplication(..., overrides?: Overrides), increaseAuthorization(...,
overrides?: Overrides), processSlashing(..., overrides?: Overrides)) and adjust
imports if necessary.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 2e906162-c166-4bea-9ec2-e83d45cd37ba
📒 Files selected for processing (44)
solidity/ecdsa/contracts/WalletRegistry.solsolidity/ecdsa/deploy/00_resolve_reimbursement_pool.tssolidity/ecdsa/deploy/00_resolve_token_staking.tssolidity/ecdsa/deploy/01_deploy_ecdsa_sortition_pool.tssolidity/ecdsa/deploy/02_deploy_dkg_validator.tssolidity/ecdsa/deploy/03_deploy_wallet_registry.tssolidity/ecdsa/deploy/07_approve_wallet_registry.tssolidity/ecdsa/deploy/09_deploy_wallet_registry_governance.tssolidity/ecdsa/deploy/etherscanVerification.tssolidity/ecdsa/deploy/tenderlyVerification.tssolidity/ecdsa/external/random-beacon-export/README.mdsolidity/ecdsa/external/random-beacon-export/deploy/01_deploy_reimbursement_pool.jssolidity/ecdsa/external/random-beacon-export/deploy/02_deploy_beacon_sortition_pool.jssolidity/ecdsa/external/random-beacon-export/deploy/03_deploy_beacon_dkg_validator.jssolidity/ecdsa/external/random-beacon-export/deploy/04_deploy_random_beacon.jssolidity/ecdsa/external/random-beacon-export/deploy/05_approve_random_beacon_in_token_staking.jssolidity/ecdsa/external/random-beacon-export/deploy/06_authorize_random_beacon_in_reimbursement_pool.jssolidity/ecdsa/external/random-beacon-export/deploy/07_deploy_random_beacon_governance.jssolidity/ecdsa/external/random-beacon-export/deploy/08_transfer_governance.jssolidity/ecdsa/external/random-beacon-export/deploy/09_deploy_random_beacon_chaosnet.jssolidity/ecdsa/hardhat.config.tssolidity/ecdsa/tasks/initialize-wallet-owner.tssolidity/ecdsa/test/WalletRegistry.Authorization.test.tssolidity/ecdsa/test/WalletRegistry.CustomErrors.test.tssolidity/ecdsa/test/WalletRegistry.Rewards.test.tssolidity/ecdsa/test/WalletRegistry.Slashing.test.tssolidity/ecdsa/test/WalletRegistry.Upgrade.test.tssolidity/ecdsa/test/WalletRegistry.WalletCreation.test.tssolidity/ecdsa/test/WalletRegistryGovernance.test.tssolidity/ecdsa/test/fixtures/index.tssolidity/ecdsa/test/utils/operators.tssolidity/ecdsa/types/TokenStaking.extensions.d.tssolidity/ecdsa/types/chai.d.tssolidity/random-beacon/deploy/05_approve_random_beacon_in_token_staking.tssolidity/random-beacon/hardhat.config.tssolidity/random-beacon/test/RandomBeacon.Authorization.test.tssolidity/random-beacon/test/RandomBeacon.Callback.test.tssolidity/random-beacon/test/RandomBeacon.GroupCreation.test.tssolidity/random-beacon/test/RandomBeacon.Relay.test.tssolidity/random-beacon/test/RandomBeacon.Rewards.test.tssolidity/random-beacon/test/fixtures/index.tssolidity/random-beacon/test/system/e2e.test.tssolidity/random-beacon/test/utils/operators.tssolidity/random-beacon/types/TokenStaking.extensions.d.ts
- test/fixtures: correct walletRegistryFixture JSDoc — Allowlist is the default (useAllowlist ?? true), not TokenStaking; fix examples/remarks. - WalletCreation.test: align gas-test titles with the asserted target (221_000, not 275 000). - RandomBeacon.Authorization.test: await the requestAuthorizationDecrease setup txs in two before() hooks (were fire-and-forget). - random-beacon/hardhat.config: validate GAS_PRICE_GWEI up-front so a malformed value fails fast instead of producing a NaN gasPrice. - types/chai.d.ts: type revertedWithCustomError as awaitable + chainable with .withArgs(...), mirroring hardhat-chai-matchers. - random-beacon-export/README: document that vendored (tsc-compiled) deploy scripts call verify directly and are intentionally not wrapped (build artifacts, do not hand-edit). Skipped CodeRabbit's suggestion to wrap verification in the vendored 01/04/07/09_*.js scripts: those are tsc-compiled artifacts governed by the no-hand-edit regeneration policy; documented the divergence instead. Co-authored-by: Cursor <cursoragent@cursor.com>
…nding drift EcdsaDkgValidator is bound into WalletRegistry once, at proxy init, and no deploy step re-points it. On a redeploy-safe network (e.g. sepolia) a validator bytecode/args change redeploys the validator to a new address and overwrites its deployment artifact, while 03_deploy_wallet_registry skips an already-deployed WalletRegistry. The on-chain WalletRegistry then keeps validating DKG against the old validator while the artifact records the new one. Fail fast before deploying when the validator would actually change (fetchIfDifferent) and a WalletRegistry deployment already exists, directing the operator to wipe and redeploy both together. Re-runs that do not change the validator are unaffected, so idempotent deploys still work.
…reason Six RandomBeacon suites (Authorization, Callback, Group Creation, Relay, Rewards and the system e2e) were disabled via describe.skip with the justification that TokenStaking no longer exposes the legacy stake / increaseAuthorization / approveApplication methods on-chain. The pinned @threshold-network/solidity-contracts@1.3.0-dev.14 TokenStaking still exposes those methods, and the suites' own helpers already bridge the typechain typing gap through legacyTokenStakingAt(). The blocker was a typing gap, not on-chain availability. Re-enable the six suites and drop the inaccurate skip comments. The full random-beacon test run is green (926 passing, 6 pre-existing inner pending).
## Stack Context This is **PR 3/4** in the reorganization stack. Base: #4000 (`stack/testnet4-02-solidity-logic`) Next: #4002 ## What Changed ### 1) Added generated Sepolia deployment snapshots (ECDSA) - Added full deployment artifacts under: - `solidity/ecdsa/deployments/sepolia/.chainId` - `solidity/ecdsa/deployments/sepolia/.migrations.json` - `solidity/ecdsa/deployments/sepolia/*.json` (Allowlist, BLS, Beacon*, Ecdsa*, RandomBeacon*, ReimbursementPool, T, TokenStaking, WalletRegistry*) ### 2) Added generated Sepolia deployment snapshots (Random Beacon) - Added full deployment artifacts under: - `solidity/random-beacon/deployments/sepolia/.chainId` - `solidity/random-beacon/deployments/sepolia/*.json` (BLS, Beacon*, RandomBeacon*, ReimbursementPool, T, TokenStaking) ### 3) Repo hygiene - Removed legacy export artifact: `solidity/random-beacon/export.json` - Gitignored Yarn Berry runtime state (`.yarn/install-state.gz`) in `solidity/random-beacon` to match the ecdsa package. - Gitignored `gasReporterOutput.json` in both `solidity/ecdsa` and `solidity/random-beacon` (regenerated on every test run, not a source artifact). ## Why This Slice Exists This PR intentionally isolates generated/deployment snapshot churn so logic review stays focused in adjacent PRs. ## Out of Scope - Deploy/runtime logic changes. - Go runtime/network mapping changes. ## Test Plan - [ ] Regenerated artifact set matches expected deployment outputs. - [ ] No non-generated behavior changes are introduced in this PR. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added Sepolia testnet support with full deployment artifacts and allowlist-enabled registry flows. * **Improvements** * Stronger deployment safety checks and clearer errors for missing upstream contracts. * Verification now fails on mainnet but is tolerant on non-mainnet. * CI/dev housekeeping: ignore gas reporter outputs and set Sepolia chain IDs. * **Documentation** * Expanded guidance for vendored deploy script regeneration and usage. * **Tests** * Test fixtures updated to require explicit allowlist mode. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
Stack Context
This is PR 2/4 in the reorganization stack.
Base: #3999 (
stack/testnet4-01-ci-yarn)Next: #4001
What Changed
1) ECDSA deploy/runtime logic
solidity/ecdsa/contracts/WalletRegistry.solsolidity/ecdsa/deploy/00_resolve_reimbursement_pool.tssolidity/ecdsa/deploy/00_resolve_token_staking.tssolidity/ecdsa/deploy/01_deploy_ecdsa_sortition_pool.tssolidity/ecdsa/deploy/02_deploy_dkg_validator.tssolidity/ecdsa/deploy/03_deploy_wallet_registry.tssolidity/ecdsa/deploy/07_approve_wallet_registry.tssolidity/ecdsa/deploy/09_deploy_wallet_registry_governance.tssolidity/ecdsa/deploy/etherscanVerification.tssolidity/ecdsa/hardhat.config.tssolidity/ecdsa/tasks/initialize-wallet-owner.ts2) ECDSA test/fixture/type updates
solidity/ecdsa/test/WalletRegistry.Authorization.test.tssolidity/ecdsa/test/WalletRegistry.CustomErrors.test.tssolidity/ecdsa/test/WalletRegistry.Rewards.test.tssolidity/ecdsa/test/WalletRegistry.Slashing.test.tssolidity/ecdsa/test/WalletRegistry.Upgrade.test.tssolidity/ecdsa/test/WalletRegistry.WalletCreation.test.tssolidity/ecdsa/test/WalletRegistryGovernance.test.tssolidity/ecdsa/test/fixtures/index.tssolidity/ecdsa/test/utils/operators.tssolidity/ecdsa/types/TokenStaking.extensions.d.tssolidity/ecdsa/types/chai.d.ts3) Random beacon deploy/config/test alignment
solidity/random-beacon/deploy/05_approve_random_beacon_in_token_staking.tssolidity/random-beacon/hardhat.config.tssolidity/random-beacon/test/RandomBeacon.Authorization.test.tssolidity/random-beacon/test/RandomBeacon.Callback.test.tssolidity/random-beacon/test/RandomBeacon.GroupCreation.test.tssolidity/random-beacon/test/RandomBeacon.Relay.test.tssolidity/random-beacon/test/RandomBeacon.Rewards.test.tssolidity/random-beacon/test/system/e2e.test.tssolidity/random-beacon/test/fixtures/index.tssolidity/random-beacon/test/utils/operators.tssolidity/random-beacon/types/TokenStaking.extensions.d.ts4) Embedded export deploy scripts for ECDSA external random-beacon export
solidity/ecdsa/external/random-beacon-export/deploy/01_*.js ... 09_*.jsOut of Scope
deployments/sepoliaJSON snapshots (handled in chore(deployments): add Sepolia deployment artifacts #4001).Test Plan
cd solidity/ecdsa && yarn testcd solidity/random-beacon && yarn testSummary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation
Tests