Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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: SuperchainERC20 Factory for L1 native tokens #50
design-doc: SuperchainERC20 Factory for L1 native tokens #50
Changes from 7 commits
1f40f44
d9c30a2
6f397c3
831fb3f
7db5a5d
afed89b
58f554f
33c0f9a
530c606
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
nit, can we rename the file to something like
SuperchainERC20-factory.md
? The filename sounds like the factory lives on L1, which is not the case since it's a predeployThere 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.
My only concern is that this factory is specific for tokens that are native to L1. We can still name it like that and change notation for the others (l2 native, etc)
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.
We should consider calling out to
CreateX
, as it supports CREATE3, and provides some salt-protection mechanisms that may be useful. My suggestion would be to add CreateX as another preinstall, since then we avoid having to reimplement CREATE3 logic in this factoryDisclaimer: I helped write CreateX
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 like the idea of having a CreateX preinstall. We where discussing having the CREATE3 one. Let's discuss this in the review meeting
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 is the rationale here? Why not just use the L1 address as the salt?
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.
Doing so would imply having a single
OptimismSuperchainERC20
per L1 token. As the deployment is permissionless, anyone could "frontrun" and deploy the token with a poor metadata choice. There would be no way of changing this later on.We initially thought about using just L1 address as the salt but initializing the first deployment from L1 to bring the correct metadata in, but we ended up following the structure the
OptimismMintableERC20Factory
currently use. Any metadata will be allowed, but social consensous will pick the winner.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 could be good to detail that the beacon contract has to implement a particular interface defined in erc 1976
https://eips.ethereum.org/EIPS/eip-1967
This means that the predeploy would implement this interface and then return an address of the actual implementation. The address of the actual implementation would have to be determined similarly to the upgrade transactions for hardforks.
It could be good to talk about upgradability, so we would likely want the address returned by
implementation()
to be animmutable
and then we can have a hard fork that upgrades the predeploy implementation to include a differentimmutable
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 notice that you cover some of this later in the doc :)
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.
Added links to the EIP and references to the implementation address computation.
Is there a reason to make the implementation address an
immutable
instead of aconstant
?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.
Same comment regarding incident response. I'm ok with hardfork being the default happy case path, but we should consider also supporting upgrades via deposit transaction to support quicker changes when required. The way this would work is similar to the current setup: The L2ProxyAdmin Owner is set to the aliased address of the L1ProxyAdmin owner. This way the L1ProxyAdmin owner can send a deposit transaction to upgrade predeploys.
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.
Agree, will make this more generic
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 need a refresher on the liquidity migration spec so I'm not totally following this section. Why does the L2StandardBridge need a mapping from an L2's SuperchainERC20 address to the RemoteToken address? Is an alternate design for the SuperchainERC20 contract itself expose a getter of it's L1 token address?
Would be great if we can expand this section and clarify it. In general, diagrams showing the various flows and conversions would be useful (edit: I do see a nice deployment diagram below, it doesn't include the L2StandardBridge though)
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 will add a short refresher here. The TLDR is that the
L2StandardBridge
needs a way to check a certain superchain token address is valid, and the best way to do so is asking to the factory if that address was ever deployed. That check could also be done with a mapping fromaddress
tobool
, but having theremoteToken
here is very useful for theL2StandardBridge
as well, as it needs to verify both tokens being converted correspond to the sameremoteToken
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.
We’re looking for ways to establish common standards regarding deployments. For
SuperchainERC20
backed by an L2 native token, we’re considering usingchainId
as an additional input. Should we do the same for L1 bridged tokens (e.g., EthereumchainId
as 1)?The same reasoning can be applied to parameters stored for each representation, based on having a
remoteToken
,chainId
, and bridge.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 think we should focus specifically on L1 native tokens for this factory as they are a majority of the liquidity and then return to the problem of making a more generic factory in the future
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.
Follow up context: https://www.notion.so/defi-wonderland/Extending-SuperchainERC20-to-L2-native-tokens-and-custom-e91ca08dd3eb4434834bf74f56237abf
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.
nit, let's use a permalink to to a specific OZ version so this link doesn't break in the future if things get moved around
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 we also need a section here explaining the interactions with the L2StandardBridge
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 we're deploying an OptimismSuperchainERC20 BeaconProxy, should this diagram also have an
OptimismSuperchainERC20BeaconProxy.initialize()
call? Or is there nothing that needs to be initialized?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.
correct