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: interoperable ether #25
design-doc: interoperable ether #25
Changes from all commits
0a61f9a
391ae8c
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.
So the code currently in
contracts-bedrock/src/L2/WETH.sol
is renamed to something likeWrappedNativeToken.sol
and is still deployed at0x4200000000000000000000000000000000000006
, then a new SuperchainERC20-comatibleWETH.sol
is added and placed at a different address?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.
Yes exactly
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.
Relevant holders for OP Mainnet, looks like a mix of Aave and bridges: https://optimistic.etherscan.io/token/0x4200000000000000000000000000000000000006#balances
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.
Do you have a docs link where I can learn more? Searched briefly but didn't see it
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 cant find docs on it but you can see on the block explorer here: https://scrollscan.com/address/0x781e90f1c8fc4611c9b7497c3b47f99ef6969cbc
The balance is absurdly high
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.
Thanks, I was able to find it in the
genesis.json
of their docs here based on the balance of that account at block 0There 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.
From the user perspective, we are calling a WETH contract to move Ether (either natively, or in wrapped form between chains). Therefore I think it would be nice if we can simplify the interface to remove unify the
sendETHTo
andsendERC20To
method into a singlesendWETHTo
method. Similar forrelayETH
vs.relayERC20
. This method would be have conditionals of courseI'd also be ok with adding an overload that has no
wad
input for chains where the gas token is ETH. But having the (W)ETH holder worry about the chain's gas token with different named methods feels confusingThere 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 this could certainly simplify things, this is a good suggestion. Ideally there is a smaller 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.
This is named
sendERC20To
instead ofsendWETHTo
to avoid users confusing.fat-fingering it withsendETHTo
, right or there's any other reason?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 interface is meant to satisfy
SuperchainERC20
which is why it has this naming, per ethereum-optimism/specs#71There 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 this solution, this predeploy is the only thing that unnerves me a bit. Mostly because Scroll has it contained in a single chain, and here we will have it in many chains that interact with one another. This wouldn't be needed with the approach of allowing the
L1StandardBridge
to wrapether
intoWETH
and minting this in the L2s, 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.
There is a problem where when sending ether between 2 chains, there is no way to "mint" the ether on the remote chain. We can mint as much WETH as we want from within the protocol, but we are unable to unwrap that arbitrarily unless we stick a ton of ether into state that can only be unlocked
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.
Should this also have
if (l1Block.isCustomGasToken()) revert OnlyEther()
?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.
Not sure if we need this necessarily, if we want
weth.totalSupply()
to be more accurate we may need to call burn on ether paying chainsThere 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.
naming nit:
burn
andsource
don't sound like inverses—maybe lock/unlock or burn/mint? either way, not a blocker and can defer on this until the spec is writtenThere 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 can have the naming nit convo on the specs 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.
Can you expand on what "Enable ETH interface" means? What are the benefits/tradeoffs of this nice to have?
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 following ABI on the
StandardBridge
is disabled for custom gas token chains:bridgeETH(uint32,bytes)
bridgeETHTo(address,uint32,bytes)
receive()
You could imagine enabling these functions and having the ether deposits become WETH on L2 on custom gas token chains automatically
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 is a good question, applies to generic SuperchainERC20 tokens too
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.
By "this predeploy" do you mean SuperchainWETH or ETHLiquidity?
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.
Other risks:
type(uint248).max
into ETHLiquidity, since we might introduce a bug that lets a disproportionate amount of ETH be withdrawnThere 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.
These are great to add to the failure mode analysis