fix(evm): added tx logs events to the funtoken related txs #2161
Conversation
WalkthroughThis pull request introduces enhancements to transaction log event handling and testing infrastructure across multiple components of the Nibiru blockchain. The changes focus on improving transaction traceability, particularly for fun token-related transactions, and updating test suites to support more robust transaction and log event testing. The modifications span several packages, including EVM, RPC backend, and common test utilities, with a consistent theme of improving transaction log emission and testing mechanisms. Changes
Sequence DiagramsequenceDiagram
participant Client
participant EVMBackend
participant TransactionProcessor
participant LogEmitter
participant BlockIndexer
Client->>EVMBackend: Send Transaction
EVMBackend->>TransactionProcessor: Process Transaction
TransactionProcessor->>LogEmitter: Emit Transaction Logs
LogEmitter-->>TransactionProcessor: Log Emission Result
TransactionProcessor->>BlockIndexer: Update Transaction Index
BlockIndexer-->>TransactionProcessor: Indexing Confirmation
TransactionProcessor-->>EVMBackend: Transaction Response
EVMBackend-->>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 (
|
![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 65.12% 65.14% +0.01%
==========================================
Files 277 277
Lines 22267 22278 +11
==========================================
+ Hits 14502 14513 +11
+ Misses 6771 6770 -1
- Partials 994 995 +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.
Summary by CodeRabbit
Changelog
Testing Improvements
Transaction Handling
EVM Functionality