feat: sync evm nonce with cosmos #54
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThe changes involve multiple modifications across various files, focusing on enhancing transaction handling, account management, and contract functionalities. Key updates include the introduction of a new Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant MsgServer
participant AccountKeeper
participant StateDB
User->>MsgServer: Call transaction
MsgServer->>AccountKeeper: increaseNonce(caller)
AccountKeeper-->>MsgServer: Nonce incremented
MsgServer->>StateDB: Create or retrieve account
StateDB-->>MsgServer: Account ready
MsgServer->>User: Transaction processed
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #54 +/- ##
==========================================
+ Coverage 28.35% 28.38% +0.02%
==========================================
Files 114 113 -1
Lines 12194 12238 +44
==========================================
+ Hits 3458 3474 +16
- Misses 8236 8260 +24
- Partials 500 504 +4
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
x/evm/types/auth.pb.gois excluded by!**/*.pb.go
Files selected for processing (10)
- app/ante/ante.go (1 hunks)
- app/ante/sigverify.go (2 hunks)
- jsonrpc/backend/tx.go (3 hunks)
- proto/minievm/evm/v1/auth.proto (2 hunks)
- x/evm/keeper/context.go (2 hunks)
- x/evm/keeper/msg_server.go (2 hunks)
- x/evm/keeper/query_server.go (1 hunks)
- x/evm/state/keys.go (1 hunks)
- x/evm/state/statedb.go (11 hunks)
- x/evm/types/auth.go (2 hunks)
Files skipped from review due to trivial changes (1)
- x/evm/state/keys.go
Additional context used
GitHub Check: codecov/patch
x/evm/types/auth.go
[warning] 26-27: x/evm/types/auth.go#L26-L27
Added lines #L26 - L27 were not covered by tests
[warning] 71-71: x/evm/types/auth.go#L71
Added line #L71 was not covered by testsx/evm/keeper/msg_server.go
[warning] 149-150: x/evm/keeper/msg_server.go#L149-L150
Added lines #L149 - L150 were not covered by tests
[warning] 152-153: x/evm/keeper/msg_server.go#L152-L153
Added lines #L152 - L153 were not covered by tests
[warning] 169-170: x/evm/keeper/msg_server.go#L169-L170
Added lines #L169 - L170 were not covered by testsapp/ante/sigverify.go
[warning] 225-229: app/ante/sigverify.go#L225-L229
Added lines #L225 - L229 were not covered by tests
[warning] 232-235: app/ante/sigverify.go#L232-L235
Added lines #L232 - L235 were not covered by tests
[warning] 237-240: app/ante/sigverify.go#L237-L240
Added lines #L237 - L240 were not covered by tests
[warning] 243-243: app/ante/sigverify.go#L243
Added line #L243 was not covered by tests
[warning] 248-260: app/ante/sigverify.go#L248-L260
Added lines #L248 - L260 were not covered by tests
[warning] 263-264: app/ante/sigverify.go#L263-L264
Added lines #L263 - L264 were not covered by tests
[warning] 266-271: app/ante/sigverify.go#L266-L271
Added lines #L266 - L271 were not covered by tests
[warning] 273-275: app/ante/sigverify.go#L273-L275
Added lines #L273 - L275 were not covered by tests
[warning] 278-278: app/ante/sigverify.go#L278
Added line #L278 was not covered by tests
[warning] 282-282: app/ante/sigverify.go#L282
Added line #L282 was not covered by testsjsonrpc/backend/tx.go
[warning] 82-84: jsonrpc/backend/tx.go#L82-L84
Added lines #L82 - L84 were not covered by tests
[warning] 169-171: jsonrpc/backend/tx.go#L169-L171
Added lines #L169 - L171 were not covered by tests
[warning] 174-174: jsonrpc/backend/tx.go#L174
Added line #L174 was not covered by testsx/evm/state/statedb.go
[warning] 290-292: x/evm/state/statedb.go#L290-L292
Added lines #L290 - L292 were not covered by tests
[warning] 363-363: x/evm/state/statedb.go#L363
Added line #L363 was not covered by tests
[warning] 401-402: x/evm/state/statedb.go#L401-L402
Added lines #L401 - L402 were not covered by tests
Additional comments not posted (8)
proto/minievm/evm/v1/auth.proto (2)
16-16: LGTM!The addition of the
code_hashfield to theContractAccountmessage enhances the structure by allowing it to store a hash of the contract's code. This is crucial for contract verification and management.The code changes are approved.
25-30: LGTM!The formatting adjustments in the
ShorthandAccountmessage standardize the code style but do not affect functionality.The code changes are approved.
app/ante/ante.go (1)
97-97: LGTM!The modification in the instantiation of decorators within the
NewAnteHandlerfunction suggests a potential shift in the namespace or the implementation of theNewIncrementSequenceDecoratorfunction. This alteration may indicate a refactoring effort aimed at improving code organization or clarity.The code changes are approved.
x/evm/keeper/query_server.go (1)
86-86: Verify the correctness of the tracer variable usage.The boolean argument
falsehas been removed and thetracervariable is passed directly. Ensure that thetracervariable is correctly initialized and used in theEVMCallWithTracermethod.Run the following script to verify the initialization and usage of the
tracervariable:Verification successful
Tracer variable is correctly initialized and used.
The
tracervariable is properly initialized usinglogger.NewJSONLoggerand is of the correct type*tracing.Hooks. It is appropriately used in theEVMCallWithTracermethod, confirming the correctness of the change. No issues were found with the modification.
- Initialization:
logger.NewJSONLoggerfromgithub.com/ethereum/go-ethereum/eth/tracers/logger.- Usage: Correctly passed to
EVMCallWithTracer.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the initialization and usage of the `tracer` variable. # Test: Search for the initialization and usage of the `tracer` variable. Expect: Correct initialization and usage. rg --type go -A 5 $'var tracer *tracing.Hooks'Length of output: 47
Script:
#!/bin/bash # Search for the `tracer` variable in the `query_server.go` file to check its initialization. rg --type go 'tracer' x/evm/keeper/query_server.go # Search for the `EVMCallWithTracer` method to understand its parameters and usage. rg --type go 'func EVMCallWithTracer' -A 10 # Search for any imports or type definitions related to `tracing.Hooks`. rg --type go 'tracing.Hooks'Length of output: 1449
jsonrpc/backend/tx.go (1)
12-12: LGTM!The new import statement for
github.com/ethereum/go-ethereum/coreis necessary for the added functionality.The code changes are approved.
x/evm/keeper/context.go (2)
229-229: LGTM!The change simplifies the method interface, focusing on EVM calls without nonce management.
The code changes are approved.
233-233: LGTM!The change indicates that nonce management is no longer handled within this context, simplifying the control flow.
The code changes are approved.
x/evm/state/statedb.go (1)
264-265: LGTM!The change enhances the abstraction layer for account management, promoting better encapsulation.
The code changes are approved.
| BaseAccount: authtypes.NewBaseAccountWithAddress(addr), | ||
| CodeHash: []byte{}, |
There was a problem hiding this comment.
LGTM! But add tests.
The initialization of the CodeHash field enhances the ContractAccount structure by adding a property that may be used for further contract-related functionalities. However, the changes are not covered by tests.
The code changes are approved.
Do you want me to generate the unit testing code or open a GitHub issue to track this task?
Tools
GitHub Check: codecov/patch
[warning] 26-27: x/evm/types/auth.go#L26-L27
Added lines #L26 - L27 were not covered by tests
| @@ -67,5 +68,5 @@ | |||
| _, isShorthandAccount := account.(ShorthandAccountI) | |||
| _, isContractAccount := account.(*ContractAccount) | |||
|
|
|||
| return !isModuleAccount && !isShorthandAccount && !isContractAccount && account.GetPubKey() == nil | |||
| return !isModuleAccount && !isShorthandAccount && !isContractAccount && account.GetPubKey() == nil && account.GetSequence() == 0 | |||
There was a problem hiding this comment.
LGTM! But add tests.
The inclusion of the sequence check makes the criteria for determining whether an account is considered "empty" more stringent. However, the changes are not covered by tests.
The code changes are approved.
Do you want me to generate the unit testing code or open a GitHub issue to track this task?
Tools
GitHub Check: codecov/patch
[warning] 71-71: x/evm/types/auth.go#L71
Added line #L71 was not covered by tests
| // increaseNonce increases the nonce of the given account. | ||
| func (ms *msgServerImpl) increaseNonce(ctx context.Context, caller sdk.AccAddress) error { | ||
| senderAcc := ms.accountKeeper.GetAccount(ctx, caller) | ||
| if senderAcc == nil { | ||
| senderAcc = ms.accountKeeper.NewAccountWithAddress(ctx, caller) | ||
| } | ||
| if err := senderAcc.SetSequence(senderAcc.GetSequence() + 1); err != nil { | ||
| return err | ||
| } | ||
| ms.accountKeeper.SetAccount(ctx, senderAcc) | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Add tests for the increaseNonce function.
The function correctly increments the nonce of the specified account. However, the function is not covered by tests.
Do you want me to generate the unit testing code or open a GitHub issue to track this task?
Tools
GitHub Check: codecov/patch
[warning] 149-150: x/evm/keeper/msg_server.go#L149-L150
Added lines #L149 - L150 were not covered by tests
[warning] 152-153: x/evm/keeper/msg_server.go#L152-L153
Added lines #L152 - L153 were not covered by tests
| // increase nonce before execution like evm does | ||
| // | ||
| // NOTE: evm only increases nonce at Call not Create, so we should do the same. | ||
| if err := ms.increaseNonce(ctx, sender); err != nil { | ||
| return nil, err | ||
| } | ||
|
|
There was a problem hiding this comment.
Add tests for the modified Call function.
The function correctly invokes increaseNonce before executing the main logic. However, the added lines are not covered by tests.
Do you want me to generate the unit testing code or open a GitHub issue to track this task?
Tools
GitHub Check: codecov/patch
[warning] 169-170: x/evm/keeper/msg_server.go#L169-L170
Added lines #L169 - L170 were not covered by tests
Description
Closes: #XXXX
Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
!in the type prefix if API or client breaking changeReviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
I have...
Summary by CodeRabbit
New Features
IncrementSequenceDecoratorfor managing transaction signer sequence numbers, enhancing security against replay attacks.code_hashfield toContractAccountfor improved contract management.Bug Fixes
Refactor
accountKeeper, improving code readability and maintainability.