-
Notifications
You must be signed in to change notification settings - Fork 473
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
feat: discovery modules #486
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just some minor nits. Can you check the conflict?
src/index.js
Outdated
@@ -160,6 +160,7 @@ class Libp2p extends EventEmitter { | |||
log('libp2p is starting') | |||
try { | |||
await this._onStarting() | |||
this._isStarted = true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should put this in _onDidStart
as that keeps all post startup changes there.
src/index.js
Outdated
// Peer discovery | ||
if (this._modules.peerDiscovery) { | ||
this._setupPeerDiscovery() | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this if necessary? I think it would be good to have any necessary checks inside of _setupPeerDiscovery
. I think it will be much easier for us to avoid bugs if we have the functions handle their checks.
d6e4171
to
810e89b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! 🚀
* feat: discovery modules * chore: address review
* feat: discovery modules * chore: address review
Made necessary changes to integrate discovery modules, including the DHT random-walk.
Needs: