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!: use KVStoreService in x/auth #15520

Merged
merged 23 commits into from
Mar 27, 2023
Merged
Show file tree
Hide file tree
Changes from 16 commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
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
72 changes: 72 additions & 0 deletions runtime/store.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,11 +2,14 @@ package runtime

import (
"context"
"io"

"cosmossdk.io/core/store"

storetypes "cosmossdk.io/store/types"

dbm "github.com/cosmos/cosmos-db"

sdk "github.com/cosmos/cosmos-sdk/types"
)

Expand Down Expand Up @@ -90,3 +93,72 @@ func (store coreKVStore) Iterator(start, end []byte) (store.Iterator, error) {
func (store coreKVStore) ReverseIterator(start, end []byte) (store.Iterator, error) {
return store.kvStore.ReverseIterator(start, end), nil
}

// Adapter
var _ storetypes.KVStore = kvStoreAdapter{}

type kvStoreAdapter struct {
store store.KVStore
}

func (kvStoreAdapter) CacheWrap() storetypes.CacheWrap {
panic("unimplemented")
}

func (kvStoreAdapter) CacheWrapWithTrace(w io.Writer, tc storetypes.TraceContext) storetypes.CacheWrap {
panic("unimplemented")
}

func (kvStoreAdapter) GetStoreType() storetypes.StoreType {
panic("unimplemented")
}

func (s kvStoreAdapter) Delete(key []byte) {
err := s.store.Delete(key)
if err != nil {
panic(err)

Check warning

Code scanning / CodeQL

Panic in BeginBock or EndBlock consensus methods

Possible panics in BeginBock- or EndBlock-related consensus methods could cause a chain halt
}
}

func (s kvStoreAdapter) Get(key []byte) []byte {
bz, err := s.store.Get(key)
if err != nil {
panic(err)

Check warning

Code scanning / CodeQL

Panic in BeginBock or EndBlock consensus methods

Possible panics in BeginBock- or EndBlock-related consensus methods could cause a chain halt
}
return bz
}

func (s kvStoreAdapter) Has(key []byte) bool {
has, err := s.store.Has(key)
if err != nil {
panic(err)

Check warning

Code scanning / CodeQL

Panic in BeginBock or EndBlock consensus methods

Possible panics in BeginBock- or EndBlock-related consensus methods could cause a chain halt
}
return has
}

func (s kvStoreAdapter) Set(key []byte, value []byte) {
err := s.store.Set(key, value)
if err != nil {
panic(err)

Check warning

Code scanning / CodeQL

Panic in BeginBock or EndBlock consensus methods

Possible panics in BeginBock- or EndBlock-related consensus methods could cause a chain halt
}
}

func (s kvStoreAdapter) Iterator(start []byte, end []byte) dbm.Iterator {
it, err := s.store.Iterator(start, end)
if err != nil {
panic(err)

Check warning

Code scanning / CodeQL

Panic in BeginBock or EndBlock consensus methods

Possible panics in BeginBock- or EndBlock-related consensus methods could cause a chain halt
}
return it
}

func (s kvStoreAdapter) ReverseIterator(start []byte, end []byte) dbm.Iterator {
it, err := s.store.ReverseIterator(start, end)
if err != nil {
panic(err)
}
return it
}

func KVStoreAdapter(store store.KVStore) storetypes.KVStore {
return &kvStoreAdapter{store}
}
Comment on lines +162 to +164
Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure where to put this adapter, accepting suggestions :D

Copy link
Member

Choose a reason for hiding this comment

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

why is this needed? the new interface returns errors can we use that or?

Copy link
Member Author

Choose a reason for hiding this comment

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

Because there's the prefix store and the query.Paginate that take in the KVStore from /store/types instead of core. Once we migrate all modules we could remove this adapter and modify the other methods to take in the core KVStore.

Copy link
Member Author

Choose a reason for hiding this comment

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

So yes, because the new interface returns errors so it's not compatible

Copy link
Member

Choose a reason for hiding this comment

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

makes sense thank you

2 changes: 1 addition & 1 deletion simapp/app.go
Original file line number Diff line number Diff line change
Expand Up @@ -289,7 +289,7 @@ func NewSimApp(
bApp.SetParamStore(&app.ConsensusParamsKeeper)

// add keepers
app.AccountKeeper = authkeeper.NewAccountKeeper(appCodec, keys[authtypes.StoreKey], authtypes.ProtoBaseAccount, maccPerms, sdk.Bech32MainPrefix, authtypes.NewModuleAddress(govtypes.ModuleName).String())
app.AccountKeeper = authkeeper.NewAccountKeeper(appCodec, runtime.NewKVStoreService(keys[authtypes.StoreKey]), authtypes.ProtoBaseAccount, maccPerms, sdk.Bech32MainPrefix, authtypes.NewModuleAddress(govtypes.ModuleName).String())
Copy link
Member

@julienrbrt julienrbrt Mar 24, 2023

Choose a reason for hiding this comment

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

Same comment as before about UPGRADING.md + changelog.
I think now the category can be generalized under simapp. Moreover, we should add that if chains are generating mocks for their own testing, they will need to re-generate them.

Copy link
Member Author

Choose a reason for hiding this comment

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

Github is taking a while to update stuff here, but I've added a changelog entry and modified the upgrading doc.


app.BankKeeper = bankkeeper.NewBaseKeeper(
appCodec,
Expand Down
8 changes: 5 additions & 3 deletions x/auth/ante/expected_keepers.go
Original file line number Diff line number Diff line change
@@ -1,16 +1,18 @@
package ante

import (
"context"

sdk "github.com/cosmos/cosmos-sdk/types"
"github.com/cosmos/cosmos-sdk/x/auth/types"
)

// AccountKeeper defines the contract needed for AccountKeeper related APIs.
// Interface provides support to use non-sdk AccountKeeper for AnteHandler's decorators.
type AccountKeeper interface {
GetParams(ctx sdk.Context) (params types.Params)
GetAccount(ctx sdk.Context, addr sdk.AccAddress) sdk.AccountI
SetAccount(ctx sdk.Context, acc sdk.AccountI)
GetParams(ctx context.Context) (params types.Params)
GetAccount(ctx context.Context, addr sdk.AccAddress) sdk.AccountI
SetAccount(ctx context.Context, acc sdk.AccountI)
GetModuleAddress(moduleName string) sdk.AccAddress
}

Expand Down
7 changes: 4 additions & 3 deletions x/auth/ante/testutil/expected_keepers_mocks.go

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

4 changes: 3 additions & 1 deletion x/auth/ante/testutil_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,8 @@ import (
// ref: https://github.com/cosmos/cosmos-sdk/issues/14647
_ "cosmossdk.io/api/cosmos/bank/v1beta1"
_ "cosmossdk.io/api/cosmos/crypto/secp256k1"

"github.com/cosmos/cosmos-sdk/runtime"
_ "github.com/cosmos/cosmos-sdk/testutil/testdata/testpb"

storetypes "cosmossdk.io/store/types"
Expand Down Expand Up @@ -77,7 +79,7 @@ func SetupTestSuite(t *testing.T, isCheckTx bool) *AnteTestSuite {
}

suite.accountKeeper = keeper.NewAccountKeeper(
suite.encCfg.Codec, key, types.ProtoBaseAccount, maccPerms, sdk.Bech32MainPrefix, types.NewModuleAddress("gov").String(),
suite.encCfg.Codec, runtime.NewKVStoreService(key), types.ProtoBaseAccount, maccPerms, sdk.Bech32MainPrefix, types.NewModuleAddress("gov").String(),
)
suite.accountKeeper.GetModuleAccount(suite.ctx, types.FeeCollectorName)
err := suite.accountKeeper.SetParams(suite.ctx, types.DefaultParams())
Expand Down
76 changes: 52 additions & 24 deletions x/auth/keeper/account.go
Original file line number Diff line number Diff line change
@@ -1,14 +1,16 @@
package keeper

import (
"context"

storetypes "cosmossdk.io/store/types"

sdk "github.com/cosmos/cosmos-sdk/types"
"github.com/cosmos/cosmos-sdk/x/auth/types"
)

// NewAccountWithAddress implements AccountKeeperI.
func (ak AccountKeeper) NewAccountWithAddress(ctx sdk.Context, addr sdk.AccAddress) sdk.AccountI {
func (ak AccountKeeper) NewAccountWithAddress(ctx context.Context, addr sdk.AccAddress) sdk.AccountI {
acc := ak.proto()
err := acc.SetAddress(addr)
if err != nil {
Expand All @@ -19,7 +21,7 @@ func (ak AccountKeeper) NewAccountWithAddress(ctx sdk.Context, addr sdk.AccAddre
}

// NewAccount sets the next account number to a given account interface
func (ak AccountKeeper) NewAccount(ctx sdk.Context, acc sdk.AccountI) sdk.AccountI {
func (ak AccountKeeper) NewAccount(ctx context.Context, acc sdk.AccountI) sdk.AccountI {
if err := acc.SetAccountNumber(ak.NextAccountNumber(ctx)); err != nil {
panic(err)
}
Expand All @@ -28,21 +30,33 @@ func (ak AccountKeeper) NewAccount(ctx sdk.Context, acc sdk.AccountI) sdk.Accoun
}

// HasAccount implements AccountKeeperI.
func (ak AccountKeeper) HasAccount(ctx sdk.Context, addr sdk.AccAddress) bool {
store := ctx.KVStore(ak.storeKey)
return store.Has(types.AddressStoreKey(addr))
func (ak AccountKeeper) HasAccount(ctx context.Context, addr sdk.AccAddress) bool {
store := ak.storeService.OpenKVStore(ctx)
has, err := store.Has(types.AddressStoreKey(addr))
if err != nil {
panic(err)

Check warning

Code scanning / CodeQL

Panic in BeginBock or EndBlock consensus methods

Possible panics in BeginBock- or EndBlock-related consensus methods could cause a chain halt
}
return has
}

// HasAccountAddressByID checks account address exists by id.
func (ak AccountKeeper) HasAccountAddressByID(ctx sdk.Context, id uint64) bool {
store := ctx.KVStore(ak.storeKey)
return store.Has(types.AccountNumberStoreKey(id))
func (ak AccountKeeper) HasAccountAddressByID(ctx context.Context, id uint64) bool {
store := ak.storeService.OpenKVStore(ctx)
has, err := store.Has(types.AccountNumberStoreKey(id))
if err != nil {
panic(err)
}
return has
}

// GetAccount implements AccountKeeperI.
func (ak AccountKeeper) GetAccount(ctx sdk.Context, addr sdk.AccAddress) sdk.AccountI {
store := ctx.KVStore(ak.storeKey)
bz := store.Get(types.AddressStoreKey(addr))
func (ak AccountKeeper) GetAccount(ctx context.Context, addr sdk.AccAddress) sdk.AccountI {
store := ak.storeService.OpenKVStore(ctx)
bz, err := store.Get(types.AddressStoreKey(addr))
if err != nil {
panic(err)

Check warning

Code scanning / CodeQL

Panic in BeginBock or EndBlock consensus methods

Possible panics in BeginBock- or EndBlock-related consensus methods could cause a chain halt
}

if bz == nil {
return nil
}
Expand All @@ -51,17 +65,21 @@ func (ak AccountKeeper) GetAccount(ctx sdk.Context, addr sdk.AccAddress) sdk.Acc
}

// GetAccountAddressById returns account address by id.
func (ak AccountKeeper) GetAccountAddressByID(ctx sdk.Context, id uint64) string {
store := ctx.KVStore(ak.storeKey)
bz := store.Get(types.AccountNumberStoreKey(id))
func (ak AccountKeeper) GetAccountAddressByID(ctx context.Context, id uint64) string {
store := ak.storeService.OpenKVStore(ctx)
bz, err := store.Get(types.AccountNumberStoreKey(id))
if err != nil {
panic(err)
}

if bz == nil {
return ""
}
return sdk.AccAddress(bz).String()
}

// GetAllAccounts returns all accounts in the accountKeeper.
func (ak AccountKeeper) GetAllAccounts(ctx sdk.Context) (accounts []sdk.AccountI) {
func (ak AccountKeeper) GetAllAccounts(ctx context.Context) (accounts []sdk.AccountI) {
ak.IterateAccounts(ctx, func(acc sdk.AccountI) (stop bool) {
accounts = append(accounts, acc)
return false
Expand All @@ -71,9 +89,9 @@ func (ak AccountKeeper) GetAllAccounts(ctx sdk.Context) (accounts []sdk.AccountI
}

// SetAccount implements AccountKeeperI.
func (ak AccountKeeper) SetAccount(ctx sdk.Context, acc sdk.AccountI) {
func (ak AccountKeeper) SetAccount(ctx context.Context, acc sdk.AccountI) {
addr := acc.GetAddress()
store := ctx.KVStore(ak.storeKey)
store := ak.storeService.OpenKVStore(ctx)

bz, err := ak.MarshalAccount(acc)
if err != nil {
Expand All @@ -86,18 +104,28 @@ func (ak AccountKeeper) SetAccount(ctx sdk.Context, acc sdk.AccountI) {

// RemoveAccount removes an account for the account mapper store.
// NOTE: this will cause supply invariant violation if called
func (ak AccountKeeper) RemoveAccount(ctx sdk.Context, acc sdk.AccountI) {
func (ak AccountKeeper) RemoveAccount(ctx context.Context, acc sdk.AccountI) {
addr := acc.GetAddress()
store := ctx.KVStore(ak.storeKey)
store.Delete(types.AddressStoreKey(addr))
store.Delete(types.AccountNumberStoreKey(acc.GetAccountNumber()))
store := ak.storeService.OpenKVStore(ctx)
err := store.Delete(types.AddressStoreKey(addr))
if err != nil {
panic(err)
}

err = store.Delete(types.AccountNumberStoreKey(acc.GetAccountNumber()))
if err != nil {
panic(err)
}
}

// IterateAccounts iterates over all the stored accounts and performs a callback function.
// Stops iteration when callback returns true.
func (ak AccountKeeper) IterateAccounts(ctx sdk.Context, cb func(account sdk.AccountI) (stop bool)) {
store := ctx.KVStore(ak.storeKey)
iterator := storetypes.KVStorePrefixIterator(store, types.AddressStoreKeyPrefix)
func (ak AccountKeeper) IterateAccounts(ctx context.Context, cb func(account sdk.AccountI) (stop bool)) {
store := ak.storeService.OpenKVStore(ctx)
iterator, err := store.Iterator(types.AddressStoreKeyPrefix, storetypes.PrefixEndBytes(types.AddressStoreKeyPrefix))
if err != nil {
panic(err)
}

defer iterator.Close()
for ; iterator.Valid(); iterator.Next() {
Expand Down
11 changes: 8 additions & 3 deletions x/auth/keeper/deterministic_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -9,10 +9,12 @@ import (
"github.com/stretchr/testify/suite"
"pgregory.net/rapid"

corestore "cosmossdk.io/core/store"
storetypes "cosmossdk.io/store/types"

"github.com/cosmos/cosmos-sdk/baseapp"
"github.com/cosmos/cosmos-sdk/crypto/keys/secp256k1"
"github.com/cosmos/cosmos-sdk/runtime"
"github.com/cosmos/cosmos-sdk/testutil"
"github.com/cosmos/cosmos-sdk/testutil/testdata"
sdk "github.com/cosmos/cosmos-sdk/types"
Expand All @@ -26,6 +28,7 @@ type DeterministicTestSuite struct {
suite.Suite

key *storetypes.KVStoreKey
storeService corestore.KVStoreService
ctx sdk.Context
queryClient types.QueryClient
accountKeeper keeper.AccountKeeper
Expand All @@ -48,6 +51,7 @@ func (suite *DeterministicTestSuite) SetupTest() {

suite.Require()
key := storetypes.NewKVStoreKey(types.StoreKey)
storeService := runtime.NewKVStoreService(key)
testCtx := testutil.DefaultContextWithDB(suite.T(), key, storetypes.NewTransientStoreKey("transient_test"))
suite.ctx = testCtx.Ctx.WithBlockHeader(cmtproto.Header{})

Expand All @@ -62,7 +66,7 @@ func (suite *DeterministicTestSuite) SetupTest() {

suite.accountKeeper = keeper.NewAccountKeeper(
suite.encCfg.Codec,
key,
storeService,
types.ProtoBaseAccount,
maccPerms,
"cosmos",
Expand All @@ -74,6 +78,7 @@ func (suite *DeterministicTestSuite) SetupTest() {
suite.queryClient = types.NewQueryClient(queryHelper)

suite.key = key
suite.storeService = storeService
suite.maccPerms = maccPerms
}

Expand Down Expand Up @@ -281,7 +286,7 @@ func (suite *DeterministicTestSuite) TestGRPCQueryModuleAccounts() {

ak := keeper.NewAccountKeeper(
suite.encCfg.Codec,
suite.key,
suite.storeService,
types.ProtoBaseAccount,
maccPerms,
"cosmos",
Expand Down Expand Up @@ -327,7 +332,7 @@ func (suite *DeterministicTestSuite) TestGRPCQueryModuleAccountByName() {

ak := keeper.NewAccountKeeper(
suite.encCfg.Codec,
suite.key,
suite.storeService,
types.ProtoBaseAccount,
maccPerms,
"cosmos",
Expand Down
Loading