test: Codify the close-order invariant for OrderTracker#141
Conversation
Adds contract tests asserting that orders not registered via ``record_order`` (close orders managed by ``MMPositionCloser`` in particular) are never returned, counted, or cancelled by any ``OrderTracker`` API. This protects the existing separation between entry tracking and close-order management against future regressions that could cause ws guards to start cancelling close-side quotes.
keitaj
left a comment
There was a problem hiding this comment.
Review: OrderTracker close-order invariant tests
Pure-test PR codifying an architectural invariant. No production change, low risk. Three behavioral checks that exercise every public OrderTracker API guards route through (cancel-by-side, cancel-all-for-coin, get_order_count). Module docstring is unusually clear about why the invariant matters — that alone is worth committing.
1. Test scope vs. invariant scope (nit)
tests/test_mm_order_tracker.py:
The tests assert that correctly-used OrderTracker does not touch unregistered oids — i.e. they verify the API behavior, not the system-level claim that "close oids never reach record_order". A future refactor that mistakenly calls tracker.record_order(close_oid, ...) from the close path would still pass these tests, since record_order would dutifully add it to _tracked_orders and guards would then cancel it. Worth being explicit in the docstring that this PR locks the receiver-side contract; the caller-side discipline (only entry placement registers) is enforced by code review rather than by these tests.
Suggestion: add one line to the module docstring after the failure-mode paragraph:
Note: these tests guarantee that callers who do not register a close
oid will not see it cancelled. They do *not* guarantee no caller
registers a close oid -- that discipline lives in
``MarketMakingStrategy._place_quotes`` (single ``record_order`` site,
``reduce_only=False``).
2. Redundant assertion (nit)
tests/test_mm_order_tracker.py L43-44 and L66-67:
assert cancelled_oids == {1001, 1002} already implies 5000 not in cancelled_oids — the second line never adds signal. Acceptable as defense-in-depth documentation, but if the goal is precision, the explicit-not-in line could be dropped. No change required.
Verdict: LGTM
No blockers. Both items above are nits; the tests pass, lint passes, and the contract documentation is the actual value-add.
Summary
OrderTracker(notably close orders owned byMMPositionCloser._open_positions) are never returned, counted, or cancelled by anyOrderTrackerAPI.ImbalanceGuard,BboGuard,BboVelocityGuard) cancel orders only viaOrderTracker.cancel_orders_by_side/cancel_all_orders_for_coin, which iterate_tracked_orders(entries only). If a future refactor accidentally registered a close oid here, those guards would start cancelling close-side quotes, leaving positions to time out and fall back to taker close.Changes
tests/test_mm_order_tracker.py(new):TestCloseOrderInvariantwith three tests coveringcancel_orders_by_side,cancel_all_orders_for_coin, andget_order_count. The module docstring documents the invariant and why it matters.Test plan
flake8passes.pytest tests/ -qpasses (918 tests, +3 from this PR).🤖 Generated with Claude Code