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

feat(protocol): add solver support for l2 to l1 eth bridging #18805

Open
wants to merge 180 commits into
base: main
Choose a base branch
from

Conversation

AnshuJalan
Copy link
Collaborator

This is an extension of #18616 for L2 -> L1 FnS ETH bridging.

  • As of now, Ether bridging is facilitated directly via Bridge.sol. Thus, the Bridge contract itself acts as the “ether vault”.
  • We might as well modify Bridge.sol to incorporate a solver condition, but that would fundamentally change the core of the message passing system. Therefore, a better alternative implemented in this PR is a separate EtherBridgeWrapper.sol that handles just the “Eth in” and “Eth out” part of bridging by including a solver condition, leaving the existing functionality of Bridge.sol and surrounding infra untouched.
  • Bridge.sol can still be used for standard slow withdrawals.

dantaik and others added 30 commits November 23, 2024 14:59
Co-authored-by: xiaodino <[email protected]>
Co-authored-by: Daniel Wang <[email protected]>
Co-authored-by: dantaik <[email protected]>
@dantaik dantaik requested a review from adaki2004 January 25, 2025 13:49
@AnshuJalan AnshuJalan requested a review from dantaik January 31, 2025 09:48
@AnshuJalan
Copy link
Collaborator Author

Solver logic for eth has been merged into ERC20Vault. The formerly added EtherBridgeWrapper has been deleted.

ctoken_ = _ctoken;
// Following the "transfer and burn" pattern, as used by USDC
if (_op.token == address(0)) {
balanceChangeAmount_ = _op.amount;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should init ctoken_'s chainId, and inside onMessageInvocation, we should still check if _ctoken.chainId == block.chainid is true for Ether transfer...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

May I know what purpose this may serve for Ether? In the case of ERC20, I see this is required to choose between transferring a token if it is canonical or minting it if it is bridged.

Copy link
Contributor

Choose a reason for hiding this comment

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

The difference between Ether and ECR20 tokens is the address being 0 or not. But since we have checked _ctoken.chainId for ERC20 tokens, we shall do the same for Ether. Otherwise, _ctoken.chainId == block.chainid check shall also be removed for ERC20, but I think that will not be OK.

Base automatically changed from pacaya_fork to main February 4, 2025 09:05
@AnshuJalan
Copy link
Collaborator Author

@dantaik @cyberhorsey Would you mind managing the conflicts here? I am not sure where did these turn up from

@dantaik
Copy link
Contributor

dantaik commented Feb 6, 2025

@AnshuJalan I helped you with the conflicts, but please double check if some of your changes are deleted by mistake :D

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants