-
Notifications
You must be signed in to change notification settings - Fork 54
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
Address Book POC implementation #499
Conversation
libp2p/peerstore.nim
Outdated
|
||
PeerStore* = ref object of RootObj | ||
addressBook*: AddressBook | ||
listeners: ref seq[EventListener] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unless you're planning on passing listeners
as an argument you probably don't need a ref
here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, make sense. Fixed in 29dcc33.
libp2p/peerstore.nim
Outdated
addrChange: addrChange) | ||
|
||
proc init*(T: type PeerStore): PeerStore = | ||
var listeners: ref seq[EventListener] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just to reiterate, you don't need a ref
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in 29dcc33.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Semantic wise looks reasonable to me! I'll defer to @dryajov re nim-libp2p specifics.
# Listener and handler types # | ||
############################## | ||
|
||
AddrChangeHandler* = proc(peerId: PeerID, multiaddrs: HashSet[MultiAddress]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this communicating change per se (as in a delta) or is the set here the sum of all, existing and new, multiaddresses?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, yes, this could have been clearer. It's not merely the delta, but communicates all known multiaddrs per peer whenever there's a change as per js-implementation.
Codecov Report
@@ Coverage Diff @@
## master #499 +/- ##
==========================================
- Coverage 86.23% 86.19% -0.04%
==========================================
Files 49 49
Lines 11672 11672
==========================================
- Hits 10065 10061 -4
- Misses 1607 1611 +4
|
@dryajov any further changes from your side, or can this be merged? |
|
||
AddrChangeHandler* = proc(peerId: PeerID, multiaddrs: HashSet[MultiAddress]) | ||
|
||
EventListener* = object |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if we need an object to encapsulate the event's callback, it doesn't cary any additional information and can be easily added as parameter to the callback later on.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, sorry, I somehow missed this comment.
I see your point. The encapsulating "listener" was created to hold all event handlers that may be defined for other books (keyChange
, protoChange
, etc.) so that the Peer Store wouldn't need a separate collection for each type of handler. Can easily change to such an implementation though.
(Going to merge this for now to unstable
and address this in future PR once more books are added.)
@jm-clius I think this looks close enough to what's in js and go and I think this is a good start. I would not merge it to master right now tho and instead I will create an |
@jm-clius I've added an unstable branch, please repoint this PR there. |
This PR closes #498
It introduces a POC version of a Peer Store with Address Book, based on similar implementations in go and js.
The POC:
Suggested next steps:
WakuRelay
is a possible candidate here to implement a local Peer Store until it's fully integrated elsewhere innim-libp2p
nim-libp2p
(e.g. by building a peer manager, discovery interface, etc.)