ethereum-optimism / ethereum-optimism/optimism

OptimismMintableERC20: Migrate to solady from oz

Open
#11,879 0 comments 0 reactions 0 assignees View on GitHub
Dominant language
Go
Stars
6.5k
Forks
4k
Avg merge
2d 15h
Merged PRs (30d)
145

Description

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 https://github.com/OpenZeppelin/openzeppelin-contracts/issues/4583. We could easily port the removed code and tests from solady to the implementation of `OptimismMintableERC20` token from https://github.com/Vectorized/solady/pull/602
- Audit coverage: The solady ERC20 implementation has been audited [here](https://github.com/Vectorized/solady/blob/main/audits/cantina-solady-report.pdf). 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 https://github.com/ethereum-optimism/optimism/pull/11868 and shows what is required for the implementation:

```diff
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);
}
```

Contributor guide

Open the contributing guide

Research direction

Review the proposed diff and start with packages/contracts-bedrock/src/universal/OptimismMintableERC20.sol, then inspect the ABI and storage-layout snapshots under packages/contracts-bedrock/snapshots. Done means the contract uses solady while preserving the stated public behavior, and both snapshots reflect the ABI and storage changes shown.

Written by the indexing model from the issue text.

Assessment

Tech stack
solidity
Domain
blockchain
Issue type
Refactor
Difficulty
4/5
Estimated time
3-5 days
Activity status
Stale
Clarity
Clearly specified
Newbie friendliness
55/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.