feat: add order refresh tolerance for queue-priority preservation#144
Conversation
Adds a tolerance-based gate to the market-making refresh loop so that an existing limit order is kept across cycles when its recorded price has drifted no more than ``refresh_tolerance_bp`` basis points from the current ideal price. This preserves Hyperliquid's price-time queue priority and reduces unnecessary cancel/replace API churn during quiet markets, while still re-quoting immediately when the price moves beyond tolerance. * OrderTracker now records the placement price alongside each tracked order (4-tuple) and exposes ``refresh_orders_with_tolerance`` plus ``get_open_sides`` to support the new gate. * Market-making strategy extracts ``_compute_ideal_prices`` from ``_place_orders`` so both the run-loop refresh check and order placement consume the same price calculation. * ``_place_orders`` skips the buy or sell side when a kept order from the previous cycle still occupies that side (prevents duplicate same-side quotes). * A safety-net ``refresh_max_age_seconds`` (default ``refresh_interval_seconds * 4``) caps the lifetime of a kept order even when its price stays within tolerance. Backward compatible: ``refresh_tolerance_bp`` defaults to ``0`` which preserves the original age-only cancel behaviour. ``record_order`` continues to accept three positional arguments; orders without a recorded price fall back to the legacy refresh interval check. Adds 24 new unit tests (decision-table parametrised cases, run-loop dispatch, side-gating, cumulative observability counters) and updates ``test_mm_order_tracker`` for backward-compat coverage. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
keitaj
left a comment
There was a problem hiding this comment.
Review: Order refresh tolerance for queue-priority preservation
Solid feature. The split between a pure _compute_ideal_prices helper and the imperative _place_orders is the right shape — it lets the run-loop reuse the price calculation without side effects. Backward compatibility is well preserved (default 0 keeps the legacy age-only path; record_order still accepts three positional args). Test coverage is thorough (decision-table parametrisation + integration paths). Two should-fix points around redundant work and a one-sample volatility staleness, plus a few nits.
1. Inventory skew is computed twice per _place_orders call (should-fix)
strategies/market_making_strategy.py L688-710 / L744:
_compute_ideal_prices already calls _calculate_inventory_skew and applies the multiplier to both legs. _place_orders then calls _calculate_inventory_skew again purely to emit a debug log. With refresh tolerance enabled, the run-loop also calls _compute_ideal_prices, so the skew is computed up to 3× per cycle per coin.
_calculate_inventory_skew reads self.positions and is cheap individually, but at the loop hot path with multiple coins + multi-DEX it's avoidable redundancy.
Suggestion: Either (a) have _compute_ideal_prices return (buy, sell, skew_bps) so the caller can log it without recomputing, or (b) move the debug log inside _compute_ideal_prices and drop the trailing block in _place_orders.
2. Volatility buffer is one sample behind during run-loop tolerance check (should-fix)
strategies/market_making_strategy.py L727-739 (the _record_mid_price call) and _compute_ideal_prices BBO branch (L662-672 in the new layout):
When refresh_tolerance_bp > 0, the run loop calls _compute_ideal_prices first (using the current vol buffer) and only later — inside _place_orders — calls _record_mid_price. So the tolerance check evaluates drift against an ideal price computed from a vol buffer that is one sample stale relative to the eventual placement, and the placed price uses the freshly-updated buffer.
In practice the drift between the two ideals is bounded by one sample's worth of _get_volatility_adjusted_offset change, so the impact is small. But it can yield asymmetric decisions: the tolerance check decides "keep" against ideal X, while the freshly-placed price would have been X′ ≠ X.
Suggestion: Move _record_mid_price to the run-loop path right after get_market_data, so both call sites of _compute_ideal_prices see the updated buffer. Alternatively, compute ideal prices once in the run loop and thread them into _place_orders to avoid the second computation altogether.
3. Cancel reason labeling is biased toward "drift" when both conditions fail (nit)
strategies/mm_order_tracker.py L195-208:
if drift_bp <= tolerance_bp and age < max_age_seconds:
... # keep
elif age >= max_age_seconds and drift_bp <= tolerance_bp:
to_cancel.append((oid, side, "age"))
else:
to_cancel.append((oid, side, "drift"))The fall-through else branch covers two cases: (a) drift > tol AND age < max_age (legitimate drift cancel) and (b) drift > tol AND age >= max_age (both conditions failed; currently labeled as "drift"). The "age" branch only fires when drift is within tolerance.
For observability, cancelled_drift will overcount in volatile + idle conditions. Not blocking — the order definitely needs cancellation, the misattribution just makes the cumulative counters slightly less informative.
Suggestion: Optional — promote the first else branch to compute the dominant reason explicitly:
if age >= max_age_seconds:
to_cancel.append((oid, side, "age"))
else:
to_cancel.append((oid, side, "drift"))4. getattr(self, 'refresh_tolerance_bp', 0) inside __init__ (nit)
strategies/market_making_strategy.py L211:
The attribute is set two lines above, so the getattr fallback in the same method is dead. The defensive form is needed at the run-loop and _place_orders call sites (because tests bypass __init__), but inside __init__ itself a direct self.refresh_tolerance_bp read is unambiguous.
5. Validator accepts bool for refresh_tolerance_bp (nit)
validation/strategy_validator.py L230-235:
isinstance(True, (int, float)) is True in Python, so a config with refresh_tolerance_bp: True passes validation and is later coerced to 1.0. Pre-existing pattern in this file (taker_fallback_age_seconds has the same shape), so it's not a regression — but adding not isinstance(val, bool) would make it sturdier.
6. get_refresh_stats is queryable but not periodically logged (nit)
strategies/mm_order_tracker.py L242-252:
The cumulative kept / cancelled-by-drift / cancelled-by-age counters are exposed but no caller emits them in the cycle log. To validate Phase B rollout (tolerance=2, watch kept ratio), one would have to attach a debugger or extend the strategy log. Plumbing this into _log_fill_rate or _log_cycle would make the feedback loop tighter.
Not in scope for this PR; tracking it as a follow-up is fine.
Verdict: LGTM
No blockers. The two should-fix items are correctness / efficiency refinements that don't materially affect behaviour with the default tolerance_bp = 0. Recommend addressing them before enabling the feature in production (Phase B), but they're acceptable for merge as-is given the staged rollout in the design doc.
🤖 Generated with Claude Code
…145) Address two should-fix items from the PR #144 review: 1. ``_calculate_inventory_skew`` was being called twice per cycle in ``_place_orders`` (once inside ``_compute_ideal_prices`` and once for the trailing debug log) and up to three times when refresh tolerance is enabled (the run-loop's tolerance check also invoked ``_compute_ideal_prices``). Now ``_compute_ideal_prices`` returns ``(buy, sell, skew_bps)``; ``_place_orders`` accepts the cached tuple and only logs. 2. The volatility rolling buffer (``_record_mid_price``) was being updated inside ``_place_orders`` *after* the run-loop's tolerance check had already evaluated drift, so the tolerance check used a buffer one sample stale relative to the placed order. The buffer update now lives at the top of the BBO branch in ``_compute_ideal_prices``, and the run loop computes the ideal tuple once and threads it into ``_place_orders``, so both call paths see identical state. The threading is opt-in via the new ``ideal_prices`` keyword on ``_place_orders``; legacy callers that omit it (existing tests, the strategy fallback when prices cannot be determined) compute inline as before. Adds four targeted tests verifying the single-call invariants for both the inventory skew and the volatility buffer. Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Summary
refresh_tolerance_bpbasis points from the current ideal price.refresh_tolerance_bpdefaults to0which preserves the original age-only cancel behaviour.Changes
strategies/mm_order_tracker.py: record the placement price alongside each tracked order (4-tuple). Newrefresh_orders_with_tolerance()evaluates per-order drift vs. tolerance and falls back to the legacy refresh interval when an order has no recorded price. Newget_open_sides()exposes which sides currently have tracked orders for the run loop's duplicate-side gate. Newget_refresh_stats()returns cumulative kept / cancelled-by-drift / cancelled-by-age counters.strategies/market_making_strategy.py: extract_compute_ideal_prices()from_place_orders()so both the run-loop refresh check and order placement use the same price calculation. The run loop dispatches torefresh_orders_with_tolerancewhen the new feature is enabled, otherwise to the legacycancel_stale_orders._place_ordersskips the buy or sell side when a kept order from the previous cycle still occupies that side (prevents duplicate same-side quotes). The placement record now includes the price.bot.py: add CLI flags--refresh-tolerance-bpand--refresh-max-age-seconds; register both in the market-making config map.validation/strategy_validator.py: validate the two new parameters (tolerance ≥ 0, max age > 0).README.md: document both parameters under the human-readable strategy section and the AI YAML reference block.tests/test_mm_order_tracker.py: add 18 new tests covering keep / cancel-by-drift / cancel-by-age decisions, partial ideal-price evaluation, close-oid preservation, cumulative stats, and the parametrised decision table; preserve backward compat with the legacy three-argumentrecord_order.tests/test_refresh_tolerance.py(new): integration-style tests covering_compute_ideal_prices(regression for the refactor), the run-loop dispatch on the tolerance flag, and the duplicate-side gate in_place_orders.Test plan
test_mm_order_tracker.py+test_refresh_tolerance.py)flake8 . --max-line-length=120passespytest tests/ -qpasses (965 tests, 0 failures)refresh_tolerance_bp=0) preserves the original age-only cancel behaviour🤖 Generated with Claude Code