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

…5786)

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 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 bk.BlockedAddr(to) {
+ 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,44 +87,46 @@ 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
  }
 
  return &types.MsgCreateVestingAccountResponse{}, nil
 }
 
 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,46 +142,51 @@ 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
  }
 
  return &types.MsgCreatePermanentLockedAccountResponse{}, nil
 }
 
 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
+ return nil, sdkerrors.ErrInvalidAddress.Wrapf("invalid 'from' address: %s", err)
  }
+
  to, err := sdk.AccAddressFromBech32(msg.ToAddress)
  if err != nil {
- return nil, err
+ return nil, sdkerrors.ErrInvalidAddress.Wrapf("invalid 'to' address: %s", err)
  }
 
- if acc := ak.GetAccount(ctx, to); acc != nil {
- return nil, errorsmod.Wrapf(sdkerrors.ErrInvalidRequest, "account %s already exists", msg.ToAddress)
+ 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
+}