fix(evm): consume gas in CallContractWithInput #2180
Conversation
|
Warning Rate limit exceeded@k-yang has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 26 minutes and 54 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis pull request introduces changes to gas consumption across the Nibiru EVM codebase. The modifications primarily focus on consistently applying gas consumption at various points in the EVM execution flow, such as during contract calls, Ethereum transactions, and token operations. The changes ensure that gas is properly tracked and consumed across different methods in the keeper and precompile packages, with updates to error handling and code clarity. Changes
Sequence DiagramsequenceDiagram
participant Client
participant EVMKeeper
participant GasMeter
participant ContractExecution
Client->>EVMKeeper: Call Contract/Execute Tx
EVMKeeper->>ContractExecution: Process Transaction
ContractExecution-->>EVMKeeper: Return Response (evmResp)
EVMKeeper->>GasMeter: ConsumeGas(evmResp.GasUsed)
EVMKeeper-->>Client: Return Result
Possibly related PRs
Suggested labels
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
| @@ -39,7 +39,7 @@ func (k Keeper) CallContractWithInput( | |||
| ) (evmResp *evm.MsgEthereumTxResponse, err error) { | |||
| // This is a `defer` pattern to add behavior that runs in the case that the | |||
| // error is non-nil, creating a concise way to add extra information. | |||
| defer HandleOutOfGasPanic(&err, "CallContractError") | |||
| defer HandleOutOfGasPanic(&err, "CallContractError")() | |||
There was a problem hiding this comment.
missing parentheses was causing issues with catching OOG errors lol
| @@ -63,9 +63,11 @@ func (k Keeper) CallContractWithInput( | |||
| evmResp, err = k.ApplyEvmMsg( | |||
| ctx, evmMsg, evmObj, evm.NewNoOpTracer(), commit, txConfig.TxHash, true, | |||
| ) | |||
| if evmResp != nil { | |||
| ctx.GasMeter().ConsumeGas(evmResp.GasUsed, "CallContractWithInput") | |||
There was a problem hiding this comment.
We should always consume gas after ApplyEvmMsg(), before returning away from CallContractWithInput(), or else we lose the opportunity to apply the EvmResp.GasUsed variable.
| @@ -196,7 +196,7 @@ func (e erc20Calls) loadERC20String( | |||
| if err != nil { | |||
| return out, err | |||
| } | |||
| res, err := e.Keeper.CallContractWithInput( | |||
| evmResp, err := e.Keeper.CallContractWithInput( | |||
| @@ -114,7 +114,5 @@ func (k *Keeper) deployERC20ForBankCoin( | |||
| return gethcommon.Address{}, errors.Wrap(err, "failed to commit stateDB") | |||
| } | |||
|
|
|||
| ctx.GasMeter().ConsumeGas(evmResp.GasUsed, "deploy erc20 funtoken contract") | |||
There was a problem hiding this comment.
We no longer need to consume gas here since we consume it at a lower level in CallContractWithInput().
x/evm/keeper/gas_fees.go
Outdated
| // gas limit) if the `ApplyEvmMsg` call originated from a state transition | ||
| // where the chain set the gas limit and not an end-user. | ||
| func GasToRefund(availableRefundAmount, gasUsed uint64, fullRefundLeftoverGas bool) uint64 { | ||
| refundQuotient := params.RefundQuotientEIP3529 |
There was a problem hiding this comment.
Moved all the 1/5 calculations here.
x/evm/keeper/keeper.go
Outdated
| *err = vm.ErrOutOfGas | ||
| default: | ||
| panic(r) | ||
| } | ||
| } | ||
| if err != nil && format != "" { | ||
| if *err != nil && format != "" { |
There was a problem hiding this comment.
Need to check *err != nil, not err != nil because err is a pointer value.
| // "vm.Tracer". | ||
| leftoverGas = msg.Gas() - gasUsed | ||
| // The dirty states in `StateDB` is either committed or discarded after return | ||
| if commit { |
There was a problem hiding this comment.
I moved the StateDB.Commit() to after the gas calculations so we have accurate gas accounting prior to commit and return.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
x/evm/keeper/gas_fees.go (2)
78-85: Clarify mismatch between doc comments and boolean parameterThe comments describe refund scenarios for "user" vs. "internal (funtoken)" messages, while the function relies on the
fullRefundLeftoverGasboolean. Consider refining the doc comments or the parameter name to better reflect the decision logic (e.g.,isInternalMsg), ensuring readers understand how these scenarios map totrueorfalse.
86-90: Consolidate or reorganize verbose documentationLines 86–90 extensively outline EIP-3529 refunds, leftover gas, and internal calls. While helpful, consider merging some commentary to avoid redundancy, keeping the doc block concise while still conveying the key reasoning.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
x/evm/keeper/call_contract.go(2 hunks)x/evm/keeper/gas_fees.go(1 hunks)x/evm/keeper/keeper.go(1 hunks)x/evm/keeper/msg_server.go(4 hunks)x/evm/precompile/funtoken.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- x/evm/precompile/funtoken.go
- x/evm/keeper/call_contract.go
- x/evm/keeper/msg_server.go
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: integration-tests
🔇 Additional comments (2)
x/evm/keeper/gas_fees.go (2)
92-94: Confirm constraints on fullRefundLeftoverGas usageAllowing a
refundQuotient = 1effectively refunds all leftover gas. Validate that no external user code path can accidentally setfullRefundLeftoverGas = true, potentially enabling a full refund scenario that defeats gas constraints.
96-100: Validate integer division and overflow conditionsUsing
refundAmount := gasUsed / refundQuotientinherently truncates any fractional result. Ensure this behavior is intentional and optimal for your business logic. Also confirm thatavailableRefundAmountis large enough to avoid overflow or unexpected capping in edge cases.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2180 +/- ##
==========================================
+ Coverage 64.86% 65.00% +0.13%
==========================================
Files 277 277
Lines 22367 22349 -18
==========================================
+ Hits 14508 14527 +19
+ Misses 6869 6832 -37
Partials 990 990
|
Purpose / Abstract
Ensures consistent gas consumption in all areas of the EVM codebase by applying gas consumption at the
CallContractWithInputlayer.