Skip to content
This repository has been archived by the owner on Feb 12, 2024. It is now read-only.

[Types] SwarmAPI.connect requires a Multiaddr but libp2p.dial allows PeerId|Multiaddr|string #3638

Closed
rvagg opened this issue Apr 26, 2021 · 3 comments · Fixed by #3673
Closed
Assignees
Labels
status/ready Ready to be worked

Comments

@rvagg
Copy link
Member

rvagg commented Apr 26, 2021

Ref: ceramicnetwork/js-ceramic#1194

Similar to #3637

connect: (addr: Multiaddr, options?: AbortOptions & OptionExtension) => Promise<void>

But this passes straight through to libp2p.dial() which is more permissive:

https://github.com/libp2p/js-libp2p/blob/5372f7af2f8f1653384504af15efea8ba776534b/src/index.js#L454-L465

Should we be similarly lax here?

@vasco-santos
Copy link
Member

humm, good question. In libp2p we have the addressBook as part of the API and users can add addresses to the AddressBook and then dial using PeerId. Discovery mechanisms also add to the addressBook out of the box. In the future, we want to try to find addresses of a peer if we don't have any libp2p/js-libp2p#901 . However, now if we do not have any addresses the dial will just fail (error no addresses for given peer).

In IPFS, I think we can allow dials to PeerId, but it should probably mentioned in the docs that dial a peerId is only supported if libp2p's addressBook already has addresses for it?

@achingbrain
Copy link
Member

Peer IDs are a bit of a problem in the public API as the peer-id module pulls a lot of crypto libraries down with it - this means ipfs-core-types becomes very heavy, as does ipfs-http-client.

We've been using CIDs and strings for Peer IDs in other places as a more lightweight solution.

CIDs have always felt a bit weird (to me) as a Peer ID type, and strings are too opaque (could be a multiaddr, could be a peer id) - we could introduce a marker interface style class that just lets us drop strings as an argument and only accept multiaddrs or Peer IDs while not needing to use the full peer-id module?

@vasco-santos
Copy link
Member

we could introduce a marker interface style class that just lets us drop strings as an argument and only accept multiaddrs or Peer IDs while not needing to use the full peer-id module?

sounds good. We want to move in the future with a change to use strings everywhere in libp2p libp2p/js-libp2p#680 , but the could be a multiaddr, could be a peer id is tricky in the dial side of things. When we do this change in libp2p, we can consider moving this interface style class to the libp2p side of things

@BigLep BigLep added status/ready Ready to be worked and removed need/triage Needs initial labeling and prioritization labels May 3, 2021
@BigLep BigLep added this to Weekly Candidates in Maintenance Priorities - Go May 3, 2021
achingbrain added a commit that referenced this issue May 6, 2021
Accept peer ids as strings or multiaddrs as Multiaddrs.

Fixes #3638
Maintenance Priorities - Go automation moved this from Weekly Candidates to Done May 6, 2021
achingbrain added a commit that referenced this issue May 6, 2021
Accept peer ids as strings or multiaddrs as Multiaddrs.

Fixes #3638
@BigLep BigLep removed this from Done in Maintenance Priorities - Go May 8, 2021
@BigLep BigLep removed this from Done in Maintenance Priorities - Go May 8, 2021
@BigLep BigLep removed this from Done in Maintenance Priorities - Go May 12, 2021
@BigLep BigLep removed this from Done in Maintenance Priorities - Go May 12, 2021
@BigLep BigLep removed this from Done in Maintenance Priorities - JS May 12, 2021
@BigLep BigLep removed this from Done in Maintenance Priorities - Go May 12, 2021
SgtPooki referenced this issue in ipfs/js-kubo-rpc-client Aug 18, 2022
Accept peer ids as strings or multiaddrs as Multiaddrs.

Fixes #3638
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
status/ready Ready to be worked
Projects
No open projects
Development

Successfully merging a pull request may close this issue.

4 participants