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

Merged
merged 19 commits into from
Dec 12, 2023
Merged

refactor: remove panic usage in keeper methods #18636

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
e055d72
staking: remove panic usage in keeper methods
likhita-809 Dec 6, 2023
2735e66
fix lint
likhita-809 Dec 6, 2023
cd96e40
slashing: fix godoc about panic in keeper methods
likhita-809 Dec 6, 2023
300d614
gov: add gov changes
likhita-809 Dec 6, 2023
3e68827
distribution: add changes
likhita-809 Dec 6, 2023
ab6990f
bank: add changes
likhita-809 Dec 7, 2023
9b78a2a
Merge branch 'main' of https://github.com/cosmos/cosmos-sdk into likh…
likhita-809 Dec 7, 2023
ed0d131
distr: log error
likhita-809 Dec 7, 2023
0cc87cb
Merge branch 'main' of https://github.com/cosmos/cosmos-sdk into likh…
likhita-809 Dec 7, 2023
513ce12
address julien's comment: return err
likhita-809 Dec 8, 2023
c1ac1d2
Merge branch 'main' of https://github.com/cosmos/cosmos-sdk into likh…
likhita-809 Dec 8, 2023
b7bd994
address comments
likhita-809 Dec 8, 2023
6685fb6
address comment
likhita-809 Dec 11, 2023
e3c6c92
Merge branch 'main' of https://github.com/cosmos/cosmos-sdk into likh…
likhita-809 Dec 11, 2023
e77cbc4
add changelog
likhita-809 Dec 11, 2023
f13271f
fix changelog
likhita-809 Dec 11, 2023
c7ade9c
update changelog to be exhaustive
likhita-809 Dec 11, 2023
0e19476
Merge branch 'main' of https://github.com/cosmos/cosmos-sdk into likh…
likhita-809 Dec 12, 2023
9987ae0
improvements: address bot comments
likhita-809 Dec 12, 2023
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
View file
Open in desktop
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
View file
Open in desktop
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
11 changes: 6 additions & 5 deletions x/distribution/keeper/delegation.go
View file
Open in desktop
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 @@ -130,7 +130,8 @@ func (k Keeper) CalculateDelegationRewards(ctx context.Context, val sdk.Validato
if endingPeriod > startingPeriod {
delRewards, err := k.calculateDelegationRewardsBetween(ctx, val, startingPeriod, endingPeriod, stake)
if err != nil {
panic(err)
k.Logger(ctx).Error(err.Error())
likhita-809 marked this conversation as resolved.
Show resolved Hide resolved
return true
}
rewards = rewards.Add(delRewards...)

Expand Down
12 changes: 8 additions & 4 deletions x/distribution/keeper/grpc_query.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -269,22 +269,26 @@ func (k Querier) DelegationTotalRewards(ctx context.Context, req *types.QueryDel
func(_ int64, del sdk.DelegationI) (stop bool) {
valAddr, err := k.stakingKeeper.ValidatorAddressCodec().StringToBytes(del.GetValidatorAddr())
if err != nil {
panic(err)
k.Logger(ctx).Error(err.Error())
likhita-809 marked this conversation as resolved.
Show resolved Hide resolved
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)
k.Logger(ctx).Error(err.Error())
return true
}

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

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

delRewards = append(delRewards, types.NewDelegationDelegatorReward(del.GetValidatorAddr(), delReward))
Expand Down
14 changes: 7 additions & 7 deletions x/gov/keeper/common_test.go
View file
Open in desktop
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
16 changes: 13 additions & 3 deletions x/gov/keeper/deposit_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import (
"fmt"
"testing"

"github.com/cosmos/gogoproto/proto"
"github.com/stretchr/testify/require"

"cosmossdk.io/collections"
Expand Down Expand Up @@ -53,7 +54,10 @@ func TestDeposits(t *testing.T) {
TestAddrs := simtestutil.AddTestAddrsIncremental(bankKeeper, stakingKeeper, ctx, 2, sdkmath.NewInt(10000000*depositMultiplier))
authKeeper.EXPECT().AddressCodec().Return(address.NewBech32Codec("cosmos")).AnyTimes()

tp := TestProposal
var tp []proto.Message
if TestProposal != nil {
tp = TestProposal
likhita-809 marked this conversation as resolved.
Show resolved Hide resolved
}
proposal, err := govKeeper.SubmitProposal(ctx, tp, "", "title", "summary", TestAddrs[0], tc.expedited)
require.NoError(t, err)
proposalID := proposal.Id
Expand Down Expand Up @@ -225,7 +229,10 @@ func TestDepositAmount(t *testing.T) {
err := govKeeper.Params.Set(ctx, params)
require.NoError(t, err)

tp := TestProposal
var tp []proto.Message
if TestProposal != nil {
tp = TestProposal
}
proposal, err := govKeeper.SubmitProposal(ctx, tp, "", "title", "summary", testAddrs[0], false)
require.NoError(t, err)
proposalID := proposal.Id
Expand Down Expand Up @@ -415,7 +422,10 @@ func TestChargeDeposit(t *testing.T) {
err := govKeeper.Params.Set(ctx, params)
require.NoError(t, err)

tp := TestProposal
var tp []proto.Message
if TestProposal != nil {
tp = TestProposal
}
proposal, err := govKeeper.SubmitProposal(ctx, tp, "", "title", "summary", TestAddrs[0], false)
require.NoError(t, err)
proposalID := proposal.Id
Expand Down
Loading
Loading