Create every fork before selecting any - #158
Conversation
Foundry captures the pre-fork EVM's account set when the FIRST fork is selected, and seeds every fork created after that capture with it, so `createSelectFork` inside a network loop put networks 2..n on the wrong side of the capture. A `dep.code.length` read in a deploy script, added for logging, was enough to make every network after the first revert `MissingDependency` against a dependency that has code there. Closes #157 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. WalkthroughThe change creates all network forks before selecting any fork. Deployment and verification loops then select the pre-created forks. Tests cover fork isolation, pre-fork reads, and failures during the first network check. ChangesUpfront fork creation
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to The PR changes fork setup so all networks are created before selection and validates the behavior across all affected call sites; no actionable merge-blocking risk remains beyond normal checks and review. Sequence Diagram(s)sequenceDiagram
participant deployToNetworks
participant LibRainDeploy
participant Vm
deployToNetworks->>LibRainDeploy: createForks(vm, networks)
LibRainDeploy->>Vm: createFork(network)
Vm-->>LibRainDeploy: forkId
LibRainDeploy-->>deployToNetworks: forkIds
deployToNetworks->>Vm: selectFork(forkIds[i])
deployToNetworks->>deployToNetworks: validate dependencies and deploy
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Full details: Linked Issues checkExplanation The changes address Full details: Docstring CoverageExplanation No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 0 files. (5 skipped: 5 unsupported.) ✨ Finishing Touches🧪 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 |
Closes #157
The defect
LibRainDeployandRainDeployVerifyChainwalked their network lists withvm.createSelectForkinside the loop. Foundry captures the account set of thepre-fork EVM when the first fork is selected, and seeds every fork created
after that capture with it. With
createSelectForkin the loop, networks2..n are all created on the wrong side of that capture, so they inherit whatever
the default 31337 EVM held for any address the calling script had already
touched — an empty account.
A
dep.code.lengthread in a deploy script, typically added for logging, isenough. The first network read its chain; every network after it reverted
MissingDependencyagainst a dependency that demonstrably has code there.Forks created before the capture read their own chain, which is exactly why
the first network was always right.
The fix
Create every fork up front, then select each in turn.
LibRainDeploy.createForkscreates and selects nothing; three call sites use it:
LibRainDeploy.deployToNetworksLibRainDeploy.checkResolvedAddressesOnNetworksRainDeployVerifyChain.checkDeployedOnSupportedNetworksThis puts every network on the same side of the capture as the first one already
was.
Closed in the library rather than by a rule about what a deploy script may read,
because the trap is invisible from where a consumer sits: the read is ordinary,
the failure names a real address on a network that really has it, and nothing
connects the two.
Alternatives rejected
vm.makePersistenton the dependencies pins the empty account acrossevery fork. It makes the bug unconditional rather than fixing it.
rollForkdoes not re-seed the account set at all.vm.rpcsidesteps forking but changes the dependency check's semantics.The check is meant to read the same fork the deploy then broadcasts on, so
that a transient RPC inconsistency between two separate reads cannot report a
live dependency as missing and abort an otherwise-valid deploy.
Two limits, stated plainly
1. It cannot rescue a caller that has already selected a fork before calling.
The capture happens at the first select. A caller that selects a fork first has
already triggered it, so
createForksruns afterwards and its forks inherit thepoisoned set. There is no in-library fix for that ordering.
This does not affect the reported path.
RainDeployBroadcast.run()forks nothingbefore calling
deployAndBroadcast:checkCandidatesAnchoredToSource()andsuiteByName()are bothinternal pure, andvm.envUintdoes not fork.2. Endpoint reachability is now all-network. Every alias must be forkable
before any network is touched, so one unreachable endpoint fails the run at fork
creation rather than after the earlier networks have already been checked and
their deploys simulated. It is all-or-nothing on reachability instead of
stopping partway down the list.
To be precise about what that does not change: it does not change what
reaches a chain. Verified locally against two anvils, with a reachable first
endpoint and an unreachable second one:
createSelectForkin the loop (before)0x0createForkup front (after)0x0forge scriptsends only after the whole script has executed, so an abortedscript broadcast nothing under either shape. What changes is how far the run
gets, and therefore what an operator sees before the endpoint error — not what
lands on chain.
A consequence worth naming: paths that used to short-circuit now contact every
endpoint. The chain matrix that reverts on the first network previously never
forked the remaining six; it now forks all seven before checking any. That is
inherent to the fix, and it raises this suite's exposure to a flaky endpoint
(see QA).
QA
testDeployToNetworksIgnoresAPreForkDependencyRead,testCheckResolvedAddressesOnNetworksCreatesEveryForkFirst,testChainMatrixCreatesEveryForkFirst,testCreateForksSelectsNothing,testCreateForksIgnoresAPreForkRead- each fails on base, verified by reverting its own call site tocreateSelectForkand running the whole suite (table below), not by reasoning about it.LibRainDeploy.deployToNetworks->createForks+selectForkreverted tocreateSelectForkin the loop -> killed bytestDeployToNetworksIgnoresAPreForkDependencyRead;LibRainDeploy.checkResolvedAddressesOnNetworks-> same revert -> killed bytestCheckResolvedAddressesOnNetworksCreatesEveryForkFirst;RainDeployVerifyChain.checkDeployedOnSupportedNetworks-> same revert -> killed bytestChainMatrixCreatesEveryForkFirst. Each site reverted individually with the other two left fixed; restores verified byte-exact against pristine copies.0x7A0D94F55792C434d74a40883C6ed8545E406D12does have code on arbitrum, so the mutant'sMissingDependency("arbitrum", 0x7A0D...)is a demonstrable false negative rather than an expected result. Fork block numbers come from the endpoints themselves.RainDeployVerifyBase.deriveDeploymentdoes readfactoryAddress.codehashpre-fork, but its ownsnapshotState/revertToStateclears the capture, verified rather than assumed; every other.code/.codehashread is behindvm.makePersistentor already inside a fork.Full suite.
forge test— 350 passed, 0 failed, 0 skipped.Mutation testing. Each of the three changed call sites was individually
reverted to
createSelectForkin the loop, with the other two left fixed, andthe whole suite run. Every site has a test that dies without it:
RainDeployVerifyChain.solcheckDeployedOnSupportedNetworkstestChainMatrixCreatesEveryForkFirstvm.selectFork: No matching fork found for 6LibRainDeploy.solcheckResolvedAddressesOnNetworkstestCheckResolvedAddressesOnNetworksCreatesEveryForkFirstvm.selectFork: No matching fork found for 1LibRainDeploy.soldeployToNetworkstestDeployToNetworksIgnoresAPreForkDependencyReadMissingDependency("arbitrum", 0x7A0D94F55792C434d74a40883C6ed8545E406D12)The last is the exact failure #157 reports.
Pre-existing HyperEVM flakiness, not from this diff. Across repeated full-suite
runs,
rpc.hyperliquid.xyzintermittently answerserror code -32603: invalid block height: <head>, failing whichever test forksit first that run. It has been seen on
testZoltuFactoryCodehash,testCreditOnAHyperEvmFork,testCreditOnHyperEvmForksTheAliasBeforeItsGuards,testTheLiveSystemContractIsWhatIsPinnedandtestChainMatrixReachesTheLastSupportedNetwork— untouched tests, forkingHyperEVM through
createSelectForkas they always did. It is the endpointserving a node behind the head, and it is precisely the failure class CLAUDE.md
calls out. Re-running clears it. Noting it because CI may need a re-run, and
because the change does raise the number of HyperEVM forks per run.
Docs
CLAUDE.md's "a fork failure is not a missing deployment" note namedvm.createSelectForkonly. The paths it is about now fail undervm.createFork,so the note names both and records that the failure is now up front.
Summary by CodeRabbit
Bug Fixes
Tests