[Service] Refresh service module params logic (#861)

## Summary

The "adding_params.md" doc has (or will have) changed substantially (see
#839) since the shared module's param logic was last updated. This PR
aligns the service 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 
- #851

## 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

- [ ] I have tested my changes using the available tooling
- [ ] I have commented my code
- [ ] 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]>

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
+		logger.Info("ERROR: %s", err)
+		return nil, status.Error(codes.Internal, err.Error())
 	}
 
 	updatedParams := k.GetParams(ctx)
+
 	return &types.MsgUpdateParamResponse{
 		Params: &updatedParams,
 	}, nil

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

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",

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)
+			}
+		})
+	}
+}

x/service/types/errors.go

@@ -12,14 +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")
 	ErrServiceMissingRelayMiningDifficulty = sdkerrors.Register(ModuleName, 1116, "missing relay mining difficulty")
 )

x/service/types/genesis_test.go

@@ -102,7 +102,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
 	}

x/service/types/message_update_param.go

@@ -1,29 +1,27 @@
 package types
 
 import (
-	"fmt"
-
 	sdk "github.com/cosmos/cosmos-sdk/types"
 )
 
 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)
 	}
 }
 

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)
+		})
+	}
+}

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,
 		)
 	}