refactor(auth,vesting): move ValidateBasic logic to msgServer (#15786)

CHANGELOG.md

@@ -105,6 +105,7 @@ Ref: https://keepachangelog.com/en/1.0.0/
 
 ### API Breaking Changes
 
+* (x/*all*) [#15648](https://github.com/cosmos/cosmos-sdk/issues/15648) Make `SetParams` consistent across all modules and validate the params at the message handling instead of `SetParams` method.
 * (x/genutil) [#15679](https://github.com/cosmos/cosmos-sdk/pull/15679) `MigrateGenesisCmd` now takes a `MigrationMap` instead of having the SDK genesis migration hardcoded.
 * (client) [#15673](https://github.com/cosmos/cosmos-sdk/pull/15673) Move `client/keys.OutputFormatJSON` and `client/keys.OutputFormatText` to `client/flags` package.
 * (x/nft) [#15588](https://github.com/cosmos/cosmos-sdk/pull/15588) `NewKeeper` now takes a `KVStoreService` instead of a `StoreKey` and methods in the `Keeper` now take a `context.Context` instead of a `sdk.Context`. 

x/auth/keeper/keeper.go

@@ -286,10 +286,8 @@ func (ak AccountKeeper) getBech32Prefix() (string, error) {
 }
 
 // SetParams sets the auth module's parameters.
+// CONTRACT: This method performs no validation of the parameters.
 func (ak AccountKeeper) SetParams(ctx context.Context, params types.Params) error {
-	if err := params.Validate(); err != nil {
-		return err
-	}
 	return ak.ParamsState.Set(ctx, params)
 }
 

x/auth/keeper/msg_server.go

@@ -23,13 +23,17 @@ func NewMsgServerImpl(ak AccountKeeper) types.MsgServer {
 	}
 }
 
-func (ms msgServer) UpdateParams(goCtx context.Context, req *types.MsgUpdateParams) (*types.MsgUpdateParamsResponse, error) {
-	if ms.authority != req.Authority {
-		return nil, errors.Wrapf(govtypes.ErrInvalidSigner, "invalid authority; expected %s, got %s", ms.authority, req.Authority)
+func (ms msgServer) UpdateParams(goCtx context.Context, msg *types.MsgUpdateParams) (*types.MsgUpdateParamsResponse, error) {
+	if ms.authority != msg.Authority {
+		return nil, errors.Wrapf(govtypes.ErrInvalidSigner, "invalid authority; expected %s, got %s", ms.authority, msg.Authority)
+	}
+
+	if err := msg.Params.Validate(); err != nil {
+		return nil, err
 	}
 
 	ctx := sdk.UnwrapSDKContext(goCtx)
-	if err := ms.SetParams(ctx, req.Params); err != nil {
+	if err := ms.SetParams(ctx, msg.Params); err != nil {
 		return nil, err
 	}
 

x/auth/types/msgs.go

@@ -1,8 +1,6 @@
 package types
 
 import (
-	"cosmossdk.io/errors"
-
 	sdk "github.com/cosmos/cosmos-sdk/types"
 	"github.com/cosmos/cosmos-sdk/x/auth/migrations/legacytx"
 )
@@ -22,16 +20,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 errors.Wrap(err, "invalid authority address")
-	}
-
-	if err := msg.Params.Validate(); err != nil {
-		return err
-	}
-
-	return nil
-}

x/auth/vesting/client/cli/tx.go

@@ -74,7 +74,6 @@ timestamp.`,
 			delayed, _ := cmd.Flags().GetBool(FlagDelayed)
 
 			msg := types.NewMsgCreateVestingAccount(clientCtx.GetFromAddress(), toAddr, amount, endTime, delayed)
-
 			return tx.GenerateOrBroadcastTxCLI(clientCtx, cmd.Flags(), msg)
 		},
 	}
@@ -111,7 +110,6 @@ tokens.`,
 			}
 
 			msg := types.NewMsgCreatePermanentLockedAccount(clientCtx.GetFromAddress(), toAddr, amount)
-
 			return tx.GenerateOrBroadcastTxCLI(clientCtx, cmd.Flags(), msg)
 		},
 	}
@@ -190,15 +188,12 @@ func NewMsgCreatePeriodicVestingAccountCmd() *cobra.Command {
 				if p.Length < 0 {
 					return fmt.Errorf("invalid period length of %d in period %d, length must be greater than 0", p.Length, i)
 				}
+
 				period := types.Period{Length: p.Length, Amount: amount}
 				periods = append(periods, period)
 			}
 
 			msg := types.NewMsgCreatePeriodicVestingAccount(clientCtx.GetFromAddress(), toAddr, vestingData.StartTime, periods)
-			if err := msg.ValidateBasic(); err != nil {
-				return err
-			}
-
 			return tx.GenerateOrBroadcastTxCLI(clientCtx, cmd.Flags(), msg)
 		},
 	}

x/auth/vesting/msg_server.go

@@ -29,33 +29,39 @@ func NewMsgServerImpl(k keeper.AccountKeeper, bk types.BankKeeper) types.MsgServ
 var _ types.MsgServer = msgServer{}
 
 func (s msgServer) CreateVestingAccount(goCtx context.Context, msg *types.MsgCreateVestingAccount) (*types.MsgCreateVestingAccountResponse, error) {
-	ctx := sdk.UnwrapSDKContext(goCtx)
-	ak := s.AccountKeeper
-	bk := s.BankKeeper
-
-	if err := bk.IsSendEnabledCoins(ctx, msg.Amount...); err != nil {
-		return nil, err
-	}
-
 	from, err := sdk.AccAddressFromBech32(msg.FromAddress)
 	if err != nil {
-		return nil, err
+		return nil, sdkerrors.ErrInvalidAddress.Wrapf("invalid 'from' address: %s", err)
 	}
+
 	to, err := sdk.AccAddressFromBech32(msg.ToAddress)
 	if err != nil {
+		return nil, sdkerrors.ErrInvalidAddress.Wrapf("invalid 'to' address: %s", err)
+	}
+
+	if err := validateAmount(msg.Amount); err != nil {
 		return nil, err
 	}
 
-	if bk.BlockedAddr(to) {
+	if msg.EndTime <= 0 {
+		return nil, errorsmod.Wrap(sdkerrors.ErrInvalidRequest, "invalid end time")
+	}
+
+	ctx := sdk.UnwrapSDKContext(goCtx)
+	if err := s.BankKeeper.IsSendEnabledCoins(ctx, msg.Amount...); err != nil {
+		return nil, err
+	}
+
+	if s.BankKeeper.BlockedAddr(to) {
 		return nil, errorsmod.Wrapf(sdkerrors.ErrUnauthorized, "%s is not allowed to receive funds", msg.ToAddress)
 	}
 
-	if acc := ak.GetAccount(ctx, to); acc != nil {
+	if acc := s.AccountKeeper.GetAccount(ctx, to); acc != nil {
 		return nil, errorsmod.Wrapf(sdkerrors.ErrInvalidRequest, "account %s already exists", msg.ToAddress)
 	}
 
 	baseAccount := authtypes.NewBaseAccountWithAddress(to)
-	baseAccount = ak.NewAccount(ctx, baseAccount).(*authtypes.BaseAccount)
+	baseAccount = s.AccountKeeper.NewAccount(ctx, baseAccount).(*authtypes.BaseAccount)
 	baseVestingAccount := types.NewBaseVestingAccount(baseAccount, msg.Amount.Sort(), msg.EndTime)
 
 	var vestingAccount sdk.AccountI
@@ -65,7 +71,7 @@ func (s msgServer) CreateVestingAccount(goCtx context.Context, msg *types.MsgCre
 		vestingAccount = types.NewContinuousVestingAccountRaw(baseVestingAccount, ctx.BlockTime().Unix())
 	}
 
-	ak.SetAccount(ctx, vestingAccount)
+	s.AccountKeeper.SetAccount(ctx, vestingAccount)
 
 	defer func() {
 		telemetry.IncrCounter(1, "new", "account")
@@ -81,7 +87,7 @@ func (s msgServer) CreateVestingAccount(goCtx context.Context, msg *types.MsgCre
 		}
 	}()
 
-	if err = bk.SendCoins(ctx, from, to, msg.Amount); err != nil {
+	if err = s.BankKeeper.SendCoins(ctx, from, to, msg.Amount); err != nil {
 		return nil, err
 	}
 
@@ -89,36 +95,38 @@ func (s msgServer) CreateVestingAccount(goCtx context.Context, msg *types.MsgCre
 }
 
 func (s msgServer) CreatePermanentLockedAccount(goCtx context.Context, msg *types.MsgCreatePermanentLockedAccount) (*types.MsgCreatePermanentLockedAccountResponse, error) {
-	ctx := sdk.UnwrapSDKContext(goCtx)
-	ak := s.AccountKeeper
-	bk := s.BankKeeper
-
-	if err := bk.IsSendEnabledCoins(ctx, msg.Amount...); err != nil {
-		return nil, err
-	}
-
 	from, err := sdk.AccAddressFromBech32(msg.FromAddress)
 	if err != nil {
-		return nil, err
+		return nil, sdkerrors.ErrInvalidAddress.Wrapf("invalid 'from' address: %s", err)
 	}
+
 	to, err := sdk.AccAddressFromBech32(msg.ToAddress)
 	if err != nil {
+		return nil, sdkerrors.ErrInvalidAddress.Wrapf("invalid 'to' address: %s", err)
+	}
+
+	if err := validateAmount(msg.Amount); err != nil {
 		return nil, err
 	}
 
-	if bk.BlockedAddr(to) {
+	ctx := sdk.UnwrapSDKContext(goCtx)
+	if err := s.BankKeeper.IsSendEnabledCoins(ctx, msg.Amount...); err != nil {
+		return nil, err
+	}
+
+	if s.BankKeeper.BlockedAddr(to) {
 		return nil, errorsmod.Wrapf(sdkerrors.ErrUnauthorized, "%s is not allowed to receive funds", msg.ToAddress)
 	}
 
-	if acc := ak.GetAccount(ctx, to); acc != nil {
+	if acc := s.AccountKeeper.GetAccount(ctx, to); acc != nil {
 		return nil, errorsmod.Wrapf(sdkerrors.ErrInvalidRequest, "account %s already exists", msg.ToAddress)
 	}
 
 	baseAccount := authtypes.NewBaseAccountWithAddress(to)
-	baseAccount = ak.NewAccount(ctx, baseAccount).(*authtypes.BaseAccount)
+	baseAccount = s.AccountKeeper.NewAccount(ctx, baseAccount).(*authtypes.BaseAccount)
 	vestingAccount := types.NewPermanentLockedAccount(baseAccount, msg.Amount)
 
-	ak.SetAccount(ctx, vestingAccount)
+	s.AccountKeeper.SetAccount(ctx, vestingAccount)
 
 	defer func() {
 		telemetry.IncrCounter(1, "new", "account")
@@ -134,7 +142,7 @@ func (s msgServer) CreatePermanentLockedAccount(goCtx context.Context, msg *type
 		}
 	}()
 
-	if err = bk.SendCoins(ctx, from, to, msg.Amount); err != nil {
+	if err = s.BankKeeper.SendCoins(ctx, from, to, msg.Amount); err != nil {
 		return nil, err
 	}
 
@@ -142,38 +150,43 @@ func (s msgServer) CreatePermanentLockedAccount(goCtx context.Context, msg *type
 }
 
 func (s msgServer) CreatePeriodicVestingAccount(goCtx context.Context, msg *types.MsgCreatePeriodicVestingAccount) (*types.MsgCreatePeriodicVestingAccountResponse, error) {
-	ctx := sdk.UnwrapSDKContext(goCtx)
-
-	ak := s.AccountKeeper
-	bk := s.BankKeeper
-
 	from, err := sdk.AccAddressFromBech32(msg.FromAddress)
 	if err != nil {
-		return nil, err
-	}
-	to, err := sdk.AccAddressFromBech32(msg.ToAddress)
-	if err != nil {
-		return nil, err
+		return nil, sdkerrors.ErrInvalidAddress.Wrapf("invalid 'from' address: %s", err)
 	}
 
-	if acc := ak.GetAccount(ctx, to); acc != nil {
-		return nil, errorsmod.Wrapf(sdkerrors.ErrInvalidRequest, "account %s already exists", msg.ToAddress)
+	to, err := sdk.AccAddressFromBech32(msg.ToAddress)
+	if err != nil {
+		return nil, sdkerrors.ErrInvalidAddress.Wrapf("invalid 'to' address: %s", err)
+	}
+
+	if msg.StartTime < 1 {
+		return nil, errorsmod.Wrapf(sdkerrors.ErrInvalidRequest, "invalid start time of %d, length must be greater than 0", msg.StartTime)
 	}
 
 	var totalCoins sdk.Coins
-	for _, period := range msg.VestingPeriods {
+	for i, period := range msg.VestingPeriods {
+		if period.Length < 1 {
+			return nil, errorsmod.Wrapf(sdkerrors.ErrInvalidRequest, "invalid period length of %d in period %d, length must be greater than 0", period.Length, i)
+		}
+
 		totalCoins = totalCoins.Add(period.Amount...)
 	}
 
-	if err := bk.IsSendEnabledCoins(ctx, totalCoins...); err != nil {
+	ctx := sdk.UnwrapSDKContext(goCtx)
+	if acc := s.AccountKeeper.GetAccount(ctx, to); acc != nil {
+		return nil, errorsmod.Wrapf(sdkerrors.ErrInvalidRequest, "account %s already exists", msg.ToAddress)
+	}
+
+	if err := s.BankKeeper.IsSendEnabledCoins(ctx, totalCoins...); err != nil {
 		return nil, err
 	}
 
 	baseAccount := authtypes.NewBaseAccountWithAddress(to)
-	baseAccount = ak.NewAccount(ctx, baseAccount).(*authtypes.BaseAccount)
+	baseAccount = s.AccountKeeper.NewAccount(ctx, baseAccount).(*authtypes.BaseAccount)
 	vestingAccount := types.NewPeriodicVestingAccount(baseAccount, totalCoins.Sort(), msg.StartTime, msg.VestingPeriods)
 
-	ak.SetAccount(ctx, vestingAccount)
+	s.AccountKeeper.SetAccount(ctx, vestingAccount)
 
 	defer func() {
 		telemetry.IncrCounter(1, "new", "account")
@@ -189,9 +202,21 @@ func (s msgServer) CreatePeriodicVestingAccount(goCtx context.Context, msg *type
 		}
 	}()
 
-	if err = bk.SendCoins(ctx, from, to, totalCoins); err != nil {
+	if err = s.BankKeeper.SendCoins(ctx, from, to, totalCoins); err != nil {
 		return nil, err
 	}
 
 	return &types.MsgCreatePeriodicVestingAccountResponse{}, nil
 }
+
+func validateAmount(amount sdk.Coins) error {
+	if !amount.IsValid() {
+		return sdkerrors.ErrInvalidCoins.Wrap(amount.String())
+	}
+
+	if !amount.IsAllPositive() {
+		return sdkerrors.ErrInvalidCoins.Wrap(amount.String())
+	}
+
+	return nil
+}

x/auth/vesting/msg_server_test.go

@@ -72,6 +72,39 @@ func (s *VestingTestSuite) TestCreateVestingAccount() {
 		expErr    bool
 		expErrMsg string
 	}{
+		"empty from address": {
+			input: vestingtypes.NewMsgCreateVestingAccount(
+				[]byte{},
+				to1Addr,
+				sdk.Coins{fooCoin},
+				time.Now().Unix(),
+				true,
+			),
+			expErr:    true,
+			expErrMsg: "invalid 'from' address",
+		},
+		"empty to address": {
+			input: vestingtypes.NewMsgCreateVestingAccount(
+				fromAddr,
+				[]byte{},
+				sdk.Coins{fooCoin},
+				time.Now().Unix(),
+				true,
+			),
+			expErr:    true,
+			expErrMsg: "invalid 'to' address",
+		},
+		"": {
+			input: vestingtypes.NewMsgCreateVestingAccount(
+				fromAddr,
+				to1Addr,
+				sdk.Coins{fooCoin},
+				-10,
+				true,
+			),
+			expErr:    true,
+			expErrMsg: "invalid end time",
+		},
 		"create for existing account": {
 			preRun: func() {
 				toAcc := s.accountKeeper.NewAccountWithAddress(s.ctx, to1Addr)
@@ -125,7 +158,9 @@ func (s *VestingTestSuite) TestCreateVestingAccount() {
 
 	for name, tc := range testCases {
 		s.Run(name, func() {
-			tc.preRun()
+			if tc.preRun != nil {
+				tc.preRun()
+			}
 			_, err := s.msgServer.CreateVestingAccount(s.ctx, tc.input)
 			if tc.expErr {
 				s.Require().Error(err)
@@ -144,6 +179,24 @@ func (s *VestingTestSuite) TestCreatePermanentLockedAccount() {
 		expErr    bool
 		expErrMsg string
 	}{
+		"empty from address": {
+			input: vestingtypes.NewMsgCreatePermanentLockedAccount(
+				[]byte{},
+				to1Addr,
+				sdk.Coins{fooCoin},
+			),
+			expErr:    true,
+			expErrMsg: "invalid 'from' address",
+		},
+		"empty to address": {
+			input: vestingtypes.NewMsgCreatePermanentLockedAccount(
+				fromAddr,
+				[]byte{},
+				sdk.Coins{fooCoin},
+			),
+			expErr:    true,
+			expErrMsg: "invalid 'to' address",
+		},
 		"create for existing account": {
 			preRun: func() {
 				toAcc := s.accountKeeper.NewAccountWithAddress(s.ctx, to1Addr)
@@ -177,7 +230,10 @@ func (s *VestingTestSuite) TestCreatePermanentLockedAccount() {
 
 	for name, tc := range testCases {
 		s.Run(name, func() {
-			tc.preRun()
+			if tc.preRun != nil {
+				tc.preRun()
+			}
+
 			_, err := s.msgServer.CreatePermanentLockedAccount(s.ctx, tc.input)
 			if tc.expErr {
 				s.Require().Error(err)
@@ -197,6 +253,70 @@ func (s *VestingTestSuite) TestCreatePeriodicVestingAccount() {
 		expErr    bool
 		expErrMsg string
 	}{
+		{
+			name: "empty from address",
+			input: vestingtypes.NewMsgCreatePeriodicVestingAccount(
+				[]byte{},
+				to1Addr,
+				time.Now().Unix(),
+				[]vestingtypes.Period{
+					{
+						Length: 10,
+						Amount: sdk.NewCoins(periodCoin),
+					},
+				},
+			),
+			expErr:    true,
+			expErrMsg: "invalid 'from' address",
+		},
+		{
+			name: "empty to address",
+			input: vestingtypes.NewMsgCreatePeriodicVestingAccount(
+				fromAddr,
+				[]byte{},
+				time.Now().Unix(),
+				[]vestingtypes.Period{
+					{
+						Length: 10,
+						Amount: sdk.NewCoins(periodCoin),
+					},
+				},
+			),
+			expErr:    true,
+			expErrMsg: "invalid 'to' address",
+		},
+		{
+			name: "invalid start time",
+			input: vestingtypes.NewMsgCreatePeriodicVestingAccount(
+				fromAddr,
+				to1Addr,
+				0,
+				[]vestingtypes.Period{
+					{
+						Length: 10,
+						Amount: sdk.NewCoins(periodCoin),
+					},
+				},
+			),
+			expErr:    true,
+			expErrMsg: "invalid start time",
+		},
+		{
+			name: "invalid period",
+			input: vestingtypes.NewMsgCreatePeriodicVestingAccount(
+				fromAddr,
+				to1Addr,
+				time.Now().Unix(),
+				[]vestingtypes.Period{
+					{
+						Length: 0,
+						Amount: sdk.NewCoins(periodCoin),
+					},
+				},
+			),
+			expErr:    true,
+			expErrMsg: "invalid period",
+		},
 		{
 			name: "create for existing account",
 			preRun: func() {
@@ -245,7 +365,9 @@ func (s *VestingTestSuite) TestCreatePeriodicVestingAccount() {
 
 	for _, tc := range testCases {
 		s.Run(tc.name, func() {
-			tc.preRun()
+			if tc.preRun != nil {
+				tc.preRun()
+			}
 			_, err := s.msgServer.CreatePeriodicVestingAccount(s.ctx, tc.input)
 			if tc.expErr {
 				s.Require().Error(err)

x/auth/vesting/types/msgs.go

@@ -1,12 +1,7 @@
 package types
 
 import (
-	"fmt"
-
-	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"
 )
 
@@ -31,30 +26,6 @@ func NewMsgCreateVestingAccount(fromAddr, toAddr sdk.AccAddress, amount sdk.Coin
 	}
 }
 
-// ValidateBasic Implements Msg.
-func (msg MsgCreateVestingAccount) ValidateBasic() error {
-	if _, err := sdk.AccAddressFromBech32(msg.FromAddress); err != nil {
-		return sdkerrors.ErrInvalidAddress.Wrapf("invalid 'from' address: %s", err)
-	}
-	if _, err := sdk.AccAddressFromBech32(msg.ToAddress); err != nil {
-		return sdkerrors.ErrInvalidAddress.Wrapf("invalid 'to' address: %s", err)
-	}
-
-	if !msg.Amount.IsValid() {
-		return errorsmod.Wrap(sdkerrors.ErrInvalidCoins, msg.Amount.String())
-	}
-
-	if !msg.Amount.IsAllPositive() {
-		return errorsmod.Wrap(sdkerrors.ErrInvalidCoins, msg.Amount.String())
-	}
-
-	if msg.EndTime <= 0 {
-		return errorsmod.Wrap(sdkerrors.ErrInvalidRequest, "invalid end time")
-	}
-
-	return nil
-}
-
 // GetSignBytes returns the bytes all expected signers must sign over for a
 // MsgCreateVestingAccount.
 func (msg MsgCreateVestingAccount) GetSignBytes() []byte {
@@ -76,26 +47,6 @@ func NewMsgCreatePermanentLockedAccount(fromAddr, toAddr sdk.AccAddress, amount
 	}
 }
 
-// ValidateBasic Implements Msg.
-func (msg MsgCreatePermanentLockedAccount) ValidateBasic() error {
-	if _, err := sdk.AccAddressFromBech32(msg.FromAddress); err != nil {
-		return sdkerrors.ErrInvalidAddress.Wrapf("invalid sender address: %s", err)
-	}
-	if _, err := sdk.AccAddressFromBech32(msg.ToAddress); err != nil {
-		return sdkerrors.ErrInvalidAddress.Wrapf("invalid recipient address: %s", err)
-	}
-
-	if !msg.Amount.IsValid() {
-		return sdkerrors.ErrInvalidCoins.Wrap(msg.Amount.String())
-	}
-
-	if !msg.Amount.IsAllPositive() {
-		return sdkerrors.ErrInvalidCoins.Wrap(msg.Amount.String())
-	}
-
-	return nil
-}
-
 // GetSignBytes returns the bytes all expected signers must sign over for a
 // MsgCreatePermanentLockedAccount.
 func (msg MsgCreatePermanentLockedAccount) GetSignBytes() []byte {
@@ -132,34 +83,3 @@ func (msg MsgCreatePeriodicVestingAccount) GetSigners() []sdk.AccAddress {
 func (msg MsgCreatePeriodicVestingAccount) GetSignBytes() []byte {
 	return sdk.MustSortJSON(ModuleCdc.MustMarshalJSON(&msg))
 }
-
-// ValidateBasic Implements Msg.
-func (msg MsgCreatePeriodicVestingAccount) ValidateBasic() error {
-	from, err := sdk.AccAddressFromBech32(msg.FromAddress)
-	if err != nil {
-		return err
-	}
-	to, err := sdk.AccAddressFromBech32(msg.ToAddress)
-	if err != nil {
-		return err
-	}
-	if err := sdk.VerifyAddressFormat(from); err != nil {
-		return errorsmod.Wrapf(sdkerrors.ErrInvalidAddress, "invalid sender address: %s", err)
-	}
-
-	if err := sdk.VerifyAddressFormat(to); err != nil {
-		return errorsmod.Wrapf(sdkerrors.ErrInvalidAddress, "invalid recipient address: %s", err)
-	}
-
-	if msg.StartTime < 1 {
-		return fmt.Errorf("invalid start time of %d, length must be greater than 0", msg.StartTime)
-	}
-
-	for i, period := range msg.VestingPeriods {
-		if period.Length < 1 {
-			return fmt.Errorf("invalid period length of %d in period %d, length must be greater than 0", period.Length, i)
-		}
-	}
-
-	return nil
-}

x/crisis/keeper/msg_server.go

@@ -69,25 +69,21 @@ func (k *Keeper) VerifyInvariant(goCtx context.Context, msg *types.MsgVerifyInva
 
 // UpdateParams implements MsgServer.UpdateParams method.
 // It defines a method to update the x/crisis module parameters.
-func (k *Keeper) UpdateParams(goCtx context.Context, req *types.MsgUpdateParams) (*types.MsgUpdateParamsResponse, error) {
-	if _, err := sdk.AccAddressFromBech32(req.Authority); err != nil {
-		return nil, errors.Wrap(err, "invalid authority address")
+func (k *Keeper) UpdateParams(ctx 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 k.authority != req.Authority {
-		return nil, errors.Wrapf(govtypes.ErrInvalidSigner, "invalid authority; expected %s, got %s", k.authority, req.Authority)
-	}
-
-	if !req.ConstantFee.IsValid() {
+	if !msg.ConstantFee.IsValid() {
 		return nil, errors.Wrap(sdkerrors.ErrInvalidCoins, "invalid constant fee")
 	}
 
-	if req.ConstantFee.IsNegative() {
+	if msg.ConstantFee.IsNegative() {
 		return nil, errors.Wrap(sdkerrors.ErrInvalidCoins, "negative constant fee")
 	}
 
-	ctx := sdk.UnwrapSDKContext(goCtx)
-	if err := k.SetConstantFee(ctx, req.ConstantFee); err != nil {
+	sdkCtx := sdk.UnwrapSDKContext(ctx)
+	if err := k.SetConstantFee(sdkCtx, msg.ConstantFee); err != nil {
 		return nil, err
 	}
 

x/distribution/keeper/params.go

@@ -19,12 +19,9 @@ func (k Keeper) GetParams(ctx sdk.Context) (params types.Params) {
 	return params
 }
 
-// SetParams sets the distribution parameters to the param space.
+// SetParams sets the distribution parameters.
+// CONTRACT: This method performs no validation of the parameters.
 func (k Keeper) SetParams(ctx sdk.Context, params types.Params) error {
-	if err := params.ValidateBasic(); err != nil {
-		return err
-	}
-
 	store := ctx.KVStore(k.storeKey)
 	bz, err := k.cdc.Marshal(&params)
 	if err != nil {

x/distribution/keeper/params_test.go

@@ -1,128 +0,0 @@
-package keeper_test
-
-import (
-	"testing"
-
-	sdkmath "cosmossdk.io/math"
-	storetypes "cosmossdk.io/store/types"
-	cmtproto "github.com/cometbft/cometbft/proto/tendermint/types"
-	"github.com/golang/mock/gomock"
-	"github.com/stretchr/testify/require"
-
-	"github.com/cosmos/cosmos-sdk/testutil"
-	moduletestutil "github.com/cosmos/cosmos-sdk/types/module/testutil"
-	authtypes "github.com/cosmos/cosmos-sdk/x/auth/types"
-	"github.com/cosmos/cosmos-sdk/x/distribution"
-	"github.com/cosmos/cosmos-sdk/x/distribution/keeper"
-	distrtestutil "github.com/cosmos/cosmos-sdk/x/distribution/testutil"
-	"github.com/cosmos/cosmos-sdk/x/distribution/types"
-)
-
-func TestParams(t *testing.T) {
-	ctrl := gomock.NewController(t)
-	key := storetypes.NewKVStoreKey(types.StoreKey)
-	testCtx := testutil.DefaultContextWithDB(t, key, storetypes.NewTransientStoreKey("transient_test"))
-	encCfg := moduletestutil.MakeTestEncodingConfig(distribution.AppModuleBasic{})
-	ctx := testCtx.Ctx.WithBlockHeader(cmtproto.Header{Height: 1})
-
-	bankKeeper := distrtestutil.NewMockBankKeeper(ctrl)
-	stakingKeeper := distrtestutil.NewMockStakingKeeper(ctrl)
-	accountKeeper := distrtestutil.NewMockAccountKeeper(ctrl)
-
-	accountKeeper.EXPECT().GetModuleAddress("distribution").Return(distrAcc.GetAddress())
-
-	distrKeeper := keeper.NewKeeper(
-		encCfg.Codec,
-		key,
-		accountKeeper,
-		bankKeeper,
-		stakingKeeper,
-		"fee_collector",
-		authtypes.NewModuleAddress("gov").String(),
-	)
-
-	// default params
-	communityTax := sdkmath.LegacyNewDecWithPrec(2, 2) // 2%
-	withdrawAddrEnabled := true
-
-	testCases := []struct {
-		name      string
-		input     types.Params
-		expErr    bool
-		expErrMsg string
-	}{
-		{
-			name: "community tax > 1",
-			input: types.Params{
-				CommunityTax:        sdkmath.LegacyNewDecWithPrec(2, 0),
-				BaseProposerReward:  sdkmath.LegacyZeroDec(),
-				BonusProposerReward: sdkmath.LegacyZeroDec(),
-				WithdrawAddrEnabled: withdrawAddrEnabled,
-			},
-			expErr:    true,
-			expErrMsg: "community tax should be non-negative and less than one",
-		},
-		{
-			name: "negative community tax",
-			input: types.Params{
-				CommunityTax:        sdkmath.LegacyNewDecWithPrec(-2, 1),
-				BaseProposerReward:  sdkmath.LegacyZeroDec(),
-				BonusProposerReward: sdkmath.LegacyZeroDec(),
-				WithdrawAddrEnabled: withdrawAddrEnabled,
-			},
-			expErr:    true,
-			expErrMsg: "community tax should be non-negative and less than one",
-		},
-		{
-			name: "base proposer reward > 1",
-			input: types.Params{
-				CommunityTax:        communityTax,
-				BaseProposerReward:  sdkmath.LegacyNewDecWithPrec(1, 2),
-				BonusProposerReward: sdkmath.LegacyZeroDec(),
-				WithdrawAddrEnabled: withdrawAddrEnabled,
-			},
-			expErr:    false,
-			expErrMsg: "base proposer rewards should not be taken into account",
-		},
-		{
-			name: "bonus proposer reward > 1",
-			input: types.Params{
-				CommunityTax:        communityTax,
-				BaseProposerReward:  sdkmath.LegacyNewDecWithPrec(1, 2),
-				BonusProposerReward: sdkmath.LegacyZeroDec(),
-				WithdrawAddrEnabled: withdrawAddrEnabled,
-			},
-			expErr:    false,
-			expErrMsg: "bonus proposer rewards should not be taken into account",
-		},
-		{
-			name: "all good",
-			input: types.Params{
-				CommunityTax:        communityTax,
-				BaseProposerReward:  sdkmath.LegacyZeroDec(),
-				BonusProposerReward: sdkmath.LegacyZeroDec(),
-				WithdrawAddrEnabled: withdrawAddrEnabled,
-			},
-			expErr: false,
-		},
-	}
-
-	for _, tc := range testCases {
-		tc := tc
-		t.Run(tc.name, func(t *testing.T) {
-			expected := distrKeeper.GetParams(ctx)
-			err := distrKeeper.SetParams(ctx, tc.input)
-
-			if tc.expErr {
-				require.Error(t, err)
-				require.Contains(t, err.Error(), tc.expErrMsg)
-			} else {
-				expected = tc.input
-				require.NoError(t, err)
-			}
-
-			params := distrKeeper.GetParams(ctx)
-			require.Equal(t, expected, params)
-		})
-	}
-}

x/gov/keeper/params.go

@@ -7,6 +7,7 @@ import (
 )
 
 // SetParams sets the gov module's parameters.
+// CONTRACT: This method performs no validation of the parameters.
 func (k Keeper) SetParams(ctx sdk.Context, params v1.Params) error {
 	store := ctx.KVStore(k.storeKey)
 	bz, err := k.cdc.Marshal(&params)

x/mint/keeper/keeper.go

@@ -81,13 +81,9 @@ func (k Keeper) SetMinter(ctx sdk.Context, minter types.Minter) {
 }
 
 // SetParams sets the x/mint module parameters.
-func (k Keeper) SetParams(ctx sdk.Context, p types.Params) error {
-	if err := p.Validate(); err != nil {
-		return err
-	}
-
+func (k Keeper) SetParams(ctx sdk.Context, params types.Params) error {
 	store := ctx.KVStore(k.storeKey)
-	bz := k.cdc.MustMarshal(&p)
+	bz := k.cdc.MustMarshal(&params)
 	store.Set(types.ParamsKey, bz)
 
 	return nil

x/mint/keeper/keeper_test.go

@@ -76,7 +76,7 @@ func (s *IntegrationTestSuite) TestParams() {
 		expectErr bool
 	}{
 		{
-			name: "set invalid params",
+			name: "set invalid params (⚠️ not validated in keeper)",
 			input: types.Params{
 				MintDenom:           sdk.DefaultBondDenom,
 				InflationRateChange: sdk.NewDecWithPrec(-13, 2),
@@ -85,7 +85,7 @@ func (s *IntegrationTestSuite) TestParams() {
 				GoalBonded:          sdk.NewDecWithPrec(67, 2),
 				BlocksPerYear:       uint64(60 * 60 * 8766 / 5),
 			},
-			expectErr: true,
+			expectErr: false,
 		},
 		{
 			name: "set full valid params",

x/mint/keeper/msg_server.go

@@ -26,10 +26,6 @@ func NewMsgServerImpl(k Keeper) types.MsgServer {
 
 // UpdateParams updates the params.
 func (ms msgServer) UpdateParams(goCtx context.Context, msg *types.MsgUpdateParams) (*types.MsgUpdateParamsResponse, error) {
-	if _, err := sdk.AccAddressFromBech32(msg.Authority); err != nil {
-		return nil, errors.Wrap(err, "invalid authority address")
-	}
-
 	if ms.authority != msg.Authority {
 		return nil, errors.Wrapf(govtypes.ErrInvalidSigner, "invalid authority; expected %s, got %s", ms.authority, msg.Authority)
 	}

x/upgrade/keeper/msg_server.go

@@ -25,21 +25,17 @@ func NewMsgServerImpl(k *Keeper) types.MsgServer {
 var _ types.MsgServer = msgServer{}
 
 // SoftwareUpgrade implements the Msg/SoftwareUpgrade Msg service.
-func (k msgServer) SoftwareUpgrade(goCtx context.Context, req *types.MsgSoftwareUpgrade) (*types.MsgSoftwareUpgradeResponse, error) {
-	if _, err := sdk.AccAddressFromBech32(req.Authority); err != nil {
-		return nil, errors.Wrap(err, "authority")
+func (k msgServer) SoftwareUpgrade(goCtx context.Context, msg *types.MsgSoftwareUpgrade) (*types.MsgSoftwareUpgradeResponse, error) {
+	if k.authority != msg.Authority {
+		return nil, errors.Wrapf(gov.ErrInvalidSigner, "expected %s got %s", k.authority, msg.Authority)
 	}
 
-	if k.authority != req.Authority {
-		return nil, errors.Wrapf(gov.ErrInvalidSigner, "expected %s got %s", k.authority, req.Authority)
-	}
-
-	if err := req.Plan.ValidateBasic(); err != nil {
+	if err := msg.Plan.ValidateBasic(); err != nil {
 		return nil, errors.Wrap(err, "plan")
 	}
 
 	ctx := sdk.UnwrapSDKContext(goCtx)
-	err := k.ScheduleUpgrade(ctx, req.Plan)
+	err := k.ScheduleUpgrade(ctx, msg.Plan)
 	if err != nil {
 		return nil, err
 	}
@@ -48,17 +44,13 @@ func (k msgServer) SoftwareUpgrade(goCtx context.Context, req *types.MsgSoftware
 }
 
 // CancelUpgrade implements the Msg/CancelUpgrade Msg service.
-func (k msgServer) CancelUpgrade(goCtx context.Context, req *types.MsgCancelUpgrade) (*types.MsgCancelUpgradeResponse, error) {
-	if _, err := sdk.AccAddressFromBech32(req.Authority); err != nil {
-		return nil, errors.Wrap(err, "authority")
+func (k msgServer) CancelUpgrade(ctx context.Context, msg *types.MsgCancelUpgrade) (*types.MsgCancelUpgradeResponse, error) {
+	if k.authority != msg.Authority {
+		return nil, errors.Wrapf(gov.ErrInvalidSigner, "expected %s got %s", k.authority, msg.Authority)
 	}
 
-	if k.authority != req.Authority {
-		return nil, errors.Wrapf(gov.ErrInvalidSigner, "expected %s got %s", k.authority, req.Authority)
-	}
-
-	ctx := sdk.UnwrapSDKContext(goCtx)
-	k.ClearUpgradePlan(ctx)
+	sdkCtx := sdk.UnwrapSDKContext(ctx)
+	k.ClearUpgradePlan(sdkCtx)
 
 	return &types.MsgCancelUpgradeResponse{}, nil
 }

x/upgrade/keeper/msg_server_test.go

@@ -25,7 +25,7 @@ func (s *KeeperTestSuite) TestSoftwareUpgrade() {
 				},
 			},
 			true,
-			"authority: decoding bech32 failed",
+			"expected gov account as only signer for proposal message",
 		},
 		{
 			"unauthorized authority address",
@@ -103,7 +103,7 @@ func (s *KeeperTestSuite) TestCancelUpgrade() {
 				Authority: "authority",
 			},
 			true,
-			"authority: decoding bech32 failed",
+			"expected gov account as only signer for proposal message",
 		},
 		{
 			"unauthorized authority address",