Add insert and delete APIs to orderbook WS #2418
Conversation
luckysori
force-pushed
the
feat/orderbook-ws-api
branch
from
d31ad05 to
5ce995d
Compare
luckysori
force-pushed
the
feat/orderbook-ws-api
branch
from
5ce995d to
7c6f8ee
Compare
| }) | ||
| .await??; | ||
|
|
||
| let _ = state.tx_orderbook_feed.send(Message::DeleteOrder(order_id)); |
There was a problem hiding this comment.
order_id is the trader_order_id right? If so, the traders receiving the DeleteOrder msg might not know what todo with this.
The user connected to the websocket shouldn't know about the trader_order_id, so did this ever work? 🙈
There was a problem hiding this comment.
Maybe I made a mistake. I'll test it again locally.
There was a problem hiding this comment.
No, you didn't it's been like that before. Maybe this was a reason why we saw outdated orders on the client side...
There was a problem hiding this comment.
Hmm, I've been thinking about it and I don't see the problem with those warnings we get on the app.
As far as I can tell, what happens is:
- The maker generates an order.
- The maker deletes the order.
- The app starts and connects to the coordinator's WS, getting an
AllOrdersmessage that does not include the deleted order. - The WS still sends the app the corresponding
DeleteOrdermessage from step 2.
I don't understand why the "outdated" message is delivered to the app, but I think we should demote the WARN to DEBUG because I think this is harmless.
There was a problem hiding this comment.
I looked into it and we are actually sending the trader_order_id to all users 🙈 It's probably not an issue, but I think it wasn't meant to be used like that:
Here are we mapping the trader_order_id from the db to order.id for the orders we are returning to the users.
10101/coordinator/src/orderbook/db/orders.rs
Line 114 in 7c6f8ee
There was a problem hiding this comment.
I don't understand why the "outdated" message is delivered to the app, but I think we should demote the WARN to DEBUG because I think this is harmless.
Imho this happens because we are here also clinsing outdated orders here and we might have a race condition where the maker deletes an order and this is run
There was a problem hiding this comment.
I looked into it and we are actually sending the
trader_order_idto all users 🙈 It's probably not an issue, but I think it wasn't meant to be used like that:Here are we mapping the
trader_order_idfrom the db toorder.idfor the orders we are returning to the users.10101/coordinator/src/orderbook/db/orders.rs
Line 114 in 7c6f8ee
Yeah, that makes sense.
Did we ever have an order ID that wasn't the trader order ID though? I can't find another one.
There was a problem hiding this comment.
Yes, on DB level we have a different one.
There was a problem hiding this comment.
Okay, but that one has only ever been used internally:
// this id is only internally but needs to be here or diesel complains
#[allow(dead_code)]
pub id: i32,
Add insert and delete APIs to orderbook WS
Related to https://github.com/get10101/aristides/issues/39.