Add streaming gRPC call to listen to orderbook events #123
Comments
|
We should clearly define the API after finalizing our descussions here. In the meanwhile we should create a gRPC streaming call example. I'll open a separate issue for that. |
|
This also requires event emitter implementation on |
|
@moshababo I'd say we can make #128 be a very basic version of the orderbook subscription that this issue is for. Maybe start with a |
|
Although @itsbalamurali is right that we need to implement event emitters on the orderbook which has not been done yet, and perhaps that should be done separately from the streaming call example. Another possibility is a |
|
@sangaman yes, events are missing on the orderbook, and although we could easily add them, I think that the task should be focused on doing a technical POC with some dummy call, just so there won't be any other concerns. |
|
Right now we have:
This issue comes down to implementing |
|
At the moment 3 modes:
|
|
@michael1011 @moshababo @sangaman Proposal: Combine all order updates into
Known Problems:
|
|
To enable the client to sync with the order book state, we have 6 types of events:
Current API:
@sangaman @michael1011 @kilrau @offerm please verify that everything is correct. Possible changes:
It means that |
Error event missing on
I don't see any advantage in having separate calls
Next step: non-matching mode: #145 |
|
Let's move persisting orders into a separate discussion. I'd be very hesitant to hold on to orders that have been canceled or filled because we keep them in a priority queue, and having expired orders in there would complicate the matching process I believe. I can't think of any reason why we'd want them after they're completely filled or canceled, except possibly for historical purposes, but then we'd just persist to the db. I think we've already agreed on 6, 7, and 8. |
|
The point I'm trying to make is that this should be done with a list of a generice Event object without the discussion about how/when these Events are created. My 2 cents. |
|
I was thinking about this some more, and I can think of 3 events that a client would be interested in hearing about:
Since these seem like 3 distinct messages, I'm now leaning towards separate subscriptions for the separate events like @moshababo suggested earlier. I can probably come up with a PR with some code to discuss/review in the next couple of days. |
|
So for every event that a user may want to hear we create a subscription RPC? Also, why do you think a client would only be interested in these 3 events? Better to develop a generic solution and allow the RPC user to filter event using regexp or similar solution. |
|
In regards to the list of events, and how they are triggered - this indeed can change tomorrow, but I thought it's a good idea to go through everything we have right now. It might affect our design decisions. At the moment we see our order book state in terms of orders being Added and Removed (which includes Created, Cancelled & Split). What's missing is Swapped which we thought to handle separately. Keeping the non-active orders is not necessary for notifying state events. Once the order is Removed/Swapped, the event is notified, and the order instance is gone. If we'll introduce Swapped as @offerm suggested, it will help solve the problem of expressing matches/swap results in terms of orders being added/removed from the order book, which obviously doesn't work well. The other option is to continue to express Swapped as Removed, in addition to providing matches/swaps results which might contain different info. Our current problems with it:
My suggestion:
That being said, the same problems exists also if we'll introduce the Swapped order state, as @offerm suggested. We need to decide if when |
I'd lean towards a separate subscription for each separate message type.
Those are the only three events (related to orders at least) that come to mind.
Sounds like a good approach! |
|
This issue gets a little bit out of bounds. I updated the description with what we agreed before. Could you add a summary of the agreements on events? @offerm @moshababo @sangaman
Ok! |
|
Discussion regarding |
|
I think we should do This issue can be for the order book state events (Added, Removed), which we already specified. We can consider @offerm design suggestion, or keep our current approach. Note on terminology: I tried to use different names for different contexts. I think we should keep Incoming / Invalidation events solely for the p2p context. From XUD to client we should use Added / Removed (past sentence), instead. @michael1011 do you want to take this one? The design need to take #145 into consideration. |
|
I've started some work on this actually, will open a PR soon and we can discuss some more there. I think it will be helpful to have some actual code as a reference. |
|
@moshababo Using "Removed" implies an order is being fully removed, but we have the possibility that the order is merely having its quantity reduced, for which "Invalidated" is slightly more clear. Maybe "Canceled"? It doesn't sound quite as absolute as "Removed" and I think it makes more sense to partly cancel an order than partly remove it. Thoughts? |
This commit replaces the old `subscribePeerOrders` and `subscribeSwaps` rpc calls with three new streaming rpc calls: - subscribeNewOrders: Notifies when new incoming peer orders are added. - subscribeExpiredOrders: Notifies when peer orders become expired due to being canceled or filled remotely. - subscribeExecutedOrders: Notifies when local orders are executed via a successful swap. It also restructures and updates the `Order` grpc message type. Closes #123.
ghost
added
|
I don’t think it’s any less confusing to use “Invalidated” or “Cancelled” instead for this matter, nor that they are less absolute. We settled on solving this by defining the event payload as a tuple, orderId + quantity, so we’re always invalidating / removing a quantity. I think the current terminology works well: Cancel is for API command (the opposite to placeOrder), “Invalidation” is for the p2p layer, and “Removed” is in the context of a local order book. IMO using these different names is appropriate and actually less confusing. But I agree that the orderId + quantity tuple is a bit confusing. We can introduce another event for each context - PARTIAL_INVALIDATION, REMOVED_PARTIALLY \ SPLIT, which will make the existing events merely an alias for a full quantity case. I still prefer the current approach. |
This commit replaces the old `subscribePeerOrders` and `subscribeSwaps` rpc calls with three new streaming rpc calls: - subscribeAddedOrders: Notifies when orders are added to the order book. - subscribeRemovedOrders: Notifies when orders are removed from the order book. - subscribeSwaps: Notifies when swaps that are initiated by a remote user are completed. It also restructures and updates the `Order` grpc message type. Closes #123.
This commit replaces the old `subscribePeerOrders` and `subscribeSwaps` rpc calls with three new streaming rpc calls: - subscribeAddedOrders: Notifies when orders are added to the order book. - subscribeRemovedOrders: Notifies when orders are removed from the order book. - subscribeSwaps: Notifies when swaps that are initiated by a remote user are completed. It also restructures and updates the `Order` grpc message type. Closes #123.
This commit replaces the old `subscribePeerOrders` and `subscribeSwaps` rpc calls with three new streaming rpc calls: - subscribeAddedOrders: Notifies when orders are added to the order book. - subscribeRemovedOrders: Notifies when orders are removed from the order book. - subscribeSwaps: Notifies when swaps that are initiated by a remote user are completed. It also restructures and updates the `Order` grpc message type. Closes #123.
This commit replaces the old streaming rpc subscription calls with three new streaming rpc calls: - subscribeAddedOrders: Notifies when orders are added to the order book. - subscribeRemovedOrders: Notifies when orders are removed from the order book. - subscribeSwaps: Notifies when swaps that are initiated by a remote user are completed. It also restructures and updates the `Order` grpc message type. Closes #123.
This commit replaces the old streaming rpc subscription calls with three new streaming rpc calls: - subscribeAddedOrders: Notifies when orders are added to the order book. - subscribeRemovedOrders: Notifies when orders are removed from the order book. - subscribeSwaps: Notifies when swaps that are initiated by a remote user are completed. It also restructures and updates the `Order` grpc message type. Closes #123.
This commit replaces the old streaming rpc subscription calls with three new streaming rpc calls: - subscribeAddedOrders: Notifies when orders are added to the order book. - subscribeRemovedOrders: Notifies when orders are removed from the order book. - subscribeSwaps: Notifies when swaps that are initiated by a remote user are completed. It also restructures and updates the `Order` grpc message type. Closes #123.
This commit replaces the old streaming rpc subscription calls with three new streaming rpc calls: - subscribeAddedOrders: Notifies when orders are added to the order book. - subscribeRemovedOrders: Notifies when orders are removed from the order book. - subscribeSwaps: Notifies when swaps that are initiated by a remote user are completed. It also restructures and updates the `Order` grpc message type. Closes #123.
This issue is about notifying clients when order book events occur such as an
ownOrderbeing filled.Events:
placeOrderresponse covers(1),(3), and(6)(handled by #479)placeOrderSyncsync response covers(4),(8)(handled by #479)cancelOrderresponse covers(2)subscribeOrderscovers(4),(5),(7), and(8)placeOrder, is asynchronous and just returns an immediate success/error of placing an order. Swap results come in viasubscribeOrders. There is a secondplaceOrderSynccall which returns fail/success of a swap where we inform the client synchronously if the swap actually was successful or failed.Note:
We already have
(1),(2), and(3)taken care of.The text was updated successfully, but these errors were encountered: