Add websocket transport to transit relay #63
Conversation
|
For compatibility, I'd like to keep implementations in sync with the specification. At the moment, the latest draft spec for this is magic-wormhole/magic-wormhole-protocols#16. I implemented the client part in the Rust client and am rather satisfied, but feel free to propose changes or alternatives that better match what you have in mind. I think everything except the new hints encoding should be rather uncontroversial. |
|
I'll try to review this as soon as I can. Wasm support in Fyne is nearing completion, so I'll want to try and get wormhole-gui running there as soon as possible. |
piegamesde
force-pushed
the
websocket-relay
branch
from
5c2afad
to
6945f34
Compare
piegamesde
force-pushed
the
websocket-relay
branch
from
6945f34
to
0c84f86
Compare
piegamesde
force-pushed
the
websocket-relay
branch
from
0c84f86
to
ccdfebc
Compare
piegamesde
force-pushed
the
websocket-relay
branch
from
ccdfebc
to
be22746
Compare
|
|
||
| switch t.relayURL.Scheme { | ||
| case "tcp": | ||
| conn, err = d.DialContext(ctx, "tcp", addr) |
There was a problem hiding this comment.
So, switching to net/url would mean, the tcp based relay urls would also be encoded as tcp://host:port, right? Would that break compatibility with other wormhole clients that still follow the twisted url format? In that case, we should note that in the README perhaps?
| }) | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
There is a lot of repetition of test code. Can this be abstracted into another function with proper parameterization? May be something to keep in mind for another PR, not this one.
| func newTestRelayServer() *testRelayServer { | ||
| l, err := net.Listen("tcp", ":0") | ||
| func newTestTCPRelayServer() *testRelayServer { | ||
| l, err := net.Listen("tcp4", ":0") |
There was a problem hiding this comment.
Any specific reason why we are listening only on ipv4?
wormhole/wormhole_test.go
Outdated
| if err != nil { | ||
| panic(err) | ||
| } | ||
|
|
||
| rs := &testRelayServer{ | ||
| l: l, | ||
| addr: l.Addr().String(), | ||
| url: internal.MustNewSimpleURL("tcp:" + l.Addr().String()), |
There was a problem hiding this comment.
Is this valid, now that we are using net/url?
There was a problem hiding this comment.
Also, do we need internal/url anymore? I am guessing we don't need it as we use generic urls now?
| if relayType == "direct-tcp-v1" { | ||
| var port, err = strconv.Atoi(t.relayURL.Port()) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("invalid port") |
There was a problem hiding this comment.
I think it would make more sense to use errors.New here instead.
| case "ws", "wss": | ||
| c, _, err := websocket.Dial(ctx, t.relayURL.String(), nil) | ||
| if err != nil { | ||
| return fmt.Errorf("websocket.Dial failed") |
| transport := newFileTransport(transitKey, appID, c.relayAddr()) | ||
| relayUrl, err := c.relayURL() | ||
| if err != nil { | ||
| return nil, fmt.Errorf("Invalid relay URL") |
There was a problem hiding this comment.
Same as my comment above. Also, should this one perhaps be moved out to a global variable given that it's used in more than one place?
| @@ -296,15 +292,20 @@ func (c *Client) sendFileDirectory(ctx context.Context, offer *offerMsg, r io.Re | |||
| } | |||
| } | |||
|
|
|||
| var relayUrl, err = c.relayURL() | |||
| if err != nil { | |||
| sendErr(fmt.Errorf("Invalid relay URL")) | |||
| // TransitRelayAddress is the host:port address to offer | ||
| // TransitRelayURL is the proto://host:port address to offer | ||
| // to use for file transfers where direct connections are unavailable. | ||
| // If empty, DefaultTransitRelayAddress will be used. | ||
| TransitRelayAddress string | ||
| // If empty, DefaultTransitRelayURL will be used. | ||
| TransitRelayURL string |
There was a problem hiding this comment.
We need to be careful given that this is a braking change. I don't know what plans of @psanford are with this but, if we were to release this in a minor release, would it be possible to do this with just deprecating the old field?
| // DefaultTransitRelayAddress is the default transit server to ues. | ||
| DefaultTransitRelayAddress = "transit.magic-wormhole.io:4001" | ||
| // DefaultTransitRelayURL is the default transit server to ues. | ||
| DefaultTransitRelayURL = "tcp://transit.magic-wormhole.io:4001" |
There was a problem hiding this comment.
Some comment on breaking API as above.
piegamesde
force-pushed
the
websocket-relay
branch
from
be22746
to
c998891
Compare
|
I think it would be good if the changes weren't force-pushed every time. It makes it very hard to see changes between commits (if not impossible). |
|
I had to rebase onto latest master in order to get some of the bug fixes for testing. For force pushes that are just amends and no rebases, you can click on the "force-pushed" in the GitHub UI to get the difference between both commits. |
Co-authored-by: Ramakrishnan Muthukrishnan <ram@leastauthority.com> Co-authored-by: Bryan White <bryanchriswhite@gmail.com>
piegamesde
force-pushed
the
websocket-relay
branch
from
c998891
to
6ba7c16
Compare
Our team has been using wormhole-william as our project's client library in the browser via a WASM wrapper. This PR adds support for websocket transport to the transit relay. The transit relay needs to use a transport that is compatible with browser environments. Websockets seem to be the obvious solution here; foregoing any webrtc-based approaches (which we're not interested in pursuing right now).