OPSM: More assertions (#11994)

* refactor: rename var for clarity

* test: add more assertions from ChainAssertions, and some new ones

* chore: ensure unique, clear, consistent revert string IDs

also sorts methods alphabetically for clarity

* fix: var name after rebase

* style: forge fmt

* revert proxyAdmin -> superchainProxyAdmin name change for now

* Update packages/contracts-bedrock/scripts/DeployImplementations.s.sol
Co-authored-by: Maurelian <john@oplabs.co>

* chore: small tweaks from pr feedback

---------
Co-authored-by: Maurelian <john@oplabs.co>

packages/contracts-bedrock/scripts/DeployOPChain.s.sol

@@ -274,16 +274,17 @@ contract DeployOPChainOutput is BaseDeployIO {
         return _delayedWETHPermissionlessGameProxy;
     }
 
-    // -------- Assertions on chain architecture --------
+    // -------- Deployment Assertions --------
 
     function assertValidDeploy(DeployOPChainInput _doi) internal view {
-        assertValidSystemConfig(_doi);
+        assertValidDelayedWETHs(_doi);
+        assertValidDisputeGameFactory(_doi);
         assertValidL1CrossDomainMessenger(_doi);
+        assertValidL1ERC721Bridge(_doi);
         assertValidL1StandardBridge(_doi);
         assertValidOptimismMintableERC20Factory(_doi);
         assertValidOptimismPortal(_doi);
-        assertValidDisputeGameFactory(_doi);
-        assertValidDelayedWETHs(_doi);
+        assertValidSystemConfig(_doi);
         // TODO Other FP assertions like the dispute games, anchor state registry, etc.
         // TODO add initialization assertions
     }
@@ -293,34 +294,38 @@ contract DeployOPChainOutput is BaseDeployIO {
 
         DeployUtils.assertInitialized({ _contractAddress: address(systemConfig), _slot: 0, _offset: 0 });
 
-        require(systemConfig.owner() == _doi.systemConfigOwner(), "SC-10");
-        require(systemConfig.basefeeScalar() == _doi.basefeeScalar(), "SC-20");
-        require(systemConfig.blobbasefeeScalar() == _doi.blobBaseFeeScalar(), "SC-30");
-        require(systemConfig.batcherHash() == bytes32(uint256(uint160(_doi.batcher()))), "SC-40");
-        require(systemConfig.gasLimit() == uint64(30000000), "SC-50"); // TODO allow other gas limits?
-        require(systemConfig.unsafeBlockSigner() == _doi.unsafeBlockSigner(), "SC-60");
-        require(systemConfig.scalar() >> 248 == 1, "SC-70");
+        require(systemConfig.owner() == _doi.systemConfigOwner(), "SYSCON-10");
+        require(systemConfig.basefeeScalar() == _doi.basefeeScalar(), "SYSCON-20");
+        require(systemConfig.blobbasefeeScalar() == _doi.blobBaseFeeScalar(), "SYSCON-30");
+        require(systemConfig.batcherHash() == bytes32(uint256(uint160(_doi.batcher()))), "SYSCON-40");
+        require(systemConfig.gasLimit() == uint64(30000000), "SYSCON-50"); // TODO allow other gas limits?
+        require(systemConfig.unsafeBlockSigner() == _doi.unsafeBlockSigner(), "SYSCON-60");
+        require(systemConfig.scalar() >> 248 == 1, "SYSCON-70");
 
         IResourceMetering.ResourceConfig memory rConfig = Constants.DEFAULT_RESOURCE_CONFIG();
         IResourceMetering.ResourceConfig memory outputConfig = systemConfig.resourceConfig();
-        require(outputConfig.maxResourceLimit == rConfig.maxResourceLimit, "SC-80");
-        require(outputConfig.elasticityMultiplier == rConfig.elasticityMultiplier, "SC-90");
-        require(outputConfig.baseFeeMaxChangeDenominator == rConfig.baseFeeMaxChangeDenominator, "SC-100");
-        require(outputConfig.systemTxMaxGas == rConfig.systemTxMaxGas, "SC-110");
-        require(outputConfig.minimumBaseFee == rConfig.minimumBaseFee, "SC-120");
-        require(outputConfig.maximumBaseFee == rConfig.maximumBaseFee, "SC-130");
-
-        require(systemConfig.startBlock() == block.number, "SC-140");
-        require(systemConfig.batchInbox() == _doi.opsmProxy().chainIdToBatchInboxAddress(_doi.l2ChainId()), "SC-150");
-
-        require(systemConfig.l1CrossDomainMessenger() == address(l1CrossDomainMessengerProxy()), "SC-160");
-        require(systemConfig.l1ERC721Bridge() == address(l1ERC721BridgeProxy()), "SC-170");
-        require(systemConfig.l1StandardBridge() == address(l1StandardBridgeProxy()), "SC-180");
-        require(systemConfig.disputeGameFactory() == address(disputeGameFactoryProxy()), "SC-190");
-        require(systemConfig.optimismPortal() == address(optimismPortalProxy()), "SC-200");
-        require(systemConfig.optimismMintableERC20Factory() == address(optimismMintableERC20FactoryProxy()), "SC-210");
+        require(outputConfig.maxResourceLimit == rConfig.maxResourceLimit, "SYSCON-80");
+        require(outputConfig.elasticityMultiplier == rConfig.elasticityMultiplier, "SYSCON-90");
+        require(outputConfig.baseFeeMaxChangeDenominator == rConfig.baseFeeMaxChangeDenominator, "SYSCON-100");
+        require(outputConfig.systemTxMaxGas == rConfig.systemTxMaxGas, "SYSCON-110");
+        require(outputConfig.minimumBaseFee == rConfig.minimumBaseFee, "SYSCON-120");
+        require(outputConfig.maximumBaseFee == rConfig.maximumBaseFee, "SYSCON-130");
+
+        require(systemConfig.startBlock() == block.number, "SYSCON-140");
+        require(
+            systemConfig.batchInbox() == _doi.opsmProxy().chainIdToBatchInboxAddress(_doi.l2ChainId()), "SYSCON-150"
+        );
+
+        require(systemConfig.l1CrossDomainMessenger() == address(l1CrossDomainMessengerProxy()), "SYSCON-160");
+        require(systemConfig.l1ERC721Bridge() == address(l1ERC721BridgeProxy()), "SYSCON-170");
+        require(systemConfig.l1StandardBridge() == address(l1StandardBridgeProxy()), "SYSCON-180");
+        require(systemConfig.disputeGameFactory() == address(disputeGameFactoryProxy()), "SYSCON-190");
+        require(systemConfig.optimismPortal() == address(optimismPortalProxy()), "SYSCON-200");
+        require(
+            systemConfig.optimismMintableERC20Factory() == address(optimismMintableERC20FactoryProxy()), "SYSCON-210"
+        );
         (address gasPayingToken,) = systemConfig.gasPayingToken();
-        require(gasPayingToken == Constants.ETHER, "SC-220");
+        require(gasPayingToken == Constants.ETHER, "SYSCON-220");
     }
 
     function assertValidL1CrossDomainMessenger(DeployOPChainInput _doi) internal view {
@@ -357,8 +362,8 @@ contract DeployOPChainOutput is BaseDeployIO {
 
         DeployUtils.assertInitialized({ _contractAddress: address(factory), _slot: 0, _offset: 0 });
 
-        require(factory.BRIDGE() == address(l1StandardBridgeProxy()), "OMEF-10");
-        require(factory.bridge() == address(l1StandardBridgeProxy()), "OMEF-20");
+        require(factory.BRIDGE() == address(l1StandardBridgeProxy()), "MERC20F-10");
+        require(factory.bridge() == address(l1StandardBridgeProxy()), "MERC20F-20");
     }
 
     function assertValidL1ERC721Bridge(DeployOPChainInput _doi) internal view {
@@ -366,24 +371,24 @@ contract DeployOPChainOutput is BaseDeployIO {
 
         DeployUtils.assertInitialized({ _contractAddress: address(bridge), _slot: 0, _offset: 0 });
 
-        require(address(bridge.OTHER_BRIDGE()) == Predeploys.L2_ERC721_BRIDGE, "LEB-10");
-        require(address(bridge.otherBridge()) == Predeploys.L2_ERC721_BRIDGE, "LEB-20");
+        require(address(bridge.OTHER_BRIDGE()) == Predeploys.L2_ERC721_BRIDGE, "L721B-10");
+        require(address(bridge.otherBridge()) == Predeploys.L2_ERC721_BRIDGE, "L721B-20");
 
-        require(address(bridge.MESSENGER()) == address(l1CrossDomainMessengerProxy()), "LEB-30");
-        require(address(bridge.messenger()) == address(l1CrossDomainMessengerProxy()), "LEB-40");
-        require(address(bridge.superchainConfig()) == address(_doi.opsmProxy().superchainConfig()), "LEB-50");
+        require(address(bridge.MESSENGER()) == address(l1CrossDomainMessengerProxy()), "L721B-30");
+        require(address(bridge.messenger()) == address(l1CrossDomainMessengerProxy()), "L721B-40");
+        require(address(bridge.superchainConfig()) == address(_doi.opsmProxy().superchainConfig()), "L721B-50");
     }
 
     function assertValidOptimismPortal(DeployOPChainInput _doi) internal view {
         OptimismPortal2 portal = optimismPortalProxy();
         ISuperchainConfig superchainConfig = ISuperchainConfig(address(_doi.opsmProxy().superchainConfig()));
 
-        require(address(portal.disputeGameFactory()) == address(disputeGameFactoryProxy()), "OP-10");
-        require(address(portal.systemConfig()) == address(systemConfigProxy()), "OP-20");
-        require(address(portal.superchainConfig()) == address(superchainConfig), "OP-30");
-        require(portal.guardian() == superchainConfig.guardian(), "OP-40");
-        require(portal.paused() == superchainConfig.paused(), "OP-50");
-        require(portal.l2Sender() == Constants.DEFAULT_L2_SENDER, "OP-60");
+        require(address(portal.disputeGameFactory()) == address(disputeGameFactoryProxy()), "PORTAL-10");
+        require(address(portal.systemConfig()) == address(systemConfigProxy()), "PORTAL-20");
+        require(address(portal.superchainConfig()) == address(superchainConfig), "PORTAL-30");
+        require(portal.guardian() == superchainConfig.guardian(), "PORTAL-40");
+        require(portal.paused() == superchainConfig.paused(), "PORTAL-50");
+        require(portal.l2Sender() == Constants.DEFAULT_L2_SENDER, "PORTAL-60");
 
         // This slot is the custom gas token _balance and this check ensures
         // that it stays unset for forwards compatibility with custom gas token.

packages/contracts-bedrock/scripts/DeploySuperchain.s.sol

@@ -210,7 +210,7 @@ contract DeploySuperchainOutput is BaseDeployIO {
 
     // This function can be called to ensure all outputs are correct. Similar to `writeOutputFile`,
     // it fetches the output values using external calls to the getter methods for safety.
-    function checkOutput(DeploySuperchainInput) public {
+    function checkOutput(DeploySuperchainInput _dsi) public {
         address[] memory addrs = Solarray.addresses(
             address(this.superchainProxyAdmin()),
             address(this.superchainConfigImpl()),
@@ -230,6 +230,7 @@ contract DeploySuperchainOutput is BaseDeployIO {
         require(actualProtocolVersionsImpl == address(_protocolVersionsImpl), "200");
 
         // TODO Also add the assertions for the implementation contracts from ChainAssertions.sol
+        assertValidDeploy(_dsi);
     }
 
     function superchainProxyAdmin() public view returns (ProxyAdmin) {
@@ -256,6 +257,62 @@ contract DeploySuperchainOutput is BaseDeployIO {
         DeployUtils.assertValidContractAddress(address(_protocolVersionsProxy));
         return _protocolVersionsProxy;
     }
+
+    // -------- Deployment Assertions --------
+    function assertValidDeploy(DeploySuperchainInput _dsi) public {
+        assertValidSuperchainProxyAdmin(_dsi);
+        assertValidSuperchainConfig(_dsi);
+        assertValidProtocolVersions(_dsi);
+    }
+
+    function assertValidSuperchainProxyAdmin(DeploySuperchainInput _dsi) internal view {
+        require(superchainProxyAdmin().owner() == _dsi.proxyAdminOwner(), "SPA-10");
+    }
+
+    function assertValidSuperchainConfig(DeploySuperchainInput _dsi) internal {
+        // Proxy checks.
+        SuperchainConfig superchainConfig = superchainConfigProxy();
+        DeployUtils.assertInitialized({ _contractAddress: address(superchainConfig), _slot: 0, _offset: 0 });
+        require(superchainConfig.guardian() == _dsi.guardian(), "SUPCON-10");
+        require(superchainConfig.paused() == _dsi.paused(), "SUPCON-20");
+
+        vm.startPrank(address(0));
+        require(
+            Proxy(payable(address(superchainConfig))).implementation() == address(superchainConfigImpl()), "SUPCON-30"
+        );
+        require(Proxy(payable(address(superchainConfig))).admin() == address(superchainProxyAdmin()), "SUPCON-40");
+        vm.stopPrank();
+
+        // Implementation checks
+        superchainConfig = superchainConfigImpl();
+        require(superchainConfig.guardian() == address(0), "SUPCON-50");
+        require(superchainConfig.paused() == false, "SUPCON-60");
+    }
+
+    function assertValidProtocolVersions(DeploySuperchainInput _dsi) internal {
+        // Proxy checks.
+        ProtocolVersions pv = protocolVersionsProxy();
+        DeployUtils.assertInitialized({ _contractAddress: address(pv), _slot: 0, _offset: 0 });
+        require(pv.owner() == _dsi.protocolVersionsOwner(), "PV-10");
+        require(
+            ProtocolVersion.unwrap(pv.required()) == ProtocolVersion.unwrap(_dsi.requiredProtocolVersion()), "PV-20"
+        );
+        require(
+            ProtocolVersion.unwrap(pv.recommended()) == ProtocolVersion.unwrap(_dsi.recommendedProtocolVersion()),
+            "PV-30"
+        );
+
+        vm.startPrank(address(0));
+        require(Proxy(payable(address(pv))).implementation() == address(protocolVersionsImpl()), "PV-40");
+        require(Proxy(payable(address(pv))).admin() == address(superchainProxyAdmin()), "PV-50");
+        vm.stopPrank();
+
+        // Implementation checks.
+        pv = protocolVersionsImpl();
+        require(pv.owner() == address(0xdead), "PV-60");
+        require(ProtocolVersion.unwrap(pv.required()) == 0, "PV-70");
+        require(ProtocolVersion.unwrap(pv.recommended()) == 0, "PV-80");
+    }
 }
 
 // For all broadcasts in this script we explicitly specify the deployer as `msg.sender` because for

packages/contracts-bedrock/scripts/libraries/DeployUtils.sol

@@ -48,13 +48,15 @@ library DeployUtils {
         }
     }
 
-    // Asserts that for a given contract the value of a storage slot at an offset is 1. This
-    // is used to assert that a contract is initialized.
+    // Asserts that for a given contract the value of a storage slot at an offset is 1 or
+    // `type(uint8).max`. The value is set to 1 when a contract is initialized, and set to
+    // `type(uint8).max` when `_disableInitializers` is called.
     function assertInitialized(address _contractAddress, uint256 _slot, uint256 _offset) internal view {
         bytes32 slotVal = vm.load(_contractAddress, bytes32(_slot));
+        uint8 value = uint8((uint256(slotVal) >> (_offset * 8)) & 0xFF);
         require(
-            uint8((uint256(slotVal) >> (_offset * 8)) & 0xFF) == uint8(1),
-            "Storage value is not 1 at the given slot and offset"
+            value == 1 || value == type(uint8).max,
+            "Value at the given slot and offset does not indicate initialization"
         );
     }
 }

packages/contracts-bedrock/test/DeploySuperchain.t.sol

@@ -284,8 +284,8 @@ contract DeploySuperchain_Test is Test {
         uint256 slot = zeroOutSlotForSelector(dsi.proxyAdminOwner.selector);
         vm.expectRevert("DeploySuperchainInput: proxyAdminOwner not set");
         deploySuperchain.run(dsi, dso);
-        vm.store(address(dsi), bytes32(slot), bytes32(uint256(uint160(defaultProxyAdminOwner)))); // Restore the value
-            // we just tested.
+        // Restore the value we just tested.
+        vm.store(address(dsi), bytes32(slot), bytes32(uint256(uint160(defaultProxyAdminOwner))));
 
         slot = zeroOutSlotForSelector(dsi.protocolVersionsOwner.selector);
         vm.expectRevert("DeploySuperchainInput: protocolVersionsOwner not set");