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

feat(rpc/orderbook): handle remove order with hold #715

Merged
merged 2 commits into from
Dec 12, 2018

Conversation

sangaman
Copy link
Collaborator

Closes #552.

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.

It also adds some assertions to the removeOrder logic to ensure that we don't try to remove any part of an order that is on hold.

@sangaman sangaman requested a review from moshababo November 30, 2018 05:46
@ghost ghost assigned sangaman Nov 30, 2018
@ghost ghost added the in progress label 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.
Copy link
Collaborator

@moshababo moshababo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good.

I don't see that you disallow putting more quantity on hold, while we're in the process of removing. Is that intentional?

proto/xudrpc.proto Outdated Show resolved Hide resolved
lib/orderbook/TradingPair.ts Outdated Show resolved Hide resolved
lib/orderbook/OrderBook.ts Show resolved Hide resolved
lib/orderbook/OrderBook.ts Show resolved Hide resolved
@sangaman
Copy link
Collaborator Author

@moshababo I addressed your feedback, still have one open item about the timeouts, let me know what you think.

I believe I'm preventing putting more quantity on hold by removing all the quantity from the order up front that isn't on hold here. Therefore the order can no longer be matched/swapped and won't get any additional holds.

This adds assertions to the `removeOrder` logic to ensure that we do not
remove any portion of an order that is currently on hold.
@moshababo
Copy link
Collaborator

I believe I'm preventing putting more quantity on hold by removing all the quantity from the order up front that isn't on hold here. Therefore the order can no longer be matched/swapped and won't get any additional holds.

Agree, missed that.

@sangaman sangaman merged commit 295e7f9 into master Dec 12, 2018
@sangaman sangaman deleted the remove-order-hold branch December 12, 2018 22:06
@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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants