-
Notifications
You must be signed in to change notification settings - Fork 44
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
feat: support Peer ID represented as CID #105
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,6 +5,7 @@ | |
'use strict' | ||
|
||
const mh = require('multihashes') | ||
const CID = require('cids') | ||
const cryptoKeys = require('libp2p-crypto/src/keys') | ||
const assert = require('assert') | ||
const withIs = require('class-is') | ||
|
@@ -122,6 +123,16 @@ class PeerId { | |
return this._idB58String | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This will now become rarely used. Should we generate on demand instead of in the constructor? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It is used in |
||
} | ||
|
||
// return self-describing String representation | ||
// in default format from RFC 0001: https://github.com/libp2p/specs/pull/209 | ||
toString () { | ||
if (!this._idCIDString) { | ||
const cid = new CID(1, 'libp2p-key', this.id, 'base32') | ||
this._idCIDString = cid.toBaseEncodedString('base32') | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If you cache the CID instance you could use it in a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As noted above, we don't want to expose CID types in outputs, just Strings. |
||
} | ||
return this._idCIDString | ||
} | ||
|
||
isEqual (id) { | ||
if (Buffer.isBuffer(id)) { | ||
return this.id.equals(id) | ||
|
@@ -184,7 +195,18 @@ exports.createFromBytes = (buf) => { | |
} | ||
|
||
exports.createFromB58String = (str) => { | ||
return new PeerIdWithIs(mh.fromB58String(str)) | ||
return exports.createFromCID(str) // B58String is CIDv0 | ||
} | ||
|
||
const validMulticodec = (cid) => { | ||
// supported: 'libp2p-key' (CIDv1) and 'dag-pb' (CIDv0 converted to CIDv1) | ||
return cid.codec === 'libp2p-key' || cid.codec === 'dag-pb' | ||
} | ||
|
||
exports.createFromCID = (cid) => { | ||
cid = CID.isCID(cid) ? cid : new CID(cid) | ||
if (!validMulticodec(cid)) throw new Error('Supplied PeerID CID has invalid multicodec: ' + cid.codec) | ||
return new PeerIdWithIs(cid.multihash) | ||
} | ||
lidel marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
// Public Key input will be a buffer | ||
|
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.
Personally I would add
toCID
and havetoString
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}
orpeerID + ''
without having to explicitly calltoString
.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.
My reasoning: there is no
toCID
because there was nottoMultihash
:)We want to keep API surface small, liberal in inputs (accept CID object), but strict in outputs (just basic types:
PeerID
,String
andBuffer
)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 agree in keeping this in the context of this PR. We can iterate on this in a new PR if it justifies