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.

Sign up for GitHub

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

Jump to bottom

Replace order call should be atomic #1805

Closed
raladev opened this issue Aug 10, 2020 · 1 comment · Fixed by #1812
Closed

Replace order call should be atomic #1805

raladev opened this issue Aug 10, 2020 · 1 comment · Fixed by #1812
Labels
bug Something isn't working P3 low priority

Comments

@raladev
Copy link
Contributor

raladev commented Aug 10, 2020

Steps:

  1. buy 2 ltc/usdt 0.01 1
  2. buy 3 ltc/usdt 0.01 2
  3. buy 2 ltc/usdt 0.01 1 -r 2 - > 6 ALREADY_EXISTS: order with local id 1 already exists
  4. Check order book

Actual result:
orderbook contains only one order (first) because second one was removed, but new was not placed because id error

Expected result:
orderbook contains 2 orders, order replacement is failed.
@raladev raladev added bug Something isn't working P2 mid priority labels Aug 10, 2020
@raladev raladev changed the title replace order call old removes order before new id checking replace order call removes old order before id checking of new order Aug 10, 2020
@kilrau kilrau added P3 low priority and removed P2 mid priority labels Aug 10, 2020
@kilrau kilrau removed their assignment Aug 10, 2020
@kilrau
Copy link
Contributor

kilrau commented Aug 10, 2020 •
edited
Loading

In short: the replace order call should be atomic, atm it is not (remove can succeed, place can fail)

@kilrau kilrau changed the title replace order call removes old order before id checking of new order Replace order call should be atomic Aug 11, 2020
sangaman added a commit that referenced this issue Aug 12, 2020
@sangaman
feat(p2p): replace order in single packet
a3a70ac 
This modifies the procedure for replacing an order in the order book.
Previously, it would go like this:

1. Remove old order & send order invalidation to peers.
2. Place new order and wait for matching to complete (which may take
some time if it involves swap attempts).
3. Send new order to peers.

Now it goes:

1. Put the old order on hold, preventing further swaps/matching.
2. Place the new order and wait for matching to complete.
3. Remove the old order from the order book.
4. Send a single packet to peers with new order info and old order id.
5. Peers that receive this packet will remove the old order and add
the new one sequentially.

Closes #1805.
@sangaman sangaman mentioned this issue Aug 12, 2020
Merged
sangaman added a commit that referenced this issue Aug 14, 2020
@sangaman
feat(p2p): replace order in single packet
8fb68a4 
This modifies the procedure for replacing an order in the order book.
Previously, it would go like this:

1. Remove old order & send order invalidation to peers.
2. Place new order and wait for matching to complete (which may take
some time if it involves swap attempts).
3. Send new order to peers.

Now it goes:

1. Put the old order on hold, preventing further swaps/matching.
2. Place the new order and wait for matching to complete.
3. Remove the old order from the order book.
4. Send a single packet to peers with new order info and old order id.
5. Peers that receive this packet will remove the old order and add
the new one sequentially.

Closes #1805.
sangaman added a commit that referenced this issue Aug 14, 2020
@sangaman
feat(p2p): replace order in single packet
9dd6be1 
This modifies the procedure for replacing an order in the order book.
Previously, it would go like this:

1. Remove old order & send order invalidation to peers.
2. Place new order and wait for matching to complete (which may take
some time if it involves swap attempts).
3. Send new order to peers.

Now it goes:

1. Put the old order on hold, preventing further swaps/matching.
2. Place the new order and wait for matching to complete.
3. Remove the old order from the order book.
4. Send a single packet to peers with new order info and old order id.
5. Peers that receive this packet will remove the old order and add
the new one sequentially.

By default, replaced orders will have the same local order id as the
order they are replacing unless a different id is specified.

Closes #1805. Closes #1806.
raladev pushed a commit that referenced this issue Aug 17, 2020
@sangaman
feat(p2p): replace order in single packet (#1812)
de691b2 
This modifies the procedure for replacing an order in the order book.
Previously, it would go like this:

1. Remove old order & send order invalidation to peers.
2. Place new order and wait for matching to complete (which may take
some time if it involves swap attempts).
3. Send new order to peers.

Now it goes:

1. Put the old order on hold, preventing further swaps/matching.
2. Place the new order and wait for matching to complete.
3. Remove the old order from the order book.
4. Send a single packet to peers with new order info and old order id.
5. Peers that receive this packet will remove the old order and add
the new one sequentially.

By default, replaced orders will have the same local order id as the
order they are replacing unless a different id is specified.

Closes #1805. Closes #1806.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug Something isn't working P3 low priority
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

feat(p2p): replace order in single packet ExchangeUnion/xud
2 participants
@kilrau @raladev