diff --git a/CHANGELOG.md b/CHANGELOG.md index 16317daa03f9..e5273e6c44d6 100644 --- a/CHANGELOG.md +++ b/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`. diff --git a/x/auth/keeper/keeper.go b/x/auth/keeper/keeper.go index df6313f1340e..2111e60babdf 100644 --- a/x/auth/keeper/keeper.go +++ b/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) } diff --git a/x/auth/keeper/msg_server.go b/x/auth/keeper/msg_server.go index 8a7b324fdb21..78e92680f19c 100644 --- a/x/auth/keeper/msg_server.go +++ b/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 } diff --git a/x/auth/types/msgs.go b/x/auth/types/msgs.go index 8774549e9ae1..df6930d5fd4b 100644 --- a/x/auth/types/msgs.go +++ b/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 -} diff --git a/x/auth/vesting/client/cli/tx.go b/x/auth/vesting/client/cli/tx.go index 3c88d04500fa..816916cdb522 100644 --- a/x/auth/vesting/client/cli/tx.go +++ b/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) }, } diff --git a/x/auth/vesting/msg_server.go b/x/auth/vesting/msg_server.go index c1f9254b3b6b..5a68ffefe79d 100644 --- a/x/auth/vesting/msg_server.go +++ b/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,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 + 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 +} diff --git a/x/auth/vesting/msg_server_test.go b/x/auth/vesting/msg_server_test.go index 823d31278bfe..2a121271e10a 100644 --- a/x/auth/vesting/msg_server_test.go +++ b/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) diff --git a/x/auth/vesting/types/msgs.go b/x/auth/vesting/types/msgs.go index edf4e7b468aa..34732a96891b 100644 --- a/x/auth/vesting/types/msgs.go +++ b/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 -} diff --git a/x/crisis/keeper/msg_server.go b/x/crisis/keeper/msg_server.go index ea2aad32f001..79fa138e1a06 100644 --- a/x/crisis/keeper/msg_server.go +++ b/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 } diff --git a/x/distribution/keeper/params.go b/x/distribution/keeper/params.go index 278ec4c1bd87..2ab11eb273a7 100644 --- a/x/distribution/keeper/params.go +++ b/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(¶ms) if err != nil { diff --git a/x/distribution/keeper/params_test.go b/x/distribution/keeper/params_test.go deleted file mode 100644 index bb78c22fa6aa..000000000000 --- a/x/distribution/keeper/params_test.go +++ /dev/null @@ -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) - }) - } -} diff --git a/x/gov/keeper/params.go b/x/gov/keeper/params.go index 9e4aef9dd059..dc8e93212076 100644 --- a/x/gov/keeper/params.go +++ b/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(¶ms) diff --git a/x/mint/keeper/keeper.go b/x/mint/keeper/keeper.go index b21b6be19e0e..56c0664dd90a 100644 --- a/x/mint/keeper/keeper.go +++ b/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(¶ms) store.Set(types.ParamsKey, bz) return nil diff --git a/x/mint/keeper/keeper_test.go b/x/mint/keeper/keeper_test.go index 137dbbc5d23c..f129406a5f38 100644 --- a/x/mint/keeper/keeper_test.go +++ b/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", diff --git a/x/mint/keeper/msg_server.go b/x/mint/keeper/msg_server.go index 23abab87773b..20f9848f6416 100644 --- a/x/mint/keeper/msg_server.go +++ b/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) } diff --git a/x/upgrade/keeper/msg_server.go b/x/upgrade/keeper/msg_server.go index 2f281eba008a..dcf5900b6e24 100644 --- a/x/upgrade/keeper/msg_server.go +++ b/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 } diff --git a/x/upgrade/keeper/msg_server_test.go b/x/upgrade/keeper/msg_server_test.go index a7ae4537e4ad..aba54c5cd8dc 100644 --- a/x/upgrade/keeper/msg_server_test.go +++ b/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",