Add basic websocket support for GDAX #970
Conversation
|
This is great! It might help people review the changes if they were squashed and rebased onto master with only relevent commit names retained (requires a git push --force to the branch to change the history). |
| if (!websocket_client[product_id]) { | ||
| // OrderbookSync extends WebsocketClient and subscribes to the 'full' channel, so we can use it like one | ||
| var auth = null | ||
| if (c.gdax && !c.gdax.key && c.gdax.key !== 'YOUR-API-KEY') { |
There was a problem hiding this comment.
Typo here: should be && c.gdax.key && -- right now the logic is ignoring authentication.
I tried doing websocket authentication locally and the exchange rejected my ip, even though I'd put it in the whitelist. Others' mileage may vary.
| public_client[product_id] = new Gdax.PublicClient(product_id, c.gdax.apiURI) | ||
| } | ||
| return public_client[product_id] | ||
| } | ||
|
|
||
| function websocketClient (product_id) { |
There was a problem hiding this comment.
The gdax websocket api reportedly barfs if too many connections are opened at once; instead they recommend opening one connection and subscribing to multiple channels. Unfortunately, the node api doesn't support subscribing after connection yet.
I propose submitting a pull request to the gdax node project adding a feature that allows subscription after connection (assuming the api server allows such behavior) and subscribing to all products at once in the first connection in the interim. I'm interested in this feature and may begin working on that pull request if I can find the time.
There was a problem hiding this comment.
Just to follow up, I added this pull request at coinbase/coinbase-pro-node#179 such that some day it may be used.
There was a problem hiding this comment.
Subscribing to all the products at once in the websocket is going to be a firehose worth of information coming down, with the vast majority of it being irrelevant to the use case at hand. The OrderbookSync will be doing a lot of work to maintain all the orderbook status' for each product when it doesn't need to, and the trade cache will need to filter out any matches that aren't what we're really looking for or else it'll continually push irrelevant trades into that array. My presumption is that you're running multiple instances of zenbot against different products. In that case, even if you do subscribe to multiple products, a single instance of zenbot can only trade against a single product and they can't share information across instances so I'm not sure how it'd help that use case. The rate limit for websocket connections is 1/IP address/4 seconds. If you wait 4 seconds between starting zenbot instances, I think you'll be ok.
There was a problem hiding this comment.
You've got a point. My hope is actually different -- I want to use zenbot to archive the orderbook data, so I plan to connect to multiple exchanges at once. But others shouldn't have to bear the processing for that. Maybe if I'm lucky my patch will be merged.
I sent you a pull request so auth works at krystophv#1 . It works with my credentials now.
package.json
Outdated
| @@ -30,7 +30,7 @@ | |||
| "ccxt": "^1.10.171", | |||
| "express": "^4.16.2", | |||
| "forex.analytics": "mkmarek/forex.analytics#7bc278987700d4204e959af17de61495941d1a14", | |||
| "gdax": "^0.4.2", | |||
| "gdax": "coinbase/gdax-node", | |||
There was a problem hiding this comment.
It might be a little more resilient to say coinbase/gdax-node#semver:^0.5.0
There was a problem hiding this comment.
Not a bad suggestion at all, yarn chokes on that though:
error Invalid hosted git fragment "^0.5.0".
krystophv
force-pushed
the
gdax_websocket
branch
from
ff19d90
to
06f9a05
Compare
krystophv
force-pushed
the
gdax_websocket
branch
from
06f9a05
to
57b6f0f
Compare
|
I got an issue testing this change. I could reproduce it like this:
It will fail with message that not all keys are filled in. Nothing wrong with the the conf-file it works in live trading. |
|
@Fredrik81, I thought that issue was fixed in 22c569e 17 hours ago. When did you perform this test? Backfill works fine for me. I haven't verified the data for correctness, though. |
|
I was reading the gdax docs and found https://docs.gdax.com/#sequence-numbers stating that some messages will be dropped or sent out of order. They say at https://docs.gdax.com/#the-code-classprettyprintmatchescode-channel that the heartbeat channel should be used to identify and resync when information is lost, using either sequence numbers or trade ids. I don't know if this is a huge issue if a trade is dropped here and there, but it's perhaps the next thing missing. |
|
With same steps of Fredrik81 i get: (node:4844) DeprecationWarning: while backfilling same for --manual on live trade tested now, with corrected exchange.json of 22c569e |
|
Hello, zenbot$ ./zenbot.sh backfill gdax.ETH-BTC --days=2 gdax.ETH-BTC saved 100 trades 2 days left |
|
Anyway i don't know if these tests should be done after the successful merge of coinbase/coinbase-pro-node#179 |
|
@luponata @Fredrik81 I don't get those warnings. This PR updates the version of gdax-node depended on in package.json -- have you run npm install to download that updated version? |
|
yes i did :/ |
|
Looks like they changed the API last night with coinbase/coinbase-pro-node#178 |
|
Pinned the dependency to the old API for now with krystophv#2 |
|
|
||
| function publicClient (product_id) { | ||
| if (!public_client[product_id]) { | ||
| websocketClient(product_id) | ||
| public_client[product_id] = new Gdax.PublicClient(product_id, c.gdax.apiURI) |
There was a problem hiding this comment.
To use new API we can change: Gdax.PublicClient(product_id, c.gdax.apiURI) to Gdax.PublicClient(c.gdax.apiURI)
There was a problem hiding this comment.
I think I'll probably just pin the dependency for now (via PR from @fuzzyTew above). The idea of going to the new API does appeal to me, but as we just experienced, it's much better to have a consistent working install than to have the dependency update underneath us and potentially break things for people who might not have the expertise to diagnose such issues.
There was a problem hiding this comment.
yeah it's a good change to set specific API version so stuff doesn't break uncontrolled.
| cache.trades = cache.trades.slice(fromIndex) | ||
| cache.trade_ids = cache.trade_ids.slice(fromIndex) | ||
| return | ||
| } | ||
| client.getProductTrades(args, function (err, resp, body) { |
There was a problem hiding this comment.
Need to append product_id here change: client.getProductTrades(args, function (err, resp, body) to client.getProductTrades(opts.product_id, args, function (err, resp, body)
There was a problem hiding this comment.
@krystophv is this something you're still looking at?
There was a problem hiding this comment.
@DeviaVir it's not an issue any longer since we pinned the dependency instead
Pin gdax-node dependency to version 0.5.0
|
Thanks everyone for the interest and testing. I merged @fuzzyTew's PR pinning the dependency to a tag so the dependency doesn't update with breaking changes unexpectedly in the future. I guess we'll just need to check up on the dependency periodically to see if it's got a new tag (and at that point update it). Hopefully they just start using NPM properly, but that might be a bit much to hope for :/ |
|
I've tried to update the code for the development changes in gdax-node at https://github.com/fuzzyTew/zenbot/tree/gdax_websocket_subscribe mostly so I can use the subscribe/unsubscribe change, but it also has changes equivalent to @Fredrik81's now. I haven't really tested it. |
|
It updates/download data super quick. this will be a big improvement when live. |
|
Once this lands, I'll have another PR up shortly that will build out the local orders cache based on the websocket feed so we can avoid making getOrder calls too. |
|
I can confirm that with the last API version change the problem i faced is gone. |
|
solved |
|
@luponata that's clearly a rate limit exceeded error. and not entirely relevant to this PR discussion. if you feel it's an issue with fuzzy's changes, then maybe making an issue on their branch would be appropriate, or perhaps asking around in the Discord. I'd recommend turning down the poll rate. |
|
my fault, was using a copy of conf-sample.js from another repository with too low ms on poll-trades.. |
This PR makes a first step into reading data from the websocket stream for GDAX instead of continually polling their REST APIs. It doesn't feed the websocket reports directly into the system, but rather sets up a couple of objects/arrays that will act as a buffer or cache for the incoming websocket data and then when the
exchangelevel API functions run, they're set to check the websocket cache (or websocket fed live orderbook) before making a REST call. This should initially alleviate REST calls fromgetTradesandgetQuote. At a minimum, should also be able to write a little more processing code and set up a similar cache forgetOrderin the near future.