From 0c5d1204460639b1588a2638e8cc9b658ad4df06 Mon Sep 17 00:00:00 2001 From: baku-ccron Date: Sun, 20 Sep 2026 11:52:46 +0000 Subject: [PATCH 1/3] test: a candidate that names another contract is refused (RED) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two tests, both failing against unmodified source, for rainlanguage/rain.factory.deploy#34. `MisanchoredDeploySuites` records a consistent snapshot of `MockDeployableV2` under an `artifactPath` that names `MockDeployable`, and wires `sourceCreationCode` at the same recorded bytes — the exact mutation shape the issue found surviving a consumer's whole suite. The declaration is internally silent: every check internal to a snapshot passes on it. Today `checkCandidatesAnchoredToSource` compares that value with itself, so: [FAIL: next call did not revert as expected] testCandidateThatNamesAnotherContractIsRefused() [FAIL: Error != expected error: UnknownDeploymentSuite("", "misanchored-candidate") != CandidateSourceMismatch("misanchored-candidate", ...)] testRunRefusesToBroadcastACandidateThatNamesAnotherContract() The second is the one that matters: `run()` got PAST the anchor and failed later, at suite selection, which is what an anchor that does not run on the irreversible path looks like from inside a completed dispatch. The discriminating controls pass in the same run — `testCandidatesPresentAnswers` (a declaration whose candidates really are their named contracts) and `testRunRefusesToBroadcastACandidateThatIsNotItsSource` (a declaration that contradicts itself). Co-Authored-By: Claude Opus 5 (1M context) --- test/abstract/MisanchoredDeploySuites.sol | 65 ++++++++++++++++++++ test/concrete/MisanchoredDeploy.sol | 20 ++++++ test/src/abstract/RainDeployBroadcast.t.sol | 38 ++++++++++++ test/src/abstract/RainDeploySuitesBase.t.sol | 47 ++++++++++++++ 4 files changed, 170 insertions(+) create mode 100644 test/abstract/MisanchoredDeploySuites.sol create mode 100644 test/concrete/MisanchoredDeploy.sol diff --git a/test/abstract/MisanchoredDeploySuites.sol b/test/abstract/MisanchoredDeploySuites.sol new file mode 100644 index 0000000..b60f7f9 --- /dev/null +++ b/test/abstract/MisanchoredDeploySuites.sol @@ -0,0 +1,65 @@ +// SPDX-License-Identifier: LicenseRef-DCL-1.0 +// SPDX-FileCopyrightText: Copyright (c) 2020 Rain Open Source Software Ltd +pragma solidity ^0.8.25; + +import {DeployCandidate, DeploySuite, RainDeploySuitesBase} from "../../src/abstract/RainDeploySuitesBase.sol"; +import {LibRainDeploy} from "../../src/lib/LibRainDeploy.sol"; +import {MockDeployableV2} from "../concrete/MockDeployableV2.sol"; + +/// @title MisanchoredDeploySuites +/// @notice A declaration that says it deploys one contract and records another, +/// with NOTHING in the declaration to say so. +/// +/// The single candidate names `MockDeployable` as the contract it is a snapshot +/// of — that is what `artifactPath` is, the `:` the explorer +/// verification command is run with — and every recorded field is +/// `MockDeployableV2`'s. The snapshot is perfectly consistent with itself: the +/// address is the one its recorded creation code derives, the code hash is the +/// one that creation code produces, and the runtime code hashes to it. So every +/// check internal to a snapshot passes, and the only claim left to check is the +/// one the anchor makes: that the record is what the named contract COMPILES TO. +/// +/// This is the declaration the source anchor has to be able to refuse without +/// being handed the source by the thing it is checking. `SourceMismatchDeploy` +/// is its sibling and is NOT the same fixture: there the declaration itself +/// supplies the contradiction, so an anchor that believed the declaration would +/// still catch it. Here the declaration is internally silent, and the only +/// origin left that is not the declaration's own word is the compiler's +/// artifact for the contract the candidate names. +/// +/// ONE candidate, and no releases. The loop-reaches-every-candidate property is +/// `SourceMismatchDeploySuites`' subject and is pinned there; this fixture is +/// about where the anchor's SOURCE operand comes from, which a single candidate +/// says with nothing else in the way. The key is its own, shared with no other +/// fixture, because `DEPLOYMENT_SUITE` is a process-wide variable other tests +/// write. +abstract contract MisanchoredDeploySuites is RainDeploySuitesBase { + /// @inheritdoc RainDeploySuitesBase + function releasedSuites() internal pure override returns (DeploySuite[] memory suites) { + suites = new DeploySuite[](0); + } + + /// @inheritdoc RainDeploySuitesBase + function candidateSuites() internal pure override returns (DeployCandidate[] memory candidates) { + candidates = new DeployCandidate[](1); + candidates[0] = DeployCandidate({ + snapshot: DeploySuite({ + suite: "misanchored-candidate", + creationCode: type(MockDeployableV2).creationCode, + storedDeployedAddress: LibRainDeploy.zoltuAddress(type(MockDeployableV2).creationCode), + storedBytecodeHash: keccak256(type(MockDeployableV2).runtimeCode), + storedRuntimeCode: type(MockDeployableV2).runtimeCode, + // NOT `MockDeployableV2`. The candidate claims to be a snapshot + // of `MockDeployable`, and records the other contract. + artifactPath: "test/concrete/MockDeployable.sol:MockDeployable", + dependencies: new address[](0) + }), + // The exemption, spelled: both operands of the anchor are the + // RECORD, so the comparison is a value against itself and is + // satisfied for any candidate whatsoever. This is the mutation + // rainlanguage/rain.factory.deploy#34 found surviving a consumer's + // whole suite. + sourceCreationCode: type(MockDeployableV2).creationCode + }); + } +} diff --git a/test/concrete/MisanchoredDeploy.sol b/test/concrete/MisanchoredDeploy.sol new file mode 100644 index 0000000..f272a1d --- /dev/null +++ b/test/concrete/MisanchoredDeploy.sol @@ -0,0 +1,20 @@ +// 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 {ExternalDeploySuites} from "../abstract/ExternalDeploySuites.sol"; +import {MisanchoredDeploySuites} from "../abstract/MisanchoredDeploySuites.sol"; + +/// @title MisanchoredDeploy +/// A deploy repo's whole script over a declaration that names one contract and +/// records another. +/// +/// A real script rather than a declaration fixture, for the reason +/// `SourceMismatchDeploy` is one: the claim under test is that the anchor holds +/// on the path that BROADCASTS. A declaration that could only be driven through +/// the external wrappers would leave `run()` — the irreversible, multi-chain, +/// key-custody action — asserted about by nothing, and `run()` is exactly where +/// a neutered anchor costs a permanent `CREATE2` address on every chain the +/// dispatch reached. +contract MisanchoredDeploy is MisanchoredDeploySuites, ExternalDeploySuites, RainDeployBroadcast {} diff --git a/test/src/abstract/RainDeployBroadcast.t.sol b/test/src/abstract/RainDeployBroadcast.t.sol index 71d86f8..9f1de12 100644 --- a/test/src/abstract/RainDeployBroadcast.t.sol +++ b/test/src/abstract/RainDeployBroadcast.t.sol @@ -8,6 +8,7 @@ import {CandidateSourceMismatch, UnknownDeploymentSuite} from "../../../src/abst import {LibRainDeploy} from "../../../src/lib/LibRainDeploy.sol"; import {ExampleDeploy} from "../../concrete/ExampleDeploy.sol"; import {ExampleDeploySingleNetwork} from "../../concrete/ExampleDeploySingleNetwork.sol"; +import {MisanchoredDeploy} from "../../concrete/MisanchoredDeploy.sol"; import {SourceMismatchDeploy} from "../../concrete/SourceMismatchDeploy.sol"; import {StalePinDeploy, STALE_PIN_ADDRESS} from "../../concrete/StalePinDeploy.sol"; import {MissingDependencyDeploy, ABSENT_DEPENDENCY} from "../../concrete/MissingDependencyDeploy.sol"; @@ -407,6 +408,43 @@ contract RainDeployBroadcastTest is Test { mismatch.run(); } + /// `run()` MUST refuse a candidate that records a contract other than the + /// one it NAMES, even when the declaration itself says nothing is wrong. + /// + /// The sibling test above hands `run()` a declaration that contradicts + /// itself, so an anchor that trusted the declaration would still catch it. + /// This one does not: `MisanchoredDeploySuites` records a consistent + /// snapshot of `MockDeployableV2`, names `MockDeployable` as the contract + /// it is a snapshot of, and — in the shape + /// rainlanguage/rain.factory.deploy#34 found surviving a consumer's whole + /// suite — would offer the anchor its own recorded bytes as the source side + /// if the anchor were willing to take them. Everything internal to the + /// snapshot agrees with everything else. + /// + /// Asserted on `run()` and not only through the external wrapper, because + /// this is the reason the anchor lives on the declaration at all. A guard + /// the irreversible action does not run is not a guard: broadcasting is + /// `workflow_dispatch` on a ref with no required-green gate, and `CREATE2` + /// at a zero salt puts the wrong bytes at their own permanent address on + /// every chain the dispatch reached. + /// + /// The anchor is reached before `DEPLOYMENT_SUITE` resolves and before + /// `DEPLOYMENT_KEY` is read, which is why no env var is written here and + /// why the revert that arrives is the anchor's rather than the selection's. + function testRunRefusesToBroadcastACandidateThatNamesAnotherContract() external { + MisanchoredDeploy misanchored = new MisanchoredDeploy(); + + vm.expectRevert( + abi.encodeWithSelector( + CandidateSourceMismatch.selector, + "misanchored-candidate", + keccak256(type(MockDeployableV2).creationCode), + keccak256(type(MockDeployable).creationCode) + ) + ); + misanchored.run(); + } + /// The default target set MUST be every supported network, so a /// deterministic deployment reaches one address on every chain from one /// dispatch and no repo restates the list. diff --git a/test/src/abstract/RainDeploySuitesBase.t.sol b/test/src/abstract/RainDeploySuitesBase.t.sol index ece036e..8ef6907 100644 --- a/test/src/abstract/RainDeploySuitesBase.t.sol +++ b/test/src/abstract/RainDeploySuitesBase.t.sol @@ -5,6 +5,7 @@ pragma solidity =0.8.25; import {Test} from "forge-std-1.16.2/src/Test.sol"; import { + CandidateSourceMismatch, DeploySuite, DuplicateDeploySuite, InvalidDeploySuiteKey, @@ -15,10 +16,12 @@ import {ExampleDeploy} from "../../concrete/ExampleDeploy.sol"; import {CollidingCandidateDeploySuites} from "../../concrete/CollidingCandidateDeploySuites.sol"; import {DuplicateDeploySuites} from "../../concrete/DuplicateDeploySuites.sol"; import {EmptyKeyDeploySuites} from "../../concrete/EmptyKeyDeploySuites.sol"; +import {MisanchoredDeploy} from "../../concrete/MisanchoredDeploy.sol"; import {NoCandidateDeploySuites} from "../../concrete/NoCandidateDeploySuites.sol"; import {SameLengthKeyDeploySuites} from "../../concrete/SameLengthKeyDeploySuites.sol"; import {SeparatorKeyDeploySuites} from "../../concrete/SeparatorKeyDeploySuites.sol"; import {ShortestKeyDeploySuites} from "../../concrete/ShortestKeyDeploySuites.sol"; +import {MockDeployable} from "../../concrete/MockDeployable.sol"; import {MockDeployableV2} from "../../concrete/MockDeployableV2.sol"; import {LibRainDeploy} from "../../../src/lib/LibRainDeploy.sol"; @@ -262,6 +265,50 @@ contract RainDeploySuitesBaseTest is Test { sSuites.externalCheckCandidatesAnchoredToSource(); } + /// A candidate whose record is not what the contract it NAMES compiles to + /// MUST be refused, and no field of the declaration may be able to say + /// otherwise. + /// + /// The anchor is the only check that catches a snapshot of the wrong + /// contract, so its source operand cannot be something the declaration + /// hands it. A declaration that supplies BOTH operands can satisfy the + /// anchor by construction: point the source side at the same recorded bytes + /// and the comparison is a value against itself — green for any candidate + /// whatsoever, including a snapshot of an entirely different contract, and + /// green on the broadcast path as well as in CI. That is not a hypothetical + /// spelling. It is the mutation rainlanguage/rain.factory.deploy#34 found + /// SURVIVING a consumer's whole suite. + /// + /// `MisanchoredDeploySuites` is that declaration, and it is internally + /// SILENT about the contradiction. Its snapshot is consistent with itself, + /// its recorded bytes are the current compilation of a contract this repo + /// really has, and nothing it declares disagrees with anything else it + /// declares. The only thing that says it is `MockDeployableV2`'s snapshot + /// wearing `MockDeployable`'s name is the compiler's artifact for the + /// contract the candidate names — an origin the declaration does not own. + /// + /// The reported source hash is asserted, not merely the refusal. It is + /// `MockDeployable`'s, the contract the candidate NAMES, which is what says + /// the operand was read from that contract's artifact rather than from the + /// recorded bytes the declaration offered for it. + /// + /// `testCandidatesPresentAnswers` above is the discriminating case: a + /// declaration whose candidates really are snapshots of the contracts they + /// name passes this same call. + function testCandidateThatNamesAnotherContractIsRefused() external { + MisanchoredDeploy misanchored = new MisanchoredDeploy(); + + vm.expectRevert( + abi.encodeWithSelector( + CandidateSourceMismatch.selector, + "misanchored-candidate", + keccak256(type(MockDeployableV2).creationCode), + keccak256(type(MockDeployable).creationCode) + ) + ); + misanchored.externalCheckCandidatesAnchoredToSource(); + } + /// A key is the whole string, not its length. Two DIFFERENT keys of the /// SAME length MUST be two suites: both declared, each selecting its own /// record, and a third key of that length still unknown. From 60588d4474c0b22dd7f517f291e9ae661a1f7f90 Mon Sep 17 00:00:00 2001 From: baku-ccron Date: Sun, 20 Sep 2026 12:13:46 +0000 Subject: [PATCH 2/3] fix: read the candidate source anchor from the compiler, not the declaration MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes rainlanguage/rain.factory.deploy#34. `checkCandidatesAnchoredToSource` is documented as the ONLY check that catches a snapshot of the wrong contract, and as the one guard the irreversible broadcast path cannot be run past. It took both of its operands from the declaration: `snapshot.creationCode` for the record and a `DeployCandidate.sourceCreationCode` field for the source. An ordinary field is one a consumer fills in, so pointing it at the same generated constant as the record made the anchor compare a value with itself — satisfied by construction for any candidate at all, including a snapshot of an entirely different contract, with a consumer's whole suite green and `RainDeployBroadcast.run()` running the same neutered definition before it broadcast. The source operand now comes from the COMPILER. `DeployCandidate` loses `sourceCreationCode`, and the anchor resolves the candidate's own `snapshot.artifactPath` — the `:` artifact id that already existed, and that `forge verify-contract` already takes — through `vm.getCode`. A consumer names the contract; what that contract compiles to is not something it gets a say in, so there is no longer an operand with which to spell the exemption. `StdConstants.VM` rather than inheriting `CommonBase`, because `RainDeploySuitesBase` is inherited alongside `Script` and `Test`, which already declare `vm`. A library constant is zero inheritance change for every consumer. Reading the compiler at all makes the check `internal view` rather than `internal pure`, and `testSnapshotMatchesSource` `external view` with it. `script/Build.sol` writes its snapshots from `vm.getCode(candidate.snapshot.artifactPath)` as well, so generating and then checking is one claim rather than two spellings of it. Verified: `forge script script/Build.sol` over a clean tree is still a no-op. `artifactPath` becomes load-bearing for a candidate. A path that resolves to no artifact, or to more than one, now fails inside the cheatcode at the anchor — which for a candidate is BEFORE the broadcast. Previously that field was read only by the verification command `LibRainDeploy` prints after a deploy, so a path left behind by a moved or renamed source file cost a deploy before it cost a test. ONE test is deleted, deliberately: `RegistryDeploySuitesTest.testCandidatesAnchorAgainstCurrentSource`, an AST assertion that every candidate's `sourceCreationCode` was spelled `type(X).creationCode`. Its subject field no longer exists, so the property is unspellable rather than unchecked. Its sole helper `expressionShape` goes with it. The mirror assertion `testCandidatesRecordTheGeneratedConstants` — the half a declaration CAN still get wrong — is untouched and still runs. Suite: 490 passed before, 491 after (+2 new, -1 deleted), with the same 76 RPC-env failures in both, identical line for line. Co-Authored-By: Claude Opus 5 (1M context) --- README.md | 30 +++- script/Build.sol | 15 +- src/abstract/RainDeploySuitesBase.sol | 132 ++++++++++++++---- src/abstract/RainDeployVerifySnapshotBase.sol | 10 +- src/abstract/RegistryDeploySuites.sol | 23 +-- test/abstract/ExampleDeploySuites.sol | 6 +- test/abstract/ExternalDeploySuites.sol | 6 +- test/abstract/MisanchoredDeploySuites.sol | 34 +++-- test/abstract/SourceMismatchDeploySuites.sol | 24 ++-- .../CollidingCandidateDeploySuites.sol | 6 +- test/concrete/DuplicateDeploySuites.sol | 3 +- test/concrete/EmptyKeyDeploySuites.sol | 3 +- test/concrete/MissingDependencyDeploy.sol | 3 +- test/concrete/MultiSuiteDeploy.sol | 6 +- test/concrete/SameLengthKeyDeploySuites.sol | 3 +- test/concrete/SeparatorKeyDeploySuites.sol | 3 +- test/concrete/ShortestKeyDeploySuites.sol | 3 +- test/concrete/StaleCodeHashDeploy.sol | 3 +- test/concrete/StalePinDeploy.sol | 3 +- .../RainDeployVerifyChainCandidate.t.sol | 3 +- .../abstract/RainDeployVerifyChainEmpty.t.sol | 3 +- .../RainDeployVerifySnapshotBase.t.sol | 17 ++- ...ainDeployVerifySnapshotBaseCandidate.t.sol | 3 +- test/src/abstract/RegistryDeploySuites.t.sol | 71 +++------- test/src/lib/GeneratedSnapshotShape.t.sol | 58 ++++---- 25 files changed, 265 insertions(+), 206 deletions(-) diff --git a/README.md b/README.md index f0b0b54..2a964e9 100644 --- a/README.md +++ b/README.md @@ -132,13 +132,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, 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 | +| Group | Anchored to | Catches | Cannot catch | +| -------- | -------------------------- | ----------------------------------------- | -------------------------------- | +| Internal | the recorded set | an inconsistently generated set | a snapshot of the wrong contract | +| Source | `vm.getCode(artifactPath)` | 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 @@ -148,6 +148,22 @@ to have diverged from current source, so anchoring one to source asserts something false by design. That is a property of the assertion, and there is no field on a released version with which to opt in or out. +Neither is there a field on a CANDIDATE with which to satisfy it. A candidate +names the contract it is a snapshot of, in its `artifactPath`, and that is the +whole of what it says about its source; the anchor resolves that `:` +through `vm.getCode` and compares the record against what the compiler's own +artifact holds. A declaration that supplied the source side as a value could +point it at the same generated constant as the record and make the one check +that catches a snapshot of the wrong contract compare a value with itself — +green for any candidate whatsoever, on the broadcast path as well as in CI. It +used to be able to (rainlanguage/rain.factory.deploy#34); the field is gone. + +`artifactPath` is therefore LOAD-BEARING on a candidate. It must resolve, +uniquely, to the contract the snapshot is of. A path left behind by a moved or +renamed source file now fails at the anchor — before the broadcast — where it +previously only produced a `forge verify-contract` line a human read after the +deploy. + It runs over EVERY candidate, and a declaration that names none at all is refused with `NoDeployCandidates` rather than passed as a loop with nothing in it. A candidate the source anchor never reaches is a contract whose snapshot diff --git a/script/Build.sol b/script/Build.sol index dcb1f6c..5dd1b8f 100644 --- a/script/Build.sol +++ b/script/Build.sol @@ -15,9 +15,9 @@ struct GeneratedContract { string contractName; /// Prefix for the constants the alias lib exports, e.g. `ADDRESS_REGISTRY`. string constantPrefix; - /// Snapshots are written from its `sourceCreationCode` and - /// `snapshot.dependencies`; the released lib takes its suite key and - /// artifact path from its `snapshot`. + /// Snapshots are written from what its `snapshot.artifactPath` currently + /// compiles to, plus `snapshot.dependencies`; the released lib takes its + /// suite key and artifact path from its `snapshot`. DeployCandidate candidate; } @@ -90,6 +90,13 @@ contract Build is BuildScript, RegistryDeploySuites { } /// @inheritdoc BuildScript + /// @dev The bytes written are what the candidate's own `artifactPath` + /// compiles to RIGHT NOW, resolved through `vm.getCode` — the same origin, + /// read the same way, that `checkCandidatesAnchoredToSource` will hold the + /// written snapshot against. Regenerating and then checking is therefore + /// one claim rather than two spellings of it, and a declaration whose path + /// names the wrong contract writes a snapshot that goes red on the next + /// run instead of a snapshot nothing disagrees with. function regenerateSnapshots() internal override { GeneratedContract[] memory contracts = generatedContracts(); for (uint256 i = 0; i < contracts.length; i++) { @@ -98,7 +105,7 @@ contract Build is BuildScript, RegistryDeploySuites { recordRoot(), LibRainDeploySnapshot.CANDIDATE, contracts[i].contractName, - contracts[i].candidate.sourceCreationCode, + vm.getCode(contracts[i].candidate.snapshot.artifactPath), contracts[i].candidate.snapshot.dependencies ); } diff --git a/src/abstract/RainDeploySuitesBase.sol b/src/abstract/RainDeploySuitesBase.sol index 594ce21..812e6d8 100644 --- a/src/abstract/RainDeploySuitesBase.sol +++ b/src/abstract/RainDeploySuitesBase.sol @@ -2,6 +2,8 @@ // SPDX-FileCopyrightText: Copyright (c) 2020 Rain Open Source Software Ltd pragma solidity ^0.8.25; +import {StdConstants} from "forge-std-1.16.2/src/StdConstants.sol"; + /// Thrown when two suites share a key. The key selects what gets broadcast, so /// a duplicate makes the selection ambiguous and one of the two unreachable. /// @param suite The key declared more than once. @@ -81,8 +83,9 @@ error NoDeployCandidates(); /// @param suite The candidate's key. /// @param storedCreationCodeHash Hash of the creation code the candidate /// records. -/// @param sourceCreationCodeHash Hash of `type(X).creationCode` for the -/// contract the candidate claims to be. +/// @param sourceCreationCodeHash Hash of the creation code the contract the +/// candidate NAMES — its `artifactPath` — currently compiles to, read from the +/// compiler's own artifact rather than from anything the declaration says. error CandidateSourceMismatch(string suite, bytes32 storedCreationCodeHash, bytes32 sourceCreationCodeHash); /// One deployable unit: a named snapshot of one contract. @@ -119,13 +122,17 @@ struct DeploySuite { /// suite broadcasts the exact bytes its audit covered, whatever the current /// source now compiles to. /// - /// `type(X).creationCode` is what a candidate pairs this AGAINST, so - /// spelling the type expression here puts both operands of - /// `checkCandidatesAnchoredToSource` on the source side and leaves the one - /// check that catches a snapshot of the wrong contract comparing source to - /// itself, green. Fixtures that derive a whole mock suite do that on + /// What a candidate is anchored AGAINST is what `artifactPath` currently + /// compiles to, so spelling `type(X).creationCode` here puts both operands + /// of `checkCandidatesAnchoredToSource` on the source side and leaves the + /// one check that catches a snapshot of the wrong contract comparing source + /// to itself, green. Fixtures that derive a whole mock suite do that on /// purpose, because they have no record and are exercising other /// assertions; a declaration of a real deployment never does. + /// + /// This is the remaining half a declaration can get wrong, and it is the + /// half a declaration has to own: the record is the point. The SOURCE half + /// was an ordinary field until it was not — see `DeployCandidate`. bytes creationCode; /// The deploy address recorded for this suite. address storedDeployedAddress; @@ -134,11 +141,27 @@ struct DeploySuite { /// The runtime code recorded for this suite. A generated `RUNTIME_CODE` /// constant. bytes storedRuntimeCode; - /// `:`, for the explorer verification command. + /// `:`: the contract this suite is a snapshot OF. /// /// Declared rather than derived from the contract name. `src/concrete/` /// holds only the flattest repos; a repo that groups concretes into /// subdirectories has paths no naming convention recovers. + /// + /// Two things read it, and for a CANDIDATE the second is the load-bearing + /// one. `LibRainDeploy` prints it as the `forge verify-contract` command a + /// human runs against a freshly broadcast contract, and + /// `checkCandidatesAnchoredToSource` resolves it through `vm.getCode` to + /// get the creation code the named contract currently compiles to — the + /// source half of the one check that catches a snapshot of the wrong + /// contract. Naming the contract is therefore the whole of what a candidate + /// says about its source, and it is all it gets to say: what that contract + /// COMPILES TO is the compiler's answer, not the declaration's. + /// + /// A path that resolves to no artifact, or to more than one, now fails at + /// the anchor — which for a candidate is before the broadcast. It used to + /// fail nowhere in this package: the printed verification command is read + /// by a human AFTER the deploy, so a path left behind by a moved or renamed + /// source file cost a deploy before it cost a test. string artifactPath; /// Addresses that MUST already have code on a network before this suite is /// broadcast there. Ordinarily other suites' recorded addresses: a @@ -147,25 +170,40 @@ struct DeploySuite { address[] dependencies; } -/// The rolling candidate: the snapshot that tracks current source rather than a -/// frozen release, paired with the current source's creation code it MUST -/// equal. +/// The rolling candidate: a snapshot that tracks current source rather than a +/// frozen release, and that MUST equal what the contract it names currently +/// compiles to. +/// +/// That anchor is the ONLY thing that catches a snapshot of the wrong contract. +/// Every check internal to a snapshot is satisfied by a consistent snapshot of +/// the wrong thing, so without it there is nothing that says the recorded bytes +/// belong to the contract this repo compiles. /// -/// This pairing is the ONLY thing that catches a snapshot of the wrong -/// contract. Every check internal to a snapshot is satisfied by a consistent -/// snapshot of the wrong thing, so without an anchor to source there is nothing -/// that says the recorded bytes belong to the contract this repo compiles. +/// Anchoring is a property of the TYPE, not a value the declaration supplies. +/// `checkCandidatesAnchoredToSource` reads the source side out of the +/// compiler's artifact for `snapshot.artifactPath`, so a declaration has no +/// operand to hand it and therefore no way to spell an exemption. It used to +/// carry that operand as an ordinary `sourceCreationCode` field, and an +/// ordinary field is one a consumer fills in: pointing it at the same generated +/// constant as `snapshot.creationCode` left the one check that catches a +/// snapshot of the wrong contract comparing a value with itself — green for any +/// candidate whatsoever, on the broadcast path as well as in CI, with a whole +/// consumer suite passing through it (rainlanguage/rain.factory.deploy#34). /// -/// It is deliberately absent from `DeploySuite` and therefore from released -/// suites: a released tag is MEANT to diverge from current source, so anchoring -/// one to source would fail on every release that is not the newest. That is a -/// property of the assertion, not an opt-out — there is no way for a caller to -/// spell "released, and also skip the checks that do apply". +/// ONE field, deliberately, and the type is not folded back into `DeploySuite` +/// for it. The type is what separates a rolling candidate from a frozen +/// release, and that separation is the whole of what it carries: a released tag +/// is MEANT to diverge from current source, so anchoring one asserts something +/// false by design. `releasedSuites()` returns `DeploySuite[]` and +/// `candidateSuites()` returns this, so which of the two an entry is is +/// something the compiler makes a repo state rather than a flag beside the data +/// — there is no way for a caller to spell "released, and also skip the checks +/// that do apply", and none to spell "candidate, and also skip the anchor". struct DeployCandidate { - /// The candidate's own recorded snapshot, checked exactly as any other. + /// The candidate's own recorded snapshot, checked exactly as any other + /// suite is, and additionally anchored to what its `artifactPath` compiles + /// to. DeploySuite snapshot; - /// `type(X).creationCode` for the contract the candidate claims to be. - bytes sourceCreationCode; } /// @title RainDeploySuitesBase @@ -191,7 +229,8 @@ abstract contract RainDeploySuitesBase { function releasedSuites() internal pure virtual returns (DeploySuite[] memory); /// The rolling candidates — one snapshot per contract this repo compiles - /// right now, each paired with the source it MUST equal. + /// right now, each naming the contract it MUST be the current compilation + /// of. /// /// A list because a repo deploys as many contracts as it deploys, and each /// of them has its own rolling snapshot and its own source to be anchored @@ -228,7 +267,8 @@ abstract contract RainDeploySuitesBase { return candidates; } - /// EVERY candidate MUST record the creation code this repo compiles. + /// EVERY candidate MUST record the creation code the contract it NAMES + /// currently compiles to. /// /// This is the ONLY check that catches a snapshot of the wrong contract. /// Everything else a snapshot is asked is internal to the snapshot — the @@ -262,13 +302,47 @@ abstract contract RainDeploySuitesBase { /// Candidates alone, and there is no way to spell an exemption. A released /// suite is MEANT to diverge from current source — it records bytes that /// are already on chain — so anchoring one to source asserts something - /// false by design, which is why `DeploySuite` carries no source at all and - /// only `DeployCandidate` does. - function checkCandidatesAnchoredToSource() internal pure { + /// false by design, which is why only `candidateSuites()` is read here. + /// + /// ## Where the source operand comes from + /// + /// The COMPILER, through `vm.getCode` on the candidate's `artifactPath` — + /// foundry's own resolution of a `:` artifact id, the same form + /// `forge verify-contract` takes. Never from the declaration. + /// + /// A declaration that supplied both operands could satisfy this by + /// construction, and one did: `DeployCandidate` used to carry the source + /// creation code as a field, so pointing that field at the same generated + /// constant as `snapshot.creationCode` made the comparison a value against + /// itself — satisfied for any candidate, including a snapshot of an + /// entirely different contract, with a consumer's whole suite still green + /// and `RainDeployBroadcast` running the same neutered definition before it + /// broadcast (rainlanguage/rain.factory.deploy#34). "No way to spell an + /// exemption" is only true of an operand the declaration cannot reach. A + /// consumer names the contract; what that contract compiles to is not + /// something it gets a say in. + /// + /// Not derived from the contract NAME either — `artifactPath` is the whole + /// `:`, because a repo that groups its concretes into + /// subdirectories has paths no naming convention recovers, and because that + /// field already exists and is already the one thing a suite says about + /// which contract it is. + /// + /// `view` rather than `pure` follows from reading the compiler at all, and + /// costs nothing: both callers are a `Script` and a `Test`, which is the + /// only place a cheatcode exists. + /// + /// An `artifactPath` that resolves to no artifact, or to more than one, + /// reverts inside the cheatcode naming the path, before this comparison is + /// reached. That failure is new and it is the right one: the field was + /// previously read only by the verification command `LibRainDeploy` prints + /// AFTER a broadcast, so a path left behind by a moved or renamed source + /// file cost a deploy before it cost a test. + function checkCandidatesAnchoredToSource() internal view { DeployCandidate[] memory candidates = checkedCandidateSuites(); for (uint256 i = 0; i < candidates.length; i++) { bytes32 stored = keccak256(candidates[i].snapshot.creationCode); - bytes32 source = keccak256(candidates[i].sourceCreationCode); + bytes32 source = keccak256(StdConstants.VM.getCode(candidates[i].snapshot.artifactPath)); if (stored != source) { revert CandidateSourceMismatch(candidates[i].snapshot.suite, stored, source); } diff --git a/src/abstract/RainDeployVerifySnapshotBase.sol b/src/abstract/RainDeployVerifySnapshotBase.sol index c7e1ebe..27101ec 100644 --- a/src/abstract/RainDeployVerifySnapshotBase.sol +++ b/src/abstract/RainDeployVerifySnapshotBase.sol @@ -387,15 +387,19 @@ abstract contract RainDeployVerifySnapshotBase is RainDeployVerifyBase { } } - /// EVERY candidate MUST be a snapshot of the contract this repo compiles, - /// not of some other contract that happens to be internally consistent. + /// EVERY candidate MUST be a snapshot of the contract it NAMES, as this + /// repo currently compiles it, not of some other contract that happens to + /// be internally consistent. /// /// The check itself is `RainDeploySuitesBase.checkCandidatesAnchoredToSource` /// rather than anything here, because `RainDeployBroadcast` runs the same /// definition before it broadcasts. A second spelling on this side is a /// spelling the deploy does not run, which is exactly the state this test /// would otherwise be reporting green about. - function testSnapshotMatchesSource() external pure { + /// + /// `view` because that one definition reads the compiler's artifact for the + /// named contract rather than an operand the declaration hands it. + function testSnapshotMatchesSource() external view { checkCandidatesAnchoredToSource(); } } diff --git a/src/abstract/RegistryDeploySuites.sol b/src/abstract/RegistryDeploySuites.sol index 6ac6a52..721f2f5 100644 --- a/src/abstract/RegistryDeploySuites.sol +++ b/src/abstract/RegistryDeploySuites.sol @@ -3,8 +3,6 @@ pragma solidity ^0.8.25; import {DeployCandidate, DeploySuite, RainDeploySuitesBase} from "./RainDeploySuitesBase.sol"; -import {AddressRegistry} from "../concrete/AddressRegistry.sol"; -import {MigrationRegistry} from "../concrete/MigrationRegistry.sol"; import { CREATION_CODE as ADDRESS_REGISTRY_CREATION_CODE_CANDIDATE, RUNTIME_CODE as ADDRESS_REGISTRY_RUNTIME_CODE_CANDIDATE @@ -98,9 +96,14 @@ abstract contract RegistryDeploySuites is RainDeploySuitesBase { /// The creation code and runtime code are RECORDED, read from the rolling /// `src/generated/candidate/` snapshot. That is what makes the source /// anchor mean something: it compares the recorded creation code against - /// `type(AddressRegistry).creationCode`, so editing the contract without - /// re-running `script/Build.sol` fails. While nothing was recorded, that - /// check compared source against itself and could only pass. + /// whatever the `artifactPath` below currently compiles to, so editing the + /// contract without re-running `script/Build.sol` fails. While nothing was + /// recorded, that check compared source against itself and could only pass. + /// + /// Nothing here supplies the source half, and there is no field left to + /// supply it with. Naming the contract in `artifactPath` is the whole of + /// what this declaration says about its source; what that contract compiles + /// to is the compiler's answer, read out of its artifact. /// /// `AddressRegistry` reads nothing and calls nothing at construction, so it /// has no dependency that must already be on chain. @@ -122,8 +125,7 @@ abstract contract RegistryDeploySuites is RainDeploySuitesBase { storedRuntimeCode: ADDRESS_REGISTRY_RUNTIME_CODE_CANDIDATE, artifactPath: "src/concrete/AddressRegistry.sol:AddressRegistry", dependencies: new address[](0) - }), - sourceCreationCode: type(AddressRegistry).creationCode + }) }); } @@ -132,7 +134,9 @@ abstract contract RegistryDeploySuites is RainDeploySuitesBase { /// Everything said about the `AddressRegistry` candidate holds here /// unchanged: the pins are aliased from the generated snapshot, the /// creation and runtime code are recorded rather than derived, and the - /// source anchor is what says the record describes THIS contract. + /// source anchor — the record held against what this candidate's + /// `artifactPath` compiles to — is what says the record describes THIS + /// contract. /// /// `MigrationRegistry` has no constructor argument, no compile-time /// authority and no dependency to be on chain first — the namespace is @@ -150,8 +154,7 @@ abstract contract RegistryDeploySuites is RainDeploySuitesBase { storedRuntimeCode: MIGRATION_REGISTRY_RUNTIME_CODE_CANDIDATE, artifactPath: "src/concrete/MigrationRegistry.sol:MigrationRegistry", dependencies: new address[](0) - }), - sourceCreationCode: type(MigrationRegistry).creationCode + }) }); } } diff --git a/test/abstract/ExampleDeploySuites.sol b/test/abstract/ExampleDeploySuites.sol index 49e3a59..eca0846 100644 --- a/test/abstract/ExampleDeploySuites.sol +++ b/test/abstract/ExampleDeploySuites.sol @@ -83,8 +83,7 @@ abstract contract ExampleDeploySuites is RainDeploySuitesBase { storedRuntimeCode: ADDRESS_REGISTRY_RUNTIME_CODE, artifactPath: "src/concrete/AddressRegistry.sol:AddressRegistry", dependencies: new address[](0) - }), - sourceCreationCode: type(AddressRegistry).creationCode + }) }); candidates[1] = DeployCandidate({ snapshot: DeploySuite({ @@ -95,8 +94,7 @@ abstract contract ExampleDeploySuites is RainDeploySuitesBase { storedRuntimeCode: type(MockDeployableV2).runtimeCode, artifactPath: "test/concrete/MockDeployableV2.sol:MockDeployableV2", dependencies: new address[](0) - }), - sourceCreationCode: type(MockDeployableV2).creationCode + }) }); } } diff --git a/test/abstract/ExternalDeploySuites.sol b/test/abstract/ExternalDeploySuites.sol index bb134ff..6d411b2 100644 --- a/test/abstract/ExternalDeploySuites.sol +++ b/test/abstract/ExternalDeploySuites.sol @@ -40,7 +40,11 @@ abstract contract ExternalDeploySuites is RainDeploySuitesBase { } /// Runs the source anchor over the fixture's own declaration. - function externalCheckCandidatesAnchoredToSource() external pure { + /// + /// `view` because the anchor reads the compiler's artifact for the contract + /// each candidate names, which is the whole point of it: an operand the + /// declaration could supply is an operand the declaration could satisfy. + function externalCheckCandidatesAnchoredToSource() external view { checkCandidatesAnchoredToSource(); } } diff --git a/test/abstract/MisanchoredDeploySuites.sol b/test/abstract/MisanchoredDeploySuites.sol index b60f7f9..75c479f 100644 --- a/test/abstract/MisanchoredDeploySuites.sol +++ b/test/abstract/MisanchoredDeploySuites.sol @@ -20,19 +20,23 @@ import {MockDeployableV2} from "../concrete/MockDeployableV2.sol"; /// one the anchor makes: that the record is what the named contract COMPILES TO. /// /// This is the declaration the source anchor has to be able to refuse without -/// being handed the source by the thing it is checking. `SourceMismatchDeploy` -/// is its sibling and is NOT the same fixture: there the declaration itself -/// supplies the contradiction, so an anchor that believed the declaration would -/// still catch it. Here the declaration is internally silent, and the only -/// origin left that is not the declaration's own word is the compiler's -/// artifact for the contract the candidate names. +/// being handed the source by the thing it is checking, and it is the +/// regression fixture for rainlanguage/rain.factory.deploy#34. While +/// `DeployCandidate` carried a `sourceCreationCode` field, this declaration +/// pointed it at its own recorded bytes and passed: the anchor compared a value +/// with itself, so it was satisfied by construction for any candidate at all, +/// and the whole suite — and the broadcast — went green over it. There is no +/// longer a field with which to spell that, which is the only reason this +/// fixture cannot spell it. /// /// ONE candidate, and no releases. The loop-reaches-every-candidate property is -/// `SourceMismatchDeploySuites`' subject and is pinned there; this fixture is -/// about where the anchor's SOURCE operand comes from, which a single candidate -/// says with nothing else in the way. The key is its own, shared with no other -/// fixture, because `DEPLOYMENT_SUITE` is a process-wide variable other tests -/// write. +/// `SourceMismatchDeploySuites`' subject and is pinned there, behind a +/// genuinely anchored first entry; this fixture is about where the anchor's +/// SOURCE operand comes from, which a lone candidate says with nothing else in +/// the way — there is no earlier entry for a short loop to have stopped at and +/// no other suite for a refusal to have been about. The key is its own, shared +/// with no other fixture, because `DEPLOYMENT_SUITE` is a process-wide variable +/// other tests write. abstract contract MisanchoredDeploySuites is RainDeploySuitesBase { /// @inheritdoc RainDeploySuitesBase function releasedSuites() internal pure override returns (DeploySuite[] memory suites) { @@ -53,13 +57,7 @@ abstract contract MisanchoredDeploySuites is RainDeploySuitesBase { // of `MockDeployable`, and records the other contract. artifactPath: "test/concrete/MockDeployable.sol:MockDeployable", dependencies: new address[](0) - }), - // The exemption, spelled: both operands of the anchor are the - // RECORD, so the comparison is a value against itself and is - // satisfied for any candidate whatsoever. This is the mutation - // rainlanguage/rain.factory.deploy#34 found surviving a consumer's - // whole suite. - sourceCreationCode: type(MockDeployableV2).creationCode + }) }); } } diff --git a/test/abstract/SourceMismatchDeploySuites.sol b/test/abstract/SourceMismatchDeploySuites.sol index 767309a..6b7e2ee 100644 --- a/test/abstract/SourceMismatchDeploySuites.sol +++ b/test/abstract/SourceMismatchDeploySuites.sol @@ -22,14 +22,14 @@ import {MockDeployableV2} from "../concrete/MockDeployableV2.sol"; /// The broken candidate is a CONSISTENT snapshot: the address it records is the /// address its recorded creation code derives, the code hash is the one that /// creation code produces, and the runtime code hashes to it. Every check -/// internal to a snapshot passes on it. Only the pairing with -/// `sourceCreationCode` says it describes the wrong contract, which is why the -/// source anchor is the only thing that can catch it. +/// internal to a snapshot passes on it. Only the pairing with what its +/// `artifactPath` compiles to says it describes the wrong contract, which is +/// why the source anchor is the only thing that can catch it. /// /// `MockDeployable` and `MockDeployableV2` are the pair, deliberately: the -/// snapshot is `V2`'s while the source is `MockDeployable`'s, which is exactly -/// the shape of a snapshot regenerated from a build that has since moved, or -/// generated from the wrong contract in a repo that compiles several. +/// snapshot is `V2`'s while the contract it names is `MockDeployable`, which is +/// exactly the shape of a snapshot regenerated from a build that has since +/// moved, or generated from the wrong contract in a repo that compiles several. /// /// TWO candidates, broken one LAST, behind a genuinely anchored one. A loop /// that stops at the first entry is invisible against a single candidate and @@ -61,8 +61,7 @@ abstract contract SourceMismatchDeploySuites is RainDeploySuitesBase { storedRuntimeCode: ADDRESS_REGISTRY_RUNTIME_CODE, artifactPath: "src/concrete/AddressRegistry.sol:AddressRegistry", dependencies: new address[](0) - }), - sourceCreationCode: type(AddressRegistry).creationCode + }) }); candidates[1] = DeployCandidate({ snapshot: DeploySuite({ @@ -71,10 +70,13 @@ abstract contract SourceMismatchDeploySuites is RainDeploySuitesBase { storedDeployedAddress: LibRainDeploy.zoltuAddress(type(MockDeployableV2).creationCode), storedBytecodeHash: keccak256(type(MockDeployableV2).runtimeCode), storedRuntimeCode: type(MockDeployableV2).runtimeCode, - artifactPath: "test/concrete/MockDeployableV2.sol:MockDeployableV2", + // `MockDeployable`, while every recorded field above is + // `MockDeployableV2`'s. This is the whole of the fixture's + // breakage: the candidate names one contract and records + // another. + artifactPath: "test/concrete/MockDeployable.sol:MockDeployable", dependencies: new address[](0) - }), - sourceCreationCode: type(MockDeployable).creationCode + }) }); } } diff --git a/test/concrete/CollidingCandidateDeploySuites.sol b/test/concrete/CollidingCandidateDeploySuites.sol index cd84176..7311422 100644 --- a/test/concrete/CollidingCandidateDeploySuites.sol +++ b/test/concrete/CollidingCandidateDeploySuites.sol @@ -44,8 +44,7 @@ contract CollidingCandidateDeploySuites is ExternalDeploySuites { storedRuntimeCode: ADDRESS_REGISTRY_RUNTIME_CODE, artifactPath: "src/concrete/AddressRegistry.sol:AddressRegistry", dependencies: new address[](0) - }), - sourceCreationCode: type(AddressRegistry).creationCode + }) }); candidates[1] = DeployCandidate({ snapshot: DeploySuite({ @@ -56,8 +55,7 @@ contract CollidingCandidateDeploySuites is ExternalDeploySuites { storedRuntimeCode: type(MockDeployableV2).runtimeCode, artifactPath: "test/concrete/MockDeployableV2.sol:MockDeployableV2", dependencies: new address[](0) - }), - sourceCreationCode: type(MockDeployableV2).creationCode + }) }); } } diff --git a/test/concrete/DuplicateDeploySuites.sol b/test/concrete/DuplicateDeploySuites.sol index e2c78bc..5a10463 100644 --- a/test/concrete/DuplicateDeploySuites.sol +++ b/test/concrete/DuplicateDeploySuites.sol @@ -39,7 +39,6 @@ contract DuplicateDeploySuites is ExternalDeploySuites { /// @inheritdoc RainDeploySuitesBase function candidateSuites() internal pure override returns (DeployCandidate[] memory candidates) { candidates = new DeployCandidate[](1); - candidates[0] = - DeployCandidate({snapshot: collidingSuite(), sourceCreationCode: type(AddressRegistry).creationCode}); + candidates[0] = DeployCandidate({snapshot: collidingSuite()}); } } diff --git a/test/concrete/EmptyKeyDeploySuites.sol b/test/concrete/EmptyKeyDeploySuites.sol index 8019e3e..0ccf987 100644 --- a/test/concrete/EmptyKeyDeploySuites.sol +++ b/test/concrete/EmptyKeyDeploySuites.sol @@ -57,8 +57,7 @@ contract EmptyKeyDeploySuites is ExternalDeploySuites { storedRuntimeCode: type(MockDeployableV2).runtimeCode, artifactPath: "test/concrete/MockDeployableV2.sol:MockDeployableV2", dependencies: new address[](0) - }), - sourceCreationCode: type(MockDeployableV2).creationCode + }) }); return candidates; } diff --git a/test/concrete/MissingDependencyDeploy.sol b/test/concrete/MissingDependencyDeploy.sol index 81a62e5..eb87349 100644 --- a/test/concrete/MissingDependencyDeploy.sol +++ b/test/concrete/MissingDependencyDeploy.sol @@ -51,8 +51,7 @@ contract MissingDependencyDeploy is RainDeployBroadcast { storedRuntimeCode: type(MockDeployable).runtimeCode, artifactPath: "test/concrete/MockDeployable.sol:MockDeployable", dependencies: dependencies - }), - sourceCreationCode: type(MockDeployable).creationCode + }) }); } } diff --git a/test/concrete/MultiSuiteDeploy.sol b/test/concrete/MultiSuiteDeploy.sol index a4faf70..1f81a75 100644 --- a/test/concrete/MultiSuiteDeploy.sol +++ b/test/concrete/MultiSuiteDeploy.sol @@ -48,8 +48,7 @@ contract MultiSuiteDeploy is RainDeployBroadcast { storedRuntimeCode: type(MockDeployableV2).runtimeCode, artifactPath: "test/concrete/MockDeployableV2.sol:MockDeployableV2", dependencies: new address[](0) - }), - sourceCreationCode: type(MockDeployableV2).creationCode + }) }); candidates[1] = DeployCandidate({ snapshot: DeploySuite({ @@ -60,8 +59,7 @@ contract MultiSuiteDeploy is RainDeployBroadcast { storedRuntimeCode: type(MockDeployable).runtimeCode, artifactPath: "test/concrete/MockDeployable.sol:MockDeployable", dependencies: new address[](0) - }), - sourceCreationCode: type(MockDeployable).creationCode + }) }); } } diff --git a/test/concrete/SameLengthKeyDeploySuites.sol b/test/concrete/SameLengthKeyDeploySuites.sol index e96ab35..c60d963 100644 --- a/test/concrete/SameLengthKeyDeploySuites.sol +++ b/test/concrete/SameLengthKeyDeploySuites.sol @@ -49,8 +49,7 @@ contract SameLengthKeyDeploySuites is ExternalDeploySuites { storedRuntimeCode: type(MockDeployableV2).runtimeCode, artifactPath: "test/concrete/MockDeployableV2.sol:MockDeployableV2", dependencies: new address[](0) - }), - sourceCreationCode: type(MockDeployableV2).creationCode + }) }); } } diff --git a/test/concrete/SeparatorKeyDeploySuites.sol b/test/concrete/SeparatorKeyDeploySuites.sol index 9ce8dab..2b94081 100644 --- a/test/concrete/SeparatorKeyDeploySuites.sol +++ b/test/concrete/SeparatorKeyDeploySuites.sol @@ -41,8 +41,7 @@ contract SeparatorKeyDeploySuites is ExternalDeploySuites { storedRuntimeCode: type(MockDeployableV2).runtimeCode, artifactPath: "test/concrete/MockDeployableV2.sol:MockDeployableV2", dependencies: new address[](0) - }), - sourceCreationCode: type(MockDeployableV2).creationCode + }) }); return candidates; } diff --git a/test/concrete/ShortestKeyDeploySuites.sol b/test/concrete/ShortestKeyDeploySuites.sol index 2493df2..98a6a9b 100644 --- a/test/concrete/ShortestKeyDeploySuites.sol +++ b/test/concrete/ShortestKeyDeploySuites.sol @@ -49,8 +49,7 @@ contract ShortestKeyDeploySuites is ExternalDeploySuites { storedRuntimeCode: type(MockDeployableV2).runtimeCode, artifactPath: "test/concrete/MockDeployableV2.sol:MockDeployableV2", dependencies: new address[](0) - }), - sourceCreationCode: type(MockDeployableV2).creationCode + }) }); return candidates; } diff --git a/test/concrete/StaleCodeHashDeploy.sol b/test/concrete/StaleCodeHashDeploy.sol index 318fd5a..bdace34 100644 --- a/test/concrete/StaleCodeHashDeploy.sol +++ b/test/concrete/StaleCodeHashDeploy.sol @@ -50,8 +50,7 @@ contract StaleCodeHashDeploy is RainDeployBroadcast { storedRuntimeCode: type(MockDeployableV2).runtimeCode, artifactPath: "test/concrete/MockDeployableV2.sol:MockDeployableV2", dependencies: new address[](0) - }), - sourceCreationCode: type(MockDeployableV2).creationCode + }) }); } } diff --git a/test/concrete/StalePinDeploy.sol b/test/concrete/StalePinDeploy.sol index 402f164..c2c20ee 100644 --- a/test/concrete/StalePinDeploy.sol +++ b/test/concrete/StalePinDeploy.sol @@ -42,8 +42,7 @@ contract StalePinDeploy is RainDeployBroadcast { storedRuntimeCode: type(MockDeployableV2).runtimeCode, artifactPath: "test/concrete/MockDeployableV2.sol:MockDeployableV2", dependencies: new address[](0) - }), - sourceCreationCode: type(MockDeployableV2).creationCode + }) }); } } diff --git a/test/src/abstract/RainDeployVerifyChainCandidate.t.sol b/test/src/abstract/RainDeployVerifyChainCandidate.t.sol index aca55c5..fa9d384 100644 --- a/test/src/abstract/RainDeployVerifyChainCandidate.t.sol +++ b/test/src/abstract/RainDeployVerifyChainCandidate.t.sol @@ -62,8 +62,7 @@ contract RainDeployVerifyChainCandidateTest is RainDeployVerifyChain { storedRuntimeCode: type(MockDeployableV2).runtimeCode, artifactPath: "test/concrete/MockDeployableV2.sol:MockDeployableV2", dependencies: new address[](0) - }), - sourceCreationCode: type(MockDeployableV2).creationCode + }) }); } diff --git a/test/src/abstract/RainDeployVerifyChainEmpty.t.sol b/test/src/abstract/RainDeployVerifyChainEmpty.t.sol index 08685f5..8e2e46f 100644 --- a/test/src/abstract/RainDeployVerifyChainEmpty.t.sol +++ b/test/src/abstract/RainDeployVerifyChainEmpty.t.sol @@ -54,8 +54,7 @@ contract RainDeployVerifyChainEmptyTest is RainDeployVerifyChain { storedRuntimeCode: type(MockDeployableV2).runtimeCode, artifactPath: "test/concrete/MockDeployableV2.sol:MockDeployableV2", dependencies: new address[](0) - }), - sourceCreationCode: type(MockDeployableV2).creationCode + }) }); } diff --git a/test/src/abstract/RainDeployVerifySnapshotBase.t.sol b/test/src/abstract/RainDeployVerifySnapshotBase.t.sol index 1fc3d5f..1caf980 100644 --- a/test/src/abstract/RainDeployVerifySnapshotBase.t.sol +++ b/test/src/abstract/RainDeployVerifySnapshotBase.t.sol @@ -556,10 +556,13 @@ contract RainDeployVerifySnapshotBaseTest is ExampleDeploySuites, RainDeployVeri DeployCandidate memory candidate = sMismatch.externalCheckedCandidateSuites()[1]; // It really is the wrong contract: the snapshot records `MockDeployableV2` - // while the source it claims to be is `MockDeployable`. + // while the contract it NAMES is `MockDeployable`. Read back through + // the same resolution the anchor uses, so this says the two really do + // differ rather than restating the fixture's own literals. assertEq(keccak256(candidate.snapshot.creationCode), keccak256(type(MockDeployableV2).creationCode)); - assertEq(keccak256(candidate.sourceCreationCode), keccak256(type(MockDeployable).creationCode)); - assertNotEq(keccak256(candidate.snapshot.creationCode), keccak256(candidate.sourceCreationCode)); + assertEq(candidate.snapshot.artifactPath, "test/concrete/MockDeployable.sol:MockDeployable"); + assertEq(keccak256(vm.getCode(candidate.snapshot.artifactPath)), keccak256(type(MockDeployable).creationCode)); + assertNotEq(keccak256(candidate.snapshot.creationCode), keccak256(vm.getCode(candidate.snapshot.artifactPath))); // Every internal check passes anyway. this.externalCheckInternallyConsistent(candidate.snapshot); @@ -578,8 +581,8 @@ contract RainDeployVerifySnapshotBaseTest is ExampleDeploySuites, RainDeployVeri /// loop that reached it from one that reported a fixed entry or the first. /// /// The inherited `testSnapshotMatchesSource` is the passing case: it runs - /// this same function over `ExampleDeploySuites`, whose candidates are their - /// own source. + /// this same function over `ExampleDeploySuites`, whose candidates really + /// are the current compilation of the contracts they name. function testWrongContractSnapshotCaughtBySource() external { DeployCandidate[] memory candidates = sMismatch.externalCheckedCandidateSuites(); assertEq(candidates.length, 2); @@ -588,7 +591,9 @@ contract RainDeployVerifySnapshotBaseTest is ExampleDeploySuites, RainDeployVeri // to advance, and it is a different contract at a different address // rather than the same entry under two keys. assertEq(candidates[0].snapshot.suite, "anchored-candidate"); - assertEq(keccak256(candidates[0].snapshot.creationCode), keccak256(candidates[0].sourceCreationCode)); + assertEq( + keccak256(candidates[0].snapshot.creationCode), keccak256(vm.getCode(candidates[0].snapshot.artifactPath)) + ); assertNotEq(candidates[0].snapshot.storedDeployedAddress, candidates[1].snapshot.storedDeployedAddress); vm.expectRevert( diff --git a/test/src/abstract/RainDeployVerifySnapshotBaseCandidate.t.sol b/test/src/abstract/RainDeployVerifySnapshotBaseCandidate.t.sol index 1619940..4f51d0a 100644 --- a/test/src/abstract/RainDeployVerifySnapshotBaseCandidate.t.sol +++ b/test/src/abstract/RainDeployVerifySnapshotBaseCandidate.t.sol @@ -50,8 +50,7 @@ contract RainDeployVerifySnapshotBaseCandidateTest is RainDeployVerifySnapshotBa storedRuntimeCode: type(MockDeployable).runtimeCode, artifactPath: "test/concrete/MockDeployable.sol:MockDeployable", dependencies: new address[](0) - }), - sourceCreationCode: type(MockDeployable).creationCode + }) }); } diff --git a/test/src/abstract/RegistryDeploySuites.t.sol b/test/src/abstract/RegistryDeploySuites.t.sol index d1fff4b..d82bcfb 100644 --- a/test/src/abstract/RegistryDeploySuites.t.sol +++ b/test/src/abstract/RegistryDeploySuites.t.sol @@ -19,13 +19,15 @@ import {LibRainDeploySnapshot} from "../../../src/lib/LibRainDeploySnapshot.sol" /// only thing that consumes it is `deployToNetworks`, on a fork, under an RPC /// this suite does not have. /// -/// The source anchor's two operands are the other thing nothing could reach. -/// `checkCandidatesAnchoredToSource` compares two `bytes`, and where they come -/// from is a property of how the declaration is SPELLED rather than of any -/// value it produces: a candidate that reads `type(X).creationCode` into the -/// recorded field compares source against itself, and on a green tree the two +/// The RECORD half of the source anchor is the other thing nothing could reach. +/// `checkCandidatesAnchoredToSource` compares the bytes a candidate records +/// against what its `artifactPath` currently compiles to, and the source half +/// is no longer anything the declaration can reach — it is read from the +/// compiler's artifact. The recorded half still is: a candidate that reads +/// `type(X).creationCode` into the recorded field puts BOTH operands on the +/// source side and compares source against itself, and on a green tree the two /// spellings are byte-identical, so no runtime assertion can tell them apart. -/// The assertions below read the compiler's AST of the declaration itself, for +/// The assertion below reads the compiler's AST of the declaration itself, for /// the reason `GeneratedSnapshotShapeTest` gives for reading it rather than the /// source text: this is about structure, not formatting. contract RegistryDeploySuitesTest is RegistryDeploySuites, Test { @@ -63,12 +65,13 @@ contract RegistryDeploySuitesTest is RegistryDeploySuites, Test { /// PROPERTY: every candidate's RECORDED creation code and runtime code are /// read from the rolling `src/generated/candidate/` snapshot. /// - /// This is the half of the source anchor that has to come from the record. - /// Spelling it `type(X).creationCode` — to drop an import, or to make the - /// two fields of a candidate read alike — puts both operands of - /// `checkCandidatesAnchoredToSource` on the source side and makes the one - /// check that catches a snapshot of the wrong contract a tautology for that - /// candidate, with nothing red anywhere. + /// This is the half of the source anchor that has to come from the record, + /// and since the source half stopped being a field it is the ONLY half a + /// declaration can still get wrong. Spelling it `type(X).creationCode` — to + /// drop an import, or because the record and the compiler agree today + /// anyway — puts both operands of `checkCandidatesAnchoredToSource` on the + /// source side and makes the one check that catches a snapshot of the wrong + /// contract a tautology for that candidate, with nothing red anywhere. /// /// The runtime code is held to the record by the same assertion because it /// is what would be left of the record side. A candidate whose creation @@ -76,7 +79,7 @@ contract RegistryDeploySuitesTest is RegistryDeploySuites, Test { /// which stops being internal the moment one operand is source; that only /// holds while the rest of the snapshot is the generated file, and a /// refactor reaching for the type expression reaches for both `bytes` - /// fields at once. + /// fields of the snapshot at once. /// /// The identifier is matched against the declaration id the IMPORT resolved /// to, rather than against a constant name written here. A name asserts @@ -100,28 +103,6 @@ contract RegistryDeploySuitesTest is RegistryDeploySuites, Test { } } - /// PROPERTY: every candidate's `sourceCreationCode` is - /// `type(X).creationCode`. - /// - /// The mirror of the assertion above, and undetectable at runtime for the - /// same reason. A candidate whose source side reads the generated constant - /// compares the record against itself, which is a tautology arrived at from - /// the other direction — and it is the spelling a refactor lands on when it - /// notices that the two fields hold equal bytes. - function testCandidatesAnchorAgainstCurrentSource() external view { - string memory json = vm.readFile(DECLARATION_ARTIFACT); - string[] memory candidates = candidateLiteralPaths(json); - assertEq(candidates.length, checkedCandidateSuites().length, "a declared candidate has no struct literal"); - - for (uint256 i = 0; i < candidates.length; i++) { - assertEq( - expressionShape(json, fieldPath(json, candidates[i], "sourceCreationCode")), - "type().creationCode", - "candidate does not anchor to current source" - ); - } - } - /// The JSON path of every `DeployCandidate` struct literal the declaration /// returns. /// @@ -309,24 +290,4 @@ contract RegistryDeploySuitesTest is RegistryDeploySuites, Test { } assertTrue(imported, string.concat("candidate does not read the rolling snapshot for ", field)); } - - /// A field value's shape, as `().` for a member access on a - /// call and as its node type otherwise. - /// - /// One string compared once, rather than a walk that reads `memberName` off - /// a node that may not have one: the assertion that would have caught it is - /// then the assertion that reports it. - /// @param json The declaration's artifact. - /// @param path The field value's path. - /// @return The shape. - function expressionShape(string memory json, string memory path) internal view returns (string memory) { - string memory nodeType = vm.parseJsonString(json, string.concat(path, ".nodeType")); - string memory callee = string.concat(path, ".expression.expression.name"); - if (keccak256(bytes(nodeType)) != keccak256("MemberAccess") || !vm.keyExistsJson(json, callee)) { - return nodeType; - } - return string.concat( - vm.parseJsonString(json, callee), "().", vm.parseJsonString(json, string.concat(path, ".memberName")) - ); - } } diff --git a/test/src/lib/GeneratedSnapshotShape.t.sol b/test/src/lib/GeneratedSnapshotShape.t.sol index 7f7cd6c..4b04a1e 100644 --- a/test/src/lib/GeneratedSnapshotShape.t.sol +++ b/test/src/lib/GeneratedSnapshotShape.t.sol @@ -185,19 +185,16 @@ contract GeneratedSnapshotShapeTest is RegistryDeploySuites, Test { /// the path is a file this repo has, and the artifact that whole string /// selects is the contract the candidate is anchored to. /// - /// `artifactPath` is the one field of a suite that nothing derives, and - /// until this assertion nothing checked either. Two things read it: - /// `LibRainDeploy` prints it as the `forge verify-contract` command a - /// human runs against a freshly broadcast contract, and - /// `candidateContractName` above takes the contract this whole shape spec - /// is about out of it. Only the `:` half was ever read by a check — - /// `testEveryCandidateHasASnapshot` matches it against the generator's - /// output — so a path left behind by a moved or renamed source file passed - /// every assertion in this repo, including all of the ones here, because - /// the half that moved is the half nothing read. `bytecode_hash = "none"` - /// and `cbor_metadata = false` mean moving a file changes no creation code, - /// no address and no code hash either, so there is nothing else for it to - /// be caught by. + /// `artifactPath` is the field a candidate says which contract it is with. + /// Three things read it now: `LibRainDeploy` prints it as the + /// `forge verify-contract` command a human runs against a freshly broadcast + /// contract, `candidateContractName` above takes the contract this whole + /// shape spec is about out of it, and + /// `RainDeploySuitesBase.checkCandidatesAnchoredToSource` resolves it to + /// get the source half of the anchor. `bytecode_hash = "none"` and + /// `cbor_metadata = false` mean moving a file changes no creation code, no + /// address and no code hash, so the path itself is the only thing that + /// can be wrong about a path. /// /// Resolved through `vm.getCode`, which is foundry's own resolution of a /// `:` artifact id — the same form `forge verify-contract` @@ -206,25 +203,30 @@ contract GeneratedSnapshotShapeTest is RegistryDeploySuites, Test { /// in a comment or in a longer identifier, and defeated by a declaration /// written with no space before its brace. /// - /// `vm.isFile` on the path half AS WELL, because `vm.getCode` resolves an - /// artifact id by path SUFFIX while the printed command is run from the - /// repo root. `concrete/AddressRegistry.sol:AddressRegistry` resolves for - /// `vm.getCode` — uniquely, and to the right contract — and still names no - /// file anybody can point `forge verify-contract` at. That is the one shape - /// of wrong path nothing else in this repo goes red on. + /// `vm.isFile` on the path half is the assertion here that nothing else + /// makes, and it is why this test survived the anchor learning to resolve + /// this field for itself. `vm.getCode` resolves an artifact id by path + /// SUFFIX, while the printed verification command is run from the repo + /// root: `concrete/AddressRegistry.sol:AddressRegistry` resolves for + /// `vm.getCode` — uniquely, and to the right contract, so the anchor is + /// perfectly happy with it — and still names no file anybody can point + /// `forge verify-contract` at. That is the one shape of wrong path nothing + /// else in this repo goes red on. /// /// A path outside the `fs_permissions` roots fails here as a cheatcode /// revert naming the path rather than as this assertion's own message, /// which is foundry refusing to look rather than looking and not finding. /// - /// Compared against the candidate's own `sourceCreationCode` rather than - /// merely required to resolve to something, because resolving is not the - /// same as resolving to the RIGHT contract. Two candidates whose declared - /// paths are swapped resolve perfectly, and every other assertion in this - /// repo stays green through it — including `testEveryCandidateHasASnapshot` - /// above, because swapping both halves leaves the SET of declared names - /// exactly as it was. This is the assertion that says each candidate's path - /// names ITS own contract rather than one of its siblings'. + /// The creation code comparison is a RESTATEMENT of + /// `checkCandidatesAnchoredToSource`, which now reads the source side out + /// of this same field through this same cheatcode, and which the BROADCAST + /// runs. It is kept because it costs nothing and because it is what makes + /// the `vm.isFile` line above readable as the one thing that is not also + /// the anchor; the guarantee it describes — two candidates whose declared + /// paths are swapped resolve perfectly, and swapping both halves leaves the + /// SET of declared names exactly as `testEveryCandidateHasASnapshot` wants + /// it — lives in the anchor now, on the path that deploys, rather than only + /// in a test. /// /// Hashed rather than compared as bytes, for the reason /// `CandidateSourceMismatch` gives for hashing: creation codes run to tens @@ -240,7 +242,7 @@ contract GeneratedSnapshotShapeTest is RegistryDeploySuites, Test { assertTrue(vm.isFile(parts[0]), string.concat("artifact path names no such file: ", declared)); assertEq( keccak256(vm.getCode(declared)), - keccak256(candidates[i].sourceCreationCode), + keccak256(candidates[i].snapshot.creationCode), string.concat("artifact path resolves to another contract: ", declared) ); } From e59ff13f00f05eaa4864b28719af0c32fa14a3ed Mon Sep 17 00:00:00 2001 From: baku-ccron Date: Sun, 20 Sep 2026 13:26:55 +0000 Subject: [PATCH 3/3] docs: cut the rationale essays this PR added to its comments Co-Authored-By: Claude Opus 5 (1M context) --- script/Build.sol | 10 -- src/abstract/RainDeploySuitesBase.sol | 102 ++---------------- src/abstract/RainDeployVerifySnapshotBase.sol | 3 - src/abstract/RegistryDeploySuites.sol | 12 +-- test/abstract/ExternalDeploySuites.sol | 4 - test/abstract/MisanchoredDeploySuites.sol | 34 +----- test/abstract/SourceMismatchDeploySuites.sol | 13 +-- test/concrete/MisanchoredDeploy.sol | 11 -- test/src/abstract/RainDeployBroadcast.t.sol | 23 ---- test/src/abstract/RainDeploySuitesBase.t.sol | 30 ------ .../RainDeployVerifySnapshotBase.t.sol | 7 +- test/src/abstract/RegistryDeploySuites.t.sol | 22 ++-- test/src/lib/GeneratedSnapshotShape.t.sol | 39 ++----- 13 files changed, 35 insertions(+), 275 deletions(-) diff --git a/script/Build.sol b/script/Build.sol index 5dd1b8f..da26857 100644 --- a/script/Build.sol +++ b/script/Build.sol @@ -15,9 +15,6 @@ struct GeneratedContract { string contractName; /// Prefix for the constants the alias lib exports, e.g. `ADDRESS_REGISTRY`. string constantPrefix; - /// Snapshots are written from what its `snapshot.artifactPath` currently - /// compiles to, plus `snapshot.dependencies`; the released lib takes its - /// suite key and artifact path from its `snapshot`. DeployCandidate candidate; } @@ -90,13 +87,6 @@ contract Build is BuildScript, RegistryDeploySuites { } /// @inheritdoc BuildScript - /// @dev The bytes written are what the candidate's own `artifactPath` - /// compiles to RIGHT NOW, resolved through `vm.getCode` — the same origin, - /// read the same way, that `checkCandidatesAnchoredToSource` will hold the - /// written snapshot against. Regenerating and then checking is therefore - /// one claim rather than two spellings of it, and a declaration whose path - /// names the wrong contract writes a snapshot that goes red on the next - /// run instead of a snapshot nothing disagrees with. function regenerateSnapshots() internal override { GeneratedContract[] memory contracts = generatedContracts(); for (uint256 i = 0; i < contracts.length; i++) { diff --git a/src/abstract/RainDeploySuitesBase.sol b/src/abstract/RainDeploySuitesBase.sol index 812e6d8..690eda0 100644 --- a/src/abstract/RainDeploySuitesBase.sol +++ b/src/abstract/RainDeploySuitesBase.sol @@ -84,8 +84,7 @@ error NoDeployCandidates(); /// @param storedCreationCodeHash Hash of the creation code the candidate /// records. /// @param sourceCreationCodeHash Hash of the creation code the contract the -/// candidate NAMES — its `artifactPath` — currently compiles to, read from the -/// compiler's own artifact rather than from anything the declaration says. +/// candidate NAMES — its `artifactPath` — currently compiles to. error CandidateSourceMismatch(string suite, bytes32 storedCreationCodeHash, bytes32 sourceCreationCodeHash); /// One deployable unit: a named snapshot of one contract. @@ -122,17 +121,10 @@ struct DeploySuite { /// suite broadcasts the exact bytes its audit covered, whatever the current /// source now compiles to. /// - /// What a candidate is anchored AGAINST is what `artifactPath` currently - /// compiles to, so spelling `type(X).creationCode` here puts both operands - /// of `checkCandidatesAnchoredToSource` on the source side and leaves the - /// one check that catches a snapshot of the wrong contract comparing source - /// to itself, green. Fixtures that derive a whole mock suite do that on - /// purpose, because they have no record and are exercising other - /// assertions; a declaration of a real deployment never does. - /// - /// This is the remaining half a declaration can get wrong, and it is the - /// half a declaration has to own: the record is the point. The SOURCE half - /// was an ordinary field until it was not — see `DeployCandidate`. + /// Spelling `type(X).creationCode` here puts both operands of + /// `checkCandidatesAnchoredToSource` on the source side and leaves the one + /// check that catches a snapshot of the wrong contract comparing source to + /// itself, green. bytes creationCode; /// The deploy address recorded for this suite. address storedDeployedAddress; @@ -147,21 +139,9 @@ struct DeploySuite { /// holds only the flattest repos; a repo that groups concretes into /// subdirectories has paths no naming convention recovers. /// - /// Two things read it, and for a CANDIDATE the second is the load-bearing - /// one. `LibRainDeploy` prints it as the `forge verify-contract` command a - /// human runs against a freshly broadcast contract, and - /// `checkCandidatesAnchoredToSource` resolves it through `vm.getCode` to - /// get the creation code the named contract currently compiles to — the - /// source half of the one check that catches a snapshot of the wrong - /// contract. Naming the contract is therefore the whole of what a candidate - /// says about its source, and it is all it gets to say: what that contract - /// COMPILES TO is the compiler's answer, not the declaration's. - /// - /// A path that resolves to no artifact, or to more than one, now fails at - /// the anchor — which for a candidate is before the broadcast. It used to - /// fail nowhere in this package: the printed verification command is read - /// by a human AFTER the deploy, so a path left behind by a moved or renamed - /// source file cost a deploy before it cost a test. + /// For a candidate this is load-bearing: `checkCandidatesAnchoredToSource` + /// resolves it through `vm.getCode`, so a path that resolves to no + /// artifact, or to more than one, fails at the anchor before the broadcast. string artifactPath; /// Addresses that MUST already have code on a network before this suite is /// broadcast there. Ordinarily other suites' recorded addresses: a @@ -173,36 +153,9 @@ struct DeploySuite { /// The rolling candidate: a snapshot that tracks current source rather than a /// frozen release, and that MUST equal what the contract it names currently /// compiles to. -/// -/// That anchor is the ONLY thing that catches a snapshot of the wrong contract. -/// Every check internal to a snapshot is satisfied by a consistent snapshot of -/// the wrong thing, so without it there is nothing that says the recorded bytes -/// belong to the contract this repo compiles. -/// -/// Anchoring is a property of the TYPE, not a value the declaration supplies. -/// `checkCandidatesAnchoredToSource` reads the source side out of the -/// compiler's artifact for `snapshot.artifactPath`, so a declaration has no -/// operand to hand it and therefore no way to spell an exemption. It used to -/// carry that operand as an ordinary `sourceCreationCode` field, and an -/// ordinary field is one a consumer fills in: pointing it at the same generated -/// constant as `snapshot.creationCode` left the one check that catches a -/// snapshot of the wrong contract comparing a value with itself — green for any -/// candidate whatsoever, on the broadcast path as well as in CI, with a whole -/// consumer suite passing through it (rainlanguage/rain.factory.deploy#34). -/// -/// ONE field, deliberately, and the type is not folded back into `DeploySuite` -/// for it. The type is what separates a rolling candidate from a frozen -/// release, and that separation is the whole of what it carries: a released tag -/// is MEANT to diverge from current source, so anchoring one asserts something -/// false by design. `releasedSuites()` returns `DeploySuite[]` and -/// `candidateSuites()` returns this, so which of the two an entry is is -/// something the compiler makes a repo state rather than a flag beside the data -/// — there is no way for a caller to spell "released, and also skip the checks -/// that do apply", and none to spell "candidate, and also skip the anchor". struct DeployCandidate { /// The candidate's own recorded snapshot, checked exactly as any other - /// suite is, and additionally anchored to what its `artifactPath` compiles - /// to. + /// suite is, and anchored to what its `artifactPath` compiles to. DeploySuite snapshot; } @@ -302,42 +255,7 @@ abstract contract RainDeploySuitesBase { /// Candidates alone, and there is no way to spell an exemption. A released /// suite is MEANT to diverge from current source — it records bytes that /// are already on chain — so anchoring one to source asserts something - /// false by design, which is why only `candidateSuites()` is read here. - /// - /// ## Where the source operand comes from - /// - /// The COMPILER, through `vm.getCode` on the candidate's `artifactPath` — - /// foundry's own resolution of a `:` artifact id, the same form - /// `forge verify-contract` takes. Never from the declaration. - /// - /// A declaration that supplied both operands could satisfy this by - /// construction, and one did: `DeployCandidate` used to carry the source - /// creation code as a field, so pointing that field at the same generated - /// constant as `snapshot.creationCode` made the comparison a value against - /// itself — satisfied for any candidate, including a snapshot of an - /// entirely different contract, with a consumer's whole suite still green - /// and `RainDeployBroadcast` running the same neutered definition before it - /// broadcast (rainlanguage/rain.factory.deploy#34). "No way to spell an - /// exemption" is only true of an operand the declaration cannot reach. A - /// consumer names the contract; what that contract compiles to is not - /// something it gets a say in. - /// - /// Not derived from the contract NAME either — `artifactPath` is the whole - /// `:`, because a repo that groups its concretes into - /// subdirectories has paths no naming convention recovers, and because that - /// field already exists and is already the one thing a suite says about - /// which contract it is. - /// - /// `view` rather than `pure` follows from reading the compiler at all, and - /// costs nothing: both callers are a `Script` and a `Test`, which is the - /// only place a cheatcode exists. - /// - /// An `artifactPath` that resolves to no artifact, or to more than one, - /// reverts inside the cheatcode naming the path, before this comparison is - /// reached. That failure is new and it is the right one: the field was - /// previously read only by the verification command `LibRainDeploy` prints - /// AFTER a broadcast, so a path left behind by a moved or renamed source - /// file cost a deploy before it cost a test. + /// false by design. function checkCandidatesAnchoredToSource() internal view { DeployCandidate[] memory candidates = checkedCandidateSuites(); for (uint256 i = 0; i < candidates.length; i++) { diff --git a/src/abstract/RainDeployVerifySnapshotBase.sol b/src/abstract/RainDeployVerifySnapshotBase.sol index 27101ec..72d3d77 100644 --- a/src/abstract/RainDeployVerifySnapshotBase.sol +++ b/src/abstract/RainDeployVerifySnapshotBase.sol @@ -396,9 +396,6 @@ abstract contract RainDeployVerifySnapshotBase is RainDeployVerifyBase { /// definition before it broadcasts. A second spelling on this side is a /// spelling the deploy does not run, which is exactly the state this test /// would otherwise be reporting green about. - /// - /// `view` because that one definition reads the compiler's artifact for the - /// named contract rather than an operand the declaration hands it. function testSnapshotMatchesSource() external view { checkCandidatesAnchoredToSource(); } diff --git a/src/abstract/RegistryDeploySuites.sol b/src/abstract/RegistryDeploySuites.sol index 721f2f5..9291849 100644 --- a/src/abstract/RegistryDeploySuites.sol +++ b/src/abstract/RegistryDeploySuites.sol @@ -97,13 +97,7 @@ abstract contract RegistryDeploySuites is RainDeploySuitesBase { /// `src/generated/candidate/` snapshot. That is what makes the source /// anchor mean something: it compares the recorded creation code against /// whatever the `artifactPath` below currently compiles to, so editing the - /// contract without re-running `script/Build.sol` fails. While nothing was - /// recorded, that check compared source against itself and could only pass. - /// - /// Nothing here supplies the source half, and there is no field left to - /// supply it with. Naming the contract in `artifactPath` is the whole of - /// what this declaration says about its source; what that contract compiles - /// to is the compiler's answer, read out of its artifact. + /// contract without re-running `script/Build.sol` fails. /// /// `AddressRegistry` reads nothing and calls nothing at construction, so it /// has no dependency that must already be on chain. @@ -134,9 +128,7 @@ abstract contract RegistryDeploySuites is RainDeploySuitesBase { /// Everything said about the `AddressRegistry` candidate holds here /// unchanged: the pins are aliased from the generated snapshot, the /// creation and runtime code are recorded rather than derived, and the - /// source anchor — the record held against what this candidate's - /// `artifactPath` compiles to — is what says the record describes THIS - /// contract. + /// source anchor is what says the record describes THIS contract. /// /// `MigrationRegistry` has no constructor argument, no compile-time /// authority and no dependency to be on chain first — the namespace is diff --git a/test/abstract/ExternalDeploySuites.sol b/test/abstract/ExternalDeploySuites.sol index 6d411b2..6a5aa5b 100644 --- a/test/abstract/ExternalDeploySuites.sol +++ b/test/abstract/ExternalDeploySuites.sol @@ -40,10 +40,6 @@ abstract contract ExternalDeploySuites is RainDeploySuitesBase { } /// Runs the source anchor over the fixture's own declaration. - /// - /// `view` because the anchor reads the compiler's artifact for the contract - /// each candidate names, which is the whole point of it: an operand the - /// declaration could supply is an operand the declaration could satisfy. function externalCheckCandidatesAnchoredToSource() external view { checkCandidatesAnchoredToSource(); } diff --git a/test/abstract/MisanchoredDeploySuites.sol b/test/abstract/MisanchoredDeploySuites.sol index 75c479f..ec70ddf 100644 --- a/test/abstract/MisanchoredDeploySuites.sol +++ b/test/abstract/MisanchoredDeploySuites.sol @@ -6,37 +6,6 @@ import {DeployCandidate, DeploySuite, RainDeploySuitesBase} from "../../src/abst import {LibRainDeploy} from "../../src/lib/LibRainDeploy.sol"; import {MockDeployableV2} from "../concrete/MockDeployableV2.sol"; -/// @title MisanchoredDeploySuites -/// @notice A declaration that says it deploys one contract and records another, -/// with NOTHING in the declaration to say so. -/// -/// The single candidate names `MockDeployable` as the contract it is a snapshot -/// of — that is what `artifactPath` is, the `:` the explorer -/// verification command is run with — and every recorded field is -/// `MockDeployableV2`'s. The snapshot is perfectly consistent with itself: the -/// address is the one its recorded creation code derives, the code hash is the -/// one that creation code produces, and the runtime code hashes to it. So every -/// check internal to a snapshot passes, and the only claim left to check is the -/// one the anchor makes: that the record is what the named contract COMPILES TO. -/// -/// This is the declaration the source anchor has to be able to refuse without -/// being handed the source by the thing it is checking, and it is the -/// regression fixture for rainlanguage/rain.factory.deploy#34. While -/// `DeployCandidate` carried a `sourceCreationCode` field, this declaration -/// pointed it at its own recorded bytes and passed: the anchor compared a value -/// with itself, so it was satisfied by construction for any candidate at all, -/// and the whole suite — and the broadcast — went green over it. There is no -/// longer a field with which to spell that, which is the only reason this -/// fixture cannot spell it. -/// -/// ONE candidate, and no releases. The loop-reaches-every-candidate property is -/// `SourceMismatchDeploySuites`' subject and is pinned there, behind a -/// genuinely anchored first entry; this fixture is about where the anchor's -/// SOURCE operand comes from, which a lone candidate says with nothing else in -/// the way — there is no earlier entry for a short loop to have stopped at and -/// no other suite for a refusal to have been about. The key is its own, shared -/// with no other fixture, because `DEPLOYMENT_SUITE` is a process-wide variable -/// other tests write. abstract contract MisanchoredDeploySuites is RainDeploySuitesBase { /// @inheritdoc RainDeploySuitesBase function releasedSuites() internal pure override returns (DeploySuite[] memory suites) { @@ -53,8 +22,7 @@ abstract contract MisanchoredDeploySuites is RainDeploySuitesBase { storedDeployedAddress: LibRainDeploy.zoltuAddress(type(MockDeployableV2).creationCode), storedBytecodeHash: keccak256(type(MockDeployableV2).runtimeCode), storedRuntimeCode: type(MockDeployableV2).runtimeCode, - // NOT `MockDeployableV2`. The candidate claims to be a snapshot - // of `MockDeployable`, and records the other contract. + // Deliberately NOT `MockDeployableV2`, which every recorded field above is. artifactPath: "test/concrete/MockDeployable.sol:MockDeployable", dependencies: new address[](0) }) diff --git a/test/abstract/SourceMismatchDeploySuites.sol b/test/abstract/SourceMismatchDeploySuites.sol index 6b7e2ee..be1d6ec 100644 --- a/test/abstract/SourceMismatchDeploySuites.sol +++ b/test/abstract/SourceMismatchDeploySuites.sol @@ -22,14 +22,10 @@ import {MockDeployableV2} from "../concrete/MockDeployableV2.sol"; /// The broken candidate is a CONSISTENT snapshot: the address it records is the /// address its recorded creation code derives, the code hash is the one that /// creation code produces, and the runtime code hashes to it. Every check -/// internal to a snapshot passes on it. Only the pairing with what its -/// `artifactPath` compiles to says it describes the wrong contract, which is -/// why the source anchor is the only thing that can catch it. +/// internal to a snapshot passes on it. Only the source anchor can catch it. /// /// `MockDeployable` and `MockDeployableV2` are the pair, deliberately: the -/// snapshot is `V2`'s while the contract it names is `MockDeployable`, which is -/// exactly the shape of a snapshot regenerated from a build that has since -/// moved, or generated from the wrong contract in a repo that compiles several. +/// snapshot is `V2`'s while the contract it names is `MockDeployable`. /// /// TWO candidates, broken one LAST, behind a genuinely anchored one. A loop /// that stops at the first entry is invisible against a single candidate and @@ -70,10 +66,7 @@ abstract contract SourceMismatchDeploySuites is RainDeploySuitesBase { storedDeployedAddress: LibRainDeploy.zoltuAddress(type(MockDeployableV2).creationCode), storedBytecodeHash: keccak256(type(MockDeployableV2).runtimeCode), storedRuntimeCode: type(MockDeployableV2).runtimeCode, - // `MockDeployable`, while every recorded field above is - // `MockDeployableV2`'s. This is the whole of the fixture's - // breakage: the candidate names one contract and records - // another. + // Deliberately NOT `MockDeployableV2`, which every recorded field above is. artifactPath: "test/concrete/MockDeployable.sol:MockDeployable", dependencies: new address[](0) }) diff --git a/test/concrete/MisanchoredDeploy.sol b/test/concrete/MisanchoredDeploy.sol index f272a1d..76a6068 100644 --- a/test/concrete/MisanchoredDeploy.sol +++ b/test/concrete/MisanchoredDeploy.sol @@ -6,15 +6,4 @@ import {RainDeployBroadcast} from "../../src/abstract/RainDeployBroadcast.sol"; import {ExternalDeploySuites} from "../abstract/ExternalDeploySuites.sol"; import {MisanchoredDeploySuites} from "../abstract/MisanchoredDeploySuites.sol"; -/// @title MisanchoredDeploy -/// A deploy repo's whole script over a declaration that names one contract and -/// records another. -/// -/// A real script rather than a declaration fixture, for the reason -/// `SourceMismatchDeploy` is one: the claim under test is that the anchor holds -/// on the path that BROADCASTS. A declaration that could only be driven through -/// the external wrappers would leave `run()` — the irreversible, multi-chain, -/// key-custody action — asserted about by nothing, and `run()` is exactly where -/// a neutered anchor costs a permanent `CREATE2` address on every chain the -/// dispatch reached. contract MisanchoredDeploy is MisanchoredDeploySuites, ExternalDeploySuites, RainDeployBroadcast {} diff --git a/test/src/abstract/RainDeployBroadcast.t.sol b/test/src/abstract/RainDeployBroadcast.t.sol index 9f1de12..7b9c22d 100644 --- a/test/src/abstract/RainDeployBroadcast.t.sol +++ b/test/src/abstract/RainDeployBroadcast.t.sol @@ -408,29 +408,6 @@ contract RainDeployBroadcastTest is Test { mismatch.run(); } - /// `run()` MUST refuse a candidate that records a contract other than the - /// one it NAMES, even when the declaration itself says nothing is wrong. - /// - /// The sibling test above hands `run()` a declaration that contradicts - /// itself, so an anchor that trusted the declaration would still catch it. - /// This one does not: `MisanchoredDeploySuites` records a consistent - /// snapshot of `MockDeployableV2`, names `MockDeployable` as the contract - /// it is a snapshot of, and — in the shape - /// rainlanguage/rain.factory.deploy#34 found surviving a consumer's whole - /// suite — would offer the anchor its own recorded bytes as the source side - /// if the anchor were willing to take them. Everything internal to the - /// snapshot agrees with everything else. - /// - /// Asserted on `run()` and not only through the external wrapper, because - /// this is the reason the anchor lives on the declaration at all. A guard - /// the irreversible action does not run is not a guard: broadcasting is - /// `workflow_dispatch` on a ref with no required-green gate, and `CREATE2` - /// at a zero salt puts the wrong bytes at their own permanent address on - /// every chain the dispatch reached. - /// - /// The anchor is reached before `DEPLOYMENT_SUITE` resolves and before - /// `DEPLOYMENT_KEY` is read, which is why no env var is written here and - /// why the revert that arrives is the anchor's rather than the selection's. function testRunRefusesToBroadcastACandidateThatNamesAnotherContract() external { MisanchoredDeploy misanchored = new MisanchoredDeploy(); diff --git a/test/src/abstract/RainDeploySuitesBase.t.sol b/test/src/abstract/RainDeploySuitesBase.t.sol index 8ef6907..386d87b 100644 --- a/test/src/abstract/RainDeploySuitesBase.t.sol +++ b/test/src/abstract/RainDeploySuitesBase.t.sol @@ -265,36 +265,6 @@ contract RainDeploySuitesBaseTest is Test { sSuites.externalCheckCandidatesAnchoredToSource(); } - /// A candidate whose record is not what the contract it NAMES compiles to - /// MUST be refused, and no field of the declaration may be able to say - /// otherwise. - /// - /// The anchor is the only check that catches a snapshot of the wrong - /// contract, so its source operand cannot be something the declaration - /// hands it. A declaration that supplies BOTH operands can satisfy the - /// anchor by construction: point the source side at the same recorded bytes - /// and the comparison is a value against itself — green for any candidate - /// whatsoever, including a snapshot of an entirely different contract, and - /// green on the broadcast path as well as in CI. That is not a hypothetical - /// spelling. It is the mutation rainlanguage/rain.factory.deploy#34 found - /// SURVIVING a consumer's whole suite. - /// - /// `MisanchoredDeploySuites` is that declaration, and it is internally - /// SILENT about the contradiction. Its snapshot is consistent with itself, - /// its recorded bytes are the current compilation of a contract this repo - /// really has, and nothing it declares disagrees with anything else it - /// declares. The only thing that says it is `MockDeployableV2`'s snapshot - /// wearing `MockDeployable`'s name is the compiler's artifact for the - /// contract the candidate names — an origin the declaration does not own. - /// - /// The reported source hash is asserted, not merely the refusal. It is - /// `MockDeployable`'s, the contract the candidate NAMES, which is what says - /// the operand was read from that contract's artifact rather than from the - /// recorded bytes the declaration offered for it. - /// - /// `testCandidatesPresentAnswers` above is the discriminating case: a - /// declaration whose candidates really are snapshots of the contracts they - /// name passes this same call. function testCandidateThatNamesAnotherContractIsRefused() external { MisanchoredDeploy misanchored = new MisanchoredDeploy(); diff --git a/test/src/abstract/RainDeployVerifySnapshotBase.t.sol b/test/src/abstract/RainDeployVerifySnapshotBase.t.sol index 1caf980..ec96617 100644 --- a/test/src/abstract/RainDeployVerifySnapshotBase.t.sol +++ b/test/src/abstract/RainDeployVerifySnapshotBase.t.sol @@ -556,9 +556,7 @@ contract RainDeployVerifySnapshotBaseTest is ExampleDeploySuites, RainDeployVeri DeployCandidate memory candidate = sMismatch.externalCheckedCandidateSuites()[1]; // It really is the wrong contract: the snapshot records `MockDeployableV2` - // while the contract it NAMES is `MockDeployable`. Read back through - // the same resolution the anchor uses, so this says the two really do - // differ rather than restating the fixture's own literals. + // while the contract it NAMES is `MockDeployable`. assertEq(keccak256(candidate.snapshot.creationCode), keccak256(type(MockDeployableV2).creationCode)); assertEq(candidate.snapshot.artifactPath, "test/concrete/MockDeployable.sol:MockDeployable"); assertEq(keccak256(vm.getCode(candidate.snapshot.artifactPath)), keccak256(type(MockDeployable).creationCode)); @@ -581,8 +579,7 @@ contract RainDeployVerifySnapshotBaseTest is ExampleDeploySuites, RainDeployVeri /// loop that reached it from one that reported a fixed entry or the first. /// /// The inherited `testSnapshotMatchesSource` is the passing case: it runs - /// this same function over `ExampleDeploySuites`, whose candidates really - /// are the current compilation of the contracts they name. + /// this same function over `ExampleDeploySuites`. function testWrongContractSnapshotCaughtBySource() external { DeployCandidate[] memory candidates = sMismatch.externalCheckedCandidateSuites(); assertEq(candidates.length, 2); diff --git a/test/src/abstract/RegistryDeploySuites.t.sol b/test/src/abstract/RegistryDeploySuites.t.sol index d82bcfb..8f244f3 100644 --- a/test/src/abstract/RegistryDeploySuites.t.sol +++ b/test/src/abstract/RegistryDeploySuites.t.sol @@ -20,13 +20,10 @@ import {LibRainDeploySnapshot} from "../../../src/lib/LibRainDeploySnapshot.sol" /// this suite does not have. /// /// The RECORD half of the source anchor is the other thing nothing could reach. -/// `checkCandidatesAnchoredToSource` compares the bytes a candidate records -/// against what its `artifactPath` currently compiles to, and the source half -/// is no longer anything the declaration can reach — it is read from the -/// compiler's artifact. The recorded half still is: a candidate that reads -/// `type(X).creationCode` into the recorded field puts BOTH operands on the -/// source side and compares source against itself, and on a green tree the two -/// spellings are byte-identical, so no runtime assertion can tell them apart. +/// A candidate that reads `type(X).creationCode` into the recorded field puts +/// BOTH operands of `checkCandidatesAnchoredToSource` on the source side, and on +/// a green tree the two spellings are byte-identical, so no runtime assertion +/// can tell them apart. /// The assertion below reads the compiler's AST of the declaration itself, for /// the reason `GeneratedSnapshotShapeTest` gives for reading it rather than the /// source text: this is about structure, not formatting. @@ -65,13 +62,10 @@ contract RegistryDeploySuitesTest is RegistryDeploySuites, Test { /// PROPERTY: every candidate's RECORDED creation code and runtime code are /// read from the rolling `src/generated/candidate/` snapshot. /// - /// This is the half of the source anchor that has to come from the record, - /// and since the source half stopped being a field it is the ONLY half a - /// declaration can still get wrong. Spelling it `type(X).creationCode` — to - /// drop an import, or because the record and the compiler agree today - /// anyway — puts both operands of `checkCandidatesAnchoredToSource` on the - /// source side and makes the one check that catches a snapshot of the wrong - /// contract a tautology for that candidate, with nothing red anywhere. + /// This is the half of the source anchor that has to come from the record. + /// Spelling it `type(X).creationCode` instead puts both operands of + /// `checkCandidatesAnchoredToSource` on the source side and makes that check + /// a tautology for that candidate, with nothing red anywhere. /// /// The runtime code is held to the record by the same assertion because it /// is what would be left of the record side. A candidate whose creation diff --git a/test/src/lib/GeneratedSnapshotShape.t.sol b/test/src/lib/GeneratedSnapshotShape.t.sol index 4b04a1e..655c26e 100644 --- a/test/src/lib/GeneratedSnapshotShape.t.sol +++ b/test/src/lib/GeneratedSnapshotShape.t.sol @@ -185,16 +185,9 @@ contract GeneratedSnapshotShapeTest is RegistryDeploySuites, Test { /// the path is a file this repo has, and the artifact that whole string /// selects is the contract the candidate is anchored to. /// - /// `artifactPath` is the field a candidate says which contract it is with. - /// Three things read it now: `LibRainDeploy` prints it as the - /// `forge verify-contract` command a human runs against a freshly broadcast - /// contract, `candidateContractName` above takes the contract this whole - /// shape spec is about out of it, and - /// `RainDeploySuitesBase.checkCandidatesAnchoredToSource` resolves it to - /// get the source half of the anchor. `bytecode_hash = "none"` and - /// `cbor_metadata = false` mean moving a file changes no creation code, no - /// address and no code hash, so the path itself is the only thing that - /// can be wrong about a path. + /// `bytecode_hash = "none"` and `cbor_metadata = false` mean moving a file + /// changes no creation code, no address and no code hash, so the path itself + /// is the only thing that can be wrong about a path. /// /// Resolved through `vm.getCode`, which is foundry's own resolution of a /// `:` artifact id — the same form `forge verify-contract` @@ -203,31 +196,17 @@ contract GeneratedSnapshotShapeTest is RegistryDeploySuites, Test { /// in a comment or in a longer identifier, and defeated by a declaration /// written with no space before its brace. /// - /// `vm.isFile` on the path half is the assertion here that nothing else - /// makes, and it is why this test survived the anchor learning to resolve - /// this field for itself. `vm.getCode` resolves an artifact id by path - /// SUFFIX, while the printed verification command is run from the repo - /// root: `concrete/AddressRegistry.sol:AddressRegistry` resolves for - /// `vm.getCode` — uniquely, and to the right contract, so the anchor is - /// perfectly happy with it — and still names no file anybody can point - /// `forge verify-contract` at. That is the one shape of wrong path nothing - /// else in this repo goes red on. + /// `vm.isFile` on the path half AS WELL, because `vm.getCode` resolves an + /// artifact id by path SUFFIX while the printed command is run from the + /// repo root. `concrete/AddressRegistry.sol:AddressRegistry` resolves for + /// `vm.getCode` — uniquely, and to the right contract — and still names no + /// file anybody can point `forge verify-contract` at. That is the one shape + /// of wrong path nothing else in this repo goes red on. /// /// A path outside the `fs_permissions` roots fails here as a cheatcode /// revert naming the path rather than as this assertion's own message, /// which is foundry refusing to look rather than looking and not finding. /// - /// The creation code comparison is a RESTATEMENT of - /// `checkCandidatesAnchoredToSource`, which now reads the source side out - /// of this same field through this same cheatcode, and which the BROADCAST - /// runs. It is kept because it costs nothing and because it is what makes - /// the `vm.isFile` line above readable as the one thing that is not also - /// the anchor; the guarantee it describes — two candidates whose declared - /// paths are swapped resolve perfectly, and swapping both halves leaves the - /// SET of declared names exactly as `testEveryCandidateHasASnapshot` wants - /// it — lives in the anchor now, on the path that deploys, rather than only - /// in a test. - /// /// Hashed rather than compared as bytes, for the reason /// `CandidateSourceMismatch` gives for hashing: creation codes run to tens /// of kilobytes, and a failure message carrying two of them is a failure