fix: sequence should be higher or equal than expected during checktx and equal during deliver tx #22299
Conversation
testinginprod
requested review from
facundomedica,
aaronc,
a team and
sontrinh16
as code owners
📝 Walkthrough📝 WalkthroughWalkthroughThe pull request introduces enhancements to the Changes
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)
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
|
|
@testinginprod 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: 0
🧹 Outside diff range and nitpick comments (1)
x/auth/ante/sigverify.go (1)
309-320: Sequence number verification logic is correct, but standardize error messagesThe changes correctly implement the desired sequence number checks:
- During
ExecModeCheck, transactions with a sequence number greater than or equal to the account sequence are accepted.- During other execution modes, the sequence number must exactly match the account sequence.
However, there is a minor inconsistency in the error messages' punctuation and phrasing:
First error message uses a comma and specifies "expected higher than or equal to %d":
"account sequence mismatch, expected higher than or equal to %d, got %d", acc.GetSequence(), sig.Sequence,Second error message uses a colon and specifies "expected %d":
"account sequence mismatch: expected %d, got %d", acc.GetSequence(), sig.Sequence,For consistency and clarity, consider standardizing both the punctuation (either use a comma or a colon) and the phrasing of the error messages.
Apply this diff to unify the error messages:
- "account sequence mismatch, expected higher than or equal to %d, got %d", acc.GetSequence(), sig.Sequence, + "account sequence mismatch: expected sequence >= %d, got %d", acc.GetSequence(), sig.Sequence,
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (2)
- x/accounts/defaults/base/account.go (4 hunks)
- x/auth/ante/sigverify.go (1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
x/accounts/defaults/base/account.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.
🔇 Additional comments (2)
x/accounts/defaults/base/account.go (1)
115-116:⚠️ Potential issueError message does not accurately reflect the condition
The error message on line 116 states:
sequence number must be higher than: %d, got: %dThis implies that the
gotSeqmust be greater thansignerData.Sequence. However, the condition allowsgotSeqto be equal tosignerData.Sequence(i.e.,gotSeq >= signerData.Sequence).To align the error message with the condition, consider updating it to:
- "sequence number must be greater than or equal to: %d, got: %d"
- "sequence number must be at least: %d, got: %d"
Apply this diff to correct the error message:
- return nil, fmt.Errorf("sequence number must be higher than: %d, got: %d", signerData.Sequence, gotSeq) + return nil, fmt.Errorf("sequence number must be greater than or equal to: %d, got: %d", signerData.Sequence, gotSeq)Likely invalid or redundant comment.
x/auth/ante/sigverify.go (1)
327-329: Condition for skipping signature verification is appropriateThe condition to skip signature verification:
if execMode == transaction.ExecModeSimulate || isRecheckTx(ctx, svd.ak.GetEnvironment().TransactionService) || !isSigverifyTx(ctx) { return nil }correctly identifies scenarios where signature verification is unnecessary, including simulation mode, re-check transactions, and contexts where signature verification is not required.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
x/accounts/defaults/base/utils_test.go (2)
69-74: Consider adding comments to explain the purpose oftransactionService.While the implementation looks correct, it would be beneficial to add comments explaining the purpose of
transactionServiceand itsExecModemethod. This will improve code readability and make the testing setup more clear for other developers.Consider adding comments like this:
// transactionService is a mock implementation of the transaction service for testing purposes. type transactionService struct{} // ExecMode always returns ExecModeFinalize for testing scenarios. func (t transactionService) ExecMode(ctx context.Context) transaction.ExecMode { return transaction.ExecModeFinalize }
84-86: LGTM! Consider aligning field assignments for better readability.The addition of
TransactionServiceto theEnvironmentstruct is correct and consistent with the newly introduced type. To improve readability and maintain consistency, consider aligning the field assignments vertically:Environment: appmodulev2.Environment{ EventService: eventService{}, HeaderService: headerService{}, TransactionService: transactionService{}, },
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (3)
- x/accounts/defaults/base/utils_test.go (2 hunks)
- x/auth/ante/ante_test.go (2 hunks)
- x/auth/ante/sigverify_test.go (1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
x/accounts/defaults/base/utils_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/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"x/auth/ante/sigverify_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"
🔇 Additional comments (4)
x/accounts/defaults/base/utils_test.go (1)
Line range hint
69-86: Consider adding or updating tests for the newtransactionService.While the changes enhance the mock dependencies by adding the
transactionService, there are no explicit tests covering this new functionality. To ensure comprehensive test coverage:
- Add unit tests for the
transactionService.ExecModemethod.- Update existing tests that use
makeMockDependenciesto verify that theTransactionServiceis correctly included and utilized.This will help maintain the robustness of the test suite and ensure that the new functionality is working as expected in various scenarios.
To help identify existing tests that might need updating, you can run the following command:
Would you like assistance in drafting the additional test cases?
x/auth/ante/sigverify_test.go (1)
234-234: Correctly setting execution mode toExecModeFinalizefor non-recheck test casesBy explicitly setting
ctx = ctx.WithExecMode(sdk.ExecModeFinalize)in theelseblock, the test ensures that whentc.recheckisfalse, the execution mode is appropriately set toExecModeFinalize. This accurately simulates transaction processing during thedeliver txphase, which aligns with the PR's objective to adjust transaction sequence validation during this phase.x/auth/ante/ante_test.go (2)
918-919: LGTM! Setting execution mode for test accuracy.The context is correctly updated to
sdk.ExecModeFinalizeto simulate the finalize execution mode during testing, ensuring the test case reflects the intended behavior.
1086-1087: LGTM! Context execution mode appropriately updated.Setting
suite.ctxtosdk.ExecModeFinalizeensures the test operates under the correct execution mode, accurately reflecting the finalization process in this scenario.
julienrbrt
added
the
backport/v0.52.x
Description
This is a QOL change where TX sequence will be expected to be higher equal than sequence during checktx, and equal during deliver tx.
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