[Persistence][Utility] Pools Address hack removal + state accessor fix for params and flags by deblasis · Pull Request #654 · pokt-network/pocket

deblasis · 2023-04-06T12:08:18Z

Description

While working on #327 (KV Store PR) I realized that the scope was growing and it sounded sensible to create a specific PR with these fixes that I believe are gonna be needed regardless.

Issue

I didn't bother creating an issue since I guess this constitutes bug/techdebt even if it's necessary work for #327 if we move forward with the KV store approach.

Type of change

Please mark the relevant option(s):

List of changes

Fixed flag/params keying. It was using the hash of the value and not the name, making queries by name impossible
Updated pools read/write functions to use a real address, not a string that is named address and obviously is not hexadecimal etc, etc.
Added test to describe behaviour and catch future bugs (we already missed a pool that wasn't added to the FriendlyNames: Pools_POOLS_FISHERMAN_STAKE)
Updated runtime and tests to reflect the changes

Testing

make develop_test; if any code changes were made
Docker Compose LocalNet; if any major functionality was changed or introduced
k8s LocalNet; if any infrastructure or configuration changes were made

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 added, or updated, godoc format comments on touched members (see: tip.golang.org/doc/comment)
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)

cr-gpt · 2023-04-06T12:08:22Z

Seems you are using me but didn't get OPENAI_API_KEY seted in Variables for this repo. you could follow readme for more information

github-actions

The changelog validation failed with the following output:

Missing changelog in module: build/

Missing changelog in module: persistence/

Missing changelog in module: runtime/

Missing changelog in module: shared/

Missing changelog in module: utility/

Changelog verification failed. See error messages for more detail.

Please update the relevant CHANGELOG.md files and ensure they follow the correct format.

reviewpad · 2023-04-06T12:14:18Z

AI-Generated Summary: This pull request primarily focuses on updating the handling of pool addresses throughout the codebase. Changes have been made to replace usage of friendly names with hexadecimal addresses, which include modifications to various functions, error messages, test files, configurations, and more. Specific files affected include validator.go, account.go, account_test.go, utility/unit_of_work, state_test.go, pools.go, and several others. Additionally, new test files and utility functions for handling pool addresses have been introduced. Overall, the changes in this pull request improve consistency, accuracy, and security by transitioning from pool friendly names to hexadecimal addresses.

cr-gpt · 2023-04-06T12:46:19Z

Seems you are using me but didn't get OPENAI_API_KEY seted in Variables for this repo. you could follow readme for more information

The check succeeded, dismissing the review comment.

reviewpad · 2023-04-06T12:47:23Z

AI-Generated Summary: This pull request primarily focuses on refactoring pool address handling across various files, transitioning from using friendly names (strings) to byte arrays (hexadecimal format) for better organization and consistency. Key changes include updating function signatures and implementations to change the input parameter from name to address []byte, modifying error messages to display the hex-encoded string of the address, and adding tests to ensure proper handling of pool addresses. In addition to updating the code, several files were updated to reflect these changes: genesis.json and configs.yaml files received new stake pool addresses, and CHANGELOG.md files were updated to include the new changes in their respective versions. Moreover, error handling and debugging have been improved in some files by making error messages more descriptive and changing hashing methods where necessary.

dylanlott

LGTM. Passed tests locally and produced blocks in Auto & Manual mode. ✅

dylanlott · 2023-04-06T17:36:15Z

shared/modules/persistence_module.go

+	AddPoolAmount(address []byte, amount string) error
+	SubtractPoolAmount(address []byte, amount string) error
+	SetPoolAmount(address []byte, amount string) error
+	InsertPool(address []byte, amount string) error


[]byte is definitely an improvement here 👍

Olshansk

This PR is:

I left one actionable comment. I think it's fine if you leave a TODO. We'll definitely revisit it organically when it matters.

Olshansk · 2023-04-06T21:05:15Z

utility/unit_of_work/account_test.go

 	uow := newTestingUtilityUnitOfWork(t, 0)
-	testPoolName := "TEST_POOL"
+
+	_, _, poolAddr := keygen.GetInstance().Next()


Man, whoever designed this interface was thinking ahead

Olshansk · 2023-04-06T21:11:07Z

shared/core/types/pools.go

@@ -1,18 +1,35 @@
 package types


Was doing something related in v0 this week: https://github.com/pokt-network/pocket-core/pull/1540/files

shared/core/types/pools_test.go

persistence/test/account_test.go

Co-authored-by: Daniel Olshansky <[email protected]>

cr-gpt · 2023-04-06T21:52:57Z

Seems you are using me but didn't get OPENAI_API_KEY seted in Variables for this repo. you could follow readme for more information

reviewpad · 2023-04-06T21:53:55Z

AI-Generated Summary: This pull request introduces changes focused on refactoring how pools are handled within the code, switching from friendly names to addresses for pool identification. The changes include updating function signatures for pool operations, modifying test cases to use dynamically generated pool addresses, and updating pool addresses in configuration files. Additionally, a new test file, pools_test.go, is added to test the Address method of the Pools type. Overall, these changes aim to make the code more maintainable, improve the management of pool addresses, and enhance the test coverage.

cr-gpt · 2023-04-06T21:56:57Z

Seems you are using me but didn't get OPENAI_API_KEY seted in Variables for this repo. you could follow readme for more information

reviewpad · 2023-04-06T21:57:54Z

AI-Generated Summary: This pull request includes a variety of changes primarily focused on modifying the way pool addresses are handled, shifting from human-readable string representations to hexadecimal representations. Several methods in different interfaces, such as PersistenceWriteContext and PersistenceReadContext, have been updated to accept address parameters of type []byte rather than name parameters of type string. Appropriate error handling, looping, and conversion adjustments have been made throughout the codebase to accommodate these changes.

Additionally, tests and error messages have been updated accordingly. A new test file, pools_test.go, has been introduced, featuring tests for the Pools_Address method, including edge cases, custom scenarios, and real-world cases. State hash values have been updated, and the Merkle tree logic has been adjusted. The genesis.json file and other configuration files have been updated with the new pool addresses in hexadecimal format.

Changelogs have also been updated noting various changes, such as replacing string pool names with their hexadecimal address format, incorporating the keygen package for address generation, and enhancing error handling. Overall, this pull request brings consistency to the handling of pool addresses and provides a more reliable implementation.

…its_and_bobs1 Signed-off-by: Alessandro De Blasis <[email protected]>

cr-gpt · 2023-04-06T21:58:17Z

Seems you are using me but didn't get OPENAI_API_KEY seted in Variables for this repo. you could follow readme for more information

github-actions · 2023-04-06T21:58:50Z

persistence/docs/CHANGELOG.md

+
 ## [0.0.0.43] - 2023-04-06

 - Added `SAVEPOINTS_ROLLBACKS.md` design document


The code patch you provided includes changes to a project's changelog, mentioning the updates as version 0.0.0.44 on 2023-04-06. Three changes are mentioned:

Fixed flag/params keying: This fix implies that there was a problem with how flags or parameters were being handled.

Updated pools read/write functions to use a real address, not a string that is named address and obviously is not hexadecimal: This change indicates an improvement in the way addresses are handled. It seems that previously, a string was being used rather than a proper hexadecimal address.

Updated tests: This suggests that the tests were updated, presumably to ensure the correctness of the implemented fixes.

While I can't provide specific bug risks or suggestions for improvement without seeing the actual code changes, it appears to be a positive update. Make sure the logic behind these changes is sound, and double-check to confirm that the updated tests effectively cover the fixed issues.

reviewpad · 2023-04-06T22:04:28Z

AI-Generated Summary: This pull request introduces several changes to the codebase, mainly focused on the persistence module, to replace the name parameter and pool names with address []byte and hexadecimal address representations, respectively. Major updates include modified function signatures for pool operations, updated error messages, and changes to the genesis.json and configs.yaml files. Tests have been updated accordingly to work with the actual pool addresses, and a new test file pools_test.go has been added in the "shared/core/types" package to test the Address method of the Pools type. The changes aim to improve code readability and consistency, as well as the accuracy and effectiveness of test cases.

* pokt/main: [Utility][RPC][CLI] Querying governance parameters (Issue #619) (#622) [Persistence][Utility] Separate all CreateAndApply functions into more functional components - Issue #508 (#652) [Persistence][Utility] Pools Address hack removal + state accessor fix for params and flags (#654) [PERSISTENCE] SavePoints and Rollbacks design document (Issue #493) (#533) Update reviewpad.yml Added ChatGPT-CodeReview workflow (#649) Update reviewpad.yml Added default reviewpad.yml file (#648) [DevNet] tweaks for remote environments (#601) [Documentation] Swap validator and non-validator triggers when finished synching (#646) [Consensus] Configuration entry point state sync (#528)

…p-modules * pokt/main: [Utility][RPC][CLI] Querying governance parameters (Issue pokt-network#619) (pokt-network#622) [Persistence][Utility] Separate all CreateAndApply functions into more functional components - Issue pokt-network#508 (pokt-network#652) [Persistence][Utility] Pools Address hack removal + state accessor fix for params and flags (pokt-network#654) [PERSISTENCE] SavePoints and Rollbacks design document (Issue pokt-network#493) (pokt-network#533) Update reviewpad.yml Added ChatGPT-CodeReview workflow (pokt-network#649) Update reviewpad.yml Added default reviewpad.yml file (pokt-network#648) [DevNet] tweaks for remote environments (pokt-network#601) [Documentation] Swap validator and non-validator triggers when finished synching (pokt-network#646) [Consensus] Configuration entry point state sync (pokt-network#528)

…p-modules * pokt/main: update pocket repo read.me (#667) Update reviewpad.yml [KEYBASE] Add improve comment on keybase config (#665) [E2E] Chore: Doc updates (#663) [E2E] Adds staking, unstaking, and sending tests (#653) [Utility][RPC][CLI] Querying governance parameters (Issue #619) (#622) [Persistence][Utility] Separate all CreateAndApply functions into more functional components - Issue #508 (#652) [Persistence][Utility] Pools Address hack removal + state accessor fix for params and flags (#654) [PERSISTENCE] SavePoints and Rollbacks design document (Issue #493) (#533) Update reviewpad.yml Added ChatGPT-CodeReview workflow (#649) Update reviewpad.yml Added default reviewpad.yml file (#648) [DevNet] tweaks for remote environments (#601) [Documentation] Swap validator and non-validator triggers when finished synching (#646) [Consensus] Configuration entry point state sync (#528)

deblasis added 16 commits April 6, 2023 10:55

refactor(shared): pools addresses + added missing fishermanstakepool

b0a6e2c

refactor(persistence): removed getaddress hack

def2418

chore(persistence): removed comment about the GetAddress hack

acca48d

refactor(shared): updating pools addresses to real addresses

7fecda7

fix(persistence): fixed pools test that was considering unspecified

94de517

refactor(utility): using Address() instead of FriendlyName()

32658ff

refactor(runtime): using Address() instead of FriendlyName()

858c20b

refactor(persistence): using Address() instead of FriendlyName()

b9c9bca

fix(persistence): params/keys keying fix (name vs hash)

bad1392

test(persistence): updated DeterministicStateWhenUpdatingAppStake

908e158

test(persistence): updated tests

6d4a459

refactor(shared): updated to use address instead of name

1b1b68f

refactor(utility): updated to use address

608fecc

refactor(shared): updated pools

04596df

refactor(persistence): updated to use address

e438506

refactor(runtime): updated generator

01f550d

deblasis added bug Something isn't working - expected behaviour is incorrect utility Utility specific changes persistence Persistence specific changes code health Nice to have code improvement labels Apr 6, 2023

deblasis added this to the M1: Pocket PoS (Proof of Stake) milestone Apr 6, 2023

deblasis self-assigned this Apr 6, 2023

github-actions bot previously requested changes Apr 6, 2023

View reviewed changes

reviewpad bot added the large label Apr 6, 2023

deblasis added the gpt review Triggers a code review from https://github.com/anc95/ChatGPT-CodeReview label Apr 6, 2023

deblasis added 3 commits April 6, 2023 13:37

chore(shared): lint

ed1b032

docs(build): CHANGELOG

30a02eb

docs(persistence): CHANGELOG

9f8115e

deblasis added 3 commits April 6, 2023 13:42

docs(runtime): CHANGELOG

548498d

docs(shared): CHANGELOG

f51c58d

docs(utility): CHANGELOG

132785e

deblasis marked this pull request as ready for review April 6, 2023 12:47

reviewpad bot requested a review from Olshansk April 6, 2023 12:48

reviewpad bot added the waiting-for-review label Apr 6, 2023

deblasis requested a review from dylanlott April 6, 2023 12:48

dylanlott approved these changes Apr 6, 2023

View reviewed changes

Olshansk approved these changes Apr 6, 2023

View reviewed changes

Update persistence/test/account_test.go

54e8431

Co-authored-by: Daniel Olshansky <[email protected]>

chore(shared): added TODO

733be50

Merge remote-tracking branch 'upstream/main' into issue/persistence_b…

d2caa53

…its_and_bobs1 Signed-off-by: Alessandro De Blasis <[email protected]>

github-actions bot reviewed Apr 6, 2023

View reviewed changes

deblasis merged commit 726619b into main Apr 6, 2023


		## [0.0.0.43] - 2023-04-06

		- Added `SAVEPOINTS_ROLLBACKS.md` design document

Conversation

deblasis commented Apr 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Issue

Type of change

List of changes

Testing

Required Checklist

If Applicable Checklist

Uh oh!

cr-gpt bot commented Apr 6, 2023

Uh oh!

github-actions bot left a comment

Choose a reason for hiding this comment

Uh oh!

reviewpad bot commented Apr 6, 2023

Uh oh!

cr-gpt bot commented Apr 6, 2023

Uh oh!

reviewpad bot commented Apr 6, 2023

Uh oh!

dylanlott left a comment

Choose a reason for hiding this comment

Uh oh!

dylanlott Apr 6, 2023

Choose a reason for hiding this comment

Uh oh!

Olshansk left a comment

Choose a reason for hiding this comment

Uh oh!

Olshansk Apr 6, 2023

Choose a reason for hiding this comment

Uh oh!

Olshansk Apr 6, 2023

Choose a reason for hiding this comment

Uh oh!

deblasis Apr 6, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

cr-gpt bot commented Apr 6, 2023

Uh oh!

reviewpad bot commented Apr 6, 2023

Uh oh!

cr-gpt bot commented Apr 6, 2023

Uh oh!

reviewpad bot commented Apr 6, 2023

Uh oh!

cr-gpt bot commented Apr 6, 2023

Uh oh!

github-actions bot Apr 6, 2023

Choose a reason for hiding this comment

Uh oh!

reviewpad bot commented Apr 6, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

deblasis commented Apr 6, 2023 •

edited

Loading