[P2P] refactor: peerstore provider (part 1)

[bryanchriswhite]

Description

1. Simplify PeerstoreProvider interface
2. Extend it to support retrieval of the unstaked actor peerstore
3. Implement the extended interface in persistencePeerstoreProvider

Before

classDiagram 

class InterruptableModule {
    <<interface>>
    Start() error
    Stop() error
}
class IntegratableModule {
    <<interface>>
    +SetBus(bus Bus)
    +GetBus() Bus
}
class InitializableModule {
    <<interface>>
    +GetModuleName() string
    +Create(bus Bus, opts ...Option) (Module, error)
}
class Module {
    <<interface>>
}

Module --|> InitializableModule
Module --|> IntegratableModule
Module --|> InterruptableModule


class PeerstoreProvider {
    <<interface>>
    +GetStakedPeerstoreAtHeight(height int) (Peerstore, error)
    +GetP2PConfig() *P2PConfig
}

class persistencePeerstoreProvider

class rpcPeerstoreProvider

persistencePeerstoreProvider --|> PeerstoreProvider

rpcPeerstoreProvider --|> PeerstoreProvider
PeerstoreProvider --|> Module

Loading

After

classDiagram 

class IntegratableModule {
    <<interface>>
    +GetBus() Bus
    +SetBus(bus Bus)
}

class PeerstoreProvider {
    <<interface>>
    +GetStakedPeerstoreAtHeight(height int) (Peerstore, error)
    +GetUnstakedPeerstore() (Peerstore, error)
}

class persistencePeerstoreProvider
class rpcPeerstoreProvider
class p2pModule

class unstakedPeerstoreProvider {
    <<interface>>
    +GetUnstakedPeerstore() (Peerstore, error)
}

persistencePeerstoreProvider --|> PeerstoreProvider
persistencePeerstoreProvider --> p2pModule : from Bus
rpcPeerstoreProvider --> p2pModule : from Bus
p2pModule --|> unstakedPeerstoreProvider

rpcPeerstoreProvider --|> PeerstoreProvider
rpcPeerstoreProvider --|> Module
PeerstoreProvider --|> IntegratableModule

class Module {
    <<interface>>
}

Module --|> InitializableModule
Module --|> IntegratableModule
Module --|> InterruptableModule

Loading

Issue

Realted:

• [Core] Improve dependency injection for submodules #810

Dependants:

• [P2P] Background gossip using libp2p-pubsub #505
• [Client] [Tooling] refactor: CLI #806

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

• Replaced embedded modules.Module with simpler modules.IntegratableModule in PeerstoreProvider interface
• Removed unused PeerstoreProvider#GetP2PConfig() method
• Added PeerstoreProvider#GetUnstakedPeerstore() method
• Added p2pPeerstoreProvider implementation of PeerstoreProvider interface
• Added Factory generic type

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]

I threw together a mindmap of my initial thinking, which was mainly focusd on whether this new API should be a debug-only thing (as it's being used in a "debug CLI" scenario). TLDR; I think "no" is the answer I landed on (plus some additional thinking):

mindmap
Q: should p2p peerstore provider be debug only?
    NO
        REASONING
            Will likely fit better with future versions / refactors
                r1(**Perhaps it would and/or already does make sense to split PeerstoreProvider into StakedPeerstoreProvider and UnstakedPeerstoreProvider)
        CONSEQUENCES
            c1[MUST design a non-debug way to access p2p module's peerstores]
                eg[e.g.: add GetUnstakedPeerstore method to modules.P2PModule]
                    Does this fight against peerstore provider / modular design?
                        YES because...
                            p[provides multiple ways to access the same thing: un/staked-actor-router peerstore]
                        NO because...
                            allows any module to get either peerstore without having to know anything about p2p module
                            no other single module can support this interface
                                justified by the need in the CLI
                    Alternatives
                        ?
            Makes Peerstore interface type "module shared"; i.e. it must be moved to another package avoid potential import cycles
            w[WORKAROUND: in #804, I'm working around the need for this change by defining a consumer-side interface which I assert that the P2P module implements, in addition to its module interface]

    YES
        REASONING
            r1[Only currently needed in a low-level / debug scenarios]
        CONSEQUENCES
            c1[Simpler implementation; no need for PeerstoreProvider interface refactor #804]
            c2[Harder to maintain and more complex to reason about; would likely rely on build tags]

Loading

** Here's what splitting PeerstoreProvider into StakedPeerstoreProvider and UnstakedPeerstoreProvider would look like:

classDiagram 

class IntegratableModule {
    <<interface>>
    +SetBus(bus Bus)
    +GetBus() Bus
}

class PeerstoreProvider {
    <<interface>>
    
}

class StakedPeerstoreProvider {
    <<interface>>
    +GetStakedPeerstoreAtHeight(height int) (Peerstore, error)
}

class UnstakedPeerstoreProvider {
    <<interface>>
    +GetUnstakedPeerstore() (Peerstore, error)
}

class persistencePeerstoreProvider
class rpcPeerstoreProvider
class p2pPeerstoreProvider {
  -persistencePeerstoreProvider
  -p2pModule P2PModule
}
class p2pModule

p2pPeerstoreProvider --o p2pModule
p2pPeerstoreProvider --* persistencePeerstoreProvider
p2pPeerstoreProvider --|> PeerstoreProvider

rpcPeerstoreProvider --|> StakedPeerstoreProvider
persistencePeerstoreProvider --|> StakedPeerstoreProvider

PeerstoreProvider --* StakedPeerstoreProvider
PeerstoreProvider --* UnstakedPeerstoreProvider

PeerstoreProvider --|> IntegratableModule
StakedPeerstoreProvider --|> IntegratableModule
UnstakedPeerstoreProvider --|> IntegratableModule

Loading

[dylanlott]

First round feedback, waiting for E2E test results 👍

[dylanlott]

Nit: consider passing the bus to the Errrof for debugging purposes.

[bryanchriswhite]

The bus doesn't implement Stringer, what do you expect/want the DX to look/feel like?

[Olshansk]

☝️ The bus is just a point to our "DI framework" and "pubsub" so curious what you had in mind for this @dylanlott?

[Olshansk]

On the topic of "not being afraid to change everything", take into consideration that maybe "providers" aren't the best approach with our bus based system.

Not making a comment on removing/extending them, but just sharing that it is not set in stone so don't feel constrained.