-
Notifications
You must be signed in to change notification settings - Fork 807
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
Subscriptions: Respect DisableAssetWebsocketSupport for assets #1693
base: master
Are you sure you want to change the base?
Conversation
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.
Minor thing, tested looks good 👍
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.
Approved
da3ffc9
to
939e766
Compare
Rebased |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1693 +/- ##
==========================================
+ Coverage 36.95% 36.98% +0.03%
==========================================
Files 414 414
Lines 180443 180447 +4
==========================================
+ Hits 66677 66735 +58
+ Misses 105923 105872 -51
+ Partials 7843 7840 -3
|
feed889
to
e58f029
Compare
at := e.GetAssetTypes(true) | ||
at := []asset.Item{} | ||
for _, a := range e.GetAssetTypes(true) { | ||
if e.IsAssetWebsocketSupported(a) { |
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.
With only Index
asset enabled
Only trade sub enabled:
{
"enabled": true,
"channel": "trade",
"asset": "all"
},
[ERROR] | WEBSOCKET | 14/11/2024 11:49:43 | Bitmex websocket: subscription failure, trade all : subscription template did not generate the expected number of asset records: Got 1; Expected 0
Do you classify this as something to address?
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.
Absolutely. That's a bug in the sub template (which I've made a few times now).
I'll fix.
Subscriptions:
Bitmex:
Fixes #1692
Type of change
How has this been tested
TestGenerateSubscriptions
covers the requirement, once index was added to configtest