[TECHDEBT] Change the PersistenceRWContext interface to pass paramName when getting a parameter

[andrewnguyen22]

Objective

Deprecate

• GetBlocksPerSession(height int64) (int, error)
  and
• GetServiceNodesPerSessionAt(height int64) (int, error)

for

• GetParameter(paramName string, height int64) (interface, error)

Origin Document

We genericized the param setters like:

SetParam(paramName string, value interface{}) error

We need a similar pattern for getters

Goals

• Lower code footprint,
• Remove placeholder constants
• Maintain a strict pattern

Deliverable

• Code complete implementation
• Updated documentation

General issue deliverables

• Update the appropriate CHANGELOG
• Update any relevant READMEs (local and/or global)
• Update any relevant global documentation & references
• If applicable, update the source code tree explanation
• If applicable, add or update a state, sequence or flowchart diagram using mermaid

---

Creator: @andrewnguyen22

[Olshansk]

Great starter task!

[Jasonyou1995]

I might need some help to get started (some super basic questions):

• Task questions
  • Do I need to implement the GetParameter function?
  • Should I comment out the two deprecated functions GetBlocksPerSession and GetServiceNodesPerSessionAt then combine them to GetParameter?
  • I saw the ./utility/context.go and ./persistence/module.go scripts are using this interface, should I update them all?
• Documentation questions
  • What is the specific documentation I should update other than CHANGELOG?

Thank you!

[Olshansk]

Do I need to implement the GetParameter function?

Yes.

Should I comment out the two deprecated functions GetBlocksPerSession and GetServiceNodesPerSessionAt then combine them to GetParameter?

Yes.

I saw the ./utility/context.go and ./persistence/module.go scripts are using this interface, should I update them all?

Yes.

What is the specific documentation I should update other than CHANGELOG?

For this change, since it's small. The changelog is enough.

[h5law]

I would be interested in taking up this task if its still relevant. The only question I have is around determining the type from the paramName argument to the GetParameter function.

If the function is solely to replace the two ones listed why not use this function which is already defined? As both the functions are int32 types if I am correct.

func (p PostgresContext) GetIntParam(paramName string, height int64) (int, error) {
	v, _, err := getParamOrFlag[int](p, types.ParamsTableName, paramName, height)
	return v, err
}

However, if you are aiming to really generalise the 3 wrappers for the getParamOrFlag function you would need to know the type of the parameter. Or simply just return it as a string. Please let me know if I am wrong here as I am not super familiar with the codebase.

I think this involves editing the following files:

shared/modules/persistence_module.go <-- comment out old functions define new function in interface
persistence/gov.go <-- comment out old functions add new GetParameter function logic
utility/gov.go <-- Comment out GetBlocksPerSession replace with GetParameter
utility/test/gov_test.go <-- Comment out GetBlocksPerSession test replace with new GetParameter test

In addition to the CHANGELOG.md

Let me know the situation I am happy to get to work. The main question is about knowing the type the paramName string will return from the database.

One way this could work is by having the function signature like: GetParameter(paramName string, value any, height int64) (any, error) which is similar to what the SetParam function does. By explicitly saying the value we want to receive from his function it becomes a lot easier. Otherwise I am not too sure about how to infer the return type.

[h5law]

@Olshansk what do you think about this?

[Olshansk]

I would be interested in taking up this task if its still relevant

Yup, still relevant!

If the function is solely to replace the two ones listed why not use this function which is already defined? As both the functions are int32 types if I am correct.

This is the first step of this task and what I would call the "first commit" as you start working on it locally. It is to simply make these two functions follow the same pattern as the rest of the codebase by using SetParam(paramName string, value interface{}) error from the PersistenceModule interface.

However, if you are aiming to really generalise the 3 wrappers for the getParamOrFlag function you would need to know the type of the parameter. Or simply just return it as a string. Please let me know if I am wrong here as I am not super familiar with the codebase.

Similar to how we have SetParam(paramName string, value interface{}) error in PersistenceModule, we were thinking of adding a GetParam(paramName string, height int64) (interface{}, error) to PersistenceReadContext. There might even be an opportunity to use GetParam[T int | string | []byte](paramName string, height int64) (T, error) so the caller doesn't have to type cast it explicitly after retrieving the data.

In terms of files that'll be affected, you're on the right track.

Disclaimer: I haven't tried implementing the suggestion above so there may be some hurdles I'm unaware of.

---

cc @deblasis for context. I know you're planning on making some other changes to the persistence interface and curious what you think about this change.

[h5law]

@Olshansk I have actually wrote the code for this PR and update the unit test in utility/test/gov_test.go (all working successfully) I ended up going for an automatic type detection method by checking the types in the genesis_state protobuf (but reading the json tag to avoid having the proto imports used) I will open a PR so you can check it out and I don't mind changing to your proposed method as it will probably be easier to have explicit types given. My method will also never allow for a []byte return type as none of the functions in the genesis state actually have this type (reading this from persistence/types/persistence_genesis.pb.go). I'll get to work on the PR and the changes now

[deblasis]

@Olshansk

cc @deblasis for context. I know you're planning on making some other changes to the persistence interface and curious what you think about this change.

Hi @h5law !

I would be interested in taking up this task if its still relevant. The only question I have is around determining the type from the paramName argument to the GetParameter function.

In order to determine the type of the params we can leverage the struct tags

These are parsed in parseGovProto() which should return everything you need to move forward. Do not hesitate if you have any questions.

Oh as I was writing I saw your message and it seems you already found your way :) well done!