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 21 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
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,7 @@ Ref: https://keepachangelog.com/en/1.0.0/

### API Breaking Changes

* (x/auth) [#15517](https://github.com/cosmos/cosmos-sdk/pull/15517) `NewAccountKeeper` now takes a `KVStoreService` instead of a `StoreKey` and methods in the `Keeper` now take a `context.Context` instead of a `sdk.Context`.
* (x/consensus) [#15517](https://github.com/cosmos/cosmos-sdk/pull/15517) `NewKeeper` now takes a `KVStoreService` instead of a `StoreKey`.
* (x/bank) [#15477](https://github.com/cosmos/cosmos-sdk/pull/15477) `banktypes.NewMsgMultiSend` and `keeper.InputOutputCoins` only accept one input.
* (mempool) [#15328](https://github.com/cosmos/cosmos-sdk/pull/15328) The `PriorityNonceMempool` is now generic over type `C comparable` and takes a single `PriorityNonceMempoolConfig[C]` argument. See `DefaultPriorityNonceMempoolConfig` for how to construct the configuration and a `TxPriority` type.
Expand Down
28 changes: 19 additions & 9 deletions UPGRADING.md
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,21 @@ The `gogoproto.goproto_stringer = false` annotation has been removed from most p
Previously, all modules were required to be set in `OrderBeginBlockers`, `OrderEndBlockers` and `OrderInitGenesis / OrderExportGenesis` in `app.go` / `app_config.go`.
This is no longer the case, the assertion has been loosened to only require modules implementing, respectively, the `module.BeginBlockAppModule`, `module.EndBlockAppModule` and `module.HasGenesis` interfaces.

### Modules Keepers

The following modules `NewKeeper` function now take a `KVStoreService` instead of a `StoreKey`:

- `x/auth`
- `x/consensus`

When not using depinject, the `runtime.NewKVStoreService` method can be used to create a `KVStoreService` from a `StoreKey`:

```diff
- app.ConsensusParamsKeeper = consensusparamkeeper.NewKeeper(appCodec, keys[consensusparamtypes.StoreKey], authtypes.NewModuleAddress(govtypes.ModuleName).String())
+ app.ConsensusParamsKeeper = consensusparamkeeper.NewKeeper(appCodec, runtime.NewKVStoreService(keys[consensusparamtypes.StoreKey]), authtypes.NewModuleAddress(govtypes.ModuleName).String())
facundomedica marked this conversation as resolved.
Show resolved Hide resolved
```


### Packages

#### Store
Expand All @@ -75,18 +90,13 @@ All the store imports are now renamed to use `cosmossdk.io/store` instead of `gi

### Modules

#### `x/capability`
#### `x/auth`

Capability was moved to [IBC-GO](https://github.com/cosmos/ibc-go). IBC V8 will contain the necessary changes to incorporate the new module location
Methods in the `AccountKeeper` now use `context.Context` instead of `sdk.Context`. Any module that has an interface for it will need to update and re-generate mocks if needed.

#### `x/consensus`

The `NewKeeper` method now takes a `KVStoreService` instead of a `StoreKey`. When not using depinject, the `runtime.NewKVStoreService` method can be used to create a `KVStoreService` from a `StoreKey`.
#### `x/capability`

```diff
- app.ConsensusParamsKeeper = consensusparamkeeper.NewKeeper(appCodec, keys[consensusparamtypes.StoreKey], authtypes.NewModuleAddress(govtypes.ModuleName).String())
+ app.ConsensusParamsKeeper = consensusparamkeeper.NewKeeper(appCodec, runtime.NewKVStoreService(keys[consensusparamtypes.StoreKey]), authtypes.NewModuleAddress(govtypes.ModuleName).String())
```
Capability was moved to [IBC-GO](https://github.com/cosmos/ibc-go). IBC V8 will contain the necessary changes to incorporate the new module location

#### `x/gov`

Expand Down
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
11 changes: 8 additions & 3 deletions tests/integration/bank/keeper/keeper_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ import (
"github.com/cosmos/cosmos-sdk/baseapp"
"github.com/cosmos/cosmos-sdk/codec"
codectypes "github.com/cosmos/cosmos-sdk/codec/types"
"github.com/cosmos/cosmos-sdk/runtime"
"github.com/cosmos/cosmos-sdk/testutil"
"github.com/cosmos/cosmos-sdk/testutil/configurator"
"github.com/cosmos/cosmos-sdk/testutil/sims"
Expand Down Expand Up @@ -158,8 +159,10 @@ func initKeepersWithmAccPerms(f *fixture, blockedAddrs map[string]bool) (authkee
maccPerms[authtypes.Minter] = []string{authtypes.Minter}
maccPerms[multiPerm] = []string{authtypes.Burner, authtypes.Minter, authtypes.Staking}
maccPerms[randomPerm] = []string{"random"}

storeService := runtime.NewKVStoreService(f.fetchStoreKey(authtypes.StoreKey).(*storetypes.KVStoreKey))
authKeeper := authkeeper.NewAccountKeeper(
appCodec, f.fetchStoreKey(types.StoreKey), authtypes.ProtoBaseAccount,
appCodec, storeService, authtypes.ProtoBaseAccount,
maccPerms, sdk.Bech32MainPrefix, authtypes.NewModuleAddress(govtypes.ModuleName).String(),
)
bankKeeper := keeper.NewBaseKeeper(
Expand Down Expand Up @@ -1194,8 +1197,9 @@ func TestBalanceTrackingEvents(t *testing.T) {

maccPerms[multiPerm] = []string{authtypes.Burner, authtypes.Minter, authtypes.Staking}

storeService := runtime.NewKVStoreService(f.fetchStoreKey(authtypes.StoreKey).(*storetypes.KVStoreKey))
f.accountKeeper = authkeeper.NewAccountKeeper(
f.appCodec, f.fetchStoreKey(authtypes.StoreKey),
f.appCodec, storeService,
authtypes.ProtoBaseAccount, maccPerms, sdk.Bech32MainPrefix,
authtypes.NewModuleAddress(govtypes.ModuleName).String(),
)
Expand Down Expand Up @@ -1324,9 +1328,10 @@ func TestMintCoinRestrictions(t *testing.T) {

maccPerms := make(map[string][]string)
maccPerms[multiPerm] = []string{authtypes.Burner, authtypes.Minter, authtypes.Staking}
storeService := runtime.NewKVStoreService(f.fetchStoreKey(authtypes.StoreKey).(*storetypes.KVStoreKey))

f.accountKeeper = authkeeper.NewAccountKeeper(
f.appCodec, f.fetchStoreKey(authtypes.StoreKey),
f.appCodec, storeService,
authtypes.ProtoBaseAccount, maccPerms, sdk.Bech32MainPrefix,
authtypes.NewModuleAddress(govtypes.ModuleName).String(),
)
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
Loading