contracts-bedrock: cleanup

Co-authored-by: refcell.eth <abigger87@gmail.com>
Co-authored-by: clabby <ben@clab.by>

packages/contracts-bedrock/.gas-snapshot

@@ -571,9 +571,9 @@ SetPrevBaseFee_Test:test_setPrevBaseFee_succeeds() (gas: 11549)
 StandardBridge_Stateless_Test:test_isCorrectTokenPair_succeeds() (gas: 49936)
 StandardBridge_Stateless_Test:test_isOptimismMintableERC20_succeeds() (gas: 33072)
 SystemConfig_Initialize_Test:test_initialize_startBlockOverride_succeeds() (gas: 55905)
-SystemConfig_Initialize_Test:test_initialize_values_succeeds() (gas: 64908)
-SystemConfig_Initialize_TestFail:test_initialize_lowGasLimit_reverts() (gas: 94905)
-SystemConfig_Initialize_TestFail:test_initialize_startBlock_reverts() (gas: 64695)
+SystemConfig_Initialize_Test:test_initialize_values_succeeds() (gas: 64946)
+SystemConfig_Initialize_TestFail:test_initialize_lowGasLimit_reverts() (gas: 94515)
+SystemConfig_Initialize_TestFail:test_initialize_startBlock_reverts() (gas: 64676)
 SystemConfig_Setters_TestFail:test_setBatcherHash_notOwner_reverts() (gas: 15607)
 SystemConfig_Setters_TestFail:test_setGasConfig_notOwner_reverts() (gas: 15577)
 SystemConfig_Setters_TestFail:test_setGasLimit_notOwner_reverts() (gas: 15676)

packages/contracts-bedrock/semver-lock.json

@@ -4,7 +4,7 @@
   "src/L1/L1StandardBridge.sol": "0xa35dc0ab143043063c3bff73c8b065e401c23296a2017258ce8a87f4a4da9416",
   "src/L1/L2OutputOracle.sol": "0x8f32ccb4c5cb63a459a0e79ee412177dc03fd279fdaaf1dac69e8c714902e857",
   "src/L1/OptimismPortal.sol": "0xeaa47a63e8a3bcfdb7dfd3e6c8608369e34e362d9de82f3acf13cbc27c070bf7",
-  "src/L1/SystemConfig.sol": "0xe946cd924e46fcf480c8aaecb7a674f284d7e6699366136bfe6080a46e9d2482",
+  "src/L1/SystemConfig.sol": "0x689a3339e9167e62fac105eaae0390b39fc7dfee21b19c5f78636d98ee97148e",
   "src/L2/BaseFeeVault.sol": "0xa596e60762f16192cfa86459fcb9f4da9d8f756106eedac643a1ffeafbbfcc5f",
   "src/L2/GasPriceOracle.sol": "0xc735a8bf01ad8bca194345748537bfd9924909c0342bc133c4a31e2fb8cb9882",
   "src/L2/L1Block.sol": "0x7fbfc8b4da630386636c665570321fdec287b0867dbe0f91c2e7cd5b7697c220",

packages/contracts-bedrock/src/L1/SystemConfig.sol

@@ -43,7 +43,7 @@ contract SystemConfig is OwnableUpgradeable, Semver {
     ///         Storing it at this deterministic storage slot allows for decoupling the storage
     ///         layout from the way that `solc` lays out storage. The `op-node` uses a storage
     ///         proof to fetch this value.
-    ///         NOTE: this value will be migrated to another storage slot in a future version.
+    /// @dev    NOTE: this value will be migrated to another storage slot in a future version.
     ///         User input should not be placed in storage in this contract until this migration
     ///         happens. It is unlikely that keccak second preimage resistance will be broken,
     ///         but it is better to be safe than sorry.
@@ -202,15 +202,10 @@ contract SystemConfig is OwnableUpgradeable, Semver {
     /// @notice High level getter for the unsafe block signer address.
     ///         Unsafe blocks can be propagated across the p2p network if they are signed by the
     ///         key corresponding to this address.
-    /// @return Address of the unsafe block signer.
+    /// @return addr_ Address of the unsafe block signer.
     // solhint-disable-next-line ordering
-    function unsafeBlockSigner() external view returns (address) {
-        address addr;
-        bytes32 slot = UNSAFE_BLOCK_SIGNER_SLOT;
-        assembly {
-            addr := sload(slot)
-        }
-        return addr;
+    function unsafeBlockSigner() external view returns (address addr_) {
+        addr_ = _getAddress(UNSAFE_BLOCK_SIGNER_SLOT);
     }
 
     /// @notice Stores an address in an arbitrary storage slot, `_slot`.
@@ -220,9 +215,8 @@ contract SystemConfig is OwnableUpgradeable, Semver {
     ///      in arbitrary storage slots. Solc will add checks that the data passed as `_addr`
     ///      is 20 bytes or less.
     function _setAddress(address _addr, bytes32 _slot) internal {
-        bytes32 slot = _slot;
         assembly {
-            sstore(slot, _addr)
+            sstore(_slot, _addr)
         }
     }
 
@@ -230,59 +224,63 @@ contract SystemConfig is OwnableUpgradeable, Semver {
     ///         These storage slots decouple the storage layout from
     ///         solc's automation.
     /// @param _slot The storage slot to retrieve the address from.
-    function _getAddress(bytes32 _slot) internal view returns (address) {
-        address addr;
-        bytes32 slot = _slot;
+    function _getAddress(bytes32 _slot) internal view returns (address addr_) {
         assembly {
-            addr := sload(slot)
+            addr_ := sload(_slot)
         }
-        return addr;
     }
 
     /// @notice Getter for the L1CrossDomainMessenger address.
-    function l1CrossDomainMessenger() external view returns (address) {
-        return _getAddress(L1_CROSS_DOMAIN_MESSENGER_SLOT);
+    function l1CrossDomainMessenger() external view returns (address addr_) {
+        addr_ = _getAddress(L1_CROSS_DOMAIN_MESSENGER_SLOT);
     }
 
     /// @notice Getter for the L1ERC721Bridge address.
-    function l1ERC721Bridge() external view returns (address) {
-        return _getAddress(L1_ERC_721_BRIDGE_SLOT);
+    function l1ERC721Bridge() external view returns (address addr_) {
+        addr_ = _getAddress(L1_ERC_721_BRIDGE_SLOT);
     }
 
     /// @notice Getter for the L1StandardBridge address.
-    function l1StandardBridge() external view returns (address) {
-        return _getAddress(L1_STANDARD_BRIDGE_SLOT);
+    function l1StandardBridge() external view returns (address addr_) {
+        addr_ = _getAddress(L1_STANDARD_BRIDGE_SLOT);
     }
 
     /// @notice Getter for the L2OutputOracle address.
-    function l2OutputOracle() external view returns (address) {
-        return _getAddress(L2_OUTPUT_ORACLE_SLOT);
+    function l2OutputOracle() external view returns (address addr_) {
+        addr_ = _getAddress(L2_OUTPUT_ORACLE_SLOT);
     }
 
     /// @notice Getter for the OptimismPortal address.
-    function optimismPortal() external view returns (address) {
-        return _getAddress(OPTIMISM_PORTAL_SLOT);
+    function optimismPortal() external view returns (address addr_) {
+        addr_ = _getAddress(OPTIMISM_PORTAL_SLOT);
     }
 
     /// @notice Getter for the OptimismMintableERC20Factory address.
-    function optimismMintableERC20Factory() external view returns (address) {
-        return _getAddress(OPTIMISM_MINTABLE_ERC20_FACTORY_SLOT);
+    function optimismMintableERC20Factory() external view returns (address addr_) {
+        addr_ = _getAddress(OPTIMISM_MINTABLE_ERC20_FACTORY_SLOT);
     }
 
     /// @notice Getter for the BatchInbox address.
-    function batchInbox() external view returns (address) {
-        return _getAddress(BATCH_INBOX_SLOT);
+    function batchInbox() external view returns (address addr_) {
+        addr_ = _getAddress(BATCH_INBOX_SLOT);
     }
 
     /// @notice Sets the start block in a backwards compatible way. Proxies
-    ///         that were initialized before this existed will not have it
-    ///         be set by `block.number`.
+    ///         that were initialized before the startBlock existed in storage
+    ///         can have their start block set by a user provided override.
+    ///         A start block of 0 indicates that there is no override and the
+    ///         start block will be set by `block.number`.
+    /// @dev    This logic is used to patch legacy with new storage values. In the
+    ///         next version, it should remove the override and set the start block
+    ///         to `block.number` if the value in storage is 0. This will allow it
+    ///         to be reinitialized again and also work for fresh deployments.
+    /// @param  _startBlock The start block override to set in storage.
     function _setStartBlock(uint256 _startBlock) internal {
+        require(startBlock == 0, "SystemConfig: cannot override an already set start block");
         if (_startBlock != 0) {
             // There is an override, it cannot already be set.
-            require(startBlock == 0, "SystemConfig: cannot override an already set start block");
             startBlock = _startBlock;
-        } else if (startBlock == 0) {
+        } else {
             // There is no override and it is not set in storage. Set it to the block number.
             startBlock = block.number;
         }