-
Notifications
You must be signed in to change notification settings - Fork 22
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
design-doc: mirror standard #36
Conversation
protocol/superc20/mirror-standard.md
Outdated
Below, we show what the implementation of the mirror contract would look like. | ||
|
||
```solidity | ||
contract Mirror is Superc20 { |
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.
Since this references the design in ethereum-optimism/specs#71, can we assume general consensus on the design in that PR?
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.
an we assume general consensus on the design in that PR?
I was going to ask a similar question, and I think the answer is no we cannot assume general consensus, since that PR is a bit stale with a lot of open comments. It feels like that PR is a prerequisite for this one + interoperable ether (#25), so I would suggest reprioritizing the SuperchainERC20 standard so we can finalize that first
return LEGACY_TOKEN.balanceOf(account); | ||
} | ||
|
||
function transfer(address recipient, uint256 amount) public override returns (bool) { |
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.
It would be good to have the flow that shows a cross chain transfer, perhaps as a mermaid diagram. My current understanding, assuming the mirror token is already deployed on both domains:
- holding
OptimismMintableERC20Token
- call mirror
transfer
to self - call mirror superchain erc20
sendERC20
per interop: token standard specs#71 (comment), this burns the token - on remote domain, send the executing transaction that mints the token
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.
+1 I would suggest removing code from this design doc, and replace it with diagrams showing the user flows and data flows. Per the design doc template:
As a rule of thumb, including code snippets (except for defining an external API) is likely too low level.
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 just created some basic flows in Figma: https://www.figma.com/board/IXC6WnDaqrheplxbCZpw3l/%5BExternal%5D-Mirror-Standard?node-id=0-1&t=3tsA67ghaqocKaRv-1
will migrate these to Mermaid after discussion
|
||
### Deployment | ||
|
||
Deploying mirror versions will require introducing a new predeploy for`SuperchainERC20` factory. This factory would accomplish the following: |
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.
What happens to the existing OptimismMintableERC20Factory
on L1 and L2?
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.
There is no need to modify or deprecate the OptimismMintableERC20Factory
, but we can discuss if there is any considerable advantage of doing so.
protocol/superc20/mirror-standard.md
Outdated
- Designed to enable same-address deployments across OP Chains. | ||
- Address predictability for each token, we should know the corresponding Mirror. | ||
- Simple Factory code. | ||
- It may serve to create both `SuperchainERC20` standard tokens and mirror versions. |
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.
It sounds like this is uncertain, we should finalize the design here before merging
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.
The current Factory design doc only covers Mirror, but we think it can be extended. We can discuss this.
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.
So yes, agree we should cover and solve this before merging
We will want to describe how to do withdrawals back to L1 using this standard |
Closing this in favor of the convert approach |
Description
This document presents the
SuperchainERC20
Mirror Standard, which enables existing tokens deployed throughOptimismMintableERC20
Factory in L2 (also called legacy tokens) to be interoperable without any liquidity migration process.