Bitfinex handle heartbeat sequenceId #257
Conversation
…enceID validation
|
For reference, this only really shows up on lower activity markets since heartbeats don't get sent unless a channel doesn't have activity for 15 seconds. I was able to replicate by using the DTX/USD market which has a daily volume of $13 lol. Testing low activity markets on other exchanges may yield similar results if not accounted for. |
src/exchanges/bitfinex-client.js
Outdated
| /** | ||
| * | ||
| * @param {Object} params | ||
| * @param {Boolean} [params.enableEmptyHeartbeatEvents] (optional, default false). if true, emits empty events for all channels on heartbeat events which includes the sequenceId. |
There was a problem hiding this comment.
new constructor param enableEmptyHeartbeatEvents, to enable empty Ticker and Trade events w/the sequenceId when these channels receive a heartbeat
|
@bmancini55 I just made some changes to add a new constructor arg to keep backwards compatibility and allow sequenceId validation optionally, lmk if this seems like the right approach |
src/exchanges/bitfinex-client.js
Outdated
| return; | ||
| } | ||
| if (Array.isArray(msg[1])) { | ||
| // trade snapshot example msg: |
There was a problem hiding this comment.
this wasn't handling the initial trades snapshot previously, so I fixed that as well
|
I forgot to create an issue, sorry about that. I've just made one here #258 |
src/exchanges/bitfinex-client.js
Outdated
| * | ||
| * @param {Object} params | ||
| * @param {Boolean} [params.enableEmptyHeartbeatEvents] (optional, default false). if true, emits empty events for all channels on heartbeat events which includes the sequenceId. | ||
| * @param {String} [params.tradeMessageType] (optional, defaults to "tu"). one of "tu", "te", or "all". determines whether to use trade channel events of type "te" or "tu", or all trade events. see https://blog.bitfinex.com/api/websocket-api-update/. |
There was a problem hiding this comment.
new constructor param tradeMessageType to specify which trade events to handle. it was previously "tu" and that's probably fine for most ppl, but now you can specify "te", "tu", or "all"
There was a problem hiding this comment.
@evan-coygo, thanks a ton for digging into this and figuring out the gaps! Much appreciated!
Just a couple nits and minor refactoring recommendations. This change adds a ton of cyclomatic complexity to the individual methods. I'd rather see that complexity in the _onMessage method which already acts as a message router. The individual handler methods should be really simple. So most of the recommendations are around extracting heartbeat message building into new methods. With these new methods you should be able to route heartbeats to the appropriate heartbeat method inside _onMessage. Happy to explain further if that's not clear.
|
hey @bmancini55 I've been using my fork of this for awhile but I'd like to finally get this into the main repo if possible as anyone who is trying to validate order books while also subcsribing to tickers are going to want this change. I noticed that the codebase has gone through a number of changes and is now all in Typescript so before I dive back into this I just wanted to see if you think the proper approach has changed now that the codebase has changed a bit. Thanks! |
|
@evan-coygo I think we're still good with the approach. I don't believe the underlying code has changed much with the move to TS. I think the hold up last time was just on a final refactor to make the code more readable with the additional heartbeat logic. If you'd like to fix the conflicts and update the PR I'm happy to give it a final review before merge! |
|
@bmancini55 I've pulled in master and redid all of my changes in TypeScript as well as the changes you requested |
__tests__/TestRunner.ts
Outdated
| * @param spec test specification | ||
| * @param [next] optional, callback to execute when tests are complete | ||
| */ | ||
| export function testClient(spec: TestSpec, next?: (...args: any[]) => void) { |
There was a problem hiding this comment.
The problem: In order to test two different variations of constructor arguments to BitfinexClient I wanted to be able to run testClient() once, wait for it to complete, then run testClient() again with other constructor arguments.
The solution: I just wrapped the entire test in one describe() and used after() to call the callback (if provided). No other changes.
(you can add ?w=1 to the URL of this page to ignore whitespace in the diff and see my actual changes)
| /** | ||
| * Handle heartbeat messages on each channel. | ||
| */ | ||
| protected _onHeartbeatMessage(msg: any, channel: any) { |
There was a problem hiding this comment.
this will handle each channel's heartbeat event by calling the corresponding heartbeat handler. If enableEmptyHeartbeatEvents is true each of these will fire an empty event for that channel with the sequenceId included
|
@bmancini55 could I get a review? |
This commit reverts changes to the test suite that introduce a callback since this isn't necessary to achieve the functionality needed. In mocha sibling describe blocks will execute serially, meaning a sibling waits for the completion of the prior describe block. Since the test suite uses a describe block at its root, we can simply run the test suite again using a different variables by adding multiple calls to the test suite inside the same spec file.
This converts "te", "tu", and "all" string values into an Enum to improve semantics of the constructor options and readability of the code.
This adds a change to process unsubscribe events and clean up the subscription metadata. This allows us to remove the override of _subscribe which breaks idempotency of the subscribe method.
bmancini55
force-pushed
the
fix/bitfinex-heartbeat-sequenceId
branch
from
2325032
to
0f85fab
Compare
|
@evan-coygo thanks for making the TS conversion and refactoring things! I pushed a few commits so we can get this across the line. f51756d - reverts the changes to the test suite. Spec files run in parallel via CI. Sibling scoped e523355 - converts the trade event types to an Enum and just cleans up the impacted code. 0f85fab - handles unsubscribe events so that we don't need to override Let me know if you have any questions / concerns and if not, I'll get this merged and released today. |
|
Thanks! |
The hunt for 100% valid order books continues apparently. for #258
This is a similar update to the one I did for Gemini in #244. Sequence ID is shared across all channels on Bitfinex and if you're subscribed to l2 or l3 book you need to receive the sequenceId of heartbeat events, even if it's a heartbeat for the ticker channel.
For backwards compatibility I've just emitted an empty l2 or l3 update w/the sequenceId included from the heartbeat.New constructor params
I've addded two new constructor params to allow new behavior that allows sequenceId validation while maintaining backwards compatibility.
@param {Boolean} [params.enableEmptyHeartbeatEvents]* (optional, default false). if true, emits empty events for all channels on heartbeat events which includes the sequenceId.
@param {String} [params.tradeMessageType]* (optional, defaults to "tu"). whether to use trade channel events of type "te" or "tu" or "all". see https://blog.bitfinex.com/api/websocket-api-update/
Explanation of changes
When the hearbeat is received on a
"l2update"or"l3update"I just emit an emptyLevel2UpdateorLevel3Updateevent w/thesequenceId. This should be backwards compatible so no harm in emitting it.For when the hearbeat is received on a
"trade"or"ticker", I've added a new boolean constructor paramenableEmptyHeartbeatEvents. An emptyTradeorTickerobject w/nothing but asequenceIdfrom a heartbeat seems non-backwards compatible, so withenableEmptyHeartbeatEvents = false(the default) the behavior will be the same as previously and these heartbeats will be ignored. WhenenableEmptyHeartbeatEvents = true, emptyTradeandTickerevents will be emitted w/thesequenceIdof each heartbeat, allowing you to validate all websocket messages.I've also added an option to choose which type of trade update is used, "te" or "tu". The difference is "tu" comes after a short delay and includes the orderId, "te" comes immediately and doesn't have the orderId. From some testing I believe this delay for "tu" might cause an issue w/sequenceId validation so I think "te" is preferred when validating sequenceIds. The default has been "tu" so far, so I've kept that the default and enabled optionally using "te" instead.