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

chore: introduce Submodule interface #855

Merged
merged 104 commits into from
Jul 11, 2023
Merged

Conversation

bryanchriswhite
Copy link
Contributor

@bryanchriswhite bryanchriswhite commented Jun 23, 2023

Description

Introduces the Submodule interface and applies it to the P2P module's peerstore provider.

Issue

Deliverables 1, 2, 3, & 5:

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

  • fixed typo; renamee IntegratableModule to IntegrableModule
  • renamed InitializableModule to InjectableModule
  • removed InjectableModule#Create() as Moudle also embeds ModuleFactoryWithConfig
  • added Submodule interface type
  • refactored peerstore providers as submodules
  • updated module README

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)

* pokt/main:
  [Persistence] TreeStore Refactor (#756)
  [General] Ignore false positive secret uncovered by `ggshield` on main (#848)
  [Utility] Add trustless relay to CLI (#778)
  Devlog 9 (#846)
  [Configs] Cleanup private keys and genesis file (#827)
* refactor/unicast-router:
  chore: cleanup TODOs
  [Persistence] TreeStore Refactor (#756)
  [General] Ignore false positive secret uncovered by `ggshield` on main (#848)
  [Utility] Add trustless relay to CLI (#778)
  Devlog 9 (#846)
  [Configs] Cleanup private keys and genesis file (#827)
@bryanchriswhite bryanchriswhite added core Core infrastructure - protocol related code health Nice to have code improvement labels Jun 23, 2023
@bryanchriswhite bryanchriswhite self-assigned this Jun 23, 2023
@reviewpad reviewpad bot added the medium Pull request is medium label Jun 23, 2023
@bryanchriswhite bryanchriswhite changed the title Chore/introduce submodule chore: introduce submodule Jun 23, 2023
@bryanchriswhite bryanchriswhite changed the title chore: introduce submodule chore: introduce Submodule interface Jun 23, 2023
@reviewpad reviewpad bot added large Pull request is large and removed medium Pull request is medium labels Jun 23, 2023
@bryanchriswhite bryanchriswhite added the do not merge Prevent merging even with sufficient approvals label Jun 23, 2023
* feat/integrate-bg-router:
  docs: clarify broadcast table
  docs: improve legend definitions
  chore: add TODO README
  docs: update TOC
  chore: add issue # to TECHDEBT comment
  chore: review feedback improvements
  chore: background router comment and var name cleanup
  docs: add architecture design language section
  docs: README improvements (review feedback)
@bryanchriswhite bryanchriswhite requested a review from Olshansk July 6, 2023 09:38
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 I pulled the branch and edited some formatting while reading through it.

I think there is still opportunity to simplify this in the future, but I've unfortunately reached the exhaustion level of reading about modules and submodules for now 😓

bryanchriswhite and others added 7 commits July 7, 2023 09:45
* pokt/main:
  [Utility] Update E2E feature path template doc (#870)
  [IBC] Add nil check on proof for membership and non-membership proof creation (#877)
  Added git diff state to devlog10
  Devlog 10 (#872)
  [Documentation] Add IBC Module introduction as an example (#853)
  [Persistence][Bug] Fix Actor Schema Assignment for ValidatorActor in GetActor (#857)
  QOL: add bash completion for p1 to localnet client (#865)
* feat/integrate-bg-router:
  fix: goimports
  fix: unstaked actor bootstrapping FSM transition
  chore: add error log
  test: improve background router validation test
  docs: fix mistake in peer discovery section
  [Utility] Update E2E feature path template doc (#870)
  [IBC] Add nil check on proof for membership and non-membership proof creation (#877)
  Added git diff state to devlog10
  Devlog 10 (#872)
  [Documentation] Add IBC Module introduction as an example (#853)
  [Persistence][Bug] Fix Actor Schema Assignment for ValidatorActor in GetActor (#857)
  QOL: add bash completion for p1 to localnet client (#865)
@@ -33,6 +33,10 @@ This document outlines how we structured the code by splitting it into modules,
- [Get the module `bus`](#get-the-module-bus)
- [Stop the module](#stop-the-module)

## tl;dr Just show me an example

If you're just interested in an example PR that introduced a new module to the codebase, see [#842](https://github.com/pokt-network/pocket/pull/842) which added the first iteration of the IBC module
Copy link
Contributor

Choose a reason for hiding this comment

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

🥹

Copy link
Contributor

@dylanlott dylanlott left a comment

Choose a reason for hiding this comment

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

LGTM 👍 Replacing providers with submodules seems worth an ADR, if you want help writing that up let's take a look at it @bryanchriswhite

* pokt/main:
  [P2P] Integrate background router (#732)
  Update main README.md
  [Bug] Fix CI linter errors (#885)
  [Tooling] Block `IN_THIS_*` comments from passing CI (#889)
@bryanchriswhite bryanchriswhite changed the base branch from feat/integrate-bg-router to main July 11, 2023 09:01
@bryanchriswhite bryanchriswhite removed the do not merge Prevent merging even with sufficient approvals label Jul 11, 2023
@gitguardian
Copy link

gitguardian bot commented Jul 11, 2023

⚠️ GitGuardian has uncovered 2 secrets following the scan of your pull request.

Please consider investigating the findings and remediating the incidents. Failure to do so may lead to compromising the associated services or software components.

🔎 Detected hardcoded secrets in your pull request
GitGuardian id Secret Commit Filename
5841025 Generic High Entropy Secret 4b19d8f charts/pocket/templates/configmap-genesis.yaml View secret
5841025 Generic High Entropy Secret cf886a7 charts/pocket/templates/configmap-genesis.yaml View secret
🛠 Guidelines to remediate hardcoded secrets
  1. Understand the implications of revoking this secret by investigating where it is used in your code.
  2. Replace and store your secrets safely. Learn here the best practices.
  3. Revoke and rotate these secrets.
  4. If possible, rewrite git history. Rewriting git history is not a trivial act. You might completely break other contributing developers' workflow and you risk accidentally deleting legitimate data.

To avoid such incidents in the future consider


🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.

Our GitHub checks need improvements? Share your feedbacks!

@bryanchriswhite
Copy link
Contributor Author

LGTM +1 Replacing providers with submodules seems worth an ADR, if you want help writing that up let's take a look at it @bryanchriswhite

@dylanlott it's more like the providers need to become submodules.

@bryanchriswhite bryanchriswhite merged commit c7412b6 into main Jul 11, 2023
@bryanchriswhite
Copy link
Contributor Author

🚨 Please don't delete this branch! I will do so when all downstream branches have been rebased, thank you! 🚨

bryanchriswhite added a commit that referenced this pull request Jul 11, 2023
* pokt/main:
  chore: introduce `Submodule` interface (#855)
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 e2e-devnet-test Runs E2E tests on devnet large Pull request is large push-image Build and push a container image on non-main builds waiting-for-review
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

[Core] Improve dependency injection for submodules
4 participants