Support ETH withdrawal and deposit with custom gas fee token #198

boyuan-chen · 2024-06-03T22:14:20Z

📋 Add associated issues, tickets, docs URL here.

Overview

Describe what your Pull Request is about in a few sentences.

Enable ETH deposit and withdrawal when the custom gas fee token is enabled

Changes

Describe your changes and implementation choices. More details make PRs easier to review.

I added a function called l2ETHToken in the Op stack just like the gasPayingToken function. This l2ETHToken function is to record the l2 ETH ERC20 contract address. Both L1 and L2 can call smart contracts to retrieve this address.

ETH (BNB) deposit
Users call OptimismPortal.depositTransaction() or send ETH to OptimismPortal. The OptimismPortal calls the L1CrossChainMessenger to build the cross chain message and emit it in the event. The L2 will apply the message to L2StandardBridge.
ETH (BNB) withdrawal
Users call L2StandardBridge to withdraw it. L2StandardBridge directly calls L2ToL1Messenger to build the cross chain message and emit it in the event. On L1, we send the ETH if the _tx.sender is L2StandardBridge.

Testing

Describe how to test your new feature/bug fix and if possible, a step by step guide on how to demo this.

The integration tests are added

codecov-commenter · 2024-06-03T22:25:15Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 40.13%. Comparing base (46db47b) to head (90a94c5).

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@                             Coverage Diff                              @@
##           block-custom-token-from-l1standardbridge     #198      +/-   ##
============================================================================
- Coverage                                     42.72%   40.13%   -2.60%     
============================================================================
  Files                                            38       75      +37     
  Lines                                          4002     5183    +1181     
  Branches                                        621      826     +205     
============================================================================
+ Hits                                           1710     2080     +370     
- Misses                                         2185     2996     +811     
  Partials                                        107      107

Flag	Coverage Δ
cannon-go-tests	`81.92% <ø> (ø)`
chain-mon-tests	`27.14% <ø> (?)`
common-ts-tests	`26.72% <ø> (?)`
contracts-ts-tests	`12.25% <ø> (ø)`
core-utils-tests	`44.03% <ø> (?)`
sdk-tests	`34.91% <ø> (-2.69%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files	Coverage Δ
packages/sdk/tasks/deposit-bnb.ts	`7.18% <ø> (ø)`
packages/sdk/tasks/index.ts	`100.00% <ø> (ø)`

... and 36 files with indirect coverage changes

jyellick

Overall LGTM, I like that the diff is much smaller vs the first attempt. Still a few comments if you could reply.

packages/contracts-bedrock/test/L2/L2StandardBridge.t.sol

jyellick · 2024-06-06T18:46:55Z

packages/contracts-bedrock/src/universal/StandardBridge.sol

+    /// @param _from       Address of the sender.
+    /// @param _target     Address of the receiver.
+    /// @param _amount     Amount of the token sent.
+    function _initiateBridgeETHERC20(


I am not a solidity developer, but it feels a bit wrong to me that this function should only ever be invoked with the L1 Eth's corresponding L2 ERC20 token address as local token, and the zero address as remote token.

Maybe it's more idiomatic to leave it as written, but if we know the required call parameters, why even include them as parameters to this function?

Minimally, could we note in the doc what the required values for these parameters are, or perhaps check them explicitly?

We can hardcode the _remoteToken. For the _localToken address, it can be any ERC20 token addresses on L2.

Maybe this is my misunderstanding. When is this function intended to be called? I thought it was only invoked when the L2 gas token is not the L1 native gas token, and the token being transferred was the wrapped version of the L1 native ETH? Is that not the case?

Yes, the wrapped version of the L1 native ETH can be any addresses on L2, so we don't know the _localToken address. We can read this address from the L1Block contract.

jyellick · 2024-06-06T18:47:47Z

packages/contracts-bedrock/src/deployment/SystemDictator.sol

@@ -332,7 +333,7 @@ contract SystemDictator is OwnableUpgradeable {
        (bool success,) = address(config.implementationAddressConfig.addressDeprecatorImpl).delegatecall(
            abi.encodeWithSelector(bytes4(keccak256("deprecateAddresses()")))
        );
-        require(success, "Reverted: Error deprecating addresses");


I haven't looked at the blame, but it's unclear to me why these messages are changing? There are a few other similar instances as well.

I simplified the error message to reduce the contract size

Are you concerned about this generating merge conflicts? Was the contract size becoming problematic?

Yes, this can be the problem. Once we finish the migration, this contract can be removed.

bedrock-devnet/hardhat/__init__.py

packages/contracts-bedrock/src/L1/L1CrossDomainMessenger.sol

packages/contracts-bedrock/src/L1/OptimismPortal.sol

souhaillM · 2024-06-07T09:28:32Z

packages/contracts-bedrock/src/L1/OptimismPortal.sol

+        if (token != Constants.ETHER && msg.value != 0 && l2ETHToken == address(0)) revert NoValue();
+        if (token != Constants.ETHER && msg.value != 0 && l2ETHToken != address(0)) {
+            require(


We can combine the initial check for token != Constants.ETHER and msg.value != 0 into a single condition.

Not sure how to make changes. Could you please clarify this?

souradeep-das

LGTM!

packages/contracts-bedrock/src/universal/CrossDomainMessenger.sol

souradeep-das · 2024-06-07T11:00:50Z

packages/contracts-bedrock/src/L1/OptimismPortal.sol

@@ -385,7 +394,7 @@ contract OptimismPortal is Initializable, ResourceMetering, ISemver {

            // Only transfer value when a non zero value is specified. This saves gas in the case of
            // using the standard bridge or arbitrary message passing.
-            if (_tx.value != 0) {
+            if (_tx.value != 0 && l2Sender != Predeploys.L2_STANDARD_BRIDGE) {


so ETHERC20 and other ERC20 withdrawals have l2Sender set as L2_STANDARD_BRIDGE
and Boba withdrawals have l2Sender != L2_STANDARD_BRIDGE (directly triggered on L2toL1MessagePasser and thats the only way)?

Yes. There are three cases:

Withdrawal BOBA
Users can only trigger this action in L2ToL1Messenger. The l2Sender is the msg.sender (user wallet address)

Withdrawal L1 native token
Users can only trigger this action in L2StandardBridge. The L2StandardBridge calls L2ToL1Messenger and the l2Sender is L2StandardBridge

Withdrawal other ERC20 tokens
Users can only trigger this action in L2StandardBridge. The L2StandardBridge calls L2CrossDomainMessenger and L2CrossDomainMessenger calls L2ToL1Messenger. Te l2Sender is L2CrossDomainMessenger.

thats great! If we could record these somewhere or on internal specs, would be great for us and future audit.

Yes, I will write up a documentation.

souradeep-das · 2024-06-07T13:08:27Z

packages/contracts-bedrock/scripts/Deploy.s.sol

@@ -1016,7 +1016,8 @@ contract Deploy is Deployer {
                        disputeGameFactory: mustGetAddress("DisputeGameFactoryProxy"),
                        optimismPortal: mustGetAddress("OptimismPortalProxy"),
                        optimismMintableERC20Factory: mustGetAddress("OptimismMintableERC20FactoryProxy"),
-                        gasPayingToken: customGasTokenAddress
+                        gasPayingToken: customGasTokenAddress,
+                        l2ETHToken: address(0)


I see we are going with l2ETHToken = addr(0) for the tests. So the two possible values of l2ETHToken are either addr(0) for Boba-ETH and L2BNB for Boba-BNB right?

Another unit tests are added. For the local deployment we assume that this l2ETHToken is disabled.

I modified the deployment script, so devs can configure it.

Support ETH withdrawal and deposit with custom gas fee token

c0f768f

boyuan-chen added 2 commits June 3, 2024 15:56

Fix some tests

62d0db4

Update bindings

17e3de4

boyuan-chen requested review from souradeep-das, jyellick and souhaillM June 3, 2024 23:17

boyuan-chen added 3 commits June 3, 2024 17:03

Fix tests

32a6410

Disable unit tests

e839ba0

Lint contracts

7e9531c

jyellick reviewed Jun 6, 2024

View reviewed changes

boyuan-chen added 3 commits June 6, 2024 14:37

Add unit tests

0b55334

Lint contract

544e98b

Update gas snapshot

217ddc8

souhaillM requested changes Jun 7, 2024

View reviewed changes

souradeep-das approved these changes Jun 7, 2024

View reviewed changes

souradeep-das reviewed Jun 7, 2024

View reviewed changes

boyuan-chen added 6 commits June 7, 2024 09:11

Use calldata in baseGas

1877ce6

Avoid nested conditions

104507c

Fix tests

ee49d95

Add l2ETHTokenAddress to deployment script

8d660f8

Add more tests

36eab4d

Fix lint

90a94c5

souhaillM approved these changes Jun 7, 2024

View reviewed changes

boyuan-chen merged commit c09aef0 into block-custom-token-from-l1standardbridge Jun 7, 2024
72 checks passed

boyuan-chen deleted the support-eth-withdrawal branch June 7, 2024 21:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support ETH withdrawal and deposit with custom gas fee token #198

Support ETH withdrawal and deposit with custom gas fee token #198

boyuan-chen commented Jun 3, 2024 •

edited

Loading

codecov-commenter commented Jun 3, 2024 •

edited

Loading

jyellick left a comment

jyellick Jun 6, 2024 •

edited

Loading

boyuan-chen Jun 6, 2024 •

edited

Loading

jyellick Jun 6, 2024

boyuan-chen Jun 6, 2024

jyellick Jun 6, 2024

boyuan-chen Jun 6, 2024

jyellick Jun 6, 2024

boyuan-chen Jun 6, 2024

souhaillM Jun 7, 2024

boyuan-chen Jun 7, 2024

souradeep-das left a comment

souradeep-das Jun 7, 2024

boyuan-chen Jun 7, 2024

souradeep-das Jun 7, 2024

boyuan-chen Jun 7, 2024

souradeep-das Jun 7, 2024

boyuan-chen Jun 7, 2024

boyuan-chen Jun 7, 2024

Support ETH withdrawal and deposit with custom gas fee token #198

Support ETH withdrawal and deposit with custom gas fee token #198

Conversation

boyuan-chen commented Jun 3, 2024 • edited Loading

Overview

Changes

Testing

codecov-commenter commented Jun 3, 2024 • edited Loading

Codecov Report

jyellick left a comment

Choose a reason for hiding this comment

jyellick Jun 6, 2024 • edited Loading

Choose a reason for hiding this comment

boyuan-chen Jun 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

souradeep-das left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

boyuan-chen commented Jun 3, 2024 •

edited

Loading

codecov-commenter commented Jun 3, 2024 •

edited

Loading

jyellick Jun 6, 2024 •

edited

Loading

boyuan-chen Jun 6, 2024 •

edited

Loading