feat: add public key support #4

vasco-santos · 2018-08-06T20:51:38Z

Adding public key methods for allowing full interop with go-ipfs

alanshaw · 2018-08-08T15:44:11Z

src/errors.js

@@ -5,3 +5,5 @@ exports.ERR_UNRECOGNIZED_VALIDITY = 'ERR_UNRECOGNIZED_VALIDITY'
 exports.ERR_SIGNATURE_CREATION = 'ERR_SIGNATURE_CREATION'
 exports.ERR_SIGNATURE_VERIFICATION = 'ERR_SIGNATURE_VERIFICATION'
 exports.ERR_UNRECOGNIZED_FORMAT = 'ERR_UNRECOGNIZED_FORMAT'
+exports.ERR_PEER_ID_FROM_PUBLIC_KEY = 'ERR_PEER_ID_FROM_PUBLIC_KEY'
+exports.ERR_PUBLICK_KEY_FROM_ID = 'ERR_PUBLICK_KEY_FROM_ID'


typo: "PUBLICK"

alanshaw · 2018-08-08T15:51:06Z

src/index.js

@@ -4,6 +4,9 @@ const base32Encode = require('base32-encode')
 const Big = require('big.js')
 const NanoDate = require('nano-date').default
 const { Key } = require('interface-datastore')
+const crypto = require('libp2p-crypto')
+const peerId = require('peer-id')


I would import this as PeerId - it is a class and saves you having to use peerIdResult to differentiate between this and a peer ID instance.

alanshaw · 2018-08-08T15:52:17Z

src/index.js

@@ -13,6 +16,7 @@ const ipnsEntryProto = require('./pb/ipns.proto')
 const { parseRFC3339 } = require('./utils')
 const ERRORS = require('./errors')

+const ID_MULTIHASH_CODE = 0


Can this be imported from multihashes? If not then it needs a comment to explain what it is.

Just created a PR multiformats/js-multihash#57 for that 🙂

alanshaw · 2018-08-08T15:53:32Z

src/index.js

+    if (err) {
+      const error = 'cannot create peer id from the public key'
+
+      log.error(error)


Much more useful to log err than the message

alanshaw · 2018-08-08T15:54:15Z

src/index.js

+      const error = 'cannot create peer id from the public key'
+
+      log.error(error)
+      return callback(Object.assign(new Error(error), { code: ERRORS.ERR_PEER_ID_FROM_PUBLIC_KEY }))


Either assign the code to err or use a module like explain-error to retain the stack.

alanshaw · 2018-08-08T15:57:54Z

src/index.js

+
+    // If we vailed to extract the public key from the peer ID, embed it in the record.
+    entry.pubKey = crypto.keys.marshalPublicKey(publicKey)
+    return callback(null, entry)


Minor, no need to return a call to callback when it's the last statement of the function.

alanshaw · 2018-08-08T15:59:18Z

src/index.js

+    return callback(null, pubKey)
+  } else {
+    return callback(null, peerId.pubKey)
+  }


Minor, use if with a single return or if & else with no returns.

alanshaw · 2018-08-08T15:59:42Z

src/index.js

@@ -120,7 +157,13 @@ const embedPublicKey = (publicKey, entry, callback) => {
 * @return {Void}
 */
 const extractPublicKey = (peerId, entry, callback) => {
-  callback(new Error('not implemented yet'))
+  if (entry.pubKey) {
+    const pubKey = crypto.keys.unmarshalPublicKey(entry.pubKey)


Needs a try/catch around this?

alanshaw · 2018-08-08T16:00:55Z

src/index.js

+  if (validityType.toString() === '0') {
+    return 'EOL'
+  } else {
+    log.error('unrecognized validity type')


Put the validityType in the log message for better debugging

alanshaw · 2018-08-08T16:02:17Z

src/index.js

@@ -139,16 +182,16 @@ const getLocalKey = (key) => new Key(`/ipns/${rawStdEncoding(key)}`)
 * Get key for sharing the record in the routing mechanism.
 * Format: ${base32(/ipns/<HASH>)}, ${base32(/pk/<HASH>)}
 *
- * @param {Buffer} key peer identifier object.
+ * @param {Buffer} pid peer identifier object.


Not specifically an object, a buffer. Does this need an explanation of how you might extract this from a PeerId instance?

… into feat/add-public-key-support

alanshaw

Nice! Just a few small issues to clean up and this will be gtg.

alanshaw · 2018-08-09T10:27:49Z

src/index.js

+    try {
+      const pubKey = crypto.keys.unmarshalPublicKey(entry.pubKey)
+
+      return callback(null, pubKey)


Avoid callback in a try block - if the callback throws, it'll be caught and the callback will be called again - very hard to debug!

alanshaw · 2018-08-09T10:30:35Z

src/index.js

 *
- * @param {Object} publicKey public key for validating the record.
+ * @param {Object} publicKey public key to embed.
 * @param {Object} entry ipns entry record.
 * @param {function(Error)} [callback]
 * @return {Void}
 */
 const embedPublicKey = (publicKey, entry, callback) => {


Check publicKey, publicKey.bytes and entry exist?

alanshaw · 2018-08-09T10:32:33Z

src/index.js

+    }
+
+    // If we failed to extract the public key from the peer ID, embed it in the record.
+    entry.pubKey = crypto.keys.marshalPublicKey(publicKey)


try/catch around this?

alanshaw · 2018-08-09T10:34:11Z

src/index.js

@@ -120,7 +153,17 @@ const embedPublicKey = (publicKey, entry, callback) => {
 * @return {Void}
 */
 const extractPublicKey = (peerId, entry, callback) => {


Check peerId and entry exist

alanshaw · 2018-08-09T10:39:31Z

package.json

@@ -49,8 +51,7 @@
    "dirty-chai": "^2.0.1",
    "ipfs": "^0.29.3",
    "ipfsd-ctl": "^0.36.0",
-    "libp2p-crypto": "^0.13.0",
-    "multihashes": "^0.4.13"
+    "libp2p-crypto": "^0.13.0"


This needs to move to dependencies

@Stebalien

A working version of **IPNS working locally** is here! 🚀 😎 💪 Steps: - [x] Create a new repo (**js-ipns**) like it was made with go in the last week ([go-ipns](https://github.com/ipfs/go-ipns)) and port the related code from this PR to there - [x] Resolve IPNS names in publish, in order to verify if they exist (as it is being done for regular files) before being published - [x] Handle remaining parameters in publish and resolve (except ttl). - [x] Test interface core spec [interface-ipfs-core#327](ipfs-inactive/interface-js-ipfs-core#327) - [x] Test interoperability with go. [interop#26](ipfs/interop#26) - [x] Integrate logging - [x] Write unit tests - [x] Add support for the lifetime with nanoseconds precision - [x] Add Cache - [x] Add initializeKeyspace - [x] Republish Some notes, regarding the previous steps: - There is an optional parameter not implemented in this PR, which is `ttl`, since it is still experimental, we can add it in a separate PR. Finally, thanks @Stebalien for your help and time answering all my questions regarding the IPNS implementation in GO. Moreover, since there are no specs, and not that much documentation, I have been writing a document with all the IPNS workflow. It is a WIP by now, but it is currently available [here](ipfs/specs#184). Related PRs: - [x] [js-ipns#4](ipfs/js-ipns#4) - [x] [js-ipfs-repo#173](ipfs/js-ipfs-repo#173) - [x] [js-ipfs#1496](#1496) - [x] [interface-ipfs-core#327](ipfs-inactive/interface-js-ipfs-core#327) - [x] enable `interface-ipfs-core` tests for IPNS in `js-ipfs`

feat: add public key support

d88884c

vasco-santos requested review from alanshaw and daviddias August 6, 2018 20:51

ghost assigned vasco-santos Aug 6, 2018

ghost added the status/in-progress In progress label Aug 6, 2018

This was referenced Aug 8, 2018

test: add ipns tests ipfs/interop#26

Merged

feat: ipns locally ipfs/js-ipfs#1400

Merged

alanshaw requested changes Aug 8, 2018

View reviewed changes

vasco-santos added 3 commits August 8, 2018 18:50

fix: code review

f76b391

fix: code review

7812af8

Merge branch 'feat/add-public-key-support' of github.com:ipfs/js-ipns…

bc86534

… into feat/add-public-key-support

alanshaw requested changes Aug 9, 2018

View reviewed changes

fix: code review

1c023a6

alanshaw approved these changes Aug 9, 2018

View reviewed changes

vasco-santos merged commit e743632 into master Aug 9, 2018

ghost removed the status/in-progress In progress label Aug 9, 2018

vasco-santos deleted the feat/add-public-key-support branch August 9, 2018 11:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add public key support #4

feat: add public key support #4

vasco-santos commented Aug 6, 2018

alanshaw Aug 8, 2018

alanshaw Aug 8, 2018

alanshaw Aug 8, 2018

vasco-santos Aug 8, 2018

alanshaw Aug 8, 2018

alanshaw Aug 8, 2018

alanshaw Aug 8, 2018

alanshaw Aug 8, 2018

alanshaw Aug 8, 2018

alanshaw Aug 8, 2018

alanshaw Aug 8, 2018

alanshaw left a comment

alanshaw Aug 9, 2018

vasco-santos Aug 9, 2018

alanshaw Aug 9, 2018

alanshaw Aug 9, 2018

alanshaw Aug 9, 2018

alanshaw Aug 9, 2018

feat: add public key support #4

feat: add public key support #4

Conversation

vasco-santos commented Aug 6, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alanshaw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment