Skip to content
This repository has been archived by the owner on Mar 10, 2020. It is now read-only.

fix: dht validate if receiving stream #890

Merged
merged 6 commits into from
Dec 11, 2018

Conversation

vasco-santos
Copy link
Contributor

@vasco-santos vasco-santos commented Nov 9, 2018

With go-ipfs we receive a stream of data. However, we currently do not receive a stream using js-ipfs. Types of notifications in go-ipfs are defined here.

Needs:

@ghost ghost assigned vasco-santos Nov 9, 2018
@ghost ghost added the in progress label Nov 9, 2018
@vasco-santos vasco-santos force-pushed the fix/dht-validate-if-receiving-stream branch 5 times, most recently from 79e1c4a to bc9f037 Compare November 10, 2018 23:49
@vasco-santos vasco-santos force-pushed the fix/dht-validate-if-receiving-stream branch from bc9f037 to 390699a Compare November 11, 2018 00:09
@vasco-santos vasco-santos force-pushed the fix/dht-validate-if-receiving-stream branch 8 times, most recently from d6c24c5 to 173950c Compare November 14, 2018 12:06
@vasco-santos vasco-santos force-pushed the fix/dht-validate-if-receiving-stream branch from 173950c to eb54a85 Compare November 20, 2018 21:11
@vasco-santos vasco-santos force-pushed the fix/dht-validate-if-receiving-stream branch from eb54a85 to f7f0c86 Compare December 5, 2018 10:45
@vasco-santos vasco-santos force-pushed the fix/dht-validate-if-receiving-stream branch from f7f0c86 to f703c17 Compare December 5, 2018 14:46
addrs = res.responses[0].addrs
}

// inconsistencies js / go - go does not add `/ipfs/{id}` to the address
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to fix this here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, go-ipfs response contains only address. As a consequence, it will fail the following test:

ipfs/interface-ipfs-core/js/src/dht/findpeer.js#L48

I can change the test in interface-ipfs-core and remove this if you feel that it is less hacky.

Copy link
Contributor

@alanshaw alanshaw Dec 10, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What I'm concerned about is that we're making assumptions about the format of the address. What does go-ipfs return when it's a circuit address? Would that cause a false positive?

Ideally both implementations should return the same format! Do you know the reason why go-ipfs returns the addr without the peer ID? Perhaps for a smaller payload? It would be pretty easy for js-ipfs to decapsulate the peer id from the addrs wouldn't it?

} else {
id = res.responses[0].id
addrs = res.responses[0].addrs
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apologies if I wasn't clear, interface-ipfs-core should return lowered property names but the HTTP API should be compatible with go-ipfs.

Here I expect us to be converting "Uppered" property names to lowered form. In js-ipfs the HTTP API should take the lowered property names from core and "Upper" them to be compatible. I know this is a PITA but this is consistent with all our APIs.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will change it! Thanks for clarifying.

@vasco-santos vasco-santos force-pushed the fix/dht-validate-if-receiving-stream branch 3 times, most recently from 268be9a to bc35a9c Compare December 10, 2018 16:19
@alanshaw
Copy link
Contributor

[email protected] is released

return peerInfo
})

callback(null, responses)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needs switch to returning a single response :D

@vasco-santos vasco-santos force-pushed the fix/dht-validate-if-receiving-stream branch from bc35a9c to c9dabf2 Compare December 10, 2018 22:10
Alan Shaw added 2 commits December 11, 2018 06:48
License: MIT
Signed-off-by: Alan Shaw <[email protected]>
License: MIT
Signed-off-by: Alan Shaw <[email protected]>
@ghost ghost assigned alanshaw Dec 11, 2018
@alanshaw alanshaw merged commit 05a84a4 into master Dec 11, 2018
@alanshaw alanshaw deleted the fix/dht-validate-if-receiving-stream branch December 11, 2018 09:00
@ghost ghost removed the in progress label Dec 11, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants