-
Notifications
You must be signed in to change notification settings - Fork 43
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(p2p): replace order in single packet #1812
Conversation
a8a91e6
to
054b763
Compare
Commit: a3a70ac
|
035121e
to
8fb68a4
Compare
I made several fixes and new tests, including some manual testing. I mainly just want to make sure the simulation tests here pass, since I was having some trouble running them locally. I'll check back tomorrow and make sure everything is good with the tests, but in the meantime this is ready for testing/review. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be possible to use the same order id when replacing the order?
➜ xud git:(replace-order-packet) ✗ ./bin/xucli sell 0.001 BTC/USDT 10000 --order_id="abc123" no matches found
➜ xud git:(replace-order-packet) ✗ ./bin/xucli sell 0.001 BTC/USDT 13000 --replace_order_id="abc123" --order_id="abc123"
6 ALREADY_EXISTS: order with local id abc123 already exists
At the moment arby's implementation is using:
`arby-${pairId}-buy-order`
`arby-${pairId}-sell-order`;
as order ids which works against master branch, but fails in this branch.
|
Yes I can try and handle #1806 here as well. |
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.
8fb68a4
to
9dd6be1
Compare
Replaced orders now use the id of the order they are replacing by default, all tests are passing, and the issue with the cli not displaying matches has been fixed. Thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Works nicely - good job 👍
Tested with arby's ExchangeUnion/market-maker-bot#75 branch
This modifies the procedure for replacing an order in the order book. Previously, it would go like this:
some time if it involves swap attempts).
Now it goes:
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.