Skip to content
This repository has been archived by the owner on Jun 26, 2023. It is now read-only.

chore: peer discovery should emit peer id #19

Closed

Conversation

vasco-santos
Copy link
Member

@vasco-santos vasco-santos commented Jan 13, 2020

BREAKING CHANGE: peer discovery event param is a PeerId instance instead of a PeerInfo instance

Context: libp2p/js-peer-info#85

I added _onPeer as a mandatory function to be implemented, so that we can test the event emitted uniformly. Any better ideas to make it?

BREAKING CHANGE: peer discovery event param is a PeerId instance instead of a PeerInfo instance
@vasco-santos vasco-santos force-pushed the chore/peer-discovery-should-emit-peer-id branch from 0b505b3 to b199e5a Compare January 13, 2020 09:27
@jacobheun
Copy link
Contributor

@vasco-santos I think we need to write up a clearer plan for how multiaddrs and protocols will make it into the PeerStore. The problem with this PR is that there isn't a clear way for multiaddrs for the peer to be tracked. Many of the discovery modules include the known addresses of the peer, if we change to this, we will lose that and won't be able to connect to the peers.

One approach might be to pass the PeerStore to the discovery modules. Instead of emitting peers they could put to the PeerStore, and add any other known metadata. We should probably flush out the v2 PeerStore before making changes here.

@vasco-santos
Copy link
Member Author

@jacobheun I agree, my idea would be to provide the peerStore so that the peers are properly tracked. Anyway, the discovery services should emit the peer event with the peer-id of a peer once it is discovered and added to the peerStore.

Anyway, we can start by having a clear definition for the peerStorev2

@vasco-santos vasco-santos deleted the chore/peer-discovery-should-emit-peer-id branch April 16, 2020 13:34
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants