feat: add public key support by vasco-santos · Pull Request #4 · ipfs/js-ipns

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

 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

 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

 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

 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

 * 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 {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

    "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

Conversation

vasco-santos commented Aug 6, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alanshaw left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants