altangent / ccxws

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

Support for klines in all exchanges #75

Closed slidenerd closed 3 years ago

slidenerd commented 5 years ago

First of all thank you for the amazing integration across various exchanges. I looked through the code and see subscribeTicker, subscribeTrade etc but dont find klines anywhere. Do you have any plans of integrating klines from each exchange?

Thannks

bmancini55 commented 5 years ago

Yes, just need to find some bandwidth to work on it.

Always happy to accept a pull requests to add it to any exchange as well.

slidenerd commented 5 years ago

Interesting, in that case, I ll try adding klines to each exchange one by one and then run some tests, will update you if everything goes well/bad :)

bmancini55 commented 5 years ago

I just pushed a branch with a few additions to the base clients to scaffold candle subscriptions: https://github.com/altangent/ccxws/tree/candles

I can merge this into master once a few details have been worked out and it should be pretty straight forward to implement individual exchanges after this code has been merged.

The biggest issue is that there is no pattern for passing data to subscriptions. Candles subs will require the time period as an argument. Ideally the library would be flexible enough to allow multiple subscriptions to the same market with different time periods. A generic example:

let market1 = { id: 'BTCUSDT', base: 'BTC', quote: 'USDT };
let market2 = { id: 'LTCBTC' base: 'LTC', quote: 'BTC' };

client.subscribeCandles(market1, '1m');
client.subscribeCandles(market1, '1d');
client.subscribeCandles(market2, '1d');

The above is a departure from existing patterns and would be a nice addition to the library.

Alternatively, as with orderbooks, candle subscriptions could be managed at a client level setting and the additional data would be automatically sent with the subscriptions. For example:

let client = ccxws.binance();
client.candleInterval = '1m';
client.subscribeCandles(market);

Thoughts?

slidenerd commented 5 years ago

I will have to go through your code in detail and try to understand what you are trying to do. i have an implementation on my end of connecting various exchanges but it is very different conceptually than what you have implemented

  1. The way I do it is at the heart of everything there is a monitor
  2. The monitor simply takes previous symbols and compares them to current symbols for each exchange as per a specified interval
  3. Each exchange extending the monitor uses a restApi and a wsApi
  4. The restApi is responsible for methods that update the state of the monitor such as symbols
  5. The wsApi is responsible for subscribing/unsubscribing to tickers trades candles and orderbooks based on the state of the symbols maintained by the monitor
  6. The monitor fires a change event when the symbols change.
  7. This change event is used to unsubscribe resubscribe etc
  8. The wsApi of each exchange also has access to the restApi component of that exchange through a shared interface
  9. Lol your method is radically different from mine. I will through it in absolute detail.

NEvertheless in 2-3 days I ll begin working on it, got a current project that is 99.5% complete and needs some tests + Ec2 deploy, once done will dive into this

So far I see a bunch of client-spec files and it seems each exchange extends these spec files. Some rough explanation of the inner workings if mentioned in the documentation or anywhere else for the matter would be super appreciated

bmancini55 commented 5 years ago

Ah sounds like an interesting approach. This code is very much emergent as additional exchanges were implemented. The architecture started with a nice idea and definitely has some rough edges now. There is information on the architecture in the contributing guide that may be helpful.

Side note is that I have plans to refactor the integration tests into a single test fixture runner. Once that is done it will ensure all clients have equal and thorough coverage. That should make future refactoring much easier.

bmancini55 commented 5 years ago

The more I think about this, the more it makes sense to use a single configuration setting for the client to control candle resolution.

The most common use case is subscribing to a single candle resolution for a market (such as providing data to a chart). Being able to subscribe to multiple resolutions for a market is a "nice to have" not a hard requirement.

Limiting to a single resolution is also a more universal implementation. It prevents issues with clients such as Poloniex and Gemini that use a single event "room" per market that send updates for all types of data. For these types of clients, subscribes to multiple resolutions of candles at a time would necessitate multiple socket connections. Essentially, we can't guarantee that a single socket allows multiple resolution subscription.

Lastly, this retains the same pattern as orderbooks where resolution data is attached on a per client basis.

The net result is that if a user wanted to subscribe to multiple candle periods at a time the user would be required to create multiple clients.

let client1 = new ccxws.binance();
client1.candleResolution = '1d';
client1.subscribeCandles(market);

let client2 = new ccxws.binance();
client2.candleResolution = '1m';
client2.subscribeCandles(market);

I

lejacobroy commented 4 years ago

Hi, What is the status on this branch? do you have a roadmap for the merge? Thanks!

bmancini55 commented 4 years ago

Finishing up the baseline code and hoping to have it merged this week.

bmancini55 commented 4 years ago

Initial implementation has been merged and includes Binance and Kraken. Still need to add support for Bibox, BitMEX, Cex, Coinex, Gate, HitBTC, Huobi, OKEx, ZB but that's going to be a work in progress.

lejacobroy commented 4 years ago

Great work! I started playing with it and it works as expected.

I would add an optional "quote/symbol" field in the returned data, as it would remove the need to open an "exchange.on()" for every markets (leading to a MaxListenerExceededWarning)

lejacobroy commented 4 years ago

Oh, nevermind, I just saw that it is automatically returned also!

This seems to be a wonderful new feature! Thanks again!

ghost commented 4 years ago

Amazing repo, thank you for your hard work, are you planning to add support for klines on more exchanges, currently I can see it works with 3 of them?

bmancini55 commented 4 years ago

Yep, it's a work in progress. Haven't had much time to work on it in the last month but hoping to get back into it over the next few weeks. I'll continue to update this issue as work is performed.

ghost commented 4 years ago

sorry to ask but i was wondering, its been a while since any changes were made to the kline section, are you working on it secretly or something, past few releases i havent seen anything on klines :) would be great to see something, keep up the awesome work

bmancini55 commented 4 years ago

Not much progress unfortunately. I'm trying to free up a bit of time every week to work on this library. Finishing the klines is the next thing I want to chip away at.