Remove cancel event for limit order when all size fills, enable posts below min size #347
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
The inline modifier needs to be commented out for coverage testing per aptos-labs/aptos-core#9154
alnoki
force-pushed
the
limit-order-cancel-reason
branch
from
6b27fd3 to
f5d5c13
Compare
| @@ -3470,7 +3469,7 @@ module econia::market { | |||
| let market_order_id = | |||
| ((order_book_ref_mut.counter as u128) << SHIFT_COUNTER); | |||
| // If order eligible to post: | |||
| if (option::is_none(&cancel_reason_option)) { | |||
| if (option::is_none(&cancel_reason_option) && (remaining_size > 0)) { | |||
There was a problem hiding this comment.
This combined with
econia/src/move/econia/sources/market.move
Lines 3507 to 3508 in f5d5c13
mean that any order that is cancelled here will have remaining_size set to zero. However, this shouldn't be the case. If, for instance, an IOC order partially filled and then cancelled due to the entire order not being immediately fillable, the remaining size should still not be zero as the order was not fully filled.
There was a problem hiding this comment.
Ok, but in the future please link the commit in which the fix was made so I can look at the diff
There was a problem hiding this comment.
@bogberry will do, thanks for clarifying
alnoki
force-pushed
the
limit-order-cancel-reason
branch
from
96bd399 to
4e864ee
Compare
alnoki
changed the title
Summary of changes
Previously, if a
NO_RESTRICTIONlimit order fully filled across the spread, it would emit aCancelOrderEventwithCANCEL_REASON_TOO_SMALL_AFTER_MATCHING.Now, if a
NO_RESTRICTIONlimit order fills across the spread it will not emit aCancelOrderEventfor a full fill. Additionally, limit orders that first fill across the spread can post to the book at any nonzero size.Versus market orders and swaps
Now, cancel event behavior is consistent across limit orders, market orders, and swap orders. If an order fully fills during the transaction in which it was placed, it will emit A
Place<Limit | Market | Swap>OrderEventandFillEvent(s), but not aCancelOrderEvent.Testing
Move source code has been tested to 100% coverage, but there is no way to validate event fields inside a Move unit test. I documented Aptos' event testing deficiency in October (aptos-labs/aptos-core#5369), and in May I cited insufficient event testing as the likely reason for an error in Aptos' own code that I corrected through a PR (aptos-labs/aptos-core#8448).
The inability to write Move unit tests that assert event emissions is a contributing factor to this PR's changes not getting slated into #321 , and seeing as this issue was uncovered during discussions about projected event emission schedules, it is suggested that all event emission cases and field be asserted during end-to-end tests in Python and/or Rust.