-
Notifications
You must be signed in to change notification settings - Fork 200
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
perf(evm-keeper-precompile): implement sorted map for k.precompiles
to remove dead code
#1996
Conversation
Warning Rate limit exceeded@k-yang has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 17 minutes and 10 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 changes in the Nibiru project significantly enhance the Ethereum Virtual Machine (EVM) functionalities. Notable improvements include the introduction of atto denomination for NIBI, the consolidation of account query endpoints, and the transition from unordered to sorted maps for optimized data handling. Additionally, several deprecated precompile-related features have been removed, streamlining the codebase. Overall, these changes improve performance, usability, and clarity for developers and users alike. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant EVM Keeper
participant Account Query
participant Precompile Manager
Client->>Account Query: Query account info (either address type)
Account Query->>EVM Keeper: Retrieve account data
EVM Keeper->>Account Query: Return account data
Account Query->>Client: Send account info response
Client->>Precompile Manager: Call precompile function
Precompile Manager->>EVM Keeper: Check precompile availability
EVM Keeper->>Precompile Manager: Return availability status
Precompile Manager->>Client: Send precompile result
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 (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1996 +/- ##
==========================================
+ Coverage 65.75% 65.89% +0.14%
==========================================
Files 261 261
Lines 16471 16466 -5
==========================================
+ Hits 10830 10850 +20
+ Misses 4837 4815 -22
+ Partials 804 801 -3
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
x/evm/evm.pb.go
is excluded by!**/*.pb.go
Files selected for processing (18)
- CHANGELOG.md (1 hunks)
- proto/eth/evm/v1/evm.proto (1 hunks)
- x/common/omap/impl.go (4 hunks)
- x/common/omap/omap.go (7 hunks)
- x/common/omap/omap_test.go (2 hunks)
- x/evm/errors.go (1 hunks)
- x/evm/keeper/evm_state.go (2 hunks)
- x/evm/keeper/grpc_query_test.go (1 hunks)
- x/evm/keeper/keeper.go (2 hunks)
- x/evm/keeper/msg_server.go (3 hunks)
- x/evm/keeper/precompiles.go (2 hunks)
- x/evm/params.go (4 hunks)
- x/evm/precompile/funtoken.go (1 hunks)
- x/evm/precompile/funtoken_test.go (3 hunks)
- x/evm/precompile/precompile.go (2 hunks)
- x/evm/precompile/precompile_test.go (1 hunks)
- x/oracle/keeper/ballot.go (1 hunks)
- x/oracle/keeper/update_exchange_rates.go (1 hunks)
Additional context used
GitHub Check: codecov/patch
x/evm/keeper/precompiles.go
[warning] 20-21: x/evm/keeper/precompiles.go#L20-L21
Added lines #L20 - L21 were not covered by tests
[warning] 40-40: x/evm/keeper/precompiles.go#L40
Added line #L40 was not covered by testsx/common/omap/omap.go
[warning] 67-67: x/common/omap/omap.go#L67
Added line #L67 was not covered by tests
Additional comments not posted (33)
x/evm/keeper/precompiles.go (2)
40-40
: Ensure test coverage forIsAvailablePrecompile
.The
IsAvailablePrecompile
function now uses theHas
method of the sorted map. Ensure that this change is also covered by tests to verify its correctness.Verification successful
Test Coverage for
IsAvailablePrecompile
is AdequateThe
IsAvailablePrecompile
function is covered by tests inx/evm/precompile/funtoken_test.go
, which verify the presence of addresses in the precompiles map. No further action is needed.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify test coverage for the `IsAvailablePrecompile` function. # Test: Search for test cases that cover `IsAvailablePrecompile`. Expect: Tests that check the presence of addresses in the sorted map. rg --type go 'IsAvailablePrecompile' -A 5Length of output: 1452
Tools
GitHub Check: codecov/patch
[warning] 40-40: x/evm/keeper/precompiles.go#L40
Added line #L40 was not covered by tests
11-22
: Ensure test coverage forAddPrecompiles
.The
AddPrecompiles
function now uses a sorted map to manage precompiles, which is a performance improvement. However, ensure that this new logic is covered by tests, especially the loop that sets new precompiles.Tools
GitHub Check: codecov/patch
[warning] 20-21: x/evm/keeper/precompiles.go#L20-L21
Added lines #L20 - L21 were not covered by testsx/evm/precompile/precompile_test.go (1)
24-49
: Good test coverage for sorted map functionality.The
TestOrderedPrecompileAddresses
function effectively verifies that the VM precompiles are ordered and that the ordered map produces the expected ordered address slice. This test ensures the correctness of the new sorted map implementation.x/common/omap/impl.go (1)
59-75
: Verify correctness ofSortedMap_EthAddress
implementation.The
SortedMap_EthAddress
function constructs a sorted map for Ethereum addresses usingaddrSorter
. Ensure that the sorting logic, which converts addresses tobig.Int
, is correct and efficient.Verification successful
SortedMap_EthAddress
Implementation VerifiedThe
SortedMap_EthAddress
function is correctly implemented and its sorting logic is validated through tests inx/common/omap/omap_test.go
. The function is also integrated into other parts of the codebase, further supporting its correctness.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the correctness of the `SortedMap_EthAddress` implementation. # Test: Search for usage and tests of `SortedMap_EthAddress`. Expect: Correct sorting logic for Ethereum addresses. rg --type go 'SortedMap_EthAddress' -A 10Length of output: 1706
x/evm/params.go (2)
12-19
: LGTM! Simplification ofDefaultParams
.The removal of the
ActivePrecompiles
field aligns with the objective to eliminate unnecessary precompile management.
Line range hint
35-49
:
LGTM! Simplification ofValidate
function.The removal of precompile validation is consistent with the objective to streamline parameter management.
x/evm/keeper/keeper.go (1)
47-51
: LGTM! Improved data structure for precompiles.The use of
omap.SortedMap
for theprecompiles
field enhances the efficiency and organization of precompiled contract management.x/evm/precompile/funtoken_test.go (3)
22-24
: Function Renaming Approved.The renaming of
TestPrecompileSuite
toTestSuite
is appropriate as it reflects a broader scope for the test suite.
48-49
: Method Call Update Approved.The replacement of
deps.EvmKeeper.PrecompileSet().Has(precompileAddr.ToAddr())
withdeps.EvmKeeper.IsAvailablePrecompile(precompileAddr.ToAddr())
provides a more direct check for precompile availability.
80-81
: Consistent Method Call Update Approved.The use of
deps.EvmKeeper.IsAvailablePrecompile(precompileAddr.ToAddr())
maintains consistency in checking precompile availability.x/evm/precompile/precompile.go (2)
1-14
: Documentation Enhancements Approved.The expanded documentation provides clear insights into the package's components and functionality, improving usability and understanding.
95-98
: Interface Modification Approved.The explicit requirement for
NibiruPrecompile
to implementvm.PrecompiledContract
enhances clarity and strengthens contract integrity.x/common/omap/omap_test.go (5)
21-23
: Test Suite Initialization Approved.The use of
testify/suite
for initializing and running the test suite improves test organization and reusability.
Line range hint
27-75
:
Functionality Verification Approved.The
TestLenHasKeys
function is well-structured and effectively verifies the functionality ofSortedMap
.
81-121
: Comprehensive Operation Testing Approved.The
TestGetSetDelete
function comprehensively tests theGet
,Set
, andDelete
operations on aSortedMap
.
128-156
: Complex Key Handling Approved.The
TestPair
function effectively tests the sorting and key handling ofSortedMap
withasset.Pair
keys.
158-183
: Ethereum Address Sorting Approved.The
TestEthAddress
function is well-structured and ensures the correct sorting of Ethereum addresses in aSortedMap
.x/oracle/keeper/update_exchange_rates.go (1)
89-89
: Verify the impact of switching toomap.SortedMap_Pair
.The change to use
omap.SortedMap_Pair
ensures deterministic ordering ofpairVotes
. Verify that this change improves performance or correctness, especially in scenarios where the order of processing votes is critical.x/evm/keeper/evm_state.go (1)
Line range hint
56-56
:
Verify the implications of removing sorting onActivePrecompiles
.The removal of
slices.Sort(params.ActivePrecompiles)
suggests that the order ofActivePrecompiles
is no longer significant. Verify that this change does not adversely affect any downstream operations that rely on the order of precompiles.x/oracle/keeper/ballot.go (1)
121-121
: Verify the impact of switching toomap.SortedMap_Pair
.The change to use
omap.SortedMap_Pair
ensures deterministic ordering ofpairVotes
. Verify that this change improves performance or correctness, especially in scenarios where the order of processing votes is critical.Verification successful
Deterministic Ordering with
omap.SortedMap_Pair
Improves CorrectnessThe change to use
omap.SortedMap_Pair
ensures deterministic ordering ofpairVotes
, which is critical for correctness when processing votes. This ordering allows for efficient iteration, particularly in scenarios where certain pairs need to be skipped, as indicated by comments in the code. The presence of a test case inomap_test.go
further supports the functionality of this change. No performance issues were identified related to this change.
- Usage in
x/oracle/keeper/ballot.go
andx/oracle/keeper/update_exchange_rates.go
ensures ordered iteration.- Test case in
x/common/omap/omap_test.go
likely verifies sorting functionality.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of `omap.SortedMap_Pair` and its impact on performance and correctness. # Test: Search for the usage of `omap.SortedMap_Pair` in the repository. Expect: Consistent usage across related functions. rg --type go 'omap.SortedMap_Pair'Length of output: 322
Script:
#!/bin/bash # Search for comments or documentation around the usage of `omap.SortedMap_Pair` in the relevant files. rg --type go -A 5 'omap.SortedMap_Pair' x/oracle/keeper/ballot.go x/oracle/keeper/update_exchange_rates.go # Check for test cases related to `omap.SortedMap_Pair` in `omap_test.go`. rg --type go 'SortedMap_Pair' x/common/omap/omap_test.goLength of output: 1174
x/common/omap/omap.go (9)
25-28
: Well-definedSortedMap
struct.The
SortedMap
struct is appropriately defined with fields to maintain sorted order using generics.
Line range hint
33-37
: EffectiveSorter
interface.The
Sorter
interface is well-defined to facilitate key ordering inSortedMap
.Tools
GitHub Check: codecov/patch
[warning] 67-67: x/common/omap/omap.go#L67
Added line #L67 was not covered by tests
39-42
: UsefulSorterLeq
function.The
SorterLeq
function is correctly implemented to provide "less than or equal" comparison functionality.
44-56
: Encapsulation ensured inData
andInternalData
methods.The methods are well-implemented to manage access to the map data while maintaining encapsulation.
Line range hint
65-84
: EfficientensureOrder
method.The
ensureOrder
method is crucial for maintaining the sorted state of the map and is implemented efficiently.Tools
GitHub Check: codecov/patch
[warning] 67-67: x/common/omap/omap.go#L67
Added line #L67 was not covered by tests
88-94
: CorrectBuildFrom
method.The
BuildFrom
method is correctly implemented for constructingSortedMap
instances from existing data.
Line range hint
101-111
: EfficientRange
method.The
Range
method provides a lazy iteration mechanism over sorted keys, which is efficient and well-implemented.
134-161
: EfficientSet
method.The
Set
method is crucial for maintaining the map's integrity and is implemented efficiently using binary search.
173-187
: CorrectDelete
method.The
Delete
method is correctly implemented to maintain the map's integrity after key deletions.proto/eth/evm/v1/evm.proto (1)
51-53
: Appropriate deprecation ofactive_precompiles
field.The deprecation and reservation of the
active_precompiles
field simplify precompile management by considering all as active by default, aligning with the PR objectives.x/evm/keeper/msg_server.go (1)
257-257
: Verify the correctness ofk.precompiles.Keys()
.Ensure that
k.precompiles.Keys()
returns the addresses in the expected order and format for the EVM.x/evm/keeper/grpc_query_test.go (1)
441-441
: Confirm alignment ofTestQueryParams
with EVM parameter changes.Ensure that the focus on
EvmDenom
correctly reflects the intended changes in the EVM parameters and that the test adequately validates this aspect.CHANGELOG.md (1)
104-104
: Changelog entry is clear and concise.The entry accurately describes the performance improvement related to the implementation of a sorted map for
k.precompiles
.
// Union combines new key-value pairs into the ordered map. | ||
func (om *SortedMap[K, V]) Union(kvMap map[K]V) { | ||
for key, val := range kvMap { | ||
om.data[key] = val | ||
} | ||
om.isOrdered = false | ||
om.ensureOrder() // TODO perf: make this more efficient with a clever insert. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct Union
method with potential for optimization.
The Union
method is correctly implemented, but there is a TODO for performance improvement.
Would you like assistance in optimizing the Union
method for better performance?
// TODO: https://github.com/NibiruChain/nibiru/issues/1990 | ||
// We need to determine an appropriate gas value for the transaction to | ||
// configure this function and add a assertions around the gas usage to | ||
// the precompile's test suite. UD-DEBUG: not implemented yet. Currently | ||
// set to 0 gasPrice |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Informative comments on RequiredGas
method.
The expanded comments provide clarity on the method's status and future work. Consider addressing the TODO to determine an appropriate gas value.
Would you like assistance in determining an appropriate gas value for the RequiredGas
method?
Purpose / Abstract
This pull request completes a smaller performance TODO comment:
Implementing this change removes the need for certain struct fields, functions,
validation, and sorting. It generally removes dead code from x/evm.
evm.AvailableEVMExtensions
constantevm.Params.ActivePrecompiles
field, as this can be read directlyfrom the map.
SortedMap[gethcommon.Address, vm.PrecompiledContract]
words,
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Refactor
Tests