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 P2P submodules #895

Merged
merged 9 commits into from
Jul 13, 2023
Merged

[P2P] Refactor P2P submodules #895

merged 9 commits into from
Jul 13, 2023

Conversation

bryanchriswhite
Copy link
Contributor

@bryanchriswhite bryanchriswhite commented Jul 11, 2023 •
edited
Loading

@Reviewer

This PR may be more digestible / reviewable on a commit-by-commit basis. Commits are organized logically and any given line is only modified in a single commit, with few exceptions*.

*(In the interest of preserving the git-time-continuum 👮🚨, this applies in batches of commits between comments or reviews by humans, only once "in review")

Description

Refactor P2P module dependencies as submodules, ultimately to support usage P2P module usage in the CLI.

Issue

Related:

Dependant(s):

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

  • decoupled P2P module dependencies from Consensus module (disambiguates modules registry names)
    • added "current_height_provider" module registry slot
    • promoted CurrentHeightProvider to a submodule interface type
    • added consensusCurrentHeightProvider implementation
  • promoted Router interface to a submodule interface type
    • added "staked_actor_router" modules registry slot
    • added "unstaked_actor_router" modules registry slot
    • converted backgroundRouter implementation to submodule
    • converted rainTreeRouter implementation to submodule
    • simplified router base config

Testing

  • make develop_test; if any code changes were made
  • make test_e2e on k8s LocalNet; if any code changes were made
  • e2e-devnet-test passes tests on DevNet; if any code was changed
  • Docker Compose LocalNet; if any major functionality was changed or introduced
  • k8s LocalNet; if any infrastructure or configuration changes were made

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 added, or updated, godoc format comments on touched members (see: tip.golang.org/doc/comment)
  • 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)

@bryanchriswhite bryanchriswhite added p2p P2P specific changes code health Nice to have code improvement labels Jul 11, 2023
@bryanchriswhite bryanchriswhite self-assigned this Jul 11, 2023
@reviewpad reviewpad bot added the large Pull request is large label Jul 11, 2023
@bryanchriswhite bryanchriswhite linked an issue Jul 11, 2023 that may be closed by this pull request
[P2P] Peer discovery debug CLI sub-commands #730
Open
10 tasks
@bryanchriswhite bryanchriswhite force-pushed the refactor/p2p-submodules branch from 0992775 to e11a55a Compare July 11, 2023 14:48
@bryanchriswhite bryanchriswhite added push-image Build and push a container image on non-main builds e2e-devnet-test Runs E2E tests on devnet labels Jul 11, 2023
@bryanchriswhite bryanchriswhite force-pushed the refactor/p2p-submodules branch 5 times, most recently from 3f0dd91 to 44a2750 Compare July 12, 2023 08:21
@bryanchriswhite bryanchriswhite force-pushed the refactor/p2p-submodules branch from 44a2750 to 8bfecbf Compare July 12, 2023 08:53
@bryanchriswhite bryanchriswhite force-pushed the refactor/p2p-submodules branch from 8bfecbf to 6f87169 Compare July 12, 2023 09:53
@bryanchriswhite bryanchriswhite marked this pull request as ready for review July 12, 2023 10:12
Olshansk
Olshansk approved these changes Jul 12, 2023
View reviewed changes
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.

@bryanchriswhite Don't really have any good / constructive feedback on this. As long as things work, superficially this looks pretty good!

p2p/utils_test.go
mockModulesRegistry.EXPECT().GetModule(current_height_provider.ModuleName).Return(nil, runtime.ErrModuleNotRegistered(current_height_provider.ModuleName)).AnyTimes()
mockBus.EXPECT().GetModulesRegistry().Return(mockModulesRegistry).AnyTimes()
modulesRegistry := runtime.NewModulesRegistry()
mockBus.EXPECT().
Copy link
Member

Choose a reason for hiding this comment

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

I'm 👁️ ing patterns emerge!

p2p/types/router.go Outdated Show resolved Hide resolved
p2p/types/router.go Outdated Show resolved Hide resolved
consensus/module.go Outdated Show resolved Hide resolved
p2p/module.go Outdated Show resolved Hide resolved
@bryanchriswhite bryanchriswhite mentioned this pull request Jul 13, 2023
Merged
20 tasks
@bryanchriswhite @Olshansk
chore: review feedback improvments
0a0c9d3 
- comment updates
- one-lining error checks

Co-authored-by: Daniel Olshansky <[email protected]>
@bryanchriswhite
Merge remote-tracking branch 'pokt/main' into refactor/p2p-submodules
0f89875 
* pokt/main:
  [IBC] Make the IBC Host a submodule with access to the bus (#868)
  [IBC] Implement ICS-24 - Tracking IBC store transitions in the network state (#847)
@bryanchriswhite bryanchriswhite merged commit 41c3d32 into main Jul 13, 2023
@bryanchriswhite bryanchriswhite deleted the refactor/p2p-submodules branch July 13, 2023 12:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Olshansk Olshansk Olshansk approved these changes

@dylanlott dylanlott Awaiting requested review from dylanlott

Assignees

@bryanchriswhite bryanchriswhite

Labels
code health Nice to have code improvement e2e-devnet-test Runs E2E tests on devnet large Pull request is large p2p P2P specific changes push-image Build and push a container image on non-main builds waiting-for-review
Projects
V1 Dashboard
Status: Done
Milestone
M1: Pocket PoS (Proof of Stake)
Development

Successfully merging this pull request may close these issues.

[P2P] Peer discovery debug CLI sub-commands
2 participants
@bryanchriswhite @Olshansk