search

pokt-network / pocket

Official implementation of the Pocket Network Protocol v1
https://pokt.network
MIT License
61 stars 33 forks source link

[P2P] refactor: peerstore provider (part 1) #804

Closed bryanchriswhite closed 1 year ago

bryanchriswhite commented 1 year ago

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

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

Issue

Realted:

Dependants:

Type of change

Please mark the relevant option(s):

List of changes

Testing

Required Checklist

If Applicable Checklist

codecov[bot] commented 1 year ago

Codecov Report

Patch coverage: 45.45% and project coverage change: -0.02 :warning:

Comparison is base (f72e1f0) 31.38% compared to head (2e4d60d) 31.37%.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #804 +/- ## ========================================== - Coverage 31.38% 31.37% -0.02% ========================================== Files 107 107 Lines 9080 9085 +5 ========================================== Hits 2850 2850 - Misses 5889 5895 +6 + Partials 341 340 -1 ``` | [Impacted Files](https://app.codecov.io/gh/pokt-network/pocket/pull/804?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pokt-network) | Coverage Δ | | |---|---|---| | [app/client/cli/debug.go](https://app.codecov.io/gh/pokt-network/pocket/pull/804?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pokt-network#diff-YXBwL2NsaWVudC9jbGkvZGVidWcuZ28=) | `9.90% <0.00%> (ø)` | | | [p2p/bootstrap.go](https://app.codecov.io/gh/pokt-network/pocket/pull/804?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pokt-network#diff-cDJwL2Jvb3RzdHJhcC5nbw==) | `43.66% <0.00%> (ø)` | | | [p2p/module.go](https://app.codecov.io/gh/pokt-network/pocket/pull/804?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pokt-network#diff-cDJwL21vZHVsZS5nbw==) | `67.04% <55.55%> (-1.28%)` | :arrow_down: |

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

bryanchriswhite commented 1 year ago

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]

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