Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Handle remove order request when order is on hold for pending swap #552

Closed
sangaman opened this issue Oct 1, 2018 · 4 comments
Closed
Assignees
Labels
has PR issues with an open PR order book swaps
Milestone

Comments

@sangaman
Copy link
Collaborator

sangaman commented Oct 1, 2018

With the possibility that an order is on hold for a pending swap, we need to handle the case when we receive a request to remove such an order from the order book. I'm thinking the best approach would probably be to hold off on removing that order until any holds are removed, but there are a few things to consider.

  • Don't allow any new holds to be placed on an order that we are in the process of removing
  • How should we respond to a removeOrder request if the order gets fully swapped - do we just respond with the "order not found" error as if the order never existed in the first place?
  • Off the top of my head the approach I'd go for is to listen to the swap.failed and swap.paid events and recheck if order.hold === 0 after each one, does that sound reasonable?
@kilrau kilrau self-assigned this Oct 26, 2018
@kilrau kilrau added this to the 1.0.0-alpha.3 milestone Oct 26, 2018
@kilrau
Copy link
Contributor

kilrau commented Oct 26, 2018

I'll take this one, related #587 (comment)

@kilrau kilrau added the question/tbd Further information or discussion needed label Nov 7, 2018
@kilrau kilrau changed the title Handle a remove order request when the order is on hold for a pending swap [Concept] Handle a remove order request when the order is on hold for a pending swap Nov 7, 2018
@kilrau kilrau modified the milestones: 1.0.0-alpha.4, 1.0.0-alpha.5 Nov 16, 2018
@kilrau kilrau changed the title [Concept] Handle a remove order request when the order is on hold for a pending swap [Concept] Handle a remove order request when the order is on hold for a pending swap (matching mode) Nov 18, 2018
@kilrau
Copy link
Contributor

kilrau commented Nov 18, 2018

Only applicable for matching mode.

Your suggestion sounds good. Would definitely go for a different error message, sth along the lines "order currently swapping, order will be automatically removed if swap doesn't succeed".

@kilrau kilrau modified the milestones: 1.0.0-alpha.5, 1.0.0-alpha.6 Nov 18, 2018
@kilrau kilrau removed the question/tbd Further information or discussion needed label Nov 18, 2018
@kilrau kilrau changed the title [Concept] Handle a remove order request when the order is on hold for a pending swap (matching mode) Handle remove order request when order is on hold for pending swap (matching mode) Nov 18, 2018
@kilrau kilrau assigned sangaman and unassigned kilrau Nov 18, 2018
@sangaman
Copy link
Collaborator Author

I don't think this would apply only to matching mode - in non-matching mode you can still submit orders to xud and remove them. There's the same possibility that someone tries to swap your order at the same time as you try to remove it.

And instead of an error message I'm adding a field to the response to indicate whether a portion of the order was on hold. I don't think it's an "error" per se since the order will still be removed eventually, just that some of it was in the middle of being swapped.

@sangaman sangaman changed the title Handle remove order request when order is on hold for pending swap (matching mode) Handle remove order request when order is on hold for pending swap Nov 30, 2018
sangaman added a commit that referenced this issue Nov 30, 2018
This handles the possibility that we attempt to remove an order via the
rpc layer while that order has a hold for a pending swap. Instead of
removing the entire order immediately, this only removes the available
portion of the order and then waits until any swaps finish before
removing the portions that were on hold.

It also adds a field to the `RemoveOrder` rpc response which indicates
whether any portion of the order was on hold. This gives feedback up
front as to whether an order was removed immediately, or whether there
are ongoing swaps which are being allowed to finish.

Closes #552.
@ghost ghost added the in progress label Nov 30, 2018
sangaman added a commit that referenced this issue Nov 30, 2018
This handles the possibility that we attempt to remove an order via the
rpc layer while that order has a hold for a pending swap. Instead of
removing the entire order immediately, this only removes the available
portion of the order and then waits until any swaps finish before
removing the portions that were on hold.

It also adds a field to the `RemoveOrder` rpc response which indicates
whether any portion of the order was on hold. This gives feedback up
front as to whether an order was removed immediately, or whether there
are ongoing swaps which are being allowed to finish.

Closes #552.
sangaman added a commit that referenced this issue Nov 30, 2018
This handles the possibility that we attempt to remove an order via the
rpc layer while that order has a hold for a pending swap. Instead of
removing the entire order immediately, this only removes the available
portion of the order and then waits until any swaps finish before
removing the portions that were on hold.

It also adds a field to the `RemoveOrder` rpc response which indicates
whether any portion of the order was on hold. This gives feedback up
front as to whether an order was removed immediately, or whether there
are ongoing swaps which are being allowed to finish.

Closes #552.
@kilrau
Copy link
Contributor

kilrau commented Nov 30, 2018

I don't think this would apply only to matching mode - in non-matching mode you can still submit orders to xud and remove them. There's the same possibility that someone tries to swap your order at the same time as you try to remove it.

I understand what you are saying is that the exchange could still try to invalidate/remove its ownOrder while it's currently being swapped with a peer BUT this is something we strictly should avoid IMO by XUD removing the order from the exchange before finalizing a swap with a peer. An exchange is not used to a "pending removal" or failed removal. This would be new functionality to implement which we want to avoid as much as possible in nomatching mode. If an order is in the order book and unfilled, it can be removed. Do you agree? @sangaman
Nevertheless adding the functionality to nomatching mode doesn't hurt.

And instead of an error message I'm adding a field to the response to indicate whether a portion of the order was on hold. I don't think it's an "error" per se since the order will still be removed eventually, just that some of it was in the middle of being swapped.

Sounds good.

@kilrau kilrau modified the milestones: 1.0.0-alpha.5, 1.0.0-alpha.6 Dec 5, 2018
@kilrau kilrau added the has PR issues with an open PR label Dec 6, 2018
@ghost ghost removed the in progress label Dec 12, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
has PR issues with an open PR order book swaps
Projects
None yet
Development

No branches or pull requests

2 participants