Fix Ed25519 PeerID generation #186
Conversation
This commit pushes further fixes to the generation of Ed25519 peer IDs, building upon the discussion in ipfs/js-ipfs#3591 and the subsequent pull request #185. The purpose of this new pull request is to harmonize the encoding of PeerIDs for Ed25519 keys such that the same new format is used everywhere: peer IDs when assigned upon key generation, peer IDs when shown via key listing, as well as the peer IDs displayed as IPNS names when the key is used as the basis for an IPNS record. Concretely, this changes the peer ID representation of Ed25519 keys from the `Qm...` format to the newer `1...` format. The accompanying test has been modified accordingly.
| @@ -84,8 +83,8 @@ class Ed25519PrivateKey { | |||
| * @returns {Promise<String>} | |||
| */ | |||
| async id () { | |||
| const hash = await this.public.hash() | |||
There was a problem hiding this comment.
Should the hash function on the public key return the identity hash? The public and private keys still have sha2-256 hash functions associated with them.
There was a problem hiding this comment.
@aschmahmann For what it's worth, the peer ID generated here matches those generated for signed IPNS records, and so this appears to follow the correct spec.
There was a problem hiding this comment.
Sure, you've made the id function behave correctly. However, I'm not sure what the public.hash() function is used for and that is potentially another bug.
There was a problem hiding this comment.
public.hash() calls sha.multihashing(this.bytes, 'sha2-256'), effectively returning a SHA2-based multihash of the public key. I used this call specifically because it followed the spec, and because the same call was used elsewhere in functions of the same depth in the same prototype.
There was a problem hiding this comment.
The hash should likely just be returning the identity hash, but that would be breaking across the api so that should happen outside of this, perhaps when we revisit reducing the bundle size of this repo for browsers.
This change looks good and fixes the existing bug.
This commit pushes further fixes to the generation of Ed25519 peer IDs,
building upon the discussion in ipfs/js-ipfs#3591 and the subsequent
pull request #185.
The purpose of this new pull request is to harmonize the encoding of
PeerIDs for Ed25519 keys such that the same new format is used
everywhere: peer IDs when assigned upon key generation, peer IDs when
shown via key listing, as well as the peer IDs displayed as IPNS names
when the key is used as the basis for an IPNS record.
Concretely, this changes the peer ID representation of Ed25519 keys from
the
Qm...format to the newer1...format.The accompanying test has been modified accordingly.