From efbebfad59ac85d8f885fc10fa5426737a638e49 Mon Sep 17 00:00:00 2001 From: Bryan White Date: Mon, 14 Oct 2024 22:56:05 +0200 Subject: [PATCH] [Proof] Refresh proof module params logic (#851) ## Summary The "adding_params.md" doc has (or will have) changed substantially (see #839) since the proof module's param logic was last updated. This PR aligns the proof module's param logic with the snippets in the updated docs. Specifically, this refresh includes: - Moving validation logic from `msgServer#UpdateParam()` to `MsgUpdateParam#ValidateBasic()`. - Replacing magic strings of param names with their standard variable counterparts (i.e. `prooftypes.Param...`). - Improving some local variable names. - Replacing usages of `interface{}` with `any`. - Improving validation for all coin type params to be consistent with application, gateway, and supplier coin type validation. # Dependencies - #809 - #843 - #844 - #845 - #847 - #848 - #849 - #850 - #857 - #852 ## Dependents - #861 ## Issue - #612 ## Type of change Select one or more from the following: - [ ] New feature, functionality or library - [ ] Consensus breaking; add the `consensus-breaking` label if so. See #791 for details - [ ] Bug fix - [x] Code health or cleanup - [ ] Documentation - [ ] Other (specify) ## Testing - [ ] **Documentation**: `make docusaurus_start`; only needed if you make doc changes - [ ] **Unit Tests**: `make go_develop_and_test` - [ ] **LocalNet E2E Tests**: `make test_e2e` - [ ] **DevNet E2E Tests**: Add the `devnet-test-e2e` label to the PR. ## Sanity Checklist - [x] I have tested my changes using the available tooling - [ ] I have commented my code - [x] I have performed a self-review of my own code; both comments & source code - [ ] I create and reference any new tickets, if applicable - [ ] I have left TODOs throughout the codebase, if applicable --------- Co-authored-by: Redouane Lakrache Co-authored-by: Daniel Olshansky Co-authored-by: red-0ne --- x/proof/keeper/msg_server_update_param.go | 63 ++++++++++--------- .../keeper/msg_server_update_param_test.go | 8 +-- x/proof/keeper/msg_update_params_test.go | 5 +- x/proof/keeper/params_test.go | 26 ++++---- x/proof/types/message_update_param.go | 27 +++++--- x/proof/types/params.go | 52 +++++++++------ 6 files changed, 108 insertions(+), 73 deletions(-) diff --git a/x/proof/keeper/msg_server_update_param.go b/x/proof/keeper/msg_server_update_param.go index d98db8206..6e6b3603c 100644 --- a/x/proof/keeper/msg_server_update_param.go +++ b/x/proof/keeper/msg_server_update_param.go @@ -2,6 +2,10 @@ package keeper import ( "context" + "fmt" + + "google.golang.org/grpc/codes" + "google.golang.org/grpc/status" "github.com/pokt-network/poktroll/x/proof/types" ) @@ -12,58 +16,59 @@ 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.ErrProofInvalidSigner.Wrapf("invalid authority; expected %s, got %s", k.GetAuthority(), msg.Authority) + return nil, status.Error( + codes.InvalidArgument, + types.ErrProofInvalidSigner.Wrapf( + "invalid authority; expected %s, got %s", + k.GetAuthority(), msg.Authority, + ).Error(), + ) } params := k.GetParams(ctx) switch msg.Name { case types.ParamProofRequestProbability: - value, ok := msg.AsType.(*types.MsgUpdateParam_AsFloat) - if !ok { - return nil, types.ErrProofParamInvalid.Wrapf("unsupported value type for %s param: %T", msg.Name, msg.AsType) - } - - params.ProofRequestProbability = value.AsFloat + logger = logger.With("param_value", msg.GetAsFloat()) + params.ProofRequestProbability = msg.GetAsFloat() case types.ParamProofRequirementThreshold: - value, ok := msg.AsType.(*types.MsgUpdateParam_AsCoin) - if !ok { - return nil, types.ErrProofParamInvalid.Wrapf("unsupported value type for %s param: %T", msg.Name, msg.AsType) - } - - params.ProofRequirementThreshold = value.AsCoin + logger = logger.With("param_value", msg.GetAsCoin()) + params.ProofRequirementThreshold = msg.GetAsCoin() case types.ParamProofMissingPenalty: - value, ok := msg.AsType.(*types.MsgUpdateParam_AsCoin) - if !ok { - return nil, types.ErrProofParamInvalid.Wrapf("unsupported value type for %s param: %T", msg.Name, msg.AsType) - } - - params.ProofMissingPenalty = value.AsCoin + logger = logger.With("param_value", msg.GetAsCoin()) + params.ProofMissingPenalty = msg.GetAsCoin() case types.ParamProofSubmissionFee: - value, ok := msg.AsType.(*types.MsgUpdateParam_AsCoin) - if !ok { - return nil, types.ErrProofParamInvalid.Wrapf("unsupported value type for %s param: %T", msg.Name, msg.AsType) - } - - params.ProofSubmissionFee = value.AsCoin + logger = logger.With("param_value", msg.GetAsCoin()) + params.ProofSubmissionFee = msg.GetAsCoin() default: - return nil, types.ErrProofParamInvalid.Wrapf("unsupported param %q", msg.Name) + return nil, status.Error( + codes.InvalidArgument, + types.ErrProofParamInvalid.Wrapf("unsupported param %q", msg.Name).Error(), + ) } if err := params.ValidateBasic(); err != nil { - return nil, err + return nil, status.Error(codes.InvalidArgument, err.Error()) } if err := k.SetParams(ctx, params); err != nil { - return nil, err + err = fmt.Errorf("unable to set params: %w", err) + logger.Error(fmt.Sprintf("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/proof/keeper/msg_server_update_param_test.go b/x/proof/keeper/msg_server_update_param_test.go index ba3a39c31..a53d688ec 100644 --- a/x/proof/keeper/msg_server_update_param_test.go +++ b/x/proof/keeper/msg_server_update_param_test.go @@ -39,7 +39,7 @@ func TestMsgUpdateParam_UpdateProofRequestProbabilityOnly(t *testing.T) { require.Equal(t, expectedProofRequestProbability, res.Params.ProofRequestProbability) // Ensure the other parameters are unchanged - testkeeper.AssertDefaultParamsEqualExceptFields(t, &defaultParams, res.Params, "ProofRequestProbability") + testkeeper.AssertDefaultParamsEqualExceptFields(t, &defaultParams, res.Params, string(prooftypes.KeyProofRequestProbability)) } func TestMsgUpdateParam_UpdateProofRequirementThresholdOnly(t *testing.T) { @@ -66,7 +66,7 @@ func TestMsgUpdateParam_UpdateProofRequirementThresholdOnly(t *testing.T) { require.Equal(t, &expectedProofRequirementThreshold, res.Params.ProofRequirementThreshold) // Ensure the other parameters are unchanged - testkeeper.AssertDefaultParamsEqualExceptFields(t, &defaultParams, res.Params, "ProofRequirementThreshold") + testkeeper.AssertDefaultParamsEqualExceptFields(t, &defaultParams, res.Params, string(prooftypes.KeyProofRequirementThreshold)) } func TestMsgUpdateParam_UpdateProofMissingPenaltyOnly(t *testing.T) { @@ -93,7 +93,7 @@ func TestMsgUpdateParam_UpdateProofMissingPenaltyOnly(t *testing.T) { require.Equal(t, &expectedProofMissingPenalty, res.Params.ProofMissingPenalty) // Ensure the other parameters are unchanged - testkeeper.AssertDefaultParamsEqualExceptFields(t, &defaultParams, res.Params, "ProofMissingPenalty") + testkeeper.AssertDefaultParamsEqualExceptFields(t, &defaultParams, res.Params, string(prooftypes.KeyProofMissingPenalty)) } func TestMsgUpdateParam_UpdateProofSubmissionFeeOnly(t *testing.T) { @@ -120,5 +120,5 @@ func TestMsgUpdateParam_UpdateProofSubmissionFeeOnly(t *testing.T) { require.Equal(t, &expectedProofSubmissionFee, res.Params.ProofSubmissionFee) // Ensure the other parameters are unchanged - testkeeper.AssertDefaultParamsEqualExceptFields(t, &defaultParams, res.Params, "ProofSubmissionFee") + testkeeper.AssertDefaultParamsEqualExceptFields(t, &defaultParams, res.Params, string(prooftypes.KeyProofSubmissionFee)) } diff --git a/x/proof/keeper/msg_update_params_test.go b/x/proof/keeper/msg_update_params_test.go index 2c6739855..d7509dc93 100644 --- a/x/proof/keeper/msg_update_params_test.go +++ b/x/proof/keeper/msg_update_params_test.go @@ -42,8 +42,9 @@ func TestMsgUpdateParams(t *testing.T) { params: &types.MsgUpdateParams{ Authority: k.GetAuthority(), Params: types.Params{ - ProofMissingPenalty: &types.DefaultProofMissingPenalty, - ProofSubmissionFee: &types.MinProofSubmissionFee, + ProofRequirementThreshold: &types.DefaultProofRequirementThreshold, + ProofMissingPenalty: &types.DefaultProofMissingPenalty, + ProofSubmissionFee: &types.MinProofSubmissionFee, }, }, shouldError: false, diff --git a/x/proof/keeper/params_test.go b/x/proof/keeper/params_test.go index 128ada551..43e197d02 100644 --- a/x/proof/keeper/params_test.go +++ b/x/proof/keeper/params_test.go @@ -106,7 +106,7 @@ func TestParams_ValidateProofMissingPenalty(t *testing.T) { { desc: "invalid denomination", proofMissingPenalty: &invalidDenomCoin, - expectedErr: prooftypes.ErrProofParamInvalid.Wrap("invalid coin denom: invalid_denom"), + expectedErr: prooftypes.ErrProofParamInvalid.Wrap("invalid proof_missing_penalty denom: invalid_denom"), }, { desc: "missing", @@ -124,12 +124,12 @@ func TestParams_ValidateProofMissingPenalty(t *testing.T) { }, } - for _, tt := range tests { - t.Run(tt.desc, func(t *testing.T) { - err := prooftypes.ValidateProofMissingPenalty(tt.proofMissingPenalty) - if tt.expectedErr != nil { + for _, test := range tests { + t.Run(test.desc, func(t *testing.T) { + err := prooftypes.ValidateProofMissingPenalty(test.proofMissingPenalty) + if test.expectedErr != nil { require.Error(t, err) - require.Contains(t, err.Error(), tt.expectedErr.Error()) + require.EqualError(t, err, test.expectedErr.Error()) } else { require.NoError(t, err) } @@ -155,7 +155,7 @@ func TestParams_ValidateProofSubmissionFee(t *testing.T) { { desc: "invalid denomination", proofSubmissionFee: &invalidDenomCoin, - expectedErr: prooftypes.ErrProofParamInvalid.Wrap("invalid coin denom: invalid_denom"), + expectedErr: prooftypes.ErrProofParamInvalid.Wrap("invalid proof_submission_fee denom: invalid_denom"), }, { desc: "missing", @@ -171,7 +171,7 @@ func TestParams_ValidateProofSubmissionFee(t *testing.T) { desc: "below minimum", proofSubmissionFee: &belowMinProofSubmissionFee, expectedErr: prooftypes.ErrProofParamInvalid.Wrapf( - "ProofSubmissionFee param is below minimum value %s: got %s", + "proof_submission_fee is below minimum value %s: got %s", prooftypes.MinProofSubmissionFee, belowMinProofSubmissionFee, ), @@ -182,12 +182,12 @@ func TestParams_ValidateProofSubmissionFee(t *testing.T) { }, } - for _, tt := range tests { - t.Run(tt.desc, func(t *testing.T) { - err := prooftypes.ValidateProofSubmissionFee(tt.proofSubmissionFee) - if tt.expectedErr != nil { + for _, test := range tests { + t.Run(test.desc, func(t *testing.T) { + err := prooftypes.ValidateProofSubmissionFee(test.proofSubmissionFee) + if test.expectedErr != nil { require.Error(t, err) - require.Contains(t, err.Error(), tt.expectedErr.Error()) + require.EqualError(t, err, test.expectedErr.Error()) } else { require.NoError(t, err) } diff --git a/x/proof/types/message_update_param.go b/x/proof/types/message_update_param.go index b4ed1960d..ad7142bef 100644 --- a/x/proof/types/message_update_param.go +++ b/x/proof/types/message_update_param.go @@ -33,9 +33,10 @@ func NewMsgUpdateParam(authority string, name string, value any) (*MsgUpdatePara }, nil } -// ValidateBasic performs a basic validation of the MsgUpdateParam fields. It ensures -// the parameter name is supported and the parameter type matches the expected type for -// a given parameter name. +// ValidateBasic performs a basic validation of the MsgUpdateParam fields. It ensures: +// 1. The parameter name is supported. +// 2. The parameter type matches the expected type for a given parameter name. +// 3. The parameter value is valid (according to its respective validation function). func (msg *MsgUpdateParam) ValidateBasic() error { // Validate the address if _, err := sdk.AccAddressFromBech32(msg.Authority); err != nil { @@ -50,13 +51,25 @@ func (msg *MsgUpdateParam) ValidateBasic() error { // Parameter name must be supported by this module. switch msg.Name { case ParamProofRequestProbability: - return msg.paramTypeIsFloat() + if err := msg.paramTypeIsFloat(); err != nil { + return err + } + return ValidateProofRequestProbability(msg.GetAsFloat()) case ParamProofRequirementThreshold: - return msg.paramTypeIsCoin() + if err := msg.paramTypeIsCoin(); err != nil { + return err + } + return ValidateProofRequirementThreshold(msg.GetAsCoin()) case ParamProofMissingPenalty: - return msg.paramTypeIsCoin() + if err := msg.paramTypeIsCoin(); err != nil { + return err + } + return ValidateProofMissingPenalty(msg.GetAsCoin()) case ParamProofSubmissionFee: - return msg.paramTypeIsCoin() + if err := msg.paramTypeIsCoin(); err != nil { + return err + } + return ValidateProofSubmissionFee(msg.GetAsCoin()) default: return ErrProofParamNameInvalid.Wrapf("unsupported param %q", msg.Name) } diff --git a/x/proof/types/params.go b/x/proof/types/params.go index 2a46bda77..16d77287f 100644 --- a/x/proof/types/params.go +++ b/x/proof/types/params.go @@ -118,10 +118,10 @@ func (params *Params) ValidateBasic() error { // ValidateProofRequestProbability validates the ProofRequestProbability param. // NB: The argument is an interface type to satisfy the ParamSetPair function signature. -func ValidateProofRequestProbability(v interface{}) error { - proofRequestProbability, ok := v.(float32) +func ValidateProofRequestProbability(proofRequestProbabilityAny any) error { + proofRequestProbability, ok := proofRequestProbabilityAny.(float32) if !ok { - return ErrProofParamInvalid.Wrapf("invalid parameter type: %T", v) + return ErrProofParamInvalid.Wrapf("invalid parameter type: %T", proofRequestProbabilityAny) } if proofRequestProbability < 0 || proofRequestProbability > 1 { @@ -133,10 +133,22 @@ func ValidateProofRequestProbability(v interface{}) error { // ValidateProofRequirementThreshold validates the ProofRequirementThreshold param. // NB: The argument is an interface type to satisfy the ParamSetPair function signature. -func ValidateProofRequirementThreshold(v interface{}) error { - _, ok := v.(*cosmostypes.Coin) +func ValidateProofRequirementThreshold(proofRequirementThresholdAny any) error { + proofRequirementThresholdCoin, ok := proofRequirementThresholdAny.(*cosmostypes.Coin) if !ok { - return ErrProofParamInvalid.Wrapf("invalid parameter type: %T", v) + return ErrProofParamInvalid.Wrapf("invalid parameter type: %T", proofRequirementThresholdAny) + } + + if proofRequirementThresholdCoin == nil { + return ErrProofParamInvalid.Wrap("missing proof_requirement_threshold") + } + + if proofRequirementThresholdCoin.Denom != volatile.DenomuPOKT { + return ErrProofParamInvalid.Wrapf("invalid proof_requirement_threshold denom: %s", proofRequirementThresholdCoin.Denom) + } + + if proofRequirementThresholdCoin.IsZero() || proofRequirementThresholdCoin.IsNegative() { + return ErrProofParamInvalid.Wrapf("invalid proof_requirement_threshold amount: %s <= 0", proofRequirementThresholdCoin) } return nil @@ -144,29 +156,33 @@ func ValidateProofRequirementThreshold(v interface{}) error { // ValidateProofMissingPenalty validates the ProofMissingPenalty param. // NB: The argument is an interface type to satisfy the ParamSetPair function signature. -func ValidateProofMissingPenalty(v interface{}) error { - coin, ok := v.(*cosmostypes.Coin) +func ValidateProofMissingPenalty(proofMissingPenaltyAny any) error { + proofMissingPenaltyCoin, ok := proofMissingPenaltyAny.(*cosmostypes.Coin) if !ok { - return ErrProofParamInvalid.Wrapf("invalid parameter type: %T", v) + return ErrProofParamInvalid.Wrapf("invalid parameter type: %T", proofMissingPenaltyAny) } - if coin == nil { + if proofMissingPenaltyCoin == nil { return ErrProofParamInvalid.Wrap("missing proof_missing_penalty") } - if coin.Denom != volatile.DenomuPOKT { - return ErrProofParamInvalid.Wrapf("invalid coin denom: %s", coin.Denom) + if proofMissingPenaltyCoin.Denom != volatile.DenomuPOKT { + return ErrProofParamInvalid.Wrapf("invalid proof_missing_penalty denom: %s", proofMissingPenaltyCoin.Denom) + } + + if proofMissingPenaltyCoin.IsZero() || proofMissingPenaltyCoin.IsNegative() { + return ErrProofParamInvalid.Wrapf("invalid proof_missing_penalty amount: %s <= 0", proofMissingPenaltyCoin) } return nil } -// ValidateProofSubmission validates the ProofSubmissionFee param. +// ValidateProofSubmissionFee validates the ProofSubmissionFee param. // NB: The argument is an interface type to satisfy the ParamSetPair function signature. -func ValidateProofSubmissionFee(v interface{}) error { - submissionFeeCoin, ok := v.(*cosmostypes.Coin) +func ValidateProofSubmissionFee(proofSubmissionFeeAny any) error { + submissionFeeCoin, ok := proofSubmissionFeeAny.(*cosmostypes.Coin) if !ok { - return ErrProofParamInvalid.Wrapf("invalid parameter type: %T", v) + return ErrProofParamInvalid.Wrapf("invalid parameter type: %T", proofSubmissionFeeAny) } if submissionFeeCoin == nil { @@ -174,12 +190,12 @@ func ValidateProofSubmissionFee(v interface{}) error { } if submissionFeeCoin.Denom != volatile.DenomuPOKT { - return ErrProofParamInvalid.Wrapf("invalid coin denom: %s", submissionFeeCoin.Denom) + return ErrProofParamInvalid.Wrapf("invalid proof_submission_fee denom: %s", submissionFeeCoin.Denom) } if submissionFeeCoin.Amount.LT(MinProofSubmissionFee.Amount) { return ErrProofParamInvalid.Wrapf( - "ProofSubmissionFee param is below minimum value %s: got %s", + "proof_submission_fee is below minimum value %s: got %s", MinProofSubmissionFee, submissionFeeCoin, )