-
Notifications
You must be signed in to change notification settings - Fork 106
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
Peer Discovery Interface #123
Conversation
Codecov Report
@@ Coverage Diff @@
## master #123 +/- ##
=======================================
Coverage 89.78% 89.78%
=======================================
Files 33 33
Lines 685 685
=======================================
Hits 615 615
Misses 70 70 Continue to review full report at Codecov.
|
I actually have some code layout around that implement these interface as well as some basic testing around kadmelia package. I'll clean this up and create a PR |
""" | ||
Find peers on the networking providing a particular service | ||
:param service: service that peers must provide | ||
:return: peerstore containing found peers on the network |
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.
If we look at the return value of the go implementation: https://github.com/libp2p/go-libp2p-discovery/blob/master/interface.go#L19
find_peers
could return a generator or PeerInfo
instead of a PeerStore
That would prevent to load too much in memory in case there are a lot of peers to be discovered
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.
Resolved in new commit
from abc import ABC, abstractmethod | ||
# pylint: disable=too-few-public-methods | ||
|
||
class IDiscoverer(ABC): |
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.
shouldn't it be : IAdvertiser
?
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.
Yes
@zaibon We would welcome a PR on this, but in the meantime our team will be focusing on pubsub until the discv5 spec is more solidified, then we will implement discv5. |
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.
LGTM, we will close this PR for now. @zaibon we look forward to your contribution!
I have added a peer discovery interface modeled after the interface used in the go repo