chore(evm): simplified config by removing old eth forks #1911
Conversation
WalkthroughThe recent changes simplify Ethereum configuration handling by removing old Ethereum forks and related checks. The updates streamline the code by refactoring configuration access, removing deprecated fields, and ensuring consistent intrinsic gas calculations. These modifications enhance code readability and maintainability while ensuring the system remains up-to-date with current Ethereum standards. Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1911 +/- ##
==========================================
- Coverage 63.13% 62.91% -0.22%
==========================================
Files 245 245
Lines 16011 15900 -111
==========================================
- Hits 10108 10003 -105
+ Misses 5161 5157 -4
+ Partials 742 740 -2
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Outside diff range and nitpick comments (3)
x/evm/keeper/grpc_query.go (3)
Line range hint
1-1: Ensure the copyright year reflects the latest changes.
Line range hint
1-1: Add a nil check fortracerafter initialization to prevent potential runtime errors.+ if tracer == nil { + return nil, 0, grpcstatus.Error(grpccodes.Internal, "tracer initialization failed") + }
Line range hint
1-1: Enhance the error message for execution timeouts to provide more specific information.- tracer.Stop(errors.New("execution timeout")) + tracer.Stop(errors.New("execution timeout reached during transaction tracing"))
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (2)
x/evm/evm.pb.gois excluded by!**/*.pb.gox/evm/query.pb.gois excluded by!**/*.pb.go
Files selected for processing (17)
- CHANGELOG.md (1 hunks)
- app/evmante_eth.go (3 hunks)
- app/evmante_sigverify.go (1 hunks)
- eth/rpc/backend/call_tx.go (1 hunks)
- eth/rpc/backend/chain_info.go (1 hunks)
- eth/rpc/backend/chain_info_test.go (2 hunks)
- proto/eth/evm/v1/evm.proto (3 hunks)
- proto/eth/evm/v1/query.proto (1 hunks)
- x/evm/chain_config.go (2 hunks)
- x/evm/chain_config_test.go (1 hunks)
- x/evm/evmtest/smart_contract_test.go (2 hunks)
- x/evm/keeper/gas_fees.go (3 hunks)
- x/evm/keeper/grpc_query.go (1 hunks)
- x/evm/keeper/msg_server.go (2 hunks)
- x/evm/keeper/vm_config.go (1 hunks)
- x/evm/params.go (5 hunks)
- x/evm/statedb/statedb.go (1 hunks)
Files skipped from review due to trivial changes (2)
- CHANGELOG.md
- proto/eth/evm/v1/query.proto
Additional context used
GitHub Check: codecov/patch
app/evmante_sigverify.go
[warning] 37-37: app/evmante_sigverify.go#L37
Added line #L37 was not covered by testsx/evm/keeper/gas_fees.go
[warning] 146-146: x/evm/keeper/gas_fees.go#L146
Added line #L146 was not covered by tests
[warning] 150-151: x/evm/keeper/gas_fees.go#L150-L151
Added lines #L150 - L151 were not covered by testseth/rpc/backend/call_tx.go
[warning] 48-48: eth/rpc/backend/call_tx.go#L48
Added line #L48 was not covered by testsapp/evmante_eth.go
[warning] 273-273: app/evmante_eth.go#L273
Added line #L273 was not covered by tests
GitHub Check: break-check
proto/eth/evm/v1/evm.proto
[failure] 1-1:
Previously present message "ChainConfig" was deleted from file.
[failure] 10-10:
Previously present field "5" with name "chain_config" on message "Params" was deleted.
[failure] 114-114:
Previously present field "10" with name "overrides" on message "TraceConfig" was deleted.
Additional comments not posted (17)
x/evm/chain_config_test.go (1)
10-11: The refactoring of the test to use the simplifiedValidatefunction aligns well with the PR's objectives of simplifying the configuration.x/evm/evmtest/smart_contract_test.go (1)
39-39: The update to useEthereumConfigdirectly in the test setup is a good reflection of the simplified configuration access across the codebase.Also applies to: 44-44
x/evm/chain_config.go (1)
14-35: The simplification of theEthereumConfigfunction by removing conditional fork logic and setting all fork blocks to zero is a clear reflection of the PR's goal to simplify the Ethereum configuration.app/evmante_sigverify.go (1)
37-37: The update to useEthereumConfigdirectly in theAnteHandlemethod aligns with the PR's objectives. However, it's important to ensure this line is covered by tests to maintain code quality.Verification successful
The update to use
EthereumConfigdirectly in theAnteHandlemethod aligns with the PR's objectives. However, it's important to ensure this line is covered by tests to maintain code quality. The test filesx/devgas/v1/ante/ante_test.go,app/evmante_test.go, andapp/ante/commission_test.gocontain references toAnteHandle, indicating that the method is covered by tests.
x/devgas/v1/ante/ante_test.goapp/evmante_test.goapp/ante/commission_test.goScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify if the new line in `AnteHandle` is covered by tests. # Test: Search for test coverage. Expect: Coverage for line 37 in `app/evmante_sigverify.go`. rg --type go $'AnteHandle' --files-with-matches | xargs grep 'TestAnteHandle'Length of output: 74
Script:
#!/bin/bash # Description: Verify if the new line in `AnteHandle` is covered by tests. # Step 1: Find all test files that might cover `EthSigVerificationDecorator` or `AnteHandle`. test_files=$(rg --type go --files-with-matches 'EthSigVerificationDecorator|AnteHandle') # Step 2: Check the contents of these test files for references to `EthSigVerificationDecorator` or `AnteHandle`. for file in $test_files; do echo "Checking file: $file" grep -E 'EthSigVerificationDecorator|AnteHandle' "$file" doneLength of output: 9871
Script:
#!/bin/bash # Description: Verify if the new line in `AnteHandle` is covered by tests. # Step 1: Check specific test files for coverage of `EthSigVerificationDecorator`'s `AnteHandle` method. test_files=( "x/devgas/v1/ante/ante_test.go" "app/evmante_test.go" "app/ante/commission_test.go" ) # Step 2: Search for `EthSigVerificationDecorator` and `AnteHandle` references in these test files. for file in "${test_files[@]}"; do echo "Checking file: $file" grep -E 'EthSigVerificationDecorator|AnteHandle' "$file" doneLength of output: 1293
Tools
GitHub Check: codecov/patch
[warning] 37-37: app/evmante_sigverify.go#L37
Added line #L37 was not covered by testsx/evm/keeper/vm_config.go (1)
22-22: LGTM! The update to useevm.EthereumConfig(chainID)simplifies the configuration access as intended.proto/eth/evm/v1/evm.proto (2)
22-22: LGTM! The reservation of thechain_configfield is a clean way to handle deprecation while avoiding breaking changes.
136-136: LGTM! The reservation of thechain_configfield inTraceConfigis consistent with the changes inParams.x/evm/keeper/gas_fees.go (2)
28-28: LGTM! Removing the old fork checks simplifies the intrinsic gas calculation.
135-135: Ensure test coverage for the new logic inVerifyFee.Also applies to: 146-151
Verification successful
Test coverage for the new logic in
VerifyFeeis confirmed.
- The parameters
isCheckTxandintrinsicGasare referenced in the test files, indicating that the new logic is being tested.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify test coverage for the new logic in `VerifyFee`. # Test: Search for the function usage in test files. Expect: Coverage for the new logic. rg --type go $'VerifyFee' --files-with-matches | xargs cat | grep -E 'isCheckTx|intrinsicGas'Length of output: 339
x/evm/params.go (1)
Line range hint
1-1: LGTM! The removal ofChainConfigfrom theParamsstruct simplifies the parameter structure as intended.eth/rpc/backend/chain_info.go (1)
45-50: Simplified configuration access inChainConfigfunction.Verification successful
The new configuration access pattern using
evm.EthereumConfigis consistently used across the project.
x/evm/keeper/vm_config.gox/evm/evmtest/smart_contract_test.goeth/rpc/backend/call_tx.goeth/rpc/backend/chain_info.goapp/evmante_sigverify.goapp/evmante_eth.goScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the new configuration access pattern is used consistently across the project. # Test: Search for the old configuration access pattern. Expect: No occurrences. rg --type go $'params.ChainConfig.EthereumConfig'Length of output: 48
Script:
#!/bin/bash # Description: Verify that the new configuration access pattern is used consistently across the project. # Test: Search for the new configuration access pattern. Expect: Consistent usage. rg --type go $'evm.EthereumConfig'Length of output: 463
eth/rpc/backend/chain_info_test.go (1)
64-64: Updated test case to reflect new configuration access pattern.eth/rpc/backend/call_tx.go (1)
48-48: Use of simplified configuration access inResendfunction.Tools
GitHub Check: codecov/patch
[warning] 48-48: eth/rpc/backend/call_tx.go#L48
Added line #L48 was not covered by testsapp/evmante_eth.go (1)
273-273: Use of simplified configuration access inAnteHandlefunction ofCanTransferDecorator.Tools
GitHub Check: codecov/patch
[warning] 273-273: app/evmante_eth.go#L273
Added line #L273 was not covered by testsx/evm/statedb/statedb.go (1)
366-371: The changes toPrepareAccessListalign well with the PR's objectives of simplifying Ethereum configuration handling.x/evm/keeper/msg_server.go (1)
Line range hint
355-442: The refactoring ofApplyEvmMsgto include direct access list preparation enhances consistency across transaction types, aligning with the PR's goals.x/evm/keeper/grpc_query.go (1)
Line range hint
1-1: Removed conditional check fortraceConfig.Overrides.Verification successful
The search results indicate that
traceConfigis used extensively within thex/evm/keeper/grpc_query.gofile. However, there are no other references totraceConfig.Overridesoutside of this context. This suggests that the removal of the conditional check fortraceConfig.Overridesdoes not impact other parts of the system.
x/evm/keeper/grpc_query.go: Multiple references totraceConfigbut no critical dependencies ontraceConfig.Overrides.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the removal of the conditional check for `traceConfig.Overrides` does not affect other parts of the system. # Test: Search for usage of `traceConfig.Overrides` in other parts of the codebase. Expect: No critical dependencies on this field. rg --type go 'traceConfig.Overrides'Length of output: 36
Script:
#!/bin/bash # Description: Verify that the removal of the conditional check for `traceConfig.Overrides` does not affect other parts of the system. # Test: Search for usage of `traceConfig` in other parts of the codebase. Expect: No critical dependencies on this field. rg --type go 'traceConfig'Length of output: 1321
| func Validate() error { | ||
| // NOTE: chain ID is not needed to check config order | ||
| if err := cc.EthereumConfig(nil).CheckConfigForkOrder(); err != nil { | ||
| if err := EthereumConfig(nil).CheckConfigForkOrder(); err != nil { |
There was a problem hiding this comment.
The addition of the Validate function to check the configuration's fork order is a good practice. Consider adding more comprehensive tests to ensure its correctness.
Would you like me to help by adding more test cases for the Validate function?
Less code, less bugs
Summary by CodeRabbit
Refactor
Tests
Chores