[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 <[email protected]> Co-authored-by: Daniel Olshansky <[email protected]> Co-authored-by: red-0ne <[email protected]>
pokt-network · Oct 14, 2024 · efbebfa · efbebfa
1 parent 2ac298a
commit efbebfa
Show file tree

Hide file tree

Showing 6 changed files with 108 additions and 73 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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,53 +133,69 @@ 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
 }
 
 // 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 {
 		return ErrProofParamInvalid.Wrap("missing proof_submission_fee")
 	}
 
 	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,
 		)