-
Notifications
You must be signed in to change notification settings - Fork 456
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
fix: Arb gas estimate brotli compression calldata byte estimate and estimateGasUsed adding arb L2 gas cost #468
fix: Arb gas estimate brotli compression calldata byte estimate and estimateGasUsed adding arb L2 gas cost #468
Conversation
Good to see these getting fixed, let me know if you have further question. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes look good generally, but dont we need some test coverage here?
I spent some time thinking about how to add testing coverages for this PR. It's a bit tricky. We could assert that the Arbitrum Sequencer posting calldata L2 -> L1 cost as unit test, but it's hard to find both non-compressed and compressed calldata online. Instead I added the unit test to cover that the compressed bytes are used for gas estimate, as well as 16 gas units per calldata byte. The most tricky part is to cover the change in best-swap-route. There's no existing unit test coverage against this giant function, and it's too time consuming to write one at this point. But this is the crux that we are missing, which is causing inflated network cost on web app. So what I did is to write a A/B test between non-simulated gas vs simulated gas, to make sure they are not like 90% away from each other anymore. 50% away from each other is still a lot, but it's already a great improvement from before this PR. |
…tly failing and blocking the PR
New dependencies detected. Learn more about Socket for GitHub ↗︎
|
Base-network integ-test failing with simulation ones. I debugged on my local and saw it was due to Tenderly returning 500. Don't see routing-api quote endpoint success rate down for base network (we use different Tenderly project for this github action vs prod env), so I assume this is a transient issue. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice!
Small note @jsy1218. The Error: https://nextjs.org/docs/messages/module-not-found Import trace for requested module: |
What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
Bug fixes
What is the current behavior? (You can also link to an open issue here)
There are 3 bugs:
estimatedGasUsed
does not add the Arbitrum L1 calldata posting fee in terms of the L2 gas price.There are 3 fixes:
estimatedGasUsed
calculation, add back the Arbitrum L2->L1 gas cost in terms of L2 Arb gas price.copy fix: arbitrum network cost estimate #466 (comment) shows the updated
estimatedGasUsed
under two CLIs. With all 3 fixes, I'm able to replicate the same gas estimate under that historical block:Also if I run the same CLI from the main branch (with getGasData refactoring, because I'm requesting historical block number), then I can see main
estimatedGasUsed
is still way off, if I don't request simulate: