fix(evm): proper tx gas refund outside of CallContract #2132
Conversation
WalkthroughThis pull request updates the Nibiru project's Changes
Sequence DiagramsequenceDiagram
participant Client
participant EVMKeeper
participant BlockContext
participant GasMeter
Client->>EVMKeeper: Submit Ethereum Transaction
EVMKeeper->>GasMeter: Reset Gas Meter
EVMKeeper->>BlockContext: Execute Transaction
BlockContext-->>EVMKeeper: Transaction Response
EVMKeeper->>GasMeter: Calculate Gas Used
EVMKeeper->>BlockContext: Add Gas to Block
EVMKeeper-->>Client: Transaction Receipt
Possibly related PRs
Suggested reviewers
Poem
Finishing Touches
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 (
|
There was a problem hiding this comment.
The code looks correct, but based on the failing test(s), this change seems to introduce an issue where a required refund is bigger than what's given as input by the msg sender.
The issue may be resolved by broadcasting the message with a full transaction rather than using gRPC keeper methods directly, as that will guarantee the ante handler runs before the new gas logic
| if errBlockGasUsed != nil { | ||
| return nil, errors.Wrap(errBlockGasUsed, "error adding transient gas used") | ||
| } | ||
| k.ResetGasMeterAndConsumeGas(ctx, blockGasUsed) |
There was a problem hiding this comment.
Shouldn't we consume evmResp.GasUsed instead of blockGasUsed here?
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (6)
eth/rpc/backend/call_tx.go (1)
342-345: Add a descriptive comment for the new public method
Providing a concise doc comment will help future maintainers understand the method's purpose at a glance.+// ClientCtx returns the Cosmos SDK client.Context used by this Backend. func (b *Backend) ClientCtx() client.Context { return b.clientCtx }x/evm/precompile/funtoken_test.go (1)
265-266: Maintain a consistent approach for resetting the gas meter.
Resetting the gas meter here ensures a fresh gas count before the subsequent test assertion. Confirm that this step aligns with your overall testing strategy and accurately reflects real-world gas usage patterns.x/evm/keeper/funtoken_from_coin.go (1)
85-85: Clarify the rationale for resetting the gas meter.Resetting the gas meter on error is a valid approach. However, ensure that the newly set limit is consistent with the code logic and does not mask potential out-of-gas issues or cause unpredictable gas usage tracking.
eth/rpc/backend/backend_suite_test.go (1)
205-212: Naming consistency for getters.
getUnibiBalanceis a concise, self-descriptive function. Confirm that naming aligns with similar helper functions to ensure consistent clarity throughout the codebase.eth/rpc/backend/gas_used_test.go (1)
5-5: Fix import ordering to satisfy project lint requirements.The pipeline indicates that file imports are not sorted with “-local github.com/NibiruChain/nibiru”. Please apply the recommended settings to pass the lint step.
🧰 Tools
🪛 GitHub Actions: Linter
[warning] 5-5: File is not
goimports-ed with -local github.com/NibiruChain/nibirux/evm/keeper/funtoken_from_coin_test.go (1)
258-259: Encapsulate or document the rationale for resetting the gas meter at this point.While calling
deps.ResetGasMeter()after converting ERC-20 back to bank coins may be useful for measuring subsequent operations independently, it’s not immediately obvious why this reset is necessary here. Briefly documenting this rationale (e.g., "resetting gas for subsequent tests") would improve clarity.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
CHANGELOG.md(1 hunks)eth/rpc/backend/backend_suite_test.go(4 hunks)eth/rpc/backend/call_tx.go(2 hunks)eth/rpc/backend/gas_used_test.go(1 hunks)x/evm/keeper/call_contract.go(0 hunks)x/evm/keeper/funtoken_from_coin.go(1 hunks)x/evm/keeper/funtoken_from_coin_test.go(1 hunks)x/evm/keeper/keeper.go(2 hunks)x/evm/keeper/msg_server.go(2 hunks)x/evm/precompile/funtoken_test.go(2 hunks)
💤 Files with no reviewable changes (1)
- x/evm/keeper/call_contract.go
🧰 Additional context used
🪛 golangci-lint (1.62.2)
eth/rpc/backend/backend_suite_test.go
[error] 195-195: func (*BackendSuite).broadcastSDKTx is unused
(unused)
eth/rpc/backend/gas_used_test.go
[error] 70-70: ineffectual assignment to nonce
(ineffassign)
🪛 GitHub Check: lint
eth/rpc/backend/backend_suite_test.go
[failure] 195-195:
func (*BackendSuite).broadcastSDKTx is unused (unused)
eth/rpc/backend/gas_used_test.go
[failure] 70-70:
ineffectual assignment to nonce (ineffassign)
🪛 GitHub Actions: Linter
eth/rpc/backend/backend_suite_test.go
[error] 195-195: func (*BackendSuite).broadcastSDKTx is unused
eth/rpc/backend/gas_used_test.go
[warning] 5-5: File is not goimports-ed with -local github.com/NibiruChain/nibiru
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: integration-tests
🔇 Additional comments (11)
eth/rpc/backend/call_tx.go (1)
12-12: Confirmed required import usage
The new import ofclientis necessary for returning theclient.Contextin the newly introduced function. No issues found.x/evm/precompile/funtoken_test.go (1)
176-177: Consider verifying usage ofResetGasMeterfor each test block.
Repeatedly resetting the gas meter can help isolate gas usage between discrete steps in your test, but it may artificially mask cumulative gas consumption issues that span multiple operations.Use the following script to confirm all other calls to
ResetGasMeterand evaluate if they are placed consistently:✅ Verification successful
Gas meter reset usage is appropriate and consistent
The current implementation follows the established pattern across the codebase where:
- Test files use
ResetGasMeter()to isolate gas measurements for individual operations- Production code uses
ResetGasMeterAndConsumeGas()to track cumulative gas usage🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for all calls to ResetGasMeter across the codebase for context. rg "ResetGasMeter("Length of output: 100
Script:
#!/bin/bash # Search for ResetGasMeter calls with proper regex rg "ResetGasMeter\(" --type go # Search for ResetGasMeter references without parentheses rg "ResetGasMeter" --type go # Use ast-grep to find method calls ast-grep --pattern 'ResetGasMeter($_)'Length of output: 3170
x/evm/keeper/funtoken_from_coin.go (2)
81-81: Validate the sales of EVM response.Capturing
evmRespis a good improvement, ensuring you can retrieve and handle gas usage details. Keep verifying that no other fields inevmRespare needed for accurate error reporting or usage analytics.
88-92: Confirm consistent blockGasUsed updates.Your approach to adding
evmResp.GasUsedto the block gas used and consuming gas again is valid. Just ensure that the transient gas usage is not double counted if subsequent logic also adjusts the gas meter.x/evm/keeper/keeper.go (2)
107-107: Defer usage correction.Invoking
HandleOutOfGasPanic(... )()directly in the defer statement is appropriate if you intend to call the returned function immediately upon exit. Validate that this behavior is intentional and does not alter any previously expected lazy evaluation dynamics.
150-150: Enhance out-of-gas error handling.Explicitly assigning
vm.ErrOutOfGashelps unify error types across the EVM environment and simplifies potential user-facing messages. This is a solid improvement.x/evm/keeper/msg_server.go (4)
78-79: Validate the formula for leftover gas.The logic to calculate
refundGasasevmMsg.Gas() - evmResp.GasUsedlooks correct. Just confirm upstream logic ensuresevmResp.GasUsedis never greater thanevmMsg.Gas()to avoid an underflow scenario.
86-89: Ensure correct block gas usage accounting.After adding the gas used to the block’s total usage via
k.AddToBlockGasUsed, the code callsk.ResetGasMeterAndConsumeGaswith the updated value. Verify that the order of operations (refund → block usage update → reset meter) is correct and does not trigger unexpected over-counting or under-counting of gas.
565-565: Confirm necessity of gas meter reset.Calling
k.ResetGasMeterAndConsumeGas(ctx, ctx.GasMeter().Limit())upon error is helpful to ensure the next operation has a fresh meter. However, check that you handle partial or consumed gas properly if you plan to charge the user for the gas used until this point.
572-572: Should you consume evmResp.GasUsed here instead of blockGasUsed? [Previously Raised]A past review comment (“k-yang: Shouldn't we consume
evmResp.GasUsedinstead ofblockGasUsedhere?”) already raised the same question. If the logic truly requires usingblockGasUsedas the input toResetGasMeterAndConsumeGas, consider adding a clarifying comment to justify it, or refactor if it’s an oversight.CHANGELOG.md (1)
56-56: Good additional entry for transparent versioning.Adding the fix reference is helpful for tracking PR #2132. Everything looks consistent with the project's changelog guidelines.
| blockGasUsed, errBlockGasUsed := k.AddToBlockGasUsed(ctx, evmResp.GasUsed) | ||
| if errBlockGasUsed != nil { | ||
| return nil, errors.Wrap(errBlockGasUsed, "error adding transient gas used") | ||
| } |
There was a problem hiding this comment.
Assess potential error impact on block gas used.
This snippet adds to the block’s total gas usage and then checks for errors. Confirm that an error during AddToBlockGasUsed or subsequent steps won’t accidentally skip the consumption of already-calculated usage, resulting in potential gas mismatch.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
eth/rpc/backend/backend_suite_test.go (2)
171-191: Consider parameterizing the timeout duration.The function enhancement to return the TransactionReceipt is well-implemented. However, the hardcoded 5-second timeout might need adjustment based on different network conditions.
Consider making the timeout duration configurable:
-func WaitForReceipt(s *BackendSuite, txHash gethcommon.Hash) (*big.Int, *gethcommon.Hash, *backend.TransactionReceipt) { +func WaitForReceipt(s *BackendSuite, txHash gethcommon.Hash, timeout time.Duration) (*big.Int, *gethcommon.Hash, *backend.TransactionReceipt) { - ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + ctx, cancel := context.WithTimeout(context.Background(), timeout)
98-98: Consider utilizing the TransactionReceipt information.The TransactionReceipt return value from WaitForReceipt is being ignored but could provide valuable information for test assertions.
Consider capturing and utilizing the receipt:
- blockNumber, blockHash, _ := WaitForReceipt(s, transferTxHash) + blockNumber, blockHash, receipt := WaitForReceipt(s, transferTxHash) + s.Require().Equal(uint64(params.TxGas), receipt.GasUsed, "Transfer should use exact TxGas")Also applies to: 107-107
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
eth/rpc/backend/backend_suite_test.go(4 hunks)eth/rpc/backend/gas_used_test.go(1 hunks)x/evm/precompile/funtoken_test.go(7 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- eth/rpc/backend/gas_used_test.go
- x/evm/precompile/funtoken_test.go
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: integration-tests
🔇 Additional comments (2)
eth/rpc/backend/backend_suite_test.go (2)
13-13: LGTM!The addition of the hexutil import is appropriate for handling Ethereum-style numeric values.
193-194: Remove or implement the incomplete function.The
broadcastSDKTxfunction appears to be incomplete and unused.Either implement the function if needed or remove it to maintain code cleanliness.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2132 +/- ##
==========================================
+ Coverage 65.12% 65.14% +0.02%
==========================================
Files 277 277
Lines 22267 22284 +17
==========================================
+ Hits 14502 14518 +16
Misses 6771 6771
- Partials 994 995 +1
|
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
eth/rpc/backend/backend_suite_test.go (2)
170-191: Consider improving the transaction wait implementation.While the core logic is correct, consider the following improvements:
- Use proper logging instead of fmt.Println
- Make the polling interval configurable
- Make the context timeout duration configurable
Example improvement:
-func WaitForReceipt(s *BackendSuite, txHash gethcommon.Hash) (*big.Int, *gethcommon.Hash, *backend.TransactionReceipt) { +func WaitForReceipt(s *BackendSuite, txHash gethcommon.Hash, opts ...WaitOption) (*big.Int, *gethcommon.Hash, *backend.TransactionReceipt) { + // Default options + options := waitOptions{ + timeout: 5 * time.Second, + pollInterval: time.Second, + } + for _, opt := range opts { + opt(&options) + } + - ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + ctx, cancel := context.WithTimeout(context.Background(), options.timeout) defer cancel() for { receipt, err := s.backend.GetTransactionReceipt(txHash) if err != nil { return nil, nil, nil } if receipt != nil { return receipt.BlockNumber, &receipt.BlockHash, receipt } select { case <-ctx.Done(): - fmt.Println("Timeout reached, transaction not included in a block yet.") + s.T().Logf("Timeout reached, transaction not included in a block yet.") return nil, nil, nil default: - time.Sleep(1 * time.Second) + time.Sleep(options.pollInterval) } } } +type waitOptions struct { + timeout time.Duration + pollInterval time.Duration +} + +type WaitOption func(*waitOptions) + +func WithTimeout(d time.Duration) WaitOption { + return func(o *waitOptions) { + o.timeout = d + } +} + +func WithPollInterval(d time.Duration) WaitOption { + return func(o *waitOptions) { + o.pollInterval = d + } +}
193-200: Consider a more descriptive function name.The function name could be more explicit about the unit conversion from Wei to native units. Consider renaming to
getUnibiBalanceInNativeUnitsor similar to make the conversion clear to callers.-func (s *BackendSuite) getUnibiBalance(address gethcommon.Address) *big.Int { +func (s *BackendSuite) getUnibiBalanceInNativeUnits(address gethcommon.Address) *big.Int {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
eth/rpc/backend/backend_suite_test.go(3 hunks)eth/rpc/backend/gas_used_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- eth/rpc/backend/gas_used_test.go
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: integration-tests
🔇 Additional comments (1)
eth/rpc/backend/backend_suite_test.go (1)
97-98: LGTM! Properly handling the updated WaitForReceipt return values.The changes correctly adapt to the new function signature while maintaining proper error handling.
Also applies to: 106-109
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
x/evm/precompile/funtoken_test.go (1)
Line range hint
354-377: Consider parameterizing the gas test values.The test uses hardcoded gas values. Consider extracting these values into named constants or test parameters to make the test more maintainable and the gas limits more visible.
+const ( + testDefaultGasLimit = 50_000 + testCustomGasLimit = evmtest.FunTokenGasLimitSendToEvm +) + func (s *FuntokenSuite) TestPrecompileLocalGas() { deps := s.deps randomAcc := testutil.AccAddress() - bankDenom := "unibi" + bankDenom := "unibi" // Consider adding a comment explaining the choice of denomination funtoken := evmtest.CreateFunTokenForBankCoin(&s.deps, bankDenom, &s.Suite)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
CHANGELOG.md(1 hunks)x/evm/precompile/funtoken_test.go(7 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- CHANGELOG.md
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: integration-tests
🔇 Additional comments (4)
x/evm/precompile/funtoken_test.go (4)
32-32: LGTM! Simplified struct definition.The removal of the
funtokenfield fromFuntokenSuitestruct improves the test design by moving the funtoken creation closer to where it's used, making the tests more self-contained.
171-172: Good addition of gas meter resets.The strategic placement of
ResetGasMeter()calls before critical operations ensures accurate gas measurements and prevents gas calculation errors from previous operations affecting subsequent tests.Also applies to: 260-261
Line range hint
1-700: Comprehensive test coverage with good error cases.The test suite demonstrates thorough coverage including:
- Happy path scenarios
- Error cases with insufficient gas
- Token conversion between bank and EVM
- Various denomination handling
- Gas management
Line range hint
155-168: Verify test coverage for different denominations.The test now uses "anycoin" instead of a more specific denomination. While this demonstrates flexibility, ensure there are additional test cases covering platform-specific denominations (e.g., "unibi") to validate real-world scenarios.
Run this script to check test coverage for different denominations:
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
x/evm/precompile/funtoken_test.go (2)
155-158: Consider using a constant for the test denomination.The hardcoded string "anycoin" could be moved to a package-level constant or test helper to maintain consistency across tests and make updates easier.
+const testBankDenom = "anycoin" + func (s *FuntokenSuite) TestHappyPath() { deps := evmtest.NewTestDeps() s.T().Log("Create FunToken mapping and ERC20") - bankDenom := "anycoin" + bankDenom := testBankDenom
351-353: Consider consistent test denomination usage.The test uses "unibi" while other tests use "anycoin". Consider using the same denomination across all tests unless testing specific denomination-related behaviors.
- bankDenom := "unibi" + bankDenom := testBankDenom
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
x/evm/precompile/funtoken_test.go(10 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: integration-tests
🔇 Additional comments (3)
x/evm/precompile/funtoken_test.go (3)
32-32: LGTM! Simplified struct definition.The removal of the
funtokenfield fromFuntokenSuitestruct improves test isolation by creating funtoken instances within individual test methods.
171-172: LGTM! Proper gas meter management.The strategic placement of
ResetGasMetercalls before key operations ensures accurate gas tracking and prevents gas measurement interference between different operations.Also applies to: 257-258
225-225: LGTM! Fee collector funding.Proper initialization of the fee collector account ensures realistic testing of gas consumption and fee handling.
Also applies to: 432-432, 577-577
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
eth/rpc/backend/backend_suite_test.go (1)
197-204: Consider adding block number parameter for historical balance queries.While the implementation is correct for latest balance queries, consider adding an optional block number parameter to allow querying historical balances, which could be useful for testing state transitions.
-func (s *BackendSuite) getUnibiBalance(address gethcommon.Address) *big.Int { +func (s *BackendSuite) getUnibiBalance(address gethcommon.Address, blockNumber *rpc.BlockNumber) *big.Int { - latestBlock := rpc.EthLatestBlockNumber - latestBlockOrHash := rpc.BlockNumberOrHash{BlockNumber: &latestBlock} + if blockNumber == nil { + blockNumber = &rpc.EthLatestBlockNumber + } + latestBlockOrHash := rpc.BlockNumberOrHash{BlockNumber: blockNumber} balance, err := s.backend.GetBalance(address, latestBlockOrHash) s.Require().NoError(err) return evm.WeiToNative(balance.ToInt()) }x/evm/keeper/msg_server.go (1)
566-574: Consider optimizing gas meter resets.The code resets the gas meter twice:
- On error with the gas limit
- After successful execution with the block gas used
While functionally correct, this could be simplified to a single reset after determining the final gas consumption.
- k.ResetGasMeterAndConsumeGas(ctx, ctx.GasMeter().Limit()) - return nil, err + return nil, err } blockGasUsed, errBlockGasUsed := k.AddToBlockGasUsed(ctx, evmResp.GasUsed) if errBlockGasUsed != nil { return nil, errors.Wrap(errBlockGasUsed, "error adding transient gas used") } k.ResetGasMeterAndConsumeGas(ctx, blockGasUsed)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
CHANGELOG.md(1 hunks)eth/rpc/backend/backend_suite_test.go(5 hunks)eth/rpc/backend/gas_used_test.go(1 hunks)eth/rpc/backend/nonce_test.go(2 hunks)x/evm/keeper/msg_server.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- CHANGELOG.md
- eth/rpc/backend/gas_used_test.go
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: integration-tests
🔇 Additional comments (6)
eth/rpc/backend/nonce_test.go (2)
16-18: LGTM! Good synchronization practice.The addition of mutex lock/unlock prevents potential nonce conflicts during concurrent transaction broadcasts, making the test more reliable.
45-48: LGTM! Proper handling of WaitForReceipt return values.The update correctly handles the new transaction receipt return value while maintaining the existing test logic.
eth/rpc/backend/backend_suite_test.go (2)
37-38: LGTM! Clear mutex declaration.The package-level mutex with descriptive comment clearly indicates its purpose for synchronizing concurrent transaction broadcasts.
174-190: LGTM! Enhanced transaction receipt handling.The function now properly returns the transaction receipt along with block information, maintaining consistent error handling across all return paths.
x/evm/keeper/msg_server.go (2)
78-79: LGTM: Gas refund calculation is correct.The logic properly calculates the refund amount as the difference between the message gas limit and actual gas used.
86-89: Verify block gas limit handling.While the error handling is proper, we should verify that the block gas limit is properly enforced.
Run this script to check block gas limit enforcement:
✅ Verification successful
Block gas limit is properly enforced
The block gas limit is comprehensively enforced through ante handlers before transaction execution, with proper error handling and event emission for cases where transactions exceed the limit. The
AddToBlockGasUsedfunction is called after these validations, making it safe.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for block gas limit enforcement in the codebase # Search for block gas limit checks rg -A 3 "BlockGasLimit|block.*gas.*limit" # Look for gas overflow protection ast-grep --pattern 'if $_ > BlockGasLimit { $$$ }'Length of output: 9839
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
eth/rpc/backend/backend_suite_test.go (2)
102-102: Consider utilizing the transaction receipt for additional validations.The transaction receipt contains valuable information that could be used to enhance test validations, such as gas used, logs, and status.
- blockNumber, blockHash, _ := WaitForReceipt(s, transferTxHash) + blockNumber, blockHash, receipt := WaitForReceipt(s, transferTxHash) + s.Require().Equal(uint64(1), receipt.Status, "Transaction should succeed")Also applies to: 111-111
174-190: Enhance error handling with specific error types.The current implementation returns nil values for both timeout and error cases, making it difficult for callers to distinguish between different failure scenarios.
Consider wrapping errors and using custom error types:
-func WaitForReceipt(s *BackendSuite, txHash gethcommon.Hash) (*big.Int, *gethcommon.Hash, *backend.TransactionReceipt) { +func WaitForReceipt(s *BackendSuite, txHash gethcommon.Hash) (*big.Int, *gethcommon.Hash, *backend.TransactionReceipt, error) { ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) defer cancel() for { receipt, err := s.backend.GetTransactionReceipt(txHash) if err != nil { - return nil, nil, nil + return nil, nil, nil, fmt.Errorf("failed to get receipt: %w", err) } if receipt != nil { - return receipt.BlockNumber, &receipt.BlockHash, receipt + return receipt.BlockNumber, &receipt.BlockHash, receipt, nil } select { case <-ctx.Done(): - fmt.Println("Timeout reached, transaction not included in a block yet.") - return nil, nil, nil + return nil, nil, nil, fmt.Errorf("timeout waiting for receipt: %w", ctx.Err()) default: time.Sleep(1 * time.Second) } } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
eth/rpc/backend/backend_suite_test.go(5 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: integration-tests
🔇 Additional comments (3)
eth/rpc/backend/backend_suite_test.go (3)
37-38: LGTM: Good synchronization practice for concurrent tests.The addition of
testMutexis a good practice for preventing race conditions in concurrent transaction tests.
197-204: LGTM: Well-implemented balance retrieval with proper unit conversion.The method correctly handles balance retrieval and unit conversion from Wei to Native units.
206-211: LGTM: Correct implementation of nonce retrieval.The method properly uses the provided address parameter and pending block number for accurate nonce tracking. This addresses the issue mentioned in previous reviews about incorrect address usage.
Unique-Divine
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
eth/rpc/backend/backend_suite_test.go (1)
174-190: Consider optimizing the polling interval.While the implementation is correct, the 1-second sleep interval might make tests run slower than necessary. Consider reducing it to improve test execution time.
- time.Sleep(1 * time.Second) + time.Sleep(100 * time.Millisecond)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
CHANGELOG.md(1 hunks)eth/rpc/backend/backend_suite_test.go(5 hunks)x/evm/keeper/msg_server.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- CHANGELOG.md
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: integration-tests
🔇 Additional comments (6)
eth/rpc/backend/backend_suite_test.go (3)
37-38: LGTM! Good synchronization practice.Adding a mutex for synchronizing concurrent transaction broadcasts is a good practice to prevent race conditions in tests.
197-204: LGTM! Well-implemented balance retrieval.The method properly handles balance retrieval and unit conversion, with appropriate error handling.
206-211: LGTM! Correctly implemented nonce retrieval.The method properly uses the provided address parameter for nonce retrieval, fixing the issue mentioned in past reviews.
x/evm/keeper/msg_server.go (3)
79-81: LGTM: Improved gas refund calculationThe change correctly calculates the refund based on the actual gas used by the EVM transaction (
evmResp.GasUsed) instead of the block's gas usage. This aligns with Ethereum's gas refund mechanism.
87-90: LGTM: Proper block gas trackingThe block gas usage is now correctly tracked after the refund calculation, with proper error handling and descriptive error messages.
571-579: Verify gas handling sequenceThe gas handling sequence looks correct but should be verified for edge cases:
- Gas meter reset on contract call failure
- Block gas usage tracking
- Final gas meter reset with actual usage
Run this script to check for similar patterns in other contract call implementations:
✅ Verification successful
Gas handling sequence is correctly implemented
The implementation follows the established pattern in the codebase for handling gas consumption and tracking:
- Resets and consumes all gas on contract call failure
- Tracks block gas usage for successful executions
- Performs final gas meter reset with actual usage
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for consistent gas handling patterns in contract calls # Look for CallContract followed by gas meter operations ast-grep --pattern 'CallContract($$$) $$$ ResetGasMeterAndConsumeGas($$$) $$$ AddToBlockGasUsed($$$)'Length of output: 111
Script:
#!/bin/bash # Search for all gas handling related patterns echo "=== CallContract usage ===" rg "CallContract" -A 5 -B 2 echo -e "\n=== ResetGasMeterAndConsumeGas usage ===" rg "ResetGasMeterAndConsumeGas" -A 2 -B 2 echo -e "\n=== AddToBlockGasUsed usage ===" rg "AddToBlockGasUsed" -A 2 -B 2Length of output: 32311
Summary by CodeRabbit
Bug Fixes
Testing
Documentation