Skip to content

Commit

Permalink
[Shared] Refresh shared module params logic (#852)
Browse files Browse the repository at this point in the history
## 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 shared 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. `sharedtypes.Param...`).
- Improving some local variable names.
- Replacing usages of `interface{}` with `any`.

## Dependencies

- #809
- #843 
- #844 
- #845
- #847
- #848
- #849
- #850
- #857

## Dependents

- #851 
- #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]>
  • Loading branch information
4 people authored Oct 14, 2024
1 parent 9f5451d commit 2ac298a
Show file tree
Hide file tree
Showing 5 changed files with 155 additions and 120 deletions.
108 changes: 44 additions & 64 deletions x/shared/keeper/msg_server_update_param.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,105 +2,85 @@ package keeper

import (
"context"
"fmt"

"google.golang.org/grpc/codes"
"google.golang.org/grpc/status"

"github.com/pokt-network/poktroll/x/shared/types"
)

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.ErrSharedInvalidSigner.Wrapf("invalid authority; expected %s, got %s", k.GetAuthority(), msg.Authority)
return nil, status.Error(
codes.InvalidArgument,
types.ErrSharedInvalidSigner.Wrapf(
"invalid authority; expected %s, got %s",
k.GetAuthority(), msg.Authority,
).Error(),
)
}

params := k.GetParams(ctx)

switch msg.Name {
case types.ParamNumBlocksPerSession:
value, ok := msg.AsType.(*types.MsgUpdateParam_AsInt64)
if !ok {
return nil, types.ErrSharedParamInvalid.Wrapf("unsupported value type for %s param: %T", msg.Name, msg.AsType)
}

params.NumBlocksPerSession = uint64(value.AsInt64)
logger = logger.With("param_value", msg.GetAsInt64())
params.NumBlocksPerSession = uint64(msg.GetAsInt64())
case types.ParamGracePeriodEndOffsetBlocks:
value, ok := msg.AsType.(*types.MsgUpdateParam_AsInt64)
if !ok {
return nil, types.ErrSharedParamInvalid.Wrapf("unsupported value type for %s param: %T", msg.Name, msg.AsType)
}

params.GracePeriodEndOffsetBlocks = uint64(value.AsInt64)
logger = logger.With("param_value", msg.GetAsInt64())
params.GracePeriodEndOffsetBlocks = uint64(msg.GetAsInt64())
case types.ParamClaimWindowOpenOffsetBlocks:
value, ok := msg.AsType.(*types.MsgUpdateParam_AsInt64)
if !ok {
return nil, types.ErrSharedParamInvalid.Wrapf("unsupported value type for %s param: %T", msg.Name, msg.AsType)
}

params.ClaimWindowOpenOffsetBlocks = uint64(value.AsInt64)
logger = logger.With("param_value", msg.GetAsInt64())
params.ClaimWindowOpenOffsetBlocks = uint64(msg.GetAsInt64())
case types.ParamClaimWindowCloseOffsetBlocks:
value, ok := msg.AsType.(*types.MsgUpdateParam_AsInt64)
if !ok {
return nil, types.ErrSharedParamInvalid.Wrapf("unsupported value type for %s param: %T", msg.Name, msg.AsType)
}

params.ClaimWindowCloseOffsetBlocks = uint64(value.AsInt64)
logger = logger.With("param_value", msg.GetAsInt64())
params.ClaimWindowCloseOffsetBlocks = uint64(msg.GetAsInt64())
case types.ParamProofWindowOpenOffsetBlocks:
value, ok := msg.AsType.(*types.MsgUpdateParam_AsInt64)
if !ok {
return nil, types.ErrSharedParamInvalid.Wrapf("unsupported value type for %s param: %T", msg.Name, msg.AsType)
}

params.ProofWindowOpenOffsetBlocks = uint64(value.AsInt64)
logger = logger.With("param_value", msg.GetAsInt64())
params.ProofWindowOpenOffsetBlocks = uint64(msg.GetAsInt64())
case types.ParamProofWindowCloseOffsetBlocks:
value, ok := msg.AsType.(*types.MsgUpdateParam_AsInt64)
if !ok {
return nil, types.ErrSharedParamInvalid.Wrapf("unsupported value type for %s param: %T", msg.Name, msg.AsType)
}

params.ProofWindowCloseOffsetBlocks = uint64(value.AsInt64)
logger = logger.With("param_value", msg.GetAsInt64())
params.ProofWindowCloseOffsetBlocks = uint64(msg.GetAsInt64())
case types.ParamSupplierUnbondingPeriodSessions:
value, ok := msg.AsType.(*types.MsgUpdateParam_AsInt64)
if !ok {
return nil, types.ErrSharedParamInvalid.Wrapf("unsupported value type for %s param: %T", msg.Name, msg.AsType)
}

params.SupplierUnbondingPeriodSessions = uint64(value.AsInt64)
logger = logger.With("param_value", msg.GetAsInt64())
params.SupplierUnbondingPeriodSessions = uint64(msg.GetAsInt64())
case types.ParamApplicationUnbondingPeriodSessions:
value, ok := msg.AsType.(*types.MsgUpdateParam_AsInt64)
if !ok {
return nil, types.ErrSharedParamInvalid.Wrapf("unsupported value type for %s param: %T", msg.Name, msg.AsType)
}

params.ApplicationUnbondingPeriodSessions = uint64(value.AsInt64)
logger = logger.With("param_value", msg.GetAsInt64())
params.ApplicationUnbondingPeriodSessions = uint64(msg.GetAsInt64())
case types.ParamComputeUnitsToTokensMultiplier:
value, ok := msg.AsType.(*types.MsgUpdateParam_AsInt64)
if !ok {
return nil, types.ErrSharedParamInvalid.Wrapf("unsupported value type for %s param: %T", msg.Name, msg.AsType)
}
computeUnitsToTokensMultiplier := uint64(value.AsInt64)

if err := types.ValidateComputeUnitsToTokensMultiplier(computeUnitsToTokensMultiplier); err != nil {
return nil, err
}

params.ComputeUnitsToTokensMultiplier = computeUnitsToTokensMultiplier
logger = logger.With("param_value", msg.GetAsInt64())
params.ComputeUnitsToTokensMultiplier = uint64(msg.GetAsInt64())
default:
return nil, types.ErrSharedParamInvalid.Wrapf("unsupported param %q", msg.Name)
return nil, status.Error(
codes.InvalidArgument,
types.ErrSharedParamInvalid.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, 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
Expand Down
52 changes: 35 additions & 17 deletions x/shared/keeper/msg_server_update_param_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@ import (
authtypes "github.com/cosmos/cosmos-sdk/x/auth/types"
govtypes "github.com/cosmos/cosmos-sdk/x/gov/types"
"github.com/stretchr/testify/require"
"google.golang.org/grpc/codes"
"google.golang.org/grpc/status"

"github.com/pokt-network/poktroll/x/shared/keeper"
"github.com/pokt-network/poktroll/x/shared/types"
Expand Down Expand Up @@ -50,7 +52,7 @@ func TestMsgUpdateParam_UpdateNumBlocksPerSession(t *testing.T) {
require.Equal(t, uint64(expectedNumBlocksPerSession), res.Params.NumBlocksPerSession)

// Ensure the other parameters are unchanged
testkeeper.AssertDefaultParamsEqualExceptFields(t, &testSharedParams, res.Params, "NumBlocksPerSession")
testkeeper.AssertDefaultParamsEqualExceptFields(t, &testSharedParams, res.Params, string(sharedtypes.KeyNumBlocksPerSession))
}

func TestMsgUpdateParam_UpdateClaimWindowOpenOffsetBlocks(t *testing.T) {
Expand Down Expand Up @@ -95,7 +97,7 @@ func TestMsgUpdateParam_UpdateClaimWindowOpenOffsetBlocks(t *testing.T) {
require.Equal(t, uint64(expectedClaimWindowOpenOffestBlocks), res.Params.ClaimWindowOpenOffsetBlocks)

// Ensure the other parameters are unchanged
testkeeper.AssertDefaultParamsEqualExceptFields(t, &sharedParams, res.Params, "ClaimWindowOpenOffsetBlocks")
testkeeper.AssertDefaultParamsEqualExceptFields(t, &sharedParams, res.Params, string(sharedtypes.KeyClaimWindowOpenOffsetBlocks))
}

func TestMsgUpdateParam_UpdateClaimWindowCloseOffsetBlocks(t *testing.T) {
Expand Down Expand Up @@ -140,7 +142,7 @@ func TestMsgUpdateParam_UpdateClaimWindowCloseOffsetBlocks(t *testing.T) {
require.Equal(t, uint64(expectedClaimWindowCloseOffestBlocks), res.Params.ClaimWindowCloseOffsetBlocks)

// Ensure the other parameters are unchanged
testkeeper.AssertDefaultParamsEqualExceptFields(t, &sharedParams, res.Params, "ClaimWindowCloseOffsetBlocks")
testkeeper.AssertDefaultParamsEqualExceptFields(t, &sharedParams, res.Params, string(sharedtypes.KeyClaimWindowCloseOffsetBlocks))
}

func TestMsgUpdateParam_UpdateProofWindowOpenOffsetBlocks(t *testing.T) {
Expand Down Expand Up @@ -185,7 +187,7 @@ func TestMsgUpdateParam_UpdateProofWindowOpenOffsetBlocks(t *testing.T) {
require.Equal(t, uint64(expectedProofWindowOpenOffestBlocks), res.Params.ProofWindowOpenOffsetBlocks)

// Ensure the other parameters are unchanged
testkeeper.AssertDefaultParamsEqualExceptFields(t, &sharedParams, res.Params, "ProofWindowOpenOffsetBlocks")
testkeeper.AssertDefaultParamsEqualExceptFields(t, &sharedParams, res.Params, string(sharedtypes.KeyProofWindowOpenOffsetBlocks))
}

func TestMsgUpdateParam_UpdateProofWindowCloseOffsetBlocks(t *testing.T) {
Expand Down Expand Up @@ -230,7 +232,7 @@ func TestMsgUpdateParam_UpdateProofWindowCloseOffsetBlocks(t *testing.T) {
require.Equal(t, uint64(expectedProofWindowCloseOffestBlocks), res.Params.ProofWindowCloseOffsetBlocks)

// Ensure the other parameters are unchanged
testkeeper.AssertDefaultParamsEqualExceptFields(t, &sharedParams, res.Params, "ProofWindowCloseOffsetBlocks")
testkeeper.AssertDefaultParamsEqualExceptFields(t, &sharedParams, res.Params, string(sharedtypes.KeyProofWindowCloseOffsetBlocks))
}

func TestMsgUpdateParam_UpdateGracePeriodEndOffsetBlocks(t *testing.T) {
Expand Down Expand Up @@ -263,11 +265,11 @@ func TestMsgUpdateParam_UpdateGracePeriodEndOffsetBlocks(t *testing.T) {
require.Equal(t, uint64(expectedGracePeriodEndOffestBlocks), res.Params.GetGracePeriodEndOffsetBlocks())

// Ensure the other parameters are unchanged
testkeeper.AssertDefaultParamsEqualExceptFields(t, &sharedParams, res.Params, "GracePeriodEndOffsetBlocks")
testkeeper.AssertDefaultParamsEqualExceptFields(t, &sharedParams, res.Params, string(sharedtypes.KeyGracePeriodEndOffsetBlocks))
}

func TestMsgUpdateParam_UpdateSupplierUnbondingPeriodSessions(t *testing.T) {
var expectedSupplierUnbondingPerid int64 = 5
var expectedSupplierUnbondingPeriod int64 = 5

k, ctx := testkeeper.SharedKeeper(t)
msgSrv := keeper.NewMsgServerImpl(k)
Expand All @@ -276,21 +278,21 @@ func TestMsgUpdateParam_UpdateSupplierUnbondingPeriodSessions(t *testing.T) {
require.NoError(t, k.SetParams(ctx, testSharedParams))

// Ensure the default values are different from the new values we want to set
require.NotEqual(t, uint64(expectedSupplierUnbondingPerid), testSharedParams.GetSupplierUnbondingPeriodSessions())
require.NotEqual(t, uint64(expectedSupplierUnbondingPeriod), testSharedParams.GetSupplierUnbondingPeriodSessions())

// Update the supplier unbonding period param
updateParamMsg := &sharedtypes.MsgUpdateParam{
Authority: authtypes.NewModuleAddress(govtypes.ModuleName).String(),
Name: sharedtypes.ParamSupplierUnbondingPeriodSessions,
AsType: &sharedtypes.MsgUpdateParam_AsInt64{AsInt64: expectedSupplierUnbondingPerid},
AsType: &sharedtypes.MsgUpdateParam_AsInt64{AsInt64: expectedSupplierUnbondingPeriod},
}
res, err := msgSrv.UpdateParam(ctx, updateParamMsg)
require.NoError(t, err)

require.Equal(t, uint64(expectedSupplierUnbondingPerid), res.Params.GetSupplierUnbondingPeriodSessions())
require.Equal(t, uint64(expectedSupplierUnbondingPeriod), res.Params.GetSupplierUnbondingPeriodSessions())

// Ensure the other parameters are unchanged
testkeeper.AssertDefaultParamsEqualExceptFields(t, &testSharedParams, res.Params, "SupplierUnbondingPeriodSessions")
testkeeper.AssertDefaultParamsEqualExceptFields(t, &testSharedParams, res.Params, string(sharedtypes.KeySupplierUnbondingPeriodSessions))

// Ensure that a supplier unbonding period that is less than the cumulative
// proof window close blocks is not allowed.
Expand All @@ -300,7 +302,13 @@ func TestMsgUpdateParam_UpdateSupplierUnbondingPeriodSessions(t *testing.T) {
AsType: &sharedtypes.MsgUpdateParam_AsInt64{AsInt64: 1},
}
_, err = msgSrv.UpdateParam(ctx, updateParamMsg)
require.ErrorIs(t, err, sharedtypes.ErrSharedParamInvalid)
require.EqualError(t, err, status.Error(
codes.InvalidArgument,
sharedtypes.ErrSharedParamInvalid.Wrapf(
"SupplierUnbondingPeriodSessions (%v session) (%v blocks) must be greater than the cumulative ProofWindowCloseOffsetBlocks (%v)",
1, 4, 10,
).Error(),
).Error())
}

func TestMsgUpdateParam_UpdateApplicationUnbondingPeriodSessions(t *testing.T) {
Expand All @@ -327,7 +335,7 @@ func TestMsgUpdateParam_UpdateApplicationUnbondingPeriodSessions(t *testing.T) {
require.Equal(t, uint64(expectedApplicationUnbondingPerid), res.Params.GetApplicationUnbondingPeriodSessions())

// Ensure the other parameters are unchanged
testkeeper.AssertDefaultParamsEqualExceptFields(t, &testSharedParams, res.Params, "ApplicationUnbondingPeriodSessions")
testkeeper.AssertDefaultParamsEqualExceptFields(t, &testSharedParams, res.Params, string(sharedtypes.KeyApplicationUnbondingPeriodSessions))

// Ensure that a application unbonding period that is less than the cumulative
// proof window close blocks is not allowed.
Expand All @@ -337,7 +345,13 @@ func TestMsgUpdateParam_UpdateApplicationUnbondingPeriodSessions(t *testing.T) {
AsType: &sharedtypes.MsgUpdateParam_AsInt64{AsInt64: 1},
}
_, err = msgSrv.UpdateParam(ctx, updateParamMsg)
require.ErrorIs(t, err, sharedtypes.ErrSharedParamInvalid)
require.EqualError(t, err, status.Error(
codes.InvalidArgument,
sharedtypes.ErrSharedParamInvalid.Wrapf(
"ApplicationUnbondingPeriodSessions (%v session) (%v blocks) must be greater than the cumulative ProofWindowCloseOffsetBlocks (%v)",
1, 4, 10,
).Error(),
).Error())
}

func TestMsgUpdateParam_ComputeUnitsToTokenMultiplier(t *testing.T) {
Expand All @@ -364,7 +378,7 @@ func TestMsgUpdateParam_ComputeUnitsToTokenMultiplier(t *testing.T) {
require.Equal(t, uint64(expectedComputeUnitsToTokenMultiplier), res.Params.GetComputeUnitsToTokensMultiplier())

// Ensure the other parameters are unchanged
testkeeper.AssertDefaultParamsEqualExceptFields(t, &testSharedParams, res.Params, "ComputeUnitsToTokensMultiplier")
testkeeper.AssertDefaultParamsEqualExceptFields(t, &testSharedParams, res.Params, string(sharedtypes.KeyComputeUnitsToTokensMultiplier))

// Ensure that compute units to token multiplier that is less than 1 is not allowed.
updateParamMsg = &sharedtypes.MsgUpdateParam{
Expand All @@ -373,7 +387,12 @@ func TestMsgUpdateParam_ComputeUnitsToTokenMultiplier(t *testing.T) {
AsType: &sharedtypes.MsgUpdateParam_AsInt64{AsInt64: 0},
}
_, err = msgSrv.UpdateParam(ctx, updateParamMsg)
require.ErrorIs(t, err, sharedtypes.ErrSharedParamInvalid)
require.EqualError(t, err, status.Error(
codes.InvalidArgument,
sharedtypes.ErrSharedParamInvalid.Wrapf(
"invalid ComputeUnitsToTokensMultiplier: (%d)", 0,
).Error(),
).Error())
}

// getMinActorUnbondingPeriodSessions returns the actors unbonding period
Expand All @@ -386,6 +405,5 @@ func getMinActorUnbondingPeriodSessions(
) uint64 {
deltaBlocks := newParamBlocksValue - oldParamBlocksValue
newProofWindowCloseBlocks := types.GetSessionEndToProofWindowCloseBlocks(params) + deltaBlocks

return (newProofWindowCloseBlocks / params.NumBlocksPerSession) + 1
}
62 changes: 49 additions & 13 deletions x/shared/types/message_update_param.go
Original file line number Diff line number Diff line change
Expand Up @@ -31,9 +31,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 {
Expand All @@ -48,16 +49,51 @@ func (msg *MsgUpdateParam) ValidateBasic() error {
// Parameter name must be supported by this module.
switch msg.Name {
// TODO_IMPROVE: Add a Uint64 asType instead of using int64 for uint64 params.
case ParamNumBlocksPerSession,
ParamGracePeriodEndOffsetBlocks,
ParamClaimWindowOpenOffsetBlocks,
ParamClaimWindowCloseOffsetBlocks,
ParamProofWindowOpenOffsetBlocks,
ParamProofWindowCloseOffsetBlocks,
ParamSupplierUnbondingPeriodSessions,
ParamApplicationUnbondingPeriodSessions,
ParamComputeUnitsToTokensMultiplier:
return msg.paramTypeIsInt64()
case ParamNumBlocksPerSession:
if err := msg.paramTypeIsInt64(); err != nil {
return err
}
return ValidateNumBlocksPerSession(uint64(msg.GetAsInt64()))
case ParamGracePeriodEndOffsetBlocks:
if err := msg.paramTypeIsInt64(); err != nil {
return err
}
return ValidateGracePeriodEndOffsetBlocks(uint64(msg.GetAsInt64()))
case ParamClaimWindowOpenOffsetBlocks:
if err := msg.paramTypeIsInt64(); err != nil {
return err
}
return ValidateClaimWindowOpenOffsetBlocks(uint64(msg.GetAsInt64()))
case ParamClaimWindowCloseOffsetBlocks:
if err := msg.paramTypeIsInt64(); err != nil {
return err
}
return ValidateClaimWindowCloseOffsetBlocks(uint64(msg.GetAsInt64()))
case ParamProofWindowOpenOffsetBlocks:
if err := msg.paramTypeIsInt64(); err != nil {
return err
}
return ValidateProofWindowOpenOffsetBlocks(uint64(msg.GetAsInt64()))
case ParamProofWindowCloseOffsetBlocks:
if err := msg.paramTypeIsInt64(); err != nil {
return err
}
return ValidateProofWindowCloseOffsetBlocks(uint64(msg.GetAsInt64()))
case ParamSupplierUnbondingPeriodSessions:
if err := msg.paramTypeIsInt64(); err != nil {
return err
}
return ValidateSupplierUnbondingPeriodSessions(uint64(msg.GetAsInt64()))
case ParamApplicationUnbondingPeriodSessions:
if err := msg.paramTypeIsInt64(); err != nil {
return err
}
return ValidateApplicationUnbondingPeriodSessions(uint64(msg.GetAsInt64()))
case ParamComputeUnitsToTokensMultiplier:
if err := msg.paramTypeIsInt64(); err != nil {
return err
}
return ValidateComputeUnitsToTokensMultiplier(uint64(msg.GetAsInt64()))
default:
return ErrSharedParamNameInvalid.Wrapf("unsupported param %q", msg.Name)
}
Expand Down
Loading

0 comments on commit 2ac298a

Please sign in to comment.