feat: introduce OptimismSuperchainERC20Factory #30
Conversation
* 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
* feat: add superchain erc20 factory implementation * fix: remove createX comments
* test: add superchain erc20 factory tests * test: add erc20 asserts
| import { IBeacon } from "@openzeppelin/contracts-v5/proxy/beacon/IBeacon.sol"; | ||
| import { ISemver } from "src/universal/ISemver.sol"; | ||
|
|
||
| /// @custom:proxied |
There was a problem hiding this comment.
This contract isn't really proxied, right?
There was a problem hiding this comment.
All predeploys are proxied by default, maybe we should add it to the not proxied list
There was a problem hiding this comment.
If this the case, then leaving it like this is okay. I had the idea that this one was just going to have different code installed on hardforks, but I really don't remember where I got that from.
| /// This is used to keep track of the token deployments. | ||
| mapping(address superchainToken => address remoteToken) public deployments; | ||
|
|
||
| /// @notice Emitted when a OptimismSuperchainERC20 is deployed. |
There was a problem hiding this comment.
| /// @notice Emitted when a OptimismSuperchainERC20 is deployed. | |
| /// @notice Emitted when an OptimismSuperchainERC20 is deployed. |
| external | ||
| returns (address _superchainERC20) | ||
| { | ||
| // Encode the `initialize` call data for the OptimismSuperchainERC20. |
There was a problem hiding this comment.
Is it common for them to have these inline comments? I perhaps would remove them otherwise
There was a problem hiding this comment.
hmmm not very common in practice, we can delete them
| bytes32 salt = keccak256(abi.encode(_remoteToken, _name, _symbol, _decimals)); | ||
| address deployment = _calculateTokenAddress(salt, address(superchainERC20Factory)); | ||
|
|
||
| vm.expectEmit(address(superchainERC20Factory)); |
There was a problem hiding this comment.
should this have a true, true statement for the indexed arguments?
There was a problem hiding this comment.
i started changing them all after this comment ethereum-optimism#11479 (comment)
|
External review at ethereum-optimism#11617 |
---------- Co-authored-by: 0xng <[email protected]> Co-authored-by: 0xParticle <[email protected]> Co-authored-by: gotzenx <[email protected]>
|
This branch will be closed since its not needed. Dev is the base branch that we sync since this repo is the fork. We already have the PR for this branch pointing to OPs repo. Once merged, our |
Closes OPT-197
TODO: Set the correct address of the OptimismSuperchainERC20 implementation.