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.

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

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

Merged
merged 16 commits into from
Jan 20, 2023

Conversation

h5law
Copy link
Contributor

@h5law h5law commented Jan 17, 2023

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):

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

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

  • 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 h5law added core Core infrastructure - protocol related persistence Persistence specific changes code health Nice to have code improvement labels Jan 17, 2023
@h5law h5law requested a review from Olshansk January 17, 2023 18:15
@h5law h5law self-assigned this Jan 17, 2023
@h5law
Copy link
Contributor Author

h5law commented Jan 17, 2023

@Olshansk I have also addressed the discussion comment in the persistence/account.go file and included the code for the relevant change if desired - commented out though. The current functionality is identical to how it was prior - only using shared logic instead. As the GetAccountAmount() and GetPoolAmount() will both by default return "0" (as before) this change seemed useful to include if desired I can uncomment or remove it.

@h5law h5law changed the title [Persistence] Consolidate common behaviour between Pool and Account into a shared interface (Issue #102) [Persistence] Consolidate common behaviour between Pool and Account into a shared interface (Issue #102) Jan 18, 2023
persistence/account.go Outdated Show resolved Hide resolved
persistence/account.go Outdated Show resolved Hide resolved
persistence/shared_sql.go Outdated Show resolved Hide resolved
persistence/types/pool.go Show resolved Hide resolved
persistence/types/base_account.go Outdated Show resolved Hide resolved
persistence/types/protocol_account.go Show resolved Hide resolved
persistence/types/shared_sql.go Outdated Show resolved Hide resolved
persistence/shared_sql.go Outdated Show resolved Hide resolved
persistence/shared_sql.go Outdated Show resolved Hide resolved
persistence/genesis.go Outdated Show resolved Hide resolved
@h5law h5law requested a review from Olshansk January 20, 2023 18:22
persistence/types/account_shared_sql.go Show resolved Hide resolved
persistence/types/base_account.go Outdated Show resolved Hide resolved
persistence/shared_sql.go Outdated Show resolved Hide resolved
Copy link
Member

@Olshansk Olshansk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@h5law LGTM!

Just a couple things regarding review:

  1. Make sure to re-request review when you need me to take another look
  2. Don't hesitate to resolve the comments yourself if your confident you addressed the comment
  3. Just double checking that LocalNet works fine? If so, go ahead and squash-and-merge!

@h5law
Copy link
Contributor Author

h5law commented Jan 20, 2023

@Olshansk

1. Make sure to re-request review when you need me to take another look
2. Don't hesitate to resolve the comments yourself if your confident you addressed the comment

I thought I did these but will make sure going forward 🫡

3. Just double checking that LocalNet works fine? If so, go ahead and squash-and-merge!

All good - will update CHANGELOGs and merge after that

@h5law h5law merged commit 4f2a1dd into pokt-network:main Jan 20, 2023
@h5law h5law deleted the pool_account_interface branch January 20, 2023 22:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code health Nice to have code improvement core Core infrastructure - protocol related persistence Persistence specific changes
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

[Persistence] Consolidate common behaviour between Pool and Account into a shared interface
2 participants