Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RFC 0001: Text Peer Ids as CIDs #209

Merged
merged 3 commits into from
Oct 1, 2019
Merged

RFC 0001: Text Peer Ids as CIDs #209

merged 3 commits into from
Oct 1, 2019

Conversation

lidel
Copy link
Member

@lidel lidel commented Aug 19, 2019

Important Note: this is the first RFC ever, following "option 2" from #198 and creating a template for future RFCs as a side-effect. The goal is to kick-off the RFC process. I am not a native speaker, feel free to tweak sentences that sound weird.

The RFC 0001 aims to modify Peer ID spec to alter the default string representation from Multihash to CIDv1 in Base32 and to support encoding/decoding text peerids as CIDs.

Changes

  • Added RFC/0000-RFC-TEMPLATE.md the TEMPLATE every future RFC should follow
    • Removed the template as suggested by @jacobheun – we will add it in separate PR after this PR is finalized
  • Added RFC/0001-text-peerid-cid.md the first RFC to test the process
  • Modified peer-ids/peer-ids.md to reflect changes from the RFC

Feedback needed

  • the process: does the template include everything we expect from this and future RFCs?
  • the RFC: is the proposed change clear enough? anything missing?

cc ipfs/kubo#5287, multiformats/multicodec#130, libp2p/go-libp2p-core#41, https://github.com/ipfs/ipfs/issues/337

Examples:

- SHA256 Peer Id encoded as canonical [CIDv1][cid-versions]:
`bafzbeie5745rpv2m6tjyuugywy4d5ewrqgqqhfnf445he3omzpjbx5xqxe` ([inspect](http://cid.ipfs.io/#bafzbeie5745rpv2m6tjyuugywy4d5ewrqgqqhfnf445he3omzpjbx5xqxe))
Copy link
Member Author

Choose a reason for hiding this comment

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

FYSA "inspect" link requires multiformats/cid-utils-website#14 to work correctly

Copy link
Contributor

@jacobheun jacobheun left a comment

Choose a reason for hiding this comment

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

A couple nits on the template but otherwise this RFC and the template looks good to me.

Before we merge the RFC we should pull the template out of this PR and merge that separately to avoid muddying the commit of the change.

RFC/0000-RFC-TEMPLATE.md Outdated Show resolved Hide resolved
RFC/0000-RFC-TEMPLATE.md Outdated Show resolved Hide resolved
RFC/0001-text-peerid-cid.md Outdated Show resolved Hide resolved
This is an RFC to modify peerid spec to alter the default string
representation from Multihash to CIDv1 in Base32 and to support
encoding/decoding text peerids as CIDs.

It is also the first RFC ever, following suggestions from
#198
and creating a template for future RFCs as a side-effect.

License: MIT
Signed-off-by: Marcin Rataj <lidel@lidel.org>
@lidel
Copy link
Member Author

lidel commented Aug 28, 2019

I rebased this PR to remove template and make RFC-0001 easier to audit.
Will add it in a separate PR after this one is finalized and merged.

Copy link
Contributor

@yusefnapora yusefnapora left a comment

Choose a reason for hiding this comment

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

This looks great to me 👍 Thanks for making the first RFC!

RFC/0001-text-peerid-cid.md Show resolved Hide resolved
peer-ids/peer-ids.md Outdated Show resolved Hide resolved
peer-ids/peer-ids.md Show resolved Hide resolved
License: MIT
Signed-off-by: Marcin Rataj <lidel@lidel.org>
@lidel lidel requested a review from Stebalien September 2, 2019 15:00
@lidel
Copy link
Member Author

lidel commented Sep 9, 2019

@raulk thoughts? :)

Copy link
Member

@raulk raulk left a comment

Choose a reason for hiding this comment

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

I think we need to mandate backwards compatibility with base58 encoded peer IDs string representations.

Implementations parsing IDs from text MUST support both base58 CIDv0 and CIDv1, and they MUST generate base32-encoded CIDv1, and optionally CIDv0 behind a flag.

We may want to provide a web tool to perform conversions, if we don't already.

In the future we can deprecate CIDv0, but if we don't do it this way, we may end up breaking the UX/DX of cross-language demos. May be worth the breakage, but I want us to be explicit about this.

License: MIT
Signed-off-by: Marcin Rataj <lidel@lidel.org>
@lidel
Copy link
Member Author

lidel commented Sep 30, 2019

@raulk CIDv0 is the same as current text representation (Peer ID being multihash in base58btc), making this a relatively safe change.

Existing CID libraries provide conversion methods and support old representation out of the box, but I've added explicit requirement to the spec in b621ac5.

Let me know if there is anything else blocking this.

@lidel lidel requested a review from raulk September 30, 2019 13:40
Copy link
Member

@raulk raulk left a comment

Choose a reason for hiding this comment

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

Thanks for bearing with me while I got to this, @lidel ;-) LGTM now.

I appreciate your adherence to the unofficial RFC process -- good to do some test runs. Once we land a specific format for RFC docs, we'll retrofit this one if needed.

@Stebalien
Copy link
Member

@raulk can we 🚢 this?

@raulk
Copy link
Member

raulk commented Oct 1, 2019

@Stebalien so we've locked in consensus for making the change. I'm not sure how to deal with change management across implementations, though. We haven't worked out the procedural details of how to coordinate the implementation of an RFC across libraries, and how to signal varying support in a matrix-like format :-(

In a nutshell, I want to avoid getting in a situation where go-libp2p spits out string representations of multiaddrs with CIDv1 peer IDs by default to stdout, and when users copy-paste those multiaddrs to an application powered by py-libp2p, it blows up.

I could be paranoid, though.

@Stebalien
Copy link
Member

@raulk the first step is https://github.com/libp2p/go-libp2p-core/pull/41/files. That will add support for parsing cid-peer-ids but it won't spit them out by default.

I've created a tracking issue: #216.

@Stebalien Stebalien merged commit 9a63e50 into master Oct 1, 2019
@Stebalien Stebalien deleted the rfc/0001-cid-peerid branch October 1, 2019 18:47
lidel added a commit to libp2p/js-peer-id that referenced this pull request Oct 25, 2019
This change adds two functions:

- createFromCID accepts CID as String|CID|Buffer and created PeerId from
  the multihash value inside of it
- toCIDString serializes PeerId multihash into a CIDv1 in Base32,
  as agreed in libp2p/specs#209

License: MIT
Signed-off-by: Marcin Rataj <lidel@lidel.org>
lidel added a commit to libp2p/js-peer-id that referenced this pull request Oct 25, 2019
This change 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,
  as agreed in libp2p/specs#209

License: MIT
Signed-off-by: Marcin Rataj <lidel@lidel.org>
lidel added a commit to libp2p/js-peer-id that referenced this pull request Oct 25, 2019
This change 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,
  as agreed in libp2p/specs#209

License: MIT
Signed-off-by: Marcin Rataj <lidel@lidel.org>
vasco-santos pushed a commit to libp2p/js-peer-id that referenced this pull request Nov 4, 2019
* feat: support Peer ID represented as CID

This change 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,
  as agreed in libp2p/specs#209

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

* refactor: rename toCIDString to toString

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>

* docs: comment tests

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

* feat: validate CID multicodec

- 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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants