Xud: Add streaming gRPC call to listen to orderbook events

[sangaman]

This issue is about notifying clients when order book events occur such as an ownOrder being filled.

Events:

1. ownOrder added success/error (placeOrder)
2. ownOrder removed success/error (cancelOrder)
3. ownOrder removed (due to internal match)
4. ownOrder removed (due to a successful swap that was instantiated remotely)
5. peerOrder added
6. peerOrder removed (due to non-internal match and successful swap)
7. peerOrder removed (due to peer order invalidation)
8. peerOrder removed (due to swap failure)

placeOrder response covers (1), (3), and (6) (handled by https://github.com/ExchangeUnion/xud/issues/479)
placeOrderSync sync response covers (4), (8) (handled by https://github.com/ExchangeUnion/xud/issues/479)
cancelOrder response covers (2)
subscribeOrders covers (4), (5), (7), and (8)

placeOrder, is asynchronous and just returns an immediate success/error of placing an order. Swap results come in via subscribeOrders. There is a second placeOrderSync call 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.

[moshababo]

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.

[itsbalamurali]

This also requires event emitter implementation on OrderBook and MatchingEngine beforehand and also predefining the events and their types.

[sangaman]

@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 SubscribeOrders call that triggers an event whenever a peerOrder is received. I imagine we'll definitely want something like that and we can always tweak it later if necessary, plus it would be laying the groundwork for this issue. I'd be interested to get started on that soon.

[sangaman]

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 SubscribePeers call which triggers events when a new peer connects, since we already have some events set up on the p2p side of things.

[moshababo]

@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.

[michael1011]

Right now we have:

• SubscribePeerOrders which takes care of new peer orders and cancellations of those.
• SubscribeSwaps which should allow clients to subscribe to executed swaps.

This issue comes down to implementing SubscribeSwaps because SubscribePeerOrders does everything else?

[kilrau]

At the moment 3 modes:

1. Synchronous placeOrder call, non-internal match removes order immediately (problem)
2. SubscribeSwaps - external match remove orders
3. SubscribePeerOrders - remove invalidated peer orders

[kilrau]

@michael1011 @moshababo @sangaman

Proposal: Combine all order updates into subscribeOrders

• invalidates (local) OrderIDs: internal match, external match (after swap succeeded), invalidated peer orders, TBD: which additional info apart from orderID?
• add new peer orders, TBD: which additional info apart from orderID?
• Remove SubscribePeerOrders and SubscribeSwaps rpc calls

Known Problems:

• ownOrder will show up e.g. in UI of exchange for some milliseconds, even though it got matched immediately in xud, because the stream event comes in with a bit of delay. Since this has no direct negative implications, I believe that's acceptable

[kilrau]

Related: https://github.com/ExchangeUnion/xud/issues/298, https://github.com/ExchangeUnion/xud/issues/423

[moshababo]

To enable the client to sync with the order book state, we have 6 types of events:

1. ownOrder added
2. ownOrder removed (due to internal match)
3. ownOrder removed (due to a succesful swap that was instantiated remotely)
4. peerOrder added
5. peerOrder removed (due to non-internal match)
6. peerOrder removed (due to peer order invalidation)

Current API:

• placeOrder response:

  • if remaining order exists, then it was added to the order book (ownOrder added (1)).
  • if internal match occurred, its maker order was removed (ownOrder removed (2)).
  • if non-internal match occurred, its maker order was removed (peerOrder removed (5)), and swap is instantiated. if it fails later, the maker order (peerOrder) won't be re-added, but we planned to retry matching the taker order (ownOrder). If so, and if the taker will not match, then it would be added to the order book, but there's no way to announce ownOrder added (1) to the client. This can be fixed by delegating the responsibility for re-matching to the client, after he heard that the the swap failed.

• subscribePeerOrder stream: provides both peerOrder added (4) & peerOrder removed (6) events.

• subscribeSwaps stream: if swap instantiated locally, it doesn't cover any event that placeOrder response doesn't. If instantiated remotely, and succeed, it provides the event ownOrder removed (3), which there is no other way to know about.

@sangaman @michael1011 @kilrau @offerm please verify that everything is correct.

Possible changes:

• Keep placeOrder response the same.

• Add subscribeOwnOrders to cover order removed (3), in addition to ownOrder added (1) + ownOrder removed (2) in an async manner. It will help putting all these together, and also to solve the problem of announcing ownOrder added (1) in case of no match after a failed swap.

• Change subscribePeerOrder to cover peerOrder removed (5) in an async manner, in addition to the existing peerOrder added (4) + peerOrder removed (6).

• Remove subscribeSwaps (at least for the sake of syncing with the order book state. it might be needed for other things)

It means that ownOrder removed (2) + peerOrder removed (5) could be both sync and async. Same for ownOrder added (1), if we don't try to rematch after a failed swap. if we do, it could be only async.

[kilrau]

@sangaman @michael1011 @kilrau @offerm please verify that everything is correct.

Error event missing on placeOrder. Also ownOrder removed due to cancelOrder. Don't know what's common practice here - include a success/error code or separate event. Rest looks good.

1) ownOrder added success/error (placeOrder)
2) ownOrder removed success/error (cancelOrder)
3) ownOrder removed (due to internal match)
4) ownOrder removed (due to a successful swap that was instantiated remotely)
5) peerOrder added
6) peerOrder removed (due to non-internal match)
7) peerOrder removed (due to peer order invalidation)

Possible changes:

I don't see any advantage in having separate calls subscribePeerOrders and subscribeOwnOrders. I suggest just two:

placeOrder: sync response (1)
cancelOrder: sync response (2)
subscribeOrders: streaming (3), (4), (5), (6), (7)

Next step: non-matching mode: https://github.com/ExchangeUnion/xud/issues/145
Other opinions? @sangaman @michael1011 @offerm @itsbalamurali

[sangaman]

I've been thinking about this as I'm working on swaps clearing orders from the orderbook, so this is a good discussion to have.

There's another use case to cover, which is a peer order being removed because we tried to swap but the swap failed or was otherwise rejected, I'll call this swapFailed. Here they are numbered again for clarity:

1. ownOrder added success/error (placeOrder)
2. ownOrder removed success/error (cancelOrder)
3. ownOrder removed (due to internal match)
4. ownOrder removed (due to a successful swap that was instantiated remotely)
5. peerOrder added
6. peerOrder removed (due to non-internal match and successful swap)
7. peerOrder removed (due to peer order invalidation)
8. peerOrder removed (due to swap failure)

I'm thinking that for placeOrder, we'd want to inform the client synchronously in the response which matches actually happened. If we respond with the matches that we tried to swap, we may mislead the client. It would be confusing, for instance, if the placeOrder response says an order was filled at a certain price whereas in reality it was filled at a different price (or perhaps none at all). For this reason I think we should wait until matching - and any resulting swaps - is complete before responding to placeOrder.

However, if a swap attempt fails, I think we can just notify the client via streaming subscription as they happen and not in the placeOrder response.

I'd agree that we don't need separate calls for subscribing to own orders and peer orders. We probably don't need a separate subscribe swaps call either. Just one endpoint that updates us any time there's an update to an order that isn't a part of a synchronous response.

In summary:

placeOrder response covers (1), (3), and (6)
cancelOrder response covers (2)
subscribeOrders covers (4), (5), (7), and (8)

We already have (1) , (2), and (3) taken care of. I'm working on (4), (6), and (8) and will open the PR for review as soon as it's ready. There'll still need to be more work done on properly notifying the client via subscribeOrders though.

[moshababo]

@kilrau yes, the sync responses will have error codes, and I missed cancelOrder.

@sangaman I've considered non-internal match removal of peerOrder to cover the cases of swap failure/success, since the peerOrder is being removed before the swap, and never go back.
That's obviously changes if you want to change placeOrder to return the swap resolution.

It currently announce the client about matches that are about to trigger swaps. If the swap will fail, then yes, the match is irrelevant. It can be considered to be a misleading information only if we'll automatically try to rematch the taker order, but we can fix that by not doing so or by introducing something like subscribeMatches.

I think it might still be valuable to have an immediate, pre-swap response. Delaying the response until we'll have the swap resolution can technically be synchronous for the client (in the same way that long-polling is), but in practice it's async, as there's no upper-bound for the response time.

If you think no one will care about matches, but rather only about swap results, then we can do so. But if there's multiple matches, we'll need to aggregate all the results, and if one of them will fail, we'll have another saga of matching. We'll basically have to wait until all swap succeed, or there's no more matching.

I don't see any advantage in having separate calls subscribePeerOrders and subscribeOwnOrders. I suggest just two:

I'm usually in favor of separating as much as possible on the API level, and not to relay on response fields for differentiation. But it doesn't really matter, we can combine them.

[sangaman]

I definitely feel it makes sense to have placeOrder return the actual resolution of the matches/swaps. This is also consistent with how making lightning payments via the lnd cli or wallets work, you don't get a response until the payment succeeds or fails even though that occasionally can take a while. (Edit: lnd does have a difference between sendPayment and sendPaymentSync, the latter is synchronous while the former responds right away without any info on whether the payment succeeded, we could take a separate approach for placeOrder and placeOrderSync perhaps). Another consideration is that the value we return for remainingOrder may not be accurate if we don't wait for swap resolution. We should definitely have time limits on swaps so that a placeOrder call doesn't just hang for minutes, though.

If not I actually think it would make more sense to have an empty response and then notify about matches and remaining order via subscription, since I don't believe the client can take any action on the pending results.

I don't have that strong of an opinion for now on how to structure the streaming subscription rpc calls (whether it's one or several). We could see which makes more sense when it comes to implementation based on how the details and calls actually look like, if the data for events has the same or almost the same structure I think it might make sense to combine them. Although the fact that the client will probably handle an update to an own order completely differently from an update to a peer order might justify separate calls.

[kilrau]

Agreement: support synchronous & asynchronous mode. Could you list calls & events each? @michael1011

If one is used, the other one is not active, to avoid double info. Make sure to document properly.

[moshababo]

Another consideration is that the value we return for remainingOrder may not be accurate if we don't wait for swap resolution.

Is it not accurate because that on failed swap we'll try to re-match the taker order, or because of a different reason?

We should definitely have time limits on swaps so that a placeOrder call doesn't just hang for minutes, though.

Are you talking about cutting failed-swap/re-matching iterations, or also actual swap executions?

If not I actually think it would make more sense to have an empty response and then notify about matches and remaining order via subscription, since I don't believe the client can take any action on the pending results.

So you're suggesting that on placeOrder the client won't get any response info, requiring him to subscribe to the streaming calls to get it, while placeOrderSync we'll wrap the streaming info in the response for him, and not stream it? @kilrau

I don't have that strong of an opinion for now on how to structure the streaming subscription rpc calls (whether it's one or several). We could see which makes more sense when it comes to implementation based on how the details and calls actually look like, if the data for events has the same or almost the same structure I think it might make sense to combine them. Although the fact that the client will probably handle an update to an own order completely differently from an update to a peer order might justify separate calls.

It's indeed depends on implementation details, so we can see what's more suitable further ahead.

[sangaman]

Is it not accurate because that on failed swap we'll try to re-match the taker order, or because of a different reason?

Yes exactly.

Are you talking about cutting failed-swap/re-matching iterations, or also actual swap executions?

I was thinking cutting the actual swap executions, but in thinking about this a bit more I believe that can only be done by the maker - since once the taker sends their side of the swap, they can't do anything about the pending HTLC. The maker can always call off the whole thing by failing the attempted payment to them, since only they know the preimage. Might be worth discussing separately.

So you're suggesting that on placeOrder the client won't get any response info, requiring him to subscribe to the streaming calls to get it, while placeOrderSync we'll wrap the streaming info in the response for him, and not stream it? @kilrau

Yeah that's what we were discussing and seems like a good approach to me. placeOrder just gets an empty response or an error if the call was invalid, then the outcome of the order is returned via a subscription. placeOrderSync waits until any and all swaps are resolved and returns a response like we have today, and the swaps pertaining to that order are not streamed via the subscription.

[kilrau]

Are you talking about cutting failed-swap/re-matching iterations, or also actual swap executions?

I was thinking cutting the actual swap executions, but in thinking about this a bit more I believe that can only be done by the maker - since once the taker sends their side of the swap, they can't do anything about the pending HTLC. The maker can always call off the whole thing by failing the attempted payment to them, since only they know the preimage. Might be worth discussing separately.

Yep, let's please discuss this here: https://github.com/ExchangeUnion/xud/issues/413, not part of this issue.

Yeah that's what we were discussing and seems like a good approach to me. placeOrder just gets an empty response or an error if the call was invalid, then the outcome of the order is returned via a subscription. placeOrderSync waits until any and all swaps are resolved and returns a response like we have today, and the swaps pertaining to that order are not streamed via the subscription.

Correct.

[kilrau]

Could you update https://github.com/ExchangeUnion/xud/issues/123#issuecomment-419304684 with placeOrderSync put it as main description of the issue? @sangaman

[moshababo]

@sangaman
Designing the new API should take #145 into consideration. I'd suggest to discuss it first before implementing.

In regards to having a synchronous version for placeOrder, i'd suggest to utilize Peer.wait in order to create a synchronous code flow in Swap.beginSwap (which could be awaited from OrderBook.handleMatch, and so to affect MatchingResults). I think this change would be good for our code base, regardless of placeOrderSync.
The actual LND call is synchronous, so we don't need extra work there.

The other option is to manage a global state for the swap procedures, which I think would be a far worse solution.

I think this should be implemented separately from the high-level API changes. Let me know if you want me to work on it.

[sangaman]

Moving the placeOrder related discussion to #479.

[offerm]

Guys,

Sorry to jump into the discussion. Want to share with you my 2 cents.

I got here as a result of opening #479. As a user, I felt the need for such an option (sync place order). I see that you are discussing this for 3 months now, a long time for something that should be simple.

Going over the comments it is easy to see that while you try to have a clean separation between the system modules the discussion about this feature touches all modules: Order, OrderBook, Swap, P2P, etc. Moreover, the discussion is getting deep into the flows, logic, and dependencies. It also refers to and depends on many other issues. This means that even if you make something working now it will easily break when you make changes or hold you back from making these changes. This is wrong.

Instead of the discussion above I suggest to look at the problem differently:
We have an Order and that order faces Events. An event may be anything in the lifecycle of the order (Create, add, canceled, swapped, split, etc). What we want is a way to subscribe to these Events.
I suggest that an order includes a list of Events and that list can be used to implement the subscribeOrders RPC call. The Event object should be generic and descriptive so it can be used regardless of when/where it was created/added to the Order.

Having a list of Order's events open the door to showing these events also on listOrders and cancelOrder. Also, note that when using sell/buy wait I'm expecting to see all the events related to that order until I stop the command or until the order reaches a final state.

So my suggestion:

1. create an Event structure to describe a generic event (KISS)
2. maintain a list of Events per order. create an interface to add Event.
3. make sure we don't delete orders from memory before we can reload that order from storage (since we don't have it now, don't delete any order) [We will not face performance issues due to this in the near future].
4. add a state to the order that tells if the order is Active or not. This state can be used to ignore an order that was canceled, executed, fulfilled, etc.
5. create subscribeOrdersEvents RPC - should now be much easier and not dependent on anything else. have an option to filter the events based on orderId
6. buy/sell with wait should use the subscribeOrdersEvents until the order is at a final state.
7. Partial taking of order and failed swap - should not create a new order. Just update the remaining amount for sell/buy
8. no need for subscribeSwaps

I hope that can help getting our of the dependency loop I see here.

[sangaman]

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.

[offerm]

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.
Makes everything simple and not dependent on other components/logic.

My 2 cents.

[sangaman]

I was thinking about this some more, and I can think of 3 events that a client would be interested in hearing about:

• A new peer order, for which the message will contain the order info.
• An invalidated peer order, for which the message will contain the order identifier (orderId, pairId, quantity invalidated)
• An executed own order, for which the message will contain orderId/pairId/quantity executed and possibly also pub key of the peer we swapped with, r_hash, or whatever else we figure is relevant. This can cover both maker and taker orders.

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.

[offerm]

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.

[moshababo]

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:

• placeOrder response should return the matches, which also affect the order book state. Should we notify them anyway? Same for remainingOrder, which is also part of the response.
• if placeOrder is async, where do we return the results? do we expose them only as order book state events? if placeOrder is sync, and we're aggregating the results, should we drop the async streaming events?

My suggestion:

• We shouldn't mix matching results with the order book state events. When placeOrderSync is called, we'll aggregate the matches/swaps results (including remainingOrder), and stream the order book state events (Added / Removed) to subscribeOrders, in addition. (in contrary to what @kilrau suggested)
• if placeOrder is called (async), we'll stream the exact same results back to it in async manner. This will make it very similar to placeOrderSync, and I think that's how sendPayment is implemented in LND.
• on a successful maker swap (which wasn't triggered by placeOrder, but by a peer), we'll stream only order book state events (Removed).

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 placeOrderSync includes them, are they going to be streamed to subscribeOrders as well or not, and how are we going to implement async placeOrder.

[sangaman]

So for every event that a user may want to hear we create a subscription RPC?

I'd lean towards a separate subscription for each separate message type.

Also, why do you think a client would only be interested in these 3 events?

Those are the only three events (related to orders at least) that come to mind.

We shouldn't mix matching results with the order book state events. When placeOrderSync is called, we'll aggregate the matches/swaps results (including remainingOrder), and stream the order book state events (Added / Removed) to subscribeOrders, in addition. (in contrary to what @kilrau suggested)
if placeOrder is called (async), we'll stream the exact same results back to it in async manner. This will make it very similar to placeOrderSync, and I think that's how sendPayment is implemented in LND.
on a successful maker swap (which wasn't triggered by placeOrder, but by a peer), we'll stream only order book state events (Removed).

Sounds like a good approach!

[kilrau]

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

We shouldn't mix matching results with the order book state events. When placeOrderSync is called, we'll aggregate the matches/swaps results (including remainingOrder), and stream the order book state events (Added / Removed) to subscribeOrders, in addition. (in contrary to what @kilrau suggested)
if placeOrder is called (async), we'll stream the exact same results back to it in async manner. This will make it very similar to placeOrderSync, and I think that's how sendPayment is implemented in LND.
on a successful maker swap (which wasn't triggered by placeOrder, but by a peer), we'll stream only order book state events (Removed).

Ok!

[kilrau]

Discussion regarding placeOrderSync: https://github.com/ExchangeUnion/xud/issues/479

[moshababo]

I think we should do placeOrder / placeOrderSync separately, as part of #479.

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.

[sangaman]

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.

[sangaman]

@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?

[moshababo]

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.