bmoscon / cryptofeed

Cryptocurrency Exchange Websocket Data Feed Handler
Other
2.14k stars 668 forks source link

Unnecessary check in order book update handler #966

Closed Naseefabu closed 1 year ago

Naseefabu commented 1 year ago

Hi,

I've been looking through the code for the order book update handler and noticed that there's an unnecessary check in the _check_update_id method. Specifically, in the elif msg['U'] <= self.last_update_id[std_pair] and msg['u'] <= self.last_update_id[std_pair]: block, it's redundant to check both U and u fields against self.last_update_id[std_pair]. Checking only u is sufficient in determining whether to skip the event. Because if msg['u'] is less than self.last_update_id[std_pair] then that already implies msg['U'] also less than self.last_update_id[std_pair].

I believe this can be simplified by removing the U check and modifying the code to look like this:

elif msg['u'] <= self.last_update_id[std_pair]:
    return True # skip the event
bmoscon commented 1 year ago

not going to change the code here. it follows what the binance docs say and its been changed by people in the past and lead to inconsistencies and broken behavior in corner cases.

Naseefabu commented 1 year ago

Since msg['u'] will always be greater than or equal to msg['U'], the statement self.last_update_id[std_pair] + 1 == msg['U'] will never be true if elif msg['U'] <= self.last_update_id[std_pair] + 1 <= msg['u'] is not true. So, if the first elif statement is not hit, the second elif statement will also not be hit. Therefore, the second elif statement is unnecessary and can be removed.