fix(evm): ensure only one copy of StateDB when executing Ethereum txs #2165
Conversation
📝 WalkthroughWalkthroughThis pull request introduces comprehensive changes to the Nibiru EVM module, focusing on enhancing the management of state databases, transaction handling, and contract interactions. The modifications span multiple files across the EVM package, introducing a more structured approach to EVM transactions by integrating the Singleton StateDB pattern and refining how EVM contexts are managed during transaction processing. Changes
Possibly related PRs
Suggested reviewers
Poem
Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
| BaseFeeWei: baseFeeWeiPerGas, | ||
| } | ||
|
|
||
| stateDB := ctd.NewStateDB( |
There was a problem hiding this comment.
Creating a new StateDB here uses the anteCtx, which gets tossed when the tx runs. That means whenever StateDB commits, it's committing to a out of scope ctx.
It's better to just read the bank balances directly via the Cosmos ctx (which is what core.CanTransfer does anyways).
| @@ -173,7 +173,7 @@ func SendTransaction(s *BackendSuite, tx *gethcore.LegacyTx, waitForNextBlock bo | |||
|
|
|||
| // WaitForReceipt waits for a transaction to be included in a block, returns block number, block hash and receipt | |||
| func WaitForReceipt(s *BackendSuite, txHash gethcommon.Hash) (*big.Int, *gethcommon.Hash, *backend.TransactionReceipt) { | |||
| ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) | |||
| ctx, cancel := context.WithTimeout(context.Background(), 5*time.Minute) | |||
There was a problem hiding this comment.
I increased the wait time because a 5 second timeout was causing issues when I stepped through the code with a debugger (which takes longer than 5 seconds).
| @@ -154,10 +154,6 @@ func (s *BackendSuite) TestGasUsedFunTokens() { | |||
| s.Require().NotNil(blockNumber2) | |||
| s.Require().NotNil(blockNumber3) | |||
|
|
|||
| // TXs should have been included in the same block | |||
| s.Require().Equal(blockNumber1, blockNumber2) | |||
There was a problem hiding this comment.
Unnecessary to have the txs in the same block.
| @@ -494,7 +494,7 @@ func (n *Network) LatestHeight() (int64, error) { | |||
| // committed after a given block. If that height is not reached within a timeout, | |||
| // an error is returned. Regardless, the latest height queried is returned. | |||
| func (n *Network) WaitForHeight(h int64) (int64, error) { | |||
| return n.WaitForHeightWithTimeout(h, 40*time.Second) | |||
| return n.WaitForHeightWithTimeout(h, 5*time.Minute) | |||
There was a problem hiding this comment.
Same issue with the debugger timing out.
| // | ||
| // Note: This function handles both contract method calls and simulations, | ||
| // depending on the 'commit' parameter. | ||
| func (k Keeper) CallContract( |
There was a problem hiding this comment.
I realized that CallContract is basically CallContractWithInput, but CallContractWithInput is more generic because it allows for contract deployments. I refactored the code to just use CallContractWithInput and removed CallContract to reduce the API surface of EVMKeeper.
| fromAcc gethcommon.Address, | ||
| contract *gethcommon.Address, | ||
| commit bool, | ||
| contractInput []byte, | ||
| gasLimit uint64, | ||
| ) (evmResp *evm.MsgEthereumTxResponse, evmObj *vm.EVM, err error) { |
There was a problem hiding this comment.
We don't need to return evmObj anymore because it comes from the upper scope.
There was a problem hiding this comment.
Basically we pass in evmObj now from the upper scope.
| return gethcommon.Address{}, errors.Wrap(err, "failed to commit stateDB") | ||
| } | ||
| // Don't need the StateDB anymore because it's not usable after committing | ||
| k.Bank.StateDB = nil |
There was a problem hiding this comment.
We always have to set the Bank.StateDB to nil so that the StateDB doesn't accrue pending changes from the BeginBlocker/EndBlocker hooks (e.g. distribution and fee_collector balance changes).
Plus, there's no guarantee that StateDB is usable after Commit() is called, so it's better to just let the Golang garbage collector dispose of it.
| @@ -69,15 +69,15 @@ func (p precompileFunToken) Run( | |||
| method := startResult.Method | |||
| switch PrecompileMethod(method.Name) { | |||
| case FunTokenMethod_sendToBank: | |||
| bz, err = p.sendToBank(startResult, contract.CallerAddress, readonly) | |||
| bz, err = p.sendToBank(startResult, contract.CallerAddress, readonly, evm) | |||
There was a problem hiding this comment.
We pass in the evm object to precompile methods that need access to StateDB.
There was a problem hiding this comment.
Yes, this looks so much cleaner and less error-prone
k-yang
changed the title
![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
🧹 Nitpick comments (22)
x/evm/evmtest/tx.go (1)
362-374: Add documentation for MOCK_GETH_MESSAGE.While the mock message is a useful testing utility, its purpose and usage aren't clear. Consider:
- Adding documentation explaining when and how to use this mock.
- Making the mock more configurable through a constructor function.
+// MOCK_GETH_MESSAGE is a mock geth Message used for testing EVM message processing. +// It represents a basic transaction message with default values. var MOCK_GETH_MESSAGE = gethcore.NewMessage(x/evm/statedb/statedb.go (2)
Line range hint
673-679: Enhance error message for precompile limitThe error message could be more descriptive to help developers understand the impact and potential solutions.
- "exceeded maximum number Nibiru-specific precompiled contract calls in one transaction (%d).", + "exceeded maximum number of Nibiru-specific precompiled contract calls (%d) in one transaction. This limit prevents excessive state branching. Consider breaking down the transaction into smaller ones.",
Line range hint
642-651: Consider adding metrics for cache context usageTo monitor the performance impact of cache contexts, consider adding metrics for:
- Number of cache contexts created per transaction
- Memory usage of cached multistores
- Frequency of reaching the precompile call limit
x/evm/statedb/journal_test.go (5)
25-53: Add intermediate state assertions for better test coverageWhile the test effectively validates the final state, adding assertions after each state-modifying operation would provide better visibility into the state transitions.
Consider adding these assertions:
s.Require().NoError(err, deployResp) erc20 := deployResp.ContractAddr + // Assert dirty count after deployment + initialDirtyCount := stateDB.DebugDirtiesCount() + s.Require().Greater(initialDirtyCount, 0, "deployment should create dirty entries") input, err := deps.EvmKeeper.ERC20().ABI.Pack("mint", deps.Sender.EthAddr, big.NewInt(69_420)) s.Require().NoError(err)
55-79: Add balance validation for comprehensive testingThe test thoroughly checks dirty counts but should also validate the actual balance changes to ensure state modifications are correct.
Consider adding these balance checks:
stateDB.SubBalance(randomAcc, balDelta) + expectedBalance := new(big.Int).Add(balDelta, balDelta) + expectedBalance.Sub(expectedBalance, balDelta) + actualBalance := stateDB.GetBalance(randomAcc) + s.Require().Zero(expectedBalance.Cmp(actualBalance), + "balance mismatch after operations") + if stateDB.DebugDirtiesCount() != 4 {
Line range hint
138-160: Document security implications of contract self-callsThe test case for contract self-calls should explicitly document the security implications to help developers understand the importance of the restriction.
Add a more detailed comment explaining the security context:
s.Run("Transfer 69_000 tokens", func() { - // The contract calling itself is invalid in this context. + // SECURITY: Contract self-calls are restricted to prevent potential + // reentrancy attacks and maintain a clear caller hierarchy. This is + // particularly important for functions that modify state, as it prevents + // a contract from manipulating its own state in unexpected ways. // Note the comment in vm.Contract:
202-204: Document snapshot hierarchy for clarityAdd a diagram or detailed comment explaining the snapshot hierarchy to make the test flow clearer.
Consider adding this documentation:
// NOTE: that the [StateDB.Commit] fn has not been called yet. We're still // mid-transaction. + // Snapshot hierarchy: + // - Snapshot 3: Initial state (counter = 7) + // - Snapshot 5: After first increment (counter = 7 + 5) + // - Current state: After second increment (counter = 7 + 5 + 50)
Line range hint
242-267: Enhance debug output readabilityThe debug output could be more structured for easier parsing and reading.
Consider formatting the output like this:
func debugDirtiesCountMismatch(db *statedb.StateDB, t *testing.T) { lines := []string{} + lines = append(lines, "=== StateDB Debug Information ===") dirties := db.DebugDirties() stateObjects := db.DebugStateObjects() for addr, dirtyCount := range dirties { - lines = append(lines, fmt.Sprintf("Dirty addr: %s, dirtyCount=%d", addr, dirtyCount)) + lines = append(lines, fmt.Sprintf("\nAddress: %s\n Dirty Count: %d", addr, dirtyCount))x/evm/keeper/erc20.go (1)
Line range hint
187-212: RefactorloadERC20StringandloadERC20Uint8to reduce duplicationThe functions
loadERC20StringandloadERC20Uint8share similar logic. Consider refactoring them into a single generic function to eliminate code duplication and improve maintainability.Example refactor:
func (e erc20Calls) loadERC20Value[T any]( ctx sdk.Context, evmObj *vm.EVM, erc20Abi *gethabi.ABI, erc20Contract gethcommon.Address, methodName string, ) (out T, err error) { input, err := erc20Abi.Pack(methodName) if err != nil { return out, err } res, err := e.Keeper.CallContractWithInput( ctx, evmObj, evm.EVM_MODULE_ADDRESS, &erc20Contract, false, input, getCallGasWithLimit(ctx, Erc20GasLimitQuery), ) if err != nil { return out, err } err = erc20Abi.UnpackIntoInterface(&out, methodName, res.Ret) return out, err }Usage:
func (e erc20Calls) LoadERC20Name( ctx sdk.Context, evmObj *vm.EVM, abi *gethabi.ABI, erc20 gethcommon.Address, ) (string, error) { return e.loadERC20Value[string](ctx, evmObj, abi, erc20, "name") } func (e erc20Calls) LoadERC20Decimals( ctx sdk.Context, evmObj *vm.EVM, abi *gethabi.ABI, erc20 gethcommon.Address, ) (uint8, error) { return e.loadERC20Value[uint8](ctx, evmObj, abi, erc20, "decimals") }Also applies to: 221-246
x/evm/precompile/funtoken_test.go (1)
101-105: Refactor repeated EVM setup code into a helper functionThe initialization of
stateDBandevmObjis repeated in multiple test cases. Consider creating a helper function to reduce code duplication and enhance readability.Example:
func (deps *TestDeps) NewEVMObj(ctx sdk.Context) *vm.EVM { stateDB := deps.EvmKeeper.NewStateDB(ctx, statedb.NewEmptyTxConfig(gethcommon.BytesToHash(ctx.HeaderHash()))) return deps.EvmKeeper.NewEVM(ctx, evmtest.MOCK_GETH_MESSAGE, deps.EvmKeeper.GetEVMConfig(ctx), evm.NewNoOpTracer(), stateDB) }Usage in tests:
evmObj := deps.NewEVMObj(deps.Ctx)x/evm/evmtest/eth_test.go (1)
26-27: Ensure returned values are utilized or errors are handledIn the test:
s.T().Log("Test NewEthTxMsgAsCmt") _, _, _ = evmtest.NewEthTxMsgAsCmt(s.T())The returned values from
evmtest.NewEthTxMsgAsCmt(s.T())are ignored. If these values are unnecessary for the test, consider explicitly stating this with a comment. Alternatively, if errors or important data are returned, ensure they are properly handled or asserted.x/evm/evmtest/tx_test.go (1)
34-35: Clarify assertion description to match expected balanceIn the assertion:
evmtest.AssertBankBalanceEqualWithDescription( s.T(), deps, evm.EVMBankDenom, deps.Sender.EthAddr, big.NewInt(69_000), "expect nonzero balance", )The description "expect nonzero balance" does not accurately reflect the specific expected balance of
69_000. Consider updating the description to provide clearer context:-"expect nonzero balance" +"expect balance of 69,000 units after transfer"This enhances test readability and aids in diagnosing failures.
x/evm/keeper/erc20_test.go (1)
23-44: Consider adding test cases for edge casesWhile the basic mint functionality is tested, consider adding test cases for:
- Minting zero tokens
- Minting tokens that exceed uint256 max value
- Minting to zero address
x/evm/precompile/oracle_test.go (1)
89-97: Consider parameterizing gas limitThe gas limit is hardcoded in both contract calls. Consider making it a parameter of the test function for better flexibility.
Also applies to: 170-178
x/evm/keeper/funtoken_from_erc20.go (1)
127-140: Consider extracting EVM message configuration.The EVM message configuration contains multiple hardcoded values (gas price, value, etc.). Consider extracting these values into constants or configuration parameters for better maintainability.
+const ( + defaultGasPrice = 0 + defaultValue = 0 + defaultGasLimit = 0 +) + +func newDefaultEvmMessage(from, to gethcommon.Address, nonce uint64) gethcore.Message { + return gethcore.NewMessage( + from, + &to, + nonce, + big.NewInt(defaultValue), + defaultGasLimit, + big.NewInt(defaultGasPrice), + big.NewInt(defaultGasPrice), + big.NewInt(defaultGasPrice), + []byte{}, + gethcore.AccessList{}, + false, + ) +}x/evm/precompile/test/export.go (2)
65-77: Improved contract interaction with packed inputs.The change to use
CallContractWithInputwith packed ABI data is more robust than direct ABI calls. However, consider adding error handling for the ABI pack operation.contractInput, err := embeds.SmartContract_Wasm.ABI.Pack( string(precompile.WasmMethod_instantiate), m.Admin, m.CodeID, msgArgsBz, m.Label, []precompile.WasmBankCoin{}, ) -s.NoError(err) +if err != nil { + return nil, fmt.Errorf("failed to pack contract input: %w", err) +}
176-180: Consider adding retry mechanism for state queries.The state query might fail temporarily due to state transitions. Consider adding a retry mechanism with backoff for more robust testing.
x/evm/precompile/wasm_test.go (3)
38-40: Consider extracting test setup into helper function.The EVM setup code is repeated across multiple test functions. Consider extracting it into a helper function to improve maintainability.
+func setupTestEVM(deps *evmtest.TestDeps) *vm.EVM { + stateDB := deps.EvmKeeper.NewStateDB( + deps.Ctx, + statedb.NewEmptyTxConfig(gethcommon.BytesToHash(deps.Ctx.HeaderHash())), + ) + return deps.EvmKeeper.NewEVM( + deps.Ctx, + evmtest.MOCK_GETH_MESSAGE, + deps.EvmKeeper.GetEVMConfig(deps.Ctx), + evm.NewNoOpTracer(), + stateDB, + ) +}
506-534: Consider using table-driven subtests for error cases.The error test cases could be more concise using table-driven subtests. This would make it easier to add new test cases and maintain existing ones.
Line range hint
538-544: Improve test failure messages.The error assertions could be more descriptive to help diagnose test failures.
-s.Require().NoError(err) +s.Require().NoError(err, "failed to execute contract with input: %v", contractInput) -s.NotNil(ethTxResp) +s.NotNil(ethTxResp, "expected non-nil response for successful contract execution") -s.NotEmpty(ethTxResp.Ret) +s.NotEmpty(ethTxResp.Ret, "expected non-empty return value from contract execution")x/evm/keeper/funtoken_from_erc20_test.go (1)
Line range hint
332-336: Enhanced balance assertions with descriptive messages.The balance assertions now include descriptive messages, making test failures more informative and easier to debug.
Consider extracting the common balance assertion patterns into helper functions to reduce code duplication.
Also applies to: 354-354, 360-360, 370-370, 406-406, 413-413, 420-420, 450-450, 457-457, 464-464, 563-563, 571-571, 579-579, 641-641, 674-674, 682-682, 690-690, 698-698
x/evm/keeper/funtoken_from_coin_test.go (1)
510-531: Comprehensive balance checks with clear descriptions.The balance assertions are thorough and well-documented, checking multiple accounts and both bank and ERC20 balances.
Consider creating a test helper function to reduce the repetitive balance check blocks.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (35)
CHANGELOG.md(1 hunks)app/evmante/evmante_can_transfer.go(3 hunks)app/evmante/evmante_can_transfer_test.go(1 hunks)eth/rpc/backend/backend_suite_test.go(3 hunks)eth/rpc/backend/gas_used_test.go(0 hunks)x/common/testutil/testnetwork/network.go(1 hunks)x/evm/evmmodule/genesis_test.go(4 hunks)x/evm/evmtest/erc20.go(3 hunks)x/evm/evmtest/eth_test.go(1 hunks)x/evm/evmtest/evmante.go(2 hunks)x/evm/evmtest/test_deps.go(1 hunks)x/evm/evmtest/tx.go(4 hunks)x/evm/evmtest/tx_test.go(1 hunks)x/evm/keeper/bank_extension_test.go(2 hunks)x/evm/keeper/call_contract.go(3 hunks)x/evm/keeper/erc20.go(9 hunks)x/evm/keeper/erc20_test.go(1 hunks)x/evm/keeper/funtoken_from_coin.go(2 hunks)x/evm/keeper/funtoken_from_coin_test.go(21 hunks)x/evm/keeper/funtoken_from_erc20.go(4 hunks)x/evm/keeper/funtoken_from_erc20_test.go(13 hunks)x/evm/keeper/grpc_query.go(16 hunks)x/evm/keeper/msg_server.go(17 hunks)x/evm/keeper/random_test.go(1 hunks)x/evm/keeper/vm_config.go(3 hunks)x/evm/msg.go(0 hunks)x/evm/precompile/funtoken.go(9 hunks)x/evm/precompile/funtoken_test.go(8 hunks)x/evm/precompile/oracle_test.go(3 hunks)x/evm/precompile/test/export.go(8 hunks)x/evm/precompile/wasm_test.go(9 hunks)x/evm/statedb/journal_test.go(2 hunks)x/evm/statedb/statedb.go(2 hunks)x/evm/statedb/statedb_test.go(2 hunks)x/evm/tx.go(1 hunks)
💤 Files with no reviewable changes (2)
- eth/rpc/backend/gas_used_test.go
- x/evm/msg.go
✅ Files skipped from review due to trivial changes (2)
- x/evm/tx.go
- x/evm/statedb/statedb_test.go
🧰 Additional context used
📓 Learnings (1)
x/evm/keeper/funtoken_from_erc20_test.go (1)
Learnt from: k-yang
PR: NibiruChain/nibiru#2129
File: x/evm/embeds/contracts/TestInfiniteRecursionERC20.sol:24-32
Timestamp: 2024-12-26T04:43:43.966Z
Learning: The `TestInfiniteRecursionERC20` contract is intentionally designed to test infinite recursion scenarios in ERC20-based contracts for the `FunToken` architecture.
🔇 Additional comments (63)
x/evm/evmtest/tx.go (3)
Line range hint
69-79: LGTM! Improved state management by using EvmKeeper.The change from
deps.NewStateDB().GetNonce()todeps.EvmKeeper.GetAccNonce()aligns with the PR objective of maintaining a single StateDB instance, reducing the risk of state inconsistencies.
Line range hint
80-89: LGTM! Improved error handling.The error handling is now more idiomatic with better context through error wrapping and clearer control flow using early returns.
206-206: LGTM! Consistent use of EvmKeeper for nonce retrieval.The change maintains consistency with other nonce retrieval modifications, adhering to the single StateDB instance pattern.
x/evm/statedb/statedb.go (2)
264-267: LGTM: Consistent context management patternThe implementation correctly prioritizes the cached context when available, ensuring state consistency during EVM transaction execution.
Also applies to: 329-332
Line range hint
589-591: Verify transaction isolationThe commit pattern ensures atomic updates, but we should verify that no state leaks between transactions.
✅ Verification successful
Transaction isolation is properly maintained ✅
The cache context is properly scoped to each StateDB instance and changes are only written back during explicit commits. Since StateDB is recreated per transaction, there's no risk of state leaks between transactions.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for potential state leaks through the cache context rg -A 5 "CacheContext|writeToCommitCtxFromCacheCtx" --type go # Check for proper cleanup after commit ast-grep --pattern 'func $_(s *StateDB) { $$$ s.writeToCommitCtxFromCacheCtx = nil $$$ }'Length of output: 3422
x/evm/statedb/journal_test.go (1)
11-11: LGTM: Clean import organizationThe addition of the gethcommon import is appropriate for the new functionality.
x/evm/keeper/call_contract.go (1)
33-39: Function signature refactoring enhances clarity.Modifying
CallContractWithInputto includeevmObj *vm.EVMas a parameter makes the EVM context explicit, improving the function's clarity and consistency. This change aligns with the overall refactoring effort to passevmObjwhere necessary across the codebase.x/evm/keeper/funtoken_from_coin.go (1)
110-115: Properly dispose ofstateDBafter commit.After committing
stateDB, settingk.Bank.StateDBtonilensures that it doesn't accrue pending changes from hooks and avoids using an unusablestateDBafter commit. This practice aligns with best practices for managing thestateDBlifecycle.x/evm/keeper/erc20.go (3)
70-77: FunctionMintupdated correctly withevmObjparameterThe
Mintfunction now includes theevmObj *vm.EVMparameter and uses it appropriately when callingCallContractWithInput. This enhances the ability to maintain the EVM context during contract interactions.
Line range hint
91-120:Transferfunction correctly handles EVM context and balance checksThe
Transferfunction has been updated to include theevmObj *vm.EVMparameter. The balance checks before and after the transfer ensure that the recipient's balance increases as expected, accommodating fee-on-transfer tokens.
147-147:BalanceOffunction updated for EVM context consistencyIncluding
evmObj *vm.EVMin theBalanceOffunction maintains consistency across ERC20 interactions, ensuring that all contract calls are executed within the correct EVM context.x/evm/precompile/funtoken_test.go (5)
32-34: InitializedepsandfuntokeninFuntokenSuiteAdding
depsandfuntokento theFuntokenSuitestruct, and initializing them inSetupSuite, improves the test setup by providing necessary dependencies and the FunToken instance for testing.
182-198: Validate error handling when minting tokens from a non-ownerThe test case correctly checks that attempting to mint tokens from a non-owner account results in the expected "Ownable: caller is not the owner" error.
304-364: Improve test coverage for gas handling incallBankSendLocalGasThe test cases for
callBankSendLocalGasmethod adequately cover scenarios with sufficient and insufficient gas amounts. This ensures that gas limits are respected and errors are properly handled when gas is insufficient.
Line range hint
367-483: Ensure consistency in FunToken conversions between EVM and bank balancesThe tests for
TestSendToEvm_MadeFromCoineffectively validate the conversion of tokens from bank coins to EVM tokens and back. The balance checks confirm that the token amounts are correctly updated in both the EVM and bank module accounts.
Line range hint
491-626: Comprehensive testing of ERC20-based FunToken conversionsThe
TestSendToEvm_MadeFromERC20test covers the full cycle of token transfers between ERC20 tokens and bank coins, including transfers between accounts and the correct handling of balances after conversions.x/evm/keeper/msg_server.go (6)
50-64: Simplify EVM configuration retrieval inEthereumTxBy removing unnecessary parameters from
GetEVMConfigand directly usingevmCfg, the code becomes cleaner and reduces potential sources of error in configuration management.
Line range hint
118-145: RefactorNewEVMfunction for improved clarityThe
NewEVMfunction now acceptsevmCfg statedb.EVMConfig, which streamlines the EVM initialization process and aligns with the updated configuration handling throughout the codebase.
253-261: PassevmObjtoApplyEvmMsgfor better state managementIntroducing the
evmObj *vm.EVMparameter toApplyEvmMsgdecouples EVM creation from message application, allowing for greater flexibility and control over the EVM instance used during execution.
350-354: EnsureStateDBis properly committed and discardedAfter committing the
StateDB, settingk.Bank.StateDB = nilprevents any unintended reuse of the state database, which is essential for maintaining state integrity and avoiding potential side effects.
Line range hint
538-574: Handle errors and state updates when converting coins to EVM tokensIn
convertCoinToEvmBornCoin, the steps to mint ERC20 tokens are clearly defined, and error handling is properly implemented. Ensure that the EVM response is checked for failures to handle any potential execution errors.
Line range hint
605-674: Correctly manage token transfers inconvertCoinToEvmBornERC20The function accurately handles the conversion process for tokens born from an ERC20, including burning bank coins and transferring ERC20 tokens. Committing the
StateDBensures that state changes are properly saved.x/evm/keeper/grpc_query.go (12)
273-273: Retrieval of EVM configuration improves code clarityThe use of
evmCfg := k.GetEVMConfig(ctx)streamlines the retrieval of EVM configuration parameters, enhancing code readability and maintainability.
279-279: Proper message construction with updated EVM configurationConstructing the message with
args.ToMessage(req.GasCap, evmCfg.BaseFeeWei)ensures that the base fee is accurately incorporated, aligning with the updated configuration.
287-289: Appropriate instantiation of StateDB and EVM instancesInitializing
stateDBandevmwith the updated configuration enhances clarity in the transaction processing flow.
327-327: Consistent retrieval of EVM configurationAgain, using
evmCfg := k.GetEVMConfig(ctx)maintains consistency across methods and reduces redundancy.
372-372: Accurate message conversion for gas estimationConverting
argstoevmMsgusingargs.ToMessage(req.GasCap, evmCfg.BaseFeeWei)ensures that gas estimation is based on the correct fee parameters.
382-393: Efficient gas limit adjustment in message reconstructionReconstructing
evmMsgwith the updated gas limit is correctly handled to facilitate binary search for gas estimation.
507-507: Message construction includes updated base feeUsing
ethTx.AsMessage(signer, evmCfg.BaseFeeWei)ensures that the Ethereum transaction message incorporates the correct base fee from the updated configuration.
538-538: Correct message creation for transaction tracingConstructing the message with
tx.AsMessage(signer, evmCfg.BaseFeeWei)accurately reflects the transaction parameters for tracing.
622-622: Correct invocation ofTraceEthTxMsgwith updated configurationThe call to
k.TraceEthTxMsgincludes the updatedevmCfg, ensuring that the tracing operates under the correct EVM settings.
674-674: Proper conversion of base fee to Wei unitsConverting
baseFeeMicronibiPerGasto Wei withevm.NativeToWei(baseFeeMicronibiPerGas)before assigning toevmCfg.BaseFeeWeiensures accurate fee calculations.
693-693: Accurate message extraction from Ethereum transactionCreating the message with
ethTx.AsMessage(signer, evmCfg.BaseFeeWei)correctly incorporates the base fee for message execution.
791-793: Consistent EVM and StateDB initialization for transaction executionInitializing
stateDBandevmObjwith the current context and configuration maintains consistency in transaction execution and tracing.x/evm/evmtest/tx_test.go (1)
34-35: Use of enhanced assertion method improves test clarityReplacing
AssertBankBalanceEqualwithAssertBankBalanceEqualWithDescriptionadds valuable context to the test output, aiding in debugging and understanding test results.x/evm/evmtest/test_deps.go (1)
52-52: Verify the return type ofHeaderHash()The change from
deps.Ctx.HeaderHash().Bytes()todeps.Ctx.HeaderHash()seems unnecessary sinceHeaderHash()should already return bytes. This might cause type safety issues ifHeaderHash()doesn't return the expected type.Let's verify the return type of
HeaderHash():x/evm/keeper/random_test.go (3)
18-19: LGTM! Proper initialization of StateDB and EVM instance.The explicit creation of StateDB and EVM instance before the test ensures clean state management.
25-26: LGTM! Better modularity with ERC20 accessor.The change from direct keeper call to
ERC20().LoadERC20BigIntimproves modularity and encapsulation.
34-35: LGTM! Proper state reset after block time change.Correctly reinitializing StateDB and EVM instance after block time change ensures clean state for the second random value generation.
x/evm/keeper/vm_config.go (2)
14-20: LGTM! Simplified GetEVMConfig with better return type.The function now returns a value instead of a pointer, which is more appropriate since EVMConfig is a small struct and doesn't need pointer semantics.
59-65: LGTM! Improved error handling with clear documentation.The function now gracefully handles the case of missing validator by returning an empty address. The added comment clearly explains why this is acceptable in a PoS context.
app/evmante/evmante_can_transfer_test.go (1)
17-19: LGTM! Removed unused field.Good cleanup by removing the unused
ctxSetupfield from the test case struct.x/evm/keeper/erc20_test.go (2)
19-21: LGTM: Proper initialization of EVM contextThe creation of a single StateDB instance and EVM object aligns with the PR objective of ensuring only one copy of StateDB exists during transaction execution.
85-86: Verify balance assertion after burnThe balance assertions look correct, but consider adding a total supply check after burning to ensure tokens are properly destroyed.
x/evm/evmtest/erc20.go (1)
19-38: LGTM: Improved error reporting in balance assertionsThe addition of descriptions to balance assertions enhances debugging capabilities. The error message construction properly handles empty descriptions.
x/evm/evmmodule/genesis_test.go (1)
54-70: LGTM: Proper EVM context initializationThe creation of EVM context with a single StateDB instance is well-structured and includes all necessary parameters.
x/evm/keeper/funtoken_from_erc20.go (2)
31-36: Function signature change improves EVM context handling.The addition of the
evmObjparameter toFindERC20Metadataenhances the function by providing a consistent EVM execution context for all ERC20 operations.
122-126: Verify StateDB initialization and error handling.The StateDB initialization includes a nil check and fallback creation, but consider the following:
- The comment mentions "dummy values" but doesn't explain why they're acceptable.
- The error case for
NewStateDBis not handled.✅ Verification successful
StateDB initialization pattern is correct and consistent with codebase.
The "dummy values" comment is appropriate as this is a standard pattern used throughout the codebase for state queries. The nil check provides sufficient safety, and the lack of error handling is consistent with the established pattern in the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other instances of StateDB initialization to verify consistency rg "NewStateDB.*NewEmptyTxConfig" -A 2Length of output: 14970
eth/rpc/backend/backend_suite_test.go (1)
176-176: Increased timeout aligns with debugging needs.The timeout increase from 5 seconds to 5 minutes is reasonable for debugging purposes, as noted in the past review comments.
x/common/testutil/testnetwork/network.go (1)
497-497: LGTM: Increased timeout to handle debugger delays.The timeout extension from 40 seconds to 5 minutes addresses the debugger timing out issue mentioned in the past review comments. This change is reasonable for test environments where debugging sessions may require longer wait times.
x/evm/keeper/funtoken_from_erc20_test.go (3)
11-12: LGTM: Added necessary imports for EVM state management.The new imports support the StateDB and EVM object management changes, improving the test's ability to handle Ethereum state.
Also applies to: 23-24
46-48: LGTM: Proper EVM state initialization.The code correctly initializes the StateDB and EVM object with appropriate configuration, ensuring accurate state management during tests.
Also applies to: 49-51
62-71: Well-structured test organization with comprehensive scenarios.The test cases are well-organized into sub-tests, covering:
- Insufficient funds scenario
- Happy path with proper event verification
- Already registered ERC20 scenario
- Invalid sender scenario
- Missing ERC20 address scenario
This structure improves test readability and maintenance.
Also applies to: 73-127, 129-168
x/evm/precompile/funtoken.go (3)
72-72: LGTM: Consistent EVM object propagation in precompile methods.The changes ensure that the EVM object is properly passed through the precompile methods, aligning with the architectural decision mentioned in past reviews.
Also applies to: 74-74, 80-80
174-182: Proper state management in ERC20 operations.The code correctly manages EVM state during ERC20 operations:
- Transfer from caller to EVM module
- Burn operation for coin-originated tokens
Also applies to: 195-197
625-626: LGTM: Consistent state handling in ERC20 mint/transfer operations.The code maintains consistent state handling whether minting new tokens or transferring existing ones, properly using the EVM object for state management.
Also applies to: 631-636, 644-645, 650-650
x/evm/keeper/funtoken_from_coin_test.go (2)
32-34: LGTM: Consistent EVM state initialization across test cases.The code properly initializes StateDB and EVM objects with appropriate configuration in all test cases, ensuring consistent state management.
Also applies to: 175-176, 485-486, 601-602
377-383: Well-structured contract interactions with clear parameter packing.The contract input preparation is clear and consistent, with proper parameter packing for various test scenarios.
Also applies to: 423-427, 536-542, 649-655
x/evm/evmtest/evmante.go (2)
23-31: LGTM! Clear and consistent variable naming.The renaming from
ethContractCreationTxParamstoevmTxArgsimproves code clarity and maintains consistency with other EVM-related code.
70-78: LGTM! Proper contract creation setup.The explicit setting of
To: nilfor contract creation transactions is correct and follows EVM standards.x/evm/keeper/bank_extension_test.go (2)
361-366: LGTM! Improved test structure.The test setup is cleaner and more focused on verifying StateDB consistency.
425-425: LGTM! Proper StateDB pointer comparison.The test correctly verifies that the same StateDB instance is maintained throughout the operations.
CHANGELOG.md (1)
81-81: LGTM! Clear and well-placed changelog entry.The changelog entry accurately describes the implementation of the Singleton StateDB pattern for EVM transactions.
x/evm/precompile/oracle_test.go
Outdated
| contractInput, err := embeds.SmartContract_Oracle.ABI.Pack( | ||
| string(precompile.OracleMethod_queryExchangeRate), | ||
| "unibi:uusd", | ||
| ) | ||
| s.Require().NoError(err) | ||
| txConfig := deps.EvmKeeper.TxConfig(ctx, gethcommon.BigToHash(big.NewInt(0))) | ||
| stateDB := deps.EvmKeeper.NewStateDB(ctx, txConfig) | ||
| evmCfg := deps.EvmKeeper.GetEVMConfig(ctx) | ||
| evmMsg := gethcore.NewMessage( | ||
| evm.EVM_MODULE_ADDRESS, | ||
| &evm.EVM_MODULE_ADDRESS, | ||
| deps.EvmKeeper.GetAccNonce(ctx, evm.EVM_MODULE_ADDRESS), | ||
| big.NewInt(0), | ||
| OracleGasLimitQuery, | ||
| big.NewInt(0), | ||
| big.NewInt(0), | ||
| big.NewInt(0), | ||
| contractInput, | ||
| gethcore.AccessList{}, | ||
| false, | ||
| ) | ||
| evmObj := deps.EvmKeeper.NewEVM(ctx, evmMsg, evmCfg, nil /*tracer*/, stateDB) | ||
| return deps.EvmKeeper.CallContractWithInput( |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor duplicated EVM context creation
The EVM context creation code is duplicated between the two test cases. Consider extracting this into a helper function.
+ func createEvmContext(deps evmtest.TestDeps, ctx sdk.Context, contractInput []byte) *vm.EVM {
+ txConfig := deps.EvmKeeper.TxConfig(ctx, gethcommon.BigToHash(big.NewInt(0)))
+ stateDB := deps.EvmKeeper.NewStateDB(ctx, txConfig)
+ evmCfg := deps.EvmKeeper.GetEVMConfig(ctx)
+ evmMsg := gethcore.NewMessage(
+ evm.EVM_MODULE_ADDRESS,
+ &evm.EVM_MODULE_ADDRESS,
+ deps.EvmKeeper.GetAccNonce(ctx, evm.EVM_MODULE_ADDRESS),
+ big.NewInt(0),
+ OracleGasLimitQuery,
+ big.NewInt(0),
+ big.NewInt(0),
+ big.NewInt(0),
+ contractInput,
+ gethcore.AccessList{},
+ false,
+ )
+ return deps.EvmKeeper.NewEVM(ctx, evmMsg, evmCfg, nil /*tracer*/, stateDB)
+ }Also applies to: 148-170
| err = stateDB.Commit() | ||
| if err != nil { | ||
| return nil, errors.Wrap(err, "failed to commit stateDB") | ||
| } | ||
| // Don't need the StateDB anymore because it's not usable after committing | ||
| k.Bank.StateDB = nil | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ensure StateDB cleanup is handled correctly.
The code commits the StateDB and sets it to nil, but:
- Consider using
deferto ensure cleanup happens even in error cases. - Document why setting StateDB to nil is necessary after commit.
+// Commit the StateDB and clean it up as it's not usable after committing
+defer func() {
+ if stateDB != nil {
+ if err := stateDB.Commit(); err != nil {
+ err = errors.Wrap(err, "failed to commit stateDB")
+ }
+ // StateDB is not usable after committing
+ k.Bank.StateDB = nil
+ }
+}()
-err = stateDB.Commit()
-if err != nil {
- return nil, errors.Wrap(err, "failed to commit stateDB")
-}
-// Don't need the StateDB anymore because it's not usable after committing
-k.Bank.StateDB = nilCommittable suggestion skipped: line range outside the PR's diff.
| ethTxResp, err := deps.EvmKeeper.CallContractWithInput( | ||
| deps.Ctx, | ||
| evmObj, | ||
| deps.Sender.EthAddr, | ||
| &precompile.PrecompileAddr_Wasm, | ||
| finalizeTx, | ||
| commit, | ||
| input, | ||
| WasmGasLimitExecute, | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error propagation in contract calls.
The function swallows the error from CallContractWithInput. Consider propagating it to allow better error handling in tests.
ethTxResp, err := deps.EvmKeeper.CallContractWithInput(
deps.Ctx,
evmObj,
deps.Sender.EthAddr,
&precompile.PrecompileAddr_Wasm,
commit,
input,
WasmGasLimitExecute,
)
-s.Require().NoError(err)
+if err != nil {
+ return fmt.Errorf("failed to call contract: %w", err)
+}Committable suggestion skipped: line range outside the PR's diff.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
k-yang
force-pushed
the
refactor/evm/call-contract
branch
from
c66de45 to
b4f3ac8
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2165 +/- ##
==========================================
- Coverage 65.14% 64.86% -0.29%
==========================================
Files 277 277
Lines 22238 22283 +45
==========================================
- Hits 14488 14454 -34
- Misses 6760 6839 +79
Partials 990 990
|
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (1)
x/evm/keeper/grpc_query.go (1)
491-495:⚠️ Potential issuePotential unit mismatch when assigning base fee.
The base fee is assigned directly without proper unit conversion, which could lead to incorrect values being used in the EVM configuration.
Apply this diff to ensure proper unit conversion:
evmCfg := k.GetEVMConfig(ctx) baseFeeWeiPerGas := k.BaseFeeWeiPerGas(ctx) if baseFeeWeiPerGas != nil { - evmCfg.BaseFeeWei = baseFeeWeiPerGas + evmCfg.BaseFeeWei = evm.NativeToWei(baseFeeWeiPerGas) }
🧹 Nitpick comments (21)
x/evm/precompile/funtoken_test.go (5)
80-115: Consider adding edge cases for address validation.While the test covers basic address validation, consider adding edge cases for:
- Invalid hex addresses
- Malformed bech32 addresses
- Zero addresses
Line range hint
119-246: Add test cases for concurrent token operations.The happy path tests cover basic operations, but consider adding test cases for:
- Concurrent token transfers
- Race conditions in state updates
- Edge cases around zero amounts
Line range hint
349-464: Consider testing token precision and rounding.While testing bank to EVM conversion, add test cases for:
- Token amounts with decimal places
- Maximum token amounts
- Minimum token amounts
Line range hint
472-606: Add failure recovery test cases.The ERC20 conversion tests should include scenarios for:
- Failed transfers
- Transaction reversals
- State recovery after failures
608-693: Consider adding validation in parsing functions.The response parsing functions could benefit from additional validation:
- Check for nil response
- Validate returned values are within expected ranges
- Add more descriptive error messages
Example improvement:
func (out FunTokenWhoAmIReturn) ParseFromResp( evmResp *evm.MsgEthereumTxResponse, ) (ethAddr gethcommon.Address, bech32Addr string, err error) { + if evmResp == nil { + return ethAddr, "", fmt.Errorf("nil response") + } + if len(evmResp.Ret) == 0 { + return ethAddr, "", fmt.Errorf("empty response data") + } err = embeds.SmartContract_FunToken.ABI.UnpackIntoInterface( &out, "whoAmI", evmResp.Ret, ) if err != nil { return } + if out.NibiruAcc.EthAddr == (gethcommon.Address{}) { + return ethAddr, "", fmt.Errorf("invalid ethereum address") + } + if out.NibiruAcc.Bech32Addr == "" { + return ethAddr, "", fmt.Errorf("invalid bech32 address") + } return out.NibiruAcc.EthAddr, out.NibiruAcc.Bech32Addr, nil }x/evm/precompile/wasm_test.go (4)
38-115: Consider enhancing test coverage with gas and event assertions.The test successfully validates the basic functionality, but could be enhanced by:
- Asserting gas consumption is within expected ranges
- Validating emitted events match expected values
Example addition:
ethTxResp, err := deps.EvmKeeper.CallContractWithInput( deps.Ctx, evmObj, deps.Sender.EthAddr, &precompile.PrecompileAddr_Wasm, true, contractInput, WasmGasLimitExecute, ) s.Require().NoError(err) s.Require().NotEmpty(ethTxResp.Ret) +// Assert gas usage +s.Assert().Less(ethTxResp.GasUsed, WasmGasLimitExecute, "Gas usage should be within limits") + +// Assert expected events were emitted +events := deps.Ctx.EventManager().Events() +s.Assert().Contains(events, sdk.Event{ + Type: "wasm", + Attributes: []sdk.Attribute{ + {Key: "action", Value: "create_denom"}, + }, +})
119-138: Add error case validation for counter increments.While the happy path is well tested, consider adding test cases for:
- Overflow conditions
- Invalid increment values
- Concurrent increment attempts
Line range hint
472-492: Add test cases for transaction reverts.Consider adding test cases for:
- Out of gas scenarios
- Transaction reverts due to state changes
- Partial execution rollbacks
Line range hint
503-545: Enhance atomicity validation.While the test effectively validates basic atomicity, consider adding:
- State validation for other contracts affected by the transaction
- Bank balance checks to ensure no partial transfers occurred
- Event validation to confirm no partial events were emitted
Example addition:
// Verify that no state changes occurred test.AssertWasmCounterState(&s.Suite, deps, evmObj, wasmContract, 0) + +// Verify no partial events were emitted +events := deps.Ctx.EventManager().Events() +s.Assert().Empty(events, "No events should be emitted on failure") + +// Verify bank balances remained unchanged +balance := deps.App.BankKeeper.GetBalance(deps.Ctx, deps.Sender.NibiruAddr, "unibi") +s.Assert().Equal(sdk.NewInt(100), balance.Amount, "Balance should remain unchanged")x/evm/statedb/journal_test.go (5)
24-51: Add intermediate state verification.The test verifies the final state after commit but doesn't check the intermediate state. Consider adding assertions to verify the dirty count before commit to ensure state changes are properly tracked.
) s.Require().NoError(err) + // Verify dirty count before commit + s.Require().NotEqual(0, evmObj.StateDB.(*statedb.StateDB).DebugDirtiesCount(), "Expected non-zero dirty count before commit") s.Require().EqualValues(0, evmObj.StateDB.(*statedb.StateDB).DebugDirtiesCount())
80-80: Rename test to better reflect its content.The current test name
TestContractCallsAnotherContractdoesn't fully reflect that it's testing both successful transfers and invalid self-calls. Consider a more descriptive name likeTestERC20TransfersAndInvalidCalls.
138-138: Use distinct names for sub-tests.This sub-test has the same name as the previous one but tests a different scenario (invalid self-call). Consider renaming it to "Transfer 69_000 tokens - Invalid self-call" for clarity.
214-216: Extract magic numbers into named constants.The test uses magic numbers for counter values in assertions. Consider extracting these into named constants for better readability and maintenance.
+const ( + initialIncrement = 7 + secondIncrement = 5 + thirdIncrement = 50 +) + test.AssertWasmCounterState( - &s.Suite, deps, evmObj, helloWorldCounterWasm, 7+5+50, + &s.Suite, deps, evmObj, helloWorldCounterWasm, initialIncrement+secondIncrement+thirdIncrement, )Also applies to: 225-227, 230-232
Line range hint
242-268: Consider using structured logging.The debug function builds a string representation of the state. Consider using structured logging for better parsing and filtering of debug information.
-func debugDirtiesCountMismatch(db *statedb.StateDB, t *testing.T) { +func debugDirtiesCountMismatch(db *statedb.StateDB, t *testing.T) { dirties := db.DebugDirties() stateObjects := db.DebugStateObjects() for addr, dirtyCount := range dirties { - lines = append(lines, fmt.Sprintf("Dirty addr: %s, dirtyCount=%d", addr, dirtyCount)) + t.Logf("State object details: %+v", map[string]interface{}{ + "address": addr, + "dirtyCount": dirtyCount, + "balance": maybeObj.Balance().String(), + "suicided": maybeObj.Suicided, + "dirtyCode": maybeObj.DirtyCode, + "storageEntries": len(maybeObj.DirtyStorage), + })x/evm/keeper/funtoken_from_erc20_test.go (4)
95-105: Consider adding more event assertions.While the test verifies the
EventFunTokenCreatedevent, it would be beneficial to also assert the event attributes individually for more thorough validation.testutil.RequireContainsTypedEvent( s.T(), deps.Ctx, &evm.EventFunTokenCreated{ BankDenom: expectedBankDenom, Erc20ContractAddress: erc20Addr.String(), Creator: deps.Sender.NibiruAddr.String(), IsMadeFromCoin: false, }, ) +// Assert individual event attributes +events := deps.Ctx.EventManager().Events() +for _, event := range events { + if event.Type == "event_funtoken_created" { + for _, attr := range event.Attributes { + switch string(attr.Key) { + case "bank_denom": + s.Equal(expectedBankDenom, string(attr.Value)) + case "erc20_contract_address": + s.Equal(erc20Addr.String(), string(attr.Value)) + // ... assert other attributes + } + } + } +}
154-164: Enhance error message specificity.The error message "either the "from_erc20" or "from_bank_denom" must be set" could be more specific by indicating which field was actually provided.
-s.ErrorContains(err, "either the \"from_erc20\" or \"from_bank_denom\" must be set") +s.ErrorContains(err, fmt.Sprintf( + "invalid request: either \"from_erc20\" or \"from_bank_denom\" must be set, got from_erc20=%v, from_bank_denom=%q", + nil, + "", +))
202-213: Extract magic numbers into named constants.The gas limit value is hardcoded. Consider extracting it into a named constant for better maintainability.
+const ( + // DefaultMintGasLimit represents the gas limit for minting ERC20 tokens + DefaultMintGasLimit = keeper.Erc20GasLimitExecute +) contractInput, err := embeds.SmartContract_ERC20Minter.ABI.Pack("mint", deps.Sender.EthAddr, big.NewInt(69_420)) s.Require().NoError(err) evmObj, _ := deps.NewEVM() _, err = deps.EvmKeeper.CallContractWithInput( deps.Ctx, evmObj, deps.Sender.EthAddr, /*from*/ &deployResp.ContractAddr, /*to*/ true, /*commit*/ contractInput, - keeper.Erc20GasLimitExecute, + DefaultMintGasLimit, )
338-343: Document the security implications of each test case.The test cases for malicious contracts would benefit from detailed documentation explaining the security implications and mitigation strategies.
Add documentation blocks explaining:
- The attack vector being tested
- The expected behavior
- The mitigation strategy
- The potential impact if not handled correctly
Example:
+// TestFunTokenFromERC20MaliciousTransfer verifies that the system: +// 1. Properly handles malicious contracts that attempt to exploit gas limits +// 2. Prevents potential DoS attacks through excessive gas consumption +// 3. Maintains consistent state even when transactions fail +// 4. Protects against economic attacks by enforcing gas limits s.Require().NoError(testapp.FundAccount( deps.App.BankKeeper, deps.Ctx, deps.Sender.NibiruAddr, deps.EvmKeeper.FeeForCreateFunToken(deps.Ctx), ))Also applies to: 394-399, 464-469
x/evm/keeper/grpc_query.go (3)
Line range hint
372-393: Standardize error handling patterns.The error handling in the message creation could be more descriptive and consistent.
evmMsg, err := args.ToMessage(req.GasCap, evmCfg.BaseFeeWei) if err != nil { - return nil, grpcstatus.Error(grpccodes.Internal, err.Error()) + return nil, grpcstatus.Errorf( + grpccodes.Internal, + "failed to create message from args: %v, gas cap: %d, base fee: %s", + err, req.GasCap, evmCfg.BaseFeeWei, + ) }
Line range hint
334-393: Document gas estimation strategy.The binary search implementation for gas estimation would benefit from detailed documentation explaining the strategy and edge cases.
Add documentation explaining:
- Why binary search is used
- The bounds selection strategy
- Edge cases and how they're handled
- Performance characteristics
Example:
+// Binary search strategy for gas estimation: +// 1. Start with lo = TxGas - 1 and hi = min(user specified gas, block gas limit) +// 2. Try mid point and adjust bounds based on execution success +// 3. Continue until optimal gas value is found +// 4. Handle edge cases: +// - Out of gas errors +// - Execution reverts +// - Insufficient funds +// Time complexity: O(log n) where n is the gas range executable := func(gas uint64) (vmError bool, rsp *evm.MsgEthereumTxResponse, err error) { // ... existing code ... }
Line range hint
722-793: Add safety checks for tracer configuration.The tracer configuration handling could benefit from additional validation to prevent potential issues.
+const maxTracerTimeout = 30 * time.Second + func (k *Keeper) TraceEthTxMsg( ctx sdk.Context, evmCfg statedb.EVMConfig, txConfig statedb.TxConfig, msg gethcore.Message, traceConfig *evm.TraceConfig, tracerJSONConfig json.RawMessage, ) (*any, uint, error) { + // Validate tracer JSON config + if len(tracerJSONConfig) > 0 { + var rawMap map[string]interface{} + if err := json.Unmarshal(tracerJSONConfig, &rawMap); err != nil { + return nil, 0, grpcstatus.Errorf( + grpccodes.InvalidArgument, + "invalid tracer config: %v", + err, + ) + } + } + + // Ensure timeout is within reasonable bounds + if timeout > maxTracerTimeout { + return nil, 0, grpcstatus.Errorf( + grpccodes.InvalidArgument, + "timeout %v exceeds maximum allowed %v", + timeout, + maxTracerTimeout, + ) + } // ... rest of the implementation }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
app/evmante/evmante_can_transfer.go(3 hunks)x/evm/evmmodule/genesis_test.go(4 hunks)x/evm/evmtest/test_deps.go(2 hunks)x/evm/keeper/erc20.go(9 hunks)x/evm/keeper/erc20_test.go(1 hunks)x/evm/keeper/funtoken_from_coin_test.go(19 hunks)x/evm/keeper/funtoken_from_erc20_test.go(11 hunks)x/evm/keeper/grpc_query.go(16 hunks)x/evm/keeper/random_test.go(1 hunks)x/evm/precompile/funtoken_test.go(8 hunks)x/evm/precompile/nibiru_evm_utils_test.go(2 hunks)x/evm/precompile/oracle_test.go(2 hunks)x/evm/precompile/wasm_test.go(9 hunks)x/evm/statedb/journal_test.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- x/evm/evmtest/test_deps.go
- x/evm/keeper/random_test.go
🧰 Additional context used
📓 Learnings (1)
x/evm/keeper/funtoken_from_erc20_test.go (1)
Learnt from: k-yang
PR: NibiruChain/nibiru#2129
File: x/evm/embeds/contracts/TestInfiniteRecursionERC20.sol:24-32
Timestamp: 2024-12-26T04:43:43.966Z
Learning: The `TestInfiniteRecursionERC20` contract is intentionally designed to test infinite recursion scenarios in ERC20-based contracts for the `FunToken` architecture.
🪛 GitHub Check: lint
x/evm/keeper/erc20_test.go
[failure] 37-37:
Error return value of stateDB.Commit is not checked (errcheck)
[failure] 66-66:
Error return value of stateDB.Commit is not checked (errcheck)
[failure] 84-84:
Error return value of stateDB.Commit is not checked (errcheck)
[failure] 99-99:
Error return value of stateDB.Commit is not checked (errcheck)
🪛 golangci-lint (1.62.2)
x/evm/keeper/erc20_test.go
[error] 37-37: Error return value of stateDB.Commit is not checked
(errcheck)
[error] 66-66: Error return value of stateDB.Commit is not checked
(errcheck)
[error] 84-84: Error return value of stateDB.Commit is not checked
(errcheck)
[error] 99-99: Error return value of stateDB.Commit is not checked
(errcheck)
🔇 Additional comments (11)
x/evm/precompile/funtoken_test.go (2)
Line range hint
1-29: LGTM! Well-structured test suite setup.The test suite follows Go testing best practices using testify/suite.
Line range hint
32-75: LGTM! Comprehensive error case testing.The test cases thoroughly cover various error scenarios for ABI packing.
x/evm/precompile/wasm_test.go (2)
33-35: LGTM! Standard test suite setup.The test suite setup follows Go testing best practices using testify/suite.
332-349: LGTM! Well-structured error case validation.The table-driven tests effectively validate error conditions with clear error messages.
x/evm/statedb/journal_test.go (1)
53-78: Well-structured test with clear expectations!The test thoroughly validates the StateDB behavior with:
- Clear documentation of expected dirty counts
- Comprehensive balance operations (add/sub)
- Proper state verification before and after commit
- Detailed debug information for failures
x/evm/precompile/oracle_test.go (1)
65-71: Refactor duplicated code for creatingcontractInputandevmObjThe code for packing
contractInputand creatingevmObjis duplicated in therunQueryfunction and in the"test IOracle.chainLinkLatestRoundData"block. Consider extracting this logic into a helper function to improve maintainability and reduce code duplication.Also applies to: 129-136
app/evmante/evmante_can_transfer.go (1)
67-80: LGTM! Improved balance check implementation.The changes correctly remove the redundant StateDB creation and instead use bank balance directly. This aligns with the PR objective of ensuring only one copy of StateDB when executing Ethereum transactions.
x/evm/keeper/erc20.go (1)
70-77: LGTM! Consistent addition of evmObj parameter.The changes consistently add the evmObj parameter across all ERC20-related functions, which aligns with the PR objective of maintaining a single StateDB instance.
Also applies to: 91-103, 159-166
x/evm/keeper/erc20_test.go (1)
17-24: LGTM! Well-structured test cases.The test cases are well-organized using s.Run blocks and properly test various scenarios including non-owner operations, insufficient funds, and successful transfers.
Also applies to: 26-41, 43-53, 55-72, 74-87, 89-102
x/evm/precompile/nibiru_evm_utils_test.go (1)
28-30: LGTM! Standard test suite setup.The addition of TestUtilsSuite follows Go testing conventions for setting up test suites using testify.
x/evm/keeper/funtoken_from_erc20_test.go (1)
58-164: Well-structured test cases with clear separation of concerns!The test cases are well-organized using sub-tests, making it easy to understand and maintain. Each test case has a clear description and consistent setup.
| @@ -27,7 +27,8 @@ func (s *FunTokenFromCoinSuite) TestCreateFunTokenFromCoin() { | |||
| // Compute contract address. FindERC20 should fail | |||
| nonce := deps.NewStateDB().GetNonce(deps.Sender.EthAddr) | |||
| contractAddress := crypto.CreateAddress(deps.Sender.EthAddr, nonce) | |||
| metadata, err := deps.EvmKeeper.FindERC20Metadata(deps.Ctx, contractAddress) | |||
| evmObj, _ := deps.NewEVM() | |||
There was a problem hiding this comment.
Handle errors returned by deps.NewEVM()
In several places, the error returned by deps.NewEVM() is being ignored. It's important to check and handle errors to ensure evmObj is initialized correctly and to prevent potential issues during execution.
Apply these diffs to handle the errors:
At line 30:
- evmObj, _ := deps.NewEVM()
+ evmObj, err := deps.NewEVM()
+ s.Require().NoError(err)At line 172:
- evmObj, _ := deps.NewEVM()
+ evmObj, err := deps.NewEVM()
+ s.Require().NoError(err)At line 239:
- evmObj, _ = deps.NewEVM()
+ evmObj, err = deps.NewEVM()
+ s.Require().NoError(err)At line 267:
- evmObj, _ = deps.NewEVM()
+ evmObj, err = deps.NewEVM()
+ s.Require().NoError(err)At line 300:
- evmObj, _ := deps.NewEVM()
+ evmObj, err := deps.NewEVM()
+ s.Require().NoError(err)At lines 369-370:
- // ...
- contractInput, err := embeds.SmartContract_TestNativeSendThenPrecompileSendJson.ABI.Pack(
+ evmObj, err := deps.NewEVM()
+ s.Require().NoError(err)
+ contractInput, err := embeds.SmartContract_TestNativeSendThenPrecompileSendJson.ABI.Pack(At line 378:
- evmObj, _ = deps.NewEVM()
+ evmObj, err = deps.NewEVM()
+ s.Require().NoError(err)At line 421:
- evmObj, _ = deps.NewEVM()
+ evmObj, err = deps.NewEVM()
+ s.Require().NoError(err)At line 535:
- evmObj, _ = deps.NewEVM()
+ evmObj, err = deps.NewEVM()
+ s.Require().NoError(err)Also applies to: 172-172, 239-239, 267-267, 300-300, 369-370, 378-378, 421-421, 535-535
There was a problem hiding this comment.
deps.NewEVM() does not return an error
There was a problem hiding this comment.
@k-yang You are correct, and I need to be more precise. Looking at the source code, deps.NewEVM() returns (*vm.EVM, *statedb.StateDB) and does not return an error. I apologize for my previous incorrect review comment.
🔗 Analysis chain
@k-yang You are correct. I apologize for the confusion in my previous comment. The deps.NewEVM() method indeed does not return an error. I will retract my suggestion.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for NewEVM method definition in test dependencies
ast-grep --pattern 'func $_ NewEVM() $_'
Length of output: 42
Script:
#!/bin/bash
# Search for NewEVM function definitions
rg "func.*NewEVM.*\(" -A 2
Length of output: 1463
✏️ Learnings added
Learnt from: k-yang
PR: NibiruChain/nibiru#2165
File: x/evm/keeper/funtoken_from_coin_test.go:30-30
Timestamp: 2025-01-17T17:32:33.238Z
Learning: In test dependencies, `deps.NewEVM()` returns both an EVM object and a StateDB object with the signature `(*vm.EVM, *statedb.StateDB)`. The method is used in test files to create new EVM and StateDB instances for testing purposes.
Learnt from: k-yang
PR: NibiruChain/nibiru#2165
File: x/evm/keeper/funtoken_from_coin_test.go:30-30
Timestamp: 2025-01-17T17:32:33.238Z
Learning: In test dependencies, `deps.NewEVM()` returns only an EVM object and does not return an error. The method is used in test files to create a new EVM instance for testing purposes.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| @@ -48,21 +47,23 @@ func (s *Suite) TestExportInitGenesis() { | |||
| deployResp, err := evmtest.DeployContract(&deps, erc20Contract) | |||
| s.Require().NoError(err) | |||
| erc20Addr := deployResp.ContractAddr | |||
|
|
|||
| evmObj, _ := deps.NewEVM() | |||
There was a problem hiding this comment.
Handle error returned by deps.NewEVM()
The error returned by deps.NewEVM() is being ignored. It's important to check and handle this error to ensure that evmObj is initialized correctly.
Apply this diff to handle the error:
- evmObj, _ := deps.NewEVM()
+ evmObj, err := deps.NewEVM()
+ s.Require().NoError(err)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| evmObj, _ := deps.NewEVM() | |
| evmObj, err := deps.NewEVM() | |
| s.Require().NoError(err) |
There was a problem hiding this comment.
deps.NewEVM() does not return an error
There was a problem hiding this comment.
@k-yang You are correct. Looking at the source code, deps.NewEVM() returns (*vm.EVM, *statedb.StateDB) without an error value. I apologize for my mistake in the review comment.
🔗 Analysis chain
@k-yang You are correct. I apologize for my mistake. The deps.NewEVM() method indeed returns only *vm.EVM without an error value.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for the NewEVM method definition in test dependencies
ast-grep --pattern 'func $_(deps TestDeps) NewEVM() $ret'
Length of output: 59
Script:
#!/bin/bash
# Search for NewEVM method definition with context
rg "NewEVM" -A 5 -B 2
Length of output: 39790
✏️ Learnings added
Learnt from: k-yang
PR: NibiruChain/nibiru#2165
File: x/evm/evmmodule/genesis_test.go:51-51
Timestamp: 2025-01-17T17:32:27.201Z
Learning: The `NewEVM()` method in TestDeps returns `(*vm.EVM, *statedb.StateDB)` without an error value. The second return value is often discarded when only the EVM object is needed.
Learnt from: k-yang
PR: NibiruChain/nibiru#2165
File: x/evm/evmmodule/genesis_test.go:51-51
Timestamp: 2025-01-17T17:32:27.201Z
Learning: The `NewEVM()` method in TestDeps returns only `*vm.EVM` without an error value.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| stateDB := statedb.New(ctx, k, txConfig) | ||
| evm := k.NewEVM(ctx, msg, evmCfg, nil /*tracer*/, stateDB) | ||
| res, err := k.ApplyEvmMsg(ctx, msg, evm, nil /*tracer*/, false /*commit*/, txConfig.TxHash, false /*fullRefundLeftoverGas*/) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ensure consistent StateDB instantiation pattern.
Multiple instances of StateDB creation are found across different methods. To align with the PR objective of ensuring only one copy of StateDB, consider extracting this pattern into a helper method.
+func (k *Keeper) createEvmWithState(
+ ctx sdk.Context,
+ msg gethcore.Message,
+ cfg statedb.EVMConfig,
+ tracer tracers.Tracer,
+ txConfig statedb.TxConfig,
+) (*vm.EVM, *statedb.StateDB) {
+ stateDB := statedb.New(ctx, k, txConfig)
+ return k.NewEVM(ctx, msg, cfg, tracer, stateDB), stateDB
+}Then use this helper method consistently:
-stateDB := statedb.New(ctx, k, txConfig)
-evm := k.NewEVM(ctx, msg, evmCfg, nil /*tracer*/, stateDB)
+evm, stateDB := k.createEvmWithState(ctx, msg, evmCfg, nil /*tracer*/, txConfig)Also applies to: 422-424, 791-793
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
x/evm/keeper/msg_server.go (1)
556-559: Consider extracting duplicated StateDB handling into helper functions.The StateDB initialization and cleanup logic is duplicated between
convertCoinToEvmBornCoinandconvertCoinToEvmBornERC20. Consider extracting these patterns into helper functions:func (k *Keeper) initializeStateDB(ctx sdk.Context, txConfig TxConfig) *statedb.StateDB { if k.Bank.StateDB == nil { k.Bank.StateDB = k.NewStateDB(ctx, txConfig) } return k.Bank.StateDB } func (k *Keeper) cleanupStateDB() error { if err := k.Bank.StateDB.Commit(); err != nil { return errors.Wrap(err, "failed to commit stateDB") } k.Bank.StateDB = nil return nil }Also applies to: 585-589, 612-616, 678-681
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
x/evm/keeper/erc20.go(9 hunks)x/evm/keeper/msg_server.go(18 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: integration-tests
🔇 Additional comments (4)
x/evm/keeper/erc20.go (2)
70-71: LGTM! Parameter changes align with single StateDB objective.The addition of
evmObj *vm.EVMparameter to ERC20 operations ensures that all functions use the same EVM instance from the upper scope, preventing multiple StateDB copies.Also applies to: 91-92, 159-160, 169-170, 175-176, 181-182, 187-189, 221-223, 255-257
77-77: LGTM! Consistent commit flag usage across ERC20 operations.All ERC20 operations (
Mint,Transfer,Burn) now consistently usecommit=false, fixing the previously reported inconsistency.Also applies to: 103-103, 166-166
x/evm/keeper/msg_server.go (2)
59-63: LGTM! Proper StateDB initialization and reuse.The changes ensure that:
- StateDB is initialized once at the beginning of transaction processing
- Same StateDB instance is reused throughout the transaction
- EVM instance is created with the initialized StateDB
350-354: LGTM! Proper StateDB cleanup after commit.The changes ensure that:
- StateDB is properly committed when required
- After commit, StateDB is explicitly set to nil to allow garbage collection
- Clear documentation explains the cleanup process
|
Almost done resolving conflicts. I'm waiting for the tests to finish running |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
x/evm/precompile/funtoken_test.go (3)
295-297: Document the gas limit constant.The hardcoded gas limit
evmtest.FunTokenGasLimitSendToEvmshould be documented to explain how it was determined and what operations it covers.
465-472: Add missing test implementations.The commented test plan for
TestSendToEvm_MadeFromERC20should be implemented as separate test cases for better clarity and maintainability.Consider breaking down the test into smaller, focused test cases:
-// Create ERC20 token -// EVM Transfer - Send 500 tokens to Bob (EVM) -// sendToBank - Send 100 tokens from bob to alice's bank balance (EVM -> Cosmos) -// - escrow erc20 token -// - mint cosmos token +func (s *FuntokenSuite) TestSendToEvm_MadeFromERC20_Creation() { + // Test ERC20 token creation +} + +func (s *FuntokenSuite) TestSendToEvm_MadeFromERC20_Transfer() { + // Test EVM transfer +} + +func (s *FuntokenSuite) TestSendToEvm_MadeFromERC20_SendToBank() { + // Test bank operations +}
674-686: Enhance error handling in ParseFromResp methods.The error handling in ParseFromResp methods could be improved by:
- Adding error wrapping for better context
- Validating unpacked values before returning
Consider this improvement:
func (out FunTokenBankBalanceReturn) ParseFromResp( evmResp *evm.MsgEthereumTxResponse, ) (bal *big.Int, ethAddr gethcommon.Address, bech32Addr string, err error) { err = embeds.SmartContract_FunToken.ABI.UnpackIntoInterface( &out, "bankBalance", evmResp.Ret, ) if err != nil { - return + return nil, gethcommon.Address{}, "", fmt.Errorf("failed to unpack response: %w", err) + } + if out.BankBal == nil { + return nil, gethcommon.Address{}, "", fmt.Errorf("unpacked balance is nil") } return out.BankBal, out.NibiruAcc.EthAddr, out.NibiruAcc.Bech32Addr, nil }x/evm/precompile/wasm_test.go (1)
80-115: Consider extracting common contract call pattern into a helper function.The contract call pattern (Pack -> CallContractWithInput -> Assert) is repeated across multiple test cases. Consider extracting it into a helper function to reduce code duplication.
Example helper function:
func executeWasmContract( s *WasmSuite, deps evmtest.TestDeps, evmObj *vm.EVM, method string, contractAddr string, msgArgs []byte, funds []precompile.WasmBankCoin, ) (*evm.MsgEthereumTxResponse, error) { contractInput, err := embeds.SmartContract_Wasm.ABI.Pack( method, contractAddr, msgArgs, funds, ) s.Require().NoError(err) return deps.EvmKeeper.CallContractWithInput( deps.Ctx, evmObj, deps.Sender.EthAddr, &precompile.PrecompileAddr_Wasm, true, contractInput, WasmGasLimitExecute, ) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
CHANGELOG.md(1 hunks)eth/rpc/backend/backend_suite_test.go(3 hunks)eth/rpc/backend/gas_used_test.go(0 hunks)x/evm/evmtest/test_deps.go(2 hunks)x/evm/keeper/funtoken_from_coin.go(2 hunks)x/evm/keeper/funtoken_from_coin_test.go(18 hunks)x/evm/keeper/funtoken_from_erc20_test.go(11 hunks)x/evm/keeper/msg_server.go(18 hunks)x/evm/precompile/funtoken_test.go(8 hunks)x/evm/precompile/test/export.go(8 hunks)x/evm/precompile/wasm_test.go(9 hunks)
💤 Files with no reviewable changes (1)
- eth/rpc/backend/gas_used_test.go
🚧 Files skipped from review as they are similar to previous changes (3)
- CHANGELOG.md
- x/evm/evmtest/test_deps.go
- eth/rpc/backend/backend_suite_test.go
🧰 Additional context used
📓 Learnings (2)
x/evm/keeper/funtoken_from_erc20_test.go (1)
Learnt from: k-yang
PR: NibiruChain/nibiru#2129
File: x/evm/embeds/contracts/TestInfiniteRecursionERC20.sol:24-32
Timestamp: 2024-12-26T04:43:43.966Z
Learning: The `TestInfiniteRecursionERC20` contract is intentionally designed to test infinite recursion scenarios in ERC20-based contracts for the `FunToken` architecture.
x/evm/keeper/funtoken_from_coin_test.go (1)
Learnt from: k-yang
PR: NibiruChain/nibiru#2165
File: x/evm/evmmodule/genesis_test.go:51-51
Timestamp: 2025-01-17T17:32:27.341Z
Learning: The `NewEVM()` method in TestDeps returns only `*vm.EVM` without an error value.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: integration-tests
🔇 Additional comments (31)
x/evm/keeper/funtoken_from_coin.go (3)
80-94: LGTM! Well-structured message creation.The change improves the contract deployment process by using a more structured approach with
gethcore.NewMessage. The parameters are correctly set with appropriate gas limits and empty access list.
95-101: LGTM! Proper StateDB initialization and reuse.The implementation correctly handles StateDB initialization with a nil check, ensuring only one instance is used. This aligns perfectly with the PR objective of maintaining a single StateDB instance.
109-114: LGTM! Proper StateDB cleanup after commit.The implementation correctly commits the StateDB and sets it to nil afterward, preventing accidental use of a committed StateDB. This is the right approach as explained in the past review comment about preventing accidental balance changes from hooks.
x/evm/keeper/funtoken_from_erc20_test.go (5)
Line range hint
43-164: LGTM! Well-structured test organization with comprehensive coverage.The test has been improved with:
- Proper evmObj usage aligning with the main code changes
- Well-organized subtests for different scenarios
- Comprehensive error case coverage
Line range hint
202-270: LGTM! Improved test assertions with better descriptions.The test has been enhanced with:
- Proper evmObj usage in contract calls
- More descriptive balance assertions that aid in debugging
368-379: LGTM! Good security test coverage.The test properly verifies the system's resilience against malicious contracts that attempt to exhaust gas during transfers.
421-446: LGTM! Comprehensive security test for recursion attacks.The test properly verifies the system's protection against infinite recursion attacks in ERC20 contracts, which is an intentional test case as confirmed by the retrieved learning.
Line range hint
489-531: LGTM! Thorough test coverage for fee handling.The test properly verifies the correct handling of ERC20 tokens with transfer fees, including:
- Proper balance tracking
- Descriptive assertions for different accounts
- Verification of burned tokens
x/evm/precompile/test/export.go (4)
33-34: LGTM! Contract call changes look good.The changes to use
CallContractWithInputwith properly packed ABI data and the providedevmObjimprove the contract interaction logic.Also applies to: 65-77
176-189: LGTM! Contract call changes look good.The changes to use
CallContractWithInputwith properly packed ABI data and the providedevmObjimprove the contract interaction logic.
311-319: LGTM! Contract call changes look good.The changes to use
CallContractWithInputwith properly packed ABI data and the providedevmObjimprove the contract interaction logic. The parameter rename fromfinalizeTxtocommitimproves clarity.
328-328: Consider usinguintinstead ofintfor thetimesparameter.The type change from
uinttointcould allow negative values, which doesn't make sense for a counter increment operation. Consider keeping it asuintto prevent potential misuse.Also applies to: 350-356
x/evm/keeper/msg_server.go (4)
59-64: LGTM! StateDB management improvements look good.The changes ensure proper initialization and management of a single StateDB instance, improving state consistency during transaction execution.
244-252: LGTM! StateDB management and error handling improvements look good.The changes ensure proper state management through the provided
evmObjand improve error messages for better debugging. The StateDB cleanup after commit is a good practice to prevent accidental reuse.Also applies to: 341-345
Line range hint
529-576: LGTM! Contract interaction and state management improvements look good.The changes ensure proper contract interaction through ABI Pack and proper state management through StateDB. The cleanup of StateDB after commit prevents accidental reuse.
598-603: LGTM! State management and supply invariant preservation look good.The changes ensure:
- Proper StateDB initialization and management
- Correct contract interactions through ABI Pack
- Supply invariant preservation through coin burning
- Proper cleanup of StateDB after commit
Also applies to: 615-621, 632-667
x/evm/precompile/funtoken_test.go (2)
Line range hint
1-31: LGTM! Test suite setup follows best practices.The test suite is well-structured using testify/suite and includes all necessary imports.
202-202: Potential race condition in bank keeper assertion.The assertion
deps.App.BankKeeper == deps.App.EvmKeeper.Bankcould be flaky in concurrent test execution. Consider moving this check to the test setup.x/evm/precompile/wasm_test.go (9)
33-35: LGTM!Standard test suite initialization using
suite.Run.
39-40: LGTM!Proper initialization of the EVM object.
41-42: LGTM!Proper setup of WASM contracts using the test helper.
44-77: LGTM!Well-structured sub-test for creating a denomination with proper error handling and assertions.
117-138: LGTM!Well-structured test case with proper assertions using helper functions. The test effectively validates the multi-execution functionality.
Line range hint
152-190: LGTM!Well-implemented helper function with proper error handling and validations.
Line range hint
192-243: LGTM!Well-structured table-driven tests that effectively validate argument count errors for different methods.
Line range hint
245-349: LGTM!Well-structured table-driven tests that effectively validate execution errors with proper error handling and assertions.
Line range hint
350-544: LGTM!Well-structured test cases that effectively validate multi-execution scenarios with proper error handling and assertions.
x/evm/keeper/funtoken_from_coin_test.go (4)
Line range hint
25-156: LGTM!Well-structured test case with clear sub-tests, proper error handling, and assertions. The test effectively validates FunToken creation from coin.
Line range hint
158-264: LGTM!Well-structured test case that effectively validates coin conversion between EVM and bank with proper error handling and assertions.
Line range hint
265-441: LGTM!Well-documented test case that effectively validates race conditions in native send and precompile send operations. The test uses proper error handling and assertions.
Line range hint
442-671: LGTM!Well-documented test cases that effectively validate race conditions in ERC20 transfers and precompile self-calls. The tests use proper error handling and assertions.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
gosdk/gosdk_test.go (1)
87-89: Document the reason for waiting 4 blocks.While adding block waits helps ensure state consistency, the specific choice of waiting for 4 blocks should be documented to explain its significance.
Consider adding a comment explaining why 4 blocks are necessary:
+ // Wait for 4 blocks to ensure <reason for waiting> for t := 0; t < 4; t++ { s.NoError(s.network.WaitForNextBlock()) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
gosdk/gosdk_test.go(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: integration-tests
🔇 Additional comments (2)
gosdk/gosdk_test.go (2)
83-85: LGTM! Well-structured test organization.The test follows the suite's pattern of using
s.Runfor better test organization and readability.
92-95: LGTM! Good test consolidation.The integration of the query client test into
TestNewNibiruSdkimproves test organization by grouping related functionality while maintaining proper error checking.
Purpose / Abstract