altangent / ccxws

WebSocket client for 38 cryptocurrency exchanges
MIT License
621 stars 186 forks source link

Refactor to make functionality changes easier #149

Open bmancini55 opened 4 years ago

bmancini55 commented 4 years ago

It is a non-trivial task to understand what needs to be done to implement a new exchange or to modify an existing exchange for additional functionality.

To name a few of the variables that add complexity:

A number of exchanges, such as Coinbase and Bitfinex are fairly simple. Other exchanges such as Binance, Gemini, and Poloniex are completely one off. Others such as Bibox are a s***show because they only allow 20 markets per socket.

Some work has already been done to make things more uniform. The creation of a uniform testing system in #86 helped ensure there is consistency across clients with similar functionality.

The first step of this issue is going to be to document the peculiarities and commonalities of each exchange supported by this library. This will help determine what needs to be done to remove one-off solutions in each library and provide some consistency (if possible).

evan-coygo commented 4 years ago

What are you thinking for a migration approach? Refactoring all exchanges at once could require a ton of work so perhaps it might be appropriate to create a new v2 branch that doesn't guarantee to support every exchange out of the gate, otherwise you could migrate one exchange at a time but this might make the codebase even more confusing in the interim as you'd have some exchanges on the old paradigm and some on the new paradigm.

I personally am more invested in #148 than in refactoring the entire codebase, so anything that could allow me to contribute to private feeds for the few exchanges I'm using (6 atm) ASAP would be my vote.

bmancini55 commented 4 years ago

I think it would be a piecemeal approach where exchanges are migrated to the new pattern. The challenge with that is that you can end up with another emergent design. That's why I think a good starting point is collecting information on exchange capabilities. That part is also required for #148, so there is some overlap in that information collection.

I'm not sure a refactor would result in a cleaner code base, but it's worth documenting the functionality and brainstorming on designs.

fbadiola commented 4 years ago

Hello,

I'm working in a solution that creates subclients transparently for exchanges that are limited by subscriptions per socket. This aims to solve Kucoin and Bibox subscription limit problem.

I would to know if anyone is working on this. Anyone can tell me?

bmancini55 commented 4 years ago

Hi @fbadiola, thanks for asking. I've been updating notes in the wiki as I'm fixing issues (https://github.com/altangent/ccxws/wiki/Refactor-Research). It's a bit of a mess that needs to be organized and I need to do a brain-dump of my plans.

The biggest bang for the buck refactor is going to around obtaining a socket for a subscription. Right now that's a mixed bag on how it works across various exchanges. I'm thinking this process should be a black-box async process specific to an exchange's requirements. Abstracting this part of the process away allows for single sockets, multi sockets, authorization, REST token initialization, limits per socket, sockets per market, etc.

Once a socket is obtained it is paired with the subscription command (type, market, arguments) for the duration of the subscription lifetime. This can be used to handle socket events by treating the subscription command and socket as first class arguments in the various handlers.

That's the theory at least. At this point most exchanges are using BasicClient or MultiClient. The major work will be creating a new BasicClient type that supports this functionality and migrating existing clients to it. This should ultimately make the library much easier to extend.

fbadiola commented 4 years ago

Yes, I'm agree but I'm thinking in other approach that maybe allows iterate this refactor without impact in other clients at moment. I would like you tell me ur opinion about it:

My idea is create a Clients that enhances other clients. e.g.

LoadBalanceClient.create((clientOptions) => new KuCoinClient(clientOptions), { maxSubscriptions: 100 })

LoadBalanceClient accepts different strategies to balance it (FillHoles, RoundRobin...). It's a client wrapper that fulfills the interface of any other client but limits the maximum subscriptions per client.

In the same way, we could made other implementation like e.g. RateLimitSubscriptionClient that fills with differents strategies like LeakyBucket, TokenBucket...

All of them we can be composed:

const KucoinBalancedClient = LoadBalanceClient.create((clientOptions) => new KuCoinClient(clientOptions), { maxSubscriptions: 100 })

const KucoinBalancedAndRateLimitedClient = RateLimitClient.create((clientOptions) => new KucoinBalancedClient(clientOptions), { strategy: (params) => new LeakyBucketStrategy(params) });

IMHO, this creates a composable architecture to enhance clients without affect their implementation and allow reuse more code.

How about?

bmancini55 commented 4 years ago

Cool! Thanks so much for sharing your ideas! I certainly agree that we need to move complexity towards composition, there are a couple of ways we should be able to do that, so it's awesome to discuss and explore.

If you haven't already, take a look at BasicMultiClient. It is essentially a decorator over BasicClient that adds functionality to split subscriptions into different sockets. It was an early attempt to solve this problem, but I don't believe using a decorator is the right tool for the client level. IMO the number of variables affecting various sub-systems of the client have the potential to create a large amount of combinatorial complexity and potential for cross-talk.

In patterns terms, what I am proposing is treating the new base client as a facade. That is we use a simple interface that abstracts complex sub-systems.

One of those subsystems would be obtaining a socket. The rest of the code inside the facade doesn't care about the rules used to obtain a socket, only that when it's needed a socket can be retrieved.

IMO what you're proposing is perfect for composing functionality of the subsystems themselves. For example Kucoin uses a multisocket strategy with a limiter.

Sorry I can't be more concrete with code. I'm typing from my phone due to an unfortunate water on laptop issue this morning haha.

fbadiola commented 4 years ago

Yep, I think we're absolutely align. My decorators proposal is on top of clients (subsystems) and it doesn't affect other under the hood changes.

Obviously, BasicClient responsability should be splitted into smaller parts. Facade is a good approach for this. If u wish, i can help u with these tasks.

BTW, my objetive only affect to enhance clients without update them. If u want, I send a PR as soon as I will have got decorators approach that doesnt affect actual codebase and we can iterate about this when BasicClient refactor done.

Sorry I can't be more concrete with code. I'm typing from my phone due to an unfortunate water on laptop issue this morning haha.

No problem! Ouch!!! Water and laptops are no good idea... hahaha

P.S:

I certainly agree that we need to move complexity towards composition, there are a couple of ways we should be able to do that, so it's awesome to discuss and explore.

Yeah, there're a lot of ways. I like discuss about architecture :rofl:

bmancini55 commented 4 years ago

Thanks for the PR, will take a look once I'm I'm back up and running.