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

[TECHDEBT] [P2P] P2P integration with utility for addressBook lookups #270

Closed
8 tasks
deblasis opened this issue Oct 3, 2022 · 1 comment · Fixed by #378
Closed
8 tasks

[TECHDEBT] [P2P] P2P integration with utility for addressBook lookups #270

deblasis opened this issue Oct 3, 2022 · 1 comment · Fixed by #378
Assignees
@deblasis
Labels
p2p P2P specific changes utility Utility specific changes
Milestone
M1: Pocket PoS (P...

Comments

@deblasis
Copy link
Contributor

deblasis commented Oct 3, 2022 •
edited by jessicadaugherty
Loading

Objective

Tend for the TODO

// CLEANUP(drewsky): These functions will turn into more of a "ActorToAddrBook" when we have a closer
// integration with utility.

Origin Document

Techdebt week emphasizes code health. Particularly, removing all placeholder and hacky code.

The past few months, shared/modules have built up quite a few TODOs.

The PR should remove all non-blocked TODOs

Goals

  • Address tech debt

Deliverable

  • Rename ValidatorMapToNetworkPeer to ActorMapToNetworkPeer or ActorsToNetworkPeer
  • Look into the changing the interface call from ConsensusModule.ValidatorMap() to PersistenceModule.GetActorsAtHeight() because all the staked protocol level actors can help in broadcasting messages.

Non-goals / Non-deliverables

General issue deliverables

  • Update the appropriate CHANGELOG
  • Update any relevant READMEs (local and/or global)
  • Update any relevant global documentation & references
  • If applicable, update the source code tree explanation
  • If applicable, add or update a state, sequence or flowchart diagram using mermaid

[Optional] Testing Methodology

  • _REPLACE_ME: Make sure to update the testing methodology appropriately_
  • Task specific tests: make ...
  • All tests: make test_all
  • LocalNet: verify a LocalNet is still functioning correctly by following the instructions at docs/development/README.md

Creator: @deblasis
Co-Owners: @Olshansk
@deblasis deblasis added p2p P2P specific changes utility Utility specific changes labels Oct 3, 2022
@deblasis deblasis self-assigned this Oct 3, 2022
@deblasis deblasis mentioned this issue Oct 3, 2022
Closed
7 tasks
@Olshansk Olshansk moved this to Backlog in V1 Dashboard Oct 4, 2022
@Olshansk
Copy link
Member

Olshansk commented Oct 5, 2022

The action item for this ticket is:

  1. Rename ValidatorMapToNetworkPeer to ActorMapToNetworkPeer or ActorsToNetworkPeer

  2. Look into the changing the interface call from ConsensusModule.ValidatorMap() to PersistenceModule.GetActorsAtHeight() because all the staked protocol level actors can help in broadcasting messages.

@andrewnguyen22 Do you think we should only have validators participating in RainTree or any protocol level actor that stakes.

@Olshansk Olshansk moved this from Backlog to In Research in V1 Dashboard Oct 5, 2022
@Olshansk Olshansk removed the triage It requires some decision-making at team level (it can't be worked on as it stands) label Oct 5, 2022
@jessicadaugherty jessicadaugherty moved this from In Research to Up Next in V1 Dashboard Oct 11, 2022
@jessicadaugherty jessicadaugherty moved this from Up Next to In Progress in V1 Dashboard Nov 29, 2022
@deblasis deblasis mentioned this issue Dec 5, 2022
Merged
16 tasks
@deblasis deblasis moved this from In Progress to In Review in V1 Dashboard Dec 5, 2022
@deblasis deblasis mentioned this issue Dec 7, 2022
Merged
16 tasks
deblasis added a commit that referenced this issue Dec 16, 2022
@deblasis @Olshansk
[TECHDEBT] [P2P] P2P integration with utility for addressBook lookups…
a3594e6 
… - (Issue #270) (#378)

## Description

This PR builds on top of #374 (the base branch is not `main` in order to
highlight the delta of the changes) in order to tend to the TECHDEBT
TODOs listed in #270

## Issue

Fixes #270 

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

- Consensus
- `LeaderElectionModule`'s `electNextLeaderDeterministicRoundRobin` now
uses `Persistence` to access the list of validators instead of the
static `ValidatorMap`.
  - Updated tests (mock configs) accounting for the updated logic
- P2P
  - `ValidatorMapToAddrBook` renamed to `ActorToAddrBook`
  - `ValidatorToNetworkPeer` renamed to `ActorToNetworkPeer`

## 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`
<!--
If you added additional tests or infrastructure, describe it here.

Bonus points for images and videos or gifs.
-->

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

Signed-off-by: Alessandro De Blasis <[email protected]>
Co-authored-by: Daniel Olshansky <[email protected]>
Repository owner moved this from In Review to Done in V1 Dashboard Dec 16, 2022
Olshansk added a commit that referenced this issue Dec 16, 2022
@deblasis @Olshansk
[TECHDEBT] [P2P] P2P integration with utility for addressBook lookups…
ae35a5b 
… - (Issue #270) (#378)

This PR builds on top of #374 (the base branch is not `main` in order to
highlight the delta of the changes) in order to tend to the TECHDEBT
TODOs listed in #270

Fixes #270

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

- Consensus
- `LeaderElectionModule`'s `electNextLeaderDeterministicRoundRobin` now
uses `Persistence` to access the list of validators instead of the
static `ValidatorMap`.
  - Updated tests (mock configs) accounting for the updated logic
- P2P
  - `ValidatorMapToAddrBook` renamed to `ActorToAddrBook`
  - `ValidatorToNetworkPeer` renamed to `ActorToNetworkPeer`

- [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`
<!--
If you added additional tests or infrastructure, describe it here.

Bonus points for images and videos or gifs.
-->

- [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

- [ ] 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)

Signed-off-by: Alessandro De Blasis <[email protected]>
Co-authored-by: Daniel Olshansky <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@deblasis deblasis

Labels
p2p P2P specific changes utility Utility specific changes
Projects
V1 Dashboard
Status: Done
Milestone
M1: Pocket PoS (Proof of Stake)
2 participants
@Olshansk @deblasis