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.

Sign up for GitHub

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

Jump to bottom

Bitfinex: Resubscribe orderbook after checksum err #1303

Merged
merged 2 commits into from
Sep 14, 2023
Merged

Bitfinex: Resubscribe orderbook after checksum err #1303

merged 2 commits into from
Sep 14, 2023

Conversation

gbjk
Copy link
Collaborator

@gbjk gbjk commented Aug 6, 2023

Note:
This PR builds upon #1302 and contains a single commit from it.
It should not be merged first or in isolation.

It is, however, the driver and context for the atomic fixes in #1302

Type of change

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

How has this been tested

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

@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 17, 2023

Let's get the refactor merged before this one, then I can rebase and get clarity about any issues.
Many of the linter issues are fixed already

@gbjk gbjk force-pushed the bugfix/bfx_ob_reset branch from 68f6236 to 36e29f8 Compare September 6, 2023 12:45
@gbjk
Copy link
Collaborator Author

gbjk commented Sep 6, 2023 •
edited
Loading

This PR is still vaild on top of the rewrite of Bitfinex overall.
I've rebased it already.

There's 2 follow ups that I don't want to roll into this:

  • Switch to bulk updates and try to get to the bottom of why we're even getting so many checksum errs
  • chanForSub is a parochial solution and we need field a better overall solution. On it already 👍

gloriousCode
gloriousCode requested changes Sep 13, 2023
View reviewed changes
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.

Confirming that I am resubscribing now, thanks!

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
@gbjk gbjk requested a review from gloriousCode September 13, 2023 08:32
@gbjk gbjk force-pushed the bugfix/bfx_ob_reset branch from df64a41 to f2bcb78 Compare September 13, 2023 08:37
thrasher-
thrasher- requested changes Sep 14, 2023
View reviewed changes
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.

One minor nit for future asset support

exchanges/bitfinex/bitfinex_websocket.go Outdated Show resolved Hide resolved
exchanges/bitfinex/bitfinex_test.go Outdated Show resolved Hide resolved
@gbjk gbjk requested a review from thrasher- September 14, 2023 02:02
shazbert
shazbert requested changes Sep 14, 2023
View reviewed changes
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.

Looks good, just two minor issues from me. 👍

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_ob_reset branch from 8e92666 to 22ffbf5 Compare September 14, 2023 03:29
@gbjk
Copy link
Collaborator Author

gbjk commented Sep 14, 2023

Rebased to just 2 commits. Been getting sloppy about that recently; sorry.
Second commit is revertible in case thrasher doesn't agree with shazbert 😂

@gbjk gbjk requested a review from shazbert September 14, 2023 03:31
shazbert
shazbert approved these changes Sep 14, 2023
View reviewed changes
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.

tACK! Thanks 🖖

thrasher-
thrasher- approved these changes Sep 14, 2023
View reviewed changes
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.

Changes ACK, thanks!

gloriousCode
gloriousCode approved these changes Sep 14, 2023
View reviewed changes
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.

tACK. Thanks for writing the test!

@thrasher- thrasher- merged commit ade2d9c into thrasher-corp:master Sep 14, 2023
shazbert pushed a commit to shazbert/gocryptotrader that referenced this pull request Nov 10, 2023
@gbjk
Bitfinex: Resubscribe orderbook after checksum err (thrasher-corp#1303)
3622909 
* Bitfinex: Resubscribe orderbook after checksum err

* Bitfinex: chanForSub return only first match
@gbjk gbjk deleted the bugfix/bfx_ob_reset branch November 28, 2023 11:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@shazbert shazbert shazbert approved these changes

@gloriousCode gloriousCode gloriousCode approved these changes

@thrasher- thrasher- thrasher- approved these changes

Assignees

@gbjk gbjk

Labels
review me This pull request is ready for review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@gbjk @thrasher- @gloriousCode @shazbert