Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bitfinex: Refactor wsUpdate handling #1317

Merged
merged 31 commits into from
Sep 1, 2023

Conversation

gbjk
Copy link
Collaborator

@gbjk gbjk commented Aug 10, 2023

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 x in [] as item is complete.

  • OCD appeasement

How has this been tested

  • go test ./... -race
  • golangci-lint run

gbjk added 8 commits August 1, 2023 11:29
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 shazbert requested a review from a team August 10, 2023 23:06
@shazbert shazbert added the review me This pull request is ready for review label Aug 10, 2023
Copy link
Collaborator

@shazbert shazbert left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for opening this PR. Just some things I noticed over first pass. 🚀

exchanges/bitfinex/bitfinex_test.go Outdated Show resolved Hide resolved
exchanges/bitfinex/bitfinex_websocket.go Show resolved Hide resolved
exchanges/bitfinex/bitfinex_websocket.go Outdated Show resolved Hide resolved
exchanges/bitfinex/bitfinex_websocket.go Outdated Show resolved Hide resolved
exchanges/bitfinex/bitfinex_websocket.go Show resolved Hide resolved
exchanges/bitfinex/bitfinex_websocket.go Show resolved Hide resolved
exchanges/bitfinex/bitfinex_websocket.go Show resolved Hide resolved
exchanges/bitfinex/bitfinex_websocket.go Outdated Show resolved Hide resolved
exchanges/bitfinex/bitfinex_websocket.go Outdated Show resolved Hide resolved
@gbjk gbjk force-pushed the bugfix/bfx_ws_refactor branch from 3e8f84d to f002f56 Compare August 16, 2023 08:41
@gbjk
Copy link
Collaborator Author

gbjk commented Aug 16, 2023

@shazbert Done.

Declined the type assertion fix - It's unchanged from original, and another big chore.
Also: Better ways to handle it outlined.

Copy link
Collaborator

@gloriousCode gloriousCode left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is lovely. Thank you for putting some extra attention to Bitfinex. My actual change requests are really quite tiny

exchanges/bitfinex/bitfinex_websocket.go Outdated Show resolved Hide resolved
exchanges/bitfinex/bitfinex_websocket.go Show resolved Hide resolved
exchanges/bitfinex/bitfinex_websocket.go Show resolved Hide resolved
@gbjk gbjk requested a review from shazbert August 18, 2023 09:10
@gbjk gbjk requested review from gloriousCode and shazbert August 22, 2023 03:21
Copy link
Collaborator

@shazbert shazbert left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM tACK!

Copy link
Collaborator

@gloriousCode gloriousCode left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the lint update and thanks for combining everything into this one PR. tACK

Copy link
Collaborator

@thrasher- thrasher- left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some very nice improvements, thank you sir @gbjk ! Found some issues:

exchanges/bitfinex/bitfinex_websocket.go Outdated Show resolved Hide resolved
gbjk added 4 commits August 28, 2023 15:04
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
Copy link
Collaborator

@thrasher- thrasher- left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

1.patch

@gbjk
Copy link
Collaborator Author

gbjk commented Aug 30, 2023

@thrasher- Thanks for that. Well spotted.
And yeah, I don't want to have to deal with incorrect symbols that "late".
They should be handled as early as possble.
So I'll take the alternative, and also clean it up a little.

gbjk added 2 commits August 30, 2023 15:55
This improves overall handling and errors on a few current assumptions
about key structure
@gbjk
Copy link
Collaborator Author

gbjk commented Aug 30, 2023

@thrasher- Fixed and improved. Thanks for catching that.
I've aimed to be more specific without adding any new codewhiff.

@gbjk gbjk requested a review from thrasher- August 30, 2023 09:00
Copy link
Collaborator

@thrasher- thrasher- left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for making those changes, subs are working great! This is a tACK from me after these two minor changes

exchanges/bitfinex/bitfinex_websocket.go Outdated Show resolved Hide resolved
exchanges/bitfinex/bitfinex_wrapper.go Show resolved Hide resolved
@gbjk
Copy link
Collaborator Author

gbjk commented Sep 1, 2023

@thrasher- Changes applied.

@gbjk gbjk requested a review from thrasher- September 1, 2023 04:57
Copy link
Collaborator

@thrasher- thrasher- left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tACK, nice stuff!

@thrasher- thrasher- merged commit 6ab4c27 into thrasher-corp:master Sep 1, 2023
@gbjk gbjk deleted the bugfix/bfx_ws_refactor branch September 2, 2023 04:58
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
Labels
review me This pull request is ready for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants