Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DHT addon #93

Closed
wants to merge 2 commits into from
Closed

DHT addon #93

wants to merge 2 commits into from

Conversation

BoBeR182
Copy link

@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
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
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
Member

nurupo commented Sep 5, 2016

Do we want this in toxcore master?

@GrayHatter
Copy link

Do we want this in toxcore master?

I don't think we do

@JFreegman
Copy link
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
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

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
Member

iphydf commented Sep 9, 2016

My thoughts on this: #119

@iphydf
Copy link
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