-
Notifications
You must be signed in to change notification settings - Fork 48
feat: adding limited space mirror factory #37
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,90 @@ | ||
| # Summary | ||
|
|
||
| *The work present below is part of the [Interoperability project](https://github.com/ethereum-optimism/optimism/issues/10899).* | ||
|
|
||
| A factory to spin up `ISuperchainERC20` Mirrors of `OptimismMintableERC20Tokens` is required. This document aims to provide a design for such a factory. The Mirrors need to be proxied to address any future addition of features. The factory will deploy these proxies. Therefore, from now on, when we say `Mirror` we refer to the Proxy and not the implementation. | ||
|
|
||
| # Problem Statement + Context | ||
|
|
||
| A vast amount of `OptimismMintableERC20Tokens` currently exist in the ecosystem. These tokens do not have interoperable properties. Therefore, they require a migration to become interoperable. The migration will be performed through Mirrors, as specified by [Mirror Standard](https://github.com/ethereum-optimism/design-docs/pull/36). These Mirrors must be easily deployed through a predefined factory to ensure all Mirrors share the same functionality and address across chains and also to verify their legitimacy. | ||
|
|
||
| The factory must facilitate the creation of a single legitimate Mirror per `OptimistismMintableERC20Token` to avoid liquidity fragmentation and unnecessary complexity. | ||
|
|
||
| # Alternatives Considered | ||
| - Using `CREATE`. Discarded due to nonce dependability. | ||
| - Using `CREATE3`. Viable, yet not preferred due to increased complexity. | ||
|
|
||
| # Proposed Solution | ||
|
|
||
| Limiting the scope to `OptimismMintableERC20Tokens` allows using `CREATE2` instead of `CREATE3`, which is more expensive and complex. This is because the variables CREATE2 uses to define the address of the deployed contract provides all the properties we need to maintain the same address accross chains and easily verify whether a mirror was deployed by the official factory or not: | ||
|
|
||
| - `Salt`: If we use the `token` address, we can link the `Mirror` to this `token`. This will allow other contracts to know whether a caller is a legitimate `Mirror` or not. | ||
| - `Sender Address`: In this case, it will be the Factory address itself, or the address of the factory's proxy. This works well, as it the factory can be deployed at the same address accross different chains. For chains in the superchain, it will be a predeploy. This predeploy, however, won't have a pretty address. Instead, it has to be deployed at the address where the L1 Factory is deployed so the address is maintained for all chains. | ||
| - `Init Code`: The same proxy's init code must be used accross chains. | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If we change the solidity version, new chains deployed after that version change will have proxies with different initcode, losing the "same address" benefit?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Correct, I missed this. I have pushed an alternative approach with |
||
|
|
||
| The salt doesn't need to be more than the `OptimismMintableERC20Token` address. The `name` and `symbol`, and `decimals` can be fetched with the `token` address alone in the implementation. The factory will be the `Mirror`'s admin, and the proxy will have to either call `setStorage` immediately with the `token` at the time of deployment, or add a `TOKEN_SLOT` and update it on the constructor. | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I'm not following what this sentence means, I don't see |
||
|
|
||
| The factory should be upgradable or have setters to allow the modification of the implementations for the current `Mirrors`. The deployment and update of already-deployed `Mirrors` to new implementations should be open to anyone. | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This sounds like implementations can be changed permissionlessly so I think I may be misunderstanding something here |
||
|
|
||
| We can track versioning through numbers or the addresses of the implementations used. | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For contract versioning, wouldn't we just use the standard semver versioning scheme we use for other contracts? https://github.com/ethereum-optimism/optimism/blob/develop/packages/contracts-bedrock/VERSIONING.md |
||
|
|
||
| This design allows the `StandardBridge` or any contract that needs to check the legitimacy of a `Mirror` to call `MIRROR_FACTORY.getMirrorAddress()` and validate whether the calling user is a mirror that corresponds to a given `token`. As an illustrative example of a potential function of the `StandardBridge`: | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should be clear that this is only for the |
||
|
|
||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you describe exactly how this is secure? In terms of invariants. Also hypothetically if we modified the bytecode used for the create2 deployment, it should still be secure in theory but allow for fragmentation?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I can add invariants, but you raise a great point. If modifying the init code used for create2 is a possibility, many issues appear:
It seems to me that I need to rewrite this with the |
||
| ```solidity | ||
| function mirrorMint(address _token, uint256 _amount) external { | ||
| address _mirror = MIRROR_FACTORY.getMirrorAddress(_token); | ||
| if (_mirror != msg.sender) revert(); | ||
| ///...mint logic | ||
| } | ||
|
|
||
| ``` | ||
|
|
||
| Example code: | ||
|
|
||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. small nit: using tabs here makes it a little harder to read |
||
| ```solidity | ||
| contract MirrorFactory { | ||
| string public constant MIRROR_VERSION = "1.0.0"; | ||
| Mirror public constant MIRROR_IMPL = <PLACEHOLDER_ADDRESS>; | ||
|
|
||
| function deployMirror(address _token) external returns (address) { | ||
| require(_isOptimismMintableERC20(_token), "not OptimismMintableERC20Token"); | ||
|
|
||
| // Note: There's no _token slot in this proxy | ||
| Proxy _mirror = address(new L2ChugSplashProxy{salt: keccak256(abi.encode(_token))}(address(this), _token)); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Perhaps we could just use |
||
|
|
||
| // Note: This function does not exist on L2ChugSplashProxy. | ||
| _mirror.setImplementation(MIRROR_IMPL); | ||
| } | ||
|
|
||
| function upgradeMirror(address _mirror) external { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should remove this function and think about a 1967 beacon based design for simplicity. We don't yet have a predeploy system for 1967 proxies, but thinking of making a predeploy that looks like this: contract Mirror1967Beacon {
function implementation() {
return address(0x..);
}
}The above contract would be a predeploy and it would hardcode in an address that it returns code of. As part of a hardfork, we deploy smart contracts. We would deploy the implementation and we would be able to know the address ahead of time, see here for previous work on how we can know the address ahead of time. We would be able to install the bytecode into the genesis for new chains
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What this would allow us to do is to hardfork to update the
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Perfect, this makes a lot of sense. Thanks for the suggestion, will update this.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Another thought, we could do something like this: contract MirrorFactory {
address implementation public;
constructor() {
implementation = new MirrorSuperchainERC20();
}
function deploy() {
proxy = new Proxy({ beacon: address(this) });
}
}Then upgrading the implementation would be updating the factory
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agreed with @tynes, my read of this design doc makes it seem like we want to use a beacon pattern here |
||
| // Note: This function does not exist on L2ChugSplashProxy. | ||
| _mirror.setImplementation(MIRROR_IMPL); | ||
| } | ||
|
|
||
| function _isOptimismMintableERC20(address _token) internal view returns (bool) { | ||
| return ERC165Checker.supportsInterface(_token, type(ILegacyMintableERC20).interfaceId) | ||
| || ERC165Checker.supportsInterface(_token, type(IOptimismMintableERC20).interfaceId); | ||
| } | ||
|
|
||
| function getMirrorAddress(address _token) external view returns (address _mirror) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we use a mapping that is populated rather than computing the address on chain? Otherwise we will need to forever pin the specific version of solc |
||
| _mirror = address(uint160(uint(keccak256(abi.encodePacked( | ||
| bytes1(0xff), | ||
| address(this), | ||
| keccak256(abi.encode(_token)), | ||
| keccak256(abi.encodePacked( | ||
| type(L2ChugSplashProxy).creationCode, | ||
| abi.encode(_token) | ||
| )) | ||
| ))))) | ||
| } | ||
| } | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Similar to #36 (comment), we should focus less on specific code here and more on high level design and flows |
||
|
|
||
| ``` | ||
|
|
||
| # Risks & Uncertainties | ||
|
|
||
| This only accounts for `OptimismMintableERC20Tokens`. The design space is limited for simplicity. Legacy tokens are not considered. New interoperable tokens using the `Superc20` standard without Mirrors, are not taken into account for this factory. | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is a user story I'd like to see fleshed out more, as the high level paths are unclear to me:
|
||
|
|
||
| If the design space were to be extended `Create3` and extra predeploys would be needed. | ||
|
|
||
| Current `OptimismMintableERC20Tokens` deployed by OP and Ethereum mainnet factories have different versions and, therefore, different bytecodes. This should not be a problem as long as these tokens grant `burn` and `mint` permissions to the `StandardBridge` and conform to the `ERC20` interface. | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would really appreciate an overall architecture/glossary section in one of these design docs, there are a lot of new terms and I want to make sure the overall design is unambiguous:
(Something like that, I don't know if that's exactly correct)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perfect, agreed. Will add it.