fix(evm): StateDB multistore cache for precompile reversion and a safer Nibiru bank keeper that respects the EVM #2094
Conversation
1. Created NibiruBankKeeper with safety around NIBI transfers inside of EthereumTx. 2. The "PrecompileCalled" JournalChange now has a propery implementation and a strong test case to show that reverting the precompile calls works as intended. 3. Remove unneeded functions created for testing with low-level struct fields.
Unique-Divine
force-pushed
the
ud/db-cache
branch
from
f288698 to
7f904a0
Compare
WalkthroughThe changes in this pull request encompass a comprehensive update to the Nibiru EVM codebase, including enhancements to the changelog format, modifications to keeper structures, and updates to dependency management. Notable additions include a new Changes
Poem
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 (
|
| @@ -605,7 +619,7 @@ func (app *NibiruApp) initAppModules( | |||
| ), | |||
| auth.NewAppModule(appCodec, app.AccountKeeper, authsims.RandomGenesisAccounts, app.GetSubspace(authtypes.ModuleName)), | |||
| vesting.NewAppModule(app.AccountKeeper, app.BankKeeper), | |||
| bank.NewAppModule(appCodec, app.BankKeeper, app.AccountKeeper, app.GetSubspace(banktypes.ModuleName)), | |||
| bank.NewAppModule(appCodec, app.bankBaseKeeper, app.AccountKeeper, app.GetSubspace(banktypes.ModuleName)), | |||
There was a problem hiding this comment.
This line's required to appease the module manager when it does a type assertion on an object of type bankkeeper.Keeper (an interface), asserting that it's actually a bankkeeper.BaseKeeper struct. I didn't see a simple way to force x/bank to use the NibiruBankKeeper struct without editing the Bank module directly, so I opted for what's written here, which is quite simple.
Edit: Moved the NibiruBankKeeper to #2095. The rationale behind these lines are in that PR
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2094 +/- ##
==========================================
+ Coverage 64.36% 64.47% +0.11%
==========================================
Files 270 271 +1
Lines 21192 21242 +50
==========================================
+ Hits 13640 13696 +56
+ Misses 6603 6591 -12
- Partials 949 955 +6
|
Unique-Divine
force-pushed
the
ud/db-cache
branch
from
efbd52e to
717ce1c
Compare
Unique-Divine
force-pushed
the
ud/db-cache
branch
from
4184906 to
7c34423
Compare
There was a problem hiding this comment.
Moved these to the bottom since the workhorse functions in this file are below this
x/evm/keeper/erc20.go
Outdated
There was a problem hiding this comment.
This was here because of the repeated calls during gas estimation, not for EVM reversions
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Outside diff range and nitpick comments (19)
x/evm/keeper/precompiles.go (1)
25-27: Consider adding method documentation.Adding a doc comment would help clarify the method's purpose and its role in the precompile execution flow.
+// IsPrecompile checks if the given address corresponds to a registered precompiled +// contract in the EVM. This validation is essential before executing precompiles +// to ensure proper state management. func (k *Keeper) IsPrecompile(addr gethcommon.Address) bool {x/evm/statedb/debug.go (3)
9-18: LGTM with a minor documentation suggestionThe function is well-implemented for debugging purposes. Consider adding an example usage in the documentation to make it more developer-friendly.
// DebugDirtiesCount is a test helper to inspect how many entries in the journal // are still dirty (uncommitted). After calling [StateDB.Commit], this function -// should return zero. +// should return zero. +// +// Example: +// +// state.DebugDirtiesCount() // Returns number of uncommitted changes +// state.Commit() +// state.DebugDirtiesCount() // Should return 0
20-23: Consider returning a copy of the dirties mapThe function currently returns a direct reference to the internal dirties map. While acceptable for debugging, consider returning a copy to prevent accidental modifications.
func (s *StateDB) DebugDirties() map[common.Address]int { - return s.Journal.dirties + copyMap := make(map[common.Address]int) + for k, v := range s.Journal.dirties { + copyMap[k] = v + } + return copyMap
25-29: LGTM with documentation enhancementThe function provides essential debug access to journal entries. Consider expanding the documentation to explain the significance of JournalChange objects in the context of state tracking.
// DebugEntries is a test helper that returns the sequence of [JournalChange] -// objects added during execution. +// objects added during execution. JournalChange objects represent modifications +// to the state that can be reverted, which is crucial for maintaining state +// consistency during precompile execution and potential reversions.x/evm/keeper/keeper.go (1)
67-67: LGTM: Constructor parameter type update is consistent.The parameter type change aligns with the struct field update. This change promotes direct usage of the Cosmos SDK's bank keeper implementation.
Consider documenting the rationale for removing the custom interface in the module's documentation, as this represents a significant architectural decision that affects how the EVM module interacts with the bank module.
x/evm/keeper/statedb.go (1)
75-78: Consider expanding the documentation for this critical state management method.While the added clarification about
StateDB.Commit()is helpful, consider enhancing the documentation further to:
- Explain the atomicity guarantees
- Document the relationship with precompile state changes
- Clarify the implications of minting/burning operations
Example addition:
// SetAccBalance update account's balance, compare with current balance first, // then decide to mint or burn. // Implements the `statedb.Keeper` interface. // Only called by `StateDB.Commit()`. +// The method ensures atomic updates of account balances during state transitions. +// It's critical for maintaining consistency between EVM and Cosmos SDK state, +// especially during precompile operations that may need to be reverted.x/evm/precompile/wasm_test.go (2)
Line range hint
102-113: Consider adding assertions for intermediate state.The test verifies the final state after each increment (2 and 69), but given the PR's focus on state management, it would be valuable to add assertions that verify:
- The state remains consistent during multi-step operations
- No unintended state changes occur between operations
Add intermediate state checks:
test.IncrementWasmCounterWithExecuteMulti( &s.Suite, &deps, wasmContract, 2, true) +// Verify no unintended state changes +s.assertWasmCounterStateRaw(deps, wasmContract, 2) test.AssertWasmCounterState(&s.Suite, deps, wasmContract, 2) -s.assertWasmCounterStateRaw(deps, wasmContract, 2)
Line range hint
1-276: Enhance test coverage for state management scenarios.Given the PR's focus on state management and precompile reversion, consider adding these test cases:
- Test reverting a precompile call within a multi-step transaction
- Test state consistency when mixing EVM and non-EVM state changes
- Test the deep copy behavior of the new JournalChange struct
Example test structure:
func (s *WasmSuite) TestPrecompileReversion() { // Setup initial state // Perform EVM state change // Execute precompile that modifies non-EVM state // Trigger reversion // Verify both EVM and non-EVM states are properly reverted }x/evm/statedb/state_object.go (1)
Line range hint
1-300: Excellent implementation of EVM state management.The codebase demonstrates high quality with:
- Clear separation between native (unibi) and EVM (wei) state
- Proper state journaling for reversions
- Comprehensive documentation
- Thread-safe context handling
The context changes contribute to the PR's goal of improving state management during precompile execution and reversion.
Consider adding debug logging for state transitions in development environments to aid in troubleshooting complex state management scenarios.
x/evm/keeper/erc20.go (1)
222-222: Document the purpose of NoOpTracer.Consider adding a comment explaining why
NewNoOpTracer()was chosen here, particularly in the context of precompile reversion handling and state synchronization.Add documentation above the
ApplyEvmMsgcall:+ // NoOpTracer is used here to handle precompile reversions while maintaining + // state synchronization between EVM and non-EVM states evmResp, evmObj, err = k.ApplyEvmMsg( ctx, evmMsg, evm.NewNoOpTracer(), commit, evmCfg, txConfig, )x/evm/evmtest/tx.go (2)
261-264: LGTM! Consider adding constants for transaction types.The
GethTxTypetype alias is well-defined. Consider adding constants for the supported transaction types to improve code readability and maintainability.const ( LegacyTxType GethTxType = 0x00 AccessListTxType GethTxType = 0x01 DynamicFeeTxType GethTxType = 0x02 )
265-294: LGTM! Consider documenting default values.The template functions are well-structured and provide good defaults. Consider adding documentation to explain the default values used, especially for gas-related fields.
x/evm/statedb/journal_test.go (1)
153-153: Minor Typographical Correction in Log MessageConsider correcting the log message to:
- s.T().Log("Expect exactly 0 dirty journal entry for the precompile snapshot") + s.T().Log("Expect exactly 0 dirty journal entries for the precompile snapshot")x/evm/statedb/journal.go (2)
336-336: Typo in comment: 'shoud' should be 'should'In line 336~, the word "shoud" is misspelled. It should be corrected to "should".
355-356: Address the TODO comment regarding event emissionThere is a TODO comment indicating the need to check the correctness of the emitted events. Please ensure that the events are emitted correctly and consider resolving this TODO.
Would you like assistance in verifying and updating the event emission code?
x/evm/keeper/msg_server.go (4)
Line range hint
218-321: Consider refactoringApplyEvmMsgfor improved maintainabilityThe
ApplyEvmMsgfunction spans over 100 lines and handles multiple responsibilities, which can make it challenging to read and maintain. Refactoring it into smaller, well-defined helper functions can enhance readability and facilitate easier testing and debugging.
Line range hint
218-321: Improve error handling consistencyThroughout the
ApplyEvmMsgfunction, error handling can be made more consistent. For instance, wrapping errors with context-specific messages can help identify the source of failures more effectively. Ensure that all errors are consistently wrapped and provide clear, actionable information.
Line range hint
218-321: Add unit tests forApplyEvmMsgand related methodsGiven the complexity of
ApplyEvmMsg, ensure that there are comprehensive unit tests covering various execution paths, including successful executions, failures, and edge cases. This will increase confidence in the changes and help prevent regressions.Would you like assistance in creating unit tests for this function?
Line range hint
24-75: Enhance error messages inEthereumTxIn the
EthereumTxmethod, errors are wrapped with messages like"failed to apply ethereum core message". Consider providing more context-specific information in error messages to aid in debugging and user support.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (24)
- CHANGELOG.md (1 hunks)
- app/keepers.go (3 hunks)
- go.mod (1 hunks)
- x/evm/deps.go (0 hunks)
- x/evm/evmtest/test_deps.go (1 hunks)
- x/evm/evmtest/tx.go (2 hunks)
- x/evm/keeper/erc20.go (1 hunks)
- x/evm/keeper/keeper.go (3 hunks)
- x/evm/keeper/msg_server.go (1 hunks)
- x/evm/keeper/precompiles.go (1 hunks)
- x/evm/keeper/statedb.go (2 hunks)
- x/evm/precompile/funtoken.go (1 hunks)
- x/evm/precompile/precompile.go (2 hunks)
- x/evm/precompile/test/export.go (3 hunks)
- x/evm/precompile/wasm.go (1 hunks)
- x/evm/precompile/wasm_test.go (1 hunks)
- x/evm/statedb/access_list.go (1 hunks)
- x/evm/statedb/config.go (1 hunks)
- x/evm/statedb/debug.go (1 hunks)
- x/evm/statedb/interfaces.go (2 hunks)
- x/evm/statedb/journal.go (3 hunks)
- x/evm/statedb/journal_test.go (7 hunks)
- x/evm/statedb/state_object.go (3 hunks)
- x/evm/statedb/statedb.go (10 hunks)
💤 Files with no reviewable changes (1)
- x/evm/deps.go
✅ Files skipped from review due to trivial changes (2)
- x/evm/statedb/access_list.go
- x/evm/statedb/config.go
🔇 Additional comments (45)
x/evm/keeper/precompiles.go (1)
25-27: LGTM! Implementation aligns with security requirements.The
IsPrecompilemethod provides a thread-safe way to validate addresses before precompile execution, which is crucial for the new state management system.x/evm/statedb/interfaces.go (2)
3-4: LGTM: Copyright notice is appropriate and current.
31-32: LGTM: IsPrecompile method addition aligns with security objectives.The addition of
IsPrecompilemethod to the Keeper interface is a crucial part of the security enhancements for precompile state management. This method will help ensure proper identification of precompiled contracts before state modifications.Let's verify the consistent implementation of this method across the codebase:
✅ Verification successful
Implementation of IsPrecompile is consistent and well-integrated
The verification shows that:
IsPrecompileis properly implemented inx/evm/keeper/precompiles.gousing a sorted map of precompiled contracts- The precompiles map is initialized during Keeper setup and used consistently
- The implementation is integrated with the EVM execution context via
WithPrecompiles- The method is used as expected for precompile address validation
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify IsPrecompile implementation and usage # Expected: Find implementation and all usage points of IsPrecompile # Find the implementation echo "=== IsPrecompile Implementation ===" ast-grep --pattern 'func $_($_ Keeper) IsPrecompile(addr common.Address) bool { $$$ }' # Find all usage points echo -e "\n=== IsPrecompile Usage ===" rg -A 2 'IsPrecompile\('Length of output: 548
Script:
#!/bin/bash # Let's verify the precompiles.Has implementation and usage to ensure completeness # Check precompiles.Has implementation echo "=== Precompiles Has Implementation ===" ast-grep --pattern 'func ($_ $_) Has(addr $_) bool { $$$ }' # Check where precompiles is initialized/set in Keeper echo -e "\n=== Precompiles Initialization ===" rg -A 3 'precompiles\s*=' x/evm/keeper/ # Check if there are any other methods using precompiles echo -e "\n=== Other Precompiles Usage ===" rg 'precompiles\.' x/evm/keeper/Length of output: 869
x/evm/statedb/debug.go (2)
1-8: LGTM: Clean package structure and importsThe package declaration, copyright notice, and imports are well-organized and minimal.
31-39: Note about stateObject pointer safetyWhile the function creates a copy of the map, it still shares the same stateObject pointers. This is likely intentional for debugging purposes, but be aware that modifications to the returned stateObjects will affect the original state.
✅ Verification successful
Let me gather more information about the stateObject implementation to verify the pointer safety concern.
Pointer safety warning is valid and important
The verification confirms that
stateObjecthas multiple mutating methods (SetBalance,SetCode,SetState, etc.) that modify the underlying object's state. SinceDebugStateObjectsshares the same pointers, any modifications through the debug map can indeed affect the original state. This matches the review comment's observation.Key mutating operations:
- Balance modifications via
SetBalance,AddBalance,SubBalance- Code changes through
SetCode- State changes via
SetState- Nonce updates using
SetNonce🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify if stateObject is used elsewhere in the codebase rg -A 2 "type stateObject struct"Length of output: 165
Script:
#!/bin/bash # Check the stateObject implementation for mutating methods rg -A 5 "func \(.*stateObject\)" x/evm/statedb/state_object.go # Check if there are any other debug functions that handle stateObject rg -A 5 "func.*Debug.*stateObject" x/evm/statedb/Length of output: 3269
x/evm/keeper/keeper.go (4)
18-18: LGTM: Import of Cosmos SDK bank keeper.The direct import of the Cosmos SDK bank keeper implementation aligns with the shift away from a custom interface, promoting better standardization and maintainability.
74-74: Formatting change.
44-44: Verify integration with JournalChange functionality.The bank keeper changes are part of implementing a safer state management system. We should verify the integration with the new JournalChange functionality mentioned in the PR objectives.
Also applies to: 67-67
✅ Verification successful
Integration with JournalChange functionality is properly implemented
The verification shows that the bank keeper integration is correctly implemented with the JournalChange functionality:
- The JournalChange system is implemented in
x/evm/statedb/journal.goand properly handles precompile state changes- Bank keeper operations in precompiles are protected through:
SavePrecompileCalledJournalChangein statedb for state tracking- Safe state management functions like
SafeMintCoinsin precompile operations- Proper module-based coin transfers and burns with error handling
The bank keeper changes align with the PR's security enhancements for precompile reversion.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for JournalChange usage in relation to bank keeper echo "Searching for JournalChange struct usage..." ast-grep --pattern 'struct JournalChange { $$$ }' echo "Searching for bank keeper usage in precompile context..." rg -A 5 "OnRunStart.*bankKeeper"Length of output: 403
Script:
#!/bin/bash # Let's try a broader search for JournalChange and bank keeper interactions echo "Searching for JournalChange mentions..." rg "JournalChange" echo -e "\nSearching for bank keeper usage in state management..." rg -A 5 "bankKeeper.*" "x/evm" echo -e "\nSearching for precompile state management..." rg -A 5 "precompile.*state" "x/evm"Length of output: 11885
44-44: Verify the implications of using SDK's bank keeper directly.The change from
evm.BankKeepertobankkeeper.Keeperremoves an abstraction layer. While this aligns with the PR's goal of implementing a safer bank keeper, we should verify:
- That all required bank keeper methods are available
- That no security constraints are bypassed
✅ Verification successful
Let me try to find the actual usage patterns and interface definitions with more specific search patterns. The previous command might have failed due to syntax issues, so I've improved the search patterns and added type filtering for Go files. This should help us understand:
- How the bank keeper is actually used in the codebase
- What was the previous interface definition
- Where and how the bank keeper is imported and initialized
Based on the search results, I can now generate a final response as we have sufficient information about the bank keeper usage and the change's implications.
The change to use
bankkeeper.Keeperdirectly is safe and appropriateThe search results show that the bank keeper is used only for standard bank module operations:
- Balance queries (
GetBalance)- Token operations (
MintCoins,BurnCoins)- Transfer operations (
SendCoinsFromModuleToAccount,SendCoinsFromAccountToModule)- Denomination metadata operations (
GetDenomMetaData,SetDenomMetaData)All these operations are part of the standard Cosmos SDK bank keeper interface, and no custom constraints were bypassed by removing the custom interface. The usage patterns align perfectly with the SDK's bank keeper capabilities.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for all bank keeper method usages to ensure compatibility echo "Searching for bank keeper method usages..." rg -A 1 "bankKeeper\." x/evm/ # Search for the old interface definition to understand removed constraints echo "Searching for old BankKeeper interface..." rg "type BankKeeper interface" x/evm/Length of output: 403
Script:
#!/bin/bash # Let's try a more comprehensive search for bank keeper usage echo "Searching for bank keeper method calls..." rg -A 2 "bankKeeper\." --type go x/evm/ echo -e "\nSearching for BankKeeper interface definition..." rg -A 5 "type BankKeeper interface" --type go x/evm/ echo -e "\nSearching for bankkeeper imports..." rg "bankkeeper" --type go x/evm/ echo -e "\nLooking for keeper initialization..." rg -A 3 "NewKeeper.*bankkeeper" --type go x/evm/Length of output: 4238
x/evm/keeper/statedb.go (2)
20-25: LGTM! Well-structured addition for StateDB instantiation.This method provides a clean interface for StateDB creation and aligns well with the PR's goal of improving state management for precompile reversion. The implementation follows good dependency injection patterns.
20-25: Verify error handling during precompile state transitions.Given this code's critical role in state management and the PR's focus on precompile reversion, let's verify the error handling paths:
Also applies to: 75-78
x/evm/precompile/precompile.go (3)
194-202: LGTM: Robust state caching implementation for precompile execution.The implementation correctly:
- Creates a cached context for isolated state operations
- Saves journal entries for state tracking
- Commits changes with proper error handling
207-207: LGTM: Proper usage of cached context.The cached context is correctly returned in the result struct, ensuring isolated state operations.
150-151: LGTM: PrecompileJournalEntry addition aligns with state tracking requirements.The new field enables proper state tracking for precompile execution, which is crucial for the state synchronization mechanism described in the PR objectives.
Let's verify the PrecompileCalled type implementation:
✅ Verification successful
PrecompileJournalEntry field is correctly implemented for state reversion
The verification confirms that:
PrecompileCalledis properly implemented instatedb/journal.gowith necessary fields for multistore and events- It implements the
JournalChangeinterface with aRevertmethod that correctly handles state rollback- The implementation is well-documented and integrated with the state management system
- The field is used appropriately in the precompile execution flow for state synchronization
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify the PrecompileCalled type definition and its usage ast-grep --pattern 'type PrecompileCalled struct { $$$ }'Length of output: 267
Script:
#!/bin/bash # Let's verify the usage and revert handling of PrecompileCalled ast-grep --pattern 'func (j PrecompileCalled) Revert($$$) { $$$ }' # Also check where PrecompileCalled is used in the codebase rg "PrecompileCalled" -A 3Length of output: 3744
x/evm/precompile/funtoken.go (1)
75-75: LGTM: Simplified return statement aligns with new state management.The removal of explicit commit operation is correct as state changes are now handled by ApplyEvmMsg at the end of the Ethereum transaction, which aligns with the new JournalChange-based state management approach.
Let's verify the state management implementation:
✅ Verification successful
Let me gather more information about the state management implementation, particularly focusing on the precompile execution and state changes.
Based on the gathered information, I can now generate the final response:
Verified: State management is correctly handled through StateDB and journaling
The simplified return statement is correct as the state changes are properly managed through:
- StateDB's journaling mechanism via
CacheCtxForPrecompileandSavePrecompileCalledJournalChange- Explicit commit handling in the precompile base implementation
- Final state application in
ApplyEvmMsg🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify state management implementation # Expected: Find JournalChange usage and state handling in ApplyEvmMsg # Check for JournalChange implementation rg -A 5 "type JournalChange struct" # Verify state handling in ApplyEvmMsg rg -A 10 "func.*ApplyEvmMsg"Length of output: 740
Script:
#!/bin/bash # Check for state management in precompile execution rg -A 10 "func.*Run.*\(.*\).*\{" x/evm/precompile/ # Look for OnRunStart implementation rg -A 10 "func.*OnRunStart.*\(.*\).*\{" x/evm/precompile/ # Search for state commit patterns in precompiles rg "commit|StateDB|stateDB" x/evm/precompile/Length of output: 2239
x/evm/precompile/wasm_test.go (1)
103-103: Verify test coverage for precompile reversion scenarios.While the addition of
finalizeTx=truealigns with the PR's state management improvements, this happy path test doesn't validate the reversion behavior mentioned in the PR objectives. Consider adding test cases that verify:
- State consistency when a precompile call is reverted
- Proper handling of the multistore cache during reversion
- Behavior when state changes occur before and after precompile execution
Let's check if there are other test files covering these scenarios:
Also applies to: 109-109
x/evm/statedb/state_object.go (2)
264-264: LGTM: Consistent context usage for state queries.The context change in
GetCommittedState()maintains consistency with the state management improvements and aligns with the changes in theCode()method.Let's verify the context usage pattern in state-related operations:
#!/bin/bash # Search for all state query operations to ensure consistent context usage ast-grep --pattern 'GetState($ctx, $_)'
202-202: LGTM: Context change aligns with state management improvements.The switch to
evmTxCtxin theCode()method is consistent with the PR's objective of improving state management during EVM transactions and precompile execution.Let's verify that this context change is consistently applied across the codebase:
✅ Verification successful
Context change is correctly implemented and consistent
The switch to
evmTxCtxin theCode()method is part of a broader, consistent pattern across the codebase where:
- All state access operations (GetCode, GetState, etc.) use
evmTxCtx- The context change aligns with the state management architecture where
evmTxCtxis the persistent context for official state commits- The implementation properly handles state caching and reversion through
cacheCtx🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining instances of direct ctx usage in state management rg -g '*.go' 'db\.ctx' x/evm/Length of output: 41
Script:
#!/bin/bash # Let's search for any usage of ctx or evmTxCtx in the state management related files rg -g '*.go' -A 3 -B 3 '(ctx|evmTxCtx)' x/evm/statedb/ # Also check for any GetCode calls to verify context usage pattern rg -g '*.go' -A 3 -B 3 'GetCode' x/evm/statedb/ # Search for any state object related methods to ensure context consistency ast-grep --pattern 'func ($s *stateObject) $_($$$) { $$$ }'Length of output: 15961
x/evm/precompile/wasm.go (2)
Line range hint
1-400: Verify test coverage for state reversion scenarios.Given the critical nature of Wasm precompile operations (especially those involving funds transfer and contract deployment), please ensure comprehensive test coverage for state reversion scenarios. Consider adding tests that specifically verify:
- Proper reversion of funds transfer in execute/executeMulti when EVM transaction reverts
- State consistency after failed contract instantiation
- Multi-operation scenarios where some operations succeed before a revert
Let's check the existing test coverage:
#!/bin/bash # Description: Analyze test coverage for state reversion scenarios # Search for relevant test files echo "Searching for Wasm precompile tests..." fd -e go -e sol test.*wasm # Look for reversion test cases echo "Searching for reversion test cases..." rg -p "TestRevert|test.*revert|should.*revert" -t go # Check for fund transfer tests echo "Searching for fund transfer tests..." rg -p "test.*funds.*transfer|test.*execute.*funds" -t go
65-65: LGTM: State management improvement aligns with PR objectives.The removal of state commitment from the Run method is a positive change that supports proper state reversion. This aligns with the PR's objective of ensuring synchronized state handling between EVM and non-EVM operations during precompile execution.
Let's verify the state handling implementation:
✅ Verification successful
Based on the search results, I can now generate a final response:
Verified: State management change is correctly implemented
The removal of state commitment from the Wasm precompile's Run method is correct because:
- The Run method now properly delegates state management to the EVM layer, as seen in the implementation
- The statedb package contains journal implementation that handles state snapshots and reversions (
x/evm/statedb/journal.go)- Error handling is in place for revert scenarios (
x/evm/errors.go)This change aligns with the PR's objective of improving precompile reversion handling by ensuring state changes are managed at the appropriate level in the execution stack.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify state handling implementation across the codebase # Looking for: # 1. Where state commitment now occurs # 2. Implementation of JournalChange struct # 3. Usage of multistore caching # Search for JournalChange struct and its usage echo "Searching for JournalChange implementation..." rg -p "type.*JournalChange.*struct" # Search for multistore caching implementation echo "Searching for multistore caching..." rg -p "CacheMultiStore|cache.*multistore" # Search for state commitment patterns echo "Searching for state commitment patterns..." rg -p "CommitCache|Commit.*State|StateDB.*commit"Length of output: 656
Script:
#!/bin/bash # Let's try a more focused search approach # 1. Look for state handling in wasm precompile echo "Searching for wasm precompile implementation..." rg -p "func.*Run.*" x/evm/precompile/wasm.go -A 10 # 2. Look for state management in precompile base echo "Searching for precompile state management..." rg -p "type.*StateDB" x/evm/precompile/ # 3. Check for journal/state changes in EVM package echo "Searching for state journal implementation..." rg -p "type.*journal|func.*snapshot|func.*revert" x/evm/ # 4. Look for multistore state handling echo "Searching for multistore state handling..." fd -e go | xargs rg -p "func.*CacheMultiStore|func.*Commit.*Store"Length of output: 1325
x/evm/keeper/erc20.go (1)
222-222: Verify NoOpTracer usage in precompile scenarios.The addition of
NewNoOpTracer()appears to be related to handling precompile reversions. However, we should verify if this tracer type is appropriate for all contract execution scenarios, especially those involving state-modifying precompiles.Let's verify the tracer usage in precompile calls:
✅ Verification successful
Let me gather more context about the tracer usage and its relationship with precompiles.
Based on the search results, I can now generate a final response regarding the NoOpTracer usage:
NoOpTracer is appropriate for this precompile reversion scenario
The NoOpTracer implementation is a no-op tracer that implements the vm.EVMLogger interface without performing any actual tracing operations. This is suitable for the precompile reversion case because:
- The tracer is used consistently across other precompile calls in the codebase (grpc_query.go, evmante_can_transfer.go)
- The NoOpTracer doesn't add any overhead while still providing the required interface for EVM execution
- The precompile reversion functionality relies on the StateDB journal system rather than tracing capabilities
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for other tracer implementations and their usage patterns # Look for other tracer implementations rg "NewNoOpTracer|interface\s+EVMTracer|type.*Tracer\s+struct" # Look for precompile-related code that might need tracing ast-grep --pattern 'func $_(ctx $_, msg $_, tracer $_) $_ { $$$ }'Length of output: 652
Script:
#!/bin/bash # Check for tracer usage in ApplyEvmMsg and related functions rg -A 5 "ApplyEvmMsg.*tracer" # Look for precompile-related code and state handling rg -A 5 "precompile|StateDB" x/evm/ # Check vmtracer.go implementation cat x/evm/vmtracer.goLength of output: 120066
x/evm/precompile/test/export.go (2)
5-5: LGTM: Import changes are properly organized.The addition of "math/big" is necessary for handling large integers in the new VM call implementation, and the imports are correctly organized with standard library imports separated by a blank line.
Also applies to: 11-12
273-273: LGTM: Parameter addition supports precompile reversion.The new
finalizeTxparameter aligns with the PR objectives by providing control over transaction finalization during precompile execution, which is crucial for the state management system.Let's verify the consistent usage of this parameter:
Also applies to: 315-315
✅ Verification successful
Parameter usage is consistent across test files
The verification shows that all calls to
IncrementWasmCounterWithExecuteMultiproperly handle thefinalizeTxparameter:
- In
x/evm/precompile/wasm_test.go: Explicitly setsfinalizeTxtotrue- In
x/evm/statedb/journal_test.go: UsescommitEvmTxvariable to control finalization- The parameter is correctly propagated to
CallContractWithInputin the implementationThe parameter addition and its usage are consistent with the intended behavior for precompile reversion control.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for consistent usage of finalizeTx parameter in test files # Expect: All test calls should explicitly specify the finalization behavior rg -A 2 "IncrementWasmCounterWithExecuteMulti\(" "**/*_test.go"Length of output: 118
Script:
#!/bin/bash # Let's try searching without the test file restriction first to find all usages rg -A 2 "IncrementWasmCounterWithExecuteMulti\(" # Also search for the function definition to understand its usage ast-grep --pattern 'func IncrementWasmCounterWithExecuteMulti($$$) $$$'Length of output: 4486
go.mod (1)
247-248: LGTM! Dependency replacements align with PR objectives.The replacements to NibiruChain's forks of
wasmdandcosmos-sdkare necessary for implementing deep copies of the multistore and supporting precompile reversion handling.Let's verify these dependencies are properly tagged:
✅ Verification successful
✅ Dependency versions verified in NibiruChain repositories
The required versions exist as tags in both repositories:
v0.44.0-nibirutag exists in NibiruChain/wasmdv0.47.11-nibirutag exists in NibiruChain/cosmos-sdkBoth repositories also have corresponding branches, confirming these are maintained forks.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the specified versions exist in the NibiruChain forks # Test: Check if the wasmd version exists gh release view v0.44.0-nibiru --repo NibiruChain/wasmd # Test: Check if the cosmos-sdk version exists gh release view v0.47.11-nibiru --repo NibiruChain/cosmos-sdkLength of output: 157
Script:
#!/bin/bash # Let's check if these versions exist as tags instead of releases # and also look for any related branches or commits # Check wasmd tags and branches echo "=== wasmd repository ===" gh api repos/NibiruChain/wasmd/tags --paginate | jq -r '.[].name' | grep -i nibiru gh api repos/NibiruChain/wasmd/branches --paginate | jq -r '.[].name' | grep -i nibiru echo -e "\n=== cosmos-sdk repository ===" # Check cosmos-sdk tags and branches gh api repos/NibiruChain/cosmos-sdk/tags --paginate | jq -r '.[].name' | grep -i nibiru gh api repos/NibiruChain/cosmos-sdk/branches --paginate | jq -r '.[].name' | grep -i nibiru # Let's also check if these are valid go modules by trying to list versions echo -e "\n=== Go module versions ===" go list -m -versions github.com/NibiruChain/wasmd go list -m -versions github.com/NibiruChain/cosmos-sdkLength of output: 869
app/keepers.go (4)
143-143: LGTM: Proper field addition for bank keeper separation.The addition of
bankBaseKeeperfield toprivateKeepersstruct follows the recommended pattern for keeper organization in Cosmos SDK applications.
267-274: LGTM: Secure initialization of bank keeper.The initialization follows best practices:
- Creates base keeper first
- Assigns it to the app's BankKeeper field
- Uses proper permission settings through
BlockedAddresses()- Correctly sets governance module address for params
This change aligns with the PR objective of implementing a safer bank keeper.
611-611: LGTM: Correct module initialization with base keeper.The bank module is properly initialized using the
bankBaseKeeperinstead of the interface, which provides better type safety and aligns with the PR's security objectives.
Line range hint
267-611: Verify keeper dependencies are properly initialized.The bank keeper is used by multiple modules. Let's verify that the initialization order is correct and all dependent modules are using the proper keeper reference.
✅ Verification successful
Bank keeper initialization and dependencies are properly ordered
The initialization sequence is correct:
bankBaseKeeperis initialized first withNewBaseKeeperapp.BankKeeperis set to referencebankBaseKeeper- All dependent modules (Staking, Oracle, Inflation, DevGas, TokenFactory) are initialized after the bank keeper
The code follows the proper initialization order, ensuring that dependent modules have access to a fully initialized bank keeper interface.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for potential keeper initialization order issues # Test: Search for BankKeeper usage in keeper initialization rg -A 2 "BankKeeper.*NewKeeper"Length of output: 33
Script:
#!/bin/bash # Let's try a more comprehensive search for keeper initialization patterns # First, search for NewKeeper patterns that take BankKeeper as parameter ast-grep --pattern 'NewKeeper($_,$_,$_,app.BankKeeper,$$$)' # Also search for direct BankKeeper references in keeper initialization rg -l "BankKeeper.*NewKeeper|NewKeeper.*BankKeeper" # Finally, let's check the actual keeper initialization order in the app rg "app\..*Keeper\s*=.*NewKeeper" app/keepers.goLength of output: 1388
Script:
#!/bin/bash # Let's check the initialization order of bank keeper and its dependencies rg -B 2 -A 2 "app\.BankKeeper\s*=.*" app/keepers.go # Also check for any modules that require BankKeeper in their constructor rg "BankKeeper.*interface" --type go # And check the actual BankKeeper initialization rg -B 2 -A 2 "bankBaseKeeper.*NewBaseKeeper" app/keepers.goLength of output: 1484
CHANGELOG.md (1)
72-80: LGTM! The changelog entry is well-documented.The entry clearly describes:
- The relationship to previous changes (PR fix(evm): commit temporary state on precompile start, avoiding bug stemming from uncommitted, dirty journal entries in the EVM StateDB #2086)
- The introduction of the new
JournalChangestruct- The purpose and benefits of the changes
- The specific scenarios where this fix is important
x/evm/statedb/journal_test.go (9)
9-14: Approval of Updated Import StatementsThe addition of
github.com/MakeNowJust/heredoc/v2and other import statements are appropriate and necessary for the updated code logic.
23-23: Function Renaming Enhances ClarityRenaming the test function to
TestComplexJournalChangesaccurately reflects the scope and complexity of the test cases within, improving code readability.
39-41: Assertion Function Used CorrectlyThe use of
test.AssertWasmCounterStatewith the initial counter value of0is appropriate and ensures the starting state is verified.
Line range hint
62-72: Effective Validation of Dirty Journal EntriesThe test accurately checks the count of dirty journal entries after state modifications using
stateDB.DebugDirtiesCount(), ensuring the StateDB behaves as expected during balance changes.
77-80: Proper State Commit and VerificationCommitting the StateDB and verifying that the dirty journal entries are cleared confirms the integrity of the commit operation.
102-102: Correct Dirty Entry Count After Contract CallThe assertion ensuring that
stateDB.DebugDirtiesCount()equals2after the contract call is precise and validates the expected state changes.
128-219: Addition of Comprehensive Test Cases for Precompile SnapshotsThe new subtests in
"Precompile calls populate snapshots"effectively test the StateDB's behavior with commit and revert operations during precompile calls. This strengthens the test suite by covering complex scenarios involving snapshots and ensuring state consistency.
191-197: Proper Error Handling with TryCatch FunctionThe use of
common.TryCatchto capture and assert errors when reverting to a non-existent snapshot (revision id 2) is appropriate and ensures error conditions are correctly tested.
225-226: Utilization of Debug Functions for Enhanced LoggingThe
debugDirtiesCountMismatchfunction effectively logs detailed information about dirty entries and state objects, aiding in debugging and ensuring test clarity.x/evm/statedb/journal.go (2)
341-343: Addition ofPrecompileCalledstruct is appropriateThe new
PrecompileCalledstruct is correctly defined withMultiStoreandEventsfields to handle state changes from precompile calls.
350-359:Revertmethod implementation ensures proper state rollbackThe
Revertmethod inPrecompileCalledeffectively reverts the state to its prior condition before the precompile call by restoringcacheCtxand updating the event manager and multistore.x/evm/statedb/statedb.go (1)
107-111: Ensure all references toGetContext()are updated toGetEvmTxContext()Please verify that any calls to
GetContext()have been updated toGetEvmTxContext()to reflect the method signature change.Run the following script to find any references to the old
GetContext()method:#!/bin/bash # Description: Find all references to 'GetContext(' in the codebase excluding 'statedb.go' # Expected result: No matches found rg '\.GetContext\(' --type go --glob '!x/evm/statedb/statedb.go'x/evm/keeper/msg_server.go (4)
Line range hint
273-275: Verify gas refund calculation aligns with EIP-3529The gas refund logic uses
params.RefundQuotientEIP3529, capping refunds to a fraction of the gas used. Verify that this implementation correctly reflects the EIP-3529 specification and that it calculates refunds accurately in all scenarios.Consider running tests to confirm the refund calculations are correct.
Line range hint
299-305: Ensure correct gas usage and overflow checksThe checks for gas overflow and gas usage are critical for preventing exploitation. Confirm that the conditions:
if msg.Gas() < leftoverGas { return nil, evmObj, errors.Wrap(evm.ErrGasOverflow, "apply message") }and subsequent gas calculations accurately represent intended logic and prevent any potential gas-related vulnerabilities.
Line range hint
218-321: Check for potential race conditions with concurrent state accessEnsure that the state modifications within
ApplyEvmMsgare thread-safe and properly synchronized. In scenarios with concurrent transactions, improper state handling could lead to race conditions or inconsistent states.If applicable, review concurrency mechanisms in place to handle state access.
255-255: EnsureNewStateDBinitializes the state correctlyThe instantiation of
stateDBhas been updated to usek.NewStateDB(ctx, txConfig). This change aligns with the new state management approach. Verify thatk.NewStateDBproperly initializes the state database with all necessary configurations and handles any potential edge cases.You may run the following script to check the implementation of
NewStateDB:✅ Verification successful
Let me gather more information about the state initialization by checking the
statedb.Newimplementation.
Let me check the implementation of the helper functions used in state initialization to ensure they're properly implemented.
StateDB initialization verified and properly implemented
The
NewStateDBimplementation correctly initializes all required components:
- Creates a new StateDB with proper context and keeper
- Initializes state objects map for tracking accounts
- Sets up journal for state changes tracking
- Configures access list for EIP-2930
- Properly handles transaction configuration
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the implementation of k.NewStateDB # Expected: Find the definition of NewStateDB with correct parameters. ast-grep --pattern $'func (k *Keeper) NewStateDB(ctx $_, txConfig $_) $_ { $$$ }' x/evm/keeper/statedb.goLength of output: 404
Script:
#!/bin/bash # Check the implementation of statedb.New ast-grep --pattern $'func New(ctx $_, keeper $_, txConfig $_) $_ { $$$ }' # Also check for any test files that verify the state initialization rg -g "**/*_test.go" "TestNew.*StateDB" -A 5Length of output: 709
Script:
#!/bin/bash # Check implementation of journal and accessList initialization ast-grep --pattern $'func newJournal() $_ { $$$ }' ast-grep --pattern $'func newAccessList() $_ { $$$ }' # Check if there are any other relevant tests rg -g "**/*_test.go" "statedb.*Test" -A 5Length of output: 647
| return deps.EvmKeeper.NewStateDB( | ||
| deps.Ctx, |
There was a problem hiding this comment.
💡 Codebase verification
Direct statedb.New usage needs to be migrated to EvmKeeper.NewStateDB
Found inconsistent StateDB creation patterns that should be standardized:
app/evmante/evmante_can_transfer.go: Uses directstatedb.Newinstead ofEvmKeeper.NewStateDBx/evm/statedb/statedb_test.go: Uses directstatedb.Newin tests
The only legitimate direct usage is in x/evm/keeper/statedb.go which implements the NewStateDB method itself.
🔗 Analysis chain
LGTM! Improved StateDB creation pattern.
This change properly delegates StateDB creation to EvmKeeper, ensuring test scenarios use the same state management path as production code. This alignment strengthens test reliability and supports the PR's goal of safer state handling.
Let's verify consistent StateDB creation patterns across the codebase:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any direct statedb.New calls that should use EvmKeeper.NewStateDB
# Expected: Only EvmKeeper should directly call statedb.New
# Search for direct statedb.New usage
echo "Checking for direct statedb.New usage..."
rg "statedb\.New\(" -A 3
# Search for EvmKeeper.NewStateDB usage
echo "Checking for EvmKeeper.NewStateDB usage..."
rg "\.NewStateDB\(" -A 3
Length of output: 1451
| if err = stateDB.SavePrecompileCalledJournalChange(contract.Address(), journalEntry); err != nil { | ||
| return res, err | ||
| } | ||
| if err = stateDB.CommitCacheCtx(); err != nil { | ||
| return res, fmt.Errorf("error committing dirty journal entries: %w", err) |
There was a problem hiding this comment.
Consider transaction-like semantics for journal entry and cache operations.
If CommitCacheCtx fails after successfully saving the journal entry, we should consider reverting the saved journal entry to maintain consistency.
Consider wrapping both operations in a transaction-like pattern:
- if err = stateDB.SavePrecompileCalledJournalChange(contract.Address(), journalEntry); err != nil {
- return res, err
- }
- if err = stateDB.CommitCacheCtx(); err != nil {
- return res, fmt.Errorf("error committing dirty journal entries: %w", err)
- }
+ if err = stateDB.SavePrecompileCalledJournalChange(contract.Address(), journalEntry); err != nil {
+ return res, err
+ }
+ if err = stateDB.CommitCacheCtx(); err != nil {
+ // Revert the journal entry if cache commit fails
+ stateDB.RevertPrecompileCalledJournalChange(contract.Address())
+ return res, fmt.Errorf("error committing dirty journal entries: %w", err)
+ }Committable suggestion was skipped due to low confidence.
| func IncrementWasmCounterWithExecuteMultiViaVMCall( | ||
| s *suite.Suite, | ||
| deps *evmtest.TestDeps, | ||
| wasmContract sdk.AccAddress, | ||
| times uint, | ||
| finalizeTx bool, | ||
| evmObj *vm.EVM, | ||
| ) error { | ||
| msgArgsBz := []byte(` | ||
| { | ||
| "increment": {} | ||
| } | ||
| `) | ||
|
|
||
| // Parse funds argument. | ||
| var funds []precompile.WasmBankCoin // blank funds | ||
| fundsJson, err := json.Marshal(funds) | ||
| s.NoErrorf(err, "fundsJson: %s", fundsJson) | ||
| err = json.Unmarshal(fundsJson, &funds) | ||
| s.Require().NoError(err, "fundsJson %s, funds %s", fundsJson, funds) | ||
|
|
||
| // The "times" arg determines the number of messages in the executeMsgs slice | ||
| executeMsgs := []struct { | ||
| ContractAddr string `json:"contractAddr"` | ||
| MsgArgs []byte `json:"msgArgs"` | ||
| Funds []precompile.WasmBankCoin `json:"funds"` | ||
| }{ | ||
| {wasmContract.String(), msgArgsBz, funds}, | ||
| } | ||
| if times == 0 { | ||
| executeMsgs = executeMsgs[:0] // force empty | ||
| } else { | ||
| for i := uint(1); i < times; i++ { | ||
| executeMsgs = append(executeMsgs, executeMsgs[0]) | ||
| } | ||
| } | ||
| s.Require().Len(executeMsgs, int(times)) // sanity check assertion | ||
|
|
||
| callArgs := []any{ | ||
| executeMsgs, | ||
| } | ||
| input, err := embeds.SmartContract_Wasm.ABI.Pack( | ||
| string(precompile.WasmMethod_executeMulti), | ||
| callArgs..., | ||
| ) | ||
| s.Require().NoError(err) | ||
|
|
||
| contract := precompile.PrecompileAddr_Wasm | ||
| leftoverGas := serverconfig.DefaultEthCallGasLimit | ||
| _, _, err = evmObj.Call( | ||
| vm.AccountRef(deps.Sender.EthAddr), | ||
| contract, | ||
| input, | ||
| leftoverGas, | ||
| big.NewInt(0), | ||
| ) | ||
| return err | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider refactoring duplicated code and improving gas limit configuration.
While the function serves a specific purpose for testing intermediate states using direct VM calls, there are two areas for improvement:
-
There's significant code duplication with
IncrementWasmCounterWithExecuteMulti. Consider extracting the common message preparation and input packing logic into a shared helper function. -
The gas limit is hardcoded using
serverconfig.DefaultEthCallGasLimit. Consider making it configurable through a parameter to support different test scenarios.
Here's a suggested refactor to reduce duplication:
+// prepareWasmExecuteMultiInput prepares the input for Wasm executeMulti calls
+func prepareWasmExecuteMultiInput(
+ s *suite.Suite,
+ wasmContract sdk.AccAddress,
+ times uint,
+) ([]byte, error) {
+ msgArgsBz := []byte(`{ "increment": {} }`)
+
+ // Parse funds argument.
+ var funds []precompile.WasmBankCoin
+ fundsJson, err := json.Marshal(funds)
+ if err != nil {
+ return nil, err
+ }
+ err = json.Unmarshal(fundsJson, &funds)
+ if err != nil {
+ return nil, err
+ }
+
+ // Prepare execute messages
+ executeMsgs := []struct {
+ ContractAddr string `json:"contractAddr"`
+ MsgArgs []byte `json:"msgArgs"`
+ Funds []precompile.WasmBankCoin `json:"funds"`
+ }{
+ {wasmContract.String(), msgArgsBz, funds},
+ }
+ if times == 0 {
+ executeMsgs = executeMsgs[:0]
+ } else {
+ for i := uint(1); i < times; i++ {
+ executeMsgs = append(executeMsgs, executeMsgs[0])
+ }
+ }
+ s.Require().Len(executeMsgs, int(times))
+
+ // Pack input
+ return embeds.SmartContract_Wasm.ABI.Pack(
+ string(precompile.WasmMethod_executeMulti),
+ []any{executeMsgs}...,
+ )
+}
func IncrementWasmCounterWithExecuteMulti(
s *suite.Suite,
deps *evmtest.TestDeps,
wasmContract sdk.AccAddress,
times uint,
finalizeTx bool,
) (evmObj *vm.EVM) {
- msgArgsBz := []byte(`{ "increment": {} }`)
- // ... remove duplicated message preparation code ...
+ input, err := prepareWasmExecuteMultiInput(s, wasmContract, times)
+ s.Require().NoError(err)
ethTxResp, evmObj, err := deps.EvmKeeper.CallContractWithInput(
deps.Ctx, deps.Sender.EthAddr, &precompile.PrecompileAddr_Wasm, finalizeTx, input,
)
s.Require().NoError(err)
s.Require().NotEmpty(ethTxResp.Ret)
return evmObj
}
func IncrementWasmCounterWithExecuteMultiViaVMCall(
s *suite.Suite,
deps *evmtest.TestDeps,
wasmContract sdk.AccAddress,
times uint,
finalizeTx bool,
evmObj *vm.EVM,
+ gasLimit uint64, // Add gas limit parameter
) error {
- msgArgsBz := []byte(`{ "increment": {} }`)
- // ... remove duplicated message preparation code ...
+ input, err := prepareWasmExecuteMultiInput(s, wasmContract, times)
+ if err != nil {
+ return err
+ }
contract := precompile.PrecompileAddr_Wasm
- leftoverGas := serverconfig.DefaultEthCallGasLimit
+ if gasLimit == 0 {
+ gasLimit = serverconfig.DefaultEthCallGasLimit
+ }
_, _, err = evmObj.Call(
vm.AccountRef(deps.Sender.EthAddr),
contract,
input,
- leftoverGas,
+ gasLimit,
big.NewInt(0),
)
return err
}Committable suggestion was skipped due to low confidence.
| func NewEthTxMsgFromTxData( | ||
| deps *TestDeps, | ||
| txType GethTxType, | ||
| innerTxData []byte, | ||
| nonce uint64, | ||
| to *gethcommon.Address, | ||
| value *big.Int, | ||
| gas uint64, | ||
| accessList gethcore.AccessList, | ||
| ) (*evm.MsgEthereumTx, error) { | ||
| if innerTxData == nil { | ||
| innerTxData = []byte{} | ||
| } | ||
|
|
||
| var ethCoreTx *gethcore.Transaction | ||
| switch txType { | ||
| case gethcore.LegacyTxType: | ||
| innerTx := TxTemplateLegacyTx() | ||
| innerTx.Nonce = nonce | ||
| innerTx.Data = innerTxData | ||
| innerTx.To = to | ||
| innerTx.Value = value | ||
| innerTx.Gas = gas | ||
| ethCoreTx = gethcore.NewTx(innerTx) | ||
| case gethcore.AccessListTxType: | ||
| innerTx := TxTemplateAccessListTx() | ||
| innerTx.Nonce = nonce | ||
| innerTx.Data = innerTxData | ||
| innerTx.AccessList = accessList | ||
| innerTx.To = to | ||
| innerTx.Value = value | ||
| innerTx.Gas = gas | ||
| ethCoreTx = gethcore.NewTx(innerTx) | ||
| case gethcore.DynamicFeeTxType: | ||
| innerTx := TxTemplateDynamicFeeTx() | ||
| innerTx.Nonce = nonce | ||
| innerTx.Data = innerTxData | ||
| innerTx.To = to | ||
| innerTx.Value = value | ||
| innerTx.Gas = gas | ||
| innerTx.AccessList = accessList | ||
| ethCoreTx = gethcore.NewTx(innerTx) | ||
| default: | ||
| return nil, fmt.Errorf( | ||
| "received unknown tx type (%v) in NewEthTxMsgFromTxData", txType) | ||
| } | ||
|
|
||
| ethTxMsg := new(evm.MsgEthereumTx) | ||
| if err := ethTxMsg.FromEthereumTx(ethCoreTx); err != nil { | ||
| return ethTxMsg, err | ||
| } | ||
|
|
||
| ethTxMsg.From = deps.Sender.EthAddr.Hex() | ||
| return ethTxMsg, ethTxMsg.Sign(deps.GethSigner(), deps.Sender.KeyringSigner) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding input validation and refactoring common field assignments.
The function is well-documented and handles different transaction types correctly. Consider these improvements:
- Add validation for the
valueparameter to prevent potential issues with nil or negative values. - Extract common field assignments to reduce code duplication.
Here's a suggested refactoring:
func NewEthTxMsgFromTxData(
deps *TestDeps,
txType GethTxType,
innerTxData []byte,
nonce uint64,
to *gethcommon.Address,
value *big.Int,
gas uint64,
accessList gethcore.AccessList,
) (*evm.MsgEthereumTx, error) {
+ if value == nil {
+ return nil, fmt.Errorf("value cannot be nil")
+ }
+ if value.Sign() < 0 {
+ return nil, fmt.Errorf("negative value not allowed")
+ }
+
if innerTxData == nil {
innerTxData = []byte{}
}
+ // Common fields for all transaction types
+ commonFields := struct {
+ nonce uint64
+ data []byte
+ to *gethcommon.Address
+ value *big.Int
+ gas uint64
+ }{nonce, innerTxData, to, value, gas}
+
var ethCoreTx *gethcore.Transaction
switch txType {
case gethcore.LegacyTxType:
innerTx := TxTemplateLegacyTx()
- innerTx.Nonce = nonce
- innerTx.Data = innerTxData
- innerTx.To = to
- innerTx.Value = value
- innerTx.Gas = gas
+ innerTx.Nonce = commonFields.nonce
+ innerTx.Data = commonFields.data
+ innerTx.To = commonFields.to
+ innerTx.Value = commonFields.value
+ innerTx.Gas = commonFields.gas
ethCoreTx = gethcore.NewTx(innerTx)
// ... similar changes for other casesCommittable suggestion was skipped due to low confidence.
| // cacheCtx: An sdk.Context produced from the [StateDB.ctx] with the | ||
| // multi-store cached and a new event manager. The cached context | ||
| // (`cacheCtx`) is written to the persistent context (`ctx`) when | ||
| // `writeCacheCtx` is called. | ||
| cacheCtx sdk.Context |
There was a problem hiding this comment.
Update comments to reflect evmTxCtx
The comments for cacheCtx refer to [StateDB.ctx] and ctx, but the field has been renamed to evmTxCtx. Please update the comments to reflect this change for consistency.
| ctx sdk.Context | ||
|
|
||
| // evmTxCtx is the persistent context used for official `StateDB.Commit` calls. | ||
| evmTxCtx sdk.Context |
There was a problem hiding this comment.
💡 Codebase verification
Based on the search results, I can now provide a final response:
ctx references need to be updated in commitCtx method
The field renaming from ctx to evmTxCtx is mostly complete, but there are still references to ctx in the commitCtx method that should be using evmTxCtx instead. The following locations need to be updated:
x/evm/statedb/statedb.go: In thecommitCtxmethod:s.keeper.DeleteAccount(ctx, obj.Address())s.keeper.SetCode(ctx, obj.CodeHash(), obj.code)s.keeper.SetAccount(ctx, obj.Address(), obj.account.ToNative())s.keeper.SetState(ctx, obj.Address(), key, dirtyVal.Bytes())
Additionally, there's an outdated comment referencing StateDB.ctx that should be updated:
- Line ~56: Comment mentions
[StateDB.ctx]which should be[StateDB.evmTxCtx]
🔗 Analysis chain
Verify all references to ctx have been updated to evmTxCtx
Please ensure that all code within StateDB that previously referenced ctx now correctly references evmTxCtx to prevent any unintended behavior.
Run the following script to verify that there are no remaining references to s.ctx:
Let me search more specifically for s.ctx references to verify the review comment.
Let me search specifically for the StateDB struct and its usage to verify the field renaming.
Let me search for any remaining references to ctx in the statedb file to ensure complete verification.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all references to 's.ctx' in the codebase excluding 'statedb.go'
# Expected result: No matches found
rg 's\.ctx' --type go --glob '!x/evm/statedb/statedb.go'
Length of output: 957
Script:
#!/bin/bash
# Search specifically for 's.ctx' in go files
rg 's\.ctx' --type go -A 2
# Also search for potential references to ctx field in StateDB struct
ast-grep --pattern 'type StateDB struct {
$$$
ctx $$$
$$$
}'
Length of output: 19683
Script:
#!/bin/bash
# Search for StateDB struct and its field usage
ast-grep --pattern 'type StateDB struct {
$$$
}'
# Also search for any remaining references to evmTxCtx in the statedb file
rg 'evmTxCtx' x/evm/statedb/statedb.go -A 2
Length of output: 3732
Script:
#!/bin/bash
# Search for any remaining references to 'ctx' in statedb.go
rg '\bctx\b' x/evm/statedb/statedb.go -A 2
# Also search for any method calls using ctx vs evmTxCtx
ast-grep --pattern '$obj.$method(ctx, $$$)'
Length of output: 1159
| @@ -1,3 +1,5 @@ | |||
| package statedb | |||
There was a problem hiding this comment.
Is there a linter that moves package declarations above file-level comments?
| @@ -17,6 +17,13 @@ import ( | |||
|
|
|||
| var _ statedb.Keeper = &Keeper{} | |||
|
|
|||
| func (k *Keeper) NewStateDB( | |||
There was a problem hiding this comment.
I would get rid of this function because it's only a one-line replacement, we use statedb.New in other parts of the codebase, and it keeps EVMKeeper more decoupled from StateDB.
There was a problem hiding this comment.
The rationale's not as obvious with the next change separated out onto #2095. For context, this is the full function.
func (evmKeeper *Keeper) NewStateDB(
ctx sdk.Context, txConfig statedb.TxConfig,
) *statedb.StateDB {
stateDB := statedb.New(ctx, evmKeeper, txConfig)
evmKeeper.Bank.ResetStateDB(stateDB)
return stateDB
}This constructor's meant to be used any time the EVM keeper generates a StateDB to make sure that its bank keeper reference is always pointing to the current StateDB for any EVM transaction.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
CHANGELOG.md (1)
72-80: Consider improving the changelog entry format and clarity.
The changelog entry for PR #2094 could be enhanced for better readability and understanding:
- Break down the long lines into bullet points for better readability
- Add more details about the security implications and potential risks mitigated
- Clarify how the
JournalChangestruct interacts with state management
Suggested format:
- [#2094](https://github.com/NibiruChain/nibiru/pull/2094) - fix(evm): Following
- from the changs in #2086, this pull request implements a new `JournalChange`
- struct that saves a deep copy of the state multi store before each
- state-modifying, Nibiru-specific precompiled contract is called (`OnRunStart`).
- Additionally, we commit the `StateDB` there as well. This guarantees that the
- non-EVM and EVM state will be in sync even if there are complex, multi-step
- Ethereum transactions, such as in the case of an EthereumTx that influences the
- `StateDB`, then calls a precompile that also changes non-EVM state, and then EVM
- reverts inside of a try-catch.
+ [#2094](https://github.com/NibiruChain/nibiru/pull/2094) - fix(evm): State consistency improvements for precompile execution
+
+ Following changes in #2086, this PR:
+ - Implements a new `JournalChange` struct to manage state during precompile execution
+ - Creates a deep copy of the state multistore before executing state-modifying precompiles
+ - Commits StateDB changes during `OnRunStart` phase
+ - Ensures state consistency between EVM and non-EVM states
+ - Handles complex scenarios including:
+ * Multi-step Ethereum transactions
+ * Precompile calls that modify non-EVM state
+ * EVM reverts within try-catch blocks
+
+ Security Impact:
+ - Prevents state inconsistencies during precompile execution
+ - Maintains data integrity across EVM and non-EVM states
+ - Provides proper rollback mechanisms for failed transactions📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- CHANGELOG.md (1 hunks)
- x/evm/precompile/funtoken.go (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- x/evm/precompile/funtoken.go
Purpose / Abstract
fix(evm): Following from the changes in #2086, this pull request implements a critical security
fix
Multistore copy to safely revert precompile changes outside EVM
First, we add new
JournalChangestruct that saves a deep copy of thestate multi store before each state-modifying, Nibiru-specific precompiled
contract is called (
OnRunStart). Additionally, we commit theStateDBthereas well. This guarantees that the non-EVM and EVM state will be in sync even
if there are complex, multi-step Ethereum transactions, such as in the case of
an EthereumTx that influences the
StateDB, then calls a precompile that alsochanges non-EVM state, and then EVM reverts inside of a try-catch.
Related Tickets
Summary by CodeRabbit
Release Notes
New Features
JournalChangestruct for state consistency during contract calls.IsPrecompilemethod for checking precompiled contracts.IncrementWasmCounterWithExecuteMultiand new VM call support.NewStateDBmethod for creating state database instances.PrecompileCalledstruct for managing state changes from precompiles.Improvements
StateDBfor better state handling.Bug Fixes
Documentation