imp(evm): Remove duplicate definitions of precompile addresses #2683
Conversation
|
Warning Rate limit exceeded@MalteHerrmann has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 7 minutes and 23 seconds before requesting another review. How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughThe recent updates streamline the handling of precompile addresses in the EVM module, enhancing maintainability by removing hardcoded constants and consolidating definitions in the Changes
Sequence Diagram(s)sequenceDiagram
participant Dev as Developer
participant EVM as EVM Module
participant evmtypes as evmtypes Package
participant App as Application
Dev->>EVM: Initialize precompile addresses
EVM->>evmtypes: Fetch DefaultStaticPrecompiles
evmtypes-->>EVM: Return consolidated addresses
EVM-->>App: Use addresses in application logic
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 Configuration File (
|
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range, codebase verification and nitpick comments (1)
app/app.go (1)
Imports for precompiles are still present and need to be reviewed.
The following files still contain imports for precompiles:
x/evm/keeper/static_precompiles.go- Various test files in
precompilesdirectoryapp/upgrades/v16/upgrades.goPlease review these imports to ensure they are correctly handled.
Analysis chain
Line range hint
1-950:
Verify the removal of specific precompile imports.Ensure that the removed imports for precompiles are no longer needed and that their references have been correctly updated.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the removed imports for precompiles are no longer needed. # Test: Search for the removed imports. Expect: No results. rg --type go --pattern 'github.com/evmos/evmos/v19/precompiles/bank' --files-with-matches rg --type go --pattern 'github.com/evmos/evmos/v19/precompiles/bech32' --files-with-matches rg --type go --pattern 'github.com/evmos/evmos/v19/precompiles/distribution' --files-with-matches rg --type go --pattern 'github.com/evmos/evmos/v19/precompiles/ics20' --files-with-matches rg --type go --pattern 'github.com/evmos/evmos/v19/precompiles/p256' --files-with-matches rg --type go --pattern 'github.com/evmos/evmos/v19/precompiles/staking' --files-with-matches rg --type go --pattern 'github.com/evmos/evmos/v19/precompiles/vesting' --files-with-matchesLength of output: 3530
Script:
#!/bin/bash # Description: Verify that the removed imports for precompiles are no longer needed. # Test: Search for the removed imports. Expect: No results. rg 'github.com/evmos/evmos/v19/precompiles/bank' --files-with-matches rg 'github.com/evmos/evmos/v19/precompiles/bech32' --files-with-matches rg 'github.com/evmos/evmos/v19/precompiles/distribution' --files-with-matches rg 'github.com/evmos/evmos/v19/precompiles/ics20' --files-with-matches rg 'github.com/evmos/evmos/v19/precompiles/p256' --files-with-matches rg 'github.com/evmos/evmos/v19/precompiles/staking' --files-with-matches rg 'github.com/evmos/evmos/v19/precompiles/vesting' --files-with-matchesLength of output: 2213
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (14)
- CHANGELOG.md (1 hunks)
- app/app.go (2 hunks)
- app/app_test.go (1 hunks)
- precompiles/bank/bank.go (1 hunks)
- precompiles/bech32/bech32.go (2 hunks)
- precompiles/distribution/distribution.go (2 hunks)
- precompiles/ics20/ics20.go (2 hunks)
- precompiles/p256/p256.go (2 hunks)
- precompiles/p256/p256_test.go (1 hunks)
- precompiles/staking/staking.go (3 hunks)
- precompiles/vesting/vesting.go (2 hunks)
- x/evm/keeper/integration_test.go (1 hunks)
- x/evm/types/params.go (2 hunks)
- x/evm/types/precompiles.go (1 hunks)
Files skipped from review due to trivial changes (3)
- CHANGELOG.md
- precompiles/bank/bank.go
- x/evm/types/precompiles.go
Additional context used
GitHub Check: CodeQL
precompiles/bech32/bech32.go
[warning] 50-50: Directly using the bech32 constants
Directly using the bech32 constants instead of the configuration values
Additional comments not posted (14)
precompiles/bech32/bech32.go (2)
14-14: Import statement is correct.The import statement for
evmtypesis correctly added.
50-50: Refactored to use centralized address definition.The
Addressmethod now usesevmtypes.Bech32PrecompileAddressinstead of a hardcoded constant, which improves maintainability and reduces redundancy.Tools
GitHub Check: CodeQL
[warning] 50-50: Directly using the bech32 constants
Directly using the bech32 constants instead of the configuration valuesprecompiles/p256/p256.go (2)
25-25: Import statement is correct.The import statement for
evmtypesis correctly added.
44-44: Refactored to use centralized address definition.The
Addressmethod now usesevmtypes.P256PrecompileAddressinstead of a hardcoded constant, which improves maintainability and reduces redundancy.precompiles/p256/p256_test.go (2)
14-14: Import statement is correct.The import statement for
evmtypesis correctly added.
20-20: Test updated to use centralized address definition.The
TestAddressfunction now usesevmtypes.P256PrecompileAddressinstead ofp256.PrecompileAddress, ensuring consistency with the new centralized address definition.precompiles/vesting/vesting.go (1)
20-20: LGTM! But verify the centralized precompile address usage.The change to use
evmtypes.VestingPrecompileAddressinstead of a hardcoded address improves maintainability. Ensure thatevmtypes.VestingPrecompileAddressis correctly defined and used throughout the codebase.Also applies to: 76-79
Verification successful
Verified: Centralized precompile address usage is correct and consistent.
The
evmtypes.VestingPrecompileAddressis correctly defined and used consistently across the codebase. The change enhances maintainability by centralizing the precompile address definition.
x/evm/types/params.go: Included in theDefaultStaticPrecompilesarray.precompiles/vesting/vesting.go: Used in theSetAddressmethod of the vesting precompile contract.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the definition and usage of `evmtypes.VestingPrecompileAddress`. # Test: Search for the definition and usage of `VestingPrecompileAddress`. Expect: Correct definition and consistent usage. rg --type go 'VestingPrecompileAddress'Length of output: 376
Script:
#!/bin/bash # Verify the context of the usage of `VestingPrecompileAddress` in `x/evm/types/params.go` and `precompiles/vesting/vesting.go` # Display the surrounding lines of the usage in `x/evm/types/params.go` rg --type go --context 10 'VestingPrecompileAddress' x/evm/types/params.go # Display the surrounding lines of the usage in `precompiles/vesting/vesting.go` rg --type go --context 10 'VestingPrecompileAddress' precompiles/vesting/vesting.goLength of output: 1859
precompiles/ics20/ics20.go (1)
17-17: LGTM! But verify the centralized precompile address usage.The change to use
evmtypes.ICS20PrecompileAddressinstead of a hardcoded address improves maintainability. Ensure thatevmtypes.ICS20PrecompileAddressis correctly defined and used throughout the codebase.Also applies to: 61-64
Verification successful
Verified: Centralized precompile address usage
The
evmtypes.ICS20PrecompileAddressis correctly defined and consistently used across the codebase, enhancing maintainability.
x/evm/types/precompiles.go: Definition and usagex/evm/types/params.go: Usageprecompiles/ics20/ics20.go: UsageScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the definition and usage of `evmtypes.ICS20PrecompileAddress`. # Test: Search for the definition and usage of `ICS20PrecompileAddress`. Expect: Correct definition and consistent usage. rg --type go 'ICS20PrecompileAddress'Length of output: 373
precompiles/distribution/distribution.go (1)
16-16: LGTM! But verify the centralized precompile address usage.The change to use
evmtypes.DistributionPrecompileAddressinstead of a hardcoded address improves maintainability. Ensure thatevmtypes.DistributionPrecompileAddressis correctly defined and used throughout the codebase.Also applies to: 57-60
Verification successful
Verified: Centralized precompile address usage is correct.
The
evmtypes.DistributionPrecompileAddressis correctly defined and consistently used throughout the codebase.
- Defined in
x/evm/types/precompiles.go- Used in
x/evm/types/params.go- Used in
precompiles/distribution/distribution.goScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the definition and usage of `evmtypes.DistributionPrecompileAddress`. # Test: Search for the definition and usage of `DistributionPrecompileAddress`. Expect: Correct definition and consistent usage. rg --type go 'DistributionPrecompileAddress'Length of output: 406
app/app_test.go (1)
111-112: LGTM! Ensure the static list is accurate.The change simplifies the initialization of
availablePrecompilesand improves maintainability by centralizing the source of truth for precompile addresses.However, ensure that
evmtypes.AvailableStaticEVMExtensionsaccurately reflects the expected precompiles.Verification successful
Verification Successful: Static List is Accurate
The static list
AvailableStaticEVMExtensionsaccurately reflects the expected precompiles. The change improves maintainability by centralizing the source of truth for precompile addresses.
x/evm/types/precompiles.go: Contains a well-defined list of precompile addresses.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the accuracy of `evmtypes.AvailableStaticEVMExtensions`. # Test: Search for the definition of `AvailableStaticEVMExtensions`. Expect: Correct and up-to-date list of precompiles. rg --type go --pattern 'AvailableStaticEVMExtensions'Length of output: 465
Script:
#!/bin/bash # Description: Verify the accuracy of `evmtypes.AvailableStaticEVMExtensions`. # Test: Search for the definition of `AvailableStaticEVMExtensions`. Expect: Correct and up-to-date list of precompiles. rg 'AvailableStaticEVMExtensions'Length of output: 423
Script:
#!/bin/bash # Description: Display the contents of `x/evm/types/precompiles.go` to verify the accuracy of `AvailableStaticEVMExtensions`. # Test: Display the file content. Expect: Correct and up-to-date list of precompiles. cat x/evm/types/precompiles.goLength of output: 1185
precompiles/staking/staking.go (1)
64-65: LGTM! Ensure the centralized address is correct.The change improves maintainability by centralizing the definition of the staking precompile address.
However, ensure that
evmtypes.StakingPrecompileAddressis correctly defined and used consistently.Verification successful
The centralized address is correct and used consistently.
The address
0x0000000000000000000000000000000000000800is defined inx/evm/types/precompiles.goand used consistently across multiple files, includingprecompiles/staking/staking.go.
x/evm/types/precompiles.go: Definition and usage.x/evm/types/params.go: Usage.x/evm/keeper/integration_test.go: Usage.precompiles/staking/staking.go: Usage.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the definition and usage of `StakingPrecompileAddress`. # Test: Search for the definition of `StakingPrecompileAddress`. Expect: Correct and consistent usage. rg --type go --pattern 'StakingPrecompileAddress'Length of output: 461
Script:
#!/bin/bash # Description: Verify the definition and usage of `StakingPrecompileAddress`. # Test: Search for the definition of `StakingPrecompileAddress`. Expect: Correct and consistent usage. rg 'StakingPrecompileAddress'Length of output: 475
x/evm/types/params.go (1)
29-35: LGTM! Ensure the updated references are correct.The change improves maintainability by centralizing the definitions of precompile addresses.
However, ensure that the updated references are correct and consistent.
Verification successful
LGTM! The updated references to precompile addresses are correct and consistent.
The change improves maintainability by centralizing the definitions of precompile addresses.
- Definitions found in:
x/evm/types/precompiles.go- Usages found in:
x/evm/types/params.go,x/evm/keeper/integration_test.go,precompiles/vesting/vesting.go,precompiles/distribution/distribution.go,precompiles/p256/p256_test.go,precompiles/p256/p256.go,precompiles/ics20/ics20.go,precompiles/staking/staking.go,precompiles/bech32/bech32.goScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the updated references to precompile addresses. # Test: Search for the definitions of the updated precompile addresses. Expect: Correct and consistent references. rg --type go --pattern 'P256PrecompileAddress|Bech32PrecompileAddress|StakingPrecompileAddress|DistributionPrecompileAddress|ICS20PrecompileAddress|VestingPrecompileAddress|BankPrecompileAddress'Length of output: 609
Script:
#!/bin/bash # Description: Verify the updated references to precompile addresses. # Test: Search for the definitions of the updated precompile addresses. Expect: Correct and consistent references. rg 'P256PrecompileAddress|Bech32PrecompileAddress|StakingPrecompileAddress|DistributionPrecompileAddress|ICS20PrecompileAddress|VestingPrecompileAddress|BankPrecompileAddress' --type goLength of output: 2652
x/evm/keeper/integration_test.go (1)
317-317: Verify the correctness of the new reference.Ensure that
evmtypes.StakingPrecompileAddressis the correct and intended reference for thecontractAddress.Verification successful
The new reference is correct.
The reference to
evmtypes.StakingPrecompileAddressinx/evm/keeper/integration_test.gois correct and consistent with the intended refactor.
StakingPrecompileAddressis defined inx/evm/types/precompiles.go.- It is used in multiple places, confirming its correctness and consistency.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the definition of `StakingPrecompileAddress` in `evmtypes`. # Test: Search for the definition. Expect: Only one definition. rg --type go --pattern 'StakingPrecompileAddress' --files-with-matches | xargs rg --type go --pattern 'StakingPrecompileAddress'Length of output: 950
Script:
#!/bin/bash # Description: Verify the definition of `StakingPrecompileAddress` in `evmtypes`. # Test: Search for the definition. Expect: Only one definition. rg --type go 'StakingPrecompileAddress'Length of output: 485
app/app.go (1)
Line range hint
944-950:
Verify the correctness of the new initialization.Ensure that
evmtypes.DefaultStaticPrecompilescorrectly initializes theblockedPrecompilesHexand that the additional addresses fromvm.PrecompiledAddressesBerlinare correctly appended.Verification successful
The initialization of
blockedPrecompilesHexis correct.The
blockedPrecompilesHexvariable is initialized withevmtypes.DefaultStaticPrecompiles, which is an array of precompile addresses defined inx/evm/types/params.go. Additional addresses fromvm.PrecompiledAddressesBerlin, defined inx/evm/core/vm/contracts.go, are correctly appended to this array. Both definitions are appropriate and consistent with their usage in the refactored code.
evmtypes.DefaultStaticPrecompilesis initialized with a set of precompile addresses.vm.PrecompiledAddressesBerlinis an array of precompile addresses used in the Berlin release.The new initialization of
blockedPrecompilesHexis valid and aligns with the intended refactor.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the initialization of `DefaultStaticPrecompiles` in `evmtypes`. # Test: Search for the definition. Expect: Only one definition. rg --type go --pattern 'DefaultStaticPrecompiles' --files-with-matches | xargs rg --type go --pattern 'DefaultStaticPrecompiles'Length of output: 950
Script:
#!/bin/bash # Description: Verify the initialization of `DefaultStaticPrecompiles` in `evmtypes`. # Step 1: Find the file containing the definition of `DefaultStaticPrecompiles`. rg --type go --files-with-matches 'DefaultStaticPrecompiles' # Step 2: Inspect the definition of `DefaultStaticPrecompiles` in the identified file(s). rg --type go 'DefaultStaticPrecompiles' $(rg --type go --files-with-matches 'DefaultStaticPrecompiles')Length of output: 687
Script:
#!/bin/bash # Description: Extract the definition of `DefaultStaticPrecompiles` from `x/evm/types/params.go`. # Extract the lines around the definition of `DefaultStaticPrecompiles`. rg --type go --context 10 'DefaultStaticPrecompiles' x/evm/types/params.goLength of output: 1648
Script:
#!/bin/bash # Description: Verify the definition and usage of `vm.PrecompiledAddressesBerlin`. # Step 1: Find the file containing the definition of `PrecompiledAddressesBerlin`. rg --type go --files-with-matches 'PrecompiledAddressesBerlin' # Step 2: Inspect the definition of `PrecompiledAddressesBerlin` in the identified file(s). rg --type go 'PrecompiledAddressesBerlin' $(rg --type go --files-with-matches 'PrecompiledAddressesBerlin')Length of output: 877
Script:
#!/bin/bash # Description: Extract the definition of `PrecompiledAddressesBerlin` from `x/evm/core/vm/contracts.go`. # Extract the lines around the definition of `PrecompiledAddressesBerlin`. rg --type go --context 10 'PrecompiledAddressesBerlin' x/evm/core/vm/contracts.goLength of output: 1503
There was a problem hiding this comment.
Great work @MalteHerrmann, left some comment but it's ok for me!
Co-authored-by: stepit <48993133+0xstepit@users.noreply.github.com> Signed-off-by: MalteHerrmann <42640438+MalteHerrmann@users.noreply.github.com>
This PR refactors the use of the precompile addresses to remove duplicate definitions that could potentially go out of sync at one point.
Instead, the available static precompiles are moved to a corresponding variable in the EVM types.
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
evmtypespackage.