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

chore: fix l1 gas for scroll and blast #307

Merged
merged 14 commits into from
Mar 13, 2024

Conversation

npty
Copy link
Member

@npty npty commented Mar 8, 2024

Description

AXE-3369

This PR fixes error when calling estimateGasFee and specify blast or scroll as a destination chain.

Additionally, it also removes goerli-based testnet chains from various places such as types, configs, and tests.

@npty npty changed the title chore/fix l1 gas for scroll and blast chore: fix l1 gas for scroll and blast Mar 8, 2024
@npty npty requested a review from canhtrinh March 8, 2024 12:47
@npty npty self-assigned this Mar 8, 2024
@canhtrinh
Copy link
Collaborator

@npty code changes look good. can you explain in the ticket though (for tracking purposes):

  • how you identified during your tests that this change needed to be made? (i.e. how did you know it wasn't working for scroll or blast?)
  • what this fix does?

@npty
Copy link
Member Author

npty commented Mar 12, 2024

@npty code changes look good. can you explain in the ticket though (for tracking purposes):

  • how you identified during your tests that this change needed to be made? (i.e. how did you know it wasn't working for scroll or blast?)
  • what this fix does?

Issue identification

Created a simple script to call estimateGasFee function to every destination L2 chains. The script looks similar to this test

What cause an issue

The root cause for Blast and Scroll are different:

  • Blast: we've used ethereum-multicall lib to query necessary data from the L1GasOracle contract to calculate L1 fee correctly. Unfortunately, the library hasn't added built-in Multicall contract for blast chain yet, but it allows us to pass custom Multicall contract address. I found that there's a opened PR to support Blast chain, so I get the Multicall contract address from there.

  • Scroll: The SDK has made incorrect assumption that every optimism-based chain deploys a L1GasOracle contract on 0x420000000000000000000000000000000000000F address. Most optimism-based chain deployed the L1GasOracle contract on 0x420000000000000000000000000000000000000F, but unfortunately, scroll deployed it on 0x5300000000000000000000000000000000000002.

Solution

  • Blast: Passed custom multicall contract address to the Multicall lib specifically for Blast. Otherwise, we passed undefined for other chains. If we have to add another hardcoded multicall contract address for a new L2 chain in the future, I might consider removing Multicall and read the contract directly instead.

  • Scroll: Since we cannot assume that L1GasOracle contract address for optimism-based chain was deterministically deployed on 0x420000000000000000000000000000000000000F, I asked @nrsirapop to also return l1_gas_oracle_address from the getFees api, in the destination_native_token field instead. Here's the related pr for that changes on getFees api.

@npty
Copy link
Member Author

npty commented Mar 13, 2024

@canhtrinh two updates to the pr:

  1. I removed ethereum-multicall since I found the blast-like issue on fraxtal chain (can’t find multicall contract address) and we’re likely to see these issues more in the future.

  2. For optimism-sepolia overhead and scalar function calls will throw an error after their ecotone version upgraded (which is already upgraded on sepolia, and i think they’ll propagate that to mainnet soon). So, I decided to use their getL1Fee function instead which is simpler because it’s already included overhead and scalar values into the calculation and we don’t have to care about upgrade, they will also handle it for us.

@npty
Copy link
Member Author

npty commented Mar 13, 2024

Test Result

Test Conditions:

  • Source chain is L1 chain
  • Destination chain is L2 chain.
  • Running tests against testnet and mainnet
========== Mainnet L1 -> L2 fee estimations ==========
ethereum -> optimism: 0.001870751054945818
avalanche -> arbitrum: 0.019656897628718523
binance -> mantle: 0.001716210624111552
fantom -> base: 9.288128850327441796
polygon -> scroll: 23.189157385607013385
moonbeam -> blast: 0.745724919839190597
celo -> fraxtal: 0.361549055957938272

========== Testnet L1 -> L2 fee estimations ==========
ethereum-sepolia -> optimism-sepolia: 0.00016066276690561
avalanche -> arbitrum-sepolia: 0.010792133799551444
binance -> mantle-sepolia: 0.000782709162415579
fantom -> base-sepolia: 0.797701951868419293
polygon -> blast-sepolia: 6.164618427947222709
moonbeam -> fraxtal: 7.79572529373434686

@npty npty merged commit 5cfe02e into main Mar 13, 2024
4 checks passed
@npty npty deleted the chore/fix-l1-gas-for-scroll-and-blast branch March 13, 2024 13:11
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.

2 participants