fix(ctb): universal contract comments/errs/events (#2940)

Standardizes the comments, errors, and events for all contracts within
the /universal folder.

.changeset/gorgeous-buses-switch.md

+---
+'@eth-optimism/contracts-bedrock': patch
+---
+
+Standardizes comments, errors, and events for contracts in the /universal package

packages/contracts-bedrock/.gas-snapshot

@@ -103,18 +103,18 @@ OVM_ETH_Test:test_transfer() (gas: 10726)
 OVM_ETH_Test:test_transferFrom() (gas: 13008)
 OptimismMintableERC20_Test:test_bridge() (gas: 9785)
 OptimismMintableERC20_Test:test_burn() (gas: 52791)
-OptimismMintableERC20_Test:test_burnRevertsFromNotBridge() (gas: 13211)
+OptimismMintableERC20_Test:test_burnRevertsFromNotBridge() (gas: 13241)
 OptimismMintableERC20_Test:test_erc165_supportsInterface() (gas: 7828)
 OptimismMintableERC20_Test:test_l1Token() (gas: 9779)
 OptimismMintableERC20_Test:test_l2Bridge() (gas: 9790)
 OptimismMintableERC20_Test:test_mint() (gas: 65687)
-OptimismMintableERC20_Test:test_mintRevertsFromNotBridge() (gas: 13235)
+OptimismMintableERC20_Test:test_mintRevertsFromNotBridge() (gas: 13265)
 OptimismMintableERC20_Test:test_remoteToken() (gas: 9784)
 OptimismMintableTokenFactory_Test:test_bridge() (gas: 9707)
-OptimismMintableTokenFactory_Test:test_createStandardL2Token() (gas: 1100125)
-OptimismMintableTokenFactory_Test:test_createStandardL2TokenSameTwice() (gas: 2181161)
-OptimismMintableTokenFactory_Test:test_createStandardL2TokenShouldRevertIfRemoteIsZero() (gas: 9374)
-OptimismMintableTokenFactory_Test:test_initializeShouldRevert() (gas: 12696)
+OptimismMintableTokenFactory_Test:test_createStandardL2Token() (gas: 1115359)
+OptimismMintableTokenFactory_Test:test_createStandardL2TokenSameTwice() (gas: 2211629)
+OptimismMintableTokenFactory_Test:test_createStandardL2TokenShouldRevertIfRemoteIsZero() (gas: 9407)
+OptimismMintableTokenFactory_Test:test_initializeShouldRevert() (gas: 12732)
 OptimismPortalUpgradeable_Test:test_cannotInitImpl() (gas: 10936)
 OptimismPortalUpgradeable_Test:test_cannotInitProxy() (gas: 15934)
 OptimismPortalUpgradeable_Test:test_initValuesOnProxy() (gas: 16034)
@@ -137,12 +137,12 @@ OptimismPortal_Test:test_simple_isOutputFinalized() (gas: 23877)
 Proxy_Test:test_clashingFunctionSignatures() (gas: 101427)
 Proxy_Test:test_implementationKey() (gas: 20942)
 Proxy_Test:test_implementationProxyCallIfNotAdmin() (gas: 30021)
-Proxy_Test:test_implementationZeroAddress() (gas: 48006)
+Proxy_Test:test_implementationZeroAddress() (gas: 48007)
 Proxy_Test:test_itDelegatesToTheImplementation() (gas: 45173)
 Proxy_Test:test_ownerKey() (gas: 19113)
 Proxy_Test:test_ownerProxyCallIfNotAdmin() (gas: 34733)
 Proxy_Test:test_payableUpgradeToAndCall() (gas: 53887)
-Proxy_Test:test_revertUpgradeToAndCall() (gas: 104501)
+Proxy_Test:test_revertUpgradeToAndCall() (gas: 104654)
 Proxy_Test:test_upgradeToAndCall() (gas: 125238)
 Proxy_Test:test_zeroAddressCaller() (gas: 14758)
 ProxyAdmin_Test:test_chugsplashChangeProxyAdmin() (gas: 35647)
@@ -155,7 +155,7 @@ ProxyAdmin_Test:test_delegateResolvedGetProxyAdmin() (gas: 17685)
 ProxyAdmin_Test:test_delegateResolvedGetProxyImplementation() (gas: 62016)
 ProxyAdmin_Test:test_delegateResolvedUpgrade() (gas: 58422)
 ProxyAdmin_Test:test_delegateResolvedUpgradeAndCall() (gas: 97948)
-ProxyAdmin_Test:test_erc1967ChangeProxyAdmin() (gas: 33862)
+ProxyAdmin_Test:test_erc1967ChangeProxyAdmin() (gas: 33863)
 ProxyAdmin_Test:test_erc1967GetProxyAdmin() (gas: 15672)
 ProxyAdmin_Test:test_erc1967GetProxyImplementation() (gas: 52124)
 ProxyAdmin_Test:test_erc1967Upgrade() (gas: 50036)

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

@@ -41,7 +41,7 @@ contract OptimismMintableERC20_Test is Bridge_Initializer {
 
     function test_mintRevertsFromNotBridge() external {
         // NOT the bridge
-        vm.expectRevert("Only L2 Bridge can mint and burn");
+        vm.expectRevert("OptimismMintableERC20: only bridge can mint and burn");
         vm.prank(address(alice));
         L2Token.mint(alice, 100);
     }
@@ -61,7 +61,7 @@ contract OptimismMintableERC20_Test is Bridge_Initializer {
 
     function test_burnRevertsFromNotBridge() external {
         // NOT the bridge
-        vm.expectRevert("Only L2 Bridge can mint and burn");
+        vm.expectRevert("OptimismMintableERC20: only bridge can mint and burn");
         vm.prank(address(alice));
         L2Token.burn(alice, 100);
     }

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

@@ -17,7 +17,7 @@ contract OptimismMintableTokenFactory_Test is Bridge_Initializer {
     }
 
     function test_initializeShouldRevert() external {
-        vm.expectRevert("Already initialized.");
+        vm.expectRevert("OptimismMintableTokenFactory: already initialized");
         L2TokenFactory.initialize(address(L1Bridge));
     }
 
@@ -73,7 +73,7 @@ contract OptimismMintableTokenFactory_Test is Bridge_Initializer {
 
     function test_createStandardL2TokenShouldRevertIfRemoteIsZero() external {
         address remote = address(0);
-        vm.expectRevert("Must provide L1 token address");
+        vm.expectRevert("OptimismMintableTokenFactory: must provide remote token address");
         L2TokenFactory.createStandardL2Token(remote, "Beep", "BOOP");
     }
 }

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

@@ -184,7 +184,7 @@ contract Proxy_Test is Test {
         // Set the new SimpleStorage as the implementation
         // and call. This reverts because the calldata doesn't
         // match a function on the implementation.
-        vm.expectRevert();
+        vm.expectRevert("Proxy: delegatecall to new implementation contract failed");
         vm.prank(alice);
         proxy.upgradeToAndCall(
             address(simpleStorage),

packages/contracts-bedrock/contracts/universal/OptimismMintableERC20.sol

@@ -6,32 +6,44 @@ import "./SupportedInterfaces.sol";
 
 /**
  * @title OptimismMintableERC20
- * This contract represents the remote representation
- * of an ERC20 token. It is linked to the address of
- * a token in another domain and tokens can be locked
- * in the StandardBridge which will mint tokens in the
- * other domain.
+ * @notice OptimismMintableERC20 is a standard extension of the base ERC20 token contract designed
+ *         to allow the StandardBridge contracts to mint and burn tokens. This makes it possible to
+ *         use an OptimismMintablERC20 as the L2 representation of an L1 token, or vice-versa.
+ *         Designed to be backwards compatible with the older StandardL2ERC20 token which was only
+ *         meant for use on L2.
  */
 contract OptimismMintableERC20 is ERC20 {
-    event Mint(address indexed _account, uint256 _amount);
-    event Burn(address indexed _account, uint256 _amount);
+    /**
+     * @notice Emitted whenever tokens are minted for an account.
+     *
+     * @param account Address of the account tokens are being minted for.
+     * @param amount  Amount of tokens minted.
+     */
+    event Mint(address indexed account, uint256 amount);
 
     /**
-     * @notice The address of the token in the remote domain
+     * @notice Emitted whenever tokens are burned from an account.
+     *
+     * @param account Address of the account tokens are being burned from.
+     * @param amount  Amount of tokens burned.
+     */
+    event Burn(address indexed account, uint256 amount);
+
+    /**
+     * @notice Address of the corresponding version of this token on the remote chain.
      */
     address public remoteToken;
 
     /**
-     * @notice The address of the bridge responsible for
-     * minting. It is in the same domain.
+     * @notice Address of the StandardBridge on this network.
      */
     address public bridge;
 
     /**
-     * @param _bridge Address of the L2 standard bridge.
+     * @param _bridge      Address of the L2 standard bridge.
      * @param _remoteToken Address of the corresponding L1 token.
-     * @param _name ERC20 name.
-     * @param _symbol ERC20 symbol.
+     * @param _name        ERC20 name.
+     * @param _symbol      ERC20 symbol.
      */
     constructor(
         address _bridge,
@@ -44,17 +56,16 @@ contract OptimismMintableERC20 is ERC20 {
     }
 
     /**
-     * @notice Returns the corresponding L1 token address.
-     * This is a legacy function and wraps the remoteToken value.
+     * @custom:legacy
+     * @notice Legacy getter for the remote token. Use remoteToken going forward.
      */
     function l1Token() public view returns (address) {
         return remoteToken;
     }
 
     /**
-     * @notice The address of the bridge contract
-     * responsible for minting tokens. This is a legacy
-     * getter function
+     * @custom:legacy
+     * @notice Legacy getter for the bridge. Use bridge going forward.
      */
     function l2Bridge() public view returns (address) {
         return bridge;
@@ -64,15 +75,18 @@ contract OptimismMintableERC20 is ERC20 {
      * @notice A modifier that only allows the bridge to call
      */
     modifier onlyBridge() {
-        require(msg.sender == bridge, "Only L2 Bridge can mint and burn");
+        require(msg.sender == bridge, "OptimismMintableERC20: only bridge can mint and burn");
         _;
     }
 
     /**
-     * @notice ERC165
+     * @notice ERC165 interface check function.
+     *
+     * @param _interfaceId Interface ID to check.
+     *
+     * @return Whether or not the interface is supported by this contract.
      */
-    // slither-disable-next-line external-function
-    function supportsInterface(bytes4 _interfaceId) public pure returns (bool) {
+    function supportsInterface(bytes4 _interfaceId) external pure returns (bool) {
         bytes4 iface1 = type(IERC165).interfaceId;
         bytes4 iface2 = type(IL1Token).interfaceId;
         bytes4 iface3 = type(IRemoteToken).interfaceId;
@@ -80,22 +94,24 @@ contract OptimismMintableERC20 is ERC20 {
     }
 
     /**
-     * @notice The bridge can mint tokens
+     * @notice Allows the StandardBridge on this network to mint tokens.
+     *
+     * @param _to     Address to mint tokens to.
+     * @param _amount Amount of tokens to mint.
      */
-    // slither-disable-next-line external-function
-    function mint(address _to, uint256 _amount) public virtual onlyBridge {
+    function mint(address _to, uint256 _amount) external virtual onlyBridge {
         _mint(_to, _amount);
-
         emit Mint(_to, _amount);
     }
 
     /**
-     * @notice The bridge can burn tokens
+     * @notice Allows the StandardBridge on this network to burn tokens.
+     *
+     * @param _from   Address to burn tokens from.
+     * @param _amount Amount of tokens to burn.
      */
-    // slither-disable-next-line external-function
-    function burn(address _from, uint256 _amount) public virtual onlyBridge {
+    function burn(address _from, uint256 _amount) external virtual onlyBridge {
         _burn(_from, _amount);
-
         emit Burn(_from, _amount);
     }
 }

packages/contracts-bedrock/contracts/universal/OptimismMintableTokenFactory.sol

@@ -6,50 +6,77 @@ import { OptimismMintableERC20 } from "../universal/OptimismMintableERC20.sol";
 import { Lib_PredeployAddresses } from "../libraries/Lib_PredeployAddresses.sol";
 
 /**
+ * @custom:proxied
+ * @custom:predeployed 0x4200000000000000000000000000000000000012
  * @title OptimismMintableTokenFactory
- * @dev Factory contract for creating standard remote token representations of
- * local ERC20s. This can be used to bridge native L1 ERC20s to L2 or native L2
- * ERC20s to L1. The tokens created through this factory are meant to operate
- * with the StandardBridge contract for deposits/withdrawals.
- * This contract is a predeploy on L2 at 0x4200000000000000000000000000000000000012
- * TODO: deploy to a deterministic address on L1 networks?
- * TODO: should this be extended for L1/L2 with hardcoded values in
- * the base contract's initialize?
+ * @notice OptimismMintableTokenFactory is a factory contract that generates OptimismMintableERC20
+ *         contracts on the network it's deployed to. Simplifies the deployment process for users
+ *         who may be less familiar with deploying smart contracts. Designed to be backwards
+ *         compatible with the older StandardL2ERC20Factory contract.
  */
 contract OptimismMintableTokenFactory {
-    event StandardL2TokenCreated(address indexed _remoteToken, address indexed _localToken);
+    /**
+     * @custom:legacy
+     * @notice Emitted whenever a new OptimismMintableERC20 is created. Legacy version of the newer
+     *         OptimismMintableERC20Created event. We recommend relying on that event instead.
+     *
+     * @param remoteToken Address of the token on the remote chain.
+     * @param localToken  Address of the created token on the local chain.
+     */
+    event StandardL2TokenCreated(address indexed remoteToken, address indexed localToken);
+
+    /**
+     * @notice Emitted whenever a new OptimismMintableERC20 is created.
+     *
+     * @param localToken  Address of the created token on the local chain.
+     * @param remoteToken Address of the corresponding token on the remote chain.
+     * @param deployer    Address of the account that deployed the token.
+     */
     event OptimismMintableTokenCreated(
-        address indexed _localToken,
-        address indexed _remoteToken,
-        address _deployer
+        address indexed localToken,
+        address indexed remoteToken,
+        address deployer
     );
 
+    /**
+     * @notice Address of the StandardBridge on this chain.
+     */
     address public bridge;
 
     /**
-     * @dev Initialize the factory
-     * On L2 _bridge should be Lib_PredeployAddresses.L2_STANDARD_BRIDGE,
-     * On L1 _bridge should be the L1StandardBridge
+     * @notice Initializer.
+     *
+     * @param _bridge Address of the StandardBridge on this chain.
      */
     function initialize(address _bridge) public {
-        require(bridge == address(0), "Already initialized.");
+        require(bridge == address(0), "OptimismMintableTokenFactory: already initialized");
         bridge = _bridge;
     }
 
     /**
-     * @dev Creates an instance of the standard ERC20 token on L2.
-     * @param _remoteToken Address of the corresponding L1 token.
-     * @param _name ERC20 name.
-     * @param _symbol ERC20 symbol.
-     * @return Address of the new token.
+     * @notice Creates an instance of the OptimismMintableERC20 contract. Legacy version of the
+     *         newer createOptimismMintableERC20 function, which has a more intuitive name.
+     *
+     * @param _remoteToken Address of the token on the remote chain.
+     * @param _name        ERC20 name.
+     * @param _symbol      ERC20 symbol.
+     *
+     * @return Address of the newly created token.
      */
     function createStandardL2Token(
         address _remoteToken,
         string memory _name,
         string memory _symbol
     ) external returns (address) {
-        require(_remoteToken != address(0), "Must provide L1 token address");
-        require(bridge != address(0), "Must initialize first");
+        require(
+            _remoteToken != address(0),
+            "OptimismMintableTokenFactory: must provide remote token address"
+        );
+
+        require(
+            bridge != address(0),
+            "OptimismMintableTokenFactory: must initialize contract first"
+        );
 
         OptimismMintableERC20 localToken = new OptimismMintableERC20(
             bridge,
@@ -58,7 +85,7 @@ contract OptimismMintableTokenFactory {
             _symbol
         );
 
-        // Legacy Purposes
+        // Emit the old event too for legacy support.
         emit StandardL2TokenCreated(_remoteToken, address(localToken));
         emit OptimismMintableTokenCreated(_remoteToken, address(localToken), msg.sender);
 

packages/contracts-bedrock/contracts/universal/Proxy.sol

@@ -3,22 +3,22 @@ pragma solidity ^0.8.9;
 
 /**
  * @title Proxy
- * @notice Proxy is a transparent proxy that passes through the call
- *         if the caller is the owner or if the caller is `address(0)`,
- *         meaning that the call originated from an offchain simulation.
+ * @notice Proxy is a transparent proxy that passes through the call if the caller is the owner or
+ *         if the caller is address(0), meaning that the call originated from an off-chain
+ *         simulation.
  */
 contract Proxy {
     /**
-     * @notice An event that is emitted each time the implementation is changed.
-     *         This event is part of the EIP 1967 spec.
+     * @notice An event that is emitted each time the implementation is changed. This event is part
+     *         of the EIP-1967 specification.
      *
      * @param implementation The address of the implementation contract
      */
     event Upgraded(address indexed implementation);
 
     /**
-     * @notice An event that is emitted each time the owner is upgraded.
-     *         This event is part of the EIP 1967 spec.
+     * @notice An event that is emitted each time the owner is upgraded. This event is part of the
+     *         EIP-1967 specification.
      *
      * @param previousAdmin The previous owner of the contract
      * @param newAdmin      The new owner of the contract
@@ -40,12 +40,12 @@ contract Proxy {
         0xb53127684a568b3173ae13b9f8a6016e243e63b6e8ee1178d6a717850b5d6103;
 
     /**
-     * @notice set the initial owner during contract deployment. The
-     *         owner is stored at the eip1967 owner storage slot so that
-     *         storage collision with the implementation is not possible.
+     * @notice Sets the initial admin during contract deployment. Admin address is stored at the
+     *         EIP-1967 admin storage slot so that accidental storage collision with the
+     *         implementation is not possible.
      *
-     * @param _admin Address of the initial contract owner. The owner has
-     *               the ability to access the transparent proxy interface.
+     * @param _admin Address of the initial contract admin. Admin as the ability to access the
+     *               transparent proxy interface.
      */
     constructor(address _admin) {
         _changeAdmin(_admin);
@@ -58,11 +58,10 @@ contract Proxy {
     }
 
     /**
-     * @notice A modifier that reverts if not called by the owner
-     *         or by `address(0)` to allow `eth_call` to interact
-     *         with the proxy without needing to use low level storage
-     *         inspection. It is assumed that nobody controls the private
-     *         key for `address(0)`.
+     * @notice A modifier that reverts if not called by the owner or by address(0) to allow
+     *         eth_call to interact with this proxy without needing to use low-level storage
+     *         inspection. We assume that nobody is able to trigger calls from address(0) during
+     *         normal EVM execution.
      */
     modifier proxyCallIfNotAdmin() {
         if (msg.sender == _getAdmin() || msg.sender == address(0)) {
@@ -74,23 +73,21 @@ contract Proxy {
     }
 
     /**
-     * @notice Set the implementation contract address. The code at this
-     *         address will execute when this contract is called.
+     * @notice Set the implementation contract address. The code at the given address will execute
+     *         when this contract is called.
      *
-     * @param _implementation The address of the implementation contract
+     * @param _implementation Address of the implementation contract.
      */
     function upgradeTo(address _implementation) external proxyCallIfNotAdmin {
         _setImplementation(_implementation);
     }
 
     /**
-     * @notice Set the implementation and call a function in a single
-     *         transaction. This is useful to ensure atomic `initialize()`
-     *         based upgrades.
+     * @notice Set the implementation and call a function in a single transaction. Useful to ensure
+     *         atomic execution of initialization-based upgrades.
      *
-     * @param _implementation The address of the implementation contract
-     * @param _data           The calldata to delegatecall the new
-     *                        implementation with
+     * @param _implementation Address of the implementation contract.
+     * @param _data           Calldata to delegatecall the new implementation with.
      */
     function upgradeToAndCall(address _implementation, bytes calldata _data)
         external
@@ -100,7 +97,7 @@ contract Proxy {
     {
         _setImplementation(_implementation);
         (bool success, bytes memory returndata) = _implementation.delegatecall(_data);
-        require(success);
+        require(success, "Proxy: delegatecall to new implementation contract failed");
         return returndata;
     }
 
@@ -146,7 +143,7 @@ contract Proxy {
     /**
      * @notice Queries the implementation address.
      *
-     * @return implementation address.
+     * @return Implementation address.
      */
     function _getImplementation() internal view returns (address) {
         address implementation;
@@ -172,7 +169,7 @@ contract Proxy {
     /**
      * @notice Queries the owner of the proxy contract.
      *
-     * @return owner address.
+     * @return Owner address.
      */
     function _getAdmin() internal view returns (address) {
         address owner;
@@ -187,7 +184,6 @@ contract Proxy {
      */
     function _doProxyCall() internal {
         address implementation = _getImplementation();
-
         require(implementation != address(0), "Proxy: implementation not initialized");
 
         assembly {

packages/contracts-bedrock/contracts/universal/SupportedInterfaces.sol

@@ -5,17 +5,17 @@ pragma solidity ^0.8.9;
 import { IERC165 } from "@openzeppelin/contracts/utils/introspection/IERC165.sol";
 
 interface IRemoteToken {
-    function mint(address _to, uint256 _amount) external virtual;
+    function mint(address _to, uint256 _amount) external;
 
-    function burn(address _from, uint256 _amount) external virtual;
+    function burn(address _from, uint256 _amount) external;
 
-    function remoteToken() external virtual;
+    function remoteToken() external;
 }
 
 interface IL1Token {
-    function mint(address _to, uint256 _amount) external virtual;
+    function mint(address _to, uint256 _amount) external;
 
-    function burn(address _from, uint256 _amount) external virtual;
+    function burn(address _from, uint256 _amount) external;
 
-    function l1Token() external virtual;
+    function l1Token() external;
 }