From deff81e89a23cdba814c9e8f43ec791ebf57cd63 Mon Sep 17 00:00:00 2001 From: David Meister Date: Sat, 15 Aug 2026 12:09:28 +0000 Subject: [PATCH 1/2] test: drive run() through a successful broadcast Co-Authored-By: Claude Opus 5 (1M context) --- test/concrete/ExampleDeploySingleNetwork.sol | 33 ++++++ test/src/abstract/RainDeployBroadcast.t.sol | 100 ++++++++++++++++++- 2 files changed, 128 insertions(+), 5 deletions(-) create mode 100644 test/concrete/ExampleDeploySingleNetwork.sol diff --git a/test/concrete/ExampleDeploySingleNetwork.sol b/test/concrete/ExampleDeploySingleNetwork.sol new file mode 100644 index 0000000..f574522 --- /dev/null +++ b/test/concrete/ExampleDeploySingleNetwork.sol @@ -0,0 +1,33 @@ +// SPDX-License-Identifier: LicenseRef-DCL-1.0 +// SPDX-FileCopyrightText: Copyright (c) 2020 Rain Open Source Software Ltd +pragma solidity =0.8.25; + +import {RainDeployBroadcast} from "../../src/abstract/RainDeployBroadcast.sol"; +import {LibRainDeploy} from "../../src/lib/LibRainDeploy.sol"; +import {ExampleDeploySuites} from "../abstract/ExampleDeploySuites.sol"; + +/// @title ExampleDeploySingleNetwork +/// A repo that bootstraps one chain per dispatch, which is the case +/// `deployNetworks()` is overridable FOR — `st0x.deploy` selects between +/// Ethereum and HyperEVM per dispatch rather than fanning out to everything. +/// +/// The same declaration as `ExampleDeploy` and a different target set, so the +/// only thing a broadcast through this fixture can differ in is where it went. +/// A run that reached a network this does not name is a suite deployed +/// somewhere the repo did not ask for, and the override being ignored is not +/// observable any other way: `deployNetworks()` returning the right list says +/// nothing about `run()` consulting it. +/// +/// ARBITRUM rather than any other alias, deliberately. It is the network +/// `supportedNetworks()` lists FIRST and this returns ONLY, and +/// `deployToNetworks` forks each network in turn without restoring — so the +/// chain left selected after a completed run is arbitrum here and polygon for +/// the default, which is the difference an assertion can see. A single-element +/// override naming polygon would end on polygon either way and assert nothing. +contract ExampleDeploySingleNetwork is ExampleDeploySuites, RainDeployBroadcast { + /// @inheritdoc RainDeployBroadcast + function deployNetworks() internal pure override returns (string[] memory networks) { + networks = new string[](1); + networks[0] = LibRainDeploy.ARBITRUM_ONE; + } +} diff --git a/test/src/abstract/RainDeployBroadcast.t.sol b/test/src/abstract/RainDeployBroadcast.t.sol index 1c2c178..5219924 100644 --- a/test/src/abstract/RainDeployBroadcast.t.sol +++ b/test/src/abstract/RainDeployBroadcast.t.sol @@ -7,17 +7,37 @@ import {Test} from "forge-std-1.16.1/src/Test.sol"; import {UnknownDeploymentSuite} from "../../../src/abstract/RainDeploySuitesBase.sol"; import {LibRainDeploy} from "../../../src/lib/LibRainDeploy.sol"; import {ExampleDeploy} from "../../concrete/ExampleDeploy.sol"; +import {ExampleDeploySingleNetwork} from "../../concrete/ExampleDeploySingleNetwork.sol"; +import {MockDeployableV2} from "../../concrete/MockDeployableV2.sol"; /// @title RainDeployBroadcastTest /// @notice The broadcast entry point, driven exactly as the `Manual sol /// artifacts` workflow drives it — through `DEPLOYMENT_SUITE` and /// `DEPLOYMENT_KEY` env vars. /// -/// Nothing here broadcasts. Every case is one that fails before -/// `deployAndBroadcast` is reached, which is the half of `run()` that can be -/// tested without a key, an RPC and real money — and, not coincidentally, the -/// half that decides WHAT would be deployed. +/// Both halves of `run()` are driven here: the selection, through the cases +/// that fail before `deployAndBroadcast` is reached, and the broadcast itself, +/// on a fork. +/// +/// The broadcast half is not the hazard it sounds like, and this file used to +/// say it was. `vm.startBroadcast` under `forge test` executes the deploy on +/// the fork and records a transaction it never sends, so a success case here +/// costs a test key that has never held anything, the same public RPC every +/// other fork test uses, and no money — which is exactly what the sibling +/// `LibRainDeployTest` has always relied on to drive this same library through +/// this same path. Leaving it untested did not avoid a risk; it left the org's +/// only broadcast entry point, inherited by every downstream deploy repo, +/// asserting nothing about what it would deploy or where. contract RainDeployBroadcastTest is Test { + /// Chain id of Arbitrum One, which is what `deployNetworks()` being honoured + /// looks like from inside a completed run. + /// + /// The literal rather than a second fork of the same alias to compare + /// against: an `arbitrum` RPC alias misconfigured onto some other chain + /// would agree with itself and quietly assert nothing, while a fact of the + /// world fails loudly. + uint256 constant ARBITRUM_ONE_CHAIN_ID = 42161; + ExampleDeploy internal sDeploy; /// A deploy repo's whole script: the fixture declaration plus @@ -74,7 +94,19 @@ contract RainDeployBroadcastTest is Test { /// asserted the ordering only in a bare shell — and a key that PARSES makes /// both orderings produce the same revert, so it would assert nothing about /// ordering in the one place the suite actually runs. - function testRunSelectsTheSuiteFromTheEnvBeforeTheKeyAndNeverDefaults() external { + /// + /// ## And then the suite it names, actually broadcast + /// + /// The two legs above decide WHAT would be deployed. They say nothing about + /// the nine lines that carry that decision to a chain, and those nine lines + /// are the entire body of `script/Deploy.sol`. A third leg runs `run()` all + /// the way through on a fork, so that the selected suite's fields, and the + /// target set they go to, are observed rather than assumed. + /// + /// Here rather than in a test of its own for the reason the first two are + /// here: this leg has to SET `DEPLOYMENT_SUITE`, and a second test doing + /// that is a second test racing the first over one process-global variable. + function testRunSelectsTheSuiteFromTheEnvBeforeTheKeyNeverDefaultsAndBroadcastsIt() external { // `DEPLOYMENT_SUITE` has to be ABSENT and no cheatcode makes it so, so // the precondition is asserted: a value set outside this test reports // itself by name here rather than as a surprising revert payload. @@ -103,6 +135,64 @@ contract RainDeployBroadcastTest is Test { ) ); sDeploy.run(); + + // ## Then the suite it names, actually broadcast + // + // A fixture that names ONE network, so that where the broadcast went is + // observable at all. `sDeploy` takes the default target set, and a suite + // deployed to all five chains and a suite deployed to the one chain the + // repo asked for are indistinguishable from a fixture that asks for all + // five. + ExampleDeploySingleNetwork single = new ExampleDeploySingleNetwork(); + + // Derived here from the same source the declaration derives them from, + // not pinned as literals: both are a function of the compiler settings, + // so a pinned copy would be a second thing to keep in step with + // `foundry.toml` and would fail as a stale constant rather than as the + // wiring defect this leg is about. + address expectedAddress = LibRainDeploy.zoltuAddress(type(MockDeployableV2).creationCode); + bytes32 expectedCodeHash = keccak256(type(MockDeployableV2).runtimeCode); + + vm.setEnv("DEPLOYMENT_SUITE", "second-address-candidate"); + // Parseable, unlike the value above, because this leg has to get PAST + // the key. `vm.rememberKey` takes it and the deploy is broadcast as the + // address it derives; the key itself is a test constant that has never + // transacted anywhere. + vm.setEnv("DEPLOYMENT_KEY", "0xa11ce"); + + single.run(); + + // The suite `DEPLOYMENT_SUITE` NAMED is the suite on chain: at the + // address its own creation code derives, holding the code hash that + // creation code produces. + // + // This is what says the SELECTED entry reached `deployAndBroadcast` + // rather than some other entry of the registry, and it says it because + // of which entry it is. `MockDeployableV2` is a mock that exists on no + // supported chain, so this address has code only because this run put + // it there. The other address the declaration carries is + // `AddressRegistry`, which this repo really has deployed, so a `run()` + // that ignored the selection and broadcast that instead would take the + // already-deployed skip branch and SUCCEED — leaving this assertion the + // only thing between the two outcomes. + assertGt(expectedAddress.code.length, 0); + assertEq(expectedAddress.codehash, expectedCodeHash); + + // The OVERRIDE is what was broadcast to, rather than + // `LibRainDeploy.supportedNetworks()` reached past it. + // + // `deployToNetworks` forks each network in turn and never restores, so a + // completed run leaves the LAST network of `deployNetworks()` selected: + // arbitrum for this fixture's single-element override, polygon for the + // default. That is the whole of the difference an assertion can see, and + // an override `run()` ignored is a repo that asked for one chain getting + // a suite on five — with a revert partway through leaving a dispatch + // half done. + // + // `testDeployNetworksDefaultsToSupportedNetworks` asserts the default + // VALUE of that function; nothing until here asserted that `run()` is + // what consults it. + assertEq(block.chainid, ARBITRUM_ONE_CHAIN_ID); } /// The default target set MUST be every supported network, so a From 4fc03da615ceaf196967465267c8be57a14bff62 Mon Sep 17 00:00:00 2001 From: David Meister Date: Sat, 15 Aug 2026 12:12:24 +0000 Subject: [PATCH 2/2] test: comments describe current behaviour, and the setEnv count follows the file Co-Authored-By: Claude Opus 5 (1M context) --- test/src/abstract/RainDeployBroadcast.t.sol | 25 ++++++++++----------- 1 file changed, 12 insertions(+), 13 deletions(-) diff --git a/test/src/abstract/RainDeployBroadcast.t.sol b/test/src/abstract/RainDeployBroadcast.t.sol index 5219924..281ed0f 100644 --- a/test/src/abstract/RainDeployBroadcast.t.sol +++ b/test/src/abstract/RainDeployBroadcast.t.sol @@ -19,15 +19,14 @@ import {MockDeployableV2} from "../../concrete/MockDeployableV2.sol"; /// that fail before `deployAndBroadcast` is reached, and the broadcast itself, /// on a fork. /// -/// The broadcast half is not the hazard it sounds like, and this file used to -/// say it was. `vm.startBroadcast` under `forge test` executes the deploy on -/// the fork and records a transaction it never sends, so a success case here -/// costs a test key that has never held anything, the same public RPC every -/// other fork test uses, and no money — which is exactly what the sibling -/// `LibRainDeployTest` has always relied on to drive this same library through -/// this same path. Leaving it untested did not avoid a risk; it left the org's -/// only broadcast entry point, inherited by every downstream deploy repo, -/// asserting nothing about what it would deploy or where. +/// The broadcast half costs no key custody and no money. `vm.startBroadcast` +/// under `forge test` executes the deploy against the fork and records a +/// transaction it never sends, so the whole of it is a test key that has never +/// held anything and the same RPC alias every other fork test in this repo +/// forks — which is what the sibling `LibRainDeployTest` drives this same +/// library through this same path on. Without it, the org's only broadcast +/// entry point, inherited by every downstream deploy repo, asserts nothing +/// about what it would deploy or where. contract RainDeployBroadcastTest is Test { /// Chain id of Arbitrum One, which is what `deployNetworks()` being honoured /// looks like from inside a completed run. @@ -56,10 +55,10 @@ contract RainDeployBroadcastTest is Test { /// holding two values for one variable is two tests racing over one /// variable, and this contract was exactly that until it was seen losing /// the race: the unset case resolved the `address-registry` the other test - /// had written. Sequenced inside one test the two values cannot interleave, - /// and every `vm.setEnv` in this repo is now one of the two below — with - /// the write to `DEPLOYMENT_SUITE` after the only read that needs it - /// absent, so there is no write left for another test to observe. + /// had written. Sequenced inside one test the values cannot interleave, and + /// every `vm.setEnv` in this repo is one of the four below — with every + /// write to `DEPLOYMENT_SUITE` after the only read that needs it absent, so + /// there is no write left for another test to observe. /// /// ## Unset first, and genuinely unset ///