fix(auth): support legacy global AccountNumber when query historical state #23743
Conversation
📝 WalkthroughWalkthroughThis pull request introduces legacy support for global account numbers in the x/auth module. It updates the Calculator class by adding a new method, modifying an existing method’s signature, and renaming a variable. The accounts module enhances the account number generation logic to retrieve a legacy account number from the KV store when the default account number is zero, with corresponding test coverage. In the auth module, the local legacy key is replaced by a reference to an imported constant, and a new legacy key variable is declared for future use. Changes
Suggested labels
Suggested reviewers
Possibly related PRs
✨ Finishing Touches
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
|
![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
🧹 Nitpick comments (3)
CHANGELOG.md (3)
4-11: Improve changelog principles section formattingThe guiding principles section would be more readable with bullet points instead of a numbered list.
-Guiding Principles: - -Changelogs are for humans, not machines. -There should be an entry for every single version. -The same types of changes should be grouped. -Versions and sections should be linkable. -The latest version comes first. -The release date of each version is displayed. -Mention whether you follow Semantic Versioning. +# Guiding Principles + +* Changelogs are for humans, not machines +* There should be an entry for every single version +* The same types of changes should be grouped +* Versions and sections should be linkable +* The latest version comes first +* The release date of each version is displayed +* Mention whether you follow Semantic Versioning
14-22: Improve usage section formattingThe usage section would be more readable with bullet points and better spacing.
-Change log entries are to be added to the Unreleased section under the -appropriate stanza (see below). Each entry is required to include a tag and -the Github issue reference in the following format: - -* (<tag>) \#<issue-number> message +# Usage + +* Change log entries should be added to the "Unreleased" section under the appropriate stanza +* Each entry must include: + * A tag in parentheses + * The GitHub issue reference number + * A descriptive message +* Format: `* (<tag>) #<issue-number> message`
24-34: Improve stanza types section formattingThe stanza types section would be clearer with bullet points and descriptions.
-Types of changes (Stanzas): - -"Features" for new features. -"Improvements" for changes in existing functionality. -"Deprecated" for soon-to-be removed features. -"Bug Fixes" for any bug fixes. -"Client Breaking" for breaking Protobuf, gRPC and REST routes used by end-users. -"CLI Breaking" for breaking CLI commands. -"API Breaking" for breaking exported APIs used by developers building on SDK. -"State Machine Breaking" for any changes that result in a different AppState given same genesisState and txList. +# Types of Changes (Stanzas) + +* **Features**: New functionality additions +* **Improvements**: Enhancements to existing features +* **Deprecated**: Features planned for removal +* **Bug Fixes**: Corrections to existing functionality +* **Client Breaking**: Breaking changes to Protobuf/gRPC/REST APIs affecting end users +* **CLI Breaking**: Breaking changes to command line interfaces +* **API Breaking**: Breaking changes to exported APIs used by developers +* **State Machine Breaking**: Changes affecting AppState determinism
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
CHANGELOG.md(1 hunks)x/accounts/keeper.go(2 hunks)x/accounts/keeper_test.go(2 hunks)x/auth/migrations/v5/migrate.go(2 hunks)x/auth/types/keys.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
`**/*.go`: Review the Golang code for conformity with the Ub...
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
x/auth/types/keys.gox/auth/migrations/v5/migrate.gox/accounts/keeper_test.gox/accounts/keeper.go
`**/*_test.go`: "Assess the unit test code assessing suffici...
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"
x/accounts/keeper_test.go
`**/*.md`: "Assess the documentation for misspellings, gramm...
**/*.md: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"
CHANGELOG.md
🪛 golangci-lint (1.62.2)
x/accounts/keeper_test.go
6-6: File is not gci-ed with --skip-generated -s standard -s default -s prefix(cosmossdk.io) -s prefix(github.com/cosmos/cosmos-sdk) --custom-order
(gci)
7-7: ST1019: package "github.com/cosmos/gogoproto/types" is being imported more than once
(stylecheck)
8-8: ST1019(related information): other import of "github.com/cosmos/gogoproto/types"
(stylecheck)
x/accounts/keeper.go
26-26: File is not gci-ed with --skip-generated -s standard -s default -s prefix(cosmossdk.io) -s prefix(github.com/cosmos/cosmos-sdk) --custom-order
(gci)
🪛 GitHub Actions: Lint
x/accounts/keeper_test.go
[error] 6-6: File is not gci-ed with --skip-generated -s standard -s default -s prefix(cosmossdk.io) -s prefix(github.com/cosmos/cosmos-sdk) --custom-order (gci)
[error] 13-13: File is not gci-ed with --skip-generated -s standard -s default -s prefix(cosmossdk.io) -s prefix(github.com/cosmos/cosmos-sdk) --custom-order (gci)
[error] 7-7: ST1019: package "github.com/cosmos/gogoproto/types" is being imported more than once (stylecheck)
x/accounts/keeper.go
[error] 11-11: File is not gci-ed with --skip-generated -s standard -s default -s prefix(cosmossdk.io) -s prefix(github.com/cosmos/cosmos-sdk) --custom-order (gci)
[error] 26-26: File is not gci-ed with --skip-generated -s standard -s default -s prefix(cosmossdk.io) -s prefix(github.com/cosmos/cosmos-sdk) --custom-order (gci)
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: tests (03)
- GitHub Check: tests (02)
- GitHub Check: tests (01)
- GitHub Check: tests (00)
- GitHub Check: test-simapp-v2
- GitHub Check: test-system-v2
- GitHub Check: Analyze
- GitHub Check: Summary
🔇 Additional comments (8)
x/auth/types/keys.go (1)
32-33: LGTM!The new variable
LegacyGlobalAccountNumberKeyis well-defined and properly documented.x/auth/migrations/v5/migrate.go (1)
15-15: LGTM!The migration code correctly uses the imported constant
authtypes.LegacyGlobalAccountNumberKeyinstead of a local variable.Also applies to: 39-39
x/accounts/keeper_test.go (1)
96-122: LGTM!The test function thoroughly validates the
NextAccountNumberfunctionality, including the legacy account number retrieval.CHANGELOG.md (5)
1-3: LGTM: Clear and concise title and descriptionThe changelog title and description provide a clear overview of the project and its purpose.
39-41: LGTM: Clear unreleased section headerThe unreleased section header is well formatted and includes a clear note about module changelogs.
43-49: LGTM: Well organized features sectionThe features section is well organized with clear bullet points and issue references.
51-56: LGTM: Clear bug fixes sectionThe bug fixes section clearly documents the issues fixed with proper references.
57-60: LGTM: Clear removed sectionThe removed section clearly documents removed functionality with proper references.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
x/accounts/keeper_test.go(2 hunks)x/auth/migrations/v5/migrate_test.go(2 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
`**/*.go`: Review the Golang code for conformity with the Ub...
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
x/accounts/keeper_test.gox/auth/migrations/v5/migrate_test.go
`**/*_test.go`: "Assess the unit test code assessing suffici...
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"
x/accounts/keeper_test.gox/auth/migrations/v5/migrate_test.go
🪛 golangci-lint (1.62.2)
x/accounts/keeper_test.go
6-6: File is not gci-ed with --skip-generated -s standard -s default -s prefix(cosmossdk.io) -s prefix(github.com/cosmos/cosmos-sdk) --custom-order
(gci)
x/auth/migrations/v5/migrate_test.go
10-10: File is not gci-ed with --skip-generated -s standard -s default -s prefix(cosmossdk.io) -s prefix(github.com/cosmos/cosmos-sdk) --custom-order
(gci)
🪛 GitHub Actions: Lint
x/accounts/keeper_test.go
[error] 6-6: File is not gci-ed with --skip-generated -s standard -s default -s prefix(cosmossdk.io) -s prefix(github.com/cosmos/cosmos-sdk) --custom-order (gci)
[error] 12-12: File is not gci-ed with --skip-generated -s standard -s default -s prefix(cosmossdk.io) -s prefix(github.com/cosmos/cosmos-sdk) --custom-order (gci)
⏰ Context from checks skipped due to timeout of 90000ms (15)
- GitHub Check: tests (01)
- GitHub Check: test-x-mint
- GitHub Check: test-x-epochs
- GitHub Check: test-x-consensus
- GitHub Check: test-tools-benchmark
- GitHub Check: test-x-upgrade
- GitHub Check: test-x-tx
- GitHub Check: test-x-staking
- GitHub Check: test-x-accounts
- GitHub Check: test-simapp-v2
- GitHub Check: test-sim-nondeterminism
- GitHub Check: test-integration
- GitHub Check: test-system-v2
- GitHub Check: Analyze
- GitHub Check: Summary
🔇 Additional comments (3)
x/auth/migrations/v5/migrate_test.go (1)
26-26: LGTM!The change improves code clarity by making the source of the constant explicit through the
authtypespackage.x/accounts/keeper_test.go (2)
6-8: Fix duplicate import of gogoproto/types package.The package "github.com/cosmos/gogoproto/types" is being imported twice.
🧰 Tools
🪛 golangci-lint (1.62.2)
6-6: File is not
gci-ed with --skip-generated -s standard -s default -s prefix(cosmossdk.io) -s prefix(github.com/cosmos/cosmos-sdk) --custom-order(gci)
🪛 GitHub Actions: Lint
[error] 6-6: File is not
gci-ed with --skip-generated -s standard -s default -s prefix(cosmossdk.io) -s prefix(github.com/cosmos/cosmos-sdk) --custom-order (gci)
95-121: LGTM! Comprehensive test coverage.The test thoroughly validates the
NextAccountNumbermethod by:
- Testing initial zero account number scenario
- Testing legacy global account number retrieval
- Including proper error handling and assertions
Co-authored-by: yihuang <huang@crypto.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
x/accounts/keeper.go (1)
140-161: Consider adding error documentation.The changes to
NextAccountNumbermethod look good, but consider documenting the error types that can be returned in the function's documentation.Add documentation for error types:
func (k Keeper) NextAccountNumber( ctx context.Context, -) (accNum uint64, err error) { +) (accNum uint64, err error) { + // Returns collections.ErrNotFound if no account number exists and legacy account number retrieval fails + // Returns error if setting the next account number fails
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
x/accounts/keeper.go(3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.go`: Review the Golang code for conformity with the Ub...
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
x/accounts/keeper.go
⏰ Context from checks skipped due to timeout of 90000ms (11)
- GitHub Check: tests (03)
- GitHub Check: tests (02)
- GitHub Check: tests (01)
- GitHub Check: tests (00)
- GitHub Check: test-simapp-v2
- GitHub Check: test-system-v2
- GitHub Check: test-sim-nondeterminism
- GitHub Check: test-integration
- GitHub Check: Analyze
- GitHub Check: build (amd64)
- GitHub Check: Summary
🔇 Additional comments (3)
x/accounts/keeper.go (3)
12-12: LGTM! Import statements are well-organized.The new imports
gogotypesandauthtypesare correctly added and necessary for the legacy account number support.Also applies to: 26-26
127-138: LGTM! Error handling follows best practices.The
GetAccountNumberLegacymethod properly handles errors and provides descriptive error messages. The implementation correctly retrieves and unmarshals the legacy account number from the KV store.
144-150: LGTM! Fallback mechanism for historical states.The fallback to legacy account numbers when
collections.ErrNotFoundis encountered is well-implemented. The comments clearly explain the behavior and its implications for state machine migration.
|
Is it still happening if your put x/auth first in the module manager: cosmos-sdk/types/module/module.go Line 442 in 9816440 |
yihuang
changed the title
the issue is when using latest binary to query historical state that is before the migration, to keep it compatible with that situation, because account-number is very commonly used. |
Okay, yeah adding this fallback for such a common query makes sense. Usually, we tell people to use the previous binary, but that exception makes sense. |
|
This should be backported to 0.53 btw |
|
@julienrbrt will this be released with 0.53? i quickly checked and i dont see it included yet in 0.53 rc |
…state (cosmos#23743) Co-authored-by: HuangYi <huang@crypto.com>
6 participants
Description
Closes: #23741
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
Refactor