-
Notifications
You must be signed in to change notification settings - Fork 14
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
Arbitrum gas limit multiplier #4781
Arbitrum gas limit multiplier #4781
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4781 +/- ##
======================================
- Coverage 72% 72% -0%
======================================
Files 424 426 +2
Lines 72319 72239 -80
Branches 72319 72239 -80
======================================
- Hits 52261 52045 -216
- Misses 17519 17647 +128
- Partials 2539 2547 +8 ☔ View full report in Codecov by Sentry. |
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.
LGTM - will wait for @kylezs and/or @AlastairHolmes to give the all-clear on the witnessing side.
engine/src/evm/rpc/node_interface.rs
Outdated
// This is a kind of precompile on Arbitrum (although not deployed on chain) | ||
const NODE_INTERFACE_ADDRESS: &str = "0x00000000000000000000000000000000000000C8"; |
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.
What do you mean? Is there some kind of reference we can provide a link to here, or somewhere we can check this?
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.
https://docs.arbitrum.io/build-decentralized-apps/nodeinterface/reference
It's an implementation at a node level as if it were a contract deployed at that address but it's not really deployed. I'll improve that comment with the link.
* origin/main: Arbitrum gas limit multiplier (#4781) fix: don't set code red on "agg-key set by gov-key" (#4813) fix: sign tx with correct key during rotation (#4794) chore: arbitrum witnesser permission (#4798) Solana (#4414) feat: try-runtime build step on dev ci (#4807) feat: optimistic build, streamlined ci-main (#4806) chore: log raw dispatch error (#4809) feat: allow for single binary CFE upgrades (#4634) fix: take fee on to usdc (#4801) (#4804) refactor: minor cleanup of retrier code and vault pallet (#4803) feat: cf_boost_pool_details rpc (#4780) # Conflicts: # Cargo.lock # state-chain/chains/src/sol.rs # state-chain/chains/src/sol/program_instructions.rs # state-chain/chains/src/sol/sol_tx_building_blocks.rs # state-chain/chains/src/sol/token_instructions.rs # state-chain/runtime/src/safe_mode.rs
Checklist
Please conduct a thorough self-review before opening the PR.
Summary
Gas required for transactions in Arbitrum changes over time depending on several factors, mainly Ethereum calldata price. It's not that Arbitrum gas price for a transaction changes over time, which it also does, but the amount of gas needed per transaction changes over time.
Therefore, I am implementing a
gas_limit_multiplier
that is used to track the gas limit variability. For now this is only needed for ingress/egress fee calculations.For context, Arbitrum have NodeInterface call that returns parameters related to fees. That includes, among others, a gas estimation that I'm using to calculate the multiplier, and the current l2 base fee. Since we are anyway not doing anything fancy with
fee_history
we can instead replace it for the value returned by the NodeInterface. I have checked that it behaves correctly.