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.

Sign up for GitHub

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/accounts)!: accounts and auth module use the same account number tracking #20405

Merged
merged 32 commits into from
Jun 3, 2024
Merged

refactor(x/accounts)!: accounts and auth module use the same account number tracking #20405

Show file tree
Hide file tree
Changes from 15 commits
Commits
Show all changes
32 commits
Select commit Hold shift + click to select a range
df6ab84
add migration for auth
sontrinh16 May 15, 2024
8d84d4a
clean up interface
sontrinh16 May 15, 2024
daf2b1c
re gen mock
sontrinh16 May 15, 2024
6f9370e
fix tests
sontrinh16 May 15, 2024
61d223e
lint
sontrinh16 May 15, 2024
0181991
Merge branch 'main' into auth_accounts_use_same_acc_number
sontrinh16 May 21, 2024
d140922
update migration logic
sontrinh16 May 23, 2024
f697324
register migrator
sontrinh16 May 23, 2024
724cb22
use upgrade handler instead
sontrinh16 May 23, 2024
142ced2
minor
sontrinh16 May 23, 2024
60c07e5
Merge branch 'auth_accounts_use_same_acc_number' of https://github.co…
sontrinh16 May 23, 2024
6f210de
clean up
sontrinh16 May 23, 2024
631fd73
remove stuffs
sontrinh16 May 23, 2024
79ec18a
Merge branch 'main' into auth_accounts_use_same_acc_number
sontrinh16 May 23, 2024
6d85cea
Merge branch 'main' into auth_accounts_use_same_acc_number
sontrinh16 May 27, 2024
a8e711c
add auth migration
sontrinh16 May 28, 2024
5b44990
remove code in upgrade handler
sontrinh16 May 28, 2024
8fdcd06
Merge branch 'auth_accounts_use_same_acc_number' of https://github.co…
sontrinh16 May 28, 2024
8feaaba
minor
sontrinh16 May 28, 2024
63f0f01
address comments
sontrinh16 May 29, 2024
91cce64
Merge branch 'main' into auth_accounts_use_same_acc_number
sontrinh16 May 29, 2024
cf8b56b
lint
sontrinh16 May 29, 2024
eb79394
Merge branch 'auth_accounts_use_same_acc_number' of https://github.co…
sontrinh16 May 29, 2024
db13681
remove unnecessary func
sontrinh16 May 29, 2024
b0ce6f9
cleanup
sontrinh16 May 29, 2024
effe701
remove config that got into this PR
sontrinh16 May 29, 2024
5da3500
Merge branch 'main' into auth_accounts_use_same_acc_number
sontrinh16 May 30, 2024
35c522e
address comments
sontrinh16 Jun 3, 2024
b2cfa49
nit
sontrinh16 Jun 3, 2024
c09f833
Merge branch 'main' into auth_accounts_use_same_acc_number
sontrinh16 Jun 3, 2024
cfac669
minor
sontrinh16 Jun 3, 2024
c2edaa6
Merge branch 'auth_accounts_use_same_acc_number' of https://github.co…
sontrinh16 Jun 3, 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: 9 additions & 0 deletions simapp/upgrades.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,15 @@ func (app SimApp) RegisterUpgradeHandlers() {
app.UpgradeKeeper.SetUpgradeHandler(
UpgradeName,
func(ctx context.Context, _ upgradetypes.Plan, fromVM module.VersionMap) (module.VersionMap, error) {
// sync accounts module and auth module account number
Copy link
Contributor

Choose a reason for hiding this comment

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

@julienrbrt @tac0turtle where should the migration live?

it needs to set x/accounts Account number to auth's account number.

Copy link
Member

Choose a reason for hiding this comment

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

if accounts does not have a dependency on auth but auth has a dep on accounts then I think it would be good for auth to have the migration and set it in the other module

Copy link
Member Author

Choose a reason for hiding this comment

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

added the migration in

currentAccNum, err := app.AuthKeeper.AccountNumber.Peek(ctx)
if err != nil {
return nil, err
}
err = app.AccountsKeeper.AccountNumber.Set(ctx, currentAccNum)
if err != nil {
return nil, err
}
return app.ModuleManager.RunMigrations(ctx, app.Configurator(), fromVM)
},
)
Expand Down
7 changes: 7 additions & 0 deletions tests/integration/bank/keeper/deterministic_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package keeper_test

import (
"context"
"testing"

"github.com/golang/mock/gomock"
Expand Down Expand Up @@ -84,6 +85,12 @@ func initDeterministicFixture(t *testing.T) *deterministicFixture {
// gomock initializations
ctrl := gomock.NewController(t)
acctsModKeeper := authtestutil.NewMockAccountsModKeeper(ctrl)
accNum := uint64(0)
acctsModKeeper.EXPECT().NextAccountNumber(gomock.Any()).AnyTimes().DoAndReturn(func(ctx context.Context) (uint64, error) {
currentNum := accNum
accNum++
return currentNum, nil
})

accountKeeper := authkeeper.NewAccountKeeper(
runtime.NewEnvironment(runtime.NewKVStoreService(keys[authtypes.StoreKey]), log.NewNopLogger()),
Expand Down
7 changes: 7 additions & 0 deletions tests/integration/distribution/keeper/msg_server_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package keeper_test

import (
"context"
"fmt"
"testing"

Expand Down Expand Up @@ -95,6 +96,12 @@ func initFixture(t *testing.T) *fixture {
// gomock initializations
ctrl := gomock.NewController(t)
acctsModKeeper := authtestutil.NewMockAccountsModKeeper(ctrl)
accNum := uint64(0)
acctsModKeeper.EXPECT().NextAccountNumber(gomock.Any()).AnyTimes().DoAndReturn(func(ctx context.Context) (uint64, error) {
currentNum := accNum
accNum++
return currentNum, nil
})

accountKeeper := authkeeper.NewAccountKeeper(
runtime.NewEnvironment(runtime.NewKVStoreService(keys[authtypes.StoreKey]), log.NewNopLogger()),
Expand Down
6 changes: 6 additions & 0 deletions tests/integration/evidence/keeper/infraction_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,6 +107,12 @@ func initFixture(tb testing.TB) *fixture {
// gomock initializations
ctrl := gomock.NewController(tb)
acctsModKeeper := authtestutil.NewMockAccountsModKeeper(ctrl)
accNum := uint64(0)
acctsModKeeper.EXPECT().NextAccountNumber(gomock.Any()).AnyTimes().DoAndReturn(func(ctx context.Context) (uint64, error) {
currentNum := accNum
accNum++
return currentNum, nil
})

maccPerms := map[string][]string{
pooltypes.ModuleName: {},
Expand Down
13 changes: 13 additions & 0 deletions tests/integration/example/example_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package integration_test

import (
"context"
"fmt"
"io"
"testing"
Expand Down Expand Up @@ -49,6 +50,12 @@ func Example() {
// gomock initializations
ctrl := gomock.NewController(&testing.T{})
acctsModKeeper := authtestutil.NewMockAccountsModKeeper(ctrl)
accNum := uint64(0)
acctsModKeeper.EXPECT().NextAccountNumber(gomock.Any()).AnyTimes().DoAndReturn(func(ctx context.Context) (uint64, error) {
currentNum := accNum
accNum++
return currentNum, nil
})

accountKeeper := authkeeper.NewAccountKeeper(
runtime.NewEnvironment(runtime.NewKVStoreService(keys[authtypes.StoreKey]), log.NewNopLogger()),
Expand Down Expand Up @@ -147,6 +154,12 @@ func Example_oneModule() {
// gomock initializations
ctrl := gomock.NewController(&testing.T{})
acctsModKeeper := authtestutil.NewMockAccountsModKeeper(ctrl)
accNum := uint64(0)
acctsModKeeper.EXPECT().NextAccountNumber(gomock.Any()).AnyTimes().DoAndReturn(func(ctx context.Context) (uint64, error) {
currentNum := accNum
accNum++
return currentNum, nil
})

accountKeeper := authkeeper.NewAccountKeeper(
runtime.NewEnvironment(runtime.NewKVStoreService(keys[authtypes.StoreKey]), log.NewNopLogger()),
Expand Down
7 changes: 7 additions & 0 deletions tests/integration/gov/keeper/keeper_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package keeper_test

import (
"context"
"testing"

"github.com/golang/mock/gomock"
Expand Down Expand Up @@ -77,6 +78,12 @@ func initFixture(tb testing.TB) *fixture {
// gomock initializations
ctrl := gomock.NewController(tb)
acctsModKeeper := authtestutil.NewMockAccountsModKeeper(ctrl)
accNum := uint64(0)
acctsModKeeper.EXPECT().NextAccountNumber(gomock.Any()).AnyTimes().DoAndReturn(func(ctx context.Context) (uint64, error) {
currentNum := accNum
accNum++
return currentNum, nil
})

accountKeeper := authkeeper.NewAccountKeeper(
runtime.NewEnvironment(runtime.NewKVStoreService(keys[authtypes.StoreKey]), log.NewNopLogger()),
Expand Down
9 changes: 8 additions & 1 deletion tests/integration/slashing/keeper/keeper_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package keeper_test

import (
"context"
"testing"
"time"

Expand Down Expand Up @@ -81,8 +82,14 @@ func initFixture(tb testing.TB) *fixture {
queryRouter := baseapp.NewGRPCQueryRouter()

// gomock initializations
ctrl := gomock.NewController(&testing.T{})
ctrl := gomock.NewController(tb)
acctsModKeeper := authtestutil.NewMockAccountsModKeeper(ctrl)
accNum := uint64(0)
acctsModKeeper.EXPECT().NextAccountNumber(gomock.Any()).AnyTimes().DoAndReturn(func(ctx context.Context) (uint64, error) {
currentNum := accNum
accNum++
return currentNum, nil
})

accountKeeper := authkeeper.NewAccountKeeper(
runtime.NewEnvironment(runtime.NewKVStoreService(keys[authtypes.StoreKey]), log.NewNopLogger(), runtime.EnvWithQueryRouterService(queryRouter), runtime.EnvWithMsgRouterService(msgRouter)),
Expand Down
9 changes: 8 additions & 1 deletion tests/integration/staking/keeper/common_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package keeper_test

import (
"context"
"math/big"
"testing"

Expand Down Expand Up @@ -134,7 +135,7 @@ func initFixture(tb testing.TB) *fixture {
}

// gomock initializations
ctrl := gomock.NewController(&testing.T{})
ctrl := gomock.NewController(tb)
acctsModKeeper := authtestutil.NewMockAccountsModKeeper(ctrl)

accountKeeper := authkeeper.NewAccountKeeper(
Expand Down Expand Up @@ -187,6 +188,12 @@ func initFixture(tb testing.TB) *fixture {

// set default staking params
assert.NilError(tb, stakingKeeper.Params.Set(sdkCtx, types.DefaultParams()))
accNum := uint64(0)
acctsModKeeper.EXPECT().NextAccountNumber(gomock.Any()).AnyTimes().DoAndReturn(func(ctx context.Context) (uint64, error) {
currentNum := accNum
accNum++
return currentNum, nil
})

f := fixture{
app: integrationApp,
Expand Down
7 changes: 7 additions & 0 deletions tests/integration/staking/keeper/deterministic_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package keeper_test

import (
"context"
"testing"
"time"

Expand Down Expand Up @@ -92,6 +93,12 @@ func initDeterministicFixture(t *testing.T) *deterministicFixture {
// gomock initializations
ctrl := gomock.NewController(t)
acctsModKeeper := authtestutil.NewMockAccountsModKeeper(ctrl)
accNum := uint64(0)
acctsModKeeper.EXPECT().NextAccountNumber(gomock.Any()).AnyTimes().DoAndReturn(func(ctx context.Context) (uint64, error) {
currentNum := accNum
accNum++
return currentNum, nil
})

accountKeeper := authkeeper.NewAccountKeeper(
runtime.NewEnvironment(runtime.NewKVStoreService(keys[authtypes.StoreKey]), log.NewNopLogger()),
Expand Down
17 changes: 17 additions & 0 deletions x/accounts/keeper.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,6 +107,23 @@ func (k Keeper) IsAccountsModuleAccount(
return hasAcc
}

func (k Keeper) NextAccountNumber(
ctx context.Context,
) (accNum uint64, err error) {
accNum, err = k.AccountNumber.Next(ctx)
if err != nil {
return 0, err
}

return accNum, nil
}

// only use to migrate account number
func (k Keeper) SetAccountNumber(ctx context.Context, accNum uint64) error {
sontrinh16 marked this conversation as resolved.
Show resolved Hide resolved
err := k.AccountNumber.Set(ctx, accNum)
sontrinh16 marked this conversation as resolved.
Show resolved Hide resolved
return err
}

// Init creates a new account of the given type.
func (k Keeper) Init(
ctx context.Context,
Expand Down
7 changes: 7 additions & 0 deletions x/auth/ante/testutil_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package ante_test

import (
"context"
"testing"

abci "github.com/cometbft/cometbft/abci/types"
Expand Down Expand Up @@ -79,6 +80,12 @@ func SetupTestSuite(t *testing.T, isCheckTx bool) *AnteTestSuite {
suite.ctx = testCtx.Ctx.WithIsCheckTx(isCheckTx).WithBlockHeight(1)
suite.encCfg = moduletestutil.MakeTestEncodingConfig(codectestutil.CodecOptions{}, auth.AppModule{})

accNum := uint64(0)
suite.acctsModKeeper.EXPECT().NextAccountNumber(gomock.Any()).AnyTimes().DoAndReturn(func(ctx context.Context) (uint64, error) {
currNum := accNum
accNum++
return currNum, nil
})
maccPerms := map[string][]string{
"fee_collector": nil,
"mint": {"minter"},
Expand Down
7 changes: 7 additions & 0 deletions x/auth/keeper/deterministic_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package keeper_test

import (
"context"
"encoding/hex"
"sort"
"sync/atomic"
Expand Down Expand Up @@ -69,6 +70,12 @@ func (suite *DeterministicTestSuite) SetupTest() {
ctrl := gomock.NewController(suite.T())
acctsModKeeper := authtestutil.NewMockAccountsModKeeper(ctrl)
suite.acctsModKeeper = acctsModKeeper
accNum := uint64(0)
suite.acctsModKeeper.EXPECT().NextAccountNumber(gomock.Any()).AnyTimes().DoAndReturn(func(ctx context.Context) (uint64, error) {
currNum := accNum
accNum++
return currNum, nil
})

maccPerms := map[string][]string{
"fee_collector": nil,
Expand Down
8 changes: 5 additions & 3 deletions x/auth/keeper/keeper.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,8 +97,10 @@ type AccountKeeper struct {
authority string

// State
Schema collections.Schema
Params collections.Item[types.Params]
Schema collections.Schema
Params collections.Item[types.Params]

// Only use for migration
sontrinh16 marked this conversation as resolved.
Show resolved Hide resolved
AccountNumber collections.Sequence
// Accounts key: AccAddr | value: AccountI | index: AccountsIndex
Accounts *collections.IndexedMap[sdk.AccAddress, sdk.AccountI, AccountsIndexes]
Expand Down Expand Up @@ -182,7 +184,7 @@ func (ak AccountKeeper) GetSequence(ctx context.Context, addr sdk.AccAddress) (u
// NextAccountNumber returns and increments the global account number counter.
// If the global account number is not set, it initializes it with value 0.
func (ak AccountKeeper) NextAccountNumber(ctx context.Context) uint64 {
n, err := ak.AccountNumber.Next(ctx)
n, err := ak.AccountsModKeeper.NextAccountNumber(ctx)
sontrinh16 marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

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

Address the error handling issue in NextAccountNumber to prevent potential service disruptions.

-	panic(err)
+	if err != nil {
+		return 0, err
+	}

This change allows the caller to handle the error appropriately instead of causing a panic, enhancing the robustness of the service.

Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.

Suggested change
n, err := ak.AccountsModKeeper.NextAccountNumber(ctx)
n, err := ak.AccountsModKeeper.NextAccountNumber(ctx)
if err != nil {
return 0, err
}

if err != nil {
panic(err)
}
Expand Down
7 changes: 7 additions & 0 deletions x/auth/keeper/keeper_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package keeper_test

import (
"context"
"testing"

"github.com/golang/mock/gomock"
Expand Down Expand Up @@ -62,6 +63,12 @@ func (suite *KeeperTestSuite) SetupTest() {
ctrl := gomock.NewController(suite.T())
acctsModKeeper := authtestutil.NewMockAccountsModKeeper(ctrl)
suite.acctsModKeeper = acctsModKeeper
accNum := uint64(0)
suite.acctsModKeeper.EXPECT().NextAccountNumber(gomock.Any()).AnyTimes().DoAndReturn(func(ctx context.Context) (uint64, error) {
currNum := accNum
accNum++
return currNum, nil
})

maccPerms := map[string][]string{
"fee_collector": nil,
Expand Down
15 changes: 15 additions & 0 deletions x/auth/testutil/expected_keepers_mocks.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions x/auth/types/expected_keepers.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,4 +19,5 @@ type BankKeeper interface {
type AccountsModKeeper interface {
SendModuleMessageUntyped(ctx context.Context, sender []byte, msg protoiface.MessageV1) (protoiface.MessageV1, error)
IsAccountsModuleAccount(ctx context.Context, accountAddr []byte) bool
NextAccountNumber(ctx context.Context) (accNum uint64, err error)
}
15 changes: 15 additions & 0 deletions x/auth/vesting/testutil/expected_keepers_mocks.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

8 changes: 8 additions & 0 deletions x/distribution/migrations/v4/migrate_funds_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package v4_test

import (
"context"
"testing"

"github.com/golang/mock/gomock"
Expand Down Expand Up @@ -53,6 +54,13 @@ func TestFundsMigration(t *testing.T) {
stakingKeeper := distrtestutil.NewMockStakingKeeper(ctrl)
poolKeeper := distrtestutil.NewMockPoolKeeper(ctrl)

accNum := uint64(0)
acctsModKeeper.EXPECT().NextAccountNumber(gomock.Any()).AnyTimes().DoAndReturn(func(ctx context.Context) (uint64, error) {
currNum := accNum
accNum++
return currNum, nil
})

// create account keeper
accountKeeper := authkeeper.NewAccountKeeper(
runtime.NewEnvironment(runtime.NewKVStoreService(keys[authtypes.StoreKey]), log.NewNopLogger()),
Expand Down
Loading
Loading