-
Notifications
You must be signed in to change notification settings - Fork 822
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
Huobi: Add subscription configuration #1604
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1604 +/- ##
==========================================
+ Coverage 37.04% 37.07% +0.02%
==========================================
Files 414 414
Lines 180305 180275 -30
==========================================
+ Hits 66800 66832 +32
+ Misses 105657 105578 -79
- Partials 7848 7865 +17
|
b4f8fa0
to
315c634
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good stuff, minor potential nit
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also doesn't seek to make subs synchronous
Why not? Huobi has id support which would allow for SendMessageReceiveResponse
It's just not the focus of this PR. I have a list of things to fix on Huobi after this is merged, and synchronous That said: I can sniff making it synchronous now it it's a dealbreaker for this to be merged. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested OB levels ✅
One more thing
d789658
to
fedd1ab
Compare
8c132cd
to
bae9e4d
Compare
2a8075d
to
58a8c92
Compare
c0c226c
to
86b8b4c
Compare
This became too much for this PR. It just snowballed endlessly. I'm nearly done, but it should not get stacked on top of this. So let's review this in the context of the title only. |
There was a problem hiding this 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, one petty request
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tACK
2ba4b27
to
9c726f4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All looks good, only minor things with the known caveat of disabled auth subs
Rename ErrPrivateChannelName to ErrUseConstChannelName
8bbe191
to
6d25b82
Compare
There was a problem hiding this 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!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good stuff tACK!
Just adds configurable subs.
This PR does not fix auth WS usage, which appears broken due to using deprecated v1 paths.
Also doesn't seek to make subs synchronous.
Stacked Dependencies
Type of change