From e055d728adf1b45f54e3cfe1e05a144013a32df3 Mon Sep 17 00:00:00 2001 From: likhita-809 Date: Wed, 6 Dec 2023 10:48:59 +0530 Subject: [PATCH 01/14] staking: remove panic usage in keeper methods --- x/staking/keeper/alias_functions.go | 7 +++++-- x/staking/keeper/delegation.go | 15 +++++++++------ x/staking/keeper/keeper_test.go | 4 ++-- x/staking/keeper/slash.go | 16 +++++++++++----- x/staking/keeper/val_state_change.go | 22 ++++++++++++++-------- x/staking/keeper/validator.go | 25 +++++-------------------- 6 files changed, 46 insertions(+), 43 deletions(-) diff --git a/x/staking/keeper/alias_functions.go b/x/staking/keeper/alias_functions.go index 671bc8a7323c..c5dffdddbc60 100644 --- a/x/staking/keeper/alias_functions.go +++ b/x/staking/keeper/alias_functions.go @@ -2,6 +2,7 @@ package keeper import ( "context" + "fmt" "cosmossdk.io/collections" storetypes "cosmossdk.io/store/types" @@ -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: %X", address) + } if validator.IsBonded() { stop := fn(i, validator) // XXX is this safe will the validator unexposed fields be able to get written to? if stop { diff --git a/x/staking/keeper/delegation.go b/x/staking/keeper/delegation.go index f6e6de9f48c1..2fb46c9fa94c 100644 --- a/x/staking/keeper/delegation.go +++ b/x/staking/keeper/delegation.go @@ -219,7 +219,7 @@ func (k Keeper) GetDelegatorBonded(ctx context.Context, delegator sdk.AccAddress err := k.IterateDelegatorDelegations(ctx, delegator, func(delegation types.Delegation) bool { validatorAddr, err := k.validatorAddressCodec.StringToBytes(delegation.ValidatorAddress) if err != nil { - panic(err) // shouldn't happen + return true } validator, err := k.GetValidator(ctx, validatorAddr) if err == nil { @@ -717,7 +717,7 @@ func (k Keeper) Delegate( // all non bonded if subtractAccount { if tokenSrc == types.Bonded { - panic("delegation token source cannot be bonded") + return math.LegacyZeroDec(), fmt.Errorf("delegation token source cannot be bonded") } var sendName string @@ -728,7 +728,7 @@ func (k Keeper) Delegate( case validator.IsUnbonding(), validator.IsUnbonded(): sendName = types.NotBondedPoolName default: - panic("invalid validator status") + return math.LegacyZeroDec(), fmt.Errorf("invalid validator status") } bondDenom, err := k.BondDenom(ctx) @@ -760,7 +760,7 @@ func (k Keeper) Delegate( return math.LegacyDec{}, err } default: - panic("unknown token source bond status") + return math.LegacyZeroDec(), fmt.Errorf("unknown token source bond status") } } @@ -834,7 +834,10 @@ func (k Keeper) Unbond( if err != nil { return amount, err } - validator = k.mustGetValidator(ctx, valbz) + validator, err = k.GetValidator(ctx, valbz) + if err != nil { + return amount, fmt.Errorf("validator record not found for address: %X", valbz) + } } if delegation.Shares.IsZero() { @@ -906,7 +909,7 @@ func (k Keeper) getBeginInfo( return validator.UnbondingTime, validator.UnbondingHeight, false, nil default: - panic(fmt.Sprintf("unknown validator status: %s", validator.Status)) + return completionTime, height, false, fmt.Errorf("unknown validator status: %s", validator.Status) } } diff --git a/x/staking/keeper/keeper_test.go b/x/staking/keeper/keeper_test.go index d1bbe681b131..b13404cabc69 100644 --- a/x/staking/keeper/keeper_test.go +++ b/x/staking/keeper/keeper_test.go @@ -356,7 +356,7 @@ func (s *KeeperTestSuite) TestUnbondingDelegationsMigrationToColls() { }, }, } - bz := stakingtypes.MustMarshalUBD(s.cdc, ubd) + bz := s.cdc.MustMarshal(&ubd) s.ctx.KVStore(s.key).Set(getUBDKey(delAddrs[i], valAddrs[i]), bz) s.ctx.KVStore(s.key).Set(getUBDByValIndexKey(delAddrs[i], valAddrs[i]), []byte{}) }, @@ -444,7 +444,7 @@ func (s *KeeperTestSuite) TestValidatorsMigrationToColls() { Commission: stakingtypes.NewCommission(math.LegacyZeroDec(), math.LegacyZeroDec(), math.LegacyZeroDec()), MinSelfDelegation: math.ZeroInt(), } - valBz := stakingtypes.MustMarshalValidator(s.cdc, &val) + valBz := s.cdc.MustMarshal(&val) // legacy Set method s.ctx.KVStore(s.key).Set(getValidatorKey(valAddrs[i]), valBz) }, diff --git a/x/staking/keeper/slash.go b/x/staking/keeper/slash.go index d6be7b93f0c5..4272dae59acc 100644 --- a/x/staking/keeper/slash.go +++ b/x/staking/keeper/slash.go @@ -57,7 +57,7 @@ func (k Keeper) Slash(ctx context.Context, consAddr sdk.ConsAddress, infractionH // Log the slash attempt for future reference (maybe we should tag it too) conStr, err := k.consensusAddressCodec.BytesToString(consAddr) if err != nil { - panic(err) + return math.NewInt(0), err } logger.Error( @@ -191,7 +191,7 @@ func (k Keeper) Slash(ctx context.Context, consAddr sdk.ConsAddress, infractionH return math.NewInt(0), err } default: - panic("invalid validator status") + return math.NewInt(0), fmt.Errorf("invalid validator status") } logger.Info( @@ -210,7 +210,10 @@ func (k Keeper) SlashWithInfractionReason(ctx context.Context, consAddr sdk.Cons // jail a validator func (k Keeper) Jail(ctx context.Context, consAddr sdk.ConsAddress) error { - validator := k.mustGetValidatorByConsAddr(ctx, consAddr) + validator, err := k.GetValidatorByConsAddr(ctx, consAddr) + if err != nil { + return fmt.Errorf("validator with consensus-Address %s not found", consAddr) + } if err := k.jailValidator(ctx, validator); err != nil { return err } @@ -222,7 +225,10 @@ func (k Keeper) Jail(ctx context.Context, consAddr sdk.ConsAddress) error { // unjail a validator func (k Keeper) Unjail(ctx context.Context, consAddr sdk.ConsAddress) error { - validator := k.mustGetValidatorByConsAddr(ctx, consAddr) + validator, err := k.GetValidatorByConsAddr(ctx, consAddr) + if err != nil { + return fmt.Errorf("validator with consensus-Address %s not found", consAddr) + } if err := k.unjailValidator(ctx, validator); err != nil { return err } @@ -363,7 +369,7 @@ func (k Keeper) SlashRedelegation(ctx context.Context, srcValidator types.Valida case dstValidator.IsUnbonded() || dstValidator.IsUnbonding(): notBondedBurnedAmount = notBondedBurnedAmount.Add(tokensToBurn) default: - panic("unknown validator status") + return math.ZeroInt(), fmt.Errorf("unknown validator status") } } diff --git a/x/staking/keeper/val_state_change.go b/x/staking/keeper/val_state_change.go index 73675b3d4c5c..a70d69a11d49 100644 --- a/x/staking/keeper/val_state_change.go +++ b/x/staking/keeper/val_state_change.go @@ -156,10 +156,13 @@ func (k Keeper) ApplyAndReturnValidatorSetUpdates(ctx context.Context) (updates // everything that is iterated in this loop is becoming or already a // part of the bonded validator set valAddr := sdk.ValAddress(iterator.Value()) - validator := k.mustGetValidator(ctx, valAddr) + validator, err := k.GetValidator(ctx, valAddr) + if err != nil { + return nil, fmt.Errorf("validator record not found for address: %X", valAddr) + } if validator.Jailed { - panic("should never retrieve a jailed validator from the power store") + return nil, fmt.Errorf("should never retrieve a jailed validator from the power store") } // if we get to a zero-power validator (which we don't bond), @@ -185,7 +188,7 @@ func (k Keeper) ApplyAndReturnValidatorSetUpdates(ctx context.Context) (updates case validator.IsBonded(): // no state change default: - panic("unexpected validator status") + return nil, fmt.Errorf("unexpected validator status") } // fetch the old power bytes @@ -218,7 +221,10 @@ func (k Keeper) ApplyAndReturnValidatorSetUpdates(ctx context.Context) (updates } for _, valAddrBytes := range noLongerBonded { - validator := k.mustGetValidator(ctx, sdk.ValAddress(valAddrBytes)) + validator, err := k.GetValidator(ctx, sdk.ValAddress(valAddrBytes)) + if err != nil { + return nil, fmt.Errorf("validator record not found for address: %X", sdk.ValAddress(valAddrBytes)) + } validator, err = k.bondedToUnbonding(ctx, validator) if err != nil { return nil, err @@ -274,7 +280,7 @@ func (k Keeper) ApplyAndReturnValidatorSetUpdates(ctx context.Context) (updates func (k Keeper) bondedToUnbonding(ctx context.Context, validator types.Validator) (types.Validator, error) { if !validator.IsBonded() { - panic(fmt.Sprintf("bad state transition bondedToUnbonding, validator: %v\n", validator)) + return types.Validator{}, fmt.Errorf("bad state transition bondedToUnbonding, validator: %v\n", validator) } return k.BeginUnbondingValidator(ctx, validator) @@ -282,7 +288,7 @@ func (k Keeper) bondedToUnbonding(ctx context.Context, validator types.Validator func (k Keeper) unbondingToBonded(ctx context.Context, validator types.Validator) (types.Validator, error) { if !validator.IsUnbonding() { - panic(fmt.Sprintf("bad state transition unbondingToBonded, validator: %v\n", validator)) + return types.Validator{}, fmt.Errorf("bad state transition unbondingToBonded, validator: %v\n", validator) } return k.bondValidator(ctx, validator) @@ -290,7 +296,7 @@ func (k Keeper) unbondingToBonded(ctx context.Context, validator types.Validator func (k Keeper) unbondedToBonded(ctx context.Context, validator types.Validator) (types.Validator, error) { if !validator.IsUnbonded() { - panic(fmt.Sprintf("bad state transition unbondedToBonded, validator: %v\n", validator)) + return types.Validator{}, fmt.Errorf("bad state transition unbondedToBonded, validator: %v\n", validator) } return k.bondValidator(ctx, validator) @@ -388,7 +394,7 @@ func (k Keeper) BeginUnbondingValidator(ctx context.Context, validator types.Val // sanity check if validator.Status != types.Bonded { - panic(fmt.Sprintf("should not already be unbonded or unbonding, validator: %v\n", validator)) + return validator, fmt.Errorf("should not already be unbonded or unbonding, validator: %v\n", validator) } id, err := k.IncrementUnbondingID(ctx) diff --git a/x/staking/keeper/validator.go b/x/staking/keeper/validator.go index 6b6eef790975..b95dd87a7742 100644 --- a/x/staking/keeper/validator.go +++ b/x/staking/keeper/validator.go @@ -34,15 +34,6 @@ func (k Keeper) GetValidator(ctx context.Context, addr sdk.ValAddress) (validato return validator, nil } -func (k Keeper) mustGetValidator(ctx context.Context, addr sdk.ValAddress) types.Validator { - validator, err := k.GetValidator(ctx, addr) - if err != nil { - panic(fmt.Sprintf("validator record not found for address: %X\n", addr)) - } - - return validator -} - // GetValidatorByConsAddr gets a single validator by consensus address func (k Keeper) GetValidatorByConsAddr(ctx context.Context, consAddr sdk.ConsAddress) (validator types.Validator, err error) { opAddr, err := k.ValidatorByConsensusAddress.Get(ctx, consAddr) @@ -57,15 +48,6 @@ func (k Keeper) GetValidatorByConsAddr(ctx context.Context, consAddr sdk.ConsAdd return k.GetValidator(ctx, opAddr) } -func (k Keeper) mustGetValidatorByConsAddr(ctx context.Context, consAddr sdk.ConsAddress) types.Validator { - validator, err := k.GetValidatorByConsAddr(ctx, consAddr) - if err != nil { - panic(fmt.Errorf("validator with consensus-Address %s not found", consAddr)) - } - - return validator -} - // SetValidator sets the main record holding validator details func (k Keeper) SetValidator(ctx context.Context, validator types.Validator) error { valBz, err := k.ValidatorAddressCodec().StringToBytes(validator.GetOperator()) @@ -317,7 +299,10 @@ func (k Keeper) GetBondedValidatorsByPower(ctx context.Context) ([]types.Validat i := 0 for ; iterator.Valid() && i < int(maxValidators); iterator.Next() { address := iterator.Value() - validator := k.mustGetValidator(ctx, address) + validator, err := k.GetValidator(ctx, address) + if err != nil { + return nil, fmt.Errorf("validator record not found for address: %X", address) + } if validator.IsBonded() { validators[i] = validator @@ -383,7 +368,7 @@ func (k Keeper) GetLastValidators(ctx context.Context) (validators []types.Valid err = k.LastValidatorPower.Walk(ctx, nil, func(key []byte, _ gogotypes.Int64Value) (bool, error) { // sanity check if i >= int(maxValidators) { - panic("more validators than maxValidators found") + return true, fmt.Errorf("more validators than maxValidators found") } validator, err := k.GetValidator(ctx, key) From 2735e66433f16430cad6bf32ad3a6ad4199513b1 Mon Sep 17 00:00:00 2001 From: likhita-809 Date: Wed, 6 Dec 2023 10:54:35 +0530 Subject: [PATCH 02/14] fix lint --- x/staking/keeper/val_state_change.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/x/staking/keeper/val_state_change.go b/x/staking/keeper/val_state_change.go index a70d69a11d49..3d09937c1ef4 100644 --- a/x/staking/keeper/val_state_change.go +++ b/x/staking/keeper/val_state_change.go @@ -280,7 +280,7 @@ func (k Keeper) ApplyAndReturnValidatorSetUpdates(ctx context.Context) (updates func (k Keeper) bondedToUnbonding(ctx context.Context, validator types.Validator) (types.Validator, error) { if !validator.IsBonded() { - return types.Validator{}, fmt.Errorf("bad state transition bondedToUnbonding, validator: %v\n", validator) + return types.Validator{}, fmt.Errorf("bad state transition bondedToUnbonding, validator: %v", validator) } return k.BeginUnbondingValidator(ctx, validator) @@ -288,7 +288,7 @@ func (k Keeper) bondedToUnbonding(ctx context.Context, validator types.Validator func (k Keeper) unbondingToBonded(ctx context.Context, validator types.Validator) (types.Validator, error) { if !validator.IsUnbonding() { - return types.Validator{}, fmt.Errorf("bad state transition unbondingToBonded, validator: %v\n", validator) + return types.Validator{}, fmt.Errorf("bad state transition unbondingToBonded, validator: %v", validator) } return k.bondValidator(ctx, validator) @@ -296,7 +296,7 @@ func (k Keeper) unbondingToBonded(ctx context.Context, validator types.Validator func (k Keeper) unbondedToBonded(ctx context.Context, validator types.Validator) (types.Validator, error) { if !validator.IsUnbonded() { - return types.Validator{}, fmt.Errorf("bad state transition unbondedToBonded, validator: %v\n", validator) + return types.Validator{}, fmt.Errorf("bad state transition unbondedToBonded, validator: %v", validator) } return k.bondValidator(ctx, validator) @@ -394,7 +394,7 @@ func (k Keeper) BeginUnbondingValidator(ctx context.Context, validator types.Val // sanity check if validator.Status != types.Bonded { - return validator, fmt.Errorf("should not already be unbonded or unbonding, validator: %v\n", validator) + return validator, fmt.Errorf("should not already be unbonded or unbonding, validator: %v", validator) } id, err := k.IncrementUnbondingID(ctx) From cd96e4065d2b39b5b1aac6d86ed242c25a21fe7a Mon Sep 17 00:00:00 2001 From: likhita-809 Date: Wed, 6 Dec 2023 11:12:16 +0530 Subject: [PATCH 03/14] slashing: fix godoc about panic in keeper methods --- x/slashing/keeper/signing_info.go | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/x/slashing/keeper/signing_info.go b/x/slashing/keeper/signing_info.go index 80ebc6550305..c68a9a9a982b 100644 --- a/x/slashing/keeper/signing_info.go +++ b/x/slashing/keeper/signing_info.go @@ -22,7 +22,7 @@ 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 { @@ -33,8 +33,7 @@ func (k Keeper) JailUntil(ctx context.Context, consAddr sdk.ConsAddress, jailTim 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 { From 300d6140d9441ee8b2886a9b1146909e24797d7c Mon Sep 17 00:00:00 2001 From: likhita-809 Date: Wed, 6 Dec 2023 11:39:05 +0530 Subject: [PATCH 04/14] gov: add gov changes --- x/gov/keeper/common_test.go | 14 ++++----- x/gov/keeper/deposit_test.go | 16 +++++++++-- x/gov/keeper/grpc_query_test.go | 51 +++++++++++++++++++++++++++------ x/gov/keeper/hooks_test.go | 6 +++- x/gov/keeper/keeper_test.go | 11 +++++-- x/gov/keeper/msg_server_test.go | 9 +++++- x/gov/keeper/proposal_test.go | 21 +++++++++++--- x/gov/keeper/tally_test.go | 8 +++++- x/gov/keeper/vote_test.go | 6 +++- 9 files changed, 113 insertions(+), 29 deletions(-) diff --git a/x/gov/keeper/common_test.go b/x/gov/keeper/common_test.go index 1e3a55ffda5b..fa22c871a988 100644 --- a/x/gov/keeper/common_test.go +++ b/x/gov/keeper/common_test.go @@ -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. @@ -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 } return []sdk.Msg{ banktypes.NewMsgSend(govAcct, addr, sdk.NewCoins(sdk.NewCoin("stake", math.NewInt(1000)))), legacyProposalMsg, - } + }, nil } type mocks struct { diff --git a/x/gov/keeper/deposit_test.go b/x/gov/keeper/deposit_test.go index 3def399e811b..bef9462446cb 100644 --- a/x/gov/keeper/deposit_test.go +++ b/x/gov/keeper/deposit_test.go @@ -4,6 +4,7 @@ import ( "fmt" "testing" + "github.com/cosmos/gogoproto/proto" "github.com/stretchr/testify/require" "cosmossdk.io/collections" @@ -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 + } proposal, err := govKeeper.SubmitProposal(ctx, tp, "", "title", "summary", TestAddrs[0], tc.expedited) require.NoError(t, err) proposalID := proposal.Id @@ -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 @@ -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 diff --git a/x/gov/keeper/grpc_query_test.go b/x/gov/keeper/grpc_query_test.go index efc6c63d5395..4d9bf9c214cb 100644 --- a/x/gov/keeper/grpc_query_test.go +++ b/x/gov/keeper/grpc_query_test.go @@ -5,6 +5,8 @@ import ( "fmt" "time" + "github.com/cosmos/gogoproto/proto" + "cosmossdk.io/math" v3 "cosmossdk.io/x/gov/migrations/v3" v1 "cosmossdk.io/x/gov/types/v1" @@ -443,6 +445,10 @@ func (suite *KeeperTestSuite) TestLegacyGRPCQueryProposals() { func (suite *KeeperTestSuite) TestGRPCQueryVote() { ctx, queryClient, addrs := suite.ctx, suite.queryClient, suite.addrs + var tp []proto.Message + if TestProposal != nil { + tp = TestProposal + } var ( req *v1.QueryVoteRequest @@ -496,7 +502,7 @@ func (suite *KeeperTestSuite) TestGRPCQueryVote() { "no votes present", func() { var err error - proposal, err = suite.govKeeper.SubmitProposal(ctx, TestProposal, "", "title", "summary", addrs[0], false) + proposal, err = suite.govKeeper.SubmitProposal(ctx, tp, "", "title", "summary", addrs[0], false) suite.Require().NoError(err) req = &v1.QueryVoteRequest{ @@ -558,7 +564,10 @@ func (suite *KeeperTestSuite) TestGRPCQueryVote() { func (suite *KeeperTestSuite) TestLegacyGRPCQueryVote() { ctx, queryClient, addrs := suite.ctx, suite.legacyQueryClient, suite.addrs - + var tp []proto.Message + if TestProposal != nil { + tp = TestProposal + } var ( req *v1beta1.QueryVoteRequest expRes *v1beta1.QueryVoteResponse @@ -611,7 +620,7 @@ func (suite *KeeperTestSuite) TestLegacyGRPCQueryVote() { "no votes present", func() { var err error - proposal, err = suite.govKeeper.SubmitProposal(ctx, TestProposal, "", "title", "summary", addrs[0], false) + proposal, err = suite.govKeeper.SubmitProposal(ctx, tp, "", "title", "summary", addrs[0], false) suite.Require().NoError(err) req = &v1beta1.QueryVoteRequest{ @@ -674,6 +683,10 @@ func (suite *KeeperTestSuite) TestLegacyGRPCQueryVote() { func (suite *KeeperTestSuite) TestGRPCQueryVotes() { suite.reset() ctx, queryClient := suite.ctx, suite.queryClient + var tp []proto.Message + if TestProposal != nil { + tp = TestProposal + } addrs := simtestutil.AddTestAddrsIncremental(suite.bankKeeper, suite.stakingKeeper, ctx, 2, math.NewInt(30000000)) @@ -718,7 +731,7 @@ func (suite *KeeperTestSuite) TestGRPCQueryVotes() { "create a proposal and get votes", func() { var err error - proposal, err = suite.govKeeper.SubmitProposal(ctx, TestProposal, "", "title", "summary", addrs[0], false) + proposal, err = suite.govKeeper.SubmitProposal(ctx, tp, "", "title", "summary", addrs[0], false) suite.Require().NoError(err) req = &v1.QueryVotesRequest{ @@ -778,6 +791,10 @@ func (suite *KeeperTestSuite) TestGRPCQueryVotes() { func (suite *KeeperTestSuite) TestLegacyGRPCQueryVotes() { suite.reset() ctx, queryClient := suite.ctx, suite.legacyQueryClient + var tp []proto.Message + if TestProposal != nil { + tp = TestProposal + } addrs := simtestutil.AddTestAddrsIncremental(suite.bankKeeper, suite.stakingKeeper, ctx, 2, math.NewInt(30000000)) @@ -822,7 +839,7 @@ func (suite *KeeperTestSuite) TestLegacyGRPCQueryVotes() { "create a proposal and get votes", func() { var err error - proposal, err = suite.govKeeper.SubmitProposal(ctx, TestProposal, "", "title", "summary", addrs[0], false) + proposal, err = suite.govKeeper.SubmitProposal(ctx, tp, "", "title", "summary", addrs[0], false) suite.Require().NoError(err) req = &v1beta1.QueryVotesRequest{ @@ -1057,6 +1074,10 @@ func (suite *KeeperTestSuite) TestLegacyGRPCQueryParams() { func (suite *KeeperTestSuite) TestGRPCQueryDeposit() { suite.reset() ctx, queryClient, addrs := suite.ctx, suite.queryClient, suite.addrs + var tp []proto.Message + if TestProposal != nil { + tp = TestProposal + } var ( req *v1.QueryDepositRequest @@ -1110,7 +1131,7 @@ func (suite *KeeperTestSuite) TestGRPCQueryDeposit() { "no deposits proposal", func() { var err error - proposal, err = suite.govKeeper.SubmitProposal(ctx, TestProposal, "", "title", "summary", addrs[0], false) + proposal, err = suite.govKeeper.SubmitProposal(ctx, tp, "", "title", "summary", addrs[0], false) suite.Require().NoError(err) suite.Require().NotNil(proposal) @@ -1159,6 +1180,10 @@ func (suite *KeeperTestSuite) TestGRPCQueryDeposit() { func (suite *KeeperTestSuite) TestLegacyGRPCQueryDeposit() { ctx, queryClient, addrs := suite.ctx, suite.legacyQueryClient, suite.addrs + var tp []proto.Message + if TestProposal != nil { + tp = TestProposal + } var ( req *v1beta1.QueryDepositRequest @@ -1212,7 +1237,7 @@ func (suite *KeeperTestSuite) TestLegacyGRPCQueryDeposit() { "no deposits proposal", func() { var err error - proposal, err = suite.govKeeper.SubmitProposal(ctx, TestProposal, "", "title", "summary", addrs[0], false) + proposal, err = suite.govKeeper.SubmitProposal(ctx, tp, "", "title", "summary", addrs[0], false) suite.Require().NoError(err) suite.Require().NotNil(proposal) @@ -1262,6 +1287,10 @@ func (suite *KeeperTestSuite) TestLegacyGRPCQueryDeposit() { func (suite *KeeperTestSuite) TestGRPCQueryDeposits() { ctx, queryClient, addrs := suite.ctx, suite.queryClient, suite.addrs + var tp []proto.Message + if TestProposal != nil { + tp = TestProposal + } var ( req *v1.QueryDepositsRequest @@ -1303,7 +1332,7 @@ func (suite *KeeperTestSuite) TestGRPCQueryDeposits() { "create a proposal and get deposits", func() { var err error - proposal, err = suite.govKeeper.SubmitProposal(ctx, TestProposal, "", "title", "summary", addrs[0], true) + proposal, err = suite.govKeeper.SubmitProposal(ctx, tp, "", "title", "summary", addrs[0], true) suite.Require().NoError(err) req = &v1.QueryDepositsRequest{ @@ -1359,6 +1388,10 @@ func (suite *KeeperTestSuite) TestGRPCQueryDeposits() { func (suite *KeeperTestSuite) TestLegacyGRPCQueryDeposits() { suite.reset() ctx, queryClient, addrs := suite.ctx, suite.legacyQueryClient, suite.addrs + var tp []proto.Message + if TestProposal != nil { + tp = TestProposal + } var ( req *v1beta1.QueryDepositsRequest @@ -1400,7 +1433,7 @@ func (suite *KeeperTestSuite) TestLegacyGRPCQueryDeposits() { "create a proposal and get deposits", func() { var err error - proposal, err = suite.govKeeper.SubmitProposal(ctx, TestProposal, "", "title", "summary", addrs[0], false) + proposal, err = suite.govKeeper.SubmitProposal(ctx, tp, "", "title", "summary", addrs[0], false) suite.Require().NoError(err) req = &v1beta1.QueryDepositsRequest{ diff --git a/x/gov/keeper/hooks_test.go b/x/gov/keeper/hooks_test.go index 1b7fb616f0fd..bd7dec714406 100644 --- a/x/gov/keeper/hooks_test.go +++ b/x/gov/keeper/hooks_test.go @@ -5,6 +5,7 @@ import ( "testing" "time" + "github.com/cosmos/gogoproto/proto" "github.com/stretchr/testify/require" "cosmossdk.io/x/gov" @@ -74,7 +75,10 @@ func TestHooks(t *testing.T) { require.False(t, govHooksReceiver.AfterProposalFailedMinDepositValid) require.False(t, govHooksReceiver.AfterProposalVotingPeriodEndedValid) - tp := TestProposal + var tp []proto.Message + if TestProposal != nil { + tp = TestProposal + } _, err := govKeeper.SubmitProposal(ctx, tp, "", "test", "summary", sdk.AccAddress("cosmos1ghekyjucln7y67ntx7cf27m9dpuxxemn4c8g4r"), false) require.NoError(t, err) require.True(t, govHooksReceiver.AfterProposalSubmissionValid) diff --git a/x/gov/keeper/keeper_test.go b/x/gov/keeper/keeper_test.go index da7f83e62d85..a66e33761cc0 100644 --- a/x/gov/keeper/keeper_test.go +++ b/x/gov/keeper/keeper_test.go @@ -3,6 +3,7 @@ package keeper_test import ( "testing" + "github.com/cosmos/gogoproto/proto" "github.com/stretchr/testify/require" "github.com/stretchr/testify/suite" @@ -88,7 +89,10 @@ func TestIncrementProposalNumber(t *testing.T) { addrBz, err := ac.StringToBytes(address1) require.NoError(t, err) - tp := TestProposal + var tp []proto.Message + if TestProposal != nil { + tp = TestProposal + } _, err = govKeeper.SubmitProposal(ctx, tp, "", "test", "summary", addrBz, false) require.NoError(t, err) _, err = govKeeper.SubmitProposal(ctx, tp, "", "test", "summary", addrBz, false) @@ -115,7 +119,10 @@ func TestProposalQueues(t *testing.T) { authKeeper.EXPECT().AddressCodec().Return(address.NewBech32Codec("cosmos")).AnyTimes() // create test proposals - tp := TestProposal + var tp []proto.Message + if TestProposal != nil { + tp = TestProposal + } proposal, err := govKeeper.SubmitProposal(ctx, tp, "", "test", "summary", addrBz, false) require.NoError(t, err) diff --git a/x/gov/keeper/msg_server_test.go b/x/gov/keeper/msg_server_test.go index 0c8be118bc42..bddd682303a9 100644 --- a/x/gov/keeper/msg_server_test.go +++ b/x/gov/keeper/msg_server_test.go @@ -4,6 +4,8 @@ import ( "strings" "time" + "github.com/cosmos/gogoproto/proto" + sdkmath "cosmossdk.io/math" banktypes "cosmossdk.io/x/bank/types" v1 "cosmossdk.io/x/gov/types/v1" @@ -1781,7 +1783,12 @@ func (suite *KeeperTestSuite) TestSubmitProposal_InitialDeposit() { err := govKeeper.Params.Set(ctx, params) suite.Require().NoError(err) - msg, err := v1.NewMsgSubmitProposal(TestProposal, tc.initialDeposit, address.String(), "test", "Proposal", "description of proposal", false) + var tp []proto.Message + if TestProposal != nil { + tp = TestProposal + } + + msg, err := v1.NewMsgSubmitProposal(tp, tc.initialDeposit, address.String(), "test", "Proposal", "description of proposal", false) suite.Require().NoError(err) // System under test diff --git a/x/gov/keeper/proposal_test.go b/x/gov/keeper/proposal_test.go index 27cfeeaf55cc..a3314be70e9a 100644 --- a/x/gov/keeper/proposal_test.go +++ b/x/gov/keeper/proposal_test.go @@ -6,6 +6,7 @@ import ( "strings" "testing" + "github.com/cosmos/gogoproto/proto" "github.com/stretchr/testify/require" "cosmossdk.io/collections" @@ -29,7 +30,10 @@ func (suite *KeeperTestSuite) TestGetSetProposal() { } for _, tc := range testCases { - tp := TestProposal + var tp []proto.Message + if TestProposal != nil { + tp = TestProposal + } proposal, err := suite.govKeeper.SubmitProposal(suite.ctx, tp, "", "test", "summary", suite.addrs[0], tc.expedited) suite.Require().NoError(err) proposalID := proposal.Id @@ -57,7 +61,10 @@ func (suite *KeeperTestSuite) TestDeleteProposal() { // delete non-existing proposal suite.Require().ErrorIs(suite.govKeeper.DeleteProposal(suite.ctx, 10), collections.ErrNotFound) - tp := TestProposal + var tp []proto.Message + if TestProposal != nil { + tp = TestProposal + } proposal, err := suite.govKeeper.SubmitProposal(suite.ctx, tp, "", "test", "summary", suite.addrs[0], tc.expedited) suite.Require().NoError(err) proposalID := proposal.Id @@ -81,7 +88,10 @@ func (suite *KeeperTestSuite) TestActivateVotingPeriod() { } for _, tc := range testCases { - tp := TestProposal + var tp []proto.Message + if TestProposal != nil { + tp = TestProposal + } proposal, err := suite.govKeeper.SubmitProposal(suite.ctx, tp, "", "test", "summary", suite.addrs[0], tc.expedited) suite.Require().NoError(err) @@ -112,7 +122,10 @@ func (suite *KeeperTestSuite) TestDeleteProposalInVotingPeriod() { for _, tc := range testCases { suite.reset() - tp := TestProposal + var tp []proto.Message + if TestProposal != nil { + tp = TestProposal + } proposal, err := suite.govKeeper.SubmitProposal(suite.ctx, tp, "", "test", "summary", suite.addrs[0], tc.expedited) suite.Require().NoError(err) suite.Require().Nil(proposal.VotingStartTime) diff --git a/x/gov/keeper/tally_test.go b/x/gov/keeper/tally_test.go index a3df191673ce..58deadafb589 100644 --- a/x/gov/keeper/tally_test.go +++ b/x/gov/keeper/tally_test.go @@ -4,6 +4,7 @@ import ( "context" "testing" + "github.com/cosmos/gogoproto/proto" "github.com/golang/mock/gomock" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -393,8 +394,13 @@ func TestTally(t *testing.T) { } return nil }) + var tp []proto.Message + if TestProposal != nil { + tp = TestProposal + } + // Submit and activate a proposal - proposal, err := govKeeper.SubmitProposal(ctx, TestProposal, "", "title", "summary", delAddrs[0], tt.expedited) + proposal, err := govKeeper.SubmitProposal(ctx, tp, "", "title", "summary", delAddrs[0], tt.expedited) require.NoError(t, err) err = govKeeper.ActivateVotingPeriod(ctx, proposal) require.NoError(t, err) diff --git a/x/gov/keeper/vote_test.go b/x/gov/keeper/vote_test.go index e5fcd96e46bd..f6660f96729c 100644 --- a/x/gov/keeper/vote_test.go +++ b/x/gov/keeper/vote_test.go @@ -3,6 +3,7 @@ package keeper_test import ( "testing" + "github.com/cosmos/gogoproto/proto" "github.com/stretchr/testify/require" "cosmossdk.io/collections" @@ -20,7 +21,10 @@ func TestVotes(t *testing.T) { addrs := simtestutil.AddTestAddrsIncremental(bankKeeper, stakingKeeper, ctx, 2, sdkmath.NewInt(10000000)) authKeeper.EXPECT().AddressCodec().Return(address.NewBech32Codec("cosmos")).AnyTimes() - tp := TestProposal + var tp []proto.Message + if TestProposal != nil { + tp = TestProposal + } proposal, err := govKeeper.SubmitProposal(ctx, tp, "", "title", "description", sdk.AccAddress("cosmos1ghekyjucln7y67ntx7cf27m9dpuxxemn4c8g4r"), false) require.NoError(t, err) proposalID := proposal.Id From 3e68827e7bbbe98468aba2e7b53d86e0fbbab49b Mon Sep 17 00:00:00 2001 From: likhita-809 Date: Wed, 6 Dec 2023 12:14:48 +0530 Subject: [PATCH 05/14] distribution: add changes --- x/distribution/keeper/delegation.go | 10 +++++----- x/distribution/keeper/grpc_query.go | 8 ++++---- 2 files changed, 9 insertions(+), 9 deletions(-) diff --git a/x/distribution/keeper/delegation.go b/x/distribution/keeper/delegation.go index 8b9008a1e19f..b798c95d0c53 100644 --- a/x/distribution/keeper/delegation.go +++ b/x/distribution/keeper/delegation.go @@ -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) @@ -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) @@ -130,7 +130,7 @@ 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) + return true } rewards = rewards.Add(delRewards...) diff --git a/x/distribution/keeper/grpc_query.go b/x/distribution/keeper/grpc_query.go index 48a66ba21fdc..59c13b4b3ac6 100644 --- a/x/distribution/keeper/grpc_query.go +++ b/x/distribution/keeper/grpc_query.go @@ -269,22 +269,22 @@ 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) + return true } val, err := k.stakingKeeper.Validator(ctx, valAddr) if err != nil { - panic(err) + return true } endingPeriod, err := k.IncrementValidatorPeriod(ctx, val) if err != nil { - panic(err) + return true } delReward, err := k.CalculateDelegationRewards(ctx, val, del, endingPeriod) if err != nil { - panic(err) + return true } delRewards = append(delRewards, types.NewDelegationDelegatorReward(del.GetValidatorAddr(), delReward)) From ab6990f5ecb14b0f70a4e6a39dae74fea1690c07 Mon Sep 17 00:00:00 2001 From: likhita-809 Date: Thu, 7 Dec 2023 14:34:31 +0530 Subject: [PATCH 06/14] bank: add changes --- x/bank/keeper/keeper.go | 34 +++++++++++++++--------------- x/bank/keeper/keeper_test.go | 41 ++++++++++++++++++------------------ 2 files changed, 37 insertions(+), 38 deletions(-) diff --git a/x/bank/keeper/keeper.go b/x/bank/keeper/keeper.go index e3bafc0a1ad1..e8e2329ac546 100644 --- a/x/bank/keeper/keeper.go +++ b/x/bank/keeper/keeper.go @@ -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) { @@ -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) @@ -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) @@ -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 { diff --git a/x/bank/keeper/keeper_test.go b/x/bank/keeper/keeper_test.go index 5a25feb7d26c..4aa3b503f08e 100644 --- a/x/bank/keeper/keeper_test.go +++ b/x/bank/keeper/keeper_test.go @@ -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) @@ -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) @@ -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)) From ed0d131277e0ab4cfbd853b14a48751fed4832dd Mon Sep 17 00:00:00 2001 From: likhita-809 Date: Thu, 7 Dec 2023 15:42:34 +0530 Subject: [PATCH 07/14] distr: log error --- x/distribution/keeper/delegation.go | 1 + x/distribution/keeper/grpc_query.go | 4 ++++ 2 files changed, 5 insertions(+) diff --git a/x/distribution/keeper/delegation.go b/x/distribution/keeper/delegation.go index b798c95d0c53..b0957c33c099 100644 --- a/x/distribution/keeper/delegation.go +++ b/x/distribution/keeper/delegation.go @@ -130,6 +130,7 @@ 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 { + k.Logger(ctx).Error(err.Error()) return true } rewards = rewards.Add(delRewards...) diff --git a/x/distribution/keeper/grpc_query.go b/x/distribution/keeper/grpc_query.go index 59c13b4b3ac6..5096f4ed8294 100644 --- a/x/distribution/keeper/grpc_query.go +++ b/x/distribution/keeper/grpc_query.go @@ -269,21 +269,25 @@ 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 { + k.Logger(ctx).Error(err.Error()) return true } val, err := k.stakingKeeper.Validator(ctx, valAddr) if err != nil { + k.Logger(ctx).Error(err.Error()) return true } endingPeriod, err := k.IncrementValidatorPeriod(ctx, val) if err != nil { + k.Logger(ctx).Error(err.Error()) return true } delReward, err := k.CalculateDelegationRewards(ctx, val, del, endingPeriod) if err != nil { + k.Logger(ctx).Error(err.Error()) return true } From 513ce12129c924afe9be9c510ebdb1edbee924f0 Mon Sep 17 00:00:00 2001 From: likhita-809 Date: Fri, 8 Dec 2023 14:02:25 +0530 Subject: [PATCH 08/14] address julien's comment: return err --- x/distribution/keeper/delegation.go | 6 +++++- x/distribution/keeper/grpc_query.go | 12 ++++++++---- 2 files changed, 13 insertions(+), 5 deletions(-) diff --git a/x/distribution/keeper/delegation.go b/x/distribution/keeper/delegation.go index b0957c33c099..e97a694a67f7 100644 --- a/x/distribution/keeper/delegation.go +++ b/x/distribution/keeper/delegation.go @@ -123,6 +123,7 @@ 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) { @@ -130,7 +131,7 @@ 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 { - k.Logger(ctx).Error(err.Error()) + iterErr = err return true } rewards = rewards.Add(delRewards...) @@ -143,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 } diff --git a/x/distribution/keeper/grpc_query.go b/x/distribution/keeper/grpc_query.go index 5096f4ed8294..16039f340f0f 100644 --- a/x/distribution/keeper/grpc_query.go +++ b/x/distribution/keeper/grpc_query.go @@ -264,30 +264,31 @@ 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 { - k.Logger(ctx).Error(err.Error()) + iterErr = err return true } val, err := k.stakingKeeper.Validator(ctx, valAddr) if err != nil { - k.Logger(ctx).Error(err.Error()) + iterErr = err return true } endingPeriod, err := k.IncrementValidatorPeriod(ctx, val) if err != nil { - k.Logger(ctx).Error(err.Error()) + iterErr = err return true } delReward, err := k.CalculateDelegationRewards(ctx, val, del, endingPeriod) if err != nil { - k.Logger(ctx).Error(err.Error()) + iterErr = err return true } @@ -296,6 +297,9 @@ func (k Querier) DelegationTotalRewards(ctx context.Context, req *types.QueryDel return false }, ) + if iterErr != nil { + return nil, iterErr + } if err != nil { return nil, err } From b7bd9948d584785bf102c84a07f272f1a67ecfb7 Mon Sep 17 00:00:00 2001 From: likhita-809 Date: Fri, 8 Dec 2023 14:45:34 +0530 Subject: [PATCH 09/14] address comments --- x/distribution/keeper/delegation.go | 4 ++-- x/slashing/keeper/signing_info.go | 5 +++-- x/staking/keeper/alias_functions.go | 2 +- x/staking/keeper/delegation.go | 13 +++++++++---- 4 files changed, 15 insertions(+), 9 deletions(-) diff --git a/x/distribution/keeper/delegation.go b/x/distribution/keeper/delegation.go index e97a694a67f7..36482b407b68 100644 --- a/x/distribution/keeper/delegation.go +++ b/x/distribution/keeper/delegation.go @@ -183,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) } } diff --git a/x/slashing/keeper/signing_info.go b/x/slashing/keeper/signing_info.go index c68a9a9a982b..51e258fea960 100644 --- a/x/slashing/keeper/signing_info.go +++ b/x/slashing/keeper/signing_info.go @@ -3,6 +3,7 @@ package keeper import ( "context" "errors" + "fmt" "time" "github.com/bits-and-blooms/bitset" @@ -26,7 +27,7 @@ func (k Keeper) HasValidatorSigningInfo(ctx context.Context, consAddr sdk.ConsAd func (k Keeper) JailUntil(ctx context.Context, consAddr sdk.ConsAddress, jailTime time.Time) error { signInfo, err := k.ValidatorSigningInfo.Get(ctx, consAddr) if err != nil { - 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 @@ -37,7 +38,7 @@ func (k Keeper) JailUntil(ctx context.Context, consAddr sdk.ConsAddress, jailTim func (k Keeper) Tombstone(ctx context.Context, consAddr sdk.ConsAddress) error { signInfo, err := k.ValidatorSigningInfo.Get(ctx, consAddr) if err != nil { - 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 { diff --git a/x/staking/keeper/alias_functions.go b/x/staking/keeper/alias_functions.go index c5dffdddbc60..fde1c5254e1e 100644 --- a/x/staking/keeper/alias_functions.go +++ b/x/staking/keeper/alias_functions.go @@ -59,7 +59,7 @@ func (k Keeper) IterateBondedValidatorsByPower(ctx context.Context, fn func(inde address := iterator.Value() validator, err := k.GetValidator(ctx, address) if err != nil { - return fmt.Errorf("validator record not found for address: %X", address) + 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? diff --git a/x/staking/keeper/delegation.go b/x/staking/keeper/delegation.go index 2fb46c9fa94c..5f8aab0fd9f0 100644 --- a/x/staking/keeper/delegation.go +++ b/x/staking/keeper/delegation.go @@ -216,9 +216,11 @@ func (k Keeper) GetDelegatorUnbonding(ctx context.Context, delegator sdk.AccAddr func (k Keeper) GetDelegatorBonded(ctx context.Context, delegator sdk.AccAddress) (math.Int, error) { bonded := math.LegacyZeroDec() + var iterErr error err := k.IterateDelegatorDelegations(ctx, delegator, func(delegation types.Delegation) bool { validatorAddr, err := k.validatorAddressCodec.StringToBytes(delegation.ValidatorAddress) if err != nil { + iterErr = err return true } validator, err := k.GetValidator(ctx, validatorAddr) @@ -229,6 +231,9 @@ func (k Keeper) GetDelegatorBonded(ctx context.Context, delegator sdk.AccAddress } return false }) + if iterErr != nil { + return bonded.RoundInt(), iterErr + } return bonded.RoundInt(), err } @@ -728,7 +733,7 @@ func (k Keeper) Delegate( case validator.IsUnbonding(), validator.IsUnbonded(): sendName = types.NotBondedPoolName default: - return math.LegacyZeroDec(), fmt.Errorf("invalid validator status") + return math.LegacyZeroDec(), fmt.Errorf("invalid validator status: %v", validator.Status) } bondDenom, err := k.BondDenom(ctx) @@ -760,7 +765,7 @@ func (k Keeper) Delegate( return math.LegacyDec{}, err } default: - return math.LegacyZeroDec(), fmt.Errorf("unknown token source bond status") + return math.LegacyZeroDec(), fmt.Errorf("unknown token source bond status: %v", tokenSrc) } } @@ -832,7 +837,7 @@ func (k Keeper) Unbond( validator.TokensFromShares(delegation.Shares).TruncateInt().LT(validator.MinSelfDelegation) { err = k.jailValidator(ctx, validator) if err != nil { - return amount, err + return amount, fmt.Errorf("failed to get updated validator after jailing: %v", err) } validator, err = k.GetValidator(ctx, valbz) if err != nil { @@ -909,7 +914,7 @@ func (k Keeper) getBeginInfo( return validator.UnbondingTime, validator.UnbondingHeight, false, nil default: - return completionTime, height, false, fmt.Errorf("unknown validator status: %s", validator.Status) + return completionTime, height, false, fmt.Errorf("unknown validator status: %v", validator.Status) } } From 6685fb6106af8e0d750a2b3a5e95a9fce9e63aac Mon Sep 17 00:00:00 2001 From: likhita-809 Date: Mon, 11 Dec 2023 10:55:56 +0530 Subject: [PATCH 10/14] address comment --- x/gov/keeper/common_test.go | 14 ++++----- x/gov/keeper/deposit_test.go | 16 ++--------- x/gov/keeper/grpc_query_test.go | 51 ++++++--------------------------- x/gov/keeper/hooks_test.go | 6 +--- x/gov/keeper/keeper_test.go | 11 ++----- x/gov/keeper/msg_server_test.go | 9 +----- x/gov/keeper/proposal_test.go | 21 +++----------- x/gov/keeper/tally_test.go | 7 +---- x/gov/keeper/vote_test.go | 6 +--- 9 files changed, 29 insertions(+), 112 deletions(-) diff --git a/x/gov/keeper/common_test.go b/x/gov/keeper/common_test.go index fa22c871a988..1e3a55ffda5b 100644 --- a/x/gov/keeper/common_test.go +++ b/x/gov/keeper/common_test.go @@ -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. @@ -43,16 +43,16 @@ var ( const mintModuleName = "mint" // getTestProposal creates and returns a test proposal message. -func getTestProposal() ([]sdk.Msg, error) { +func getTestProposal() []sdk.Msg { legacyProposalMsg, err := v1.NewLegacyContent(v1beta1.NewTextProposal("Title", "description"), authtypes.NewModuleAddress(types.ModuleName).String()) if err != nil { - return nil, err + panic(err) } return []sdk.Msg{ banktypes.NewMsgSend(govAcct, addr, sdk.NewCoins(sdk.NewCoin("stake", math.NewInt(1000)))), legacyProposalMsg, - }, nil + } } type mocks struct { diff --git a/x/gov/keeper/deposit_test.go b/x/gov/keeper/deposit_test.go index bef9462446cb..3def399e811b 100644 --- a/x/gov/keeper/deposit_test.go +++ b/x/gov/keeper/deposit_test.go @@ -4,7 +4,6 @@ import ( "fmt" "testing" - "github.com/cosmos/gogoproto/proto" "github.com/stretchr/testify/require" "cosmossdk.io/collections" @@ -54,10 +53,7 @@ func TestDeposits(t *testing.T) { TestAddrs := simtestutil.AddTestAddrsIncremental(bankKeeper, stakingKeeper, ctx, 2, sdkmath.NewInt(10000000*depositMultiplier)) authKeeper.EXPECT().AddressCodec().Return(address.NewBech32Codec("cosmos")).AnyTimes() - var tp []proto.Message - if TestProposal != nil { - tp = TestProposal - } + tp := TestProposal proposal, err := govKeeper.SubmitProposal(ctx, tp, "", "title", "summary", TestAddrs[0], tc.expedited) require.NoError(t, err) proposalID := proposal.Id @@ -229,10 +225,7 @@ func TestDepositAmount(t *testing.T) { err := govKeeper.Params.Set(ctx, params) require.NoError(t, err) - var tp []proto.Message - if TestProposal != nil { - tp = TestProposal - } + tp := TestProposal proposal, err := govKeeper.SubmitProposal(ctx, tp, "", "title", "summary", testAddrs[0], false) require.NoError(t, err) proposalID := proposal.Id @@ -422,10 +415,7 @@ func TestChargeDeposit(t *testing.T) { err := govKeeper.Params.Set(ctx, params) require.NoError(t, err) - var tp []proto.Message - if TestProposal != nil { - tp = TestProposal - } + tp := TestProposal proposal, err := govKeeper.SubmitProposal(ctx, tp, "", "title", "summary", TestAddrs[0], false) require.NoError(t, err) proposalID := proposal.Id diff --git a/x/gov/keeper/grpc_query_test.go b/x/gov/keeper/grpc_query_test.go index 4d9bf9c214cb..efc6c63d5395 100644 --- a/x/gov/keeper/grpc_query_test.go +++ b/x/gov/keeper/grpc_query_test.go @@ -5,8 +5,6 @@ import ( "fmt" "time" - "github.com/cosmos/gogoproto/proto" - "cosmossdk.io/math" v3 "cosmossdk.io/x/gov/migrations/v3" v1 "cosmossdk.io/x/gov/types/v1" @@ -445,10 +443,6 @@ func (suite *KeeperTestSuite) TestLegacyGRPCQueryProposals() { func (suite *KeeperTestSuite) TestGRPCQueryVote() { ctx, queryClient, addrs := suite.ctx, suite.queryClient, suite.addrs - var tp []proto.Message - if TestProposal != nil { - tp = TestProposal - } var ( req *v1.QueryVoteRequest @@ -502,7 +496,7 @@ func (suite *KeeperTestSuite) TestGRPCQueryVote() { "no votes present", func() { var err error - proposal, err = suite.govKeeper.SubmitProposal(ctx, tp, "", "title", "summary", addrs[0], false) + proposal, err = suite.govKeeper.SubmitProposal(ctx, TestProposal, "", "title", "summary", addrs[0], false) suite.Require().NoError(err) req = &v1.QueryVoteRequest{ @@ -564,10 +558,7 @@ func (suite *KeeperTestSuite) TestGRPCQueryVote() { func (suite *KeeperTestSuite) TestLegacyGRPCQueryVote() { ctx, queryClient, addrs := suite.ctx, suite.legacyQueryClient, suite.addrs - var tp []proto.Message - if TestProposal != nil { - tp = TestProposal - } + var ( req *v1beta1.QueryVoteRequest expRes *v1beta1.QueryVoteResponse @@ -620,7 +611,7 @@ func (suite *KeeperTestSuite) TestLegacyGRPCQueryVote() { "no votes present", func() { var err error - proposal, err = suite.govKeeper.SubmitProposal(ctx, tp, "", "title", "summary", addrs[0], false) + proposal, err = suite.govKeeper.SubmitProposal(ctx, TestProposal, "", "title", "summary", addrs[0], false) suite.Require().NoError(err) req = &v1beta1.QueryVoteRequest{ @@ -683,10 +674,6 @@ func (suite *KeeperTestSuite) TestLegacyGRPCQueryVote() { func (suite *KeeperTestSuite) TestGRPCQueryVotes() { suite.reset() ctx, queryClient := suite.ctx, suite.queryClient - var tp []proto.Message - if TestProposal != nil { - tp = TestProposal - } addrs := simtestutil.AddTestAddrsIncremental(suite.bankKeeper, suite.stakingKeeper, ctx, 2, math.NewInt(30000000)) @@ -731,7 +718,7 @@ func (suite *KeeperTestSuite) TestGRPCQueryVotes() { "create a proposal and get votes", func() { var err error - proposal, err = suite.govKeeper.SubmitProposal(ctx, tp, "", "title", "summary", addrs[0], false) + proposal, err = suite.govKeeper.SubmitProposal(ctx, TestProposal, "", "title", "summary", addrs[0], false) suite.Require().NoError(err) req = &v1.QueryVotesRequest{ @@ -791,10 +778,6 @@ func (suite *KeeperTestSuite) TestGRPCQueryVotes() { func (suite *KeeperTestSuite) TestLegacyGRPCQueryVotes() { suite.reset() ctx, queryClient := suite.ctx, suite.legacyQueryClient - var tp []proto.Message - if TestProposal != nil { - tp = TestProposal - } addrs := simtestutil.AddTestAddrsIncremental(suite.bankKeeper, suite.stakingKeeper, ctx, 2, math.NewInt(30000000)) @@ -839,7 +822,7 @@ func (suite *KeeperTestSuite) TestLegacyGRPCQueryVotes() { "create a proposal and get votes", func() { var err error - proposal, err = suite.govKeeper.SubmitProposal(ctx, tp, "", "title", "summary", addrs[0], false) + proposal, err = suite.govKeeper.SubmitProposal(ctx, TestProposal, "", "title", "summary", addrs[0], false) suite.Require().NoError(err) req = &v1beta1.QueryVotesRequest{ @@ -1074,10 +1057,6 @@ func (suite *KeeperTestSuite) TestLegacyGRPCQueryParams() { func (suite *KeeperTestSuite) TestGRPCQueryDeposit() { suite.reset() ctx, queryClient, addrs := suite.ctx, suite.queryClient, suite.addrs - var tp []proto.Message - if TestProposal != nil { - tp = TestProposal - } var ( req *v1.QueryDepositRequest @@ -1131,7 +1110,7 @@ func (suite *KeeperTestSuite) TestGRPCQueryDeposit() { "no deposits proposal", func() { var err error - proposal, err = suite.govKeeper.SubmitProposal(ctx, tp, "", "title", "summary", addrs[0], false) + proposal, err = suite.govKeeper.SubmitProposal(ctx, TestProposal, "", "title", "summary", addrs[0], false) suite.Require().NoError(err) suite.Require().NotNil(proposal) @@ -1180,10 +1159,6 @@ func (suite *KeeperTestSuite) TestGRPCQueryDeposit() { func (suite *KeeperTestSuite) TestLegacyGRPCQueryDeposit() { ctx, queryClient, addrs := suite.ctx, suite.legacyQueryClient, suite.addrs - var tp []proto.Message - if TestProposal != nil { - tp = TestProposal - } var ( req *v1beta1.QueryDepositRequest @@ -1237,7 +1212,7 @@ func (suite *KeeperTestSuite) TestLegacyGRPCQueryDeposit() { "no deposits proposal", func() { var err error - proposal, err = suite.govKeeper.SubmitProposal(ctx, tp, "", "title", "summary", addrs[0], false) + proposal, err = suite.govKeeper.SubmitProposal(ctx, TestProposal, "", "title", "summary", addrs[0], false) suite.Require().NoError(err) suite.Require().NotNil(proposal) @@ -1287,10 +1262,6 @@ func (suite *KeeperTestSuite) TestLegacyGRPCQueryDeposit() { func (suite *KeeperTestSuite) TestGRPCQueryDeposits() { ctx, queryClient, addrs := suite.ctx, suite.queryClient, suite.addrs - var tp []proto.Message - if TestProposal != nil { - tp = TestProposal - } var ( req *v1.QueryDepositsRequest @@ -1332,7 +1303,7 @@ func (suite *KeeperTestSuite) TestGRPCQueryDeposits() { "create a proposal and get deposits", func() { var err error - proposal, err = suite.govKeeper.SubmitProposal(ctx, tp, "", "title", "summary", addrs[0], true) + proposal, err = suite.govKeeper.SubmitProposal(ctx, TestProposal, "", "title", "summary", addrs[0], true) suite.Require().NoError(err) req = &v1.QueryDepositsRequest{ @@ -1388,10 +1359,6 @@ func (suite *KeeperTestSuite) TestGRPCQueryDeposits() { func (suite *KeeperTestSuite) TestLegacyGRPCQueryDeposits() { suite.reset() ctx, queryClient, addrs := suite.ctx, suite.legacyQueryClient, suite.addrs - var tp []proto.Message - if TestProposal != nil { - tp = TestProposal - } var ( req *v1beta1.QueryDepositsRequest @@ -1433,7 +1400,7 @@ func (suite *KeeperTestSuite) TestLegacyGRPCQueryDeposits() { "create a proposal and get deposits", func() { var err error - proposal, err = suite.govKeeper.SubmitProposal(ctx, tp, "", "title", "summary", addrs[0], false) + proposal, err = suite.govKeeper.SubmitProposal(ctx, TestProposal, "", "title", "summary", addrs[0], false) suite.Require().NoError(err) req = &v1beta1.QueryDepositsRequest{ diff --git a/x/gov/keeper/hooks_test.go b/x/gov/keeper/hooks_test.go index bd7dec714406..1b7fb616f0fd 100644 --- a/x/gov/keeper/hooks_test.go +++ b/x/gov/keeper/hooks_test.go @@ -5,7 +5,6 @@ import ( "testing" "time" - "github.com/cosmos/gogoproto/proto" "github.com/stretchr/testify/require" "cosmossdk.io/x/gov" @@ -75,10 +74,7 @@ func TestHooks(t *testing.T) { require.False(t, govHooksReceiver.AfterProposalFailedMinDepositValid) require.False(t, govHooksReceiver.AfterProposalVotingPeriodEndedValid) - var tp []proto.Message - if TestProposal != nil { - tp = TestProposal - } + tp := TestProposal _, err := govKeeper.SubmitProposal(ctx, tp, "", "test", "summary", sdk.AccAddress("cosmos1ghekyjucln7y67ntx7cf27m9dpuxxemn4c8g4r"), false) require.NoError(t, err) require.True(t, govHooksReceiver.AfterProposalSubmissionValid) diff --git a/x/gov/keeper/keeper_test.go b/x/gov/keeper/keeper_test.go index a66e33761cc0..da7f83e62d85 100644 --- a/x/gov/keeper/keeper_test.go +++ b/x/gov/keeper/keeper_test.go @@ -3,7 +3,6 @@ package keeper_test import ( "testing" - "github.com/cosmos/gogoproto/proto" "github.com/stretchr/testify/require" "github.com/stretchr/testify/suite" @@ -89,10 +88,7 @@ func TestIncrementProposalNumber(t *testing.T) { addrBz, err := ac.StringToBytes(address1) require.NoError(t, err) - var tp []proto.Message - if TestProposal != nil { - tp = TestProposal - } + tp := TestProposal _, err = govKeeper.SubmitProposal(ctx, tp, "", "test", "summary", addrBz, false) require.NoError(t, err) _, err = govKeeper.SubmitProposal(ctx, tp, "", "test", "summary", addrBz, false) @@ -119,10 +115,7 @@ func TestProposalQueues(t *testing.T) { authKeeper.EXPECT().AddressCodec().Return(address.NewBech32Codec("cosmos")).AnyTimes() // create test proposals - var tp []proto.Message - if TestProposal != nil { - tp = TestProposal - } + tp := TestProposal proposal, err := govKeeper.SubmitProposal(ctx, tp, "", "test", "summary", addrBz, false) require.NoError(t, err) diff --git a/x/gov/keeper/msg_server_test.go b/x/gov/keeper/msg_server_test.go index bddd682303a9..0c8be118bc42 100644 --- a/x/gov/keeper/msg_server_test.go +++ b/x/gov/keeper/msg_server_test.go @@ -4,8 +4,6 @@ import ( "strings" "time" - "github.com/cosmos/gogoproto/proto" - sdkmath "cosmossdk.io/math" banktypes "cosmossdk.io/x/bank/types" v1 "cosmossdk.io/x/gov/types/v1" @@ -1783,12 +1781,7 @@ func (suite *KeeperTestSuite) TestSubmitProposal_InitialDeposit() { err := govKeeper.Params.Set(ctx, params) suite.Require().NoError(err) - var tp []proto.Message - if TestProposal != nil { - tp = TestProposal - } - - msg, err := v1.NewMsgSubmitProposal(tp, tc.initialDeposit, address.String(), "test", "Proposal", "description of proposal", false) + msg, err := v1.NewMsgSubmitProposal(TestProposal, tc.initialDeposit, address.String(), "test", "Proposal", "description of proposal", false) suite.Require().NoError(err) // System under test diff --git a/x/gov/keeper/proposal_test.go b/x/gov/keeper/proposal_test.go index a3314be70e9a..27cfeeaf55cc 100644 --- a/x/gov/keeper/proposal_test.go +++ b/x/gov/keeper/proposal_test.go @@ -6,7 +6,6 @@ import ( "strings" "testing" - "github.com/cosmos/gogoproto/proto" "github.com/stretchr/testify/require" "cosmossdk.io/collections" @@ -30,10 +29,7 @@ func (suite *KeeperTestSuite) TestGetSetProposal() { } for _, tc := range testCases { - var tp []proto.Message - if TestProposal != nil { - tp = TestProposal - } + tp := TestProposal proposal, err := suite.govKeeper.SubmitProposal(suite.ctx, tp, "", "test", "summary", suite.addrs[0], tc.expedited) suite.Require().NoError(err) proposalID := proposal.Id @@ -61,10 +57,7 @@ func (suite *KeeperTestSuite) TestDeleteProposal() { // delete non-existing proposal suite.Require().ErrorIs(suite.govKeeper.DeleteProposal(suite.ctx, 10), collections.ErrNotFound) - var tp []proto.Message - if TestProposal != nil { - tp = TestProposal - } + tp := TestProposal proposal, err := suite.govKeeper.SubmitProposal(suite.ctx, tp, "", "test", "summary", suite.addrs[0], tc.expedited) suite.Require().NoError(err) proposalID := proposal.Id @@ -88,10 +81,7 @@ func (suite *KeeperTestSuite) TestActivateVotingPeriod() { } for _, tc := range testCases { - var tp []proto.Message - if TestProposal != nil { - tp = TestProposal - } + tp := TestProposal proposal, err := suite.govKeeper.SubmitProposal(suite.ctx, tp, "", "test", "summary", suite.addrs[0], tc.expedited) suite.Require().NoError(err) @@ -122,10 +112,7 @@ func (suite *KeeperTestSuite) TestDeleteProposalInVotingPeriod() { for _, tc := range testCases { suite.reset() - var tp []proto.Message - if TestProposal != nil { - tp = TestProposal - } + tp := TestProposal proposal, err := suite.govKeeper.SubmitProposal(suite.ctx, tp, "", "test", "summary", suite.addrs[0], tc.expedited) suite.Require().NoError(err) suite.Require().Nil(proposal.VotingStartTime) diff --git a/x/gov/keeper/tally_test.go b/x/gov/keeper/tally_test.go index 58deadafb589..3ba27edf2fc4 100644 --- a/x/gov/keeper/tally_test.go +++ b/x/gov/keeper/tally_test.go @@ -4,7 +4,6 @@ import ( "context" "testing" - "github.com/cosmos/gogoproto/proto" "github.com/golang/mock/gomock" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -394,13 +393,9 @@ func TestTally(t *testing.T) { } return nil }) - var tp []proto.Message - if TestProposal != nil { - tp = TestProposal - } // Submit and activate a proposal - proposal, err := govKeeper.SubmitProposal(ctx, tp, "", "title", "summary", delAddrs[0], tt.expedited) + proposal, err := govKeeper.SubmitProposal(ctx, TestProposal, "", "title", "summary", delAddrs[0], tt.expedited) require.NoError(t, err) err = govKeeper.ActivateVotingPeriod(ctx, proposal) require.NoError(t, err) diff --git a/x/gov/keeper/vote_test.go b/x/gov/keeper/vote_test.go index f6660f96729c..e5fcd96e46bd 100644 --- a/x/gov/keeper/vote_test.go +++ b/x/gov/keeper/vote_test.go @@ -3,7 +3,6 @@ package keeper_test import ( "testing" - "github.com/cosmos/gogoproto/proto" "github.com/stretchr/testify/require" "cosmossdk.io/collections" @@ -21,10 +20,7 @@ func TestVotes(t *testing.T) { addrs := simtestutil.AddTestAddrsIncremental(bankKeeper, stakingKeeper, ctx, 2, sdkmath.NewInt(10000000)) authKeeper.EXPECT().AddressCodec().Return(address.NewBech32Codec("cosmos")).AnyTimes() - var tp []proto.Message - if TestProposal != nil { - tp = TestProposal - } + tp := TestProposal proposal, err := govKeeper.SubmitProposal(ctx, tp, "", "title", "description", sdk.AccAddress("cosmos1ghekyjucln7y67ntx7cf27m9dpuxxemn4c8g4r"), false) require.NoError(t, err) proposalID := proposal.Id From e77cbc4ba6d3a127e5b1e87730ee01f1233206f8 Mon Sep 17 00:00:00 2001 From: likhita-809 Date: Mon, 11 Dec 2023 11:07:19 +0530 Subject: [PATCH 11/14] add changelog --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 1f2c93f1862b..b3a35bd2dc6b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -56,6 +56,7 @@ Ref: https://keepachangelog.com/en/1.0.0/ ### Improvements +* (x/*all*) [18636](https://github.com/cosmos/cosmos-sdk/pull/18636) Removed the usage of `panic` in keeper methods across all x/* modules and replaced with proper error returns. * (client/keys) [#18663](https://github.com/cosmos/cosmos-sdk/pull/18663) Improve ` keys add` by displaying mnemonic discreetly on an alternate screen and adding `--indiscreet` option to disable it. * (telemetry) [#18646] (https://github.com/cosmos/cosmos-sdk/pull/18646) Enable statsd and dogstatsd telemetry sinks * (types) [#18440](https://github.com/cosmos/cosmos-sdk/pull/18440) Add `AmountOfNoValidation` to `sdk.DecCoins`. From f13271fce8501a87d47e2b8ba015a18f3dd4b29d Mon Sep 17 00:00:00 2001 From: likhita-809 Date: Mon, 11 Dec 2023 11:26:11 +0530 Subject: [PATCH 12/14] fix changelog --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index b3a35bd2dc6b..9e466e59202d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -56,7 +56,7 @@ Ref: https://keepachangelog.com/en/1.0.0/ ### Improvements -* (x/*all*) [18636](https://github.com/cosmos/cosmos-sdk/pull/18636) Removed the usage of `panic` in keeper methods across all x/* modules and replaced with proper error returns. +* (x/*all*) [#18636](https://github.com/cosmos/cosmos-sdk/pull/18636) Removed the usage of `panic` in keeper methods across all x/* modules and replaced with proper error returns. * (client/keys) [#18663](https://github.com/cosmos/cosmos-sdk/pull/18663) Improve ` keys add` by displaying mnemonic discreetly on an alternate screen and adding `--indiscreet` option to disable it. * (telemetry) [#18646] (https://github.com/cosmos/cosmos-sdk/pull/18646) Enable statsd and dogstatsd telemetry sinks * (types) [#18440](https://github.com/cosmos/cosmos-sdk/pull/18440) Add `AmountOfNoValidation` to `sdk.DecCoins`. From c7ade9c7988ef62a33da1af8a41af447e6555e66 Mon Sep 17 00:00:00 2001 From: likhita-809 Date: Mon, 11 Dec 2023 18:37:05 +0530 Subject: [PATCH 13/14] update changelog to be exhaustive --- CHANGELOG.md | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 9e466e59202d..8d9d17996095 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -56,7 +56,11 @@ Ref: https://keepachangelog.com/en/1.0.0/ ### Improvements -* (x/*all*) [#18636](https://github.com/cosmos/cosmos-sdk/pull/18636) Removed the usage of `panic` in keeper methods across all x/* modules and replaced with proper error returns. +* (x/bank) [#18636](https://github.com/cosmos/cosmos-sdk/pull/18636) `SendCoinsFromModuleToAccount`, `SendCoinsFromModuleToModule`, `SendCoinsFromAccountToModule`, `DelegateCoinsFromAccountToModule`, `UndelegateCoinsFromModuleToAccount`, `MintCoins` and `BurnCoins` methods now returns an error instead of panicking if any module accounts does not exist or unauthorized. +* (x/distribution) [#18636](https://github.com/cosmos/cosmos-sdk/pull/18636) `CalculateDelegationRewards` and `DelegationTotalRewards` methods no longer panics on any sanity checks and instead returns appropriate erros. +* (x/slashing) [#18636](https://github.com/cosmos/cosmos-sdk/pull/18636) `JailUntil` and `Tombstone` methods no longer panics if the signing info does not exist for the validator but instead returns error. +* (x/staking) [#18636](https://github.com/cosmos/cosmos-sdk/pull/18636) `IterateBondedValidatorsByPower`, `GetDelegatorBonded`, `Delegate`, `Unbond`, `Slash`, `Jail`, `SlashRedelegation`, `ApplyAndReturnValidatorSetUpdates` methods no longer panics on any kind of errors but instead returns appropriate errors. + * Usage of `Must...` kind of functions are avoided in keeper methods. * (client/keys) [#18663](https://github.com/cosmos/cosmos-sdk/pull/18663) Improve ` keys add` by displaying mnemonic discreetly on an alternate screen and adding `--indiscreet` option to disable it. * (telemetry) [#18646] (https://github.com/cosmos/cosmos-sdk/pull/18646) Enable statsd and dogstatsd telemetry sinks * (types) [#18440](https://github.com/cosmos/cosmos-sdk/pull/18440) Add `AmountOfNoValidation` to `sdk.DecCoins`. From 9987ae02aeb9683581226e584155f62cf2b3ffc1 Mon Sep 17 00:00:00 2001 From: likhita-809 Date: Tue, 12 Dec 2023 10:39:27 +0530 Subject: [PATCH 14/14] improvements: address bot comments --- CHANGELOG.md | 2 +- x/staking/keeper/delegation.go | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index dde015c08778..9b7495a0f55e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -54,7 +54,7 @@ Ref: https://keepachangelog.com/en/1.0.0/ ### Improvements * (x/bank) [#18636](https://github.com/cosmos/cosmos-sdk/pull/18636) `SendCoinsFromModuleToAccount`, `SendCoinsFromModuleToModule`, `SendCoinsFromAccountToModule`, `DelegateCoinsFromAccountToModule`, `UndelegateCoinsFromModuleToAccount`, `MintCoins` and `BurnCoins` methods now returns an error instead of panicking if any module accounts does not exist or unauthorized. -* (x/distribution) [#18636](https://github.com/cosmos/cosmos-sdk/pull/18636) `CalculateDelegationRewards` and `DelegationTotalRewards` methods no longer panics on any sanity checks and instead returns appropriate erros. +* (x/distribution) [#18636](https://github.com/cosmos/cosmos-sdk/pull/18636) `CalculateDelegationRewards` and `DelegationTotalRewards` methods no longer panics on any sanity checks and instead returns appropriate errors. * (x/slashing) [#18636](https://github.com/cosmos/cosmos-sdk/pull/18636) `JailUntil` and `Tombstone` methods no longer panics if the signing info does not exist for the validator but instead returns error. * (x/staking) [#18636](https://github.com/cosmos/cosmos-sdk/pull/18636) `IterateBondedValidatorsByPower`, `GetDelegatorBonded`, `Delegate`, `Unbond`, `Slash`, `Jail`, `SlashRedelegation`, `ApplyAndReturnValidatorSetUpdates` methods no longer panics on any kind of errors but instead returns appropriate errors. * Usage of `Must...` kind of functions are avoided in keeper methods. diff --git a/x/staking/keeper/delegation.go b/x/staking/keeper/delegation.go index 558f7db28cf9..651cc6310b62 100644 --- a/x/staking/keeper/delegation.go +++ b/x/staking/keeper/delegation.go @@ -722,7 +722,7 @@ func (k Keeper) Delegate( // all non bonded if subtractAccount { if tokenSrc == types.Bonded { - return math.LegacyZeroDec(), fmt.Errorf("delegation token source cannot be bonded") + return math.LegacyZeroDec(), fmt.Errorf("delegation token source cannot be bonded; expected Unbonded or Unbonding, got Bonded") } var sendName string @@ -837,7 +837,7 @@ func (k Keeper) Unbond( validator.TokensFromShares(delegation.Shares).TruncateInt().LT(validator.MinSelfDelegation) { err = k.jailValidator(ctx, validator) if err != nil { - return amount, fmt.Errorf("failed to get updated validator after jailing: %v", err) + return amount, fmt.Errorf("failed to jail validator: %v", err) } validator, err = k.GetValidator(ctx, valbz) if err != nil {