build(solidity): raise hardhat to 2.19.5 and pin smock, in both packages - #4202
Merged
Conversation
Prerequisite for removing @defi-wonderland/smock, which is archived upstream and
marked "Package no longer supported" on npm. Nothing about the tests changes
here; this only moves the three dependencies that have to move first, and
proves the suites are unaffected.
Three coupled bumps, none of which works alone:
hardhat ^2.10.0 -> 2.19.5
@defi-wonderland/smock ^2.0.7 -> 2.3.4
hardhat-gas-reporter ^1.0.8 -> ^2.3.0
hardhat first, because the contract-backed mock that replaces smock configures
itself with transactions, and a transaction mines a block. Keeping that from
moving the chain clock needs `allowBlocksWithSameTimestamp`, and that option
does not exist in 2.10.0 -- it arrives by 2.14. 2.19.5 is the last version smock
still works on, so it is the one version with both properties.
smock second, because 2.0.7 does not survive that bump. On hardhat 2.19.5 it
dies before any test runs:
TypeError: vm.on is not a function
at new ObservableVM (@defi-wonderland/smock/src/observable-vm.ts:19:35)
at Sandbox.create (@defi-wonderland/smock/src/sandbox.ts:82:12)
2.3.4 is the version that works there. It is pinned exactly rather than left as
a range: `^2.0.7` resolves to 2.4.1 today, so the lockfile was one regeneration
away from silently jumping four minor versions into an archived package. That
drift is also why 2.4.1 is not the target -- it claims hardhat >=2.21 support
and still fails, differently, with `provider.init is not a function`.
hardhat-gas-reporter third, because 1.x bundles eth-gas-reporter, which the
mocha inside hardhat 2.19.5 refuses to load on Node 22 -- ERR_MOCHA_INVALID_REPORTER,
raised before a single test executes. Only random-beacon hits it, because only
random-beacon enables the reporter, but both packages are moved to keep them
aligned. This matters now rather than later because #4201 puts CI on Node 22.
Measured, whole suite, each package on both runtimes:
before (hh 2.10, smock 2.0.7) after
random-beacon 926 passing / 6 pending / 0 926 / 6 / 0
ecdsa 650 passing / 44 pending / 0 650 / 44 / 0
Identical on Node 18 and Node 22. No test file is touched.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughA provider-backed programmable Solidity mock and TypeScript wrapper replace Smock usage across ECDSA and random-beacon tests. Tests now support ABI-shaped returns, reverts, call recording, static-call handling, resets, and asynchronous assertions. ChangesProgrammable mock infrastructure
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related issues
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
Adds `contracts/test/MockContract.sol` and `test/helpers/mock.ts` to both packages, with their own self-tests. Nothing uses them yet -- smock is still installed and every existing call site is untouched, so this changes no behaviour. The removal is the next commit. Ported from threshold-network/tbtc-v2#1062, where it has since replaced 554 smock call sites and runs green on hardhat 2.29 and Node 24. It carries the fixes that migration surfaced, including two found only last week: argument comparison by value rather than representation, and relocating an address-pinned mock before configuring it rather than after, since `hardhat_setCode` copies code and not storage. smock configured its fakes by mutating in-process JavaScript state inside Hardhat's EVM, which is why its API was synchronous -- and why it breaks on every hardhat past 2.19. This drives an ordinary deployed contract over the public provider API, so it depends on nothing Hardhat can change underneath it. The one behavioural difference is that configuration is a transaction and must be awaited, which is what makes the next commit touch call sites at all. `allowBlocksWithSameTimestamp` is set in both configs because of that: a transaction mines a block, and Hardhat otherwise forces each block at least a second after its parent, so configuring a mock would silently advance the chain clock. The helper pins the next block's timestamp before every write; this option is what lets it reuse the current one. Verified in both packages -- 22 self-tests each, covering the zero-value return layer, calldata-specific answers, reverts, call recording, `msg.value`, the STATICCALL restriction on recording, and address pinning: ecdsa 22 passing random-beacon 22 passing Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Four files: the shared `fakeTokenStaking` helper and the three suites that use it. `smock.fake` becomes `createMock`, `FakeContract` becomes `Mock`, and the `reverts` configuration is awaited, since configuring this mock is a transaction rather than a JavaScript assignment. The interesting part is what came back. Three contexts were parked as `context.skip` behind // FIXME: Blocked by wonderland-archive/smock#101 so the "token staking seize call fails" paths -- the ones asserting that a failing `seize` does not take the whole operation down with it -- had not run in years. They are enabled here and pass against `MockContract`, which is the point of replacing smock rather than merely pinning it. Also drops three `tokenStakingFake.seize.reset()` teardowns. Each ran *after* `restoreSnapshot()`, and the mock is created inside that snapshot, so the reset was operating on state where the mock no longer existed. Under smock it was a synchronous no-op; here it would be an unawaited transaction. before 926 passing / 6 pending / 0 failing after 954 passing / 0 pending / 0 failing 954 = 926 + 22 mock self-tests + the 6 recovered. No test file loses coverage and nothing is skipped any more. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Sixteen files. `smock.fake` becomes `createMock`, `FakeContract` becomes `Mock`, `chai.use(smock.matchers)` goes, and the chai matchers become explicit helper calls -- `to.have.been.called` and the `to.be.calledWith` spelling both appear here, so both forms are rewritten. Configuration is awaited throughout, since it is a transaction now rather than a JavaScript assignment. before 650 passing / 44 pending / 0 failing after 673 passing / 44 pending / 0 failing 673 = 650 + the 23 mock self-tests. No suite loses a test. Four things needed judgement rather than a mechanical rewrite. `expectCalledWith` is new, added to the helper here. smock's `calledWith` says nothing about how many times a function ran, so translating those two sites to the existing `expectCalledOnceWith` would have quietly added an assertion the tests never made. It has its own self-test, including the negative case. Five assertions in the TIP-092 migration suite checked that `IStaking.authorizedStake` had been called. It is `view`, so Solidity reaches it by STATICCALL, where recording is impossible -- the mock refuses the assertion loudly rather than answering zero. Four of the five already asserted the value the mock returns, which is the same proof by a stronger route, so the call check is dropped with a comment saying why. The fifth, `updateOperatorStatus`, had no other assertion at all; it now asserts the operator is up to date, which is only true if the registry read the allowlist and synced the pool to it. `smock.mock` at WalletRegistry.RandomBeacon.test.ts:236 was never a mock in any meaningful sense -- it deployed `RandomBeaconStub` and only ever read `.address`, with no `setVariable` or `getVariable` anywhere. It is now a plain factory deploy. `resetMock` is gone. Both callers were smock-era bookkeeping that became harmful once resetting meant sending a transaction. In the RandomBeacon suite it ran *after* `restoreSnapshot()`, so it was both redundant and, left unawaited, landed in the following context and wiped the call log that context asserted on. In `createNewWallet` it mined a block in the middle of a shared helper, which shifted `WalletRegistry - Wallets`'s cumulative state enough to flip an `isWalletMember` assertion in the full run while passing in isolation. Nothing depends on either reset; the suite is green without them. One gas budget moved. `approveDkgResult` reaches the wallet owner, and recording a call SSTOREs its calldata -- 447k against a 274k +/- 1000 budget. Switching recording off for that suite's wallet owner, which nothing in the file inspects, brings it to ~286k. The remaining ~12k is `MockContract`'s fallback dispatch, real work that smock's fake did not do, so the expectation moves 274 000 -> 286 000 and the tolerance stays. In production the wallet owner is a real contract and costs more than either number. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The dependency is gone from both packages. Upstream archived it with:⚠️ DEPRECATED – DO NOT USE It may contain outdated, insecure, or vulnerable code and should not be used in production or as a dependency in any project. No support, updates, or security patches will be provided. npm carries the matching deprecation and the repository is archived. That is the reason this work happened. The toolchain ceiling was only what made it unavoidable rather than schedulable: no smock version reaches hardhat 2.26+, and hardhat 2.26+ is what Node 24 requires, so this also unblocks that bump. Verified with smock fully uninstalled -- not merely unused: random-beacon 955 passing / 0 pending / 0 failing ecdsa 673 passing / 44 pending / 0 failing Against the pre-migration baselines of 926/6/0 and 650/44/0. The increases are the 23 mock self-tests in each package and the six tests in random-beacon that smock's issue #101 had kept behind `context.skip` since they were written. BREAKING CHANGE: `@defi-wonderland/smock` is no longer a dependency. Any out-of-tree test importing it will fail to resolve. Use `createMock` from `test/helpers/mock` instead; the API is the same shape except that configuration returns a promise and must be awaited. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
**Stacked on #4202.** Removes `@defi-wonderland/smock`, which upstream archived with this notice: >⚠️ **DEPRECATED – DO NOT USE** > > It may contain **outdated, insecure, or vulnerable code** and should **not** > be used in production or as a dependency in any project. > > No support, updates, or security patches will be provided. That is the reason. The toolchain ceiling — no smock version reaches hardhat 2.26+, which is what Node 24 needs — is what made it unavoidable rather than schedulable, and removing it unblocks that bump. ## Result Verified with smock **uninstalled**, not merely unused: | | before | after | | --- | --- | --- | | `random-beacon` | 926 passing / 6 pending / 0 failing | **955 / 0 / 0** | | `ecdsa` | 650 passing / 44 pending / 0 failing | **673 / 44 / 0** | The increases are the 23 mock self-tests in each package, plus **six tests in `random-beacon` that smock kept disabled**. They sat behind ``` // FIXME: Blocked by wonderland-archive/smock#101 context.skip("when token staking seize call fails", ...) ``` — the paths asserting that a failing `seize` does not take the whole operation down with it. They pass against `MockContract`. That is the point of replacing smock rather than pinning it. ## The replacement `contracts/test/MockContract.sol` plus `test/helpers/mock.ts`, ported from threshold-network/tbtc-v2#1062 where it has replaced 554 call sites and runs on hardhat 2.29 / Node 24. smock configured fakes by mutating in-process JavaScript inside Hardhat's EVM, which is why it broke on every hardhat past 2.19; this drives an ordinary deployed contract over the public provider API. The one behavioural difference is that **configuration is a transaction and must be awaited**, which is why call sites change at all. `allowBlocksWithSameTimestamp` is set in both configs so that configuring a mock cannot advance the chain clock. ## Judgement calls worth reviewing **`expectCalledWith` is new.** smock's `calledWith` says nothing about call count, so reusing the existing `expectCalledOnceWith` for those two sites would have silently strengthened the assertion. It has its own self-test including the negative case. **Five assertions on a `view` function had to change.** The TIP-092 suite checked that `IStaking.authorizedStake` was called; it is `view`, so Solidity reaches it by STATICCALL where recording is impossible, and the mock refuses loudly rather than answering zero. Four already asserted the value the mock returns — the same proof by a stronger route — so the call check is dropped with a comment. The fifth, `updateOperatorStatus`, had **no other assertion**; it now asserts the operator is up to date, which holds only if the registry read the allowlist and synced the pool to it. **`resetMock` is gone, and this one caused a real failure.** Both callers were smock-era bookkeeping that became harmful once resetting meant sending a transaction. In the RandomBeacon suite it ran *after* `restoreSnapshot()`, so it was redundant and, unawaited, landed in the next context and wiped the call log that context asserted on. In `createNewWallet` it mined a block inside a shared helper, which shifted cumulative state enough to flip an `isWalletMember` assertion in the full run while passing in isolation. **One gas budget moved**, and it is the only assertion relaxed rather than replaced. `approveDkgResult` reaches the wallet owner; recording a call SSTOREs its calldata — 447k against a 274k ± 1000 budget. Switching recording off for that suite (nothing there inspects the wallet owner's calls) gives ~286k. The residual ~12k is `MockContract`'s fallback dispatch, real work smock's fake did not do, so the expectation moves to 286 000 and the tolerance stays. In production the wallet owner is a real contract and costs more than either. **`smock.mock` was a false alarm.** The one use only ever read `.address`, with no `setVariable`/`getVariable`, so it is now a plain factory deploy. ## Next hardhat 2.19.5 → 2.29 and Node 22 → 24, which this unblocks.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (6)
solidity/ecdsa/test/helpers/mock.ts (3)
522-551: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winProxy handle keys silently shadow same-named ABI functions, in both copies of
mock.ts. Theproperty in basebranch resolves before the ABI lookup, so a mocked interface with a function namedaddress,connect,reset,wallet,mockContractorsetRecordingyields the handle member instead of aMockedFunction— the same silent-shadowing hazardassertNoSelectorCollisionwas written to prevent on the Solidity side.
solidity/ecdsa/test/helpers/mock.ts#L522-L551: add a construction-time check that no name inbyNamecollides with the handle's reserved keys, throwing the wayassertNoSelectorCollisiondoes.solidity/random-beacon/test/helpers/mock.ts#L522-L551: add the identical check.🤖 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/helpers/mock.ts` around lines 522 - 551, Add a construction-time reserved-key collision check before creating the Proxy handle in solidity/ecdsa/test/helpers/mock.ts:522-551 and apply the identical change in solidity/random-beacon/test/helpers/mock.ts:522-551. Validate every name in byName against the handle keys address, wallet, mockContract, connect, reset, and setRecording, and throw using the same pattern and behavior as assertNoSelectorCollision; leave the Proxy lookup logic unchanged.
364-371: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
hardhat_setBalanceunconditionally overwrites the balance at a pinned address, in both copies ofmock.ts. Pinning a mock over an address that already held a funded contract silently resets it to 10,000 ETH — a side effect absent from theoptions.addressdocumentation at Lines 298-299.
solidity/ecdsa/test/helpers/mock.ts#L364-L371: gate thehardhat_setBalancecall on the current balance being insufficient, or document the overwrite; also drop the now-redundant 1000 ETH top-up insolidity/ecdsa/test/utils/randomBeacon.ts.solidity/random-beacon/test/helpers/mock.ts#L364-L371: apply the identical gate.🤖 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/helpers/mock.ts` around lines 364 - 371, Update the mock setup around hardhat_setBalance in solidity/ecdsa/test/helpers/mock.ts:364-371 and solidity/random-beacon/test/helpers/mock.ts:364-371 to check the impersonated address balance first and only fund it when insufficient, preserving existing funded balances. Remove the redundant 1000 ETH top-up from solidity/ecdsa/test/utils/randomBeacon.ts.
399-421: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winA
whenCalledWithentry that fails to encode degrades to aconsole.warnno-op.The narrowing silently disappears and the test still passes against the selector-wide default; a
console.warnin a CI log is easy to miss. The rationale cites preserving smock's behaviour for pre-existing tbtc tests, but nothing insolidity/ecdsaorsolidity/random-beaconis shown to depend on that leniency. Consider throwing by default with an explicit escape hatch for the known-bad call sites.🤖 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/helpers/mock.ts` around lines 399 - 421, Update the whenCalledWith handling around targetInterface.encodeFunctionData to throw when arguments cannot be encoded, rather than logging a warning and returning. Add an explicit opt-in escape hatch for the known call sites that require smock-compatible leniency, and use it only there; preserve the existing warning-and-skip behavior when that escape hatch is enabled.solidity/ecdsa/contracts/test/MockContract.sol (2)
110-124: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftMisplaced ERC-7201 doc comment in both copies of
MockContract.sol. The two files are verbatim duplicates, so the detachedSTATE_SLOTdocumentation — sitting above theRECORD_GAS_STIPENDblock and leavingSTATE_SLOTundocumented — exists twice and must be fixed in both.
solidity/ecdsa/contracts/test/MockContract.sol#L110-L124: move thekeccak256("tbtc.test.MockContract.state.v1")@devblock down so it sits directly above theSTATE_SLOTdeclaration at Line 123.solidity/random-beacon/contracts/test/MockContract.sol#L110-L124: apply the identical move.More broadly: these two contracts,
IMockTarget.sol,MockTargetConsumer.solandtest/helpers/mock.tsare all byte-identical across the two packages. Every fix in this review has to be applied twice, and the copies will drift. Worth extracting into a shared test-contracts package, or at minimum generating one copy from the other in a build step.🤖 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/contracts/test/MockContract.sol` around lines 110 - 124, Move the ERC-7201 STATE_SLOT documentation so it immediately precedes STATE_SLOT, leaving RECORD_GAS_STIPEND documented only by its gas-stipend comments. Apply this identical change in solidity/ecdsa/contracts/test/MockContract.sol lines 110-124 and solidity/random-beacon/contracts/test/MockContract.sol lines 110-124; keep both duplicate MockContract copies synchronized.
52-60: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueComments cite tBTC Bridge artifacts that don't exist in this package.
test/fixtures/bridge.ts, "19 call sites",transferTokensWithPayload/Wormhole (Lines 96-99) andAbstractBTCDepositor/bridge.depositParameters()(Lines 478-481) are not part ofsolidity/ecdsaorsolidity/random-beacon. The reasoning holds; the citations should be re-pointed at this package's actual pinned-address call sites (e.g.fakeRandomBeaconinsolidity/ecdsa/test/utils/randomBeacon.ts) or generalized.🤖 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/contracts/test/MockContract.sol` around lines 52 - 60, Update the NatSpec comment above the hashed base slot in MockContract to remove nonexistent Bridge-specific references such as test/fixtures/bridge.ts and the “19 call sites” claim. Reword the rationale generically or cite actual pinned-address usage in this package, such as fakeRandomBeacon in randomBeacon.ts, while preserving the explanation that hardhat_setCode leaves existing storage intact.solidity/ecdsa/test/helpers/mock.test.ts (1)
281-301: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNo coverage for
setRecording.
setRecording(false)is the documented escape hatch for gas-measuring tests (mock.tsLines 117-124), and the PR notes a gas budget had to be adjusted. A regression there would silently reinstate the mock's SSTORE cost in someone else's gas assertion. A pair of cases — recording off suppressescallCount, recording back on resumes it — would close the gap. Happy to draft them.🤖 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/helpers/mock.test.ts` around lines 281 - 301, Add coverage in the existing “call assertions on read-only functions” tests for the mock’s setRecording behavior: verify setRecording(false) suppresses callCount recording, then call setRecording(true) and verify subsequent calls are recorded again. Use the existing target/readValue setup and restore recording state so the tests remain isolated.
🤖 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/contracts/test/MockContract.sol`:
- Around line 349-351: Move the raw-calldata `@notice` documentation from above
__mock__callValueForSelectorAt to directly above __mock__callForSelectorAt,
leaving only the msg.value notice attached to the value accessor. Apply this
identical documentation correction in
solidity/ecdsa/contracts/test/MockContract.sol lines 349-351 and
solidity/random-beacon/contracts/test/MockContract.sol lines 349-351.
- Around line 110-124: Move the ERC-7201 derivation documentation from above
RECORD_GAS_STIPEND to directly above STATE_SLOT, keeping the gas-stipend
explanation attached only to RECORD_GAS_STIPEND and the slot derivation comment
attached to STATE_SLOT.
In `@solidity/ecdsa/test/helpers/mock.test.ts`:
- Around line 168-171: Update the comment in the reset behavior test near
consumer.doThing to state that reset clears only the selector-specific response
while preserving the construction-time base zero-return layer, so the call
returns the ABI-encoded zero value and lastResult() is false. Leave the test
behavior and assertions unchanged.
In `@solidity/ecdsa/test/helpers/mock.ts`:
- Around line 612-613: The JSDoc summary for expectCalledOnceWith is attached to
normalizeForComparison in both copies of mock.ts. Move that JSDoc line from
immediately before normalizeForComparison to immediately before export async
function expectCalledOnceWith in solidity/ecdsa/test/helpers/mock.ts and
solidity/random-beacon/test/helpers/mock.ts.
In `@solidity/random-beacon/contracts/test/MockContract.sol`:
- Around line 110-124: Move the ERC-7201 keccak256/state-slot documentation so
it directly precedes the STATE_SLOT declaration, keeping the RECORD_GAS_STIPEND
documentation attached only to RECORD_GAS_STIPEND.
---
Nitpick comments:
In `@solidity/ecdsa/contracts/test/MockContract.sol`:
- Around line 110-124: Move the ERC-7201 STATE_SLOT documentation so it
immediately precedes STATE_SLOT, leaving RECORD_GAS_STIPEND documented only by
its gas-stipend comments. Apply this identical change in
solidity/ecdsa/contracts/test/MockContract.sol lines 110-124 and
solidity/random-beacon/contracts/test/MockContract.sol lines 110-124; keep both
duplicate MockContract copies synchronized.
- Around line 52-60: Update the NatSpec comment above the hashed base slot in
MockContract to remove nonexistent Bridge-specific references such as
test/fixtures/bridge.ts and the “19 call sites” claim. Reword the rationale
generically or cite actual pinned-address usage in this package, such as
fakeRandomBeacon in randomBeacon.ts, while preserving the explanation that
hardhat_setCode leaves existing storage intact.
In `@solidity/ecdsa/test/helpers/mock.test.ts`:
- Around line 281-301: Add coverage in the existing “call assertions on
read-only functions” tests for the mock’s setRecording behavior: verify
setRecording(false) suppresses callCount recording, then call setRecording(true)
and verify subsequent calls are recorded again. Use the existing
target/readValue setup and restore recording state so the tests remain isolated.
In `@solidity/ecdsa/test/helpers/mock.ts`:
- Around line 522-551: Add a construction-time reserved-key collision check
before creating the Proxy handle in solidity/ecdsa/test/helpers/mock.ts:522-551
and apply the identical change in
solidity/random-beacon/test/helpers/mock.ts:522-551. Validate every name in
byName against the handle keys address, wallet, mockContract, connect, reset,
and setRecording, and throw using the same pattern and behavior as
assertNoSelectorCollision; leave the Proxy lookup logic unchanged.
- Around line 364-371: Update the mock setup around hardhat_setBalance in
solidity/ecdsa/test/helpers/mock.ts:364-371 and
solidity/random-beacon/test/helpers/mock.ts:364-371 to check the impersonated
address balance first and only fund it when insufficient, preserving existing
funded balances. Remove the redundant 1000 ETH top-up from
solidity/ecdsa/test/utils/randomBeacon.ts.
- Around line 399-421: Update the whenCalledWith handling around
targetInterface.encodeFunctionData to throw when arguments cannot be encoded,
rather than logging a warning and returning. Add an explicit opt-in escape hatch
for the known call sites that require smock-compatible leniency, and use it only
there; preserve the existing warning-and-skip behavior when that escape hatch is
enabled.
🪄 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: 0ea76113-2655-492f-8412-99f6d1cc230e
⛔ Files ignored due to path filters (2)
solidity/ecdsa/yarn.lockis excluded by!**/yarn.lock,!**/*.locksolidity/random-beacon/yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (35)
.gitignoresolidity/ecdsa/contracts/test/IMockTarget.solsolidity/ecdsa/contracts/test/MockContract.solsolidity/ecdsa/contracts/test/MockTargetConsumer.solsolidity/ecdsa/hardhat.config.tssolidity/ecdsa/package.jsonsolidity/ecdsa/test/Allowlist.test.tssolidity/ecdsa/test/DKGValidator.test.tssolidity/ecdsa/test/WalletRegistry.Authorization.test.tssolidity/ecdsa/test/WalletRegistry.CustomErrors.test.tssolidity/ecdsa/test/WalletRegistry.DualMode.test.tssolidity/ecdsa/test/WalletRegistry.Inactivity.test.tssolidity/ecdsa/test/WalletRegistry.Parameters.test.tssolidity/ecdsa/test/WalletRegistry.RandomBeacon.test.tssolidity/ecdsa/test/WalletRegistry.Rewards.test.tssolidity/ecdsa/test/WalletRegistry.Slashing.test.tssolidity/ecdsa/test/WalletRegistry.WalletCreation.test.tssolidity/ecdsa/test/WalletRegistry.WalletOwner.test.tssolidity/ecdsa/test/WalletRegistry.Wallets.test.tssolidity/ecdsa/test/fixtures/index.tssolidity/ecdsa/test/helpers/mock.test.tssolidity/ecdsa/test/helpers/mock.tssolidity/ecdsa/test/utils/randomBeacon.tssolidity/ecdsa/test/utils/wallets.tssolidity/random-beacon/contracts/test/IMockTarget.solsolidity/random-beacon/contracts/test/MockContract.solsolidity/random-beacon/contracts/test/MockTargetConsumer.solsolidity/random-beacon/hardhat.config.tssolidity/random-beacon/package.jsonsolidity/random-beacon/test/RandomBeacon.Authorization.test.tssolidity/random-beacon/test/RandomBeacon.GroupCreation.test.tssolidity/random-beacon/test/RandomBeacon.Relay.test.tssolidity/random-beacon/test/helpers/mock.test.tssolidity/random-beacon/test/helpers/mock.tssolidity/random-beacon/test/mocks/staking.ts
💤 Files with no reviewable changes (2)
- solidity/random-beacon/package.json
- solidity/ecdsa/package.json
Comment on lines
+110
to
+124
| /// @dev keccak256("tbtc.test.MockContract.state.v1") - 1, masked, per the | ||
| /// ERC-7201 convention. | ||
| /// @dev Gas handed to the recording self-call. | ||
| /// | ||
| /// An explicit cap matters more than the value. try/catch does not | ||
| /// recover from out-of-gas: an uncapped sub-call takes 63/64 of what | ||
| /// is left, so if recording runs out, the caller is left with too | ||
| /// little to continue and the whole transaction reverts. Capping it | ||
| /// means a failed recording costs this much and nothing more, which | ||
| /// keeps recording genuinely best-effort. Generous enough for any | ||
| /// realistic calldata. | ||
| uint256 private constant RECORD_GAS_STIPEND = 1_000_000; | ||
|
|
||
| bytes32 private constant STATE_SLOT = | ||
| 0xdd9627cd601a6555f69239ac336fba8db95c419564b84ebb3ee2c21fed88ae00; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
The ERC-7201 doc comment is detached from STATE_SLOT.
Lines 110-111 document the state slot derivation but are immediately followed by the RECORD_GAS_STIPEND doc block, so both @dev blocks land on the gas constant while STATE_SLOT at Line 123 is undocumented.
📝 Proposed fix
- /// `@dev` keccak256("tbtc.test.MockContract.state.v1") - 1, masked, per the
- /// ERC-7201 convention.
/// `@dev` Gas handed to the recording self-call.
///
/// An explicit cap matters more than the value. try/catch does not
@@
uint256 private constant RECORD_GAS_STIPEND = 1_000_000;
+ /// `@dev` keccak256("tbtc.test.MockContract.state.v1") - 1, masked, per the
+ /// ERC-7201 convention.
bytes32 private constant STATE_SLOT =
0xdd9627cd601a6555f69239ac336fba8db95c419564b84ebb3ee2c21fed88ae00;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| /// @dev keccak256("tbtc.test.MockContract.state.v1") - 1, masked, per the | |
| /// ERC-7201 convention. | |
| /// @dev Gas handed to the recording self-call. | |
| /// | |
| /// An explicit cap matters more than the value. try/catch does not | |
| /// recover from out-of-gas: an uncapped sub-call takes 63/64 of what | |
| /// is left, so if recording runs out, the caller is left with too | |
| /// little to continue and the whole transaction reverts. Capping it | |
| /// means a failed recording costs this much and nothing more, which | |
| /// keeps recording genuinely best-effort. Generous enough for any | |
| /// realistic calldata. | |
| uint256 private constant RECORD_GAS_STIPEND = 1_000_000; | |
| bytes32 private constant STATE_SLOT = | |
| 0xdd9627cd601a6555f69239ac336fba8db95c419564b84ebb3ee2c21fed88ae00; | |
| /// `@dev` Gas handed to the recording self-call. | |
| /// | |
| /// An explicit cap matters more than the value. try/catch does not | |
| /// recover from out-of-gas: an uncapped sub-call takes 63/64 of what | |
| /// is left, so if recording runs out, the caller is left with too | |
| /// little to continue and the whole transaction reverts. Capping it | |
| /// means a failed recording costs this much and nothing more, which | |
| /// keeps recording genuinely best-effort. Generous enough for any | |
| /// realistic calldata. | |
| uint256 private constant RECORD_GAS_STIPEND = 1_000_000; | |
| /// `@dev` keccak256("tbtc.test.MockContract.state.v1") - 1, masked, per the | |
| /// ERC-7201 convention. | |
| bytes32 private constant STATE_SLOT = | |
| 0xdd9627cd601a6555f69239ac336fba8db95c419564b84ebb3ee2c21fed88ae00; |
🤖 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/contracts/test/MockContract.sol` around lines 110 - 124, Move
the ERC-7201 derivation documentation from above RECORD_GAS_STIPEND to directly
above STATE_SLOT, keeping the gas-stipend explanation attached only to
RECORD_GAS_STIPEND and the slot derivation comment attached to STATE_SLOT.
Comment on lines
+349
to
+351
| /// @notice Raw calldata of the i-th call recorded for one selector. | ||
| /// @notice msg.value of the i-th call recorded for one selector. | ||
| function __mock__callValueForSelectorAt(bytes4 selector, uint256 index) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Duplicated @notice tag in both copies of MockContract.sol. The first @notice describes __mock__callForSelectorAt but is attached to __mock__callValueForSelectorAt, leaving the former undocumented; some solc versions reject two @notice tags on one item outright.
solidity/ecdsa/contracts/test/MockContract.sol#L349-L351: move///@noticeRaw calldata of the i-th call recorded for one selector.down to Line 371, above__mock__callForSelectorAt.solidity/random-beacon/contracts/test/MockContract.sol#L349-L351: apply the identical move.
📍 Affects 2 files
solidity/ecdsa/contracts/test/MockContract.sol#L349-L351(this comment)solidity/random-beacon/contracts/test/MockContract.sol#L349-L351
🤖 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/contracts/test/MockContract.sol` around lines 349 - 351, Move
the raw-calldata `@notice` documentation from above __mock__callValueForSelectorAt
to directly above __mock__callForSelectorAt, leaving only the msg.value notice
attached to the value accessor. Apply this identical documentation correction in
solidity/ecdsa/contracts/test/MockContract.sol lines 349-351 and
solidity/random-beacon/contracts/test/MockContract.sol lines 349-351.
Comment on lines
+168
to
+171
| expect(await target.doThing.callCount()).to.equal(0) | ||
| // The configured `true` is gone, so the call answers with empty data. | ||
| await consumer.doThing(ethers.constants.AddressZero, 1) | ||
| expect(await consumer.lastResult()).to.equal(false) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Comment misstates why the call answers false.
reset() clears only the selector-wide response; the base zero-return layer installed at construction is deliberately untouched (see MockContract.sol Lines 66-67). So the call answers the ABI-encoded zero, not empty data — which is precisely what the assertion relies on.
📝 Proposed fix
- // The configured `true` is gone, so the call answers with empty data.
+ // The configured `true` is gone, so the call falls through to the base
+ // layer, which answers the encoded zero of the return type.
await consumer.doThing(ethers.constants.AddressZero, 1)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| expect(await target.doThing.callCount()).to.equal(0) | |
| // The configured `true` is gone, so the call answers with empty data. | |
| await consumer.doThing(ethers.constants.AddressZero, 1) | |
| expect(await consumer.lastResult()).to.equal(false) | |
| expect(await target.doThing.callCount()).to.equal(0) | |
| // The configured `true` is gone, so the call falls through to the base | |
| // layer, which answers the encoded zero of the return type. | |
| await consumer.doThing(ethers.constants.AddressZero, 1) | |
| expect(await consumer.lastResult()).to.equal(false) |
🤖 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/helpers/mock.test.ts` around lines 168 - 171, Update the
comment in the reset behavior test near consumer.doThing to state that reset
clears only the selector-specific response while preserving the
construction-time base zero-return layer, so the call returns the ABI-encoded
zero value and lastResult() is false. Leave the test behavior and assertions
unchanged.
Comment on lines
+612
to
+613
| /** `expect(fake.fn).to.have.been.calledOnceWith(...args)` */ | ||
| /** |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Stray JSDoc line lands on normalizeForComparison in both copies of mock.ts. The expectCalledOnceWith summary is immediately followed by normalizeForComparison's own doc block, so it documents the wrong function and expectCalledOnceWith at Line 643 has no doc.
solidity/ecdsa/test/helpers/mock.ts#L612-L613: move the/** expect(fake.fn).to.have.been.calledOnceWith(...args) */line down to just aboveexport async function expectCalledOnceWith.solidity/random-beacon/test/helpers/mock.ts#L612-L613: apply the identical move.
📍 Affects 2 files
solidity/ecdsa/test/helpers/mock.ts#L612-L613(this comment)solidity/random-beacon/test/helpers/mock.ts#L612-L613
🤖 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/helpers/mock.ts` around lines 612 - 613, The JSDoc
summary for expectCalledOnceWith is attached to normalizeForComparison in both
copies of mock.ts. Move that JSDoc line from immediately before
normalizeForComparison to immediately before export async function
expectCalledOnceWith in solidity/ecdsa/test/helpers/mock.ts and
solidity/random-beacon/test/helpers/mock.ts.
Comment on lines
+110
to
+124
| /// @dev keccak256("tbtc.test.MockContract.state.v1") - 1, masked, per the | ||
| /// ERC-7201 convention. | ||
| /// @dev Gas handed to the recording self-call. | ||
| /// | ||
| /// An explicit cap matters more than the value. try/catch does not | ||
| /// recover from out-of-gas: an uncapped sub-call takes 63/64 of what | ||
| /// is left, so if recording runs out, the caller is left with too | ||
| /// little to continue and the whole transaction reverts. Capping it | ||
| /// means a failed recording costs this much and nothing more, which | ||
| /// keeps recording genuinely best-effort. Generous enough for any | ||
| /// realistic calldata. | ||
| uint256 private constant RECORD_GAS_STIPEND = 1_000_000; | ||
|
|
||
| bytes32 private constant STATE_SLOT = | ||
| 0xdd9627cd601a6555f69239ac336fba8db95c419564b84ebb3ee2c21fed88ae00; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
The ERC-7201 doc comment is detached from STATE_SLOT.
Same as the ECDSA copy: Lines 110-111 document the state slot but sit above the RECORD_GAS_STIPEND doc block, leaving STATE_SLOT at Line 123 undocumented.
🤖 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/contracts/test/MockContract.sol` around lines 110 -
124, Move the ERC-7201 keccak256/state-slot documentation so it directly
precedes the STATE_SLOT declaration, keeping the RECORD_GAS_STIPEND
documentation attached only to RECORD_GAS_STIPEND.
piotr-roslaniec
approved these changes
Jul 27, 2026
piotr-roslaniec
left a comment
piotr-roslaniec
left a comment
Collaborator
There was a problem hiding this comment.
Self-approve as repo admin. Dependency-only bump (hardhat 2.10.0 -> 2.19.5, smock 2.0.7 -> 2.3.4, hardhat-gas-reporter 1.0.8 -> 2.3.0). No test/source changes. Author's body documents why this exact combination is required to keep allowBlocksWithSameTimestamp while smock still works. CI is green.
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.
Dependencies only. No test file is touched. First step in removing
@defi-wonderland/smock, which upstream has archived with this notice:npm carries the matching deprecation, the repo is archived, and the last push
was 2025-05-20. That is the reason to remove it. The toolchain ceiling below is
not the motivation — it is what makes removal unavoidable rather than
schedulable, since no smock version reaches the hardhat that Node 24 needs.
Independent of #4201 — different regions of the same
package.jsonfiles, andthe two merge cleanly in either order (verified).
Three coupled bumps, in both packages
None of them works alone, which is why they land together.
hardhat first. The contract-backed mock that will replace smock configures
itself with transactions, and a transaction mines a block. Keeping that from
moving the chain clock needs
allowBlocksWithSameTimestamp— and that optiondoes not exist in 2.10.0. Checked against the published tarballs: absent in
2.10.0, present by 2.14.0. 2.19.5 is the last version smock still works on, so
it is the one version with both properties.
smock second, because 2.0.7 does not survive that bump. On hardhat 2.19.5 it
dies before any test runs:
2.3.4 is the version that works there, and it is pinned exactly rather than
left as a range.
^2.0.7resolves to 2.4.1 today, so the lockfile was oneregeneration away from silently jumping four minor versions into an archived
package.
2.4.1 is deliberately not the target. It advertises
hardhat >=2.21.0supportand still fails — differently — on a real plugin stack:
That is what makes the smock removal unavoidable rather than optional: there is
no smock version that reaches hardhat 2.26+, and hardhat 2.26+ is what Node 24
requires.
hardhat-gas-reporter third. 1.x bundles
eth-gas-reporter, which the mochainside hardhat 2.19.5 refuses to load on Node 22 —
ERR_MOCHA_INVALID_REPORTER, raised before a single test executes. Onlyrandom-beaconhits it, because onlyrandom-beaconenables the reporter, butboth packages move together to keep them aligned. This matters now rather than
later because #4201 puts CI on Node 22.
Verified
Whole suite, each package, on both runtimes:
random-beaconecdsaIdentical on Node 18.20.8 and Node 22.23.1.
Where this is going
MockContract.sol+test/helpers/mock.ts), portable from test(solidity): toolchain-independent replacement for smock (foundation) tbtc-v2#1062smock is gone
Roughly 26
smock.fakesites across 20 files, most of them inecdsa. The onesmock.mockcall, atWalletRegistry.RandomBeacon.test.ts:236, only reads.addressand never touchessetVariable, so it is a plain factory deploy andneeds no mock at all.
Summary by CodeRabbit
Tests
Chores