diff --git a/src/lib/LibRainDeploy.sol b/src/lib/LibRainDeploy.sol index 616ec9f..c3bfde9 100644 --- a/src/lib/LibRainDeploy.sol +++ b/src/lib/LibRainDeploy.sol @@ -50,6 +50,11 @@ library LibRainDeploy { /// do not pair up. error ResolvedAddressesLengthMismatch(uint256 readCallsLength, uint256 expectedAddressesLength); + /// Thrown when a post-deploy check is given no reads. A check with nothing + /// to read passes on every network having asserted nothing, which is + /// indistinguishable from every read checking out. + error NoResolvedAddressReads(address target); + /// Thrown when a post-deploy read reverts, or answers with something that is /// not a single address-sized word. error ResolvedAddressReadFailed(string network, address target, uint256 index, bytes returnData); @@ -263,6 +268,12 @@ library LibRainDeploy { /// Only the consumer knows where it stored what it resolved, so the consumer /// supplies the reads. Each entry in `readCalls` is static-called against /// `target` and MUST answer with exactly one address. + /// + /// An empty read set is REFUSED. A check with nothing to read returns having + /// asserted nothing, which is indistinguishable from every read checking + /// out, and it is what a consumer that built its read list from a source + /// that came back empty hands in — right before it migrates onto the + /// deployment this was supposed to verify. /// @param network The network name, for the error only. /// @param target The deployed contract to read. /// @param readCalls The calldata for each read, e.g. @@ -278,6 +289,13 @@ library LibRainDeploy { if (readCalls.length != expectedAddresses.length) { revert ResolvedAddressesLengthMismatch(readCalls.length, expectedAddresses.length); } + // After the pairing check, not before it: an unpaired call is a + // mispairing whichever side is empty, and reporting the empty pair as a + // mismatch of zero against zero would say nothing. The empty pair is the + // one case pairing cannot see, so it is its own error. + if (readCalls.length == 0) { + revert NoResolvedAddressReads(target); + } for (uint256 i = 0; i < readCalls.length; i++) { // The consumer supplies the reads, so the call is low level by // construction: there is no interface here to call through. Excluded @@ -317,6 +335,10 @@ library LibRainDeploy { /// than expected is a burned deterministic address, found while nothing /// points at it yet — which is the whole reason to verify before migrating /// onto a deployment rather than trusting it. + /// + /// An empty network set and an empty read set are both REFUSED, before + /// anything is forked. Either one makes this return success across every + /// network having read nothing at all. /// @param vm The Vm instance to use for forking. /// @param networks The list of network names to check. /// @param target The deployed contract to read on each network. @@ -338,6 +360,11 @@ library LibRainDeploy { if (readCalls.length != expectedAddresses.length) { revert ResolvedAddressesLengthMismatch(readCalls.length, expectedAddresses.length); } + // Same reason: an empty read set is reported without an RPC round trip, + // so it cannot be masked by an outage on the first network. + if (readCalls.length == 0) { + revert NoResolvedAddressReads(target); + } for (uint256 i = 0; i < networks.length; i++) { // createSelectFork returns a fork id that is not needed here; bind // and reference it so the unused-return lint stays satisfied. diff --git a/test/src/lib/LibRainDeploy.t.sol b/test/src/lib/LibRainDeploy.t.sol index 23d302c..83fac19 100644 --- a/test/src/lib/LibRainDeploy.t.sol +++ b/test/src/lib/LibRainDeploy.t.sol @@ -972,6 +972,39 @@ contract LibRainDeployTest is Test { this.externalCheckResolvedAddresses("test_network", address(this), readCalls, expectedAddresses); } + /// A check with no reads MUST be refused rather than pass. It is the same + /// hazard `NoNetworks` covers one argument along: an empty read set is + /// indistinguishable from every read checking out, and it is what a + /// consumer that built its read list from an empty config hands in — right + /// before it migrates onto the deployment this was supposed to verify. + function testCheckResolvedAddressesNoReadsReverts(address target) external { + vm.expectRevert(abi.encodeWithSelector(LibRainDeploy.NoResolvedAddressReads.selector, target)); + this.externalCheckResolvedAddresses("test_network", target, new bytes[](0), new address[](0)); + } + + /// The refusal is about the READS, not about the pairing: an empty pair is + /// the one case a length mismatch cannot see, which is why it is its own + /// error. Empty reads against a non-empty expected list stays a mismatch, + /// pinning that pairing is still checked first, and a single real read + /// still passes, so emptiness alone is what is refused. + function testCheckResolvedAddressesEmptyPairIsNotALengthMismatch(bytes32 name, address account) external { + vm.assume(account != address(0)); + (, MockResolvedOwner consumer) = deployRegistryAndConsumer(name, account); + + vm.expectRevert(abi.encodeWithSelector(LibRainDeploy.NoResolvedAddressReads.selector, address(consumer))); + this.externalCheckResolvedAddresses("test_network", address(consumer), new bytes[](0), new address[](0)); + + // Empty reads against a non-empty expected list is a mispairing, and is + // still reported as one. + vm.expectRevert( + abi.encodeWithSelector(LibRainDeploy.ResolvedAddressesLengthMismatch.selector, uint256(0), uint256(1)) + ); + this.externalCheckResolvedAddresses("test_network", address(consumer), new bytes[](0), expected(account)); + + // One read still passes, so the refusal is about emptiness alone. + LibRainDeploy.checkResolvedAddresses("test_network", address(consumer), ownerReadCalls(), expected(account)); + } + /// `checkResolvedAddressesOnNetworks` MUST revert with `NoNetworks` when /// given none, so an empty target set can never be mistaken for every read /// checking out. @@ -998,6 +1031,17 @@ contract LibRainDeployTest is Test { this.externalCheckResolvedAddressesOnNetworks(networks, address(this), readCalls, expectedAddresses); } + /// And an empty read set is refused before any network is forked too, so it + /// is reported without an RPC round trip and cannot be masked by an outage. + function testCheckResolvedAddressesOnNetworksNoReadsRevertsBeforeForking() external { + string[] memory networks = new string[](1); + // Not a configured RPC alias, so forking it is itself an error. + networks[0] = "unconfigured_network"; + + vm.expectRevert(abi.encodeWithSelector(LibRainDeploy.NoResolvedAddressReads.selector, address(this))); + this.externalCheckResolvedAddressesOnNetworks(networks, address(this), new bytes[](0), new address[](0)); + } + /// `checkResolvedAddressesOnNetworks` MUST fork each network in turn and /// pass when the deployed contract holds the expected address on all of /// them. The deployment is made persistent so the same contract is present