Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor(x/auth): Fix system test #20531

Merged
merged 40 commits into from
Jun 12, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
40 commits
Select commit Hold shift + click to select a range
214605b
change initial version to 0
sontrinh16 May 31, 2024
aefee6a
Revert "change initial version to 0"
sontrinh16 Jun 1, 2024
495ab79
Merge branch 'main' of https://github.com/cosmos/cosmos-sdk
sontrinh16 Jun 1, 2024
f761eb1
Merge branch 'main' of https://github.com/cosmos/cosmos-sdk
sontrinh16 Jun 3, 2024
13cd0df
Merge branch 'main' of https://github.com/cosmos/cosmos-sdk
sontrinh16 Jun 3, 2024
9c3170a
fix system test
sontrinh16 Jun 3, 2024
c32693e
Merge branch 'main' into son/quick_fix
sontrinh16 Jun 3, 2024
af58812
move acc num sync to upgarde handler
sontrinh16 Jun 4, 2024
3b33ae9
revert main logic for now
sontrinh16 Jun 4, 2024
e6afd5e
Merge branch 'son/quick_fix' of https://github.com/cosmos/cosmos-sdk …
sontrinh16 Jun 4, 2024
521c566
minor
sontrinh16 Jun 4, 2024
c1a08da
fix accounts init genesis problem and fix system and sim test
sontrinh16 Jun 4, 2024
dfebc64
go mod tidy
sontrinh16 Jun 4, 2024
d1244ae
minor
sontrinh16 Jun 4, 2024
94e9b2b
Merge branch 'main' into son/quick_fix
sontrinh16 Jun 4, 2024
1d9cfcf
address comments
sontrinh16 Jun 5, 2024
d4d9a11
Merge branch 'son/quick_fix' of https://github.com/cosmos/cosmos-sdk …
sontrinh16 Jun 5, 2024
ba123e6
Merge branch 'main' into son/quick_fix
sontrinh16 Jun 9, 2024
c43114f
make deprecate
sontrinh16 Jun 9, 2024
e5206f6
add test case
sontrinh16 Jun 10, 2024
179641d
address comments
sontrinh16 Jun 10, 2024
663ef59
update upgrade doc
sontrinh16 Jun 10, 2024
610ef6f
minor
sontrinh16 Jun 10, 2024
7388cbd
add test for upgrade logic
sontrinh16 Jun 10, 2024
3441aae
minor
sontrinh16 Jun 10, 2024
e68685a
duplicate comment
sontrinh16 Jun 10, 2024
51742cc
Merge branch 'main' into son/quick_fix
sontrinh16 Jun 10, 2024
f60aa6b
Update simapp/upgrades_test.go
sontrinh16 Jun 10, 2024
49ef1e3
fix simapp test
sontrinh16 Jun 10, 2024
5f9818b
Merge branch 'son/quick_fix' of https://github.com/cosmos/cosmos-sdk …
sontrinh16 Jun 10, 2024
053ae26
address comment
sontrinh16 Jun 11, 2024
eae8154
changelog, add detail err log
sontrinh16 Jun 11, 2024
3ef3407
address comments
sontrinh16 Jun 11, 2024
c9aa0f2
lint
sontrinh16 Jun 11, 2024
6cf6894
fix upgrading doc
sontrinh16 Jun 11, 2024
c22d05c
Merge branch 'main' into son/quick_fix
sontrinh16 Jun 11, 2024
75d5bab
more lint
sontrinh16 Jun 12, 2024
c2c2b1c
fix conflict
sontrinh16 Jun 12, 2024
69d6738
lint
sontrinh16 Jun 12, 2024
1d1b745
lint more
sontrinh16 Jun 12, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 2 additions & 7 deletions UPGRADING.md
Original file line number Diff line number Diff line change
Expand Up @@ -254,15 +254,10 @@ For modules that have migrated, verify you are checking against `collections.Err

#### `x/accounts`

Accounts's AccountNumber will be used as a global account number tracking replacing Auth legacy AccountNumber. Must set accounts's AccountNumber with auth's AccountNumber value in upgrade handler:
Accounts's AccountNumber will be used as a global account number tracking replacing Auth legacy AccountNumber. Must set accounts's AccountNumber with auth's AccountNumber value in upgrade handler. This is done through auth keeper MigrateAccountNumber function.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would add that this should be done in the ugprade handler of an app when wanting using x/accounts.

Can we have this as a diff of upgrade.go like we did for protocolpool?


```go
currentAccNum, err := app.AuthKeeper.RemoveLegacyAccountNumber(ctx)
if err != nil {
return nil, err
}

err = app.AccountsKeeper.InitAccountNumberSeqUnsafe(ctx, currentAccNum)
err := app.AuthKeeper.MigrateAccountNumber(ctx)
if err != nil {
return nil, err
}
Expand Down
13 changes: 1 addition & 12 deletions simapp/upgrades.go
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ func (app SimApp) RegisterUpgradeHandlers() {
UpgradeName,
func(ctx context.Context, _ upgradetypes.Plan, fromVM appmodule.VersionMap) (appmodule.VersionMap, error) {
// sync accounts and auth module account number
sontrinh16 marked this conversation as resolved.
Show resolved Hide resolved
err := app.syncAccountNumber(ctx)
err := app.AuthKeeper.MigrateAccountNumberUnsafe(ctx)
if err != nil {
return nil, err
}
Expand Down Expand Up @@ -56,14 +56,3 @@ func (app SimApp) RegisterUpgradeHandlers() {
app.SetStoreLoader(upgradetypes.UpgradeStoreLoader(upgradeInfo.Height, &storeUpgrades))
}
}

func (app SimApp) syncAccountNumber(ctx context.Context) error {
currentAccNum, err := app.AuthKeeper.RemoveLegacyAccountNumber(ctx)
if err != nil {
return err
}

err = app.AccountsKeeper.InitAccountNumberSeqUnsafe(ctx, currentAccNum)

return err
}
2 changes: 1 addition & 1 deletion simapp/upgrades_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ func TestSyncAccountNumber(t *testing.T) {
require.NoError(t, err)
require.Equal(t, uint64(10), num)

app.syncAccountNumber(ctx)
app.AuthKeeper.MigrateAccountNumberUnsafe(ctx)
sontrinh16 marked this conversation as resolved.
Show resolved Hide resolved

// make sure the DB entry for this key is deleted
v, err = store.Get(bytesKey)
Expand Down
15 changes: 15 additions & 0 deletions x/auth/keeper/keeper.go
Original file line number Diff line number Diff line change
Expand Up @@ -338,3 +338,18 @@ func (ak AccountKeeper) NonAtomicMsgsExec(ctx context.Context, signer sdk.AccAdd

return msgResponses, nil
}

// MigrateAccountNumberUnsafe migrates auth's account number to accounts's account number
// and delete store entry for auth legacy GlobalAccountNumberKey.
//
// Should only use in an upgrade handler for migrate process.
func (ak AccountKeeper) MigrateAccountNumberUnsafe(ctx context.Context) error {
sontrinh16 marked this conversation as resolved.
Show resolved Hide resolved
currentAccNum, err := ak.RemoveLegacyAccountNumber(ctx)
if err != nil {
return err
}

err = ak.AccountsModKeeper.InitAccountNumberSeqUnsafe(ctx, currentAccNum)

return err
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review the usage of MigrateAccountNumberUnsafe.

The MigrateAccountNumberUnsafe method is critical for the migration process but lacks detailed error handling and logging. Consider adding more robust error handling and detailed logging to trace the migration steps and potential issues.

Loading