From d973d769eccb38281c4968b0ef0b0df8b1aadf0a Mon Sep 17 00:00:00 2001 From: giuseppere Date: Fri, 11 Sep 2026 12:33:46 +0200 Subject: [PATCH 1/6] feat(store): make StoreFactory upgradeable (UUPS) --- DEPLOYMENTS.md | 6 +- RELEASE_ARTIFACTS.md | 2 +- contracts/store/IStoreFactory.sol | 4 +- contracts/store/StoreFactory.sol | 59 ++++++++---- scripts/deploy/BaseDeployer.s.sol | 34 +++++-- scripts/deploy/DeployCore.s.sol | 12 +-- scripts/deploy/DotnsDeployer.s.sol | 8 +- scripts/deploy/WireDeployments.s.sol | 4 +- test/base/BaseDotns.t.sol | 8 +- .../unit/deploy/DeterministicDeployment.t.sol | 40 ++++---- .../unit/deploy/StoreBeaconVerification.t.sol | 96 ++++++++++++++----- test/unit/store/StoreFactory.t.sol | 68 ++++++++++++- 12 files changed, 240 insertions(+), 101 deletions(-) diff --git a/DEPLOYMENTS.md b/DEPLOYMENTS.md index 9ff0c1180..ce2a1a9a7 100644 --- a/DEPLOYMENTS.md +++ b/DEPLOYMENTS.md @@ -357,7 +357,7 @@ Choosing and changing addresses: - To intentionally move the entire address set (a clean re-deploy that must not collide with the previous one), bump `CREATE3_SALT_NAMESPACE` (`v1` becomes `v2`). Every address shifts together. - Do not reuse a `label` for a different contract. The wire stage and external tooling key off stable labels, so a reused label silently repoints them. -Two other manifest entries are not CREATE3-derived: `LabelStoreBeacon` and `UserStoreBeacon`. They are deployed inside the `StoreFactory` constructor (and owned by it, so the factory owner can upgrade store implementations), so their addresses are `keccak(StoreFactory, nonce)`. They stay put across resets while `StoreFactory`'s bytecode is unchanged, but a change to that constructor can move them. This is deliberate: only the core CREATE3 contracts are guaranteed stable, so do not treat the beacon addresses as network-stable, read them from the manifest or the factory. +Two other manifest entries are not CREATE3-derived: `LabelStoreBeacon` and `UserStoreBeacon`. They are deployed inside the `StoreFactory` initialiser, which runs by delegatecall from the proxy constructor, so they are owned by the `StoreFactory` proxy and their addresses are `keccak(StoreFactory proxy, nonce)`. Owning them from the proxy is what keeps store-implementation upgrades available across a factory upgrade: the beacons answer to an address whose logic can be replaced, rather than to the code deployed on day one. They stay put across resets while the initialiser is unchanged, but a change to it can move them. This is deliberate: only the core CREATE3 contracts are guaranteed stable, so do not treat the beacon addresses as network-stable, read them from the manifest or the factory. The one address that is not CREATE3-derived is the CREATE3 factory itself: it bootstraps the scheme, so it cannot deploy itself. The first deploy stage deploys it directly and records it on the protocol registry under the `CREATE3_FACTORY` key; every later stage resolves it from there rather than from an environment variable. Because every other address is derived from the factory's address, the factory must sit at the same address on each chain for the rest of the set to match. Deploy it as the deployer's first transaction on a fresh account (or through a deterministic singleton deployer) so its nonce-derived address is identical across chains. @@ -374,9 +374,9 @@ Matching works in two steps, in `BaseDeployer._requireExpectedCode`: The reference copies are throwaway and are deployed with broadcasting paused, so they are never sent as transactions. -That second step is what rejects a genuine artefact deployed against someone else's constructor arguments: a real `StoreFactory` pointed at an attacker's protocol registry has the right length and shape, and differs only in the values its constructor wrote. +That second step is what rejects a genuine artefact deployed against someone else's constructor arguments: a real `DotnsPopLens` pointed at an attacker's protocol registry has the right length and shape, and differs only in the values its constructor wrote. -**What the bytecode check cannot cover.** Immutables whose values are address-derived are indistinguishable between an honest deploy and any other, because they legitimately differ every time. `StoreFactory` is the case that matters: it deploys its own beacons, so `labelStoreBeacon` and `userStoreBeacon` differ on every deploy and are skipped by the comparison. Its `protocolRegistry`, which is an immutable set from a constructor argument, is part of that comparison. Its owner is not: `Ownable` keeps that in storage rather than in runtime code, so it is caught by the wire stage's `owner()` assertions instead. +**What the bytecode check cannot cover.** Immutables whose values are address-derived are indistinguishable between an honest deploy and any other, because they legitimately differ every time. Only `UUPSUpgradeable.__self` is in that class now, and every UUPS implementation carries it, so the masking handles it uniformly. `StoreFactory` used to be the case that mattered, because it minted its own beacons into immutables; behind a proxy it holds the beacons and `protocolRegistry` in storage and carries no immutables of its own, so its implementation compares exactly. Immutables set from a constructor argument stay inside the comparison, which is what rejects an artefact built against someone else's addresses: `DotnsPopLens.protocolRegistry` and `DotnsFlatPricing.deposit` are the two that remain. An owner is never covered here, since `Ownable` keeps it in storage rather than runtime code; the wire stage's `owner()` assertions cover it instead. The beacons are checked separately. The verification stage asserts that each beacon's code is the `UpgradeableBeacon` artefact, that the factory owns it, and that its implementation is the `LabelStore` or `UserStore` artefact this release builds. None of the three contracts carries immutables, so each comparison is exact. diff --git a/RELEASE_ARTIFACTS.md b/RELEASE_ARTIFACTS.md index 6f09af9c3..7a0cf8c4e 100644 --- a/RELEASE_ARTIFACTS.md +++ b/RELEASE_ARTIFACTS.md @@ -42,7 +42,7 @@ The release surface is decided in `.github/abi-contracts.txt` so a contract reac - One entry can serve more than one live network. `paseo-assethub` is the deployment that both previewnet and Paseo Asset Hub Next V2 run, because every network deployed through the shared CREATE3 factory lands on the same addresses. So expect entries named after deployments, not after every chain you might connect to. - The names under `contracts` (`DotnsRegistrar`, `PopRules`) do not change, and a name always means the same contract. Your code can depend on that. - Addresses are copied from the manifest verbatim, which the deploy pipeline writes EIP-55 checksummed. Compare them case-insensitively rather than relying on the casing. -- `LabelStoreBeacon` and `UserStoreBeacon` appear when deployed but are not network-stable, because the `StoreFactory` constructor deploys them. Read them from the factory rather than pinning them. +- `LabelStoreBeacon` and `UserStoreBeacon` appear when deployed but are not network-stable, because the `StoreFactory` initialiser deploys them. Read them from the factory rather than pinning them. ## `release-manifest.json` diff --git a/contracts/store/IStoreFactory.sol b/contracts/store/IStoreFactory.sol index 86246913b..18f256511 100644 --- a/contracts/store/IStoreFactory.sol +++ b/contracts/store/IStoreFactory.sol @@ -35,11 +35,11 @@ interface IStoreFactory { /// @param user The invalid user argument. error InvalidUser(address user); - /// @notice Thrown when a zero protocol registry address is supplied to the constructor. + /// @notice Thrown when a zero protocol registry address is supplied to the initialiser. /// @param protocolRegistry The invalid registry argument. error InvalidProtocolRegistry(address protocolRegistry); - /// @notice Thrown when a zero implementation address is supplied to the constructor or an + /// @notice Thrown when a zero implementation address is supplied to a store-implementation /// upgrade. @param implementation The invalid implementation argument. error InvalidImplementation(address implementation); diff --git a/contracts/store/StoreFactory.sol b/contracts/store/StoreFactory.sol index 1397251f0..ddfe123ed 100644 --- a/contracts/store/StoreFactory.sol +++ b/contracts/store/StoreFactory.sol @@ -1,7 +1,11 @@ // SPDX-License-Identifier: MIT pragma solidity ^0.8.34; -import {Ownable} from "@openzeppelin/contracts/access/Ownable.sol"; +import {Initializable} from "@openzeppelin/contracts-upgradeable/proxy/utils/Initializable.sol"; +import {UUPSUpgradeable} from "@openzeppelin/contracts-upgradeable/proxy/utils/UUPSUpgradeable.sol"; +import { + OwnableUpgradeable +} from "@openzeppelin/contracts-upgradeable/access/OwnableUpgradeable.sol"; import {BeaconProxy} from "@openzeppelin/contracts/proxy/beacon/BeaconProxy.sol"; import {UpgradeableBeacon} from "@openzeppelin/contracts/proxy/beacon/UpgradeableBeacon.sol"; @@ -24,22 +28,22 @@ import {StoreAuth} from "../utils/StoreAuth.sol"; /// The factory owns both beacons so the factory owner can upgrade implementations for /// every proxy atomically. Neither per-user mapping is ever transferred, reassigned, or /// overwritten after the first write; bindings are permanent. +/// @dev Lives behind its own UUPS proxy. The per-user bindings and both beacon addresses are +/// proxy storage, so the factory is upgraded in place: the bindings cannot be exported, so +/// a replacement reached by re-pointing `STORE_FACTORY` would start with an empty directory. /// @custom:security-contact admin@parity.io -contract StoreFactory is Ownable, IStoreFactory { +contract StoreFactory is Initializable, UUPSUpgradeable, OwnableUpgradeable, IStoreFactory { /// @notice Beacon backing every `LabelStore` proxy. /// @dev Public getter name is interface-constrained by @custom:contract IStoreFactory. - // forge-lint: disable-next-line(screaming-snake-case-immutable) - address public immutable override labelStoreBeacon; + address public override labelStoreBeacon; /// @notice Beacon backing every `UserStore` proxy. /// @dev Public getter name is interface-constrained by @custom:contract IStoreFactory. - // forge-lint: disable-next-line(screaming-snake-case-immutable) - address public immutable override userStoreBeacon; + address public override userStoreBeacon; /// @notice Protocol registry used to authorise `deployLabelStoreFor` callers. /// @dev Public getter name is interface-constrained by @custom:contract IStoreFactory. - // forge-lint: disable-next-line(screaming-snake-case-immutable) - address public immutable override protocolRegistry; + address public override protocolRegistry; /// @dev user => their permanent `LabelStore`. Set once per user, forever. mapping(address user => address store) private _labelStores; @@ -53,6 +57,9 @@ contract StoreFactory is Ownable, IStoreFactory { /// @dev Insertion-order list of every `UserStore` proxy ever claimed. Append-only. address[] private _userStoreList; + /// @dev Reserved storage space to allow for layout changes in future upgrades. + uint256[50] private __gap; + /// @notice Restricts `deployLabelStoreFor` to the owner or a component named in /// @custom:function StoreAuth.isStoreWriter. modifier onlyOwnerOrProtocol() { @@ -60,22 +67,29 @@ contract StoreFactory is Ownable, IStoreFactory { _; } - /// @notice Deploys the factory together with both store implementations and beacons. - /// @dev A single `new StoreFactory(protocolRegistry, owner)` call wires everything: + /// @custom:oz-upgrades-unsafe-allow constructor + constructor() { + _disableInitializers(); + } + + /// @notice Initialises the factory together with both store implementations and beacons. + /// @dev Callable exactly once via `Initializable`, otherwise + /// @custom:reverts InvalidInitialization. A single initialiser call wires everything: /// - Deploys a fresh `LabelStore` implementation. /// - Deploys a fresh `UserStore` implementation. - /// - Constructs both `UpgradeableBeacon` instances, owned by `address(this)` - /// so `upgrade*Implementation` can delegate to `beacon.upgradeTo`. - /// Keeping the implementation deployments inside the constructor removes a class of - /// operator error: there is no "did I deploy the implementation first?" step and no - /// way to pass the wrong implementation address. `protocolRegistry_` must be - /// non-zero, otherwise @custom:reverts InvalidProtocolRegistry. - /// @dev Implementations and beacons are deployed inline so a single factory address fully - /// describes the store topology, removing a class of operator error around mismatched - /// beacons. + /// - Constructs both `UpgradeableBeacon` instances, owned by `address(this)`, which + /// under the proxy is the proxy itself, so `upgrade*Implementation` can delegate to + /// `beacon.upgradeTo` and the beacons outlive any implementation swap. + /// Keeping the implementation deployments here removes a class of operator error: + /// there is no "did I deploy the implementation first?" step and no way to pass the + /// wrong implementation address. `protocolRegistry_` must be non-zero, otherwise + /// @custom:reverts InvalidProtocolRegistry. + /// @param initialOwner Account that owns this factory and can upgrade it and the store + /// implementations. /// @param protocolRegistry_ The protocol registry for writer auth on label stores. - /// @param owner_ Account that owns this factory and can upgrade store implementations. - constructor(address protocolRegistry_, address owner_) Ownable(owner_) { + function initialize(address initialOwner, address protocolRegistry_) external initializer { + __Ownable_init(initialOwner); + require(protocolRegistry_ != address(0), InvalidProtocolRegistry(protocolRegistry_)); IDotnsProtocolRegistry(protocolRegistry_).isRegisteredAddress(address(0)); @@ -191,6 +205,9 @@ contract StoreFactory is Ownable, IStoreFactory { versionString = "1.0.0"; } + /// @inheritdoc UUPSUpgradeable + function _authorizeUpgrade(address newImplementation) internal override onlyOwner {} + /// @notice Internal authorisation check deferred from the `onlyOwnerOrProtocol` modifier. function _onlyOwnerOrProtocol() internal view { if (msg.sender == owner()) return; diff --git a/scripts/deploy/BaseDeployer.s.sol b/scripts/deploy/BaseDeployer.s.sol index d7d2aa09b..6ec16afba 100644 --- a/scripts/deploy/BaseDeployer.s.sol +++ b/scripts/deploy/BaseDeployer.s.sol @@ -180,15 +180,16 @@ abstract contract BaseDeployer is Script { require(addr.code.length != 0, string.concat(name, ": no code")); } - /// @notice The store beacons point at this release's store implementations, and the factory - /// owns them. - /// @dev The one thing the CREATE3 occupancy check cannot assert. `StoreFactory` deploys its - /// own beacons, so their addresses are immutables that differ on every honest deploy and - /// are necessarily skipped when an occupant is compared against this run's artefact. An - /// attacker squatting the factory address supplies the real artefact and the real - /// constructor arguments, both public, so the beacons are the only thing left under - /// their control. `LabelStore` and `UserStore` carry no immutables, so their runtime - /// code compares exactly. + /// @notice The factory points at this release's protocol registry, and the store beacons + /// point at this release's store implementations and are owned by the factory. + /// @dev The things the CREATE3 occupancy check cannot assert. `StoreFactory` is a UUPS proxy, + /// and a proxy's runtime code is the same whatever it delegates to and whatever its + /// initialiser wrote, so an occupant compared against this run's artefact matches on + /// code alone. Its configuration lives in proxy storage and has to be read back: the + /// registry pointer here, the owner in the caller's ownership assertions, and the + /// beacons, which the initialiser mints and a squatter therefore chooses. + /// `LabelStore` and `UserStore` carry no immutables, so their runtime code compares + /// exactly. /// @dev The beacon contracts themselves are pinned by codehash first. Without that, the /// checks below only prove that whatever sits at those addresses answered `owner()` and /// `implementation()` the way this stage wanted at verification time; a bespoke contract @@ -199,7 +200,20 @@ abstract contract BaseDeployer is Script { /// anything else leaves every store on the network following an implementation the /// verified owner can never rotate. /// @param storeFactory The deployed store factory. - function _verifyStoreImplementations(address storeFactory) internal view { + /// @param protocolRegistry The protocol registry this run wired, which the factory must + /// already point at. + function _verifyStoreImplementations( + address storeFactory, + address protocolRegistry + ) + internal + view + { + require( + IStoreFactory(storeFactory).protocolRegistry() == protocolRegistry, + "StoreFactory: wrong protocol registry" + ); + address labelBeacon = IStoreFactory(storeFactory).labelStoreBeacon(); address userBeacon = IStoreFactory(storeFactory).userStoreBeacon(); diff --git a/scripts/deploy/DeployCore.s.sol b/scripts/deploy/DeployCore.s.sol index 9ad1f2c96..d57b97b7e 100644 --- a/scripts/deploy/DeployCore.s.sol +++ b/scripts/deploy/DeployCore.s.sol @@ -16,10 +16,10 @@ import {Multicall3} from "../../contracts/utils/Multicall3.sol"; /// @notice First stage of the DotNS fresh-deploy pipeline. Bootstraps the /// CREATE3 factory, deploys the protocol registry through it, records /// the factory on the registry, then deploys the foundational -/// name-ownership layer: the Store factory and three UUPS proxies -/// (registrar, reverse resolver, forward registry) that all bind to the -/// protocol registry at init, plus the generic Multicall3 helper for -/// client and tooling batching. +/// name-ownership layer: four UUPS proxies (store factory, registrar, +/// reverse resolver, forward registry) that all bind to the protocol +/// registry at init, plus the generic Multicall3 helper for client and +/// tooling batching. /// @dev Runs in its own `forge script` process; the OpenZeppelin validator's /// per-call memory never crosses the process boundary into later stages. /// @custom:security-contact admin@parity.io @@ -56,10 +56,10 @@ contract DeployCore is BaseDeployer { function _deployStoreFactory(address owner, address protocolRegistry) internal { StoreFactory factory = StoreFactory( - _broadcastDeployCreate3( + _broadcastDeployUups( owner, "StoreFactory.sol:StoreFactory", - abi.encode(protocolRegistry, owner), + abi.encodeCall(StoreFactory.initialize, (owner, protocolRegistry)), "StoreFactory" ) ); diff --git a/scripts/deploy/DotnsDeployer.s.sol b/scripts/deploy/DotnsDeployer.s.sol index e7ff5a795..79825363b 100644 --- a/scripts/deploy/DotnsDeployer.s.sol +++ b/scripts/deploy/DotnsDeployer.s.sol @@ -163,10 +163,10 @@ contract DotnsDeployer is BaseDeployer { returns (address proxy) { storeFactory = StoreFactory( - _broadcastDeployCreate3( + _broadcastDeployUups( owner, "StoreFactory.sol:StoreFactory", - abi.encode(protocolRegistryProxy, owner), + abi.encodeCall(StoreFactory.initialize, (owner, protocolRegistryProxy)), "StoreFactory" ) ); @@ -466,7 +466,7 @@ contract DotnsDeployer is BaseDeployer { _verifyRegistryPointers(deployment); _verifyControllerAuthorisation(deployment); - _verifyStoreImplementations(deployment.storeFactory); + _verifyStoreImplementations(deployment.storeFactory, address(protocolRegistry)); require(DotnsRegistry(deployment.registry).recordExists(bytes32(0)), "Root record missing"); require( @@ -613,6 +613,8 @@ contract DotnsDeployer is BaseDeployer { expected, "NameWhitelist: not wired" ); + // StoreFactory's pointer is asserted in `_verifyStoreImplementations`, with the rest of + // what has to be read out of its proxy storage. } function _assertPointer(address actual, address expected, string memory label) internal pure { diff --git a/scripts/deploy/WireDeployments.s.sol b/scripts/deploy/WireDeployments.s.sol index 76761c706..23a32981e 100644 --- a/scripts/deploy/WireDeployments.s.sol +++ b/scripts/deploy/WireDeployments.s.sol @@ -156,8 +156,6 @@ contract WireDeployments is BaseDeployer { DotnsProtocolRegistry(addr.protocolRegistry).owner() == expectedOwner, "ProtocolRegistry: wrong owner" ); - // Ownable rather than UUPS, so it sits outside the proxy list above, but it - // owns the beacons behind every user store and belongs in the same check. require( StoreFactory(addr.storeFactory).owner() == expectedOwner, "StoreFactory: wrong owner" ); @@ -201,7 +199,7 @@ contract WireDeployments is BaseDeployer { "PopController: not authorised" ); - _verifyStoreImplementations(addr.storeFactory); + _verifyStoreImplementations(addr.storeFactory, addr.protocolRegistry); console.log("=== Deployment verification complete ==="); } diff --git a/test/base/BaseDotns.t.sol b/test/base/BaseDotns.t.sol index d77a6c6a1..2d771ca59 100644 --- a/test/base/BaseDotns.t.sol +++ b/test/base/BaseDotns.t.sol @@ -228,8 +228,12 @@ abstract contract BaseDotns is Test { assertEq(protocolRegistry.tld(), string.concat(".", TLD_LABEL)); IDotnsProtocolRegistry registry = IDotnsProtocolRegistry(protocolRegistryAddress); - storeFactory = new StoreFactory(protocolRegistryAddress, owner); - vm.label(address(storeFactory), "StoreFactory"); + address storeFactoryAddress = Upgrades.deployUUPSProxy( + "StoreFactory.sol:StoreFactory", + abi.encodeCall(StoreFactory.initialize, (owner, protocolRegistryAddress)) + ); + storeFactory = StoreFactory(storeFactoryAddress); + vm.label(storeFactoryAddress, "StoreFactory"); address dotnsRegistrarAddress = Upgrades.deployUUPSProxy( "DotnsRegistrar.sol:DotnsRegistrar", diff --git a/test/unit/deploy/DeterministicDeployment.t.sol b/test/unit/deploy/DeterministicDeployment.t.sol index 70ba4f8b9..50f9974b1 100644 --- a/test/unit/deploy/DeterministicDeployment.t.sol +++ b/test/unit/deploy/DeterministicDeployment.t.sol @@ -4,6 +4,7 @@ pragma solidity ^0.8.34; import {Test} from "forge-std/Test.sol"; import {Create3Factory} from "../../../contracts/deploy/Create3Factory.sol"; +import {DotnsPopLens} from "../../../contracts/registrars/DotnsPopLens.sol"; import {DotnsRegistrar} from "../../../contracts/registrars/DotnsRegistrar.sol"; import {DotnsRegistry} from "../../../contracts/registry/DotnsRegistry.sol"; import {DotnsProtocolRegistry} from "../../../contracts/registry/DotnsProtocolRegistry.sol"; @@ -82,15 +83,14 @@ contract DeterministicDeploymentTest is Test { address realRegistry = address(new DotnsProtocolRegistry()); address foreignRegistry = address(new DotnsProtocolRegistry()); - bytes32 salt = deployer.create3Salt("StoreFactory", "contract"); + bytes32 salt = deployer.create3Salt("DotnsPopLens", "contract"); vm.prank(attacker); factory.deploy( - salt, - abi.encodePacked(type(StoreFactory).creationCode, abi.encode(foreignRegistry, attacker)) + salt, abi.encodePacked(type(DotnsPopLens).creationCode, abi.encode(foreignRegistry)) ); _assertAdoptionRejected( - "StoreFactory.sol:StoreFactory", abi.encode(realRegistry, owner), "StoreFactory" + "DotnsPopLens.sol:DotnsPopLens", abi.encode(realRegistry), "DotnsPopLens" ); } @@ -107,9 +107,10 @@ contract DeterministicDeploymentTest is Test { bytes memory first = template; bytes memory second = bytes.concat(template); - // `StoreFactory`'s first immutable range: 32 bytes at 509. Differ in exactly one byte, - // as two addresses sharing every other byte in that word would. - uint256 start = 509; + // `StoreFactory`'s first immutable range: 32 bytes at 1882, one of the two sites + // `UUPSUpgradeable.__self` is read from. Differ in exactly one byte, as two addresses + // sharing every other byte in that word would. + uint256 start = 1882; uint256 length = 32; second[start + 21] = second[start + 21] == bytes1(0x01) ? bytes1(0x02) : bytes1(0x01); @@ -126,20 +127,15 @@ contract DeterministicDeploymentTest is Test { /// @notice A resumed run adopts its own earlier deployment of an artefact carrying /// immutables. The reject cases below exercise the reference-diff path; this is the /// one that proves it still says yes to an honest resume. - /// @dev `StoreFactory` is the demanding case: its constructor deploys fresh beacons every - /// time, so the second run's reference copies differ from the occupant exactly where - /// the comparison must skip. A check that compared those bytes would force a salt bump - /// on every interrupted run. + /// @dev `StoreFactory` is the demanding case: as a UUPS implementation it bakes its own + /// address into `UUPSUpgradeable.__self`, so the second run's reference copies differ + /// from the occupant exactly where the comparison must skip. A check that compared + /// those bytes would force a salt bump on every interrupted run. function test_resume_adopts_an_immutable_carrying_artefact() public { - address protocolRegistry = address(new DotnsProtocolRegistry()); - bytes memory constructorData = abi.encode(protocolRegistry, owner); - - address first = deployer.deployCreate3( - owner, "StoreFactory.sol:StoreFactory", constructorData, "StoreFactory" - ); - address second = deployer.deployCreate3( - owner, "StoreFactory.sol:StoreFactory", constructorData, "StoreFactory" - ); + address first = + deployer.deployCreate3(owner, "StoreFactory.sol:StoreFactory", "", "StoreFactory"); + address second = + deployer.deployCreate3(owner, "StoreFactory.sol:StoreFactory", "", "StoreFactory"); assertEq(second, first, "an honest resume of an immutable artefact was not adopted"); } @@ -434,10 +430,10 @@ contract DeterministicDeploymentTest is Test { addr.multicall3 = deployer.deployCreate3(owner, "Multicall3.sol:Multicall3", bytes(""), "Multicall3"); - addr.storeFactory = deployer.deployCreate3( + addr.storeFactory = deployer.deployUups( owner, "StoreFactory.sol:StoreFactory", - abi.encode(addr.protocolRegistry, owner), + abi.encodeCall(StoreFactory.initialize, (owner, addr.protocolRegistry)), "StoreFactory" ); diff --git a/test/unit/deploy/StoreBeaconVerification.t.sol b/test/unit/deploy/StoreBeaconVerification.t.sol index f0670864d..1be776131 100644 --- a/test/unit/deploy/StoreBeaconVerification.t.sol +++ b/test/unit/deploy/StoreBeaconVerification.t.sol @@ -3,6 +3,7 @@ pragma solidity ^0.8.34; import {Test} from "forge-std/Test.sol"; import {UpgradeableBeacon} from "@openzeppelin/contracts/proxy/beacon/UpgradeableBeacon.sol"; +import {ERC1967Proxy} from "@openzeppelin/contracts/proxy/ERC1967/ERC1967Proxy.sol"; import {WireDeployments} from "../../../scripts/deploy/WireDeployments.s.sol"; import {DotnsProtocolRegistry} from "../../../contracts/registry/DotnsProtocolRegistry.sol"; @@ -11,8 +12,14 @@ import {StoreFactory} from "../../../contracts/store/StoreFactory.sol"; /// @notice Exposes the wire stage's beacon check on its own. contract WireVerifyHarness is WireDeployments { - function verifyStoreImplementations(address storeFactory) external view { - _verifyStoreImplementations(storeFactory); + function verifyStoreImplementations( + address storeFactory, + address protocolRegistry + ) + external + view + { + _verifyStoreImplementations(storeFactory, protocolRegistry); } } @@ -21,10 +28,12 @@ contract WireVerifyHarness is WireDeployments { contract ForeignImplFactory { address public labelStoreBeacon; address public userStoreBeacon; + address public protocolRegistry; - constructor(address labelImpl, address userImpl) { + constructor(address labelImpl, address userImpl, address protocolRegistry_) { labelStoreBeacon = address(new UpgradeableBeacon(labelImpl, address(this))); userStoreBeacon = address(new UpgradeableBeacon(userImpl, address(this))); + protocolRegistry = protocolRegistry_; } } @@ -32,10 +41,12 @@ contract ForeignImplFactory { contract PrebuiltBeaconFactory { address public labelStoreBeacon; address public userStoreBeacon; + address public protocolRegistry; - constructor(address labelBeacon, address userBeacon) { + constructor(address labelBeacon, address userBeacon, address protocolRegistry_) { labelStoreBeacon = labelBeacon; userStoreBeacon = userBeacon; + protocolRegistry = protocolRegistry_; } } @@ -69,23 +80,25 @@ contract ForeignStore { /// @title StoreBeaconVerificationTests /// @notice Covers the one property the CREATE3 occupancy check cannot assert: which /// implementations the store beacons point at, and who owns the beacons. -/// @dev `StoreFactory` deploys its own beacons, so their addresses are immutables that differ on -/// every honest deploy and are skipped when an occupant is compared byte for byte. An -/// attacker adopting the factory address uses the real artefact and the real, public -/// constructor arguments, so the beacons are the only thing left under their control. +/// @dev `StoreFactory` is a UUPS proxy, and a proxy's runtime code says nothing about what its +/// initialiser wrote, so an occupant compared byte for byte matches on code alone. Everything +/// a squatter controls, the registry pointer and the beacons, lives in proxy storage and has +/// to be read back. contract StoreBeaconVerificationTests is Test { WireVerifyHarness private wire; address private owner; + address private registry; function setUp() public { wire = new WireVerifyHarness(); owner = makeAddr("wire-owner"); + registry = address(new DotnsProtocolRegistry()); } /// @notice An honestly deployed factory passes. function test_accepts_an_honestly_deployed_factory() public { - StoreFactory factory = new StoreFactory(address(_registry()), owner); - wire.verifyStoreImplementations(address(factory)); + StoreFactory factory = _honestFactory(); + wire.verifyStoreImplementations(address(factory), registry); } /// @notice Beacons pointing at implementations this release did not build are rejected. @@ -93,30 +106,31 @@ contract StoreBeaconVerificationTests is Test { /// comparison is what fires. This is the attacker's actual position: everything else /// about their factory can be made to look right. function test_rejects_foreign_store_implementations() public { - ForeignImplFactory factory = - new ForeignImplFactory(address(new ForeignStore()), address(new ForeignStore())); + ForeignImplFactory factory = new ForeignImplFactory( + address(new ForeignStore()), address(new ForeignStore()), registry + ); vm.expectRevert(bytes("LabelStoreBeacon: unexpected implementation")); - wire.verifyStoreImplementations(address(factory)); + wire.verifyStoreImplementations(address(factory), registry); } /// @notice A correct label store with a foreign user store is still rejected, so the second /// beacon is not left unchecked once the first passes. function test_rejects_a_foreign_user_store_alone() public { - StoreFactory honest = new StoreFactory(address(_registry()), owner); + StoreFactory honest = _honestFactory(); address realLabelImpl = UpgradeableBeacon(honest.labelStoreBeacon()).implementation(); ForeignImplFactory factory = - new ForeignImplFactory(realLabelImpl, address(new ForeignStore())); + new ForeignImplFactory(realLabelImpl, address(new ForeignStore()), registry); vm.expectRevert(bytes("UserStoreBeacon: unexpected implementation")); - wire.verifyStoreImplementations(address(factory)); + wire.verifyStoreImplementations(address(factory), registry); } /// @notice A beacon the factory does not own is rejected: the verified factory owner could /// never rotate the store implementations, and nothing else would show it. function test_rejects_a_beacon_the_factory_does_not_own() public { - StoreFactory honest = new StoreFactory(address(_registry()), owner); + StoreFactory honest = _honestFactory(); address realLabelImpl = UpgradeableBeacon(honest.labelStoreBeacon()).implementation(); address realUserImpl = UpgradeableBeacon(honest.userStoreBeacon()).implementation(); @@ -124,10 +138,10 @@ contract StoreBeaconVerificationTests is Test { address outsider = makeAddr("outsider"); address labelBeacon = address(new UpgradeableBeacon(realLabelImpl, outsider)); address userBeacon = address(new UpgradeableBeacon(realUserImpl, outsider)); - PrebuiltBeaconFactory factory = new PrebuiltBeaconFactory(labelBeacon, userBeacon); + PrebuiltBeaconFactory factory = new PrebuiltBeaconFactory(labelBeacon, userBeacon, registry); vm.expectRevert(bytes("LabelStoreBeacon: not owned by the factory")); - wire.verifyStoreImplementations(address(factory)); + wire.verifyStoreImplementations(address(factory), registry); } /// @notice A beacon that is not `UpgradeableBeacon` is rejected even when it answers both @@ -136,22 +150,56 @@ contract StoreBeaconVerificationTests is Test { /// at verification time. This one reports the release's implementation and the factory /// as owner, passes both semantic checks, and can be repointed immediately afterwards. function test_rejects_a_beacon_that_merely_answers_the_views() public { - StoreFactory honest = new StoreFactory(address(_registry()), owner); + StoreFactory honest = _honestFactory(); address realLabelImpl = UpgradeableBeacon(honest.labelStoreBeacon()).implementation(); address realUserImpl = UpgradeableBeacon(honest.userStoreBeacon()).implementation(); address factory = makeAddr("factory"); address labelBeacon = address(new LyingBeacon(realLabelImpl, factory)); address userBeacon = address(new LyingBeacon(realUserImpl, factory)); - vm.etch(factory, address(new PrebuiltBeaconFactory(labelBeacon, userBeacon)).code); + vm.etch(factory, address(new PrebuiltBeaconFactory(labelBeacon, userBeacon, registry)).code); vm.store(factory, bytes32(uint256(0)), bytes32(uint256(uint160(labelBeacon)))); vm.store(factory, bytes32(uint256(1)), bytes32(uint256(uint160(userBeacon)))); + // `vm.etch` copies code, not storage, so the registry pointer has to be planted too or + // this fixture would trip the pointer check before reaching the beacon check under test. + vm.store(factory, bytes32(uint256(2)), bytes32(uint256(uint160(registry)))); vm.expectRevert(bytes("LabelStoreBeacon: unexpected beacon code")); - wire.verifyStoreImplementations(factory); + wire.verifyStoreImplementations(factory, registry); + } + + /// @notice A factory built from this release's artefacts but initialised against someone + /// else's registry is rejected. + /// @dev Every beacon assertion still passes here: the factory really did mint its own beacons + /// off the real implementations. Only the pointer is wrong. + function test_rejects_a_factory_initialised_against_a_foreign_registry() public { + address foreignRegistry = address(new DotnsProtocolRegistry()); + StoreFactory squat = StoreFactory( + address( + new ERC1967Proxy( + address(new StoreFactory()), + abi.encodeCall(StoreFactory.initialize, (owner, foreignRegistry)) + ) + ) + ); + + // The beacon topology is genuine, so nothing else in the check would object. + assertEq(UpgradeableBeacon(squat.labelStoreBeacon()).owner(), address(squat)); + assertEq(UpgradeableBeacon(squat.userStoreBeacon()).owner(), address(squat)); + + vm.expectRevert(bytes("StoreFactory: wrong protocol registry")); + wire.verifyStoreImplementations(address(squat), registry); } - function _registry() private returns (DotnsProtocolRegistry registry) { - registry = new DotnsProtocolRegistry(); + /// @notice A real factory deployed the way the pipeline deploys it, behind its own UUPS proxy. + function _honestFactory() private returns (StoreFactory factory) { + factory = StoreFactory( + address( + new ERC1967Proxy( + address(new StoreFactory()), + abi.encodeCall(StoreFactory.initialize, (owner, registry)) + ) + ) + ); } } diff --git a/test/unit/store/StoreFactory.t.sol b/test/unit/store/StoreFactory.t.sol index 2ab7acd52..20fd8e708 100644 --- a/test/unit/store/StoreFactory.t.sol +++ b/test/unit/store/StoreFactory.t.sol @@ -9,6 +9,8 @@ import {ILabelStore} from "../../../contracts/store/ILabelStore.sol"; import {IUserStore} from "../../../contracts/store/IUserStore.sol"; import {UpgradeableBeacon} from "@openzeppelin/contracts/proxy/beacon/UpgradeableBeacon.sol"; import {Ownable} from "@openzeppelin/contracts/access/Ownable.sol"; +import {ERC1967Proxy} from "@openzeppelin/contracts/proxy/ERC1967/ERC1967Proxy.sol"; +import {Initializable} from "@openzeppelin/contracts-upgradeable/proxy/utils/Initializable.sol"; /// @title LabelStoreV2 /// @notice Test-only LabelStore implementation extended with a version marker, used to verify @@ -25,21 +27,79 @@ contract LabelStoreV2 is LabelStore { /// @notice Unit tests for StoreFactory: beacon wiring, authorisation, deployment and claim flows, /// beacon upgrades, and enumeration. contract StoreFactoryTests is BaseDotns { - function test_constructor_reverts_on_zero_registry() public { + function test_initialize_reverts_on_zero_registry() public { + address implementation = address(new StoreFactory()); vm.expectRevert( abi.encodeWithSelector(IStoreFactory.InvalidProtocolRegistry.selector, address(0)) ); - new StoreFactory(address(0), owner); + new ERC1967Proxy( + implementation, abi.encodeCall(StoreFactory.initialize, (owner, address(0))) + ); } - function test_constructor_deploys_both_beacons_and_implementations() public { - StoreFactory fresh = new StoreFactory(address(protocolRegistry), owner); + function test_initialize_deploys_both_beacons_and_implementations() public { + StoreFactory fresh = _freshFactory(); assertTrue(fresh.labelStoreBeacon() != address(0)); assertTrue(fresh.userStoreBeacon() != address(0)); assertTrue(fresh.labelStoreBeacon() != fresh.userStoreBeacon()); assertEq(fresh.owner(), owner); } + /// @notice The beacons belong to the proxy, not to the implementation that minted them. + /// @dev Owning them anywhere else would strand every store-implementation upgrade on a + /// contract the pipeline never wires in. + function test_initialize_mints_beacons_owned_by_the_proxy() public { + StoreFactory fresh = _freshFactory(); + assertEq(UpgradeableBeacon(fresh.labelStoreBeacon()).owner(), address(fresh)); + assertEq(UpgradeableBeacon(fresh.userStoreBeacon()).owner(), address(fresh)); + } + + function test_initialize_reverts_on_second_call() public { + StoreFactory fresh = _freshFactory(); + vm.expectRevert(Initializable.InvalidInitialization.selector); + fresh.initialize(owner, address(protocolRegistry)); + } + + /// @notice The implementation is inert: its initialisers are disabled at construction, so a + /// third party cannot claim it and mint beacons it controls. + function test_implementation_cannot_be_initialised_directly() public { + StoreFactory implementation = new StoreFactory(); + vm.expectRevert(Initializable.InvalidInitialization.selector); + implementation.initialize(owner, address(protocolRegistry)); + } + + /// @notice The factory is upgraded in place, and only by its owner, carrying its bindings + /// and beacons across. + function test_upgrade_is_owner_gated_and_preserves_bindings() public { + vm.prank(owner); + address store = storeFactory.deployLabelStoreFor(ed); + address beacon = storeFactory.labelStoreBeacon(); + + address newImplementation = address(new StoreFactory()); + vm.prank(ed); + vm.expectRevert(abi.encodeWithSelector(Ownable.OwnableUnauthorizedAccount.selector, ed)); + storeFactory.upgradeToAndCall(newImplementation, bytes("")); + + vm.prank(owner); + storeFactory.upgradeToAndCall(newImplementation, bytes("")); + + assertEq(storeFactory.getLabelStore(ed), store, "binding survived the upgrade"); + assertEq(storeFactory.labelStoreBeacon(), beacon, "beacon survived the upgrade"); + assertEq(storeFactory.protocolRegistry(), address(protocolRegistry)); + } + + /// @notice Deploys a factory the way the pipeline does, behind its own ERC1967 proxy. + function _freshFactory() private returns (StoreFactory fresh) { + fresh = StoreFactory( + address( + new ERC1967Proxy( + address(new StoreFactory()), + abi.encodeCall(StoreFactory.initialize, (owner, address(protocolRegistry))) + ) + ) + ); + } + function test_beacon_owner_is_factory_for_both_beacons() public view { assertEq(UpgradeableBeacon(storeFactory.labelStoreBeacon()).owner(), address(storeFactory)); assertEq(UpgradeableBeacon(storeFactory.userStoreBeacon()).owner(), address(storeFactory)); From f80e6d077428c7ad4b4ed042a171a7f875a95703 Mon Sep 17 00:00:00 2001 From: giuseppere Date: Fri, 11 Sep 2026 12:43:08 +0200 Subject: [PATCH 2/6] comment nit --- contracts/store/StoreFactory.sol | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/contracts/store/StoreFactory.sol b/contracts/store/StoreFactory.sol index ddfe123ed..2e0ae8fc3 100644 --- a/contracts/store/StoreFactory.sol +++ b/contracts/store/StoreFactory.sol @@ -80,10 +80,10 @@ contract StoreFactory is Initializable, UUPSUpgradeable, OwnableUpgradeable, ISt /// - Constructs both `UpgradeableBeacon` instances, owned by `address(this)`, which /// under the proxy is the proxy itself, so `upgrade*Implementation` can delegate to /// `beacon.upgradeTo` and the beacons outlive any implementation swap. - /// Keeping the implementation deployments here removes a class of operator error: - /// there is no "did I deploy the implementation first?" step and no way to pass the - /// wrong implementation address. `protocolRegistry_` must be non-zero, otherwise - /// @custom:reverts InvalidProtocolRegistry. + /// The implementations are deployed here rather than accepted as parameters, so the call + /// carries no ordering dependency on a prior deploy and exposes no argument through which + /// a mismatched implementation could reach a beacon. `protocolRegistry_` must be + /// non-zero, otherwise @custom:reverts InvalidProtocolRegistry. /// @param initialOwner Account that owns this factory and can upgrade it and the store /// implementations. /// @param protocolRegistry_ The protocol registry for writer auth on label stores. From e1be064b5d2696cc801deaea46afdd35055976a6 Mon Sep 17 00:00:00 2001 From: giuseppere Date: Fri, 11 Sep 2026 13:59:32 +0200 Subject: [PATCH 3/6] update contract addresses --- deployments/paseo-assethub/420420417.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/deployments/paseo-assethub/420420417.json b/deployments/paseo-assethub/420420417.json index bbb35225e..29c4aacfb 100644 --- a/deployments/paseo-assethub/420420417.json +++ b/deployments/paseo-assethub/420420417.json @@ -1 +1 @@ -{"Create3Factory":"0x8533c79E058c5a6489CAFeCA86dc600E029D75f5","DotnsContentResolver":"0x7F74D7CD50f5a834270E2ad395a01b01891AB37d","DotnsCostModelRegistry":"0x8bfd1f0957e73716732e725802f13830B5682da4","DotnsFlatPricing":"0xD839B281dF72Df44fF275305E72cAEEc0fDAA648","DotnsNameEscrow":"0x4881Afb78e7C908cAe818168B926229D93376520","DotnsNameWhitelist":"0x420166cD67Ca0233094E492a4BbA67045eD7C38C","DotnsPopController":"0xCC932348606cc1f3318cADeC5A5Cd2CA447f8a4b","DotnsPopLens":"0xAE374b07c7e6f473CBa21d57e36AC15C631Abc51","DotnsPopResolver":"0xDaC984884EcA8Fc44011f1D6C49B27828390A72B","DotnsProtocolRegistry":"0xD19e3D0C97CF501125a04A97405e3e6592fa846E","DotnsRegistrar":"0x4f06E818Ba3d987704fd91cf3d868E4b019106Ab","DotnsRegistrarController":"0xBdaA01bD1bA67d709F2b1fF286Da0d854977EA30","DotnsRegistry":"0xf34054fd76BbF85f216cf9908226D5f0A72E50CA","DotnsResolver":"0xbd1165E549DF96F083c0A16f61590927bC187009","DotnsReverseResolver":"0xee3883d7eB60Ee9BCD7F3bcD8f2f05302A9Cc035","LabelStoreBeacon":"0xb57Ebc2e7085616d4906D1fE49af1cE13f7dffeF","Multicall3":"0xB4468000abD87D3c56cbFBd153161223D7b109e5","PopRules":"0x747B456bE03aec0b42bd85C51513730FBD45DA31","StoreFactory":"0x709A027F446a9e2a4BB9cb9a9c754435b19e32B7","UserStoreBeacon":"0xb7C995601679840d36F37E86DB2d7dF30797eC5C","_seed":"0x0000000000000000000000000000000000000000"} +{"Create3Factory":"0x8533c79E058c5a6489CAFeCA86dc600E029D75f5","DotnsContentResolver":"0x7F74D7CD50f5a834270E2ad395a01b01891AB37d","DotnsCostModelRegistry":"0x8bfd1f0957e73716732e725802f13830B5682da4","DotnsFlatPricing":"0xD839B281dF72Df44fF275305E72cAEEc0fDAA648","DotnsNameEscrow":"0x4881Afb78e7C908cAe818168B926229D93376520","DotnsNameWhitelist":"0x420166cD67Ca0233094E492a4BbA67045eD7C38C","DotnsPopController":"0xCC932348606cc1f3318cADeC5A5Cd2CA447f8a4b","DotnsPopLens":"0xAE374b07c7e6f473CBa21d57e36AC15C631Abc51","DotnsPopResolver":"0xDaC984884EcA8Fc44011f1D6C49B27828390A72B","DotnsProtocolRegistry":"0xD19e3D0C97CF501125a04A97405e3e6592fa846E","DotnsRegistrar":"0x4f06E818Ba3d987704fd91cf3d868E4b019106Ab","DotnsRegistrarController":"0xBdaA01bD1bA67d709F2b1fF286Da0d854977EA30","DotnsRegistry":"0xf34054fd76BbF85f216cf9908226D5f0A72E50CA","DotnsResolver":"0xbd1165E549DF96F083c0A16f61590927bC187009","DotnsReverseResolver":"0xee3883d7eB60Ee9BCD7F3bcD8f2f05302A9Cc035","LabelStoreBeacon":"0x2227d9807F5A71332Aaa0640643030f2A3bf84cD","Multicall3":"0xB4468000abD87D3c56cbFBd153161223D7b109e5","PopRules":"0x747B456bE03aec0b42bd85C51513730FBD45DA31","StoreFactory":"0x99605a926FcB40aB520F659c6505E5ff862771f6","UserStoreBeacon":"0x3d1Ca165f7A5e387C2df02DB2FadD3149c1C72ad","_seed":"0x0000000000000000000000000000000000000000"} From 54913bc3578333f915d1ea31fc28adbaadcf477d Mon Sep 17 00:00:00 2001 From: giuseppere Date: Sun, 13 Sep 2026 04:59:16 +0200 Subject: [PATCH 4/6] CR fixes --- contracts/store/IStoreFactory.sol | 3 ++- contracts/store/StoreFactory.sol | 2 ++ scripts/deploy/DeployCore.s.sol | 3 +++ test/unit/store/StoreFactory.t.sol | 12 ++++++++++++ 4 files changed, 19 insertions(+), 1 deletion(-) diff --git a/contracts/store/IStoreFactory.sol b/contracts/store/IStoreFactory.sol index 18f256511..8087590d3 100644 --- a/contracts/store/IStoreFactory.sol +++ b/contracts/store/IStoreFactory.sol @@ -40,7 +40,8 @@ interface IStoreFactory { error InvalidProtocolRegistry(address protocolRegistry); /// @notice Thrown when a zero implementation address is supplied to a store-implementation - /// upgrade. @param implementation The invalid implementation argument. + /// upgrade. + /// @param implementation The invalid implementation argument. error InvalidImplementation(address implementation); /// @notice Thrown when an unauthorised address attempts to deploy a label store. diff --git a/contracts/store/StoreFactory.sol b/contracts/store/StoreFactory.sol index 2e0ae8fc3..2ee94cdc0 100644 --- a/contracts/store/StoreFactory.sol +++ b/contracts/store/StoreFactory.sol @@ -91,6 +91,8 @@ contract StoreFactory is Initializable, UUPSUpgradeable, OwnableUpgradeable, ISt __Ownable_init(initialOwner); require(protocolRegistry_ != address(0), InvalidProtocolRegistry(protocolRegistry_)); + // Probing the registry rejects a wrong address here rather than at the first store deploy, + // and is what catches the two address arguments being passed the wrong way round. IDotnsProtocolRegistry(protocolRegistry_).isRegisteredAddress(address(0)); protocolRegistry = protocolRegistry_; diff --git a/scripts/deploy/DeployCore.s.sol b/scripts/deploy/DeployCore.s.sol index d57b97b7e..d2508b0b1 100644 --- a/scripts/deploy/DeployCore.s.sol +++ b/scripts/deploy/DeployCore.s.sol @@ -68,6 +68,9 @@ contract DeployCore is BaseDeployer { vm.label(factory.userStoreBeacon(), "UserStoreBeacon"); logDeployment("LabelStoreBeacon", factory.labelStoreBeacon()); logDeployment("UserStoreBeacon", factory.userStoreBeacon()); + // Asserted here because this stage can run without the wire stage. Both initialiser + // arguments are addresses, so reading the registry back is what catches a transposed pair. + _verifyStoreImplementations(address(factory), protocolRegistry); } function _deployMulticall3(address owner) internal { diff --git a/test/unit/store/StoreFactory.t.sol b/test/unit/store/StoreFactory.t.sol index 20fd8e708..fb4e155b4 100644 --- a/test/unit/store/StoreFactory.t.sol +++ b/test/unit/store/StoreFactory.t.sol @@ -86,6 +86,18 @@ contract StoreFactoryTests is BaseDotns { assertEq(storeFactory.getLabelStore(ed), store, "binding survived the upgrade"); assertEq(storeFactory.labelStoreBeacon(), beacon, "beacon survived the upgrade"); assertEq(storeFactory.protocolRegistry(), address(protocolRegistry)); + + // Beacon ownership sits with the proxy, so the upgraded logic must still be able to + // rotate the implementation. + address labelStoreV2 = address(new LabelStoreV2()); + vm.prank(owner); + storeFactory.upgradeLabelStoreImplementation(labelStoreV2); + assertEq( + UpgradeableBeacon(beacon).implementation(), + labelStoreV2, + "the upgraded factory still owns its beacon" + ); + assertEq(LabelStoreV2(store).versionMarker(), "v2", "the live store follows the rotation"); } /// @notice Deploys a factory the way the pipeline does, behind its own ERC1967 proxy. From e1bb36d0950193dccd51dbd9901c38c5b1a171ef Mon Sep 17 00:00:00 2001 From: giuseppere Date: Tue, 15 Sep 2026 01:18:49 +0200 Subject: [PATCH 5/6] Add `expected.json` for expected address checks --- .github/workflows/deploy-contracts.yml | 42 ++++++++++++----------- CONTRIBUTING.md | 2 ++ DEPLOYMENTS.md | 10 ++++++ RELEASE_ARTIFACTS.md | 1 + deployments/expected.json | 22 ++++++++++++ deployments/paseo-assethub/420420417.json | 2 +- scripts/genesis/build-genesis.sh | 29 +++++++++------- 7 files changed, 74 insertions(+), 34 deletions(-) create mode 100644 deployments/expected.json diff --git a/.github/workflows/deploy-contracts.yml b/.github/workflows/deploy-contracts.yml index fc20fe8b9..1dfdb6323 100644 --- a/.github/workflows/deploy-contracts.yml +++ b/.github/workflows/deploy-contracts.yml @@ -26,10 +26,12 @@ jobs: # Shared across steps. ACCOUNT_* is anvil test account 7 (public test keys, # never valid on a real network) used to run the pipeline. FACTORY_DEPLOYER is # the public address of the single-purpose factory key. CANONICAL is the - # committed manifest, the address set this deploy has to reproduce; MANIFEST is + # committed expected-address set this deploy has to reproduce: what a fresh + # deploy of this revision lands through the pinned factory (see "Two address + # files, two roles" in DEPLOYMENTS.md). MANIFEST is # the file this CI deploy writes. PINNED_FACTORY is read out of CANONICAL after # checkout rather than repeated here, so the expectation cannot drift from the - # manifest it is meant to describe. PRIVATE_KEY is deliberately absent: it is set + # file it is meant to describe. PRIVATE_KEY is deliberately absent: it is set # only on the first deploy to import the keystore, and must not reach the resume # run, which reuses the already-imported account. env: @@ -40,7 +42,7 @@ jobs: # any CREATE3 address, so the reproduction still lands the canonical set. DOTNS_TLD: dot FACTORY_DEPLOYER: "0xd498F7BC5bB3cBdd0068c3deEbbd814b69C3F164" - CANONICAL: deployments/paseo-assethub/420420417.json + CANONICAL: deployments/expected.json MANIFEST: deployments/paseo-local/420420420.json steps: @@ -273,24 +275,24 @@ jobs: exit 1 fi - # This CI deploy reproduces the published address set. The canonical factory + # This CI deploy reproduces the expected address set. The canonical factory # was deployed above, and every DotNS address is a pure function of that # factory plus a fixed salt, so the freshly deployed manifest must equal the - # committed manifest. Assert that, print the expected-vs-actual table, then + # committed expected set. Assert that, print the expected-vs-actual table, then # rerun the pipeline to prove the deploy is resumable: a rerun adopts every # contract and still lands on the same set. - - name: Verify addresses match the committed manifest + - name: Verify addresses match the expected set id: verify_addresses if: steps.verify.outcome == 'success' run: | set +e - # Markdown expected-vs-actual table: expected = committed manifest, + # Markdown expected-vs-actual table: expected = the committed expected set, # actual = this CI deployment. Any row whose addresses differ is MOVED. emit_table() { - echo "### Deployed addresses vs the committed manifest" + echo "### Deployed addresses vs the expected set" echo "" - echo "Expected is the committed manifest; actual is this CI deployment of the same pipeline." + echo "Expected is the committed expected-address set; actual is this CI deployment of the same pipeline." echo "" echo "| Contract | Expected | Actual | Match |" echo "|:---------|:---------|:-------|:-----:|" @@ -302,11 +304,11 @@ jobs: ' } - # True when the deployed manifest equals the committed one. Sort keys so only value + # True when the deployed manifest equals the expected set. Sort keys so only value # differences, never ordering, register as a mismatch. Underscore-prefixed keys are - # deploy metadata rather than addresses: `_seed` varies per run, and `_deployedFrom` - # exists only in a committed manifest, never in a fresh deploy's output. The release - # generator and the dotns-releases report drop the same prefix. + # deploy metadata rather than addresses: `_seed` varies per run, and the expected + # set carries no metadata at all. The release generator and the dotns-releases + # report drop the same prefix. addresses() { jq -S 'with_entries(select(.key | startswith("_") | not))' "$1" } @@ -328,15 +330,15 @@ jobs: if ! matches_canonical; then { - echo "### Deployed addresses do not match the committed manifest" + echo "### Deployed addresses do not match the expected set" echo "" - echo "The pipeline no longer reproduces the published set. Rows marked MOVED" - echo "below differ from the committed manifest; update it, or restore the" - echo "salt or label that moved." + echo "The pipeline no longer reproduces the expected addresses. Rows marked" + echo "MOVED below differ from deployments/expected.json; update it, or" + echo "restore the salt or label that moved." echo "" cat table.md } > deploy-error.md - fail "Failed - addresses differ from the committed manifest" + fail "Failed - addresses differ from the expected set" fi # Resume: rerun the same pinned pipeline. It must adopt every contract @@ -360,7 +362,7 @@ jobs: { echo "### Resume moved addresses" echo "" - echo "The rerun no longer matches the committed manifest, so a resumed deploy would" + echo "The rerun no longer matches the expected set, so a resumed deploy would" echo "relocate contracts." echo "" emit_table @@ -371,7 +373,7 @@ jobs: # Surface the table under the passing row too, so the address set is on # record every run, not only on failure. cp table.md deploy-error.md - echo "result=Reproduces the committed manifest; resume verified" >> "$GITHUB_OUTPUT" + echo "result=Reproduces the expected address set; resume verified" >> "$GITHUB_OUTPUT" echo "has_details=true" >> "$GITHUB_OUTPUT" - name: Set final result diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 6d9051183..bfb8d633a 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -135,6 +135,8 @@ Any new contract address that other contracts need to read must be looked up thr If you are adding a new contract category, add a `bytes32` key for it in `DotnsConstants.sol`, wire it up in `WireDeployments.s.sol`, and list the contract and its interface in `.github/abi-contracts.txt` so their ABIs ship in the release artifact. Read it the same way every existing contract does. +A change that moves or adds an address (a new salt, a new contract, a contract restructured behind a proxy) must update `deployments/expected.json` in the same PR — that diff is where review sees the move — and must NOT touch any `deployments//.json`. Those are records of live networks, updated only by a real deploy on that network; editing one from a code PR publishes an address nothing is deployed at. The two files diverging is normal and means a redeploy or migration is owed on that network — see "Two address files, two roles" in `DEPLOYMENTS.md`. + Bad — the registrar address is frozen at construction, so rotating it needs an upgrade: ```solidity diff --git a/DEPLOYMENTS.md b/DEPLOYMENTS.md index ce2a1a9a7..7296349be 100644 --- a/DEPLOYMENTS.md +++ b/DEPLOYMENTS.md @@ -281,6 +281,16 @@ forge test --match-path 'test/fork/**' -vvvvv If the deployment was intended to update a public environment, update the address tables in this file from the deployment manifest in the same change that updates the generated deployment JSON. +### Two address files, two roles: network manifests and the expected set + +`deployments//.json` is a **network record**: what is deployed on that live network right now. It is updated only by a real deploy or migration on that network, never by a code change. Everything that answers for reality reads these files: releases copy their addresses verbatim, and pointing tooling or the wire stage at an address with nothing behind it breaks whatever reads it. + +`deployments/expected.json` is the **expected set**: the addresses a fresh deploy of the current revision lands through the pinned CREATE3 factory. It is a property of the code, not of any network; the CI deploy job and `scripts/genesis/build-genesis.sh` verify against it, and releases never publish it. + +The two files can legitimately disagree: after a code change moves an address, the expected set carries the new address while every network manifest keeps the old one until that network actually redeploys. The difference between them is the migration backlog, readable as a diff, and it is resolved per network by the event that relocates the contract: a wipe-and-redeploy on a test network, a deliberate migration on one that never wipes. + +Before deploying to a live network, diff its manifest against `deployments/expected.json`. If any address diverges, run the pipeline against that network only as that planned wipe or migration: run outside it, the pipeline deploys the diverged contracts beside the live ones with empty state and repoints their registry keys, stranding any state behind the old addresses. After the planned deploy, commit the manifest it writes and update the address tables in this file in the same change. + ## Name grants (whitelisting) Reserved registration is gated on `DotnsNameWhitelist`. A grant binds one label to one beneficiary address and is single use: `registerReserved` requires a grant naming `registration.owner`, spends it on the mint, and refuses a second attempt. See the [README economics section](./README.md#economics) for what a grant does and does not confer; this section covers the mechanics. diff --git a/RELEASE_ARTIFACTS.md b/RELEASE_ARTIFACTS.md index 7a0cf8c4e..9f3a08b11 100644 --- a/RELEASE_ARTIFACTS.md +++ b/RELEASE_ARTIFACTS.md @@ -42,6 +42,7 @@ The release surface is decided in `.github/abi-contracts.txt` so a contract reac - One entry can serve more than one live network. `paseo-assethub` is the deployment that both previewnet and Paseo Asset Hub Next V2 run, because every network deployed through the shared CREATE3 factory lands on the same addresses. So expect entries named after deployments, not after every chain you might connect to. - The names under `contracts` (`DotnsRegistrar`, `PopRules`) do not change, and a name always means the same contract. Your code can depend on that. - Addresses are copied from the manifest verbatim, which the deploy pipeline writes EIP-55 checksummed. Compare them case-insensitively rather than relying on the casing. +- Only per-network manifests are published. `deployments/expected.json` — the fresh-deploy address set that CI and the genesis builder verify against (see `DEPLOYMENTS.md`) — is not a network and never appears here, so a release cut while an address move is awaiting its network's redeploy still advertises the addresses each live network actually runs. - `LabelStoreBeacon` and `UserStoreBeacon` appear when deployed but are not network-stable, because the `StoreFactory` initialiser deploys them. Read them from the factory rather than pinning them. ## `release-manifest.json` diff --git a/deployments/expected.json b/deployments/expected.json new file mode 100644 index 000000000..df7bb3c03 --- /dev/null +++ b/deployments/expected.json @@ -0,0 +1,22 @@ +{ + "Create3Factory": "0x8533c79E058c5a6489CAFeCA86dc600E029D75f5", + "DotnsContentResolver": "0x7F74D7CD50f5a834270E2ad395a01b01891AB37d", + "DotnsCostModelRegistry": "0x8bfd1f0957e73716732e725802f13830B5682da4", + "DotnsFlatPricing": "0xD839B281dF72Df44fF275305E72cAEEc0fDAA648", + "DotnsNameEscrow": "0x4881Afb78e7C908cAe818168B926229D93376520", + "DotnsNameWhitelist": "0x420166cD67Ca0233094E492a4BbA67045eD7C38C", + "DotnsPopController": "0xCC932348606cc1f3318cADeC5A5Cd2CA447f8a4b", + "DotnsPopLens": "0xfe5A45f7fD58D1A6FE09455DB799405b1dcE9411", + "DotnsPopResolver": "0xDaC984884EcA8Fc44011f1D6C49B27828390A72B", + "DotnsProtocolRegistry": "0xD19e3D0C97CF501125a04A97405e3e6592fa846E", + "DotnsRegistrar": "0x4f06E818Ba3d987704fd91cf3d868E4b019106Ab", + "DotnsRegistrarController": "0xBdaA01bD1bA67d709F2b1fF286Da0d854977EA30", + "DotnsRegistry": "0xf34054fd76BbF85f216cf9908226D5f0A72E50CA", + "DotnsResolver": "0xbd1165E549DF96F083c0A16f61590927bC187009", + "DotnsReverseResolver": "0xee3883d7eB60Ee9BCD7F3bcD8f2f05302A9Cc035", + "LabelStoreBeacon": "0x2227d9807F5A71332Aaa0640643030f2A3bf84cD", + "Multicall3": "0xB4468000abD87D3c56cbFBd153161223D7b109e5", + "PopRules": "0x747B456bE03aec0b42bd85C51513730FBD45DA31", + "StoreFactory": "0x99605a926FcB40aB520F659c6505E5ff862771f6", + "UserStoreBeacon": "0x3d1Ca165f7A5e387C2df02DB2FadD3149c1C72ad" +} diff --git a/deployments/paseo-assethub/420420417.json b/deployments/paseo-assethub/420420417.json index 29c4aacfb..bbb35225e 100644 --- a/deployments/paseo-assethub/420420417.json +++ b/deployments/paseo-assethub/420420417.json @@ -1 +1 @@ -{"Create3Factory":"0x8533c79E058c5a6489CAFeCA86dc600E029D75f5","DotnsContentResolver":"0x7F74D7CD50f5a834270E2ad395a01b01891AB37d","DotnsCostModelRegistry":"0x8bfd1f0957e73716732e725802f13830B5682da4","DotnsFlatPricing":"0xD839B281dF72Df44fF275305E72cAEEc0fDAA648","DotnsNameEscrow":"0x4881Afb78e7C908cAe818168B926229D93376520","DotnsNameWhitelist":"0x420166cD67Ca0233094E492a4BbA67045eD7C38C","DotnsPopController":"0xCC932348606cc1f3318cADeC5A5Cd2CA447f8a4b","DotnsPopLens":"0xAE374b07c7e6f473CBa21d57e36AC15C631Abc51","DotnsPopResolver":"0xDaC984884EcA8Fc44011f1D6C49B27828390A72B","DotnsProtocolRegistry":"0xD19e3D0C97CF501125a04A97405e3e6592fa846E","DotnsRegistrar":"0x4f06E818Ba3d987704fd91cf3d868E4b019106Ab","DotnsRegistrarController":"0xBdaA01bD1bA67d709F2b1fF286Da0d854977EA30","DotnsRegistry":"0xf34054fd76BbF85f216cf9908226D5f0A72E50CA","DotnsResolver":"0xbd1165E549DF96F083c0A16f61590927bC187009","DotnsReverseResolver":"0xee3883d7eB60Ee9BCD7F3bcD8f2f05302A9Cc035","LabelStoreBeacon":"0x2227d9807F5A71332Aaa0640643030f2A3bf84cD","Multicall3":"0xB4468000abD87D3c56cbFBd153161223D7b109e5","PopRules":"0x747B456bE03aec0b42bd85C51513730FBD45DA31","StoreFactory":"0x99605a926FcB40aB520F659c6505E5ff862771f6","UserStoreBeacon":"0x3d1Ca165f7A5e387C2df02DB2FadD3149c1C72ad","_seed":"0x0000000000000000000000000000000000000000"} +{"Create3Factory":"0x8533c79E058c5a6489CAFeCA86dc600E029D75f5","DotnsContentResolver":"0x7F74D7CD50f5a834270E2ad395a01b01891AB37d","DotnsCostModelRegistry":"0x8bfd1f0957e73716732e725802f13830B5682da4","DotnsFlatPricing":"0xD839B281dF72Df44fF275305E72cAEEc0fDAA648","DotnsNameEscrow":"0x4881Afb78e7C908cAe818168B926229D93376520","DotnsNameWhitelist":"0x420166cD67Ca0233094E492a4BbA67045eD7C38C","DotnsPopController":"0xCC932348606cc1f3318cADeC5A5Cd2CA447f8a4b","DotnsPopLens":"0xAE374b07c7e6f473CBa21d57e36AC15C631Abc51","DotnsPopResolver":"0xDaC984884EcA8Fc44011f1D6C49B27828390A72B","DotnsProtocolRegistry":"0xD19e3D0C97CF501125a04A97405e3e6592fa846E","DotnsRegistrar":"0x4f06E818Ba3d987704fd91cf3d868E4b019106Ab","DotnsRegistrarController":"0xBdaA01bD1bA67d709F2b1fF286Da0d854977EA30","DotnsRegistry":"0xf34054fd76BbF85f216cf9908226D5f0A72E50CA","DotnsResolver":"0xbd1165E549DF96F083c0A16f61590927bC187009","DotnsReverseResolver":"0xee3883d7eB60Ee9BCD7F3bcD8f2f05302A9Cc035","LabelStoreBeacon":"0xb57Ebc2e7085616d4906D1fE49af1cE13f7dffeF","Multicall3":"0xB4468000abD87D3c56cbFBd153161223D7b109e5","PopRules":"0x747B456bE03aec0b42bd85C51513730FBD45DA31","StoreFactory":"0x709A027F446a9e2a4BB9cb9a9c754435b19e32B7","UserStoreBeacon":"0xb7C995601679840d36F37E86DB2d7dF30797eC5C","_seed":"0x0000000000000000000000000000000000000000"} diff --git a/scripts/genesis/build-genesis.sh b/scripts/genesis/build-genesis.sh index 01db830de..79663514b 100755 --- a/scripts/genesis/build-genesis.sh +++ b/scripts/genesis/build-genesis.sh @@ -33,7 +33,7 @@ GENESIS_OUT="" # set once DOTNS_TLD is validated, below SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" DEPLOYMENT_FILE="deployments/localhost/31337.json" -CANONICAL_MANIFEST="deployments/paseo-assethub/420420417.json" +CANONICAL_MANIFEST="deployments/expected.json" # Who OWNS the contracts in the genesis state (REQUIRED, one of the three below). # @@ -58,7 +58,7 @@ ADMIN_KEY="${DOTNS_ADMIN_KEY:-}" # a pure function of the Create3Factory address, and the factory address is # keccak(deployer, nonce 0) — see "Deterministic addresses (CREATE3)" in # DEPLOYMENTS.md. Deploying the factory from this key as its first transaction is -# what makes the genesis addresses equal the live ones, which is asserted below. +# what makes the genesis addresses equal the expected set, which is asserted below. FACTORY_DEPLOYER_KEY="${FACTORY_DEPLOYER_KEY:-}" # TLD the genesis registry initialises with. Required by DeployCore, which reads @@ -232,19 +232,22 @@ echo "" [ -f "$DEPLOYMENT_FILE" ] \ || { echo "Error: no deployment manifest at $DEPLOYMENT_FILE — a stage failed above" >&2; exit 1; } -# ---- Address parity with the live deployment ---- -# The committed manifest is the only source of truth for DotNS addresses, so this is a -# hard failure, not a warning: a genesis built from a different factory key carries a -# different address set, and nothing downstream would notice. +# ---- Address parity with the expected set ---- +# The committed expected set (deployments/expected.json) records what a fresh deploy of +# this revision lands through the pinned factory, so this is a hard failure, not a +# warning: a genesis built from a different factory key carries a different address set, +# and nothing downstream would notice. A genesis is a fresh chain, which is exactly what +# the expected set describes; per-network manifests record live networks instead and are +# not consulted here. # -# Underscore-prefixed keys are metadata rather than contracts (`_seed`, and -# `_deployedFrom` once dotns-releases#12 lands), so they are filtered by prefix. +# Underscore-prefixed keys are metadata rather than contracts (`_seed` in a fresh +# deploy's output), so they are filtered by prefix. # -# Compared entry-by-entry: the local manifest may carry newer contracts not yet deployed -# live, so only the canonical entries are asserted. +# Compared entry-by-entry: the local manifest may carry newer contracts not yet in the +# expected set, so only the expected entries are asserted. # if [ ! -f "$CANONICAL_MANIFEST" ]; then - echo "Error: no canonical manifest at $CANONICAL_MANIFEST." >&2 + echo "Error: no expected-address set at $CANONICAL_MANIFEST." >&2 echo " Addresses cannot be verified, so the genesis would ship unchecked." >&2 exit 1 fi @@ -260,7 +263,7 @@ ACTUAL=$(jq -r --slurpfile canon "$CANONICAL_MANIFEST" ' if ! diff -u -L "expected ($CANONICAL_MANIFEST)" -L "actual (this build)" \ <(printf '%s\n' "$EXPECTED") <(printf '%s\n' "$ACTUAL"); then { - echo "Error: the deploy no longer reproduces the committed address set." + echo "Error: the deploy no longer reproduces the expected address set." echo " Lines marked -/+ above differ from $CANONICAL_MANIFEST." echo "" echo " Two things cause this:" @@ -272,7 +275,7 @@ if ! diff -u -L "expected ($CANONICAL_MANIFEST)" -L "actual (this build)" \ } >&2 exit 1 fi -echo " ✓ all live addresses reproduced ($(jq 'with_entries(select(.key | startswith("_") | not)) | length' "$CANONICAL_MANIFEST") contracts)" +echo " ✓ all expected addresses reproduced ($(jq 'with_entries(select(.key | startswith("_") | not)) | length' "$CANONICAL_MANIFEST") contracts)" echo "" # ---- Dump anvil state and extract ---- From 719f41bccaaedef6cc31fd4936a38dec105a8199 Mon Sep 17 00:00:00 2001 From: giuseppere Date: Tue, 15 Sep 2026 01:39:24 +0200 Subject: [PATCH 6/6] Docs nits --- .github/workflows/deploy-contracts.yml | 4 ++-- CONTRIBUTING.md | 2 +- DEPLOYMENTS.md | 4 ++-- .../unit/deploy/DeterministicDeployment.t.sol | 24 ++++++++++++------- 4 files changed, 20 insertions(+), 14 deletions(-) diff --git a/.github/workflows/deploy-contracts.yml b/.github/workflows/deploy-contracts.yml index 1dfdb6323..65aeb5016 100644 --- a/.github/workflows/deploy-contracts.yml +++ b/.github/workflows/deploy-contracts.yml @@ -27,8 +27,8 @@ jobs: # never valid on a real network) used to run the pipeline. FACTORY_DEPLOYER is # the public address of the single-purpose factory key. CANONICAL is the # committed expected-address set this deploy has to reproduce: what a fresh - # deploy of this revision lands through the pinned factory (see "Two address - # files, two roles" in DEPLOYMENTS.md). MANIFEST is + # deploy of this revision lands through the pinned factory (see "Network + # manifests and the expected set" in DEPLOYMENTS.md). MANIFEST is # the file this CI deploy writes. PINNED_FACTORY is read out of CANONICAL after # checkout rather than repeated here, so the expectation cannot drift from the # file it is meant to describe. PRIVATE_KEY is deliberately absent: it is set diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index bfb8d633a..cd4903ac7 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -135,7 +135,7 @@ Any new contract address that other contracts need to read must be looked up thr If you are adding a new contract category, add a `bytes32` key for it in `DotnsConstants.sol`, wire it up in `WireDeployments.s.sol`, and list the contract and its interface in `.github/abi-contracts.txt` so their ABIs ship in the release artifact. Read it the same way every existing contract does. -A change that moves or adds an address (a new salt, a new contract, a contract restructured behind a proxy) must update `deployments/expected.json` in the same PR — that diff is where review sees the move — and must NOT touch any `deployments//.json`. Those are records of live networks, updated only by a real deploy on that network; editing one from a code PR publishes an address nothing is deployed at. The two files diverging is normal and means a redeploy or migration is owed on that network — see "Two address files, two roles" in `DEPLOYMENTS.md`. +A change that moves or adds an address (a new salt, a new contract, a contract restructured behind a proxy) must update `deployments/expected.json` in the same PR — that diff is where review sees the move — and must NOT touch any `deployments//.json`. Those are records of live networks, updated only by a real deploy on that network; editing one from a code PR publishes an address nothing is deployed at. The expected set diverging from a network's manifest is normal and means a redeploy or migration is owed on that network — see "Network manifests and the expected set" in `DEPLOYMENTS.md`. Bad — the registrar address is frozen at construction, so rotating it needs an upgrade: diff --git a/DEPLOYMENTS.md b/DEPLOYMENTS.md index 7296349be..1fd1c98ed 100644 --- a/DEPLOYMENTS.md +++ b/DEPLOYMENTS.md @@ -281,13 +281,13 @@ forge test --match-path 'test/fork/**' -vvvvv If the deployment was intended to update a public environment, update the address tables in this file from the deployment manifest in the same change that updates the generated deployment JSON. -### Two address files, two roles: network manifests and the expected set +### Network manifests and the expected set `deployments//.json` is a **network record**: what is deployed on that live network right now. It is updated only by a real deploy or migration on that network, never by a code change. Everything that answers for reality reads these files: releases copy their addresses verbatim, and pointing tooling or the wire stage at an address with nothing behind it breaks whatever reads it. `deployments/expected.json` is the **expected set**: the addresses a fresh deploy of the current revision lands through the pinned CREATE3 factory. It is a property of the code, not of any network; the CI deploy job and `scripts/genesis/build-genesis.sh` verify against it, and releases never publish it. -The two files can legitimately disagree: after a code change moves an address, the expected set carries the new address while every network manifest keeps the old one until that network actually redeploys. The difference between them is the migration backlog, readable as a diff, and it is resolved per network by the event that relocates the contract: a wipe-and-redeploy on a test network, a deliberate migration on one that never wipes. +The expected set can legitimately disagree with a network manifest: after a code change moves an address, the expected set carries the new address while every network manifest keeps the old one until that network actually redeploys. The difference between them is the migration backlog, readable as a diff, and it is resolved per network by the event that relocates the contract: a wipe-and-redeploy on a test network, a deliberate migration on one that never wipes. Before deploying to a live network, diff its manifest against `deployments/expected.json`. If any address diverges, run the pipeline against that network only as that planned wipe or migration: run outside it, the pipeline deploys the diverged contracts beside the live ones with empty state and repoints their registry keys, stranding any state behind the old addresses. After the planned deploy, commit the manifest it writes and update the address tables in this file in the same change. diff --git a/test/unit/deploy/DeterministicDeployment.t.sol b/test/unit/deploy/DeterministicDeployment.t.sol index 50f9974b1..c66196185 100644 --- a/test/unit/deploy/DeterministicDeployment.t.sol +++ b/test/unit/deploy/DeterministicDeployment.t.sol @@ -54,11 +54,17 @@ contract DeterministicDeploymentTest is Test { _assertAdoptionRejected("Multicall3.sol:Multicall3", "", "Multicall3"); } - /// @notice The same rejection applies to an artefact carrying constructor-set immutables, - /// which is the case the check cannot answer by codehash alone. - /// @dev `StoreFactory` bakes its beacon addresses into runtime code, so two honest deploys - /// differ. The check masks the immutable ranges rather than comparing lengths: a length - /// comparison accepts any occupant padded to the same size. + /// @notice The same rejection applies to an artefact carrying immutables, which is the case + /// the check cannot answer by codehash alone. + /// @dev `StoreFactory` carries `UUPSUpgradeable.__self`, so two honest deploys of the + /// implementation differ. The check masks the immutable ranges rather than comparing + /// lengths: a length comparison accepts any occupant padded to the same size. + /// + /// Synthetic fixture: production deploys `StoreFactory` behind a proxy through the + /// `:implementation` and `:proxy` salts with `initialize`, so no stage uses this + /// `:contract` salt or these constructor bytes any more. The rejection is salt-agnostic, + /// which is what this pins; the production UUPS shape is covered by + /// `StoreBeaconVerification` and the `_deployCore` suite below. function test_foreign_occupant_is_rejected_for_an_immutable_carrying_artefact() public { bytes32 salt = deployer.create3Salt("StoreFactory", "contract"); @@ -71,13 +77,13 @@ contract DeterministicDeploymentTest is Test { ); } - /// @notice A real `StoreFactory` deployed against an attacker's constructor arguments is + /// @notice A real `DotnsPopLens` deployed against an attacker's constructor arguments is /// rejected, not adopted. /// @dev The case bytecode comparison alone cannot answer. The occupant is the genuine /// artefact, so its length and shape match; only the values its constructor baked in - /// differ. Comparing against a reference built with this run's arguments catches it, - /// while the beacons `StoreFactory` deploys itself vary on every honest deploy and are - /// necessarily skipped. + /// differ. Comparing against a reference built with this run's arguments catches it: + /// `DotnsPopLens.protocolRegistry` is constructor-set, so it stays inside the + /// comparison rather than being masked as address-derived. function test_same_artefact_with_foreign_constructor_args_is_rejected() public { address attacker = makeAddr("attacker"); address realRegistry = address(new DotnsProtocolRegistry());