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(bank): migrate to use env var #19477

Merged
merged 6 commits into from
Feb 20, 2024
Merged

refactor(bank): migrate to use env var #19477

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
8fc9cad
migrate bank to use env
tac0turtle Feb 19, 2024
2cd406a
linting
tac0turtle Feb 19, 2024
f4434af
fix linting
tac0turtle Feb 19, 2024
c8e0903
fix events
tac0turtle Feb 19, 2024
655ceeb
changelog
tac0turtle Feb 19, 2024
5061b91
Merge branch 'main' into marko/bank_env
tac0turtle Feb 19, 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
2 changes: 1 addition & 1 deletion simapp/app.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -308,8 +308,8 @@ func NewSimApp(
app.AuthKeeper = authkeeper.NewAccountKeeper(appCodec, runtime.NewKVStoreService(keys[authtypes.StoreKey]), authtypes.ProtoBaseAccount, maccPerms, addressCodec, sdk.Bech32MainPrefix, authtypes.NewModuleAddress(govtypes.ModuleName).String())

app.BankKeeper = bankkeeper.NewBaseKeeper(
runtime.NewEnvironment(runtime.NewKVStoreService(keys[banktypes.StoreKey]), logger),
appCodec,
runtime.NewKVStoreService(keys[banktypes.StoreKey]),
app.AuthKeeper,
BlockedAddresses(),
authtypes.NewModuleAddress(govtypes.ModuleName).String(),
Expand Down
3 changes: 2 additions & 1 deletion tests/integration/bank/keeper/deterministic_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,8 +90,9 @@ func initDeterministicFixture(t *testing.T) *deterministicFixture {
accountKeeper.GetAuthority(): false,
}
bankKeeper := keeper.NewBaseKeeper(

runtime.NewEnvironment(runtime.NewKVStoreService(keys[banktypes.StoreKey]), log.NewNopLogger()),
cdc,
runtime.NewKVStoreService(keys[banktypes.StoreKey]),
accountKeeper,
blockedAddresses,
authority.String(),
Expand Down
2 changes: 1 addition & 1 deletion tests/integration/distribution/keeper/msg_server_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,8 +97,8 @@ func initFixture(t *testing.T) *fixture {
accountKeeper.GetAuthority(): false,
}
bankKeeper := bankkeeper.NewBaseKeeper(
runtime.NewEnvironment(runtime.NewKVStoreService(keys[banktypes.StoreKey]), log.NewNopLogger()),
cdc,
runtime.NewKVStoreService(keys[banktypes.StoreKey]),
accountKeeper,
blockedAddresses,
authority.String(),
Expand Down
2 changes: 1 addition & 1 deletion tests/integration/evidence/keeper/infraction_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,8 +117,8 @@ func initFixture(tb testing.TB) *fixture {
accountKeeper.GetAuthority(): false,
}
bankKeeper := bankkeeper.NewBaseKeeper(
runtime.NewEnvironment(runtime.NewKVStoreService(keys[banktypes.StoreKey]), log.NewNopLogger()),
cdc,
runtime.NewKVStoreService(keys[banktypes.StoreKey]),
accountKeeper,
blockedAddresses,
authority.String(),
Expand Down
2 changes: 1 addition & 1 deletion tests/integration/gov/keeper/keeper_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,8 +84,8 @@ func initFixture(tb testing.TB) *fixture {
accountKeeper.GetAuthority(): false,
}
bankKeeper := bankkeeper.NewBaseKeeper(
runtime.NewEnvironment(runtime.NewKVStoreService(keys[banktypes.StoreKey]), log.NewNopLogger()),
cdc,
runtime.NewKVStoreService(keys[banktypes.StoreKey]),
accountKeeper,
blockedAddresses,
authority.String(),
Expand Down
2 changes: 1 addition & 1 deletion tests/integration/slashing/keeper/keeper_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,8 +85,8 @@ func initFixture(tb testing.TB) *fixture {
accountKeeper.GetAuthority(): false,
}
bankKeeper := bankkeeper.NewBaseKeeper(
runtime.NewEnvironment(runtime.NewKVStoreService(keys[banktypes.StoreKey]), log.NewNopLogger()),
cdc,
runtime.NewKVStoreService(keys[banktypes.StoreKey]),
accountKeeper,
blockedAddresses,
authority.String(),
Expand Down
2 changes: 1 addition & 1 deletion tests/integration/staking/keeper/common_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -134,8 +134,8 @@ func initFixture(tb testing.TB) *fixture {
accountKeeper.GetAuthority(): false,
}
bankKeeper := bankkeeper.NewBaseKeeper(
runtime.NewEnvironment(runtime.NewKVStoreService(keys[banktypes.StoreKey]), log.NewNopLogger()),
cdc,
runtime.NewKVStoreService(keys[banktypes.StoreKey]),
accountKeeper,
blockedAddresses,
authority.String(),
Expand Down
2 changes: 1 addition & 1 deletion tests/integration/staking/keeper/deterministic_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,8 +98,8 @@ func initDeterministicFixture(t *testing.T) *deterministicFixture {
accountKeeper.GetAuthority(): false,
}
bankKeeper := bankkeeper.NewBaseKeeper(
runtime.NewEnvironment(runtime.NewKVStoreService(keys[banktypes.StoreKey]), log.NewNopLogger()),
cdc,
runtime.NewKVStoreService(keys[banktypes.StoreKey]),
accountKeeper,
blockedAddresses,
authority.String(),
Expand Down
2 changes: 1 addition & 1 deletion x/auth/vesting/fuzz_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,8 +107,8 @@ func FuzzMsgServerCreateVestingAccount(f *testing.F) {
authKeeper := banktestutil.NewMockAccountKeeper(ctrl)
authKeeper.EXPECT().AddressCodec().Return(address.NewBech32Codec("cosmos")).AnyTimes()
bankKeeper := keeper.NewBaseKeeper(
runtime.NewEnvironment(storeService, log.NewNopLogger()),
encCfg.Codec,
storeService,
authKeeper,
map[string]bool{accAddrs[4].String(): true},
authtypes.NewModuleAddress(banktypes.GovModuleName).String(),
Expand Down
1 change: 1 addition & 0 deletions x/bank/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,5 +40,6 @@ Ref: https://keepachangelog.com/en/1.0.0/
### API Breaking Changes

* [#17569](https://github.com/cosmos/cosmos-sdk/pull/17569) `BurnCoins` takes an address instead of a module name
* [#19477](https://github.com/cosmos/cosmos-sdk/pull/19477) `appmodule.Environment` is passed to bank `NewKeeper`

### Bug Fixes
11 changes: 5 additions & 6 deletions x/bank/depinject.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@ package bank
import (
modulev1 "cosmossdk.io/api/cosmos/bank/module/v1"
"cosmossdk.io/core/appmodule"
"cosmossdk.io/core/store"
"cosmossdk.io/depinject"
"cosmossdk.io/depinject/appconfig"
"cosmossdk.io/log"
Expand All @@ -28,10 +27,10 @@ func init() {
type ModuleInputs struct {
depinject.In

Config *modulev1.Module
Cdc codec.Codec
StoreService store.KVStoreService
Logger log.Logger
Config *modulev1.Module
Cdc codec.Codec
Environment appmodule.Environment
Logger log.Logger

AccountKeeper types.AccountKeeper
}
Expand Down Expand Up @@ -79,8 +78,8 @@ func ProvideModule(in ModuleInputs) ModuleOutputs {
}

bankKeeper := keeper.NewBaseKeeper(
in.Environment,
in.Cdc,
in.StoreService,
in.AccountKeeper,
blockedAddresses,
authStr,
Expand Down
8 changes: 4 additions & 4 deletions x/bank/keeper/collections_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,16 +30,16 @@ func TestBankStateCompatibility(t *testing.T) {
ctx := testCtx.Ctx.WithHeaderInfo(header.Info{Time: time.Now()})
encCfg := moduletestutil.MakeTestEncodingConfig()

storeService := runtime.NewKVStoreService(key)
env := runtime.NewEnvironment(runtime.NewKVStoreService(key), log.NewNopLogger())

// gomock initializations
ctrl := gomock.NewController(t)
authKeeper := banktestutil.NewMockAccountKeeper(ctrl)
authKeeper.EXPECT().AddressCodec().Return(address.NewBech32Codec("cosmos")).AnyTimes()

k := keeper.NewBaseKeeper(
env,
encCfg.Codec,
storeService,
authKeeper,
map[string]bool{accAddrs[4].String(): true},
authtypes.NewModuleAddress(banktypes.GovModuleName).String(),
Expand All @@ -56,7 +56,7 @@ func TestBankStateCompatibility(t *testing.T) {
)
require.NoError(t, err)
// we set the index key to the old value.
require.NoError(t, storeService.OpenKVStore(ctx).Set(rawKey, bankDenomAddressLegacyIndexValue))
require.NoError(t, env.KVStoreService.OpenKVStore(ctx).Set(rawKey, bankDenomAddressLegacyIndexValue))

// test walking is ok
err = k.Balances.Indexes.Denom.Walk(ctx, nil, func(indexingKey string, indexedKey sdk.AccAddress) (stop bool, err error) {
Expand All @@ -79,7 +79,7 @@ func TestBankStateCompatibility(t *testing.T) {
err = k.Balances.Indexes.Denom.Reference(ctx, collections.Join(sdk.AccAddress("test"), "atom"), math.ZeroInt(), nil)
require.NoError(t, err)

newRawValue, err := storeService.OpenKVStore(ctx).Get(rawKey)
newRawValue, err := env.KVStoreService.OpenKVStore(ctx).Get(rawKey)
require.NoError(t, err)
require.Equal(t, []byte{}, newRawValue)
}
2 changes: 1 addition & 1 deletion x/bank/keeper/grpc_query.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -172,7 +172,7 @@ func (k BaseKeeper) DenomsMetadata(c context.Context, req *types.QueryDenomsMeta
if req == nil {
return nil, status.Errorf(codes.InvalidArgument, "empty request")
}
kvStore := runtime.KVStoreAdapter(k.storeService.OpenKVStore(c))
kvStore := runtime.KVStoreAdapter(k.environment.KVStoreService.OpenKVStore(c))
store := prefix.NewStore(kvStore, types.DenomMetadataPrefix)

metadatas := []types.Metadata{}
Expand Down
45 changes: 23 additions & 22 deletions x/bank/keeper/keeper.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,8 @@ import (
"context"
"fmt"

"cosmossdk.io/core/store"
"cosmossdk.io/core/appmodule"
"cosmossdk.io/core/event"
errorsmod "cosmossdk.io/errors"
"cosmossdk.io/log"
"cosmossdk.io/math"
Expand Down Expand Up @@ -58,7 +59,7 @@ type BaseKeeper struct {

ak types.AccountKeeper
cdc codec.BinaryCodec
storeService store.KVStoreService
environment appmodule.Environment
mintCoinsRestrictionFn types.MintingRestrictionFn
logger log.Logger
}
Expand All @@ -82,8 +83,8 @@ func (k BaseKeeper) GetPaginatedTotalSupply(ctx context.Context, pagination *que
// to receive funds through direct and explicit actions, for example, by using a MsgSend or
// by using a SendCoinsFromModuleToAccount execution.
func NewBaseKeeper(
env appmodule.Environment,
cdc codec.BinaryCodec,
storeService store.KVStoreService,
ak types.AccountKeeper,
blockedAddrs map[string]bool,
authority string,
Expand All @@ -97,10 +98,10 @@ func NewBaseKeeper(
logger = logger.With(log.ModuleKey, "x/"+types.ModuleName)

return BaseKeeper{
BaseSendKeeper: NewBaseSendKeeper(cdc, storeService, ak, blockedAddrs, authority, logger),
BaseSendKeeper: NewBaseSendKeeper(env, cdc, ak, blockedAddrs, authority, logger),
ak: ak,
cdc: cdc,
storeService: storeService,
environment: env,
mintCoinsRestrictionFn: types.NoOpMintingRestrictionFn,
logger: logger,
}
Expand Down Expand Up @@ -156,10 +157,13 @@ func (k BaseKeeper) DelegateCoins(ctx context.Context, delegatorAddr, moduleAccA
if err != nil {
return err
}
sdkCtx := sdk.UnwrapSDKContext(ctx)
sdkCtx.EventManager().EmitEvent(
types.NewCoinSpentEvent(delAddrStr, amt),
)
if err := k.environment.EventService.EventManager(ctx).EmitKV(
types.EventTypeCoinSpent,
event.NewAttribute(types.AttributeKeySpender, delAddrStr),
event.NewAttribute(sdk.AttributeKeyAmount, amt.String()),
); err != nil {
return err
}

err = k.addCoins(ctx, moduleAccAddr, amt)
if err != nil {
Expand Down Expand Up @@ -345,8 +349,6 @@ func (k BaseKeeper) UndelegateCoinsFromModuleToAccount(
// MintCoins creates new coins from thin air and adds it to the module account.
// An error is returned if the module account does not exist or is unauthorized.
func (k BaseKeeper) MintCoins(ctx context.Context, moduleName string, amounts sdk.Coins) error {
sdkCtx := sdk.UnwrapSDKContext(ctx)

err := k.mintCoinsRestrictionFn(ctx, amounts)
if err != nil {
k.logger.Error(fmt.Sprintf("Module %q attempted to mint coins %s it doesn't have permission for, error %v", moduleName, amounts, err))
Expand Down Expand Up @@ -378,12 +380,13 @@ func (k BaseKeeper) MintCoins(ctx context.Context, moduleName string, amounts sd
if err != nil {
return err
}

// emit mint event
sdkCtx.EventManager().EmitEvent(
types.NewCoinMintEvent(addrStr, amounts),
return k.environment.EventService.EventManager(ctx).EmitKV(
types.EventTypeCoinMint,
event.NewAttribute(types.AttributeKeyMinter, addrStr),
event.NewAttribute(sdk.AttributeKeyAmount, amounts.String()),
)

return nil
}

// BurnCoins burns coins deletes coins from the balance of an account.
Expand Down Expand Up @@ -418,12 +421,11 @@ func (k BaseKeeper) BurnCoins(ctx context.Context, address []byte, amounts sdk.C
return err
}
// emit burn event
sdkCtx := sdk.UnwrapSDKContext(ctx)
sdkCtx.EventManager().EmitEvent(
types.NewCoinBurnEvent(addrStr, amounts),
return k.environment.EventService.EventManager(ctx).EmitKV(
types.EventTypeCoinBurn,
event.NewAttribute(types.AttributeKeyBurner, addrStr),
event.NewAttribute(sdk.AttributeKeyAmount, amounts.String()),
)

return nil
}

// setSupply sets the supply for the given coin
Expand All @@ -446,8 +448,7 @@ func (k BaseKeeper) trackDelegation(ctx context.Context, addr sdk.AccAddress, ba
vacc, ok := acc.(types.VestingAccount)
if ok {
// TODO: return error on account.TrackDelegation
sdkCtx := sdk.UnwrapSDKContext(ctx)
vacc.TrackDelegation(sdkCtx.HeaderInfo().Time, balance, amt)
vacc.TrackDelegation(k.environment.HeaderService.GetHeaderInfo(ctx).Time, balance, amt)
Copy link
Contributor

Choose a reason for hiding this comment

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

The comment regarding the potential return of an error from TrackDelegation suggests a future improvement. It's important to track this as a TODO or open an issue to ensure it's addressed, as error handling is crucial for robustness and reliability.

Would you like me to open a GitHub issue to track the enhancement of error handling in TrackDelegation and TrackUndelegation?

k.ak.SetAccount(ctx, acc)
}

Expand Down
8 changes: 4 additions & 4 deletions x/bank/keeper/keeper_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -131,7 +131,7 @@ func (suite *KeeperTestSuite) SetupTest() {
ctx := testCtx.Ctx.WithHeaderInfo(header.Info{Time: time.Now()})
encCfg := moduletestutil.MakeTestEncodingConfig()

storeService := runtime.NewKVStoreService(key)
env := runtime.NewEnvironment(runtime.NewKVStoreService(key), log.NewNopLogger())

// gomock initializations
ctrl := gomock.NewController(suite.T())
Comment on lines 131 to 137
Copy link
Contributor

Choose a reason for hiding this comment

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

📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [134-143]

The repeated creation of runtime.Environment instances in both SetupTest and TestGetAuthority functions is consistent and necessary for isolating test environments. However, consider if there's a way to reduce code duplication, perhaps by creating a helper function for initializing the environment that can be reused across tests.

// Suggestion to reduce duplication by creating a helper function.
func newTestEnvironment(key storetypes.StoreKey) runtime.Environment {
    return runtime.NewEnvironment(runtime.NewKVStoreService(key), log.NewNopLogger())
}

Then replace the direct calls with newTestEnvironment(key) where appropriate.

Also applies to: 303-306

Expand All @@ -140,8 +140,8 @@ func (suite *KeeperTestSuite) SetupTest() {
suite.ctx = ctx
suite.authKeeper = authKeeper
suite.bankKeeper = keeper.NewBaseKeeper(
env,
encCfg.Codec,
storeService,
suite.authKeeper,
map[string]bool{accAddrs[4].String(): true},
authtypes.NewModuleAddress(banktypes.GovModuleName).String(),
Expand Down Expand Up @@ -300,11 +300,11 @@ func (suite *KeeperTestSuite) TestPrependSendRestriction() {
}

func (suite *KeeperTestSuite) TestGetAuthority() {
storeService := runtime.NewKVStoreService(storetypes.NewKVStoreKey(banktypes.StoreKey))
env := runtime.NewEnvironment(runtime.NewKVStoreService(storetypes.NewKVStoreKey(banktypes.StoreKey)), log.NewNopLogger())
NewKeeperWithAuthority := func(authority string) keeper.BaseKeeper {
return keeper.NewBaseKeeper(
env,
moduletestutil.MakeTestEncodingConfig().Codec,
storeService,
suite.authKeeper,
nil,
authority,
Expand Down
Loading
Loading