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

Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>

CHANGELOG.md

@@ -200,6 +200,7 @@ Every module contains its own CHANGELOG.md. Please refer to the module you are i
 
 * (simapp) [#19146](https://github.com/cosmos/cosmos-sdk/pull/19146) Replace `--v` CLI option with `--validator-count`/`-n`.
 * (module) [#19370](https://github.com/cosmos/cosmos-sdk/pull/19370) Deprecate `module.Configurator`, use `appmodule.HasMigrations` and `appmodule.HasServices` instead from Core API.
+* (x/auth) [#20531](https://github.com/cosmos/cosmos-sdk/pull/20531) Deprecate auth keeper `NextAccountNumber`, use `keeper.AccountsModKeeper.NextAccountNumber` instead.
 
 ## [v0.50.7](https://github.com/cosmos/cosmos-sdk/releases/tag/v0.50.7) - 2024-06-04
 

UPGRADING.md

@@ -252,6 +252,19 @@ Most of Cosmos SDK modules have migrated to [collections](https://docs.cosmos.ne
 Many functions have been removed due to this changes as the API can be smaller thanks to collections.
 For modules that have migrated, verify you are checking against `collections.ErrNotFound` when applicable.
 
+#### `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. This is done through auth keeper MigrateAccountNumber function.
+
+```go
+import authkeeper "cosmossdk.io/x/auth/keeper" 
+...
+err := authkeeper.MigrateAccountNumberUnsafe(ctx, &app.AuthKeeper)
+if err != nil {
+	return nil, err
+}
+```
+
 #### `x/auth`
 
 Auth was spun out into its own `go.mod`. To import it use `cosmossdk.io/x/auth`

crypto/codec/amino.go

@@ -4,6 +4,7 @@ import (
 	"github.com/cometbft/cometbft/crypto/sr25519"
 
 	"cosmossdk.io/core/legacy"
+
 	bls12_381 "github.com/cosmos/cosmos-sdk/crypto/keys/bls12_381"
 	"github.com/cosmos/cosmos-sdk/crypto/keys/ed25519"
 	kmultisig "github.com/cosmos/cosmos-sdk/crypto/keys/multisig"

crypto/codec/cmt.go

@@ -6,6 +6,7 @@ import (
 	"github.com/cometbft/cometbft/crypto/encoding"
 
 	"cosmossdk.io/errors"
+
 	bls12_381 "github.com/cosmos/cosmos-sdk/crypto/keys/bls12_381"
 	"github.com/cosmos/cosmos-sdk/crypto/keys/ed25519"
 	"github.com/cosmos/cosmos-sdk/crypto/keys/secp256k1"

simapp/upgrades.go

@@ -6,6 +6,7 @@ import (
 	"cosmossdk.io/core/appmodule"
 	storetypes "cosmossdk.io/store/types"
 	"cosmossdk.io/x/accounts"
+	authkeeper "cosmossdk.io/x/auth/keeper"
 	epochstypes "cosmossdk.io/x/epochs/types"
 	protocolpooltypes "cosmossdk.io/x/protocolpool/types"
 	upgradetypes "cosmossdk.io/x/upgrade/types"
@@ -25,6 +26,12 @@ func (app SimApp) RegisterUpgradeHandlers() {
 	app.UpgradeKeeper.SetUpgradeHandler(
 		UpgradeName,
 		func(ctx context.Context, _ upgradetypes.Plan, fromVM appmodule.VersionMap) (appmodule.VersionMap, error) {
+			// sync accounts and auth module account number
+			err := authkeeper.MigrateAccountNumberUnsafe(ctx, &app.AuthKeeper)
+			if err != nil {
+				return nil, err
+			}
+
 			return app.ModuleManager.RunMigrations(ctx, app.Configurator(), fromVM)
 		},
 	)

simapp/upgrades_test.go

@@ -0,0 +1,54 @@
+package simapp
+
+import (
+	"testing"
+
+	cmtproto "github.com/cometbft/cometbft/api/cometbft/types/v1"
+	"github.com/stretchr/testify/require"
+
+	"cosmossdk.io/collections"
+	authkeeper "cosmossdk.io/x/auth/keeper"
+	authtypes "cosmossdk.io/x/auth/types"
+)
+
+// TestSyncAccountNumber tests if accounts module account number is set correctly with the value get from auth.
+// Also check if the store entry for auth GlobalAccountNumberKey is successfully deleted.
+func TestSyncAccountNumber(t *testing.T) {
+	app := Setup(t, true)
+	ctx := app.NewUncachedContext(true, cmtproto.Header{})
+
+	bytesKey := authtypes.GlobalAccountNumberKey
+	store := app.AuthKeeper.KVStoreService.OpenKVStore(ctx)
+
+	// initially there is no value set yet
+	v, err := store.Get(bytesKey)
+	require.NoError(t, err)
+	require.Nil(t, v)
+
+	// set value for legacy account number
+	v, err = collections.Uint64Value.Encode(10)
+	require.NoError(t, err)
+	err = store.Set(bytesKey, v)
+	require.NoError(t, err)
+
+	// make sure value are updated
+	v, err = store.Get(bytesKey)
+	require.NoError(t, err)
+	require.NotEmpty(t, v)
+	num, err := collections.Uint64Value.Decode(v)
+	require.NoError(t, err)
+	require.Equal(t, uint64(10), num)
+
+	err = authkeeper.MigrateAccountNumberUnsafe(ctx, &app.AuthKeeper)
+	require.NoError(t, err)
+
+	// make sure the DB entry for this key is deleted
+	v, err = store.Get(bytesKey)
+	require.NoError(t, err)
+	require.Nil(t, v)
+
+	// check if accounts's account number is updated
+	currentNum, err := app.AccountsKeeper.AccountNumber.Peek(ctx)
+	require.NoError(t, err)
+	require.Equal(t, uint64(10), currentNum)
+}

x/accounts/genesis.go

@@ -64,19 +64,28 @@ func (k Keeper) exportAccount(ctx context.Context, addr []byte, accType string,
 }
 
 func (k Keeper) ImportState(ctx context.Context, genState *v1.GenesisState) error {
-	err := k.AccountNumber.Set(ctx, genState.AccountNumber)
-	if err != nil {
-		return err
-	}
-
+	var largestNum *uint64
+	var err error
 	// import accounts
 	for _, acc := range genState.Accounts {
 		err = k.importAccount(ctx, acc)
 		if err != nil {
 			return fmt.Errorf("%w: %s", err, acc.Address)
 		}
+
+		accNum := acc.AccountNumber
+
+		if largestNum == nil || *largestNum < accNum {
+			largestNum = &accNum
+		}
 	}
-	return nil
+
+	if largestNum != nil {
+		// set the account number to the highest account number to avoid duplicate account number
+		err = k.AccountNumber.Set(ctx, *largestNum)
+	}
+
+	return err
 }
 
 func (k Keeper) importAccount(ctx context.Context, acc *v1.GenesisAccount) error {

x/accounts/genesis_test.go

@@ -51,6 +51,23 @@ func TestGenesis(t *testing.T) {
 	resp, err = k.Query(ctx, addr2, &types.DoubleValue{})
 	require.NoError(t, err)
 	require.Equal(t, &types.UInt64Value{Value: 20}, resp)
+
+	// reset state
+	k, ctx = newKeeper(t, func(deps implementation.Dependencies) (string, implementation.Account, error) {
+		acc, err := NewTestAccount(deps)
+		return testAccountType, acc, err
+	})
+
+	// modify the accounts account number
+	state.Accounts[0].AccountNumber = 99
+
+	err = k.ImportState(ctx, state)
+	require.NoError(t, err)
+
+	currentAccNum, err := k.AccountNumber.Peek(ctx)
+	require.NoError(t, err)
+	// AccountNumber should be set to the highest account number in the genesis state
+	require.Equal(t, uint64(99), currentAccNum)
 }
 
 func TestImportAccountError(t *testing.T) {

x/auth/keeper/keeper.go

@@ -102,6 +102,10 @@ type AccountKeeper struct {
 	Schema collections.Schema
 	Params collections.Item[types.Params]
 
+	// only use for upgrade handler
+	//
+	// Deprecated: move to accounts module accountNumber
+	accountNumber collections.Sequence
 	// Accounts key: AccAddr | value: AccountI | index: AccountsIndex
 	Accounts *collections.IndexedMap[sdk.AccAddress, sdk.AccountI, AccountsIndexes]
 }
@@ -135,6 +139,7 @@ func NewAccountKeeper(
 		permAddrs:         permAddrs,
 		authority:         authority,
 		Params:            collections.NewItem(sb, types.ParamsKey, "params", codec.CollValue[types.Params](cdc)),
+		accountNumber:     collections.NewSequence(sb, types.GlobalAccountNumberKey, "account_number"),
 		Accounts:          collections.NewIndexedMap(sb, types.AddressStoreKeyPrefix, "accounts", sdk.AccAddressKey, codec.CollInterfaceValue[sdk.AccountI](cdc), NewAccountIndexes(sb)),
 	}
 	schema, err := sb.Build()
@@ -145,6 +150,22 @@ func NewAccountKeeper(
 	return ak
 }
 
+// removeLegacyAccountNumberUnsafe is used for migration purpose only. It deletes the sequence in the DB
+// and returns the last value used on success.
+// Deprecated
+func (ak AccountKeeper) removeLegacyAccountNumberUnsafe(ctx context.Context) (uint64, error) {
+	accNum, err := ak.accountNumber.Peek(ctx)
+	if err != nil {
+		return 0, err
+	}
+
+	// Delete DB entry for legacy account number
+	store := ak.KVStoreService.OpenKVStore(ctx)
+	err = store.Delete(types.GlobalAccountNumberKey.Bytes())
+
+	return accNum, err
+}
+
 // GetAuthority returns the x/auth module's authority.
 func (ak AccountKeeper) GetAuthority() string {
 	return ak.authority
@@ -317,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 migrating account number.
+func MigrateAccountNumberUnsafe(ctx context.Context, ak *AccountKeeper) error {
+	currentAccNum, err := ak.removeLegacyAccountNumberUnsafe(ctx)
+	if err != nil {
+		return fmt.Errorf("failed to migrate account number: %w", err)
+	}
+
+	err = ak.AccountsModKeeper.InitAccountNumberSeqUnsafe(ctx, currentAccNum)
+
+	return err
+}

x/auth/keeper/migrations.go

@@ -3,7 +3,6 @@ package keeper
 import (
 	"context"
 
-	"cosmossdk.io/collections"
 	v5 "cosmossdk.io/x/auth/migrations/v5"
 	"cosmossdk.io/x/auth/types"
 
@@ -13,16 +12,11 @@ import (
 // Migrator is a struct for handling in-place store migrations.
 type Migrator struct {
 	keeper AccountKeeper
-	// accNum is use in v4 to v5 and v5 to v6 migration
-	accNum collections.Sequence
 }
 
 // NewMigrator returns a new Migrator.
 func NewMigrator(keeper AccountKeeper) Migrator {
-	sb := collections.NewSchemaBuilder(keeper.Environment.KVStoreService)
-	accNumSeq := collections.NewSequence(sb, types.GlobalAccountNumberKey, "account_number")
-
-	return Migrator{keeper: keeper, accNum: accNumSeq}
+	return Migrator{keeper: keeper}
 }
 
 // Migrate1to2 migrates from version 1 to 2.
@@ -48,18 +42,7 @@ func (m Migrator) Migrate3to4(ctx context.Context) error {
 // It migrates the GlobalAccountNumber from being a protobuf defined value to a
 // big-endian encoded uint64, it also migrates it to use a more canonical prefix.
 func (m Migrator) Migrate4To5(ctx context.Context) error {
-	return v5.Migrate(ctx, m.keeper.KVStoreService, m.accNum)
-}
-
-// Migrate5To6 migrates the x/auth module state from the consensus version 5 to 6.
-// It migrates the GlobalAccountNumber from x/auth to x/accounts .
-func (m Migrator) Migrate5To6(ctx context.Context) error {
-	currentAccNum, err := m.accNum.Peek(ctx)
-	if err != nil {
-		return err
-	}
-
-	return m.keeper.AccountsModKeeper.InitAccountNumberSeqUnsafe(ctx, currentAccNum)
+	return v5.Migrate(ctx, m.keeper.KVStoreService, m.keeper.accountNumber)
 }
 
 // V45SetAccount implements V45_SetAccount

x/auth/module.go

@@ -27,7 +27,7 @@ import (
 
 // ConsensusVersion defines the current x/auth module consensus version.
 const (
-	ConsensusVersion = 6
+	ConsensusVersion = 5
 	GovModuleName    = "gov"
 )
 
@@ -113,9 +113,6 @@ func (am AppModule) RegisterMigrations(mr appmodule.MigrationRegistrar) error {
 	if err := mr.Register(types.ModuleName, 4, m.Migrate4To5); err != nil {
 		return fmt.Errorf("failed to migrate x/%s from version 4 to 5: %w", types.ModuleName, err)
 	}
-	if err := mr.Register(types.ModuleName, 5, m.Migrate5To6); err != nil {
-		return fmt.Errorf("failed to migrate x/%s from version 5 to 6: %w", types.ModuleName, err)
-	}
 
 	return nil
 }

x/genutil/client/cli/init.go

@@ -9,10 +9,14 @@ import (
 	"os"
 	"path/filepath"
 
-	errorsmod "cosmossdk.io/errors"
-	"cosmossdk.io/math/unsafe"
 	cfg "github.com/cometbft/cometbft/config"
 	cmttypes "github.com/cometbft/cometbft/types"
+	"github.com/cosmos/go-bip39"
+	"github.com/spf13/cobra"
+
+	errorsmod "cosmossdk.io/errors"
+	"cosmossdk.io/math/unsafe"
+
 	"github.com/cosmos/cosmos-sdk/client"
 	"github.com/cosmos/cosmos-sdk/client/flags"
 	"github.com/cosmos/cosmos-sdk/client/input"
@@ -21,8 +25,6 @@ import (
 	"github.com/cosmos/cosmos-sdk/version"
 	"github.com/cosmos/cosmos-sdk/x/genutil"
 	"github.com/cosmos/cosmos-sdk/x/genutil/types"
-	"github.com/cosmos/go-bip39"
-	"github.com/spf13/cobra"
 )
 
 const (