refactor(networking): unify peer data models, remove StoredInfo #1597
Conversation
alrevuelta
force-pushed
the
unify-peer-types
branch
2 times, most recently
from
1aa7301
to
c4c65f8
Compare
alrevuelta
force-pushed
the
unify-peer-types
branch
from
c4c65f8
to
36b8315
Compare
There was a problem hiding this comment.
Thanks! This improves consistency. I wouldn't couple concepts around PeerBooks with the Peer though and leave those as part of the waku_peer_store.
Side note: Fwiw, I've never been fully convinced of the term RemotePeerInfo - it was introduced to differentiate it from PeerInfo, which became (but wasn't always) solely local peer info. Perhaps we can think of a better term at some point...
waku/v2/utils/peers.nim
Outdated
| # Keeps track of the Connectedness state of a peer | ||
| ConnectionBook* = ref object of PeerBook[Connectedness] | ||
|
|
||
| # Last failed connection attemp timestamp | ||
| LastFailedConnBook* = ref object of PeerBook[Moment] | ||
|
|
||
| # Failed connection attempts | ||
| NumberFailedConnBook* = ref object of PeerBook[int] | ||
|
|
||
| # Keeps track of when peers were disconnected in Unix timestamps | ||
| DisconnectBook* = ref object of PeerBook[int64] | ||
|
|
||
| # Keeps track of the origin of a peer | ||
| SourceBook* = ref object of PeerBook[PeerOrigin] | ||
|
|
||
| # Direction | ||
| DirectionBook* = ref object of PeerBook[PeerDirection] | ||
|
|
||
| # ENR Book | ||
| ENRBook* = ref object of PeerBook[enr.Record] |
There was a problem hiding this comment.
Even though the peers util module itself should probably move closer to the peer_manager, I don't think the concept of a PeerBook should mix with the concept of a Peer. In other words, I'd leave the Books part of waku_peer_store for now.
There was a problem hiding this comment.
You are totally right, fixed c5a699c
(I kind of regret using the peerbook, but that's another story :P)
There was a problem hiding this comment.
Overall, LGTM ✅
We should have a conversation about the "Peer" data model. I concur with @jm-clius. The RemotePeerInfo type overlaps with libp2p's PeerId.
As I see it:
We should come up with a "waku peer type" (e.g., as simple as
Peer) that should be compatible ad convertible to/from and contain the following peer types and peer info formats:
- nim-libp2p's
PeerId- nim-ibp2p's Multiaddress
- nim-eth
PeerId- nim-eth ENR
RecordThis also affects the key types. Additionally, we should come up with a "waku key type" (e.g., as simple as
Key) that should be compatible ad convertible to/from and contain the following key types:
- nim-eth key types
- nim-libp2p key types
Summary:
RemotePeerInfoandStoredInfostore very similar content, being totally redundant.RemotePeerInfoandStoredInfo, removing the later leaving onlyRemotePeerInfo.