Skip to content

feat(l2): use new approach for fee tokens instead of native tokens #5008

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 14 commits into
base: main
Choose a base branch
from
Open

feat(l2): use new approach for fee tokens instead of native tokens #5008

wants to merge 14 commits into from

Conversation

@tomip01
Copy link
Contributor

@tomip01 tomip01 commented Oct 23, 2025 •
edited
Loading

Note

This PR needs #5024

Motivation

We want a new approach for having native tokens in the L2. We want to keep the balance of the accounts to remain being ETH but allow the fees of the transactions to be paid with an ERC20.

Description

  • Reverted commit f6ccc00.
  • Added CustomFeeTransaction as a new variant to the Transaction enum, and updated the TxType enum and its associated methods to recognize the new type.
  • Enforced that CustomFeeTransaction is not allowed in P2P transaction processing, returning an error if encountered.
  • Modified l2_hook to deduct and refund fees according to the ERC20 tokens desired as the user

@github-actions
Copy link

github-actions bot commented Oct 23, 2025 •
edited
Loading

Lines of code report

Total lines added: 0
Total lines removed: 86
Total lines changed: 86

Detailed view 
+----------------------------------+-------+------+
| File                             | Lines | Diff |
+----------------------------------+-------+------+
| ethrex/cmd/ethrex/l2/deployer.rs | 1037  | -86  |
+----------------------------------+-------+------+

tomip01
tomip01 commented Oct 23, 2025
View reviewed changes
crates/l2/contracts/src/l1/CommonBridge.sol
Comment on lines -90 to -93
/// @notice The L1 token address that is treated as the one to be bridged to the L2.
/// @dev If set to address(0), ETH is considered the native token.
/// Otherwise, this address is used for native token deposits and withdrawals.
address public NATIVE_TOKEN_L1;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

do we deprecate this var?

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 can safely this as we didn't deploy any L2 since this was added

@tomip01 tomip01 changed the title Revert "feat(l2): custom native token (#4622)" feat(l2): use new approach for fee tokens instead of native tokens Oct 23, 2025
@github-actions github-actions bot added the L2 Rollup client label Oct 23, 2025
@tomip01 tomip01 marked this pull request as ready for review October 24, 2025 19:06
@tomip01 tomip01 requested review from a team, ManuelBilbao and jrchatruc as code owners October 24, 2025 19:06
Copilot AI review requested due to automatic review settings October 24, 2025 19:06
Copilot AI reviewed Oct 24, 2025
View reviewed changes
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR reverts the native token approach for L2, returning to ETH as the native balance while preparing for a future feature to allow fee payment with ERC20 tokens. The change simplifies the bridge implementation by removing the configurable native token support and standardizing on ETH.

Key changes:

  • Removed native token configuration and related logic throughout the codebase
  • Updated bridge contracts to use ETH_TOKEN constant instead of configurable NATIVE_TOKEN_L1
  • Simplified deposit/withdrawal flows to only handle ETH transfers
  • Updated tests to remove native token conditionals

Reviewed Changes

Copilot reviewed 11 out of 11 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
fixtures/contracts/ERC20/NativeTokenTest.bin Removed test ERC20 token contract binary
docs/l2/fundamentals/native_tokens.md Removed native token documentation
docs/l2/fundamentals/contracts.md Updated constant name from NATIVE_TOKEN_L2 to ETH_TOKEN
crates/l2/tests/tests.rs Removed native token parameters and conditional logic from test functions
crates/l2/sdk/src/sdk.rs Updated default bridge address
crates/l2/docker-compose.yaml Removed native token environment variable
crates/l2/contracts/src/l2/CommonBridgeL2.sol Renamed constant and simplified withdrawal encoding
crates/l2/contracts/src/l1/interfaces/ICommonBridge.sol Removed _amount parameter from deposit function signature
crates/l2/contracts/src/l1/CommonBridge.sol Removed native token logic, simplified deposit/withdrawal to ETH-only
cmd/ethrex/l2/deployer.rs Removed native token configuration and ERC20 approval logic from deployer
.github/workflows/pr-main_l2.yaml Removed native token test workflow configuration

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

ManuelBilbao
ManuelBilbao reviewed Oct 27, 2025
View reviewed changes
cmd/ethrex/l2/deployer.rs Outdated
const APPROVE_SIGNATURE: &str = "approve(address,uint256)";

#[derive(Clone, Copy, Default)]
#[derive(Clone, Copy)]
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd remove Copy too

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done! a217eff

crates/l2/contracts/src/l1/CommonBridge.sol Outdated
Comment on lines 210 to 213
_deposit(l2Recipient);
}

function _deposit(address l2Recipient) private {
Copy link
Contributor

Choose a reason for hiding this comment

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

We can now have _deposit body directly in deposit and call deposit() on receive()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done it here a217eff

crates/l2/contracts/src/l1/CommonBridge.sol
Comment on lines -90 to -93
/// @notice The L1 token address that is treated as the one to be bridged to the L2.
/// @dev If set to address(0), ETH is considered the native token.
/// Otherwise, this address is used for native token deposits and withdrawals.
address public NATIVE_TOKEN_L1;
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 can safely this as we didn't deploy any L2 since this was added

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

Reviewers

@ManuelBilbao ManuelBilbao ManuelBilbao left review comments

Copilot code review Copilot Copilot left review comments

@jrchatruc jrchatruc Awaiting requested review from jrchatruc jrchatruc is a code owner

At least 2 approving reviews are required to merge this pull request.

Assignees

@tomip01 tomip01

Labels

L2 Rollup client

Projects

ethrex_l2
Status: No status

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@tomip01 @ManuelBilbao