fix: increment sequence number at every call and create #102
Conversation
WalkthroughThe pull request modifies the handling of sequence numbers in the EVM's Changes
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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 #102 +/- ##
==========================================
+ Coverage 28.33% 28.40% +0.06%
==========================================
Files 126 126
Lines 14024 14037 +13
==========================================
+ Hits 3974 3987 +13
+ Misses 9489 9488 -1
- Partials 561 562 +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: 1
🧹 Outside diff range and nitpick comments (3)
x/evm/keeper/msg_server.go (1)
266-292: Consider adding debug logging for sequence number operationsWhile the implementation is correct, adding debug logging would improve observability of sequence number changes, especially useful for debugging in production environments.
Consider adding logs like this:
func (k *msgServerImpl) handleSequenceIncremented(ctx context.Context, sender sdk.AccAddress, isCreate bool) error { sdkCtx := sdk.UnwrapSDKContext(ctx) + k.Logger(ctx).Debug("handling sequence increment", + "sender", sender.String(), + "is_create", isCreate) if sdkCtx.Value(evmante.ContextKeySequenceIncremented) == nil { + k.Logger(ctx).Debug("sequence increment flag not set") return nil } incremented := sdkCtx.Value(evmante.ContextKeySequenceIncremented).(*bool) if isCreate && *incremented { + k.Logger(ctx).Debug("decrementing sequence for create operation") // if the sequence is already incremented, decrement it to prevent double incrementing the sequence number at create. acc := k.accountKeeper.GetAccount(ctx, sender) if err := acc.SetSequence(acc.GetSequence() - 1); err != nil { return err } k.accountKeeper.SetAccount(ctx, acc) } else if !isCreate && !*incremented { + k.Logger(ctx).Debug("incrementing sequence for call operation") // if the sequence is not incremented and the message is call, increment the sequence number. acc := k.accountKeeper.GetAccount(ctx, sender) if err := acc.SetSequence(acc.GetSequence() + 1); err != nil { return err } k.accountKeeper.SetAccount(ctx, acc) } // set the flag to false *incremented = false + k.Logger(ctx).Debug("reset sequence increment flag") return nil }x/evm/keeper/msg_server_test.go (2)
184-247: Enhance test readability and reliability.While the test logic is correct, consider the following improvements:
- Add descriptive comments for each test case to clarify the expected behavior.
- Use named constants or variables for sequence increments to make the intent clearer.
- Consider splitting into separate sub-tests using
t.Run()to isolate test cases.Here's a suggested refactor:
func Test_MsgServer_NonceIncrement_Call(t *testing.T) { + const ( + initialIncrement = 1 + callIncrement = 1 + createIncrement = 1 + ) + ctx, input := createDefaultTestInput(t) _, _, addr := keyPubAddr() caller := common.BytesToAddress(addr.Bytes()) // ... setup code ... - // increment sequence + // Setup: Set initial sequence and increment flag incremented := true ctx = ctx.WithValue(evmante.ContextKeySequenceIncremented, &incremented) acc := input.AccountKeeper.GetAccount(ctx, addr) - seq := acc.GetSequence() + 1 + seq := acc.GetSequence() + initialIncrement acc.SetSequence(seq) input.AccountKeeper.SetAccount(ctx, acc) - // should not increment sequence - msgServer := keeper.NewMsgServerImpl(&input.EVMKeeper) - res, err := msgServer.Call(ctx, &types.MsgCall{ - Sender: addr.String(), - ContractAddr: contractAddr.Hex(), - Input: hexutil.Encode(inputBz), - }) + t.Run("first call should not increment sequence when flag is set", func(t *testing.T) { + msgServer := keeper.NewMsgServerImpl(&input.EVMKeeper) + res, err := msgServer.Call(ctx, &types.MsgCall{ + Sender: addr.String(), + ContractAddr: contractAddr.Hex(), + Input: hexutil.Encode(inputBz), + }) + require.NoError(t, err) + require.Equal(t, "0x", res.Result) + require.NotEmpty(t, res.Logs) + acc := input.AccountKeeper.GetAccount(ctx, addr) + require.Equal(t, seq, acc.GetSequence(), "sequence should remain unchanged") + }) + t.Run("subsequent call should increment sequence", func(t *testing.T) { + // ... similar refactor for second call test + }) + t.Run("create should increment sequence", func(t *testing.T) { + // ... similar refactor for create test + }) }
184-307: Overall assessment: Tests are functional but could be improved.The tests effectively verify the sequence increment behavior for both contract calls and creations. While they are functionally correct, consider the suggested refactors to improve:
- Test maintainability through helper functions
- Code reusability by reducing duplication
- Test clarity with better structure and documentation
The core test logic and assertions are solid, making this a good foundation for the sequence increment fixes.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
x/evm/ante/sequence.go(1 hunks)x/evm/keeper/msg_server.go(4 hunks)x/evm/keeper/msg_server_test.go(2 hunks)
🧰 Additional context used
📓 Learnings (1)
x/evm/keeper/msg_server.go (1)
Learnt from: beer-1
PR: initia-labs/minievm#92
File: x/evm/keeper/msg_server.go:269-0
Timestamp: 2024-10-30T04:33:01.501Z
Learning: In `x/evm/keeper/msg_server.go`, within the `handleSequenceIncremented` method, it's safe to decrement the sequence number without checking for underflow because it was already incremented earlier.
🔇 Additional comments (6)
x/evm/ante/sequence.go (1)
46-47: Verify thread safety and context behavior with pointers.
The change to use a pointer for the incremented flag is a good approach as it enables state reversion after the first call. However, there are some important considerations:
-
Thread Safety: Using a shared pointer without synchronization could lead to race conditions if the context is accessed concurrently. Consider adding appropriate synchronization mechanisms.
-
Context Behavior: Ensure that the context properly handles pointer values, especially during cloning or value retrieval operations.
Let's verify the context usage pattern:
x/evm/keeper/msg_server.go (4)
36-39: LGTM: Simplified error handling in Create method
The change to directly handle errors from handleSequenceIncremented improves code clarity and maintains proper sequence number management.
101-104: LGTM: Consistent error handling in Create2 method
The change maintains consistency with the Create method implementation.
166-169: LGTM: Consistent error handling in Call method
The change maintains consistency with other message handlers while properly managing sequence numbers for call operations.
261-264: LGTM: Clear documentation and improved error handling
The documentation clearly explains the different sequence number increment behaviors between Cosmos SDK and EVM, which is crucial for understanding this complex interaction.
x/evm/keeper/msg_server_test.go (1)
12-12: LGTM: Import addition is appropriate.
The added import for evmante is necessary for accessing the context key used in sequence increment 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
Bug Fixes
Tests