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: remove panic usage in keeper methods #18636

Merged
merged 19 commits into from
Dec 12, 2023
Merged
Show file tree
Hide file tree
Changes from 12 commits
Commits
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
34 changes: 17 additions & 17 deletions x/bank/keeper/keeper.go
Original file line number Diff line number Diff line change
Expand Up @@ -254,14 +254,14 @@ func (k BaseKeeper) SetDenomMetaData(ctx context.Context, denomMetaData types.Me
}

// SendCoinsFromModuleToAccount transfers coins from a ModuleAccount to an AccAddress.
// It will panic if the module account does not exist. An error is returned if
// An error is returned if the module account does not exist or if
// the recipient address is black-listed or if sending the tokens fails.
func (k BaseKeeper) SendCoinsFromModuleToAccount(
ctx context.Context, senderModule string, recipientAddr sdk.AccAddress, amt sdk.Coins,
) error {
senderAddr := k.ak.GetModuleAddress(senderModule)
if senderAddr == nil {
panic(errorsmod.Wrapf(sdkerrors.ErrUnknownAddress, "module account %s does not exist", senderModule))
return errorsmod.Wrapf(sdkerrors.ErrUnknownAddress, "module account %s does not exist", senderModule)
}

if k.BlockedAddr(recipientAddr) {
Expand All @@ -272,74 +272,74 @@ func (k BaseKeeper) SendCoinsFromModuleToAccount(
}

// SendCoinsFromModuleToModule transfers coins from a ModuleAccount to another.
// It will panic if either module account does not exist.
// An error is returned if either module accounts does not exist.
func (k BaseKeeper) SendCoinsFromModuleToModule(
ctx context.Context, senderModule, recipientModule string, amt sdk.Coins,
) error {
senderAddr := k.ak.GetModuleAddress(senderModule)
if senderAddr == nil {
panic(errorsmod.Wrapf(sdkerrors.ErrUnknownAddress, "module account %s does not exist", senderModule))
return errorsmod.Wrapf(sdkerrors.ErrUnknownAddress, "module account %s does not exist", senderModule)
}

recipientAcc := k.ak.GetModuleAccount(ctx, recipientModule)
if recipientAcc == nil {
panic(errorsmod.Wrapf(sdkerrors.ErrUnknownAddress, "module account %s does not exist", recipientModule))
return errorsmod.Wrapf(sdkerrors.ErrUnknownAddress, "module account %s does not exist", recipientModule)
}

return k.SendCoins(ctx, senderAddr, recipientAcc.GetAddress(), amt)
}

// SendCoinsFromAccountToModule transfers coins from an AccAddress to a ModuleAccount.
// It will panic if the module account does not exist.
// An error is returned if the module account does not exist.
func (k BaseKeeper) SendCoinsFromAccountToModule(
ctx context.Context, senderAddr sdk.AccAddress, recipientModule string, amt sdk.Coins,
) error {
recipientAcc := k.ak.GetModuleAccount(ctx, recipientModule)
if recipientAcc == nil {
panic(errorsmod.Wrapf(sdkerrors.ErrUnknownAddress, "module account %s does not exist", recipientModule))
return errorsmod.Wrapf(sdkerrors.ErrUnknownAddress, "module account %s does not exist", recipientModule)
}

return k.SendCoins(ctx, senderAddr, recipientAcc.GetAddress(), amt)
}

// DelegateCoinsFromAccountToModule delegates coins and transfers them from a
// delegator account to a module account. It will panic if the module account
// delegator account to a module account. An error is returned if the module account
// does not exist or is unauthorized.
func (k BaseKeeper) DelegateCoinsFromAccountToModule(
ctx context.Context, senderAddr sdk.AccAddress, recipientModule string, amt sdk.Coins,
) error {
recipientAcc := k.ak.GetModuleAccount(ctx, recipientModule)
if recipientAcc == nil {
panic(errorsmod.Wrapf(sdkerrors.ErrUnknownAddress, "module account %s does not exist", recipientModule))
return errorsmod.Wrapf(sdkerrors.ErrUnknownAddress, "module account %s does not exist", recipientModule)
}

if !recipientAcc.HasPermission(authtypes.Staking) {
panic(errorsmod.Wrapf(sdkerrors.ErrUnauthorized, "module account %s does not have permissions to receive delegated coins", recipientModule))
return errorsmod.Wrapf(sdkerrors.ErrUnauthorized, "module account %s does not have permissions to receive delegated coins", recipientModule)
}

return k.DelegateCoins(ctx, senderAddr, recipientAcc.GetAddress(), amt)
}

// UndelegateCoinsFromModuleToAccount undelegates the unbonding coins and transfers
// them from a module account to the delegator account. It will panic if the
// them from a module account to the delegator account. An error is returned if the
// module account does not exist or is unauthorized.
func (k BaseKeeper) UndelegateCoinsFromModuleToAccount(
ctx context.Context, senderModule string, recipientAddr sdk.AccAddress, amt sdk.Coins,
) error {
acc := k.ak.GetModuleAccount(ctx, senderModule)
if acc == nil {
panic(errorsmod.Wrapf(sdkerrors.ErrUnknownAddress, "module account %s does not exist", senderModule))
return errorsmod.Wrapf(sdkerrors.ErrUnknownAddress, "module account %s does not exist", senderModule)
}

if !acc.HasPermission(authtypes.Staking) {
panic(errorsmod.Wrapf(sdkerrors.ErrUnauthorized, "module account %s does not have permissions to undelegate coins", senderModule))
return errorsmod.Wrapf(sdkerrors.ErrUnauthorized, "module account %s does not have permissions to undelegate coins", senderModule)
}

return k.UndelegateCoins(ctx, acc.GetAddress(), recipientAddr, amt)
}

// MintCoins creates new coins from thin air and adds it to the module account.
// It will panic if the module account does not exist or is unauthorized.
// 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)

Expand All @@ -350,11 +350,11 @@ func (k BaseKeeper) MintCoins(ctx context.Context, moduleName string, amounts sd
}
acc := k.ak.GetModuleAccount(ctx, moduleName)
if acc == nil {
panic(errorsmod.Wrapf(sdkerrors.ErrUnknownAddress, "module account %s does not exist", moduleName))
return errorsmod.Wrapf(sdkerrors.ErrUnknownAddress, "module account %s does not exist", moduleName)
}

if !acc.HasPermission(authtypes.Minter) {
panic(errorsmod.Wrapf(sdkerrors.ErrUnauthorized, "module account %s does not have permissions to mint tokens", moduleName))
return errorsmod.Wrapf(sdkerrors.ErrUnauthorized, "module account %s does not have permissions to mint tokens", moduleName)
}

err = k.addCoins(ctx, acc.GetAddress(), amounts)
Expand All @@ -379,7 +379,7 @@ func (k BaseKeeper) MintCoins(ctx context.Context, moduleName string, amounts sd
}

// BurnCoins burns coins deletes coins from the balance of the module account.
// It will panic if the module account does not exist or is unauthorized.
// An error is returned if the module account does not exist or is unauthorized.
func (k BaseKeeper) BurnCoins(ctx context.Context, address []byte, amounts sdk.Coins) error {
acc := k.ak.GetAccount(ctx, address)
if acc == nil {
Expand Down
41 changes: 20 additions & 21 deletions x/bank/keeper/keeper_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -396,20 +396,17 @@ func (suite *KeeperTestSuite) TestSupply_DelegateUndelegateCoins() {
require.NoError(keeper.SendCoinsFromModuleToAccount(ctx, banktypes.MintModuleName, holderAcc.GetAddress(), initCoins))

authKeeper.EXPECT().GetModuleAddress("").Return(nil)
require.Panics(func() {
_ = keeper.SendCoinsFromModuleToAccount(ctx, "", holderAcc.GetAddress(), initCoins)
})
err := keeper.SendCoinsFromModuleToAccount(ctx, "", holderAcc.GetAddress(), initCoins)
require.Error(err)

authKeeper.EXPECT().GetModuleAddress(burnerAcc.Name).Return(burnerAcc.GetAddress())
authKeeper.EXPECT().GetModuleAccount(ctx, "").Return(nil)
require.Panics(func() {
_ = keeper.SendCoinsFromModuleToModule(ctx, authtypes.Burner, "", initCoins)
})
err = keeper.SendCoinsFromModuleToModule(ctx, authtypes.Burner, "", initCoins)
require.Error(err)

authKeeper.EXPECT().GetModuleAddress("").Return(nil)
require.Panics(func() {
_ = keeper.SendCoinsFromModuleToAccount(ctx, "", baseAcc.GetAddress(), initCoins)
})
err = keeper.SendCoinsFromModuleToAccount(ctx, "", baseAcc.GetAddress(), initCoins)
require.Error(err)

authKeeper.EXPECT().GetModuleAddress(holderAcc.Name).Return(holderAcc.GetAddress())
authKeeper.EXPECT().GetAccount(suite.ctx, holderAcc.GetAddress()).Return(holderAcc)
Expand Down Expand Up @@ -456,20 +453,17 @@ func (suite *KeeperTestSuite) TestSupply_SendCoins() {
require.NoError(keeper.SendCoinsFromModuleToAccount(ctx, banktypes.MintModuleName, holderAcc.GetAddress(), initCoins))

authKeeper.EXPECT().GetModuleAddress("").Return(nil)
require.Panics(func() {
_ = keeper.SendCoinsFromModuleToModule(ctx, "", holderAcc.GetName(), initCoins)
})
err := keeper.SendCoinsFromModuleToModule(ctx, "", holderAcc.GetName(), initCoins)
require.Error(err)

authKeeper.EXPECT().GetModuleAddress(burnerAcc.Name).Return(burnerAcc.GetAddress())
authKeeper.EXPECT().GetModuleAccount(ctx, "").Return(nil)
require.Panics(func() {
_ = keeper.SendCoinsFromModuleToModule(ctx, authtypes.Burner, "", initCoins)
})
err = keeper.SendCoinsFromModuleToModule(ctx, authtypes.Burner, "", initCoins)
require.Error(err)

authKeeper.EXPECT().GetModuleAddress("").Return(nil)
require.Panics(func() {
_ = keeper.SendCoinsFromModuleToAccount(ctx, "", baseAcc.GetAddress(), initCoins)
})
err = keeper.SendCoinsFromModuleToAccount(ctx, "", baseAcc.GetAddress(), initCoins)
require.Error(err)

authKeeper.EXPECT().GetModuleAddress(holderAcc.Name).Return(holderAcc.GetAddress())
authKeeper.EXPECT().GetAccount(suite.ctx, holderAcc.GetAddress()).Return(holderAcc)
Expand Down Expand Up @@ -508,16 +502,21 @@ func (suite *KeeperTestSuite) TestSupply_MintCoins() {
require.NoError(err)

authKeeper.EXPECT().GetModuleAccount(ctx, "").Return(nil)
require.Panics(func() { _ = keeper.MintCoins(ctx, "", initCoins) }, "no module account")
err = keeper.MintCoins(ctx, "", initCoins)
require.Error(err)
require.ErrorContains(err, "module account does not exist")

suite.mockMintCoins(burnerAcc)
require.Panics(func() { _ = keeper.MintCoins(ctx, authtypes.Burner, initCoins) }, "invalid permission")
err = keeper.MintCoins(ctx, authtypes.Burner, initCoins)
require.Error(err)
require.ErrorContains(err, fmt.Sprintf("module account %s does not have permissions to mint tokens: unauthorized", authtypes.Burner))

suite.mockMintCoins(minterAcc)
require.Error(keeper.MintCoins(ctx, authtypes.Minter, sdk.Coins{sdk.Coin{Denom: "denom", Amount: math.NewInt(-10)}}), "insufficient coins")

authKeeper.EXPECT().GetModuleAccount(ctx, randomPerm).Return(nil)
require.Panics(func() { _ = keeper.MintCoins(ctx, randomPerm, initCoins) })
err = keeper.MintCoins(ctx, randomPerm, initCoins)
require.Error(err)

suite.mockMintCoins(minterAcc)
require.NoError(keeper.MintCoins(ctx, authtypes.Minter, initCoins))
Expand Down
19 changes: 12 additions & 7 deletions x/distribution/keeper/delegation.go
Original file line number Diff line number Diff line change
Expand Up @@ -51,17 +51,17 @@ func (k Keeper) calculateDelegationRewardsBetween(ctx context.Context, val sdk.V
) (sdk.DecCoins, error) {
// sanity check
if startingPeriod > endingPeriod {
panic("startingPeriod cannot be greater than endingPeriod")
return sdk.DecCoins{}, fmt.Errorf("startingPeriod cannot be greater than endingPeriod")
}

// sanity check
if stake.IsNegative() {
panic("stake should not be negative")
return sdk.DecCoins{}, fmt.Errorf("stake should not be negative")
}

valBz, err := k.stakingKeeper.ValidatorAddressCodec().StringToBytes(val.GetOperator())
if err != nil {
panic(err)
return sdk.DecCoins{}, err
}

// return staking * (ending - starting)
likhita-809 marked this conversation as resolved.
Show resolved Hide resolved
Expand All @@ -77,7 +77,7 @@ func (k Keeper) calculateDelegationRewardsBetween(ctx context.Context, val sdk.V

difference := ending.CumulativeRewardRatio.Sub(starting.CumulativeRewardRatio)
if difference.IsAnyNegative() {
panic("negative rewards should not be possible")
return sdk.DecCoins{}, fmt.Errorf("negative rewards should not be possible")
}
// note: necessary to truncate so we don't allow withdrawing more rewards than owed
rewards := difference.MulDecTruncate(stake)
Expand Down Expand Up @@ -123,14 +123,16 @@ func (k Keeper) CalculateDelegationRewards(ctx context.Context, val sdk.Validato
// Slashes this block happened after reward allocation, but we have to account
// for them for the stake sanity check below.
endingHeight := uint64(sdkCtx.BlockHeight())
var iterErr error
if endingHeight > startingHeight {
err = k.IterateValidatorSlashEventsBetween(ctx, valAddr, startingHeight, endingHeight,
func(height uint64, event types.ValidatorSlashEvent) (stop bool) {
endingPeriod := event.ValidatorPeriod
if endingPeriod > startingPeriod {
delRewards, err := k.calculateDelegationRewardsBetween(ctx, val, startingPeriod, endingPeriod, stake)
if err != nil {
panic(err)
iterErr = err
return true
}
rewards = rewards.Add(delRewards...)

Expand All @@ -142,6 +144,9 @@ func (k Keeper) CalculateDelegationRewards(ctx context.Context, val sdk.Validato
return false
},
)
if iterErr != nil {
return sdk.DecCoins{}, iterErr
}
if err != nil {
return sdk.DecCoins{}, err
}
Expand Down Expand Up @@ -178,10 +183,10 @@ func (k Keeper) CalculateDelegationRewards(ctx context.Context, val sdk.Validato
if stake.LTE(currentStake.Add(marginOfErr)) {
stake = currentStake
} else {
panic(fmt.Sprintf("calculated final stake for delegator %s greater than current stake"+
return sdk.DecCoins{}, fmt.Errorf("calculated final stake for delegator %s greater than current stake"+
"\n\tfinal stake:\t%s"+
"\n\tcurrent stake:\t%s",
del.GetDelegatorAddr(), stake, currentStake))
del.GetDelegatorAddr(), stake, currentStake)
}
}

Expand Down
16 changes: 12 additions & 4 deletions x/distribution/keeper/grpc_query.go
Original file line number Diff line number Diff line change
Expand Up @@ -264,34 +264,42 @@ func (k Querier) DelegationTotalRewards(ctx context.Context, req *types.QueryDel
return nil, err
}

var iterErr error
err = k.stakingKeeper.IterateDelegations(
ctx, delAdr,
func(_ int64, del sdk.DelegationI) (stop bool) {
valAddr, err := k.stakingKeeper.ValidatorAddressCodec().StringToBytes(del.GetValidatorAddr())
if err != nil {
panic(err)
iterErr = err
return true
likhita-809 marked this conversation as resolved.
Show resolved Hide resolved
}

val, err := k.stakingKeeper.Validator(ctx, valAddr)
if err != nil {
panic(err)
iterErr = err
return true
}

endingPeriod, err := k.IncrementValidatorPeriod(ctx, val)
if err != nil {
panic(err)
iterErr = err
return true
}

delReward, err := k.CalculateDelegationRewards(ctx, val, del, endingPeriod)
if err != nil {
panic(err)
iterErr = err
return true
}

delRewards = append(delRewards, types.NewDelegationDelegatorReward(del.GetValidatorAddr(), delReward))
total = total.Add(delReward...)
return false
},
)
if iterErr != nil {
return nil, iterErr
}
if err != nil {
return nil, err
}
Expand Down
14 changes: 7 additions & 7 deletions x/gov/keeper/common_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -31,10 +31,10 @@ import (
)

var (
_, _, addr = testdata.KeyTestPubAddr()
govAcct = authtypes.NewModuleAddress(types.ModuleName)
poolAcct = authtypes.NewModuleAddress(pooltypes.ModuleName)
TestProposal = getTestProposal()
_, _, addr = testdata.KeyTestPubAddr()
govAcct = authtypes.NewModuleAddress(types.ModuleName)
poolAcct = authtypes.NewModuleAddress(pooltypes.ModuleName)
TestProposal, _ = getTestProposal()
)

// mintModuleName duplicates the mint module's name to avoid a cyclic dependency with x/mint.
Expand All @@ -43,16 +43,16 @@ var (
const mintModuleName = "mint"

// getTestProposal creates and returns a test proposal message.
func getTestProposal() []sdk.Msg {
func getTestProposal() ([]sdk.Msg, error) {
legacyProposalMsg, err := v1.NewLegacyContent(v1beta1.NewTextProposal("Title", "description"), authtypes.NewModuleAddress(types.ModuleName).String())
if err != nil {
panic(err)
return nil, err
likhita-809 marked this conversation as resolved.
Show resolved Hide resolved
}

return []sdk.Msg{
banktypes.NewMsgSend(govAcct, addr, sdk.NewCoins(sdk.NewCoin("stake", math.NewInt(1000)))),
legacyProposalMsg,
}
}, nil
}

type mocks struct {
Expand Down
Loading
Loading