fix: code review

Author: vasco-santos

README.md

@@ -595,8 +595,8 @@ The core API is grouped into several areas:
   - [`ipfs.bootstrap.rm(peer, [options], [callback])`](https://github.com/ipfs/interface-ipfs-core/blob/master/SPEC/BOOTSTRAP.md#bootstraprm)
 
 - [dht](https://github.com/ipfs/interface-ipfs-core/tree/master/SPEC/)
-  - [`ipfs.dht.findpeer(peerId, [callback])`](https://github.com/ipfs/interface-ipfs-core/blob/master/SPEC/DHT.md#dhtfindpeer)
-  - [`ipfs.dht.findprovs(multihash, [callback])`](https://github.com/ipfs/interface-ipfs-core/blob/master/SPEC/DHT.md#dhtfindprovs)
+  - [`ipfs.dht.findPeer(peerId, [callback])`](https://github.com/ipfs/interface-ipfs-core/blob/master/SPEC/DHT.md#dhtfindpeer)
+  - [`ipfs.dht.findProvs(multihash, [callback])`](https://github.com/ipfs/interface-ipfs-core/blob/master/SPEC/DHT.md#dhtfindprovs)
   - [`ipfs.dht.get(key, [callback])`](https://github.com/ipfs/interface-ipfs-core/blob/master/SPEC/DHT.md#dhtget)
   - [`ipfs.dht.provide(cid, [callback])`](https://github.com/ipfs/interface-ipfs-core/blob/master/SPEC/DHT.md#dhtprovide)
   - [`ipfs.dht.put(key, value, [callback])`](https://github.com/ipfs/interface-ipfs-core/blob/master/SPEC/DHT.md#dhtput)

src/cli/commands/dht/find-providers.js

@@ -17,7 +17,7 @@ module.exports = {
 
   handler (argv) {
     const opts = {
-      numProviders: argv['num-providers']
+      maxNumProviders: argv['num-providers']
     }
 
     argv.ipfs.dht.findProvs(argv.key, opts, (err, result) => {

src/core/components/dht.js

@@ -7,7 +7,11 @@ const PeerInfo = require('peer-info')
 const CID = require('cids')
 const each = require('async/each')
 const setImmediate = require('async/setImmediate')
-const errCode = require('err-code')
+const errcode = require('err-code')
+
+const debug = require('debug')
+const log = debug('jsipfs:dht')
+log.error = debug('jsipfs:dht:error')
 
 module.exports = (self) => {
   return {
@@ -16,7 +20,7 @@ module.exports = (self) => {
      *
      * @param {Buffer} key
      * @param {Object} options - get options
-     * @param {number} options.maxTimeout - optional timeout
+     * @param {number} options.timeout - optional timeout
      * @param {function(Error)} [callback]
      * @returns {Promise|void}
      */
@@ -30,9 +34,11 @@ module.exports = (self) => {
 
       if (!Buffer.isBuffer(key)) {
         try {
-          key = (new CID(key)).buffer()
+          key = (new CID(key)).buffer
         } catch (err) {
-          return setImmediate(() => callback(errCode(err, 'ERR_INVALID_CID')))
+          log.error(err)
+
+          return setImmediate(() => callback(errcode(err, 'ERR_INVALID_CID')))
         }
       }
 
@@ -54,9 +60,11 @@ module.exports = (self) => {
     put: promisify((key, value, callback) => {
       if (!Buffer.isBuffer(key)) {
         try {
-          key = (new CID(key)).buffer()
+          key = (new CID(key)).buffer
         } catch (err) {
-          return setImmediate(() => callback(errCode(err, 'ERR_INVALID_CID')))
+          log.error(err)
+
+          return setImmediate(() => callback(errcode(err, 'ERR_INVALID_CID')))
         }
       }
 
@@ -68,7 +76,7 @@ module.exports = (self) => {
      *
      * @param {CID} key - They key to find providers for.
      * @param {Object} options - findProviders options
-     * @param {number} options.maxTimeout - how long the query should maximally run, in milliseconds (default: 60000)
+     * @param {number} options.timeout - how long the query should maximally run, in milliseconds (default: 60000)
      * @param {number} options.maxNumProviders - maximum number of providers to find
      * @param {function(Error, Array<PeerInfo>)} [callback]
      * @returns {Promise<PeerInfo>|void}
@@ -80,14 +88,14 @@ module.exports = (self) => {
       }
 
       options = options || {}
-      options.timeout = options.maxTimeout // TODO create PR kad-dht
-      options.maxNumProviders = options.numProviders
 
       if (typeof key === 'string') {
         try {
           key = new CID(key)
         } catch (err) {
-          return setImmediate(() => callback(errCode(err, 'ERR_INVALID_CID')))
+          log.error(err)
+
+          return setImmediate(() => callback(errcode(err, 'ERR_INVALID_CID')))
         }
       }
 
@@ -98,8 +106,8 @@ module.exports = (self) => {
      * Query the DHT for all multiaddresses associated with a `PeerId`.
      *
      * @param {PeerId} peer - The id of the peer to search for.
-     * @param {function(Error, Array<PeerInfo>)} [callback]
-     * @returns {Promise<Array<PeerInfo>>|void}
+     * @param {function(Error, PeerInfo)} [callback]
+     * @returns {Promise<PeerInfo>|void}
      */
     findPeer: promisify((peer, callback) => {
       if (typeof peer === 'string') {
@@ -138,11 +146,15 @@ module.exports = (self) => {
         }
 
         if (!has) {
-          return callback(new Error('block(s) not found locally, cannot provide'))
+          const errMsg = 'block(s) not found locally, cannot provide'
+
+          log.error(errMsg)
+          return callback(errcode(errMsg, 'ERR_BLOCK_NOT_FOUND'))
         }
 
         if (options.recursive) {
           // TODO: Implement recursive providing
+          return callback(errcode('not implemented yet', 'ERR_NOT_IMPLEMENTED_YET'))
         } else {
           each(keys, (cid, cb) => {
             self.libp2p.contentRouting.provide(cid, cb)
@@ -163,17 +175,18 @@ module.exports = (self) => {
         try {
           peerId = PeerId.createFromB58String(peerId)
         } catch (err) {
+          log.error(err)
           return callback(err)
         }
       }
 
       // TODO expose this method in peerRouting
       self.libp2p._dht.getClosestPeers(peerId.toBytes(), (err, peerIds) => {
         if (err) {
+          log.error(err)
           return callback(err)
         }
 
-        // callback(null, peerIds)
         callback(null, peerIds.map((id) => new PeerInfo(id)))
       })
     })

src/http/api/resources/dht.js

@@ -1,6 +1,7 @@
 'use strict'
 
 const Joi = require('joi')
+
 const CID = require('cids')
 
 const debug = require('debug')
@@ -23,6 +24,13 @@ exports.findPeer = {
       if (err) {
         log.error(err)
 
+        if (err.code === 'ERR_LOOKUP_FAILED') {
+          return reply({
+            Message: err.toString(),
+            Code: 0
+          }).code(404)
+        }
+
         return reply({
           Message: err.toString(),
           Code: 0
@@ -44,15 +52,17 @@ exports.findProvs = {
   validate: {
     query: Joi.object().keys({
       arg: Joi.string().required(),
-      'num-providers': Joi.number().integer().default(20)
+      'num-providers': Joi.number().integer().default(20),
+      timeout: Joi.number()
     }).unknown()
   },
   handler: (request, reply) => {
     const ipfs = request.server.app.ipfs
     const { arg } = request.query
-    const cid = new CID(arg)
 
-    ipfs.dht.findProvs(cid, request.query, (err, res) => {
+    request.query.maxNumProviders = request.query['num-providers']
+
+    ipfs.dht.findProvs(arg, request.query, (err, res) => {
       if (err) {
         log.error(err)
 
@@ -76,7 +86,8 @@ exports.findProvs = {
 exports.get = {
   validate: {
     query: Joi.object().keys({
-      arg: Joi.string().required()
+      arg: Joi.string().required(),
+      timeout: Joi.number()
     }).unknown()
   },
   handler: (request, reply) => {

test/http-api/inject/dht.js

@@ -27,14 +27,14 @@ module.exports = (http) => {
         })
       })
 
-      it('returns 500 if peerId is provided as there is no peers in the routing table', (done) => {
+      it('returns 404 if peerId is provided as there is no peers in the routing table', (done) => {
         const peerId = 'QmQ2zigjQikYnyYUSXZydNXrDRhBut2mubwJBaLXobMt3A'
 
         api.inject({
           method: 'GET',
           url: `/api/v0/dht/findpeer?arg=${peerId}`
         }, (res) => {
-          expect(res.statusCode).to.equal(500)
+          expect(res.statusCode).to.equal(404)
           done()
         })
       })