fix: keep old orders around on websocket reconnect #756
Conversation
| reset = false, | ||
| ) | ||
| // fills is sometimes null on this message, doesn't mean there are none | ||
| if (fills != null) { |
There was a problem hiding this comment.
I think this is why the fills were disappearing earlier too. This message has null fills even though there are some.
There was a problem hiding this comment.
reset = false is passed here, so it shouldn't overwrite the existing fills when payload is null.
There was a problem hiding this comment.
even when it's true fills is null though
There was a problem hiding this comment.
I mean processFills() handles the case when fills == null already... if fill == null, the existing fills won't be overwritten
There was a problem hiding this comment.
subaccount.fills = fillsProcessor.process(
existing = if (reset) null else subaccount.fills,
payload = payload ?: emptyList(),
subaccountNumber = subaccount.subaccountNumber,
)
if reset is true it's going to nuke it, which we don't want because payload is also null
| @@ -250,7 +253,6 @@ internal open class SubaccountProcessor( | |||
| existing.assetPositions = assetPositionsProcessor.process( | |||
| payload = payload.assetPositions, | |||
| ) | |||
| existing.orders = null | |||
There was a problem hiding this comment.
not sure why we did this, also in the non-static codepath. I get that this message contains orders so we can overwrite it, but then we miss out on filled orders in that table.
There was a problem hiding this comment.
This fix makes sense... Just need to test the scenarios of user logging out or changing the env. I think the entire subaccount state should be nuked in those cases, so that the order list becomes empty.
|
Feels like the upside of merging this isn't really worth the possible downside so close to unlimited cut. |
So we can render old notifications.