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

[P2P] Refactor CreateWithProvider using the new bus based DI #429

Closed
10 tasks
deblasis opened this issue Jan 6, 2023 · 1 comment · Fixed by #521
Closed
10 tasks

[P2P] Refactor CreateWithProvider using the new bus based DI #429

deblasis opened this issue Jan 6, 2023 · 1 comment · Fixed by #521
Assignees
@deblasis
Labels
code health Nice to have code improvement p2p P2P specific changes
Milestone
M1: Pocket PoS (P...

Comments

@deblasis
Copy link
Contributor

deblasis commented Jan 6, 2023

Objective

Improve code-health and consistency of patterns

Origin Document

While developing #402 and #331 I had to inject some dependencies into the P2P module. This was our first use case in our codebase and the initial approach was indeed marked as a HACK from the beginning but there was something missing that is now available via #425: the ability to register modules dynamically on the bus instance, effectively enabling Dependency Injection

This ticket was descoped from #425 which became quite large and scope-creepy:

#425 (comment)

Goals

  • Remove the CreateWithProviders constructor and use the standardized Create instead.
  • Refactoring the way we access the dependencies AddrBookProvider and CurrentHeightProvider by registering them at the caller site and consuming them inside the P2P Create()

Deliverable

  • Refactored P2P module Create
  • Dependency injection for AddrBookProvider and CurrentHeightProvider

Non-goals / Non-deliverables

  • Changes in P2P logic

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

  • Task specific tests or benchmarks: make ...
  • New tests or benchmarks: 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 the p2p P2P specific changes label Jan 6, 2023
@deblasis deblasis self-assigned this Jan 6, 2023
deblasis added a commit that referenced this issue Jan 6, 2023
@deblasis
refactor(P2P): marking // TODO(#429):
96cd1be
@deblasis deblasis mentioned this issue Jan 6, 2023
Merged
16 tasks
@Olshansk Olshansk added the code health Nice to have code improvement label Jan 6, 2023
deblasis added a commit that referenced this issue Jan 9, 2023
@deblasis
docs(P2P): marking todo #429
c76a391
deblasis added a commit that referenced this issue Jan 9, 2023
@deblasis
docs(P2P): cleanup (#429)
90d331d
@deblasis deblasis mentioned this issue Feb 6, 2023
Closed
16 tasks
@deblasis deblasis mentioned this issue Feb 16, 2023
Merged
16 tasks
@deblasis deblasis linked a pull request Feb 16, 2023 that will close this issue
[P2P] KISS 2 - Peer discovery [Merge me after #520] - (Issues: #416, #429) #521
Merged
16 tasks
bryanchriswhite added a commit that referenced this issue Feb 20, 2023
@bryanchriswhite
Merge remote-tracking branch 'pokt/main' into chore/libp2p
e8a19b9 
* pokt/main:
  [Infra] KISS 3 - Cluster Manager [Merge me after #521] - (Issues: #490) (#522)
  Refactor/fix state sync logs (#515)
  [P2P] KISS 2 - Peer discovery [Merge me after #520] - (Issues: #416, #429) (#521)
  [Core] KISS 1 - Finite State Machine [Merge me first] - (Issue: #499) (#520)
  [CLI] Stake command bugfix (#518)
  [CLI] Cannot run make localnet_client_debug: Cannot initialise the keybase with the validator keys: Unable to find YAML file (#517)
  Fix the link shown by `make go_doc`
  Fixed duplicate GITHUB_WIKI tag
  [Documentation] Update Devlog Formatting (#512)
  [Docs & Bugs] Minor fixes post keybase changes (#513)
  [Utility] Foundational bugs, tests, code cleanup and improvements (1 / 2) (#503)
  [Tooling] Integrate Keybase w/ CLI (Issue #484 ) (#501)
  update devlog2.md
  update devlog2.md
  Update devlog1.md
bryanchriswhite added a commit that referenced this issue Feb 20, 2023
@bryanchriswhite
Merge remote-tracking branch 'pokt/main' into chore/libp2p
b0dbfec 
* pokt/main:
  [Infra] KISS 3 - Cluster Manager [Merge me after #521] - (Issues: #490) (#522)
  Refactor/fix state sync logs (#515)
  [P2P] KISS 2 - Peer discovery [Merge me after #520] - (Issues: #416, #429) (#521)
  [Core] KISS 1 - Finite State Machine [Merge me first] - (Issue: #499) (#520)
  [CLI] Stake command bugfix (#518)
  [CLI] Cannot run make localnet_client_debug: Cannot initialise the keybase with the validator keys: Unable to find YAML file (#517)
  Fix the link shown by `make go_doc`
  Fixed duplicate GITHUB_WIKI tag
  [Documentation] Update Devlog Formatting (#512)
  [Docs & Bugs] Minor fixes post keybase changes (#513)
  [Utility] Foundational bugs, tests, code cleanup and improvements (1 / 2) (#503)
  [Tooling] Integrate Keybase w/ CLI (Issue #484 ) (#501)
  update devlog2.md
  update devlog2.md
  Update devlog1.md
@jessicadaugherty
Copy link
Contributor

scope-creepy is my new favorite phrase haha

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@deblasis deblasis

Labels
code health Nice to have code improvement p2p P2P specific changes
Projects
V1 Dashboard
Status: Done
Milestone
M1: Pocket PoS (Proof of Stake)
3 participants
@Olshansk @deblasis @jessicadaugherty