-
Notifications
You must be signed in to change notification settings - Fork 6
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
chore: peer-discovery not using peer-info #5
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,7 +4,6 @@ const Emittery = require('emittery') | |
const debug = require('debug') | ||
|
||
const PeerId = require('peer-id') | ||
const PeerInfo = require('peer-info') | ||
const multiaddr = require('multiaddr') | ||
|
||
const PB = require('./peer.proto') | ||
|
@@ -31,17 +30,20 @@ class PubsubPeerDiscovery extends Emittery { | |
* @constructor | ||
* @param {Libp2p} param0.libp2p Our libp2p node | ||
* @param {number} [param0.interval = 10000] How often (ms) we should broadcast our infos | ||
* @param {Array<Multiaddr>} param0.multiaddrs Multiaddrs used by the node to listen. | ||
* @param {Array<string>} [param0.topics = PubsubPeerDiscovery.TOPIC] What topics to subscribe to. If set, the default will NOT be used. | ||
* @param {boolean} [param0.listenOnly = false] If true, we will not broadcast our peer data | ||
*/ | ||
constructor ({ | ||
libp2p, | ||
interval = 10000, | ||
topics, | ||
multiaddrs = [], | ||
listenOnly = false | ||
}) { | ||
super() | ||
this.libp2p = libp2p | ||
this.multiaddrs = multiaddrs | ||
this.interval = interval | ||
this._intervalId = null | ||
this._listenOnly = listenOnly | ||
|
@@ -54,6 +56,7 @@ class PubsubPeerDiscovery extends Emittery { | |
} | ||
|
||
this.removeListener = this.off.bind(this) | ||
this.removeAllListeners = this.clearListeners.bind(this) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What's using this? If this is being used we likely need to get more explicit about what event api peer-discovery modules should be exposing. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It is being used fot the tests: https://github.com/libp2p/js-libp2p-interfaces/blob/v0.3.0/src/peer-discovery/tests/index.js#L25 I agree that we should be explicit about it. |
||
} | ||
|
||
/** | ||
|
@@ -97,8 +100,8 @@ class PubsubPeerDiscovery extends Emittery { | |
*/ | ||
_broadcast () { | ||
const peer = { | ||
publicKey: this.libp2p.peerInfo.id.pubKey.bytes, | ||
addrs: this.libp2p.peerInfo.multiaddrs.toArray().map(ma => ma.buffer) | ||
publicKey: this.libp2p.peerId.pubKey.bytes, | ||
addrs: this.multiaddrs.map(ma => ma.buffer) | ||
} | ||
|
||
const encodedPeer = PB.Peer.encode(peer) | ||
|
@@ -120,16 +123,23 @@ class PubsubPeerDiscovery extends Emittery { | |
const peerId = await PeerId.createFromPubKey(peer.publicKey) | ||
|
||
// Ignore if we received our own response | ||
if (peerId.equals(this.libp2p.peerInfo.id)) return | ||
if (peerId.equals(this.libp2p.peerId)) return | ||
|
||
const peerInfo = new PeerInfo(peerId) | ||
peer.addrs.forEach(buffer => peerInfo.multiaddrs.add(multiaddr(buffer))) | ||
log('discovered peer %j on %j', peerId, message.topicIDs) | ||
this.emit('peer', peerInfo) | ||
this._onNewPeer(peerId, peer.addrs) | ||
} catch (err) { | ||
log.error(err) | ||
} | ||
} | ||
|
||
_onNewPeer (peerId, addrs) { | ||
jacobheun marked this conversation as resolved.
Show resolved
Hide resolved
|
||
if (!this._intervalId) return | ||
|
||
this.emit('peer', { | ||
id: peerId, | ||
multiaddrs: addrs.map(b => multiaddr(b)) | ||
}) | ||
} | ||
} | ||
|
||
PubsubPeerDiscovery.TOPIC = TOPIC | ||
|
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.
How are these intended to get passed in? I would recommend not doing this and instead we should just leverage the announce addrs. I don't believe that's done yet, so in the interim we may want to just do
this.libp2p.transportManager.getAddrs()
. This removes any need to add special options for the addresses, which is not friendly for new users.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.
Ok, we can use the
transportManager.getAddrs()
and I will open an issue to use theaddressManager
in the futureThere 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.
Removed the param, using the transport manager for now and issue created #6