feat: add indexer tests #115
Conversation
WalkthroughThis pull request introduces several modifications primarily focused on the Changes
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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 (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #115 +/- ##
===========================================
+ Coverage 40.68% 56.79% +16.11%
===========================================
Files 108 108
Lines 9970 9914 -56
===========================================
+ Hits 4056 5631 +1575
+ Misses 5336 3471 -1865
- Partials 578 812 +234
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (19)
x/evm/keeper/txutils_test.go (6)
Line range hint
65-73: Refactor private key generation and transaction signing into a helper function to reduce duplication.The code for generating random ECDSA private keys and signing Ethereum transactions is repeated across multiple tests (
Test_DynamicFeeTxConversion,Test_AccessTxConversion,Test_LegacyTxConversion). Extracting this logic into a helper function will improve maintainability and reduce code duplication.Consider applying this refactor:
+func generateSignedEthTx(tx coretypes.Transaction, chainID *big.Int) (*coretypes.Transaction, *ethsecp256k1.PrivKey, error) { + privKey, err := ecdsa.GenerateKey(crypto.S256(), rand.Reader) + if err != nil { + return nil, nil, err + } + signer := coretypes.LatestSignerForChainID(chainID) + signedTx, err := coretypes.SignTx(tx, signer, privKey) + if err != nil { + return nil, nil, err + } + cosmosKey := ethsecp256k1.PrivKey{ + Key: crypto.FromECDSA(privKey), + } + return signedTx, &cosmosKey, nil +}Then replace the repeated code in each test with calls to
generateSignedEthTx.Also applies to: 155-163, 246-254
Line range hint
66-70: Userand.Readerdirectly when generating ECDSA private keys.Instead of generating random bytes and creating a
bytes.Reader, you can userand.Readerdirectly for simplicity and efficiency.Apply this change:
-randBytes := make([]byte, 64) -_, err = rand.Read(randBytes) -require.NoError(t, err) -reader := bytes.NewReader(randBytes) -privKey, err := ecdsa.GenerateKey(crypto.S256(), reader) +privKey, err := ecdsa.GenerateKey(crypto.S256(), rand.Reader) require.NoError(t, err)
Line range hint
355-364: EnhanceEqualEthTransactionto compare all relevant fields, including signatures.The
EqualEthTransactionfunction currently compares several transaction fields but omits the signatures and, in the case of dynamic fee transactions, the access lists. Including these fields ensures a more comprehensive validation of transaction equality.Consider updating
EqualEthTransaction:func EqualEthTransaction(t *testing.T, expected, actual *coretypes.Transaction) { require.Equal(t, expected.ChainId(), actual.ChainId()) require.Equal(t, expected.Nonce(), actual.Nonce()) require.Equal(t, expected.GasTipCap(), actual.GasTipCap()) require.Equal(t, expected.GasFeeCap(), actual.GasFeeCap()) require.Equal(t, expected.Gas(), actual.Gas()) require.Equal(t, expected.To(), actual.To()) require.Equal(t, expected.Data(), actual.Data()) require.Equal(t, expected.Value(), actual.Value()) require.Equal(t, expected.Type(), actual.Type()) + require.Equal(t, expected.SignatureValues(), actual.SignatureValues()) + require.Equal(t, expected.AccessList(), actual.AccessList()) }
Line range hint
295-297: Define a constant for customSignModeto improve readability.Using
SignMode(10)without a defined constant can be unclear. Defining a constant or using an existingSignModeenhances code readability and maintainability.Define a custom
SignMode:+const SignMode_TEST_UNSPECIFIED signing.SignMode = 10 err = txBuilder.SetSignatures(signing.SignatureV2{ PubKey: cosmosKey.PubKey(), Data: &signing.SingleSignatureData{ - SignMode: signing.SignMode(10), + SignMode: SignMode_TEST_UNSPECIFIED, Signature: sigBytes, }, })Alternatively, use an existing
SignModeif appropriate.
Line range hint
271-273: Handle potential integer underflow when adjustingvin legacy transaction signature.Subtracting
(35 + ethChainID.Uint64()*2)fromvcould result in an integer underflow ifvis less than the subtracted value. Ensure thatvis sufficiently large or add checks to prevent underflow.Apply this check:
sigBytes[64] = byte(v.Uint64() - (35 + ethChainID.Uint64()*2)) +require.True(t, v.Uint64() >= (35 + ethChainID.Uint64()*2), "v value is too small and may cause underflow")
Line range hint
312-314: Check for nilAccessListbefore assignment to prevent potentialnildereference.In
Test_AccessTxConversion, when setting theAccessListin theMsgCall, ensure that theAccessListfrom the Ethereum transaction is not nil to prevent a potentialnildereference.Add a nil check:
if ethTx.AccessList() != nil { msg.AccessList = types.ConvertAccessList(ethTx.AccessList()) } +require.NotNil(t, msg.AccessList)indexer/abci_test.go (3)
24-27: Avoid magic numbers in event index.The test uses a magic number
-3to access a specific event. Consider extracting this as a named constant or adding a comment explaining why this specific index is used.+// contractCreatedEventIndex represents the position of the contract creation event +// in the events array. It's -3 because... +const contractCreatedEventIndex = -3 + events := finalizeRes.TxResults[0].Events -createEvent := events[len(events)-3] +createEvent := events[len(events)+contractCreatedEventIndex]
31-36: Consider testing with realistic gas limits.Using
NewInfiniteGasMetermight mask potential gas-related issues in production. Consider adding test cases with realistic gas limits.
43-46: Consider extracting test constants and documenting large values.The test uses a large token amount (1_000_000_000_000). Consider:
- Extracting this as a named constant for better maintainability
- Adding a comment explaining why this specific amount was chosen
- Testing with different amounts including edge cases
+const ( + // testMintAmount represents a large token amount used for testing + // This value was chosen because... + testMintAmount = 1_000_000_000_000 +) + -tx, evmTxHash = generateMintERC20Tx(t, app, privKeys[0], common.BytesToAddress(contractAddr), addrs[0], new(big.Int).SetUint64(1_000_000_000_000)) +tx, evmTxHash = generateMintERC20Tx(t, app, privKeys[0], common.BytesToAddress(contractAddr), addrs[0], new(big.Int).SetUint64(testMintAmount))indexer/indexer_test.go (2)
24-29: Consider improving number literal readability.The large number literal could be more readable by using numeric separators or constants.
Consider this improvement:
-genCoins = sdk.NewCoins(sdk.NewCoin(baseDenom, math.NewInt(1_000_000_000_000_000_000).MulRaw(1_000_000))).Sort() +const ( + baseAmount = 1_000_000_000_000_000_000 // 1 quintillion + multiplier = 1_000_000 // 1 million +) +genCoins = sdk.NewCoins(sdk.NewCoin(baseDenom, math.NewInt(baseAmount).MulRaw(multiplier))).Sort()
31-34: Enhance test failure messages.Consider adding a more descriptive error message to help debug test failures.
-require.True(t, balances.Equal(app.BankKeeper.GetAllBalances(ctxCheck, addr.Bytes()))) +actualBalance := app.BankKeeper.GetAllBalances(ctxCheck, addr.Bytes()) +require.True(t, balances.Equal(actualBalance), + "Balance mismatch for address %s: expected %s, got %s", + addr.Hex(), balances, actualBalance)indexer/tx_helper_test.go (4)
33-35: Consider generating fresh random bytes for each keyThe current implementation reuses the same random bytes buffer for multiple key generations. While this doesn't immediately compromise security due to the subsequent use of these bytes as a reader, it's better practice to generate fresh random bytes for each key.
for i := 0; i < n; i++ { - randBytes := make([]byte, 64) - _, err := rand.Read(randBytes) + privKey, err := ecdsa.GenerateKey(crypto.S256(), rand.Reader) require.NoError(t, err) - reader := bytes.NewReader(randBytes) - privKey, err := ecdsa.GenerateKey(crypto.S256(), reader) - require.NoError(t, err)
63-64: Consider making gas parameters configurableThe gas limit and price are currently hard-coded. Consider making these configurable parameters to accommodate different transaction types and network conditions.
- gasLimit := new(big.Int).SetUint64(1_000_000) - gasPrice := new(big.Int).SetUint64(1_000_000_000) + func generateTx( + t *testing.T, + app *minitiaapp.MinitiaApp, + privKey *ecdsa.PrivateKey, + to *common.Address, + inputBz []byte, + gasLimit uint64, + gasPrice uint64, + ) (sdk.Tx, common.Hash) {
121-145: Enhance documentation and add validation for executeTxsThe function would benefit from:
- More detailed documentation explaining the block finalization process
- Validation of input transactions
- Height validation to ensure sequential block heights
-// execute txs and finalize block and commit block +// executeTxs processes the provided transactions by: +// 1. Encoding each transaction +// 2. Creating a finalize block request with the encoded transactions +// 3. Executing the transactions in a new block +// 4. Committing the block to the chain +// Returns the finalize request and response for verification purposes. func executeTxs(t *testing.T, app *minitiaapp.MinitiaApp, txs ...sdk.Tx) (*abcitypes.RequestFinalizeBlock, *abcitypes.ResponseFinalizeBlock) { + require.NotEmpty(t, txs, "at least one transaction is required") + txsBytes := make([][]byte, len(txs)) for i, tx := range txs { + require.NotNil(t, tx, "transaction cannot be nil") txBytes, err := app.TxConfig().TxEncoder()(tx) require.NoError(t, err)
1-2: Add package documentationConsider adding a package-level documentation comment that describes the purpose and usage of these test helpers.
+// Package indexer_test provides utility functions for testing Ethereum transactions +// within the indexer package. It includes helpers for key generation, transaction +// creation, and block execution in a test environment. package indexer_testapp/test_helpers.go (2)
Line range hint
31-47: Consider using a more conventional MaxGas valueThe MaxGas value of 1,234,000,000 seems arbitrary. Consider using a more conventional value like 1,000,000,000 (1 billion) or document why this specific value was chosen.
140-144: Document the zero GasRefundRatio configurationSetting
GasRefundRatioto zero is a significant choice that affects gas refunds in the EVM. Please add a comment explaining why this specific value was chosen for the test environment.// set evm genesis params var evmGenesis evmtypes.GenesisState app.AppCodec().MustUnmarshalJSON(genesisState[evmtypes.ModuleName], &evmGenesis) +// Set GasRefundRatio to zero to disable gas refunds in test environment evmGenesis.Params.GasRefundRatio = math.LegacyZeroDec() genesisState[evmtypes.ModuleName] = app.AppCodec().MustMarshalJSON(&evmGenesis)indexer/indexer.go (1)
183-186: Consider comprehensive resource cleanupWhile stopping
txPendingMapis good, consider cleaning up other resources:
- Close subscription channels (
blockChans,logsChans,pendingChans)- Ensure pending operations in the store are completed
Here's a suggested implementation:
func (e *EVMIndexerImpl) Stop() { e.txPendingMap.Stop() + // Close all subscription channels + for _, ch := range e.blockChans { + close(ch) + } + for _, ch := range e.logsChans { + close(ch) + } + for _, ch := range e.pendingChans { + close(ch) + } + // Clear subscription slices + e.blockChans = nil + e.logsChans = nil + e.pendingChans = nil }app/app.go (1)
Line range hint
1-609: Well-structured integration of the EVM indexerThe changes demonstrate good architectural design by properly handling the complete lifecycle of the EVM indexer:
- Initialization during app setup
- Integration with the mempool
- Proper cleanup during shutdown
- Clean accessor method for testing
This structure will facilitate the addition of indexer tests while maintaining clean separation of concerns.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (8)
app/app.go(1 hunks)app/indexer.go(1 hunks)app/test_helpers.go(4 hunks)indexer/abci_test.go(1 hunks)indexer/indexer.go(4 hunks)indexer/indexer_test.go(1 hunks)indexer/tx_helper_test.go(1 hunks)x/evm/keeper/txutils_test.go(1 hunks)
🔇 Additional comments (12)
indexer/abci_test.go (3)
1-16: LGTM! Package and imports are well-organized.
The package name follows Go conventions for external test packages, and imports are properly grouped.
20-23: Verify transaction generation and execution helpers.
The test relies on generateCreateERC20Tx and executeTxs helper functions. Let's verify their implementations.
#!/bin/bash
# Search for helper function implementations
rg -p "func generate.*ERC20Tx|func executeTxs" -A 1017-19: Verify the setupIndexer helper function implementation.
The test relies on setupIndexer helper function. Let's verify its implementation to ensure proper test initialization.
✅ Verification successful
The previous search might have failed due to pattern matching issues. Let's try a simpler ripgrep search and also get the full context of the test file to understand where setupIndexer is defined.
Helper function setupIndexer is properly implemented and used correctly
The verification shows that setupIndexer is well-implemented in indexer/indexer_test.go. It:
- Creates an app with test addresses and private keys via
createApp - Initializes an in-memory database
- Creates a new EVMIndexer instance with the necessary dependencies
- Returns all required components for the test: app, indexer, addresses, and private keys
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for setupIndexer implementation
ast-grep --pattern 'func setupIndexer($$$) ($$$) {
$$$
}'
# Backup: Search using ripgrep if not found
rg -p "func setupIndexer" -A 10
Length of output: 1543
Script:
#!/bin/bash
# Search for setupIndexer implementation with ripgrep in test files
rg -p "func setupIndexer" --type go -A 20
# Also search for the file containing the test to get full context
fd "abci_test.go" --exec cat {}
Length of output: 3064
indexer/indexer_test.go (2)
1-22: LGTM! Well-organized package structure and imports.
The package follows Go conventions with proper test package naming, and imports are logically grouped.
36-60:
Add missing generateKeys function.
The generateKeys function is called but not defined in this file.
Please ensure the generateKeys helper function is properly defined or imported.
app/indexer.go (2)
28-28: LGTM: EVM indexer initialization updated correctly
The NewEVMIndexer call has been properly updated to match the new signature with five parameters.
Line range hint 44-46: Verify consistency of OPChildKeeper usage
While OPChildKeeper was removed from EVMIndexer, it's still being used in blocksubmodule.NewBlockSubmodule. Please verify if this is intentional or if the block submodule should also be updated.
✅ Verification successful
Let me gather more information about the block submodule to verify the OPChildKeeper usage.
Let me check the block submodule implementation specifically.
Let me check the block submodule's implementation from the kvindexer repository to understand its dependency on OPChildKeeper.
OPChildKeeper is still required in block submodule
The OPChildKeeper is actively being used in multiple parts of the application:
- It's a required parameter in
blocksubmodule.NewBlockSubmodule - It's used in various other components like
blocksdk.go, gas price calculations, and ante handlers - The removal from
EVMIndexerappears to be a specific optimization for that component, while the block submodule still needs this dependency for its functionality
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if OPChildKeeper is still required in block submodule
# Search for OPChildKeeper usage in block submodule implementation
rg -A 5 "OPChildKeeper" --type go
# Search for any recent changes to block submodule
git log -p -- "**/block/*" "**/block/**"
Length of output: 6087
Script:
#!/bin/bash
# Search for block submodule implementation and its usage of OPChildKeeper
ast-grep --pattern 'func NewBlockSubmodule($$$) {
$$$
}'
# Also search for any direct references to OPChildKeeper in block-related files
rg -l "OPChildKeeper" --type go | grep -i "block"
Length of output: 124
Script:
#!/bin/bash
# Find and examine the block submodule implementation
fd blocksubmodule --type f --exec cat {}
# Also check the specific file where NewBlockSubmodule is used
cat app/indexer.go
Length of output: 2960
Script:
#!/bin/bash
# Check the imports and implementation in the kvindexer repository
rg -p "github.com/initia-labs/kvindexer/submodules/block" --type go
# Also check for any direct references to OPChild in block-related files
rg -p "OPChild" --type go | grep -i block
Length of output: 314
app/test_helpers.go (2)
15-15: LGTM: Import additions are appropriate
The new imports are necessary for handling EVM types and mathematical operations in the test setup.
Also applies to: 26-26, 28-28
134-134: Consider reducing minimum gas price for tests
The minimum gas price of 1,000,000,000 seems quite high for a test environment. This might make testing expensive operations challenging. Consider using a lower value unless there's a specific reason for this high amount.
#!/bin/bash
# Description: Check if this gas price is consistent with other test files
# Look for other minimum gas price configurations in test files
rg -A 2 "MinGasPrices" --type go --glob "*test*.go"indexer/indexer.go (2)
54-56: LGTM: Clean interface extension
The addition of the Stop() method to the EVMIndexer interface is well-documented and properly placed.
66-67: Verify removal of opChildKeeper dependency
The removal of opChildKeeper field suggests a change in architecture. Let's verify this doesn't break existing functionality.
✅ Verification successful
Removal of opChildKeeper is safe and properly refactored
The removal of opChildKeeper is verified to be safe. The base fee calculation that previously depended on opChildKeeper.GetParams() has been refactored to use evmKeeper.BaseFee() instead. This is evidenced by:
- The
baseFeemethod that usedopChildKeeperhas been removed - Base fee calculation is now handled by the EVM keeper (seen in
x/evm/keeper/context_utils.go) - The indexer correctly uses
evmKeeper.BaseFee()for all base fee related operations (seen inindexer/abci.go)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining references to opChildKeeper
rg "opChildKeeper" --type go
# Search for any calls that might still pass opChildKeeper to NewEVMIndexer
ast-grep --pattern 'NewEVMIndexer($$$, opChildKeeper, $$$)'
Length of output: 90
Script:
#!/bin/bash
# Let's check git history to see the changes related to opChildKeeper
git log -p | grep -A 10 -B 10 "opChildKeeper"
# Search for any NewEVMIndexer constructor calls to see how it's being used
ast-grep --pattern 'NewEVMIndexer($$$)'
# Check the indexer package for any potential references
fd indexer.go --exec cat {}
Length of output: 5454
Script:
#!/bin/bash
# Let's check for any remaining usages of baseFee method since git history shows it used opChildKeeper
ast-grep --pattern 'baseFee($$$)'
# Check for any GetParams calls that might have replaced the opChildKeeper.GetParams
ast-grep --pattern 'GetParams($$$)'
# Look for any files that might contain the replacement logic for base fee calculation
rg "baseFee" -A 5 -B 5 --type go
Length of output: 12002
app/app.go (1)
607-609: LGTM: Clean implementation of the EVMIndexer getter
The method follows Go conventions and provides a clean interface to access the EVM indexer instance.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (11)
indexer/reader_test.go (3)
29-34: Improve Event Retrieval Logic for RobustnessAccessing the event using
events[len(events)-3]is brittle and may lead to issues if the number or order of events changes. Consider iterating over the events to find the one with the desired type to make the test more robust.You can refactor the code as follows:
events := finalizeRes.TxResults[0].Events -createEvent := events[len(events)-3] +var createEvent cmttypes.Event +found := false +for _, event := range events { + if event.Type == evmtypes.EventTypeContractCreated { + createEvent = event + found = true + break + } +} +require.True(t, found, "ContractCreated event not found") require.Equal(t, evmtypes.EventTypeContractCreated, createEvent.GetType()) contractAddr, err := hexutil.Decode(createEvent.Attributes[0].Value) require.NoError(t, err)
21-154: Consider SplittingTest_Readerinto Smaller Test FunctionsThe
Test_Readerfunction is lengthy and encompasses multiple logical test scenarios, which can make it harder to maintain and understand. Splitting it into smaller, focused test functions for each scenario would improve readability and ease of maintenance.For example:
- One test for contract creation and indexing.
- Another test for minting tokens and verifying transactions.
- Separate tests for block header indexing, transaction hash mapping, and receipt retrieval.
96-97: DeclarecountVariables Within Their ScopeThe variable
countis reused in multiple contexts. To prevent potential issues and improve clarity, declarecountwithin the scope of each iteration.You can modify the code as follows:
- count := 0 + var count int err = indexer.IterateBlockTxs(ctx, uint64(finalizeReq.Height), func(tx *rpctypes.RPCTransaction) (bool, error) { + count = 0 count++And
- count = 0 + var count int err = indexer.IterateBlockTxReceipts(ctx, uint64(finalizeReq.Height), func(receipt *coretypes.Receipt) (bool, error) { + count = 0 count++Alternatively, declare
countinside each callback function to limit its scope.Also applies to: 120-121
indexer/mempool_test.go (3)
16-19: Consider testing block and logs channel functionalityThe test immediately closes
blockChanandlogsChanwithout testing their functionality. Consider adding test cases for these channels to ensure comprehensive coverage of the subscription mechanism.
34-38: Consider cleaning up the query contextThe test creates a query context but doesn't clean it up. Consider using
deferto ensure proper cleanup of any resources associated with the context.
40-43: Consider adding more comprehensive verificationThe test only verifies the transaction hash. Consider adding more assertions to verify other transaction properties (e.g., sender, recipient, value) to ensure complete functionality of the mempool wrapper.
indexer/reader.go (1)
56-56: Fix the documentation comment.The comment "IterateBlockTxs implements EVMIndexer" is incorrect for this method. It should be "IterateBlockTxReceipts implements EVMIndexer" to match the actual method name.
-// IterateBlockTxs implements EVMIndexer. +// IterateBlockTxReceipts implements EVMIndexer.indexer/tx_helper_test.go (2)
29-51: Add input validation and optimize memory allocation.Consider the following improvements:
- Validate the input parameter
nto prevent negative values or excessive allocations- Document the reason for using 64 bytes of randomness
- Pre-allocate the
randBytesbuffer outside the loopfunc generateKeys(t *testing.T, n int) ([]common.Address, []*ecdsa.PrivateKey) { + require.Greater(t, n, 0, "number of keys must be positive") + require.Less(t, n, 1000, "excessive number of keys requested") // adjust limit as needed + addrs := make([]common.Address, n) privKeys := make([]*ecdsa.PrivateKey, n) + randBytes := make([]byte, 64) // 64 bytes for strong entropy for i := 0; i < n; i++ { - randBytes := make([]byte, 64) _, err := rand.Read(randBytes) require.NoError(t, err)
137-169: Enhance transaction execution validation and error reporting.The transaction execution helpers could be improved in several ways:
- Add validation for the transactions slice
- Handle edge cases in height calculation
- Provide more detailed error information
func executeTxs(t *testing.T, app *minitiaapp.MinitiaApp, txs ...sdk.Tx) (*abcitypes.RequestFinalizeBlock, *abcitypes.ResponseFinalizeBlock) { + require.NotNil(t, app, "app cannot be nil") + require.NotEmpty(t, txs, "at least one transaction required") + txsBytes := make([][]byte, len(txs)) for i, tx := range txs { + require.NotNil(t, tx, "transaction cannot be nil") txBytes, err := app.TxConfig().TxEncoder()(tx) require.NoError(t, err) txsBytes[i] = txBytes } + height := app.LastBlockHeight() + require.GreaterOrEqual(t, height, int64(0), "invalid block height") + finalizeReq := &abcitypes.RequestFinalizeBlock{ Txs: txsBytes, - Height: app.LastBlockHeight() + 1, + Height: height + 1, }func checkTxResult(t *testing.T, txResult *abcitypes.ExecTxResult, expectSuccess bool) { + require.NotNil(t, txResult, "transaction result cannot be nil") + if expectSuccess { - require.Equal(t, abcitypes.CodeTypeOK, txResult.Code) + require.Equal(t, abcitypes.CodeTypeOK, txResult.Code, + "transaction failed with code %d: %s", txResult.Code, txResult.Log) } else { - require.NotEqual(t, abcitypes.CodeTypeOK, txResult.Code) + require.NotEqual(t, abcitypes.CodeTypeOK, txResult.Code, + "transaction unexpectedly succeeded") + t.Logf("Expected failure - code: %d, log: %s", txResult.Code, txResult.Log) } }indexer/indexer.go (2)
183-186: Consider comprehensive cleanup in Stop method.While stopping the
txPendingMapis good, consider also:
- Closing subscription channels (blockChans, logsChans, pendingChans)
- Cleaning up DB connections if necessary
Here's a suggested implementation:
func (e *EVMIndexerImpl) Stop() { e.txPendingMap.Stop() + // Close all subscription channels + for _, ch := range e.blockChans { + close(ch) + } + for _, ch := range e.logsChans { + close(ch) + } + for _, ch := range e.pendingChans { + close(ch) + } + // Clear channel slices + e.blockChans = nil + e.logsChans = nil + e.pendingChans = nil + + // Close DB if needed + if e.db != nil { + _ = e.db.Close() + } }
Line range hint
127-131: Consider making the pending tx TTL configurable.The TTL for pending transactions is hardcoded to 1 minute. Consider making this configurable through the EVMKeeper config, similar to how
IndexerCacheSizeis handled.Example implementation:
// Use ttlcache to cope with abnormal cases like tx not included in a block txPendingMap: ttlcache.New( // pending tx lifetime is 1 minute in indexer - ttlcache.WithTTL[common.Hash, *rpctypes.RPCTransaction](time.Minute), + ttlcache.WithTTL[common.Hash, *rpctypes.RPCTransaction](cfg.IndexerPendingTxTTL), ),
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (8)
app/app.go(1 hunks)indexer/abci_test.go(1 hunks)indexer/indexer.go(5 hunks)indexer/mempool_test.go(1 hunks)indexer/reader.go(1 hunks)indexer/reader_test.go(1 hunks)indexer/tx_helper_test.go(1 hunks)jsonrpc/backend/tx.go(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- jsonrpc/backend/tx.go
🚧 Files skipped from review as they are similar to previous changes (2)
- app/app.go
- indexer/abci_test.go
🔇 Additional comments (6)
indexer/mempool_test.go (2)
20-20: Verify the generateCreateERC20Tx helper function implementation
The test relies on the generateCreateERC20Tx helper function. Let's verify its implementation to ensure proper transaction generation.
✅ Verification successful
Helper function implementation looks correct and follows expected pattern
The generateCreateERC20Tx helper function implementation is verified and follows a consistent pattern:
- Creates a query context
- Gets ERC20 factory address from EVMKeeper
- Packs ABI data for "createERC20" with proper parameters
- Delegates to common
generateTxhelper for final transaction creation
The implementation aligns with other transaction generation helpers like generateMintERC20Tx and generateTransferERC20Tx that follow the same pattern of ABI packing and generateTx delegation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for generateCreateERC20Tx function implementation
ast-grep --pattern 'func generateCreateERC20Tx($$$) ($$$) {
$$$
}'
# Search for any related transaction generation functions
rg -A 10 'func generate.*Tx'
Length of output: 3789
13-14: Verify the setupIndexer helper function implementation
The test relies on the setupIndexer helper function. Let's verify its implementation to ensure proper test environment setup.
✅ Verification successful
The search results show two different setupIndexer functions:
- In
indexer/indexer_test.go: A test helper that creates a minimal setup with just the EVM indexer - In
app/indexer.go: A more comprehensive setup function for production use
The setupIndexer used in mempool_test.go is the one from indexer/indexer_test.go, which is appropriate for testing purposes. Let's verify the createApp helper it depends on to ensure complete understanding.
Test setup and cleanup implementation is appropriate
The test setup is well-implemented with proper test helpers:
setupIndexercreates a minimal test environment with an in-memory databasecreateAppinitializes the app with test accounts and balances- The
defer app.Close()ensures proper cleanup
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for setupIndexer function implementation
ast-grep --pattern 'func setupIndexer($$$) ($$$) {
$$$
}'
# Search for any related test setup functions
rg -A 10 'func setup.*Indexer'
Length of output: 5566
Script:
#!/bin/bash
# Search for createApp function implementation
ast-grep --pattern 'func createApp($$$) ($$$) {
$$$
}'
# Also search for any direct references to ensure we're looking at the right one
rg 'createApp.*MinitiaApp'
Length of output: 1693
indexer/reader.go (1)
Line range hint 56-65: LGTM! Implementation looks solid.
The method implementation is well-structured with proper:
- Error handling for receipt retrieval
- Use of the collections API for iteration
- Consistent implementation pattern with other methods in the file
indexer/indexer.go (3)
66-67: LGTM: Improved type safety with explicit CacheStore typing.
The explicit typing of store as *CacheStore enhances type safety and code clarity.
38-38: Method rename improves clarity.
The spelling correction from IterateBlockTxRecepts to IterateBlockTxReceipts enhances code clarity.
Let's verify if there are any remaining occurrences of the old spelling:
✅ Verification successful
Method rename was properly implemented across the codebase
The spelling correction from IterateBlockTxRecepts to IterateBlockTxReceipts has been consistently applied across all relevant files:
- Interface definition in
indexer/indexer.go - Implementation in
indexer/reader.go - Tests in
indexer/reader_test.go - Usage in
jsonrpc/backend/tx.go
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining instances of the old method name
rg "IterateBlockTxRecepts"
Length of output: 26
Script:
#!/bin/bash
# Let's verify both the old and new method names across the codebase
# First, check for any implementations or usages of the new method name
echo "Checking for IterateBlockTxReceipts:"
rg "IterateBlockTxReceipts" -A 2
# Check git history for the rename to ensure it was a complete change
echo -e "\nChecking git diff for the rename:"
git diff HEAD~1 HEAD | grep -E "(\+|\-).*(IterateBlockTxRecepts|IterateBlockTxReceipts)"
Length of output: 2248
54-56: Breaking change: New interface method.
The addition of the Stop() method is a breaking change that requires all implementations of EVMIndexer to be updated.
Let's verify all implementations:
There was a problem hiding this comment.
Actionable comments posted: 18
🧹 Outside diff range and nitpick comments (56)
jsonrpc/backend/web3_test.go (1)
9-17: Add test documentation and error scenarios.While the test implementation is correct, consider these improvements:
- Add a brief comment describing the test's purpose and expectations
- Add test cases for error scenarios (e.g., RPC failures)
+// Test_ClientVersion verifies that the backend correctly returns +// the client version from the underlying RPC client func Test_ClientVersion(t *testing.T) { - test := setupBackend(t) - backend, cometRPC := test.backend, test.cometRPC + t.Run("success case", func(t *testing.T) { + test := setupBackend(t) + backend, cometRPC := test.backend, test.cometRPC - cometRPC.ClientVersion = "v0.6.2" - version, err := backend.ClientVersion() - require.NoError(t, err) - require.Equal(t, "v0.6.2", version) + cometRPC.ClientVersion = "v0.6.2" + version, err := backend.ClientVersion() + require.NoError(t, err) + require.Equal(t, "v0.6.2", version) + }) + + t.Run("error case", func(t *testing.T) { + test := setupBackend(t) + backend, cometRPC := test.backend, test.cometRPC + + // Simulate RPC failure + cometRPC.SetClientVersionError(errors.New("rpc error")) + _, err := backend.ClientVersion() + require.Error(t, err) + }) }jsonrpc/backend/net_test.go (2)
13-15: Consider making version check more maintainable.The hardcoded version "1" might become outdated. Consider:
- Using a constant for the version number
- Adding test cases for invalid versions
22-25: Enhance test coverage and remove redundant type conversion.Two suggestions for improvement:
- Add test cases for edge cases (0 peers, maximum peers)
- Remove unnecessary uint conversion if NPeers is already uint
-require.Equal(t, uint(10), uint(peerCount)) +require.Equal(t, 10, peerCount)jsonrpc/backend/backend_test.go (2)
23-30: Consider adding field documentation.The struct is well-designed and contains all necessary components for testing. However, adding documentation for each field would improve maintainability.
Consider adding documentation like this:
type testInput struct { + // app is the MinitiaApp instance used for testing app *minitiaapp.MinitiaApp + // indexer is the EVM indexer instance indexer indexer.EVMIndexer + // backend is the JSON-RPC backend being tested backend *backend.JSONRPCBackend + // addrs contains the Ethereum addresses for testing addrs []common.Address + // privKeys contains the corresponding private keys privKeys []*ecdsa.PrivateKey + // cometRPC is a mock RPC client for testing cometRPC *tests.MockCometRPC }
32-61: Consider adding cleanup mechanism.The setup function creates various resources but doesn't provide a mechanism for cleanup. Consider returning a cleanup function to ensure proper resource management.
Example implementation:
-func setupBackend(t *testing.T) testInput { +func setupBackend(t *testing.T) (testInput, func()) { app, addrs, privKeys := tests.CreateApp(t) indexer := app.EVMIndexer() // ... rest of the setup code ... - return testInput{ + ti := testInput{ app: app, indexer: indexer, backend: backend, addrs: addrs, privKeys: privKeys, cometRPC: mockCometRPC, } + cleanup := func() { + if err := backend.Close(); err != nil { + t.Logf("failed to cleanup backend: %v", err) + } + if err := app.Close(); err != nil { + t.Logf("failed to cleanup app: %v", err) + } + } + return ti, cleanup }tests/app_creator.go (1)
28-31: Enhance error message for balance mismatchThe current assertion could be more informative when it fails. Consider using
require.Equalwith a descriptive message.- require.True(t, balances.Equal(app.BankKeeper.GetAllBalances(ctxCheck, addr.Bytes()))) + actualBalance := app.BankKeeper.GetAllBalances(ctxCheck, addr.Bytes()) + require.Equal(t, balances.String(), actualBalance.String(), + "balance mismatch for address %s", addr.String())app/ante/fee.go (2)
52-60: LGTM with a suggestion for error handlingThe changes improve gas metering control by replacing the infinite gas meter with a fixed limit. The gas meter restoration is properly handled.
Consider adding more specific error handling for gas out-of-memory scenarios:
ctx, err = fd.inner.AnteHandle(ctx.WithGasMeter(storetypes.NewGasMeter(gasLimitForFeeDeduction)), tx, simulate, noopAnteHandler) // restore the original gas meter ctx = ctx.WithGasMeter(gasMeter) if err != nil { + if errors.Is(err, storetypes.ErrGasOverflow) { + return ctx, errorsmod.Wrapf(sdkerrors.ErrOutOfGas, + "fee deduction requires too much gas (limit: %d)", gasLimitForFeeDeduction) + } return ctx, err }
62-64: Add documentation for the noopAnteHandlerThe implementation is correct, but consider adding a docstring to explain:
- The purpose of this handler
- Why it's needed in the fee deduction flow
- When it's used instead of the regular next handler
+// noopAnteHandler is a no-operation ante handler used during fee deduction to skip +// the execution of the next handler in the chain, as fee deduction is handled separately. func noopAnteHandler(ctx sdk.Context, _ sdk.Tx, _ bool) (sdk.Context, error) { return ctx, nil }jsonrpc/backend/filters_test.go (3)
16-19: Add test documentation.Consider adding a doc comment explaining the test's purpose, setup requirements, and expected outcomes.
+// Test_GetLogsByHeight validates the GetLogsByHeight functionality by: +// 1. Creating an ERC20 contract +// 2. Performing token minting operations +// 3. Verifying that retrieved logs match block receipts func Test_GetLogsByHeight(t *testing.T) {
24-26: Avoid magic numbers in event access.The index
len(events)-3is brittle and could break if the event order changes.Consider either:
- Using a helper function to find the event by type
- Adding a constant or comment explaining why this specific index is used
-createEvent := events[len(events)-3] +// Helper function approach +createEvent := findEventByType(events, evmtypes.EventTypeContractCreated)
31-37: Document the purpose of multiple minting transactions.The test creates two separate minting transactions with large token amounts. Consider adding comments explaining:
- Why two separate transactions are needed
- The significance of the token amount (1_000_000_000_000)
indexer/mempool.go (2)
Line range hint
39-45: Improve error handling in Insert methodThe method continues execution after logging the conversion error. Consider whether it's appropriate to proceed with mempool insertion if EVM conversion fails.
if err != nil { m.indexer.logger.Error("failed to convert CosmosTx to EthTx", "err", err) - return err + // Either return early here to prevent insertion of invalid transactions + return fmt.Errorf("failed to convert to EthTx: %w", err) + // Or add a comment explaining why we continue despite the error }
80-83: Add documentation for Inner() methodThe method implementation looks good, but could benefit from more detailed documentation explaining its purpose and usage scenarios.
-// Inner returns the inner mempool. +// Inner returns the underlying mempool implementation. +// This method is primarily intended for testing and advanced usage scenarios +// where direct access to the base mempool implementation is required. func (m *MempoolWrapper) Inner() mempool.Mempool {app/ante/fee_test.go (1)
Line range hint
14-75: Consider refactoring to improve test maintainabilityWhile the test is functional, consider these improvements:
- Convert to table-driven tests to reduce code duplication across cases
- Extract magic numbers (200_000, 500000) into named constants
Example refactor:
func (suite *AnteTestSuite) Test_NotSpendingGasForTxWithFeeDenom() { const ( defaultGasLimit = 200_000 testGasMeter = 500000 ) testCases := []struct { name string feeAmount sdk.Coins expectZeroGas bool }{ { name: "only fee denom", feeAmount: sdk.NewCoins(sdk.NewCoin(feeDenom, math.NewInt(100))), expectZeroGas: true, }, // ... other test cases } for _, tc := range testCases { suite.Run(tc.name, func() { // ... test setup gasMeter := storetypes.NewGasMeter(testGasMeter) _, err = feeAnte.AnteHandle(suite.ctx.WithGasMeter(gasMeter), tx, false, noopAnteHandler) suite.Require().NoError(err) if tc.expectZeroGas { suite.Require().Zero(gasMeter.GasConsumed()) } else { suite.Require().NotZero(gasMeter.GasConsumed()) } }) } }jsonrpc/backend/feehistory_test.go (5)
27-29: Consider making event validation more robust.The current event validation relies on a hardcoded index (
len(events)-3), which could break if the event order changes. Consider:
- Finding the event by type instead of index
- Validating all relevant event attributes
-createEvent := events[len(events)-3] -require.Equal(t, evmtypes.EventTypeContractCreated, createEvent.GetType()) +var createEvent sdk.Event +for _, event := range events { + if event.Type == evmtypes.EventTypeContractCreated { + createEvent = event + break + } +} +require.NotNil(t, createEvent, "ContractCreated event not found") +require.Greater(t, len(createEvent.Attributes), 0, "Event attributes missing")
34-35: Consider documenting the token amount or using a constant.The minted amount (1_000_000_000_000) is a magic number. Consider:
- Adding a comment explaining the choice of this amount
- Using a named constant for better maintainability
+// Initial token supply: 1M tokens with 6 decimals +const initialTokenSupply = 1_000_000_000_000 tx, _ = tests.GenerateMintERC20Tx(t, app, privKeys[0], common.BytesToAddress(contractAddr), addrs[0], - new(big.Int).SetUint64(1_000_000_000_000)) + new(big.Int).SetUint64(initialTokenSupply))
47-52: Consider extracting gas calculations into helper functions.The gas and fee calculations are complex and would benefit from being extracted into helper functions with clear documentation.
+// calculateGasFees returns the gas limit, base fee, tip cap, and fee cap based on minimum gas price +func calculateGasFees(minGasPrice sdk.DecCoin) (gasLimit uint64, baseFee, gasTipCap, gasFeeCap *big.Int) { + gasLimit = uint64(1_000_000) + baseFee = minGasPrice.Amount.TruncateInt().BigInt() + baseFeeCap := new(big.Int).Mul(baseFee, big.NewInt(int64(gasLimit))) + gasTipCap = new(big.Int).Mul(minGasPrice.Amount.TruncateInt().BigInt(), big.NewInt(1_000)) + gasFeeCap = new(big.Int).Add(baseFeeCap, gasTipCap) + return +}
53-57: Consider improving transaction generation code.Several improvements could make the code more maintainable:
- The hardcoded nonce (2) should be retrieved dynamically
- The number of transactions (5) should be a named constant
- Consider using a helper function for batch transaction generation
+const numTestTransactions = 5 + +// getCurrentNonce retrieves the current nonce for the account +func getCurrentNonce(app *MinitiaApp, addr common.Address) (uint64, error) { + // Implementation to get current nonce +} + -nonce := 2 -txs := make([]sdk.Tx, 5) +nonce, err := getCurrentNonce(app, addrs[0]) +require.NoError(t, err) +txs := make([]sdk.Tx, numTestTransactions)
67-75: Consider enhancing fee history validation.The current test could be more comprehensive:
- Add test cases for edge cases (empty blocks, max gas used)
- Document why 40, 60 percentiles were chosen
- Test with different block ranges
Consider creating a test table:
testCases := []struct { name string blockCount uint64 percentiles []float64 expected struct { reward [][]*big.Int baseFee []*big.Int gasUsedRatio []float64 blobBaseFee []*big.Int blobGasUsedRatio []float64 } }{ { name: "current implementation", blockCount: 1, percentiles: []float64{40, 60}, // ... current expected values }, // Add more test cases }jsonrpc/backend/gas_test.go (7)
21-21: Clean up unused variables in the assignment.Several variables from the tuple assignment are unused.
-app, _, backend, _, _ := input.app, input.addrs, input.backend, input.addrs, input.privKeys +app, backend := input.app, input.backend
30-30: Document the purpose of the sleep and consider making it configurable.The hard-coded sleep duration could make tests flaky and its purpose is unclear. Consider:
- Adding a comment explaining why this delay is necessary
- Making the duration configurable via a constant
- Using a more deterministic approach if possible
+// Sleep to ensure the backend has synchronized with the latest state +const gasQueryDelay = 3*time.Second + 500*time.Millisecond -time.Sleep(3*time.Second + 500*time.Millisecond) +time.Sleep(gasQueryDelay)
39-39: Clean up unused variables in the assignment.Similar to the previous test, several variables from the tuple assignment are unused.
-_, _, backend, _, _ := input.app, input.addrs, input.backend, input.addrs, input.privKeys +backend := input.backend
61-64: Extract magic numbers into named constants.The token amounts and other numeric values should be constants with meaningful names for better maintainability and readability.
+const ( + tokenDecimals = 12 + mintAmount = 1_000_000_000_000 // 1M tokens with 12 decimals + transferAmount = 1_000_000 // 0.001 tokens with 12 decimals +) -new(big.Int).SetUint64(1_000_000_000_000) +new(big.Int).SetUint64(mintAmount) -new(big.Int).SetUint64(1_000_000) +new(big.Int).SetUint64(transferAmount)
87-88: Extract gas thresholds into named constants.The gas threshold values should be constants with meaningful names.
+const ( + minExpectedGas = 10_000 +) -require.Greater(t, gasEstimated, uint64(10_000)) +require.Greater(t, gasEstimated, uint64(minExpectedGas), "estimated gas too low")
76-76: Document the purpose of the sleep and consider making it configurable.Similar to the previous test, the sleep duration should be documented and potentially made configurable.
+// Sleep to ensure the state is synchronized before estimation +const estimateGasDelay = 3 * time.Second -time.Sleep(3 * time.Second) +time.Sleep(estimateGasDelay)
19-89: Overall approval with suggestions for test stability.The test suite comprehensively covers gas-related functionality. However, consider these architectural improvements:
- Create a test helper package for common ERC20 test operations
- Implement a more deterministic way to wait for state synchronization instead of using sleep
- Consider table-driven tests for different gas estimation scenarios
jsonrpc/backend/cosmos_test.go (1)
34-35: Consider using constants for token amounts.The token amounts (1_000_000_000_000 and 1_000_000) should be defined as named constants to improve readability and maintainability.
+const ( + initialMintAmount = 1_000_000_000_000 + transferAmount = 1_000_000 +) -tx, evmTxHash := tests.GenerateMintERC20Tx(t, app, privKeys[0], common.BytesToAddress(contractAddr), addrs[0], new(big.Int).SetUint64(1_000_000_000_000)) -tx2, evmTxHash2 := tests.GenerateTransferERC20Tx(t, app, privKeys[0], common.BytesToAddress(contractAddr), addrs[1], new(big.Int).SetUint64(1_000_000), tests.SetNonce(2)) +tx, evmTxHash := tests.GenerateMintERC20Tx(t, app, privKeys[0], common.BytesToAddress(contractAddr), addrs[0], new(big.Int).SetUint64(initialMintAmount)) +tx2, evmTxHash2 := tests.GenerateTransferERC20Tx(t, app, privKeys[0], common.BytesToAddress(contractAddr), addrs[1], new(big.Int).SetUint64(transferAmount), tests.SetNonce(2))app/ante/ante_test.go (1)
138-140: Add documentation for the noopAnteHandler function.While the implementation is correct, please add a doc comment explaining the purpose and usage of this test utility function.
+// noopAnteHandler returns a no-operation ante handler that simply returns the input context +// and nil error. It's used in tests where the ante handler behavior is not relevant. func noopAnteHandler(ctx sdk.Context, _ sdk.Tx, _ bool) (sdk.Context, error) { return ctx, nil }tests/mock_comet.go (1)
118-188: Improve documentation and error messages for unimplemented methods.The "unused methods" section would benefit from:
- Documentation explaining why these methods are left unimplemented
- More descriptive panic messages indicating these are intentionally unimplemented mock methods
Consider this improvement:
-// unused methods +// Unimplemented methods - these methods are not required for the current test scenarios +// and will panic if called. If you need any of these methods, implement them as needed. func (m *MockCometRPC) DumpConsensusState(context.Context) (*ctypes.ResultDumpConsensusState, error) { - panic("implement me") + panic("DumpConsensusState: intentionally unimplemented mock method") }Apply similar changes to all unimplemented methods.
jsonrpc/backend/block_test.go (8)
20-20: Clean up unused variables in tuple assignment.Several variables from the tuple assignment are unused. Consider using the blank identifier
_for unused values.-app, _, backend, _, _ := input.app, input.addrs, input.backend, input.addrs, input.privKeys +app, backend := input.app, input.backend
29-29: Clean up unused variables in tuple assignment.Several variables from the tuple assignment are unused.
-app, _, backend, _, privKeys := input.app, input.addrs, input.backend, input.addrs, input.privKeys +app, backend, privKeys := input.app, input.backend, input.privKeys
75-75: Clean up unused variables in tuple assignment.Several variables from the tuple assignment are unused.
-app, _, backend, _, privKeys := input.app, input.addrs, input.backend, input.addrs, input.privKeys +app, backend, privKeys := input.app, input.backend, input.privKeys
73-87: Add test case for non-existent block hash.The test should also verify behavior when querying with a non-existent block hash.
Consider adding:
nonExistentHash := common.HexToHash("0x1234") header, err := backend.GetHeaderByHash(nonExistentHash) require.NoError(t, err) require.Nil(t, header)
98-99: Avoid magic numbers in array index.The index
-3is a magic number. Consider extracting it to a named constant or adding a comment explaining why this specific index is used.
104-105: Fix misleading comment.The comment mentions minting 1_000_000 tokens but the actual value is 1_000_000_000_000.
-// mint 1_000_000 tokens to the first address +// mint 1_000_000_000_000 tokens to the first address
89-131: Add test cases for special block numbers.The test should verify behavior with special block numbers (latest, pending, safe, finalized) similar to Test_GetHeaderByNumber.
1-175: Consider reorganizing tests for better maintainability.The test file could benefit from better organization:
- Group related test cases using subtests (t.Run)
- Consider table-driven tests for special block numbers
- Add benchmark tests for performance-critical operations
Example structure:
func TestBlockQueries(t *testing.T) { t.Run("BlockNumber", func(t *testing.T) {...}) t.Run("GetHeaderByNumber", func(t *testing.T) { testCases := []struct{ name string blockNum rpc.BlockNumber wantErr bool }{ {"latest block", rpc.LatestBlockNumber, false}, {"pending block", rpc.PendingBlockNumber, false}, // ... more cases } for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) {...}) } }) // ... more test groups }jsonrpc/backend/eth_test.go (6)
20-42: Consider improving test reliability and coverageA few suggestions to enhance this test:
- Replace the hard-coded sleep with a more reliable mechanism to wait for the feeFetcher
- Consider extracting magic numbers (1_000_000_000_000_000_000) into named constants
- Add test cases for error scenarios (invalid addresses, non-existent blocks)
+const ( + oneGasInWei = 1_000_000_000_000_000_000 +) func Test_GetBalance(t *testing.T) { input := setupBackend(t) app, _, backend, _, privKeys := input.app, input.addrs, input.backend, input.addrs, input.privKeys // send 1GAS to newly created account addr := common.Address{1, 2, 3} - tx, _ := tests.GenerateTx(t, app, privKeys[0], &addr, nil, big.NewInt(1_000_000_000_000_000_000)) + tx, _ := tests.GenerateTx(t, app, privKeys[0], &addr, nil, big.NewInt(oneGasInWei)) _, finalizeRes := tests.ExecuteTxs(t, app, tx) tests.CheckTxResult(t, finalizeRes.TxResults[0], true) - // wait feeFetcher interval - time.Sleep(3 * time.Second) + // TODO: Replace with a more reliable mechanism to wait for the feeFetcher + // For example: polling with timeout or using a channel-based notification // Test cases... + + // Add error cases + t.Run("invalid address", func(t *testing.T) { + _, err := backend.GetBalance(common.Address{}, rpc.BlockNumberOrHashWithNumber(-1)) + require.Error(t, err) + }) }
44-100: Enhance test robustness and readabilityConsider the following improvements:
- Extract magic numbers for token amounts into named constants
- Add more assertions for contract deployment events
- Verify events emitted during token transfer
+const ( + initialMintAmount = 1_000_000_000_000 + transferAmount = 1_000_000 +) func Test_Call(t *testing.T) { // ... existing setup ... - tx, _ = tests.GenerateMintERC20Tx(t, app, privKeys[0], contractEVMAddr, addrs[0], new(big.Int).SetUint64(1_000_000_000_000)) - tx2, _ := tests.GenerateTransferERC20Tx(t, app, privKeys[0], contractEVMAddr, addrs[1], new(big.Int).SetUint64(1_000_000), tests.SetNonce(2)) + tx, _ = tests.GenerateMintERC20Tx(t, app, privKeys[0], contractEVMAddr, addrs[0], new(big.Int).SetUint64(initialMintAmount)) + tx2, _ := tests.GenerateTransferERC20Tx(t, app, privKeys[0], contractEVMAddr, addrs[1], new(big.Int).SetUint64(transferAmount), tests.SetNonce(2)) _, finalizeRes = tests.ExecuteTxs(t, app, tx, tx2) tests.CheckTxResult(t, finalizeRes.TxResults[0], true) tests.CheckTxResult(t, finalizeRes.TxResults[1], true) + // Verify transfer events + transferEvent := finalizeRes.TxResults[1].Events[len(finalizeRes.TxResults[1].Events)-1] + require.Equal(t, evmtypes.EventTypeTransfer, transferEvent.GetType())
102-155: Improve storage test documentation and structureThe test thoroughly verifies storage slots but could benefit from:
- Moving storage slot documentation into constants with clear descriptions
- Creating a helper function to reduce the repetitive storage verification code
+// ERC20 storage layout +const ( + // Slot 0: Owner address + ownerSlot = "00" + // Slot 3: Contract name + nameSlot = "03" + // Slot 4: Contract symbol + symbolSlot = "04" + // Slot 5: Decimals + decimalsSlot = "05" + // Slot 6: Total supply + totalSupplySlot = "06" +) + +// verifyStorageSlot is a helper function to check storage values +func verifyStorageSlot(t *testing.T, backend Backend, addr common.Address, slot string, expected []byte) { + slotHash := common.HexToHash(slot) + retBz, err := backend.GetStorageAt(addr, slotHash, rpc.BlockNumberOrHashWithNumber(-1)) + require.NoError(t, err) + require.Equal(t, expected, []byte(retBz)) +} func Test_StorageAt(t *testing.T) { // ... existing setup ... - slot := common.HexToHash("03") - retBz, err := backend.GetStorageAt(common.BytesToAddress(contractAddr), slot, rpc.BlockNumberOrHashWithNumber(-1)) - require.NoError(t, err) - require.Equal(t, common.Hex2Bytes("666f6f0000000000000000000000000000000000000000000000000000000006"), []byte(retBz)) + verifyStorageSlot(t, backend, common.BytesToAddress(contractAddr), nameSlot, + common.Hex2Bytes("666f6f0000000000000000000000000000000000000000000000000000000006"))
157-181: Add documentation and error cases for code verificationThe test should:
- Document the byte manipulation logic used to extract runtime code
- Add test cases for non-existent contracts
func Test_Code(t *testing.T) { // ... existing setup ... + // The EVM contract bytecode consists of two parts: + // 1. Initialization code that runs during contract creation + // 2. Runtime code that is stored on the blockchain + // The sequence 5ff3fe is used to separate these parts erc20Code := hexutil.MustDecode(erc20.Erc20MetaData.Bin) initCodeOP := common.Hex2Bytes("5ff3fe") initCodePos := bytes.Index(erc20Code, initCodeOP) erc20RuntimeCode := erc20Code[initCodePos+3:] require.True(t, bytes.Equal(erc20RuntimeCode, code)) + // Test non-existent contract + t.Run("non-existent contract", func(t *testing.T) { + code, err := backend.GetCode(common.Address{1}, rpc.BlockNumberOrHashWithNumber(-1)) + require.NoError(t, err) + require.Empty(t, code) + }) }
183-192: Add documentation and error cases for chain ID testThe test is concise but could be enhanced by:
- Adding comments explaining the chain ID conversion logic
- Testing with various chain ID formats
func Test_ChainID(t *testing.T) { input := setupBackend(t) app, _, backend, _, _ := input.app, input.addrs, input.backend, input.addrs, input.privKeys + // Test successful conversion + t.Run("valid chain ID", func(t *testing.T) { chainID, err := backend.ChainId() require.NoError(t, err) + // Convert Cosmos chain ID to Ethereum chain ID + // This conversion ensures compatibility with EVM-based tools and clients evmChainID := evmtypes.ConvertCosmosChainIDToEthereumChainID(app.ChainID()) require.Equal(t, evmChainID, chainID.ToInt()) + }) + + // Test with different chain ID formats + t.Run("different chain ID formats", func(t *testing.T) { + testCases := []struct { + name string + chainID string + expectErr bool + }{ + {"mainnet", "initia_7777-1", false}, + {"testnet", "initia_7777-2", false}, + // Add more test cases + } + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + // Test chain ID conversion + }) + } + }) }
1-192: Overall feedback on the test suiteThe test suite provides good coverage of the Ethereum backend functionality. The tests are well-structured and cover essential aspects of the system. However, consider the following general improvements:
- Add more error cases across all tests
- Extract common test setup code into helper functions
- Add more documentation for complex operations
- Consider using table-driven tests where appropriate
jsonrpc/backend/feehistory.go (1)
231-233: TODO: Implement dynamic base fee calculationThe current implementation uses a static base fee approach, which might affect fee estimation accuracy. While this simplification works for now, we should track the need for proper dynamic base fee calculation.
Would you like me to:
- Create a GitHub issue to track the implementation of dynamic base fee calculation?
- Provide a more detailed comment explaining the current limitations and future requirements?
jsonrpc/namespaces/eth/filters/api.go (3)
Line range hint
129-177: Consider implementing graceful shutdown for filter subscriptionsThe
eventLoopmethod handles subscription management but lacks graceful shutdown handling for spawned goroutines. This could lead to goroutine leaks when the API is shut down.Consider implementing the following improvements:
- Add a WaitGroup to track all spawned goroutines
- Implement proper cleanup of all channels during shutdown
- Add documentation about the lifecycle management
type FilterAPI struct { + wg sync.WaitGroup // ... existing fields ... } func (api *FilterAPI) eventLoop() { + defer func() { + // Close all subscription channels + for _, s := range api.subscriptions { + close(s.err) + } + }() // ... rest of the method ... }
224-224: Enhance error handling with logging and cleanupThe error handling in filter goroutines could be improved with proper logging and resource cleanup.
Consider adding logging and cleanup:
- case <-s.err: // subscription is uninstalled + case <-s.err: + api.logger.Debug("filter subscription terminated", + "id", id, + "type", s.ty) + // Cleanup any remaining resources + api.filtersMut.Lock() + delete(api.filters, id) + api.filtersMut.Unlock() returnAlso applies to: 270-270, 348-348
129-130: Consider architectural improvements for production readinessWhile the implementation is functionally correct, consider the following improvements for production readiness:
- Context propagation for better resource management
- Backpressure handling for high-load scenarios
- Metrics for monitoring subscription lifecycle
Consider implementing:
- Channel buffer sizes based on expected load
- Metrics for:
- Active subscriptions count
- Event processing latency
- Channel buffer utilization
- Backpressure handling:
- Skip events if channels are full
- Log warnings when events are dropped
Example metrics implementation:
type FilterAPI struct { metrics struct { activeSubscriptions prometheus.Gauge eventProcessingLatency prometheus.Histogram droppedEvents prometheus.Counter } // ... existing fields ... }jsonrpc/backend/tx_test.go (4)
16-131: Test_SendRawTransaction: Consider adding error casesThe test thoroughly covers the happy path for transaction pool management, including pending and queued transactions. However, it lacks test cases for error scenarios.
Consider adding test cases for:
- Invalid transaction format
- Insufficient balance
- Invalid nonce (too high)
- Gas price below minimum
133-174: Test_GetTransactionCount: Add test for pending transactionsWhile the test covers transaction counts at different block heights and with block hash, it's missing a test case for
latestandpendingblock numbers.Add test cases using:
+count, err = backend.GetTransactionCount(common.BytesToAddress(addrs[0].Bytes()), rpc.BlockNumberOrHashWithNumber(rpc.LatestBlockNumber)) +require.NoError(t, err) +require.Equal(t, "0x3", count.String()) + +count, err = backend.GetTransactionCount(common.BytesToAddress(addrs[0].Bytes()), rpc.BlockNumberOrHashWithNumber(rpc.PendingBlockNumber)) +require.NoError(t, err) +require.Equal(t, "0x3", count.String())
345-411: Test_PendingTransactions: Add test for transaction removalThe test verifies pending transaction addition but doesn't verify transaction removal after execution.
Add a test case that:
- Executes pending transactions
- Verifies they are removed from the pending pool
- Verifies they appear in the block
413-455: Test_BlockReceipts: Consider testing failed transactionsThe test only covers successful transactions. Adding test cases for failed transactions would improve coverage.
Add test cases for:
- Failed contract creation
- Failed token transfer (insufficient balance)
- Failed transaction due to out of gas
tests/tx_helper.go (4)
35-40: Simplify key generation by usingrand.ReaderdirectlyCurrently, the code generates random bytes and creates a reader to pass to
ecdsa.GenerateKey. You can simplify this by usingrand.Readerdirectly, which is the standard and secure approach.Here's a diff to simplify the code:
- randBytes := make([]byte, 64) - _, err := rand.Read(randBytes) - require.NoError(t, err) - reader := bytes.NewReader(randBytes) - privKey, err := ecdsa.GenerateKey(crypto.S256(), reader) + privKey, err := ecdsa.GenerateKey(crypto.S256(), rand.Reader) require.NoError(t, err)
143-144: Parameterize token details inGenerateCreateInitiaERC20TxThe token name, symbol, and decimals are hardcoded as
"foo"and6. Consider accepting these values as parameters to enhance the flexibility of your tests.
159-160: Parameterize token details inGenerateCreateERC20TxCurrently, the token name and symbol are hardcoded as
"foo". To make the function more versatile for testing different tokens, consider passing the token name, symbol, and decimals as parameters.
211-217: Ensure error codes are properly checked inCheckTxResultThe function checks if
txResult.Codeequalsabcitypes.CodeTypeOK. To make the tests more robust, consider also checking for non-zeroGasUsedor verifying the transaction result data when successful.jsonrpc/backend/txpool_test.go (1)
148-249: Add comments to test functions to improve readability and maintainabilityThe test functions lack descriptive comments explaining their purpose and the scenarios they cover. Adding comments will enhance code readability and help future maintainers understand the test cases.
Consider adding comments like:
// Test_TxPoolStatus verifies that the TxPoolStatus method returns correct counts of pending and queued transactionsPlace similar comments at the beginning of each test function to summarize their intent.
Also applies to: 251-354
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (30)
.codecov.yml(1 hunks)app/ante/ante_test.go(2 hunks)app/ante/fee.go(2 hunks)app/ante/fee_test.go(2 hunks)app/app.go(2 hunks)app/const.go(0 hunks)indexer/abci_test.go(1 hunks)indexer/indexer.go(5 hunks)indexer/mempool.go(1 hunks)indexer/mempool_test.go(1 hunks)indexer/reader_test.go(1 hunks)jsonrpc/backend/backend_test.go(1 hunks)jsonrpc/backend/block_test.go(1 hunks)jsonrpc/backend/cosmos_test.go(1 hunks)jsonrpc/backend/eth_test.go(1 hunks)jsonrpc/backend/feehistory.go(1 hunks)jsonrpc/backend/feehistory_test.go(1 hunks)jsonrpc/backend/filters_test.go(1 hunks)jsonrpc/backend/gas_test.go(1 hunks)jsonrpc/backend/net_test.go(1 hunks)jsonrpc/backend/tx.go(3 hunks)jsonrpc/backend/tx_test.go(1 hunks)jsonrpc/backend/txpool_test.go(1 hunks)jsonrpc/backend/web3_test.go(1 hunks)jsonrpc/namespaces/eth/api.go(1 hunks)jsonrpc/namespaces/eth/filters/api.go(4 hunks)tests/app_creator.go(1 hunks)tests/mock_comet.go(1 hunks)tests/tx_helper.go(1 hunks)x/evm/keeper/erc20_stores_test.go(1 hunks)
💤 Files with no reviewable changes (1)
- app/const.go
✅ Files skipped from review due to trivial changes (1)
- x/evm/keeper/erc20_stores_test.go
🚧 Files skipped from review as they are similar to previous changes (6)
- app/app.go
- indexer/abci_test.go
- indexer/indexer.go
- indexer/mempool_test.go
- indexer/reader_test.go
- jsonrpc/backend/tx.go
🔇 Additional comments (35)
jsonrpc/backend/web3_test.go (1)
10-10: Verify the setupBackend helper function.
The test relies on a setupBackend helper function that's not visible in the provided code. Let's verify its implementation to ensure it properly initializes the test environment.
✅ Verification successful
The setupBackend helper function is well-implemented and properly initializes the test environment
The helper function implementation is found in backend_test.go and it:
- Creates a test app with addresses and private keys using
tests.CreateApp - Sets up necessary contexts (background, server, and client) with proper codecs and configurations
- Initializes a mock CometRPC instance
- Creates and returns a new JSONRPCBackend with all required dependencies
- Uses proper error handling with
require.NoError - Returns a comprehensive
testInputstruct with all necessary test components
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find and verify the setupBackend helper function implementation
# Expected: Find the helper function that sets up the test backend with proper mocks
# Search for the setupBackend function definition
ast-grep --pattern 'func setupBackend(t *testing.T) $_'
# Search for any test helper files that might contain the function
rg -t go "func setupBackend" --glob 'jsonrpc/backend/*_test.go'
Length of output: 2103
.codecov.yml (2)
40-40: LGTM! Standard practice for coverage configuration.
Adding tests/ to the ignore list is a common practice to prevent test files from artificially inflating coverage metrics while still maintaining accurate coverage reporting for production code.
40-40: Verify no production code exists in the tests directory.
While excluding test directories is standard, let's verify that no production code is accidentally placed in the tests/ directory.
✅ Verification successful
Let me check the content of these non-test files to determine if they contain production code or test utilities.
No production code found in tests directory - files contain test utilities only
After reviewing the content of the non-test files in the tests directory, I can confirm that these files contain only test utilities and mock implementations:
app_creator.go: Contains test helper functions for creating test applicationsmock_comet.go: Contains mock implementation of CometBFT interfaces for testingtx_helper.go: Contains test utilities for generating and executing transactions in tests
These files are appropriately placed in the tests directory as they are testing infrastructure code, not production code.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for potential production code in tests/ directory
# Look for suspicious patterns that might indicate production code
# Check for exported functions/types in tests/ (potential sign of production code)
rg -t go '^\s*export\s+' tests/ || echo "No exported symbols found in tests/"
# Look for imports of the tests/ directory (shouldn't be imported by production code)
rg -t go "\".*tests/.*\"" . --glob '!tests/**' || echo "No imports of tests/ found in production code"
# Check for non-test files in tests/ (files without _test.go suffix)
fd -e go -E '*_test.go' . tests/ || echo "No non-test Go files found in tests/"
Length of output: 357
Script:
#!/bin/bash
# Check the content of non-test files in tests/ directory
echo "=== Content of tests/app_creator.go ==="
cat tests/app_creator.go
echo -e "\n=== Content of tests/mock_comet.go ==="
cat tests/mock_comet.go
echo -e "\n=== Content of tests/tx_helper.go ==="
cat tests/tx_helper.go
Length of output: 14027
jsonrpc/backend/net_test.go (2)
1-8: LGTM! Package and imports are well-structured.
The package name and imports follow Go best practices with minimal dependencies.
10-10: Verify setupBackend helper function implementation.
All tests rely on the setupBackend helper function. Let's verify its implementation to ensure proper test initialization.
Also applies to: 19-20, 29-30
✅ Verification successful
setupBackend helper function is properly implemented and used correctly
The helper function implementation in backend_test.go shows a well-structured test setup that:
- Creates a test app with addresses and private keys
- Sets up the necessary context with codecs and registries
- Configures JSON-RPC with mock Comet RPC
- Returns a comprehensive test input struct with all required components
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for setupBackend function implementation
ast-grep --pattern 'func setupBackend($$$) $$$'
Length of output: 1953
jsonrpc/backend/backend_test.go (2)
1-21: LGTM! Package declaration and imports are well-organized.
The package follows Go conventions with the _test suffix, and imports are logically grouped.
34-34: Verify indexer initialization.
Based on the AI summary, there have been changes to the indexer setup in other files. Let's verify that the indexer initialization is consistent with these changes.
✅ Verification successful
Based on the search results, I can now provide a conclusive response about the indexer initialization verification:
EVMIndexer initialization is correctly implemented and consistent
The codebase shows a well-structured implementation where:
EVMIndexer()is properly defined inapp/app.goas a getter method- The indexer is initialized via
NewEVMIndexer()inapp/indexer.gowith all required dependencies - The initialization is properly integrated with the app setup through
SetEVMIndexer() - The implementation is consistent across all usages in test files and JSON-RPC backend
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify indexer initialization changes
# Check EVMIndexer method implementation
ast-grep --pattern 'func (app *MinitiaApp) EVMIndexer() indexer.EVMIndexer {
$$$
}'
# Check setupIndexer function changes
ast-grep --pattern 'func setupIndexer($$$) {
$$$
}'
Length of output: 143
Script:
#!/bin/bash
# Let's try a broader search to understand the indexer initialization
# Search for EVMIndexer references
rg "EVMIndexer" -A 3
# Search for indexer initialization patterns
rg "indexer\." -A 3
# Look for files containing indexer in their name
fd indexer
Length of output: 23181
tests/app_creator.go (4)
1-57: Overall implementation looks good!
The test utilities provide a solid foundation for the indexer tests mentioned in the PR objectives. The implementation includes proper setup of test accounts, balance verification, and state management.
51-54: Verify block height increment logic
The block finalization and commit sequence looks correct, but let's verify the height increment logic across the codebase.
✅ Verification successful
The search results show consistent usage of LastBlockHeight() + 1 pattern across the codebase for block height increments. Let's verify the finalization and commit sequence to ensure complete correctness:
Block height increment logic is correctly implemented
The verification confirms that:
- The pattern
LastBlockHeight() + 1is consistently used across the codebase for block height increments - The FinalizeBlock and Commit sequence follows the correct order in all test cases
- The height increment is atomic and there are no competing increment patterns
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other block height increment patterns
rg "LastBlockHeight\(\)" --type go -A 3
Length of output: 6733
Script:
#!/bin/bash
# Search for FinalizeBlock and Commit patterns
rg "FinalizeBlock.*Height.*LastBlockHeight" --type go -A 3
# Also check Commit patterns
rg "\.Commit\(\)" --type go -A 3
Length of output: 2097
34-34: Verify the GenerateKeys function implementation
The GenerateKeys function is called but not defined in this file. Let's verify its implementation.
✅ Verification successful
GenerateKeys function is properly implemented and used correctly
The GenerateKeys function is well-implemented in tests/tx_helper.go. It:
- Takes the expected parameters (testing.T and number of keys)
- Returns the correct types ([]common.Address, []*ecdsa.PrivateKey)
- Uses secure random number generation
- Properly generates ECDSA keys and derives addresses
- Includes error handling with test assertions
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for GenerateKeys function definition
ast-grep --pattern 'func GenerateKeys($$$) ($$$, $$$)'
Length of output: 1193
24-26: Verify the genesis coins amount for testing edge cases
The genesis coins amount is set to a very large value (1e24). While this ensures sufficient funds for testing, it might mask potential issues with smaller amounts or edge cases.
Let's check if there are any tests that specifically handle edge cases with smaller amounts:
✅ Verification successful
The large genesis coin amount is appropriate for the test environment
After analyzing the codebase, the large genesis amount (1e24) is justified because:
- This is a test utility file (
app_creator.go) used specifically for creating test environments - The coins are used to initialize test accounts with sufficient balance for various EVM and token-related tests
- The file contains test helpers for ERC20 operations (mint, transfer) and contract deployments which require substantial gas fees
- The balance checks (
checkBalance) are used to verify the initial setup rather than testing edge cases - Edge cases with smaller amounts can still be tested through the ERC20 transfer and mint operations, which accept custom amounts
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for test cases involving balance checks or transfers
rg -A 5 "NewCoin.*BaseDenom" --type go
Length of output: 40
Script:
#!/bin/bash
# Let's try a broader search for balance-related test cases
rg -l "sdk\.NewCoin" --type go tests/
Length of output: 60
Script:
#!/bin/bash
# Search for any test files that might be testing with different coin amounts
rg "NewCoin.*Int" --type go tests/
Length of output: 171
Script:
#!/bin/bash
# Look for any balance checks or assertions in tests
rg "Balance|GetBalance" --type go tests/
Length of output: 630
Script:
#!/bin/bash
# Check the actual test implementation files that might use this app_creator
fd -e go . tests/ --exec cat {}
Length of output: 13712
app/ante/fee.go (1)
34-35: Verify the gas limit value for fee deduction
The gas limit of 1,000,000 seems arbitrary. Please provide justification for this specific value and consider documenting:
- Why this specific limit was chosen
- What operations are expected to fit within this limit
- What happens if the fee deduction operations exceed this limit
✅ Verification successful
Let me search for more context about gas limits in the codebase.
Let me search for the implementation details of fee deduction and gas refund to understand the relationship between these gas limits.
Let me check one more thing - the gas meter usage in both fee deduction and refund contexts.
Gas limit value is appropriately set for fee deduction operations
The gas limit of 1,000,000 is justified based on the codebase analysis:
- It matches the gas limit used for gas refund operations (
gasLimitForRefund = 1_000_000inapp/posthandler/gasrefund.go) - The value is used consistently across test cases for EVM transactions and fee calculations
- The gas meter is temporarily set to this limit specifically for fee deduction operations and then restored to the original gas meter
- This provides sufficient headroom for fee-related operations while preventing excessive gas consumption
The constant is well-documented and its usage pattern in both fee deduction and refund contexts shows it's an intentional design choice for gas-metering of fee operations.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for similar gas limit constants in the codebase for comparison
rg -i "const.*gas.*limit.*=.*1.*000.*000"
Length of output: 131050
Script:
#!/bin/bash
# Search for gas limit related constants and their usage
rg -A 3 "gasLimit.*=.*1.*000.*000|const.*gas.*=.*1.*000.*000"
Length of output: 2561
Script:
#!/bin/bash
# Search for fee deduction and gas refund implementation
rg -A 10 "GasFreeFeeDecorator.*AnteHandle|GasRefundDecorator.*PostHandle"
Length of output: 1570
Script:
#!/bin/bash
# Search for gas meter usage in fee and refund contexts
rg -A 5 "WithGasMeter.*gasLimit|NewGasMeter.*gasLimit"
Length of output: 1447
jsonrpc/backend/filters_test.go (1)
1-14: LGTM! Clean package structure and imports.
The package declaration and import organization follow Go best practices.
indexer/mempool.go (2)
Line range hint 15-19: Implementation correctly satisfies the Mempool interface
The MempoolWrapper properly implements all required methods of the mempool.Mempool interface while adding EVM-specific functionality.
Line range hint 39-62: Review concurrent access patterns in Insert method
The goroutine accessing indexer.pendingChans could potentially face race conditions if the channels slice is modified concurrently. Consider implementing proper synchronization mechanisms.
Let's check for other concurrent access patterns in the codebase:
Consider these improvements:
- Add mutex protection for pendingChans access
- Use a buffered channel or implement backpressure handling
- Consider adding context cancellation support for clean shutdown
✅ Verification successful
Let me analyze the concurrent access patterns from the search results to make a final determination.
The search results show:
- In
indexer/indexer.go, channels are created and stored in slices:
blockChan := make(chan *coretypes.Header)
logsChan := make(chan []*coretypes.Log)
pendingChan := make(chan *rpctypes.RPCTransaction)
e.blockChans = append(e.blockChans, blockChan)
e.logsChans = append(e.logsChans, logsChan)Let me check how these slices are protected:
Let me try a different search pattern to understand the Indexer implementation better:
After reviewing the implementation, I can now provide a final response:
Concurrent access to pendingChans is safe in this implementation
The concurrent access pattern in the Insert method is actually safe because:
- The
pendingChansslice is only modified duringSubscribe()which happens during setup/initialization - After initialization,
pendingChansis only read from, never written to - The channel operations themselves are thread-safe by design in Go
- The TTL cache (
txPendingMap) used for pending transactions is also thread-safe as it's provided by thettlcachelibrary
The goroutine in the Insert method is following a common and safe pattern for broadcasting events to multiple subscribers through channels.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other goroutine usages and channel operations
rg -A 3 "go func\(\)"
rg -A 3 "make\(chan"
Length of output: 8216
Script:
#!/bin/bash
# Search for mutex usage in indexer
rg -A 3 "sync\.(\w+)?Mutex" indexer/
# Check indexer struct definition
rg -A 10 "type\s+Indexer\s+struct" indexer/
Length of output: 84
Script:
#!/bin/bash
# Look for the indexer implementation
rg -l "type Indexer" indexer/
# Then get the content of the main indexer file
cat indexer/indexer.go
Length of output: 6410
app/ante/fee_test.go (3)
28-31: LGTM: Proper error handling added for coin minting operations
The addition of error handling for MintCoins operations improves test reliability by ensuring the test setup is successful.
53-53: LGTM: Consistent updates to AnteHandle calls
The changes consistently update all AnteHandle calls with the new parameter while maintaining appropriate gas consumption assertions for each test case. The test coverage is comprehensive, including:
- Mixed denomination fees
- Non-fee denomination
- No fees
- Gas simulation
Also applies to: 60-60, 67-67, 74-75
45-46: Verify the noopAnteHandler implementation
The test uses noopAnteHandler which isn't defined in this file. Let's verify its implementation.
✅ Verification successful
noopAnteHandler implementation is correctly defined and used
The noopAnteHandler is properly defined in both app/ante/ante_test.go and app/ante/fee.go as a simple pass-through handler that returns the context without modifications:
func noopAnteHandler(ctx sdk.Context, _ sdk.Tx, _ bool) (sdk.Context, error) {
return ctx, nil
}This implementation is appropriate for testing the fee ante handler in isolation, as it allows the test to focus solely on fee-related behavior without interference from other ante handlers.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for noopAnteHandler definition in the codebase
rg -A 5 "noopAnteHandler" --type go
Length of output: 2505
jsonrpc/backend/feehistory_test.go (2)
1-18: LGTM! Package and imports are well-organized.
The test package is correctly named and includes all necessary dependencies.
1-76: Overall, the test implementation is solid.
The test covers the essential functionality of fee history. While there are opportunities for improvement in terms of code organization and test coverage, the current implementation is functional and provides good basic coverage.
jsonrpc/backend/gas_test.go (1)
1-18: LGTM! Well-organized package structure and imports.
The package follows Go conventions with proper test package naming and logically grouped imports.
jsonrpc/backend/cosmos_test.go (3)
1-16: LGTM!
The package declaration and imports are well-organized and follow Go conventions.
18-21: Verify the setupBackend helper function.
The test relies on a setupBackend helper, but its implementation is not visible in the current context.
✅ Verification successful
Based on the gathered information, I can now provide a final response. The setupBackend helper function and its dependencies have been sufficiently verified through the search results. The function is properly implemented in jsonrpc/backend/backend_test.go and uses tests.CreateApp() from tests/app_creator.go to set up the test environment. The helper creates a test application with necessary components including addresses, private keys, and proper context initialization.
The setupBackend helper is properly implemented and used consistently across test files.
- The helper is used in multiple test files for consistent test environment setup
- It properly initializes the application, addresses, and private keys through
tests.CreateApp() - The function returns a
testInputstruct containing all necessary components for the tests - The implementation follows testing best practices with proper context initialization
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for setupBackend implementation
ast-grep --pattern 'func setupBackend($$$) {
$$$
}'
Length of output: 55
Script:
#!/bin/bash
# Let's try a broader search for setupBackend using ripgrep
rg -A 10 "setupBackend" --type go
Length of output: 23755
Script:
#!/bin/bash
# Let's check the tests package for CreateApp implementation
rg -A 10 "CreateApp" --type go
Length of output: 4554
1-98: Verify test coverage and related components.
The tests cover the basic functionality well, but we should verify:
- Test coverage for the backend package
- Implementation of the tested methods (
CosmosTxHashByTxHashandTxHashByCosmosTxHash)
✅ Verification successful
Let me gather more information about the implementation of these methods.
Let me check the test setup code to complete the verification.
Let me check the complete EVMIndexer interface to verify all the hash mapping methods.
Based on the gathered information, I can now provide the final response:
Tests adequately cover hash mapping functionality
The test implementation in cosmos_test.go properly verifies the CosmosTxHashByTxHash and TxHashByCosmosTxHash methods which are part of the EVMIndexer interface. The tests:
- Cover both successful mapping scenarios
- Use realistic test data with ERC20 contract interactions
- Verify bidirectional hash mapping
- Match the actual implementation in
cosmos.gowhich handles error cases appropriately
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for the implementation of tested methods
ast-grep --pattern 'func ($_ *Backend) CosmosTxHashByTxHash($$$) {
$$$
}'
ast-grep --pattern 'func ($_ *Backend) TxHashByCosmosTxHash($$$) {
$$$
}'
# Check for any other test files in the backend package
fd -e go -E '*_test.go' . | grep backend
Length of output: 500
Script:
#!/bin/bash
# Search for the implementation in cosmos.go
rg -A 10 "func \(.*Backend\) (CosmosTxHashByTxHash|TxHashByCosmosTxHash)" jsonrpc/backend/cosmos.go
# Check for other test files specifically
fd ".*_test.go$" . | grep backend
# Look for any related test setup code
rg "func setupBackend" jsonrpc/backend/
Length of output: 1439
Script:
#!/bin/bash
# Check the test setup implementation
rg -A 15 "func setupBackend" jsonrpc/backend/backend_test.go
# Check if EVMIndexer interface is defined
rg "type EVMIndexer interface" -A 5
Length of output: 1012
Script:
#!/bin/bash
# Get the complete EVMIndexer interface definition
rg "type EVMIndexer interface" -A 30 indexer/indexer.go
# Check the mock implementation used in tests
rg "type mockEVMIndexer struct" -A 10 "tests/"
Length of output: 1286
app/ante/ante_test.go (1)
61-63: LGTM! Verify EVM keeper initialization in other test suites.
The EVM keeper initialization is correctly placed after parameter setup and follows consistent error handling patterns.
Let's verify if other test suites might need similar initialization:
✅ Verification successful
Based on the search results, I can see that the EVM keeper initialization is used in multiple test suites, particularly in:
app/ante/ante_test.go- Initializes EVM keeper after setting paramsapp/posthandler/gasrefund_test.go- Initializes with decimals for fee token setupapp/ibc-hooks/test files - Uses EVM keeper for contract deployment and interactionx/evm/keeper/test files - Uses EVM keeper extensively for core EVM functionality tests
Let me verify if there are any other test suites that might need initialization but don't have it:
Based on the search results, I can see that EVM keeper initialization is handled in different ways across test suites:
createTestInputin test files useswithInitializeparameter to control initialization- Some test suites call
Initialize()directly - Others use
InitializeWithDecimals()for specific decimal precision
Let me verify if there are any test files that use EVMKeeper but don't properly initialize it:
Let me verify the initialization patterns in the test files that use EVMKeeper:
Let me check how other test files handle initialization:
Based on the search results, I can now provide a complete analysis of the EVM keeper initialization across test suites:
LGTM! The EVM keeper initialization is properly handled across test suites.
The initialization is handled in three main ways:
- Most test files use
createTestInput(t, false, true)which callsInitializeWithDecimalswhenwithInitializeis true - Some test files like
app/ante/ante_test.godirectly callInitialize(ctx)after setting params - Specific test files like
app/posthandler/gasrefund_test.gouseInitializeWithDecimalsfor custom decimal precision
The initialization in app/ante/ante_test.go is correct and follows the established patterns in the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other test suites that might need EVMKeeper initialization
rg -l "EVMKeeper" --type go | grep "_test.go" | while read -r file; do
echo "=== $file ==="
rg "EVMKeeper" -A 5 -B 5 "$file"
done
Length of output: 83128
Script:
#!/bin/bash
# Search for test files that might be using EVMKeeper but not initializing it
rg -l "EVMKeeper" --type go | grep "_test.go" | while read -r file; do
echo "=== $file ==="
rg "Initialize|EVMKeeper" -B 5 -A 5 "$file"
done
Length of output: 87556
Script:
#!/bin/bash
# Search for test files that use EVMKeeper methods but don't have initialization
rg -l "EVMKeeper" --type go | grep "_test.go" | while read -r file; do
echo "=== $file ==="
# Look for files that use EVMKeeper methods but don't have Initialize calls
rg "EVMKeeper\.(EVMCall|EVMCreate|EVMStaticCall)" "$file" -A 5 -B 5
done
Length of output: 33162
Script:
#!/bin/bash
# Search for test files that use createTestInput with withInitialize parameter
rg -l "createTestInput.*withInitialize" --type go | while read -r file; do
echo "=== $file ==="
rg "createTestInput.*withInitialize|Initialize" -A 5 -B 5 "$file"
done
Length of output: 3809
Script:
#!/bin/bash
# Search for test files that directly call Initialize or InitializeWithDecimals
rg -l "EVMKeeper" --type go | grep "_test.go" | while read -r file; do
echo "=== $file ==="
rg "Initialize|InitializeWithDecimals" -B 5 -A 5 "$file"
done
Length of output: 10518
tests/mock_comet.go (3)
22-32: LGTM! Well-structured interface compliance and struct definition.
The code properly implements interface compliance checks and defines a minimal but sufficient struct for mocking RPC functionality.
34-50: LGTM! Clean constructor and setter implementations.
Good use of the fluent interface pattern for setters, making the mock easy to configure in tests.
1-188: Verify mock coverage against actual usage in tests.
Let's ensure that all required methods for the test scenarios are implemented.
✅ Verification successful
Mock implementation is sufficient for current test usage
The mock implementation is correctly implemented for its current usage. From the test file analysis:
- The mock is only used in
jsonrpc/backend/backend_test.goas a client context dependency for the JSON-RPC backend tests - It's initialized but not directly called, serving as a dependency injection for the client context
- The implemented methods (Status, BroadcastTxSync, UnconfirmedTxs, NumUnconfirmedTxs, NetInfo) cover the core CometRPC interface requirements
- The unimplemented methods that panic are acceptable as they're not used in the test scenarios
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all test files that use MockCometRPC and verify method usage
# Find all test files that import this mock
echo "Test files using MockCometRPC:"
rg -l "MockCometRPC" --type go --glob "*_test.go"
# Find all method calls on MockCometRPC instances
echo -e "\nMethods called on MockCometRPC:"
rg "mock\w*\.(Status|BroadcastTxSync|UnconfirmedTxs|NumUnconfirmedTxs|NetInfo)" --type go --glob "*_test.go"
Length of output: 342
Script:
#!/bin/bash
# Let's examine the test file that uses MockCometRPC to understand its usage
echo "Content of jsonrpc/backend/backend_test.go:"
cat jsonrpc/backend/backend_test.go
# Also search for any other potential usage patterns
echo -e "\nSearching for mock variable declarations and initializations:"
rg "mock\s*:=.*MockCometRPC|mock\s*=.*MockCometRPC" --type go
# Search for any interface method calls with different naming patterns
echo -e "\nSearching for potential method calls with different patterns:"
rg "mock\w*\.\w+" --type go --glob "*_test.go"
Length of output: 2612
jsonrpc/backend/eth_test.go (1)
1-18: LGTM! Package and imports are well-organized.
The package declaration and imports are appropriate for the test file's purpose.
jsonrpc/backend/feehistory.go (1)
Line range hint 1-290: Verify fee calculation behavior after removing dynamic base fee calculation
The removal of dynamic base fee calculation (previously using eip1559) represents a significant change in fee estimation strategy. This needs verification to ensure it aligns with the chain's fee market behavior.
Let's analyze the impact:
Consider adding metrics or logging to monitor fee estimation accuracy in production.
jsonrpc/namespaces/eth/filters/api.go (1)
130-130: Verify thread-safety with race detector
The comment indicates that api.subscriptions is not thread-safe. Let's verify there are no potential race conditions in the codebase.
✅ Verification successful
Thread-safety implementation is correct
The implementation correctly handles thread-safety through a well-designed event loop pattern:
- The
api.subscriptionsmap is only accessed within theeventLoopgoroutine - All subscription operations are synchronized through dedicated channels (
install/uninstall) - Filter operations are protected by a mutex (
filtersMut) - The comment accurately documents the thread-safety guarantee
The synchronization pattern ensures there are no race conditions:
- Event broadcasting is handled by the single
eventLoopgoroutine - Filter modifications are protected by mutex
- Subscription lifecycle is managed through channels
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for potential race conditions in filter API implementation
# Look for direct accesses to api.subscriptions outside eventLoop
rg -n "api\.subscriptions" --type go
# Look for potential concurrent map access patterns
ast-grep --pattern 'api.subscriptions[$_] = $_'
Length of output: 697
Script:
#!/bin/bash
# Let's examine the context around these accesses to understand the synchronization mechanism
# Check the struct definition and initialization
rg -B 5 -A 5 "type API struct" jsonrpc/namespaces/eth/filters/api.go
# Look for eventLoop implementation to verify single-goroutine access
rg -B 10 -A 10 "func.*eventLoop" jsonrpc/namespaces/eth/filters/api.go
# Check for any other goroutine spawns that might access the subscriptions
rg "go \w+" jsonrpc/namespaces/eth/filters/api.go
Length of output: 766
Script:
#!/bin/bash
# Let's check how the channels are used to synchronize access and verify the subscription operations
# Look for subscription-related method implementations
rg -B 5 -A 10 "func \(api \*FilterAPI\) (New|Uninstall)" jsonrpc/namespaces/eth/filters/api.go
# Check the initialization of channels and context
rg -B 5 -A 5 "NewAPI" jsonrpc/namespaces/eth/filters/api.go
# Look for any subscription operations outside eventLoop
rg -A 5 -B 5 "type subscription struct" jsonrpc/namespaces/eth/filters/api.go
Length of output: 2080
Script:
#!/bin/bash
# Let's check the complete file content to understand the synchronization pattern
# Get the full content of the file
cat jsonrpc/namespaces/eth/filters/api.go
# Also check for any install/uninstall channel operations
rg -B 5 -A 5 "installSubscription|uninstallSubscription" jsonrpc/namespaces/eth/filters/api.go
# Look for the channel declarations and initialization
rg -B 2 -A 2 "make\(chan" jsonrpc/namespaces/eth/filters/api.go
Length of output: 14880
jsonrpc/namespaces/eth/api.go (1)
147-149: Verify the impact of the context parameter removal.
Let's check how this change affects other parts of the codebase.
✅ Verification successful
Let me generate one more verification script to check the interface definition in the backend package.
Based on the verification results, I can now provide the final response:
Context parameter removal is consistent with the codebase
The verification shows that the GetBlockReceipts method implementation is consistent across all its usages:
- The main implementation in
jsonrpc/namespaces/eth/api.go - The backend implementation in
jsonrpc/backend/tx.go - Test usages in
jsonrpc/backend/filters_test.goandjsonrpc/backend/tx_test.go
None of these implementations use a context parameter, and all calls to this method align with the current signature of (blockNrOrHash rpc.BlockNumberOrHash) ([]map[string]interface{}, error).
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for implementations and usages of GetBlockReceipts
# to verify the impact of removing the context parameter
# Check for interface definitions and implementations
echo "Checking for interface definitions and implementations..."
ast-grep --pattern 'interface $_ {
$$$
GetBlockReceipts($$$)
$$$
}'
# Check for method calls
echo "Checking for method calls..."
rg -A 2 'GetBlockReceipts\('
# Check for similar methods to verify consistency
echo "Checking similar receipt-related methods for context usage..."
ast-grep --pattern 'func $_($ctx context.Context, $$$) $$ {
$$$
GetReceipt($$$)
$$$
}'
Length of output: 1699
Script:
#!/bin/bash
# Check the backend interface definition
echo "Checking backend interface definition..."
rg -B 2 -A 5 'type Backend interface' jsonrpc/backend/
# Check if there are any other implementations of Backend
echo "Checking for Backend implementations..."
ast-grep --pattern 'type $_ struct {
$$$
}
func ($v $_) GetBlockReceipts($$$) $$ {
$$$
}'
Length of output: 334
jsonrpc/backend/tx_test.go (1)
1-455: Overall test structure and coverage looks good
The test suite provides comprehensive coverage of the transaction-related functionality:
- Transaction pool management
- Transaction retrieval by various methods
- Receipt handling
- Block transaction counting
- Raw transaction handling
jsonrpc/backend/txpool_test.go (2)
174-183: Handle potential errors when sending transactions
Currently, the tests assume that transactions are successfully sent and do not handle the possibility of failures. Consider adding checks to ensure that the transactions are accepted by the backend and handle any errors appropriately.
Ensure that the backend.SendRawTransaction calls are succeeding as expected. If there is a chance of failure, add appropriate error handling or assertions.
Also applies to: 185-194, 196-205, 215-224, 234-243, 277-286, 288-297, 299-308, 318-327, 338-347
310-314: Verify correctness of transaction pool status assertions
In your assertions for the transaction pool status, ensure that the expected counts of pending and queued transactions are accurate based on the test scenarios.
Run the following script to cross-verify the expected transaction counts:
Check whether the counts returned match the assertions in your tests at lines 207-211 (Test_TxPoolStatus) and lines 310-314 (Test_TxPoolInspect).
Also applies to: 226-230, 207-211
✅ Verification successful
The previous attempt to verify using curl commands wasn't appropriate as this is a test environment, not a running node. Let's analyze the test code itself to verify the correctness of the assertions.
I need to verify the setup code to understand how the transaction pool is initialized and managed before making a final determination about the correctness of the assertions.
After analyzing the test file and setup code, I can now provide a final response about the transaction pool assertions:
Transaction pool assertions are correctly implemented
The test assertions at lines 310-314 are accurate based on the test scenario setup:
- The test correctly expects 1 pending transaction for
addrs[1]because it submitted transaction with nonce 0 (tx10) - The test correctly expects 2 queued transactions for
addrs[0]because it submitted transactions with nonce gaps (tx04 and tx06, while nonce 3 is missing)
The assertions align with Ethereum's transaction pool behavior where:
- Transactions with sequential nonces go to pending pool
- Transactions with nonce gaps go to queued pool until the gaps are filled
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the counts of pending and queued transactions in the txpool
# Extract txpool status
curl -s -X POST --data '{"jsonrpc":"2.0","method":"txpool_status","params":[],"id":1}' http://localhost:8545 | jq
# Inspect txpool content
curl -s -X POST --data '{"jsonrpc":"2.0","method":"txpool_content","params":[],"id":1}' http://localhost:8545 | jq
Length of output: 232
Script:
#!/bin/bash
# Let's analyze the test file to understand the test setup and verification logic
# First, let's see the full test file content
cat jsonrpc/backend/txpool_test.go
# Then, let's find any helper functions or setup code that might be relevant
rg -l "setupTestBlockchain|setupBackend" jsonrpc/backend/
Length of output: 14715
Script:
#!/bin/bash
# Let's check the setup code to understand how the test environment is initialized
cat jsonrpc/backend/backend_test.go
Length of output: 1621
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (3)
x/evm/contracts/counter/Counter.sol (2)
111-112: Update comment to accurately reflect the function behaviorThe comment should clarify that this function calls
recursive_revert_without_allow_failurewithallow_failureset to true, allowing the recursive call to fail without reverting the entire transaction chain.- // This function call `recursive_revert_without_allow_failure` with allow_failure set to true. + // This function calls `recursive_revert_without_allow_failure` with allow_failure set to true, + // allowing the recursive call to fail without reverting the entire transaction chain.
112-124: Consider adding safeguards against deep recursionThe function could benefit from additional safety measures:
- Add a maximum recursion depth check
- Use a more descriptive revert message
- Consider utilizing the callback_id parameter for better transaction tracking
function recursive_revert_with_allow_failure(uint64 n) public { + require(n <= 100, "Maximum recursion depth exceeded"); emit recursive_called(n); if (n == 0) { - revert("revert reason dummy value for test"); + revert("Counter: Reached base case, intentionally reverting"); } // call recursive_revert without allow_failure COSMOS_CONTRACT.execute_cosmos_with_options( _recursive_revert_without_allow_failure(n), - ICosmos.Options(true, 0) + ICosmos.Options(true, n) // Use n as callback_id for tracking ); }x/evm/keeper/context_test.go (1)
438-462: Enhance test documentation and coverageWhile the test effectively verifies the basic functionality, consider the following improvements:
- Add a descriptive comment explaining the test's purpose and expected behavior
- Add test cases with different depths to verify boundary conditions
- Verify the content of the retained log to ensure it's the correct one
- Add assertions for events emitted during execution
func Test_RecursiveExecuteCosmosAndRevertInSubMessage(t *testing.T) { + // Test verifies that when executing recursive cosmos messages with allowed failure: + // 1. Only the first log is retained + // 2. All subsequent logs are reverted + // 3. The execution continues successfully despite sub-message failures ctx, input := createDefaultTestInput(t) _, _, addr := keyPubAddr() counterBz, err := hexutil.Decode(counter.CounterBin) require.NoError(t, err) // deploy counter contract caller := common.BytesToAddress(addr.Bytes()) retBz, contractAddr, _, err := input.EVMKeeper.EVMCreate(ctx, caller, counterBz, nil, nil) require.NoError(t, err) require.NotEmpty(t, retBz) require.Len(t, contractAddr, 20) // call recursive_revert_with_allow_failure function parsed, err := counter.CounterMetaData.GetAbi() require.NoError(t, err) - inputBz, err := parsed.Pack("recursive_revert_with_allow_failure", uint64(2)) + // Test with different depths + testCases := []struct { + name string + depth uint64 + expectedLogs int + expectError bool + }{ + {"depth 1", 1, 1, false}, + {"depth 2", 2, 1, false}, + {"depth 10", 10, 1, false}, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + inputBz, err := parsed.Pack("recursive_revert_with_allow_failure", tc.depth) + require.NoError(t, err) + + _, logs, err := input.EVMKeeper.EVMCall(ctx, caller, contractAddr, inputBz, nil, nil) + if tc.expectError { + require.Error(t, err) + return + } + require.NoError(t, err) + require.Equal(t, tc.expectedLogs, len(logs)) + + // Verify the content of the retained log + if len(logs) > 0 { + log := logs[0] + require.Equal(t, contractAddr.Hex(), log.Address) + // Add specific log content verification based on contract events + } + + // Verify events + events := ctx.EventManager().Events() + var foundSubmsgEvent bool + for _, event := range events { + if event.Type == types.EventTypeSubmsg { + foundSubmsgEvent = true + // Add specific event attribute verification + break + } + } + require.True(t, foundSubmsgEvent) + }) + } - require.NoError(t, err) - - _, logs, err := input.EVMKeeper.EVMCall(ctx, caller, contractAddr, inputBz, nil, nil) - require.NoError(t, err) - require.Equal(t, 1, len(logs)) // except the first log, all logs are reverted }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
x/evm/contracts/counter/Counter.go(2 hunks)x/evm/contracts/counter/Counter.sol(1 hunks)x/evm/keeper/context_test.go(1 hunks)
🔇 Additional comments (3)
x/evm/contracts/counter/Counter.sol (1)
110-159: Verify recursive call patterns and gas implications
The new recursive functions introduce additional complexity to the system. Let's verify:
- Similar recursive patterns in the codebase
- Gas consumption patterns
- COSMOS_CONTRACT integration points
✅ Verification successful
Recursive patterns and gas consumption appear safe
Based on the codebase analysis:
- The recursive pattern is isolated to the Counter contract and is specifically designed for testing purposes (as indicated by the revert message "revert reason dummy value for test").
- COSMOS_CONTRACT usage follows consistent patterns across the codebase, primarily for execute_cosmos and execute_cosmos_with_options calls.
- Gas considerations are well-documented in other contracts (ERC721, ERC20) but not critical for these test functions.
The recursive implementation is:
- Controlled by the 'n' parameter that decrements each call
- Has explicit failure handling with allow_failure options
- Limited to test scenarios and not used in production flows
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for similar recursive patterns and COSMOS_CONTRACT usage
echo "Checking for recursive patterns and COSMOS_CONTRACT usage..."
ast-grep --pattern 'function $FNAME($ARGS) $_ {
$$$
COSMOS_CONTRACT.execute_cosmos_with_options($_, $_);
$$$
}'
# Search for potential gas-related issues
echo "Checking for gas-related comments or patterns..."
rg -i "gas|recursion|depth" --type sol
Length of output: 426
Script:
#!/bin/bash
# Search for similar recursive patterns and COSMOS_CONTRACT usage with proper file extension
echo "Checking for recursive patterns and COSMOS_CONTRACT usage..."
fd -e sol . | xargs ast-grep --pattern 'function $FNAME($ARGS) $_ {
$$$
COSMOS_CONTRACT.execute_cosmos_with_options($_, $_);
$$$
}'
# Search for potential gas-related issues with proper file extension
echo "Checking for gas-related comments or patterns..."
fd -e sol . | xargs rg -i "gas|recursion|depth"
# Check for other COSMOS_CONTRACT usages
echo "Checking for COSMOS_CONTRACT usages..."
fd -e sol . | xargs rg "COSMOS_CONTRACT"
Length of output: 3317
x/evm/contracts/counter/Counter.go (2)
34-35: ABI and Bin updated to include new functions
The ABI and binary have been updated correctly to include the new methods. Ensure they match the latest Solidity contract.
456-497: New methods added for recursive revert functionality
The methods RecursiveRevertWithAllowFailure and RecursiveRevertWithoutAllowFailure, along with their session variants, have been correctly added following existing patterns.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (4)
jsonrpc/namespaces/eth/filters/api_test.go (3)
25-33: Add documentation to the test input structure.While the structure is well-designed, adding documentation would improve maintainability and help other developers understand the purpose of each field.
Consider adding documentation like this:
+// testInput encapsulates all dependencies required for filter API testing type testInput struct { + // app is the Minitia application instance app *minitiaapp.MinitiaApp + // indexer handles EVM event indexing indexer indexer.EVMIndexer + // backend provides JSON-RPC functionality backend *backend.JSONRPCBackend + // addrs contains test Ethereum addresses addrs []common.Address + // privKeys contains corresponding private keys privKeys []*ecdsa.PrivateKey + // cometRPC is a mock RPC client for testing cometRPC *tests.MockCometRPC + // filterAPI is the main filter API being tested filterAPI *filters.FilterAPI }
35-68: Consider enhancing the setup function's robustness.While the setup is comprehensive, there are several potential improvements:
- Extract the filter timeout to a constant:
+const defaultFilterTimeout = 3 * time.Second func setupFilterAPI(t *testing.T) testInput { // ... cfg := config.DefaultJSONRPCConfig() cfg.Enable = true - cfg.FilterTimeout = 3 * time.Second + cfg.FilterTimeout = defaultFilterTimeout
- Add context with timeout:
- ctx := context.Background() + ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) + t.Cleanup(cancel)
- Add cleanup in error cases:
backend, err := backend.NewJSONRPCBackend(ctx, app, app.Logger(), svrCtx, clientCtx, cfg) - require.NoError(t, err) + if err != nil { + app.Close() + t.Fatalf("failed to create backend: %v", err) + }
1-78: Consider extracting test setup to a shared package.The
testInputstructure andsetupFilterAPIfunction could be useful for other test files testing related functionality. Consider moving them to a shared test utilities package (e.g.,filters/testing) to promote code reuse.app/upgrade.go (1)
Line range hint
30-121: Consider separating OPINIT and MINIEVM upgrade handlers.The current upgrade handler combines upgrades for both OPINIT and MINIEVM components. Consider splitting these into separate handlers to:
- Enable independent versioning and upgrades of components
- Reduce complexity and potential points of failure
- Improve maintainability and testing
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
app/upgrade.go(2 hunks)jsonrpc/namespaces/eth/filters/api_test.go(1 hunks)tests/app_creator.go(1 hunks)tests/tx_helper.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/app_creator.go
- tests/tx_helper.go
🔇 Additional comments (3)
jsonrpc/namespaces/eth/filters/api_test.go (1)
1-23: LGTM! Package and imports are well-organized.
The package name follows Go testing conventions with the _test suffix, and imports are properly grouped into standard library, external, and internal packages.
app/upgrade.go (2)
41-47: LGTM! Improved parameter handling logic.
The new conditional check for HookMaxGas is a better approach as it:
- Prevents unnecessary state updates when the parameter is already set
- Only applies the default value when needed
- Reduces potential for conflicts with existing configurations
Line range hint 51-116: Verify contract upgrade logic integrity.
While this section remains unchanged, it's critical to verify the contract upgrade logic since it:
- Deploys new ERC20 wrapper and factory contracts
- Updates existing ERC20 contract implementations
- Modifies contract storage
Please ensure:
- All contract addresses are correctly preserved
- Code hash calculations remain accurate
- Storage key construction is consistent
✅ Verification successful
Let me generate additional scripts to verify the contract upgrade logic integrity.
This will help verify:
- Storage key prefixes used in contract storage
- Contract deployment methods implementation
- Code hash calculation logic
- Storage key construction for contract code and size
Let me generate one final verification script to check the ERC20 contract deployment and factory methods.
This will help verify the contract deployment implementation details to ensure the upgrade logic is sound.
Contract upgrade logic verified successfully
The contract upgrade implementation is sound and maintains integrity:
-
Contract Deployment:
- Proper handling of ERC20 wrapper and factory deployments with collision checks
- Factory deployment is a prerequisite for wrapper deployment
- Appropriate error handling for address collisions
-
Code Update Logic:
- Correct extraction of runtime code using bytecode analysis
- Proper code hash calculation using crypto.Keccak256Hash
- Consistent storage key construction using CodeKeyPrefix and CodeSizeKeyPrefix
- Atomic updates of both code and code size
-
Storage Management:
- Proper key prefixes defined for code and code size storage
- Consistent key construction combining contract address and code hash
- Proper state updates through VMStore.Set operations
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for any related contract address or storage key changes
echo "Checking for contract-related constants..."
rg "0x[a-fA-F0-9]{40}" --type go
echo "Checking for storage key prefixes..."
ast-grep --pattern 'var $_ = []byte($_)'
Length of output: 136436
Script:
#!/bin/bash
# Check for storage key prefixes and state variables
echo "Checking storage key prefixes..."
ast-grep --pattern 'var $_ = []byte("$_")'
echo "Checking EVM keeper contract deployment methods..."
ast-grep --pattern 'func ($_ *EVMKeeper) DeployERC20$_($_ $_, $_ $_) error'
echo "Checking contract code hash calculation..."
rg "crypto.Keccak256Hash" --type go -A 2
echo "Checking VMStore key construction..."
rg "CodeKeyPrefix|CodeSizeKeyPrefix" --type go -A 2
Length of output: 2211
Script:
#!/bin/bash
# Check ERC20 deployment and factory methods
echo "Checking ERC20 deployment methods..."
rg "DeployERC20(Factory|Wrapper)" --type go -A 10
echo "Checking EVM keeper contract deployment..."
rg "EVMKeeper.*Deploy.*ERC20" --type go -A 5
Length of output: 4354
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (11)
jsonrpc/backend/backend_test.go (3)
29-36: Consider adding documentation for the test input structure.While the field names are self-explanatory, adding GoDoc comments would improve code maintainability and help other developers understand the purpose of each field.
+// testInput encapsulates all dependencies required for backend testing type testInput struct { + // app is the MinitiaApp instance used for testing app *minitiaapp.MinitiaApp + // indexer manages the EVM state indexing indexer indexer.EVMIndexer + // backend is the JSON-RPC backend being tested backend *backend.JSONRPCBackend + // addrs contains test Ethereum addresses addrs []common.Address + // privKeys contains private keys corresponding to addrs privKeys []*ecdsa.PrivateKey + // cometRPC is a mock RPC client for testing cometRPC *tests.MockCometRPC }
38-67: Consider adding cleanup mechanism for test resources.The setup function initializes resources but doesn't provide a way to clean them up. Consider adding a cleanup method to ensure proper resource management.
type testInput struct { app *minitiaapp.MinitiaApp indexer indexer.EVMIndexer backend *backend.JSONRPCBackend addrs []common.Address privKeys []*ecdsa.PrivateKey cometRPC *tests.MockCometRPC + cleanup func() } func setupBackend(t *testing.T) testInput { app, addrs, privKeys := tests.CreateApp(t) indexer := app.EVMIndexer() + + t.Cleanup(func() { + if err := backend.Close(); err != nil { + t.Logf("failed to cleanup backend: %v", err) + } + if err := app.Close(); err != nil { + t.Logf("failed to cleanup app: %v", err) + } + })
91-104: Consider using context timeout for query goroutines.The query goroutines could potentially run indefinitely if the test fails before cancellation. Consider adding a timeout context.
- ctx, cancel := context.WithCancel(context.Background()) + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Minute) + defer cancel()jsonrpc/types/tx_test.go (1)
Line range hint
15-115: Consider refactoring tests to use table-driven approachWhile the current test coverage is good, the test structure could be improved to reduce code duplication and make it easier to add more test cases.
Consider refactoring to a table-driven approach:
func TestRPCTransaction(t *testing.T) { tests := []struct { name string txConfig interface{} // LegacyTx, AccessListTx, or DynamicFeeTx wantErr bool }{ { name: "legacy transaction", txConfig: &coretypes.LegacyTx{ // ... existing config }, }, // Add more test cases } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { // Common test logic here }) } }Also consider adding edge cases:
- Nil addresses
- Empty transaction data
- Zero values
- Maximum values for numeric fields
jsonrpc/namespaces/eth/filters/api_test.go (4)
32-40: Add documentation for the testInput struct.Consider adding a doc comment explaining the purpose and usage of each field in the struct.
+// testInput encapsulates all components required for testing the filter API +// including the application instance, indexer, backend, test addresses, +// private keys, mock RPC client, and the filter API under test. type testInput struct { app *minitiaapp.MinitiaApp indexer indexer.EVMIndexer backend *backend.JSONRPCBackend addrs []common.Address privKeys []*ecdsa.PrivateKey cometRPC *tests.MockCometRPC filterAPI *filters.FilterAPI }
54-56: Consider making filter timeout configurable.The filter timeout is hardcoded to 3 seconds. Consider making it configurable through a test parameter or environment variable for flexibility in different test environments.
cfg := config.DefaultJSONRPCConfig() cfg.Enable = true -cfg.FilterTimeout = 3 * time.Second +cfg.FilterTimeout = getFilterTimeout() // New helper function to get timeout from env or default
77-148: Enhance Test_NewPendingTransactionFilter_FullTx documentation and coverage.While the test is comprehensive, consider:
- Adding doc comments describing test scenarios
- Testing error cases (e.g., invalid filter ID)
- Testing filter cleanup after timeout
+// Test_NewPendingTransactionFilter_FullTx verifies that: +// - A new pending transaction filter can be created with full transaction details +// - The filter correctly captures and returns complete transaction objects +// - Multiple transactions are properly tracked and retrieved func Test_NewPendingTransactionFilter_FullTx(t *testing.T) { // ... existing test code ... + + t.Run("invalid filter ID", func(t *testing.T) { + _, err := input.filterAPI.GetFilterChanges("invalid-id") + require.Error(t, err) + }) + + t.Run("filter timeout", func(t *testing.T) { + // Wait for filter timeout + time.Sleep(4 * time.Second) + _, err := input.filterAPI.GetFilterChanges(filterID) + require.Error(t, err) + }) }
245-291: Enhance Test_NewFilter with additional test cases.The test covers basic functionality and invalid block range, but consider adding:
- Tests for different filter criteria (e.g., address filtering, topic filtering)
- Edge cases for block ranges (e.g., pending blocks)
- Filter removal scenarios
Example additional test cases:
t.Run("address filtering", func(t *testing.T) { filterID, err := input.filterAPI.NewFilter(ethfilters.FilterCriteria{ FromBlock: big.NewInt(app.LastBlockHeight()), Addresses: []common.Address{contractAddr}, }) require.NoError(t, err) // ... test address filtering }) t.Run("topic filtering", func(t *testing.T) { filterID, err := input.filterAPI.NewFilter(ethfilters.FilterCriteria{ FromBlock: big.NewInt(app.LastBlockHeight()), Topics: [][]common.Hash{{common.HexToHash("Transfer(address,address,uint256)")}}, }) require.NoError(t, err) // ... test topic filtering })tests/tx_helper.go (3)
31-53: Usecrypto.GenerateKey()for key generationInstead of manually generating random bytes and passing them to
ecdsa.GenerateKey, consider usingcrypto.GenerateKey()for simplicity and clarity. This function securely generates a private key and is the standard approach.Apply this diff to simplify the key generation:
func GenerateKeys(t *testing.T, n int) ([]common.Address, []*ecdsa.PrivateKey) { addrs := make([]common.Address, n) privKeys := make([]*ecdsa.PrivateKey, n) for i := 0; i < n; i++ { - randBytes := make([]byte, 64) - _, err := rand.Read(randBytes) - require.NoError(t, err) - reader := bytes.NewReader(randBytes) - privKey, err := ecdsa.GenerateKey(crypto.S256(), reader) + privKey, err := crypto.GenerateKey() require.NoError(t, err) cosmosKey := ethsecp256k1.PrivKey{ Key: crypto.FromECDSA(privKey), } addrBz := cosmosKey.PubKey().Address() addr := common.BytesToAddress(addrBz) addrs[i] = addr privKeys[i] = privKey } return addrs, privKeys }
143-146: Parameterize token details for flexibilityIn
GenerateCreateInitiaERC20Tx, the token name, symbol, and decimals are hardcoded as"foo","foo", and6. Consider parameterizing these values to enhance flexibility and reusability for testing different token configurations.Apply this diff to parameterize the token details:
-func GenerateCreateInitiaERC20Tx(t *testing.T, app *minitiaapp.MinitiaApp, privKey *ecdsa.PrivateKey, opts ...Opt) (sdk.Tx, common.Hash) { +func GenerateCreateInitiaERC20Tx(t *testing.T, app *minitiaapp.MinitiaApp, privKey *ecdsa.PrivateKey, name string, symbol string, decimals uint8, opts ...Opt) (sdk.Tx, common.Hash) { abi, err := initia_erc20.InitiaErc20MetaData.GetAbi() require.NoError(t, err) bin, err := hexutil.Decode(initia_erc20.InitiaErc20MetaData.Bin) require.NoError(t, err) - inputBz, err := abi.Pack("", "foo", "foo", uint8(6)) + inputBz, err := abi.Pack("", name, symbol, decimals) require.NoError(t, err) return GenerateTx(t, app, privKey, nil, append(bin, inputBz...), new(big.Int).SetUint64(0), opts...) }
159-162: Parameterize token details for flexibilitySimilarly, in
GenerateCreateERC20Tx, hardcoded token details limit the function's reusability. Parameterizing the token name, symbol, and decimals will make the function more adaptable for different test cases.Apply this diff to parameterize the token details:
-func GenerateCreateERC20Tx(t *testing.T, app *minitiaapp.MinitiaApp, privKey *ecdsa.PrivateKey, opts ...Opt) (sdk.Tx, common.Hash) { +func GenerateCreateERC20Tx(t *testing.T, app *minitiaapp.MinitiaApp, privKey *ecdsa.PrivateKey, name string, symbol string, decimals uint8, opts ...Opt) (sdk.Tx, common.Hash) { ctx, err := app.CreateQueryContext(0, false) require.NoError(t, err) ethFactoryAddr, err := app.EVMKeeper.GetERC20FactoryAddr(ctx) require.NoError(t, err) abi, err := erc20_factory.Erc20FactoryMetaData.GetAbi() require.NoError(t, err) - inputBz, err := abi.Pack("createERC20", "foo", "foo", uint8(6)) + inputBz, err := abi.Pack("createERC20", name, symbol, decimals) require.NoError(t, err) return GenerateTx(t, app, privKey, ðFactoryAddr, inputBz, new(big.Int).SetUint64(0), opts...) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (9)
.github/workflows/test.yml(1 hunks)app/upgrade.go(2 hunks)jsonrpc/backend/backend_test.go(1 hunks)jsonrpc/namespaces/eth/filters/api.go(5 hunks)jsonrpc/namespaces/eth/filters/api_test.go(1 hunks)jsonrpc/types/tx.go(2 hunks)jsonrpc/types/tx_test.go(1 hunks)tests/app_creator.go(1 hunks)tests/tx_helper.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- .github/workflows/test.yml
- app/upgrade.go
- jsonrpc/namespaces/eth/filters/api.go
- tests/app_creator.go
🔇 Additional comments (6)
jsonrpc/backend/backend_test.go (1)
1-27: LGTM! Package structure and imports are well-organized.
The package follows Go conventions with proper test package naming and logical import grouping.
jsonrpc/types/tx.go (2)
6-6: LGTM: Import of yaml package
The yaml.v3 package is appropriately imported and is the latest stable version.
139-143: Verify the String() method usage in tests
Since this PR adds indexer tests, let's verify how this method is being used in the test suite to ensure the proposed changes align with the testing requirements.
✅ Verification successful
String() method is used in filter API tests and is correctly implemented
The verification shows that:
- The
String()method is actively used injsonrpc/namespaces/eth/filters/api_test.gofor test assertions - The implementation using YAML marshaling is appropriate as it provides a consistent string representation for test comparisons
- The type is well-tested with dedicated test cases in
jsonrpc/types/tx_test.go
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for usage of String() method in test files
rg -t go "RPCTransaction.*String\(\)" --glob "*_test.go"
Length of output: 58
Script:
#!/bin/bash
# Let's first check if the RPCTransaction type is used in tests
rg -t go "RPCTransaction" --glob "*_test.go" -A 2
# Also check for any test files in the same directory
fd ".*_test.go$" jsonrpc/types/
# Let's also check where this String method is actually used in the codebase
rg -t go "RPCTransaction.*String\(\)"
Length of output: 2283
jsonrpc/namespaces/eth/filters/api_test.go (2)
1-30: LGTM! Well-structured package and imports.
The package structure follows Go testing conventions and imports are appropriately organized.
1-291: Overall assessment: Well-structured and comprehensive test suite.
The test file provides good coverage of the filter API functionality. While there are opportunities for improvement in documentation, code organization, and test coverage, the current implementation is solid and follows good testing practices.
tests/tx_helper.go (1)
117-122: Handle explicit zero nonce in GenerateTx
If the transaction nonce is explicitly set to zero using SetNonce(0), the current condition will still fetch the nonce from the account, potentially overwriting the intended value. To respect an explicit zero nonce, consider introducing a flag to check if the nonce was set by the caller.
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (6)
jsonrpc/backend/backend_test.go (1)
38-67: Consider adding cleanup mechanism and component validation.While the setup is comprehensive, consider the following improvements:
- Add a cleanup mechanism using
t.Cleanup()to ensure proper resource disposal- Add validation for critical components like indexer and backend initialization
Here's a suggested improvement:
func setupBackend(t *testing.T) testInput { app, addrs, privKeys := tests.CreateApp(t) indexer := app.EVMIndexer() + require.NotNil(t, indexer, "indexer should not be nil") + + t.Cleanup(func() { + if err := app.Close(); err != nil { + t.Logf("failed to cleanup app: %v", err) + } + }) // ... rest of the setup code ... backend, err := backend.NewJSONRPCBackend(ctx, app, app.Logger(), svrCtx, clientCtx, cfg) require.NoError(t, err) + require.NotNil(t, backend, "backend should not be nil") return testInput{ // ... struct initialization ... } }jsonrpc/namespaces/eth/filters/api_test.go (5)
32-40: Add documentation for the testInput struct.Consider adding a doc comment explaining the purpose and usage of each field in the struct.
+// testInput encapsulates all components required for filter API testing +// including the application instance, indexer, backend, test addresses, +// private keys, mock RPC client, and filter API instance. type testInput struct {
54-56: Consider making the filter timeout configurable.The filter timeout is hardcoded to 3 seconds. Consider making it configurable through test parameters or environment variables for flexibility in different test environments.
+const defaultFilterTimeout = 3 * time.Second + cfg := config.DefaultJSONRPCConfig() cfg.Enable = true -cfg.FilterTimeout = 3 * time.Second +cfg.FilterTimeout = defaultFilterTimeout
77-148: Add comprehensive test documentation and consider table-driven tests.The test covers the full transaction filter functionality but could benefit from:
- Clear documentation of test scenarios
- Table-driven tests for different transaction types
+// Test_NewPendingTransactionFilter_FullTx verifies that: +// - A new pending transaction filter can be created with full transaction details +// - The filter correctly captures contract creation and token minting transactions +// - The returned transactions match the expected format without block details func Test_NewPendingTransactionFilter_FullTx(t *testing.T) {
245-291: Enhance filter criteria test coverage.The
Test_NewFilterfunction could benefit from additional test cases:
- Testing with different address filters
- Testing with specific event signatures
- Testing with multiple topics
Consider adding these test cases:
func Test_NewFilter(t *testing.T) { testCases := []struct { name string criteria ethfilters.FilterCriteria wantErr bool }{ { name: "with address filter", criteria: ethfilters.FilterCriteria{ Addresses: []common.Address{common.HexToAddress("0x123")}, }, }, { name: "with event signature", criteria: ethfilters.FilterCriteria{ Topics: [][]common.Hash{{common.HexToHash("0x123")}}, }, }, // Add more test cases } // ... test implementation }
287-290: Replace t.Logf with meaningful assertions.The test uses
t.Logffor debugging. Consider replacing it with meaningful assertions about the log contents.for _, change := range changes.([]*coretypes.Log) { require.Equal(t, txHash2, change.TxHash) - t.Logf("%v", change) + // Add specific assertions about log contents + require.NotEmpty(t, change.Topics) + require.NotEmpty(t, change.Data) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (11)
.github/workflows/test.yml(1 hunks)app/upgrade.go(2 hunks)indexer/abci_test.go(1 hunks)indexer/mempool_test.go(1 hunks)jsonrpc/backend/backend_test.go(1 hunks)jsonrpc/namespaces/eth/filters/api.go(5 hunks)jsonrpc/namespaces/eth/filters/api_test.go(1 hunks)jsonrpc/types/tx.go(2 hunks)jsonrpc/types/tx_test.go(1 hunks)tests/app_creator.go(1 hunks)tests/tx_helper.go(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- jsonrpc/types/tx_test.go
🚧 Files skipped from review as they are similar to previous changes (7)
- .github/workflows/test.yml
- app/upgrade.go
- indexer/abci_test.go
- indexer/mempool_test.go
- jsonrpc/namespaces/eth/filters/api.go
- tests/app_creator.go
- tests/tx_helper.go
🔇 Additional comments (3)
jsonrpc/backend/backend_test.go (1)
1-37: LGTM! Well-structured test setup.
The imports are well-organized, and the testInput struct is properly designed to encapsulate all necessary testing components.
jsonrpc/types/tx.go (1)
6-6: LGTM: Import added for YAML serialization
The yaml package import is correctly placed and properly grouped with other external dependencies.
jsonrpc/namespaces/eth/filters/api_test.go (1)
1-30: LGTM: Package structure and imports are well organized.
The package follows Go conventions with appropriate test suffix and necessary imports.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (7)
tests/jsonrpc_creator.go (2)
38-48: Document potential port race conditionWhile the implementation follows a common pattern for finding a free port, there's a small time window between closing the listener and the actual use of the port where another process could potentially claim it. Consider adding a comment documenting this limitation.
// getFreePort asks the kernel for a free open port that is ready to use. +// Note: There's a small race window between releasing and using the port. +// In practice, this is rarely an issue in test environments. func getFreePort(t *testing.T) (port int) {
62-67: Consider making configuration values configurableThe filter timeout and other configuration values are hard-coded. Consider making these configurable through test parameters or environment variables for better flexibility in different test environments.
cfg := config.DefaultJSONRPCConfig() cfg.Enable = true -cfg.FilterTimeout = 3 * time.Second +cfg.FilterTimeout = getConfigDuration("FILTER_TIMEOUT", 3 * time.Second)jsonrpc/namespaces/eth/filters/subscriptions_test.go (4)
92-109: Simplify synchronization logicThe current implementation uses two WaitGroups which makes the code harder to understand and maintain. Consider using a channel-based approach for better readability and reliability.
Here's a suggested improvement:
-wg := sync.WaitGroup{} // wait to receive header from channel -wg2 := sync.WaitGroup{} // wait for header to be set -wg.Add(1) -wg2.Add(1) +headerReady := make(chan struct{}) +done := make(chan struct{}) var expectedHeader *coretypes.Header go func() { + defer close(done) for { select { case <-ctx.Done(): return case header := <-ch: - wg2.Wait() + <-headerReady require.NotNil(t, header) require.Equal(t, expectedHeader, header) - wg.Done() + return } } }()
169-170: Remove magic number in event selectionThe index
len(events)-3is a magic number that makes the code harder to maintain. Consider using a helper function to find the event by type.Suggested improvement:
-createEvent := events[len(events)-3] -require.Equal(t, evmtypes.EventTypeContractCreated, createEvent.GetType()) +func findEventByType(events []sdk.Event, eventType string) (sdk.Event, error) { + for _, event := range events { + if event.GetType() == eventType { + return event, nil + } + } + return sdk.Event{}, fmt.Errorf("event type %s not found", eventType) +} + +createEvent, err := findEventByType(events, evmtypes.EventTypeContractCreated) +require.NoError(t, err)
142-162: Simplify log collection logicThe current implementation uses a complex combination of WaitGroups and slice comparison. Consider using a more straightforward approach with channels.
Here's a suggested improvement:
-wg := sync.WaitGroup{} // wait to receive logs from channel -wg2 := sync.WaitGroup{} // wait for logs to be set -wg.Add(1) -wg2.Add(1) +logsReady := make(chan struct{}) +done := make(chan struct{}) var expectedLogs []*coretypes.Log -var logs []*coretypes.Log +logs := make([]*coretypes.Log, 0) go func() { + defer close(done) for { select { case <-ctx.Done(): return case log := <-ch: - wg2.Wait() + <-logsReady logs = append(logs, log) if len(logs) == len(expectedLogs) { require.Equal(t, expectedLogs, logs) - wg.Done() + return } } } }()
1-178: Consider implementing a test helper package for common patternsThe test file implements several common patterns (WebSocket setup, transaction execution, event waiting) that could be extracted into a test helper package. This would:
- Reduce code duplication
- Provide consistent error handling
- Make tests more maintainable
- Ensure reliable cleanup of resources
Consider creating a
testutilpackage with helpers like:
WithWebSocket(t *testing.T, fn func(*websocket.Conn))WaitForEvent[T any](ctx context.Context, ch <-chan T, matcher func(T) bool) (T, error)WaitForIndexer(t *testing.T, app *app.MinitiaApp) errorjsonrpc/namespaces/eth/filters/api.go (1)
Line range hint
129-177: Consider adding safeguards for channel operations and memory managementWhile the
eventLoopimplementation is generally good, consider the following improvements:
- Add buffer size limits for channels to prevent memory exhaustion under high load
- Add error handling for channel operations
- Document channel closure handling strategy
- Consider adding metrics for monitoring subscription counts and event processing
Here's a suggested improvement:
func (api *FilterAPI) eventLoop() { + // Add metrics + var ( + activeSubscriptions = 0 + processedEvents = 0 + ) + + // Add periodic cleanup + cleanup := time.NewTicker(5 * time.Minute) + defer cleanup.Stop() + for { select { case block := <-api.blockChan: + processedEvents++ for _, s := range api.subscriptions { if s.ty == ethfilters.BlocksSubscription { + // Add non-blocking send with timeout + select { + case s.headerChan <- block: + case <-time.After(100 * time.Millisecond): + api.logger.Error("failed to send block to subscription", "id", s.id) + } } } // Similar changes for other event types... + case <-cleanup.C: + // Log metrics + api.logger.Info("event loop stats", + "active_subscriptions", activeSubscriptions, + "processed_events", processedEvents) } } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
jsonrpc/namespaces/eth/filters/api.go(7 hunks)jsonrpc/namespaces/eth/filters/api_test.go(1 hunks)jsonrpc/namespaces/eth/filters/subscriptions_test.go(1 hunks)tests/jsonrpc_creator.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- jsonrpc/namespaces/eth/filters/api_test.go
🔇 Additional comments (6)
tests/jsonrpc_creator.go (2)
27-36: LGTM! Well-structured test input definition
The TestInput struct is well-organized and contains all necessary components for comprehensive JSON-RPC testing, including application instance, indexer, backend, addresses, and network configurations.
50-90: Verify proper cleanup in dependent test files
The CreateAppWithJSONRPC function starts services but doesn't provide explicit cleanup mechanisms. We should verify that test files using this function properly clean up resources.
✅ Verification successful
Let me generate additional scripts to verify cleanup patterns in more test files and check the TestInput struct definition to understand what resources need cleanup.
Based on the gathered information, I can now generate the final response as the verification is complete.
Proper cleanup mechanisms are in place
The test files using CreateAppWithJSONRPC demonstrate appropriate cleanup patterns:
- All test cases properly defer
input.App.Close()to clean up the app resources - Context cancellation is handled with
defer cancel()for websocket connections - Websocket subscriptions are cleaned up with
sub.Unsubscribe()
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for tests using CreateAppWithJSONRPC and verify cleanup
echo "Searching for test files using CreateAppWithJSONRPC..."
rg -l "CreateAppWithJSONRPC" --type go | while read -r file; do
echo "Checking $file for cleanup patterns..."
# Look for cleanup patterns in test files
rg -A 10 "CreateAppWithJSONRPC" "$file"
done
Length of output: 1964
Script:
#!/bin/bash
# First, let's check the TestInput struct definition
echo "Checking TestInput struct definition..."
rg "type TestInput struct" -A 10 --type go
# Then, let's find all test files using CreateAppWithJSONRPC
echo -e "\nSearching for all test files using CreateAppWithJSONRPC..."
rg -l "CreateAppWithJSONRPC" --type go
# For each file found, let's look for defer statements and cleanup patterns
echo -e "\nChecking test cleanup patterns in all files..."
rg "CreateAppWithJSONRPC" -B 5 -A 15 --type go
Length of output: 7214
jsonrpc/namespaces/eth/filters/subscriptions_test.go (1)
1-18: LGTM! Package and imports are well organized.
jsonrpc/namespaces/eth/filters/api.go (3)
52-52: LGTM: Good optimization using RWMutex
The change from sync.Mutex to sync.RWMutex is a good optimization that allows multiple concurrent reads while maintaining exclusive writes, which is appropriate for this use case where read operations (checking filter status) are more frequent than writes.
213-217: LGTM: Consistent and correct usage of RLock/RUnlock
The changes to use RLock/RUnlock for read operations are well-implemented:
- Consistent pattern across all read operations
- Minimal lock scopes to reduce contention
- Proper lock/unlock pairing
Also applies to: 219-223, 265-269, 343-347, 409-411, 450-452
Line range hint 129-177: Verify concurrent access patterns and potential race conditions
Let's verify the concurrent access patterns and potential race conditions in the codebase.
Also applies to: 213-223, 265-269, 343-347, 409-411, 450-452
✅ Verification successful
Concurrent access patterns are properly synchronized
The verification results show that:
- The
api.filtersmap is properly protected byfiltersMut(RWMutex) with consistent locking patterns:- Write operations (install/uninstall) use
Lock() - Read operations use
RLock()
- Write operations (install/uninstall) use
- The
api.subscriptionsmap is only accessed from theeventLoopgoroutine, as noted in the comment "api.subscriptions is not thread-safe and should only be accessed from this goroutine" - The subscription channels (
install,uninstall) provide thread-safe communication between goroutines - No potential deadlock scenarios were found as the locks are acquired and released in a consistent order
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for potential race conditions and concurrent access patterns
# Look for other goroutines accessing the filters map
rg -A 5 "api\.filters\[" --type go
# Look for other subscription management code
rg -A 5 "api\.subscriptions\[" --type go
# Check for other RWMutex usage patterns
ast-grep --pattern 'sync.RWMutex'
# Look for potential deadlock scenarios with multiple locks
rg -A 5 "Lock|RLock" --type go
Length of output: 12097
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (5)
x/evm/config/config.go (2)
29-30: Enhance field documentationConsider expanding the documentation to explain the implications of disabling the indexer, similar to how it's described in the config template.
- // DisableIndexer is the flag to disable indexer + // DisableIndexer determines whether the EVM indexer is disabled. + // When true, JSON-RPC queries for blocks, transactions, and receipts will return empty results.
56-56: Enhance flag descriptionConsider providing a more detailed description that explains the implications of disabling the indexer.
- startCmd.Flags().Bool(flagDisableIndexer, DefaultDisableIndexer, "Disable evm indexer") + startCmd.Flags().Bool(flagDisableIndexer, DefaultDisableIndexer, "Disable EVM indexer (blocks JSON-RPC queries for blocks, transactions, and receipts)")indexer/abci.go (3)
Line range hint
196-204: Improve goroutine management for event emission.The current implementation could lead to goroutine leaks if the
donechannel receives before timeout. Consider usingcontext.WithTimeoutfor better cancellation handling.-done := make(chan struct{}) -go e.blockEventsEmitter(&blockEvents{header: &blockHeader, logs: blockLogs}, done) -go func() { - select { - case <-done: - case <-time.After(5 * time.Second): - e.logger.Error("block event emitter timed out") - } -}() +ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) +defer cancel() +done := make(chan struct{}) +go e.blockEventsEmitter(&blockEvents{header: &blockHeader, logs: blockLogs}, done) +select { +case <-done: + e.logger.Debug("block event emission completed") +case <-ctx.Done(): + e.logger.Error("block event emitter timed out") +}
Line range hint
206-215: Track the TODO for ABCI listener state changes.The TODO comment indicates a limitation in ABCI listener state changes. This should be tracked and addressed when the referenced issue is resolved.
Would you like me to create a GitHub issue to track this TODO and link it to cosmos/cosmos-sdk#22246?
Line range hint
35-217: Consider extracting complex logic into smaller functions.The
ListenFinalizeBlockmethod is quite long and handles multiple responsibilities. Consider breaking it down into smaller, focused functions for better maintainability and testability:
- Transaction processing
- Receipt generation
- Block header creation
- Event emission
Example refactor for the transaction processing part:
func (e *EVMIndexerImpl) processTxs(sdkCtx sdk.Context, req abci.RequestFinalizeBlock, res abci.ResponseFinalizeBlock, baseFee *big.Int) ([]*coretypes.Transaction, []*coretypes.Receipt, error) { txIndex := uint(0) cumulativeGasUsed := uint64(0) ethTxs := make([]*coretypes.Transaction, 0, len(req.Txs)) receipts := make([]*coretypes.Receipt, 0, len(req.Txs)) for idx, txBytes := range req.Txs { ethTx, receipt, err := e.processSingleTx(sdkCtx, txBytes, idx, res.TxResults[idx], &txIndex, &cumulativeGasUsed, baseFee) if err != nil { return nil, nil, err } if ethTx != nil { ethTxs = append(ethTxs, ethTx) receipts = append(receipts, receipt) } } return ethTxs, receipts, nil }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
indexer/abci.go(1 hunks)indexer/indexer.go(4 hunks)x/evm/config/config.go(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- indexer/indexer.go
🔇 Additional comments (4)
x/evm/config/config.go (3)
48-48: LGTM!
The implementation correctly loads the configuration value using appropriate type casting.
71-74: LGTM!
The template changes are well-documented and clearly explain the implications of the setting.
39-39: Verify impact of enabling indexer by default
Setting DisableIndexer to true by default could impact existing deployments and JSON-RPC functionality. Please verify:
- The impact on system resources when indexer is enabled
- The behavior of existing JSON-RPC clients
indexer/abci.go (1)
30-32: Consider adding debug logging for disabled state.
While the early return is a good practice, adding debug logging when the indexer is disabled would improve observability and help diagnose issues.
if !e.enabled {
+ e.logger.Debug("skipping block finalization - indexer is disabled")
return nil
}Let's verify the test coverage for this new condition:
Description
Tracking Issue: cosmos/cosmos-sdk#22650
Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
!in the type prefix if API or client breaking changeReviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
I have...
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation
Chores