fix: thread ideal prices through _place_orders to remove double work

[keitaj]

Summary

Address two should-fix items from the PR #144 review:

1. Inventory skew was computed twice per cycle (or three times when refresh tolerance is enabled). _compute_ideal_prices now returns the skew alongside the prices, and _place_orders consumes the cached tuple — no recomputation, just logging.

2. Volatility buffer was one sample stale during the run-loop tolerance check. _record_mid_price was called inside _place_orders after the run loop had already evaluated drift, so the tolerance comparison used a buffer one sample behind the placed order. The buffer update now lives at the top of the BBO branch of _compute_ideal_prices, and the run loop computes the ideal tuple once and threads it into _place_orders — 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, preserving full backward compatibility.

Changes

• strategies/market_making_strategy.py:

  • _compute_ideal_prices now returns Optional[Tuple[float, float, float]] (buy, sell, skew_bps) and updates _record_mid_price directly when in BBO mode with a valid book.
  • _place_orders accepts an optional ideal_prices keyword. When provided it skips the recompute. When omitted it falls back to the inline path. The trailing _calculate_inventory_skew call (which existed only to emit a debug log) is gone — the skew arrives via the threaded tuple.
  • The run loop's no-position branch computes _compute_ideal_prices once and passes the tuple to both the tolerance refresh and _place_orders.
  • The _record_mid_price call inside _place_orders is removed (now handled by _compute_ideal_prices).

• tests/test_refresh_tolerance.py:

  • Existing _compute_ideal_prices and run-loop dispatch tests updated for the new 3-tuple shape and the OrderSide.*.value keying.
  • New TestSingleComputePerCycle class with 4 tests directly verifying:
    • _calculate_inventory_skew is called exactly once when ideal_prices is threaded into _place_orders.
    • _calculate_inventory_skew is still called exactly once on the legacy (no-threading) path.
    • _record_mid_price fires exactly once per cycle in BBO mode.
    • _record_mid_price does not fire in non-BBO mode (preserves pre-PR behaviour).

Test plan

• flake8 . --max-line-length=120 passes
• pytest tests/ -q passes (969 tests, 0 failures — 4 new tests under TestSingleComputePerCycle)
• Backward compatibility: legacy _place_orders(coin) callers (existing test fixtures) still work via the inline fallback
• No behaviour change with refresh_tolerance_bp = 0 (default)

🤖 Generated with Claude Code

[keitaj]

Review: Thread ideal prices through _place_orders to remove double work

Both should-fix items from PR #144 are addressed cleanly. The 3-tuple return from _compute_ideal_prices plus the optional ideal_prices kwarg on _place_orders is the right shape — opt-in threading from the run loop, transparent fallback for legacy callers, and a clear single source of truth for the volatility buffer update. The four new TestSingleComputePerCycle tests directly verify the invariants (skew once, vol-buffer once in BBO, vol-buffer never in non-BBO). No blockers, no should-fix; a few nits below for follow-up.

---

1. Redundant _compute_ideal_prices call when ideal is None (nit)

strategies/market_making_strategy.py L411-471:

When the run loop gets ideal = None (no market data or mid <= 0), it dispatches to the legacy cancel_stale_orders and then later still invokes _place_orders(coin, ideal_prices=None). The inline fallback inside _place_orders re-runs _compute_ideal_prices (returns None again) just to emit the "No market data" warning. That's two calls for the same negative result.

Suggestion: Short-circuit in the run loop when ideal is None:

ideal = self._compute_ideal_prices(coin)
if ideal is None:
    logger.warning(f"[mm] No market data for {coin}, skipping")
    continue

This also lets you simplify the if ideal is None branch around the tolerance dispatch (no longer reachable). The cooldown / auto-exclude / max-positions checks would be skipped too — fine, since they have no useful work to do without market data.

---

2. Imbalance check refetches market_data separately (nit)

strategies/market_making_strategy.py L788-797:

The imbalance branch does its own self.market_data.get_market_data(coin) after the threaded ideal-prices path has already gone through _compute_ideal_prices (which also called get_market_data). With WS market data the cost is a cached dict lookup, so the practical cost is negligible — but if you want full elimination of redundant fetches, threading book_imbalance (or market_data) into the tuple would close the gap.

Not worth complicating the signature unless it shows up in a profile.

---

3. _compute_ideal_prices is no longer side-effect-free (nit)

strategies/market_making_strategy.py L654-666:

The previous docstring described it as a "pure helper (no side effects on rolling buffers, no order placement)". The new implementation does mutate _recent_mids via _record_mid_price in the BBO branch. The new docstring acknowledges this clearly (Updates the volatility rolling buffer via _record_mid_price), so functional correctness is fine. The function name hints at purity, though, and a future maintainer might be surprised. Renaming to something like _refresh_ideal_prices would communicate the dual role; bikeshed-level so feel free to ignore.

---

Verdict: LGTM

No blockers, no should-fix. Both items from the previous review are correctly resolved with direct test coverage, and the threading API preserves backward compatibility for the existing test fixtures and the strategy fallback path. Ready to merge.

🤖 Generated with Claude Code