Skip to content

DHT addon#93

Closed
BoBeR182 wants to merge 2 commits intoTokTok:masterfrom
JFreegman:DHT-addon
Closed

DHT addon#93
BoBeR182 wants to merge 2 commits intoTokTok:masterfrom
JFreegman:DHT-addon

Conversation

@BoBeR182
Copy link
Copy Markdown

@BoBeR182 BoBeR182 commented Sep 3, 2016

allows for 3rd party apps to harvest DHT info


This change is NOT reviewable. Use Github reviews instead.

@BoBeR182 BoBeR182 changed the title Dht addon DHT addon Sep 3, 2016
@BoBeR182
Copy link
Copy Markdown
Author

BoBeR182 commented Sep 4, 2016

:lgtm:


Reviewed 2 of 2 files at r1.
Review status: all files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

@iphydf
Copy link
Copy Markdown
Member

iphydf commented Sep 4, 2016

Can you make the callback stateless as described in #40? Also, the merge commit is empty, you can remove it.

@iphydf iphydf self-assigned this Sep 4, 2016
@nurupo
Copy link
Copy Markdown
Member

nurupo commented Sep 5, 2016

Do we want this in toxcore master?

@GrayHatter
Copy link
Copy Markdown

Do we want this in toxcore master?

I don't think we do

@JFreegman
Copy link
Copy Markdown
Member

JFreegman commented Sep 8, 2016

This allows developers to leverage the DHT without using a third party implementation or having to write their own (which is very helpful for certain use cases). On the other hand, it's not used by the core itself and is not required for typical client usage, so it could be considered unnecessary. Is low-level access to the DHT something we want toxcore supply? If so, this probably belongs in the public API. Right now it's written as a bit of a hack and doesn't really fit in anywhere.

@BoBeR182
Copy link
Copy Markdown
Author

BoBeR182 commented Sep 8, 2016

I feel that creating a low level DHT access to the network can be helpful for some clients that want finer control over statistics or who you set as bootstrap nodes. Developers having an easier time means better apps get developed and more user growth.

@GrayHatter
Copy link
Copy Markdown

I'm all for adding this as an undocumented, unsupported API.

I'm not sure if this is the BEST way to go about it. But isn't not the worst way.

@iphydf
Copy link
Copy Markdown
Member

iphydf commented Sep 9, 2016

My thoughts on this: #119

@iphydf
Copy link
Copy Markdown
Member

iphydf commented Sep 22, 2016

Please discuss this change with irungentoo on the PR you made on his repo. If you have thoughts on #119, please post them there. You're very welcome to contribute ideas for a generalised DHT API that this callback could be part of.

@iphydf iphydf closed this Sep 22, 2016
@iphydf iphydf modified the milestone: v0.0.1 Nov 6, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

5 participants