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

Shouldn't category parameter on subscribeV5 be optional? #299

Open
caiusCitiriga opened this issue Nov 14, 2023 · 5 comments
Open

Shouldn't category parameter on subscribeV5 be optional? #299

caiusCitiriga opened this issue Nov 14, 2023 · 5 comments
Labels
enhancement New feature or request question General question / more info needed

Comments

@caiusCitiriga
Copy link
Contributor

caiusCitiriga commented Nov 14, 2023

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?

this.WSClient.subscribeV5(['order', 'position', 'wallet'], 'spot');

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?

@tiagosiebler
Copy link
Owner

tiagosiebler commented Nov 15, 2023

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 getTickers has multiple signatures in the RestClientV5, in case you're not sure what I mean. Curious if that would work here....

@caiusCitiriga
Copy link
Contributor Author

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:

  • Am I passing ONLY private topics? Don't even care about the category parameter.

  • Am I passing ONLY public topics? I should be aware that I MUST pass the category too.

  • Am I passing a MIX of public and private topics? I should be aware that I MUST pass the category too. In this case the category is ONE for ALL the public topics.

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?

@tiagosiebler
Copy link
Owner

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()

@caiusCitiriga
Copy link
Contributor Author

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 😆

@caiusCitiriga
Copy link
Contributor Author

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.

@tiagosiebler tiagosiebler added enhancement New feature or request question General question / more info needed labels Dec 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request question General question / more info needed
Projects
None yet
Development

No branches or pull requests

2 participants