fix: update dht responses #389
Conversation
ghost
assigned 
vasco-santos
force-pushed
the
fix/update-dht-responses
branch
4 times, most recently
from
09703c3
to
bdb6563
Compare
vasco-santos
changed the title
vasco-santos
force-pushed
the
fix/update-dht-responses
branch
from
bdb6563
to
7575ec9
Compare
js/src/dht/findpeer.js
Outdated
| expect(err).to.not.exist() | ||
| // TODO upgrade the answer, format is weird | ||
| expect(peer[0].Responses[0].ID).to.be.equal(nodeB.peerId.id) | ||
| expect(res.Responses[0].Addrs).to.deep.include(nodeB.peerId.addresses[0]) |
There was a problem hiding this comment.
is deep necessary - the values are just strings right?
There was a problem hiding this comment.
Can we please fix the object that is returned here to not have go-style uppercased first letter properties? - for consistency with the rest of the API.
There was a problem hiding this comment.
According to http api docs, the addrs is an array of addresses. that's why I am using the deep.include.
Changed the uppercase letter. However, the inconsistencies will have to be solved in ipfs-api in ipfs/js-ipfs-api#890. go-ipfs sends the uppercase style, and js-ipfs (aiming to have its core implementation compliant with this interface) must send its response in lowercase. As a result, ipfs-api using js-ipfs will receive the lowercase ones. I added the logic to deal with this inconsistency in the ipfs-api PR, but I am not sure if it is the better approach in this case.
js/src/dht/findprovs.js
Outdated
| @@ -60,7 +60,7 @@ module.exports = (createCommon, options) => { | |||
| }, | |||
| (cidV0, cb) => nodeA.dht.findprovs(cidV0, cb), | |||
| (provs, cb) => { | |||
| expect(provs.map((p) => p.id.toB58String())) | |||
| expect(provs.Responses.map((p) => p.ID)) | |||
There was a problem hiding this comment.
Can we please fix the object that is returned here to not have go-style uppercased first letter properties? - for consistency with the rest of the API.
There was a problem hiding this comment.
Documentation or implementation for dht.findprovs needs to be updated - provs is not an array of PeerInfo instances.
https://github.com/ipfs/interface-ipfs-core/blob/master/SPEC/DHT.md#dhtfindprovs
vasco-santos
force-pushed
the
fix/update-dht-responses
branch
4 times, most recently
from
7a510f9
to
685ba00
Compare
vasco-santos
force-pushed
the
fix/update-dht-responses
branch
from
886ddd1
to
f258ad7
Compare
vasco-santos
force-pushed
the
fix/update-dht-responses
branch
from
f258ad7
to
8eead51
Compare
js/src/dht/findprovs.js
Outdated
| @@ -55,12 +55,12 @@ module.exports = (createCommon, options) => { | |||
| waterfall([ | |||
| (cb) => nodeB.object.new('unixfs-dir', cb), | |||
| (dagNode, cb) => { | |||
| const cidV0 = new CID(dagNode.toJSON().multihash) | |||
| const cidV0 = new CID(dagNode.toJSON().hash) | |||
| nodeB.dht.provide(cidV0, (err) => cb(err, cidV0)) | |||
There was a problem hiding this comment.
object.new now returns a CID
vasco-santos
force-pushed
the
fix/update-dht-responses
branch
from
8eead51
to
6db447f
Compare
In the context of the ipfs/js-ipfs#856 and ipfs/js-ipfs-api/pull/890