test: assert [rpc_endpoints] and [etherscan] are exactly supportedNetworks() - #112
test: assert [rpc_endpoints] and [etherscan] are exactly supportedNetworks()#112thedavidmeister wants to merge 1 commit into
Conversation
…works()
The supported-network list was spelled three times — supportedNetworks(),
[rpc_endpoints] and [etherscan] — with nothing joining them. Only the RPC
pairing was enforced, and only incidentally, by the fork tests failing on a
name with no alias. Nothing anywhere read [etherscan], so a network added
without an explorer key broadcast, spent the gas, and only then failed
--verify with no API key configured for the chain.
testSupportedNetworksAreFullyConfigured reads foundry.toml and asserts both
sections against supportedNetworks() by membership in both directions, so the
three lists can only be one list: a missing entry is the broadcast-then-fail,
and a stray entry is config nothing reads. Reading the raw file rather than
the resolved config keeps the ${VAR} values out of it, so the test needs no
RPC and fails on the PR that drifts rather than at dispatch time.
Closes #47
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 20 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
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 |
1 participant
Closes #47
What was wrong
"Which networks this org deploys to" was spelled three times with nothing joining them:
LibRainDeploy.supportedNetworks()(src/lib/LibRainDeploy.sol:237-245)[rpc_endpoints](foundry.toml:60-65)[etherscan](foundry.toml:71-76)Only one pairing was enforced, and only incidentally:
testZoltuFactoryCodehashforks every namesupportedNetworks()returns, so a name with no[rpc_endpoints]alias fails there. Nothing in the repo read[etherscan]at all — grep oversrc/,script/andtest/forreadFile|parseToml|keyExistsfinds only.package.version, artifact JSON reads and snapshot record reads.The consequence is the one
foundry.toml:67-70already documents and then leaves to memory:rainix-manual-sol-artifactspasses--verify, so a network insupportedNetworks()with no[etherscan]entry broadcasts, spends the gas, and then fails with no API key configured for the chain.The fix
testSupportedNetworksAreFullyConfiguredintest/src/lib/LibRainDeploy.t.solreadsfoundry.tomland asserts both sections are EXACTLYsupportedNetworks().Asserted in both directions, by membership:
[rpc_endpoints]alias and an[etherscan]key — a missing one is the broadcast-then-fail above;[rpc_endpoints]alias and every[etherscan]key is a supported network — a stray one is config nothing ever reads.One direction alone is not enough: containment forwards passes for a section carrying an alias nothing deploys to, and containment backwards passes for a network with no config at all. Membership both ways makes the sets equal, so the three lists can only be one list.
Membership rather than the key counts the issue proposed. It is strictly stronger — equal counts plus forward containment is only set equality if
supportedNetworks()has no duplicates, which this test would then be leaning ontestSupportedNetworksto guarantee — and the failure names the offending alias instead of only reporting that two numbers differ.The raw file is read rather than forge's resolved config, because the values are
${VAR}interpolations that only exist in CI. The keys are the whole contract here and they are in the text — so the test needs no RPC and fails on the PR that drifts, rather than at dispatch time with a hot key.fs_permissionsalready granted{ access = "read", path = "./foundry.toml" }andLibRainDeploySnapshot.deployTagalready reads that file the same way, so no config change was needed.The only non-test change is a comment on
[etherscan]naming the test that now enforces it, so the next person editing that section learns it is checked rather than rediscovering the hazard.Mutation evidence
Five mutants, one per drift direction. Every run was checked for
1 total testsas well as for[FAIL, so a mutant that failed to compile could not be miscounted as killed.supportedNetworks(), no config at allsupported network has no [rpc_endpoints] alias: optimism[rpc_endpoints]alias,[etherscan]missed — the hazard in the issuesupported network has no [etherscan] key: optimism[rpc_endpoints]alias[rpc_endpoints] alias is not a supported network: optimism[etherscan]key[etherscan] key is not a supported network: optimism[etherscan]entry deletedsupported network has no [etherscan] key: polygonM2 is the scenario both findings describe end to end, and it is the one nothing in the repo caught before this test.
Scope
Test-only plus one comment. No behaviour change:
supportedNetworks(),[rpc_endpoints]and[etherscan]are all untouched and already agree.QA
testSupportedNetworksAreFullyConfigured— fails on base under each of the five drifts below (verified by applying each mutation to a base tree via.fixes/mutate.shand runningforge test --match-test; each run asserted1 total testsactually RAN as well as[FAIL, so a non-compiling mutant cannot be miscounted as killed). It is a new test: before it nothing in the repo read[etherscan], so the base suite is green on every mutant below.LibRainDeploy.supportedNetworks()new string[](5)→new string[](6)+networks[5] = "optimism"→ killed bytestSupportedNetworksAreFullyConfigured(supported network has no [rpc_endpoints] alias: optimism); that same mutation plusoptimismadded to[rpc_endpoints]— the exact hazard the issue describes — → killed bytestSupportedNetworksAreFullyConfigured(supported network has no [etherscan] key: optimism);foundry.toml[rpc_endpoints]+=optimismwith the library untouched → killed ([rpc_endpoints] alias is not a supported network: optimism);foundry.toml[etherscan]+=optimismwith the library untouched → killed ([etherscan] key is not a supported network: optimism);foundry.toml[etherscan]−=polygon→ killed (supported network has no [etherscan] key: polygon). 5 mutants, 5 killed.LibRainDeploy.supportedNetworks(), which is what the deploy actually iterates (RainDeployBroadcast.deployNetworks()defaults to it). The expected key sets are never restated in the test — they are read fromsupportedNetworks()at runtime and compared against keys parsed out of thefoundry.tomltext, so the test cannot drift from the list it is about. The consequence being guarded is stated independently byfoundry.toml:67-70and byrainix-manual-sol-artifactspassing--verify.[rpc_endpoints]and[etherscan]asserted againstsupportedNetworks()in both directions; covered —[rpc_endpoints]forward and reverse,[etherscan]forward and reverse. Implemented by membership rather than the proposed key counts: strictly stronger (count equality is only set equality given no duplicates insupportedNetworks(), which the count form would be leaning ontestSupportedNetworksto supply) and it names the offending alias on failure instead of only reporting that two numbers differ. Nofs_permissionschange was needed, as both findings predicted.