refactor: fix review

package.json

@@ -74,12 +74,12 @@
     "ncp": "^2.0.0",
     "qs": "^6.5.2",
     "rimraf": "^2.6.2",
+    "sinon": "^7.1.1",
     "stream-to-promise": "^2.2.0"
   },
   "dependencies": {
     "@nodeutils/defaults-deep": "^1.1.0",
     "async": "^2.6.1",
-    "base32.js": "~0.1.0",
     "big.js": "^5.2.2",
     "binary-querystring": "~0.1.2",
     "bl": "^2.1.2",

src/core/components/init.js

@@ -113,7 +113,7 @@ module.exports = function init (self) {
       (_, cb) => {
         const offlineDatastore = new OfflineDatastore(self._repo)
 
-        self._ipns = new IPNS(offlineDatastore, self)
+        self._ipns = new IPNS(offlineDatastore, self._repo, self._peerInfo, self._keychain, self._options)
         cb(null, true)
       },
       // add empty unixfs dir object (go-ipfs assumes this exists)

src/core/components/pre-start.js

@@ -7,10 +7,6 @@ const waterfall = require('async/waterfall')
 const Keychain = require('libp2p-keychain')
 const defaultsDeep = require('@nodeutils/defaults-deep')
 const NoKeychain = require('./no-keychain')
-
-const IPNS = require('../ipns')
-const OfflineDatastore = require('../ipns/routing/offline-datastore')
-
 /*
  * Load stuff from Repo into memory
  */
@@ -99,13 +95,6 @@ module.exports = function preStart (self) {
 
         cb()
       },
-      // Setup offline routing for IPNS.
-      (cb) => {
-        const offlineDatastore = new OfflineDatastore(self._repo)
-
-        self._ipns = new IPNS(offlineDatastore, self)
-        cb()
-      },
       (cb) => self.pin._load(cb)
     ], callback)
   }

src/core/components/start.js

@@ -43,14 +43,14 @@ module.exports = (self) => {
 
         // TODO Add IPNS pubsub if enabled
 
-        // NOTE: Until the IPNS over DHT is not ready, it is being replaced by the local repo datastore
-        // When DHT is added, If local option enabled, should receive offlineDatastore as well
+        // NOTE: IPNS routing is being replaced by the local repo datastore while the IPNS over DHT is not ready
+        // When DHT is added, if local option enabled, should receive offlineDatastore as well
         const offlineDatastore = new OfflineDatastore(self._repo)
         ipnsStores.push(offlineDatastore)
 
         // Create ipns routing with a set of datastores
         const routing = new TieredDatastore(ipnsStores)
-        self._ipns = new IPNS(routing, self)
+        self._ipns = new IPNS(routing, self._repo, self._peerInfo, self._keychain, self._options)
 
         self._bitswap = new Bitswap(
           self._libp2pNode,

src/core/ipns/index.js

@@ -4,6 +4,7 @@ const { createFromPrivKey } = require('peer-id')
 const series = require('async/series')
 const Receptacle = require('receptacle')
 
+const errcode = require('err-code')
 const debug = require('debug')
 const log = debug('jsipfs:ipns')
 log.error = debug('jsipfs:ipns:error')
@@ -16,9 +17,9 @@ const path = require('./path')
 const defaultRecordTtl = 60 * 1000
 
 class IPNS {
-  constructor (routing, ipfs) {
-    this.publisher = new IpnsPublisher(routing, ipfs._repo)
-    this.republisher = new IpnsRepublisher(this.publisher, ipfs)
+  constructor (routing, repo, peerInfo, keychain, options) {
+    this.publisher = new IpnsPublisher(routing, repo)
+    this.republisher = new IpnsRepublisher(this.publisher, repo, peerInfo, keychain, options)
     this.resolver = new IpnsResolver(routing)
     this.cache = new Receptacle({ max: 1000 }) // Create an LRU cache with max 1000 items
     this.routing = routing
@@ -55,11 +56,20 @@ class IPNS {
 
   // Resolve
   resolve (name, options, callback) {
+    if (typeof name !== 'string') {
+      const errMsg = `name received is not valid`
+
+      log.error(errMsg)
+      return callback(errcode(new Error(errMsg), 'ERR_INVALID_NAME'))
+    }
+
     if (typeof options === 'function') {
       callback = options
       options = {}
     }
 
+    options = options || {}
+
     // If recursive, we should not try to get the cached value
     if (!options.nocache && !options.recursive) {
       // Try to get the record from cache

src/core/ipns/publisher.js

@@ -32,7 +32,7 @@ class IpnsPublisher {
 
     PeerId.createFromPrivKey(privKey.bytes, (err, peerId) => {
       if (err) {
-        callback(err)
+        return callback(err)
       }
 
       this._updateOrCreateRecord(privKey, value, lifetime, peerId, (err, record) => {

src/core/ipns/republisher.js

@@ -18,10 +18,12 @@ const defaultBroadcastInterval = 4 * hour
 const defaultRecordLifetime = 24 * hour
 
 class IpnsRepublisher {
-  constructor (publisher, ipfs) {
+  constructor (publisher, repo, peerInfo, keychain, options) {
     this._publisher = publisher
-    this._ipfs = ipfs
-    this._repo = ipfs._repo
+    this._repo = repo
+    this._peerInfo = peerInfo
+    this._keychain = keychain
+    this._options = options
     this._republishHandle = null
   }
 
@@ -62,8 +64,8 @@ class IpnsRepublisher {
       }
     }
 
-    const { privKey } = this._ipfs._peerInfo.id
-    const { pass } = this._ipfs._options
+    const { privKey } = this._peerInfo.id
+    const { pass } = this._options
 
     republishHandle.runPeriodically((done) => {
       this._republishEntries(privKey, pass, () => done(defaultBroadcastInterval))
@@ -98,16 +100,16 @@ class IpnsRepublisher {
       }
 
       // keychain needs pass to get the cryptographic keys
-      if (this._ipfs._keychain && Boolean(pass)) {
-        this._ipfs._keychain.listKeys((err, list) => {
+      if (this._keychain && Boolean(pass)) {
+        this._keychain.listKeys((err, list) => {
           if (err) {
             log.error(err)
             return
           }
 
           each(list, (key, cb) => {
             waterfall([
-              (cb) => this._ipfs._keychain.exportKey(key.name, pass, cb),
+              (cb) => this._keychain.exportKey(key.name, pass, cb),
               (pem, cb) => crypto.keys.import(pem, pass, cb)
             ], (err, privKey) => {
               if (err) {

src/core/ipns/resolver.js

@@ -89,19 +89,33 @@ class IpnsResolver {
 
   // resolve ipns entries from the provided routing
   _resolveName (name, callback) {
-    const peerId = PeerId.createFromB58String(name)
+    let peerId
+
+    try {
+      peerId = PeerId.createFromB58String(name)
+    } catch (err) {
+      return callback(err)
+    }
+
     const { routingKey } = ipns.getIdKeys(peerId.toBytes())
 
     // TODO DHT - get public key from routing?
     // https://github.com/ipfs/go-ipfs/blob/master/namesys/routing.go#L70
     // https://github.com/libp2p/go-libp2p-routing/blob/master/routing.go#L99
 
     this._routing.get(routingKey.toBuffer(), (err, res) => {
-      if (err || !res) {
+      if (err) {
         const errMsg = `record requested was not found for ${name} (${routingKey}) in the network`
 
         log.error(errMsg)
-        return callback(errcode(new Error(errMsg), 'ERR_NO_NETWORK_RECORD_FOUND'))
+        return callback(errcode(new Error(errMsg), 'ERR_NO_RECORD_FOUND'))
+      }
+
+      if (!res) {
+        const errMsg = `record requested was empty for ${name} (${routingKey}) in the network`
+
+        log.error(errMsg)
+        return callback(errcode(new Error(errMsg), 'ERR_EMPTY_RECORD_FOUND'))
       }
 
       if (!Buffer.isBuffer(res)) {
@@ -111,8 +125,15 @@ class IpnsResolver {
         return callback(errcode(new Error(errMsg), 'ERR_INVALID_RECORD_RECEIVED'))
       }
 
-      const record = Record.deserialize(res)
-      const ipnsEntry = ipns.unmarshal(record.value)
+      let ipnsEntry
+
+      try {
+        const record = Record.deserialize(res)
+        ipnsEntry = ipns.unmarshal(record.value)
+      } catch (err) {
+        log.error(err)
+        return callback(err)
+      }
 
       ipns.extractPublicKey(peerId, ipnsEntry, (err, pubKey) => {
         if (err) {

src/core/ipns/routing/offline-datastore.js

@@ -3,6 +3,11 @@
 const { Key } = require('interface-datastore')
 const { encodeBase32 } = require('./utils')
 
+const errcode = require('err-code')
+const debug = require('debug')
+const log = debug('jsipfs:ipns:offline-datastore')
+log.error = debug('jsipfs:ipns:offline-datastore:error')
+
 // Offline datastore aims to mimic the same encoding as routing when storing records
 // to the local datastore
 class OfflineDatastore {
@@ -18,8 +23,30 @@ class OfflineDatastore {
    * @returns {void}
    */
   put (key, value, callback) {
-    // encode key properly - base32(/ipns/{cid})
-    const routingKey = new Key('/' + encodeBase32(key), false)
+    if (!Buffer.isBuffer(key)) {
+      const errMsg = `Offline datastore key must be a buffer`
+
+      log.error(errMsg)
+      return callback(errcode(new Error(errMsg), 'ERR_INVALID_KEY'))
+    }
+
+    if (!Buffer.isBuffer(value)) {
+      const errMsg = `Offline datastore value must be a buffer`
+
+      log.error(errMsg)
+      return callback(errcode(new Error(errMsg), 'ERR_INVALID_VALUE'))
+    }
+
+    let routingKey
+
+    try {
+      routingKey = this._routingKey(key)
+    } catch (err) {
+      const errMsg = `Not possible to generate the routing key`
+
+      log.error(errMsg)
+      return callback(errcode(new Error(errMsg), 'ERR_GENERATING_ROUTING_KEY'))
+    }
 
     this._repo.datastore.put(routingKey, value, callback)
   }
@@ -31,11 +58,31 @@ class OfflineDatastore {
    * @returns {void}
    */
   get (key, callback) {
-    // encode key properly - base32(/ipns/{cid})
-    const routingKey = new Key('/' + encodeBase32(key), false)
+    if (!Buffer.isBuffer(key)) {
+      const errMsg = `Offline datastore key must be a buffer`
+
+      log.error(errMsg)
+      return callback(errcode(new Error(errMsg), 'ERR_INVALID_KEY'))
+    }
+
+    let routingKey
+
+    try {
+      routingKey = this._routingKey(key)
+    } catch (err) {
+      const errMsg = `Not possible to generate the routing key`
+
+      log.error(errMsg)
+      return callback(errcode(new Error(errMsg), 'ERR_GENERATING_ROUTING_KEY'))
+    }
 
     this._repo.datastore.get(routingKey, callback)
   }
+
+  // encode key properly - base32(/ipns/{cid})
+  _routingKey (key) {
+    return new Key('/' + encodeBase32(key), false)
+  }
 }
 
 exports = module.exports = OfflineDatastore

src/core/ipns/routing/utils.js

@@ -1,8 +1,7 @@
 'use strict'
 
-const base32 = require('base32.js')
+const multibase = require('multibase')
 
 module.exports.encodeBase32 = (buf) => {
-  const enc = new base32.Encoder()
-  return enc.write(buf).finalize()
+  return multibase.encode('base32', buf)
 }