fix(auth): audit issues with unordered txs #23392
Conversation
📝 WalkthroughWalkthroughThis pull request introduces modifications to handling unordered transactions across multiple files in the blockchain system. The changes primarily focus on updating the logic for processing unordered transactions, specifically shifting from using gas limits to timeout timestamps for nonce determination. The modifications impact transaction identification, signature verification, and mempool management, ensuring a more consistent approach to handling transactions that do not follow the standard sequential processing model. Changes
Sequence DiagramsequenceDiagram
participant TX as Unordered Transaction
participant Mempool as Mempool
participant SigVerify as Signature Verification
participant Ante as Ante Handler
TX->>Mempool: Insert Transaction
Mempool->>Mempool: Validate Timeout Timestamp
Mempool-->>TX: Nonce Assigned
TX->>SigVerify: Verify Signature
SigVerify->>SigVerify: Check Unordered Flag
SigVerify-->>Ante: Skip Sequence Verification
Ante->>TX: Process Transaction
Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (3)
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
Documentation and Community
|
|
@facundomedica your pull request is missing a changelog! |
![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
🧹 Nitpick comments (6)
x/auth/ante/unordered.go (2)
210-210: Correct the error message when writing gas to buffer.The error message currently states "failed to write unordered to buffer," which is misleading. It should accurately reflect the operation of writing gas to the buffer.
Apply this diff to correct the error message:
if err := binary.Write(buf, binary.LittleEndian, sigTx.GetGas()); err != nil { return errorsmod.Wrap( sdkerrors.ErrInvalidRequest, - "failed to write unordered to buffer", + "failed to write gas to buffer", ) }
223-245: Ensure consistent error handling inaddSignaturesfunction.In the
defaultcase of theswitchstatement, the error is returned usingfmt.Errorf, whereas elsewhere, errors are wrapped usingerrorsmod.Wrap. For consistency and better error categorization, consider usingerrorsmod.Wrapfinstead offmt.Errorf.Apply this diff to make the error handling consistent:
default: - return fmt.Errorf("unexpected SignatureData %T", data) + return errorsmod.Wrapf( + sdkerrors.ErrInvalidRequest, + "unexpected SignatureData %T", + data, + ) }types/mempool/sender_nonce.go (1)
148-154: Refactor duplicated code for handling unordered transactions.The logic for handling unordered transactions in both the
Insert(lines 148-154) andRemove(lines 239-245) methods is duplicated. To improve maintainability and reduce code duplication, consider refactoring this code into a helper function.Here's an example of how you might refactor the code:
func getUnorderedNonce(tx sdk.Tx) (uint64, error) { if unordered, ok := tx.(sdk.TxWithUnordered); ok && unordered.GetUnordered() { timestamp := unordered.GetTimeoutTimeStamp().Unix() if timestamp < 0 { return 0, errors.New("invalid timestamp value") } return uint64(timestamp), nil } return 0, nil }Then, update the
Insertmethod:// Insert adds a tx to the mempool. func (snm *SenderNonceMempool) Insert(_ context.Context, tx sdk.Tx) error { // Existing code... nonce := sig.Sequence - // if it's an unordered tx, we use the timeout timestamp instead of the nonce - if unordered, ok := tx.(sdk.TxWithUnordered); ok && unordered.GetUnordered() { - timestamp := unordered.GetTimeoutTimeStamp().Unix() - if timestamp < 0 { - return errors.New("invalid timestamp value") - } - nonce = uint64(timestamp) - } + // Get the nonce for unordered transactions + if unorderedNonce, err := getUnorderedNonce(tx); err != nil { + return err + } else if unorderedNonce != 0 { + nonce = unorderedNonce + } // Existing code... }And similarly, update the
Removemethod:// Remove removes a tx from the mempool. func (snm *SenderNonceMempool) Remove(tx sdk.Tx) error { // Existing code... nonce := sig.Sequence - // if it's an unordered tx, we use the timeout timestamp instead of the nonce - if unordered, ok := tx.(sdk.TxWithUnordered); ok && unordered.GetUnordered() { - timestamp := unordered.GetTimeoutTimeStamp().Unix() - if timestamp < 0 { - return errors.New("invalid timestamp value") - } - nonce = uint64(timestamp) - } + // Get the nonce for unordered transactions + if unorderedNonce, err := getUnorderedNonce(tx); err != nil { + return err + } else if unorderedNonce != 0 { + nonce = unorderedNonce + } // Existing code... }Also applies to: 239-245
types/mempool/priority_nonce.go (1)
227-233: Refactor duplicated code for handling unordered transactions.The logic for handling unordered transactions in both the
Insert(lines 227-233) andRemove(lines 472-478) methods is duplicated. To enhance maintainability and reduce code duplication, consider extracting this logic into a shared helper function.Create a helper function like this:
func getUnorderedNonce(tx sdk.Tx) (uint64, error) { if unordered, ok := tx.(sdk.TxWithUnordered); ok && unordered.GetUnordered() { timestamp := unordered.GetTimeoutTimeStamp().Unix() if timestamp < 0 { return 0, errors.New("invalid timestamp value") } return uint64(timestamp), nil } return 0, nil }Then, modify the
Insertmethod:// Insert attempts to insert a Tx into the mempool. func (mp *PriorityNonceMempool[C]) Insert(ctx context.Context, tx sdk.Tx) error { // Existing code... nonce := sig.Sequence - // if it's an unordered tx, we use the timeout timestamp instead of the nonce - if unordered, ok := tx.(sdk.TxWithUnordered); ok && unordered.GetUnordered() { - timestamp := unordered.GetTimeoutTimeStamp().Unix() - if timestamp < 0 { - return errors.New("invalid timestamp value") - } - nonce = uint64(timestamp) - } + // Get the nonce for unordered transactions + if unorderedNonce, err := getUnorderedNonce(tx); err != nil { + return err + } else if unorderedNonce != 0 { + nonce = unorderedNonce + } // Existing code... }And update the
Removemethod:// Remove removes a transaction from the mempool. func (mp *PriorityNonceMempool[C]) Remove(tx sdk.Tx) error { // Existing code... nonce := sig.Sequence - // if it's an unordered tx, we use the timeout timestamp instead of the nonce - if unordered, ok := tx.(sdk.TxWithUnordered); ok && unordered.GetUnordered() { - timestamp := unordered.GetTimeoutTimeStamp().Unix() - if timestamp < 0 { - return errors.New("invalid timestamp value") - } - nonce = uint64(timestamp) - } + // Get the nonce for unordered transactions + if unorderedNonce, err := getUnorderedNonce(tx); err != nil { + return err + } else if unorderedNonce != 0 { + nonce = unorderedNonce + } // Existing code... }Also applies to: 472-478
x/auth/ante/sigverify.go (2)
337-337: Fix indentation.The line is indented incorrectly compared to the surrounding code.
- sdkerrors.ErrWrongSequence, + sdkerrors.ErrWrongSequence,
325-339: Document security implications of unordered transactions.The introduction of unordered transactions that bypass sequence checks is a significant architectural change. Consider:
- Adding documentation about potential security implications.
- Clarifying when unordered transactions should/shouldn't be used.
- Documenting how replay protection works for unordered transactions.
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
types/mempool/priority_nonce.go(2 hunks)types/mempool/sender_nonce.go(2 hunks)x/auth/ante/sigverify.go(1 hunks)x/auth/ante/unordered.go(5 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
types/mempool/priority_nonce.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
types/mempool/sender_nonce.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
x/auth/ante/unordered.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
x/auth/ante/sigverify.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Summary
🔇 Additional comments (1)
x/auth/ante/sigverify.go (1)
322-324: LGTM! Clean implementation of unordered transaction check.The code follows Go's best practices for type assertion and interface checking.
julienrbrt
added
the
backport/v0.52.x
|
Can we get tests to cover this new logic? |
| return err | ||
| timestamp := unordered.GetTimeoutTimeStamp().Unix() | ||
| if timestamp < 0 { | ||
| return errors.New("invalid timestamp value") |
There was a problem hiding this comment.
This was in the old code before but as you touched it ( ;-) ), it would make sense to have this block moved before snm.senders is set in L145. We should not add elements to the object before the error cases are handled
| } | ||
| } | ||
| default: | ||
| return fmt.Errorf("unexpected SignatureData %T", data) |
There was a problem hiding this comment.
personal preference:
This works with the specified types but is easy to miss and limits the types supported. If you would add a io.Writer to signing.SignatureData, you can call this and don't need the cast and ensure all types are supported.
There was a problem hiding this comment.
not sure I understand this one 🤔
Co-authored-by: Alexander Peters <[email protected]>
| isUnordered := ok && unorderedTx.GetUnordered() | ||
|
|
||
| // only check sequence if the tx is not unordered | ||
| if !isUnordered { |
There was a problem hiding this comment.
@facundomedica can we test this?
There was a problem hiding this comment.
added a test for it 👌
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
types/mempool/sender_nonce.go (1)
142-150: 🛠️ Refactor suggestionMove error check before modifying state.
The error check for invalid timestamp should be moved before the
senderTxslookup to maintain proper error handling flow.Apply this diff to improve the error handling:
// if it's an unordered tx, we use the timeout timestamp instead of the nonce if unordered, ok := tx.(sdk.TxWithUnordered); ok && unordered.GetUnordered() { timestamp := unordered.GetTimeoutTimeStamp().Unix() + if timestamp < 0 { + return errors.New("invalid timestamp value") + } nonce = uint64(timestamp) - if timestamp < 0 { - return errors.New("invalid timestamp value") - } }
🧹 Nitpick comments (5)
types/mempool/sender_nonce_test.go (2)
197-246: Enhance test coverage for unordered transactions.The test should include additional scenarios:
- Transactions with same timeout timestamp
- Transactions with expired timeout
- Invalid timeout values (nil, past time)
- Mix of ordered and unordered transactions
Line range hint
982-987: Improve test robustness.Consider these improvements:
- Use relative time values (e.g.,
time.Now().Add(duration)) to avoid potential timezone issues- Add cleanup to remove test transactions from mempool after test completion
- now := time.Now() - oneHour := now.Add(1 * time.Hour) - thirtyMin := now.Add(30 * time.Minute) - twoHours := now.Add(2 * time.Hour) - fifteenMin := now.Add(15 * time.Minute) + baseTime := time.Now() + timeouts := []time.Duration{ + time.Hour, + 30 * time.Minute, + 2 * time.Hour, + 15 * time.Minute, + }x/auth/ante/testutil_test.go (1)
245-304: Add input validation for unordered transaction parameters.Consider validating:
- Timeout timestamp for unordered transactions (should not be zero when unordered is true)
- Consistency between unordered flag and timeout value
func (suite *AnteTestSuite) CreateTestUnorderedTx( ctx sdk.Context, privs []cryptotypes.PrivKey, accNums, accSeqs []uint64, chainID string, signMode apisigning.SignMode, unordered bool, unorderedTimeout time.Time, ) (xauthsigning.Tx, error) { + if unordered && unorderedTimeout.IsZero() { + return nil, fmt.Errorf("timeout timestamp required for unordered transactions") + } suite.txBuilder.SetUnordered(unordered) suite.txBuilder.SetTimeoutTimestamp(unorderedTimeout)types/mempool/priority_nonce_test.go (1)
974-1010: Consolidate duplicate test logic.This test has similar setup and verification logic as
TestUnorderedTxinsender_nonce_test.go. Consider:
- Creating shared test utilities for unordered transaction testing
- Adding unique test cases specific to priority handling
Additionally, the test should verify priority-specific behavior:
- Interaction between priority and timeout ordering
- Priority updates for unordered transactions
🧰 Tools
🪛 golangci-lint (1.62.2)
1009-1009: File is not
gofumpt-ed with-extra(gofumpt)
🪛 GitHub Actions: Lint
[error] 1009-1009: File is not
gofumpt-ed with-extra. Formatting issue detected.types/mempool/sender_nonce.go (1)
239-245: Extract common unordered tx logic to reduce duplication.The unordered transaction handling logic is duplicated between Insert and Remove methods. Consider extracting it into a helper function.
Create a helper function like this:
// getNonceFromTx returns the appropriate nonce for a transaction, using the timeout // timestamp for unordered transactions and sequence number for regular transactions. func getNonceFromTx(tx sdk.Tx, sig signing.SignatureV2) (uint64, error) { if unordered, ok := tx.(sdk.TxWithUnordered); ok && unordered.GetUnordered() { timestamp := unordered.GetTimeoutTimeStamp().Unix() if timestamp < 0 { return 0, errors.New("invalid timestamp value") } return uint64(timestamp), nil } return sig.Sequence, nil }Then use it in both Insert and Remove methods:
- nonce := sig.Sequence - if unordered, ok := tx.(sdk.TxWithUnordered); ok && unordered.GetUnordered() { - timestamp := unordered.GetTimeoutTimeStamp().Unix() - if timestamp < 0 { - return errors.New("invalid timestamp value") - } - nonce = uint64(timestamp) - } + nonce, err := getNonceFromTx(tx, sig) + if err != nil { + return err + }
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
types/mempool/mempool_test.go(3 hunks)types/mempool/priority_nonce_test.go(1 hunks)types/mempool/sender_nonce.go(2 hunks)types/mempool/sender_nonce_test.go(2 hunks)x/auth/ante/ante_test.go(2 hunks)x/auth/ante/testutil_test.go(2 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
x/auth/ante/ante_test.go (2)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern **/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"
types/mempool/sender_nonce_test.go (2)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern **/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"
types/mempool/priority_nonce_test.go (2)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern **/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"
types/mempool/mempool_test.go (2)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern **/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"
x/auth/ante/testutil_test.go (2)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern **/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"
types/mempool/sender_nonce.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
🪛 golangci-lint (1.62.2)
x/auth/ante/ante_test.go
1418-1418: File is not gofumpt-ed with -extra
(gofumpt)
types/mempool/priority_nonce_test.go
1009-1009: File is not gofumpt-ed with -extra
(gofumpt)
🪛 GitHub Actions: Lint
x/auth/ante/ante_test.go
[error] 1418-1418: File is not gofumpt-ed with -extra. Formatting issue detected.
types/mempool/priority_nonce_test.go
[error] 1009-1009: File is not gofumpt-ed with -extra. Formatting issue detected.
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: tests (00)
- GitHub Check: test-system-v2
- GitHub Check: build (amd64)
- GitHub Check: Analyze
- GitHub Check: Summary
🔇 Additional comments (3)
types/mempool/mempool_test.go (1)
59-73: LGTM! Clean implementation of TxWithUnordered interface.The implementation correctly handles the timeout timestamp and unordered flag.
x/auth/ante/ante_test.go (2)
9-9: LGTM!The
timepackage import is required for setting the timeout timestamp in the unordered transaction test.
1389-1419: LGTM! Test coverage for unordered transactions looks good.The test thoroughly verifies that:
- A normal sequential tx with sequence 0 succeeds
- A second tx with the same sequence fails (replay protection)
- An unordered tx with the same sequence succeeds (sequence check bypassed)
The test demonstrates that unordered transactions correctly bypass sequence checks while maintaining other validations.
🧰 Tools
🪛 golangci-lint (1.62.2)
1418-1418: File is not
gofumpt-ed with-extra(gofumpt)
🪛 GitHub Actions: Lint
[error] 1418-1418: File is not
gofumpt-ed with-extra. Formatting issue detected.
types/mempool/priority_nonce_test.go
Outdated
| for i, tx := range orderedTxs { | ||
| require.Equal(t, txs[i].id, tx.(testTx).id) | ||
| } | ||
|
|
There was a problem hiding this comment.
Fix gofumpt formatting.
Run gofumpt -l -w . to fix the formatting issue.
🧰 Tools
🪛 golangci-lint (1.62.2)
1009-1009: File is not gofumpt-ed with -extra
(gofumpt)
🪛 GitHub Actions: Lint
[error] 1009-1009: File is not gofumpt-ed with -extra. Formatting issue detected.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
x/auth/ante/ante_test.go (2)
1389-1393: Test setup could be more comprehensive.Consider adding multiple test accounts to verify unordered transactions behavior across different accounts.
- accs := suite.CreateTestAccounts(1) + accs := suite.CreateTestAccounts(2)
1389-1418: Add test for concurrent unordered transactions.The test should verify that multiple unordered transactions with the same sequence can be processed concurrently.
+func TestAnteHandlerConcurrentUnorderedTx(t *testing.T) { + suite := SetupTestSuite(t, false) + accs := suite.CreateTestAccounts(1) + msg := testdata.NewTestMsg(accs[0].acc.GetAddress()) + + suite.bankKeeper.EXPECT().SendCoinsFromAccountToModule(gomock.Any(), accs[0].acc.GetAddress(), authtypes.FeeCollectorName, testdata.NewTestFeeAmount()).Return(nil).AnyTimes() + + privs, accNums, accSeqs := []cryptotypes.PrivKey{accs[0].priv}, []uint64{1000}, []uint64{0} + + // Create multiple unordered transactions with same sequence + var wg sync.WaitGroup + for i := 0; i < 3; i++ { + wg.Add(1) + go func() { + defer wg.Done() + tx, _ := suite.CreateTestUnorderedTx(suite.ctx, privs, accNums, accSeqs, suite.ctx.ChainID(), apisigning.SignMode_SIGN_MODE_DIRECT, true, time.Now().Add(time.Minute)) + _, err := suite.anteHandler(suite.ctx, tx, false) + require.NoError(t, err) + }() + } + wg.Wait() +}
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
types/mempool/priority_nonce_test.go(1 hunks)x/auth/ante/ante_test.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- types/mempool/priority_nonce_test.go
🧰 Additional context used
📓 Path-based instructions (1)
x/auth/ante/ante_test.go (2)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern **/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: tests (00)
- GitHub Check: test-system-v2
- GitHub Check: Analyze
- GitHub Check: Summary
| // we try to send another tx with the same sequence, it will fail | ||
| _, err = suite.DeliverMsgs(t, privs, []sdk.Msg{msg}, testdata.NewTestFeeAmount(), testdata.NewTestGasLimit(), accNums, accSeqs, suite.ctx.ChainID(), false) | ||
| require.Error(t, err) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance error assertion.
The error check should verify the specific error type for sequence mismatch.
- require.Error(t, err)
+ require.ErrorIs(t, err, sdkerrors.ErrWrongSequence)📝 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.
| // we try to send another tx with the same sequence, it will fail | |
| _, err = suite.DeliverMsgs(t, privs, []sdk.Msg{msg}, testdata.NewTestFeeAmount(), testdata.NewTestGasLimit(), accNums, accSeqs, suite.ctx.ChainID(), false) | |
| require.Error(t, err) | |
| // we try to send another tx with the same sequence, it will fail | |
| _, err = suite.DeliverMsgs(t, privs, []sdk.Msg{msg}, testdata.NewTestFeeAmount(), testdata.NewTestGasLimit(), accNums, accSeqs, suite.ctx.ChainID(), false) | |
| require.ErrorIs(t, err, sdkerrors.ErrWrongSequence) |
| tx, txErr := suite.CreateTestUnorderedTx(suite.ctx, privs, accNums, accSeqs, suite.ctx.ChainID(), apisigning.SignMode_SIGN_MODE_DIRECT, true, time.Now().Add(time.Minute)) | ||
| require.NoError(t, txErr) | ||
| txBytes, err := suite.clientCtx.TxConfig.TxEncoder()(tx) | ||
| bytesCtx := suite.ctx.WithTxBytes(txBytes) | ||
| require.NoError(t, err) | ||
| _, err = suite.anteHandler(bytesCtx, tx, false) | ||
| require.NoError(t, err) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add timeout validation test cases.
The test should include cases for expired and future timeouts to ensure proper validation.
+ // Test expired timeout
+ expiredTx, _ := suite.CreateTestUnorderedTx(suite.ctx, privs, accNums, accSeqs, suite.ctx.ChainID(), apisigning.SignMode_SIGN_MODE_DIRECT, true, time.Now().Add(-time.Minute))
+ _, err = suite.anteHandler(bytesCtx, expiredTx, false)
+ require.ErrorIs(t, err, sdkerrors.ErrTxTimeoutHeight)
+
+ // Test far future timeout
+ futureTx, _ := suite.CreateTestUnorderedTx(suite.ctx, privs, accNums, accSeqs, suite.ctx.ChainID(), apisigning.SignMode_SIGN_MODE_DIRECT, true, time.Now().Add(24*time.Hour))
+ _, err = suite.anteHandler(bytesCtx, futureTx, false)
+ require.ErrorIs(t, err, sdkerrors.ErrInvalidTimeout)Committable suggestion skipped: line range outside the PR's diff.
| // First send a normal sequential tx with sequence 0 | ||
| suite.bankKeeper.EXPECT().SendCoinsFromAccountToModule(gomock.Any(), accs[0].acc.GetAddress(), authtypes.FeeCollectorName, testdata.NewTestFeeAmount()).Return(nil).AnyTimes() | ||
|
|
||
| privs, accNums, accSeqs := []cryptotypes.PrivKey{accs[0].priv}, []uint64{1000}, []uint64{0} | ||
| _, err := suite.DeliverMsgs(t, privs, []sdk.Msg{msg}, testdata.NewTestFeeAmount(), testdata.NewTestGasLimit(), accNums, accSeqs, suite.ctx.ChainID(), false) | ||
| require.NoError(t, err) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add assertion for account sequence.
The test should verify that the account sequence is incremented after the first transaction.
_, err := suite.DeliverMsgs(t, privs, []sdk.Msg{msg}, testdata.NewTestFeeAmount(), testdata.NewTestGasLimit(), accNums, accSeqs, suite.ctx.ChainID(), false)
require.NoError(t, err)
+ // Verify sequence is incremented
+ acc := suite.accountKeeper.GetAccount(suite.ctx, accs[0].acc.GetAddress())
+ require.Equal(t, uint64(1), acc.GetSequence())📝 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.
| // First send a normal sequential tx with sequence 0 | |
| suite.bankKeeper.EXPECT().SendCoinsFromAccountToModule(gomock.Any(), accs[0].acc.GetAddress(), authtypes.FeeCollectorName, testdata.NewTestFeeAmount()).Return(nil).AnyTimes() | |
| privs, accNums, accSeqs := []cryptotypes.PrivKey{accs[0].priv}, []uint64{1000}, []uint64{0} | |
| _, err := suite.DeliverMsgs(t, privs, []sdk.Msg{msg}, testdata.NewTestFeeAmount(), testdata.NewTestGasLimit(), accNums, accSeqs, suite.ctx.ChainID(), false) | |
| require.NoError(t, err) | |
| // First send a normal sequential tx with sequence 0 | |
| suite.bankKeeper.EXPECT().SendCoinsFromAccountToModule(gomock.Any(), accs[0].acc.GetAddress(), authtypes.FeeCollectorName, testdata.NewTestFeeAmount()).Return(nil).AnyTimes() | |
| privs, accNums, accSeqs := []cryptotypes.PrivKey{accs[0].priv}, []uint64{1000}, []uint64{0} | |
| _, err := suite.DeliverMsgs(t, privs, []sdk.Msg{msg}, testdata.NewTestFeeAmount(), testdata.NewTestGasLimit(), accNums, accSeqs, suite.ctx.ChainID(), false) | |
| require.NoError(t, err) | |
| // Verify sequence is incremented | |
| acc := suite.accountKeeper.GetAccount(suite.ctx, accs[0].acc.GetAddress()) | |
| require.Equal(t, uint64(1), acc.GetSequence()) |
| func (s *MempoolTestSuite) TestUnorderedTx() { | ||
| t := s.T() | ||
|
|
||
| ctx := sdk.NewContext(nil, false, log.NewNopLogger()) |
There was a problem hiding this comment.
We should be able to just use context.Background() for this test
There was a problem hiding this comment.
We need an sdk.Context here, because below we call ctx.WithPriority when calling mp.Insert
…cosmos-sdk into facu/fix-unordered-part2
Description
Closes: #XXXX
Marked ready for review after the green light by the auditor.
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 changeCHANGELOG.mdReviewers 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.
Please see Pull Request Reviewer section in the contributing guide for more information on how to review a pull request.
I have...
Summary by CodeRabbit
New Features
Bug Fixes
Refactor