feat: skip sequence number check at simulation #107
Conversation
WalkthroughThe changes in this pull request primarily involve modifications to transaction handling and signature verification within the Ante module. The Changes
Possibly related PRs
Suggested reviewers
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 ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #107 +/- ##
==========================================
+ Coverage 28.07% 28.25% +0.17%
==========================================
Files 126 126
Lines 14231 14237 +6
==========================================
+ Hits 3996 4022 +26
+ Misses 9669 9642 -27
- Partials 566 573 +7
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (7)
app/ante/sigverify_test.go (5)
13-21: Consider adding clarifying comments for account setup.While the setup code is correct, it would be helpful to add a comment explaining why the public key needs to be set explicitly for the test account.
acc1 := suite.app.AccountKeeper.NewAccountWithAddress(suite.ctx, addr1) + // Set public key explicitly as it's required for signature verification acc1.SetPubKey(priv1.PubKey())
22-37: Document the test scenario setup more explicitly.While the code correctly sets up a test scenario with an invalid sequence number, the purpose could be more clearly documented.
+ // Setup a transaction with an invalid sequence number (100) to test sequence check behavior + // during simulation and non-simulation scenarios sigV2 := signing.SignatureV2{ PubKey: priv1.PubKey(), Data: &signing.SingleSignatureData{ SignMode: evmkeeper.SignMode_SIGN_MODE_ETHEREUM, Signature: nil, }, - // invalid sequence + // Use an invalid sequence number (100) to verify sequence check behavior Sequence: 100, }
38-41: Consider adding more explicit assertions for simulation mode.While the test correctly verifies that no error occurs, consider adding assertions to explicitly verify that we're in simulation mode and using Ethereum sign mode.
// 1. simulate should skip sequence check sigVerifyAnte := ante.NewSigVerificationDecorator(suite.app.AccountKeeper, suite.app.EVMKeeper, suite.app.TxConfig().SignModeHandler()) + // Verify we're using Ethereum sign mode + tx := suite.txBuilder.GetTx() + suite.Equal(evmkeeper.SignMode_SIGN_MODE_ETHEREUM, tx.GetSignatures()[0].Data.(*signing.SingleSignatureData).SignMode) _, err = sigVerifyAnte.AnteHandle(suite.ctx, suite.txBuilder.GetTx(), true, func(ctx sdk.Context, tx sdk.Tx, simulate bool) (newCtx sdk.Context, err error) { return ctx, nil }) suite.NoError(err)
43-48: Enhance error validation for sequence check failure.Consider adding more specific error validation to ensure the sequence check fails for the expected reason.
_, err = sigVerifyAnte.AnteHandle(suite.ctx, suite.txBuilder.GetTx(), true, func(ctx sdk.Context, tx sdk.Tx, simulate bool) (newCtx sdk.Context, err error) { return ctx, nil }) - suite.ErrorIs(err, sdkerrors.ErrWrongSequence) + suite.ErrorIs(err, sdkerrors.ErrWrongSequence) + suite.Contains(err.Error(), "account sequence mismatch")
50-56: Add validation for non-simulation context.Consider adding assertions to explicitly verify the non-simulation context.
// 3. non-simulate should check sequence sigV2.Data.(*signing.SingleSignatureData).SignMode = evmkeeper.SignMode_SIGN_MODE_ETHEREUM err = suite.txBuilder.SetSignatures(sigV2) suite.NoError(err) + // Create a new context with simulation flag set to false + nonSimCtx := suite.ctx.WithIsReCheckTx(true) - _, err = sigVerifyAnte.AnteHandle(suite.ctx, suite.txBuilder.GetTx(), false, func(ctx sdk.Context, tx sdk.Tx, simulate bool) (newCtx sdk.Context, err error) { return ctx, nil }) + _, err = sigVerifyAnte.AnteHandle(nonSimCtx, suite.txBuilder.GetTx(), false, func(ctx sdk.Context, tx sdk.Tx, simulate bool) (newCtx sdk.Context, err error) { + suite.False(simulate, "should be in non-simulation mode") + return ctx, nil + }) suite.ErrorIs(err, sdkerrors.ErrWrongSequence)app/ante/ante_test.go (1)
Line range hint
82-128: Consider adding explicit simulation test casesWhile the
CreateTestTxhelper now supports different signing modes, I don't see explicit test cases for the simulation scenario where sequence number checks are skipped. Consider adding test cases that:
- Verify successful transaction simulation without sequence numbers
- Compare behavior between normal and simulation modes
- Test edge cases specific to simulation mode
Example test structure:
func (suite *AnteTestSuite) TestSimulationWithoutSequence() { // Test simulation mode tx, err := suite.CreateTestTx( []cryptotypes.PrivKey{priv}, []uint64{accNum}, []uint64{accSeq}, suite.ctx.ChainID(), signing.SignMode_SIGN_MODE_DIRECT, // or appropriate simulation mode ) suite.Require().NoError(err) // Verify sequence check is skipped in simulation // Add assertions here }app/ante/sigverify.go (1)
86-92: Consider supporting multiple Ethereum signatures in simulationCurrently, the sequence check is skipped only when there is exactly one signature (
len(sigs) == 1). If there is a possibility of transactions with multiple Ethereum signatures, you may want to extend the logic to handle those cases.You could modify the condition to iterate over all signatures and set
skipSequenceChecktotrueif all meet the criteria:skipSequenceCheck := false if simulate { skip := true for _, sig := range sigs { if sigData, ok := sig.Data.(*signing.SingleSignatureData); ok { if sigData.SignMode != evmkeeper.SignMode_SIGN_MODE_ETHEREUM { skip = false break } } else { skip = false break } } skipSequenceCheck = skip }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
app/ante/ante_test.go(3 hunks)app/ante/fee_test.go(2 hunks)app/ante/sigverify.go(2 hunks)app/ante/sigverify_test.go(1 hunks)
🔇 Additional comments (8)
app/ante/sigverify_test.go (1)
1-11: LGTM! Package and imports are well-organized.
The imports are properly structured and include all necessary dependencies for the test suite.
app/ante/fee_test.go (3)
9-9: LGTM!
The addition of the authsign import is appropriate for handling sign mode conversion.
37-39: LGTM! Sign mode handling is properly implemented.
The conversion from API sign mode to internal format is correctly handled with proper error checking.
37-39: Add test case for sequence number skipping during simulation.
While the current test cases cover various fee scenarios, there's no explicit verification that sequence numbers are skipped during simulation. Consider adding a test case that specifically validates this behavior.
Let's check if there are other test files that might cover this:
Would you like me to help draft a test case that verifies sequence number skipping during simulation?
app/ante/ante_test.go (1)
82-82: LGTM: Method signature enhancement improves test flexibility
The addition of the signMode parameter allows for explicit control over transaction signing modes, which is essential for testing simulation scenarios where sequence checks need to be skipped.
app/ante/sigverify.go (3)
25-25: Import evmkeeper for Ethereum signing mode
The addition of the evmkeeper import is appropriate for accessing Ethereum-specific constants and functionalities.
85-93: Verify logic for skipping sequence check during simulation
The introduction of skipSequenceCheck to bypass the sequence check when simulating transactions with a single Ethereum signature is a valid approach. Ensure that this logic does not unintentionally affect other transaction types or introduce security risks.
To confirm that this change is safe, consider reviewing all simulation scenarios to ensure that sequence checks are appropriately enforced where necessary.
95-95: Maintain consistency in sequence checking logic
The modified condition now respects the skipSequenceCheck flag, which is good. Ensure that this change aligns with the overall authentication logic and does not affect non-simulated transactions.
Description
in normal geth, it allows to simulate gas without sequence number.
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