From a741d90b5cb70fb38e405772a5bfa95e405a6b74 Mon Sep 17 00:00:00 2001 From: baku-ccron Date: Tue, 15 Sep 2026 16:02:28 +0000 Subject: [PATCH 1/5] Assert [etherscan] entries can resolve, not just that the keys exist The config group asserted that a [etherscan] key exists per supported network and nothing about the entry. An entry carrying only a key, under an alias foundry does not itself resolve to a chain, satisfied every assertion and is config that verifies nothing: foundry resolves the section rather than the one entry the network needs, so it raises "At least one of `url` or `chain` must be present for Etherscan config with unknown alias" for whichever network --verify was pointed at. Every entry now states `chain`, and the check requires at least one of `chain` or `url` on each. Strict on every alias rather than only the ones foundry cannot resolve, so it needs no knowledge of foundry's alias table. The group's assertions move to RainDeployVerifySnapshotBase as checkNetworksConfigured, which takes the config text, so a test can hand it one; testSupportedNetworksAreFullyConfigured keeps the file read that binds them. Without that, deleting the new assertion's call site was a mutation nothing killed. Closes #192 Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01V8ViHcKLVk2YoS2joH4HdN --- README.md | 19 ++-- foundry.toml | 28 +++--- src/abstract/RainDeployVerifySnapshot.sol | 57 ++++-------- src/abstract/RainDeployVerifySnapshotBase.sol | 92 +++++++++++++++++++ .../RainDeployVerifySnapshotBase.t.sol | 71 ++++++++++++++ 5 files changed, 209 insertions(+), 58 deletions(-) diff --git a/README.md b/README.md index 25209aa..6dd8787 100644 --- a/README.md +++ b/README.md @@ -188,13 +188,17 @@ supported network missing from a section broadcasts and then fails after the gas is spent, and a section entry no supported network names is config nothing ever reads. An `[etherscan]` entry carrying neither `chain` nor `url` under an alias foundry cannot resolve is worse than missing — it takes verification down for -every entry in the section, not only its own. +every entry in the section, not only its own — so membership is not the whole of +that half: every entry has to carry at least one of `chain` or `url` as well. +That is asked of every entry rather than only of the aliases foundry cannot +resolve, because which aliases those are is foundry's own table, and stating the +chain an alias already resolves to resolves it to the same chain. It reads the raw file rather than forge's resolved config, because the values -are `${VAR}` interpolations that only exist in CI while the KEYS are the whole -contract, and the keys are in the text. So it needs no RPC and fails on the pull -request that drifts rather than at dispatch time. Reading the file at all is -what a consumer has to allow: see [Install](#install). +are `${VAR}` interpolations that only exist in CI, and nothing it asserts is a +value — the keys and the entry shapes are both in the text. So it needs no RPC +and fails on the pull request that drifts rather than at dispatch time. Reading +the file at all is what a consumer has to allow: see [Install](#install). ## Address registry @@ -640,8 +644,9 @@ fs_permissions = [{ access = "read", path = "./foundry.toml" }] ``` `[rpc_endpoints]` and `[etherscan]` then have to name exactly the networks in -`supportedNetworks()`. Missing permission fails the check rather than skipping -it, which is the intended direction: a repo that cannot read its own config is a +`supportedNetworks()`, and every `[etherscan]` entry has to carry at least one +of `chain` or `url`. Missing permission fails the check rather than skipping it, +which is the intended direction: a repo that cannot read its own config is a repo whose config nothing has checked. ## Develop diff --git a/foundry.toml b/foundry.toml index 79bfbde..b4b453d 100644 --- a/foundry.toml +++ b/foundry.toml @@ -94,21 +94,27 @@ robinhood = "${ROBINHOOD_RPC_URL}" # directions, by `testSupportedNetworksAreFullyConfigured`. Adding a network is # an edit to all three or a red test, not a broadcast that discovers it. # -# `chain` is stated on the entries whose alias foundry does not itself resolve -# to a chain. An entry with neither `chain` nor `url` under such an alias is not -# a missing key, it is a config error — "At least one of `url` or `chain` must -# be present for Etherscan config with unknown alias" — raised while resolving -# the section, so it takes down verification for every network in it and not -# only its own. +# `chain` is stated on EVERY entry, and `testSupportedNetworksAreFullyConfigured` +# requires at least one of `chain` or `url` on each. An entry with neither, under +# an alias foundry does not itself resolve to a chain, is not a missing key, it +# is a config error — "At least one of `url` or `chain` must be present for +# Etherscan config with unknown alias" — raised while resolving the section, so +# it takes down verification for every network in it and not only its own. +# +# Stated on every entry rather than only on the aliases foundry cannot resolve, +# because which aliases those are is foundry's own table and that table moves +# under a toolchain bump. Stating the chain an alias already resolves to +# resolves it to the same chain, so neither these entries nor the check reading +# them has to know the table. [etherscan] -arbitrum = { key = "${CI_DEPLOY_ARBITRUM_ETHERSCAN_API_KEY}" } -base = { key = "${CI_DEPLOY_BASE_ETHERSCAN_API_KEY}" } -base_sepolia = { key = "${CI_DEPLOY_BASE_SEPOLIA_ETHERSCAN_API_KEY}" } +arbitrum = { key = "${CI_DEPLOY_ARBITRUM_ETHERSCAN_API_KEY}", chain = 42161 } +base = { key = "${CI_DEPLOY_BASE_ETHERSCAN_API_KEY}", chain = 8453 } +base_sepolia = { key = "${CI_DEPLOY_BASE_SEPOLIA_ETHERSCAN_API_KEY}", chain = 84532 } bsc = { key = "${CI_DEPLOY_BSC_ETHERSCAN_API_KEY}", chain = 56 } ethereum = { key = "${CI_DEPLOY_ETHEREUM_ETHERSCAN_API_KEY}", chain = 1 } -flare = { key = "${CI_DEPLOY_FLARE_ETHERSCAN_API_KEY}" } +flare = { key = "${CI_DEPLOY_FLARE_ETHERSCAN_API_KEY}", chain = 14 } hyperevm = { key = "${CI_DEPLOY_HYPEREVM_ETHERSCAN_API_KEY}", chain = 999 } -polygon = { key = "${CI_DEPLOY_POLYGON_ETHERSCAN_API_KEY}" } +polygon = { key = "${CI_DEPLOY_POLYGON_ETHERSCAN_API_KEY}", chain = 137 } # Robinhood Chain (4663) is not indexed by Etherscan V2. Its Blockscout # explorer speaks the Etherscan API, so the entry points there; the key is # whatever `CI_DEPLOY_ROBINHOOD_ETHERSCAN_API_KEY` carries (Blockscout ignores diff --git a/src/abstract/RainDeployVerifySnapshot.sol b/src/abstract/RainDeployVerifySnapshot.sol index b7575f0..b4edaa9 100644 --- a/src/abstract/RainDeployVerifySnapshot.sol +++ b/src/abstract/RainDeployVerifySnapshot.sol @@ -5,7 +5,6 @@ pragma solidity ^0.8.25; import {RainDeployVerifySnapshotBase} from "./RainDeployVerifySnapshotBase.sol"; import {LibRainDeploy} from "../lib/LibRainDeploy.sol"; import {LibRainDeploySnapshot} from "../lib/LibRainDeploySnapshot.sol"; -import {LibMemoryKV, MemoryKV, MemoryKVKey, MemoryKVVal} from "rain-lib-memkv-0.1.4/src/lib/LibMemoryKV.sol"; /// @title RainDeployVerifySnapshot /// @notice What a deploy repo inherits: every assertion that needs no network, @@ -20,8 +19,6 @@ import {LibMemoryKV, MemoryKV, MemoryKVKey, MemoryKVVal} from "rain-lib-memkv-0. /// FIXTURE — the record is not its subject, and see the base for why asking it /// about the record asserts something false. abstract contract RainDeployVerifySnapshot is RainDeployVerifySnapshotBase { - using LibMemoryKV for MemoryKV; - /// Every release in the frozen record MUST be declared, so that the set the /// chain group checks is every release this repo has ever cut rather than /// the ones somebody remembered to list. @@ -75,50 +72,30 @@ abstract contract RainDeployVerifySnapshot is RainDeployVerifySnapshotBase { /// This is what makes the `[etherscan]` half enforced at all. The RPC half /// is enforced only incidentally, by the fork tests, and only forwards. /// + /// Membership is necessary and not sufficient for that half, so the SHAPE + /// of each `[etherscan]` entry is asserted beside it by + /// `checkEtherscanEntriesResolvable`: foundry resolves the whole section, so + /// an entry it cannot resolve fails `--verify` for the other entries as + /// well, after the gas is spent, while satisfying every membership + /// assertion here. + /// /// The raw file is read rather than forge's resolved config because the - /// values are `${VAR}` interpolations that exist only in CI. The KEYS are - /// the whole contract here, and they are in the text — so this needs no - /// RPC and fails on the PR that drifts rather than at dispatch time. + /// values are `${VAR}` interpolations that exist only in CI. Nothing + /// asserted here is a value — the keys, and that each `[etherscan]` entry + /// carries enough to resolve at all, are both in the text — so this needs + /// no RPC and fails on the PR that drifts rather than at dispatch time. /// /// `vm.readFile` resolves against the project root of whatever runs it, so /// the file read is the binder's own and the networks are this package's. /// A binding repo therefore needs `{ access = "read", path = /// "./foundry.toml" }` in `fs_permissions`, and one without it fails here /// rather than passing on a file it never opened. + /// + /// The assertions themselves are `checkNetworksConfigured`, in the base, + /// because they take the config as an argument and so can be handed one a + /// test builds. Reading the binder's own file is the part that cannot be, + /// and it is all that is left here. function testSupportedNetworksAreFullyConfigured() external view { - string memory config = vm.readFile("foundry.toml"); - string[] memory networks = LibRainDeploy.supportedNetworks(); - - MemoryKV networkSet = MemoryKV.wrap(0); - for (uint256 i = 0; i < networks.length; i++) { - networkSet = networkSet.set(MemoryKVKey.wrap(keccak256(bytes(networks[i]))), MemoryKVVal.wrap(0)); - } - - for (uint256 i = 0; i < networks.length; i++) { - assertTrue( - vm.keyExistsToml(config, string.concat(".rpc_endpoints.", networks[i])), - string.concat("supported network has no [rpc_endpoints] alias: ", networks[i]) - ); - assertTrue( - vm.keyExistsToml(config, string.concat(".etherscan.", networks[i])), - string.concat("supported network has no [etherscan] key: ", networks[i]) - ); - } - - string[] memory rpcAliases = vm.parseTomlKeys(config, ".rpc_endpoints"); - for (uint256 i = 0; i < rpcAliases.length; i++) { - assertTrue( - networkSet.has(MemoryKVKey.wrap(keccak256(bytes(rpcAliases[i])))), - string.concat("[rpc_endpoints] alias is not a supported network: ", rpcAliases[i]) - ); - } - - string[] memory etherscanKeys = vm.parseTomlKeys(config, ".etherscan"); - for (uint256 i = 0; i < etherscanKeys.length; i++) { - assertTrue( - networkSet.has(MemoryKVKey.wrap(keccak256(bytes(etherscanKeys[i])))), - string.concat("[etherscan] key is not a supported network: ", etherscanKeys[i]) - ); - } + checkNetworksConfigured(vm.readFile("foundry.toml"), LibRainDeploy.supportedNetworks()); } } diff --git a/src/abstract/RainDeployVerifySnapshotBase.sol b/src/abstract/RainDeployVerifySnapshotBase.sol index 9b28647..43a7572 100644 --- a/src/abstract/RainDeployVerifySnapshotBase.sol +++ b/src/abstract/RainDeployVerifySnapshotBase.sol @@ -5,6 +5,7 @@ pragma solidity ^0.8.25; import {DerivedDeploy, RainDeployVerifyBase} from "./RainDeployVerifyBase.sol"; import {DeploySuite} from "./RainDeploySuitesBase.sol"; import {LibRainDeploy} from "../lib/LibRainDeploy.sol"; +import {LibMemoryKV, MemoryKV, MemoryKVKey, MemoryKVVal} from "rain-lib-memkv-0.1.4/src/lib/LibMemoryKV.sol"; /// Thrown when the deploy address recorded for a version is not the address its /// own creation code derives. @@ -44,6 +45,15 @@ error FrozenSnapshotNotReleased(string path); /// @param path The record file with no `DEPLOYED_ADDRESS` declaration. error FrozenSnapshotUnreadable(string path); +/// Thrown when an `[etherscan]` entry carries neither `chain` nor `url`. Under +/// an alias foundry does not itself resolve to a chain that entry is not a +/// missing key, it is "At least one of `url` or `chain` must be present for +/// Etherscan config with unknown alias" — raised while foundry resolves the +/// SECTION, so it takes verification down for the other entries too and not +/// only its own. +/// @param entry The `[etherscan]` entry that cannot resolve. +error EtherscanEntryUnresolvable(string entry); + /// @title RainDeployVerifySnapshotBase /// @notice Every deploy-pin assertion that needs no network, for every suite /// a repo declares. Three groups, which catch different things and are @@ -113,6 +123,8 @@ error FrozenSnapshotUnreadable(string path); /// one spelling. Inheriting a narrower contract is a choice a reader sees in /// the inheritance list; overriding a test to nothing is one they do not. abstract contract RainDeployVerifySnapshotBase is RainDeployVerifyBase { + using LibMemoryKV for MemoryKV; + /// Checks one suite against itself: derive from its creation code, then /// require everything it records to agree with the derivation. /// @param suite The suite to check. @@ -227,6 +239,86 @@ abstract contract RainDeployVerifySnapshotBase is RainDeployVerifyBase { } } + /// Checks that every `[etherscan]` entry can resolve at all: each carries at + /// least one of `chain` or `url`. + /// + /// The entries EXISTING is not enough for the section to verify anything. + /// Foundry resolves the section rather than the single entry the network + /// being verified needs, so one entry it cannot resolve is an error raised + /// for whichever network `--verify` was pointed at — the failure mode the + /// key checks are there to keep off a broadcast, arriving from an entry + /// that satisfies them. + /// + /// Required of EVERY entry rather than only the aliases foundry cannot + /// resolve itself, because that set is foundry's table and moves under a + /// toolchain bump. Stating the chain an alias already resolves to resolves + /// it to the same chain, so the strict form is monotonic, needs to know + /// nothing of that table, and cannot red-line when foundry adds an alias. + /// @param config The raw `foundry.toml` text. + /// @param entries The `[etherscan]` entries to check. + function checkEtherscanEntriesResolvable(string memory config, string[] memory entries) internal view { + for (uint256 i = 0; i < entries.length; i++) { + if ( + !vm.keyExistsToml(config, string.concat(".etherscan.", entries[i], ".chain")) + && !vm.keyExistsToml(config, string.concat(".etherscan.", entries[i], ".url")) + ) { + revert EtherscanEntryUnresolvable(entries[i]); + } + } + } + + /// Checks a `foundry.toml`'s `[rpc_endpoints]` and `[etherscan]` sections + /// against a set of supported networks: the three lists are one list, and + /// every `[etherscan]` entry can resolve. + /// + /// Membership is asserted in BOTH directions. Containment one way alone + /// passes for a section carrying an alias nothing deploys to, and the other + /// way alone passes for a network with no config at all. Membership rather + /// than position, because a config section is keyed rather than ordered and + /// there is no order in it to assert. + /// + /// Takes the config text rather than reading it, so it can be handed one a + /// test builds. What reads the binder's own file is + /// `testSupportedNetworksAreFullyConfigured`, and see it for why the file's + /// text is the subject at all. + /// @param config The raw `foundry.toml` text. + /// @param networks The supported networks the sections must name. + function checkNetworksConfigured(string memory config, string[] memory networks) internal view { + MemoryKV networkSet = MemoryKV.wrap(0); + for (uint256 i = 0; i < networks.length; i++) { + networkSet = networkSet.set(MemoryKVKey.wrap(keccak256(bytes(networks[i]))), MemoryKVVal.wrap(0)); + } + + for (uint256 i = 0; i < networks.length; i++) { + assertTrue( + vm.keyExistsToml(config, string.concat(".rpc_endpoints.", networks[i])), + string.concat("supported network has no [rpc_endpoints] alias: ", networks[i]) + ); + assertTrue( + vm.keyExistsToml(config, string.concat(".etherscan.", networks[i])), + string.concat("supported network has no [etherscan] key: ", networks[i]) + ); + } + + string[] memory rpcAliases = vm.parseTomlKeys(config, ".rpc_endpoints"); + for (uint256 i = 0; i < rpcAliases.length; i++) { + assertTrue( + networkSet.has(MemoryKVKey.wrap(keccak256(bytes(rpcAliases[i])))), + string.concat("[rpc_endpoints] alias is not a supported network: ", rpcAliases[i]) + ); + } + + string[] memory etherscanKeys = vm.parseTomlKeys(config, ".etherscan"); + for (uint256 i = 0; i < etherscanKeys.length; i++) { + assertTrue( + networkSet.has(MemoryKVKey.wrap(keccak256(bytes(etherscanKeys[i])))), + string.concat("[etherscan] key is not a supported network: ", etherscanKeys[i]) + ); + } + + checkEtherscanEntriesResolvable(config, etherscanKeys); + } + /// Every declared suite MUST be internally consistent: what it records is /// what its own creation code derives. function testSnapshotInternallyConsistent() external { diff --git a/test/src/abstract/RainDeployVerifySnapshotBase.t.sol b/test/src/abstract/RainDeployVerifySnapshotBase.t.sol index d41b1af..dc80216 100644 --- a/test/src/abstract/RainDeployVerifySnapshotBase.t.sol +++ b/test/src/abstract/RainDeployVerifySnapshotBase.t.sol @@ -5,6 +5,7 @@ pragma solidity =0.8.25; import {ZoltuDerivationMismatch} from "../../../src/abstract/RainDeployVerifyBase.sol"; import {CandidateSourceMismatch, DeployCandidate, DeploySuite} from "../../../src/abstract/RainDeploySuitesBase.sol"; import { + EtherscanEntryUnresolvable, FrozenSnapshotNotReleased, FrozenSnapshotUnreadable, RainDeployVerifySnapshotBase, @@ -86,6 +87,14 @@ contract RainDeployVerifySnapshotBaseTest is ExampleDeploySuites, RainDeployVeri checkFrozenSnapshotsReleased(paths, released); } + /// External wrapper for `checkNetworksConfigured` so `vm.expectRevert` works + /// at the correct call depth. + /// @param config The raw `foundry.toml` text. + /// @param networks The supported networks the sections must name. + function externalCheckNetworksConfigured(string memory config, string[] memory networks) external view { + checkNetworksConfigured(config, networks); + } + /// External wrapper for `recordedDeployedAddress` so `vm.expectRevert` /// works at the correct call depth. /// @param path The record file, for the error only. @@ -624,4 +633,66 @@ contract RainDeployVerifySnapshotBaseTest is ExampleDeploySuites, RainDeployVeri assertEq(suites[i].storedDeployedAddress.code.length, 0); } } + + /// The networks the config fixtures below are written against. Named for + /// nothing real, so no fixture here reads as a claim about the networks + /// this repo actually deploys to. + /// @return networks The three fixture networks. + function fixtureNetworks() internal pure returns (string[] memory) { + string[] memory networks = new string[](3); + networks[0] = "alpha"; + networks[1] = "beta"; + networks[2] = "gamma"; + return networks; + } + + /// A config whose sections name exactly the fixture networks and whose + /// `[etherscan]` entries can all resolve — `chain` alone on two of them and + /// `url` alone on the third, because each is sufficient by itself. + string constant CONFIG_RESOLVABLE = "[rpc_endpoints]\n" "alpha = \"${ALPHA_RPC_URL}\"\n" + "beta = \"${BETA_RPC_URL}\"\n" "gamma = \"${GAMMA_RPC_URL}\"\n" "\n" "[etherscan]\n" + "alpha = { key = \"${A}\", chain = 999 }\n" "beta = { key = \"${B}\", url = \"https://example.com/api\" }\n" + "gamma = { key = \"${C}\", chain = 1000 }\n"; + + /// `CONFIG_RESOLVABLE` with the LAST `[etherscan]` entry stripped back to + /// its key, so a check that stops before the end of the section lets it + /// through. + string constant CONFIG_LAST_ENTRY_UNRESOLVABLE = "[rpc_endpoints]\n" "alpha = \"${ALPHA_RPC_URL}\"\n" + "beta = \"${BETA_RPC_URL}\"\n" "gamma = \"${GAMMA_RPC_URL}\"\n" "\n" "[etherscan]\n" + "alpha = { key = \"${A}\", chain = 999 }\n" "beta = { key = \"${B}\", url = \"https://example.com/api\" }\n" + "gamma = { key = \"${C}\" }\n"; + + /// `CONFIG_RESOLVABLE` with the MIDDLE `[etherscan]` entry stripped back to + /// its key. That entry is the one `url` alone was carrying, so this is also + /// what a `url` deleted from an otherwise untouched entry leaves behind. + string constant CONFIG_MIDDLE_ENTRY_UNRESOLVABLE = "[rpc_endpoints]\n" "alpha = \"${ALPHA_RPC_URL}\"\n" + "beta = \"${BETA_RPC_URL}\"\n" "gamma = \"${GAMMA_RPC_URL}\"\n" "\n" "[etherscan]\n" + "alpha = { key = \"${A}\", chain = 999 }\n" "beta = { key = \"${B}\" }\n" + "gamma = { key = \"${C}\", chain = 1000 }\n"; + + /// A config whose sections name the networks and whose `[etherscan]` + /// entries can all resolve MUST pass, so the failing cases below are + /// discriminating rather than a check that cannot succeed. + function testConfigWithResolvableEtherscanEntriesPasses() external view { + this.externalCheckNetworksConfigured(CONFIG_RESOLVABLE, fixtureNetworks()); + } + + /// An `[etherscan]` entry carrying only a `key` MUST fail, naming itself. + /// This is the whole subject of the shape assertion: such an entry + /// satisfies every membership assertion beside it — the sections here name + /// exactly the networks, in both directions — and is still config that + /// verifies nothing, on any network in the section rather than only on its + /// own. + function testConfigWithUnresolvableLastEtherscanEntryReverts() external { + vm.expectRevert(abi.encodeWithSelector(EtherscanEntryUnresolvable.selector, "gamma")); + this.externalCheckNetworksConfigured(CONFIG_LAST_ENTRY_UNRESOLVABLE, fixtureNetworks()); + } + + /// The unresolvable entry MUST be caught wherever it sits, and reported as + /// itself. A config section is keyed rather than ordered, so which entry is + /// the broken one is not something the check gets to assume. + function testConfigWithUnresolvableMiddleEtherscanEntryReverts() external { + vm.expectRevert(abi.encodeWithSelector(EtherscanEntryUnresolvable.selector, "beta")); + this.externalCheckNetworksConfigured(CONFIG_MIDDLE_ENTRY_UNRESOLVABLE, fixtureNetworks()); + } } From 110806960743c2302531eeb5456e4fb4ec4b858f Mon Sep 17 00:00:00 2001 From: baku-ccron Date: Tue, 15 Sep 2026 20:32:57 +0000 Subject: [PATCH 2/5] Bind every declared `chain` to the network its alias actually forks MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `[etherscan]` now states a `chain` on all nine entries and the config group asserts each entry carries `chain` or `url`, but nothing re-checks that a stated id is the id its network reports. The nine were read off `cast chain-id` once, by hand, and from then on a wrong one resolves, reads as well-formed, passes every check that reads the text — and is what `--verify` submits, so the deployment is verified against another chain's explorer after the gas is spent. `testSupportedNetworkChainIdsAreBound` forks every supported network that states a `chain` and compares it against `block.chainid`. The same comparison catches the mirror case, an `[rpc_endpoints]` alias bound to a different network than it names, which is worse: every chain-anchored assertion ever made through that alias was made somewhere nobody named. In `RainDeployVerifyChain` because the subject is the endpoint. This is the contract that forks, and keeping the comparison out of the snapshot half is what leaves that half bindable by a job with no RPC credentials. It reads the binder's own `foundry.toml`, so the chain half now needs the same `fs_permissions` entry the snapshot half already does. An entry resolving through a `url` alone states no id and is skipped, since the config group requires only one of the two. Every entry being that way is `NoDeclaredChainIds` rather than a check that passes having forked nothing. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01V8ViHcKLVk2YoS2joH4HdN --- README.md | 40 +++-- foundry.toml | 5 + src/abstract/RainDeployVerifyChain.sol | 126 +++++++++++++++ test/src/abstract/RainDeployVerifyChain.t.sol | 151 ++++++++++++++++++ 4 files changed, 309 insertions(+), 13 deletions(-) diff --git a/README.md b/README.md index 6dd8787..507822f 100644 --- a/README.md +++ b/README.md @@ -124,13 +124,13 @@ guard. Five groups, sorted by what each is anchored to and therefore by what each can catch: -| Group | Anchored to | Catches | Cannot catch | -| -------- | ---------------------- | ------------------------------------- | -------------------------------- | -| Internal | the recorded set | an inconsistently generated set | a snapshot of the wrong contract | -| Source | `type(X).creationCode` | a snapshot of the wrong contract | anything about any chain | -| Record | the frozen record | a release the declaration missed | what a declared suite records | -| Chain | the networks | never deployed, or not there any more | anything about a candidate | -| Config | `foundry.toml` | a network it cannot fork or verify on | anything about a suite | +| Group | Anchored to | Catches | Cannot catch | +| -------- | ---------------------- | ----------------------------------------- | -------------------------------- | +| Internal | the recorded set | an inconsistently generated set | a snapshot of the wrong contract | +| Source | `type(X).creationCode` | a snapshot of the wrong contract | anything about any chain | +| Record | the frozen record | a release the declaration missed | what a declared suite records | +| Chain | the networks | never deployed, gone, or a wrong chain id | anything about a candidate | +| Config | `foundry.toml` | a network it cannot fork or verify on | anything about a suite | The internal group's blind spot is not a gap to close there: every check in it asks the recorded bytes to agree with each other, and the wrong contract's bytes @@ -200,6 +200,19 @@ value — the keys and the entry shapes are both in the text. So it needs no RPC and fails on the pull request that drifts rather than at dispatch time. Reading the file at all is what a consumer has to allow: see [Install](#install). +Whether a stated `chain` IS the network its alias forks is the one thing about +that config the text cannot settle, so it belongs to the chain group instead: +`testSupportedNetworkChainIdsAreBound` forks every supported network that states +one and compares it against `block.chainid`. A wrong id resolves and satisfies +every check that reads the text, and `chain` is what `--verify` submits, so the +deployment is verified against another chain's explorer after the gas is spent. +The same comparison catches the mirror case — an `[rpc_endpoints]` alias bound +to a different network than it names — which is worse, because every +chain-anchored assertion ever made through that alias was made somewhere nobody +named. An entry resolving through a `url` alone states no id and is skipped; +every entry being that way is refused rather than passed as a check with no +subject. + ## Address registry `AddressRegistry` binds an opaque `bytes32` name to an address. An immutable @@ -636,18 +649,19 @@ The versions have to match: the import paths are version-qualified, which is deliberate — it is what stops a consumer's incompatible copy from silently satisfying these imports. -The config group reads the CONSUMING repo's `foundry.toml`, so that repo has to -allow it and has to have the sections to be read: +The config and chain groups both read the CONSUMING repo's `foundry.toml`, so +that repo has to allow it and has to have the sections to be read: ```toml fs_permissions = [{ access = "read", path = "./foundry.toml" }] ``` `[rpc_endpoints]` and `[etherscan]` then have to name exactly the networks in -`supportedNetworks()`, and every `[etherscan]` entry has to carry at least one -of `chain` or `url`. Missing permission fails the check rather than skipping it, -which is the intended direction: a repo that cannot read its own config is a -repo whose config nothing has checked. +`supportedNetworks()`, every `[etherscan]` entry has to carry at least one of +`chain` or `url`, and a `chain` it states has to be the chain id the endpoint +bound to that alias reports. Missing permission fails the check rather than +skipping it, which is the intended direction: a repo that cannot read its own +config is a repo whose config nothing has checked. ## Develop diff --git a/foundry.toml b/foundry.toml index b4b453d..d5f12c6 100644 --- a/foundry.toml +++ b/foundry.toml @@ -106,6 +106,11 @@ robinhood = "${ROBINHOOD_RPC_URL}" # under a toolchain bump. Stating the chain an alias already resolves to # resolves it to the same chain, so neither these entries nor the check reading # them has to know the table. +# +# Each stated `chain` is compared against the chain id its `[rpc_endpoints]` +# alias actually reports, by `testSupportedNetworkChainIdsAreBound`. That is the +# one claim here the text cannot settle on its own: a wrong id resolves, reads +# as well-formed, and is what `--verify` submits. [etherscan] arbitrum = { key = "${CI_DEPLOY_ARBITRUM_ETHERSCAN_API_KEY}", chain = 42161 } base = { key = "${CI_DEPLOY_BASE_ETHERSCAN_API_KEY}", chain = 8453 } diff --git a/src/abstract/RainDeployVerifyChain.sol b/src/abstract/RainDeployVerifyChain.sol index b50ab26..cb0fe03 100644 --- a/src/abstract/RainDeployVerifyChain.sol +++ b/src/abstract/RainDeployVerifyChain.sol @@ -29,6 +29,31 @@ error CodeHashMismatchOnNetwork( string network, string suite, address deployedAddress, bytes32 expectedCodeHash, bytes32 actualCodeHash ); +/// Thrown when the chain id a network's `[etherscan]` entry declares is not the +/// chain id the endpoint bound to that network's `[rpc_endpoints]` alias +/// reports. Either the declaration is wrong — and `chain` is what `--verify` +/// submits, so the deployment is verified against another chain's explorer — or +/// the alias is bound to a different network than the one it names, and +/// everything ever checked through it was checked somewhere else. +/// @param network The network name, as configured in `[rpc_endpoints]`. +/// @param declared The chain id the `[etherscan]` entry states. +/// @param reported The chain id the endpoint answers with. +error NetworkChainIdMismatch(string network, uint256 declared, uint256 reported); + +/// Thrown when no supported network's `[etherscan]` entry declares a `chain` at +/// all. That is not nothing to check, it is a config in which every entry +/// resolves through a `url` alone, and a check with no subject passes having +/// forked nothing — indistinguishable from every declared id being right. +error NoDeclaredChainIds(); + +/// The chain id one network's `[etherscan]` entry states. +struct DeclaredChainId { + /// The network name, as configured in `[rpc_endpoints]` and `[etherscan]`. + string network; + /// The chain id the entry states. + uint256 chainId; +} + /// @title RainDeployVerifyChain /// @notice The only deploy-pin assertions anchored to something outside the /// repo: across every network in `LibRainDeploy.supportedNetworks()`, every @@ -134,4 +159,105 @@ abstract contract RainDeployVerifyChain is RainDeployVerifyBase { function testSuitesLiveOnEverySupportedNetwork() external { checkDeployedOnSupportedNetworks(deriveDeployments(releasedSuites())); } + + /// The chain id each supported network's `[etherscan]` entry states, for + /// the networks that state one. + /// + /// An entry with no `chain` is not a gap here. The config group requires + /// only that an entry carry at least one of `chain` or `url`, so one that + /// resolves through a `url` alone makes no claim about which chain its + /// alias is, and there is nothing about it to compare. What WOULD be a gap + /// is every entry being that way, which is why `checkNetworkChainIds` + /// refuses an empty declaration set rather than passing on it. + /// + /// Takes the config text rather than reading it, so a test can hand it one + /// it built. Reading the binder's own file is + /// `testSupportedNetworkChainIdsAreBound`. + /// @param config The raw `foundry.toml` text. + /// @param networks The supported networks whose entries to read. + /// @return The declaration of every network that states a chain id, in + /// `networks` order. + function declaredChainIds(string memory config, string[] memory networks) + internal + view + returns (DeclaredChainId[] memory) + { + uint256 declaredCount = 0; + for (uint256 i = 0; i < networks.length; i++) { + if (vm.keyExistsToml(config, string.concat(".etherscan.", networks[i], ".chain"))) { + declaredCount++; + } + } + + DeclaredChainId[] memory declared = new DeclaredChainId[](declaredCount); + uint256 next = 0; + for (uint256 i = 0; i < networks.length; i++) { + string memory key = string.concat(".etherscan.", networks[i], ".chain"); + if (vm.keyExistsToml(config, key)) { + declared[next] = DeclaredChainId({network: networks[i], chainId: vm.parseTomlUint(config, key)}); + next++; + } + } + return declared; + } + + /// Checks one network's declared chain id against a reported one. + /// @param network The network name, for the error only. + /// @param declared The chain id the `[etherscan]` entry states. + /// @param reported The chain id the bound endpoint answers with. + function checkNetworkChainId(string memory network, uint256 declared, uint256 reported) internal pure { + if (declared != reported) { + revert NetworkChainIdMismatch(network, declared, reported); + } + } + + /// Checks every declaration against the endpoint bound to its network's + /// `[rpc_endpoints]` alias. + /// + /// Every fork is created before any is selected, for the reason + /// `LibRainDeploy.createForks` gives. + /// @param declared The declarations to check. + function checkNetworkChainIds(DeclaredChainId[] memory declared) internal { + if (declared.length == 0) { + revert NoDeclaredChainIds(); + } + + string[] memory names = new string[](declared.length); + for (uint256 i = 0; i < declared.length; i++) { + names[i] = declared[i].network; + } + + uint256[] memory forkIds = LibRainDeploy.createForks(vm, names); + for (uint256 i = 0; i < declared.length; i++) { + vm.selectFork(forkIds[i]); + checkNetworkChainId(declared[i].network, declared[i].chainId, block.chainid); + } + } + + /// Every chain id `[etherscan]` declares MUST be the one the endpoint bound + /// to that network's `[rpc_endpoints]` alias reports. + /// + /// The config group asserts that those entries exist and can resolve, and + /// can go no further: whether `chain = 42161` is the network `arbitrum` + /// forks is a claim about the world that only a fork settles. A wrong id + /// resolves, satisfies every check that reads the text, and is what + /// `--verify` submits — so the deployment is verified against another + /// chain's explorer, after the gas is spent. The mirror of it is an + /// `[rpc_endpoints]` alias bound to a different network than it names, + /// which the same comparison catches and which is worse: every + /// chain-anchored assertion ever made through that alias was made somewhere + /// nobody named. + /// + /// Here rather than in the config group because the subject is the + /// endpoint. This is the contract that forks, and keeping the comparison + /// out of the snapshot half is what leaves that half bindable by a job with + /// no RPC endpoint at all. + /// + /// `vm.readFile` resolves against the project root of whatever runs it, so + /// the file read is the binder's own — which is why a binding repo needs + /// `{ access = "read", path = "./foundry.toml" }` in `fs_permissions` for + /// THIS half as well as the snapshot half. + function testSupportedNetworkChainIdsAreBound() external { + checkNetworkChainIds(declaredChainIds(vm.readFile("foundry.toml"), LibRainDeploy.supportedNetworks())); + } } diff --git a/test/src/abstract/RainDeployVerifyChain.t.sol b/test/src/abstract/RainDeployVerifyChain.t.sol index ea36286..180f69e 100644 --- a/test/src/abstract/RainDeployVerifyChain.t.sol +++ b/test/src/abstract/RainDeployVerifyChain.t.sol @@ -5,6 +5,9 @@ pragma solidity =0.8.25; import {DerivedDeploy} from "../../../src/abstract/RainDeployVerifyBase.sol"; import { CodeHashMismatchOnNetwork, + DeclaredChainId, + NetworkChainIdMismatch, + NoDeclaredChainIds, NotDeployedOnNetwork, RainDeployVerifyChain } from "../../../src/abstract/RainDeployVerifyChain.sol"; @@ -361,4 +364,152 @@ contract RainDeployVerifyChainTest is ExampleDeploySuites, RainDeployVerifyChain (bool extraFork,) = address(vm).call(abi.encodeWithSignature("selectFork(uint256)", networkCount)); assertFalse(extraFork, "the run opened a fork that is not one of the supported networks"); } + + /// An `[etherscan]` section in the shape the checks read, with a chain id + /// on the first and last entries and none on the middle one. + /// @return The config text. + function exampleEtherscanConfig() internal pure returns (string memory) { + return "[etherscan]\n" "arbitrum = { key = \"k\", chain = 11 }\n" + "base = { key = \"k\", url = \"https://example.com/api\" }\n" "ethereum = { key = \"k\", chain = 33 }\n"; + } + + /// External wrapper for `checkNetworkChainId` so `vm.expectRevert` works at + /// the correct call depth. + /// @param network The network name, for the error only. + /// @param declared The chain id the entry states. + /// @param reported The chain id the endpoint answers with. + function externalCheckNetworkChainId(string memory network, uint256 declared, uint256 reported) external pure { + checkNetworkChainId(network, declared, reported); + } + + /// External wrapper for `checkNetworkChainIds` so `vm.expectRevert` works + /// at the correct call depth. + /// @param declared The declarations to check. + function externalCheckNetworkChainIds(DeclaredChainId[] memory declared) external { + checkNetworkChainIds(declared); + } + + /// A declared chain id that is not the reported one MUST fail, naming the + /// network and BOTH ids. Which one is wrong — the declaration or the alias + /// the endpoint is bound to — is not something the check can know, and the + /// two are opposite fixes, so both ids are in the failure. + function testChainIdMismatchReverts() external { + vm.expectRevert(abi.encodeWithSelector(NetworkChainIdMismatch.selector, "arbitrum", 11, 22)); + this.externalCheckNetworkChainId("arbitrum", 11, 22); + } + + /// A declared chain id that IS the reported one MUST pass. Without this a + /// comparison that rejected every pair would satisfy the case above. + function testChainIdMatchPasses() external view { + this.externalCheckNetworkChainId("arbitrum", 11, 11); + } + + /// The reported id MUST come from a fork of the network's own + /// `[rpc_endpoints]` alias. + /// + /// The inherited `testSupportedNetworkChainIdsAreBound` passing cannot say + /// that: a comparison reading `block.chainid` off the unforked 31337 EVM + /// fails there for every network, and so does one reading it off the wrong + /// fork, and a green run tells the two apart from neither. So this declares + /// an id no network has, for an alias that really resolves, and the id in + /// the failure is the one THAT endpoint answers with — `1`, which is + /// Ethereum's and is neither 31337 nor the declared value. + function testChainIdIsReadFromTheForkedEndpoint() external { + DeclaredChainId[] memory declared = new DeclaredChainId[](1); + declared[0] = DeclaredChainId({network: LibRainDeploy.ETHEREUM, chainId: 987654}); + + vm.expectRevert(abi.encodeWithSelector(NetworkChainIdMismatch.selector, LibRainDeploy.ETHEREUM, 987654, 1)); + this.externalCheckNetworkChainIds(declared); + } + + /// EVERY declaration MUST be checked, not just the first. The wrong id here + /// is on the LAST entry, behind one that is right, so a loop that stopped + /// at the first agreement would pass. + function testChainIdChecksEveryDeclaration() external { + DeclaredChainId[] memory declared = new DeclaredChainId[](2); + declared[0] = DeclaredChainId({network: LibRainDeploy.ETHEREUM, chainId: 1}); + declared[1] = DeclaredChainId({network: LibRainDeploy.BASE, chainId: 987654}); + + vm.expectRevert(abi.encodeWithSelector(NetworkChainIdMismatch.selector, LibRainDeploy.BASE, 987654, 8453)); + this.externalCheckNetworkChainIds(declared); + } + + /// Nothing declared MUST fail rather than pass having forked nothing. It is + /// the one input that satisfies the loop without a subject, and it is what + /// a config whose every entry resolves through a `url` alone hands in. + function testChainIdNoDeclarationsReverts() external { + vm.expectRevert(NoDeclaredChainIds.selector); + this.externalCheckNetworkChainIds(new DeclaredChainId[](0)); + } + + /// The declarations MUST be the ids the config text states, paired with the + /// networks that state them. Distinct values, so a pairing that slipped by + /// one is a different number rather than the same one twice. + function testDeclaredChainIdsReadsTheConfigText() external view { + string[] memory networks = new string[](2); + networks[0] = LibRainDeploy.ARBITRUM_ONE; + networks[1] = LibRainDeploy.ETHEREUM; + + DeclaredChainId[] memory declared = declaredChainIds(exampleEtherscanConfig(), networks); + + assertEq(declared.length, 2); + assertEq(declared[0].network, LibRainDeploy.ARBITRUM_ONE); + assertEq(declared[0].chainId, 11); + assertEq(declared[1].network, LibRainDeploy.ETHEREUM); + assertEq(declared[1].chainId, 33); + } + + /// An entry that states no `chain` MUST be skipped rather than read as a + /// zero. The config group requires only `chain` OR `url` of an entry, so an + /// entry resolving through its `url` claims no chain id — and a zero + /// standing in for the absent claim is a mismatch against every network + /// there is. + /// + /// The skipped entry is in the MIDDLE, so the entry after it is still read + /// and still paired with its own network. + function testDeclaredChainIdsSkipsEntriesWithNoChain() external view { + string[] memory networks = new string[](3); + networks[0] = LibRainDeploy.ARBITRUM_ONE; + networks[1] = LibRainDeploy.BASE; + networks[2] = LibRainDeploy.ETHEREUM; + + DeclaredChainId[] memory declared = declaredChainIds(exampleEtherscanConfig(), networks); + + assertEq(declared.length, 2); + assertEq(declared[0].network, LibRainDeploy.ARBITRUM_ONE); + assertEq(declared[0].chainId, 11); + assertEq(declared[1].network, LibRainDeploy.ETHEREUM); + assertEq(declared[1].chainId, 33); + } + + /// A config where nothing states a `chain` MUST produce nothing to check, + /// which `checkNetworkChainIds` then refuses. Read through the same pair of + /// calls the inherited test makes, so the refusal is reachable from config + /// text rather than only from an array a test built. + function testDeclaredChainIdsOfUrlOnlyEntriesIsRefused() external { + string[] memory networks = new string[](1); + networks[0] = LibRainDeploy.BASE; + + DeclaredChainId[] memory declared = declaredChainIds(exampleEtherscanConfig(), networks); + assertEq(declared.length, 0); + + vm.expectRevert(NoDeclaredChainIds.selector); + this.externalCheckNetworkChainIds(declared); + } + + /// A network with no `[etherscan]` entry at all MUST be skipped here rather + /// than reverting on the read. Membership is the config group's assertion + /// and it names the missing network; a parse error here would fail first, + /// on a network, with nothing about the section it is missing from. + function testDeclaredChainIdsSkipsNetworksWithNoEntry() external view { + string[] memory networks = new string[](2); + networks[0] = LibRainDeploy.FLARE; + networks[1] = LibRainDeploy.ETHEREUM; + + DeclaredChainId[] memory declared = declaredChainIds(exampleEtherscanConfig(), networks); + + assertEq(declared.length, 1); + assertEq(declared[0].network, LibRainDeploy.ETHEREUM); + assertEq(declared[0].chainId, 33); + } } From 3b9025847b465efff09e967eb73f397e9a9cd91c Mon Sep 17 00:00:00 2001 From: baku-ccron Date: Tue, 15 Sep 2026 20:45:36 +0000 Subject: [PATCH 3/5] Name the chain-id binding in the group's scope statement MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The `@notice` opens by stating what the whole group asserts, and the previous commit added a second assertion family to this contract without extending it. Left alone the sentence names only the deployment half while still reading as the complete list, so the one thing a scope statement exists to answer — what is in this group — is answered wrongly. The README table in that same commit already carries both. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01V8ViHcKLVk2YoS2joH4HdN --- src/abstract/RainDeployVerifyChain.sol | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/abstract/RainDeployVerifyChain.sol b/src/abstract/RainDeployVerifyChain.sol index cb0fe03..c3e295e 100644 --- a/src/abstract/RainDeployVerifyChain.sol +++ b/src/abstract/RainDeployVerifyChain.sol @@ -57,7 +57,9 @@ struct DeclaredChainId { /// @title RainDeployVerifyChain /// @notice The only deploy-pin assertions anchored to something outside the /// repo: across every network in `LibRainDeploy.supportedNetworks()`, every -/// RELEASED suite's derived address carries code with its derived code hash. +/// RELEASED suite's derived address carries code with its derived code hash, +/// and every chain id `[etherscan]` declares is the one that network's alias +/// forks. /// /// This is the only group that can catch a suite that never deployed to a /// network, or that is not there any more. Neither is a fact the repo can hold: From dd03dd1d68b2a9ea892aeafdf28ba310abe04386 Mon Sep 17 00:00:00 2001 From: baku-ccron Date: Tue, 15 Sep 2026 22:43:15 +0000 Subject: [PATCH 4/5] Cut the rationale essays and restated docs this PR added Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01V8ViHcKLVk2YoS2joH4HdN --- foundry.toml | 21 ++------ src/abstract/RainDeployVerifyChain.sol | 47 ++---------------- src/abstract/RainDeployVerifySnapshot.sol | 17 +------ src/abstract/RainDeployVerifySnapshotBase.sol | 32 +------------ test/src/abstract/RainDeployVerifyChain.t.sol | 48 ------------------- .../RainDeployVerifySnapshotBase.t.sol | 27 ----------- 6 files changed, 11 insertions(+), 181 deletions(-) diff --git a/foundry.toml b/foundry.toml index ff3e937..cd39630 100644 --- a/foundry.toml +++ b/foundry.toml @@ -94,23 +94,10 @@ robinhood = "${ROBINHOOD_RPC_URL}" # directions, by `testSupportedNetworksAreFullyConfigured`. Adding a network is # an edit to all three or a red test, not a broadcast that discovers it. # -# `chain` is stated on EVERY entry, and `testSupportedNetworksAreFullyConfigured` -# requires at least one of `chain` or `url` on each. An entry with neither, under -# an alias foundry does not itself resolve to a chain, is not a missing key, it -# is a config error — "At least one of `url` or `chain` must be present for -# Etherscan config with unknown alias" — raised while resolving the section, so -# it takes down verification for every network in it and not only its own. -# -# Stated on every entry rather than only on the aliases foundry cannot resolve, -# because which aliases those are is foundry's own table and that table moves -# under a toolchain bump. Stating the chain an alias already resolves to -# resolves it to the same chain, so neither these entries nor the check reading -# them has to know the table. -# -# Each stated `chain` is compared against the chain id its `[rpc_endpoints]` -# alias actually reports, by `testSupportedNetworkChainIdsAreBound`. That is the -# one claim here the text cannot settle on its own: a wrong id resolves, reads -# as well-formed, and is what `--verify` submits. +# `chain` is stated on every entry because an entry carrying neither `chain` nor +# `url`, under an alias foundry does not itself resolve to a chain, is a config +# error raised while resolving the section — it takes down verification for +# every network in it and not only its own. [etherscan] arbitrum = { key = "${CI_DEPLOY_ARBITRUM_ETHERSCAN_API_KEY}", chain = 42161 } base = { key = "${CI_DEPLOY_BASE_ETHERSCAN_API_KEY}", chain = 8453 } diff --git a/src/abstract/RainDeployVerifyChain.sol b/src/abstract/RainDeployVerifyChain.sol index cff6653..8387ff7 100644 --- a/src/abstract/RainDeployVerifyChain.sol +++ b/src/abstract/RainDeployVerifyChain.sol @@ -31,26 +31,19 @@ error CodeHashMismatchOnNetwork( /// Thrown when the chain id a network's `[etherscan]` entry declares is not the /// chain id the endpoint bound to that network's `[rpc_endpoints]` alias -/// reports. Either the declaration is wrong — and `chain` is what `--verify` -/// submits, so the deployment is verified against another chain's explorer — or -/// the alias is bound to a different network than the one it names, and -/// everything ever checked through it was checked somewhere else. +/// reports. /// @param network The network name, as configured in `[rpc_endpoints]`. /// @param declared The chain id the `[etherscan]` entry states. /// @param reported The chain id the endpoint answers with. error NetworkChainIdMismatch(string network, uint256 declared, uint256 reported); /// Thrown when no supported network's `[etherscan]` entry declares a `chain` at -/// all. That is not nothing to check, it is a config in which every entry -/// resolves through a `url` alone, and a check with no subject passes having -/// forked nothing — indistinguishable from every declared id being right. +/// all. error NoDeclaredChainIds(); /// The chain id one network's `[etherscan]` entry states. struct DeclaredChainId { - /// The network name, as configured in `[rpc_endpoints]` and `[etherscan]`. string network; - /// The chain id the entry states. uint256 chainId; } @@ -179,17 +172,6 @@ abstract contract RainDeployVerifyChain is RainDeployVerifyBase { /// The chain id each supported network's `[etherscan]` entry states, for /// the networks that state one. - /// - /// An entry with no `chain` is not a gap here. The config group requires - /// only that an entry carry at least one of `chain` or `url`, so one that - /// resolves through a `url` alone makes no claim about which chain its - /// alias is, and there is nothing about it to compare. What WOULD be a gap - /// is every entry being that way, which is why `checkNetworkChainIds` - /// refuses an empty declaration set rather than passing on it. - /// - /// Takes the config text rather than reading it, so a test can hand it one - /// it built. Reading the binder's own file is - /// `testSupportedNetworkChainIdsAreBound`. /// @param config The raw `foundry.toml` text. /// @param networks The supported networks whose entries to read. /// @return The declaration of every network that states a chain id, in @@ -230,9 +212,6 @@ abstract contract RainDeployVerifyChain is RainDeployVerifyBase { /// Checks every declaration against the endpoint bound to its network's /// `[rpc_endpoints]` alias. - /// - /// Every fork is created before any is selected, for the reason - /// `LibRainDeploy.createForks` gives. /// @param declared The declarations to check. function checkNetworkChainIds(DeclaredChainId[] memory declared) internal { if (declared.length == 0) { @@ -254,26 +233,8 @@ abstract contract RainDeployVerifyChain is RainDeployVerifyBase { /// Every chain id `[etherscan]` declares MUST be the one the endpoint bound /// to that network's `[rpc_endpoints]` alias reports. /// - /// The config group asserts that those entries exist and can resolve, and - /// can go no further: whether `chain = 42161` is the network `arbitrum` - /// forks is a claim about the world that only a fork settles. A wrong id - /// resolves, satisfies every check that reads the text, and is what - /// `--verify` submits — so the deployment is verified against another - /// chain's explorer, after the gas is spent. The mirror of it is an - /// `[rpc_endpoints]` alias bound to a different network than it names, - /// which the same comparison catches and which is worse: every - /// chain-anchored assertion ever made through that alias was made somewhere - /// nobody named. - /// - /// Here rather than in the config group because the subject is the - /// endpoint. This is the contract that forks, and keeping the comparison - /// out of the snapshot half is what leaves that half bindable by a job with - /// no RPC endpoint at all. - /// - /// `vm.readFile` resolves against the project root of whatever runs it, so - /// the file read is the binder's own — which is why a binding repo needs - /// `{ access = "read", path = "./foundry.toml" }` in `fs_permissions` for - /// THIS half as well as the snapshot half. + /// A binding repo needs `{ access = "read", path = "./foundry.toml" }` in + /// `fs_permissions` for this half as well as the snapshot half. function testSupportedNetworkChainIdsAreBound() external { checkNetworkChainIds(declaredChainIds(vm.readFile("foundry.toml"), LibRainDeploy.supportedNetworks())); } diff --git a/src/abstract/RainDeployVerifySnapshot.sol b/src/abstract/RainDeployVerifySnapshot.sol index f10070a..f7a676e 100644 --- a/src/abstract/RainDeployVerifySnapshot.sol +++ b/src/abstract/RainDeployVerifySnapshot.sol @@ -76,29 +76,14 @@ abstract contract RainDeployVerifySnapshot is RainDeployVerifySnapshotBase { /// This is what makes the `[etherscan]` half enforced at all. The RPC half /// is enforced only incidentally, by the fork tests, and only forwards. /// - /// Membership is necessary and not sufficient for that half, so the SHAPE - /// of each `[etherscan]` entry is asserted beside it by - /// `checkEtherscanEntriesResolvable`: foundry resolves the whole section, so - /// an entry it cannot resolve fails `--verify` for the other entries as - /// well, after the gas is spent, while satisfying every membership - /// assertion here. - /// /// The raw file is read rather than forge's resolved config because the - /// values are `${VAR}` interpolations that exist only in CI. Nothing - /// asserted here is a value — the keys, and that each `[etherscan]` entry - /// carries enough to resolve at all, are both in the text — so this needs - /// no RPC and fails on the PR that drifts rather than at dispatch time. + /// values are `${VAR}` interpolations that exist only in CI. /// /// `vm.readFile` resolves against the project root of whatever runs it, so /// the file read is the binder's own and the networks are this package's. /// A binding repo therefore needs `{ access = "read", path = /// "./foundry.toml" }` in `fs_permissions`, and one without it fails here /// rather than passing on a file it never opened. - /// - /// The assertions themselves are `checkNetworksConfigured`, in the base, - /// because they take the config as an argument and so can be handed one a - /// test builds. Reading the binder's own file is the part that cannot be, - /// and it is all that is left here. function testSupportedNetworksAreFullyConfigured() external view { checkNetworksConfigured(vm.readFile("foundry.toml"), LibRainDeploy.supportedNetworks()); } diff --git a/src/abstract/RainDeployVerifySnapshotBase.sol b/src/abstract/RainDeployVerifySnapshotBase.sol index 8822c3f..865e7e7 100644 --- a/src/abstract/RainDeployVerifySnapshotBase.sol +++ b/src/abstract/RainDeployVerifySnapshotBase.sol @@ -54,12 +54,8 @@ error FrozenSnapshotUnreadable(string path); /// @param path The record file declaring `DEPLOYED_ADDRESS` more than once. error FrozenSnapshotAmbiguous(string path); -/// Thrown when an `[etherscan]` entry carries neither `chain` nor `url`. Under -/// an alias foundry does not itself resolve to a chain that entry is not a -/// missing key, it is "At least one of `url` or `chain` must be present for -/// Etherscan config with unknown alias" — raised while foundry resolves the -/// SECTION, so it takes verification down for the other entries too and not -/// only its own. +/// Thrown when an `[etherscan]` entry carries neither `chain` nor `url`, which +/// foundry raises while resolving the whole section rather than that entry. /// @param entry The `[etherscan]` entry that cannot resolve. error EtherscanEntryUnresolvable(string entry); @@ -300,19 +296,6 @@ abstract contract RainDeployVerifySnapshotBase is RainDeployVerifyBase { /// Checks that every `[etherscan]` entry can resolve at all: each carries at /// least one of `chain` or `url`. - /// - /// The entries EXISTING is not enough for the section to verify anything. - /// Foundry resolves the section rather than the single entry the network - /// being verified needs, so one entry it cannot resolve is an error raised - /// for whichever network `--verify` was pointed at — the failure mode the - /// key checks are there to keep off a broadcast, arriving from an entry - /// that satisfies them. - /// - /// Required of EVERY entry rather than only the aliases foundry cannot - /// resolve itself, because that set is foundry's table and moves under a - /// toolchain bump. Stating the chain an alias already resolves to resolves - /// it to the same chain, so the strict form is monotonic, needs to know - /// nothing of that table, and cannot red-line when foundry adds an alias. /// @param config The raw `foundry.toml` text. /// @param entries The `[etherscan]` entries to check. function checkEtherscanEntriesResolvable(string memory config, string[] memory entries) internal view { @@ -329,17 +312,6 @@ abstract contract RainDeployVerifySnapshotBase is RainDeployVerifyBase { /// Checks a `foundry.toml`'s `[rpc_endpoints]` and `[etherscan]` sections /// against a set of supported networks: the three lists are one list, and /// every `[etherscan]` entry can resolve. - /// - /// Membership is asserted in BOTH directions. Containment one way alone - /// passes for a section carrying an alias nothing deploys to, and the other - /// way alone passes for a network with no config at all. Membership rather - /// than position, because a config section is keyed rather than ordered and - /// there is no order in it to assert. - /// - /// Takes the config text rather than reading it, so it can be handed one a - /// test builds. What reads the binder's own file is - /// `testSupportedNetworksAreFullyConfigured`, and see it for why the file's - /// text is the subject at all. /// @param config The raw `foundry.toml` text. /// @param networks The supported networks the sections must name. function checkNetworksConfigured(string memory config, string[] memory networks) internal view { diff --git a/test/src/abstract/RainDeployVerifyChain.t.sol b/test/src/abstract/RainDeployVerifyChain.t.sol index d801445..e9168c2 100644 --- a/test/src/abstract/RainDeployVerifyChain.t.sol +++ b/test/src/abstract/RainDeployVerifyChain.t.sol @@ -406,9 +406,6 @@ contract RainDeployVerifyChainTest is ExampleDeploySuites, RainDeployVerifyChain assertFalse(extraFork, "the run opened a fork that is not one of the supported networks"); } - /// An `[etherscan]` section in the shape the checks read, with a chain id - /// on the first and last entries and none on the middle one. - /// @return The config text. function exampleEtherscanConfig() internal pure returns (string memory) { return "[etherscan]\n" "arbitrum = { key = \"k\", chain = 11 }\n" "base = { key = \"k\", url = \"https://example.com/api\" }\n" "ethereum = { key = \"k\", chain = 33 }\n"; @@ -416,45 +413,25 @@ contract RainDeployVerifyChainTest is ExampleDeploySuites, RainDeployVerifyChain /// External wrapper for `checkNetworkChainId` so `vm.expectRevert` works at /// the correct call depth. - /// @param network The network name, for the error only. - /// @param declared The chain id the entry states. - /// @param reported The chain id the endpoint answers with. function externalCheckNetworkChainId(string memory network, uint256 declared, uint256 reported) external pure { checkNetworkChainId(network, declared, reported); } /// External wrapper for `checkNetworkChainIds` so `vm.expectRevert` works /// at the correct call depth. - /// @param declared The declarations to check. function externalCheckNetworkChainIds(DeclaredChainId[] memory declared) external { checkNetworkChainIds(declared); } - /// A declared chain id that is not the reported one MUST fail, naming the - /// network and BOTH ids. Which one is wrong — the declaration or the alias - /// the endpoint is bound to — is not something the check can know, and the - /// two are opposite fixes, so both ids are in the failure. function testChainIdMismatchReverts() external { vm.expectRevert(abi.encodeWithSelector(NetworkChainIdMismatch.selector, "arbitrum", 11, 22)); this.externalCheckNetworkChainId("arbitrum", 11, 22); } - /// A declared chain id that IS the reported one MUST pass. Without this a - /// comparison that rejected every pair would satisfy the case above. function testChainIdMatchPasses() external view { this.externalCheckNetworkChainId("arbitrum", 11, 11); } - /// The reported id MUST come from a fork of the network's own - /// `[rpc_endpoints]` alias. - /// - /// The inherited `testSupportedNetworkChainIdsAreBound` passing cannot say - /// that: a comparison reading `block.chainid` off the unforked 31337 EVM - /// fails there for every network, and so does one reading it off the wrong - /// fork, and a green run tells the two apart from neither. So this declares - /// an id no network has, for an alias that really resolves, and the id in - /// the failure is the one THAT endpoint answers with — `1`, which is - /// Ethereum's and is neither 31337 nor the declared value. function testChainIdIsReadFromTheForkedEndpoint() external { DeclaredChainId[] memory declared = new DeclaredChainId[](1); declared[0] = DeclaredChainId({network: LibRainDeploy.ETHEREUM, chainId: 987654}); @@ -463,9 +440,6 @@ contract RainDeployVerifyChainTest is ExampleDeploySuites, RainDeployVerifyChain this.externalCheckNetworkChainIds(declared); } - /// EVERY declaration MUST be checked, not just the first. The wrong id here - /// is on the LAST entry, behind one that is right, so a loop that stopped - /// at the first agreement would pass. function testChainIdChecksEveryDeclaration() external { DeclaredChainId[] memory declared = new DeclaredChainId[](2); declared[0] = DeclaredChainId({network: LibRainDeploy.ETHEREUM, chainId: 1}); @@ -475,17 +449,11 @@ contract RainDeployVerifyChainTest is ExampleDeploySuites, RainDeployVerifyChain this.externalCheckNetworkChainIds(declared); } - /// Nothing declared MUST fail rather than pass having forked nothing. It is - /// the one input that satisfies the loop without a subject, and it is what - /// a config whose every entry resolves through a `url` alone hands in. function testChainIdNoDeclarationsReverts() external { vm.expectRevert(NoDeclaredChainIds.selector); this.externalCheckNetworkChainIds(new DeclaredChainId[](0)); } - /// The declarations MUST be the ids the config text states, paired with the - /// networks that state them. Distinct values, so a pairing that slipped by - /// one is a different number rather than the same one twice. function testDeclaredChainIdsReadsTheConfigText() external view { string[] memory networks = new string[](2); networks[0] = LibRainDeploy.ARBITRUM_ONE; @@ -500,14 +468,6 @@ contract RainDeployVerifyChainTest is ExampleDeploySuites, RainDeployVerifyChain assertEq(declared[1].chainId, 33); } - /// An entry that states no `chain` MUST be skipped rather than read as a - /// zero. The config group requires only `chain` OR `url` of an entry, so an - /// entry resolving through its `url` claims no chain id — and a zero - /// standing in for the absent claim is a mismatch against every network - /// there is. - /// - /// The skipped entry is in the MIDDLE, so the entry after it is still read - /// and still paired with its own network. function testDeclaredChainIdsSkipsEntriesWithNoChain() external view { string[] memory networks = new string[](3); networks[0] = LibRainDeploy.ARBITRUM_ONE; @@ -523,10 +483,6 @@ contract RainDeployVerifyChainTest is ExampleDeploySuites, RainDeployVerifyChain assertEq(declared[1].chainId, 33); } - /// A config where nothing states a `chain` MUST produce nothing to check, - /// which `checkNetworkChainIds` then refuses. Read through the same pair of - /// calls the inherited test makes, so the refusal is reachable from config - /// text rather than only from an array a test built. function testDeclaredChainIdsOfUrlOnlyEntriesIsRefused() external { string[] memory networks = new string[](1); networks[0] = LibRainDeploy.BASE; @@ -538,10 +494,6 @@ contract RainDeployVerifyChainTest is ExampleDeploySuites, RainDeployVerifyChain this.externalCheckNetworkChainIds(declared); } - /// A network with no `[etherscan]` entry at all MUST be skipped here rather - /// than reverting on the read. Membership is the config group's assertion - /// and it names the missing network; a parse error here would fail first, - /// on a network, with nothing about the section it is missing from. function testDeclaredChainIdsSkipsNetworksWithNoEntry() external view { string[] memory networks = new string[](2); networks[0] = LibRainDeploy.FLARE; diff --git a/test/src/abstract/RainDeployVerifySnapshotBase.t.sol b/test/src/abstract/RainDeployVerifySnapshotBase.t.sol index cf8e45c..b9cd83a 100644 --- a/test/src/abstract/RainDeployVerifySnapshotBase.t.sol +++ b/test/src/abstract/RainDeployVerifySnapshotBase.t.sol @@ -90,8 +90,6 @@ contract RainDeployVerifySnapshotBaseTest is ExampleDeploySuites, RainDeployVeri /// External wrapper for `checkNetworksConfigured` so `vm.expectRevert` works /// at the correct call depth. - /// @param config The raw `foundry.toml` text. - /// @param networks The supported networks the sections must name. function externalCheckNetworksConfigured(string memory config, string[] memory networks) external view { checkNetworksConfigured(config, networks); } @@ -746,10 +744,6 @@ contract RainDeployVerifySnapshotBaseTest is ExampleDeploySuites, RainDeployVeri } } - /// The networks the config fixtures below are written against. Named for - /// nothing real, so no fixture here reads as a claim about the networks - /// this repo actually deploys to. - /// @return networks The three fixture networks. function fixtureNetworks() internal pure returns (string[] memory) { string[] memory networks = new string[](3); networks[0] = "alpha"; @@ -758,51 +752,30 @@ contract RainDeployVerifySnapshotBaseTest is ExampleDeploySuites, RainDeployVeri return networks; } - /// A config whose sections name exactly the fixture networks and whose - /// `[etherscan]` entries can all resolve — `chain` alone on two of them and - /// `url` alone on the third, because each is sufficient by itself. string constant CONFIG_RESOLVABLE = "[rpc_endpoints]\n" "alpha = \"${ALPHA_RPC_URL}\"\n" "beta = \"${BETA_RPC_URL}\"\n" "gamma = \"${GAMMA_RPC_URL}\"\n" "\n" "[etherscan]\n" "alpha = { key = \"${A}\", chain = 999 }\n" "beta = { key = \"${B}\", url = \"https://example.com/api\" }\n" "gamma = { key = \"${C}\", chain = 1000 }\n"; - /// `CONFIG_RESOLVABLE` with the LAST `[etherscan]` entry stripped back to - /// its key, so a check that stops before the end of the section lets it - /// through. string constant CONFIG_LAST_ENTRY_UNRESOLVABLE = "[rpc_endpoints]\n" "alpha = \"${ALPHA_RPC_URL}\"\n" "beta = \"${BETA_RPC_URL}\"\n" "gamma = \"${GAMMA_RPC_URL}\"\n" "\n" "[etherscan]\n" "alpha = { key = \"${A}\", chain = 999 }\n" "beta = { key = \"${B}\", url = \"https://example.com/api\" }\n" "gamma = { key = \"${C}\" }\n"; - /// `CONFIG_RESOLVABLE` with the MIDDLE `[etherscan]` entry stripped back to - /// its key. That entry is the one `url` alone was carrying, so this is also - /// what a `url` deleted from an otherwise untouched entry leaves behind. string constant CONFIG_MIDDLE_ENTRY_UNRESOLVABLE = "[rpc_endpoints]\n" "alpha = \"${ALPHA_RPC_URL}\"\n" "beta = \"${BETA_RPC_URL}\"\n" "gamma = \"${GAMMA_RPC_URL}\"\n" "\n" "[etherscan]\n" "alpha = { key = \"${A}\", chain = 999 }\n" "beta = { key = \"${B}\" }\n" "gamma = { key = \"${C}\", chain = 1000 }\n"; - /// A config whose sections name the networks and whose `[etherscan]` - /// entries can all resolve MUST pass, so the failing cases below are - /// discriminating rather than a check that cannot succeed. function testConfigWithResolvableEtherscanEntriesPasses() external view { this.externalCheckNetworksConfigured(CONFIG_RESOLVABLE, fixtureNetworks()); } - /// An `[etherscan]` entry carrying only a `key` MUST fail, naming itself. - /// This is the whole subject of the shape assertion: such an entry - /// satisfies every membership assertion beside it — the sections here name - /// exactly the networks, in both directions — and is still config that - /// verifies nothing, on any network in the section rather than only on its - /// own. function testConfigWithUnresolvableLastEtherscanEntryReverts() external { vm.expectRevert(abi.encodeWithSelector(EtherscanEntryUnresolvable.selector, "gamma")); this.externalCheckNetworksConfigured(CONFIG_LAST_ENTRY_UNRESOLVABLE, fixtureNetworks()); } - /// The unresolvable entry MUST be caught wherever it sits, and reported as - /// itself. A config section is keyed rather than ordered, so which entry is - /// the broken one is not something the check gets to assume. function testConfigWithUnresolvableMiddleEtherscanEntryReverts() external { vm.expectRevert(abi.encodeWithSelector(EtherscanEntryUnresolvable.selector, "beta")); this.externalCheckNetworksConfigured(CONFIG_MIDDLE_ENTRY_UNRESOLVABLE, fixtureNetworks()); From 6bdd7863cb1b0aab66b56329c657576a243304a8 Mon Sep 17 00:00:00 2001 From: baku-ccron Date: Wed, 16 Sep 2026 00:13:41 +0000 Subject: [PATCH 5/5] docs: put back the reasons dd03dd1 cut with the restatements MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The sweep took the rationale as well as the restatement. What comes back is every sentence that says something the code does not: - `foundry.toml`: why `chain` is stated on EVERY entry rather than only on the aliases foundry cannot resolve itself, and that each one is compared against the id its alias reports. The second is this PR's whole subject. - `NetworkChainIdMismatch`, `NoDeclaredChainIds`: what a revert means for a deriving repo — a deployment verified against another chain's explorer, or a check that passes having forked nothing. - `DeclaredChainId`: the member docs, on a struct deriving repos read. - `declaredChainIds`, `checkNetworkChainIds`, `checkEtherscanEntriesResolvable`, `checkNetworksConfigured`: why an entry with no `chain` is not a gap, why the config text is a parameter rather than a read, the fork ordering, why the strict form is monotonic under a toolchain bump, and why membership is asserted in both directions. - `testSupportedNetworkChainIdsAreBound`, `testSupportedNetworksAreFullyConfigured`: why the comparison lives in the chain half, and why `fs_permissions` needs `foundry.toml` in the binder. - the test docstrings that say what each case discriminates — the mismatch that is on the LAST entry, the id no network has, the empty declaration set, the entry that satisfies every membership assertion and still verifies nothing. Left cut, because they restate a signature or a literal: the `@param`/`@return` lines on the `external*` revert wrappers and on `fixtureNetworks`, and the `exampleEtherscanConfig` block. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01V8ViHcKLVk2YoS2joH4HdN --- foundry.toml | 21 +++++++-- src/abstract/RainDeployVerifyChain.sol | 47 +++++++++++++++++-- src/abstract/RainDeployVerifySnapshot.sol | 17 ++++++- src/abstract/RainDeployVerifySnapshotBase.sol | 32 ++++++++++++- test/src/abstract/RainDeployVerifyChain.t.sol | 41 ++++++++++++++++ .../RainDeployVerifySnapshotBase.t.sol | 24 ++++++++++ 6 files changed, 171 insertions(+), 11 deletions(-) diff --git a/foundry.toml b/foundry.toml index cd39630..ff3e937 100644 --- a/foundry.toml +++ b/foundry.toml @@ -94,10 +94,23 @@ robinhood = "${ROBINHOOD_RPC_URL}" # directions, by `testSupportedNetworksAreFullyConfigured`. Adding a network is # an edit to all three or a red test, not a broadcast that discovers it. # -# `chain` is stated on every entry because an entry carrying neither `chain` nor -# `url`, under an alias foundry does not itself resolve to a chain, is a config -# error raised while resolving the section — it takes down verification for -# every network in it and not only its own. +# `chain` is stated on EVERY entry, and `testSupportedNetworksAreFullyConfigured` +# requires at least one of `chain` or `url` on each. An entry with neither, under +# an alias foundry does not itself resolve to a chain, is not a missing key, it +# is a config error — "At least one of `url` or `chain` must be present for +# Etherscan config with unknown alias" — raised while resolving the section, so +# it takes down verification for every network in it and not only its own. +# +# Stated on every entry rather than only on the aliases foundry cannot resolve, +# because which aliases those are is foundry's own table and that table moves +# under a toolchain bump. Stating the chain an alias already resolves to +# resolves it to the same chain, so neither these entries nor the check reading +# them has to know the table. +# +# Each stated `chain` is compared against the chain id its `[rpc_endpoints]` +# alias actually reports, by `testSupportedNetworkChainIdsAreBound`. That is the +# one claim here the text cannot settle on its own: a wrong id resolves, reads +# as well-formed, and is what `--verify` submits. [etherscan] arbitrum = { key = "${CI_DEPLOY_ARBITRUM_ETHERSCAN_API_KEY}", chain = 42161 } base = { key = "${CI_DEPLOY_BASE_ETHERSCAN_API_KEY}", chain = 8453 } diff --git a/src/abstract/RainDeployVerifyChain.sol b/src/abstract/RainDeployVerifyChain.sol index 8387ff7..cff6653 100644 --- a/src/abstract/RainDeployVerifyChain.sol +++ b/src/abstract/RainDeployVerifyChain.sol @@ -31,19 +31,26 @@ error CodeHashMismatchOnNetwork( /// Thrown when the chain id a network's `[etherscan]` entry declares is not the /// chain id the endpoint bound to that network's `[rpc_endpoints]` alias -/// reports. +/// reports. Either the declaration is wrong — and `chain` is what `--verify` +/// submits, so the deployment is verified against another chain's explorer — or +/// the alias is bound to a different network than the one it names, and +/// everything ever checked through it was checked somewhere else. /// @param network The network name, as configured in `[rpc_endpoints]`. /// @param declared The chain id the `[etherscan]` entry states. /// @param reported The chain id the endpoint answers with. error NetworkChainIdMismatch(string network, uint256 declared, uint256 reported); /// Thrown when no supported network's `[etherscan]` entry declares a `chain` at -/// all. +/// all. That is not nothing to check, it is a config in which every entry +/// resolves through a `url` alone, and a check with no subject passes having +/// forked nothing — indistinguishable from every declared id being right. error NoDeclaredChainIds(); /// The chain id one network's `[etherscan]` entry states. struct DeclaredChainId { + /// The network name, as configured in `[rpc_endpoints]` and `[etherscan]`. string network; + /// The chain id the entry states. uint256 chainId; } @@ -172,6 +179,17 @@ abstract contract RainDeployVerifyChain is RainDeployVerifyBase { /// The chain id each supported network's `[etherscan]` entry states, for /// the networks that state one. + /// + /// An entry with no `chain` is not a gap here. The config group requires + /// only that an entry carry at least one of `chain` or `url`, so one that + /// resolves through a `url` alone makes no claim about which chain its + /// alias is, and there is nothing about it to compare. What WOULD be a gap + /// is every entry being that way, which is why `checkNetworkChainIds` + /// refuses an empty declaration set rather than passing on it. + /// + /// Takes the config text rather than reading it, so a test can hand it one + /// it built. Reading the binder's own file is + /// `testSupportedNetworkChainIdsAreBound`. /// @param config The raw `foundry.toml` text. /// @param networks The supported networks whose entries to read. /// @return The declaration of every network that states a chain id, in @@ -212,6 +230,9 @@ abstract contract RainDeployVerifyChain is RainDeployVerifyBase { /// Checks every declaration against the endpoint bound to its network's /// `[rpc_endpoints]` alias. + /// + /// Every fork is created before any is selected, for the reason + /// `LibRainDeploy.createForks` gives. /// @param declared The declarations to check. function checkNetworkChainIds(DeclaredChainId[] memory declared) internal { if (declared.length == 0) { @@ -233,8 +254,26 @@ abstract contract RainDeployVerifyChain is RainDeployVerifyBase { /// Every chain id `[etherscan]` declares MUST be the one the endpoint bound /// to that network's `[rpc_endpoints]` alias reports. /// - /// A binding repo needs `{ access = "read", path = "./foundry.toml" }` in - /// `fs_permissions` for this half as well as the snapshot half. + /// The config group asserts that those entries exist and can resolve, and + /// can go no further: whether `chain = 42161` is the network `arbitrum` + /// forks is a claim about the world that only a fork settles. A wrong id + /// resolves, satisfies every check that reads the text, and is what + /// `--verify` submits — so the deployment is verified against another + /// chain's explorer, after the gas is spent. The mirror of it is an + /// `[rpc_endpoints]` alias bound to a different network than it names, + /// which the same comparison catches and which is worse: every + /// chain-anchored assertion ever made through that alias was made somewhere + /// nobody named. + /// + /// Here rather than in the config group because the subject is the + /// endpoint. This is the contract that forks, and keeping the comparison + /// out of the snapshot half is what leaves that half bindable by a job with + /// no RPC endpoint at all. + /// + /// `vm.readFile` resolves against the project root of whatever runs it, so + /// the file read is the binder's own — which is why a binding repo needs + /// `{ access = "read", path = "./foundry.toml" }` in `fs_permissions` for + /// THIS half as well as the snapshot half. function testSupportedNetworkChainIdsAreBound() external { checkNetworkChainIds(declaredChainIds(vm.readFile("foundry.toml"), LibRainDeploy.supportedNetworks())); } diff --git a/src/abstract/RainDeployVerifySnapshot.sol b/src/abstract/RainDeployVerifySnapshot.sol index f7a676e..f10070a 100644 --- a/src/abstract/RainDeployVerifySnapshot.sol +++ b/src/abstract/RainDeployVerifySnapshot.sol @@ -76,14 +76,29 @@ abstract contract RainDeployVerifySnapshot is RainDeployVerifySnapshotBase { /// This is what makes the `[etherscan]` half enforced at all. The RPC half /// is enforced only incidentally, by the fork tests, and only forwards. /// + /// Membership is necessary and not sufficient for that half, so the SHAPE + /// of each `[etherscan]` entry is asserted beside it by + /// `checkEtherscanEntriesResolvable`: foundry resolves the whole section, so + /// an entry it cannot resolve fails `--verify` for the other entries as + /// well, after the gas is spent, while satisfying every membership + /// assertion here. + /// /// The raw file is read rather than forge's resolved config because the - /// values are `${VAR}` interpolations that exist only in CI. + /// values are `${VAR}` interpolations that exist only in CI. Nothing + /// asserted here is a value — the keys, and that each `[etherscan]` entry + /// carries enough to resolve at all, are both in the text — so this needs + /// no RPC and fails on the PR that drifts rather than at dispatch time. /// /// `vm.readFile` resolves against the project root of whatever runs it, so /// the file read is the binder's own and the networks are this package's. /// A binding repo therefore needs `{ access = "read", path = /// "./foundry.toml" }` in `fs_permissions`, and one without it fails here /// rather than passing on a file it never opened. + /// + /// The assertions themselves are `checkNetworksConfigured`, in the base, + /// because they take the config as an argument and so can be handed one a + /// test builds. Reading the binder's own file is the part that cannot be, + /// and it is all that is left here. function testSupportedNetworksAreFullyConfigured() external view { checkNetworksConfigured(vm.readFile("foundry.toml"), LibRainDeploy.supportedNetworks()); } diff --git a/src/abstract/RainDeployVerifySnapshotBase.sol b/src/abstract/RainDeployVerifySnapshotBase.sol index 865e7e7..8822c3f 100644 --- a/src/abstract/RainDeployVerifySnapshotBase.sol +++ b/src/abstract/RainDeployVerifySnapshotBase.sol @@ -54,8 +54,12 @@ error FrozenSnapshotUnreadable(string path); /// @param path The record file declaring `DEPLOYED_ADDRESS` more than once. error FrozenSnapshotAmbiguous(string path); -/// Thrown when an `[etherscan]` entry carries neither `chain` nor `url`, which -/// foundry raises while resolving the whole section rather than that entry. +/// Thrown when an `[etherscan]` entry carries neither `chain` nor `url`. Under +/// an alias foundry does not itself resolve to a chain that entry is not a +/// missing key, it is "At least one of `url` or `chain` must be present for +/// Etherscan config with unknown alias" — raised while foundry resolves the +/// SECTION, so it takes verification down for the other entries too and not +/// only its own. /// @param entry The `[etherscan]` entry that cannot resolve. error EtherscanEntryUnresolvable(string entry); @@ -296,6 +300,19 @@ abstract contract RainDeployVerifySnapshotBase is RainDeployVerifyBase { /// Checks that every `[etherscan]` entry can resolve at all: each carries at /// least one of `chain` or `url`. + /// + /// The entries EXISTING is not enough for the section to verify anything. + /// Foundry resolves the section rather than the single entry the network + /// being verified needs, so one entry it cannot resolve is an error raised + /// for whichever network `--verify` was pointed at — the failure mode the + /// key checks are there to keep off a broadcast, arriving from an entry + /// that satisfies them. + /// + /// Required of EVERY entry rather than only the aliases foundry cannot + /// resolve itself, because that set is foundry's table and moves under a + /// toolchain bump. Stating the chain an alias already resolves to resolves + /// it to the same chain, so the strict form is monotonic, needs to know + /// nothing of that table, and cannot red-line when foundry adds an alias. /// @param config The raw `foundry.toml` text. /// @param entries The `[etherscan]` entries to check. function checkEtherscanEntriesResolvable(string memory config, string[] memory entries) internal view { @@ -312,6 +329,17 @@ abstract contract RainDeployVerifySnapshotBase is RainDeployVerifyBase { /// Checks a `foundry.toml`'s `[rpc_endpoints]` and `[etherscan]` sections /// against a set of supported networks: the three lists are one list, and /// every `[etherscan]` entry can resolve. + /// + /// Membership is asserted in BOTH directions. Containment one way alone + /// passes for a section carrying an alias nothing deploys to, and the other + /// way alone passes for a network with no config at all. Membership rather + /// than position, because a config section is keyed rather than ordered and + /// there is no order in it to assert. + /// + /// Takes the config text rather than reading it, so it can be handed one a + /// test builds. What reads the binder's own file is + /// `testSupportedNetworksAreFullyConfigured`, and see it for why the file's + /// text is the subject at all. /// @param config The raw `foundry.toml` text. /// @param networks The supported networks the sections must name. function checkNetworksConfigured(string memory config, string[] memory networks) internal view { diff --git a/test/src/abstract/RainDeployVerifyChain.t.sol b/test/src/abstract/RainDeployVerifyChain.t.sol index e9168c2..27437ed 100644 --- a/test/src/abstract/RainDeployVerifyChain.t.sol +++ b/test/src/abstract/RainDeployVerifyChain.t.sol @@ -423,15 +423,31 @@ contract RainDeployVerifyChainTest is ExampleDeploySuites, RainDeployVerifyChain checkNetworkChainIds(declared); } + /// A declared chain id that is not the reported one MUST fail, naming the + /// network and BOTH ids. Which one is wrong — the declaration or the alias + /// the endpoint is bound to — is not something the check can know, and the + /// two are opposite fixes, so both ids are in the failure. function testChainIdMismatchReverts() external { vm.expectRevert(abi.encodeWithSelector(NetworkChainIdMismatch.selector, "arbitrum", 11, 22)); this.externalCheckNetworkChainId("arbitrum", 11, 22); } + /// A declared chain id that IS the reported one MUST pass. Without this a + /// comparison that rejected every pair would satisfy the case above. function testChainIdMatchPasses() external view { this.externalCheckNetworkChainId("arbitrum", 11, 11); } + /// The reported id MUST come from a fork of the network's own + /// `[rpc_endpoints]` alias. + /// + /// The inherited `testSupportedNetworkChainIdsAreBound` passing cannot say + /// that: a comparison reading `block.chainid` off the unforked 31337 EVM + /// fails there for every network, and so does one reading it off the wrong + /// fork, and a green run tells the two apart from neither. So this declares + /// an id no network has, for an alias that really resolves, and the id in + /// the failure is the one THAT endpoint answers with — `1`, which is + /// Ethereum's and is neither 31337 nor the declared value. function testChainIdIsReadFromTheForkedEndpoint() external { DeclaredChainId[] memory declared = new DeclaredChainId[](1); declared[0] = DeclaredChainId({network: LibRainDeploy.ETHEREUM, chainId: 987654}); @@ -440,6 +456,9 @@ contract RainDeployVerifyChainTest is ExampleDeploySuites, RainDeployVerifyChain this.externalCheckNetworkChainIds(declared); } + /// EVERY declaration MUST be checked, not just the first. The wrong id here + /// is on the LAST entry, behind one that is right, so a loop that stopped + /// at the first agreement would pass. function testChainIdChecksEveryDeclaration() external { DeclaredChainId[] memory declared = new DeclaredChainId[](2); declared[0] = DeclaredChainId({network: LibRainDeploy.ETHEREUM, chainId: 1}); @@ -449,11 +468,17 @@ contract RainDeployVerifyChainTest is ExampleDeploySuites, RainDeployVerifyChain this.externalCheckNetworkChainIds(declared); } + /// Nothing declared MUST fail rather than pass having forked nothing. It is + /// the one input that satisfies the loop without a subject, and it is what + /// a config whose every entry resolves through a `url` alone hands in. function testChainIdNoDeclarationsReverts() external { vm.expectRevert(NoDeclaredChainIds.selector); this.externalCheckNetworkChainIds(new DeclaredChainId[](0)); } + /// The declarations MUST be the ids the config text states, paired with the + /// networks that state them. Distinct values, so a pairing that slipped by + /// one is a different number rather than the same one twice. function testDeclaredChainIdsReadsTheConfigText() external view { string[] memory networks = new string[](2); networks[0] = LibRainDeploy.ARBITRUM_ONE; @@ -468,6 +493,14 @@ contract RainDeployVerifyChainTest is ExampleDeploySuites, RainDeployVerifyChain assertEq(declared[1].chainId, 33); } + /// An entry that states no `chain` MUST be skipped rather than read as a + /// zero. The config group requires only `chain` OR `url` of an entry, so an + /// entry resolving through its `url` claims no chain id — and a zero + /// standing in for the absent claim is a mismatch against every network + /// there is. + /// + /// The skipped entry is in the MIDDLE, so the entry after it is still read + /// and still paired with its own network. function testDeclaredChainIdsSkipsEntriesWithNoChain() external view { string[] memory networks = new string[](3); networks[0] = LibRainDeploy.ARBITRUM_ONE; @@ -483,6 +516,10 @@ contract RainDeployVerifyChainTest is ExampleDeploySuites, RainDeployVerifyChain assertEq(declared[1].chainId, 33); } + /// A config where nothing states a `chain` MUST produce nothing to check, + /// which `checkNetworkChainIds` then refuses. Read through the same pair of + /// calls the inherited test makes, so the refusal is reachable from config + /// text rather than only from an array a test built. function testDeclaredChainIdsOfUrlOnlyEntriesIsRefused() external { string[] memory networks = new string[](1); networks[0] = LibRainDeploy.BASE; @@ -494,6 +531,10 @@ contract RainDeployVerifyChainTest is ExampleDeploySuites, RainDeployVerifyChain this.externalCheckNetworkChainIds(declared); } + /// A network with no `[etherscan]` entry at all MUST be skipped here rather + /// than reverting on the read. Membership is the config group's assertion + /// and it names the missing network; a parse error here would fail first, + /// on a network, with nothing about the section it is missing from. function testDeclaredChainIdsSkipsNetworksWithNoEntry() external view { string[] memory networks = new string[](2); networks[0] = LibRainDeploy.FLARE; diff --git a/test/src/abstract/RainDeployVerifySnapshotBase.t.sol b/test/src/abstract/RainDeployVerifySnapshotBase.t.sol index b9cd83a..6a913d5 100644 --- a/test/src/abstract/RainDeployVerifySnapshotBase.t.sol +++ b/test/src/abstract/RainDeployVerifySnapshotBase.t.sol @@ -744,6 +744,9 @@ contract RainDeployVerifySnapshotBaseTest is ExampleDeploySuites, RainDeployVeri } } + /// The networks the config fixtures below are written against. Named for + /// nothing real, so no fixture here reads as a claim about the networks + /// this repo actually deploys to. function fixtureNetworks() internal pure returns (string[] memory) { string[] memory networks = new string[](3); networks[0] = "alpha"; @@ -752,30 +755,51 @@ contract RainDeployVerifySnapshotBaseTest is ExampleDeploySuites, RainDeployVeri return networks; } + /// A config whose sections name exactly the fixture networks and whose + /// `[etherscan]` entries can all resolve — `chain` alone on two of them and + /// `url` alone on the third, because each is sufficient by itself. string constant CONFIG_RESOLVABLE = "[rpc_endpoints]\n" "alpha = \"${ALPHA_RPC_URL}\"\n" "beta = \"${BETA_RPC_URL}\"\n" "gamma = \"${GAMMA_RPC_URL}\"\n" "\n" "[etherscan]\n" "alpha = { key = \"${A}\", chain = 999 }\n" "beta = { key = \"${B}\", url = \"https://example.com/api\" }\n" "gamma = { key = \"${C}\", chain = 1000 }\n"; + /// `CONFIG_RESOLVABLE` with the LAST `[etherscan]` entry stripped back to + /// its key, so a check that stops before the end of the section lets it + /// through. string constant CONFIG_LAST_ENTRY_UNRESOLVABLE = "[rpc_endpoints]\n" "alpha = \"${ALPHA_RPC_URL}\"\n" "beta = \"${BETA_RPC_URL}\"\n" "gamma = \"${GAMMA_RPC_URL}\"\n" "\n" "[etherscan]\n" "alpha = { key = \"${A}\", chain = 999 }\n" "beta = { key = \"${B}\", url = \"https://example.com/api\" }\n" "gamma = { key = \"${C}\" }\n"; + /// `CONFIG_RESOLVABLE` with the MIDDLE `[etherscan]` entry stripped back to + /// its key. That entry is the one `url` alone was carrying, so this is also + /// what a `url` deleted from an otherwise untouched entry leaves behind. string constant CONFIG_MIDDLE_ENTRY_UNRESOLVABLE = "[rpc_endpoints]\n" "alpha = \"${ALPHA_RPC_URL}\"\n" "beta = \"${BETA_RPC_URL}\"\n" "gamma = \"${GAMMA_RPC_URL}\"\n" "\n" "[etherscan]\n" "alpha = { key = \"${A}\", chain = 999 }\n" "beta = { key = \"${B}\" }\n" "gamma = { key = \"${C}\", chain = 1000 }\n"; + /// A config whose sections name the networks and whose `[etherscan]` + /// entries can all resolve MUST pass, so the failing cases below are + /// discriminating rather than a check that cannot succeed. function testConfigWithResolvableEtherscanEntriesPasses() external view { this.externalCheckNetworksConfigured(CONFIG_RESOLVABLE, fixtureNetworks()); } + /// An `[etherscan]` entry carrying only a `key` MUST fail, naming itself. + /// This is the whole subject of the shape assertion: such an entry + /// satisfies every membership assertion beside it — the sections here name + /// exactly the networks, in both directions — and is still config that + /// verifies nothing, on any network in the section rather than only on its + /// own. function testConfigWithUnresolvableLastEtherscanEntryReverts() external { vm.expectRevert(abi.encodeWithSelector(EtherscanEntryUnresolvable.selector, "gamma")); this.externalCheckNetworksConfigured(CONFIG_LAST_ENTRY_UNRESOLVABLE, fixtureNetworks()); } + /// The unresolvable entry MUST be caught wherever it sits, and reported as + /// itself. A config section is keyed rather than ordered, so which entry is + /// the broken one is not something the check gets to assume. function testConfigWithUnresolvableMiddleEtherscanEntryReverts() external { vm.expectRevert(abi.encodeWithSelector(EtherscanEntryUnresolvable.selector, "beta")); this.externalCheckNetworksConfigured(CONFIG_MIDDLE_ENTRY_UNRESOLVABLE, fixtureNetworks());