style(ctb): clean up legacy comments and errs (#2953)

Cleans up comments and errors for legacy contracts. It's fine to update
the bytecode for these legacy contracts since we're really only using
them for their interfaces.

.changeset/lazy-kiwis-jog.md

+---
+'@eth-optimism/contracts-bedrock': patch
+---
+
+Clean up comments and errors for legacy contracts

packages/contracts-bedrock/.gas-snapshot

@@ -4,7 +4,7 @@ GasBenchMark_L1StandardBridge_Deposit:test_depositERC20_benchmark_0() (gas: 2518
 GasBenchMark_L1StandardBridge_Deposit:test_depositERC20_benchmark_1() (gas: 118133)
 GasBenchMark_L1StandardBridge_Deposit:test_depositETH_benchmark_0() (gas: 251901)
 GasBenchMark_L1StandardBridge_Deposit:test_depositETH_benchmark_1() (gas: 118108)
-GasBenchMark_L1StandardBridge_Finalize:test_finalizeETHWithdrawal_benchmark() (gas: 45374)
+GasBenchMark_L1StandardBridge_Finalize:test_finalizeETHWithdrawal_benchmark() (gas: 45385)
 GasBenchMark_L2OutputOracle:test_appendL2Output_benchmark() (gas: 68684)
 GasBenchMark_OptimismPortal:test_depositTransaction_benchmark() (gas: 75024)
 GasBenchMark_OptimismPortal:test_depositTransaction_benchmark_1() (gas: 35328)
@@ -45,18 +45,18 @@ L1CrossDomainMessenger_Test:test_L1MessengerTwiceSendMessage() (gas: 1275622)
 L1CrossDomainMessenger_Test:test_L1MessengerUnpause() (gas: 40924)
 L1CrossDomainMessenger_Test:test_L1MessengerXDomainSenderReverts() (gas: 24250)
 L1CrossDomainMessenger_Test:test_L1MessengerxDomainMessageSenderResets() (gas: 88950)
-L1StandardBridge_Test:test_depositERC20() (gas: 476982)
-L1StandardBridge_Test:test_depositERC20To() (gas: 479186)
+L1StandardBridge_Test:test_depositERC20() (gas: 476993)
+L1StandardBridge_Test:test_depositERC20To() (gas: 479197)
 L1StandardBridge_Test:test_depositETH() (gas: 270949)
 L1StandardBridge_Test:test_depositETHTo() (gas: 228771)
-L1StandardBridge_Test:test_finalizeERC20Withdrawal() (gas: 490577)
-L1StandardBridge_Test:test_finalizeETHWithdrawal() (gas: 64061)
-L1StandardBridge_Test:test_initialize() (gas: 26225)
-L1StandardBridge_Test:test_onlyEOADepositERC20() (gas: 22329)
+L1StandardBridge_Test:test_finalizeERC20Withdrawal() (gas: 490595)
+L1StandardBridge_Test:test_finalizeETHWithdrawal() (gas: 64116)
+L1StandardBridge_Test:test_initialize() (gas: 26236)
+L1StandardBridge_Test:test_onlyEOADepositERC20() (gas: 22340)
 L1StandardBridge_Test:test_onlyEOADepositETH() (gas: 40882)
-L1StandardBridge_Test:test_onlyL2BridgeFinalizeERC20Withdrawal() (gas: 36114)
-L1StandardBridge_Test:test_onlyPortalFinalizeERC20Withdrawal() (gas: 35478)
-L1StandardBridge_Test:test_receive() (gas: 415514)
+L1StandardBridge_Test:test_onlyL2BridgeFinalizeERC20Withdrawal() (gas: 36136)
+L1StandardBridge_Test:test_onlyPortalFinalizeERC20Withdrawal() (gas: 35489)
+L1StandardBridge_Test:test_receive() (gas: 415529)
 L2CrossDomainMessenger_Test:testCannot_L2MessengerPause() (gas: 10821)
 L2CrossDomainMessenger_Test:test_L2MessengerMessageVersion() (gas: 8388)
 L2CrossDomainMessenger_Test:test_L2MessengerPause() (gas: 31815)
@@ -159,7 +159,7 @@ ProxyAdmin_Test:test_chugsplashChangeProxyAdmin() (gas: 35647)
 ProxyAdmin_Test:test_chugsplashGetProxyAdmin() (gas: 15689)
 ProxyAdmin_Test:test_chugsplashGetProxyImplementation() (gas: 51167)
 ProxyAdmin_Test:test_chugsplashUpgrade() (gas: 48972)
-ProxyAdmin_Test:test_chugsplashUpgradeAndCall() (gas: 82334)
+ProxyAdmin_Test:test_chugsplashUpgradeAndCall() (gas: 82345)
 ProxyAdmin_Test:test_delegateResolvedChangeProxyAdmin() (gas: 34020)
 ProxyAdmin_Test:test_delegateResolvedGetProxyAdmin() (gas: 17708)
 ProxyAdmin_Test:test_delegateResolvedGetProxyImplementation() (gas: 62016)

packages/contracts-bedrock/contracts/L1/L2OutputOracle.sol

-//SPDX-License-Identifier: MIT
+// SPDX-License-Identifier: MIT
 pragma solidity 0.8.10;
 
 import {

packages/contracts-bedrock/contracts/L1/OptimismPortal.sol

-//SPDX-License-Identifier: MIT
+// SPDX-License-Identifier: MIT
 pragma solidity 0.8.10;
 
 import { Initializable } from "@openzeppelin/contracts/proxy/utils/Initializable.sol";

packages/contracts-bedrock/contracts/L2/L1Block.sol

-//SPDX-License-Identifier: MIT
+// SPDX-License-Identifier: MIT
 pragma solidity 0.8.10;
 
 /**

packages/contracts-bedrock/contracts/L2/L1BlockNumber.sol

-//SPDX-License-Identifier: MIT
+// SPDX-License-Identifier: MIT
 pragma solidity 0.8.10;
 
 import { L1Block } from "./L1Block.sol";

packages/contracts-bedrock/contracts/L2/SequencerFeeVault.sol

 // SPDX-License-Identifier: MIT
 pragma solidity ^0.8.9;
 
-/* Library Imports */
 import { PredeployAddresses } from "../libraries/PredeployAddresses.sol";
-
-/* Contract Imports */
 import { L2StandardBridge } from "./L2StandardBridge.sol";
 
 /**

packages/contracts-bedrock/contracts/legacy/AddressManager.sol

 // SPDX-License-Identifier: MIT
 pragma solidity ^0.8.9;
 
-/* External Imports */
 import { Ownable } from "@openzeppelin/contracts/access/Ownable.sol";
 
 /**
+ * @custom:legacy
  * @title AddressManager
+ * @notice AddressManager is a legacy contract that was used in the old version of the Optimism
+ *         system to manage a registry of string names to addresses. We now use a more standard
+ *         proxy system instead, but this contract is still necessary for backwards compatibility
+ *         with several older contracts.
  */
 contract AddressManager is Ownable {
-    /**********
-     * Events *
-     **********/
-
+    /**
+     * @notice Emitted when an address is modified in the registry.
+     *
+     * @param name       String name being set in the registry.
+     * @param newAddress Address set for the given name.
+     * @param oldAddress Address that was previously set for the given name.
+     */
     event AddressSet(string indexed name, address newAddress, address oldAddress);
 
-    /*************
-     * Variables *
-     *************/
-
+    /**
+     * @notice Mapping of the hashes of string names to addresses.
+     */
     mapping(bytes32 => address) private addresses;
 
-    /********************
-     * Public Functions *
-     ********************/
-
     /**
-     * Changes the address associated with a particular name.
-     * @param _name String name to associate an address with.
+     * @notice Changes the address associated with a particular name.
+     *
+     * @param _name    String name to associate an address with.
      * @param _address Address to associate with the name.
      */
     function setAddress(string memory _name, address _address) external onlyOwner {
@@ -38,21 +41,21 @@ contract AddressManager is Ownable {
     }
 
     /**
-     * Retrieves the address associated with a given name.
+     * @notice Retrieves the address associated with a given name.
+     *
      * @param _name Name to retrieve an address for.
+     *
      * @return Address associated with the given name.
      */
     function getAddress(string memory _name) external view returns (address) {
         return addresses[_getNameHash(_name)];
     }
 
-    /**********************
-     * Internal Functions *
-     **********************/
-
     /**
-     * Computes the hash of a name.
+     * @notice Computes the hash of a name.
+     *
      * @param _name Name to compute a hash for.
+     *
      * @return Hash of the given name.
      */
     function _getNameHash(string memory _name) internal pure returns (bytes32) {

packages/contracts-bedrock/contracts/legacy/AddressResolver.sol

-// SPDX-License-Identifier: MIT
-pragma solidity ^0.8.9;
-
-/* Library Imports */
-import { AddressManager } from "./AddressManager.sol";
-
-/**
- * @title AddressResolver
- */
-abstract contract AddressResolver {
-    /*************
-     * Variables *
-     *************/
-
-    AddressManager public libAddressManager;
-
-    /***************
-     * Constructor *
-     ***************/
-
-    /**
-     * @param _libAddressManager Address of the AddressManager.
-     */
-    constructor(address _libAddressManager) {
-        libAddressManager = AddressManager(_libAddressManager);
-    }
-
-    /********************
-     * Public Functions *
-     ********************/
-
-    /**
-     * Resolves the address associated with a given name.
-     * @param _name Name to resolve an address for.
-     * @return Address associated with the given name.
-     */
-    function resolve(string memory _name) public view returns (address) {
-        return libAddressManager.getAddress(_name);
-    }
-}

packages/contracts-bedrock/contracts/legacy/L1ChugSplashProxy.sol

@@ -9,37 +9,37 @@ interface iL1ChugSplashDeployer {
 }
 
 /**
+ * @custom:legacy
  * @title L1ChugSplashProxy
- * @dev Basic ChugSplash proxy contract for L1. Very close to being a normal proxy but has added
- * functions `setCode` and `setStorage` for changing the code or storage of the contract. Nifty!
+ * @notice Basic ChugSplash proxy contract for L1. Very close to being a normal proxy but has added
+ *         functions `setCode` and `setStorage` for changing the code or storage of the contract.
  *
- * Note for future developers: do NOT make anything in this contract 'public' unless you know what
- * you're doing. Anything public can potentially have a function signature that conflicts with a
- * signature attached to the implementation contract. Public functions SHOULD always have the
- * 'proxyCallIfNotOwner' modifier unless there's some *really* good reason not to have that
- * modifier. And there almost certainly is not a good reason to not have that modifier. Beware!
+ *         Note for future developers: do NOT make anything in this contract 'public' unless you
+ *         know what you're doing. Anything public can potentially have a function signature that
+ *         conflicts with a signature attached to the implementation contract. Public functions
+ *         SHOULD always have the `proxyCallIfNotOwner` modifier unless there's some *really* good
+ *         reason not to have that modifier. And there almost certainly is not a good reason to not
+ *         have that modifier. Beware!
  */
 contract L1ChugSplashProxy {
-    /*************
-     * Constants *
-     *************/
-
-    // "Magic" prefix. When prepended to some arbitrary bytecode and used to create a contract, the
-    // appended bytecode will be deployed as given.
+    /**
+     * @notice "Magic" prefix. When prepended to some arbitrary bytecode and used to create a
+     *         contract, the appended bytecode will be deployed as given.
+     */
     bytes13 internal constant DEPLOY_CODE_PREFIX = 0x600D380380600D6000396000f3;
 
-    // bytes32(uint256(keccak256('eip1967.proxy.implementation')) - 1)
+    /**
+     * @notice bytes32(uint256(keccak256('eip1967.proxy.implementation')) - 1)
+     */
     bytes32 internal constant IMPLEMENTATION_KEY =
         0x360894a13ba1a3210667c828492db98dca3e2076cc3735a920a3ca505d382bbc;
 
-    // bytes32(uint256(keccak256('eip1967.proxy.admin')) - 1)
+    /**
+     * @notice bytes32(uint256(keccak256('eip1967.proxy.admin')) - 1)
+     */
     bytes32 internal constant OWNER_KEY =
         0xb53127684a568b3173ae13b9f8a6016e243e63b6e8ee1178d6a717850b5d6103;
 
-    /***************
-     * Constructor *
-     ***************/
-
     /**
      * @param _owner Address of the initial contract owner.
      */
@@ -47,13 +47,9 @@ contract L1ChugSplashProxy {
         _setOwner(_owner);
     }
 
-    /**********************
-     * Function Modifiers *
-     **********************/
-
     /**
-     * Blocks a function from being called when the parent signals that the system should be paused
-     * via an isUpgrading function.
+     * @notice Blocks a function from being called when the parent signals that the system should
+     *         be paused via an isUpgrading function.
      */
     modifier onlyWhenNotPaused() {
         address owner = _getOwner();
@@ -80,18 +76,19 @@ contract L1ChugSplashProxy {
     }
 
     /**
-     * Makes a proxy call instead of triggering the given function when the caller is either the
-     * owner or the zero address. Caller can only ever be the zero address if this function is
-     * being called off-chain via eth_call, which is totally fine and can be convenient for
-     * client-side tooling. Avoids situations where the proxy and implementation share a sighash
-     * and the proxy function ends up being called instead of the implementation one.
+     * @notice Makes a proxy call instead of triggering the given function when the caller is
+     *         either the owner or the zero address. Caller can only ever be the zero address if
+     *         this function is being called off-chain via eth_call, which is totally fine and can
+     *         be convenient for client-side tooling. Avoids situations where the proxy and
+     *         implementation share a sighash and the proxy function ends up being called instead
+     *         of the implementation one.
      *
-     * Note: msg.sender == address(0) can ONLY be triggered off-chain via eth_call. If there's a
-     * way for someone to send a transaction with msg.sender == address(0) in any real context then
-     * we have much bigger problems. Primary reason to include this additional allowed sender is
-     * because the owner address can be changed dynamically and we do not want clients to have to
-     * keep track of the current owner in order to make an eth_call that doesn't trigger the
-     * proxied contract.
+     *         Note: msg.sender == address(0) can ONLY be triggered off-chain via eth_call. If
+     *         there's a way for someone to send a transaction with msg.sender == address(0) in any
+     *         real context then we have much bigger problems. Primary reason to include this
+     *         additional allowed sender is because the owner address can be changed dynamically
+     *         and we do not want clients to have to keep track of the current owner in order to
+     *         make an eth_call that doesn't trigger the proxied contract.
      */
     // slither-disable-next-line incorrect-modifier
     modifier proxyCallIfNotOwner() {
@@ -103,29 +100,29 @@ contract L1ChugSplashProxy {
         }
     }
 
-    /*********************
-     * Fallback Function *
-     *********************/
-
     // slither-disable-next-line locked-ether
     fallback() external payable {
         // Proxy call by default.
         _doProxyCall();
     }
 
-    /********************
-     * Public Functions *
-     ********************/
+    // slither-disable-next-line locked-ether
+    receive() external payable {
+        // Proxy call by default.
+        _doProxyCall();
+    }
 
     /**
-     * Sets the code that should be running behind this proxy. Note that this scheme is a bit
-     * different from the standard proxy scheme where one would typically deploy the code
-     * separately and then set the implementation address. We're doing it this way because it gives
-     * us a lot more freedom on the client side. Can only be triggered by the contract owner.
+     * @notice Sets the code that should be running behind this proxy.
+     *
+     *         Note: This scheme is a bit different from the standard proxy scheme where one would
+     *         typically deploy the code separately and then set the implementation address. We're
+     *         doing it this way because it gives us a lot more freedom on the client side. Can
+     *         only be triggered by the contract owner.
+     *
      * @param _code New contract code to run inside this contract.
      */
-    // slither-disable-next-line external-function
-    function setCode(bytes memory _code) public proxyCallIfNotOwner {
+    function setCode(bytes memory _code) external proxyCallIfNotOwner {
         // Get the code hash of the current implementation.
         address implementation = _getImplementation();
 
@@ -156,53 +153,50 @@ contract L1ChugSplashProxy {
     }
 
     /**
-     * Modifies some storage slot within the proxy contract. Gives us a lot of power to perform
-     * upgrades in a more transparent way. Only callable by the owner.
-     * @param _key Storage key to modify.
+     * @notice Modifies some storage slot within the proxy contract. Gives us a lot of power to
+     *         perform upgrades in a more transparent way. Only callable by the owner.
+     *
+     * @param _key   Storage key to modify.
      * @param _value New value for the storage key.
      */
-    // slither-disable-next-line external-function
-    function setStorage(bytes32 _key, bytes32 _value) public proxyCallIfNotOwner {
+    function setStorage(bytes32 _key, bytes32 _value) external proxyCallIfNotOwner {
         assembly {
             sstore(_key, _value)
         }
     }
 
     /**
-     * Changes the owner of the proxy contract. Only callable by the owner.
+     * @notice Changes the owner of the proxy contract. Only callable by the owner.
+     *
      * @param _owner New owner of the proxy contract.
      */
-    // slither-disable-next-line external-function
-    function setOwner(address _owner) public proxyCallIfNotOwner {
+    function setOwner(address _owner) external proxyCallIfNotOwner {
         _setOwner(_owner);
     }
 
     /**
-     * Queries the owner of the proxy contract. Can only be called by the owner OR by making an
-     * eth_call and setting the "from" address to address(0).
+     * @notice Queries the owner of the proxy contract. Can only be called by the owner OR by
+     *         making an eth_call and setting the "from" address to address(0).
+     *
      * @return Owner address.
      */
-    // slither-disable-next-line external-function
-    function getOwner() public proxyCallIfNotOwner returns (address) {
+    function getOwner() external proxyCallIfNotOwner returns (address) {
         return _getOwner();
     }
 
     /**
-     * Queries the implementation address. Can only be called by the owner OR by making an
-     * eth_call and setting the "from" address to address(0).
+     * @notice Queries the implementation address. Can only be called by the owner OR by making an
+     *         eth_call and setting the "from" address to address(0).
+     *
      * @return Implementation address.
      */
-    // slither-disable-next-line external-function
-    function getImplementation() public proxyCallIfNotOwner returns (address) {
+    function getImplementation() external proxyCallIfNotOwner returns (address) {
         return _getImplementation();
     }
 
-    /**********************
-     * Internal Functions *
-     **********************/
-
     /**
-     * Sets the implementation address.
+     * @notice Sets the implementation address.
+     *
      * @param _implementation New implementation address.
      */
     function _setImplementation(address _implementation) internal {
@@ -212,7 +206,8 @@ contract L1ChugSplashProxy {
     }
 
     /**
-     * Queries the implementation address.
+     * @notice Queries the implementation address.
+     *
      * @return Implementation address.
      */
     function _getImplementation() internal view returns (address) {
@@ -224,7 +219,8 @@ contract L1ChugSplashProxy {
     }
 
     /**
-     * Changes the owner of the proxy contract.
+     * @notice Changes the owner of the proxy contract.
+     *
      * @param _owner New owner of the proxy contract.
      */
     function _setOwner(address _owner) internal {
@@ -234,7 +230,8 @@ contract L1ChugSplashProxy {
     }
 
     /**
-     * Queries the owner of the proxy contract.
+     * @notice Queries the owner of the proxy contract.
+     *
      * @return Owner address.
      */
     function _getOwner() internal view returns (address) {
@@ -246,8 +243,10 @@ contract L1ChugSplashProxy {
     }
 
     /**
-     * Gets the code hash for a given account.
+     * @notice Gets the code hash for a given account.
+     *
      * @param _account Address of the account to get a code hash for.
+     *
      * @return Code hash for the account.
      */
     function _getAccountCodeHash(address _account) internal view returns (bytes32) {
@@ -259,7 +258,7 @@ contract L1ChugSplashProxy {
     }
 
     /**
-     * Performs the proxy call via a delegatecall.
+     * @notice Performs the proxy call via a delegatecall.
      */
     function _doProxyCall() internal onlyWhenNotPaused {
         address implementation = _getImplementation();

packages/contracts-bedrock/contracts/legacy/LegacyERC20ETH.sol

 // SPDX-License-Identifier: MIT
-pragma solidity ^0.8.9;
+pragma solidity 0.8.10;
 
-/* Library Imports */
 import { PredeployAddresses } from "../libraries/PredeployAddresses.sol";
-
-/* Contract Imports */
 import { OptimismMintableERC20 } from "../universal/OptimismMintableERC20.sol";
 
 /**

packages/contracts-bedrock/contracts/legacy/ResolvedDelegateProxy.sol

@@ -4,47 +4,48 @@ pragma solidity ^0.8.9;
 import { AddressManager } from "./AddressManager.sol";
 
 /**
+ * @custom:legacy
  * @title ResolvedDelegateProxy
+ * @notice ResolvedDelegateProxy is a legacy proxy contract that makes use of the AddressManager to
+ *         resolve the implementation address. We're maintaining this contract for backwards
+ *         compatibility so we can manage all legacy proxies where necessary.
  */
 contract ResolvedDelegateProxy {
-    /*************
-     * Variables *
-     *************/
-
-    // Using mappings to store fields to avoid overwriting storage slots in the
-    // implementation contract. For example, instead of storing these fields at
-    // storage slot `0` & `1`, they are stored at `keccak256(key + slot)`.
-    // See: https://solidity.readthedocs.io/en/v0.7.0/internals/layout_in_storage.html
-    // NOTE: Do not use this code in your own contract system.
-    //      There is a known flaw in this contract, and we will remove it from the repository
-    //      in the near future. Due to the very limited way that we are using it, this flaw is
-    //      not an issue in our system.
+    /**
+     * @notice Mapping used to store the implementation name that corresponds to this contract. A
+     *         mapping was originally used as a way to bypass the same issue normally solved by
+     *         storing the implementation address in a specific storage slot that does not conflict
+     *         with any other storage slot. Generally NOT a safe solution but works as long as the
+     *         implementation does not also keep a mapping in the first storage slot.
+     */
     mapping(address => string) private implementationName;
-    mapping(address => AddressManager) private addressManager;
 
-    /***************
-     * Constructor *
-     ***************/
+    /**
+     * @notice Mapping used to store the address of the AddressManager contract where the
+     *         implementation address will be resolved from. Same concept here as with the above
+     *         mapping. Also generally unsafe but fine if the implementation doesn't keep a mapping
+     *         in the second storage slot.
+     */
+    mapping(address => AddressManager) private addressManager;
 
     /**
-     * @param _libAddressManager Address of the AddressManager.
+     * @param _addressManager  Address of the AddressManager.
      * @param _implementationName implementationName of the contract to proxy to.
      */
-    constructor(address _libAddressManager, string memory _implementationName) {
-        addressManager[address(this)] = AddressManager(_libAddressManager);
+    constructor(AddressManager _addressManager, string memory _implementationName) {
+        addressManager[address(this)] = _addressManager;
         implementationName[address(this)] = _implementationName;
     }
 
-    /*********************
-     * Fallback Function *
-     *********************/
-
+    /**
+     * @notice Fallback, performs a delegatecall to the resolved implementation address.
+     */
     fallback() external payable {
         address target = addressManager[address(this)].getAddress(
             (implementationName[address(this)])
         );
 
-        require(target != address(0), "Target address must be initialized.");
+        require(target != address(0), "ResolvedDelegateProxy: target address must be initialized");
 
         // slither-disable-next-line controlled-delegatecall
         (bool success, bytes memory returndata) = target.delegatecall(msg.data);

packages/contracts-bedrock/contracts/libraries/Burn.sol

@@ -4,7 +4,8 @@ pragma solidity 0.8.10;
 /**
  * @title Burner
  * @notice Burner self-destructs on creation and sends all ETH to itself, removing all ETH given to
- * the contract from the circulating supply.
+ *         the contract from the circulating supply. Self-destructing is the only way to remove ETH
+ *         from the circulating supply.
  */
 contract Burner {
     constructor() payable {

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

@@ -186,7 +186,7 @@ contract Messenger_Initializer is L2OutputOracle_Initializer {
         vm.prank(multisig);
         addressManager.setAddress("OVM_L1CrossDomainMessenger", address(L1MessengerImpl));
         ResolvedDelegateProxy proxy = new ResolvedDelegateProxy(
-            address(addressManager),
+            addressManager,
             "OVM_L1CrossDomainMessenger"
         );
         L1Messenger = L1CrossDomainMessenger(address(proxy));

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

@@ -38,7 +38,7 @@ contract ProxyAdmin_Test is Test {
         // Deploy a legacy ResolvedDelegateProxy with the name `a`.
         // Whatever `a` is set to in AddressManager will be the address
         // that is used for the implementation.
-        resolved = new ResolvedDelegateProxy(address(addressManager), "a");
+        resolved = new ResolvedDelegateProxy(addressManager, "a");
 
         // Impersonate alice for setting up the admin.
         vm.startPrank(alice);