refactor(slashing): move ValidateBasic logic to msgServer (#15793)

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
 	}
 

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 {

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(&params)
 	store.Set(types.ParamsKey, bz)

x/slashing/keeper/params_test.go

@@ -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)
-		})
-	}
-}

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
-}