Skip to content
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

interop: interoperable ether transfers design doc #146

Merged
merged 15 commits into from
Nov 26, 2024

Conversation

tremarkley
Copy link
Contributor

@tremarkley tremarkley commented Oct 29, 2024

Description

This PR introduces a design for simplifying ETH transfers between two interoperable L2 chains by reducing the process from four transactions to two. The new approach leverages the SuperchainWETH contract, which now includes two new functions: sendETH and relayETH.

  • sendETH: Deposits ETH in the ETHLiquidity contract and sends a message to the destination chain, encoding the relay details.
  • relayETH: Withdraws the specified amount of ETH from ETHLiquidity on the destination chain and transfers it to the recipient.

This update streamlines L2-to-L2 ETH transfers, bypassing the need for separate wrapping and unwrapping steps. Notably, custom gas token chains are excluded from this simplification to maintain compatibility and reduce risk.

Additional context

  • Pros: Simplifies developer and user experience by enabling direct ETH transfers.
  • Cons: Adds a secondary entry point for transfers, increasing complexity slightly for developers handling asset transfers.

Any feedback on optimizing sendETH and relayETH for further efficiency or insights into handling custom gas token chains in the future is welcome.

@tremarkley
Copy link
Contributor Author

tremarkley commented Oct 29, 2024

This stack of pull requests is managed by Graphite. Learn more about stacking.

@tremarkley tremarkley changed the title interop: interoperable ether transfers interop: interoperable ether transfers design doc Oct 29, 2024
@tremarkley tremarkley force-pushed the harry/interoperable-ether-dd branch from f4d3111 to e180fb3 Compare October 29, 2024 19:04

The `SendETH` function combines the first two transactions as follows:

1. Burns `ETH` within the `ETHLiquidity` contract equivalent to the `ETH` sent.
Copy link
Contributor

@hamdiallam hamdiallam Oct 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we're not burning WETH, how about we add these functions to the ETHLiquidity contract?

I think it makes intent a bit clearer and keeps WETH completely separate as just an ERC20? No need to overload the ERC20 contract

Copy link
Contributor Author

@tremarkley tremarkley Oct 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it definitely simplifies things and creates better separation of concerns to leave SuperchainWETH as-is and move this into ETHLiquidity, so I'm onboard with that

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since ETHLiquidity holds so much ether, we have to be really careful with what functionality is added to it. It should be callable in the most minimal amount of ways. I originally designed it so that there could be only a single useful caller. We need to be very careful with any modifications to it

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tynes are you onboard with the suggested solution or do you think it is too risky of a change?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

definitely see the concern on additions to ETHLiquidity. But want to note that even if this is in SuperchainWETH, a vulnerability there would also mean a vulnerability in ETHLiquidity since the relay mechanism pulls ETH from this contract

@tremarkley tremarkley force-pushed the harry/interoperable-ether-dd branch from e180fb3 to a2f7001 Compare October 29, 2024 21:16
@tremarkley tremarkley requested a review from mds1 October 29, 2024 21:16
@tremarkley tremarkley force-pushed the harry/interoperable-ether-dd branch 2 times, most recently from 49a6f25 to 086cdb8 Compare October 29, 2024 21:21

## Custom Gas Token Chains

To simplify the solution, custom gas token chains will not be supported and must follow the original four-transaction flow, which includes wrapping and unwrapping `SuperchainWETH`.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe note that this is no problem in the superchain since standard config requires ETH as the gas token.

And then in our viem extensions we can check this about the destination client prior to calling sendETH

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, that does bring up an issue with this, there is not a good way to check at the smart contract level whether the destination is a custom gas token chain or if it is in the dependency set.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is no guarantee that standard config will always require ether for gas paying and the system was designed to be friendly to that changing

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 on no guarantee. Just noting the state of the world today but still requires addressing this since the protocol is agnostic to the superchain

@tremarkley tremarkley force-pushed the harry/interoperable-ether-dd branch from 086cdb8 to ab8d26c Compare October 29, 2024 21:56
@tremarkley tremarkley force-pushed the harry/interoperable-ether-dd branch 3 times, most recently from 872ed6e to 26827f0 Compare October 29, 2024 22:51
@tremarkley tremarkley force-pushed the harry/interoperable-ether-dd branch from 26827f0 to b2c87e9 Compare October 29, 2024 23:11

# Open Questions
- **Long-term improvements**: Could this functionality eventually extend to custom gas token chains?
- **Rollbacks**: How would rollbacks be handled in this implementation?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume rollbacks wouldn't be an issue as long as relayETH can only be called following the origin chain's finality period has passed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

by rollbacks here I am talking about ETH that is sent to a remote chain, but for some reason the relayETH message on the remote chain expires or is not able to be executed and we would like to "rollback" the ETH that was sent so the user can get their funds back

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reorgs are contingent between chains so there is no finality period. The concept of rollback is only relevant if you send to a chain where the call reverts on the remote chain and you cannot replay it or it always reverts. I think people also wanted to solve the problem of sending to a non existent chain using rollbacks, but I don't think that is possible. Now that we require the call on the remote chain to succeed, I think that the rollback design is going to be more difficult. We should think about rollbacks in a different way to solve that problem

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I recommend we use the following design for rollbacks: ethereum-optimism/specs#460

I think there is confusion around what problem we are solving for, there should be no way that the cross chain token send should revert, so we don't need a rollback solution persay, we need to "prevent send to wrong chain" solution


### Advantages

The advantage of this solution is that `SuperchainTokenBridge`would handle both `ETH` transfers and `SuperchainERC20` transfers, simplifying developer integrations.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SuperchainTokenBridgewould handle both ETH transfers and SuperchainERC20 transfers

And it would handle sending messages. Sending messages/erc20's/eth from a single contract is something developer's are used to, though its not at all a deal breaker


# Summary

New functionality is introduced to the `ETHLiquidity` contract that enables it to facilitate cross-chain `ETH` transfers to a specified recipient.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't love adding new functionality to the ETHLiquidity contract as it has a ton of virtual ether and makes reasoning about it more difficult. Its previous design only allowed one possible msg.sender. I definitely prefer to put the logic in the bridge contract to keep the liquidity contract more simple

Copy link
Contributor

@skeletor-spaceman skeletor-spaceman Nov 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 let's do this.
But have this be on the SuperWETH as originally planned :)

@tynes
Copy link
Contributor

tynes commented Nov 18, 2024

Generally much more in favor of putting the logic in the SuperchainTokenBridge given that its the cross chain bridging contract. Generally seems intuitive to put it there and mirrors our current design where the StandardBridge has ERC20 + ETH functionality

See #146 (comment)

Comment on lines +150 to +186
Add an internal `_burn` function that will be called by `ETHLiquidity#SendETH` and update the external `burn` function to call `_burn`:

```solidity
/// @notice Allows an address to lock ETH liquidity into this contract.
function burn() external payable {
if (msg.sender != Predeploys.SUPERCHAIN_WETH) revert Unauthorized();
_burn(msg.sender, msg.value);
}

/// @notice Allows an address to lock ETH liquidity into this contract.
/// @param _sender Address that sent the ETH to be locked.
/// @param _amount The amount of liquidity to burn.
function _burn(address _sender, uint256 _amount) internal {
if (IL1Block(Predeploys.L1_BLOCK_ATTRIBUTES).isCustomGasToken()) revert NotCustomGasToken();
emit LiquidityBurned(_sender, _amount);
}
```

Add an internal `_mint` function that will be called by `ETHLiquidity#RelayETH` and update the external `mint` function to call `_mint`:

```solidity
/// @notice Allows an address to unlock ETH liquidity from this contract.
/// @param _amount The amount of liquidity to unlock.
function mint(uint256 _amount) external {
if (msg.sender != Predeploys.SUPERCHAIN_WETH) revert Unauthorized();
_mint(msg.sender, _amount);
}

/// @notice Allows an address to unlock ETH liquidity from this contract.
/// @param _recipient Address to send the unlocked ETH to.
/// @param _amount The amount of ETH liquidity to unlock.
function _mint(address _recipient, uint256 _amount) internal {
if (IL1Block(Predeploys.L1_BLOCK_ATTRIBUTES).isCustomGasToken()) revert NotCustomGasToken();
new SafeSend{ value: _amount }(payable(_recipient));
emit LiquidityMinted(_recipient, _amount);
}
```
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this outdated?
not sure how this fits with the current design.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is up to date. this design is under the Alternatives Considered section and is meant to illustrate a design that was considered for adding this functionality to the ETHLiquidity contract

@tynes
Copy link
Contributor

tynes commented Nov 19, 2024

We agreed out of band that this functionality should live in the SuperchainWETH contract rather than the bridge or liquidity contracts

@tynes
Copy link
Contributor

tynes commented Nov 19, 2024

One consideration that should be thought through is sending ether to a remote chain that is custom gas token, the flow should not unwrap the weth on the other side

@tremarkley
Copy link
Contributor Author

@tynes updated the design to handle sending ether to a remote chain that is custom gas token

@tremarkley tremarkley force-pushed the harry/interoperable-ether-dd branch from c93ba0a to 5178c74 Compare November 21, 2024 01:23
Copy link
Contributor

@skeletor-spaceman skeletor-spaceman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice doc! very few nit picks on proactively pushing for a better naming from the design doc onward.

protocol/interoperable-ether-transfers.md Outdated Show resolved Hide resolved
protocol/interoperable-ether-transfers.md Outdated Show resolved Hide resolved
protocol/interoperable-ether-transfers.md Outdated Show resolved Hide resolved
protocol/interoperable-ether-transfers.md Show resolved Hide resolved
Comment on lines +89 to +94
if (msg.sender != Predeploys.L2_TO_L2_CROSS_DOMAIN_MESSENGER) revert Unauthorized();

(address crossDomainMessageSender, uint256 source) =
IL2ToL2CrossDomainMessenger(Predeploys.L2_TO_L2_CROSS_DOMAIN_MESSENGER).crossDomainMessageContext();

if (crossDomainMessageSender != address(this)) revert InvalidCrossDomainSender();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NTH: it'd be cool to have this "am I being called by myself?" abstracted into a "CDM library"

protocol/interoperable-ether-transfers.md Outdated Show resolved Hide resolved
protocol/interoperable-ether-transfers.md Outdated Show resolved Hide resolved
protocol/interoperable-ether-transfers.md Show resolved Hide resolved
@tynes tynes merged commit 9a55d2b into main Nov 26, 2024
@tynes tynes deleted the harry/interoperable-ether-dd branch November 26, 2024 22:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants