feat: add support stateful precompiles #66
Conversation
- (evm) crypto-org-chain#328 Support precompile interface. - (statedb) crypto-org-chain#333 Support native action in statedb, prepare for precompiles. - (precompile) crypto-org-chain#338 Fix simulation of precompile using context. - (precompile) crypto-org-chain#342 Problem: native action don't support mem keys - (precompile) crypto-org-chain#343 Problem: no api to convert native events to logs - (precompile) crypto-org-chain#344 Fix error handling in precompile context. - (precompile) crypto-org-chain#346 Add support for new precompile context. - (precompile) crypto-org-chain#347 Problem: miss contract in ExtStateDB interface - (precompile) crypto-org-chain#359 Problem: no efficient way to execute read-only native actions - (precompile) crypto-org-chain#371 Add StateDB itself into native context for precompiles to emit evm logs directly. - (precompile) crypto-org-chain#380 Allow init precompiled contract with rules when new evm.
* Problem: no access to ctx params in precompile * Update CHANGELOG.md Signed-off-by: mmsqe <mavis@crypto.com> --------- Signed-off-by: mmsqe <mavis@crypto.com>
Signed-off-by: Francisco de Borja Aranda Castillejo <borja@zetachain.com>
WalkthroughThe recent updates significantly enhance the Changes
Assessment against linked issues
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 Configuration File (
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 15
Outside diff range, codebase verification and nitpick comments (3)
x/evm/statedb/native.go (1)
15-17: Consider adding a comment explaining the purpose of this method.The
Dirtiedmethod returns a nil pointer. If this is a placeholder or a stub, it should be documented.x/evm/types/tracer.go (1)
48-48: FunctionDefaultActivePrecompilesnot found in the codebase.The function
DefaultActivePrecompilesused inx/evm/types/tracer.goon line 48 does not exist in the codebase. This indicates a potential issue with the usage of this function.
x/evm/types/tracer.goline 48:preCompiles := vm.DefaultActivePrecompiles(cfg.Rules(big.NewInt(height), cfg.MergeNetsplitBlock != nil))Please verify the intended function and correct the usage accordingly.
Analysis chain
LGTM! Verify the correct usage of
vm.DefaultActivePrecompiles.The change to use
vm.DefaultActivePrecompilesinstead ofvm.ActivePrecompilesappears to be correct.However, ensure that
vm.DefaultActivePrecompilesis the intended function and is correctly used throughout the codebase.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of `vm.DefaultActivePrecompiles` in the codebase. # Test: Search for the usage of `vm.DefaultActivePrecompiles`. Expect: Correct usage in relevant files. rg --type go 'vm.DefaultActivePrecompiles'Length of output: 171
Script:
#!/bin/bash # Description: Verify the definition of `vm.DefaultActivePrecompiles` in the codebase. # Test: Search for the definition of `vm.DefaultActivePrecompiles`. Expect: Correct definition and return value. ast-grep --lang go --pattern 'func DefaultActivePrecompiles($_) $_'Length of output: 69
Script:
#!/bin/bash # Description: Verify the definition of `vm.DefaultActivePrecompiles` in the codebase. # Test: Search for the definition of `vm.DefaultActivePrecompiles`. Expect: Correct definition and return value. rg --type go 'func DefaultActivePrecompiles'Length of output: 46
Script:
#!/bin/bash # Description: List all functions in the `vm` package to identify any similar or related functions to `DefaultActivePrecompiles`. # Test: List all functions in the `vm` package. Expect: Identify any similar or related functions. rg --type go --files-with-matches 'package vm' | xargs rg --type go 'func 'Length of output: 312617
store/cachekv/store_test.go (1)
38-38: Fix typo in comment.The comment has a typo: "shoudn't" should be "shouldn't".
- // update it in cache, shoudn't change mem + // update it in cache, shouldn't change mem
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (3)
go.sumis excluded by!**/*.sumx/evm/migrations/v4/types/evm.pb.gois excluded by!**/*.pb.gox/evm/migrations/v5/types/evm.pb.gois excluded by!**/*.pb.go
Files selected for processing (28)
- CHANGELOG.md (2 hunks)
- app/ante/eth.go (2 hunks)
- app/ante/interfaces.go (2 hunks)
- app/app.go (2 hunks)
- go.mod (5 hunks)
- rpc/backend/utils.go (1 hunks)
- store/cachekv/README.md (1 hunks)
- store/cachekv/bench_helper_test.go (1 hunks)
- store/cachekv/benchmark_test.go (1 hunks)
- store/cachekv/internal/btree.go (1 hunks)
- store/cachekv/internal/btree_test.go (1 hunks)
- store/cachekv/internal/memiterator.go (1 hunks)
- store/cachekv/internal/mergeiterator.go (1 hunks)
- store/cachekv/store.go (1 hunks)
- store/cachekv/store_bench_test.go (1 hunks)
- store/cachekv/store_test.go (1 hunks)
- store/cachemulti/store.go (1 hunks)
- store/cachemulti/store_test.go (1 hunks)
- x/evm/keeper/keeper.go (5 hunks)
- x/evm/keeper/state_transition.go (6 hunks)
- x/evm/statedb/interfaces.go (1 hunks)
- x/evm/statedb/mock_test.go (3 hunks)
- x/evm/statedb/native.go (1 hunks)
- x/evm/statedb/statedb.go (6 hunks)
- x/evm/statedb/statedb_test.go (21 hunks)
- x/evm/statedb/transient_storage.go (1 hunks)
- x/evm/types/interfaces.go (1 hunks)
- x/evm/types/tracer.go (1 hunks)
Additional context used
golangci-lint
x/evm/statedb/transient_storage.go
24-24: type
transientStorageis unused(unused)
27-27: func
newTransientStorageis unused(unused)
32-32: func
transientStorage.Setis unused(unused)
40-40: func
transientStorage.Getis unused(unused)
49-49: func
transientStorage.Copyis unused(unused)
x/evm/statedb/statedb.go
[warning] 93-93: unused-parameter: parameter 'params' seems to be unused, consider removing or renaming it as _
(revive)
78-78: field
transientStorageis unused(unused)
84-84: field
evmDenomis unused(unused)
85-85: field
erris unused(unused)
GitHub Check: gosec
x/evm/statedb/transient_storage.go
[failure] 51-56: the value in the range statement should be _ unless copying a map: want: for key := range m
expected exactly 1 statement (either append, delete, or copying to another map) in a range with a map, got 2store/cachemulti/store.go
[failure] 128-130: the value in the range statement should be _ unless copying a map: want: for key := range m
the value in the range statement should be _ unless copying a map: want: for key := range m
[failure] 119-121: the value in the range statement should be _ unless copying a map: want: for key := range m
the value in the range statement should be _ unless copying a map: want: for key := range m
[failure] 64-66: the value in the range statement should be _ unless copying a map: want: for key := range m
the value in the range statement should be _ unless copying a map: want: for key := range m
[failure] 142-148: the value in the range statement should be _ unless copying a map: want: for key := range m
expected exactly 1 statement (either append, delete, or copying to another map) in a range with a map, got 3
[failure] 45-54: the value in the range statement should be _ unless copying a map: want: for key := range m
expected exactly 1 statement (either append, delete, or copying to another map) in a range with a map, got 2x/evm/keeper/state_transition.go
[failure] 77-80: the value in the range statement should be _ unless copying a map: want: for key := range m
expected exactly 1 statement (either append, delete, or copying to another map) in a range with a map, got 2
GitHub Check: Run golangci-lint
x/evm/statedb/transient_storage.go
[failure] 24-24:
typetransientStorageis unused (unused)
[failure] 27-27:
funcnewTransientStorageis unused (unused)
[failure] 32-32:
functransientStorage.Setis unused (unused)
[failure] 40-40:
functransientStorage.Getis unused (unused)
[failure] 49-49:
functransientStorage.Copyis unused (unused)x/evm/statedb/statedb.go
[warning] 93-93:
unused-parameter: parameter 'params' seems to be unused, consider removing or renaming it as _ (revive)
[failure] 78-78:
fieldtransientStorageis unused (unused)
[failure] 84-84:
fieldevmDenomis unused (unused)
[failure] 85-85:
fielderris unused (unused)
GitHub Check: CodeQL
store/cachemulti/store.go
[warning] 45-54: Iteration over map
Iteration over map may be a possible source of non-determinism
[warning] 64-66: Iteration over map
Iteration over map may be a possible source of non-determinism
[warning] 72-74: Iteration over map
Iteration over map may be a possible source of non-determinism
[warning] 119-121: Iteration over map
Iteration over map may be a possible source of non-determinism
[warning] 128-130: Iteration over map
Iteration over map may be a possible source of non-determinism
[warning] 142-148: Iteration over map
Iteration over map may be a possible source of non-determinismapp/app.go
[warning] 465-467: Iteration over map
Iteration over map may be a possible source of non-determinism
[warning] 468-470: Iteration over map
Iteration over map may be a possible source of non-determinism
[warning] 471-473: Iteration over map
Iteration over map may be a possible source of non-determinism
LanguageTool
store/cachekv/README.md
[uncategorized] ~3-~3: You might be missing the article “a” here.
Context: ...tore specification ACacheKVStoreis cache wrapper for aKVStore. It extends the...(AI_EN_LECTOR_MISSING_DETERMINER_A)
[uncategorized] ~15-~15: Possible missing comma found.
Context: ...ould revisit these goals with time (for instance it's unclear that all disk writes need ...(AI_HYDRA_LEO_MISSING_COMMA)
[style] ~15-~15: You have already used this phrasing in nearby sentences. Consider replacing it to add variety to your writing.
Context: ...tance it's unclear that all disk writes need to be buffered to the end of the block), b...(REP_NEED_TO_VB)
[grammar] ~34-~34: Probable usage error. Use “and” after ‘both’.
Context: ...ys that are cached from read operations as well as ‘dirty’ keys which map to a value that ...(BOTH_AS_WELL_AS)
[style] ~34-~34: Did you mean ‘different from’? ‘Different than’ is often considered colloquial style.
Context: ...o a value that is potentially different than what is in the underlyingKVStore. V...(DIFFERENT_THAN)
[grammar] ~51-~51: The verb ‘write’ does not usually follow articles like ‘a’. Check that ‘write’ is spelled correctly; using ‘write’ as a noun may be non-standard.
Context: ...be updated in the parentKVStoreupon a write. Keys are mapped to an empty struct to ...(A_INFINITIVE)
[style] ~93-~93: ‘taking into account’ might be wordy. Consider a shorter alternative.
Context: ...irs from the underlyingKVStorewhile taking into account updated values from the cache. In the...(EN_WORDINESS_PREMIUM_TAKING_INTO_ACCOUNT)
[locale-violation] ~134-~134: In American English, ‘afterward’ is the preferred variant. ‘Afterwards’ is more commonly used in British English and other dialects.
Context: ...removing each key fromunsortedCache. Afterwards,unsortedis sorted. Lastly, it itera...(AFTERWARDS_US)
[grammar] ~140-~140: The verb ‘compute’ does not usually follow articles like ‘the’. Check that ‘compute’ is spelled correctly; using ‘compute’ as a noun may be non-standard.
Context: ...et inunsortedCache. To avoid wasting the compute spent sorting, we should ensure that a ...(A_INFINITIVE)
[uncategorized] ~140-~140: ‘Amount of’ should usually only be used with uncountable or mass nouns. Consider using “number” if this is not the case.
Context: ...ing, we should ensure that a reasonable amount of values are removed from `unsortedCac...(AMOUNTOF_TO_NUMBEROF)
[grammar] ~140-~140: Possible verb agreement error. Did you mean “is”? (Some collective nouns can be treated as both singular and plural, so ‘are’ is not always incorrect.)
Context: ...sure that a reasonable amount of values are removed fromunsortedCache. If the le...(COLLECTIVE_NOUN_VERB_AGREEMENT_VBP)
Markdownlint
store/cachekv/README.md
10-10: Expected: 4; Actual: 2
Unordered list indentation(MD007, ul-indent)
11-11: Expected: 4; Actual: 2
Unordered list indentation(MD007, ul-indent)
73-73: Expected: 0 or 2; Actual: 1
Trailing spaces(MD009, no-trailing-spaces)
79-79: Expected: 0 or 2; Actual: 1
Trailing spaces(MD009, no-trailing-spaces)
85-85: Expected: 0 or 2; Actual: 1
Trailing spaces(MD009, no-trailing-spaces)
93-93: Expected: 0 or 2; Actual: 1
Trailing spaces(MD009, no-trailing-spaces)
132-132: Expected: 0 or 2; Actual: 1
Trailing spaces(MD009, no-trailing-spaces)
136-136: Expected: 0 or 2; Actual: 1
Trailing spaces(MD009, no-trailing-spaces)
138-138: Expected: 0 or 2; Actual: 1
Trailing spaces(MD009, no-trailing-spaces)
140-140: null
Files should end with a single newline character(MD047, single-trailing-newline)
Additional comments not posted (134)
x/evm/statedb/native.go (2)
10-13: LGTM!The struct definition
nativeChangelooks good and is appropriate for its purpose.
19-21: LGTM! But verify the implementations ofrestoreNativeStateandnativeEvents.The
Revertmethod appears to correctly revert the state changes. Ensure thatrestoreNativeStateandnativeEventsare correctly implemented elsewhere.Verification successful
Let's verify the implementation of the
restoreNativeStatefunction to ensure it is correctly defined and used.
Let's search for any function related to restoring the native state, including potential variations in naming.
LGTM! The
Revertmethod is correctly implemented.The
restoreNativeStatefunction and thenativeEventsfield are correctly defined and used within the codebase.
restoreNativeStateis implemented inx/evm/statedb/statedb.go.nativeEventsis correctly managed throughout the codebase.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the implementations of `restoreNativeState` and `nativeEvents`. # Test: Search for the implementations. Expect: Correct implementations of `restoreNativeState` and `nativeEvents`. rg --type go 'func restoreNativeState' -A 10 rg --type go 'nativeEvents' -A 10Length of output: 3162
Script:
#!/bin/bash # Description: Verify the implementation of `restoreNativeState`. # Test: Search for the implementation of `restoreNativeState`. Expect: Correct implementation of `restoreNativeState`. rg --type go 'func restoreNativeState' -A 20Length of output: 46
Script:
#!/bin/bash # Description: Verify the implementation of `restoreNativeState` or similarly named functions. # Test: Search for any function related to restoring the native state. Expect: Correct implementation of such a function. rg --type go 'restore' -A 20Length of output: 5537
store/cachemulti/store_test.go (1)
13-24: LGTM!The test function
TestStoreGetKVStoreis well-written and covers the expected behavior.store/cachekv/bench_helper_test.go (3)
5-8: LGTM!The helper function
randSlicecorrectly generates a random byte slice using thecrypto/randpackage.
11-19: LGTM!The helper function
incrementByteSlicecorrectly increments a byte slice as if it were a big-endian integer.
23-43: LGTM!The helper functions
generateSequentialKeysandgenerateRandomKeyscorrectly generate slices of sequential and random keys, respectively.x/evm/statedb/interfaces.go (1)
27-29: LGTM!The added methods
StoreKeysandGetParamsin theKeeperinterface are essential and well-defined.app/ante/interfaces.go (1)
45-45: Verify compatibility ofNewEVMreturn type change.The return type of the
NewEVMmethod has been changed fromevm.EVMto*vm.EVM. Ensure that all implementations and usages of this method are compatible with the new return type.store/cachekv/internal/memiterator.go (8)
25-65: LGTM!The
newMemIteratormethod is correctly implemented and initializes the iterator based on the provided parameters.
67-69: LGTM!The
Domainmethod is correctly implemented and returns the expected values.
71-73: LGTM!The
Closemethod is correctly implemented and releases the iterator as expected.
76-80: LGTM!The
Errormethod is correctly implemented and returns the expected error.
83-85: LGTM!The
Validmethod is correctly implemented and returns the expected validity.
87-99: LGTM!The
Nextmethod is correctly implemented and advances the iterator as expected.
101-109: LGTM!The
keyInRangemethod is correctly implemented and performs the range check as expected.
119-123: LGTM!The
assertValidmethod is correctly implemented and performs the validity assertion as expected.store/cachekv/internal/btree.go (10)
28-31: LGTM!The
NewBTreemethod is correctly implemented and initializes theBTreeinstance.
33-38: LGTM!The
initmethod is correctly implemented and sets up theBTreewith the desired options.
40-45: LGTM!The
Setmethod is correctly implemented and inserts the key-value pair as expected.
47-56: LGTM!The
Getmethod is correctly implemented and retrieves the value as expected.
59-63: LGTM!The
Deletemethod is correctly implemented and deletes the key-value pair as expected.
66-70: LGTM!The
Iteratormethod is correctly implemented and creates the iterator as expected.
73-77: LGTM!The
ReverseIteratormethod is correctly implemented and creates the reverse iterator as expected.
80-91: LGTM!The
ScanDirtyItemsmethod is correctly implemented and iterates over the dirty entries as expected.
94-103: LGTM!The
Copymethod is correctly implemented and performs the copy operation as expected.
118-120: LGTM!The
newItemmethod is correctly implemented and creates theitemas expected.x/evm/types/interfaces.go (1)
52-53: LGTM! Verify the implementation of new methods.The addition of
IsSendEnabledCoinsandBlockedAddrto theBankKeeperinterface appears to be correct.However, ensure that these methods are correctly implemented in the relevant structs.
x/evm/statedb/mock_test.go (6)
6-9: LGTM! New imports are necessary and correctly used.The new imports for
evmtypesandstoretypesare necessary and correctly used in the file.
30-31: LGTM! New fields are correctly added toMockKeeper.The new fields
keysandeventConvertersare correctly added to theMockKeeperstruct.
34-39: LGTM! TheNewMockKeeperWithfunction correctly initializesMockKeeper.The
NewMockKeeperWithfunction correctly initializes theMockKeeperstruct withkeysandeventConverters.
43-45: LGTM! TheNewMockKeeperfunction correctly callsNewMockKeeperWith.The
NewMockKeeperfunction correctly initializes theMockKeeperstruct by callingNewMockKeeperWithwithnilparameters.
47-57: LGTM! New methods are correctly defined.The new methods
GetParams,StoreKeys, andEventConvertersare correctly defined and consistent with the rest of theMockKeeperstruct.
135-143: LGTM! TheClonemethod correctly clonesMockKeeper.The
Clonemethod correctly clones theMockKeeperstruct, including the new fieldskeysandeventConverters.store/cachekv/benchmark_test.go (3)
1-61: LGTM! Benchmark tests are correctly defined.The benchmark tests
DoBenchmarkDeepCacheStackand specific depth benchmarks are correctly defined and cover the necessary cases.
63-70: LGTM!CacheStackstruct is correctly defined.The
CacheStackstruct is correctly defined and integrates well within the file.
72-133: LGTM!CacheStackmethods are correctly defined.The
CacheStackmethodsCurrentStore,Reset,IsEmpty,Commit,CommitToRevision,Snapshot, andRevertToSnapshotare correctly defined and consistent with the rest of the struct.store/cachekv/store_bench_test.go (5)
135-149: LGTM!The wrapper functions for benchmark tests are correctly implemented.
17-43: Verify the existence and correctness of helper functions.Ensure that the
randSliceandgenerateSequentialKeysfunctions are defined and correctly implemented.Verification successful
Helper functions are correctly implemented.
The
randSliceandgenerateSequentialKeysfunctions are defined and correctly implemented instore/cachekv/bench_helper_test.go.
randSlicegenerates a random byte slice of a given size.generateSequentialKeysgenerates a sequence of keys starting from a given key.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the existence and correctness of helper functions. # Test: Search for the function definitions. Expect: Definitions of `randSlice` and `generateSequentialKeys`. rg --type go 'func randSlice|func generateSequentialKeys'Length of output: 237
Script:
#!/bin/bash # Extract the implementations of `randSlice` and `generateSequentialKeys` from `store/cachekv/bench_helper_test.go`. rg --type go -A 10 'func randSlice' store/cachekv/bench_helper_test.go rg --type go -A 20 'func generateSequentialKeys' store/cachekv/bench_helper_test.goLength of output: 1003
46-64: Verify the existence and correctness of helper functions.Ensure that the
randSliceandgenerateSequentialKeysfunctions are defined and correctly implemented.Verification successful
Verify the correctness of helper functions.
Ensure that the
randSliceandgenerateSequentialKeysfunctions are correctly implemented.
Helper functions are correctly implemented.
The
randSliceandgenerateSequentialKeysfunctions are correctly defined and implemented.
randSlicegenerates a random slice of bytes.generateSequentialKeysproduces sequential keys starting from a given key.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the existence and correctness of helper functions. # Test: Search for the function definitions. Expect: Definitions of `randSlice` and `generateSequentialKeys`. rg --type go 'func randSlice|func generateSequentialKeys'Length of output: 237
Script:
#!/bin/bash # Description: Extract the implementations of `randSlice` and `generateSequentialKeys`. # Extract the implementation of `randSlice`. rg --type go --context 10 'func randSlice' store/cachekv/bench_helper_test.go # Extract the implementation of `generateSequentialKeys`. rg --type go --context 10 'func generateSequentialKeys' store/cachekv/bench_helper_test.goLength of output: 954
98-133: Verify the existence and correctness of helper functions.Ensure that the
randSliceandgenerateSequentialKeysfunctions are defined and correctly implemented.Verification successful
Review the implementations of helper functions.
The
randSliceandgenerateSequentialKeysfunctions are defined instore/cachekv/bench_helper_test.go. Let's review their implementations to ensure correctness.
Verified: Helper functions are correctly implemented.
The
randSliceandgenerateSequentialKeysfunctions are defined and correctly implemented instore/cachekv/bench_helper_test.go. They are suitable for their usage in the benchmark function.
randSlicegenerates a random byte slice of a specified size.generateSequentialKeysgenerates a specified number of sequential byte slices starting from a given key.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the existence and correctness of helper functions. # Test: Search for the function definitions. Expect: Definitions of `randSlice` and `generateSequentialKeys`. rg --type go 'func randSlice|func generateSequentialKeys'Length of output: 237
Script:
#!/bin/bash # Extract and review the implementations of randSlice and generateSequentialKeys. # Extract the implementation of randSlice. rg --type go -A 10 'func randSlice' store/cachekv/bench_helper_test.go # Extract the implementation of generateSequentialKeys. rg --type go -A 15 'func generateSequentialKeys' store/cachekv/bench_helper_test.goLength of output: 882
68-92: Verify the existence and correctness of helper functions.Ensure that the
randSliceandgenerateRandomKeysfunctions are defined and correctly implemented.Verification successful
Helper functions are correctly implemented.
The
randSliceandgenerateRandomKeysfunctions are defined and correctly implemented instore/cachekv/bench_helper_test.go.
randSlicegenerates a random byte slice of a specified size.generateRandomKeyscreates a specified number of random keys usingrandSlice.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the existence and correctness of helper functions. # Test: Search for the function definitions. Expect: Definitions of `randSlice` and `generateRandomKeys`. rg --type go 'func randSlice|func generateRandomKeys'Length of output: 225
Script:
#!/bin/bash # Description: Extract and review the implementations of `randSlice` and `generateRandomKeys`. # Extract the implementation of `randSlice`. rg --type go -A 10 'func randSlice' store/cachekv/bench_helper_test.go # Extract the implementation of `generateRandomKeys`. rg --type go -A 15 'func generateRandomKeys' store/cachekv/bench_helper_test.goLength of output: 617
store/cachekv/store.go (14)
21-27: LGTM!The
NewStoremethod is correctly implemented.
29-32: LGTM!The
GetStoreTypemethod is correctly implemented.
34-45: LGTM!The
Clonemethod is correctly implemented.
47-55: LGTM!The
swapCachemethod is correctly implemented.
57-65: LGTM!The
Restoremethod is correctly implemented.
93-97: LGTM!The
Hasmethod is correctly implemented.
108-122: LGTM!The
Writemethod is correctly implemented.
124-127: LGTM!The
CacheWrapmethod is correctly implemented.
129-132: LGTM!The
CacheWrapWithTracemethod is correctly implemented.
137-145: LGTM!The
IteratorandReverseIteratormethods are correctly implemented.
175-179: LGTM!The
setCacheValuemethod is correctly implemented.
99-106: LGTM! But verify the helper function.The
Deletemethod is correctly implemented.Ensure that the
types.AssertValidKeyfunction is defined and correctly implemented.
82-91: LGTM! But verify the helper functions.The
Setmethod is correctly implemented.Ensure that the
types.AssertValidKeyandtypes.AssertValidValuefunctions are defined and correctly implemented.
67-80: LGTM! But verify the helper function.The
Getmethod is correctly implemented.Ensure that the
types.AssertValidKeyfunction is defined and correctly implemented.store/cachekv/internal/btree_test.go (3)
11-45: LGTM!The test function
TestGetSetDeleteis well-structured and covers various CRUD operations and edge cases forBTree.
47-187: LGTM!The test function
TestDBIteratoris comprehensive and covers various iterator scenarios, including forward and reverse iterators and edge cases.
190-208: LGTM!The helper function
verifyIteratorcorrectly verifies the iterator's output against the expected values.rpc/backend/utils.go (1)
260-270: LGTM! But verify the function usage in the codebase.The code changes are approved.
However, ensure that all function calls to
ParseTxLogsFromEventmatch the new signature.Verification successful
Function usage verified.
The function
ParseTxLogsFromEventis only used within therpc/backend/utils.gofile, and all instances match the new signature.
rpc/backend/utils.go: Lines 260-270Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all function calls to `ParseTxLogsFromEvent` match the new signature. # Test: Search for the function usage. Expect: Only occurances of the new signature. rg --type go -A 5 $'ParseTxLogsFromEvent'Length of output: 994
Script:
#!/bin/bash # Description: Verify if there are any other occurrences of the function `ParseTxLogsFromEvent` in the entire codebase. # Test: Search for the function usage in the entire codebase. Expect: Only occurrences of the new signature. rg 'ParseTxLogsFromEvent'Length of output: 313
store/cachekv/README.md (1)
51-51: Correct grammatical error.Use "an" before "write".
- will need to be updated in the parent `KVStore` upon a write. + will need to be updated in the parent `KVStore` upon an update.Likely invalid or redundant comment.
Tools
LanguageTool
[grammar] ~51-~51: The verb ‘write’ does not usually follow articles like ‘a’. Check that ‘write’ is spelled correctly; using ‘write’ as a noun may be non-standard.
Context: ...be updated in the parentKVStoreupon a write. Keys are mapped to an empty struct to ...(A_INFINITIVE)
go.mod (3)
38-38: LGTM!The new dependency
github.com/tidwall/btreehas been correctly added.
139-139: LGTM!The indirect dependency
github.com/holiman/uint256has been correctly updated.
229-229: LGTM!The replacement of
github.com/ethereum/go-ethereumwithgithub.com/zeta-chain/go-ethereumhas been correctly implemented.app/ante/eth.go (2)
253-253: LGTM! Simplified initialization.The change simplifies the initialization of
evmKeeperwithin the struct creation, improving readability.
308-308: LGTM! Improved readability.The change from
evm.Context().CanTransfertoevm.Context.CanTransferstreamlines the code and improves readability.x/evm/keeper/keeper.go (5)
41-42: LGTM! Useful addition for custom precompiled contracts.The
CustomContractFntype facilitates the customization of precompiled contracts.
44-44: LGTM! Improves readability and modularity.The
EventConvertertype alias improves code readability and modularity.
82-88: LGTM! Restructured for better support of custom contracts and store keys.The changes to the
Keeperstruct support custom contract functions and store keys, improving flexibility and modularity.
102-104: LGTM! Updated function signature.The changes to the
NewKeeperfunction ensure that the new fieldscustomContractFnsandkeysare properly initialized.
134-136: LGTM! Provides access to store keys.The
StoreKeysfunction improves access to store keys in theKeeperstruct.x/evm/keeper/state_transition.go (3)
19-21: LGTM! Necessary imports for new functionality.The added imports for
bytesandsortpackages are necessary for the new functionality in theNewEVMfunction.
Line range hint
55-92: LGTM! Improved handling of precompiled contracts.The changes to the
NewEVMfunction, including the return type and additional contract handling and sorting logic, improve the handling of precompiled contracts.Tools
GitHub Check: gosec
[failure] 77-80: the value in the range statement should be _ unless copying a map: want: for key := range m
expected exactly 1 statement (either append, delete, or copying to another map) in a range with a map, got 2
Line range hint
359-369: LGTM! Streamlined code.The updates to the references from
evm.Config()toevm.Configand fromevm.Context().BlockNumbertoevm.Context.BlockNumberstreamline the code and improve readability.x/evm/statedb/statedb.go (13)
30-31: LGTM! Necessary imports for new functionality.The added imports for
cachemultiandevmtypespackages are necessary for the new functionality in theStateDBstruct.
34-34: LGTM! Improves readability and consistency.The
StateDBContextKeyconstant improves code readability and consistency.
36-36: LGTM! Improves readability and modularity.The
EventConvertertype alias improves code readability and modularity.
Line range hint
54-85: LGTM! Enhanced functionality for state management and event handling.The changes to the
StateDBstruct, including the new fields, enhance its functionality for state management and event handling.Tools
golangci-lint
[warning] 93-93: unused-parameter: parameter 'params' seems to be unused, consider removing or renaming it as _
(revive)
78-78: field
transientStorageis unused(unused)
84-84: field
evmDenomis unused(unused)
85-85: field
erris unused(unused)
GitHub Check: Run golangci-lint
[warning] 93-93:
unused-parameter: parameter 'params' seems to be unused, consider removing or renaming it as _ (revive)
[failure] 78-78:
fieldtransientStorageis unused (unused)
[failure] 84-84:
fieldevmDenomis unused (unused)
[failure] 85-85:
fielderris unused (unused)
93-107: LGTM! Proper initialization of new fields.The changes to the
NewWithParamsfunction ensure proper initialization of the new fields in theStateDBstruct.Tools
golangci-lint
[warning] 93-93: unused-parameter: parameter 'params' seems to be unused, consider removing or renaming it as _
(revive)
GitHub Check: Run golangci-lint
[warning] 93-93:
unused-parameter: parameter 'params' seems to be unused, consider removing or renaming it as _ (revive)
109-111: LGTM! Provides access to native events.The
NativeEventsfunction improves access to native events in theStateDBstruct.
113-117: LGTM! Necessary for managing native cache store.The
cacheMultiStorefunction is necessary for managing the native cache store in theStateDBstruct.
338-340: LGTM! Improves state management and isolation.The
cloneNativeStatefunction improves state management and isolation in theStateDBstruct.
342-345: LGTM! Ensures proper state restoration.The
restoreNativeStatefunction ensures proper state restoration in theStateDBstruct.
347-364: LGTM! Improved handling of native actions and event emission.The
ExecuteNativeActionfunction improves the handling of native actions and event emission in theStateDBstruct.
366-369: LGTM! Improved isolation and management of read-only native actions.The
CacheContextfunction improves the isolation and management of read-only native actions in theStateDBstruct.
524-530: LGTM! Ensures proper state commitment and event emission.The
Commitfunction ensures proper state commitment and event emission in theStateDBstruct.
557-579: LGTM! Improved handling of native events.The
emitNativeEventsfunction improves the handling of native events in theStateDBstruct.store/cachekv/store_test.go (9)
17-22: LGTM!The
newCacheKVStorefunction correctly sets up a two-layer cache store to emulate real-world scenarios.
70-76: LGTM!The
TestCacheKVStoreNoNilSetfunction correctly tests for panics when setting invalid keys or values.
78-105: LGTM!The
TestCacheKVStoreNestedfunction correctly tests nested cache store scenarios and their interactions.
107-160: LGTM!The
TestCacheKVIteratorBoundsfunction correctly tests various iterator bounds scenarios for the cache store.
162-217: LGTM!The
TestCacheKVReverseIteratorBoundsfunction correctly tests various reverse iterator bounds scenarios for the cache store.
219-265: LGTM!The
TestCacheKVMergeIteratorBasicsfunction correctly tests basic merge iterator functionality for the cache store.
267-291: LGTM!The
TestCacheKVMergeIteratorDeleteLastfunction correctly tests merge iterator functionality when deleting the last item.
293-325: LGTM!The
TestCacheKVMergeIteratorDeletesfunction correctly tests merge iterator functionality when deleting items.
327-356: LGTM!The
TestCacheKVMergeIteratorChunksfunction correctly tests merge iterator functionality with chunks of data.x/evm/statedb/statedb_test.go (18)
Line range hint
55-135: LGTM!The
TestAccountfunction correctly tests various account-related scenarios in the state database.
Line range hint
141-150: LGTM!The
TestAccountOverridefunction correctly tests account override scenarios in the state database.
164-172: LGTM!The
TestDBErrorfunction correctly tests error scenarios in the state database.
180-210: LGTM!The
TestBalancefunction correctly tests various balance-related scenarios in the state database.
Line range hint
254-273: LGTM!The
TestStatefunction correctly tests various state-related scenarios in the state database.
Line range hint
298-315: LGTM!The
TestCodefunction correctly tests various code-related scenarios in the state database.
Line range hint
368-399: LGTM!The
TestRevertSnapshotfunction correctly tests various snapshot revert scenarios in the state database.
Line range hint
409-421: LGTM!The
TestNestedSnapshotfunction correctly tests nested snapshot scenarios in the state database.
427-430: LGTM!The
TestInvalidSnapshotIdfunction correctly tests invalid snapshot ID scenarios in the state database.
500-501: LGTM!The
TestAccessListfunction correctly tests various access list scenarios in the state database.
514-515: LGTM!The
TestLogfunction correctly tests various logging scenarios in the state database.
567-568: LGTM!The
TestRefundfunction correctly tests various refund scenarios in the state database.
Line range hint
586-602: LGTM!The
TestIterateStoragefunction correctly tests various storage iteration scenarios in the state database.
614-756: LGTM!The
TestNativeActionfunction correctly tests various native action execution scenarios in the state database.
Line range hint
757-764: LGTM!The
CollectContractStoragefunction correctly collects contract storage from the state database.
773-790: LGTM!The
cloneRawStatefunction correctly clones the raw state from the multi-store.
792-833: LGTM!The
newTestKeeperfunction correctly sets up a new test keeper.
835-854: LGTM!The
setupTestEnvfunction correctly sets up the test environment.app/app.go (2)
473-477: LGTM!The
EvmKeeperinitialization correctly uses the combined keys map and all required parameters are passed.
Line range hint
478-481:
LGTM!The IBC Keeper initialization and configuration are correct.
Tools
GitHub Check: CodeQL
[warning] 465-467: Iteration over map
Iteration over map may be a possible source of non-determinism
[warning] 468-470: Iteration over map
Iteration over map may be a possible source of non-determinism
[warning] 471-473: Iteration over map
Iteration over map may be a possible source of non-determinismCHANGELOG.md (2)
40-42: LGTM!The new feature entry for supporting the precompile interface in the
evmmodule is clear and follows the specified format.
61-62: LGTM!The new improvement entries for initializing precompiled contracts with rules and context in the
precompilemodule are clear and follow the specified format.store/cachemulti/store.go (7)
79-84: LGTM!The function is correctly implemented.
86-100: LGTM!The function is correctly implemented.
102-105: LGTM!The function is correctly implemented.
166-173: LGTM!The function is correctly implemented.
142-148: Optimize map iteration.The map iteration can be optimized by using
_for the value when it is not needed.- for key, store := range cms.stores { + for key := range cms.stores {Likely invalid or redundant comment.
Tools
GitHub Check: gosec
[failure] 142-148: the value in the range statement should be _ unless copying a map: want: for key := range m
expected exactly 1 statement (either append, delete, or copying to another map) in a range with a map, got 3GitHub Check: CodeQL
[warning] 142-148: Iteration over map
Iteration over map may be a possible source of non-determinism
45-54: Optimize map iteration.The map iteration can be optimized by using
_for the value when it is not needed.- for key, store := range stores { + for key := range stores {Likely invalid or redundant comment.
Tools
GitHub Check: gosec
[failure] 45-54: the value in the range statement should be _ unless copying a map: want: for key := range m
expected exactly 1 statement (either append, delete, or copying to another map) in a range with a map, got 2GitHub Check: CodeQL
[warning] 45-54: Iteration over map
Iteration over map may be a possible source of non-determinism
72-74: Optimize map iteration.The map iteration can be optimized by using
_for the value when it is not needed.- for k, v := range cms.stores { + for k := range cms.stores {Likely invalid or redundant comment.
Tools
GitHub Check: CodeQL
[warning] 72-74: Iteration over map
Iteration over map may be a possible source of non-determinismstore/cachekv/internal/mergeiterator.go (10)
27-36: LGTM!The function is correctly implemented.
38-42: LGTM!The function is correctly implemented.
44-47: LGTM!The function is correctly implemented.
49-74: LGTM!The function is correctly implemented.
76-104: LGTM!The function is correctly implemented.
106-134: LGTM!The function is correctly implemented.
136-144: LGTM!The function is correctly implemented.
146-154: LGTM!The function is correctly implemented.
156-162: LGTM!The function is correctly implemented.
164-171: LGTM!The function is correctly implemented.
There was a problem hiding this comment.
Actionable comments posted: 15
Outside diff range, codebase verification and nitpick comments (17)
x/evm/types/interfaces.go (1)
52-53: Missing Implementations forIsSendEnabledCoinsandBlockedAddrMethodsThe methods
IsSendEnabledCoinsandBlockedAddrare declared in theBankKeeperinterface but do not have corresponding implementations in the codebase. This needs to be addressed to ensure the interface is fully functional.
IsSendEnabledCoinsmethod is declared but not implemented.BlockedAddrmethod is declared but not implemented.Please implement these methods to align with the interface requirements.
Analysis chain
Verify the correctness of the new methods in
BankKeeper.Ensure that the new methods
IsSendEnabledCoinsandBlockedAddrare correctly defined and align with the existing interface methods.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the correctness of the new methods in `BankKeeper`. # Test: Search for the definition and usage of `IsSendEnabledCoins` and `BlockedAddr`. Expect: Ensure the methods are correctly defined and used. rg --type go 'IsSendEnabledCoins|BlockedAddr'Length of output: 389
Script:
#!/bin/bash # Description: Verify the implementation and usage of the new methods in `BankKeeper`. # Test: Search for the implementation of `IsSendEnabledCoins` and `BlockedAddr`. rg --type go 'func (.*) IsSendEnabledCoins' -A 10 rg --type go 'func (.*) BlockedAddr' -A 10 # Test: Search for the usage of `IsSendEnabledCoins` and `BlockedAddr` in the codebase. rg --type go 'IsSendEnabledCoins' rg --type go 'BlockedAddr'Length of output: 917
Script:
#!/bin/bash # Description: Verify the implementation of the new methods in `BankKeeper`. # Test: Search for the implementation of `IsSendEnabledCoins`. rg --type go 'func (.*) IsSendEnabledCoins' -A 20 # Test: Search for the implementation of `BlockedAddr`. rg --type go 'func (.*) BlockedAddr' -A 20Length of output: 1028
store/cachekv/store.go (1)
147-170: Consider renamingisoSortedCachefor better readability.The variable name
isoSortedCachecould be improved for readability. Consider renaming it to something more descriptive, such assortedCacheSnapshot.- isoSortedCache := store.cache.Copy() + sortedCacheSnapshot := store.cache.Copy()store/cachekv/README.md (12)
15-15: Consider rephrasing for variety.The phrase "need to be buffered" is used multiple times. Consider rephrasing for variety.
- need to be buffered to the end of the block + need to be buffered until the end of the blockTools
LanguageTool
[style] ~15-~15: You have already used this phrasing in nearby sentences. Consider replacing it to add variety to your writing.
Context: ...tance it's unclear that all disk writes need to be buffered to the end of the block), b...(REP_NEED_TO_VB)
34-34: Use "and" after "both".The correct phrasing is "both keys and values".
- both keys that are cached from read operations as well as ‘dirty’ keys + both keys and values that are cached from read operations as well as ‘dirty’ keysTools
LanguageTool
[grammar] ~34-~34: Probable usage error. Use “and” after ‘both’.
Context: ...ys that are cached from read operations as well as ‘dirty’ keys which map to a value that ...(BOTH_AS_WELL_AS)
[style] ~34-~34: Did you mean ‘different from’? ‘Different than’ is often considered colloquial style.
Context: ...o a value that is potentially different than what is in the underlyingKVStore. V...(DIFFERENT_THAN)
34-34: Consider using "different from".The phrase "different than" is often considered colloquial. Consider using "different from".
- different than what is in the underlying `KVStore` + different from what is in the underlying `KVStore`Tools
LanguageTool
[grammar] ~34-~34: Probable usage error. Use “and” after ‘both’.
Context: ...ys that are cached from read operations as well as ‘dirty’ keys which map to a value that ...(BOTH_AS_WELL_AS)
[style] ~34-~34: Did you mean ‘different from’? ‘Different than’ is often considered colloquial style.
Context: ...o a value that is potentially different than what is in the underlyingKVStore. V...(DIFFERENT_THAN)
51-51: Check the usage of "write".The verb ‘write’ does not usually follow articles like ‘a’. Check that ‘write’ is spelled correctly; using ‘write’ as a noun may be non-standard.
- updated in the parent `KVStore` upon a write + updated in the parent `KVStore` upon writingTools
LanguageTool
[grammar] ~51-~51: The verb ‘write’ does not usually follow articles like ‘a’. Check that ‘write’ is spelled correctly; using ‘write’ as a noun may be non-standard.
Context: ...be updated in the parentKVStoreupon a write. Keys are mapped to an empty struct to ...(A_INFINITIVE)
93-93: Consider a shorter alternative.The phrase "taking into account" might be wordy. Consider a shorter alternative.
- while taking into account updated values from the cache + while considering updated values from the cacheTools
LanguageTool
[style] ~93-~93: ‘taking into account’ might be wordy. Consider a shorter alternative.
Context: ...irs from the underlyingKVStorewhile taking into account updated values from the cache. In the...(EN_WORDINESS_PREMIUM_TAKING_INTO_ACCOUNT)
Markdownlint
93-93: Expected: 0 or 2; Actual: 1
Trailing spaces(MD009, no-trailing-spaces)
134-134: Use "afterward" for American English.In American English, ‘afterward’ is the preferred variant. ‘Afterwards’ is more commonly used in British English and other dialects.
- Afterwards, `unsorted` is sorted. + Afterward, `unsorted` is sorted.Tools
LanguageTool
[locale-violation] ~134-~134: In American English, ‘afterward’ is the preferred variant. ‘Afterwards’ is more commonly used in British English and other dialects.
Context: ...removing each key fromunsortedCache. Afterwards,unsortedis sorted. Lastly, it itera...(AFTERWARDS_US)
140-140: Check the usage of "compute".The verb ‘compute’ does not usually follow articles like ‘the’. Check that ‘compute’ is spelled correctly; using ‘compute’ as a noun may be non-standard.
- To avoid wasting the compute spent sorting + To avoid wasting the computation spent sortingTools
LanguageTool
[grammar] ~140-~140: The verb ‘compute’ does not usually follow articles like ‘the’. Check that ‘compute’ is spelled correctly; using ‘compute’ as a noun may be non-standard.
Context: ...et inunsortedCache. To avoid wasting the compute spent sorting, we should ensure that a ...(A_INFINITIVE)
[uncategorized] ~140-~140: ‘Amount of’ should usually only be used with uncountable or mass nouns. Consider using “number” if this is not the case.
Context: ...ing, we should ensure that a reasonable amount of values are removed from `unsortedCac...(AMOUNTOF_TO_NUMBEROF)
[grammar] ~140-~140: Possible verb agreement error. Did you mean “is”? (Some collective nouns can be treated as both singular and plural, so ‘are’ is not always incorrect.)
Context: ...sure that a reasonable amount of values are removed fromunsortedCache. If the le...(COLLECTIVE_NOUN_VERB_AGREEMENT_VBP)
Markdownlint
140-140: null
Files should end with a single newline character(MD047, single-trailing-newline)
140-140: Use "number of" for countable nouns.‘Amount of’ should usually only be used with uncountable or mass nouns. Consider using “number” if this is not the case.
- a reasonable amount of values + a reasonable number of valuesTools
LanguageTool
[grammar] ~140-~140: The verb ‘compute’ does not usually follow articles like ‘the’. Check that ‘compute’ is spelled correctly; using ‘compute’ as a noun may be non-standard.
Context: ...et inunsortedCache. To avoid wasting the compute spent sorting, we should ensure that a ...(A_INFINITIVE)
[uncategorized] ~140-~140: ‘Amount of’ should usually only be used with uncountable or mass nouns. Consider using “number” if this is not the case.
Context: ...ing, we should ensure that a reasonable amount of values are removed from `unsortedCac...(AMOUNTOF_TO_NUMBEROF)
[grammar] ~140-~140: Possible verb agreement error. Did you mean “is”? (Some collective nouns can be treated as both singular and plural, so ‘are’ is not always incorrect.)
Context: ...sure that a reasonable amount of values are removed fromunsortedCache. If the le...(COLLECTIVE_NOUN_VERB_AGREEMENT_VBP)
Markdownlint
140-140: null
Files should end with a single newline character(MD047, single-trailing-newline)
140-140: Possible verb agreement error.Possible verb agreement error. Did you mean “is”? (Some collective nouns can be treated as both singular and plural, so ‘are’ is not always incorrect.)
- amount of values are removed + amount of values is removedTools
LanguageTool
[grammar] ~140-~140: The verb ‘compute’ does not usually follow articles like ‘the’. Check that ‘compute’ is spelled correctly; using ‘compute’ as a noun may be non-standard.
Context: ...et inunsortedCache. To avoid wasting the compute spent sorting, we should ensure that a ...(A_INFINITIVE)
[uncategorized] ~140-~140: ‘Amount of’ should usually only be used with uncountable or mass nouns. Consider using “number” if this is not the case.
Context: ...ing, we should ensure that a reasonable amount of values are removed from `unsortedCac...(AMOUNTOF_TO_NUMBEROF)
[grammar] ~140-~140: Possible verb agreement error. Did you mean “is”? (Some collective nouns can be treated as both singular and plural, so ‘are’ is not always incorrect.)
Context: ...sure that a reasonable amount of values are removed fromunsortedCache. If the le...(COLLECTIVE_NOUN_VERB_AGREEMENT_VBP)
Markdownlint
140-140: null
Files should end with a single newline character(MD047, single-trailing-newline)
10-10: Fix unordered list indentation.The unordered list indentation is inconsistent. Expected: 4; Actual: 2
- * Buffer all writes to the parent store, so they can be dropped if they need to be reverted - * Allow iteration over contiguous spans of keys + * Buffer all writes to the parent store, so they can be dropped if they need to be reverted + * Allow iteration over contiguous spans of keysAlso applies to: 11-11
Tools
Markdownlint
10-10: Expected: 4; Actual: 2
Unordered list indentation(MD007, ul-indent)
73-73: Remove trailing spaces.Trailing spaces are present. Expected: 0 or 2; Actual: 1
- New values are written by setting or updating the value of a key in `cache`. `Set` does not write to `parent`. + New values are written by setting or updating the value of a key in `cache`. `Set` does not write to `parent`.Also applies to: 79-79, 85-85, 93-93, 132-132, 136-136, 138-138
Tools
Markdownlint
73-73: Expected: 0 or 2; Actual: 1
Trailing spaces(MD009, no-trailing-spaces)
140-140: Ensure file ends with a single newline character.Files should end with a single newline character.
- This amortizes the cost of processing elements across multiple calls. + This amortizes the cost of processing elements across multiple calls. +Tools
LanguageTool
[grammar] ~140-~140: The verb ‘compute’ does not usually follow articles like ‘the’. Check that ‘compute’ is spelled correctly; using ‘compute’ as a noun may be non-standard.
Context: ...et inunsortedCache. To avoid wasting the compute spent sorting, we should ensure that a ...(A_INFINITIVE)
[uncategorized] ~140-~140: ‘Amount of’ should usually only be used with uncountable or mass nouns. Consider using “number” if this is not the case.
Context: ...ing, we should ensure that a reasonable amount of values are removed from `unsortedCac...(AMOUNTOF_TO_NUMBEROF)
[grammar] ~140-~140: Possible verb agreement error. Did you mean “is”? (Some collective nouns can be treated as both singular and plural, so ‘are’ is not always incorrect.)
Context: ...sure that a reasonable amount of values are removed fromunsortedCache. If the le...(COLLECTIVE_NOUN_VERB_AGREEMENT_VBP)
Markdownlint
140-140: null
Files should end with a single newline character(MD047, single-trailing-newline)
x/evm/keeper/keeper.go (1)
102-104: ThecustomContractFnsparameter is missing inevmkeeper.NewKeeperfunction calls.The updated
NewKeeperfunction signature includes thecustomContractFnsparameter, which is not passed in the following instances:
x/evm/statedb/statedb_test.goapp/app.goPlease ensure that the
customContractFnsparameter is correctly passed in these function calls.Analysis chain
LGTM! But verify the usage of
NewKeeperfunction.The update to the function signature is necessary and correctly implemented. Ensure that the function is correctly called with the new parameters.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of `NewKeeper` function in the codebase. # Test: Search for the usage of `NewKeeper`. Expect: Correct initialization and usage with new parameters. rg --type go -A 5 $'NewKeeper'Length of output: 6764
Script:
#!/bin/bash # Description: Verify the usage of `evmkeeper.NewKeeper` function in the codebase with the updated parameters. # Test: Search for the usage of `evmkeeper.NewKeeper` and extract sufficient context to verify parameter usage. rg --type go -A 10 $'evmkeeper.NewKeeper'Length of output: 1359
Script:
#!/bin/bash # Description: Verify the usage of `customContractFns` parameter in `evmkeeper.NewKeeper` function. # Test: Search for the `customContractFns` parameter in the context of `evmkeeper.NewKeeper` usage. rg --type go -A 10 'customContractFns' | rg -B 10 'evmkeeper.NewKeeper'Length of output: 69
Script:
#!/bin/bash # Description: Extract the full function call of `evmkeeper.NewKeeper` in the identified files to verify the parameters. # Test: Extract the full function call of `evmkeeper.NewKeeper` in `x/evm/statedb/statedb_test.go`. rg --type go -A 10 -B 5 'evmkeeper.NewKeeper' x/evm/statedb/statedb_test.go # Test: Extract the full function call of `evmkeeper.NewKeeper` in `app/app.go`. rg --type go -A 10 -B 5 'evmkeeper.NewKeeper' app/app.goLength of output: 1139
store/cachekv/store_test.go (1)
38-38: Fix the typo in the comment.The comment on line 38 has a typo: "shoudn't" should be "shouldn't".
- // update it in cache, shoudn't change mem + // update it in cache, shouldn't change memx/evm/statedb/statedb_test.go (1)
Line range hint
55-127:
Add comments to the test cases for clarity.The test cases in the
TestAccountfunction could benefit from additional comments to explain the purpose of each test case.+ // Test case for non-existent account {"non-exist account", func(db *statedb.StateDB, cms sdk.MultiStore) { suite.Require().Equal(false, db.Exist(address)) suite.Require().Equal(true, db.Empty(address)) suite.Require().Equal(big.NewInt(0), db.GetBalance(address)) suite.Require().Equal([]byte(nil), db.GetCode(address)) suite.Require().Equal(common.Hash{}, db.GetCodeHash(address)) suite.Require().Equal(uint64(0), db.GetNonce(address)) }}, + // Test case for empty account {"empty account", func(db *statedb.StateDB, cms sdk.MultiStore) { db.CreateAccount(address) suite.Require().NoError(db.Commit()) ctx, keeper := newTestKeeper(suite.T(), cms) acct := keeper.GetAccount(ctx, address) suite.Require().Equal(statedb.NewEmptyAccount(), acct) suite.Require().False(acct.IsContract()) db = statedb.New(ctx, keeper, txConfig) suite.Require().Equal(true, db.Exist(address)) suite.Require().Equal(true, db.Empty(address)) suite.Require().Equal(big.NewInt(0), db.GetBalance(address)) suite.Require().Equal([]byte(nil), db.GetCode(address)) suite.Require().Equal(common.BytesToHash(emptyCodeHash), db.GetCodeHash(address)) suite.Require().Equal(uint64(0), db.GetNonce(address)) }}, + // Test case for account suicide {"suicide", func(db *statedb.StateDB, cms sdk.MultiStore) { // non-exist account. suite.Require().False(db.Suicide(address)) suite.Require().False(db.HasSuicided(address)) // create a contract account db.CreateAccount(address) db.SetCode(address, []byte("hello world")) db.AddBalance(address, big.NewInt(100)) db.SetState(address, key1, value1) db.SetState(address, key2, value2) codeHash := db.GetCodeHash(address) suite.Require().NoError(db.Commit()) ctx, keeper := newTestKeeper(suite.T(), cms) suite.Require().NotEmpty(keeper.GetCode(ctx, codeHash)) // suicide db = statedb.New(ctx, keeper, txConfig) suite.Require().False(db.HasSuicided(address)) suite.Require().True(db.Suicide(address)) // check dirty state suite.Require().True(db.HasSuicided(address)) // balance is cleared suite.Require().Equal(big.NewInt(0), db.GetBalance(address)) // but code and state are still accessible in dirty state suite.Require().Equal(value1, db.GetState(address, key1)) suite.Require().Equal([]byte("hello world"), db.GetCode(address)) suite.Require().NoError(db.Commit()) ctx, keeper = newTestKeeper(suite.T(), cms) // not accessible from StateDB anymore db = statedb.New(ctx, keeper, txConfig) suite.Require().False(db.Exist(address)) // and cleared in keeper too suite.Require().Nil(keeper.GetAccount(ctx, address)) // code is not deleted when contract suicided. suite.Require().NotEmpty(keeper.GetCode(ctx, codeHash)) }},
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (3)
go.sumis excluded by!**/*.sumx/evm/migrations/v4/types/evm.pb.gois excluded by!**/*.pb.gox/evm/migrations/v5/types/evm.pb.gois excluded by!**/*.pb.go
Files selected for processing (28)
- CHANGELOG.md (2 hunks)
- app/ante/eth.go (2 hunks)
- app/ante/interfaces.go (2 hunks)
- app/app.go (2 hunks)
- go.mod (5 hunks)
- rpc/backend/utils.go (1 hunks)
- store/cachekv/README.md (1 hunks)
- store/cachekv/bench_helper_test.go (1 hunks)
- store/cachekv/benchmark_test.go (1 hunks)
- store/cachekv/internal/btree.go (1 hunks)
- store/cachekv/internal/btree_test.go (1 hunks)
- store/cachekv/internal/memiterator.go (1 hunks)
- store/cachekv/internal/mergeiterator.go (1 hunks)
- store/cachekv/store.go (1 hunks)
- store/cachekv/store_bench_test.go (1 hunks)
- store/cachekv/store_test.go (1 hunks)
- store/cachemulti/store.go (1 hunks)
- store/cachemulti/store_test.go (1 hunks)
- x/evm/keeper/keeper.go (5 hunks)
- x/evm/keeper/state_transition.go (6 hunks)
- x/evm/statedb/interfaces.go (1 hunks)
- x/evm/statedb/mock_test.go (3 hunks)
- x/evm/statedb/native.go (1 hunks)
- x/evm/statedb/statedb.go (6 hunks)
- x/evm/statedb/statedb_test.go (21 hunks)
- x/evm/statedb/transient_storage.go (1 hunks)
- x/evm/types/interfaces.go (1 hunks)
- x/evm/types/tracer.go (1 hunks)
Additional context used
golangci-lint
x/evm/statedb/transient_storage.go
24-24: type
transientStorageis unused(unused)
27-27: func
newTransientStorageis unused(unused)
32-32: func
transientStorage.Setis unused(unused)
40-40: func
transientStorage.Getis unused(unused)
49-49: func
transientStorage.Copyis unused(unused)
x/evm/statedb/statedb.go
[warning] 93-93: unused-parameter: parameter 'params' seems to be unused, consider removing or renaming it as _
(revive)
78-78: field
transientStorageis unused(unused)
84-84: field
evmDenomis unused(unused)
85-85: field
erris unused(unused)
GitHub Check: gosec
x/evm/statedb/transient_storage.go
[failure] 51-56: the value in the range statement should be _ unless copying a map: want: for key := range m
expected exactly 1 statement (either append, delete, or copying to another map) in a range with a map, got 2store/cachemulti/store.go
[failure] 128-130: the value in the range statement should be _ unless copying a map: want: for key := range m
the value in the range statement should be _ unless copying a map: want: for key := range m
[failure] 119-121: the value in the range statement should be _ unless copying a map: want: for key := range m
the value in the range statement should be _ unless copying a map: want: for key := range m
[failure] 64-66: the value in the range statement should be _ unless copying a map: want: for key := range m
the value in the range statement should be _ unless copying a map: want: for key := range m
[failure] 142-148: the value in the range statement should be _ unless copying a map: want: for key := range m
expected exactly 1 statement (either append, delete, or copying to another map) in a range with a map, got 3
[failure] 45-54: the value in the range statement should be _ unless copying a map: want: for key := range m
expected exactly 1 statement (either append, delete, or copying to another map) in a range with a map, got 2x/evm/keeper/state_transition.go
[failure] 77-80: the value in the range statement should be _ unless copying a map: want: for key := range m
expected exactly 1 statement (either append, delete, or copying to another map) in a range with a map, got 2
GitHub Check: Run golangci-lint
x/evm/statedb/transient_storage.go
[failure] 24-24:
typetransientStorageis unused (unused)
[failure] 27-27:
funcnewTransientStorageis unused (unused)
[failure] 32-32:
functransientStorage.Setis unused (unused)
[failure] 40-40:
functransientStorage.Getis unused (unused)
[failure] 49-49:
functransientStorage.Copyis unused (unused)x/evm/statedb/statedb.go
[warning] 93-93:
unused-parameter: parameter 'params' seems to be unused, consider removing or renaming it as _ (revive)
[failure] 78-78:
fieldtransientStorageis unused (unused)
[failure] 84-84:
fieldevmDenomis unused (unused)
[failure] 85-85:
fielderris unused (unused)
GitHub Check: CodeQL
store/cachemulti/store.go
[warning] 45-54: Iteration over map
Iteration over map may be a possible source of non-determinism
[warning] 64-66: Iteration over map
Iteration over map may be a possible source of non-determinism
[warning] 72-74: Iteration over map
Iteration over map may be a possible source of non-determinism
[warning] 119-121: Iteration over map
Iteration over map may be a possible source of non-determinism
[warning] 128-130: Iteration over map
Iteration over map may be a possible source of non-determinism
[warning] 142-148: Iteration over map
Iteration over map may be a possible source of non-determinismapp/app.go
[warning] 465-467: Iteration over map
Iteration over map may be a possible source of non-determinism
[warning] 468-470: Iteration over map
Iteration over map may be a possible source of non-determinism
[warning] 471-473: Iteration over map
Iteration over map may be a possible source of non-determinism
LanguageTool
store/cachekv/README.md
[style] ~15-~15: You have already used this phrasing in nearby sentences. Consider replacing it to add variety to your writing.
Context: ...tance it's unclear that all disk writes need to be buffered to the end of the block), b...(REP_NEED_TO_VB)
[grammar] ~34-~34: Probable usage error. Use “and” after ‘both’.
Context: ...ys that are cached from read operations as well as ‘dirty’ keys which map to a value that ...(BOTH_AS_WELL_AS)
[style] ~34-~34: Did you mean ‘different from’? ‘Different than’ is often considered colloquial style.
Context: ...o a value that is potentially different than what is in the underlyingKVStore. V...(DIFFERENT_THAN)
[grammar] ~51-~51: The verb ‘write’ does not usually follow articles like ‘a’. Check that ‘write’ is spelled correctly; using ‘write’ as a noun may be non-standard.
Context: ...be updated in the parentKVStoreupon a write. Keys are mapped to an empty struct to ...(A_INFINITIVE)
[style] ~93-~93: ‘taking into account’ might be wordy. Consider a shorter alternative.
Context: ...irs from the underlyingKVStorewhile taking into account updated values from the cache. In the...(EN_WORDINESS_PREMIUM_TAKING_INTO_ACCOUNT)
[locale-violation] ~134-~134: In American English, ‘afterward’ is the preferred variant. ‘Afterwards’ is more commonly used in British English and other dialects.
Context: ...removing each key fromunsortedCache. Afterwards,unsortedis sorted. Lastly, it itera...(AFTERWARDS_US)
[grammar] ~140-~140: The verb ‘compute’ does not usually follow articles like ‘the’. Check that ‘compute’ is spelled correctly; using ‘compute’ as a noun may be non-standard.
Context: ...et inunsortedCache. To avoid wasting the compute spent sorting, we should ensure that a ...(A_INFINITIVE)
[uncategorized] ~140-~140: ‘Amount of’ should usually only be used with uncountable or mass nouns. Consider using “number” if this is not the case.
Context: ...ing, we should ensure that a reasonable amount of values are removed from `unsortedCac...(AMOUNTOF_TO_NUMBEROF)
[grammar] ~140-~140: Possible verb agreement error. Did you mean “is”? (Some collective nouns can be treated as both singular and plural, so ‘are’ is not always incorrect.)
Context: ...sure that a reasonable amount of values are removed fromunsortedCache. If the le...(COLLECTIVE_NOUN_VERB_AGREEMENT_VBP)
Markdownlint
store/cachekv/README.md
10-10: Expected: 4; Actual: 2
Unordered list indentation(MD007, ul-indent)
11-11: Expected: 4; Actual: 2
Unordered list indentation(MD007, ul-indent)
73-73: Expected: 0 or 2; Actual: 1
Trailing spaces(MD009, no-trailing-spaces)
79-79: Expected: 0 or 2; Actual: 1
Trailing spaces(MD009, no-trailing-spaces)
85-85: Expected: 0 or 2; Actual: 1
Trailing spaces(MD009, no-trailing-spaces)
93-93: Expected: 0 or 2; Actual: 1
Trailing spaces(MD009, no-trailing-spaces)
132-132: Expected: 0 or 2; Actual: 1
Trailing spaces(MD009, no-trailing-spaces)
136-136: Expected: 0 or 2; Actual: 1
Trailing spaces(MD009, no-trailing-spaces)
138-138: Expected: 0 or 2; Actual: 1
Trailing spaces(MD009, no-trailing-spaces)
140-140: null
Files should end with a single newline character(MD047, single-trailing-newline)
Additional comments not posted (128)
x/evm/statedb/native.go (3)
10-13: LGTM!The
nativeChangestruct is well-defined and appropriate for its intended functionality.
15-17: LGTM!The
Dirtiedmethod is straightforward and correctly implemented.
19-21: LGTM!The
Revertmethod is correctly implemented and aligns with the intended functionality.store/cachemulti/store_test.go (1)
13-24: LGTM!The
TestStoreGetKVStorefunction is well-defined and correctly tests theGetStoreandGetKVStoremethods.store/cachekv/bench_helper_test.go (4)
5-8: LGTM!The
randSlicefunction correctly generates a random byte slice of the given size.
11-19: LGTM!The
incrementByteSlicefunction correctly implements the byte slice increment operation.
23-33: LGTM!The
generateSequentialKeysfunction correctly generates sequential keys starting from the given key.
37-43: LGTM!The
generateRandomKeysfunction correctly generates random, unsorted keys.x/evm/statedb/interfaces.go (2)
19-22: LGTM!The import statements are appropriate for the changes made in the file.
27-29: LGTM!The
StoreKeysandGetParamsmethods are well-defined and appropriate for theKeeperinterface.x/evm/statedb/transient_storage.go (4)
40-45: Flag unused method.The
Getmethod is currently unused. If this is intentional, consider adding a comment explaining why. Otherwise, ensure it is utilized within the codebase.Tools
golangci-lint
40-40: func
transientStorage.Getis unused(unused)
GitHub Check: Run golangci-lint
[failure] 40-40:
functransientStorage.Getis unused (unused)
27-27: Flag unused function.The
newTransientStoragefunction is currently unused. If this is intentional, consider adding a comment explaining why. Otherwise, ensure it is utilized within the codebase.Verification successful
Flag unused function.
The
newTransientStoragefunction is currently unused. If this is intentional, consider adding a comment explaining why. Otherwise, ensure it is utilized within the codebase.
newTransientStoragefunction definition found inx/evm/statedb/transient_storage.gobut no usage detected.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify if `newTransientStorage` is used in the codebase. # Test: Search for the function usage. Expect: No occurrences or valid usage. rg --type go -A 5 $'newTransientStorage'Length of output: 583
Tools
golangci-lint
27-27: func
newTransientStorageis unused(unused)
GitHub Check: Run golangci-lint
[failure] 27-27:
funcnewTransientStorageis unused (unused)
24-24: Flag unused type.The
transientStoragetype is currently unused. If this is intentional, consider adding a comment explaining why. Otherwise, ensure it is utilized within the codebase.Tools
golangci-lint
24-24: type
transientStorageis unused(unused)
GitHub Check: Run golangci-lint
[failure] 24-24:
typetransientStorageis unused (unused)
32-37: Flag unused method.The
Setmethod is currently unused. If this is intentional, consider adding a comment explaining why. Otherwise, ensure it is utilized within the codebase.Verification successful
Flag unused method.
The
Setmethod is currently unused. If this is intentional, consider adding a comment explaining why. Otherwise, ensure it is utilized within the codebase.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify if `Set` method is used in the codebase. # Test: Search for the method usage. Expect: No occurrences or valid usage. rg --type go -A 5 $'Set'Length of output: 264646
Tools
golangci-lint
32-32: func
transientStorage.Setis unused(unused)
GitHub Check: Run golangci-lint
[failure] 32-32:
functransientStorage.Setis unused (unused)app/ante/interfaces.go (1)
45-45: Verify method usage forNewEVM.The
NewEVMmethod now returns a pointer tovm.EVM. Ensure that all usages of this method are updated to handle the new return type.store/cachekv/internal/memiterator.go (9)
25-65: LGTM!The
newMemIteratorfunction is correctly implemented and follows best practices.
67-69: LGTM!The
Domainmethod is correctly implemented and follows best practices.
71-73: LGTM!The
Closemethod is correctly implemented and follows best practices.
76-80: LGTM!The
Errormethod is correctly implemented and follows best practices.
83-85: LGTM!The
Validmethod is correctly implemented and follows best practices.
87-99: LGTM!The
Nextmethod is correctly implemented and follows best practices.
101-109: LGTM!The
keyInRangemethod is correctly implemented and follows best practices.
111-113: LGTM!The
Keymethod is correctly implemented and follows best practices.
115-117: LGTM!The
Valuemethod is correctly implemented and follows best practices.store/cachekv/internal/btree.go (10)
28-31: LGTM!The
NewBTreefunction is correctly implemented and follows best practices.
33-38: LGTM!The
initmethod is correctly implemented and follows best practices.
40-45: LGTM!The
Setmethod is correctly implemented and follows best practices.
47-56: LGTM!The
Getmethod is correctly implemented and follows best practices.
59-63: LGTM!The
Deletemethod is correctly implemented and follows best practices.
66-70: LGTM!The
Iteratormethod is correctly implemented and follows best practices.
73-77: LGTM!The
ReverseIteratormethod is correctly implemented and follows best practices.
80-91: LGTM!The
ScanDirtyItemsmethod is correctly implemented and follows best practices.
94-103: LGTM!The
Copymethod is correctly implemented and follows best practices.
106-110: LGTM!The
itemtype is correctly defined and follows best practices.x/evm/types/tracer.go (1)
48-48: Verify the correctness of usingvm.DefaultActivePrecompiles.Ensure that
vm.DefaultActivePrecompilesis the intended replacement forvm.ActivePrecompilesand that it correctly initializes thepreCompiles.x/evm/statedb/mock_test.go (5)
30-31: Verify the correctness of the new fields inMockKeeper.Ensure that the new fields
keysandeventConvertersare correctly added and do not introduce any issues.Verification successful
Verify the correctness of the new fields in
MockKeeper.The fields
keysandeventConvertersare correctly added and utilized in thex/evm/statedb/mock_test.gofile.
keysis used in theNewMockKeeperWithfunction and other places within the same file.eventConvertersis also used in theNewMockKeeperWithfunction and other places within the same file.These additions appear to be correct and do not introduce any issues.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the correctness of the new fields in `MockKeeper`. # Test: Search for the definition and usage of `keys` and `eventConverters`. Expect: Ensure the fields are correctly added and used. rg --type go 'keys|eventConverters'Length of output: 14871
47-49: Verify the correctness of the new methodGetParams.Ensure that the new method
GetParamsis correctly defined and used.
135-143: Verify the correctness of the updated methodClone.Ensure that the updated method
Cloneis correctly defined and used.
43-45: Verify the correctness of the refactored constructorNewMockKeeper.Ensure that the refactored constructor
NewMockKeeperis correctly defined and used.Verification successful
The refactored constructor
NewMockKeeperis correctly defined and used.
- The
NewMockKeeperfunction callsNewMockKeeperWithwithnilarguments, which is handled correctly.- The
NewMockKeeperWithfunction initializes theMockKeeperwith default values whennilarguments are provided.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the correctness of the refactored constructor `NewMockKeeper`. # Test: Search for the definition and usage of `NewMockKeeper`. Expect: Ensure the constructor is correctly defined and used. rg --type go 'NewMockKeeper'Length of output: 305
Script:
#!/bin/bash # Description: Verify the implementation of `NewMockKeeperWith` and the usage of `NewMockKeeper`. # Search for the implementation of `NewMockKeeperWith`. rg --type go 'func NewMockKeeperWith' -A 10 # Search for the usage of `NewMockKeeper`. rg --type go 'NewMockKeeper\(\)'Length of output: 828
34-39: Verify the correctness of the new constructorNewMockKeeperWith.Ensure that the new constructor
NewMockKeeperWithis correctly defined and used.Verification successful
The constructor
NewMockKeeperWithis correctly defined and used withinmock_test.go.However, there are no direct test functions in
mock_test.goto validate its correctness. Ensure that this constructor is adequately tested in the future to avoid potential issues.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the correctness of the new constructor `NewMockKeeperWith`. # Test: Search for the definition and usage of `NewMockKeeperWith`. Expect: Ensure the constructor is correctly defined and used. rg --type go 'NewMockKeeperWith'Length of output: 247
Script:
#!/bin/bash # Description: Verify the correctness of the new constructor `NewMockKeeperWith`. # Test: Search for additional usages of `NewMockKeeperWith` across the entire codebase. rg 'NewMockKeeperWith'Length of output: 237
Script:
#!/bin/bash # Description: Verify the test cases in `mock_test.go` to ensure they are correctly defined and executed. # Test: Search for test functions in `mock_test.go`. rg --type go 'func Test' x/evm/statedb/mock_test.goLength of output: 53
Script:
#!/bin/bash # Description: List all functions in `mock_test.go` to identify any custom test functions or other relevant code. # List all functions in `mock_test.go`. rg --type go --no-heading --line-number --column 'func ' x/evm/statedb/mock_test.goLength of output: 1297
Script:
#!/bin/bash # Description: Search for test functions across the entire codebase to ensure that the `NewMockKeeperWith` constructor is adequately tested. # Search for test functions across the entire codebase. rg --type go --no-heading --line-number --column 'func Test'Length of output: 11777
store/cachekv/benchmark_test.go (6)
125-133: Verify the correctness of the methodRevertToSnapshot.Ensure that the method
RevertToSnapshotis correctly defined and does not introduce any issues.
13-45: Verify the correctness of the benchmark testDoBenchmarkDeepCacheStack.Ensure that the benchmark test
DoBenchmarkDeepCacheStackis correctly defined and does not introduce any issues.Verification successful
The benchmark test
DoBenchmarkDeepCacheStackis correctly defined and used.The
CacheStackstruct and its methodsReset,Snapshot, andCurrentStoreare appropriately defined and utilized within the benchmark test. No issues were found.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the correctness of the benchmark test `DoBenchmarkDeepCacheStack`. # Test: Search for the definition and usage of `DoBenchmarkDeepCacheStack`. Expect: Ensure the benchmark test is correctly defined and used. rg --type go 'DoBenchmarkDeepCacheStack'Length of output: 392
Script:
#!/bin/bash # Description: Verify the definition and usage of `CacheStack` and its methods `Reset`, `Snapshot`, and `CurrentStore`. # Test: Search for the definition and usage of `CacheStack`. rg --type go 'CacheStack' # Test: Search for the definition and usage of `Reset` method. rg --type go 'Reset' # Test: Search for the definition and usage of `Snapshot` method. rg --type go 'Snapshot' # Test: Search for the definition and usage of `CurrentStore` method. rg --type go 'CurrentStore'Length of output: 16004
47-49: Verify the correctness of the benchmark testBenchmarkDeepCacheStack1.Ensure that the benchmark test
BenchmarkDeepCacheStack1is correctly defined and does not introduce any issues.Verification successful
Verify the correctness of the
DoBenchmarkDeepCacheStackfunction used inBenchmarkDeepCacheStack1.Ensure that the
DoBenchmarkDeepCacheStackfunction is correctly defined and used.
Verify the implementation of the
DoBenchmarkDeepCacheStackfunction.Ensure that the
DoBenchmarkDeepCacheStackfunction is correctly implemented and does not introduce any issues.
The benchmark test
BenchmarkDeepCacheStack1is correctly defined and does not introduce any issues.
- The
DoBenchmarkDeepCacheStackfunction is correctly implemented and used withinBenchmarkDeepCacheStack1.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the correctness of the benchmark test `BenchmarkDeepCacheStack1`. # Test: Search for the definition and usage of `BenchmarkDeepCacheStack1`. Expect: Ensure the benchmark test is correctly defined and used. rg --type go 'BenchmarkDeepCacheStack1'Length of output: 275
Script:
#!/bin/bash # Description: Verify the correctness of the `DoBenchmarkDeepCacheStack` function used in `BenchmarkDeepCacheStack1`. # Test: Search for the definition and usage of `DoBenchmarkDeepCacheStack`. Expect: Ensure the function is correctly defined and used. rg --type go 'DoBenchmarkDeepCacheStack'Length of output: 392
Script:
#!/bin/bash # Description: Verify the implementation of the `DoBenchmarkDeepCacheStack` function. # Test: Retrieve the implementation of `DoBenchmarkDeepCacheStack`. Expect: Ensure the function is correctly implemented. ast-grep --lang go --pattern 'func DoBenchmarkDeepCacheStack(b *testing.B, depth int) { $$$ }'Length of output: 2065
55-57: Verify the correctness of the benchmark testBenchmarkDeepCacheStack10.Ensure that the benchmark test
BenchmarkDeepCacheStack10is correctly defined and does not introduce any issues.Verification successful
The benchmark test
BenchmarkDeepCacheStack10is correctly defined and does not introduce any issues.
- The function
DoBenchmarkDeepCacheStackis correctly implemented for benchmarking.- The function
BenchmarkDeepCacheStack10correctly callsDoBenchmarkDeepCacheStackwith the appropriate depth.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the correctness of the benchmark test `BenchmarkDeepCacheStack10`. # Test: Search for the definition and usage of `BenchmarkDeepCacheStack10`. Expect: Ensure the benchmark test is correctly defined and used. rg --type go 'BenchmarkDeepCacheStack10'Length of output: 119
Script:
#!/bin/bash # Description: Verify the implementation and usage of `DoBenchmarkDeepCacheStack`. # Test: Search for the definition and usage of `DoBenchmarkDeepCacheStack`. Expect: Ensure the function is correctly defined and used. rg --type go 'DoBenchmarkDeepCacheStack'Length of output: 392
Script:
#!/bin/bash # Description: Extract the implementation details of `DoBenchmarkDeepCacheStack`. # Extract the implementation of `DoBenchmarkDeepCacheStack` from `store/cachekv/benchmark_test.go`. rg --type go -A 20 'func DoBenchmarkDeepCacheStack' store/cachekv/benchmark_test.goLength of output: 575
Script:
#!/bin/bash # Description: Extract the complete implementation details of `DoBenchmarkDeepCacheStack`. # Extract more lines from the implementation of `DoBenchmarkDeepCacheStack` from `store/cachekv/benchmark_test.go`. rg --type go -A 40 'func DoBenchmarkDeepCacheStack' store/cachekv/benchmark_test.goLength of output: 995
59-61: Verify the correctness of the benchmark testBenchmarkDeepCacheStack13.Ensure that the benchmark test
BenchmarkDeepCacheStack13is correctly defined and does not introduce any issues.
51-53: Verify the correctness of the benchmark testBenchmarkDeepCacheStack3.Ensure that the benchmark test
BenchmarkDeepCacheStack3is correctly defined and does not introduce any issues.store/cachekv/store_bench_test.go (4)
46-64: Ensure the availability ofrandSliceandgenerateSequentialKeysfunctions.The functions
randSliceandgenerateSequentialKeysare used but not defined in this file. Ensure they are available in the codebase.
98-133: Ensure the availability ofrandSliceandgenerateSequentialKeysfunctions.The functions
randSliceandgenerateSequentialKeysare used but not defined in this file. Ensure they are available in the codebase.
17-43: Ensure the availability ofrandSliceandgenerateSequentialKeysfunctions.The functions
randSliceandgenerateSequentialKeysare used but not defined in this file. Ensure they are available in the codebase.Verification successful
Verified: The functions
randSliceandgenerateSequentialKeysare available in the codebase.The functions
randSliceandgenerateSequentialKeysare defined instore/cachekv/bench_helper_test.go.
randSlice: Defined atstore/cachekv/bench_helper_test.go:1generateSequentialKeys: Defined atstore/cachekv/bench_helper_test.go:2Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the availability of `randSlice` and `generateSequentialKeys` functions. # Test: Search for the function definitions. Expect: Definitions of `randSlice` and `generateSequentialKeys`. rg --type go 'func randSlice|func generateSequentialKeys'Length of output: 237
68-92: Ensure the availability ofrandSliceandgenerateRandomKeysfunctions.The functions
randSliceandgenerateRandomKeysare used but not defined in this file. Ensure they are available in the codebase.Verification successful
The functions
randSliceandgenerateRandomKeysare available in the codebase.
randSliceis defined instore/cachekv/bench_helper_test.go.generateRandomKeysis defined instore/cachekv/bench_helper_test.go.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the availability of `randSlice` and `generateRandomKeys` functions. # Test: Search for the function definitions. Expect: Definitions of `randSlice` and `generateRandomKeys`. rg --type go 'func randSlice|func generateRandomKeys'Length of output: 225
store/cachekv/store.go (12)
21-27: LGTM!The
NewStorefunction correctly initializes theStoreobject.
29-32: LGTM!The
GetStoreTypefunction correctly returns the store type.
34-45: LGTM!The
Clonefunction correctly creates a snapshot of the cache store.
47-55: LGTM!The
swapCachefunction correctly swaps out the internal cache store.
57-65: LGTM!The
Restorefunction correctly restores the store cache to a given snapshot.
67-80: LGTM!The
Getfunction correctly retrieves the value from the store.
82-91: LGTM!The
Setfunction correctly sets the value in the store.
93-97: LGTM!The
Hasfunction correctly checks the existence of the key in the store.
99-106: LGTM!The
Deletefunction correctly deletes the key from the store.
108-122: LGTM!The
Writefunction correctly writes the cache to the parent store.
124-127: LGTM!The
CacheWrapfunction correctly wraps the store in a cache.
129-132: LGTM!The
CacheWrapWithTracefunction correctly wraps the store in a cache with tracing.go.mod (3)
38-38: LGTM! Dependency addition approved.The addition of
github.com/tidwall/btreeis necessary for the new BTree implementation.
139-139: LGTM! Dependency re-addition approved.The re-addition of
github.com/holiman/uint256as an indirect dependency is acceptable.
229-229: LGTM! Dependency replacement approved.The replacement of
github.com/ethereum/go-ethereumwithgithub.com/zeta-chain/go-ethereum v1.10.26-spcis necessary for the new stateful precompiles support.app/ante/eth.go (2)
253-253: LGTM!The initialization logic for
NewCanTransferDecoratoris straightforward and correct.
308-308: LGTM! But verify the usage ofevm.Context.The change improves readability by simplifying the method call. Ensure that
evm.Contextis correctly initialized and used.x/evm/keeper/keeper.go (4)
41-42: LGTM!The addition of
CustomContractFnis straightforward and correct.
44-44: LGTM!The addition of
EventConverteris straightforward and correct.
82-88: LGTM!The addition of
customContractFnsandkeysfields in theKeeperstruct is necessary for the new functionality and is correctly implemented.
134-136: LGTM!The addition of the
StoreKeysmethod is straightforward and correct.x/evm/keeper/state_transition.go (5)
19-21: LGTM!The addition of
bytesandsortimports is necessary and correct.
55-55: LGTM!The modification to use a pointer type for
evm.EVMis necessary and correct.
359-359: LGTM!The update to references from
evm.Config()toevm.Configsimplifies the code and improves readability.
369-369: LGTM!The update to references from
evm.Context().BlockNumbertoevm.Context.BlockNumbersimplifies the code and improves readability.
74-92: LGTM! But verify the functionality ofNewEVMfunction.The update to the
NewEVMfunction improves the handling of custom precompiled contracts and ensures that active contracts are sorted. Ensure that the logic is correctly implemented and does not introduce any issues.Tools
GitHub Check: gosec
[failure] 77-80: the value in the range statement should be _ unless copying a map: want: for key := range m
expected exactly 1 statement (either append, delete, or copying to another map) in a range with a map, got 2x/evm/statedb/statedb.go (8)
30-31: LGTM!The addition of
cachemultiandevmtypesimports is necessary and correct.
34-34: LGTM!The addition of
StateDBContextKeyconstant is necessary and correct.
36-36: LGTM!The addition of
EventConverteris straightforward and correct.
Line range hint
54-85: LGTM!The modifications and additions to the
StateDBstruct are necessary and correctly implemented.Tools
golangci-lint
[warning] 93-93: unused-parameter: parameter 'params' seems to be unused, consider removing or renaming it as _
(revive)
78-78: field
transientStorageis unused(unused)
84-84: field
evmDenomis unused(unused)
85-85: field
erris unused(unused)
GitHub Check: Run golangci-lint
[warning] 93-93:
unused-parameter: parameter 'params' seems to be unused, consider removing or renaming it as _ (revive)
[failure] 78-78:
fieldtransientStorageis unused (unused)
[failure] 84-84:
fieldevmDenomis unused (unused)
[failure] 85-85:
fielderris unused (unused)
Line range hint
109-369: LGTM!The addition of new methods in the
StateDBstruct is necessary and correctly implemented.
524-530: LGTM!The modifications to the
Commitmethod are necessary and correctly implemented.
557-579: LGTM!The addition of
emitNativeEventsmethod is necessary and correctly implemented.
90-107: LGTM! But verify the usage ofNewWithParamsfunction.The refactoring of the
Newfunction toNewWithParamsis necessary and correctly implemented. Ensure that the function is correctly called with the new argument.Verification successful
Verified: The
NewWithParamsfunction is correctly refactored and used within the codebase.The function
NewWithParamsis only used withinx/evm/statedb/statedb.go, ensuring that the refactoring does not impact other parts of the codebase.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of `NewWithParams` function in the codebase. # Test: Search for the usage of `NewWithParams`. Expect: Correct initialization and usage with new argument. rg --type go -A 5 $'NewWithParams'Length of output: 593
Script:
#!/bin/bash # Description: Verify the usage of `NewWithParams` function across the entire codebase. # Test: Search for the usage of `NewWithParams`. Expect: Correct initialization and usage with new argument. rg --type go 'NewWithParams'Length of output: 251
Tools
golangci-lint
[warning] 93-93: unused-parameter: parameter 'params' seems to be unused, consider removing or renaming it as _
(revive)
GitHub Check: Run golangci-lint
[warning] 93-93:
unused-parameter: parameter 'params' seems to be unused, consider removing or renaming it as _ (revive)store/cachekv/store_test.go (20)
17-22: LGTM!The
newCacheKVStorefunction correctly initializes a two-layer cache store.
24-25: LGTM!The
keyFmtandvalFmtfunctions correctly format keys and values as byte slices.
70-75: LGTM!The
TestCacheKVStoreNoNilSetfunction correctly tests that setting nil values panics.
78-105: LGTM!The
TestCacheKVStoreNestedfunction correctly tests nested cache store operations and their effects.
162-217: LGTM!The
TestCacheKVReverseIteratorBoundsfunction correctly tests reverse iterator bounds in theCacheKVStore.
219-265: LGTM!The
TestCacheKVMergeIteratorBasicsfunction correctly tests basic merge iterator operations in theCacheKVStore.
267-291: LGTM!The
TestCacheKVMergeIteratorDeleteLastfunction correctly tests deleting the last item in theCacheKVStore.
293-325: LGTM!The
TestCacheKVMergeIteratorDeletesfunction correctly tests deleting items in theCacheKVStore.
327-356: LGTM!The
TestCacheKVMergeIteratorChunksfunction correctly tests merge iterator operations in chunks.
358-375: LGTM!The
TestCacheKVMergeIteratorDomainfunction correctly tests the domain of merge iterators.
377-390: LGTM!The
TestCacheKVMergeIteratorRandomfunction correctly tests random operations on merge iterators.
392-435: LGTM!The
TestNilEndIteratorfunction correctly tests iterators with nil end bounds.
437-451: LGTM!The
TestIteratorDeadlockfunction correctly tests for deadlock issues in iterators.
471-494: LGTM!The
doOpfunction correctly performs random operations on theCacheKVStore.
496-519: LGTM!The
doRandomOpfunction correctly performs random operations on theCacheKVStore.
524-536: LGTM!The
assertIterateDomainfunction correctly asserts the iteration over the whole domain.
538-569: LGTM!The
assertIterateDomainCheckfunction correctly checks the iteration over the domain against a ground truth.
571-580: LGTM!The
assertIterateDomainComparefunction correctly compares the iteration over the domain against a ground truth.
582-593: LGTM!The
checkIteratorsfunction correctly checks the equality of two iterators.
128-135: Verify the iteration over none scenario.Ensure that the iteration over none scenario correctly handles cases where no keys are present.
Verification successful
The iteration over none scenario is correctly handled in the
TestCacheKVIteratorBoundsfunction.The test ensures that when no keys are present, the iterator does not find any keys and the iteration count is zero, with no errors occurring when closing the iterator.
- Lines: 128-135 in
store/cachekv/store_test.goScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the iteration over none scenario in `TestCacheKVIteratorBounds`. # Test: Search for the function usage. Expect: Correct handling of no keys. rg --type go -A 5 $'TestCacheKVIteratorBounds'Length of output: 349
Script:
#!/bin/bash # Extract the complete implementation of the `TestCacheKVIteratorBounds` function from `store/cachekv/store_test.go`. ast-grep --lang go --pattern $'func TestCacheKVIteratorBounds($_) { $$$ }' --after 100Length of output: 8684
x/evm/statedb/statedb_test.go (1)
Line range hint
141-151:
LGTM!The
TestAccountOverridefunction correctly tests the balance carryover and nonce reset when an account is overwritten.app/app.go (4)
477-477: Verify the correctness ofallKeysinitialization.Ensure that the
allKeysmap contains all expected keys fromkeys,tkeys, andmemKeysbefore initializing theEvmKeeper.
465-467: Duplicate Comment: Potential non-determinism in map iteration.Iteration over maps may lead to non-deterministic order, which can be problematic in consensus-critical code. Consider sorting the keys before iteration to ensure deterministic behavior.
Tools
GitHub Check: CodeQL
[warning] 465-467: Iteration over map
Iteration over map may be a possible source of non-determinism
468-470: Duplicate Comment: Potential non-determinism in map iteration.Iteration over maps may lead to non-deterministic order, which can be problematic in consensus-critical code. Consider sorting the keys before iteration to ensure deterministic behavior.
Tools
GitHub Check: CodeQL
[warning] 468-470: Iteration over map
Iteration over map may be a possible source of non-determinism
471-473: Duplicate Comment: Potential non-determinism in map iteration.Iteration over maps may lead to non-deterministic order, which can be problematic in consensus-critical code. Consider sorting the keys before iteration to ensure deterministic behavior.
Tools
GitHub Check: CodeQL
[warning] 471-473: Iteration over map
Iteration over map may be a possible source of non-determinismCHANGELOG.md (2)
40-43: LGTM!The new feature entry for supporting the precompile interface in the
evmmodule is clear and aligns with the PR objectives.
61-62: LGTM!The improvement entries for allowing the initialization of precompiled contracts with rules and context are clear and align with the PR objectives.
store/cachemulti/store.go (10)
79-84: LGTM!The method is straightforward and correctly sets the tracer for the
MultiStore.
102-105: LGTM!The method is straightforward and correctly returns whether tracing is enabled for the
MultiStore.
107-110: LGTM!The method correctly panics as this functionality is not supported for a cached multi-store.
112-115: LGTM!The method is straightforward and correctly returns the type of the store.
151-154: LGTM!The method is straightforward and correctly implements
CacheWrapper.
156-159: LGTM!The method is straightforward and correctly implements
CacheWrapperwith tracing.
161-164: LGTM!The method is straightforward and correctly implements
MultiStore.
166-173: LGTM!The method correctly panics as this functionality is not supported for a cached multi-store.
175-182: LGTM!The method is straightforward and correctly returns an underlying
Storeby key.
184-191: LGTM!The method is straightforward and correctly returns an underlying
KVStoreby key.store/cachekv/internal/mergeiterator.go (7)
27-36: LGTM!The method is straightforward and correctly creates a new
cacheMergeIteratorobject.
38-42: LGTM!The method is straightforward and correctly returns the domain of the parent iterator.
44-47: LGTM!The method is straightforward and correctly returns whether the iterator is valid.
136-144: LGTM!The method is straightforward and correctly closes the iterator.
146-154: LGTM!The method is straightforward and correctly returns an error if the iterator is invalid.
156-162: LGTM!The method is straightforward and correctly asserts that the iterator is valid.
164-171: LGTM!The method is straightforward and correctly compares two byte
Signed-off-by: Francisco de Borja Aranda Castillejo <borja@zetachain.com>
Signed-off-by: Francisco de Borja Aranda Castillejo <borja@zetachain.com>
There was a problem hiding this comment.
Actionable comments posted: 14
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- store/cachekv/README.md (1 hunks)
- x/evm/statedb/statedb.go (6 hunks)
- x/evm/statedb/transient_storage.go (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- x/evm/statedb/statedb.go
Additional context used
GitHub Check: gosec
x/evm/statedb/transient_storage.go
[failure] 56-61: the value in the range statement should be _ unless copying a map: want: for key := range m
expected exactly 1 statement (either append, delete, or copying to another map) in a range with a map, got 2
LanguageTool
store/cachekv/README.md
[style] ~15-~15: You have already used this phrasing in nearby sentences. Consider replacing it to add variety to your writing.
Context: ...tance it's unclear that all disk writes need to be buffered to the end of the block), b...(REP_NEED_TO_VB)
[grammar] ~34-~34: Probable usage error. Use “and” after ‘both’.
Context: ...ys that are cached from read operations as well as ‘dirty’ keys which map to a value that ...(BOTH_AS_WELL_AS)
[style] ~34-~34: Did you mean ‘different from’? ‘Different than’ is often considered colloquial style.
Context: ...o a value that is potentially different than what is in the underlyingKVStore. V...(DIFFERENT_THAN)
[grammar] ~51-~51: The verb ‘write’ does not usually follow articles like ‘a’. Check that ‘write’ is spelled correctly; using ‘write’ as a noun may be non-standard.
Context: ...be updated in the parentKVStoreupon a write. Keys are mapped to an empty struct to ...(A_INFINITIVE)
[style] ~93-~93: ‘taking into account’ might be wordy. Consider a shorter alternative.
Context: ...irs from the underlyingKVStorewhile taking into account updated values from the cache. In the ...(EN_WORDINESS_PREMIUM_TAKING_INTO_ACCOUNT)
[locale-violation] ~134-~134: In American English, ‘afterward’ is the preferred variant. ‘Afterwards’ is more commonly used in British English and other dialects.
Context: ...removing each key fromunsortedCache. Afterwards,unsortedis sorted. Lastly, it itera...(AFTERWARDS_US)
[grammar] ~140-~140: The verb ‘compute’ does not usually follow articles like ‘the’. Check that ‘compute’ is spelled correctly; using ‘compute’ as a noun may be non-standard.
Context: ...et inunsortedCache. To avoid wasting the compute spent sorting, we should ensure that a ...(A_INFINITIVE)
[uncategorized] ~140-~140: ‘Amount of’ should usually only be used with uncountable or mass nouns. Consider using “number” if this is not the case.
Context: ...ing, we should ensure that a reasonable amount of values are removed from `unsortedCac...(AMOUNTOF_TO_NUMBEROF)
[grammar] ~140-~140: Possible verb agreement error. Did you mean “is”? (Some collective nouns can be treated as both singular and plural, so ‘are’ is not always incorrect.)
Context: ...sure that a reasonable amount of values are removed fromunsortedCache. If the le...(COLLECTIVE_NOUN_VERB_AGREEMENT_VBP)
|
Signed-off-by: Francisco de Borja Aranda Castillejo <borja@zetachain.com>
There was a problem hiding this comment.
Uses zeta-chain/go-ethereum@v1.10.26-spc which enables stateful precompiles.
Could we create a branch release/v.x and make a PR to this branch for these changes? We don't need to merge it immediately and can instead use commit as a tag in this repo, just so we have every changes made tracked through PRs
chore: adapted CODEOWNERS to protocol groups @ ZETA
Sounds good. In this case it is fine but in general it's a best practice to create a new PR for any independent changes even if it's just one line change
Co-authored-by: Lucas Bertrand <lucas.bertrand.22@gmail.com>
Signed-off-by: Francisco de Borja Aranda Castillejo <borja@zetachain.com>
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range, codebase verification and nitpick comments (1)
x/evm/keeper/state_transition.go (1)
Line range hint
55-108:
LGTM! But address the static analysis hint.The changes to the
NewEVMfunction are approved.However, the static analysis tool suggests using
_for the value in the range statement unless copying a map. Consider updating the loop to:for addr := range vm.DefaultPrecompiles(rules) { contracts[addr] = vm.DefaultPrecompiles(rules)[addr] active = append(active, addr) }- for addr, c := range vm.DefaultPrecompiles(rules) { - contracts[addr] = c - active = append(active, addr) - } + for addr := range vm.DefaultPrecompiles(rules) { + contracts[addr] = vm.DefaultPrecompiles(rules)[addr] + active = append(active, addr) + }Tools
GitHub Check: gosec
[failure] 82-85: the value in the range statement should be _ unless copying a map: want: for key := range m
expected exactly 1 statement (either append, delete, or copying to another map) in a range with a map, got 2
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- x/evm/keeper/keeper.go (5 hunks)
- x/evm/keeper/state_transition.go (6 hunks)
- x/evm/statedb/statedb.go (6 hunks)
Files skipped from review as they are similar to previous changes (1)
- x/evm/statedb/statedb.go
Additional context used
GitHub Check: gosec
x/evm/keeper/state_transition.go
[failure] 82-85: the value in the range statement should be _ unless copying a map: want: for key := range m
expected exactly 1 statement (either append, delete, or copying to another map) in a range with a map, got 2
Additional comments not posted (2)
x/evm/keeper/keeper.go (2)
137-139: LGTM!The
StoreKeysfunction is a simple getter for thekeysfield and is correctly implemented.
105-107: LGTM! But verify the usage of new fields.The changes to the
NewKeeperfunction signature are approved.However, ensure that the new fields
customContractFnsandkeysare correctly used in the codebase.Verification successful
The new fields
customContractFnsandkeysare correctly used.The changes to the
NewKeeperfunction signature and the addition of the new fields are correctly implemented and utilized in the codebase.
customContractFnsis used inx/evm/keeper/state_transition.go.keysis used inx/evm/keeper/keeper.goandx/evm/statedb/mock_test.go.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of `customContractFns` and `keys` in the codebase. # Test: Search for the usage of `customContractFns` and `keys`. Expect: Proper usage in the codebase. rg --type go 'customContractFns|keys'Length of output: 14812
Script:
#!/bin/bash # Description: Gather detailed information about the `Keeper` struct, `NewKeeper` function, and methods using `customContractFns` and `keys`. # Search for the definition of the `Keeper` struct ast-grep --lang go --pattern 'type Keeper struct { $$$ }' # Search for the implementation of the `NewKeeper` function ast-grep --lang go --pattern 'func NewKeeper($_, $_, $_, $_, $_, $_, $_) *Keeper { $$$ }' # Search for methods using `customContractFns` within the `Keeper` struct rg --type go 'k\.customContractFns' # Search for methods using `keys` within the `Keeper` struct rg --type go 'k\.keys'Length of output: 4109
There was a problem hiding this comment.
this looks good to me overall, but i have a feeling that there are many changes that are tricky to understand and missing more documentation and context
even though it is cherry pick, if we can increase this that would be great, especially on higher level (for example, we dont need to document store, btree etc, but changes in evm module, app.go and similar would be great to provide more context if possible)
i am ok to merge and test this on more e2e level, both with integrating with zeta and having some concrete precompiles to see how this work in bigger picture
lumtis
changed the title
|
Looks good to me
|
| @@ -257,20 +257,17 @@ func TxLogsFromEvents(events []abci.Event, msgIndex int) ([]*ethtypes.Log, error | |||
|
|
|||
| // ParseTxLogsFromEvent parse tx logs from one event | |||
| func ParseTxLogsFromEvent(event abci.Event) ([]*ethtypes.Log, error) { | |||
| logs := make([]*evmtypes.Log, 0, len(event.Attributes)) | |||
There was a problem hiding this comment.
logs := make([]*evmtypes.Log, 0, len(event.Attributes)) preallocates memory. Curious, why it was removed?
|
|
||
| // Set sets the transient-storage `value` for `key` at the given `addr`. | ||
| //nolint | ||
| func (t transientStorage) Set(addr common.Address, key, value common.Hash) { |
There was a problem hiding this comment.
Consider making these methods unexported as transientStorage is unexported itself
Signed-off-by: Francisco de Borja Aranda Castillejo <borja@zetachain.com>
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- x/evm/keeper/state_transition.go (6 hunks)
- x/evm/statedb/native.go (1 hunks)
Files skipped from review as they are similar to previous changes (2)
- x/evm/keeper/state_transition.go
- x/evm/statedb/native.go
Signed-off-by: Francisco de Borja Aranda Castillejo <borja@zetachain.com>
| for k, v := range tkeys { | ||
| allKeys[k] = v | ||
| } |
Check warning
Code scanning / CodeQL
Iteration over map Warning
| for k, v := range memKeys { | ||
| allKeys[k] = v | ||
| } |
Check warning
Code scanning / CodeQL
Iteration over map Warning
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- app/app.go (2 hunks)
Additional context used
GitHub Check: CodeQL
app/app.go
[warning] 470-472: Iteration over map
Iteration over map may be a possible source of non-determinism
[warning] 473-475: Iteration over map
Iteration over map may be a possible source of non-determinism
|
|
||
| // allKeys contain all the application stores, so it covers all the stateful precompiled contract use cases. | ||
| allKeys := make(map[string]storetypes.StoreKey, len(keys)+len(tkeys)+len(memKeys)) | ||
| for k, v := range keys { | ||
| allKeys[k] = v | ||
| } | ||
| for k, v := range tkeys { | ||
| allKeys[k] = v | ||
| } | ||
| for k, v := range memKeys { | ||
| allKeys[k] = v | ||
| } |
There was a problem hiding this comment.
Potential non-determinism due to map iteration.
Iteration over maps in Go can lead to non-deterministic behavior because the order of iteration is random. To ensure deterministic behavior, consider sorting the keys before iterating over the maps.
allKeys := make(map[string]storetypes.StoreKey, len(keys)+len(tkeys)+len(memKeys))
// Collect and sort keys
sortedKeys := make([]string, 0, len(keys))
for k := range keys {
sortedKeys = append(sortedKeys, k)
}
sort.Strings(sortedKeys)
for _, k := range sortedKeys {
allKeys[k] = keys[k]
}
// Repeat for tkeys and memKeys
sortedTKeys := make([]string, 0, len(tkeys))
for k := range tkeys {
sortedTKeys = append(sortedTKeys, k)
}
sort.Strings(sortedTKeys)
for _, k := range sortedTKeys {
allKeys[k] = tkeys[k]
}
sortedMemKeys := make([]string, 0, len(memKeys))
for k := range memKeys {
sortedMemKeys = append(sortedMemKeys, k)
}
sort.Strings(sortedMemKeys)
for _, k := range sortedMemKeys {
allKeys[k] = memKeys[k]
}Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // allKeys contain all the application stores, so it covers all the stateful precompiled contract use cases. | |
| allKeys := make(map[string]storetypes.StoreKey, len(keys)+len(tkeys)+len(memKeys)) | |
| for k, v := range keys { | |
| allKeys[k] = v | |
| } | |
| for k, v := range tkeys { | |
| allKeys[k] = v | |
| } | |
| for k, v := range memKeys { | |
| allKeys[k] = v | |
| } | |
| // allKeys contain all the application stores, so it covers all the stateful precompiled contract use cases. | |
| allKeys := make(map[string]storetypes.StoreKey, len(keys)+len(tkeys)+len(memKeys)) | |
| // Collect and sort keys | |
| sortedKeys := make([]string, 0, len(keys)) | |
| for k := range keys { | |
| sortedKeys = append(sortedKeys, k) | |
| } | |
| sort.Strings(sortedKeys) | |
| for _, k := range sortedKeys { | |
| allKeys[k] = keys[k] | |
| } | |
| // Repeat for tkeys and memKeys | |
| sortedTKeys := make([]string, 0, len(tkeys)) | |
| for k := range tkeys { | |
| sortedTKeys = append(sortedTKeys, k) | |
| } | |
| sort.Strings(sortedTKeys) | |
| for _, k := range sortedTKeys { | |
| allKeys[k] = tkeys[k] | |
| } | |
| sortedMemKeys := make([]string, 0, len(memKeys)) | |
| for k := range memKeys { | |
| sortedMemKeys = append(sortedMemKeys, k) | |
| } | |
| sort.Strings(sortedMemKeys) | |
| for _, k := range sortedMemKeys { | |
| allKeys[k] = memKeys[k] | |
| } |
Tools
GitHub Check: CodeQL
[warning] 470-472: Iteration over map
Iteration over map may be a possible source of non-determinism
[warning] 473-475: Iteration over map
Iteration over map may be a possible source of non-determinism
Major changes
go-ethereumpackage that supports stateful precompiled contracts, which is enabled inzeta-chain/go-ethereumby cherry-picking the upstream changes in feat(vm): stateful precompiles evmos/go-ethereum#10.Minor changes
zeta-chain/go-ethereum@v1.10.26-spcwhich enables stateful precompiles.Closes Support stateful precompiled contracts #65
Summary by CodeRabbit
New Features
CacheKVStorefor faster read/write operations.cachekv.cachemulti.Refactor
Documentation
store/cachekv/README.mddetailingCacheKVStorefunctionalities and performance improvements.Tests
CacheKVStoreoperations and iterators.CacheKVStorescenarios to ensure robustness.