Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[Persistence] Consolidate common behaviour between Pool and Account into a shared interface #102

Closed
5 tasks
Olshansk opened this issue Jul 1, 2022 · 7 comments · Fixed by #442
Closed
5 tasks

[Persistence] Consolidate common behaviour between Pool and Account into a shared interface #102

Olshansk opened this issue Jul 1, 2022 · 7 comments · Fixed by #442
Assignees
@h5law
Labels
code health Nice to have code improvement community Open to or owned by a non-core team member core starter task Good for newcomers, but aimed at core team members though still open for everyone core Core infrastructure - protocol related persistence Persistence specific changes
Milestone
M*: Pocket QoL (Q...

Comments

@Olshansk
Copy link
Member

Olshansk commented Jul 1, 2022 •
edited by jessicadaugherty
Loading

Objective

Remove redundant code used by the Pool and Account actors in the persistence module introduced in #73.

This is a good starter task opportunity to get acquainted with the codebase in the persistence module.

Origin Document

From the utility specification, a Pool is described as:

	A ModulePool is a particular type that though similar in structure to an Account, the
	functionality of each is quite specialized to its use case. These pools are maintained by
	the protocol and are completely autonomous, owned by no actor on the network. Unlike Accounts,
	tokens are able to be directly minted to and burned from ModulePools. Examples of ModuleAccounts
	include StakingPools and the FeeCollector

Similar to how the functionality of Fisherman, ServiceNode, Validator and Application was consolidated via persistence/schema/protocol_actor.go and persistence/schema/base_actor.go, the functionality of Pool and Account can also be shared to remove redundant code.

Goals

Deliverables

  • Extract common functionality of Pool and Account into a common interface
  • At a minimum, the following files will need to be affected: persistence/schema/account.go, persistence/account.go, persistence/test/account_test.go

Testing Methodology

  • $ make test_persistence
  • $ make test_all
  • Run a LocalNet following the instructions in docs/development/README.md

Non-goals

  • Adding new functionality to the persistence module

Creator: @Olshansk
Owner: @DragonDmoney
Co-Owners: @andrewnguyen22
@Olshansk Olshansk added persistence Persistence specific changes code health Nice to have code improvement priority:medium labels Jul 1, 2022
@Olshansk Olshansk added this to the V1 Persistence Foundation milestone Jul 6, 2022
@DragonDmoney
Copy link

Note: I am currently working on this.

@Olshansk Olshansk moved this to In Progress in V1 Dashboard Jul 13, 2022
@Olshansk
Copy link
Member Author

+1 Updated you as an owner in the description changed this to in progress

@Olshansk Olshansk self-assigned this Jul 16, 2022
@Olshansk Olshansk added the core Core infrastructure - protocol related label Jul 26, 2022
@Olshansk Olshansk mentioned this issue Jul 27, 2022
Closed
6 tasks
@Olshansk Olshansk added the community Open to or owned by a non-core team member label Aug 3, 2022
@Olshansk
Copy link
Member Author

Olshansk commented Aug 3, 2022

@DragonDmoney Do you think this is something you can have up for review by the end of the month?

@andrewnguyen22
Copy link
Contributor

Somewhat tackled in #154 by merging the two structures in protobuf. Will leave the rest to #163

@Olshansk
Copy link
Member Author

Spoke to @DragonDmoney offline and he mentioned he won't be working on this any longer.

@Olshansk Olshansk removed their assignment Aug 22, 2022
@Olshansk Olshansk moved this from In Progress to Todo in V1 Dashboard Aug 23, 2022
@Olshansk Olshansk added the core starter task Good for newcomers, but aimed at core team members though still open for everyone label Oct 7, 2022
This was referenced Jan 12, 2023
Open
Open
Open
@h5law
Copy link
Contributor

h5law commented Jan 16, 2023

@Olshansk I would like to take a swing at this issue if its still relevant? I have working code that passes tests ready to submit a PR if its still applicable?

@Olshansk
Copy link
Member Author

@Olshansk I would like to take a swing at this issue if its still relevant? I have working code that passes tests ready to submit a PR if its still applicable?

Still applicable, let's do it!

@h5law h5law mentioned this issue Jan 17, 2023
Merged
16 tasks
@jessicadaugherty jessicadaugherty added community Open to or owned by a non-core team member starter task core starter task Good for newcomers, but aimed at core team members though still open for everyone and removed community Open to or owned by a non-core team member starter task core starter task Good for newcomers, but aimed at core team members though still open for everyone labels Jan 17, 2023
h5law added a commit that referenced this issue Jan 20, 2023
@h5law
[Persistence] Consolidate common behaviour between Pool and `Accoun…
4f2a1dd 
…t` into a shared interface (Issue #102) (#442)

## Description

This PR addresses issue #102. It consolidates `Account` and `Pool` to
use a shared interface `ProtocolAccountSchema` that generalises the
functions that both actors use.

The `ProtocolAccountSchema` interface is implemented by the
`baseProtocolAccountSchema` struct - following the same pattern laid out
by the `ProtocolActorSchema` and `baseProtocolActorSchema`. Two new
types are then created - `Account` and `Pool` these are instances of the
`baseProtocolAccountSchema` struct and are initiated with their relevant
`SQL` table names, height constraints and column names.

The functions in the `PersistenceRWContext` for both `Account` and
`Pool` utilities are now generalised to use shared logic dramatically
reducing the code footprint.

In addition to this all relevant unit tests have been updated and usages
of these functions are working correctly. Also regarding issue #149
`address []bytes` has been removed from `InsertPool`.

## Issue

Fixes #102 

## Type of change

Please mark the relevant option(s):

- [x] New feature, functionality or library
- [ ] Bug fix
- [x] Code health or cleanup
- [ ] Major breaking change
- [ ] Documentation
- [ ] Other <!-- add details here if it a different type of change -->

## List of changes

- Create `ProtocolAccountSchema` interface and
`baseProtocolAccountSchema` struct that implements this interface
- Create `Account` and `Pool` types that share the common functionality
of `baseProtocolAccountSchema`
- Generalise all `Account` and `Pool` functions to use shared logic
- Remove `address []byte` from `InsertPool()`
- Seperate `shared_sql.go` and `types/shared_sql.go` into
`account_shared_sql.go` and `actor_shared_sql.go` as well as
`types/account_shared_sql.go` and `types/actor_shared_sql.go`
respectively

## Testing

- [x] `make develop_test`
- [x]
[LocalNet](https://github.com/pokt-network/pocket/blob/main/docs/development/README.md)
w/ all of the steps outlined in the `README`

## Required Checklist

- [x] I have performed a self-review of my own code
- [x] I have commented my code, particularly in hard-to-understand areas
- [x] I have tested my changes using the available tooling
- [x] 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](https://mermaid-js.github.io) diagrams in the corresponding
README(s)
- [ ] I have added, or updated, documentation and
[mermaid.js](https://mermaid-js.github.io) diagrams in `shared/docs/*`
if I updated `shared/*`README(s)
@github-project-automation github-project-automation bot moved this from Backlog to Done in V1 Dashboard Jan 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@h5law h5law

Labels
code health Nice to have code improvement community Open to or owned by a non-core team member core starter task Good for newcomers, but aimed at core team members though still open for everyone core Core infrastructure - protocol related persistence Persistence specific changes
Projects
V1 Dashboard
Status: Done
Milestone
M*: Pocket QoL (Quality of Life)
6 participants
@Olshansk @Jasonyou1995 @andrewnguyen22 @DragonDmoney @h5law @jessicadaugherty