Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 9 commits into from
Apr 11, 2023
27 changes: 16 additions & 11 deletions x/slashing/keeper/msg_server.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
)
Expand All @@ -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
}

Expand All @@ -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
}

Expand Down
10 changes: 10 additions & 0 deletions x/slashing/keeper/msg_server_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
5 changes: 1 addition & 4 deletions x/slashing/keeper/params.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The change for this is mentioned in a changelog from this PR: #15786.
This makes consistent all SetParams methods, some were validating params whereas some did not.
Now we always validate in the msg server and only there.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we make setParams internal to avoid others accidentally calling it without calling validation or leave a contact comment saying the caller should call validate

Copy link
Member Author

@julienrbrt julienrbrt Apr 11, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You may have a module that updates other module params by calling the keeper, so making it private is a no-go imho.
We could add a comment, indeed.

return err
}

store := ctx.KVStore(k.storeKey)
bz := k.cdc.MustMarshal(&params)
store.Set(types.ParamsKey, bz)
Expand Down
127 changes: 0 additions & 127 deletions x/slashing/keeper/params_test.go

This file was deleted.

24 changes: 0 additions & 24 deletions x/slashing/types/msg.go
Original file line number Diff line number Diff line change
@@ -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"
)

Expand Down Expand Up @@ -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))
Expand All @@ -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
}