fix(auth): support legacy global AccountNumber when query historical state (port #23743) #24533

mmsqe · 2025-04-16T04:25:07Z

Description

for more info

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...

included the correct type prefix in the PR title, you can find examples of the prefixes below:
confirmed ! in the type prefix if API or client breaking change
targeted the correct branch (see PR Targeting)
provided a link to the relevant issue or specification
reviewed "Files changed" and left comments if necessary
included the necessary unit and integration tests
added a changelog entry to CHANGELOG.md
updated the relevant documentation or specification, including comments for documenting Go code
confirmed all CI checks have passed

Reviewers 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...

confirmed the correct type prefix in the PR title
confirmed all author checklist items have been addressed
reviewed state machine logic, API design and naming, documentation is accurate, tests and test coverage

Summary by CodeRabbit

Bug Fixes
- Resolved an issue where the x/auth module did not support a legacy global AccountNumber, ensuring compatibility with historical states.

…state (cosmos#23743) Co-authored-by: HuangYi <[email protected]>

ironbird-prod · 2025-04-16T04:25:10Z

Ironbird - launch a network

To use Ironbird, you can use the following commands:

/ironbird start OR /ironbird start --load-test-config= - Launch a testnet with the specified chain and load test configuration.
/ironbird chains - List of chain images that ironbird can use to spin-up testnet
/ironbird loadtests - List of load test modes that ironbird can run against testnet

Custom Load Test Configuration

You can provide a custom load test configuration using the `--load-test-config=` flag:

/ironbird start cosmos --load-test-config={
  "block_gas_limit_target": 0.75,
  "num_of_blocks": 50,
  "msgs": [
    {"weight": 0.3, "type": "MsgSend"},
    {"weight": 0.3, "type": "MsgMultiSend"},
	{"weight": 0.4, "type": "MsgArr", "ContainedType": "MsgSend", "NumMsgs": 3300}
  ]
}

Use /ironbird loadtests to see more examples.

coderabbitai · 2025-04-16T04:33:37Z

📝 Walkthrough

Walkthrough

The changes introduce support for a legacy global account number in the x/auth module. A new method retrieves the legacy account number from the old key, and the logic for incrementing the account number now falls back to this legacy value if the new storage key is not found. A new exported constant for the legacy key is added to the types package. Migration and test code are updated to reference this exported key. The changelog is updated with a bug fix entry describing these changes.

Changes

File(s)	Change Summary
CHANGELOG.md	Added a bug fix entry under "Unreleased" noting x/auth now supports legacy global AccountNumber.
x/auth/keeper/keeper.go	Added `GetAccountNumberLegacy` method; updated `NextAccountNumber` to use legacy key as fallback and handle errors.
x/auth/types/keys.go	Added exported variable `LegacyGlobalAccountNumberKey` for the legacy account number key.
x/auth/migrations/v5/migrate.go x/auth/migrations/v5/migrate_test.go	Updated to use `authtypes.LegacyGlobalAccountNumberKey` instead of a local definition for the legacy key.

Sequence Diagram(s)

sequenceDiagram
    participant Client
    participant AccountKeeper
    participant Store

    Client->>AccountKeeper: NextAccountNumber(ctx)
    AccountKeeper->>Store: Get(new account number key)
    alt Key found
        Store-->>AccountKeeper: current account number
    else Key not found (legacy state)
        AccountKeeper->>Store: Get(legacy account number key)
        Store-->>AccountKeeper: legacy account number
    end
    AccountKeeper->>Store: Set(new account number key, incremented value)
    AccountKeeper-->>Client: return current account number

📜 Recent review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 0846f9a and 4cda041.

📒 Files selected for processing (5)

CHANGELOG.md (1 hunks)
x/auth/keeper/keeper.go (2 hunks)
x/auth/migrations/v5/migrate.go (2 hunks)
x/auth/migrations/v5/migrate_test.go (2 hunks)
x/auth/types/keys.go (1 hunks)

🧰 Additional context used

🧬 Code Graph Analysis (3)

x/auth/migrations/v5/migrate_test.go (1)

x/auth/types/keys.go (1)

LegacyGlobalAccountNumberKey (36-36)

x/auth/migrations/v5/migrate.go (1)

x/auth/types/keys.go (1)

LegacyGlobalAccountNumberKey (36-36)

x/auth/keeper/keeper.go (2)

x/auth/ante/expected_keepers.go (1)

AccountKeeper (15-21)

x/auth/types/keys.go (1)

LegacyGlobalAccountNumberKey (36-36)

⏰ Context from checks skipped due to timeout of 90000ms (2)

GitHub Check: Analyze
GitHub Check: Summary

🔇 Additional comments (11)

x/auth/types/keys.go (1)

34-37: Well-defined legacy key for supporting historical state queries.

This addition is good - centralizing the definition of the legacy global account number key. It enables proper backward compatibility when querying historical states that might still use this key format for storing the global account number.

CHANGELOG.md (1)

106-106: Clear changelog entry documenting the bug fix.

This entry properly documents the support for the legacy global AccountNumber key. The reference to the original PR (#23741) is helpful for future reference.

x/auth/migrations/v5/migrate.go (3)

11-12: Good use of the centralized auth types import.

Using the imported type package ensures consistency across the codebase.

16-16: Proper use of the legacy key from auth types.

Replaced inline definition with the centralized key constant from the auth types package, which improves maintainability and consistency.

40-40: Consistent use of the legacy key constant.

Using the same constant for both accessing and deleting the legacy key ensures consistency.

x/auth/migrations/v5/migrate_test.go (2)

11-12: Good use of the centralized auth types import in tests.

Consistent with the changes in the implementation code, importing and using the auth types package in tests is a good practice.

26-26: Proper test coverage for the legacy key usage.

The test correctly uses the same constant from auth types to set up the test scenario, ensuring the migration code is properly tested.

x/auth/keeper/keeper.go (4)

9-10: Added required gogoproto import for UInt64Value.

This import is needed for the new legacy account number retrieval method.

186-197: Well-implemented method for retrieving the legacy account number.

The new GetAccountNumberLegacy method properly handles:

Opening the store

Getting the value using the legacy key

Unmarshaling the protobuf UInt64Value

Returning appropriate errors with context

This is a clean implementation that focuses on backward compatibility.

202-209: Good fallback mechanism for historical state queries.

The implementation:

First tries to get the account number from the collections store

If not found (indicating a historical/legacy state), falls back to the legacy key

Provides clear comments explaining the fallback logic

This ensures backward compatibility when querying historical states.

214-216: Proper account number increment and storage.

Instead of using the collections sequence's Next method, this implementation manually increments the counter and stores it back, which works well with the fallback mechanism.

✨ Finishing Touches

📝 Generate Docstrings

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

‼️ IMPORTANT
Auto-reply has been disabled for this repository in the CodeRabbit settings. The CodeRabbit bot will not respond to your replies unless it is explicitly tagged.

Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
- @coderabbitai generate unit testing code for this file.
- @coderabbitai modularize this function.
PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
- @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
- @coderabbitai read src/utils.ts and generate unit testing code.
- @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
- @coderabbitai help me debug CodeRabbit configuration file.

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)

@coderabbitai pause to pause the reviews on a PR.
@coderabbitai resume to resume the paused reviews.
@coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
@coderabbitai full review to do a full review from scratch and review all the files again.
@coderabbitai summary to regenerate the summary of the PR.
@coderabbitai generate docstrings to generate docstrings for this PR.
@coderabbitai resolve resolve all the CodeRabbit review comments.
@coderabbitai configuration to show the current CodeRabbit configuration for the repository.
@coderabbitai help to get help.

Other keywords and placeholders

Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (`.coderabbit.yaml`)

You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
Please see the configuration documentation for more information.
If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

Visit our Documentation for detailed information on how to use CodeRabbit.
Join our Discord Community to get help, request features, and share feedback.
Follow us on X/Twitter for updates and announcements.

technicallyty · 2025-04-16T04:45:31Z

x/auth/keeper/keeper.go

+	n, err := collections.Item[uint64](ak.AccountNumber).Get(ctx)
+	if err != nil && errors.Is(err, collections.ErrNotFound) {
+		// this won't happen in the tip of production network,
+		// but can happen when query historical states,
+		// fallback to old key for backward-compatibility.
+		n, err = ak.GetAccountNumberLegacy(ctx)
+	}


can we add some tests for this new flow here?

technicallyty · 2025-04-16T04:46:18Z

x/auth/keeper/keeper.go

@@ -181,13 +183,38 @@ func (ak AccountKeeper) GetSequence(ctx context.Context, addr sdk.AccAddress) (u
 	return acc.GetSequence(), nil
 }

+func (ak AccountKeeper) GetAccountNumberLegacy(ctx context.Context) (uint64, error) {


this can be private

x/auth/keeper/keeper.go

 	if err != nil {
 		panic(err)
 	}
+
+	if err := ak.AccountNumber.Set(ctx, n+1); err != nil {
+		panic(err)


x/auth/migrations/v5/migrate_test.go

x/auth/keeper/keeper.go

x/auth/migrations/v5/migrate.go

aljo242 · 2025-04-17T16:22:53Z

x/auth/keeper/keeper_test.go

+	data, err := val.Marshal()
+	require.NoError(t, err)
+	store := storeService.OpenKVStore(ctx)
+	err = store.Set(types.LegacyGlobalAccountNumberKey, data)
+	require.NoError(t, err)
+
+	nextNum := ak.NextAccountNumber(ctx)
+	require.Equal(t, num, nextNum)


Can this be its own t.Run(...) testcase with a description?

execute order matters, seems separate case is better

x/auth/keeper/keeper_test.go

aljo242

LGTM i would just like some more clarity in the test cases as to what they are testing

CHANGELOG.md

fix(auth): support legacy global AccountNumber when query historical …

0af6689

…state (cosmos#23743) Co-authored-by: HuangYi <[email protected]>

github-project-automation bot added this to Interchain Public Works Apr 16, 2025

github-actions bot added the C:x/auth label Apr 16, 2025

lint

4cda041

technicallyty reviewed Apr 16, 2025

View reviewed changes

github-advanced-security bot found potential problems Apr 16, 2025

View reviewed changes

Apply suggestions from code review

e6083c3

mmsqe force-pushed the fix_acc_port branch from 7fbca52 to e6083c3 Compare April 16, 2025 05:55

aljo242 reviewed Apr 16, 2025

View reviewed changes

x/auth/migrations/v5/migrate_test.go Show resolved Hide resolved

aljo242 reviewed Apr 16, 2025

View reviewed changes

x/auth/keeper/keeper.go Show resolved Hide resolved

mmsqe and others added 3 commits April 16, 2025 23:09

Apply suggestions from code review

571273a

Merge remote-tracking branch 'origin/main' into fix_acc_port

3e16af8

Merge branch 'main' into fix_acc_port

ab7d294

aljo242 reviewed Apr 17, 2025

View reviewed changes

x/auth/migrations/v5/migrate.go Outdated Show resolved Hide resolved

aljo242 reviewed Apr 17, 2025

View reviewed changes

x/auth/migrations/v5/migrate.go Outdated Show resolved Hide resolved

aljo242 added 2 commits April 17, 2025 12:20

Update x/auth/migrations/v5/migrate.go

4a8bc0e

Update x/auth/migrations/v5/migrate.go

53ff29a

aljo242 reviewed Apr 17, 2025

View reviewed changes

x/auth/keeper/keeper_test.go Outdated Show resolved Hide resolved

aljo242 reviewed Apr 17, 2025

View reviewed changes

aljo242 and others added 6 commits April 17, 2025 12:55

Merge branch 'main' into fix_acc_port

042820d

Apply suggestions from code review

a918494

Merge remote-tracking branch 'origin/main' into fix_acc_port

17d215b

cleanup

a2bc59c

Merge remote-tracking branch 'origin/main' into fix_acc_port

39ad6d5

Merge branch 'main' into fix_acc_port

0fc8f76

aljo242 reviewed Apr 23, 2025

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

Update CHANGELOG.md

a25a8c1

aljo242 approved these changes Apr 23, 2025

View reviewed changes

aljo242 enabled auto-merge April 23, 2025 14:39

Merge branch 'main' into fix_acc_port

9a699fa

aljo242 added this pull request to the merge queue Apr 23, 2025

Merged via the queue into cosmos:main with commit 3b90ba4 Apr 23, 2025
45 checks passed

github-project-automation bot moved this to 🥳 Done in Interchain Public Works Apr 23, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(auth): support legacy global AccountNumber when query historical state (port #23743) #24533

fix(auth): support legacy global AccountNumber when query historical state (port #23743) #24533

mmsqe commented Apr 16, 2025 •

edited by coderabbitai bot

Loading

ironbird-prod bot commented Apr 16, 2025

coderabbitai bot commented Apr 16, 2025 •

edited

Loading

Walkthrough

Changes

Sequence Diagram(s)

Chat

CodeRabbit Commands (Invoked using PR comments)

Other keywords and placeholders

CodeRabbit Configuration File (`.coderabbit.yaml`)

Documentation and Community

technicallyty Apr 16, 2025

technicallyty Apr 16, 2025

Check warning

aljo242 Apr 17, 2025 •

edited

Loading

mmsqe Apr 17, 2025

aljo242 left a comment

fix(auth): support legacy global AccountNumber when query historical state (port #23743) #24533

fix(auth): support legacy global AccountNumber when query historical state (port #23743) #24533

Conversation

mmsqe commented Apr 16, 2025 • edited by coderabbitai bot Loading

Description

Author Checklist

Reviewers Checklist

Summary by CodeRabbit

ironbird-prod bot commented Apr 16, 2025

coderabbitai bot commented Apr 16, 2025 • edited Loading

Walkthrough

Changes

Sequence Diagram(s)

Chat

CodeRabbit Commands (Invoked using PR comments)

Other keywords and placeholders

CodeRabbit Configuration File (.coderabbit.yaml)

Documentation and Community

technicallyty Apr 16, 2025

Choose a reason for hiding this comment

technicallyty Apr 16, 2025

Choose a reason for hiding this comment

Check warning

aljo242 Apr 17, 2025 • edited Loading

Choose a reason for hiding this comment

mmsqe Apr 17, 2025

Choose a reason for hiding this comment

aljo242 left a comment

Choose a reason for hiding this comment

mmsqe commented Apr 16, 2025 •

edited by coderabbitai bot

Loading

coderabbitai bot commented Apr 16, 2025 •

edited

Loading

CodeRabbit Configuration File (`.coderabbit.yaml`)

aljo242 Apr 17, 2025 •

edited

Loading