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

feat: Adds ws and wss multiaddr #72

Closed
wants to merge 1 commit into from

Conversation

gpestana
Copy link

Adds ws and wss multiaddr as defined in https://github.com/multiformats/multiaddr/blob/master/protocols.csv

Related to #64

@Stebalien
Copy link
Member

The WS protocol is currently defined in go-ws-transport (but we don't define the wss protocol because we can't use it).

However, I understand the issue.

@whyrusleeping

Issue: We can neither parse nor print a multiaddr unless we understand the full multiaddr.
Solutions:

  1. This PR (put every protocol definition we can in one repo).
  2. Add an "unknown" protocol to the string format: Handle unparsable multiaddrs multiaddr#70

The second completely solves the issue that JS is having but the first one is (a) easier and (b) provides better usability.

@gpestana
Copy link
Author

The 2) option sounds reasonable to me, although I might not fully understand what are the issues JS is having with the option 1). I think that if the WS protocol is implemented somewhere else, it should be documented somewhere here as it gets confusing for newcomers.

What would be the criteria to add a new protocol to the list and implement it vs treat it as unknown?

@Stebalien
Copy link
Member

The current issue for javascript land is that go-multiaddr doesn't understand protocols like wss or webrtc (?). This means that:

  1. We throw away these addresses when we encounter them (in the DHT).
  2. We can't dial addresses like /ip4/4.3.2.1/tcp/321/p2p/QmRelay/p2p-circuit/ip4/1.2.3.4/tcp/123/wss/ipfs/QmDest even if we speak all the protocols we need to speak (those before p2p-circuit).

Adding these protocols to this package will fix these issues for now. However, the second we add a new protocol, we'll have the same problem.

We can fix 1 by simply not decoding these addresses. However, that would be a large, weird change and I'd like to avoid it if possible. Note: that issue will be fixed eventually once we get ipfs/notes#291 (comment) but that's a ways off.

Fix 2 (the "unknown" protocol) fixes issue 2 from above. That's basically the only fix I can think of.

Stebalien added a commit that referenced this pull request Feb 29, 2020
Now that we define all protocols in this package, we might as well add wss.

fixes #72
@Stebalien Stebalien mentioned this pull request Feb 29, 2020
@Stebalien
Copy link
Member

We went with option 1 (define everything here) for now. Updated PR in #122.

@Stebalien Stebalien closed this Feb 29, 2020
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.

2 participants