Bitfinex handle heartbeat sequenceId

[evan-coygo]

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.

1. @param {Boolean} [params.enableEmptyHeartbeatEvents]
   • (optional, default false). if true, emits empty events for all channels on heartbeat events which includes the sequenceId.
     2. @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 empty Level2Update or Level3Update event w/the sequenceId. 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 param enableEmptyHeartbeatEvents. An empty Trade or Ticker object w/nothing but a sequenceId from a heartbeat seems non-backwards compatible, so with enableEmptyHeartbeatEvents = false (the default) the behavior will be the same as previously and these heartbeats will be ignored. When enableEmptyHeartbeatEvents = true, empty Trade and Ticker events will be emitted w/the sequenceId of 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.

[evan-coygo]

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.

[evan-coygo]

@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

[evan-coygo]

I forgot to create an issue, sorry about that. I've just made one here #258

[evan-coygo]

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!

[bmancini55]

@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!

[evan-coygo]

@bmancini55 I've pulled in master and redid all of my changes in TypeScript as well as the changes you requested

[evan-coygo]

@bmancini55 could I get a review?

[bmancini55]

@evan-coygo thanks for making the TS conversion and refactoring things! I pushed a few commits so we can get this across the line.

f51756dc448979743baeb7e37b7313fd5e2f5d45 - reverts the changes to the test suite. Spec files run in parallel via CI. Sibling scoped describe blocks run serially. Since the test suite wraps things in a describe block it's safe to call the test suite multiple times in a spec file and it will execute sequentially.

e523355294a39abdea6656ca86d435c313cb8317 - converts the trade event types to an Enum and just cleans up the impacted code.

0f85fabf988b0ed7c35fbbb08a7ab3e07658e1ed - handles unsubscribe events so that we don't need to override _subscribe, which breaks the idempotency of subscribe.

Let me know if you have any questions / concerns and if not, I'll get this merged and released today.

[evan-coygo]

Thanks!