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: Fix ws Unsubscribe and Resubscribe #1302

Closed
wants to merge 5 commits into from

Conversation

gbjk
Copy link
Collaborator

@gbjk gbjk commented Aug 6, 2023

Unsubscribe needed to use the channel id.
Resubscribe needs to have the original subscription params.

Type of change

Please delete options that are not relevant and add an x in [] as item is complete.

  • Bug fix (non-breaking change which fixes an issue)

How has this been tested

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

Unsubscribe needed to use the channel id.
Resubscribe needs to have the original subscription params.
@gbjk
Copy link
Collaborator Author

gbjk commented Aug 8, 2023

This PR introduces an error output into the tests right now:
Bitfinex Could not find an existing channel subscription: account Pair: ChannelID: 0
I'll look to fix that up shortly.

@thrasher- thrasher- added the review me This pull request is ready for review label Aug 10, 2023
@thrasher- thrasher- requested a review from a team August 10, 2023 00:08
@gbjk
Copy link
Collaborator Author

gbjk commented Aug 10, 2023

Okay, no. There's no reason to make the refactor part of this PR. Backing up.

@gbjk gbjk force-pushed the bugfix/bfx_unsub branch from d6f20e9 to 42b43f5 Compare August 10, 2023 06:58
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.
@gbjk
Copy link
Collaborator Author

gbjk commented Aug 10, 2023

Just fixed the 0 chan sub, and that's it. Will push refactor separately

This was a test artefact only, but better to be clean
@gbjk gbjk mentioned this pull request Aug 10, 2023
3 tasks
gbjk added 2 commits August 10, 2023 14:43
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
@gbjk
Copy link
Collaborator Author

gbjk commented Sep 2, 2023

Merged as part of #1317

@gbjk gbjk closed this Sep 2, 2023
@gbjk gbjk deleted the bugfix/bfx_unsub branch September 2, 2023 04:58
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.

2 participants