Orderbook rework

[mcardillo55]

My attempt at addressing #159. I removed the WebsocketClient inheritance from OrderBook. OrderBook is a good data structure, but should process incoming messages from one single open WebsocketClient.

Note: this will break anyone using OrderBook, but I think it's a needed change. Also, the test code in the file uses the python2.7 Queue module, so I'm not sure if we want to change that out for the python3 queue.Queue.

I'm open to any suggestions for changes, as long as we can still remove the amount of open WebsocketClients needed!

[danpaquin]

Hi MCardillo55, thanks for your support! I really agree with what you're saying here. We should only make one connection for all desired products, but you may confuse people with this implementation inside of OrderBookConsole:

class WebsocketConsole(WebsocketClient):
     def on_open(self):
          self.products = ['BTC-USD', 'ETH-USD']
     self.websocket_queue = Queue.Queue()

def on_message(self, msg):
     self.websocket_queue.put(msg)

order_book_btc = OrderBookConsole(product_id='BTC-USD')
order_book_eth = OrderBookConsole(product_id='ETH-USD')

wsClient = WebsocketConsole()
wsClient.start()

Is there a way we can streamline this so it becomes backwards compatible? Currently, we allow for multiple products to be set at on_open() and I know GDAX allows for multiple products in one feed. I would love to get this change implemented, but I'm sure there's a way to do it without breaking backwards compatibility.

Let me know!

[mcardillo55]

Thanks for the feedback! Glad you agree! I'll try to come up with a solution over the weekend.

[chrischris292]

+1 to this change.

[mcardillo55]

Hey @danpaquin , I removed the references to Queue in the example code to keep it compatible with Python2 & Python3

Currently, we allow for multiple products to be set at on_open() and I know GDAX allows for multiple products in one feed.

I'm not sure if you were looking for something additional with this comment? Or was it just to remove the Queue?

[lukemadera]

@mcardillo55 Thanks for this change; I've been using your fork and waiting for this to get merged in. @danpaquin is there any reason this hasn't been merged yet? The only potential issue I see, that may be what Daniel pointed out and is waiting for, is that this PR seems to hardcode in just 'BTC-USD', 'ETH-USD' as the two available products. And we want it to work with ALL products.

Secondly, I'm trying to run sandbox mode and OrderBook does not seem to allow setting the url like PublicClient and WebsocketClient allow directly, so there doesn't seem to be any way to use the sandbox with the order book?
I think it's a simple fix - the websocket is inherited right? So only the PublicClient needs the url passed in - mcardillo55#1

[mcardillo55]

Hey @lukemadera,
I can merge this in but I mostly wanted to get more feedback first before such a big change. Especially because I think it will break most user's code.

Thank you for the feedback. It's been a while since I've touched the code but I don't think anything is hardcoded in. The idea behind the design is that you will create a new OrderBook(product_id=PRODUCT_ID_HERE) for each desired product, and then in the WebSocket's on_message() pass each message to those OrderBooks via OrderBook.process_message(msg).

I have the hardcoded values in OrderBookConsole just as example code, or are you using OrderBookConsole for a different reason? Are you saying you want me to hardcode ALL products in the OrderBookConsole code?

As for your second point, this PR separates the WebSocket from OrderBook, so there should be no need to set a URL in the OrderBook. You would want to create a sandbox websocket with WebsocketClient(url="wss://ws-feed-public.sandbox.gdax.com") and then process those messages the same way in the OrderBooks.process_message(msg) method.

Finally, looking back on this code months later, I'm wondering a better deisgn would be to just create one OrderBook, have it process every incoming websocket message, and then just have it analyze which products the websocket is subscribed to on its own, and store internal OrderBooks in some sort of dict, which could then be queried. Perhaps something like order_book.get_ask('BTC-USD')... etc.

That might be an even bigger change though. So maybe not appropriate for this PR.

Any thoughts are appreciated!

[lukemadera]

Thanks for the prompt and detailed reply @mcardillo55 ! And thanks for the gdax-trader repo too by the way - I've been using / adapting that and it's been great!

As to this PR, agreed feedback is good; I've been using your fork for about a month now and have had no issues, and I agree it's better to just have 1 web-socket, so I give it the thumbs up. I think it's important to merge PRs as quickly as possible so we don't have people (like me currently) working off branches, waiting for PRs to get into the main. Then the forks all can fall behind and we lose out on core updates. You opened this PR over 6 months ago; I think it's far past time to merge (or close if there's some alternative change instead).

For the hardcoding, I was referring to these lines: https://github.com/danpaquin/gdax-python/pull/166/files#diff-ada42366aa05500bec490b1e493a01a6R279
If those are just examples, I think they should be comments. No reason (just adds confusion) to put live code that's just sample and not a complete list; either add all products or (ideally) list none, then they're passed in as you mentioned.

Finally, I am using the sandbox websocket and that's working fine, but the PublicClient was still using the live API rather than sandbox, so that needs to be changed too. I'm working off my PR branch and it's now working correctly (before it was giving bid and ask prices from live, not sandbox).
mcardillo55#1

[danpaquin]

I've hesitated on this merge because it is a non-backwards compatible update.

Any thoughts from the community on this?

[lirenz]

You can always put this into a sub-directory (and set the current one as deprecated).

I actually favor a directory of only message handlers of different types. Standard order-book (and L2 order book) are probably the most obvious candidates. But you probably want specialized handlers for stuff like ticks and matches (grouping by time to get an accurate volume metric).

The only question is how closely you want to keep true to ONLY what's specified in the gdax API.

[chanhosuh]

@mcardillo55 nice work here :).

This is a rather nice project, but I think the most glaring thing I noticed a couple years ago when going through the code was the coupling of the websocket logic to the order book logic. In fact, I ended up refactoring it to separate them and just had a websocket handler put messages into a queue as Michael originally did in his PR. This way I just needed a process that read from the queue and updated the order book. Then I would grab an order book snapshot and put that into yet another queue.. but anyway, I digress.

From prior work experience, I can say that usually this kind of thing is managed by having very simple single-purpose components that pass messages between each other (and even to themselves) and a lot of the design effort goes into the messaging topology. This has the advantage of being very decoupled, allowing scaling and multiple configurations, as has been mentioned a couple times in this discussion.

For example, putting order messages into a fan-out exchange, as in e.g. RabbitMQ, would allow different order book generators to skip or process them.

The way I manage this currently is through Docker (one to manage RabbitMQ, one for the order feed, and one for the order book generator). I know this raises the requirements for using this project, but there are big advantages gained from this approach.