Fix/baseline conflicts #52
Conversation
* feat: add superchain erc20 baseline * feat: make superchain ERC20 simpler * fix: small version fix and tests * test: fix test name
| struct OptimismSuperchainERC20Metadata { | ||
| /// @notice Address of the corresponding version of this token on the remote chain. | ||
| address remoteToken; |
There was a problem hiding this comment.
Can't we just have a storage slot for the address instead of defining a struct for that contains only 1 field?
There was a problem hiding this comment.
yes, that can be done at well, i actually like it better, good suggestion
| @@ -127,11 +131,11 @@ contract OptimismSuperchainERC20Test is Test { | |||
| uint256 _toBalanceBefore = superchainERC20.balanceOf(_to); | |||
|
|
|||
| // Look for the emit of the `Transfer` event | |||
| vm.expectEmit(true, true, true, true, address(superchainERC20)); | |||
| vm.expectEmit(address(superchainERC20)); | |||
There was a problem hiding this comment.
Removing the true, true, true, true doesn't alter the way the cheatcode checks the emitted args are done properly and in order?
There was a problem hiding this comment.
it's the same as if they were there, it's shorthand way of writing it
| /// @title SuperchainERC20Test | ||
| /// @notice Contract for testing the SuperchainERC20 contract. | ||
| contract SuperchainERC20Test is Test { | ||
| address internal constant ZERO_ADDRESS = address(0); |
There was a problem hiding this comment.
| address internal constant ZERO_ADDRESS = address(0); | |
| address internal constant ZERO_ADDRESS; |
| { | ||
| "inputs": [], | ||
| "name": "ZeroAddress", | ||
| "type": "error" | ||
| }, |
There was a problem hiding this comment.
These lines are repeated below, were them autogenerated? Are we sure we need them or could be removed?
There was a problem hiding this comment.
this is autogenerated, but if this is repeated then we may have the error in both the SuperchainERC20 and the OptimismSuperchainERC20, will check and remove accordingly
| } | ||
|
|
||
| /// @notice Tests the `relayERC20` function reverts when the `_to` address is the zero address. | ||
| function testFuzz_relayERC20_zeroAddressTo_reverts(uint256 _amount) public { |
There was a problem hiding this comment.
NIT: This should go above testFuzz_relayERC20_notCrossDomainSender_reverts
There was a problem hiding this comment.
Something I'm not entirely sure about is the zero address check in both sendERC20 and relayERC20 functions. Those are not part of the invariants in the specs, so this might not be the most general implementation.
https://github.com/ethereum-optimism/specs/blob/main/specs/interop/token-bridging.md#invariants
| /// @param _to Address to relay tokens to. | ||
| /// @param _amount Amount of tokens to relay. | ||
| function relayERC20(address _from, address _to, uint256 _amount) external virtual { | ||
| if (_to == address(0)) revert ZeroAddress(); |
There was a problem hiding this comment.
isn't this check redundant? the _to == address(0) is already checked on sendERC20. Since only messages initiated in sendERC20 can bypass the following checks, this might be removed, even if gas-wise its very cheap.
|
Comments addressed in new PR due to having to rebase again: #55 |
No description provided.