From 60e37922807665940e637ef6bb1a7347c6440285 Mon Sep 17 00:00:00 2001 From: Bryan White Date: Mon, 7 Oct 2024 12:12:07 +0200 Subject: [PATCH 1/2] chore: refresh service module params impl. to match updated docs --- x/service/keeper/msg_server_update_param.go | 46 ++++++++++----- .../keeper/msg_server_update_param_test.go | 2 +- x/service/keeper/msg_update_params_test.go | 4 +- x/service/keeper/params_test.go | 34 ++++++++++- x/service/types/errors.go | 2 - x/service/types/genesis_test.go | 2 +- x/service/types/message_update_param.go | 25 ++++---- x/service/types/message_update_param_test.go | 57 +++++++++++++++++++ x/service/types/params.go | 23 ++++---- 9 files changed, 149 insertions(+), 46 deletions(-) create mode 100644 x/service/types/message_update_param_test.go diff --git a/x/service/keeper/msg_server_update_param.go b/x/service/keeper/msg_server_update_param.go index d2d224dba..6c5a6070d 100644 --- a/x/service/keeper/msg_server_update_param.go +++ b/x/service/keeper/msg_server_update_param.go @@ -3,6 +3,9 @@ package keeper import ( "context" + "google.golang.org/grpc/codes" + "google.golang.org/grpc/status" + "github.com/pokt-network/poktroll/x/service/types" ) @@ -12,38 +15,51 @@ func (k msgServer) UpdateParam( ctx context.Context, msg *types.MsgUpdateParam, ) (*types.MsgUpdateParamResponse, error) { + logger := k.logger.With( + "method", "UpdateParam", + "param_name", msg.Name, + ) + if err := msg.ValidateBasic(); err != nil { - return nil, err + return nil, status.Error(codes.InvalidArgument, err.Error()) } if k.GetAuthority() != msg.Authority { - return nil, types.ErrServiceInvalidSigner.Wrapf("invalid authority; expected %s, got %s", k.GetAuthority(), msg.Authority) + return nil, status.Error( + codes.InvalidArgument, + types.ErrServiceInvalidSigner.Wrapf( + "invalid authority; expected %s, got %s", + k.GetAuthority(), msg.Authority, + ).Error(), + ) } params := k.GetParams(ctx) switch msg.Name { case types.ParamAddServiceFee: - value, ok := msg.AsType.(*types.MsgUpdateParam_AsCoin) - if !ok { - return nil, types.ErrServiceParamInvalid.Wrapf("unsupported value type for %s param: %T", msg.Name, msg.AsType) - } - addServiceFee := value.AsCoin - - if err := types.ValidateAddServiceFee(addServiceFee); err != nil { - return nil, err - } - - params.AddServiceFee = addServiceFee + logger = logger.With("param_value", msg.GetAsCoin()) + params.AddServiceFee = msg.GetAsCoin() default: - return nil, types.ErrServiceParamInvalid.Wrapf("unsupported param %q", msg.Name) + return nil, status.Error( + codes.InvalidArgument, + types.ErrServiceParamInvalid.Wrapf("unsupported param %q", msg.Name).Error(), + ) + } + + // Perform a global validation on all params, which includes the updated param. + // This is needed to ensure that the updated param is valid in the context of all other params. + if err := params.ValidateBasic(); err != nil { + return nil, status.Error(codes.InvalidArgument, err.Error()) } if err := k.SetParams(ctx, params); err != nil { - return nil, err + k.logger.Info("ERROR: %s", err) + return nil, status.Error(codes.Internal, err.Error()) } updatedParams := k.GetParams(ctx) + return &types.MsgUpdateParamResponse{ Params: &updatedParams, }, nil diff --git a/x/service/keeper/msg_server_update_param_test.go b/x/service/keeper/msg_server_update_param_test.go index 74c038d20..67c2eb979 100644 --- a/x/service/keeper/msg_server_update_param_test.go +++ b/x/service/keeper/msg_server_update_param_test.go @@ -14,7 +14,7 @@ import ( servicetypes "github.com/pokt-network/poktroll/x/service/types" ) -func TestMsgUpdateParam_UpdateAddServiceFee(t *testing.T) { +func TestMsgUpdateParam_UpdateAddServiceFeeOnly(t *testing.T) { expectedAddServiceFee := &sdk.Coin{Denom: volatile.DenomuPOKT, Amount: math.NewInt(1000000001)} // Set the parameters to their default values diff --git a/x/service/keeper/msg_update_params_test.go b/x/service/keeper/msg_update_params_test.go index e2340e064..e473aab7d 100644 --- a/x/service/keeper/msg_update_params_test.go +++ b/x/service/keeper/msg_update_params_test.go @@ -30,13 +30,13 @@ func TestMsgUpdateParams(t *testing.T) { expectedErrMsg: "invalid authority", }, { - desc: "send empty params", + desc: "invalid: send empty params", input: &types.MsgUpdateParams{ Authority: k.GetAuthority(), Params: types.Params{}, }, shouldError: true, - expectedErrMsg: "invalid ServiceFee", + expectedErrMsg: "missing add_service_fee", }, { desc: "valid: send default params", diff --git a/x/service/keeper/params_test.go b/x/service/keeper/params_test.go index 2c7100aef..556f1de0f 100644 --- a/x/service/keeper/params_test.go +++ b/x/service/keeper/params_test.go @@ -6,13 +6,43 @@ import ( "github.com/stretchr/testify/require" keepertest "github.com/pokt-network/poktroll/testutil/keeper" - "github.com/pokt-network/poktroll/x/service/types" + servicetypes "github.com/pokt-network/poktroll/x/service/types" ) func TestGetParams(t *testing.T) { k, ctx := keepertest.ServiceKeeper(t) - params := types.DefaultParams() + params := servicetypes.DefaultParams() require.NoError(t, k.SetParams(ctx, params)) require.EqualValues(t, params, k.GetParams(ctx)) } + +func TestParams_ValidateAddServiceFee(t *testing.T) { + tests := []struct { + desc string + addServiceFee any + expectedErr error + }{ + { + desc: "invalid type", + addServiceFee: "100upokt", + expectedErr: servicetypes.ErrServiceParamInvalid, + }, + { + desc: "valid AddServiceFee", + addServiceFee: &servicetypes.MinAddServiceFee, + }, + } + + for _, test := range tests { + t.Run(test.desc, func(t *testing.T) { + err := servicetypes.ValidateAddServiceFee(test.addServiceFee) + if test.expectedErr != nil { + require.Error(t, err) + require.Contains(t, err.Error(), test.expectedErr.Error()) + } else { + require.NoError(t, err) + } + }) + } +} diff --git a/x/service/types/errors.go b/x/service/types/errors.go index d593be500..aaab0d6d5 100644 --- a/x/service/types/errors.go +++ b/x/service/types/errors.go @@ -12,13 +12,11 @@ var ( ErrServiceMissingID = sdkerrors.Register(ModuleName, 1103, "missing service ID") ErrServiceMissingName = sdkerrors.Register(ModuleName, 1104, "missing service name") ErrServiceAlreadyExists = sdkerrors.Register(ModuleName, 1105, "service already exists") - ErrServiceInvalidServiceFee = sdkerrors.Register(ModuleName, 1106, "invalid ServiceFee") ErrServiceAccountNotFound = sdkerrors.Register(ModuleName, 1107, "account not found") ErrServiceNotEnoughFunds = sdkerrors.Register(ModuleName, 1108, "not enough funds to add service") ErrServiceFailedToDeductFee = sdkerrors.Register(ModuleName, 1109, "failed to deduct fee") ErrServiceInvalidRelayResponse = sdkerrors.Register(ModuleName, 1110, "invalid relay response") ErrServiceInvalidRelayRequest = sdkerrors.Register(ModuleName, 1111, "invalid relay request") ErrServiceInvalidOwnerAddress = sdkerrors.Register(ModuleName, 1113, "invalid owner address") - ErrServiceParamNameInvalid = sdkerrors.Register(ModuleName, 1114, "the provided param name is invalid") ErrServiceParamInvalid = sdkerrors.Register(ModuleName, 1115, "the provided param is invalid") ) diff --git a/x/service/types/genesis_test.go b/x/service/types/genesis_test.go index aa90380bc..6b4b4b77a 100644 --- a/x/service/types/genesis_test.go +++ b/x/service/types/genesis_test.go @@ -79,7 +79,7 @@ func TestGenesisState_Validate(t *testing.T) { *svc1, *svc2, }, }, - expectedErr: types.ErrServiceInvalidServiceFee, + expectedErr: types.ErrServiceParamInvalid, }, // this line is used by starport scaffolding # types/genesis/testcase } diff --git a/x/service/types/message_update_param.go b/x/service/types/message_update_param.go index 04c7db8f6..7d3ad2c15 100644 --- a/x/service/types/message_update_param.go +++ b/x/service/types/message_update_param.go @@ -1,8 +1,6 @@ package types import ( - "fmt" - sdk "github.com/cosmos/cosmos-sdk/types" ) @@ -10,20 +8,20 @@ var _ sdk.Msg = (*MsgUpdateParam)(nil) // NewMsgUpdateParam creates a new MsgUpdateParam instance for a single // governance parameter update. -func NewMsgUpdateParam(authority string, name string, value any) (*MsgUpdateParam, error) { - var valueAsType isMsgUpdateParam_AsType +func NewMsgUpdateParam(authority string, name string, asType any) (*MsgUpdateParam, error) { + var asTypeIface isMsgUpdateParam_AsType - switch v := value.(type) { + switch t := asType.(type) { case *sdk.Coin: - valueAsType = &MsgUpdateParam_AsCoin{AsCoin: v} + asTypeIface = &MsgUpdateParam_AsCoin{AsCoin: t} default: - return nil, fmt.Errorf("unexpected param value type: %T", value) + return nil, ErrServiceParamInvalid.Wrapf("unexpected param value type: %T", asType) } return &MsgUpdateParam{ Authority: authority, Name: name, - AsType: valueAsType, + AsType: asTypeIface, }, nil } @@ -36,17 +34,20 @@ func (msg *MsgUpdateParam) ValidateBasic() error { return ErrServiceInvalidAddress.Wrapf("invalid authority address %s; (%v)", msg.Authority, err) } - // Parameter value cannot be nil. + // Parameter value MUST NOT be nil. if msg.AsType == nil { return ErrServiceParamInvalid.Wrap("missing param AsType") } - // Parameter name must be supported by this module. + // Parameter name MUST be supported by this module. switch msg.Name { case ParamAddServiceFee: - return msg.paramTypeIsCoin() + if err := msg.paramTypeIsCoin(); err != nil { + return err + } + return ValidateAddServiceFee(msg.GetAsCoin()) default: - return ErrServiceParamNameInvalid.Wrapf("unsupported param %q", msg.Name) + return ErrServiceParamInvalid.Wrapf("unsupported param %q", msg.Name) } } diff --git a/x/service/types/message_update_param_test.go b/x/service/types/message_update_param_test.go new file mode 100644 index 000000000..75a45c41a --- /dev/null +++ b/x/service/types/message_update_param_test.go @@ -0,0 +1,57 @@ +package types + +import ( + "testing" + + sdkerrors "github.com/cosmos/cosmos-sdk/types/errors" + "github.com/stretchr/testify/require" + + "github.com/pokt-network/poktroll/testutil/sample" +) + +func TestMsgUpdateParam_ValidateBasic(t *testing.T) { + tests := []struct { + name string + desc string + msg MsgUpdateParam + expectedErr error + }{ + { + name: "invalid address", + desc: "invalid: authority address invalid", + msg: MsgUpdateParam{ + Authority: "invalid_address", + Name: "", // Doesn't matter for this test + AsType: &MsgUpdateParam_AsCoin{AsCoin: nil}, + }, + expectedErr: sdkerrors.ErrInvalidAddress, + }, { + desc: "invalid: param name incorrect (non-existent)", + msg: MsgUpdateParam{ + Authority: sample.AccAddress(), + Name: "non_existent", + AsType: &MsgUpdateParam_AsCoin{AsCoin: &MinAddServiceFee}, + }, + expectedErr: ErrServiceParamInvalid, + }, { + name: "valid address", + desc: "valid: correct address, param name, and type", + msg: MsgUpdateParam{ + Authority: sample.AccAddress(), + Name: ParamAddServiceFee, + AsType: &MsgUpdateParam_AsCoin{AsCoin: &MinAddServiceFee}, + }, + }, + } + + for _, test := range tests { + t.Run(test.desc, func(t *testing.T) { + err := test.msg.ValidateBasic() + if test.expectedErr != nil { + require.ErrorContains(t, err, test.expectedErr.Error()) + return + } + require.NoError(t, err) + }) + } +} diff --git a/x/service/types/params.go b/x/service/types/params.go index 542eda53f..a454d86f1 100644 --- a/x/service/types/params.go +++ b/x/service/types/params.go @@ -4,6 +4,7 @@ import ( "cosmossdk.io/math" cosmostypes "github.com/cosmos/cosmos-sdk/types" paramtypes "github.com/cosmos/cosmos-sdk/x/params/types" + "github.com/pokt-network/poktroll/app/volatile" ) @@ -55,26 +56,26 @@ func (p Params) ValidateBasic() error { } // validateAddServiceFee validates the AddServiceFee param -func ValidateAddServiceFee(v interface{}) error { - addServiceFeeCoin, ok := v.(*cosmostypes.Coin) +func ValidateAddServiceFee(addServiceFeeAny any) error { + addServiceFee, ok := addServiceFeeAny.(*cosmostypes.Coin) if !ok { - return ErrServiceInvalidServiceFee.Wrapf("invalid parameter type: %T", v) + return ErrServiceParamInvalid.Wrapf("invalid parameter type: %T", addServiceFeeAny) } - if addServiceFeeCoin == nil { - return ErrServiceInvalidServiceFee.Wrap("missing proof_submission_fee") + if addServiceFee == nil { + return ErrServiceParamInvalid.Wrap("missing add_service_fee") } - if addServiceFeeCoin.Denom != volatile.DenomuPOKT { - return ErrServiceInvalidServiceFee.Wrapf("invalid coin denom: %s", addServiceFeeCoin.Denom) + if addServiceFee.Denom != volatile.DenomuPOKT { + return ErrServiceParamInvalid.Wrapf("invalid add_service_fee denom: %s", addServiceFee.Denom) } // TODO_MAINNET: Look into better validation - if addServiceFeeCoin.Amount.LT(MinAddServiceFee.Amount) { - return ErrServiceInvalidServiceFee.Wrapf( - "AddServiceFee param is below minimum value %s: got %s", + if addServiceFee.Amount.LT(MinAddServiceFee.Amount) { + return ErrServiceParamInvalid.Wrapf( + "add_service_fee param is below minimum value %s: got %s", MinAddServiceFee, - addServiceFeeCoin, + addServiceFee, ) } From c5a9b945d577c77d65ade068162013bee50d0ee0 Mon Sep 17 00:00:00 2001 From: Bryan White Date: Mon, 7 Oct 2024 13:35:29 +0200 Subject: [PATCH 2/2] fix: linter error --- x/service/keeper/msg_server_update_param.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/x/service/keeper/msg_server_update_param.go b/x/service/keeper/msg_server_update_param.go index 6c5a6070d..ac1ea6a6c 100644 --- a/x/service/keeper/msg_server_update_param.go +++ b/x/service/keeper/msg_server_update_param.go @@ -54,7 +54,7 @@ func (k msgServer) UpdateParam( } if err := k.SetParams(ctx, params); err != nil { - k.logger.Info("ERROR: %s", err) + logger.Info("ERROR: %s", err) return nil, status.Error(codes.Internal, err.Error()) }