Bitfinex: Refactor wsUpdate handling #1317
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Unsubscribe needed to use the channel id. Resubscribe needs to have the original subscription params.
The ws channel for authenticated Trades sends two types of update: * te, Trade Executed * tu, Trade Execution Update Only the second one contains fee information. [See the docs](https://docs.bitfinex.com/reference/ws-auth-trades) This commit fixes: `exchange Bitfinex websocket error - unable to type assert trade fee` after an executed market trade on the te update
This fixes: `Bitfinex Could not find an existing channel subscription: account Pair: ChannelID: 0` It's not clear from history why we'd want to store a reference to the ubiquitous 0 channel like this, but it's definitely wrong, and anything that attempts to get channel information about 0 chan needs to be fixed anyway.
This commit doesn't break out all the sub-updater, but attempts to do something about the unmanagable size of ws update handling
gbjk
added a commit
to gbjk/gocryptotrader
that referenced
this pull request
Aug 10, 2023
This is a short term fix for handling the auth chan (0 chan) not being in subs. It's refactored much better in thrasher-corp#1317
shazbert
requested changes
Aug 16, 2023
gbjk
force-pushed
the
bugfix/bfx_ws_refactor
branch
from
3e8f84d
to
f002f56
Compare
|
@shazbert Done. Declined the type assertion fix - It's unchanged from original, and another big chore. |
shazbert
requested changes
Aug 22, 2023
gloriousCode
approved these changes
Aug 22, 2023
This simplifies the handling of subscription symbols. Now that we know the channel up front from handling the subscribed response we can limit the parsing forms needed
Margin WS Currently not fully implemented and causes subscription collisions with spot
thrasher-
requested changes
Aug 30, 2023
There was a problem hiding this comment.
I still noticed subscription errors for the added test case in this patch. I also made subscription of pairs > len 6 always contain a delimiter for all subscription types, because this was causing issues for the assetPairFromSymbol func if there was a pair like DOGEUSDT which didn't contain a delimiter (DOGEUSDT and DOGE:USDT are both valid in Bitfinex's subscription case and will return either, depending on the subscribed pair format. NOTE: the changes in assetPairFromSymbol I've made could be done on the key wsUpdate handling side, either way is fine with me.
Patch diff:
|
@thrasher- Thanks for that. Well spotted. |
This improves overall handling and errors on a few current assumptions about key structure
|
@thrasher- Fixed and improved. Thanks for catching that. |
thrasher-
requested changes
Sep 1, 2023
|
@thrasher- Changes applied. |
This was referenced Sep 2, 2023
4 tasks
shazbert
pushed a commit
to shazbert/gocryptotrader
that referenced
this pull request
Nov 10, 2023
…ts (thrasher-corp#1317) * Bitfinex: Fix cancel/update order WS ack not seen Fixes thrasher-corp#1288 * Bitfinex: Fix ws Unsubscribe and Resubscribe Unsubscribe needed to use the channel id. Resubscribe needs to have the original subscription params. * Bitfinex: Fix ws Trades Fees on te The ws channel for authenticated Trades sends two types of update: * te, Trade Executed * tu, Trade Execution Update Only the second one contains fee information. [See the docs](https://docs.bitfinex.com/reference/ws-auth-trades) This commit fixes: `exchange Bitfinex websocket error - unable to type assert trade fee` after an executed market trade on the te update * Bitfinex: Fix error on ws auth ok This fixes: `Bitfinex Could not find an existing channel subscription: account Pair: ChannelID: 0` It's not clear from history why we'd want to store a reference to the ubiquitous 0 channel like this, but it's definitely wrong, and anything that attempts to get channel information about 0 chan needs to be fixed anyway. * Bitfinex: Refactor wsUpdate handling This commit doesn't break out all the sub-updater, but attempts to do something about the unmanagable size of ws update handling * Binfinex: Fix linter issue on chanId casing * Bitfinex: Fix linter outdent complaint * Bitfinex: Fix linter issues on test * Bitfinex: Fix TestWsTradingPairSnapshot chan lookup * Bitfinex: Remove unnecessary WsAddSubs in test * Bitfinex: Fix TestWsSubscribedResponse chan * Bitfinex: Throw a specific error for bad event * Bitfinex: WS Type assertions for positionSnapshots * Bitfinex: tradeUpdate type assertion * Bitfinex: Reinstate default subscriptions * Bitfinex: Assert chan assetType is the same * Bitfinex: Lowercase error string * Bitfinex: Refactor WS eventType/chanId handling * Bitfinex: Fix linter issues * Bitfinex: Fix delimiter for pairs with more than 6 chars * Bitfinex: Fix WS handling of subscribed symbols This simplifies the handling of subscription symbols. Now that we know the channel up front from handling the subscribed response we can limit the parsing forms needed * Bitfinex: Placate the linter * Bitfinex: Disable margin assets for WS Margin WS Currently not fully implemented and causes subscription collisions with spot * Bitfinex: Fix parsing of 4 part funding keys This improves overall handling and errors on a few current assumptions about key structure * Bitfinex: Linter fixes * Bitfinex: Remove key parsing from assetPairFromSymbol * Bitfinex: Use native error wrapping * Bitfinex: Skip disabled assets in default ws subs
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This commit doesn't break out all the sub-updates, but attempts to do something about the unmanagable size of ws update handling
Note: This PR stacks on top of #1292, #1302 #1310
Type of change
Please delete options that are not relevant and add an
xin[]as item is complete.How has this been tested