Skip to content
This repository has been archived by the owner on Jul 21, 2023. It is now read-only.

fix: ipv6 naming with multiaddr-to-uri package #81

Merged
merged 2 commits into from
Jan 24, 2019

Conversation

vasco-santos
Copy link
Member

@vasco-santos vasco-santos commented Jan 23, 2019

When dht is enabled ipfs/js-ipfs#856, we found an issue where some browser nodes were using ipv6. As soon as we connect them, the connection fails as a result of using an invalid url.

Example: ws://::1:4002

Node url implementation expects a ipv6 address in the following format: ws://[::1]:4002

In addition, we do not have to maintain the multiaddr to url code previously here.

@ghost ghost assigned vasco-santos Jan 23, 2019
@ghost ghost added the status/in-progress In progress label Jan 23, 2019
Copy link
Contributor

@jacobheun jacobheun left a comment

Choose a reason for hiding this comment

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

We should get some tests added for this. Currently there are several skipped, empty ipv6 tests. Right now we're only testing filtering. It would be great to get listening and dialing tests in place for at least basic dialing and listening.

@vasco-santos
Copy link
Member Author

@jacobheun I agree, I will add those tests

@vasco-santos vasco-santos force-pushed the chore/use-multiaddr-to-uri-package branch from 94f7d00 to b7a9359 Compare January 24, 2019 10:47
@vasco-santos vasco-santos force-pushed the chore/use-multiaddr-to-uri-package branch from b7a9359 to dd15f5a Compare January 24, 2019 11:10
@jacobheun jacobheun merged commit 93ef7c3 into master Jan 24, 2019
@jacobheun jacobheun deleted the chore/use-multiaddr-to-uri-package branch January 24, 2019 15:22
@ghost ghost removed the status/in-progress In progress label Jan 24, 2019
@jacobheun
Copy link
Contributor

This fix is available in v0.12.2

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants