-
Notifications
You must be signed in to change notification settings - Fork 454
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
fix: peerRouting.findPeer() trying to find self #941
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.
Thanks @zeim839
Can you please add a test in https://github.com/libp2p/js-libp2p/blob/master/test/peer-routing/peer-routing.node.js#L53 and https://github.com/libp2p/js-libp2p/blob/master/test/peer-routing/peer-routing.node.js#L274 with a fail expect such as in https://github.com/libp2p/js-libp2p/blob/master/test/peer-routing/peer-routing.node.js#L37 ?
Co-authored-by: Vasco Santos <vasco.santos@ua.pt>
@vasco-santos Added the requested tests to the best of my knowledge. They both pass. Let me know if there's anything else I need to change/improve. |
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 (pushed commit fixing linting)
Thanks @zeim839
Resolves an issue where nodes running findPeer() on themselves return abstract aggregateErrors instead of something "more suitable" like ERR_DIALED_SELF. Original issue can be found here.
This is accomplished by comparing the _idB58String of the ID parameter to this._peerId._idB58String at peer-routing.js. if the two values are equal, an error is thrown alerting the user of the mistake. For example: