Create Common Binance Client Classes

[poshcoe]

Feature Request

While working on #918, I've noticed the large majority of methods implemented for the BinanceFuturesDataClient and BinanceSpotDataClient classes are identical, calling for a common base-class: BinanceCommonDataClient.

The same can be said of BinanceFuturesExecutionClient and BinanceSpotExecutionClient.

The absence of a common base class has already resulted in several "same-but-different" implementations between spot and futures data/execution clients. These differences need to be consolidated.

Creating a common base classes will make it much easier to maintain and improve the Binance clients going forward.

[cjdsellers]

Hi @poshcoe

Thanks for your interest in the project, and it sounds like you have some great contributions in the works.

What you're proposing seems to make a lot of sense, and indeed was the approach that I started with "How different can the API's be between Spot/Margin and Futures?". As the implementations progressed I increasingly realized that the Spot/Margin and Futures Binance exchange APIs actually contain many subtle and not so subtle differences, causing the client classes to be littered with "If futures do this, else if spot do this" branches.

So actually the separation was intentional to clean this up, and to resign to treating each as a distinct integration - as it certainly felt to me like integrating two different exchanges (and Binance also makes this separation in their documentation). I'm sure there is some common logic between the two, but to extract that would result in another layer of inheritance, and require one to traverse between the common Nautilus base classes, the common Binance base classes, and then finally the top level client classes to understand what's happening.

So I felt a clean separation between the two was more understandable, and maintainable - avoiding premature reduction of duplication, where there actually needed to be duplication.

I may be missing something, and I'm open to a proposal of how to consolidate logic, but if that involves extracting common base classes, then you may be surprised at how different the APIs actually are. One clue is the differences in messages, requiring different parsers, which was the reason for the many branches.

I'm interested to hear your further thoughts here, and maybe you've already identified how these common base classes may work?

[cjdsellers]

One idea could be to subclass the parsers and isolate the differences there, merging the two client implementations back into one.

Things may also be easier now that we've managed to push more common logic across all integrations down to the base classes. There's still more to do here with the data client subscription and request methods though.

I'm still concerned about the "same but different" factor too, and the need for many conditional branches.

[poshcoe]

I have made some headway into this, and agree that there are a surprising number of differences between the APIs, and will require introducing few more base-classes than I initially realized.

In my opinion the obscurity trade-off is worth it for the benefit of a more robust implementation.

As groundwork, I have found so far that I could fairly easily consolidate much of the spot/http/market.py and futures/http/market.py into a new base class BinanceMarketHttpApi in binance/http/market.py. I have also consolidated the structs in spot/schemas/market.py & futures/schemas/market.py with common/schemas.py.

Given the relatively low friction of the changes thus far, I'm interested to continue and see how this will pan out when applied to the data clients. Maybe it will blow out, and I'll just consider it a learning exercise :smiley:

[poshcoe]

Thanks for your interest in the project, and it sounds like you have some great contributions in the works.

@cjdsellers, I've quickly become a fan of the project and the direction you're taking it in! Happy to contribute.

[cjdsellers]

Hey @poshcoe

Have you managed to make any progress? just a heads up that there's been plenty of diffs recently on the adapter clients and live client base classes (consolidating some common async boilerplate).

[poshcoe]

Hey @cjdsellers, I'm 80% there and hope to open the PR this week. Thanks for the heads up, I'll make sure to merge again asap. I don't usually like to keep a branch for so long... but there are plenty of distractions this time of year 😄