fix(evm): added tx logs events to the funtoken related txs #2161
Conversation
onikonychev
commented
Jan 10, 2025
•
onikonychev
commented
- related There's no TxHash and Logs for CallContracts code-423n4/2024-11-nibiru-findings#23
📝 WalkthroughWalkthroughThis pull request introduces enhancements to transaction log event handling in the Nibiru EVM system. The changes span multiple files, focusing on improving transaction traceability, particularly for fun token-related transactions. Key modifications include updating test suites, modifying transaction broadcasting methods, and ensuring log events are correctly emitted and captured during various transaction types. Changes
Sequence DiagramsequenceDiagram
participant Client
participant EVMBackend
participant Keeper
participant LogEmitter
Client->>EVMBackend: Execute Transaction
EVMBackend->>Keeper: Call Contract
Keeper->>LogEmitter: Emit Transaction Logs
LogEmitter-->>Keeper: Log Events Generated
Keeper-->>EVMBackend: Transaction Processed
EVMBackend-->>Client: Transaction Receipt
Possibly related PRs
Suggested Labels
Suggested Reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (5)
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 (
|
![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 (1)
eth/rpc/backend/nonce_test.go (1)
45-45: Consider handling the ignored receipt value.The third return value from
WaitForReceiptis being ignored. Consider either using it for additional verification or documenting why it's not needed.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
CHANGELOG.md(1 hunks)eth/rpc/backend/backend_suite_test.go(6 hunks)eth/rpc/backend/nonce_test.go(2 hunks)eth/rpc/backend/tx_logs_test.go(1 hunks)eth/rpc/backend/utils_test.go(2 hunks)x/common/testutil/testnetwork/tx.go(2 hunks)x/common/testutil/testnetwork/tx_test.go(1 hunks)x/evm/evmtest/tx.go(2 hunks)x/evm/keeper/call_contract.go(1 hunks)x/evm/keeper/grpc_query_test.go(2 hunks)x/oracle/keeper/app_test.go(2 hunks)
🧰 Additional context used
🪛 golangci-lint (1.62.2)
eth/rpc/backend/backend_suite_test.go
[error] 198-198: func (*BackendSuite).getUnibiBalance is unused
(unused)
🪛 GitHub Check: lint
eth/rpc/backend/backend_suite_test.go
[failure] 198-198:
func (*BackendSuite).getUnibiBalance is unused (unused)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: integration-tests
🔇 Additional comments (20)
eth/rpc/backend/utils_test.go (2)
7-8: LGTM! Well-structured imports.The addition of Ethereum's standard libraries for handling common types, core types, and crypto operations is appropriate for implementing proper transaction log verification.
Also applies to: 11-11
24-33: Comprehensive test coverage for Transfer event logs.The test now properly verifies the structure of transaction logs, including:
- Contract address verification
- Event signature verification using Keccak256
- Topic verification for Transfer events
x/common/testutil/testnetwork/tx_test.go (1)
19-19: LGTM! Proper usage of the new API.The test correctly uses the new
BroadcastMsgsAPI by passingnilforaccountSequence, allowing the function to use the default sequence from the account.eth/rpc/backend/nonce_test.go (1)
16-18: LGTM! Thread-safe test implementation.Good practice to use mutex for preventing nonce conflicts during concurrent transaction broadcasting.
x/common/testutil/testnetwork/tx.go (3)
129-131: LGTM! Enhanced API flexibility.Good addition of the optional
accountSequenceparameter, allowing better control over transaction sequencing while maintaining backward compatibility.
153-158: LGTM! Clean sequence handling implementation.The implementation properly handles both custom and default sequence values, making it flexible for various use cases.
147-147: Verify the increased gas limit.The gas limit has been increased by 10x. Please verify that this increase is necessary and document the reason for this change.
✅ Verification successful
Test utility gas limit increase is reasonable
The increased gas limit (10M) is specifically for the test network utility and doesn't affect production code. This is a reasonable change as:
- It's isolated to test environment only
- Test scenarios often require higher gas limits to accommodate various test cases
- The production SDK broadcast still uses a lower 2M gas limit
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for gas-related issues or discussions in the codebase rg -A 3 "gas.*limit.*TO_MICRO|insufficient.*gas"Length of output: 1041
Script:
#!/bin/bash # Search for TO_MICRO constant definition and usage echo "=== TO_MICRO constant ===" rg -A 2 "TO_MICRO.*=|const.*TO_MICRO" echo -e "\n=== Gas limit configurations ===" rg -A 2 "SetGasLimit.*TO_MICRO|gasLimit.*TO_MICRO"Length of output: 826
Script:
#!/bin/bash # Search for gas-related test failures and discussions echo "=== Gas-related test patterns ===" rg -A 3 "gas.*test.*fail|test.*gas.*fail" echo -e "\n=== Gas consumption patterns in tests ===" rg -A 3 "GasConsumed|gas.*consumed" "**/*_test.go"Length of output: 325
Script:
#!/bin/bash # Search for gas consumption and configurations in tests echo "=== Gas consumption and configurations in tests ===" rg -A 3 "GasConsumed|gas.*consumed|gas.*config" --type go --type-add 'go:*.go' echo -e "\n=== Gas-related errors in tests ===" rg -A 3 "gas.*error|insufficient.*gas|out.*of.*gas" --type goLength of output: 31232
x/evm/keeper/call_contract.go (2)
141-144: LGTM! Error handling for log emission is properly implemented.The addition of log event emission with proper error handling enhances transaction traceability.
145-146: Verify transaction index update sequence.The block transaction index is correctly updated after successful log emission.
Run the following script to verify the transaction index update sequence:
✅ Verification successful
Transaction index update sequence verified
The block transaction index is consistently updated after log emissions in both
call_contract.goandmsg_server.go, following the correct sequence:
- Emit logs/events
- Calculate new index
- Update block transaction index
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that transaction index updates follow log emissions. # Test: Search for other places where block transaction index is updated. rg -A 5 'BlockTxIndex.Set'Length of output: 501
Script:
#!/bin/bash # Get context around BlockTxIndex.Set calls ast-grep --pattern 'k.EvmState.BlockTxIndex.Set($$$)' # Look for log emission patterns rg -B 10 'BlockTxIndex.Set'Length of output: 1649
x/oracle/keeper/app_test.go (2)
120-123: LGTM! Updated BroadcastMsgs call signature.The prevote broadcast correctly includes the nil account sequence parameter.
Line range hint
136-141: LGTM! Updated BroadcastMsgs call signature.The vote broadcast correctly includes the nil account sequence parameter.
eth/rpc/backend/backend_suite_test.go (2)
37-38: LGTM! Added mutex for test synchronization.The mutex prevents nonce conflicts during concurrent transaction broadcasts.
174-190: LGTM! Enhanced WaitForReceipt with transaction receipt.The function now returns additional transaction receipt information, improving test capabilities.
eth/rpc/backend/tx_logs_test.go (4)
21-29: LGTM! Well-documented test scenarios.The test cases comprehensively cover different transaction types and their expected log events.
31-33: LGTM! Proper test synchronization.The mutex is correctly used to prevent nonce conflicts during transaction broadcasts.
221-259: LGTM! Robust assertion helper.The
assertTxLogsAndTxIndexfunction thoroughly validates both transaction logs and indices.
261-288: LGTM! Comprehensive log matching.The
assertTxLogsMatchfunction performs detailed validation of log contents.x/evm/evmtest/tx.go (1)
29-29: LGTM! Enhanced function signature to include transaction response.The change to return both the transaction message and response provides better visibility into transaction execution results, which is particularly useful for tracking transaction logs and events.
Also applies to: 46-46
x/evm/keeper/grpc_query_test.go (1)
770-770: LGTM! Tests updated to handle new return value.Test code correctly adapted to handle the new function signature by properly capturing both return values, even though the response isn't needed in these test cases.
Also applies to: 846-846
CHANGELOG.md (1)
78-78: LGTM! Clear and well-placed changelog entry.The changelog entry accurately describes the changes made and is properly placed in the Nibiru EVM section.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2161 +/- ##
==========================================
+ Coverage 64.86% 64.88% +0.02%
==========================================
Files 277 277
Lines 22283 22301 +18
==========================================
+ Hits 14454 14471 +17
Misses 6839 6839
- Partials 990 991 +1
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
eth/rpc/backend/backend_suite_test.go (1)
238-238: Remove commented-out code.Commented-out code should be removed rather than left in the codebase. If this line is needed for future reference, consider documenting it in a more appropriate place.
-//recipient := crypto.CreateAddress(s.fundedAccEthAddr, 29381)eth/rpc/backend/tx_logs_test.go (1)
31-220: Consider using table-driven tests for better maintainability.While the test implementation is thorough and covers all scenarios, consider restructuring it as a table-driven test. This would make it easier to add new test cases and maintain the existing ones.
Example structure:
func (s *BackendSuite) TestLogs() { testCases := []struct { name string setup func() (txHash gethcommon.Hash) expectLogs []*gethcore.Log expectEthTx bool }{ { name: "simple eth transfer", setup: func() gethcommon.Hash { randomEthAddr := evmtest.NewEthPrivAcc().EthAddr return s.SendNibiViaEthTransfer(randomEthAddr, amountToSend, false) }, expectLogs: nil, expectEthTx: true, }, // Add other test cases... } for _, tc := range testCases { s.Run(tc.name, func() { // Test implementation }) } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
eth/rpc/backend/backend_suite_test.go(7 hunks)eth/rpc/backend/tx_logs_test.go(1 hunks)eth/rpc/backend/utils_test.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- eth/rpc/backend/utils_test.go
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: integration-tests
🔇 Additional comments (5)
eth/rpc/backend/backend_suite_test.go (3)
37-38: LGTM! Good synchronization practice.The addition of a global mutex for synchronizing concurrent transaction broadcasts is a good practice to prevent race conditions in tests.
174-190: LGTM! Enhanced receipt handling.The function now properly returns the transaction receipt along with block number and hash, with consistent error handling and timeout behavior.
199-202: LGTM! Good simplification.The function has been simplified while maintaining its functionality.
eth/rpc/backend/tx_logs_test.go (2)
22-34: LGTM! Well-documented test setup.The test documentation clearly outlines the test scenarios, and the mutex is properly used to prevent nonce conflicts during transaction broadcasting.
222-289: LGTM! Well-structured helper functions.The helper functions are well-implemented with:
- Comprehensive assertions
- Descriptive error messages
- Clear separation of concerns
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
eth/rpc/backend/tx_logs_test.go (3)
22-37: Consider adding timeout to prevent test deadlock.While the mutex handling is correct, consider adding a timeout mechanism to prevent potential deadlocks in case of test failures.
testMutex.Lock() -defer testMutex.Unlock() +defer func() { + // Ensure mutex is always unlocked, even if test panics + testMutex.Unlock() +}() + +// Add timeout context to prevent test hanging +ctx, cancel := context.WithTimeout(context.Background(), 2*time.Minute) +defer cancel()
39-120: Enhance error messages for better debugging.While the error handling is thorough, consider enhancing error messages to include more context about the transaction state.
-s.Require().Equal( - uint32(0), - txResp.Code, - fmt.Sprintf("Failed to create FunToken from ERC20. RawLog: %s", txResp.RawLog), -) +s.Require().Equal( + uint32(0), + txResp.Code, + fmt.Sprintf( + "Failed to create FunToken from ERC20. Code: %d, RawLog: %s, GasUsed: %d", + txResp.Code, + txResp.RawLog, + txResp.GasUsed, + ), +)
266-293: Consider adding type safety checks for log data.The log comparison could be more robust with additional type safety checks for the log data.
func (s *BackendSuite) assertTxLogsMatch( expectedLogs []*gethcore.Log, actualLogs []*gethcore.Log, txInfo string, ) { + if expectedLogs == nil || actualLogs == nil { + s.Fail("Nil logs provided for comparison") + return + } + for idx, expectedLog := range expectedLogs { actualLog := actualLogs[idx] + if expectedLog == nil || actualLog == nil { + s.Fail("Nil log entry encountered") + return + } + s.Require().Equal( expectedLog.Address, actualLog.Address, fmt.Sprintf("log contract address mismatch, log index %d, %s", idx, txInfo), )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
eth/rpc/backend/tx_logs_test.go(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: integration-tests
🔇 Additional comments (2)
eth/rpc/backend/tx_logs_test.go (2)
1-20: LGTM! Well-organized imports.The package declaration and import organization follow good practices, clearly separating external and internal dependencies.
1-293: Overall excellent test implementation!The test suite is well-structured and comprehensive, covering various transaction types and their log events. The implementation includes proper error handling, mutex synchronization, and thorough log verification. While there are some suggested improvements for robustness, the current implementation is solid and achieves its testing objectives.
eth/rpc/backend/tx_logs_test.go
Outdated
| // Getting block results. Note: txs could be included in more than one block | ||
| blockNumber := blockNumFirstTx.Int64() | ||
| blockRes, err := s.backend.TendermintBlockResultByNumber(&blockNumber) | ||
| s.Require().NoError(err) | ||
| s.Require().NotNil(blockRes) | ||
| txIndex := 0 | ||
| for idx, check := range checks { | ||
| if txIndex+1 > len(blockRes.TxsResults) { | ||
| blockNumber++ | ||
| if blockNumber > blockNumLastTx.Int64() { | ||
| s.Fail("TX %d not found in block results", idx) | ||
| } | ||
| txIndex = 0 | ||
| blockRes, err = s.backend.TendermintBlockResultByNumber(&blockNumber) | ||
| s.Require().NoError(err) | ||
| s.Require().NotNil(blockRes) | ||
| } | ||
| s.assertTxLogsAndTxIndex(blockRes, txIndex, check.logs, check.expectEthTx, check.txInfo) | ||
| txIndex++ | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add bounds checking for block results.
The block result iteration could be more robust with additional bounds checking.
if txIndex+1 > len(blockRes.TxsResults) {
blockNumber++
if blockNumber > blockNumLastTx.Int64() {
- s.Fail("TX %d not found in block results", idx)
+ s.Fail(
+ "Transaction %d (%s) not found in blocks %d to %d",
+ idx,
+ check.txInfo,
+ blockNumFirstTx.Int64(),
+ blockNumLastTx.Int64(),
+ )
}
txIndex = 0
+ // Add maximum block search limit
+ if blockNumber - blockNumFirstTx.Int64() > 10 {
+ s.Fail("Exceeded maximum block search range")
+ }
blockRes, err = s.backend.TendermintBlockResultByNumber(&blockNumber)
s.Require().NoError(err)
s.Require().NotNil(blockRes)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Getting block results. Note: txs could be included in more than one block | |
| blockNumber := blockNumFirstTx.Int64() | |
| blockRes, err := s.backend.TendermintBlockResultByNumber(&blockNumber) | |
| s.Require().NoError(err) | |
| s.Require().NotNil(blockRes) | |
| txIndex := 0 | |
| for idx, check := range checks { | |
| if txIndex+1 > len(blockRes.TxsResults) { | |
| blockNumber++ | |
| if blockNumber > blockNumLastTx.Int64() { | |
| s.Fail("TX %d not found in block results", idx) | |
| } | |
| txIndex = 0 | |
| blockRes, err = s.backend.TendermintBlockResultByNumber(&blockNumber) | |
| s.Require().NoError(err) | |
| s.Require().NotNil(blockRes) | |
| } | |
| s.assertTxLogsAndTxIndex(blockRes, txIndex, check.logs, check.expectEthTx, check.txInfo) | |
| txIndex++ | |
| } | |
| // Getting block results. Note: txs could be included in more than one block | |
| blockNumber := blockNumFirstTx.Int64() | |
| blockRes, err := s.backend.TendermintBlockResultByNumber(&blockNumber) | |
| s.Require().NoError(err) | |
| s.Require().NotNil(blockRes) | |
| txIndex := 0 | |
| for idx, check := range checks { | |
| if txIndex+1 > len(blockRes.TxsResults) { | |
| blockNumber++ | |
| if blockNumber > blockNumLastTx.Int64() { | |
| s.Fail( | |
| "Transaction %d (%s) not found in blocks %d to %d", | |
| idx, | |
| check.txInfo, | |
| blockNumFirstTx.Int64(), | |
| blockNumLastTx.Int64(), | |
| ) | |
| } | |
| txIndex = 0 | |
| // Add maximum block search limit | |
| if blockNumber - blockNumFirstTx.Int64() > 10 { | |
| s.Fail("Exceeded maximum block search range") | |
| } | |
| blockRes, err = s.backend.TendermintBlockResultByNumber(&blockNumber) | |
| s.Require().NoError(err) | |
| s.Require().NotNil(blockRes) | |
| } | |
| s.assertTxLogsAndTxIndex(blockRes, txIndex, check.logs, check.expectEthTx, check.txInfo) | |
| txIndex++ | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
eth/rpc/backend/tx_logs_test.go (5)
22-31: Consider enhancing test documentation with expected log events.While the documentation clearly outlines the test steps, it would be more comprehensive to include the expected log events for each transaction type. This would make the test's expectations more explicit and aid in maintenance.
32-38: Consider adding test cleanup.While the test setup is good with mutex locking and fresh block creation, consider adding cleanup steps after test execution to reset the state. This would ensure test isolation and prevent potential side effects in subsequent tests.
103-113: Consider making gas parameters configurable.The hardcoded gas values (
Gas: 1_500_000) could be problematic if gas requirements change. Consider making these configurable or calculating them dynamically based on the operation.
121-198: Consider validating log data more extensively.While the test validates log topics and addresses, it could be more thorough by also validating:
- Log data field contents
- Event parameter values
- Block and transaction hashes
272-299: Enhance log validation in helper functions.Consider adding validation for:
- Null checks for log parameters
- Log ordering verification
- Data field length validation
This would make the test more robust against edge cases.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
eth/rpc/backend/tx_logs_test.go(1 hunks)x/evm/keeper/call_contract.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- x/evm/keeper/call_contract.go
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: integration-tests
🔇 Additional comments (2)
eth/rpc/backend/tx_logs_test.go (2)
1-20: LGTM! Well-organized imports.The imports are properly organized and all dependencies are necessary for the test implementation.
208-218: Add bounds checking for block results.The block result iteration could be more robust with additional bounds checking.
| // Wait for all txs to be included in a block | ||
| blockNumFirstTx, _, _ := WaitForReceipt(s, txHashFirst) | ||
| blockNumLastTx, _, _ := WaitForReceipt(s, txHashLast) | ||
| s.Require().NotNil(blockNumFirstTx) | ||
| s.Require().NotNil(blockNumLastTx) | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add timeout handling for transaction receipt waiting.
The WaitForReceipt calls could potentially hang if transactions are never mined. Consider adding a timeout mechanism to fail fast in such scenarios.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
eth/rpc/backend/backend_suite_test.go (1)
255-255: Remove commented out code.The commented line appears to be a debugging artifact and should be removed to maintain code cleanliness.
- //recipient := crypto.CreateAddress(s.fundedAccEthAddr, 29381)x/evm/evmtest/tx.go (2)
29-29: LGTM! Consider improving function documentation.The signature change to return both transaction and response aligns well with the PR objectives. Consider adding documentation to explain the return values.
Add a function comment explaining the return values:
// ExecuteNibiTransfer executes nibi transfer +// Returns: +// - *evm.MsgEthereumTx: The executed Ethereum transaction +// - *evm.MsgEthereumTxResponse: The transaction execution response containing logs and events
46-46: Consider validating the response before returning.While the error and VM error are checked, consider validating that the response contains the expected logs and events before returning.
- return ethTxMsg, resp + // Validate response + if resp.Logs == nil { + t.Error("Expected transaction logs but got nil") + } + return ethTxMsg, resp
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
CHANGELOG.md(1 hunks)eth/rpc/backend/backend_suite_test.go(1 hunks)x/evm/evmtest/tx.go(2 hunks)x/evm/keeper/call_contract.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- CHANGELOG.md
🧰 Additional context used
🪛 GitHub Check: lint
x/evm/keeper/call_contract.go
[failure] 90-90:
too many return values
[failure] 90-90:
too many return values
[failure] 90-90:
too many return values
[failure] 90-90:
too many return values
[failure] 90-90:
too many return values
🪛 GitHub Check: build
x/evm/keeper/call_contract.go
[failure] 90-90:
too many return values
🪛 GitHub Check: e2e-evm
x/evm/keeper/call_contract.go
[failure] 90-90:
too many return values
🪛 GitHub Check: integration-tests
x/evm/keeper/call_contract.go
[failure] 90-90:
too many return values
[failure] 90-90:
too many return values
[failure] 90-90:
too many return values
[failure] 90-90:
too many return values
🪛 GitHub Check: unit-tests
x/evm/keeper/call_contract.go
[failure] 90-90:
too many return values
[failure] 90-90:
too many return values
[failure] 90-90:
too many return values
🪛 GitHub Actions: Linter
x/evm/keeper/call_contract.go
[error] 90-90: too many return values
🪛 GitHub Actions: EVM E2E tests
x/evm/keeper/call_contract.go
[error] 90-90: Function has too many return values
🪛 GitHub Actions: Unit Tests
x/evm/keeper/call_contract.go
[error] 90-90: Function has too many return values
🔇 Additional comments (4)
eth/rpc/backend/backend_suite_test.go (3)
Line range hint
42-43: LGTM! Good synchronization practice.The addition of
testMutexis a good practice for preventing race conditions during concurrent transaction broadcasts in tests.
Line range hint
198-204: Remove unused function.The
getUnibiBalancefunction is not used in any test.
Line range hint
1-284: Well-structured test suite with comprehensive coverage.The test suite demonstrates good practices:
- Comprehensive setup and teardown
- Consistent error handling
- Well-documented helper functions
- Proper transaction and contract deployment testing
x/evm/keeper/call_contract.go (1)
88-91: LGTM - Log event emission properly implemented.The addition of log event emission aligns well with the PR objectives of enhancing transaction traceability for fun token transactions. The error handling is robust, and the implementation maintains consistency with the block bloom filter updates.
🧰 Tools
🪛 GitHub Check: lint
[failure] 90-90:
too many return values
[failure] 90-90:
too many return values
[failure] 90-90:
too many return values
[failure] 90-90:
too many return values
[failure] 90-90:
too many return values🪛 GitHub Check: build
[failure] 90-90:
too many return values🪛 GitHub Check: e2e-evm
[failure] 90-90:
too many return values🪛 GitHub Check: integration-tests
[failure] 90-90:
too many return values
[failure] 90-90:
too many return values
[failure] 90-90:
too many return values
[failure] 90-90:
too many return values🪛 GitHub Check: unit-tests
[failure] 90-90:
too many return values
[failure] 90-90:
too many return values
[failure] 90-90:
too many return values🪛 GitHub Actions: Linter
[error] 90-90: too many return values
🪛 GitHub Actions: EVM E2E tests
[error] 90-90: Function has too many return values
🪛 GitHub Actions: Unit Tests
[error] 90-90: Function has too many return values
x/evm/keeper/call_contract.go
Outdated
| err = k.EmitLogEvents(ctx, evmResp) | ||
| if err != nil { | ||
| return nil, nil, errors.Wrap(err, "error emitting tx logs") | ||
| } |
There was a problem hiding this comment.
Fix incorrect return statement causing build failures.
The return statement has too many values (3) for a function that only declares 2 return parameters (*evm.MsgEthereumTxResponse, error).
Apply this fix:
if commit {
k.updateBlockBloom(ctx, evmResp, uint64(txConfig.LogIndex))
err = k.EmitLogEvents(ctx, evmResp)
if err != nil {
- return nil, nil, errors.Wrap(err, "error emitting tx logs")
+ return nil, errors.Wrap(err, "error emitting tx logs")
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| err = k.EmitLogEvents(ctx, evmResp) | |
| if err != nil { | |
| return nil, nil, errors.Wrap(err, "error emitting tx logs") | |
| } | |
| err = k.EmitLogEvents(ctx, evmResp) | |
| if err != nil { | |
| return nil, errors.Wrap(err, "error emitting tx logs") | |
| } |
🧰 Tools
🪛 GitHub Check: lint
[failure] 90-90:
too many return values
[failure] 90-90:
too many return values
[failure] 90-90:
too many return values
[failure] 90-90:
too many return values
[failure] 90-90:
too many return values
🪛 GitHub Check: build
[failure] 90-90:
too many return values
🪛 GitHub Check: e2e-evm
[failure] 90-90:
too many return values
🪛 GitHub Check: integration-tests
[failure] 90-90:
too many return values
[failure] 90-90:
too many return values
[failure] 90-90:
too many return values
[failure] 90-90:
too many return values
🪛 GitHub Check: unit-tests
[failure] 90-90:
too many return values
[failure] 90-90:
too many return values
[failure] 90-90:
too many return values
🪛 GitHub Actions: Linter
[error] 90-90: too many return values
🪛 GitHub Actions: EVM E2E tests
[error] 90-90: Function has too many return values
🪛 GitHub Actions: Unit Tests
[error] 90-90: Function has too many return values
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
eth/rpc/backend/gas_used_test.go (1)
Line range hint
82-138: Consider adding edge case tests.The test suite could be enhanced with additional scenarios:
- Maximum gas limit transactions
- Zero gas price transactions
- Contract interactions with complex state changes
eth/rpc/backend/tx_logs_test.go (2)
22-31: Enhance test documentation.While the test scenarios are well-structured, consider adding:
- Expected outcomes for each transaction type
- Explanation of the log structure expectations
- References to relevant EVM specifications
272-299: Optimize log matching efficiency.Consider using a map-based approach for matching logs when dealing with large numbers of events. This would improve performance by reducing the number of comparisons needed.
Example optimization:
func (s *BackendSuite) assertTxLogsMatch( expectedLogs []*gethcore.Log, actualLogs []*gethcore.Log, txInfo string, ) { + // Create a map of expected logs for faster lookup + expectedLogsMap := make(map[string]*gethcore.Log) + for _, log := range expectedLogs { + key := fmt.Sprintf("%s-%v", log.Address.String(), log.Topics) + expectedLogsMap[key] = log + } + for idx, expectedLog := range expectedLogs { actualLog := actualLogs[idx] + key := fmt.Sprintf("%s-%v", actualLog.Address.String(), actualLog.Topics) + if _, exists := expectedLogsMap[key]; !exists { + s.Fail(fmt.Sprintf("log not found, log index %d, %s", idx, txInfo)) + } - s.Require().Equal( - expectedLog.Address, - actualLog.Address, - fmt.Sprintf("log contract address mismatch, log index %d, %s", idx, txInfo), - ) - // ... rest of the comparisons } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
eth/rpc/backend/gas_used_test.go(4 hunks)eth/rpc/backend/tx_logs_test.go(1 hunks)x/evm/keeper/call_contract.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- x/evm/keeper/call_contract.go
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: integration-tests
🔇 Additional comments (4)
eth/rpc/backend/gas_used_test.go (2)
79-83: LGTM! Improved nonce management.The changes enhance transaction sequencing by using a dedicated nonce management method and explicitly passing the nonce to BroadcastMsgs.
Line range hint
99-133: Verify transaction execution order.While the nonce increments are correct, consider adding assertions to verify that transactions are executed in the expected order.
Run this script to verify transaction ordering:
✅ Verification successful
Transaction ordering is properly validated
The test already ensures correct transaction ordering through:
- Sequential nonce values
- Ordered receipt verification
- Block number validation
- Explicit block number equality checks where needed
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that transactions are executed in the correct order based on nonce values # Search for transaction receipts and their ordering rg -A 5 "WaitForReceipt.*txHash[123]"Length of output: 1046
Script:
#!/bin/bash # Search for block number assertions or comparisons rg -A 2 "blockNumber[123]" eth/rpc/backend/gas_used_test.goLength of output: 947
eth/rpc/backend/tx_logs_test.go (2)
115-120: Add timeout handling for transaction receipt waiting.The
WaitForReceiptcalls could potentially hang if transactions are never mined. Consider adding a timeout mechanism to fail fast in such scenarios.
208-218: Add bounds checking for block results.The block result iteration could be more robust with additional bounds checking.
