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

fix: peers page with /p2p-websocket-star addrs #1306

Merged
merged 6 commits into from
Nov 12, 2019

Conversation

hacdias
Copy link
Member

@hacdias hacdias commented Nov 12, 2019

Closes #1287 by checking if addr.address is an actual IP.

License: MIT
Signed-off-by: Henrique Dias [email protected]

License: MIT
Signed-off-by: Henrique Dias <[email protected]>
@hacdias hacdias requested a review from lidel November 12, 2019 08:00
@hacdias hacdias mentioned this pull request Nov 12, 2019
8 tasks
Copy link
Member

@lidel lidel left a comment

Choose a reason for hiding this comment

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

I am still getting Error: Invalid ip address: ws-star.discovery.libp2p.io, same as in #1287.

@hacdias try adding unit tests for isPrivateAndNearby and test it against multiaddrs like:

/dnsaddr/ws-star.discovery.libp2p.io/tcp/443/wss/p2p-websocket-star/ipfs/QmbJbcN3Fvy5bC7Tr95STx5VFiP1G1WLPCHNceh1yShfbb
/dns4/ws-star.discovery.libp2p.io/tcp/443/wss/p2p-websocket-star/ipfs/QmYy3ka6HsQzpdTXY63nUKShsVdgg5zdhMXZXFeNGPyMT4

License: MIT
Signed-off-by: Henrique Dias <[email protected]>
@hacdias
Copy link
Member Author

hacdias commented Nov 12, 2019

@lidel apparently, those errors were not only be emitted from isPrivateAndNearby, but from getPublicIP too!

Just fixed it and added a bunch of tests!

Copy link
Member

@lidel lidel left a comment

Choose a reason for hiding this comment

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

Looks good now, the error is no more 👍
Small nits below, otherwise lgtm.

src/bundles/peer-locations.test.js Show resolved Hide resolved
src/bundles/peer-locations.test.js Outdated Show resolved Hide resolved
src/bundles/peer-locations.test.js Outdated Show resolved Hide resolved
src/bundles/peer-locations.test.js Outdated Show resolved Hide resolved
@hacdias hacdias merged commit ee9e6ff into master Nov 12, 2019
@hacdias hacdias deleted the fix/peers-p2p-websocket-star branch November 12, 2019 16:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Peers: crash on /p2p-websocket-star multiaddrs
2 participants