Skip to content
This repository has been archived by the owner on Mar 10, 2020. It is now read-only.

refactor: return peer ids as strings #581

Merged
merged 5 commits into from
Jan 29, 2020
Merged

Conversation

achingbrain
Copy link
Collaborator

@achingbrain achingbrain commented Jan 24, 2020

As part of the async iterator refactor some peer IDs were being returned as CIDs - the conversion was done in IPFS but not in libp2p, which meant that where IPFS exposes libp2p directly, peer IDs were being returned as strings, e.g. pubsub topic subscribers, message senders, etc.

The aim is to convert these to CIDs in the long run but such a change needs to be driven from inside libp2p instead of piecemeal at the IPFS layer.

This PR changes the ipfs.id().id and ipfs.swarm.peers().[].peer properties to strings so we can do the CID conversion in one go.

Also adds more in-depth tests for ipfs.id and documents the output from that method.

BREAKING CHANGE:

Where PeerIDs were previously CIDs, now they are Strings

  • ipfs.bitswap.stat().peers[n] is now a String (was a CID)
  • ipfs.dht.findPeer().id is now a String (was a CID)
  • ipfs.dht.findProvs()[n].id is now a String (was a CID)
  • ipfs.dht.provide()[n].id is now a String (was a CID)
  • ipfs.dht.put()[n].id is now a String (was a CID)
  • ipfs.dht.query()[n].id is now a String (was a CID)
  • ipfs.id().id is now a String (was a CID)
  • ipfs.id().addresses[n] are now Multiaddrs (were Strings)

As part of the async iterator refactor some peer IDs were being
returned as CIDs - the conversion was done in IPFS but not in
libp2p, which meant that where IPFS exposes libp2p directly, peer
IDs were being returned as strings, e.g. pubsub topic subscribers,
message senders, etc.

The aim is to convert these to CIDs in the long run but such a
change needs to be driven from inside libp2p instead of piecemeal
at the IPFS layer.

This PR changes the `ipfs.id().id` and `ipfs.swarm.peers().[].peer`
properties to strings so we can do the CID conversion in one go.

Also adds more in-depth tests for `ipfs.id`.
SPEC/SWARM.md Show resolved Hide resolved
src/swarm/peers.js Show resolved Hide resolved
@alanshaw
Copy link
Contributor

Is ready for re-review?

@achingbrain
Copy link
Collaborator Author

Yes, should be good to go.

SPEC/MISCELLANEOUS.md Outdated Show resolved Hide resolved
@@ -27,14 +27,15 @@ module.exports = (common, options) => {
after(() => common.clean())

it('should find other peers', async () => {
const res = await nodeA.dht.findPeer(nodeB.peerId.id)

const nodeBId = await nodeB.id()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not use nodeB.peerId here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not part of the API defined in interface-js-ipfs-core so we should really remove it from ipfsd-ctl return values.

This is my mind wandering from the task in hand though so I'm happy to change it if required.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, so it shouldn't be used in the interface tests because we can't guarantee ipfsd-ctl spawned the node(s) in the test. 👍 Sounds like a good plan, I'm happy to leave this like it is. Do you want to open an issue here to track that task?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Co-Authored-By: Alan Shaw <alan.shaw@protocol.ai>
Copy link
Contributor

@alanshaw alanshaw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes here LGTM.

Copy link
Contributor

@alanshaw alanshaw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still good

@achingbrain achingbrain merged commit 153fd24 into master Jan 29, 2020
@achingbrain achingbrain deleted the return-peer-ids-as-strings branch January 29, 2020 13:34
alanshaw pushed a commit to ipfs/js-ipfs that referenced this pull request Feb 3, 2020
Also updates all examples to use the new API.

Depends on:

- [x] ipfs-inactive/interface-js-ipfs-core#581
- [x] ipfs-inactive/js-ipfs-http-client#1226
- [x] libp2p/js-libp2p#545

BREAKING CHANGE:

Where `PeerID`s were previously [CID](https://www.npmjs.com/package/cids)s, now they are Strings

- `ipfs.bitswap.stat().peers[n]` is now a String (was a CID)
- `ipfs.dht.findPeer().id` is now a String (was a CID)
- `ipfs.dht.findProvs()[n].id` is now a String (was a CID)
- `ipfs.dht.provide()[n].id` is now a String (was a CID)
- `ipfs.dht.put()[n].id` is now a String (was a CID)
- `ipfs.dht.query()[n].id` is now a String (was a CID)
- `ipfs.id().id` is now a String (was a CID)
- `ipfs.id().addresses[n]` are now [Multiaddr](https://www.npmjs.com/package/multiaddr)s (were Strings)
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