fix: use snapshot to support revert #90
Conversation
WalkthroughThe pull request introduces several modifications to the Changes
Possibly related PRs
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #90 +/- ##
==========================================
+ Coverage 28.22% 28.24% +0.02%
==========================================
Files 124 124
Lines 13736 13756 +20
==========================================
+ Hits 3877 3886 +9
- Misses 9307 9317 +10
- Partials 552 553 +1
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 15
🧹 Outside diff range and nitpick comments (9)
x/evm/precompiles/erc20_registry/contract_test.go (2)
93-94: LGTM: Improved error handling precision.The use of
require.ErrorIsprovides more precise error type checking, which is a good practice for testing specific error conditions.Consider adding a comment explaining the out-of-gas test case for better test documentation:
+// Verify that the operation fails with out-of-gas when given insufficient gas _, _, err = registry.ExtendedRun(vm.AccountRef(erc20Addr), bz, precompiles.REGISTER_GAS-1, false) require.ErrorIs(t, err, vm.ErrOutOfGas)
159-160: Update the misleading comment.The comment "out of gas panic" is inaccurate as the code is testing for an error, not a panic.
-// out of gas panic +// verify out-of-gas error for read operation _, _, err = registry.ExtendedRun(vm.AccountRef(erc20Addr), bz, precompiles.IS_STORE_REGISTERED_GAS-1, true) require.ErrorIs(t, err, vm.ErrOutOfGas)x/evm/precompiles/erc20_registry/common_test.go (2)
18-26: Add godoc comments for the MockStateDB type.While the struct fields have inline comments, adding a detailed godoc comment for the
MockStateDBtype would improve documentation. Consider documenting:
- The purpose of this mock
- How it manages context and snapshots
- Its intended use in tests
Example addition:
+// MockStateDB implements the evmtypes.StateDB interface for testing purposes. +// It maintains a stack of context snapshots that can be created and reverted, +// while leaving other StateDB methods unimplemented. type MockStateDB struct {
36-49: Add bounds check in Snapshot method.The
Snapshotmethod calculates the snapshot ID without checking ifm.snapsis empty. While this works in the current implementation, it's better to be explicit.func (m *MockStateDB) Snapshot() int { // get a current snapshot id - sid := len(m.snaps) - 1 + sid := -1 + if len(m.snaps) > 0 { + sid = len(m.snaps) - 1 + }x/evm/precompiles/cosmos/common_test.go (4)
34-39: Initialize maps in constructorThe constructor should initialize the
snapsslice to avoid potential nil pointer dereferences.func NewMockStateDB(ctx sdk.Context) *MockStateDB { return &MockStateDB{ ctx: ctx, initialCtx: ctx, + snaps: make([]*state.Snapshot, 0), } }
84-252: Enhance panic messages in unimplemented methodsThe panic messages should be more descriptive to aid in debugging.
Example for one method (apply similar pattern to others):
func (m *MockStateDB) AddAddressToAccessList(addr common.Address) { - panic("unimplemented") + panic("MockStateDB.AddAddressToAccessList: method not implemented") }
256-260: Add constructor for MockAccountKeeperThe struct should have a constructor to properly initialize its fields.
+func NewMockAccountKeeper(ac address.Codec) *MockAccountKeeper { + return &MockAccountKeeper{ + ac: ac, + accounts: make(map[string]sdk.AccountI), + } +}
329-332: Add constructor for ERC20DenomKeeperThe struct should have a constructor to properly initialize its maps.
+func NewMockERC20DenomKeeper() *MockERC20DenomKeeper { + return &MockERC20DenomKeeper{ + denomMap: make(map[string]common.Address), + addrMap: make(map[common.Address]string), + } +}x/evm/keeper/context.go (1)
Line range hint
208-227: Consider enhancing documentation for context preparation.While the implementation is solid, consider adding more detailed documentation:
- Document the purpose and implications of the recursive depth limit
- Add examples of when recursive calls might occur
- Explain the role of cosmos messages in the context
Consider adding this documentation above the function:
-// prepare SDK context for EVM execution -// 1. set cosmos messages to context -// 2. check recursive depth and increment it (the maximum depth is 16) +// prepareSDKContext prepares the SDK context for EVM execution by: +// 1. Initializing an empty slice for cosmos messages in the context, which will +// store any Cosmos SDK messages generated during EVM execution +// 2. Managing recursive depth to prevent stack overflow attacks: +// - Tracks the depth of recursive EVM calls +// - Enforces a maximum depth of 16 to prevent excessive recursion +// - This is particularly important for contract-to-contract calls and +// precompile executions that might trigger recursive EVM operations +// +// Returns: +// - Modified context with initialized message slice and updated recursive depth +// - ErrExceedMaxRecursiveDepth if the maximum recursive depth is exceeded
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (11)
- x/evm/keeper/context.go (2 hunks)
- x/evm/keeper/precompiles.go (1 hunks)
- x/evm/precompiles/cosmos/common_test.go (1 hunks)
- x/evm/precompiles/cosmos/contract.go (4 hunks)
- x/evm/precompiles/cosmos/contract_test.go (8 hunks)
- x/evm/precompiles/erc20_registry/common_test.go (1 hunks)
- x/evm/precompiles/erc20_registry/contract.go (3 hunks)
- x/evm/precompiles/erc20_registry/contract_test.go (5 hunks)
- x/evm/state/snapshot.go (1 hunks)
- x/evm/state/statedb.go (1 hunks)
- x/evm/types/expected_keeper.go (1 hunks)
🔇 Additional comments (17)
x/evm/state/snapshot.go (1)
27-29: LGTM! Clean implementation of Context getter.The new
Context()method is well-implemented, providing clean access to the underlying SDK context while maintaining encapsulation. This addition aligns well with the PR's objective of supporting revert functionality through snapshots.x/evm/keeper/precompiles.go (1)
56-56: LGTM! Verify interface implementation across precompiles.The change from
context.Contexttotypes.StateDBaligns well with supporting snapshot/revert functionality and matches Ethereum's architecture.Let's verify that all precompiles implement the WithStateDB interface:
✅ Verification successful
Based on the shell script results, I can now generate the final response:
All precompiles correctly implement WithStateDB interface
The verification confirms that both precompile contracts in the codebase implement the required
WithStateDBmethod:
ERC20RegistryPrecompileinx/evm/precompiles/erc20_registry/contract.goCosmosPrecompileinx/evm/precompiles/cosmos/contract.goBoth implementations properly handle the state database assignment, making the type assertion in
toMapsafe.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that all precompile contracts implement the WithStateDB interface # Expected: All precompile contracts should have a WithStateDB method # Search for all precompile contract definitions ast-grep --pattern 'type $_ struct { $$$ }' x/evm/precompiles/ # Search for WithStateDB implementations ast-grep --pattern 'func ($_ $_) WithStateDB(stateDB types.StateDB) vm.PrecompiledContract { $$$ }' x/evm/precompiles/Length of output: 6419
x/evm/precompiles/erc20_registry/contract_test.go (3)
78-79: LGTM: State management setup is appropriate.The transition to using
StateDBaligns with the PR's objective of supporting revert functionality through snapshots. The mock state setup is suitable for testing purposes.
110-111: LGTM: Consistent error handling implementation.The error handling follows the same precise pattern established earlier, maintaining consistency throughout the test suite.
142-143: LGTM: Error handling maintains consistency.The implementation continues to follow the established pattern for out-of-gas error checking.
x/evm/precompiles/erc20_registry/common_test.go (1)
1-16: LGTM: Well-organized package structure and imports.The package name follows Go test conventions, and imports are properly grouped.
x/evm/precompiles/cosmos/common_test.go (1)
1-352: Overall implementation looks goodThe mock implementations provide a solid foundation for testing the snapshot-based revert functionality. The StateDB implementation correctly handles context management for snapshots, while other mock implementations provide the necessary supporting functionality.
x/evm/keeper/context.go (2)
164-169: LGTM! Context preparation logic looks solid.The integration of
prepareSDKContextinCreateEVMis clean and maintains proper error handling.
196-196: Verify precompile compatibility with stateDB usage.The change to use
stateDBink.precompiles.toMaplooks correct, but we should verify that all precompile implementations support this change.Let's check for any precompile implementations that might still expect context:
✅ Verification successful
Let me gather more information about the precompile implementations to verify their compatibility with stateDB.
Let me try a different approach to check the precompile implementations.
Let me check one final aspect to ensure all precompiles properly implement the stateDB interface.
Based on the results from our verification scripts, I can now provide a final response:
Precompile implementations correctly support stateDB usage
The verification shows that:
- Both
CosmosPrecompileandERC20RegistryPrecompileproperly implement theWithStateDBmethod- The precompile structs correctly include
stateDB types.StateDBfield- The change to use
stateDBink.precompiles.toMapis consistent with the precompile implementations🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for precompile implementations that might still use context rg -g '!vendor/*' -g '*.go' 'type\s+\w+Precompile\s+struct' -A 10Length of output: 1532
Script:
#!/bin/bash # Search for precompile method signatures and their implementations ast-grep --pattern 'func ($_ *$_Precompile) $_($$$) $$$' # Also search for any direct context usage in precompile files rg -g '!vendor/*' -g '*precompile*.go' 'ctx\s+sdk\.Context'Length of output: 120
Script:
#!/bin/bash # Search for precompile method implementations and their parameters rg -g '!vendor/*' -g '*.go' 'func \((\w+)\s+\*\w+Precompile\)' -A 3 # Check how stateDB is used in precompile implementations rg -g '!vendor/*' -g '*.go' 'stateDB\s+types\.StateDB' -B 2 -A 2Length of output: 1758
Script:
#!/bin/bash # Check for the PrecompiledContract interface implementation rg -g '!vendor/*' -g '*.go' 'type\s+PrecompiledContract\s+interface' -A 10 # Look for any remaining context.Context usage in precompile methods rg -g '!vendor/*' -g '*precompile*.go' 'context\.Context|sdk\.Context' -B 2 -A 2Length of output: 159
x/evm/state/statedb.go (1)
592-598: Implementation aligns with snapshot management requirements.The method correctly integrates with the existing snapshot functionality and supports the revert feature by:
- Providing access to historical contexts
- Maintaining consistency with
Snapshot()andRevertToSnapshot()- Preserving the initial context state
Let's verify the usage of this method:
✅ Verification successful
Let me gather more context about the usage patterns by examining the implementation details.
Let me check the snapshot-related functionality to ensure complete verification.
Implementation correctly supports snapshot management and context retrieval
The verification confirms that
ContextOfSnapshotis properly implemented and used as intended:
- Correctly integrates with
Snapshot()andRevertToSnapshot()methods- Used in precompiled contracts to manage gas metering and state changes
- Properly handles the initial context case (i == -1)
- Maintains consistency in snapshot management across the codebase
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for calls to ContextOfSnapshot to understand usage patterns rg "ContextOfSnapshot" --type goLength of output: 847
Script:
#!/bin/bash # Check the implementation in contract files to understand how ContextOfSnapshot is used rg "ContextOfSnapshot" -A 5 -B 5 x/evm/precompiles/cosmos/contract.go x/evm/precompiles/erc20_registry/contract.goLength of output: 1924
Script:
#!/bin/bash # Check Snapshot implementation and related methods rg "func.*Snapshot" -A 3 x/evm/state/statedb.goLength of output: 347
x/evm/types/expected_keeper.go (2)
88-91: LGTMThe addition of the
StateDBinterface that extendsvm.StateDBand includesContextOfSnapshot(i int) sdk.Contextenhances state management and snapshot handling.
93-94: Ensure all references toWithContextare updated toWithStateDBSince the
WithContextinterface has been replaced byWithStateDB, please verify that all implementations and usages ofWithContexthave been updated accordingly to prevent any potential issues.Run the following script to find any remaining references to
WithContext:✅ Verification successful
No remaining references to
WithContextfound, all implementations useWithStateDBThe search results confirm that:
- There are no remaining references to
WithContextin the codebase- The
WithStateDBinterface is properly defined inx/evm/types/expected_keeper.go- All implementations (CosmosPrecompile and ERC20RegistryPrecompile) correctly implement the
WithStateDBinterface- All usages in tests and the precompiles mapper use the new
WithStateDBmethodThe migration from
WithContexttoWithStateDBappears to be complete and consistent throughout the codebase.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for any remaining usages of the old `WithContext` interface. # Test: Find occurrences of `WithContext`. Expect: No matches found. rg 'WithContext' --type go -A 5Length of output: 31
Script:
#!/bin/bash # Let's try a more comprehensive search approach: # 1. Search for any WithContext interface or method definitions ast-grep --pattern 'interface WithContext' ast-grep --pattern 'func ($_ $_) WithContext' # 2. Search for any WithContext method calls ast-grep --pattern '$_.WithContext($_)' # 3. Search for any WithStateDB interface or method definitions to verify the new pattern ast-grep --pattern 'interface WithStateDB' ast-grep --pattern 'func ($_ $_) WithStateDB' # 4. Search for any WithStateDB method calls ast-grep --pattern '$_.WithStateDB($_)' # 5. Let's also do a case-insensitive grep to catch any variations rg -i "withcontext" --type goLength of output: 299
Script:
#!/bin/bash # Let's try a different approach using ripgrep with more context and file type filtering # 1. Search for any references to WithContext in Go files, case-insensitive with 3 lines of context rg -i "withcontext" --type go -C 3 # 2. Search for the new WithStateDB pattern to understand the changes rg -i "withstatedb" --type go -C 3 # 3. Look specifically in the x/evm directory where the change was made rg -i "withcontext|withstatedb" --type go -C 3 "x/evm/" # 4. Search for interface definitions containing either pattern rg "type.*interface.*{" --type go -A 5Length of output: 15182
x/evm/precompiles/erc20_registry/contract.go (2)
56-57: Confirm gas usage assignment duringOutOfGaserrorIn the recovery block, upon catching an
ErrorOutOfGas, you setusedGas = suppliedGas. Ensure this reflects the intended behavior, accurately representing gas consumption when an out-of-gas error occurs. Previously, gas usage might have been calculated differently, and it's important that this assignment aligns with the overall gas accounting strategy.
64-66: Good practice: Reverting state changes on errorReverting to the snapshot when an error occurs ensures that any state changes made during execution are rolled back, maintaining consistency and preventing unintended side effects.
x/evm/precompiles/cosmos/contract.go (3)
27-27: LGTM!The interface assertion correctly ensures that
CosmosPrecompileimplements thetypes.WithStateDBinterface.
32-34: InitializestateDBfield appropriatelyThe addition of the
stateDBfield to theCosmosPrecompilestruct is appropriate. Ensure thatstateDBis properly initialized before it's used to avoidnilpointer dereferences.
106-108: Confirm state rollback works as intended on errorsThe
deferfunction rolls back to the snapshot if an error occurs by callinge.stateDB.RevertToSnapshot(snapshot). Ensure that this rollback mechanism effectively reverts any state changes made during execution whenerr != nil.
| m := make(map[common.Address]vm.PrecompiledContract) | ||
| for _, p := range ps { | ||
| m[p.addr] = p.contract.(types.WithContext).WithContext(ctx) | ||
| m[p.addr] = p.contract.(types.WithStateDB).WithStateDB(stateDB) |
There was a problem hiding this comment.
Add type assertion safety check.
The unchecked type assertion p.contract.(types.WithStateDB) could cause a panic if a precompile doesn't implement the WithStateDB interface.
Consider adding a safety check:
- m[p.addr] = p.contract.(types.WithStateDB).WithStateDB(stateDB)
+ if withState, ok := p.contract.(types.WithStateDB); ok {
+ m[p.addr] = withState.WithStateDB(stateDB)
+ } else {
+ panic(fmt.Sprintf("precompile at %s does not implement WithStateDB interface", p.addr))
+ }📝 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.
| m[p.addr] = p.contract.(types.WithStateDB).WithStateDB(stateDB) | |
| if withState, ok := p.contract.(types.WithStateDB); ok { | |
| m[p.addr] = withState.WithStateDB(stateDB) | |
| } else { | |
| panic(fmt.Sprintf("precompile at %s does not implement WithStateDB interface", p.addr)) | |
| } |
|
|
||
| // GetCodeSize implements types.StateDB. | ||
| func (m *MockStateDB) GetCodeSize(common.Address) int { | ||
| panic("unimplemented") | ||
| } | ||
|
|
||
| // GetCommittedState implements types.StateDB. | ||
| func (m *MockStateDB) GetCommittedState(common.Address, common.Hash) common.Hash { | ||
| panic("unimplemented") | ||
| } | ||
|
|
||
| // GetNonce implements types.StateDB. | ||
| func (m *MockStateDB) GetNonce(common.Address) uint64 { | ||
| panic("unimplemented") | ||
| } | ||
|
|
||
| // GetRefund implements types.StateDB. | ||
| func (m *MockStateDB) GetRefund() uint64 { | ||
| panic("unimplemented") | ||
| } | ||
|
|
||
| // GetState implements types.StateDB. | ||
| func (m *MockStateDB) GetState(common.Address, common.Hash) common.Hash { | ||
| panic("unimplemented") | ||
| } | ||
|
|
||
| // GetStorageRoot implements types.StateDB. | ||
| func (m *MockStateDB) GetStorageRoot(addr common.Address) common.Hash { | ||
| panic("unimplemented") | ||
| } | ||
|
|
||
| // GetTransientState implements types.StateDB. | ||
| func (m *MockStateDB) GetTransientState(addr common.Address, key common.Hash) common.Hash { | ||
| panic("unimplemented") | ||
| } | ||
|
|
||
| // HasSelfDestructed implements types.StateDB. | ||
| func (m *MockStateDB) HasSelfDestructed(common.Address) bool { | ||
| panic("unimplemented") | ||
| } | ||
|
|
||
| // PointCache implements types.StateDB. | ||
| func (m *MockStateDB) PointCache() *utils.PointCache { | ||
| panic("unimplemented") | ||
| } | ||
|
|
||
| // Prepare implements types.StateDB. | ||
| func (m *MockStateDB) Prepare(rules params.Rules, sender common.Address, coinbase common.Address, dest *common.Address, precompiles []common.Address, txAccesses types.AccessList) { | ||
| panic("unimplemented") | ||
| } | ||
|
|
||
| // SelfDestruct implements types.StateDB. | ||
| func (m *MockStateDB) SelfDestruct(common.Address) { | ||
| panic("unimplemented") | ||
| } | ||
|
|
||
| // Selfdestruct6780 implements types.StateDB. | ||
| func (m *MockStateDB) Selfdestruct6780(common.Address) { | ||
| panic("unimplemented") | ||
| } | ||
|
|
||
| // SetCode implements types.StateDB. | ||
| func (m *MockStateDB) SetCode(common.Address, []byte) { | ||
| panic("unimplemented") | ||
| } | ||
|
|
||
| // SetNonce implements types.StateDB. | ||
| func (m *MockStateDB) SetNonce(common.Address, uint64) { | ||
| panic("unimplemented") | ||
| } | ||
|
|
||
| // SetState implements types.StateDB. | ||
| func (m *MockStateDB) SetState(common.Address, common.Hash, common.Hash) { | ||
| panic("unimplemented") | ||
| } | ||
|
|
||
| // SetTransientState implements types.StateDB. | ||
| func (m *MockStateDB) SetTransientState(addr common.Address, key common.Hash, value common.Hash) { | ||
| panic("unimplemented") | ||
| } | ||
|
|
||
| // SlotInAccessList implements types.StateDB. | ||
| func (m *MockStateDB) SlotInAccessList(addr common.Address, slot common.Hash) (addressOk bool, slotOk bool) { | ||
| panic("unimplemented") | ||
| } | ||
|
|
||
| // SubBalance implements types.StateDB. | ||
| func (m *MockStateDB) SubBalance(common.Address, *uint256.Int, tracing.BalanceChangeReason) { | ||
| panic("unimplemented") | ||
| } | ||
|
|
||
| // SubRefund implements types.StateDB. | ||
| func (m *MockStateDB) SubRefund(uint64) { | ||
| panic("unimplemented") | ||
| } | ||
|
|
||
| // Witness implements types.StateDB. | ||
| func (m *MockStateDB) Witness() *stateless.Witness { | ||
| panic("unimplemented") | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider enhancing mock implementation.
- Make panic messages more descriptive by including the method name:
- panic("unimplemented")
+ panic("MockStateDB.AddAddressToAccessList: unimplemented")- Consider implementing critical methods that might be needed for basic testing scenarios:
GetBalanceSetState/GetStateAddBalance/SubBalance
This would make the mock more useful for common test cases while keeping other methods unimplemented.
Committable suggestion was skipped due to low confidence.
| // RevertToSnapshot implements types.StateDB. | ||
| func (m *MockStateDB) RevertToSnapshot(i int) { | ||
| if i == -1 { | ||
| m.ctx = m.initialCtx | ||
| m.snaps = m.snaps[:0] | ||
| return | ||
| } | ||
|
|
||
| // revert to the snapshot with the given id | ||
| snap := m.snaps[i] | ||
| m.ctx = snap.Context() | ||
|
|
||
| // clear the snapshots after the given id | ||
| m.snaps = m.snaps[:i] | ||
| } | ||
|
|
||
| // ContextOfSnapshot implements types.StateDB. | ||
| func (m *MockStateDB) ContextOfSnapshot(i int) sdk.Context { | ||
| if i == -1 { | ||
| return m.initialCtx | ||
| } | ||
|
|
||
| return m.snaps[i].Context() | ||
| } |
There was a problem hiding this comment.
Add bounds checking in RevertToSnapshot and ContextOfSnapshot.
Both methods access m.snaps[i] directly without verifying if the index is valid. This could panic if an invalid snapshot ID is provided.
Add validation:
func (m *MockStateDB) RevertToSnapshot(i int) {
if i == -1 {
m.ctx = m.initialCtx
m.snaps = m.snaps[:0]
return
}
+ if i < 0 || i >= len(m.snaps) {
+ panic("invalid snapshot index")
+ } func (m *MockStateDB) ContextOfSnapshot(i int) sdk.Context {
if i == -1 {
return m.initialCtx
}
+ if i < 0 || i >= len(m.snaps) {
+ panic("invalid snapshot index")
+ }📝 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.
| // RevertToSnapshot implements types.StateDB. | |
| func (m *MockStateDB) RevertToSnapshot(i int) { | |
| if i == -1 { | |
| m.ctx = m.initialCtx | |
| m.snaps = m.snaps[:0] | |
| return | |
| } | |
| // revert to the snapshot with the given id | |
| snap := m.snaps[i] | |
| m.ctx = snap.Context() | |
| // clear the snapshots after the given id | |
| m.snaps = m.snaps[:i] | |
| } | |
| // ContextOfSnapshot implements types.StateDB. | |
| func (m *MockStateDB) ContextOfSnapshot(i int) sdk.Context { | |
| if i == -1 { | |
| return m.initialCtx | |
| } | |
| return m.snaps[i].Context() | |
| } | |
| // RevertToSnapshot implements types.StateDB. | |
| func (m *MockStateDB) RevertToSnapshot(i int) { | |
| if i == -1 { | |
| m.ctx = m.initialCtx | |
| m.snaps = m.snaps[:0] | |
| return | |
| } | |
| if i < 0 || i >= len(m.snaps) { | |
| panic("invalid snapshot index") | |
| } | |
| // revert to the snapshot with the given id | |
| snap := m.snaps[i] | |
| m.ctx = snap.Context() | |
| // clear the snapshots after the given id | |
| m.snaps = m.snaps[:i] | |
| } | |
| // ContextOfSnapshot implements types.StateDB. | |
| func (m *MockStateDB) ContextOfSnapshot(i int) sdk.Context { | |
| if i == -1 { | |
| return m.initialCtx | |
| } | |
| if i < 0 || i >= len(m.snaps) { | |
| panic("invalid snapshot index") | |
| } | |
| return m.snaps[i].Context() | |
| } |
|
|
||
| // GetCodeSize implements types.StateDB. | ||
| func (m *MockStateDB) GetCodeSize(common.Address) int { | ||
| panic("unimplemented") | ||
| } | ||
|
|
||
| // GetCommittedState implements types.StateDB. | ||
| func (m *MockStateDB) GetCommittedState(common.Address, common.Hash) common.Hash { | ||
| panic("unimplemented") | ||
| } | ||
|
|
||
| // GetNonce implements types.StateDB. | ||
| func (m *MockStateDB) GetNonce(common.Address) uint64 { | ||
| panic("unimplemented") | ||
| } | ||
|
|
||
| // GetRefund implements types.StateDB. | ||
| func (m *MockStateDB) GetRefund() uint64 { | ||
| panic("unimplemented") | ||
| } | ||
|
|
||
| // GetState implements types.StateDB. | ||
| func (m *MockStateDB) GetState(common.Address, common.Hash) common.Hash { | ||
| panic("unimplemented") | ||
| } | ||
|
|
||
| // GetStorageRoot implements types.StateDB. | ||
| func (m *MockStateDB) GetStorageRoot(addr common.Address) common.Hash { | ||
| panic("unimplemented") | ||
| } | ||
|
|
||
| // GetTransientState implements types.StateDB. | ||
| func (m *MockStateDB) GetTransientState(addr common.Address, key common.Hash) common.Hash { | ||
| panic("unimplemented") | ||
| } | ||
|
|
||
| // HasSelfDestructed implements types.StateDB. | ||
| func (m *MockStateDB) HasSelfDestructed(common.Address) bool { | ||
| panic("unimplemented") | ||
| } | ||
|
|
||
| // PointCache implements types.StateDB. | ||
| func (m *MockStateDB) PointCache() *utils.PointCache { | ||
| panic("unimplemented") | ||
| } | ||
|
|
||
| // Prepare implements types.StateDB. | ||
| func (m *MockStateDB) Prepare(rules params.Rules, sender common.Address, coinbase common.Address, dest *common.Address, precompiles []common.Address, txAccesses types.AccessList) { | ||
| panic("unimplemented") | ||
| } | ||
|
|
||
| // SelfDestruct implements types.StateDB. | ||
| func (m *MockStateDB) SelfDestruct(common.Address) { | ||
| panic("unimplemented") | ||
| } | ||
|
|
||
| // Selfdestruct6780 implements types.StateDB. | ||
| func (m *MockStateDB) Selfdestruct6780(common.Address) { | ||
| panic("unimplemented") | ||
| } | ||
|
|
||
| // SetCode implements types.StateDB. | ||
| func (m *MockStateDB) SetCode(common.Address, []byte) { | ||
| panic("unimplemented") | ||
| } | ||
|
|
||
| // SetNonce implements types.StateDB. | ||
| func (m *MockStateDB) SetNonce(common.Address, uint64) { | ||
| panic("unimplemented") | ||
| } | ||
|
|
||
| // SetState implements types.StateDB. | ||
| func (m *MockStateDB) SetState(common.Address, common.Hash, common.Hash) { | ||
| panic("unimplemented") | ||
| } | ||
|
|
||
| // SetTransientState implements types.StateDB. | ||
| func (m *MockStateDB) SetTransientState(addr common.Address, key common.Hash, value common.Hash) { | ||
| panic("unimplemented") | ||
| } | ||
|
|
||
| // SlotInAccessList implements types.StateDB. | ||
| func (m *MockStateDB) SlotInAccessList(addr common.Address, slot common.Hash) (addressOk bool, slotOk bool) { | ||
| panic("unimplemented") | ||
| } | ||
|
|
||
| // SubBalance implements types.StateDB. | ||
| func (m *MockStateDB) SubBalance(common.Address, *uint256.Int, tracing.BalanceChangeReason) { | ||
| panic("unimplemented") | ||
| } | ||
|
|
||
| // SubRefund implements types.StateDB. | ||
| func (m *MockStateDB) SubRefund(uint64) { | ||
| panic("unimplemented") | ||
| } | ||
|
|
||
| // Witness implements types.StateDB. | ||
| func (m *MockStateDB) Witness() *stateless.Witness { | ||
| panic("unimplemented") | ||
| } |
There was a problem hiding this comment.
💡 Codebase verification
Snapshot functionality needs test coverage
The mock implementation provides the necessary snapshot infrastructure, but there are no actual test cases exercising this functionality. While the MockStateDB correctly implements Snapshot(), RevertToSnapshot(), and ContextOfSnapshot(), none of the existing tests in the precompiles packages utilize these methods.
Action items:
- Add test cases that create and revert snapshots to verify the state management functionality
- Include tests that validate state consistency across multiple snapshot operations
- Test edge cases like reverting to initial state (snapshot -1)
🔗 Analysis chain
Verify snapshot functionality in integration tests.
The mock implementation provides good support for testing snapshot-based state management, which aligns with the PR's objective of implementing revert functionality. Consider adding integration tests that:
- Create multiple snapshots
- Modify state between snapshots
- Revert to various snapshot points
- Verify state consistency after reverts
This will help ensure the robustness of the snapshot-revert feature.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for existing integration tests that use MockStateDB
rg -l "NewMockStateDB" --type go | grep "_test.go"
Length of output: 228
Script:
#!/bin/bash
# Check how MockStateDB is used in tests, particularly focusing on snapshot functionality
rg "NewMockStateDB|Snapshot|RevertToSnapshot" -A 5 -B 2 x/evm/precompiles/erc20_registry/contract_test.go x/evm/precompiles/erc20_registry/common_test.go x/evm/precompiles/cosmos/contract_test.go x/evm/precompiles/cosmos/common_test.go
Length of output: 11095
Script:
#!/bin/bash
# Let's check if there are any actual test cases that use the snapshot functionality
rg "Snapshot\(\)|RevertToSnapshot\(" --type go -B 5 -A 10 x/evm/precompiles/erc20_registry/contract_test.go x/evm/precompiles/cosmos/contract_test.go
Length of output: 151
| func (k MockAccountKeeper) GetAccount(ctx context.Context, addr sdk.AccAddress) sdk.AccountI { | ||
| str, _ := k.ac.BytesToString(addr.Bytes()) | ||
| return k.accounts[str] | ||
| } |
There was a problem hiding this comment.
Handle address conversion errors
The BytesToString conversion errors should be handled appropriately.
func (k MockAccountKeeper) GetAccount(ctx context.Context, addr sdk.AccAddress) sdk.AccountI {
- str, _ := k.ac.BytesToString(addr.Bytes())
+ str, err := k.ac.BytesToString(addr.Bytes())
+ if err != nil {
+ panic(fmt.Sprintf("failed to convert address bytes to string: %v", err))
+ }
return k.accounts[str]
}Also applies to: 269-273
| snapshot := e.stateDB.Snapshot() | ||
| ctx := e.stateDB.ContextOfSnapshot(snapshot).WithGasMeter(storetypes.NewGasMeter(suppliedGas)) |
There was a problem hiding this comment.
Ensure stateDB is initialized before usage
The stateDB is used to create a snapshot and context, but if stateDB is not properly initialized (due to the issue in WithStateDB), this could lead to a nil pointer dereference at runtime. After fixing the receiver in WithStateDB, please verify that stateDB is correctly set before it's used in ExtendedRun.
| func (e ERC20RegistryPrecompile) WithStateDB(stateDB types.StateDB) vm.PrecompiledContract { | ||
| e.stateDB = stateDB |
There was a problem hiding this comment.
Fix the method receiver in WithStateDB to correctly set stateDB
The method WithStateDB currently uses a value receiver (e ERC20RegistryPrecompile), which means that any modifications to e.stateDB affect only a copy of the struct and do not persist after the method returns. To ensure that stateDB is properly set on the original struct, the receiver should be a pointer.
Apply this diff to fix the receiver type:
-func (e ERC20RegistryPrecompile) WithStateDB(stateDB types.StateDB) vm.PrecompiledContract {
+func (e *ERC20RegistryPrecompile) WithStateDB(stateDB types.StateDB) vm.PrecompiledContract {📝 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.
| func (e ERC20RegistryPrecompile) WithStateDB(stateDB types.StateDB) vm.PrecompiledContract { | |
| e.stateDB = stateDB | |
| func (e *ERC20RegistryPrecompile) WithStateDB(stateDB types.StateDB) vm.PrecompiledContract { | |
| e.stateDB = stateDB |
| snapshot := e.stateDB.Snapshot() | ||
| ctx := e.stateDB.ContextOfSnapshot(snapshot).WithGasMeter(storetypes.NewGasMeter(suppliedGas)) | ||
|
|
There was a problem hiding this comment.
Check for nil stateDB to prevent potential panics
In the ExtendedRun method, e.stateDB is used without checking if it's nil. If stateDB is nil, calling e.stateDB.Snapshot() will result in a nil pointer dereference.
Consider adding a check at the beginning of the method:
if e.stateDB == nil {
return nil, 0, errors.New("stateDB is not initialized")
}| func (e CosmosPrecompile) WithStateDB(stateDB types.StateDB) vm.PrecompiledContract { | ||
| e.stateDB = stateDB | ||
| return e |
There was a problem hiding this comment.
Use a pointer receiver for WithStateDB method to update the struct
The WithStateDB method has a value receiver (e CosmosPrecompile), which means any modifications to e will not affect the original instance outside the method. To correctly update the stateDB field of the original CosmosPrecompile instance, change the receiver to a pointer.
Apply this diff to fix the issue:
-func (e CosmosPrecompile) WithStateDB(stateDB types.StateDB) vm.PrecompiledContract {
+func (e *CosmosPrecompile) WithStateDB(stateDB types.StateDB) vm.PrecompiledContract {
e.stateDB = stateDB
return e
}📝 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.
| func (e CosmosPrecompile) WithStateDB(stateDB types.StateDB) vm.PrecompiledContract { | |
| e.stateDB = stateDB | |
| return e | |
| func (e *CosmosPrecompile) WithStateDB(stateDB types.StateDB) vm.PrecompiledContract { | |
| e.stateDB = stateDB | |
| return e |
| stateDB := NewMockStateDB(ctx) | ||
| cosmosPrecompile = cosmosPrecompile.WithStateDB(stateDB).(precompiles.CosmosPrecompile) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor repeated stateDB initialization into a helper function to improve maintainability.
The initialization of stateDB and updating cosmosPrecompile is duplicated across multiple test functions. To reduce code duplication and enhance code readability, consider creating a helper function or updating the setup() function to include stateDB initialization.
Here's a suggested refactor:
func setup() (sdk.Context, codec.Codec, address.Codec, types.AccountKeeper, types.BankKeeper, precompiles.CosmosPrecompile) {
// existing setup code...
ac := codecaddress.NewBech32Codec("init")
+ cosmosPrecompile, err := precompiles.NewCosmosPrecompile(cdc, ac, ak, bk, nil, nil, nil)
+ if err != nil {
+ panic(err)
+ }
+
+ stateDB := NewMockStateDB(ctx)
+ cosmosPrecompile = cosmosPrecompile.WithStateDB(stateDB).(precompiles.CosmosPrecompile)
+
return ctx, cdc, ac, ak, bk, cosmosPrecompile
}Then, update your test functions to receive the cosmosPrecompile instance:
-func Test_CosmosPrecompile_IsBlockedAddress(t *testing.T) {
- ctx, cdc, ac, ak, bk := setup()
+func Test_CosmosPrecompile_IsBlockedAddress(t *testing.T) {
+ ctx, cdc, ac, ak, bk, cosmosPrecompile := setup()Also applies to: 115-116, 161-162, 192-193, 223-224, 323-324, 370-371, 412-413
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (4)
x/evm/contracts/counter/Counter.sol (1)
40-49: Add NatSpec documentation for the new function.The new
execute_cosmosfunction lacks documentation explaining its purpose, parameters, and potential revert conditions. This documentation is crucial for developers integrating with the contract.Apply this diff to add documentation:
+ /// @notice Executes a cosmos message with optional revert functionality + /// @param exec_msg The cosmos message to execute + /// @param call_revert If true, the function will revert after execution + /// @dev This function interacts with COSMOS_CONTRACT and may revert with "revert" message function execute_cosmos( string memory exec_msg, bool call_revert ) external {x/evm/keeper/context_test.go (3)
260-262: Consider using a named constant for the token amount.The magic number
1000000000could be made more readable by declaring it as a constant or variable with a descriptive name liketestTokenAmount.- amount := math.NewInt(1000000000) + const testTokenAmount = 1000000000 + amount := math.NewInt(testTokenAmount)
276-277: Consider using more specific error assertion.Instead of using
ErrorContains, consider using a more specific assertion to ensure the exact error type is returned.- require.ErrorContains(t, err, types.ErrReverted.Error()) + require.ErrorIs(t, err, types.ErrReverted)
242-300: Consider adding more test cases and documentation.While the test covers basic success and failure scenarios, consider:
- Adding a test case description comment explaining the test's purpose
- Testing with different token amounts (e.g., zero, very large amounts)
- Testing with invalid message formats
- Testing with unauthorized addresses
Example test description:
+// Test_RevertAfterExecuteCosmos verifies that the execute_cosmos function in the Counter contract +// correctly handles both successful and reverted Cosmos message executions, specifically testing +// MsgSend behavior and balance changes. func Test_RevertAfterExecuteCosmos(t *testing.T) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- x/evm/contracts/counter/Counter.go (2 hunks)
- x/evm/contracts/counter/Counter.sol (1 hunks)
- x/evm/keeper/context_test.go (2 hunks)
🔇 Additional comments (4)
x/evm/contracts/counter/Counter.sol (2)
40-49: Verify snapshot handling for reverts.Given the PR's objective to implement snapshot support for reverts, we should verify that state changes from
COSMOS_CONTRACT.execute_cosmos()are properly handled whencall_revertis true.Run the following script to check snapshot-related patterns:
#!/bin/bash # Description: Check snapshot handling patterns # Expected: Find snapshot-related code that handles state reversions # Search for snapshot-related code rg -l "snapshot|revert|rollback" # Search for test files that verify revert behavior fd -e sol -e go testConsider implementing a test that verifies:
- State changes from the cosmos execution
- Complete state reversion when
call_revertis true- Proper state persistence when
call_revertis false
44-44: Verify error handling for COSMOS_CONTRACT.execute_cosmos call.The external call to
COSMOS_CONTRACT.execute_cosmos()should be checked for potential failures.Run the following script to check error handling patterns in similar cosmos calls:
✅ Verification successful
Let me gather more specific information about error handling in the Counter contract and related Cosmos contract calls.
Based on the search results, I can now provide a final response since we have clear evidence of the error handling patterns in the Counter contract.
Error handling is already implemented appropriately
The
execute_cosmoscall is followed by error handling logic through thecall_revertparameter, which triggers a revert when needed. The code shows a pattern where failures can be explicitly handled:COSMOS_CONTRACT.execute_cosmos(exec_msg); if (call_revert) { revert("revert"); }This implementation allows for controlled error handling based on the execution context, which is sufficient for this use case.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check error handling patterns in cosmos contract calls # Expected: Find how other functions handle COSMOS_CONTRACT failures # Search for COSMOS_CONTRACT calls and their error handling rg -A 5 "COSMOS_CONTRACT\.execute_cosmos"Length of output: 2156
Script:
#!/bin/bash # Search for the Counter contract implementation and error handling patterns rg -A 10 "COSMOS_CONTRACT\.execute_cosmos" x/evm/contracts/counter/Counter.sol # Check for revert statements in Counter.sol rg "revert" x/evm/contracts/counter/Counter.sol # Look at the COSMOS_CONTRACT interface/implementation rg -A 5 "interface.*COSMOS_CONTRACT" x/evm/contracts/Length of output: 994
x/evm/keeper/context_test.go (1)
242-254: LGTM: Test setup and contract deployment are well-structured.The test setup follows consistent patterns with other tests in the file, including proper error handling and assertions.
x/evm/contracts/counter/Counter.go (1)
34-35: LGTM: ABI and bytecode updates for new execute_cosmos methodThe ABI and bytecode changes correctly reflect the addition of the new
execute_cosmosmethod with appropriate parameter types.
| _, _, err = input.EVMKeeper.EVMCall(ctx, caller, contractAddr, inputBz, nil, nil) | ||
| require.NoError(t, err, types.ErrReverted.Error()) |
There was a problem hiding this comment.
Fix incorrect error assertion and remove unnecessary parameter.
The error assertion appears to be incorrect. The types.ErrReverted.Error() parameter is unnecessary since we're expecting success.
- require.NoError(t, err, types.ErrReverted.Error())
+ require.NoError(t, 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.
| _, _, err = input.EVMKeeper.EVMCall(ctx, caller, contractAddr, inputBz, nil, nil) | |
| require.NoError(t, err, types.ErrReverted.Error()) | |
| _, _, err = input.EVMKeeper.EVMCall(ctx, caller, contractAddr, inputBz, nil, nil) | |
| require.NoError(t, err) |
| // ExecuteCosmos is a paid mutator transaction binding the contract method 0x24c68fce. | ||
| // | ||
| // Solidity: function execute_cosmos(string exec_msg, bool call_revert) returns() | ||
| func (_Counter *CounterTransactor) ExecuteCosmos(opts *bind.TransactOpts, exec_msg string, call_revert bool) (*types.Transaction, error) { | ||
| return _Counter.contract.Transact(opts, "execute_cosmos", exec_msg, call_revert) | ||
| } | ||
|
|
||
| // ExecuteCosmos is a paid mutator transaction binding the contract method 0x24c68fce. | ||
| // | ||
| // Solidity: function execute_cosmos(string exec_msg, bool call_revert) returns() | ||
| func (_Counter *CounterSession) ExecuteCosmos(exec_msg string, call_revert bool) (*types.Transaction, error) { | ||
| return _Counter.Contract.ExecuteCosmos(&_Counter.TransactOpts, exec_msg, call_revert) | ||
| } | ||
|
|
||
| // ExecuteCosmos is a paid mutator transaction binding the contract method 0x24c68fce. | ||
| // | ||
| // Solidity: function execute_cosmos(string exec_msg, bool call_revert) returns() | ||
| func (_Counter *CounterTransactorSession) ExecuteCosmos(exec_msg string, call_revert bool) (*types.Transaction, error) { | ||
| return _Counter.Contract.ExecuteCosmos(&_Counter.TransactOpts, exec_msg, call_revert) | ||
| } | ||
|
|
There was a problem hiding this comment.
💡 Codebase verification
ExecuteCosmos method bindings need test coverage
While the implementation is correct, there are no specific test cases for the Counter contract's ExecuteCosmos method. However, I found related test coverage:
x/evm/precompiles/cosmos/contract_test.go: ContainsTest_ExecuteCosmosx/evm/keeper/context_test.go: ContainsTest_RevertAfterExecuteCosmosx/evm/keeper/precompiles_test.go: ContainsTest_ExecuteCosmosMessage
These tests cover the underlying functionality but not the specific Counter contract bindings. Consider adding integration tests for the Counter contract's ExecuteCosmos method to ensure proper interaction with the precompile.
🔗 Analysis chain
LGTM: ExecuteCosmos method bindings are properly implemented
The new ExecuteCosmos bindings are correctly generated with proper documentation and error handling. The implementation follows the standard go-ethereum binding patterns.
Let's verify the integration test coverage for the new method:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for integration tests covering the new ExecuteCosmos method
# Test: Search for test cases using ExecuteCosmos
rg -l "ExecuteCosmos.*Test" --type go
Length of output: 39
Script:
#!/bin/bash
# Let's try a broader search for test files and ExecuteCosmos references
# Find all test files
echo "=== Test files ==="
fd "_test\.go$"
# Search for any ExecuteCosmos usage
echo -e "\n=== ExecuteCosmos references ==="
rg "ExecuteCosmos" --type go -C 2
# Check if there's a test directory specifically for counter
echo -e "\n=== Counter test files ==="
fd "counter.*test\.go$"
Length of output: 7060
Description
Closes: #XXXX
Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
!in the type prefix if API or client breaking changeReviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
I have...
Summary by CodeRabbit
Release Notes
New Features
StateDBinterface for enhanced state management in EVM operations.prepareSDKContextfunction for improved context preparation during EVM execution.MockStateDBimplementations for testing EVM state management.execute_cosmosfunction to theCountercontract for executing cosmos-related operations.Improvements
stateDBinstead ofcontext.CosmosPrecompileandERC20RegistryPrecompilecontracts.Tests
execute_cosmosfunction.Documentation
Snapshotstruct.