refactor: skip 'from' account verification during contract query #787
Conversation
WalkthroughThe pull request introduces significant modifications to various components within the codebase, primarily focused on updating method signatures to utilize the standard Go 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (4)
x/crosschain/keeper/bridge_call_in.go (1)
Line range hint
17-76: Consider breaking down BridgeCallHandler for better maintainabilityThe function handles multiple responsibilities including account validation, token deposits, EVM calls, and refund management. Consider splitting it into smaller, focused functions to improve maintainability and testability.
Suggested breakdown:
- validateBridgeCall(ctx, msg) - Handle initial validations
- processBridgeTokens(ctx, receiverAddr, msg) - Handle token deposits
- executeBridgeCall(ctx, msg, baseCoins) - Handle EVM execution
- handleRefund(ctx, msg, baseCoins) - Handle refund logic
x/evm/keeper/keeper.go (3)
Line range hint
62-100: Ensure the original gas meter is restored after execution incallEvm.In the
callEvmfunction, you replace the context's gas meter with an infinite gas meter but do not restore the original gas meter after execution. This could lead to unexpected behavior in subsequent code that relies on the gas meter.Apply this diff to restore the original gas meter:
func (k *Keeper) callEvm( ctx sdk.Context, from common.Address, contract *common.Address, value *big.Int, nonce uint64, data []byte, commit bool, ) (*types.MsgEthereumTxResponse, error) { gasMeter := ctx.GasMeter() ctx = ctx.WithGasMeter(storetypes.NewInfiniteGasMeter()) // ... existing code ... + // Restore the original gas meter. + ctx = ctx.WithGasMeter(gasMeter) return res, nil }
The deprecated
CallEVMWithoutGasmethod is still being used in test filesThe method is currently being used in several test files:
x/staking/precompile/delegation_test.gox/staking/precompile/delegation_rewards_test.gox/staking/precompile/contract_test.gox/crosschain/precompile/has_oracle_test.gox/crosschain/precompile/is_oracle_online_test.goBefore removing this deprecated method, you'll need to update these test files to use the recommended
callEvmmethod instead. The method should be removed only after updating all its usages.🔗 Analysis chain
Line range hint
46-61: Consider removing the deprecatedCallEVMWithoutGasmethod.The method
CallEVMWithoutGasis marked as deprecated with a TODO to remove it. If it's no longer used, you can safely remove it. If it's still in use, consider updating the callers to usecallEvminstead.Run the following script to check for usages of
CallEVMWithoutGasin the codebase:🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for usages of the deprecated method `CallEVMWithoutGas`. # Test: Find all occurrences where `CallEVMWithoutGas` is called. rg 'CallEVMWithoutGas\('Length of output: 481
Script:
#!/bin/bash # Let's search for any references to CallEVMWithoutGas in the entire codebase # including comments, function declarations, and method calls rg -i "callevmwithoutgas" # Also search for files that might be importing and using the keeper fd -e go | xargs rg "\".*x/evm/keeper\""Length of output: 1423
Update remaining CallEVMWithoutGas references to ExecuteEVM
The verification revealed that while
CallEVMhas been renamed toExecuteEVM, there are still several files using the deprecatedCallEVMWithoutGasmethod that need to be updated:
x/crosschain/precompile/is_oracle_online_test.go: UsingCallEVMWithoutGasx/crosschain/precompile/has_oracle_test.go: UsingCallEVMWithoutGasx/staking/precompile/contract_test.go: UsingCallEVMWithoutGasx/staking/precompile/delegation_test.go: UsingCallEVMWithoutGasx/staking/precompile/delegation_rewards_test.go: UsingCallEVMWithoutGasThe keeper.go file explicitly marks
CallEVMWithoutGasas deprecated with a comment "Deprecated: please use callEvm todo: remove this", confirming these calls should be migrated toExecuteEVM.🔗 Analysis chain
Line range hint
117-188: Verify that all calls toCallEVMare updated toExecuteEVM.The method
CallEVMhas been renamed toExecuteEVM, and the signature has been updated. Ensure that all references toCallEVMin the codebase have been updated to reflect this change.Run the following script to check for any remaining usages of
CallEVM:🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find all usages of the old `CallEVM` method. # Test: Search for `CallEVM` method calls. rg 'CallEVM\('Length of output: 481
Script:
#!/bin/bash # Let's check the implementation of CallEVM in test files to understand if they need updates rg -A 10 'func.*CallEVM' 'x/staking/testutil/staking_precompile.go' 'x/evm/testutil/evm.go' # Also search for any other potential references to CallEVM rg -l 'CallEVM' # Let's also check if there are any references to ExecuteEVM to verify the migration rg 'ExecuteEVM'Length of output: 2827
Script:
#!/bin/bash # Let's check the implementation of CallEVM in test files to see if they're properly using ExecuteEVM rg -B 5 -A 5 'CallEVM' 'x/staking/testutil/staking_precompile.go' 'x/staking/precompile/delegation_rewards_test.go' 'x/staking/precompile/delegation_test.go' 'x/staking/precompile/contract_test.go' 'x/crosschain/precompile/has_oracle_test.go' 'x/crosschain/precompile/is_oracle_online_test.go' # Let's also check the keeper.go file to see any remaining CallEVM references rg -B 5 -A 5 'CallEVM' 'x/evm/keeper/keeper.go'Length of output: 7372
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (10)
- contract/contract.go (2 hunks)
- x/crosschain/keeper/bridge_call_in.go (1 hunks)
- x/crosschain/mock/expected_keepers_mocks.go (1 hunks)
- x/crosschain/types/expected_keepers.go (1 hunks)
- x/evm/keeper/contract_code.go (4 hunks)
- x/evm/keeper/keeper.go (4 hunks)
- x/evm/keeper/msg_server.go (1 hunks)
- x/evm/testutil/evm.go (1 hunks)
- x/ibc/middleware/keeper/ibc_call.go (1 hunks)
- x/ibc/middleware/types/expected_keepers.go (1 hunks)
🔇 Additional comments (14)
x/ibc/middleware/types/expected_keepers.go (2)
12-12: Verify the handling of transaction commit behavior.The removal of the
commitparameter suggests that transaction commit behavior is now handled elsewhere in the codebase. Please ensure this is documented and properly handled in the implementation.Let's check for any commit-related changes:
#!/bin/bash # Look for commit-related changes in EVM transaction handling rg -A 5 "func.*ExecuteEVM|func.*HandleTx|func.*Commit"
12-12: Interface change looks good but needs implementation verification.The renaming from
CallEVMtoExecuteEVMis more precise and better reflects the method's purpose.Let's verify all implementations have been updated:
✅ Verification successful
All implementations have been properly updated to use ExecuteEVM
The verification shows that:
- The main implementation in
x/evm/keeper/keeper.gouses the newExecuteEVMmethod- All callers have been updated to use
ExecuteEVM:
x/ibc/middleware/keeper/ibc_call.gox/crosschain/keeper/bridge_call_in.gox/evm/testutil/evm.go- Mock implementations in
x/crosschain/mock/expected_keepers_mocks.goare also updated- No occurrences of the old
CallEVMmethod were found🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining CallEVM implementations that need updating rg "CallEVM" # Search for ExecuteEVM implementations to verify consistency rg "ExecuteEVM"Length of output: 4622
x/evm/keeper/msg_server.go (1)
27-31: LGTM! Verify nonce management impact.The addition of nonce management through
accountKeeper.GetSequencelooks good. The error handling is appropriate, and the changes align with the refactoring objective.Let's verify the module account usage and nonce management implementation:
✅ Verification successful
Let me gather more information about the callEvm usage and module account handling.
Based on the gathered information, I can now provide the final response:
Nonce management implementation is correct and consistent
The nonce management implementation in
CallContractis properly integrated with the EVM module's architecture:
- The module account's nonce is correctly retrieved using
accountKeeper.GetSequence- The nonce is properly passed to
callEvm, which is the standardized internal method for EVM interactions- Error handling for nonce retrieval is appropriate
- The implementation aligns with other EVM call patterns in the codebase
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify module account usage and nonce management # Test 1: Check if the module account is consistently used rg -A 3 "k.module.Bytes()" # Test 2: Check for other callEvm usages to ensure consistent nonce handling ast-grep --pattern 'callEvm($$$)'Length of output: 276
Script:
#!/bin/bash # Search for callEvm method definition and usages with different pattern ast-grep --pattern 'func $_ callEvm($$$)' # Search for any other methods using module account rg -A 3 "k.module" # Search for other EVM call patterns rg -A 3 "ExecuteEVM|CallEVM|callEvm"Length of output: 22849
x/crosschain/keeper/bridge_call_in.go (2)
Line range hint
78-116: Review security implications of cross-chain bridge callsThe function handles critical cross-chain token transfers. Please ensure:
- The gas limit is appropriate for all possible scenarios
- Token conversion maintains precision
- The callback mechanism cannot be exploited
#!/bin/bash # Check gas limit configuration rg "BridgeCallMaxGasLimit" --type go -A 3 # Verify token conversion safety checks ast-grep --pattern 'BaseCoinToEvm($$$)'
108-108: Verify ExecuteEVM implementation across dependent modulesThe change from
CallEVMtoExecuteEVMis part of a larger refactor. Let's verify all callers have been updated consistently.✅ Verification successful
Let me try another search to find any remaining CallEVM references and verify ExecuteEVM usage:
Change from CallEVM to ExecuteEVM is consistent across the codebase
The search results show that the migration from
CallEVMtoExecuteEVMhas been properly implemented. The only remainingCallEVMreferences are:
CallEVMWithoutGas: A different method used in test files- Test utilities in
x/staking/testutilThe
ExecuteEVMmethod is consistently used in production code paths:
x/crosschain/keeper/bridge_call_in.gox/ibc/middleware/keeper/ibc_call.go- Interface definitions in expected keepers
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining CallEVM references that might need updating rg "CallEVM" --type go # Search for ExecuteEVM usage to verify consistent parameter passing ast-grep --pattern 'ExecuteEVM($$$)'Length of output: 7701
Script:
#!/bin/bash # Search for CallEVM references rg "CallEVM" --type go -A 3 # Search for ExecuteEVM references rg "ExecuteEVM" --type go -A 3Length of output: 6649
x/crosschain/types/expected_keepers.go (2)
67-67: Verify commit parameter removal implications.The removal of the
commitparameter suggests a change in how transaction finality is handled. Please ensure:
- Transaction finality is properly managed elsewhere
- This change aligns with the EVM execution model
- All call sites are updated to handle any new finality guarantees
Let's check the call sites:
✅ Verification successful
Let me gather more information about the implementation of ExecuteEVM to understand how transaction finality is handled.
Let me check one more thing to understand how the
commitparameter was handled in the previous implementation.
The
commitparameter removal is safe and properly handled.The verification shows that:
- The
ExecuteEVMmethod is a renamed version ofCallEVMwhere thecommitparameter has been removed- The internal implementation still uses
callEvmwith thecommitparameter, defaulting totruefor transaction finality- All call sites have been properly updated to use the new interface
The change simplifies the API while maintaining the same transaction finality guarantees through the internal implementation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find ExecuteEVM call sites to verify proper handling # Find ExecuteEVM call sites echo "Checking ExecuteEVM usage patterns:" rg -A 3 "ExecuteEVM\("Length of output: 2881
Script:
#!/bin/bash # Check the implementation of ExecuteEVM in the EVM keeper echo "Checking ExecuteEVM implementation:" rg -A 20 "func \(k \*Keeper\) ExecuteEVM\(" x/evm/keeper/ # Look for any commit-related functionality in the EVM module echo -e "\nChecking for commit-related functionality:" rg -i "commit" x/evm/Length of output: 1763
Script:
#!/bin/bash # Check git history for the ExecuteEVM changes echo "Checking git history for ExecuteEVM changes:" git log -p --all | rg -A 10 -B 10 "func.*ExecuteEVM" # Check for callEvm usage echo -e "\nChecking callEvm usage:" rg -A 10 "callEvm\(" x/evm/keeper/Length of output: 5521
67-67: Breaking interface change: Verify all implementations.The renaming of
CallEVMtoExecuteEVMand removal of thecommitparameter represents a breaking change in the interface.Let's verify all implementations are updated:
✅ Verification successful
Let me gather more information to verify the implementations and changes.
Based on the comprehensive results, I can now provide the final response:
Breaking change has been properly implemented across the codebase
The renaming from
CallEVMtoExecuteEVMand removal of thecommitparameter has been consistently applied throughout the codebase:
- The interface change in
x/crosschain/types/expected_keepers.gois properly reflected in all implementations- The
commitparameter is now handled internally (always set totrueinExecuteEVM)- All callers have been updated to use the new method name and signature
- Test files and mocks have been appropriately updated
The change is part of a larger refactoring effort to standardize EVM execution, where the commit behavior is now consistently handled within the implementation rather than being configurable by callers.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find any remaining references to CallEVM that need updating # and verify ExecuteEVM implementations # Check for any remaining CallEVM references echo "Checking for CallEVM references that need updating:" rg "CallEVM" # Find ExecuteEVM implementations to verify they match new signature echo -e "\nChecking ExecuteEVM implementations:" ast-grep --pattern 'func $$$ExecuteEVM(ctx sdk.Context, from common.Address, contract *common.Address, value *big.Int, gasLimit uint64, data []byte) $$$'Length of output: 1630
Script:
#!/bin/bash # Let's check for ExecuteEVM implementations with broader pattern and context echo "Checking ExecuteEVM implementations with context:" rg "ExecuteEVM" -A 5 -B 5 # Check for any commit parameter usage in EVM-related functions echo -e "\nChecking for commit parameter usage in EVM functions:" rg "func.*EVM.*commit.*bool.*\)" -A 2 # Let's also check the git history for this change echo -e "\nChecking git history for this change:" git log -p | rg -A 5 -B 5 "CallEVM|ExecuteEVM"Length of output: 24623
x/evm/keeper/contract_code.go (4)
5-5: LGTM: Import aligns with context type changesThe addition of the "context" import is appropriate for the transition to using standard Go context.Context in method signatures.
83-83: LGTM: Proper nonce handling for contract deploymentThe addition of nonce handling ensures proper transaction ordering and correct contract address calculation.
114-114: LGTM: Context handling improvements for queriesThe changes appropriately:
- Use standard Go context.Context in the signature
- Unwrap the SDK context for EVM calls
- Use nonce=0 for read-only queries
Also applies to: 119-119
130-130: LGTM: Improved context and nonce handlingThe changes appropriately:
- Use standard Go context.Context in the signature
- Add proper nonce handling with error checking
- Unwrap the SDK context for EVM calls
Also applies to: 135-139
x/crosschain/mock/expected_keepers_mocks.go (1)
533-545: Verify context type consistency with PR objectives.The PR objectives mention updating method signatures to use
context.Context, but theExecuteEVMmethod is usingtypes.Context. This might need to be aligned with the stated objectives.contract/contract.go (2)
Line range hint
13-20: No changes in these sections.These sections contain contract address constants and helper functions that remain unchanged.
Also applies to: 164-251
Line range hint
54-162: No changes in these sections.The Contract structure and its associated methods remain unchanged.
| txResp, err := k.evmKeeper.ExecuteEVM(ctx, sender, | ||
| evmPacket.GetToAddress(), evmPacket.Value.BigInt(), uint64(limit), evmPacket.MustGetData()) |
There was a problem hiding this comment.
💡 Codebase verification
Change from CallEVM to ExecuteEVM is correct, but legacy method needs cleanup
The verification revealed that:
CallEVMWithoutGasis marked as deprecated with a comment "please use callEvm"- The method exists as legacy code that should be removed
- The change to
ExecuteEVMin the IBC middleware is correct and aligned with the refactoring
Recommendations:
- Create a follow-up issue to remove the deprecated
CallEVMWithoutGasmethod - Update test files to use
ExecuteEVMinstead of the deprecated method
🔗 Analysis chain
Method call updated from CallEVM to ExecuteEVM
The change aligns with the broader refactoring effort. Let's verify the consistency of this change across the codebase.
Remaining references to CallEVM need attention
The verification revealed inconsistencies in the codebase:
CallEVMWithoutGasis still being used in multiple test files and the EVM keeper- Both
ExecuteEVMandCallEVMvariants coexist, suggesting incomplete refactoring
Let's verify if these are intentionally different methods or oversight:
Let's check one more aspect to understand the complete picture:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that CallEVM is completely replaced with ExecuteEVM
# and there are no lingering references to the old method name.
echo "Checking for any remaining references to CallEVM..."
rg "CallEVM" --type go
echo "Verifying the new ExecuteEVM method signature in the EVM keeper interface..."
ast-grep --pattern 'interface {
$$$
ExecuteEVM($$$)
$$$
}'
Length of output: 2370
Script:
#!/bin/bash
# Check the implementation and signature of CallEVMWithoutGas
ast-grep --pattern 'func $_ CallEVMWithoutGas($_) $_$ {
$$$
}'
# Check if ExecuteEVM is called within CallEVMWithoutGas (might be a wrapper)
echo "Checking relationship between CallEVMWithoutGas and ExecuteEVM..."
rg -A 5 "CallEVMWithoutGas" x/evm/keeper/keeper.go
Length of output: 392
Script:
#!/bin/bash
# Get more context around CallEVMWithoutGas implementation
rg -B 2 -A 15 "func \(k \*Keeper\) CallEVMWithoutGas" x/evm/keeper/keeper.go
# Check if there are any direct ExecuteEVM calls in test files
echo "Checking ExecuteEVM usage in tests..."
rg "ExecuteEVM" --type go -g "*_test.go"
Length of output: 643
| tx, err := s.evmKeeper.ExecuteEVM(s.ctx, s.GetFrom(), &s.contractAddr, nil, gasLimit, data) | ||
| s.NoError(err) | ||
| return tx |
There was a problem hiding this comment.
💡 Codebase verification
Update required: CallEVM references need to be migrated to ExecuteEVM
Several files still use the deprecated CallEVM method that needs to be updated to ExecuteEVM:
x/staking/testutil/staking_precompile.go: Multiple instances ofCallEVMusagex/staking/precompile/delegation_test.go,delegation_rewards_test.go,contract_test.go: Using deprecatedCallEVMWithoutGasx/crosschain/precompile/has_oracle_test.go,is_oracle_online_test.go: Using deprecatedCallEVMWithoutGasx/evm/testutil/evm.go: Contains both oldCallEVMmethod and newExecuteEVMusagex/evm/keeper/keeper.go: Contains deprecatedCallEVMWithoutGasmethod
The change in the reviewed file is incomplete as it introduces inconsistency by using ExecuteEVM while keeping the old CallEVM method. All references should be updated to use the new ExecuteEVM method for consistency.
🔗 Analysis chain
LGTM! Verify test coverage.
The change from CallEVM to ExecuteEVM aligns with the broader refactoring effort mentioned in the PR summary.
Let's verify the test coverage and usage of this method:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any test files that might still be using the old CallEVM method name
# or might need updates due to this change
# Search for test files using EVMSuite
rg -t go "EVMSuite" --files-with-matches
# Search for any remaining references to CallEVM that might need updating
rg -t go "CallEVM" -C 3
Length of output: 5431
| @@ -110,12 +111,12 @@ func (k *Keeper) DeployUpgradableContract(ctx sdk.Context, from, logic common.Ad | |||
| } | |||
|
|
|||
| // QueryContract query contract with args and res | |||
| func (k *Keeper) QueryContract(ctx sdk.Context, from, contract common.Address, abi abi.ABI, method string, res interface{}, args ...interface{}) error { | |||
| func (k *Keeper) QueryContract(ctx context.Context, from, contract common.Address, abi abi.ABI, method string, res interface{}, args ...interface{}) error { | |||
There was a problem hiding this comment.
💡 Codebase verification
Inconsistent context type usage detected in the codebase
The codebase shows mixed usage of context types:
- The reviewed
QueryContractandApplyContractmethods usecontext.Context - Several other methods in the same package still use
sdk.Context:CreateContractWithCodeUpdateContractCodeDeployContractDeployUpgradableContractIsContract
These related contract operation methods should be updated to maintain consistency in context handling.
🔗 Analysis chain
Verify consistent context type usage across the codebase
Let's ensure all related methods have been updated to use context.Context consistently.
Also applies to: 130-130
Let me gather more information to verify the context usage more thoroughly.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining sdk.Context usage in related methods
# Test: Look for method signatures that might still be using sdk.Context
rg -A 1 "func.*QueryContract|func.*ApplyContract"
Length of output: 624
Script:
#!/bin/bash
# Search for any remaining sdk.Context usage in the x/evm directory
rg "sdk\.Context" x/evm/
# Also check for any method signatures containing Context
ast-grep --pattern 'func $_($_Context$_, $$$) $$$'
Length of output: 1816
| QueryContract(ctx context.Context, from, contract common.Address, abi abi.ABI, method string, res interface{}, args ...interface{}) error | ||
| ApplyContract(ctx context.Context, from, contract common.Address, value *big.Int, abi abi.ABI, method string, constructorData ...interface{}) (*evmtypes.MsgEthereumTxResponse, error) |
There was a problem hiding this comment.
💡 Codebase verification
Context type change requires attention in implementation
The implementation in x/evm/keeper/contract_code.go still uses sdk.UnwrapSDKContext(ctx) in the QueryContract method, which indicates a potential issue with the context type change. This suggests that while the interface has been updated to use context.Context, the implementation still relies on SDK context functionality.
x/evm/keeper/contract_code.go: TheQueryContractimplementation needs to be reviewed as it's converting the context back to SDK context:sdk.UnwrapSDKContext(ctx)
🔗 Analysis chain
Verify the impact of context type change.
The change from sdk.Context to context.Context looks good and aligns with the PR objective. However, we should ensure that all implementations of this interface have been updated accordingly.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for all implementations of the Caller interface
ast-grep --pattern 'type $_ struct {
$$$
}
func ($_ $_) QueryContract(ctx $_, $$$) error {
$$$
}'
# Search for any remaining uses of sdk.Context with QueryContract
rg -A 2 'QueryContract.*sdk\.Context'
Length of output: 144
Script:
#!/bin/bash
# Let's try a different approach to find implementations and usages
# Find all files that might contain Caller interface implementations
rg -l "QueryContract|ApplyContract" --type go
# Find struct types that implement these methods
ast-grep --pattern 'type $name struct {
$$$
}'
# Look for function implementations with context parameter
rg -A 5 'func \(.*\) QueryContract\('
rg -A 5 'func \(.*\) ApplyContract\('
# Look for any remaining imports of sdk.Context
rg 'sdk\s*"github\.com/cosmos/cosmos-sdk/types"'
Length of output: 17379
Summary by CodeRabbit
Release Notes
New Features
ExecuteEVMto streamline EVM call processes.Bug Fixes
Documentation
Chores
CallEVMWithoutGasmethod for better clarity in EVM call management.