-
-
Notifications
You must be signed in to change notification settings - Fork 76
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
Shouldn't category
parameter on subscribeV5
be optional?
#299
Comments
It could...complexity lies in that not only is it conditional (optional for private topics only), but you're able to pass an array of topics, so if even one of those topics happens to be a public one then it is required. Maybe an additional signature on this method with a union type on private topics is enough to detect this at build time? Similar to how |
I thought about multiple signatures too, but I'm afraid that would make even less intuitive on which one to use. I think the main issue here is that private and public topics could be mixed in the request. Personally I would rather have the "responsibility" of passing the correct parameters based on what I want to do. If one requests a specific topic subscription, I would also expect him to be already documented about that topic on the bybit's API docs. Therefore if the docs describes that topic as private, or public I would expect the user to accordingly pass the required parameters. Also, the SDK should (and could) perform some validation before issuing the actual requests. Does it find some topics that requires the category, but the category is not specified? Then it should throw an error that informs about the lack of the required category, for that particular use case. The uses cases I expect are:
One scenario that leaves me wondering which could be the best handling is: What if I pass a mix of topics (private and public), BUT for certain public topics I wan a category, and for other public topics I want another category? The simplest thing that pops in my mind is: make two separate calls to the method. @tiagosiebler Have you thought about this? Do you have any better ideas on how to handle it? |
That's an alternative that comes to mind as well...encourage the user to know which topics are public vs private. Also reduces the risk of any hardcoded topic list in the SDK from becoming outdated, since the user will always ultimately know what you're subscribing to... I think I'm leaning towards that - subscribePublicV5() and subscribePrivateV5() |
We don't have a list of topics in the SDK? For some reason I was sure we already had them, that's why I said "(and could)". Ok, in that case forget that, I agree that a list of that kind could quickly become outdated. The idea of having two subscribe methods one for public and one for private sounds good to me too! For sure it removes any possible ambiguity when reading an implementation afterwards, I always try to write code as speaking as possible, that's why all this dilemma was born 😆 |
When I'll have a moment to dig into it, I could issue a proposal PR and maybe work on it from there, unless you have WIP on it already. |
Since the docs for that parameter states this:
the API category this topic is for (e.g. "linear"). The value is only important when connecting to public topics and will be ignored for private topics.
If the value is important only for public topics, wouldn't it be a bit confusing having an invocation like this?
If I read the above line I would be mislead into thinking that I'm subscribing to order, position and wallet topics FOR the spot category only.
When instead that "spot" category is completely ignored in the case of a private topic.
So, wouldn't it be better having the
category
parameter optional, but required to be specified in case one or more topics are found to be public? Maybe with some internals that are able to detect this and throw an error?The text was updated successfully, but these errors were encountered: