Ticket #211: finding unnecessary public functions in the Persistence Module

[Jasonyou1995]

Description

Reducing the scope of public functions in the persistence package from public to private for readability purposes

Issue

Fixes #211

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

Changed the following exported functions to lowercase non-exported functions
[./pocket/persistence/]
GetActor
GetActorFromRow
GetChainsForActor
SetActorStakeAmount
GetActorStakeAmount
GetCtxAndTx
GetCtx
SetValidatorStakedTokens
GetValidatorStakedTokens

[./pocket/persistence/types]
ProtocolActorTableSchema
ProtocolActorChainsTableSchema
SelectChains
ReadyToUnstake
InsertChains
UpdateUnstakingHeight
UpdateStakeAmount
UpdatePausedHeight
UpdateUnstakedHeightIfPausedBefore
AccToAccInterface
TestInsertParams
AccountOrPoolSchema
InsertAcc
SelectBalance

[./pocket/persistence/test]
GetGenericActor
NewTestGenericActor

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)

[Olshansk]

@Jasonyou1995 I noticed that you change the function names but not any place where it's being called. Did the unit tests pass after making this change?

[Jasonyou1995]

Working on setting up local nodes and test environments. Will review the comments today!

[Jasonyou1995]

There is no public function needed to be changed to private. The DeleteApp function will be removed in the issue #286

[Olshansk]

@Jasonyou1995 Could you confirm how you validated that there are no public functions that need to be converted to private? Either describing the process or a script you used would help.

I did a quick search for a function called GetCtxAndTx, and seems like it is not used outside the persistence package, so it is one example of a function that can be made lowercased.

[Olshansk]

There is more work remaining here: #320 (comment)

[Jasonyou1995]

One question I have is about updating the branch. I see some new commits made after I created this branch, but I'm unsure how to keep up to date with the main branch. Does git pull and then push help?

[Olshansk]

Try git merge main.

Highly recommend going through a git fundamentals tutorial online too: https://www.atlassian.com/git

[Jasonyou1995]

Try git merge main.

Highly recommend going through a git fundamentals tutorial online too: https://www.atlassian.com/git

Thanks! That helps, and I will go through that tutorial.

[Jasonyou1995]

[Functions with zero usage outside ./pocket/persistence/]
GetTableName 0
GetChainsTableName 0
GetActorSpecificColName 0
GetTableSchema 0
GetChainsTableSchema 0
GetQuery 0
GetAllQuery 0
GetExistsQuery 0
GetReadyToUnstakeQuery 0
GetOutputAddressQuery 0
GetStakeAmountQuery 0
GetPausedHeightQuery 0
GetUnstakingHeightQuery 0
GetChainsQuery 0
InsertQuery 0
UpdateQuery 0
UpdateChainsQuery 0
UpdateUnstakingHeightQuery 0
UpdatePausedHeightQuery 0
UpdateUnstakedHeightIfPausedBeforeQuery 0
SetStakeAmountQuery 0
ClearAllQuery 0
ClearAllChainsQuery 0
GetPools 0
GetAccounts 0
GetApplications 0
GetParams 0
GetPools 0
GetAccounts 0
GetApplications 0
GetParams 0
InsertQuery 0
UpdateChainsQuery 0
GetChainsTableSchema 0
GetChainsQuery 0
ClearAllChainsQuery 0
GetActorFromRow 0
GetChainsForActor 0
InsertActor 0
UpdateActor 0
GetActorsReadyToUnstake 0
SetActorUnstakingHeightAndStatus 0
GetActorPauseHeightIfExists 0
SetActorStatusAndUnstakingHeightIfPausedBefore 0
SetActorStakeAmount 0
GetActorStakeAmount 0
GetCtxAndTx 0
GetCtx 0
DebugClearAll 0
GetValidatorStakedTokens 0

---

Here is a list of functions without usage outside the ./persistence folder. I found them with a bash I created. Please let me know what you think.

[Olshansk]

That looks like a great list and very appropriate! Let's go forward with it.

Few things:

1. Consider sharing the script in the github issue so we can reference it in the future. No need to make it "general purpose" for now.
2. I recommend using your IDEs symbol renaming features (e.g. https://code.visualstudio.com/docs/editor/refactoring#_rename-symbol) rather than doing things by hand.
3. The way I would go about it personally is:

• Rename
• Run tests
• Commit
  Repeat that until everything is done and we will then squash & merge it after reviewing & approving.

[Jasonyou1995]

I tried to use the "rename symbol" with F2 you suggested, but it looks like there are errors for the elements cannot be renamed. I will try it again on another laptop.
But if that cannot be resolved, I will use ctrl+Shift+H to replace it. Do you think that will introduce any issues?

[Jasonyou1995]

Here is the error I got for all the functions I tried to rename with the F2 feature (on another laptop). I spent a few hours looking into solutions but had no luck... If you know what's the cause pls let me know. (I didn't want to just use the find and replace method, cause that can be unsafe for future code base).

[Olshansk]

@Jasonyou1995 Firstly, feel free to use w/e IDE you'd like: VSCode, Goland, etc...

All modern IDEs (i.e. not vim, emacs, etc...) have good symbol renaming features, and you can usually trust.

If you look at GetQuery, it exists in the persistence package and the persistence/types package. If you look at GetCtxAndTx, it only exists in the persistence package. This means the former is a candidate for an improvement but the latter is not. Here is a link to a guide on golang packages: https://www.callicoder.com/golang-packages/

[Olshansk]

Update the date prior to commit

[Olshansk]

@Jasonyou1995 Note how this has conflicts with master. This is very much expected as the rest of the team continues to merge in changes. Simply merge with master and resolve the conflicts.

I'd wait until #337 is in since it's very close. Once it is, make sure to re-run all the tests and LocalNet to make sure we keep main stable before merging it in.

We can probably get this in by EOD tomorrow!

[Jasonyou1995]

@Jasonyou1995 Note how this has conflicts with master. This is very much expected as the rest of the team continues to merge in changes. Simply merge with master and resolve the conflicts.

I'd wait until #337 is in since it's very close. Once it is, make sure to re-run all the tests and LocalNet to make sure we keep main stable before merging it in.

We can probably get this in by EOD tomorrow!

Sounds great to me! I will update the conflict parts once #337 is in. Hopefully this will be merged by tomorrow!

[Olshansk]

@Jasonyou1995 Should these files be introduced?

[Jasonyou1995]

These files are not necessary, I didn't know why they showed up.

[Jasonyou1995]

Updated the suggested changes

[Olshansk]

@Jasonyou1995 I noticed you did not re-request a review but aside from one small point, the PR is close to be ready.

Please double-check that all the tests & local net are passing after merging with master and request a review then.