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

fix: added new grpc method to easily cancel all orders #1910

Merged
merged 5 commits into from
Oct 19, 2020

Conversation

rsercano
Copy link
Collaborator

attempts to resolve #1878

Sorry to implement this prior p1 ones, I had already started this, so wanted to complete first.

image

@rsercano rsercano self-assigned this Sep 21, 2020
Copy link
Collaborator

@sangaman sangaman left a comment

Choose a reason for hiding this comment

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

See comments.

@sangaman
Copy link
Collaborator

This is also definitely not a "fix" commit/PR, should be a feat as it's a new feature.

@rsercano
Copy link
Collaborator Author

This is also definitely not a "fix" commit/PR, should be a feat as it's a new feature.

Yeap sorry, my bad, I literally suck at commit messages.

Signed-off-by: rsercano <ozdemirsercan27@gmail.com>
@rsercano
Copy link
Collaborator Author

Fixed review points could you please take a look at again?

@rsercano rsercano requested review from sangaman and raladev and removed request for raladev September 22, 2020 09:34
@rsercano rsercano force-pushed the feature/cancelallorders branch from 671ab1d to 364f8a6 Compare September 23, 2020 06:33
@raladev
Copy link
Contributor

raladev commented Sep 23, 2020

  • it should be removeallorders, not cancel, because our command for single order is removeorder;
  • error throwing because of attempt of removing order in hold interrupts work of cancelallorders command. (Place 3 orders, start to fill one of them by peer, use cancelallorders command during swap = no removed orders)
    Screenshot from 2020-09-23 16-05-21
  • it would be good to add info about full or partial removing to output, because order may be removed partially (because another part of order is in hold).

Copy link
Contributor

@raladev raladev left a comment

Choose a reason for hiding this comment

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

Above

@rsercano rsercano force-pushed the feature/cancelallorders branch from 364f8a6 to dfee0b6 Compare September 24, 2020 07:52
@rsercano
Copy link
Collaborator Author

rsercano commented Sep 24, 2020

error throwing because of attempt of removing order in hold interrupts work of cancelallorders command. (Place 3 orders, start to fill one of them by peer, use cancelallorders command during swap = no removed orders)

I actually used behavior of removeOrder except this command always tries to remove entire order quantity. So in this case I guess it makes sense to add failed order ids into output due to on hold ? @sangaman @kilrau @raladev

@kilrau
Copy link
Contributor

kilrau commented Sep 24, 2020

Yes. You can simply append the failed cancellations:

cancelled order with id X
cancelled order with id Y
failed to cancel order with id Z due to "on hold"

@sangaman
Copy link
Collaborator

error throwing because of attempt of removing order in hold interrupts work of cancelallorders command. (Place 3 orders, start to fill one of them by peer, use cancelallorders command during swap = no removed orders)

I actually used behavior of removeOrder except this command always tries to remove entire order quantity. So in this case I guess it makes sense to add failed order ids into output due to on hold ? @sangaman @kilrau @raladev

My apologies, this actually looks like a bug in the existing removeOrder logic - if it sees there's a hold on an order then it removes any quantity not on hold and then waits for holds to be resolved (if a swap fails, it'll immediately remove the quantity that's no longer on hold). However, the problem is that if there's no quantity that's not on hold, it attempts to remove 0 quantity and that throws an error. So that just needs be fixed, I'll push a fix for it separately.

With that fix the behavior would be that the call immediately goes through and removes all quantity for all orders that isn't on hold - for any holds it will wait and remove any freed up quantity automatically. Does that sound good to everyone?

@kilrau
Copy link
Contributor

kilrau commented Sep 24, 2020

for any holds it will wait and remove any freed up quantity automatically

Will the call not return until all quantity is removed? If so, assuming an order is on hold for several minutes for whatever reason, when will the removeorder call return?

@sangaman
Copy link
Collaborator

for any holds it will wait and remove any freed up quantity automatically

Will the call not return until all quantity is removed? If so, assuming an order is on hold for several minutes for whatever reason, when will the removeorder call return?

The call returns immediately after its been received, no waiting for holds to expire since those could technically take hours. With the singular remove order call it returns the quantity that's still on hold. Maybe here we could return a boolean indicating whether any orders remain on hold, or possibly a list of orders that are still on hold if we think that may be helpful.

#1921 fixes the bug I mentioned above, once that's merged we can rebase this branch on master and we shouldn't hit that "can't remove 0 quantity" error any longer.

…ature/cancelallorders

� Conflicts:
�	test/simulation/xudrpc/xudrpc.pb.go
@rsercano rsercano force-pushed the feature/cancelallorders branch from dfee0b6 to b70e72d Compare September 28, 2020 11:30
…ature/cancelallorders

� Conflicts:
�	test/simulation/xudrpc/xudrpc.pb.go
@rsercano rsercano requested review from raladev and sangaman and removed request for sangaman September 28, 2020 11:31
@raladev
Copy link
Contributor

raladev commented Sep 28, 2020

#1921 is merged, @rsercano please do rebase

@raladev
Copy link
Contributor

raladev commented Sep 28, 2020

it would be good to add info about full or partial removing to output, because order may be removed partially (because another part of order is in hold).

can be done only after #1526

So

error throwing because of attempt of removing order in hold interrupts work of cancelallorders command. (Place 3 orders, start to fill one of them by peer, use cancelallorders command during swap = no removed orders)

is last that we need and it should work after the rebase

@rsercano
Copy link
Collaborator Author

Rebased @raladev thanks for checking this out !

@rsercano rsercano force-pushed the feature/cancelallorders branch from 12b1c5f to 84f0ccf Compare September 29, 2020 11:50
@rsercano
Copy link
Collaborator Author

Fixed @sangaman thanks for checking

@rsercano rsercano requested a review from sangaman September 29, 2020 11:50
ghost
ghost previously approved these changes Sep 29, 2020
raladev
raladev previously approved these changes Sep 29, 2020
@rsercano rsercano dismissed stale reviews from raladev and ghost via 071fbd9 October 19, 2020 07:20
@raladev raladev self-requested a review October 19, 2020 09:46
@raladev raladev merged commit c958e86 into master Oct 19, 2020
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.

Easy way to cancel orders in CLI
4 participants