Add edit order instructions for spot and perp #191
Conversation
| if new_max_base < 0 { | ||
| throw_err!(MangoErrorCode::Default); | ||
| } | ||
| max_base_quantity = new_max_base; |
There was a problem hiding this comment.
don't you have to adjust the max_quote_quantity in case there is some non zero filled amount?
There was a problem hiding this comment.
definitely, max_base_quantity could be i64::MAX, with max_quote_quantity being used for the order limiting
There was a problem hiding this comment.
I'm not sure what is best here. Initially, I tried to follow the example from the client: https://github.com/blockworks-foundation/mango-client-v3/blob/93240e04c6c87465758a9e02e1637d2f487d05aa/src/client.ts#L1961, where maxQuoteQuantity is optional, and defaults to i64::MAX. We could perhaps check if max_quote_quantity != i64::MAX and then adjust downwards as necessary, but I wasn't sure at which price I should convert filled_amount into quote? Unless I have it backwards as currently written?
| /// - place an order on the book at this price. | ||
| /// | ||
| /// Ignored for Market orders and potentially adjusted for PostOnlySlide orders. | ||
| price: i64, |
There was a problem hiding this comment.
It looks like these fields are identical with PlacePerpOrder2 and will stay that way. Could we put all of them into a separate struct, so the doc comments, fields and serialization code don't need to be repeated?
program/src/processor.rs
Outdated
| } | ||
| let order_id = open_orders.orders[j]; | ||
|
|
||
| if open_orders.is_bid_bits & slot_mask != 0 { |
There was a problem hiding this comment.
why even call find_by_key() for orders that don't have the same side as new_order.side or cancel_order.side?
program/src/processor.rs
Outdated
| // means slot is free | ||
| continue; | ||
| } | ||
| let order_id = open_orders.orders[j]; |
There was a problem hiding this comment.
why are we looking at orders that have order_id != cancel_order.order_id? Wouldn't this find the quantity of some random other open order?
maybe we can fully skip iterating over the 128 potential open orders and check bids/asks directly?
There was a problem hiding this comment.
Great suggestion, I have updated the logic to look directly at the bids/asks
program/src/processor.rs
Outdated
| .checked_mul(new_order.max_coin_qty.get()) | ||
| .unwrap(); | ||
| new_order.max_native_pc_qty_including_fees = | ||
| NonZeroU64::new(new_max_pc_qty_including_fees).unwrap(); |
There was a problem hiding this comment.
This just overrides whatever the user set as max-coin-qty, and doesn't account for any fees: seems suspicious but I'll need to double-check serum
There was a problem hiding this comment.
This and the below comment have been updated to properly include fees and not overwrite user inputs
program/src/processor.rs
Outdated
| NonZeroU64::new(new_max_pc_qty_including_fees).unwrap(); | ||
| } | ||
| serum_dex::matching::Side::Ask => { | ||
| new_order.max_native_pc_qty_including_fees = |
There was a problem hiding this comment.
if there's ever any use to setting this to something else than u64::max, we're overriding the users choice here.
| if new_max_base < 0 { | ||
| throw_err!(MangoErrorCode::Default); | ||
| } | ||
| max_base_quantity = new_max_base; |
There was a problem hiding this comment.
definitely, max_base_quantity could be i64::MAX, with max_quote_quantity being used for the order limiting
| .find_order_with_client_id(market_index, client_order_id) | ||
| .ok_or(throw_err!(MangoErrorCode::ClientIdNotFound))?; | ||
|
|
||
| let book_order = book.get_order(order_id, side)?; |
There was a problem hiding this comment.
this needs to deal with expired orders: maybe the old order is still on the book, but expired?
There was a problem hiding this comment.
I thought it's ok to edit/update an expired order as long as the new order is not already expired (and place_perp_order2 checks for this)? But let me know if this is not the case. I'm not sure what the behavior should be if the order is expired (maybe just throw an error?)
program/src/processor.rs
Outdated
| reduce_only, | ||
| limit, | ||
| ); | ||
| if invalid_id_ok { |
There was a problem hiding this comment.
hmm. if the previous order is missing, should we assume it was fully filled and adjust the new order accordingly?
Current behavior seems to be:
- if the old order is partially filled, cancel it; adjust new order and place it
- if the old order is missing:
- if invalid_id_ok is false: error
- if invalid_id_ok is true: do nothing (i.e. don't place the new order)
To me it sounds like "if the old order is gone (fully filled), then adjust the new order for that and place it" would also make sense.
There was a problem hiding this comment.
Initially I copied the invalid_id_ok flag to mimic the cancel_perp_order instructions, but now I think it doesn't make much sense for editing orders. To be more straightforward with the user, edit_order instructions should fail if there is no order to edit. The updated behavior is:
- if the old order is partially filled, cancel it; adjust new order and place it
- if the old order is missing (fully filled), instruction will fail because
book.get_order()will fail to find an order and will throw aMangoErrorCode::InvalidOrderId
|
this PR looks useful. i'm seeing plenty of double fills. |
Addresses: https://trello.com/c/PsOOKgpu/130-%F0%9F%90%9E-edit-order-race-condition
This PR adds instructions that edit spot and perp order via cancelling and placing.
To prevent multiple fills, the user sends a
cancel_order_sizewhich is the size that they expect to cancel. Thecancel_order_sizeis compared with the remaining order size on chain and the new order will have its size reduced if the order to be cancelled has been partially filled without knowledge of the user. If the order the user is intending to cancel has been completely filled without the knowledge of the user, the edit instructions will fail.