[TECHDEBT] Change the PersistenceRWContext interface to pass paramName when getting a parameter - (Issue #286)

[h5law]

Description

This PR is in response to issue #286. A general function to retrieve parameters from their paramName string will help to address the goals laid out in the issue. Namely a lower code footprint, the removal of placeholder constants and it allows for a pattern to be used from now on regarding retrieval of parameters - similar to the pattern for setting parameters.

The function uses the ParameterNameTypeMap built by the init() function in persistence/gov.go to find the correct return type from the parameter name string. The map is built by:

• Initialising an empty Params struct as defined in persistence/types/persistence_genesis.pb.go
• Loop through the fields of this stuct:
  • Extracting the parameter name from the json tag of each field
    • Match the json tag as it is in the format of {paramName},omitempty and the parameter name can easily be found
  • Setting ParameterNameTypeMap[paramName] = stringified type of field in struct

Building the map with the init() function and storing it in memory allows for quick access each time GetParameter is called removing the need to parse the Params struct each time it is called.

The function's signature is GetParameter(paramName string, height int64) (any, error)

The function's logic is defined in the file persistence/gov.go and works as follows:

• Using the paramName argument find the parameter return type from the ParameterNameTypeMap
• Using this stringified parameter type, call the proper getter function getParamOrFlag[int | string | []byte] and the values from this call are returned.

The old functions have been removed and replaced with the new generic function. This includes the unit test in utility/test/gov.go where the GetBlocksPerSession test now uses the GetParameter function - passing successfully. The GetSetIntParam and GetSetStringParam unit tests in persistence/test/gov_test.go have also been updated to use GetParameter also passing successfully.

ok  	github.com/pokt-network/pocket/persistence/test	20.742s
ok  	github.com/pokt-network/pocket/utility/test	17.468s

Issue

Fixes #286

Type of change

Please mark the relevant option(s):

• New feature, functionality or library
• Bug fix
• Code health or cleanup
• Major breaking change
• Documentation
• Other

List of changes

• Deprecate old functions (GetBlocksPerSession and GetServiceNodesPerSessionAt) in favour of using the more general GetParameter in shared/modules/persistence_module.go
• Add init() function in persistence/gov.go to build ParameterNameTypeMap of parameter names and their return types
• Define the logic for the GetParameter in persistence/gov.go
• Replace old function with GetParameter in utility/gov.go
• Change unit test for GetBlocksPerSession in utility/test/gov_test.go to use the new function
• Change unit tests in persistence/test/gov_test.go to use the GetParameter() function instead of the GetIntParam, and GetStringParam functions

Testing

• make develop_test
• LocalNet w/ all of the steps outlined in the README

Required Checklist

• I have performed a self-review of my own code
• I have commented my code, particularly in hard-to-understand areas
• I have tested my changes using the available tooling
• I have updated the corresponding CHANGELOG

If Applicable Checklist

• I have updated the corresponding README(s); local and/or global
• I have added tests that prove my fix is effective or that my feature works
• I have added, or updated, mermaid.js diagrams in the corresponding README(s)
• I have added, or updated, documentation and mermaid.js diagrams in shared/docs/* if I updated shared/*README(s)

[h5law]

@Olshansk I cannot assign myself to this, I don't think I have permissions to do that sort of thing.

I am also aware that in this implementation there will never be an instance where getParamOrFlag[[]byte] is used as none of the functions in the Param struct have that type. I am happy to work on a different implementation, but this feels like a good starting point.

[Olshansk]

Reflections in business logic are usually eery, but this is pretty clean.

If we do this, we can simplify all of the gov params accessors in a follow-up task.

@h5law Would you be open to it?

[h5law]

Reflections in business logic are usually eery, but this is pretty clean.

I couldn't think of any other way. I found the pokt tags of these structs not very clear as to the actual return types. BIGINT and SMALLINT I expected would return either int64 and int32 or big.Int and int but most are both int32 return types which is why I didn't end up exporting the parseGovProto function or the map it creates as I found simply using the return types of the functions this getter would ultimately be calling to be easier.

If we do this, we can simplify all of the gov params accessors in a follow-up task.

@Olshansk I would be up for taking this on - I am about to go back to uni and have a couple exams but after those I would be happy to jump in and continue this work 😄 So in around 2 weeks time, I could start on it.

[Olshansk]

@h5law Can you create a new issue explaining that we should apply this business logic in other parts of the codebase and add a TODO(#XXX): Replace all gov parameter accessors with GetParameter. No rush though, exams come first :)

Also, I've added you as an external contributor to https://github.com/orgs/pokt-network/teams/external-contributors/members

[h5law]

@Olshansk I have removed those old comments. And updated the unit tests in persistence/test/gov_test.go to use GetParameter where otherwise it was using GetIntParam, GetBytesParam and GetStringParam.

I did make one change to the logic of GetParameter() though. I noticed that all the parameters ending with _owner are actually used as []bye not strings and the rest of the string types are base 10 *big.Int types so I added this handling into the init() function to check whether the paramName string ends with _owner then its []uint8 type otherwise strings=*big.Int.

However if this usage is incorrect I can change it back. I do believe this to be the case from looking through the code, and this would be useful as we move towards fully generalising these parameter accessors. I couldn't grok the types from the pokt tags alone so maybe this is better - however now instead of []uint8 never being called string will never be called - as it is now split into *big.Int and []uint8 depending on parameter names.

Maybe it would be better to leave the *big.Int conversion to another function? And have all _owner paramNames return []byte but the other strings left as strings? Wdyt?

Also, before I create the issue can I just confirm the task:

• Replace all uses of GetIntParam GetStringParam GetBytesParam with GetParameter, across persistence, utility and any other places they are used?
• For example in utility/gov.go

Change:

func (u *UtilityContext) GetAppMinimumStake() (*big.Int, typesUtil.Error) {
	return u.getBigIntParam(typesUtil.AppMinimumStakeParamName)
}
...
func (u *UtilityContext) GetValidatorMaxMissedBlocks() (maxMissedBlocks int, err typesUtil.Error) {
	return u.getIntParam(typesUtil.ValidatorMaximumMissedBlocksParamName)
}
...
func (u *UtilityContext) getIntParam(paramName string) (int, typesUtil.Error) {
	store, height, er := u.GetStoreAndHeight()
	if er != nil {
		return 0, er
	}
	value, err := store.GetIntParam(paramName, height)
	if err != nil {
		return typesUtil.ZeroInt, typesUtil.ErrGetParam(paramName, err)
	}
	return value, nil
}
...
func (u *UtilityContext) getBigIntParam(paramName string) (*big.Int, typesUtil.Error) {
	store, height, er := u.GetStoreAndHeight()
	if er != nil {
		return nil, er
	}
	value, err := store.GetStringParam(paramName, height)
	if err != nil {
		log.Printf("err: %v\n", err)
		return nil, typesUtil.ErrGetParam(paramName, err)
	}
	return typesUtil.StringToBigInt(value)
}

To

func (u *UtilityContext) GetAppMinimumStake() (*big.Int, typesUtil.Error) {
	return u.getBigIntParam(typesUtil.AppMinimumStakeParamName)
}
...
func (u *UtilityContext) GetValidatorMaxMissedBlocks() (maxMissedBlocks int, err typesUtil.Error) {
	return u.getIntParam(typesUtil.ValidatorMaximumMissedBlocksParamName)
}
...
func (u *UtilityContext) getIntParam(paramName string) (int, typesUtil.Error) {
	store, height, er := u.GetStoreAndHeight()
	if er != nil {
		return 0, er
	}
	value, err := store.GetParameter(paramName, height)
	if err != nil {
		return typesUtil.ZeroInt, typesUtil.ErrGetParam(paramName, err)
	}
	return value.(int), nil
}
...
func (u *UtilityContext) getBigIntParam(paramName string) (*big.Int, typesUtil.Error) {
	store, height, er := u.GetStoreAndHeight()
	if er != nil {
		return nil, er
	}
	value, err := store.GetParameter(paramName, height)
	if err != nil {
		log.Printf("err: %v\n", err)
		return nil, typesUtil.ErrGetParam(paramName, err)
	}
	return value.(*big.Int), nil
}

Or?

func (u *UtilityContext) GetAppMinimumStake() (*big.Int, typesUtil.Error) {
        store, height, err := u.GetStoreAndHeight()
        if err != nil {
                return nil, err
        }
	stake, err := u.GetParameter(typesUtil.AppMinimumStakeParamName, height)
        if err != nil {
                return nil, err
        }
        return stake.(*big.Int), nil
}

[Olshansk]

Need a bit more time to think through the followup issue, but wanted to post a couple questions/commnets I had here in the meantime.

[h5law]

@Olshansk What do you think about storing the []byte type parameters (so far those ending in _owner) as []byte instead of strings? This would solve the issue around retrieving the correct type of each parameter, for the way it is to be used. I realise this is quite a big change, as editing the runtime/genesis/proto/genesis.proto file will affect the utility, shared, persistence and consesus modules I believe

[Olshansk]

@Olshansk What do you think about storing the []byte type parameters (so far those ending in _owner) as []byte instead of strings? This would solve the issue around retrieving the correct type of each parameter, for the way it is to be used. I realise this is quite a big change, as editing the runtime/genesis/proto/genesis.proto file will affect the utility, shared, persistence and consesus modules I believe

This might make parsing easier, but creating the protobufs (reading, viewing, editing) will create a much worse developer experience since we wouldn't be able to just use strings anywhere, so I'm not a big fan of the idea at the moment.

---

SIde note: the PR looks pretty good to me so could you merge with main and just re-run the tests when you have a chance.

[h5law]

@Olshansk

This might make parsing easier, but creating the protobufs (reading, viewing, editing) will create a much worse developer experience since we wouldn't be able to just use strings anywhere, so I'm not a big fan of the idea at the moment.

Yeah I understand that, no worries will continue to think on it!

SIde note: the PR looks pretty good to me so could you merge with main and just re-run the tests when you have a chance.

On it now 🫡 All tests passing, cannot merge myself but the branch is synced with main now - all ready to merge.

[Olshansk]

Let's

[Olshansk]

@h5law I added you to https://github.com/orgs/pokt-network/teams/external-contributors. I'm curious to know if this allows you to merge this in yourself, given that you have approval from someone on the core team. If so, do a "squash & merge" and use the body of the GH description as the commit message.

Also, can you create the follow-up issue and add in the details we've already discussed here? We can narrow down the requirements and scope out the solution in that thread before continuing to the implementation.