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 all 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
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
24 changes: 24 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,30 @@ 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
}

// InitAccountNumberSeqUnsafe use to set accounts account number tracking.
// Only use for account number migration.
func (k Keeper) InitAccountNumberSeqUnsafe(ctx context.Context, accNum uint64) error {
currentNum, err := k.AccountNumber.Peek(ctx)
if err != nil {
return err
}
if currentNum > accNum {
return fmt.Errorf("cannot set number lower than current account number got %v while current account number is %v", accNum, currentNum)
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.

Consider refining the error message in InitAccountNumberSeqUnsafe for clarity and accuracy.

-	return fmt.Errorf("cannot set number lower than current account number got %v while current account number is %v", accNum, currentNum)
+	return fmt.Errorf("attempted to set account number to %v, which is lower than the current account number %v", accNum, currentNum)

This change makes the error message more direct and easier to understand.

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
return fmt.Errorf("cannot set number lower than current account number got %v while current account number is %v", accNum, currentNum)
return fmt.Errorf("attempted to set account number to %v, which is lower than the current account number %v", accNum, currentNum)

}
return k.AccountNumber.Set(ctx, accNum)
}

// 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: 6 additions & 1 deletion x/auth/keeper/account.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,12 @@

// NewAccount sets the next account number to a given account interface
func (ak AccountKeeper) NewAccount(ctx context.Context, acc sdk.AccountI) sdk.AccountI {
if err := acc.SetAccountNumber(ak.NextAccountNumber(ctx)); err != nil {
accNum, err := ak.AccountsModKeeper.NextAccountNumber(ctx)
if err != nil {
panic(err)
Dismissed Show dismissed Hide dismissed
}

if err := acc.SetAccountNumber(accNum); err != nil {
panic(err)
}

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
5 changes: 4 additions & 1 deletion x/auth/keeper/genesis.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,10 @@ func (ak AccountKeeper) InitGenesis(ctx context.Context, data types.GenesisState
for _, acc := range accounts {
accNum := acc.GetAccountNumber()
for lastAccNum == nil || *lastAccNum < accNum {
n := ak.NextAccountNumber(ctx)
n, err := ak.AccountsModKeeper.NextAccountNumber(ctx)
if err != nil {
return err
}
Comment on lines +32 to +35
Copy link
Contributor

Choose a reason for hiding this comment

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

Potential logical flaw in setting the last account number.

The logic for updating lastAccNum seems to be inside a loop that checks if lastAccNum is less than accNum. This could lead to incorrect behavior if not all accounts are processed or if the account numbers are not strictly increasing. Consider revising this logic to ensure that lastAccNum correctly reflects the highest account number after all accounts are processed.

lastAccNum = &n
}
ak.SetAccount(ctx, acc)
Expand Down
13 changes: 8 additions & 5 deletions x/auth/keeper/keeper.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,8 @@ type AccountKeeperI interface {
GetSequence(context.Context, sdk.AccAddress) (uint64, error)

// Fetch the next account number, and increment the internal counter.
//
// Deprecated: keep this to avoid breaking api
NextAccountNumber(context.Context) uint64
sontrinh16 marked this conversation as resolved.
Show resolved Hide resolved

// GetModulePermissions fetches per-module account permissions
Expand Down Expand Up @@ -97,9 +99,9 @@ type AccountKeeper struct {
authority string

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

// Accounts key: AccAddr | value: AccountI | index: AccountsIndex
Accounts *collections.IndexedMap[sdk.AccAddress, sdk.AccountI, AccountsIndexes]
}
Expand Down Expand Up @@ -133,7 +135,6 @@ 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()
Expand Down Expand Up @@ -181,8 +182,10 @@ 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.
//
// Deprecated: NextAccountNumber is deprecated
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
13 changes: 11 additions & 2 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 Expand Up @@ -202,7 +209,8 @@ func (suite *KeeperTestSuite) TestInitGenesis() {
suite.Require().Equal(6, int(feeCollector.GetAccountNumber()))

// The 3rd account has account number 5, but because the FeeCollector account gets initialized last, the next should be 7.
nextNum := suite.accountKeeper.NextAccountNumber(ctx)
nextNum, err := suite.accountKeeper.AccountsModKeeper.NextAccountNumber(ctx)
suite.Require().NoError(err)
suite.Require().Equal(7, int(nextNum))

suite.SetupTest() // reset
Expand Down Expand Up @@ -237,7 +245,8 @@ func (suite *KeeperTestSuite) TestInitGenesis() {
feeCollector = suite.accountKeeper.GetModuleAccount(ctx, "fee_collector")
suite.Require().Equal(1, int(feeCollector.GetAccountNumber()))

nextNum = suite.accountKeeper.NextAccountNumber(ctx)
nextNum, err = suite.accountKeeper.AccountsModKeeper.NextAccountNumber(ctx)
suite.Require().NoError(err)
// we expect nextNum to be 2 because we initialize fee_collector as account number 1
suite.Require().Equal(2, int(nextNum))
}
Loading
Loading