fabianboesiger / ftx

Unofficial Rust API bindings for the FTX exchange.
116 stars 58 forks source link

Question on the comments "Confirmation should arrive within the next 100 updates" #103

Open xiaoyual666 opened 2 years ago

xiaoyual666 commented 2 years ago

https://github.com/fabianboesiger/ftx/blob/b7dbc9e10300bfd0eceb48f94ea1c32a0df2e26d/src/ws/mod.rs#L166

Hi,

This is not an issue, it's just a question. I'm reading through the code and found this comment. I'm wondering if you could provide more information on it? Is this something mentioned from FTX document?

Thanks!

MaxFangX commented 2 years ago

Hi @xiaoyual666, I wrote that comment - happy to explain.

There are several Types of websockets messages that the exchange will return to us, defined here:

https://github.com/fabianboesiger/ftx/blob/b7dbc9e10300bfd0eceb48f94ea1c32a0df2e26d/src/ws/model.rs#L39-L47

Whenever we subscribe_or_unsubscribe, we expect to receive a confirmation message from the exchange with Type::Subscribed or Type::Unsubscribed. However, we cannot simply assume that this confirmation will be the next message that comes to us, as there may be other messages and updates currently in transit - especially relevant when we are trying to unsubscribe while we're listening to a Channel with frequent updates such as Orderbook. So what the for _ in 0..100 does is it listens for up to 100 additional websockets messages. If the message is not the subscribe / unsubscribe confirmation, we call handle_response to add the data is to the VecDeque for the user to process later. If it is the confirmation, we stop listening for updates, and continue the subscribe / unsubscribe process for the next channel. If we never get the subscribe/unsubscribe confirmation message after 100 messages, we assume that something must have gone wrong and return an error.

Hope that helps!

fabianboesiger commented 2 years ago

Maybe it would be better to implement a timeout based on the time difference between subscription and confirmation instead of a fixed amount of messages.

MaxFangX commented 2 years ago

Good point - if we e.g. try to subscribe to a new channel on a new Ws object and FTX never receives the subscribe message, we could be waiting forever for websockets messages that will never come.

xiaoyual666 commented 2 years ago

Thanks @MaxFangX for your quick reply and @fabianboesiger for the additional thoughts! These all make much sense to me now.

I don't have much experience of coding in rust, but I have some thoughts just from software design perspective: Currently in the code, there are two places for handling the messages, one is in the subscribe_or_unsubscribe() and another is in the poll_next() of Stream. I feel it may be better to merge the first one to the second one. For example, in the subscribe_or_unsubscribe() method, it does not need to care about the response, and just need to send the message to the FTX server. Then in the next_response(), it could also handle subscription/unsubscription message just as what it did for pong message. In this way, there is no need for a counter or timer.

Please let me know if I missed anything, thanks! Meanwhile I will close this issue.

dovahcrow commented 2 years ago

I second the timeout solution or maybe expose the wait time/# messages to the user. Currently MissingSubscriptionConfirmation is guaranteed to be triggered if you subscribe to too many channels.

What I usually do is let WS methods return a oneshot channel. Then have a background task for dispatching the WS messages to correct oneshot channels.

Of course, the downside is performance: the message will go through the background task first.