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

Huobi: V2 subscription support #1703

Open
wants to merge 16 commits into
base: master
Choose a base branch
from
Open

Huobi: V2 subscription support #1703

wants to merge 16 commits into from

Conversation

gbjk
Copy link
Collaborator

@gbjk gbjk commented Nov 6, 2024 •
edited
Loading

  • Removes deprecated and non-functional WS endpoints
  • Adds V2 endpoints available as described here ( Account updates, Order updates, Trades )
  • Mock tests for Private subscriptions
  • Improves code structure for wsHandleData
  • Assertifies tests
  • Quite a few test fixes as well
  • Adds testexch.FixtureToDataHandlerWithErrors

Stacked Dependencies

Related

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

@gbjk gbjk self-assigned this Nov 6, 2024
@thrasher- thrasher- requested review from shazbert, gloriousCode and thrasher- and removed request for shazbert and gloriousCode November 6, 2024 22:29
@thrasher- thrasher- added review me This pull request is ready for review nomerge requires dependency This pull request is dependent on another, so it can't be merged until the dependent one is merged labels Nov 6, 2024
@gbjk gbjk added reconstructing Based on PR feedback, this is currently being reworked and is not to be merged review me This pull request is ready for review and removed review me This pull request is ready for review reconstructing Based on PR feedback, this is currently being reworked and is not to be merged labels Nov 7, 2024
@gbjk gbjk force-pushed the feature/huobi_v2 branch 3 times, most recently from 06c750a to 8d0146d Compare November 11, 2024 03:44
@gbjk gbjk removed the nomerge requires dependency This pull request is dependent on another, so it can't be merged until the dependent one is merged label Nov 11, 2024
@gbjk gbjk force-pushed the feature/huobi_v2 branch from 8d0146d to 2f10c4b Compare December 2, 2024 00:41
@gbjk
Copy link
Collaborator Author

gbjk commented Dec 2, 2024

Linter fixed; Rebased to resolve conflict

@gbjk gbjk force-pushed the feature/huobi_v2 branch from 2f10c4b to b88173c Compare December 2, 2024 01:14
shazbert
shazbert requested changes Dec 12, 2024
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.

Awesome update. 🍰 Just some minor nits.

exchanges/huobi/huobi_websocket.go Show resolved Hide resolved
exchanges/huobi/huobi_test.go Outdated Show resolved Hide resolved
exchanges/huobi/huobi_websocket.go Outdated Show resolved Hide resolved
exchanges/huobi/huobi_websocket.go Outdated Show resolved Hide resolved
exchanges/huobi/huobi_websocket.go Show resolved Hide resolved
exchanges/huobi/huobi_websocket.go Show resolved Hide resolved
exchanges/huobi/huobi_websocket.go Outdated Show resolved Hide resolved
exchanges/huobi/huobi_websocket.go Show resolved Hide resolved
exchanges/huobi/huobi_websocket.go Outdated Show resolved Hide resolved
@gbjk gbjk mentioned this pull request Dec 14, 2024
Open
shazbert
shazbert requested changes Dec 15, 2024
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.

Nice work! Thanks for addressing my initial nits, just one left over issue then looks good to go, good stuff! 🚀

exchanges/huobi/huobi_websocket.go Outdated Show resolved Hide resolved
@gbjk gbjk requested a review from shazbert December 17, 2024 01:53
shazbert
shazbert requested changes Dec 17, 2024
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.

🕊️ Last thing from me, I hope 🙏

exchanges/huobi/huobi_wrapper.go Show resolved Hide resolved
shazbert
shazbert reviewed Dec 17, 2024
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.

unauth tACK unauth utACK. Thanks!

shazbert
shazbert requested changes Dec 17, 2024
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.

Whoops tests broke

@gbjk
Huobi: Fix test failures on futureContracts
52dfbfd 
Per-instance future codes not getting cached causin occassional fails
@gbjk
Copy link
Collaborator Author

gbjk commented Dec 18, 2024

Whoops tests broke

This has been an transient test failure for ages, impervious to introspection.
Think I finally cracked it with:

Fixed gbjk@52dfbfd8f

but running long -count=100 -race again to be (somewhat) sure.

@gbjk
Copy link
Collaborator Author

gbjk commented Dec 18, 2024

CI failures all have 5 or 6 failures for just Coinbase and/or test server.
All clean for Huobi.

Also nothing locally for hundreds of runs.

@gbjk gbjk requested a review from shazbert December 18, 2024 02:55
shazbert
shazbert approved these changes Dec 18, 2024
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.

Changes ACK, Thanks 🫡

@shazbert shazbert added the szrc shazbert review complete label Dec 19, 2024
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

@thrasher- thrasher- Awaiting requested review from thrasher-

@gloriousCode gloriousCode Awaiting requested review from gloriousCode

At least 3 approving reviews are required to merge this pull request.

Assignees

@gbjk gbjk

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

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