RPC and WS endpoints share the same port #187
Conversation
| d.cancel() | ||
| os.Exit(1) | ||
| } | ||
| }() |
There was a problem hiding this comment.
Start() now blocks and it internally uses the context that was passed to rpcCfg.
| val = NewEtherAmount(1234567890) | ||
| require.Equal(t, fmt.Sprint(val.ToDecimals(6)), "1234.56789") |
There was a problem hiding this comment.
The expected/actual values here were reversed and it seemed like as good a place as any to use the FmtFloat function added by this PR. I've never used fmt.Sprint like above, but I imagine it behaves like "%v" in a formatted string, which in some cases prints undesirable exponent notation.
|
|
||
| // | ||
| // This file only contains mock definitions used by other test files | ||
| // |
There was a problem hiding this comment.
These mocks were previously located in ws_test.go. I didn't realize that they were also being shared with net_test.go, so I thought pulling them out into a separate file might make make it clear that they are shared. I put panics in most of them, to make it more obvious to the reader which functions are actually getting invoked. If we decide to instrument the mocks with testify's mocking at a later time (https://github.com/stretchr/testify#mock-package), we wouldn't need to instrument all functions.
I'm also curious if it might be worth an attempt to update these tests so that they don't use mocking at all?
There was a problem hiding this comment.
it's a lot more complex to update the tests to not use mocking, as we basically need to start up everything in the daemon. it might be nice to add them as integration tests, instead of unit tests.
| StatusCh: make(chan types.Status, 1), | ||
| InfoFile: "/dev/null", | ||
| } | ||
| offerExtra.StatusCh <- types.CompletedSuccess |
There was a problem hiding this comment.
I uncommented the TestSubscribeMakeOffer test. I'm open to better ideas on how to transition the state for it.
| @@ -43,67 +42,71 @@ type Config struct { | |||
|
|
|||
| // NewServer ... | |||
| func NewServer(cfg *Config) (*Server, error) { | |||
There was a problem hiding this comment.
I pulled all the things that can fail in the lead-up to launching the server out of the Start() method and into this NewServer method. One of the things is allocating the listener, as that allows us to pass 0 for the port and have the OS assign us a free ephemeral port. This is useful when testing, when the actual port used is unimportant, but it must be a free port. We want to be able to query the allocated port back from the Server object before calling the blocking method Start().
| originsOk := handlers.AllowedOrigins([]string{"*"}) | ||
| server := &http.Server{ | ||
| Addr: ln.Addr().String(), | ||
| ReadHeaderTimeout: time.Second, |
There was a problem hiding this comment.
Interestingly, the linter flagged this as vulnerable to the Slowloris attack (https://www.cloudflare.com/learning/ddos/ddos-attack-tools/slowloris/) without the ReadHeaderTimeout value, but as best I can tell the original ListenAndServe call we were using would also be vulnerable, but doesn't get flagged by the linter. I guess because there is not API to set it.
| return nil, ctx.Err() | ||
| default: | ||
| // not cancelled, continue on | ||
| } |
There was a problem hiding this comment.
This was mildly painful. We were introducing a context cancel in the TestDaemon_DevXMR[MT]aker tests in cmd/daemon to stop the http server and it was triggering a panic in this libp2p code. Apparently, this libp2p code has really slow startup times, as I even tried sleeping for a 2 whole seconds before issuing the context cancel and I was still getting panics.
There was a problem hiding this comment.
good to know, also yeah that's painful
| @@ -14,7 +14,7 @@ | |||
| import HelperText from '@smui/textfield/helper-text' | |||
| import { currentAccount, sign } from '../stores/metamask' | |||
|
|
|||
| const WS_ADDRESS = 'ws://127.0.0.1:8081' | |||
| const WS_ADDRESS = 'ws://127.0.0.1:5001/ws' | |||
There was a problem hiding this comment.
I suspect this is the only change needed, but since the UI doesn't work for me, I can't test it.
| }, | ||
| }, | ||
| }, | ||
| Flags: []cli.Flag{daemonAddrFlag}, |
There was a problem hiding this comment.
I wanted users to be able to specify --swapd-port before or after the subcommand. The spf13/cobra library makes this easy to do, but I guess it is not supported by urfave: urfave/cli#1391
When we define the same flag globally and after the command, the behavior is bad. I wasn't getting an error for the global flag (since it was listed as a global flag), but when reading it, we would get the defaulted value from the flag to the subcommand instead of the globally set value.
Example:
./swapcli --swapd-port 5432 address
The actual port used would be 5001, because the defaulted value for --swapd-port after the address subcommand was taking precedence over the 5432 value.
| TakeOfferAndSubscribe(multiaddr, offerID string, | ||
| providesAmount float64) (id uint64, ch <-chan types.Status, err error) | ||
| SubscribeSwapStatus(id types.Hash) (<-chan types.Status, error) | ||
| TakeOfferAndSubscribe(multiaddr, offerID string, providesAmount float64) (ch <-chan types.Status, err error) |
There was a problem hiding this comment.
I didn't change these functions. The interface signatures were just out of date with the existing implementation.
Combine our HTTP RPC and Websocket services into a single HTTP server, eliminating the --ws-port flag to swapd and using --swapd-port flag for swapcli.
This PR combines our HTTP RPC and Websocket services into a single HTTP server, eliminating the
--ws-portflag toswapd. This lowers end-user complexity, especially when running multipleswapdservers on one host.swapclisubscribe interactions are fixed with this PR and the--daemon-addr http://127.0.0.1:PORTflag toswapcliwas removed. Instead of passing separate http and websocket URLs toswapcli,swapclicommands now use--swapd-port PORT_NUMallowingswapclito interact with both services, while only requiring one flag from the end user.