fix: arbitrum network cost estimate #464

gzeoneth · 2024-01-03T14:57:22Z

What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
This PR fixes 2 bugs that causes inflated network cost (gas fee) estimate for Arbitrum.
What is the current behavior? (You can also link to an open issue here)
Smart Order Router is inflating the Arbitrum fee due to double counting L1 fee and a incorrect unit conversion
What is the new behavior (if this is a feature change)?
1. Removed double counting of L1 fee in calculateGasUsed logic
2. Converted per byte fee into per gas fee by dividing 16
Other information:
https://docs.arbitrum.io/devs-how-tos/how-to-estimate-gas
https://docs.arbitrum.io/arbos/gas
https://medium.com/offchainlabs/understanding-arbitrum-2-dimensional-fees-fd1d582596c9

jsy1218 · 2024-01-04T00:24:14Z

src/providers/v3/gas-data-provider.ts

    return {
      perL2TxFee: gasData[0],
-      perL1CalldataFee: gasData[1],
+      perL1CalldataFee: perL1CalldataByte.div(16),


This is great catch! I took a look at OVM_GasPriceOracle, where we borrow the code to calculate L1 gas. Indeed the result has the factor of 16 multiplied.

You might notice that for Arbitrum gas estimate, smart-order-router should use the arbitrum L1 gas estimate, instead of optimism one. As a follow-up, Uniswap internally probably will need to use the gasEstimateL1Component, assuming this is pretty accurate estimate of the calldata costs that Arbitrum Sequencer posts to L1?

that’s correct for a more accurate estimate but would require an extra rpc call, also note that it return in terms of L2 gas which should be multiplied with L2 gas price (instead of L1)

I thought gasEstimateL1Component returns both L1 gas estimate, as well as L2 base fee?

gasEstimateL1Component returns 3 things

gasEstimateForL1: an estimate of the amount of gas needed for the l1 component of this tx

l2BaseFee: the l2 base fee

l1BaseFeeEstimate: ArbOS's l1 estimate of the l1 base fee

gasEstimateForL1 is how much L2 gas we charge for the L1 calldata cost
so if you want to calculate feeForL1, that would be gasEstimateForL1 * l2BaseFee

jsy1218 · 2024-01-04T06:00:49Z

src/util/gas-factory-helpers.ts

-  if ([ChainId.ARBITRUM_ONE, ChainId.ARBITRUM_GOERLI].includes(chainId)) {
-    l2toL1FeeInWei = calculateArbitrumToL1FeeFromCalldata(
-      route.methodParameters!.calldata,
-      l2GasData as ArbitrumGasData
-    )[1];
-  } else if (


I actually think there's no double counting of L1 fees here, although the reason is a bit complex.

You probably noticed two codepaths that invokes calculateGasUsed:

EthEstimateGasSimulator.ethEstimateGas

TenderlySimulator.simulateTransaction

Those two codepaths are actually mutually exclusive to each other, due to the check in FallbackTenderlySimulator.simulateTransaction. Then inside EthEstimateGasSimulator.ethEstimateGas, from Arbitrum doc, I see the node returns the entire transaction fee at the specified L2 gas price. I assume it does not include the L1 calldata posting costs. For Tenderly simulation, I'm a bit less certain if they include the L1 fee as well.

Taking a step back, the change inside calculateGasUsed will likely have no effect, if your experience is such that you notice the abnormally high gas cost using the Uniswap web interface, not mobile, because only mobile requests a best swap quote with simulation, not web interface. The reason is a bit complex.

calculateGasUsed is only going to be invoked when upstream requests smart order router to get a best swap route with forked net simulation. The code is inside AlphaRouter. Majority of the Arbitrum abnormal high gas cost report I see is the web app screenshot, so it means the Arbitrum gas cost is not recomputed inside gas-factory-helpers, but only once inside V3HeuristicGasModelFactory

I assume it does not include the L1 calldata posting costs.

This is incorrect, unlike Optimism, Arbitrum charge L2 gas for L1 calldata posting costs. For example, a normal transfer cost 21000 L2 gas to execute and say 100000 L2 gas for L1 calldata, Arbitrum require the user to sign a transaction with at least 121000 gas to be included onchain. The RPC method "eth_estimateGas" on an Arbitrum node return the TOTAL L2 gas required, that is L2 execution gas + L1 calldata gas which for the above example would be 121000. As far as I know, tenderly also have the same behavior (that the gas estimate include L1 calldata gas)

In fact I don't really see why we need calculateArbitrumToL1FeeFromCalldata anywhere except for display purpose (show user L1 and L2 fee split) because the gas estimation would have returned the total already. Unless you are hardcoding the execution gas somewhere and want to have a more accurate L1 fee added to it.

This is incorrect, unlike Optimism, Arbitrum charge L2 gas for L1 calldata posting costs.

This is good to know. This assumption is embedded in sor gas model. Let me think through how to fix this assumption.

I am running the cli quoter, estimatedGasUsed seems to be wrong but I cannot figure out where it come from. If it uses eth_estimateGas it should return a much higher number.

Ah it does not do eth_estimateGas, it was semi-hardcoded in the gas model

I think I am not familiar with the SOR enough to properly fix this quickly, but would be nice if we can ship the div(16) fix first to make the number at least much more reasonable. Thanks for the quick response btw.

Agree, there's a bit of urgency. Let's ship this tomorrow during EST/PST business hours. I can help create a replicated PR within
https://github.com/Uniswap/smart-order-router, so all the tests should pass.

Cool, feel free to cherrypick my commit

#466 replicated here

jsy1218 · 2024-01-04T06:07:50Z

This is great stuff, thanks for the PR! Anecdotally, I saw a report, where the smart-order-router computed gas cost is about 130k, meanwhile the actual gas cost should be more than 3m. This means a factor of more than 24. Meanwhile this PR should address some discrepancies, I can see there's more to address.

Most likely the fix will concentrate in V3HeuristicGasModelFactory. One thing on top of my mind is that smart order router does not compress the calldata for any L2 chains, but the L1 format calldata. That obviously will also inflate the gas cost by the compression ratio.

jsy1218 · 2024-01-04T06:08:02Z

Also would be great if you can add some unit test coverages.

gzeoneth · 2024-01-04T06:28:04Z

This is great stuff, thanks for the PR! Anecdotally, I saw a report, where the smart-order-router computed gas cost is about 130k, meanwhile the actual gas cost should be more than 3m. This means a factor of more than 24. Meanwhile this PR should address some discrepancies, I can see there's more to address.

Most likely the fix will concentrate in V3HeuristicGasModelFactory.

It can be more than 16x because Arbitrum charge L1 calldata gas after compression, the naive 16 gas a bytes formula does not account for that. Using gasEstimateL1Component would give a closer estimate.

Also would be great if you can add some unit test coverages.

Tbh I don't know how to properly test this repo since some unit tests are failing before my changes

jsy1218 · 2024-01-04T06:38:55Z

Also would be great if you can add some unit test coverages.

Tbh I don't know how to properly test this repo since some unit tests are failing before my changes

We've hardened the repo by adding some test suites in the github CI. In order for all the tests to run successfully from your forked repo, you will need to create new repository secrets, for the following secrets used in https://github.com/Uniswap/smart-order-router/blob/main/.github/workflows/tests.yml:

  JSON_RPC_PROVIDER: ${{ secrets.JSON_RPC_PROVIDER }}
  JSON_RPC_PROVIDER_GORLI: ${{ secrets.JSON_RPC_PROVIDER_GORLI }}
  JSON_RPC_PROVIDER_OPTIMISM: ${{ secrets.JSON_RPC_PROVIDER_OPTIMISM }}
  JSON_RPC_PROVIDER_OPTIMISM_GOERLI: ${{ secrets.JSON_RPC_PROVIDER_OPTIMISM_GOERLI }}
  JSON_RPC_PROVIDER_ARBITRUM_ONE: ${{ secrets.JSON_RPC_PROVIDER_ARBITRUM_ONE }}
  JSON_RPC_PROVIDER_ARBITRUM_GOERLI: ${{ secrets.JSON_RPC_PROVIDER_ARBITRUM_GOERLI }}
  JSON_RPC_PROVIDER_POLYGON: ${{ secrets.JSON_RPC_PROVIDER_POLYGON }}
  JSON_RPC_PROVIDER_POLYGON_MUMBAI: ${{ secrets.JSON_RPC_PROVIDER_POLYGON_MUMBAI }}
  JSON_RPC_PROVIDER_CELO: ${{ secrets.JSON_RPC_PROVIDER_CELO }}
  JSON_RPC_PROVIDER_CELO_ALFAJORES: ${{ secrets.JSON_RPC_PROVIDER_CELO_ALFAJORES }}
  JSON_RPC_PROVIDER_BNB: ${{ secrets.JSON_RPC_PROVIDER_BNB }}
  JSON_RPC_PROVIDER_AVALANCHE: ${{ secrets.JSON_RPC_PROVIDER_AVALANCHE }}
  JSON_RPC_PROVIDER_BASE: ${{ secrets.JSON_RPC_PROVIDER_BASE }}
  TENDERLY_BASE_URL: ${{ secrets.TENDERLY_BASE_URL }}
  TENDERLY_USER: ${{ secrets.TENDERLY_USER }}
  TENDERLY_PROJECT: ${{ secrets.TENDERLY_PROJECT }}
  TENDERLY_ACCESS_KEY: ${{ secrets.TENDERLY_ACCESS_KEY }}

However, I can see a bit urgency in fix the Arbitrum gas estimate, so can help by replicating this PR as a https://github.com/Uniswap/smart-order-router repo, since the repo contains all those secrets already.

cgkol

Thank you for the PR!

Shall we also add some unit tests before we go through with this?

cgkol · 2024-01-03T18:37:43Z

src/util/gas-factory-helpers.ts

@@ -250,12 +250,8 @@ export async function calculateGasUsed(
  const gasPriceWei = route.gasPriceWei;
  // calculate L2 to L1 security fee if relevant
  let l2toL1FeeInWei = BigNumber.from(0);
-  if ([ChainId.ARBITRUM_ONE, ChainId.ARBITRUM_GOERLI].includes(chainId)) {
-    l2toL1FeeInWei = calculateArbitrumToL1FeeFromCalldata(


can we also delete the function definition since its not used anywhere else?

This is exported so idk

Instead of deleting calculateArbitrumToL1FeeFromCalldata, I think we will want to refactor the same code inside V3HeuristicGasModelFactory.

gzeoneth · 2024-01-04T17:01:39Z

pushed some unit test

jsy1218 · 2024-01-05T17:40:26Z

This PR can be closed with the replicated #466

Thank you @gzeoneth again for the contribution!

gzeoneth added 2 commits January 3, 2024 22:48

fix: arbitrum getGasData

275fcfe

fix: arbitrum l1 fee double count

ad63c7f

gzeoneth requested a review from a team as a code owner January 3, 2024 14:57

jsy1218 reviewed Jan 4, 2024

View reviewed changes

cgkol reviewed Jan 4, 2024

View reviewed changes

gzeoneth added 2 commits January 5, 2024 00:46

test: arb gas data provider

0637705

test: arb gas factory helper

07c0e19

jsy1218 added a commit that referenced this pull request Jan 4, 2024

replicate external PR #464

3370b4f

jsy1218 mentioned this pull request Jan 4, 2024

copy fix: arbitrum network cost estimate #466

Merged

jsy1218 added a commit that referenced this pull request Jan 5, 2024

replicate external PR #464 (#466)

c147528

jsy1218 closed this Jan 5, 2024

BitAI-dev pushed a commit to BitAI-dev/smart-order-router that referenced this pull request Jan 20, 2025

replicate external PR Uniswap/smart-order-router#464 (#466)

ab2b0d8

BitAI-dev pushed a commit to Taiki-ops/smart-order-router that referenced this pull request Jan 20, 2025

replicate external PR Uniswap/smart-order-router#464 (#466)

36dbf2c

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: arbitrum network cost estimate #464

fix: arbitrum network cost estimate #464

gzeoneth commented Jan 3, 2024 •

edited

Loading

jsy1218 Jan 4, 2024 •

edited

Loading

gzeoneth Jan 4, 2024 •

edited

Loading

jsy1218 Jan 4, 2024

gzeoneth Jan 4, 2024

jsy1218 Jan 4, 2024 •

edited

Loading

gzeoneth Jan 4, 2024 •

edited

Loading

gzeoneth Jan 4, 2024 •

edited

Loading

jsy1218 Jan 4, 2024

gzeoneth Jan 4, 2024

gzeoneth Jan 4, 2024

gzeoneth Jan 4, 2024 •

edited

Loading

jsy1218 Jan 4, 2024

gzeoneth Jan 4, 2024

jsy1218 Jan 4, 2024

jsy1218 commented Jan 4, 2024 •

edited

Loading

jsy1218 commented Jan 4, 2024

gzeoneth commented Jan 4, 2024

jsy1218 commented Jan 4, 2024

cgkol left a comment •

edited

Loading

cgkol Jan 3, 2024

gzeoneth Jan 4, 2024

jsy1218 Jan 4, 2024

gzeoneth commented Jan 4, 2024

jsy1218 commented Jan 5, 2024

fix: arbitrum network cost estimate #464

fix: arbitrum network cost estimate #464

Conversation

gzeoneth commented Jan 3, 2024 • edited Loading

jsy1218 Jan 4, 2024 • edited Loading

Choose a reason for hiding this comment

gzeoneth Jan 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsy1218 Jan 4, 2024 • edited Loading

Choose a reason for hiding this comment

gzeoneth Jan 4, 2024 • edited Loading

Choose a reason for hiding this comment

gzeoneth Jan 4, 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

gzeoneth Jan 4, 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

jsy1218 commented Jan 4, 2024 • edited Loading

jsy1218 commented Jan 4, 2024

gzeoneth commented Jan 4, 2024

jsy1218 commented Jan 4, 2024

cgkol left a comment • 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

gzeoneth commented Jan 4, 2024

jsy1218 commented Jan 5, 2024

gzeoneth commented Jan 3, 2024 •

edited

Loading

jsy1218 Jan 4, 2024 •

edited

Loading

gzeoneth Jan 4, 2024 •

edited

Loading

jsy1218 Jan 4, 2024 •

edited

Loading

gzeoneth Jan 4, 2024 •

edited

Loading

gzeoneth Jan 4, 2024 •

edited

Loading

gzeoneth Jan 4, 2024 •

edited

Loading

jsy1218 commented Jan 4, 2024 •

edited

Loading

cgkol left a comment •

edited

Loading