From c045a005293ea20762a8c0df331e07d58f6d3db9 Mon Sep 17 00:00:00 2001 From: Julien Robert Date: Wed, 12 Apr 2023 00:03:51 +0200 Subject: [PATCH] refactor(slashing): move `ValidateBasic` logic to `msgServer` (#15793) --- x/slashing/keeper/msg_server.go | 27 +++--- x/slashing/keeper/msg_server_test.go | 10 +++ x/slashing/keeper/params.go | 5 +- x/slashing/keeper/params_test.go | 127 --------------------------- x/slashing/types/msg.go | 24 ----- 5 files changed, 27 insertions(+), 166 deletions(-) delete mode 100644 x/slashing/keeper/params_test.go diff --git a/x/slashing/keeper/msg_server.go b/x/slashing/keeper/msg_server.go index d671e578fb..b566339334 100644 --- a/x/slashing/keeper/msg_server.go +++ b/x/slashing/keeper/msg_server.go @@ -6,6 +6,7 @@ import ( "cosmossdk.io/errors" sdk "github.com/cosmos/cosmos-sdk/types" + sdkerrors "github.com/cosmos/cosmos-sdk/types/errors" govtypes "github.com/cosmos/cosmos-sdk/x/gov/types" "github.com/cosmos/cosmos-sdk/x/slashing/types" ) @@ -24,12 +25,17 @@ func NewMsgServerImpl(keeper Keeper) types.MsgServer { // UpdateParams implements MsgServer.UpdateParams method. // It defines a method to update the x/slashing module parameters. -func (k msgServer) UpdateParams(goCtx context.Context, req *types.MsgUpdateParams) (*types.MsgUpdateParamsResponse, error) { - if k.authority != req.Authority { - return nil, errors.Wrapf(govtypes.ErrInvalidSigner, "invalid authority; expected %s, got %s", k.authority, req.Authority) +func (k msgServer) UpdateParams(goCtx context.Context, msg *types.MsgUpdateParams) (*types.MsgUpdateParamsResponse, error) { + if k.authority != msg.Authority { + return nil, errors.Wrapf(govtypes.ErrInvalidSigner, "invalid authority; expected %s, got %s", k.authority, msg.Authority) } + + if err := msg.Params.Validate(); err != nil { + return nil, err + } + ctx := sdk.UnwrapSDKContext(goCtx) - if err := k.SetParams(ctx, req.Params); err != nil { + if err := k.SetParams(ctx, msg.Params); err != nil { return nil, err } @@ -40,14 +46,13 @@ func (k msgServer) UpdateParams(goCtx context.Context, req *types.MsgUpdateParam // Validators must submit a transaction to unjail itself after // having been jailed (and thus unbonded) for downtime func (k msgServer) Unjail(goCtx context.Context, msg *types.MsgUnjail) (*types.MsgUnjailResponse, error) { - ctx := sdk.UnwrapSDKContext(goCtx) - - valAddr, valErr := sdk.ValAddressFromBech32(msg.ValidatorAddr) - if valErr != nil { - return nil, valErr - } - err := k.Keeper.Unjail(ctx, valAddr) + valAddr, err := sdk.ValAddressFromBech32(msg.ValidatorAddr) if err != nil { + return nil, sdkerrors.ErrInvalidAddress.Wrapf("validator input address: %s", err) + } + + ctx := sdk.UnwrapSDKContext(goCtx) + if err := k.Keeper.Unjail(ctx, valAddr); err != nil { return nil, err } diff --git a/x/slashing/keeper/msg_server_test.go b/x/slashing/keeper/msg_server_test.go index 5db479c209..2fa9bc6816 100644 --- a/x/slashing/keeper/msg_server_test.go +++ b/x/slashing/keeper/msg_server_test.go @@ -150,6 +150,16 @@ func (s *KeeperTestSuite) TestUnjail() { expErr bool expErrMsg string }{ + { + name: "invalid validator address: invalid request", + malleate: func() *slashingtypes.MsgUnjail { + return &slashingtypes.MsgUnjail{ + ValidatorAddr: "invalid", + } + }, + expErr: true, + expErrMsg: "decoding bech32 failed", + }, { name: "no self delegation: invalid request", malleate: func() *slashingtypes.MsgUnjail { diff --git a/x/slashing/keeper/params.go b/x/slashing/keeper/params.go index 4c7fc18246..787b108be6 100644 --- a/x/slashing/keeper/params.go +++ b/x/slashing/keeper/params.go @@ -49,11 +49,8 @@ func (k Keeper) GetParams(ctx sdk.Context) (params types.Params) { } // SetParams sets the x/slashing module parameters. +// CONTRACT: This method performs no validation of the parameters. func (k Keeper) SetParams(ctx sdk.Context, params types.Params) error { - if err := params.Validate(); err != nil { - return err - } - store := ctx.KVStore(k.storeKey) bz := k.cdc.MustMarshal(¶ms) store.Set(types.ParamsKey, bz) diff --git a/x/slashing/keeper/params_test.go b/x/slashing/keeper/params_test.go deleted file mode 100644 index bcf96ef136..0000000000 --- a/x/slashing/keeper/params_test.go +++ /dev/null @@ -1,127 +0,0 @@ -package keeper_test - -import ( - "time" - - sdk "github.com/cosmos/cosmos-sdk/types" - "github.com/cosmos/cosmos-sdk/x/slashing/types" -) - -func (s *KeeperTestSuite) TestParams() { - ctx, keeper := s.ctx, s.slashingKeeper - require := s.Require() - - minSignedPerWindow, err := sdk.NewDecFromStr("0.60") - require.NoError(err) - - slashFractionDoubleSign, err := sdk.NewDecFromStr("0.022") - require.NoError(err) - - slashFractionDowntime, err := sdk.NewDecFromStr("0.0089") - require.NoError(err) - - invalidVal, err := sdk.NewDecFromStr("-1") - require.NoError(err) - - testCases := []struct { - name string - input types.Params - expectErr bool - expErrMsg string - }{ - { - name: "set invalid signed blocks window", - input: types.Params{ - SignedBlocksWindow: 0, - MinSignedPerWindow: minSignedPerWindow, - DowntimeJailDuration: time.Duration(34800000000000), - SlashFractionDoubleSign: slashFractionDoubleSign, - SlashFractionDowntime: slashFractionDowntime, - }, - expectErr: true, - expErrMsg: "signed blocks window must be positive", - }, - { - name: "set invalid min signed per window", - input: types.Params{ - SignedBlocksWindow: int64(750), - MinSignedPerWindow: invalidVal, - DowntimeJailDuration: time.Duration(34800000000000), - SlashFractionDoubleSign: slashFractionDoubleSign, - SlashFractionDowntime: slashFractionDowntime, - }, - expectErr: true, - expErrMsg: "min signed per window cannot be negative", - }, - { - name: "set invalid downtime jail duration", - input: types.Params{ - SignedBlocksWindow: int64(750), - MinSignedPerWindow: minSignedPerWindow, - DowntimeJailDuration: time.Duration(0), - SlashFractionDoubleSign: slashFractionDoubleSign, - SlashFractionDowntime: slashFractionDowntime, - }, - expectErr: true, - expErrMsg: "downtime jail duration must be positive", - }, - { - name: "set invalid slash fraction double sign", - input: types.Params{ - SignedBlocksWindow: int64(750), - MinSignedPerWindow: minSignedPerWindow, - DowntimeJailDuration: time.Duration(10), - SlashFractionDoubleSign: invalidVal, - SlashFractionDowntime: slashFractionDowntime, - }, - expectErr: true, - expErrMsg: "double sign slash fraction cannot be negative", - }, - { - name: "set invalid slash fraction downtime", - input: types.Params{ - SignedBlocksWindow: int64(750), - MinSignedPerWindow: minSignedPerWindow, - DowntimeJailDuration: time.Duration(10), - SlashFractionDoubleSign: slashFractionDoubleSign, - SlashFractionDowntime: invalidVal, - }, - expectErr: true, - expErrMsg: "downtime slash fraction cannot be negative", - }, - { - name: "set all valid params", - input: types.Params{ - SignedBlocksWindow: int64(750), - MinSignedPerWindow: minSignedPerWindow, - DowntimeJailDuration: time.Duration(34800000000000), - SlashFractionDoubleSign: slashFractionDoubleSign, - SlashFractionDowntime: slashFractionDowntime, - }, - expectErr: false, - }, - } - for _, tc := range testCases { - tc := tc - s.Run(tc.name, func() { - expected := keeper.GetParams(ctx) - err := keeper.SetParams(ctx, tc.input) - - if tc.expectErr { - require.Error(err) - require.Contains(err.Error(), tc.expErrMsg) - } else { - expected = tc.input - require.NoError(err) - } - - params := keeper.GetParams(ctx) - require.Equal(params, expected) - require.Equal(keeper.SignedBlocksWindow(ctx), expected.SignedBlocksWindow) - require.Equal(keeper.MinSignedPerWindow(ctx), expected.MinSignedPerWindow.MulInt64(expected.SignedBlocksWindow).RoundInt64()) - require.Equal(keeper.DowntimeJailDuration(ctx), expected.DowntimeJailDuration) - require.Equal(keeper.SlashFractionDoubleSign(ctx), expected.SlashFractionDoubleSign) - require.Equal(keeper.SlashFractionDowntime(ctx), expected.SlashFractionDowntime) - }) - } -} diff --git a/x/slashing/types/msg.go b/x/slashing/types/msg.go index db8dc4c489..a4aa2925a9 100644 --- a/x/slashing/types/msg.go +++ b/x/slashing/types/msg.go @@ -1,10 +1,7 @@ package types import ( - errorsmod "cosmossdk.io/errors" - sdk "github.com/cosmos/cosmos-sdk/types" - sdkerrors "github.com/cosmos/cosmos-sdk/types/errors" "github.com/cosmos/cosmos-sdk/x/auth/migrations/legacytx" ) @@ -36,14 +33,6 @@ func (msg MsgUnjail) GetSignBytes() []byte { return sdk.MustSortJSON(bz) } -// ValidateBasic does a sanity check on the provided message. -func (msg MsgUnjail) ValidateBasic() error { - if _, err := sdk.ValAddressFromBech32(msg.ValidatorAddr); err != nil { - return sdkerrors.ErrInvalidAddress.Wrapf("validator input address: %s", err) - } - return nil -} - // GetSignBytes implements the LegacyMsg interface. func (msg MsgUpdateParams) GetSignBytes() []byte { return sdk.MustSortJSON(ModuleCdc.MustMarshalJSON(&msg)) @@ -54,16 +43,3 @@ func (msg MsgUpdateParams) GetSigners() []sdk.AccAddress { addr, _ := sdk.AccAddressFromBech32(msg.Authority) return []sdk.AccAddress{addr} } - -// ValidateBasic does a sanity check on the provided data. -func (msg MsgUpdateParams) ValidateBasic() error { - if _, err := sdk.AccAddressFromBech32(msg.Authority); err != nil { - return errorsmod.Wrap(err, "invalid authority address") - } - - if err := msg.Params.Validate(); err != nil { - return err - } - - return nil -}