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 14 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
1 change: 1 addition & 0 deletions x/gov/keeper/tally_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -393,6 +393,7 @@ func TestTally(t *testing.T) {
}
return nil
})

// Submit and activate a proposal
proposal, err := govKeeper.SubmitProposal(ctx, TestProposal, "", "title", "summary", delAddrs[0], tt.expedited)
require.NoError(t, err)
Expand Down
10 changes: 5 additions & 5 deletions x/slashing/keeper/signing_info.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package keeper
import (
"context"
"errors"
"fmt"
"time"

"github.com/bits-and-blooms/bitset"
Expand All @@ -22,23 +23,22 @@ func (k Keeper) HasValidatorSigningInfo(ctx context.Context, consAddr sdk.ConsAd
}

// JailUntil attempts to set a validator's JailedUntil attribute in its signing
// info. It will panic if the signing info does not exist for the validator.
// info.
func (k Keeper) JailUntil(ctx context.Context, consAddr sdk.ConsAddress, jailTime time.Time) error {
signInfo, err := k.ValidatorSigningInfo.Get(ctx, consAddr)
if err != nil {
likhita-809 marked this conversation as resolved.
Show resolved Hide resolved
return errorsmod.Wrap(err, "cannot jail validator that does not have any signing information")
return errorsmod.Wrap(err, fmt.Sprintf("cannot jail validator with consensus address %s that does not have any signing information", consAddr.String()))
}

signInfo.JailedUntil = jailTime
return k.ValidatorSigningInfo.Set(ctx, consAddr, signInfo)
}

// Tombstone attempts to tombstone a validator. It will panic if signing info for
// the given validator does not exist.
// Tombstone attempts to tombstone a validator.
func (k Keeper) Tombstone(ctx context.Context, consAddr sdk.ConsAddress) error {
signInfo, err := k.ValidatorSigningInfo.Get(ctx, consAddr)
if err != nil {
likhita-809 marked this conversation as resolved.
Show resolved Hide resolved
return types.ErrNoSigningInfoFound.Wrap("cannot tombstone validator that does not have any signing information")
return types.ErrNoSigningInfoFound.Wrap(fmt.Sprintf("cannot tombstone validator with consensus address %s that does not have any signing information", consAddr.String()))
}

if signInfo.Tombstoned {
Expand Down
7 changes: 5 additions & 2 deletions x/staking/keeper/alias_functions.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package keeper

import (
"context"
"fmt"

"cosmossdk.io/collections"
storetypes "cosmossdk.io/store/types"
Expand Down Expand Up @@ -56,8 +57,10 @@ func (k Keeper) IterateBondedValidatorsByPower(ctx context.Context, fn func(inde
i := int64(0)
for ; iterator.Valid() && i < int64(maxValidators); iterator.Next() {
address := iterator.Value()
validator := k.mustGetValidator(ctx, address)

validator, err := k.GetValidator(ctx, address)
if err != nil {
return fmt.Errorf("validator record not found for address: %s", sdk.ValAddress(address).String())
}
if validator.IsBonded() {
stop := fn(i, validator) // XXX is this safe will the validator unexposed fields be able to get written to?
if stop {
Expand Down
Loading
Loading