Market and Limit Oder Filling Logic #40
Conversation
| price: Decimal, | ||
| ) -> Result<BankMsg, ContractError> { | ||
| ensure!( | ||
| self.quantity >= quantity, |
There was a problem hiding this comment.
Shouldn't this be the other way around? With this check, partial fills fail with the error "Order does not have enough funds" which doesn't seem right
There was a problem hiding this comment.
Maybe fulfil is a misleading name, if it was the other way around the quantity in the order struct could potentially go negative? We could pass in the total amount and take the min inside this function if that makes more sense?
| @@ -36,6 +38,69 @@ impl LimitOrder { | |||
| quantity, | |||
| } | |||
| } | |||
|
|
|||
| // Transfers the specified quantity of the order's asset to the owner | |||
| pub fn fulfill( | |||
There was a problem hiding this comment.
Thoughts on simplifying the naming here to fill/fills (as opposed to fulfillment/fulfillments) to better align with standard orderbook terminology?
contracts/orderbook/src/order.rs
Outdated
| ) -> Result<Vec<BankMsg>, ContractError> { | ||
| let mut market_order: MarketOrder = order.clone().into(); | ||
| let (fulfillments, order_fulfillment_msg) = | ||
| run_market_order(storage, &mut market_order, Some(order.tick_id))?; |
There was a problem hiding this comment.
Hmm run_limit_order feels a bit confusing as an abstraction. Might be worth considering renaming to something like fill_limit_to_tick. But also, this is pretty much just doing a market order with a bound, so it might be cleaner to do the following:
- Move the
resolve_fulfillmentslogic intorun_market_order, as technically the market order should not be "complete" until the collected fills are actually resolved and written to state - Remove this abstraction and just call
run_market_orderwith the correct tick bound inplace_limitlogic when appropriate (i.e. when bid limit tick > next ask or ask limit tick < next bid)
There was a problem hiding this comment.
- Can't be done due to the mutability concerns, we could rename this method and abstract the
run_market_ordername/logic similar to the currentrun_limit_order?
There was a problem hiding this comment.
- Yea that makes sense, I'll comment the tests and we can replicate the cases for
place_limit.
There was a problem hiding this comment.
Created an issue tracking this at #42. Moving the logic into place_limit can be done in a separate PR
Closes: #12 #13
What is the purpose of the change
This pull request is to add the logic for filling market orders and, by extension, specially placed limit orders. Orders are filled by crossing ticks in ascending/descending order bounded by the orderbook's next tick data (and placed tick id for limit orders) and filling from currently placed orders.
Due to mutability issues with storage all orders that are being fulfilled are assigned as a
Fulfilment, noting the order and how much is to be filled. Once all ticks have been crossed or the placed order is filled all of theFulfilmentstructs are resolved. Resolving aFulfilmentinvolves generating aBankMsg::Sendfor the order, updatingTICK_LIQUIDITYand updating or removing the order as necessary.The resolved
Fulfilmentstructs are returned as a vector of bank messages in order to transfer funds to the order maker. The placed order's appropriate transfer message is then appended before returning.Testing and Verifying
This change added tests and can be verified as follows:
/contracts/orderbook/src/tests/test_order.sh