From ec1f0c5df102d59d1975430d1fbb9b9394132bd5 Mon Sep 17 00:00:00 2001 From: Facundo Medica <14063057+facundomedica@users.noreply.github.com> Date: Fri, 30 Jun 2023 15:11:53 +0200 Subject: [PATCH] refactor(x/distribution): audit changes (#16785) --- .../distribution/keeper/grpc_query_test.go | 2 +- .../distribution/keeper/msg_server_test.go | 8 ++-- x/distribution/abci.go | 7 +-- x/distribution/keeper/genesis.go | 4 +- x/distribution/keeper/hooks.go | 3 +- x/distribution/keeper/msg_server.go | 4 +- x/distribution/keeper/store.go | 4 +- x/distribution/types/msg.go | 46 ------------------- 8 files changed, 18 insertions(+), 60 deletions(-) diff --git a/tests/integration/distribution/keeper/grpc_query_test.go b/tests/integration/distribution/keeper/grpc_query_test.go index 63e8732bef..ee4f5fc691 100644 --- a/tests/integration/distribution/keeper/grpc_query_test.go +++ b/tests/integration/distribution/keeper/grpc_query_test.go @@ -510,7 +510,7 @@ func TestGRPCDelegationRewards(t *testing.T) { delTokens := sdk.TokensFromConsensusPower(2, sdk.DefaultPowerReduction) validator, issuedShares := val.AddTokensFromDel(delTokens) delegation := stakingtypes.NewDelegation(delAddr, f.valAddr, issuedShares) - f.stakingKeeper.SetDelegation(f.sdkCtx, delegation) + assert.NilError(t, f.stakingKeeper.SetDelegation(f.sdkCtx, delegation)) assert.NilError(t, f.distrKeeper.DelegatorStartingInfo.Set(f.sdkCtx, collections.Join(validator.GetOperator(), delAddr), types.NewDelegatorStartingInfo(2, math.LegacyNewDec(initialStake), 20))) // setup validator rewards diff --git a/tests/integration/distribution/keeper/msg_server_test.go b/tests/integration/distribution/keeper/msg_server_test.go index 71e74f0ceb..cbf4f4a6ee 100644 --- a/tests/integration/distribution/keeper/msg_server_test.go +++ b/tests/integration/distribution/keeper/msg_server_test.go @@ -181,7 +181,7 @@ func TestMsgWithdrawDelegatorReward(t *testing.T) { assert.NilError(t, err) validator.DelegatorShares = math.LegacyNewDec(100) validator.Tokens = sdk.NewInt(1000000) - f.stakingKeeper.SetValidator(f.sdkCtx, validator) + assert.NilError(t, f.stakingKeeper.SetValidator(f.sdkCtx, validator)) // set module account coins initTokens := f.stakingKeeper.TokensFromConsensusPower(f.sdkCtx, int64(1000)) @@ -197,7 +197,7 @@ func TestMsgWithdrawDelegatorReward(t *testing.T) { delTokens := sdk.TokensFromConsensusPower(2, sdk.DefaultPowerReduction) validator, issuedShares := validator.AddTokensFromDel(delTokens) delegation := stakingtypes.NewDelegation(delAddr, validator.GetOperator(), issuedShares) - f.stakingKeeper.SetDelegation(f.sdkCtx, delegation) + require.NoError(t, f.stakingKeeper.SetDelegation(f.sdkCtx, delegation)) err = f.distrKeeper.DelegatorStartingInfo.Set(f.sdkCtx, collections.Join(validator.GetOperator(), delAddr), distrtypes.NewDelegatorStartingInfo(2, math.LegacyOneDec(), 20)) require.NoError(t, err) // setup validator rewards @@ -897,8 +897,8 @@ func TestMsgDepositValidatorRewardsPool(t *testing.T) { // mint a non-staking token and send to an account amt := sdk.NewCoins(sdk.NewInt64Coin("foo", 500)) - f.bankKeeper.MintCoins(f.sdkCtx, distrtypes.ModuleName, amt) - f.bankKeeper.SendCoinsFromModuleToAccount(f.sdkCtx, distrtypes.ModuleName, addr, amt) + require.NoError(t, f.bankKeeper.MintCoins(f.sdkCtx, distrtypes.ModuleName, amt)) + require.NoError(t, f.bankKeeper.SendCoinsFromModuleToAccount(f.sdkCtx, distrtypes.ModuleName, addr, amt)) bondDenom, err := f.stakingKeeper.BondDenom(f.sdkCtx) require.NoError(t, err) diff --git a/x/distribution/abci.go b/x/distribution/abci.go index 415adcc571..2751cfab83 100644 --- a/x/distribution/abci.go +++ b/x/distribution/abci.go @@ -23,11 +23,12 @@ func BeginBlocker(ctx sdk.Context, k keeper.Keeper) error { // TODO this is Tendermint-dependent // ref https://github.com/cosmos/cosmos-sdk/issues/3095 if ctx.BlockHeight() > 1 { - k.AllocateTokens(ctx, previousTotalPower, ctx.VoteInfos()) + if err := k.AllocateTokens(ctx, previousTotalPower, ctx.VoteInfos()); err != nil { + return err + } } // record the proposer for when we payout on the next block consAddr := sdk.ConsAddress(ctx.BlockHeader().ProposerAddress) - k.SetPreviousProposerConsAddr(ctx, consAddr) - return nil + return k.SetPreviousProposerConsAddr(ctx, consAddr) } diff --git a/x/distribution/keeper/genesis.go b/x/distribution/keeper/genesis.go index 1dafc4f118..e018eeeb75 100644 --- a/x/distribution/keeper/genesis.go +++ b/x/distribution/keeper/genesis.go @@ -47,7 +47,9 @@ func (k Keeper) InitGenesis(ctx sdk.Context, data types.GenesisState) { } } - k.SetPreviousProposerConsAddr(ctx, previousProposer) + if err = k.SetPreviousProposerConsAddr(ctx, previousProposer); err != nil { + panic(err) + } for _, rew := range data.OutstandingRewards { valAddr, err := sdk.ValAddressFromBech32(rew.ValidatorAddress) diff --git a/x/distribution/keeper/hooks.go b/x/distribution/keeper/hooks.go index ae4de734aa..97b3db9e56 100644 --- a/x/distribution/keeper/hooks.go +++ b/x/distribution/keeper/hooks.go @@ -162,8 +162,7 @@ func (h Hooks) AfterDelegationModified(ctx context.Context, delAddr sdk.AccAddre // record the slash event func (h Hooks) BeforeValidatorSlashed(ctx context.Context, valAddr sdk.ValAddress, fraction sdkmath.LegacyDec) error { - h.k.updateValidatorSlashFraction(ctx, valAddr, fraction) - return nil + return h.k.updateValidatorSlashFraction(ctx, valAddr, fraction) } func (h Hooks) BeforeValidatorModified(_ context.Context, _ sdk.ValAddress) error { diff --git a/x/distribution/keeper/msg_server.go b/x/distribution/keeper/msg_server.go index 4572648f57..f4784ba28a 100644 --- a/x/distribution/keeper/msg_server.go +++ b/x/distribution/keeper/msg_server.go @@ -195,7 +195,9 @@ func (k msgServer) DepositValidatorRewardsPool(ctx context.Context, msg *types.M // Allocate tokens from the distribution module to the validator, which are // then distributed to the validator's delegators. reward := sdk.NewDecCoinsFromCoins(msg.Amount...) - k.AllocateTokensToValidator(ctx, validator, reward) + if err = k.AllocateTokensToValidator(ctx, validator, reward); err != nil { + return nil, err + } logger := k.Logger(ctx) logger.Info( diff --git a/x/distribution/keeper/store.go b/x/distribution/keeper/store.go index db5e3511ba..783a14364d 100644 --- a/x/distribution/keeper/store.go +++ b/x/distribution/keeper/store.go @@ -46,10 +46,10 @@ func (k Keeper) GetPreviousProposerConsAddr(ctx context.Context) (sdk.ConsAddres } // set the proposer public key for this block -func (k Keeper) SetPreviousProposerConsAddr(ctx context.Context, consAddr sdk.ConsAddress) { +func (k Keeper) SetPreviousProposerConsAddr(ctx context.Context, consAddr sdk.ConsAddress) error { store := k.storeService.OpenKVStore(ctx) bz := k.cdc.MustMarshal(&gogotypes.BytesValue{Value: consAddr}) - store.Set(types.ProposerKey, bz) + return store.Set(types.ProposerKey, bz) } // historical reference count (used for testcases) diff --git a/x/distribution/types/msg.go b/x/distribution/types/msg.go index f8b1f666d5..44202deaef 100644 --- a/x/distribution/types/msg.go +++ b/x/distribution/types/msg.go @@ -21,12 +21,6 @@ func NewMsgSetWithdrawAddress(delAddr, withdrawAddr sdk.AccAddress) *MsgSetWithd } } -// Return address that must sign over msg.GetSignBytes() -func (msg MsgSetWithdrawAddress) GetSigners() []sdk.AccAddress { - delegator, _ := sdk.AccAddressFromBech32(msg.DelegatorAddress) - return []sdk.AccAddress{delegator} -} - func NewMsgWithdrawDelegatorReward(delAddr sdk.AccAddress, valAddr sdk.ValAddress) *MsgWithdrawDelegatorReward { return &MsgWithdrawDelegatorReward{ DelegatorAddress: delAddr.String(), @@ -34,24 +28,12 @@ func NewMsgWithdrawDelegatorReward(delAddr sdk.AccAddress, valAddr sdk.ValAddres } } -// Return address that must sign over msg.GetSignBytes() -func (msg MsgWithdrawDelegatorReward) GetSigners() []sdk.AccAddress { - delegator, _ := sdk.AccAddressFromBech32(msg.DelegatorAddress) - return []sdk.AccAddress{delegator} -} - func NewMsgWithdrawValidatorCommission(valAddr sdk.ValAddress) *MsgWithdrawValidatorCommission { return &MsgWithdrawValidatorCommission{ ValidatorAddress: valAddr.String(), } } -// Return address that must sign over msg.GetSignBytes() -func (msg MsgWithdrawValidatorCommission) GetSigners() []sdk.AccAddress { - valAddr, _ := sdk.ValAddressFromBech32(msg.ValidatorAddress) - return []sdk.AccAddress{sdk.AccAddress(valAddr)} -} - // NewMsgFundCommunityPool returns a new MsgFundCommunityPool with a sender and // a funding amount. func NewMsgFundCommunityPool(amount sdk.Coins, depositor sdk.AccAddress) *MsgFundCommunityPool { @@ -61,27 +43,6 @@ func NewMsgFundCommunityPool(amount sdk.Coins, depositor sdk.AccAddress) *MsgFun } } -// GetSigners returns the signer addresses that are expected to sign the result -// of GetSignBytes. -func (msg MsgFundCommunityPool) GetSigners() []sdk.AccAddress { - depositor, _ := sdk.AccAddressFromBech32(msg.Depositor) - return []sdk.AccAddress{depositor} -} - -// GetSigners returns the signer addresses that are expected to sign the result -// of GetSignBytes. -func (msg MsgUpdateParams) GetSigners() []sdk.AccAddress { - authority, _ := sdk.AccAddressFromBech32(msg.Authority) - return []sdk.AccAddress{authority} -} - -// GetSigners returns the signer addresses that are expected to sign the result -// of GetSignBytes, which is the authority. -func (msg MsgCommunityPoolSpend) GetSigners() []sdk.AccAddress { - authority, _ := sdk.AccAddressFromBech32(msg.Authority) - return []sdk.AccAddress{authority} -} - // NewMsgDepositValidatorRewardsPool returns a new MsgDepositValidatorRewardsPool // with a depositor and a funding amount. func NewMsgDepositValidatorRewardsPool(depositor sdk.AccAddress, valAddr sdk.ValAddress, amount sdk.Coins) *MsgDepositValidatorRewardsPool { @@ -91,10 +52,3 @@ func NewMsgDepositValidatorRewardsPool(depositor sdk.AccAddress, valAddr sdk.Val ValidatorAddress: valAddr.String(), } } - -// GetSigners returns the signer addresses that are expected to sign the result -// of GetSignBytes, which is the depositor. -func (msg MsgDepositValidatorRewardsPool) GetSigners() []sdk.AccAddress { - depositor, _ := sdk.AccAddressFromBech32(msg.Depositor) - return []sdk.AccAddress{depositor} -}