search

altangent / ccxws

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

Bitfinex include sequenceId on ticker and trades #243

Closed evan-coygo closed 3 years ago

evan-coygo commented 3 years ago

@bmancini55 during testing I found that Bitfinex's sequenceId is shared across multiple channels (sequence is a beta feature and docs are pretty much nonexistent so that's my mistake on not finding that in my last PR). So if you've subscribed to ticker, trades, and l2update, you need to keep track of the sequenceId for all three in order to know if you've missed any messages.

I wasn't sure if adding as a documented property to Ticker and Trade was right or not so lmk how you want to handle that

evan-coygo commented 3 years ago

So that's an interesting question. Is there any value in in providing sequenceId for tickers or trades? Perhaps on trades to know if you've dropped a message. But I think less so for tickers. In any case, I'm okay leaving them in, but if you do you should add testing in the test runner and Bitfinex spec. I'm also okay just backing out the sequenceId properties. Up to you!

If i've subscribed to l2updates and trades and tickers it looks like I need to keep track of sequenceId across all three of them in order to know if any messages have been missed, so I think it is necessary in this scenario just for Bitfinex. It seems to be something unique just to them, at least from the exchanges I've tried, so I'm leaning on adding them to the payload for Bitfinex but not officially documenting them

evan-coygo commented 3 years ago

woops didn't mean to close the issue. I just updated the tests and README @bmancini55

evan-coygo commented 3 years ago

@bmancini55 any chance you can get this released soon? i'd like to publish a release of our software w/these changes and would prefer not to point to a temp fork if i can

evan-coygo commented 3 years ago

alright i believe all the requested changes were made