feat: support Peer ID represented as CID #105

lidel · 2019-10-25T17:15:20Z

Motivation

This PR implements Stage 1: Parse CIDs as peer ID from libp2p/specs#216.

TL;DR This PR does not change the default text representation of Peer IDs, all we do in Stage 1 is adding support for creating PeerId objects from CIDs.

For a wider context see libp2p/specs/RFC/0001-text-peerid-cid.md, short story is that CID support will enable IPFS project to support /ipns/{cid} in Base32 (https://github.com/ipfs/ipfs/issues/337), enabling things like http://{libp2p-key-as-cidv1b32}.ipns.dweb.link

Changes

This PR adds two functions:

createFromCID accepts CID as String|CID|Buffer and creates PeerId from the multihash value inside of it
toCIDString serializes PeerId multihash into a CIDv1 in Base32 (libp2p RFC 0001)

CIDv1 is self describing, and toString was not defined. Makes sense to use generic toString in this case. This change also: - remembers string with CID, so it is lazily generated only once - switches createFromB58String to createFromCID (b58 is CIDv0), making it easier to migrate existing codebases. License: MIT Signed-off-by: Marcin Rataj <lidel@lidel.org>

jacobheun

Just a minor request in the tests, but otherwise this looks good!

test/peer-id.spec.js

alanshaw

The implementation of toJSON should change to encode the peer ID as a CID - right?

alanshaw · 2019-10-28T13:34:38Z

README.md

@@ -197,9 +207,18 @@ Returns the Peer ID's `id` as a buffer.
 <Buffer 12 20 d6 24 39 98 f2 fc 56 34 3a d7 ed 03 42 ab 78 86 a4 eb 18 d7 36 f1 b6 7d 44 b3 7f cc 81 e0 f3 9f>
 ```

+
+### `toString()`


Personally I would add toCID and have toString as () => this.toCID().toString().

This creates a symmetric API createFromCID/toCID (although "create" is superfluous IMHO, but meh, maybe a refactor for another day) and a nice sensible representation as a string that also enables ${peerId} or peerID + '' without having to explicitly call toString.

My reasoning: there is no toCID because there was not toMultihash :)
We want to keep API surface small, liberal in inputs (accept CID object), but strict in outputs (just basic types: PeerID, String and Buffer)

I agree in keeping this in the context of this PR. We can iterate on this in a new PR if it justifies

alanshaw · 2019-10-28T13:38:46Z

src/index.js

@@ -122,6 +123,16 @@ class PeerId {
    return this._idB58String


This will now become rarely used. Should we generate on demand instead of in the constructor?

It is used in isPeerId and changing this triggers a bigger refactor.
I'd like to keep it as-is in this PR (to keep it small) and change it in Stage 2 of libp2p/specs#216
(when we "Format peer IDs as CIDs by default.")

alanshaw · 2019-10-28T13:40:08Z

src/index.js

+  toString () {
+    if (!this._idCIDString) {
+      const cid = new CID(1, 'libp2p-key', this.id, 'base32')
+      this._idCIDString = cid.toBaseEncodedString('base32')


If you cache the CID instance you could use it in a toCID also. Note that CID's cache their string representation so calling toString on it won't re-encode.

As noted above, we don't want to expose CID types in outputs, just Strings.

src/index.js

alanshaw · 2019-10-28T13:50:52Z

src/index.js

+  } else {
+    // provide more meaningful error than the one in CID.validateCID
+    throw new Error('Supplied cid value is neither String|CID|Buffer')
+  }


TODO: validate the codec is libp2p-key or that it's a v0?

This is tricky.
We want people to be able to convert CIDv0→CIDv1 and be able to use it.

See scenario below:

Execute ipfs id

You will see PeerID as Base58 (CIDv0) which has implicit multicodec dag-pb

How to convert PeerID to CIDv1?

Probably via ipfs cid base32 or https://cid.ipfs.io util

Caveat: conversion produces a valid CIDv1 with (now explicit) dag-pb multicodec

the multihash inside of CIDv1 still represents a valid PeerID, and this library does not care about the rest od CID anyway

I think the only way is to handle it gracefully is to check for both libp2p-key and dag-pb. Updated this PR to do just that + tests.

License: MIT Signed-off-by: Marcin Rataj <lidel@lidel.org>

- require CID with 'libp2p-key' (CIDv1) or 'dag-pb' (CIDv0 converted to CIDv1) - delegate CID validation to CID constructor License: MIT Signed-off-by: Marcin Rataj <lidel@lidel.org>

lidel

@alanshaw no, the implementation of toJSON should NOT change in Stage 1 (see libp2p/specs#216). In this PR we only add support for parsing CID here, but don't change current default representation (remains CIDv0).

I simplified createFromCID and applied some of your suggestions in 2b11192

lidel · 2019-11-04T10:16:48Z

ping @vasco-santos

vasco-santos

LGTM! 🚀

vasco-santos · 2019-11-04T10:26:49Z

README.md

@@ -197,9 +207,18 @@ Returns the Peer ID's `id` as a buffer.
 <Buffer 12 20 d6 24 39 98 f2 fc 56 34 3a d7 ed 03 42 ab 78 86 a4 eb 18 d7 36 f1 b6 7d 44 b3 7f cc 81 e0 f3 9f>
 ```

+
+### `toString()`


I agree in keeping this in the context of this PR. We can iterate on this in a new PR if it justifies

vasco-santos · 2019-11-04T10:31:38Z

@alanshaw any other feedback that you want to provide here? I think we can go with this

lidel requested review from alanshaw, jacobheun and vasco-santos October 25, 2019 17:15

Stebalien mentioned this pull request Oct 25, 2019

Tracking issue for peer IDs as CIDs libp2p/specs#216

Open

22 tasks

lidel commented Oct 25, 2019

View reviewed changes

src/index.js Outdated Show resolved Hide resolved

lidel commented Oct 25, 2019

View reviewed changes

src/index.js Outdated Show resolved Hide resolved

lidel mentioned this pull request Oct 25, 2019

feat: js-ipfs support of CIDs in /ipns/ content paths ipfs/js-ipfs#2566

Merged

jacobheun approved these changes Oct 28, 2019

View reviewed changes

test/peer-id.spec.js Show resolved Hide resolved

alanshaw suggested changes Oct 28, 2019

View reviewed changes

lidel added 2 commits October 28, 2019 18:43

docs: comment tests

d4fee10

License: MIT Signed-off-by: Marcin Rataj <lidel@lidel.org>

feat: validate CID multicodec

2b11192

- require CID with 'libp2p-key' (CIDv1) or 'dag-pb' (CIDv0 converted to CIDv1) - delegate CID validation to CID constructor License: MIT Signed-off-by: Marcin Rataj <lidel@lidel.org>

lidel commented Oct 28, 2019

View reviewed changes

lidel requested a review from alanshaw October 28, 2019 19:20

vasco-santos approved these changes Nov 4, 2019

View reviewed changes

alanshaw approved these changes Nov 4, 2019

View reviewed changes

vasco-santos merged commit 544ca7d into master Nov 4, 2019

daviddias deleted the feat/cid-support branch November 4, 2019 18:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: support Peer ID represented as CID #105

feat: support Peer ID represented as CID #105

lidel commented Oct 25, 2019 •

edited

Loading

jacobheun left a comment

alanshaw left a comment

alanshaw Oct 28, 2019

lidel Oct 28, 2019 •

edited

Loading

vasco-santos Nov 4, 2019

alanshaw Oct 28, 2019

lidel Oct 28, 2019 •

edited

Loading

alanshaw Oct 28, 2019

lidel Oct 28, 2019

alanshaw Oct 28, 2019

lidel Oct 28, 2019 •

edited

Loading

lidel left a comment •

edited

Loading

lidel commented Nov 4, 2019

vasco-santos left a comment

vasco-santos Nov 4, 2019

vasco-santos commented Nov 4, 2019

feat: support Peer ID represented as CID #105

feat: support Peer ID represented as CID #105

Conversation

lidel commented Oct 25, 2019 • edited Loading

Motivation

Changes

Next

jacobheun left a comment

Choose a reason for hiding this comment

alanshaw left a comment

Choose a reason for hiding this comment

alanshaw Oct 28, 2019

Choose a reason for hiding this comment

lidel Oct 28, 2019 • edited Loading

Choose a reason for hiding this comment

vasco-santos Nov 4, 2019

Choose a reason for hiding this comment

alanshaw Oct 28, 2019

Choose a reason for hiding this comment

lidel Oct 28, 2019 • edited Loading

Choose a reason for hiding this comment

alanshaw Oct 28, 2019

Choose a reason for hiding this comment

lidel Oct 28, 2019

Choose a reason for hiding this comment

alanshaw Oct 28, 2019

Choose a reason for hiding this comment

lidel Oct 28, 2019 • edited Loading

Choose a reason for hiding this comment

lidel left a comment • edited Loading

Choose a reason for hiding this comment

lidel commented Nov 4, 2019

vasco-santos left a comment

Choose a reason for hiding this comment

vasco-santos Nov 4, 2019

Choose a reason for hiding this comment

vasco-santos commented Nov 4, 2019

lidel commented Oct 25, 2019 •

edited

Loading

lidel Oct 28, 2019 •

edited

Loading

lidel Oct 28, 2019 •

edited

Loading

lidel Oct 28, 2019 •

edited

Loading

lidel left a comment •

edited

Loading