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(x/mint): remove staking as a required module #21858

Merged
merged 20 commits into from
Sep 24, 2024
Merged
6 changes: 4 additions & 2 deletions server/v2/cometbft/mempool/noop.go
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,10 @@ import (
sdk "github.com/cosmos/cosmos-sdk/types"
)

var _ Mempool[sdk.Tx] = (*NoOpMempool[sdk.Tx])(nil) // verify interface at compile time
var _ Mempool[transaction.Tx] = (*NoOpMempool[transaction.Tx])(nil)
var (
_ Mempool[sdk.Tx] = (*NoOpMempool[sdk.Tx])(nil) // verify interface at compile time
_ Mempool[transaction.Tx] = (*NoOpMempool[transaction.Tx])(nil)
)

// NoOpMempool defines a no-op mempool. Transactions are completely discarded and
// ignored when BaseApp interacts with the mempool.
Expand Down
9 changes: 6 additions & 3 deletions simapp/app.go
Original file line number Diff line number Diff line change
Expand Up @@ -165,7 +165,7 @@ type SimApp struct {
BankKeeper bankkeeper.BaseKeeper
StakingKeeper *stakingkeeper.Keeper
SlashingKeeper slashingkeeper.Keeper
MintKeeper mintkeeper.Keeper
MintKeeper *mintkeeper.Keeper
DistrKeeper distrkeeper.Keeper
GovKeeper govkeeper.Keeper
UpgradeKeeper *upgradekeeper.Keeper
Expand Down Expand Up @@ -372,7 +372,10 @@ func NewSimApp(
cometService,
)

app.MintKeeper = mintkeeper.NewKeeper(appCodec, runtime.NewEnvironment(runtime.NewKVStoreService(keys[minttypes.StoreKey]), logger.With(log.ModuleKey, "x/mint")), app.StakingKeeper, app.AuthKeeper, app.BankKeeper, authtypes.FeeCollectorName, govModuleAddr)
app.MintKeeper = mintkeeper.NewKeeper(appCodec, runtime.NewEnvironment(runtime.NewKVStoreService(keys[minttypes.StoreKey]), logger.With(log.ModuleKey, "x/mint")), app.AuthKeeper, app.BankKeeper, authtypes.FeeCollectorName, govModuleAddr)
if err := app.MintKeeper.SetMintFn(mintkeeper.DefaultMintFn(minttypes.DefaultInflationCalculationFn, app.StakingKeeper, app.MintKeeper)); err != nil {
panic(err)
}

app.PoolKeeper = poolkeeper.NewKeeper(appCodec, runtime.NewEnvironment(runtime.NewKVStoreService(keys[pooltypes.StoreKey]), logger.With(log.ModuleKey, "x/protocolpool")), app.AuthKeeper, app.BankKeeper, app.StakingKeeper, govModuleAddr)

Expand Down Expand Up @@ -467,7 +470,7 @@ func NewSimApp(
bank.NewAppModule(appCodec, app.BankKeeper, app.AuthKeeper),
feegrantmodule.NewAppModule(appCodec, app.FeeGrantKeeper, app.interfaceRegistry),
gov.NewAppModule(appCodec, &app.GovKeeper, app.AuthKeeper, app.BankKeeper, app.PoolKeeper),
mint.NewAppModule(appCodec, app.MintKeeper, app.AuthKeeper, nil),
mint.NewAppModule(appCodec, app.MintKeeper, app.AuthKeeper),
slashing.NewAppModule(appCodec, app.SlashingKeeper, app.AuthKeeper, app.BankKeeper, app.StakingKeeper, app.interfaceRegistry, cometService),
distr.NewAppModule(appCodec, app.DistrKeeper, app.StakingKeeper),
staking.NewAppModule(appCodec, app.StakingKeeper),
Expand Down
4 changes: 2 additions & 2 deletions tests/integration/example/example_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -74,8 +74,8 @@ func Example() {

// here bankkeeper and staking keeper is nil because we are not testing them
// subspace is nil because we don't test params (which is legacy anyway)
mintKeeper := mintkeeper.NewKeeper(encodingCfg.Codec, runtime.NewEnvironment(runtime.NewKVStoreService(keys[minttypes.StoreKey]), logger), nil, accountKeeper, nil, authtypes.FeeCollectorName, authority)
mintModule := mint.NewAppModule(encodingCfg.Codec, mintKeeper, accountKeeper, nil)
mintKeeper := mintkeeper.NewKeeper(encodingCfg.Codec, runtime.NewEnvironment(runtime.NewKVStoreService(keys[minttypes.StoreKey]), logger), accountKeeper, nil, authtypes.FeeCollectorName, authority)
mintModule := mint.NewAppModule(encodingCfg.Codec, mintKeeper, accountKeeper)

// create the application and register all the modules from the previous step
integrationApp := integration.NewIntegrationApp(
Expand Down
6 changes: 3 additions & 3 deletions x/accounts/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -56,8 +56,8 @@ func (a Account) AuthRetroCompatibility(ctx context.Context, _ *authtypes.QueryL

## Usage Notes

- Implement this handler only for account types you want to expose via x/auth gRPC methods.
- The `info` field in the response can be nil if your account doesn't fit the `BaseAccount` structure.
* Implement this handler only for account types you want to expose via x/auth gRPC methods.
* The `info` field in the response can be nil if your account doesn't fit the `BaseAccount` structure.

# Genesis

Expand Down Expand Up @@ -102,4 +102,4 @@ For example, given the following `genesis.json` file:
}
```

The accounts module will run the lockup account initialization message.
The accounts module will run the lockup account initialization message.
8 changes: 6 additions & 2 deletions x/mint/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -32,9 +32,13 @@ Ref: https://keepachangelog.com/en/1.0.0/

### Improvements

### Bug Fixes

### API Breaking Changes

* [#20363](https://github.com/cosmos/cosmos-sdk/pull/20363) Deprecated InflationCalculationFn in favor of MintFn, `keeper.DefaultMintFn` wrapper must be used in order to continue using it in `NewAppModule`. This is not breaking for depinject users, as both `MintFn` and `InflationCalculationFn` are accepted.
* [#19367](https://github.com/cosmos/cosmos-sdk/pull/19398) `appmodule.Environment` is received on the Keeper to get access to different application services.

### Bug Fixes
* [#21858](https://github.com/cosmos/cosmos-sdk/pull/21858) `NewKeeper` now returns a pointer to `Keeper`.
* [#21858](https://github.com/cosmos/cosmos-sdk/pull/21858) `DefaultMintFn` now takes `StakingKeeper` and `MintKeeper` as arguments to avoid staking keeper being required by mint.
* `SetMintFn` is used to replace the default minting function.
* `InflationCalculationFn` is not passed through depinject any longer, a MintFn is required instead.
38 changes: 16 additions & 22 deletions x/mint/depinject.go
tac0turtle marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
package mint

import (
"fmt"

modulev1 "cosmossdk.io/api/cosmos/mint/module/v1"
"cosmossdk.io/core/appmodule"
"cosmossdk.io/depinject"
Expand All @@ -21,28 +23,25 @@ func (am AppModule) IsOnePerModuleType() {}
func init() {
appconfig.RegisterModule(&modulev1.Module{},
appconfig.Provide(ProvideModule),
appconfig.Invoke(InvokeSetMintFn),
)
}

type ModuleInputs struct {
depinject.In

ModuleKey depinject.OwnModuleKey
Config *modulev1.Module
Environment appmodule.Environment
Cdc codec.Codec
MintFn types.MintFn `optional:"true"`
InflationCalculationFn types.InflationCalculationFn `optional:"true"` // deprecated
Config *modulev1.Module
Environment appmodule.Environment
Cdc codec.Codec

AccountKeeper types.AccountKeeper
BankKeeper types.BankKeeper
StakingKeeper types.StakingKeeper
}

type ModuleOutputs struct {
depinject.Out

MintKeeper keeper.Keeper
MintKeeper *keeper.Keeper
Module appmodule.AppModule
EpochHooks epochstypes.EpochHooksWrapper
}
Expand All @@ -67,28 +66,23 @@ func ProvideModule(in ModuleInputs) ModuleOutputs {
k := keeper.NewKeeper(
in.Cdc,
in.Environment,
in.StakingKeeper,
in.AccountKeeper,
in.BankKeeper,
feeCollectorName,
as,
)

if in.MintFn != nil && in.InflationCalculationFn != nil {
tac0turtle marked this conversation as resolved.
Show resolved Hide resolved
panic("MintFn and InflationCalculationFn cannot both be set")
}
m := NewAppModule(in.Cdc, k, in.AccountKeeper)

// if no mintFn is provided, use the default minting function
if in.MintFn == nil {
// if no inflationCalculationFn is provided, use the default inflation calculation function
if in.InflationCalculationFn == nil {
in.InflationCalculationFn = types.DefaultInflationCalculationFn
}
return ModuleOutputs{MintKeeper: k, Module: m, EpochHooks: epochstypes.EpochHooksWrapper{EpochHooks: m}}
}

in.MintFn = k.DefaultMintFn(in.InflationCalculationFn)
tac0turtle marked this conversation as resolved.
Show resolved Hide resolved
func InvokeSetMintFn(mintKeeper *keeper.Keeper, mintFn types.MintFn, stakingKeeper types.StakingKeeper) error {
if mintFn == nil && stakingKeeper == nil {
return fmt.Errorf("custom minting function or staking keeper must be supplied or available")
} else if mintFn == nil {
mintFn = keeper.DefaultMintFn(types.DefaultInflationCalculationFn, stakingKeeper, mintKeeper)
}

m := NewAppModule(in.Cdc, k, in.AccountKeeper, in.MintFn)

return ModuleOutputs{MintKeeper: k, Module: m, EpochHooks: epochstypes.EpochHooksWrapper{EpochHooks: m}}
return mintKeeper.SetMintFn(mintFn)
}
2 changes: 1 addition & 1 deletion x/mint/epoch_hooks.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ func (am AppModule) BeforeEpochStart(ctx context.Context, epochIdentifier string

oldMinter := minter

err = am.mintFn(ctx, am.keeper.Environment, &minter, epochIdentifier, epochNumber)
err = am.keeper.MintFn(ctx, &minter, epochIdentifier, epochNumber)
if err != nil {
return err
}
Expand Down
4 changes: 2 additions & 2 deletions x/mint/keeper/abci.go
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ import (
)

// BeginBlocker mints new tokens for the previous block.
func (k Keeper) BeginBlocker(ctx context.Context, mintFn types.MintFn) error {
func (k Keeper) BeginBlocker(ctx context.Context) error {
defer telemetry.ModuleMeasureSince(types.ModuleName, telemetry.Now(), telemetry.MetricKeyBeginBlocker)

// fetch stored minter & params
Expand All @@ -22,7 +22,7 @@ func (k Keeper) BeginBlocker(ctx context.Context, mintFn types.MintFn) error {

// we pass -1 as epoch number to indicate that this is not an epoch minting,
// but a regular block minting. Same with epoch id "block".
err = mintFn(ctx, k.Environment, &minter, "block", -1)
err = k.MintFn(ctx, &minter, "block", -1)
if err != nil {
return err
}
Expand Down
11 changes: 8 additions & 3 deletions x/mint/keeper/genesis_test.go
Original file line number Diff line number Diff line change
@@ -1,12 +1,14 @@
package keeper_test

import (
"context"
"testing"

"github.com/golang/mock/gomock"
"github.com/stretchr/testify/suite"

"cosmossdk.io/collections"
"cosmossdk.io/core/appmodule"
"cosmossdk.io/log"
"cosmossdk.io/math"
storetypes "cosmossdk.io/store/types"
Expand All @@ -30,7 +32,7 @@ type GenesisTestSuite struct {
suite.Suite

sdkCtx sdk.Context
keeper keeper.Keeper
keeper *keeper.Keeper
cdc codec.BinaryCodec
accountKeeper types.AccountKeeper
key *storetypes.KVStoreKey
Expand All @@ -51,14 +53,17 @@ func (s *GenesisTestSuite) SetupTest() {
s.sdkCtx = testCtx.Ctx
s.key = key

stakingKeeper := minttestutil.NewMockStakingKeeper(ctrl)
accountKeeper := minttestutil.NewMockAccountKeeper(ctrl)
bankKeeper := minttestutil.NewMockBankKeeper(ctrl)
s.accountKeeper = accountKeeper
accountKeeper.EXPECT().GetModuleAddress(minterAcc.Name).Return(minterAcc.GetAddress())
accountKeeper.EXPECT().GetModuleAccount(s.sdkCtx, minterAcc.Name).Return(minterAcc)

s.keeper = keeper.NewKeeper(s.cdc, runtime.NewEnvironment(runtime.NewKVStoreService(key), log.NewNopLogger()), stakingKeeper, accountKeeper, bankKeeper, "", "")
s.keeper = keeper.NewKeeper(s.cdc, runtime.NewEnvironment(runtime.NewKVStoreService(key), log.NewNopLogger()), accountKeeper, bankKeeper, "", "")
err := s.keeper.SetMintFn(func(ctx context.Context, env appmodule.Environment, minter *types.Minter, epochId string, epochNumber int64) error {
return nil
})
s.NoError(err)
Comment on lines +63 to +66
Copy link
Contributor

Choose a reason for hiding this comment

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

Enhance test coverage for SetMintFn

While the SetMintFn is being set, the current implementation is a no-op function. Consider adding more comprehensive tests to verify the behavior of this new functionality.

Suggestions:

  1. Add test cases that use different SetMintFn implementations to ensure it's correctly integrated.
  2. Verify that the SetMintFn is called during relevant mint operations in other test cases.

Example test case:

func (s *GenesisTestSuite) TestSetMintFn() {
    called := false
    err := s.keeper.SetMintFn(func(ctx context.Context, env appmodule.Environment, minter *types.Minter, epochId string, epochNumber int64) error {
        called = true
        return nil
    })
    s.NoError(err)

    // Trigger a mint operation
    // ...

    s.True(called, "SetMintFn was not called during minting operation")
}

}

func (s *GenesisTestSuite) TestImportExportGenesis() {
Expand Down
4 changes: 2 additions & 2 deletions x/mint/keeper/grpc_query.go
Original file line number Diff line number Diff line change
Expand Up @@ -8,12 +8,12 @@ import (

var _ types.QueryServer = queryServer{}

func NewQueryServerImpl(k Keeper) types.QueryServer {
func NewQueryServerImpl(k *Keeper) types.QueryServer {
return queryServer{k}
}

type queryServer struct {
k Keeper
k *Keeper
}

// Params returns params of the mint module.
Expand Down
4 changes: 1 addition & 3 deletions x/mint/keeper/grpc_query_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ type MintTestSuite struct {

ctx sdk.Context
queryClient types.QueryClient
mintKeeper keeper.Keeper
mintKeeper *keeper.Keeper
}

func (suite *MintTestSuite) SetupTest() {
Expand All @@ -42,14 +42,12 @@ func (suite *MintTestSuite) SetupTest() {
ctrl := gomock.NewController(suite.T())
accountKeeper := minttestutil.NewMockAccountKeeper(ctrl)
bankKeeper := minttestutil.NewMockBankKeeper(ctrl)
stakingKeeper := minttestutil.NewMockStakingKeeper(ctrl)

accountKeeper.EXPECT().GetModuleAddress("mint").Return(sdk.AccAddress{})

suite.mintKeeper = keeper.NewKeeper(
encCfg.Codec,
env,
stakingKeeper,
accountKeeper,
bankKeeper,
authtypes.FeeCollectorName,
Expand Down
47 changes: 25 additions & 22 deletions x/mint/keeper/keeper.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@ type Keeper struct {
appmodule.Environment

cdc codec.BinaryCodec
stakingKeeper types.StakingKeeper
bankKeeper types.BankKeeper
feeCollectorName string
// the address capable of executing a MsgUpdateParams message. Typically, this
Expand All @@ -30,18 +29,21 @@ type Keeper struct {
Schema collections.Schema
Params collections.Item[types.Params]
Minter collections.Item[types.Minter]

// mintFn is used to mint new coins during BeginBlock. This function is in charge of
// minting new coins based on arbitrary logic, previously done through InflationCalculationFn.
mintFn types.MintFn
}

// NewKeeper creates a new mint Keeper instance
func NewKeeper(
cdc codec.BinaryCodec,
env appmodule.Environment,
sk types.StakingKeeper,
ak types.AccountKeeper,
bk types.BankKeeper,
feeCollectorName string,
authority string,
) Keeper {
) *Keeper {
// ensure mint module account is set
if addr := ak.GetModuleAddress(types.ModuleName); addr == nil {
panic(fmt.Sprintf("the x/%s module account has not been set", types.ModuleName))
Expand All @@ -51,7 +53,6 @@ func NewKeeper(
k := Keeper{
Environment: env,
cdc: cdc,
stakingKeeper: sk,
bankKeeper: bk,
feeCollectorName: feeCollectorName,
authority: authority,
Expand All @@ -64,29 +65,25 @@ func NewKeeper(
panic(err)
}
k.Schema = schema
return k
}

// GetAuthority returns the x/mint module's authority.
func (k Keeper) GetAuthority() string {
return k.authority
return &k
}

// StakingTokenSupply implements an alias call to the underlying staking keeper's
// StakingTokenSupply to be used in BeginBlocker.
func (k Keeper) StakingTokenSupply(ctx context.Context) (math.Int, error) {
return k.stakingKeeper.StakingTokenSupply(ctx)
// SetMintFn is used to mint new coins during BeginBlock. The mintFn function is in charge of
// minting new coins based on arbitrary logic, previously done through InflationCalculationFn.
func (k *Keeper) SetMintFn(mintFn types.MintFn) error {
k.mintFn = mintFn
return nil
}

// BondedRatio implements an alias call to the underlying staking keeper's
// BondedRatio to be used in BeginBlocker.
func (k Keeper) BondedRatio(ctx context.Context) (math.LegacyDec, error) {
return k.stakingKeeper.BondedRatio(ctx)
// GetAuthority returns the x/mint module's authority.
func (k *Keeper) GetAuthority() string {
return k.authority
}

// MintCoins implements an alias call to the underlying supply keeper's
// MintCoins to be used in BeginBlocker.
func (k Keeper) MintCoins(ctx context.Context, newCoins sdk.Coins) error {
func (k *Keeper) MintCoins(ctx context.Context, newCoins sdk.Coins) error {
if newCoins.Empty() {
// skip as no coins need to be minted
return nil
Expand All @@ -97,24 +94,30 @@ func (k Keeper) MintCoins(ctx context.Context, newCoins sdk.Coins) error {

// AddCollectedFees implements an alias call to the underlying supply keeper's
// AddCollectedFees to be used in BeginBlocker.
func (k Keeper) AddCollectedFees(ctx context.Context, fees sdk.Coins) error {
func (k *Keeper) AddCollectedFees(ctx context.Context, fees sdk.Coins) error {
return k.bankKeeper.SendCoinsFromModuleToModule(ctx, types.ModuleName, k.feeCollectorName, fees)
}

func (k Keeper) DefaultMintFn(ic types.InflationCalculationFn) types.MintFn {
func (k *Keeper) MintFn(ctx context.Context, minter *types.Minter, epochId string, epochNumber int64) error {
return k.mintFn(ctx, k.Environment, minter, epochId, epochNumber)
}

// DefaultMintFn returns a default mint function. It requires the Staking module and the mint keeper.
// The default Mintfn has a requirement on staking as it uses bond to calculate inflation.
func DefaultMintFn(ic types.InflationCalculationFn, staking types.StakingKeeper, k *Keeper) types.MintFn {
return func(ctx context.Context, env appmodule.Environment, minter *types.Minter, epochId string, epochNumber int64) error {
// the default mint function is called every block, so we only check if epochId is "block" which is
// a special value to indicate that this is not an epoch minting, but a regular block minting.
if epochId != "block" {
return nil
}

stakingTokenSupply, err := k.StakingTokenSupply(ctx)
stakingTokenSupply, err := staking.StakingTokenSupply(ctx)
if err != nil {
return err
}

bondedRatio, err := k.BondedRatio(ctx)
bondedRatio, err := staking.BondedRatio(ctx)
if err != nil {
return err
}
Expand Down
Loading
Loading