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

[Consensus] Decouple ConsensusModule and PacemakerModule #395

Closed
9 tasks
jessicadaugherty opened this issue Dec 13, 2022 · 1 comment
Closed
9 tasks

[Consensus] Decouple ConsensusModule and PacemakerModule #395

jessicadaugherty opened this issue Dec 13, 2022 · 1 comment
Assignees
@gokutheengineer
Labels
code health Nice to have code improvement consensus Consensus specific changes
Milestone
M1: Pocket PoS (P...

Comments

@jessicadaugherty
Copy link
Contributor

jessicadaugherty commented Dec 13, 2022 •
edited
Loading

Objective

The pacemaker module and the consensus module are tightly coupled. By refactoring them and removing this inter-dependency, the codebase will be easier to maintain and update.

Origin Document

@gokutheengineer found a few opportunities to improve the codebase as a new engineer reading the consensus codebase

Image

Goals

  • Decouple the Consensus & Pacemaker module implementations
  • Create a codebase that is easier to maintain and update

Deliverable

  • Create a dedicated package for the pacemaker module
  • Communication between the pacemaker module and the consensus module should happen via the bus
  • Updated documentation (code tree structures)

Non-goals / Non-deliverables

  • Business logic changes to the pacemaker or consensus modules

General issue deliverables

  • Update the appropriate CHANGELOG(s)
  • Update any relevant local/global README(s)
  • Update relevant source code tree explanations
  • Add or update any relevant or supporting mermaid diagrams

Testing Methodology

  • All tests: make test_all
  • LocalNet: verify a LocalNet is still functioning correctly by following the instructions at docs/development/README.md

Creator: @jessicadaugherty
Co-owner: @Olshansk
@jessicadaugherty jessicadaugherty converted this from a draft issue Dec 13, 2022
@jessicadaugherty jessicadaugherty added consensus Consensus specific changes code health Nice to have code improvement labels Dec 13, 2022
@Olshansk Olshansk changed the title [Consensus] Refactor Pacemaker (Module) [Consensus] Decouple ConsensusModule and PacemakerModule Dec 13, 2022
@jessicadaugherty
Copy link
Contributor Author

2 potential paths:

  • Remove direct access to consensus from pacemaker
  • Decoupling/making separate modules
    • Semantic issue; only needed in this version of consensus

Conclusion: option 2 because decoupling them in scope of consensus will be useful in state sync implementation

@jessicadaugherty jessicadaugherty moved this from Up Next to In Progress in V1 Dashboard Dec 21, 2022
@gokutheengineer gokutheengineer mentioned this issue Dec 21, 2022
Open
7 tasks
This was referenced Jan 4, 2023
Merged
Open
@gokutheengineer gokutheengineer moved this from In Progress to In Review in V1 Dashboard Jan 6, 2023
gokutheengineer added a commit that referenced this issue Jan 24, 2023
@gokutheengineer @Olshansk
[Consensus] Decoupling the Pacemaker and Consensus modules (Synchrono…
1bf119b 
…us method) (#427)

## Description


This PR aims to decouple the `consensus` module and `pacemaker` module
by making the `pacemaker` module a submodule of the `consensus` module.

In a nutshell, this PR includes and introduces:
- The `Pacemaker` submodule
- A shared `ConsensusPacemaker` interface used by the `Pacemaker`
submodule
- Renames some fields to improving code quality

## Issue

Fixes (partially as intermediate step) #395

## Type of change

Please mark the relevant option(s):

- [] New feature, functionality or library
- [ ] Bug fix
- [x] Code health or cleanup
- [ ] Major breaking change
- [ ] Documentation


## List of changes

- Defined a new shared interface called `PaceMakerAccessModule` which is
implemented by `Consensus` module
- Removed the `consensus` module field from `Pacemaker` struct
- Made `pacemaker` a separate submodule
- Modified the `pacemaker` to access the `consensus` module in a
synchronous matter via the `PaceMakerAccessModule` interface

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

Co-authored-by: Daniel Olshansky <[email protected]>
Co-authored-by: Daniel Olshansky <[email protected]>
@gokutheengineer gokutheengineer moved this from In Review to Done in V1 Dashboard Jan 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@gokutheengineer gokutheengineer

Labels
code health Nice to have code improvement consensus Consensus specific changes
Projects
V1 Dashboard
Status: Done
Milestone
M1: Pocket PoS (Proof of Stake)
Development

No branches or pull requests
3 participants
@Olshansk @jessicadaugherty @gokutheengineer