feat: liquidity migration #10
Conversation
| interface IOptimismMintableERC20Factory { | ||
| function deployments(address) external view returns (address); | ||
| } | ||
|
|
There was a problem hiding this comment.
We could use an interface that is generic with just the deployments function. We currently do this sometimes, it reduces the number of files when verifying a contract and i think it will reduce compile time if we use interfaces everywhere rather than importing full implementations
|
This is looking great so far! |
|
The contract implementation looks good to me as well! |
* test: add L2 standard bridge interop unit tests * fix: add tests natspec * fix: unit tests fixes * fix: super to legacy tests failing * fix: mock and expect mint and burn
* test: add L2 standard bridge interop unit tests * fix: add tests natspec * fix: add generic factory interface
* test: add L2 standard bridge interop unit tests * fix: add tests natspec * fix: add generic factory interface * feat: modify OptimismMintableERC20Factory for convert * fix: use only a public function for create3 * feat: rollback interop factory, modify legacy one * fix: delete local token return variable
There was a problem hiding this comment.
This way of handling the new semver is missing: https://github.com/ethereum-optimism/design-docs/blob/main/smart-contract-feature-development.md#handling-semver on L2StandardBridgeInterop.
Also, I think you need to sum a minor patch on the L2StandardBridge contract since you are adding functions and logic to it with the interop extension.
| error InvalidTokenPair(); | ||
|
|
||
| // TODO: Use an existing interface with `mint` and `burn`? | ||
| interface MintableAndBurnable is IERC20 { |
There was a problem hiding this comment.
WDYT about this?
| interface MintableAndBurnable is IERC20 { | |
| interface Mintable { |
| // 3. Valid SuperchainERC20 check | ||
| address _superRemoteToken = | ||
| IOptimismERC20Factory(Predeploys.OPTIMISM_SUPERCHAIN_ERC20_FACTORY).deployments(_superAddr); | ||
| if (_superRemoteToken == address(0)) revert InvalidSuperchainAddress(); |
There was a problem hiding this comment.
Not convinced by the error name. It's understandable in context, but for a first time reader it might be confusing. Maybe InvalidSuperchainERC20Address, even if it sounds ugly. Wdty? I know it also affects the Legacy error, so maybe nay
| /// @notice The L2StandardBridgeInterop is an extension of the L2StandardBridge that allows for | ||
| /// the conversion of tokens between legacy tokens (OptimismMintableERC20 or StandardL2ERC20) | ||
| /// and SuperchainERC20 tokens. | ||
| contract L2StandardBridgeInterop is L2StandardBridge { |
There was a problem hiding this comment.
Why are extending the standard bridge instead of updating it? did you have this conversation with mark?
There was a problem hiding this comment.
It is how the interop contracts are being develop, here is a design guide https://github.com/ethereum-optimism/design-docs/blob/main/smart-contract-feature-development.md#composition-through-inheritance
| // 3. Valid SuperchainERC20 check | ||
| address _superRemoteToken = | ||
| IOptimismERC20Factory(Predeploys.OPTIMISM_SUPERCHAIN_ERC20_FACTORY).deployments(_superAddr); | ||
| if (_superRemoteToken == address(0)) revert InvalidSuperchainAddress(); |
There was a problem hiding this comment.
Is this check necessary? you are already checking that _legacy !=0 and that _legacy == _super
There was a problem hiding this comment.
I think is just for the revert error. I'd leave it, doesn't harm since we'll be using an OP chain so gas is not an issue
There was a problem hiding this comment.
Yeah, its also a very cheap check. Let's leave it for clarity :)
|
Amazing job sir! |
|
External review at ethereum-optimism#11479 |
|
Merged at ethereum-optimism#11479 |
Closes OPT-175
Closes OPT-174
Closes OPT-177