-
Notifications
You must be signed in to change notification settings - Fork 276
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
Conversation
Examples: | ||
|
||
- SHA256 Peer Id encoded as canonical [CIDv1][cid-versions]: | ||
`bafzbeie5745rpv2m6tjyuugywy4d5ewrqgqqhfnf445he3omzpjbx5xqxe` ([inspect](http://cid.ipfs.io/#bafzbeie5745rpv2m6tjyuugywy4d5ewrqgqqhfnf445he3omzpjbx5xqxe)) |
There was a problem hiding this comment.
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
There was a problem hiding this 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.
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>
20c84dd
to
8ab79d2
Compare
I rebased this PR to remove template and make RFC-0001 easier to audit. |
There was a problem hiding this 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!
License: MIT Signed-off-by: Marcin Rataj <lidel@lidel.org>
@raulk thoughts? :) |
There was a problem hiding this 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>
@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. |
There was a problem hiding this 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.
@raulk can we 🚢 this? |
@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. |
@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. |
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>
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>
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>
* 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>
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
AddedRFC/0000-RFC-TEMPLATE.md
the TEMPLATE every future RFC should followRFC/0001-text-peerid-cid.md
the first RFC to test the processpeer-ids/peer-ids.md
to reflect changes from the RFCFeedback needed
cc ipfs/kubo#5287, multiformats/multicodec#130, libp2p/go-libp2p-core#41, https://github.com/ipfs/ipfs/issues/337