Interop: SuperchainERC20 Standard #257
Conversation
Co-authored-by: skeletor <[email protected]> Co-authored-by: ng <[email protected]>
|
This doesn't yet mention the rollback functionality per #71 (comment) but that hasn't been implemented yet in the |
specs/interop/token-bridging.md
Outdated
| A possible implementation for these functionalities would look like this: | ||
|
|
||
| ```solidity | ||
| function remoteTransferFrom( |
There was a problem hiding this comment.
After discussing with @smartcontracts, we think its a good idea to remove the remote transfer from logic from the first iteration of the superchain erc20 standard to simplify things as an MVP. It will reduce the amount of test coverage we need and in the short term a permit2 like contract could be used to enable cross chain approvals. We can revisit this part of the interface again in the future if people are really asking for it
There was a problem hiding this comment.
Ok, this makes sense. The tradeoff with permit here is devex. We can upgrade later with more time to test, if we think its worth.
Do you think it makes sense to introduce a predeploy that implements permit2 and/or message relaying for these tokens? This would be a middle ground between having it in the standard and leaving all the work to the interacting contract.
These predeploys could also be implemented afterwards
There was a problem hiding this comment.
Do you think it makes sense to introduce a predeploy that implements permit2 and/or message relaying for these tokens?
We can scope this in future work, for now I'd like to leave this out
specs/interop/token-bridging.md
Outdated
| - `sendERC20(uint256 amount, uint256 chainId)(bool)` | ||
| - `sendERC20To(address to, uint256 amount, uint256 chainId)(bool)` | ||
| - `sendERC20To(address to, uint256 amount, uint256 chainId, bytes data)(bool)` | ||
| - `finalizeSendERC20(address to, uint256 amount)` |
There was a problem hiding this comment.
What is the rationale for the return values? Three return a bool but the last one doesn't. Should all they return nothing and just revert on failure?
Also, do we need different function names and arg ordering? What do you think about:
sendERC20(uint256 amount, uint256 chainId)
sendERC20(uint256 amount, uint256 chainId, address to)
sendERC20(uint256 amount, uint256 chainId, address to, bytes data)There was a problem hiding this comment.
yes, it should all return bool as per the ERC20 standard or not. I was waiting for Mark's answer to change all together. Do you have any take here? Mark suggested to return nothing in a different comment.
like your suggestion to have a single function name! Will change it :)
There was a problem hiding this comment.
Yea I agree with @tynes—the sendERC20 methods don't need to match ERC20, and "the call was successful unless it reverts" is a simpler and common pattern
specs/interop/token-bridging.md
Outdated
| - `sendERC20To(address to, uint256 amount, uint256 chainId, bytes data)(bool)` | ||
| - `finalizeSendERC20(address to, uint256 amount)` | ||
|
|
||
| `sendERC20(uint256 amount, uint256 chainId)` and `sendERC20To(address to, uint256 amount, uint256 chainId)` will burn `amount` tokens and initialize a message to the `L2ToL2CrossChainMessenger` to mint the `amount` in the target address at `chainId`. `sendERC20To(address to, uint256 amount, uint256 chainId, bytes data)` will do the same, but also include a message. |
There was a problem hiding this comment.
What happens with data? e.g. what is the use case there, given that there is no corresponding target address for the data
There was a problem hiding this comment.
This is illustrated below in the implementation, but should also be mentioned here. The target is the to, and this can be used to send funds and execute something in target. It is possible to add another target address different from to as well.
specs/interop/token-bridging.md
Outdated
| ```solidity | ||
| contract OptimismSuperchainERC20 is ERC20, ISuperchainERC20 { | ||
| constructor(string memory _name, string memory _symbol, uint256 _decimals) ERC20(_name, _symbol, _decimals) {} | ||
| function sendERC20(uint256 _amount, uint256 _chainId) external returns (bool) { |
There was a problem hiding this comment.
I would suggest diagrams over code for design docs
There was a problem hiding this comment.
specs/interop/token-bridging.md
Outdated
|
|
||
| Besides the ERC20 invariants, the SuperchainERC20 will require the following interop specific properties: | ||
|
|
||
| - Conservation of finalized `totalSupply`: The minted `amount` in `finalizeSendERC20()` should match the `amount` that was burnt in `sendERC20To()`, as long as target chain has the initiating chain in the dependency set. This implies that finalized cross-chain transactions will conserve the sum of `totalSupply` for each chain in the Superchain. |
There was a problem hiding this comment.
as long as target chain has the initiating chain in the dependency set
What happens if the initiating chain is not in the dependency set? Do the send methods need to validate that the chain ID is in the dependency set, or does the executing message just never get included? Who is expected to call the finalize method?
There was a problem hiding this comment.
Thats a very good open question, and its something we have to discuss further. We considered adding rollback functionalities for these cases, but we think its out of scope for the moment.
For that reason, I included the dependancy set clarification into the invariant. If there were a way to handle that scenario, the invariant could be extended.
specs/interop/token-bridging.md
Outdated
| Besides the ERC20 invariants, the SuperchainERC20 will require the following interop specific properties: | ||
|
|
||
| - Conservation of finalized `totalSupply`: The minted `amount` in `finalizeSendERC20()` should match the `amount` that was burnt in `sendERC20To()`, as long as target chain has the initiating chain in the dependency set. This implies that finalized cross-chain transactions will conserve the sum of `totalSupply` for each chain in the Superchain. | ||
| - Corollary 1: each in flight messages will decrease the `totalSupply` exactly by the burnt `amount`. |
There was a problem hiding this comment.
Does "in flight" mean "initiated message is safe but executing message has not happened yet"? Is it possible the executing message never occurs?
I would redefine this invariant so there is no corollary to simplify it, e.g. the "total supply is conserved across all safe blocks" (or whatever the actual invariant is)
There was a problem hiding this comment.
yes, in flight means exactly that, I can clarify. I'm not sure the two sentences are equivalent. One is saying that the totalSupply is conserved over safe blocks, the other that in between the state changed in a specific way.
Both are corolaries of the first sentence of the invariant now that I think about it more.
There was a problem hiding this comment.
As we talked about during the design my example suggestion of "total supply is conserved across all safe blocks" (or whatever the actual invariant is) is not practical, since there is no guarantee an executing message is ever sent.
From my new understanding, some invariants we have are:
- For a fixed supply token, the sum of totalSupply across chains should never increase. (It stays the same when every initiating message has its executing message, but decreases if some executing messages are never sent)
- This expands similarly for a token that has some way to be minted:
- In the absence of non-crosschain-mints (i.e. ones that are expected to increase total supply) we get the fixed supply token invariant
- When there are non-crosschain-mints, the sum of total supply across chains increases by the exact non-crosschain-mint amount
- This expands similarly for a token that has some way to be minted:
- The sum of all user balances across all chains must be exactly equal to the sum of the total supply across all chains.
- We preserve the regular ERC20 invariant that the sum of all user balances on a single chain must be exactly equal to total supply on that chain
There was a problem hiding this comment.
I like the fixed supply clarification.
Also, the sum of user's balance vs total supply is a very strong invariant we had missed, good catch.
Adding on top of that one, the sum of each user's balance across chains will behave like the totalSupply invariant, i.e. it should not increase for tokens of fix supply.
I do feel like "sum of totalSupply across chains should never increase" is not precise enough. In any situation where this is not conserved (for fixed supply tokens) then its decrease exactly the bridged amount and it's because the transaction has not been completed.
There was a problem hiding this comment.
Regarding the cross-chain mint, do you feel like this is something that should be implemented inside the sendERC20/relayERC20 logic?
specs/interop/token-bridging.md
Outdated
| - `sendERC20(address to, uint256 amount, uint256 chainId)(bool)` | ||
| - `sendERC20(address to, uint256 amount, uint256 chainId, bytes data)(bool)` | ||
| - `finalizeSendERC20(address to, uint256 amount)` | ||
| - `finalizeSendERC20(address to, uint256 amount, bytes data)` |
There was a problem hiding this comment.
small nit: can we modify the finalize functions to be relayERC20 to match the interface of the messenger? just to keep naming consistent
There was a problem hiding this comment.
Also thoughts on just having one relayERC20 function that optionally makes a CALL to to if data.length > 0? Seems like a simplification that we could make to make the interface smaller. We could either keep both sendERC20 functions or only have the one with data, I lean towards keeping both so it feels like transfer but open to keeping both to keep the standard smaller
There was a problem hiding this comment.
I really like the idea of simplifying both relayERC20 and sendERC20 to a single function.
- I don't feel the analogy with
transferis strong enough to keep both, as it's a whole different action. - UX wise, we should trust the integrations and keep the standard as lean as possible.
- Gas wise, there will be an advantage for keeping the functions separate, but its only a very small check.
There was a problem hiding this comment.
moved into single sendERC20 and relayERC20 in the latest commit. We can keep the discussion open just in case.
specs/interop/token-bridging.md
Outdated
| - Corollary 1: Cross-chain transactions will conserve the sum of `totalSupply` for each chain in the Superchain across safe blocks. | ||
| - Corollary 2: Each initiated but not finalized message (included in initiating chain but not yet in target chain) will decrease the `totalSupply` exactly by the burnt `amount`. | ||
| - Corollary 2: `SuperchainERC20s` should not charge a token fee or increase the balance when moving cross-chain. | ||
| - Question: what should we assume for chains not in the dependency set? a rollback method could modify this invariant to also consider that case. |
There was a problem hiding this comment.
We could note above that an assumption is that the remote chain has the local chain in its dependency set, and if this is not held then the funds will be locked similarly to sending funds to the wrong address
There was a problem hiding this comment.
Yes, will add this note, with the small caveat that, if at some point the target chain includes the initiating in the dependency set, funds could be recovered.
specs/interop/token-bridging.md
Outdated
| - [To discuss] Locally initiated: The bridging action should be initialized from the chain where funds are located only. | ||
| - This is because same address might correspond to different users cross-chain. For example, two SAFEs with the same address in two chains might have different owners. It it possible to distiguish an EOA from a contract by checking the size of the code in the caller's address, but this will probably change with [EIP-7702](https://github.com/ethereum/EIPs/blob/master/EIPS/eip-7702.md). | ||
| - A way to allow for remotely initiated bridging is to include remote approval, i.e. approve a certain address in a certain chainId to spend local funds. | ||
| - [To discuss] Bridge Event: `sendERC20()` should emit a `Bridged` event. |
There was a problem hiding this comment.
We could make a SendERC20 event be emitted, we would need to define it as part of the spec, same for RelayERC20, so these should be added to a list near where the functions are defined above with a small explanation of when they should be emitted
There was a problem hiding this comment.
absolutely, will add in the next commit
Description
The
SuperchainERC20standard allows to interact with ERC20s across the Superchain.This is a follow-up PR to #71.
We would love to
We will include invariants and create a design docs to discuss Factories for
SuperchainERC20s soon.