diff --git a/.pending/bugfixes/sdk/4681-mint-check b/.pending/bugfixes/sdk/4681-mint-check new file mode 100644 index 000000000000..ed395eab1651 --- /dev/null +++ b/.pending/bugfixes/sdk/4681-mint-check @@ -0,0 +1,2 @@ +#4681 panic on invalid amount on `MintCoins` and `BurnCoins` + - skip minting if inflation is set to zero \ No newline at end of file diff --git a/x/mint/internal/keeper/keeper.go b/x/mint/internal/keeper/keeper.go index bf44060db896..ff4bd029dfda 100644 --- a/x/mint/internal/keeper/keeper.go +++ b/x/mint/internal/keeper/keeper.go @@ -97,6 +97,10 @@ func (k Keeper) BondedRatio(ctx sdk.Context) sdk.Dec { // MintCoins implements an alias call to the underlying supply keeper's // MintCoins to be used in BeginBlocker. func (k Keeper) MintCoins(ctx sdk.Context, newCoins sdk.Coins) sdk.Error { + if newCoins.Empty() { + // skip as no coins need to be minted + return nil + } return k.supplyKeeper.MintCoins(ctx, types.ModuleName, newCoins) } diff --git a/x/staking/keeper/pool.go b/x/staking/keeper/pool.go index 5f9ff0535c1d..0c6cfcafe8ef 100644 --- a/x/staking/keeper/pool.go +++ b/x/staking/keeper/pool.go @@ -37,6 +37,7 @@ func (k Keeper) notBondedTokensToBonded(ctx sdk.Context, tokens sdk.Int) { // burnBondedTokens removes coins from the bonded pool module account func (k Keeper) burnBondedTokens(ctx sdk.Context, amt sdk.Int) sdk.Error { if !amt.IsPositive() { + // skip as no coins need to be burned return nil } coins := sdk.NewCoins(sdk.NewCoin(k.BondDenom(ctx), amt)) @@ -46,6 +47,7 @@ func (k Keeper) burnBondedTokens(ctx sdk.Context, amt sdk.Int) sdk.Error { // burnNotBondedTokens removes coins from the not bonded pool module account func (k Keeper) burnNotBondedTokens(ctx sdk.Context, amt sdk.Int) sdk.Error { if !amt.IsPositive() { + // skip as no coins need to be burned return nil } coins := sdk.NewCoins(sdk.NewCoin(k.BondDenom(ctx), amt)) diff --git a/x/supply/internal/keeper/account.go b/x/supply/internal/keeper/account.go index 96a4ea3f52f7..8e0d22b4e8ce 100644 --- a/x/supply/internal/keeper/account.go +++ b/x/supply/internal/keeper/account.go @@ -6,7 +6,7 @@ import ( "github.com/cosmos/cosmos-sdk/x/supply/internal/types" ) -// GetModuleAddress returns a an address based on the name +// GetModuleAddress returns a an address based on the module name func (k Keeper) GetModuleAddress(moduleName string) sdk.AccAddress { permAddr, ok := k.permAddrs[moduleName] if !ok { @@ -15,7 +15,7 @@ func (k Keeper) GetModuleAddress(moduleName string) sdk.AccAddress { return permAddr.address } -// GetModuleAddressAndPermission returns an address and permission based on the name +// GetModuleAddressAndPermission returns an address and permission based on the module name func (k Keeper) GetModuleAddressAndPermission(moduleName string) (addr sdk.AccAddress, permission string) { permAddr, ok := k.permAddrs[moduleName] if !ok { @@ -24,7 +24,8 @@ func (k Keeper) GetModuleAddressAndPermission(moduleName string) (addr sdk.AccAd return permAddr.address, permAddr.permission } -// GetModuleAccount gets the module account to the auth account store +// GetModuleAccountAndPermission gets the module account from the auth account store and its +// registered permission func (k Keeper) GetModuleAccountAndPermission(ctx sdk.Context, moduleName string) (exported.ModuleAccountI, string) { addr, perm := k.GetModuleAddressAndPermission(moduleName) if addr == nil { @@ -48,7 +49,7 @@ func (k Keeper) GetModuleAccountAndPermission(ctx sdk.Context, moduleName string return maccI, perm } -// GetModuleAccount gets the module account to the auth account store +// GetModuleAccount gets the module account from the auth account store func (k Keeper) GetModuleAccount(ctx sdk.Context, moduleName string) exported.ModuleAccountI { acc, _ := k.GetModuleAccountAndPermission(ctx, moduleName) return acc diff --git a/x/supply/internal/keeper/bank.go b/x/supply/internal/keeper/bank.go index f8277566cee5..399750e5c1b2 100644 --- a/x/supply/internal/keeper/bank.go +++ b/x/supply/internal/keeper/bank.go @@ -28,12 +28,12 @@ func (k Keeper) SendCoinsFromModuleToModule(ctx sdk.Context, senderModule, recip } // create the account if it doesn't yet exist - recipientAddr := k.GetModuleAccount(ctx, recipientModule).GetAddress() - if recipientAddr == nil { + recipientAcc := k.GetModuleAccount(ctx, recipientModule) + if recipientAcc == nil { panic(fmt.Sprintf("module account %s isn't able to be created", recipientModule)) } - return k.bk.SendCoins(ctx, senderAddr, recipientAddr, amt) + return k.bk.SendCoins(ctx, senderAddr, recipientAcc.GetAddress(), amt) } // SendCoinsFromAccountToModule transfers coins from an AccAddress to a ModuleAccount @@ -41,12 +41,12 @@ func (k Keeper) SendCoinsFromAccountToModule(ctx sdk.Context, senderAddr sdk.Acc recipientModule string, amt sdk.Coins) sdk.Error { // create the account if it doesn't yet exist - recipientAddr := k.GetModuleAccount(ctx, recipientModule).GetAddress() - if recipientAddr == nil { + recipientAcc := k.GetModuleAccount(ctx, recipientModule) + if recipientAcc == nil { panic(fmt.Sprintf("module account %s isn't able to be created", recipientModule)) } - return k.bk.SendCoins(ctx, senderAddr, recipientAddr, amt) + return k.bk.SendCoins(ctx, senderAddr, recipientAcc.GetAddress(), amt) } // DelegateCoinsFromAccountToModule delegates coins and transfers @@ -55,12 +55,12 @@ func (k Keeper) DelegateCoinsFromAccountToModule(ctx sdk.Context, senderAddr sdk recipientModule string, amt sdk.Coins) sdk.Error { // create the account if it doesn't yet exist - recipientAddr := k.GetModuleAccount(ctx, recipientModule).GetAddress() - if recipientAddr == nil { + recipientAcc := k.GetModuleAccount(ctx, recipientModule) + if recipientAcc == nil { panic(fmt.Sprintf("module account %s isn't able to be created", recipientModule)) } - return k.bk.DelegateCoins(ctx, senderAddr, recipientAddr, amt) + return k.bk.DelegateCoins(ctx, senderAddr, recipientAcc.GetAddress(), amt) } // UndelegateCoinsFromModuleToAccount undelegates the unbonding coins and transfers @@ -76,27 +76,25 @@ func (k Keeper) UndelegateCoinsFromModuleToAccount(ctx sdk.Context, senderModule return k.bk.UndelegateCoins(ctx, senderAddr, recipientAddr, amt) } -// MintCoins creates new coins from thin air and adds it to the MinterAccount. -// Panics if the name maps to a non-minter module account. +// MintCoins creates new coins from thin air and adds it to the module account. +// Panics if the name maps to a non-minter module account or if the amount is invalid. func (k Keeper) MintCoins(ctx sdk.Context, moduleName string, amt sdk.Coins) sdk.Error { - if amt.Empty() { - panic("cannot mint empty coins") - } // create the account if it doesn't yet exist acc, perm := k.GetModuleAccountAndPermission(ctx, moduleName) - addr := acc.GetAddress() - if addr == nil { + if acc == nil { return sdk.ErrUnknownAddress(fmt.Sprintf("module account %s does not exist", moduleName)) } + addr := acc.GetAddress() + if perm != types.Minter { - panic(fmt.Sprintf("Account %s does not have permissions to mint tokens", moduleName)) + panic(fmt.Sprintf("module account %s does not have permissions to mint tokens", moduleName)) } _, err := k.bk.AddCoins(ctx, addr, amt) if err != nil { - return err + panic(err) } // update total supply @@ -110,11 +108,9 @@ func (k Keeper) MintCoins(ctx sdk.Context, moduleName string, amt sdk.Coins) sdk return nil } -// BurnCoins burns coins deletes coins from the balance of the module account +// BurnCoins burns coins deletes coins from the balance of the module account. +// Panics if the name maps to a non-burner module account or if the amount is invalid. func (k Keeper) BurnCoins(ctx sdk.Context, moduleName string, amt sdk.Coins) sdk.Error { - if amt.Empty() { - panic("cannot burn empty coins") - } addr, perm := k.GetModuleAddressAndPermission(moduleName) if addr == nil { @@ -122,12 +118,12 @@ func (k Keeper) BurnCoins(ctx sdk.Context, moduleName string, amt sdk.Coins) sdk } if perm != types.Burner { - panic(fmt.Sprintf("Account %s does not have permissions to burn tokens", moduleName)) + panic(fmt.Sprintf("module account %s does not have permissions to burn tokens", moduleName)) } _, err := k.bk.SubtractCoins(ctx, addr, amt) if err != nil { - return err + panic(err) } // update total supply diff --git a/x/supply/internal/keeper/bank_test.go b/x/supply/internal/keeper/bank_test.go index e7c33e9a7b21..3f63f63e2a42 100644 --- a/x/supply/internal/keeper/bank_test.go +++ b/x/supply/internal/keeper/bank_test.go @@ -81,35 +81,32 @@ func TestMintCoins(t *testing.T) { initialSupply := keeper.GetSupply(ctx) - require.Panics(t, func() { keeper.MintCoins(ctx, "", initCoins) }) + require.Error(t, keeper.MintCoins(ctx, "", initCoins), "no module account") + require.Panics(t, func() { keeper.MintCoins(ctx, types.Burner, initCoins) }, "invalid permission") + require.Panics(t, func() { keeper.MintCoins(ctx, types.Minter, sdk.Coins{sdk.Coin{"denom", sdk.NewInt(-10)}}) }, "insufficient coins") //nolint err := keeper.MintCoins(ctx, types.Minter, initCoins) require.NoError(t, err) require.Equal(t, initCoins, getCoinsByName(ctx, keeper, types.Minter)) require.Equal(t, initialSupply.Total.Add(initCoins), keeper.GetSupply(ctx).Total) - - require.Panics(t, func() { keeper.MintCoins(ctx, types.Burner, initCoins) }) } func TestBurnCoins(t *testing.T) { nAccs := int64(4) ctx, _, keeper := createTestInput(t, false, initialPower, nAccs) - err := burnerAcc.SetCoins(initCoins) - require.NoError(t, err) + require.NoError(t, burnerAcc.SetCoins(initCoins)) keeper.SetModuleAccount(ctx, burnerAcc) initialSupply := keeper.GetSupply(ctx) initialSupply.Inflate(initCoins) keeper.SetSupply(ctx, initialSupply) - err = keeper.BurnCoins(ctx, "", initCoins) - require.Error(t, err) - - err = keeper.BurnCoins(ctx, types.Burner, initialSupply.Total) - require.Error(t, err) + require.Error(t, keeper.BurnCoins(ctx, "", initCoins), "no module account") + require.Panics(t, func() { keeper.BurnCoins(ctx, types.Minter, initCoins) }, "invalid permission") + require.Panics(t, func() { keeper.BurnCoins(ctx, types.Burner, initialSupply.Total) }, "insufficient coins") - err = keeper.BurnCoins(ctx, types.Burner, initCoins) + err := keeper.BurnCoins(ctx, types.Burner, initCoins) require.NoError(t, err) require.Equal(t, sdk.Coins(nil), getCoinsByName(ctx, keeper, types.Burner)) require.Equal(t, initialSupply.Total.Sub(initCoins), keeper.GetSupply(ctx).Total)