-
Notifications
You must be signed in to change notification settings - Fork 27
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: convert-unified #46
design-doc: convert-unified #46
Conversation
Co-authored-by: Particle <[email protected]> Co-authored-by: ng <[email protected]>
A possible implementation would look like this: | ||
|
||
```solidity | ||
|
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 am leaning towards the idea of adding a storage key/value pair to the factory for deployed addresses. We could:
- Update the factory to set storage after doing a deployment
- Do manual state surgery to fill in the storage slots on OPM + Base using an atomic upgrade to
StorageSetter
that sets values based on the offchain token list.
This would be a one time action that moves the token list on chain, we would not need to manually update the tokenlist over time. This would work well for chains like Base where they didn't use a canonical factory for some time. Then new chains don't need to worry about this storage setting and we can not need to add extra contracts to the mix.
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.
Similar to the other PR, I am concerned about the token address-mapping. The general super-erc20 part LGTM.
function isValidPair(address _legacyAddr, address _superAddr) internal view returns (bool) { | ||
IOptimismMintableERC20 legacyToken = IOptimismMintableERC20(_legacyAddr); | ||
address l1address = legacyToken.l1Token(); | ||
if (!registry.legacyDeployments(l1address)) { | ||
return false; | ||
} | ||
return superFactory.deployment(l1address) == _superAddr; | ||
} |
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.
This solidity does not check the condition:
Check
OptimismMintableERC20
is valid
Any L2 contract could implement a l1Token
function that can claim whatever here, right? To be unified, and merge its own supply with that of the real L2 token?
The _legacyAddr
needs to be checked to be registered by the contract, or to be generated from a valid OptimismMintableERC20
salt, to verify the deployment. Maybe ideally with a code-hash sanity check.
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 will want to figure out these details as part of the spec but if we go with the 1 time backport approach then we can get around needing to compute addresses on chain
We have consensus on this approach, its good to begin work on specs and break the project into 2 workstreams, one for the work itself and one for backporting the registry to the chain |
Description
This document presents a solution for migrating
OptimismMintableERC20
, which corresponds to locked liquidity in L1, to theSuperchainERC20
standard to be interop compatible.To do that, we suggest using the
L2StandardBridge
mint/burn rights over the legacy representations to allow easy conversion between theOptimismMintableERC20
and the correspondingSuperchainERC20
.In contrast to the isolated approach presented here #45, this PR focuses on the unified approach, where multiple legacy tokens can be converted into one single
SuperchainERC20
.