findProviders should return PeerIDs

[Gozala]

findProviders returns { id: PeerId, multiaddrs: Multiaddr[] }

js-libp2p/src/content-routing/index.js

Lines 43 to 52 in 8e1fc78

	/**
	* Iterates over all content routers in parallel to find providers of the given key.
	*
	* @param {CID} key - The CID key of the content to find
	* @param {object} [options]
	* @param {number} [options.timeout] - How long the query should run
	* @param {number} [options.maxNumProviders] - maximum number of providers to find
	* @returns {AsyncIterable<{ id: PeerId, multiaddrs: Multiaddr[] }>}
	*/
	async * findProviders (key, options = {}) {

While bitswap seems to assume it returns PeerIds instead. For more context please see

https://github.com/ipfs/js-ipfs-bitswap/pull/261/files#r587862380

[vasco-santos]

The link does not properly route me to the correct place I think.

This returns an async iterable if {{ id: PeerId, multiaddrs: Multiaddr[] }. In bitswap it seems to return @returns {AsyncIterable<Provider>} where Provider is:

 * @typedef {Object} Provider
 * @property {PeerId} id
 * @property {Multiaddr[]} multiaddrs

I did not understand the inconsistency here. Let me know what I am missing

[Gozala]

Here is the snapshot of the conversation in that pull

As per @achingbrain comment

libp2p.contentRouting.findProviders should return only PeerIDs.

The idea is that libp2p manages the actual multiaddrs internally and we don't worry about them at this level.

Maybe it's all correct, because types seem to align now ipfs/js-ipfs-bitswap#304

[vasco-santos]

I did not see that comment. So, you mean that findProviders should return {AsyncIterable<{peerId: PeerId>}

Ok, so @achingbrain is right in the sense that libp2p should handle internally the multiaddrs storage in the AddressBook, which is what we currently do.

The interface for contentRouting and peerRouting returns {AsyncIterable<{peerId: PeerId, multiaddrs: Multiaddr[]>}

We should discuss this further, I would like to keep the multiaddrs to be returned. Currently, libp2p will auto-dial when multiaddrs are added to the PeerStore (and is a new peer). But this will hopefully be removed later on in favour of the connection manager overhaul with proactive dial strategies. With this in mind, it is worth to return the multiaddrs to enable the user to be able to select a specific multiaddr if the user wants to do it. The drawback here is that we might know other multiaddrs from the peer that will not be returned (only the router provider records are returned).

Ideally, users will just dial with peerId and libp2p will handle everything given that we in theory will have addresses. But, we might also discover peers that are not advertising any multiaddr, so we will have no multiaddrs for it, and the user can try to dial it and libp2p will fail with "no addresses for the peer", which will be odd.

With all things considered, and as bitswap is not blocked on this, I would punt a decision here for after the connManager work + rendezvous/DHT

[vasco-santos]

I will move the issue to the interfaces repo, as this will mean changing the interface