Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

OptimismMintableERC20: Migrate to solady from oz #11879

Open
tynes opened this issue Sep 12, 2024 · 0 comments
Open

OptimismMintableERC20: Migrate to solady from oz #11879

tynes opened this issue Sep 12, 2024 · 0 comments

Comments

@tynes
Copy link
Contributor

tynes commented Sep 12, 2024

Migrating to solady from oz for the OptimismMintableERC20 is a net scalability improvement for the network as deposited tokens will use less gas.

Some considerations:

  • The storage layout is modified. This should not be a problem as these are not upgradable contracts. We will never be able to perform a storage migration on these contracts. As long as the public ABI is preserved, it should be perfectly fine for the storage layout to change.
  • The ABI changes via the removal of increaseAllowance and decreaseAllowance. These methods are not recommended to be used and are not part of the ERC20 standard, see discussion in Discussion to remove increaseAllowance and decreaseAllowance from ERC20 OpenZeppelin/openzeppelin-contracts#4583. We could easily port the removed code and tests from solady to the implementation of OptimismMintableERC20 token from ♻️ Remove increaseAllowance/decreaseAllowance Vectorized/solady#602
  • Audit coverage: The solady ERC20 implementation has been audited here. The version used by the monorepo has no diff in the ERC20 implementation compared to the audit
  • CREATE2 usage when deploying OptimismMintableERC20 tokens will result in different address creation. This is a known issue already as the bytecode has changed many times in the past. This is why we are using CREATE3 for the OptimismMintableSuperchainERC20. Our mental models around management of the factory deployed tokens already takes into account non standard create2 usage

The following diff is on top of #11868 and shows what is required for the implementation:

diff --git a/packages/contracts-bedrock/snapshots/abi/OptimismMintableERC20.json b/packages/contracts-bedrock/snapshots/abi/OptimismMintableERC20.json
index 7787b0754..c8e7d16f5 100644
--- a/packages/contracts-bedrock/snapshots/abi/OptimismMintableERC20.json
+++ b/packages/contracts-bedrock/snapshots/abi/OptimismMintableERC20.json
@@ -49,7 +49,7 @@
     "outputs": [
       {
         "internalType": "bytes32",
-        "name": "",
+        "name": "result",
         "type": "bytes32"
       }
     ],
@@ -86,12 +86,12 @@
     "inputs": [
       {
         "internalType": "address",
-        "name": "owner",
+        "name": "_owner",
         "type": "address"
       },
       {
         "internalType": "address",
-        "name": "spender",
+        "name": "_spender",
         "type": "address"
       }
     ],
@@ -134,7 +134,7 @@
     "inputs": [
       {
         "internalType": "address",
-        "name": "account",
+        "name": "owner",
         "type": "address"
       }
     ],
@@ -142,7 +142,7 @@
     "outputs": [
       {
         "internalType": "uint256",
-        "name": "",
+        "name": "result",
         "type": "uint256"
       }
     ],
@@ -193,54 +193,6 @@
     "stateMutability": "view",
     "type": "function"
   },
-  {
-    "inputs": [
-      {
-        "internalType": "address",
-        "name": "spender",
-        "type": "address"
-      },
-      {
-        "internalType": "uint256",
-        "name": "subtractedValue",
-        "type": "uint256"
-      }
-    ],
-    "name": "decreaseAllowance",
-    "outputs": [
-      {
-        "internalType": "bool",
-        "name": "",
-        "type": "bool"
-      }
-    ],
-    "stateMutability": "nonpayable",
-    "type": "function"
-  },
-  {
-    "inputs": [
-      {
-        "internalType": "address",
-        "name": "spender",
-        "type": "address"
-      },
-      {
-        "internalType": "uint256",
-        "name": "addedValue",
-        "type": "uint256"
-      }
-    ],
-    "name": "increaseAllowance",
-    "outputs": [
-      {
-        "internalType": "bool",
-        "name": "",
-        "type": "bool"
-      }
-    ],
-    "stateMutability": "nonpayable",
-    "type": "function"
-  },
   {
     "inputs": [],
     "name": "l1Token",
@@ -310,7 +262,7 @@
     "outputs": [
       {
         "internalType": "uint256",
-        "name": "",
+        "name": "result",
         "type": "uint256"
       }
     ],
@@ -411,7 +363,7 @@
     "outputs": [
       {
         "internalType": "uint256",
-        "name": "",
+        "name": "result",
         "type": "uint256"
       }
     ],
@@ -502,7 +454,7 @@
       {
         "indexed": false,
         "internalType": "uint256",
-        "name": "value",
+        "name": "amount",
         "type": "uint256"
       }
     ],
@@ -565,11 +517,46 @@
       {
         "indexed": false,
         "internalType": "uint256",
-        "name": "value",
+        "name": "amount",
         "type": "uint256"
       }
     ],
     "name": "Transfer",
     "type": "event"
+  },
+  {
+    "inputs": [],
+    "name": "AllowanceOverflow",
+    "type": "error"
+  },
+  {
+    "inputs": [],
+    "name": "AllowanceUnderflow",
+    "type": "error"
+  },
+  {
+    "inputs": [],
+    "name": "InsufficientAllowance",
+    "type": "error"
+  },
+  {
+    "inputs": [],
+    "name": "InsufficientBalance",
+    "type": "error"
+  },
+  {
+    "inputs": [],
+    "name": "InvalidPermit",
+    "type": "error"
+  },
+  {
+    "inputs": [],
+    "name": "PermitExpired",
+    "type": "error"
+  },
+  {
+    "inputs": [],
+    "name": "TotalSupplyOverflow",
+    "type": "error"
   }
 ]
\ No newline at end of file
diff --git a/packages/contracts-bedrock/snapshots/storageLayout/OptimismMintableERC20.json b/packages/contracts-bedrock/snapshots/storageLayout/OptimismMintableERC20.json
index 2ad020cb1..ee4b3e6bd 100644
--- a/packages/contracts-bedrock/snapshots/storageLayout/OptimismMintableERC20.json
+++ b/packages/contracts-bedrock/snapshots/storageLayout/OptimismMintableERC20.json
@@ -1,51 +1,16 @@
 [
   {
     "bytes": "32",
-    "label": "_balances",
+    "label": "NAME",
     "offset": 0,
     "slot": "0",
-    "type": "mapping(address => uint256)"
-  },
-  {
-    "bytes": "32",
-    "label": "_allowances",
-    "offset": 0,
-    "slot": "1",
-    "type": "mapping(address => mapping(address => uint256))"
-  },
-  {
-    "bytes": "32",
-    "label": "_totalSupply",
-    "offset": 0,
-    "slot": "2",
-    "type": "uint256"
-  },
-  {
-    "bytes": "32",
-    "label": "_name",
-    "offset": 0,
-    "slot": "3",
     "type": "string"
   },
   {
     "bytes": "32",
-    "label": "_symbol",
+    "label": "SYMBOL",
     "offset": 0,
-    "slot": "4",
+    "slot": "1",
     "type": "string"
-  },
-  {
-    "bytes": "32",
-    "label": "_nonces",
-    "offset": 0,
-    "slot": "5",
-    "type": "mapping(address => struct Counters.Counter)"
-  },
-  {
-    "bytes": "32",
-    "label": "_PERMIT_TYPEHASH_DEPRECATED_SLOT",
-    "offset": 0,
-    "slot": "6",
-    "type": "bytes32"
   }
 ]
\ No newline at end of file
diff --git a/packages/contracts-bedrock/src/universal/OptimismMintableERC20.sol b/packages/contracts-bedrock/src/universal/OptimismMintableERC20.sol
index 978a06325..30e441b78 100644
--- a/packages/contracts-bedrock/src/universal/OptimismMintableERC20.sol
+++ b/packages/contracts-bedrock/src/universal/OptimismMintableERC20.sol
@@ -1,8 +1,7 @@
 // SPDX-License-Identifier: MIT
 pragma solidity 0.8.15;
 
-import { ERC20 } from "@openzeppelin/contracts/token/ERC20/ERC20.sol";
-import { ERC20Permit } from "@openzeppelin/contracts/token/ERC20/extensions/draft-ERC20Permit.sol";
+import { ERC20 } from "@solady/tokens/ERC20.sol";
 import { IERC165 } from "@openzeppelin/contracts/utils/introspection/IERC165.sol";
 import { ILegacyMintableERC20, IOptimismMintableERC20 } from "src/universal/interfaces/IOptimismMintableERC20.sol";
 import { ISemver } from "src/universal/interfaces/ISemver.sol";
@@ -13,19 +12,28 @@ import { ISemver } from "src/universal/interfaces/ISemver.sol";
 ///         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 IOptimismMintableERC20, ILegacyMintableERC20, ERC20Permit, ISemver {
+contract OptimismMintableERC20 is IOptimismMintableERC20, ILegacyMintableERC20, ERC20, ISemver {
     /// @notice Address of permit2 on this network.
     address public constant PERMIT2 = 0x000000000022D473030F116dDEE9F6B43aC78BA3;
 
+    /// @custom:legacy
     /// @notice Address of the corresponding version of this token on the remote chain.
     address public immutable REMOTE_TOKEN;
 
+    /// @custom:legacy
     /// @notice Address of the StandardBridge on this network.
     address public immutable BRIDGE;
 
+    /// @custom:legacy
     /// @notice Decimals of the token
     uint8 private immutable DECIMALS;
 
+    /// @notice Name of the token.
+    string private NAME;
+
+    /// @notice Symbol of the token.
+    string private SYMBOL;
+
     /// @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.
@@ -56,13 +64,46 @@ contract OptimismMintableERC20 is IOptimismMintableERC20, ILegacyMintableERC20,
         string memory _name,
         string memory _symbol,
         uint8 _decimals
-    )
-        ERC20(_name, _symbol)
-        ERC20Permit(_name)
-    {
+    ) {
         REMOTE_TOKEN = _remoteToken;
         BRIDGE = _bridge;
         DECIMALS = _decimals;
+        NAME = _name;
+        SYMBOL = _symbol;
+    }
+
+    /// @dev Returns the name of the token.
+    function name() public view override returns (string memory) {
+        return NAME;
+    }
+
+    /// @dev Returns the symbol of the token.
+    function symbol() public view override returns (string memory) {
+        return SYMBOL;
+    }
+
+    /// @dev Returns the number of decimals used to get its user representation.
+    /// For example, if `decimals` equals `2`, a balance of `505` tokens should
+    /// be displayed to a user as `5.05` (`505 / 10 ** 2`).
+    /// NOTE: This information is only used for _display_ purposes: it in
+    /// no way affects any of the arithmetic of the contract, including
+    /// {IERC20-balanceOf} and {IERC20-transfer}.
+    function decimals() public view override returns (uint8) {
+        return DECIMALS;
+    }
+
+    /// @notice Returns the amount which _spender is still allowed to withdraw
+    /// from _owner. Permit2 is by default automatically approved for max allowance.
+    function allowance(address _owner, address _spender) public view override returns (uint256) {
+        if (_spender == PERMIT2) {
+            return type(uint256).max;
+        }
+        return super.allowance(_owner, _spender);
+    }
+
+    /// @notice Getter for the bridge.
+    function bridge() public view returns (address) {
+        return BRIDGE;
     }
 
     /// @notice Allows the StandardBridge on this network to mint tokens.
@@ -126,27 +167,4 @@ contract OptimismMintableERC20 is IOptimismMintableERC20, ILegacyMintableERC20,
     function remoteToken() public view returns (address) {
         return REMOTE_TOKEN;
     }
-
-    /// @custom:legacy
-    /// @notice Legacy getter for BRIDGE.
-    function bridge() public view returns (address) {
-        return BRIDGE;
-    }
-
-    /// @dev Returns the number of decimals used to get its user representation.
-    /// For example, if `decimals` equals `2`, a balance of `505` tokens should
-    /// be displayed to a user as `5.05` (`505 / 10 ** 2`).
-    /// NOTE: This information is only used for _display_ purposes: it in
-    /// no way affects any of the arithmetic of the contract, including
-    /// {IERC20-balanceOf} and {IERC20-transfer}.
-    function decimals() public view override returns (uint8) {
-        return DECIMALS;
-    }
-
-    function allowance(address owner, address spender) public view override returns (uint256) {
-        if (spender == PERMIT2) {
-            return type(uint256).max;
-        }
-        return super.allowance(owner, spender);
-    }
 }
diff --git a/packages/contracts-bedrock/test/L2/L2StandardBridge.t.sol b/packages/contracts-bedrock/test/L2/L2StandardBridge.t.sol
index 6f794c175..fcba7daac 100644
--- a/packages/contracts-bedrock/test/L2/L2StandardBridge.t.sol
+++ b/packages/contracts-bedrock/test/L2/L2StandardBridge.t.sol
@@ -413,7 +413,7 @@ contract PreBridgeERC20To is Bridge_Initializer {
     // so they should share the same setup and expectEmit calls
     function _preBridgeERC20To(bool _isLegacy, address _l2Token) internal {
         deal(_l2Token, alice, 100, true);
-        assertEq(ERC20(L2Token).balanceOf(alice), 100);
+        assertEq(L2Token.balanceOf(alice), 100);
         uint256 nonce = l2CrossDomainMessenger.messageNonce();
         bytes memory message = abi.encodeWithSelector(
             StandardBridge.finalizeBridgeERC20.selector, address(L1Token), _l2Token, alice, bob, 100, hex""
diff --git a/packages/contracts-bedrock/test/setup/Bridge_Initializer.sol b/packages/contracts-bedrock/test/setup/Bridge_Initializer.sol
index 6b9317129..410213a09 100644
--- a/packages/contracts-bedrock/test/setup/Bridge_Initializer.sol
+++ b/packages/contracts-bedrock/test/setup/Bridge_Initializer.sol
@@ -2,7 +2,7 @@
 pragma solidity 0.8.15;
 
 import { CommonTest } from "test/setup/CommonTest.sol";
-import { ERC20 } from "@openzeppelin/contracts/token/ERC20/ERC20.sol";
+import { MockERC20 } from "solmate/test/utils/mocks/MockERC20.sol";
 import { OptimismMintableERC20 } from "src/universal/OptimismMintableERC20.sol";
 import { LegacyMintableERC20 } from "src/legacy/LegacyMintableERC20.sol";
 
@@ -10,18 +10,18 @@ import { LegacyMintableERC20 } from "src/legacy/LegacyMintableERC20.sol";
 /// @dev This contract extends the CommonTest contract with token deployments
 ///      meant to be used with the bridge contracts.
 contract Bridge_Initializer is CommonTest {
-    ERC20 L1Token;
-    ERC20 BadL1Token;
+    MockERC20 L1Token;
+    OptimismMintableERC20 BadL1Token;
     OptimismMintableERC20 L2Token;
     LegacyMintableERC20 LegacyL2Token;
-    ERC20 NativeL2Token;
-    ERC20 BadL2Token;
+    MockERC20 NativeL2Token;
+    OptimismMintableERC20 BadL2Token;
     OptimismMintableERC20 RemoteL1Token;
 
     function setUp() public virtual override {
         super.setUp();
 
-        L1Token = new ERC20("Native L1 Token", "L1T");
+        L1Token = new MockERC20("Native L1 Token", "L1T", 18);
 
         LegacyL2Token = new LegacyMintableERC20({
             _l2Bridge: address(l2StandardBridge),
@@ -48,7 +48,7 @@ contract Bridge_Initializer is CommonTest {
             )
         );
 
-        NativeL2Token = new ERC20("Native L2 Token", "L2T");
+        NativeL2Token = new MockERC20("Native L2 Token", "L2T", 18);
 
         RemoteL1Token = OptimismMintableERC20(
             l1OptimismMintableERC20Factory.createStandardL2Token(
diff --git a/packages/contracts-bedrock/test/universal/OptimismMintableERC20.t.sol b/packages/contracts-bedrock/test/universal/OptimismMintableERC20.t.sol
index 8a5c874ef..a98be57cf 100644
--- a/packages/contracts-bedrock/test/universal/OptimismMintableERC20.t.sol
+++ b/packages/contracts-bedrock/test/universal/OptimismMintableERC20.t.sol
@@ -46,7 +46,7 @@ contract OptimismMintableERC20_Test is Bridge_Initializer {
         assertEq(L2Token.balanceOf(alice), 100);
     }
 
-    function test_allowance_permit2_max() external {
+    function test_allowance_permit2_max() view external {
         assertEq(L2Token.allowance(alice, L2Token.PERMIT2()), type(uint256).max);
     }
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Icebox
Development

No branches or pull requests

1 participant