chore: remove peer info usage #610
Conversation
vasco-santos
force-pushed
the
chore/remove-peer-info-usage
branch
11 times, most recently
from
95a79e8 to
b283126
Compare
|
|
||
| ### addresses | ||
|
|
||
| TODO with `address-manager`. |
There was a problem hiding this comment.
This will come in a follow up PR
vasco-santos
force-pushed
the
chore/remove-peer-info-usage
branch
7 times, most recently
from
1a344be to
1e42f42
Compare
BREAKING CHANGE: all API methods with peer-info parameters or return values were changed. You can check the API.md document, in order to check the new values to use
vasco-santos
force-pushed
the
chore/remove-peer-info-usage
branch
from
1e42f42 to
ab20987
Compare
jacobheun
left a comment
jacobheun
left a comment
There was a problem hiding this comment.
Overall this looks good! Most of my comments are things to think about and would likely be best suited for subsequent PRs if we choose to do them.
| | [`PeerInfo`][peer-info] | Peer information of the provided peer | | ||
|
|
||
| TODO: change when `peer-info` is deprecated to new pointer | ||
| | `{ id: PeerId, multiaddrInfos: Array<MultiaddrInfo>, protocols: Array<string> }` | Peer information of the provided peer | |
There was a problem hiding this comment.
Just a naming thought that occurred, should we just start referring to MultiaddrInfo as Address? The idea being, we have an AddressBook, it makes sense for it to store addresses (addrs). An Address includes a Multiaddr (describing the network location of a peer) along with other metadata (TTL, confidence, origin, etc).
There was a problem hiding this comment.
yeah, I was not totally happy with the MultiaddrInfo naming. we are already using that naming in the dht already, but I will do a follow up PR here and in the DHT to move into Address
src/get-peer-id.js
Outdated
| } | ||
|
|
||
| if (addr && peerStore) { | ||
| peerStore.addressBook.add(peer, [addr]) |
There was a problem hiding this comment.
Since we're redoing this function it might be a good time to remove the update from it as it's a bit error prone (doing a get that includes a write). Perhaps this should be getPeer and we return { id, multiaddrs }? Then the calling function can choose whether or not it wants to store the address.
| discoveryService = new DiscoveryService(Object.assign({}, config, { peerInfo: this.peerInfo, libp2p: this })) | ||
| discoveryService = new DiscoveryService(Object.assign({}, config, { | ||
| peerId: this.peerId, | ||
| multiaddrs: this.addresses.listen, |
There was a problem hiding this comment.
When we have an AddressManager the discovery service should just use that from libp2p.addressManager. While they don't right now, in the future our announce addresses will be able to change over time (AutoNat, AutoRelay, etc) and we should be sharing the most recent ones via the discovery services.
Co-Authored-By: Jacob Heun <[email protected]>
vasco-santos
force-pushed
the
chore/remove-peer-info-usage
branch
from
9329189 to
894cb69
Compare
In the context of deprecating
peer-infoas described on libp2p/js-libp2p#589, this PR removes thepeer-infousage on the API, internal code usage and tests. Moreover, all the modules were updated for the new versions also without usingpeer-infoand the docs were also updated!It is important point out that with these modifications:
{ addresses: { listen } }in the context of Add support for Addrs {listen, announce, noAnnounce} #202 and feat: address management #612 adds a AddressManager component with support for announce and noAnnounce, as well as docs for usage.BREAKING CHANGE: all API methods with peer-info parameters or return values were changed. You can check the API.md document, in order to check the new values to use