fix: return proper error message on precompile #103
Conversation
WalkthroughThe pull request includes extensive updates to the Changes
Possibly related PRs
Suggested reviewers
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 (
|
beer-1
changed the title
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #103 +/- ##
==========================================
- Coverage 28.31% 28.27% -0.04%
==========================================
Files 126 126
Lines 14016 14021 +5
==========================================
- Hits 3968 3965 -3
- Misses 9487 9494 +7
- Partials 561 562 +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: 2
🧹 Outside diff range and nitpick comments (10)
x/evm/keeper/precompiles.go (2)
13-14: Consider adding parameter validation and improving documentation.The function would benefit from:
- Documentation explaining the
rulesparameter and its impact- Input validation for nil parameters
// precompiles returns the precompiled contracts for the EVM. +// The rules parameter determines which precompiles are active based on the current +// chain configuration. The stateDB parameter is used to initialize stateful precompiles. func (k *Keeper) precompiles(rules params.Rules, stateDB types.StateDB) (vm.PrecompiledContracts, error) { + if stateDB == nil { + return nil, errors.New("stateDB cannot be nil") + }
42-45: Function naming and documentation improvements needed.
- The function name should be
PrecompileAddrsto follow Go naming conventions for exported functions- The documentation should explain what addresses are included in the returned slice
-// PrecompileAddrs returns the precompile addresses for the EVM. +// PrecompileAddrs returns all active precompile addresses for the EVM based on +// the provided rules, including both standard Ethereum precompiles and custom +// precompiles (Cosmos and ERC20Registry). -func (k *Keeper) precompileAddrs(rules params.Rules) []common.Address { +func (k *Keeper) PrecompileAddrs(rules params.Rules) []common.Address {x/evm/contracts/counter/Counter.sol (1)
46-46: LGTM! Consider adding a comment for clarity.The more descriptive revert message improves error clarity for testing scenarios.
Consider adding a comment to document that this revert is specifically for testing error handling:
+ // Revert with a test message to verify error handling in cosmos precompiles revert("revert reason dummy value for test");x/evm/precompiles/erc20_registry/contract_test.go (1)
Line range hint
75-165: Consider enhancing error handling test coverage.While the test cases cover basic error scenarios like out-of-gas and read-only violations, consider adding test cases for:
- Invalid input data handling
- State conflicts
- Revert scenarios with custom error messages
This would align with the PR's objective of improving error handling in cosmos precompiles.
Would you like me to help generate additional test cases?
x/evm/keeper/precompiles_test.go (2)
222-240: Consider adding a comment explaining the test scenario.The test setup is correct, but it would be helpful to add a comment explaining that we're intentionally using mismatched addresses to test the unauthorized access scenario.
// call execute_cosmos with revert +// Test scenario: Using mismatched addresses where the from_address in the message +// doesn't match the actual caller, which should trigger an unauthorized error inputBz, err := parsed.Pack("execute_cosmos",
246-248: Consider adding a descriptive comment for balance verification.The balance checks correctly verify that the state remains unchanged after a failed execution.
-// check balance +// Verify that balances remain unchanged after the failed execution: +// - Contract should retain its initial balance +// - Target address should have zero balance as the transfer failed require.Equal(t, amount, input.BankKeeper.GetBalance(ctx, sdk.AccAddress(contractAddr.Bytes()), denom).Amount)x/evm/precompiles/cosmos/contract.go (1)
108-114: Consider adding debug logging for errors.To aid in debugging and monitoring, consider adding debug-level logging when converting Cosmos errors to EVM errors.
if err != vm.ErrOutOfGas { + // Log the original error for debugging + e.stateDB.Logger().Debug("converting cosmos error to EVM error", "error", err) resBz = types.NewRevertReason(err) err = vm.ErrExecutionReverted }go.mod (1)
Go version 1.22.5 should be downgraded to 1.22.1
The specified Go version 1.22.5 in go.mod appears to be a non-existent version, as Go 1.22.1 is currently the latest stable release. While the codebase does use modern Go features (like generics), it doesn't require any features specific to unreleased Go versions.
- Update
go.modto usego 1.22.1instead of the non-existent version 1.22.5- The codebase is compatible with Go 1.22.x as evidenced by the generic function usage in
indexer/utils.go🔗 Analysis chain
Line range hint
3-3: Verify Go version compatibility.The specified Go version 1.22.5 appears to be beyond the current stable release (1.22.1 as of March 2024). This could lead to compatibility issues.
Let me gather more information about the Go version usage in the codebase.
Let me check the GitHub repository's release information to verify the Go version compatibility.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if any code uses features specific to Go 1.22.5 # Test: Search for potential usage of new Go 1.22+ features rg -g '*.go' 'range over integer|range over func|clear\('Length of output: 159
Script:
#!/bin/bash # Check for go.mod files and their Go versions fd -e mod -x cat {} # Check for any build constraints or Go version requirements in Go files rg -g '*.go' '^//\s*\+build|^//\s*go:build' # Check GitHub workflow files for Go version specifications fd -e yml -e yaml -p '\.github/workflows' -x cat {}Length of output: 29187
Script:
#!/bin/bash # Check repository information and releases gh api repos/initia-labs/minievm --jq '.default_branch' gh api repos/initia-labs/minievm/releases/latest --jq '.tag_name' # Check if there are any Go files with version-specific syntax rg -g '*.go' 'func\s+\w+\s*\[' # Check for type parameters (Go 1.18+) rg -g '*.go' 'any\s*[{(]' # Check for 'any' type alias (Go 1.18+)Length of output: 263
x/evm/contracts/counter/Counter.go (1)
Line range hint
1-1024: Consider implementing contract upgrade mechanism.While the current implementation is solid, consider implementing a contract upgrade mechanism for future updates. This could be achieved through:
- Using a proxy pattern
- Implementing a version control system
- Adding proper migration handlers
This would make future updates to error handling or other features more manageable.
x/evm/precompiles/erc20_registry/contract.go (1)
20-26: Consider documenting the rationale forpanicininitfunctionUsing
panicin theinitfunction can cause the application to crash during startup if the ABI retrieval fails. Adding a comment to explain why panicking here is acceptable can improve code readability and maintainability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (2)
go.sumis excluded by!**/*.sumintegration-tests/go.sumis excluded by!**/*.sum
📒 Files selected for processing (15)
go.mod(1 hunks)integration-tests/go.mod(1 hunks)x/evm/contracts/counter/Counter.go(1 hunks)x/evm/contracts/counter/Counter.sol(1 hunks)x/evm/keeper/context.go(4 hunks)x/evm/keeper/context_test.go(2 hunks)x/evm/keeper/keeper.go(0 hunks)x/evm/keeper/precompiles.go(2 hunks)x/evm/keeper/precompiles_test.go(2 hunks)x/evm/precompiles/cosmos/contract.go(4 hunks)x/evm/precompiles/cosmos/contract_test.go(12 hunks)x/evm/precompiles/erc20_registry/contract.go(2 hunks)x/evm/precompiles/erc20_registry/contract_test.go(1 hunks)x/evm/types/errors.go(2 hunks)x/evm/types/expected_keeper.go(0 hunks)
💤 Files with no reviewable changes (2)
- x/evm/keeper/keeper.go
- x/evm/types/expected_keeper.go
🔇 Additional comments (28)
x/evm/keeper/precompiles.go (2)
4-11: LGTM! Imports are well-organized.
The imports are properly structured with external dependencies followed by internal packages.
Line range hint 13-39: LGTM! Improved error handling and precompile initialization.
The implementation properly addresses the PR objectives by:
- Returning proper error messages with nil contracts
- Fixing the issue of precompiles being copied on every call by using
vm.ActivePrecompiledContracts - Properly initializing precompiles with stateDB
x/evm/precompiles/erc20_registry/contract_test.go (1)
75-76: LGTM! The initialization change aligns with the PR objectives.
The modification to pass stateDB as the first argument to NewERC20RegistryPrecompile is correct and addresses the issue of precompiles being copied on every separate call. This change helps prevent conflicts with the StateDB.
x/evm/keeper/precompiles_test.go (3)
9-9: LGTM: Import additions align with PR objectives.
The new imports for sdkerrors and vm packages are necessary for proper error handling validation in the new test case.
Also applies to: 14-15
208-220: LGTM: Test setup follows established patterns.
The test setup and contract deployment follow the same pattern as other test functions, with proper error handling and validation.
242-244: LGTM: Comprehensive error validation.
The test properly validates both the EVM execution revert and the unauthorized access error, ensuring that the precompile correctly propagates both error types. This aligns with the PR's objective of improving error handling.
x/evm/precompiles/cosmos/contract.go (4)
27-36: LGTM! Performance improvement by caching ABI.
Moving the ABI initialization to init() prevents redundant parsing on each precompile instantiation. This is a good optimization that aligns with the PR's objective of fixing precompile copying issues.
80-82: LGTM! Simplified error handling.
Direct error return is appropriate here since ExtendedRun will handle the proper error wrapping and conversion to EVM errors.
108-114: Verify error message conversion behavior.
The error handling improvements look good:
- Proper conversion of Cosmos errors to EVM errors
- State reversion ensures atomicity
- Error message conversion using
types.NewRevertReason
However, we should verify the behavior of types.NewRevertReason to ensure error messages are properly formatted for EVM clients.
#!/bin/bash
# Description: Check error message conversion implementation
# Look for NewRevertReason implementation and usage
echo "Checking NewRevertReason implementation:"
rg -A 10 "func.*NewRevertReason" --type go
echo "Checking other usages of NewRevertReason:"
rg "NewRevertReason" --type goLine range hint 54-74: Verify StateDB lifecycle management across precompiles.
The addition of stateDB parameter to the constructor looks good and helps prevent precompile copying issues. However, we should verify that the StateDB is properly managed across all precompile instances.
✅ Verification successful
Let me gather more information about the StateDB usage and management across precompiles.
Let me do one final check to understand the complete precompile initialization flow.
StateDB management in precompiles is properly implemented
The verification shows that StateDB is correctly managed across precompiles:
- StateDB is properly initialized in the Keeper's
precompilesfunction and passed to each precompile constructor - Both ERC20Registry and Cosmos precompiles follow the same pattern for StateDB handling
- StateDB operations (Snapshot, RevertToSnapshot, ContextOfSnapshot) are consistently used across precompiles for transaction safety
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for proper StateDB initialization and management across precompiles
# Look for other precompile constructors that should follow the same pattern
# Search for precompile constructors
echo "Checking precompile constructors:"
rg -A 5 "func New.*Precompile.*\(" --type go
# Search for StateDB usage in precompiles
echo "Checking StateDB usage:"
rg "stateDB.*types\.StateDB" --type go
Length of output: 1610
Script:
#!/bin/bash
# Search for all precompile struct definitions and their StateDB fields
echo "Checking precompile struct definitions:"
ast-grep --pattern 'type $_Precompile struct {
$$$
}'
# Search for StateDB initialization in precompile constructors
echo "Checking StateDB initialization in constructors:"
ast-grep --pattern 'func New$_Precompile($$$) (*$_, error) {
$$$
}'
# Search for any direct StateDB field access
echo "Checking StateDB field access:"
rg "\.stateDB\." --type go
Length of output: 1011
Script:
#!/bin/bash
# Check how precompiles are created and managed in the keeper
echo "Checking precompile initialization in keeper:"
rg -A 10 "func.*precompiles.*\(" --type go
# Check for any other StateDB-related operations
echo "Checking other StateDB operations:"
rg "Snapshot|RevertToSnapshot|ContextOfSnapshot" --type go
Length of output: 4459
integration-tests/go.mod (1)
274-274: LGTM! Verify the commit contains intended fixes.
The replace directive correctly points to the custom fork with the specific commit that addresses the error handling and precompile copying issues.
Let's verify the commit contains the intended fixes:
✅ Verification successful
✅ Commit changes align with the intended fixes
The commit modifies error handling in precompiled contracts to:
- Return output and remaining gas even when errors occur (except for out-of-gas)
- Add proper gas tracking through logger
- Fix the specific issue where errors were causing all outputs to be discarded
This change directly addresses the error handling improvements mentioned in the PR objectives.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the commit contains the intended fixes for error handling and precompile copying.
# Test: Check commit message and changes
gh api repos/initia-labs/evm/commits/3d312736d7fb \
--jq '.commit.message, .files[].filename' || \
echo "Note: Unable to verify commit. Please ensure the commit 3d312736d7fb exists in initia-labs/evm repository and contains the error handling and precompile copying fixes."
Length of output: 173
Script:
#!/bin/bash
# Let's examine the actual changes in the commit to verify the fixes
gh api repos/initia-labs/evm/commits/3d312736d7fb \
--jq '.files[].patch' || \
echo "Unable to fetch patch details"
Length of output: 653
go.mod (2)
Line range hint 274-282: LGTM! Security-focused replacements.
Good security practices observed:
- Replacing deprecated
dgrijalva/jwt-gowith maintainedgolang-jwt/jwt/v4 - Updating
gin-gonic/ginto fix GHSA-h395-qcrw-5vmq vulnerability - Using a specific version of
goleveldbto avoid simulation failures
291-291: LGTM! Custom EVM fork replacement.
The replacement of github.com/ethereum/go-ethereum with a custom fork aligns with the PR objectives to fix error handling in cosmos precompiles and address StateDB conflicts.
#!/bin/bash
# Description: Verify the custom EVM fork's commit hash matches the changes needed
# Test: Check if the specific commit contains relevant changes
gh api repos/initia-labs/evm/commits/3d312736d7fbf68af35df985b01536684bf6aafa \
--jq '.commit.message' || echo "Failed to fetch commit details"x/evm/keeper/context_test.go (2)
297-298: LGTM: Improved error assertion aligns with PR objectives
The updated error assertions properly verify both the EVM execution revert error and its accompanying reason message, which aligns with the PR's goal of returning proper error messages.
317-317: LGTM: Correct assertion for successful execution path
The NoError assertion is appropriate here as this test case verifies successful execution with valid parameters.
x/evm/precompiles/cosmos/contract_test.go (3)
67-68: LGTM: Consistent constructor changes across all test functions.
The addition of stateDB parameter to NewCosmosPrecompile constructor is consistently applied across all test functions, properly addressing the issue of precompiles being copied on every separate call.
Also applies to: 111-112, 155-156, 185-186, 215-216, 287-288, 373-374, 430-431, 470-471
279-281: LGTM: Improved error handling for unauthorized execution.
The error handling has been enhanced to:
- Return
vm.ErrExecutionRevertedfor proper EVM error handling - Include the original error message using
types.NewRevertError
This change ensures that EVM contracts receive proper revert messages while maintaining the error context.
352-354: LGTM: Consistent error handling in ExecuteCosmosWithOptions.
The error handling changes maintain consistency with Test_ExecuteCosmos, ensuring uniform error handling behavior across different execution modes.
x/evm/contracts/counter/Counter.go (4)
35-35: LGTM: Contract bytecode update.
The updated bytecode aligns with the PR objectives to improve error handling in cosmos precompiles.
Line range hint 428-429: LGTM: Contract methods properly handle errors.
The contract methods, particularly execute_cosmos, execute_cosmos_with_options, and callback, have been properly updated to handle errors and return appropriate messages, which aligns with the PR objectives.
Also applies to: 445-446, 461-462
Line range hint 642-644: LGTM: Event handlers properly implemented.
The event handlers for callback_received, increased, and recursive_called events are well-implemented with proper type handling and error management.
Also applies to: 739-741, 836-838
Line range hint 1-1024: Verify error handling implementation across the codebase.
Let's verify that the error handling changes are consistently implemented across related files.
x/evm/precompiles/erc20_registry/contract.go (1)
34-35: Verify that all calls to NewERC20RegistryPrecompile include the new stateDB parameter
The constructor NewERC20RegistryPrecompile now requires stateDB as the first argument. Please ensure that all instances where this function is called have been updated accordingly to prevent compilation errors or unexpected behavior.
Run the following script to verify:
✅ Verification successful
Based on the search results, I can generate the final response as we have found all instances where NewERC20RegistryPrecompile is called in the codebase. The results show that all three occurrences (one definition and two calls) properly include the stateDB parameter:
All calls to NewERC20RegistryPrecompile correctly include the stateDB parameter
The function is called in two locations and both already include the stateDB parameter:
x/evm/precompiles/erc20_registry/contract_test.go:registry, err := precompiles.NewERC20RegistryPrecompile(stateDB, k)x/evm/keeper/precompiles.go:erc20RegistryPrecompile, err := erc20registryprecompile.NewERC20RegistryPrecompile(stateDB, k.erc20StoresKeeper)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that all calls to `NewERC20RegistryPrecompile` include the new `stateDB` parameter.
# Test: Search for all instances of `NewERC20RegistryPrecompile` and display the surrounding lines.
rg 'NewERC20RegistryPrecompile\(' -A 2
Length of output: 865
x/evm/keeper/context.go (5)
196-196: Pass correct parameters to vm.NewEVMWithPrecompiles
The updated code correctly passes chainConfig, vmConfig, and precompiles to vm.NewEVMWithPrecompiles. This ensures that the EVM instance is initialized with the appropriate configuration and precompiled contracts, which is essential for accurate EVM behavior.
Also applies to: 198-198
243-243: Update Prepare method with rule-based precompile addresses
In EVMStaticCallWithTracer, the evm.StateDB.Prepare method now uses k.precompileAddrs(rules) to obtain precompile addresses based on the current EVM rules. This change ensures that state preparation accurately reflects the active precompiles for the given block context.
281-281: Consistent use of rule-based precompile addresses in EVMCallWithTracer
Similarly, EVMCallWithTracer now uses k.precompileAddrs(rules) in the Prepare method. This maintains consistency across different EVM call methods and ensures that precompile addresses are correctly determined based on the current chain rules.
375-375: Use rule-based precompile addresses in EVMCreateWithTracer
The Prepare method in EVMCreateWithTracer now includes k.precompileAddrs(rules). This ensures that contract creation operations consider the appropriate precompile addresses according to the current EVM rules, enhancing correctness in contract deployment.
184-189: Confirm appropriate use of default chain configuration
The code initializes chainConfig using types.DefaultChainConfig(ctx). Ensure that this default configuration aligns with the chain's actual settings, especially if there are custom chain configurations or upgrades that might affect EVM execution.
You can run the following script to check for any custom chain configurations and where they are used:
Description
Fix cosmos precompiles to return error message properly by returning vm.ErrExecutionRevert with output revert message.
Fix precompiles copied on every separate call to prevent StateDB conflict.
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
New Features
Bug Fixes
Documentation
Chores