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: Improve websocket type assertion checks for order processing #1292

Merged
merged 2 commits into from
Sep 7, 2023

Conversation

gbjk
Copy link
Collaborator

@gbjk gbjk commented Aug 1, 2023

Fixes #1288

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

@shazbert shazbert requested a review from a team August 4, 2023 01:28
@shazbert shazbert added the review me This pull request is ready for review label Aug 4, 2023
@gbjk gbjk mentioned this pull request Aug 10, 2023
3 tasks
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.

Well done this works. Just one nit.

exchanges/bitfinex/bitfinex_websocket.go Outdated Show resolved Hide resolved
@gbjk gbjk requested a review from shazbert August 11, 2023 05:55
@gbjk gbjk force-pushed the bugfix/bfx_ws_cancels branch from 5c1398e to a03e482 Compare September 2, 2023 05:17
@gbjk
Copy link
Collaborator Author

gbjk commented Sep 2, 2023

Strange as this might seem, this branch had 1 nit from @shazbert which didn't make it into the main refactor.
Whilst there are other assertions that could/should be caught, I don't want to lose this one.

Rebased on master already

@thrasher- thrasher- changed the title Bitfinex: Fix cancel/update order WS ack not seen Bitfinex: Improve websocket type assertion checks for order processing Sep 7, 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.

changes ACK!

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.

Upon second thought, would be better to annotate what stream event triggered the type assert failure so it's more helpful for the user

@@ -916,7 +916,9 @@ func (b *Bitfinex) handleWSNotification(d []interface{}, respRaw []byte) error {
}
case strings.Contains(channelName, wsOrderNewRequest):
if data[2] != nil {
if cid, ok := data[2].(float64); ok && cid > 0 {
if cid, ok := data[2].(float64); !ok {
return common.GetTypeAssertError("float64", data[2])
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
return common.GetTypeAssertError("float64", data[2])
return common.GetTypeAssertError("float64", data[2], "wsOrderNewRequest cid")

exchanges/bitfinex/bitfinex_websocket.go Outdated Show resolved Hide resolved
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!

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!

// my forced error
type assert failure from string to float64 for: on-req cid

@thrasher- thrasher- merged commit 51f7330 into thrasher-corp:master Sep 7, 2023
shazbert pushed a commit to shazbert/gocryptotrader that referenced this pull request Nov 10, 2023
thrasher-corp#1292)

* Bitfinex: Error if ws data id/cid is not a float64

* Bitfinex: Add annotation to WS id/cid assest errs
@gbjk gbjk deleted the bugfix/bfx_ws_cancels branch November 28, 2023 11:32
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.

Bitfinex: WS cancel stuck waiting for confirmation
4 participants