build(solidity): migrate Beacon and ECDSA tooling to ethers v6 - #4315
Merged
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
mswilkison
marked this pull request as ready for review
September 8, 2026 19:54
piotr-roslaniec
force-pushed
the
codex/ecdsa-strict-cli
branch
from
September 12, 2026 18:17
604049a to
40731cb
Compare
piotr-roslaniec
force-pushed
the
codex/4295-ethers-v6
branch
2 times, most recently
from
September 13, 2026 07:24
3c73436 to
62e325d
Compare
…n prereq - Override axios to ^1.8.4 (both packages) to clear CVE-2023-45857/CVE-2025-27152 pulled in transitively via Tenderly's own tenderly@0.8.0 dependency - Delete both export-baseline.sha256 manifests: unreferenced by any script, workflow, or doc, and already stale against this migration's own export changes - Fix stale Yarn >=1.22.x prerequisite in both READMEs to match the Yarn 4 packageManager pin already required by both packages
Review findings (agent-docs/reviews/pr-4315/findings.json):
- npm-ecdsa.yml:23, npm-random-beacon.yml:27, contracts-ecdsa.yml,
contracts-random-beacon.yml [P1]: the PR's own release-gate claim was
false for 4 workflow_dispatch jobs (contracts-ecdsa.yml,
contracts-random-beacon.yml) that still ran `npm publish` with a real
token. Guard all four with `if: ${{ false }}`, matching the disabled
scheduled-publish gate, with a comment documenting how to restore them
once the release gate lifts.
- npm-ecdsa.yml:7-14 [P1]: `paths:` filter never extended to utils/**,
external/**, .nvmrc, .yarnrc.yml, .yarn/patches/** even though the
published tarball includes them; a main commit touching only those
paths would silently skip publication. Extended to match
npm-random-beacon.yml's equivalent filter.
- solidity/ecdsa/external/random-beacon-export/,
pack-and-capture.cjs:100-110 [P1]: the checked-in Beacon export bundle's
only freshness check (pack-and-capture.cjs) never runs in CI. Added
bundled-beacon-export-freshness job to contracts-random-beacon.yml:
rebuilds the export and byte-diffs it against the committed bundle,
failing the PR on drift.
npm-ecdsa.yml:32-36 [P2] (hardcoded stale Node 22 vs .nvmrc's 24.11.1) was
independently resolved by the rebase onto dev, which already carries
dev's Node 24 toolchain migration for this file; no further change
needed here.
Review finding (agent-docs/reviews/pr-4315/findings.json):
- solidity/ecdsa/.yarn/patches/@openzeppelin-hardhat-upgrades-npm-2.5.1-etherscan-v2.patch,
solidity/random-beacon/.yarn/patches/@openzeppelin-hardhat-upgrades-npm-2.5.1-etherscan-v2.patch
[P1]: the patch moved every callEtherscanApi parameter into the URL
query string while keeping `method: 'POST'`, dropping the form
body/Content-Type header entirely. hre.run("verify") (used by every
standard sepolia/mainnet deploy script) calls this exact patched
module; verifysourcecode's `sourceCode` parameter is a full
standard-JSON compiler input that would serialize into a tens-of-KB
query string most explorers/intermediaries reject.
Fix: keep the existing URLSearchParams body and
Content-Type: application/x-www-form-urlencoded POST, and add the chain
id as an additional form parameter (`parameters.set('chainid', ...)`)
instead of relocating the whole payload into the query string. Applied
identically to both packages' copies of the patch (src and dist hunks).
Review findings (agent-docs/reviews/pr-4315/findings.json): - pack-and-capture.cjs:51,68-75 [P2]: `npm pack --ignore-scripts` skips each package's `prepack`, so the tarball was packed from whatever export/ already existed on disk, then byte-compared against that same source file -- a near-tautology unable to detect a stale export/. Dropped `--ignore-scripts` so `npm pack` regenerates export/ via prepack before packing. - pack-and-capture.cjs:80 [P2]: hardcoded expected artifact count (`51`/`52`) duplicated the count in ethers-v6-compatibility.md's table with no context on failure. Added a message naming the package and doc table to update when a legitimate contract addition breaks the count. - hardhat.config.cjs:64-68 [P3]: `fs.statSync(...).isDirectory()` with no existsSync guard let a missing subdirectory raise a raw ENOENT before the intended "ECDSA <subdir> export is missing" diagnostic could fire. Added the existsSync guard so the custom message fires for the missing-directory case too. - compare.cjs:130 [P3]: `blocksWithIdenticalState` (computed as before.length - 1) was published under the label "Transactions with identical resulting EVM state" -- the two only coincide because every captured block holds exactly one transaction. Corrected the key/label mismatch.
… resolver Review findings (agent-docs/reviews/pr-4315/findings.json): - solidity/random-beacon/utils/wait-for-confirmations.ts:10-13, solidity/ecdsa/external/random-beacon-export/utils/wait-for-confirmations.js:6-9 [P1]: the v6 replacement for v5's polling `provider.waitForTransaction(hash, 2, 300000)` did a single `getTransaction` call and threw immediately on `null`, with zero retries. Every call site runs right after a live-network deploy under `hre.network.tags.etherscan`, exactly where a just-mined transaction can be transiently invisible to the RPC node serving the next request. Restored polling: retry `getTransaction` on a short interval until it resolves non-null or the timeout budget is exhausted, then delegate to `transaction.wait(confirmations, remainingTimeout)`. Fixed the random-beacon source and regenerated the bundled ECDSA copy to match (not hand-edited). - solidity/ecdsa/utils/random-beacon-export.ts:27-49 [P2]: the resolver picked deploy/tasks/artifacts independently per subdirectory with no atomicity, so an interrupted/partial prepack build could silently mix sibling and bundled/npm sources. Rooted all three resolutions on a single existence check of the export root, throwing if the root exists but a specific requested subdir does not. - solidity/ecdsa/utils/random-beacon-export.ts:31 [P2]: the installed-npm-package fallback branch was unreachable for deploy/tasks in both supported layouts. Branched explicitly by export kind: artifacts resolve from sibling-or-installed-package, deploy/tasks resolve from sibling-or-bundled-copy only, throwing if the bundle is absent instead of falling through to the dead npm branch. - solidity/ecdsa/utils/random-beacon-export.ts:29-33 [P2]: the sibling-source branch preferred a gitignored, hand-regenerated local build over the verified checked-in bundle with no logging of which source was actually selected. Added a log line naming the selected source on every resolution. - solidity/ecdsa/test/utils/random-beacon-export.ts (no dedicated test) [P2]: the resolver's fallback precedence was only exercised incidentally by whichever branch other tests happened to hit. Added solidity/ecdsa/test/utils/random-beacon-export.test.ts covering the precedence and throw-vs-fallback behavior directly via temp directories.
Review findings (agent-docs/reviews/pr-4315/findings.json):
- solidity/ecdsa/test/helpers/mock.ts:632-655 (normalizeForComparison)
[P2]: every exercised call site passed only flat scalar arguments, so
the struct/array recursion behavior the docstring advertises was never
executed by any test. Added a mock.test.ts case calling a mocked
function with a struct argument and asserting expectCalledWith matches
it against a plain-JS-number expectation.
- solidity/ecdsa/test/WalletRegistry.Wallets.test.ts:495 [P3]:
`walletMembersIDs` was assigned once in a `before` hook and mutated in
place by `.reverse()`, corrupting input for a later reused test (the
sibling Slashing.test.ts already does this correctly with
`.slice().reverse()`). Applied the same non-mutating pattern here.
- solidity/ecdsa/test/helpers/gas.ts:14-15, mock.ts:68-69 [P3]: dead code
-- `requireResult()` already throws on null and returns the non-null
value, so the immediately following `if (!x) throw` guard could never
fire. Deleted the unreachable lines (random-beacon's matching instance
addressed in a separate commit).
- solidity/ecdsa/test/fixtures/index.ts,
WalletRegistry.Upgrade.test.ts:96 [P3]: deleted zero-information
trailing comments that only restated the adjacent literal (e.g.
"// Send 1.0 ETH" above `ethers.parseEther("1.0")`).
…thorization Review findings (agent-docs/reviews/pr-4315/findings.json): - solidity/random-beacon/test/tasks/initialize.test.ts:37-43 [P2]: the idempotency test re-invoked only initialize:staking, authorize:beacon, register:beacon -- never the full task nor add_beta_operator:beacon, the one subtask with no read-before-write guard. Extended the test to re-run the full `initialize` task too, which surfaced a real gap: tasks/utils/add_beta_operator.ts sent addBetaOperators unconditionally on every call. Added an `isBetaOperator` idempotency guard matching the existing register/authorize pattern. - solidity/random-beacon/test/tasks/initialize.test.ts:20-34 [P2]: the task's documented default-authorization branch (`application.minimumAuthorization()` when `authorization` is omitted) was never invoked by any test. Added a case that omits `authorization` and asserts the authorized stake equals the beacon's minimumAuthorization(). - solidity/random-beacon/test/fixtures/index.ts:137-143 [P2]: under ethers v6's BaseContract proxy, `typeof stakingAsRecord.X === "function"` is unconditionally true regardless of the underlying ABI, voiding the documented "TokenStaking variant may not expose these methods" guard. Replaced both checks with `staking.interface.hasFunction(...)`. - solidity/random-beacon/test/helpers/mock.ts:108-110 [P2]: widening `Mock<T extends BaseContract>` projected every BaseContract member into the handle's type even though the runtime proxy returns undefined for anything outside the mocked ABI/allowlist. Excluded the base-contract surface from the mapped type. - solidity/random-beacon/test/helpers/mock.ts:68-69 [P3]: dead code -- `requireResult()` already throws on null, so the following `if (!x) throw` guard could never fire. Deleted the unreachable line. - solidity/random-beacon/test/ReimbursementPool.test.ts:45,65,81,145,227, fixtures/index.ts [P3]: deleted zero-information trailing comments that only restated the adjacent literal.
Review finding (agent-docs/reviews/pr-4315/findings.json): - solidity/ecdsa/deploy/17_upgrade_wallet_registry_v2.ts, solidity/ecdsa/deploy/16_initialize_allowlist_weights.ts [P2]: both scripts received substantial v5->v6 rewrites (raw EIP-1967 slot decoding, getFunction(name)(...) indirection, ABI JSON re-encoding) but are gated by env-var skip flags no test sets, leaving the rewritten bodies entirely unexercised. Added solidity/ecdsa/test/WalletRegistry.UpgradeV2Deploy.test.ts, running deploy/17_upgrade_wallet_registry_v2.ts against the in-process hardhat network (with deploy/11_transfer_proxy_admin_ownership included, since script 17's testnet path requires ownership already transferred) and asserting: the new implementation deployment artifact is saved with a non-zero address, the WalletRegistry proxy's EIP-1967 implementation slot points at it, WalletRegistry.allowlist() reflects the script's initializeV2 call, and WalletRegistry.governance() is preserved (compared against its own pre-upgrade value, since governance here is the deployed WalletRegistryGovernance contract, not a named EOA). deploy/16_initialize_allowlist_weights.ts is not covered: it requires a deploy-data/allowlist-weights-<network>.json that does not exist for the hardhat network (only mainnet/sepolia variants are checked in), and fabricating one would mean inventing real staking-provider/weight business data rather than test scaffolding. Reported per the finding's explicit escape hatch for partial completion.
Not tied to a specific review finding -- these fix bugs the dev-rebase's
conflict resolution introduced (or exposed via `hardhat compile` +
`tsc --noEmit`, which hadn't been run clean for random-beacon in this
checkout until now), caught by verifying the fix batches above.
- solidity/random-beacon/tsconfig.json: the rebase's additive merge kept
both dev's `downlevelIteration` and this PR's `noImplicitAny`; TS 6
rejects `downlevelIteration` as deprecated. Target is ES2020, which
doesn't need it -- dropped rather than suppressed.
- solidity/ecdsa/deploy/15_deploy_allowlist.ts: a conflict resolution
left a log line referencing an undefined `proxyDeployment`. Destructure
it from `deployProxy`'s return, matching
deploy/03_deploy_wallet_registry.ts's identical pattern.
- solidity/ecdsa/test/WalletRegistryGovernance.test.ts,
solidity/random-beacon/test/RandomBeaconGovernance.test.ts: a bulk
conflict resolution replaced all call sites of the v5-only
`minedBlockTimestamp` helper with inlined v6 logic but left the now-
dead definition (using the nonexistent `ContractTransaction` type)
behind in both files. Deleted both.
- solidity/ecdsa/test/WalletRegistry.RandomBeacon.test.ts,
solidity/random-beacon/test/{Groups.test.ts,RandomBeacon.Callback.test.ts,
RandomBeacon.GroupCreation.test.ts,utils/dkg.ts}: several
`(await tx.wait()).blockNumber` sites were missed by the migration's
own null-safety pass; ethers v6's `tx.wait()` can return null. Wrapped
with the existing `requireResult()` helper (dkg.ts's `genesis()` was
missing the `receipt` variable entirely), matching the pattern already
used throughout the rest of the migration.
- solidity/random-beacon/test/tasks/initialize.test.ts: reordered the
new "defaults authorization" test (added while fixing finding #16 in a
separate commit) to run after "tops up an existing stake", not between
two reuses of the `initializedOperator` fixture -- interleaving a
second `deployments.fixture()`-backed fixture there corrupted
hardhat-network-helpers' snapshot bookkeeping
(FixtureSnapshotError/InvalidSnapshotError) for the later reuse.
Verified with FORKING_URL unset (this sandbox's ambient mainnet-fork env
var, unrelated to this repo, was flipping both packages' `hardhat`
network into forking mode against @threshold-network/solidity-contracts'
external NuCypherToken resolver, which only tolerates forking when a
real deployed token exists -- CI never sets this var): both packages'
`tsc --noEmit` are clean and their full `yarn test` suites pass (963/0
random-beacon, 692/0 ecdsa).
Same correction as codex/ecdsa-strict-cli's fc76751, applied to this branch's own further-along state: - export-baseline.sha256 (both packages): finding #11 in the tracked review concluded these files were unreferenced dead weight and this branch deleted them (05ec644). That was wrong -- the workflow's contracts-export-byte-identity job explicitly diffs against them (issue #4216's downstream-consumer byte-identity gate for @keep-network/tbtc-v2). Restored both and regenerated against this branch's own export/artifacts via the documented procedure. ecdsa's export.json (the tracked mainnet record, distinct from random-beacon's gitignored copy) was exported to a scratch path during regeneration, never touched in the working tree. - Prettier: fixed the two random-beacon files unique to this branch (test/helpers/mock.ts, test/tasks/initialize.test.ts). The two ecdsa files flagged by the same lint job are inherited from codex/ecdsa-strict-cli and fixed there; the upcoming rebase picks up that fix directly.
piotr-roslaniec
force-pushed
the
codex/4295-ethers-v6
branch
from
September 13, 2026 07:59
62e325d to
9c81ba0
Compare
The idempotency guard added to random-beacon's add_beta_operator task never propagated to ecdsa's bundled mirror, which the bundled-beacon-export-freshness CI check (contracts-random-beacon.yml) only exercises when the PR's base is main or dev. Regenerated via the process documented in solidity/ecdsa/external/random-beacon-export/README.md.
approveDkgResult's real-world gas usage clusters at ~344,900-345,000, right at the edge of the old 330,000 +/- 15,000 window (ceiling 345,000). CI observed 345,003, exceeding it by 3 gas units. Widened the delta to 20,000 for both identical assertions to give real headroom while still catching genuine regressions (which would be orders of magnitude larger).
# Conflicts: # solidity/ecdsa/package.json # solidity/ecdsa/yarn.lock # solidity/random-beacon/package.json # solidity/random-beacon/yarn.lock
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The Solidity packages still depended on ethers v5 after the strict TypeScript and Waffle-removal preparation. This ports their tests, tasks and executable deployment exports to ethers v6 while retaining Hardhat 2.29.0, maintained hardhat-deploy 1.0.4 and ES2020/CommonJS. It is the next preparation step for #4295; the Hardhat 3/Rocketh production port remains separate.
Related feasibility proof: #4316.
Stacked on #4306 (
codex/ecdsa-strict-cli). Includes the maintained-deploy and Node 24/Yarn 4 preparation, a tested v5 comparison baseline, and the v6 migration.targetname collision.export-deployment-artifactstask in both npm publication hooks, preserving explicit network selection and defaulting to Hardhat when--networkis omitted. Document publication lifecycle reproduction.Validation on Node 24.11.1: immutable installs, both strict TypeScript checks, CommonJS export/prepack builds and full lint pass (existing warnings remain). Beacon: 962 passing, including seven new confirmation and initialization-task tests. ECDSA: 679 passing, 44 existing pending, including six task entrypoint checks. The full ECDSA suite passes with sibling Beacon exports unavailable. Fresh production-contract deployments preserve all 74 per-transaction EVM state roots and byte-identical export.json for 16 Beacon / 22 ECDSA contracts. The actual packed producers match the v6 source deployment exactly, including 46 ECDSA transaction state roots, all 22 deployment records and 52 artifacts. All 19 bundled JavaScript files match Beacon's compiled exports byte for byte. All six task checks pass through the packed ECDSA configuration with both explicit v6 Beacon exports and the pinned v5 dependency, exercising ECDSA's shipped v6 task bundle. The five initialization/registration regressions reproduce the original v5 failures before the import fix. The bundled fallback also matches its like-for-like v5 baseline.
Review regression checks: both full Linux/arm64 Docker builds and container runtime checks pass. Their build contexts contain exactly the four checked-in patches, with host Yarn caches and install state excluded; the finished images omit Python, make and g++. Beacon's immutable install, the workflow's local deployment (including TokenholderTimelock), and prepack pass on Node 24.11.1.
Publication lifecycle validation on Node 24.11.1/npm 11.6.2: six offline
npm publish --dry-runchecks pass across both packages (explicit Hardhat, omitted network, and Sepolia snapshots). They exerciseprepublishOnlyandprepack, plus ECDSA'sprepare. Exported deployment records match their sources byte for byte, and every contract artifact appears in npm's packed file list. Both previous hooks reproduceHH303through the same npm lifecycle. Both gated npm workflows parse as YAML and pass Prettier and whitespace checks; all four publishing steps are covered by the disabled jobs.Two explicit data differences are documented and checked: six ECDSA gas-limit changes produce different transaction/block hashes without changing actual gas used or state; TokenStaking's exported artifact gains its compiler-verified storageLayout. All other artifact files are byte-identical. No Solidity sources, live deployment records or OpenZeppelin manifests change.
Both npm publishing workflows are disabled at the job level, blocking automatic
mainpublication todevelopmentand manual publication todevelopmentorlatest. Re-enabling them requires a coordinated release change after consumer migration, upstream fixes, release channel agreement, a compatible Beacon dependency pin, and full packed-producer ECDSA/tbtc-v2 validation. CommonJS v6 scripts still require a compatible consumer runtime. Patches do not propagate into downstream installs, and ECDSA must pin a newly released v6 Beacon before executable exports are published. Full tbtc-v2 integration, upstream releases, explorer services, the manual V2/deputy workflow and the Hardhat 3/Rocketh cutover remain release gates. Keep #4295 open. Details and reproduction commands are insolidity/docs/ethers-v6-compatibility.md.