fix: do not use posthandler to handle failed tx sequence increment
#92
Conversation
WalkthroughThe changes in this pull request primarily involve the modification and removal of sequence management decorators within the Changes
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 using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #92 +/- ##
==========================================
- Coverage 28.24% 28.19% -0.06%
==========================================
Files 124 124
Lines 13756 13710 -46
==========================================
- Hits 3886 3866 -20
+ Misses 9317 9292 -25
+ Partials 553 552 -1
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (1)
x/evm/ante/sequence.go (1)
33-33: Return the original context on error
Returning an empty sdk.Context{} may lead to unexpected behaviors downstream. It's better to return the original ctx to maintain context integrity.
Apply this diff to return the original context:
if err != nil {
- return sdk.Context{}, err
+ return ctx, err
}📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (8)
- app/ante/ante.go (2 hunks)
- app/ante/sigverify.go (0 hunks)
- app/ante/sigverify_test.go (0 hunks)
- app/posthandler/posthandler.go (0 hunks)
- app/posthandler/sequence.go (0 hunks)
- x/evm/ante/context_keys.go (1 hunks)
- x/evm/ante/sequence.go (1 hunks)
- x/evm/keeper/msg_server.go (5 hunks)
💤 Files with no reviewable changes (4)
- app/ante/sigverify.go
- app/ante/sigverify_test.go
- app/posthandler/posthandler.go
- app/posthandler/sequence.go
🔇 Additional comments (10)
x/evm/ante/context_keys.go (1)
8-8: LGTM! Verify usage of the new context key.
The addition of ContextKeySequenceIncremented follows good practices with clear naming and proper use of iota.
Let's verify how this new context key is being used in the codebase:
✅ Verification successful
Context key usage is properly integrated and follows a clear pattern
The verification shows the context key is correctly used in a coordinated way across the codebase:
- Set in
x/evm/ante/sequence.goto mark sequence increment - Checked and handled appropriately in
x/evm/keeper/msg_server.gofor create transactions - Properly cleaned up by setting to nil after processing
The implementation demonstrates proper context value lifecycle management and follows Go best practices.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check usage of ContextKeySequenceIncremented to ensure it's properly integrated
# Expected: Should find references in ante handlers or message handlers where sequence is managed
# Search for direct references to the constant
rg -A 5 "ContextKeySequenceIncremented"
# Search for methods that might be setting/getting this context value
ast-grep --pattern 'ctx.Value($$$ContextKeySequenceIncremented$$$)'
ast-grep --pattern 'ctx.WithValue($$$ContextKeySequenceIncremented$$$)'
Length of output: 947
app/ante/ante.go (2)
114-114: LGTM: Consistent sequence handling in OPinit ante handler.
The change maintains consistency with the main ante handler by using the same evmante.NewIncrementSequenceDecorator. The placement after signature verification is correct.
100-100: Verify the decorator ordering for sequence management.
The placement of evmante.NewIncrementSequenceDecorator after signature verification but before IBC and auction decorators is correct. This ensures that sequence numbers are incremented only after the transaction is validated but before any chain-specific processing.
Let's verify the sequence handling implementation:
✅ Verification successful
Decorator ordering and implementation verified successfully
The verification confirms:
- The
evmante.IncrementSequenceDecoratoris properly implemented inx/evm/ante/sequence.gowith correct sequence increment logic for transaction signers - It's used consistently in both ante handler chains in
app/ante/ante.go - No other sequence decorators remain in the codebase, confirming complete migration
- The implementation aligns with the PR's objective of moving sequence handling to the EVM module
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the sequence handling implementation in evmante package
# Test 1: Check the implementation of the new sequence decorator
ast-grep --pattern 'func NewIncrementSequenceDecorator($_) $_'
# Test 2: Verify there are no remaining references to old sequence decorator
rg "NewSequenceIncrementDecorator|SequenceIncrementDecorator" -g '!*.md'
Length of output: 411
Script:
#!/bin/bash
# Let's verify the implementation details and usage of the sequence decorator
# Check the full implementation of IncrementSequenceDecorator
rg -A 20 "type IncrementSequenceDecorator struct" x/evm/ante/sequence.go
# Check where it's being used in ante handlers
rg "IncrementSequenceDecorator" app/ante/
# Check if there are any other sequence decorators still in use
rg "Sequence.*Decorator" --type go
Length of output: 1559
x/evm/ante/sequence.go (1)
24-48: Ensure alignment with transaction sequence handling logic
This decorator modifies the sequence numbers of all signers. Ensure that this logic aligns with the overall transaction processing in the application and that it doesn't conflict with other mechanisms that handle sequence numbers.
Would you like assistance in reviewing related parts of the codebase to confirm consistency?
x/evm/keeper/msg_server.go (6)
16-16: Importing "evmante" package is necessary for sequence handling.
The addition of the evmante import is appropriate, as it provides access to constants and functions needed for the updated sequence management logic.
35-40: Proper sequence handling in the Create method.
The addition of handleSequenceIncremented ensures that the sequence number is correctly managed to prevent double incrementing during EVM contract creation.
100-105: Proper sequence handling in the Create2 method.
Calling handleSequenceIncremented in the Create2 method aligns the sequence management logic with the Create method, ensuring consistency in contract creation operations.
165-167: Ensuring correct sequence management in the Call method.
Invoking handleSequenceIncremented with isCreate set to false correctly handles the sequence number for contract calls, preventing unintended sequence adjustments.
264-277:
Check for nil account to prevent a nil pointer dereference.
In the handleSequenceIncremented function, after retrieving the sender's account, there is no check to ensure that acc is not nil before calling acc.SetSequence(). If the account does not exist, this could lead to a nil pointer dereference.
Consider adding a check for nil:
acc := k.accountKeeper.GetAccount(ctx, sender)
if acc == nil {
return ctx, sdkerrors.ErrUnknownAddress.Wrapf("account %s does not exist", sender)
}276-277: Ensure consistent context after modification.
The function returns a new context with evmante.ContextKeySequenceIncremented set to nil. Verify that this updated context is properly propagated and used in subsequent operations to maintain context integrity.
Run the following script to check for proper context propagation:
✅ Verification successful
Let me gather more context about how this context value is used in the codebase.
Context propagation is correctly implemented
The context value ContextKeySequenceIncremented is used in a well-defined flow:
- It's set to
truein the ante handler (x/evm/ante/sequence.go) - Checked in
handleSequenceIncrementedto manage account sequence - Finally set to
nilafter processing, which is the correct behavior to prevent reuse
The context modifications are properly handled and the sequence increment flag serves its intended purpose in the transaction lifecycle.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that the returned context is used after calling handleSequenceIncremented.
# Test: Search for instances where the context is not updated after handleSequenceIncremented.
# Expect: All subsequent uses of context should be the updated one.
rg -A 3 'handleSequenceIncremented\(.*\)' x/evm/keeper/ | rg -v 'ctx ='
Length of output: 1020
Script:
#!/bin/bash
# Search for usages of ContextKeySequenceIncremented
rg -A 5 "ContextKeySequenceIncremented" --type go
# Search for any code that retrieves this context value
ast-grep --pattern 'Value($_, evmante.ContextKeySequenceIncremented)'
Length of output: 885
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
x/evm/ante/sequence_test.go (3)
17-17: Consider documenting the significance of account number 50.
Adding a comment explaining why this specific account number was chosen would improve test clarity.
+ // Set arbitrary account number for testing
suite.NoError(acc.SetAccountNumber(uint64(50)))33-34: Good test isolation practice.
Testing the IncrementSequenceDecorator in isolation follows good testing practices. However, consider adding integration tests with other decorators to ensure proper interaction.
36-46: Add descriptions for test cases.
Consider adding names or descriptions for each test case to clarify the specific scenario being tested.
testCases := []struct {
+ name string
ctx sdk.Context
simulate bool
expectedSeq uint64
}{
- {suite.ctx.WithIsReCheckTx(true), false, 1},
+ {
+ name: "recheck tx without simulation",
+ ctx: suite.ctx.WithIsReCheckTx(true),
+ simulate: false,
+ expectedSeq: 1,
+ },
// ... similar changes for other test cases
}📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- x/evm/ante/sequence_test.go (1 hunks)
🔇 Additional comments (1)
x/evm/ante/sequence_test.go (1)
20-28: Consider adding validation for transaction builder operations.
While SetMsgs error is checked, other builder operations like SetFeeAmount and SetGasLimit should also be validated for completeness.
Description
Closes: #XXXX
to handle this issue: cosmos/cosmos-sdk#22396
we changed to sequence decrement be done in x/evm msg handler.
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 sequence numbers in transactions.ContextKeySequenceIncrementedfor enhanced context tracking.Bug Fixes
Removals
IncrementSequenceDecoratorand associated methods from thesigverifyandposthandlerpackages, simplifying transaction processing.Tests
IncrementSequenceDecorator.