From 33a8bf95b0a115ff314b24e72bd5d8282f46da9d Mon Sep 17 00:00:00 2001 From: keitaj Date: Wed, 14 Oct 2026 07:53:21 +0900 Subject: [PATCH 1/2] feat: log max adverse excursion per position on close events MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Records the worst unrealized loss each position reaches while open and appends `mae_bps` / `mae_age` to the `[close-reason]` line. The unrealized-loss early close (`unrealized_loss_close_bps`) cannot be calibrated from fill data alone: fills show the realized outcome, not the price path during the hold, so there is no way to count the positions that breached a candidate threshold and then recovered. Those positions are what an early close would convert from winners into losers, so they are the denominator any threshold choice needs. This adds that measurement without changing behaviour. Observation only. Three properties worth noting: - Emitted for every close reason, including profitable maker closes, since the recovered positions are the point. A position that never moved against us logs 0.0 rather than being omitted, so the denominator is not biased. - `mae_age` separates "drifted early then recovered" from "drifted late", which imply opposite verdicts on closing early. - Independent of `unrealized_loss_close_bps`. The existing unrealized-bps arithmetic sits inside an `if threshold > 0` branch, so with the feature disabled nothing was being measured — exactly the configuration that needs the data. Sampling is bounded by the main-loop cadence, so the value is a lower bound on the true excursion: an intra-cycle spike that retraces is not seen. This is documented in the method, the README and a test, because it makes the data asymmetric evidence — it can show that an early close would have cut recoveries short, but it cannot establish the converse. Adds `MarketDataManager.get_cached_market_data()`, a cache-only read. `get_market_data()` falls through to a REST L2 fetch on a cache miss, and this samples once per position per cycle, so routing through the fetching accessor could add API load when WS is off. On a miss the sample is skipped instead. A test asserts the on/off call counts are identical rather than asserting a bare "never called", which would have passed by accident of the close-placement path's own calls. `manage()` now reads market data unconditionally, so tests exercising it must state what the cache holds; the shared closer mocks default to a cache miss, which is a real state the code handles by skipping. Co-Authored-By: Claude Opus 5 --- README.md | 3 + market_data.py | 10 + strategies/market_making_strategy.py | 1 + strategies/mm_config.py | 8 + strategies/mm_position_closer.py | 60 ++++ tests/test_close_reason_logging.py | 3 + tests/test_dynamic_close_pricing.py | 3 + tests/test_ghost_position_fix.py | 3 + tests/test_mm_position_closer.py | 6 + tests/test_position_mae.py | 424 ++++++++++++++++++++++++++ tests/test_reduce_only_collision.py | 3 + tests/test_unrealized_loss_close.py | 3 + tests/test_volatility_position_age.py | 3 + 13 files changed, 530 insertions(+) create mode 100644 tests/test_position_mae.py diff --git a/README.md b/README.md index 65ed38a..1524357 100644 --- a/README.md +++ b/README.md @@ -373,6 +373,8 @@ They are retained (not removed) because both become meaningful under a two-sided - `--imbalance-guard-threshold`: Cancel one side when L2 book is skewed (0–1, default: 0) - `--close-refresh-threshold-bps`: Refresh close orders on BBO change to improve maker fill rate (default: 0 = disabled) +**Max adverse excursion logging** (`track_max_adverse_excursion`, default on): Records the worst unrealized loss each position reaches while it is open and appends `mae_bps` (positive = moved against us) and `mae_age` (position age when that worst point occurred) to the `[close-reason]` log line. Emitted for every close reason, including profitable maker closes, because the positions that drifted and then recovered are exactly what an unrealized-loss threshold would have closed out early — so they are the denominator when calibrating `unrealized_loss_close_bps`, and fills alone cannot show them. Observation only: it never influences a close decision, and samples come from a cache-only market-data read, so it adds no API calls. Two caveats for consumers: sampling happens once per main-loop cycle, making the value a **lower bound** on the true excursion (an intra-cycle spike that retraces is not seen, so counts of positions breaching a level are under-counted); and the fields append at the end of the line, so existing `last_tier` / `max_age` log parsers keep working unchanged. + **Oracle divergence guard** (`--oracle-guard`): Subscribes to the `activeAssetCtx` stream and uses the HIP-3 `oraclePx` feed (deployer-pushed every ~3s, each update capped at 1% of the previous value) as a defensive signal against stale-quote sniping. Two gates: a **divergence gate** cancels the stale side's entry quotes when `|book mid − oraclePx|` exceeds `--oracle-divergence-threshold-bps` (oracle above mid → resting SELLs are stale-cheap → cancel sells, and vice versa), and a **momentum gate** cancels both sides and blocks placement for `--oracle-guard-block-seconds` when a single oracle step pins near the 1% cap (`--oracle-momentum-cap-pin-bps`) or `--oracle-momentum-consecutive` same-direction steps of at least `--oracle-momentum-min-step-bps` occur. A staleness TTL (`--oracle-stale-ttl-seconds`) doubles as automatic market-close detection: when the oracle value stops changing (e.g. equity perps outside US hours), the guard fully disarms so a floating book is not mistaken for divergence, and re-baselines on recovery so the post-open gap never counts as a step. Cancels affect entry orders only (close orders are untouched), and any missing data — no ctx received, WS down, unparseable oracle price — leaves the guard inert. Default disabled. **Adverse selection logging** (`--enable-adverse-selection-log`): Measures mid-price movement 5s/30s/60s after each fill, logging per-coin summaries every 300s. Observation only — no trading impact. @@ -615,6 +617,7 @@ strategies: close_tier_toxicity_min_fills: 5 # --close-tier-toxicity-min-fills (min fills in window required to fire) close_tier_toxicity_floor_pct: 0.15 # --close-tier-toxicity-floor-pct (breakeven fraction floor after multiplier) unrealized_loss_close_bps: 0 # --unrealized-loss-close-bps (early taker close when unrealized loss exceeds this bps; 0 = disabled) + track_max_adverse_excursion: true # (log mae_bps/mae_age on the close-event line; observation only, no extra API calls) bbo_mode: false # --bbo-mode (place orders at best bid/ask instead of mid ± spread) bbo_offset_bps: 0 # --bbo-offset-bps (bps behind BBO; 0 = at BBO) inventory_skew_bps: 0 # --inventory-skew-bps (NO-OP in the current single-sided flow: a coin holding a position does not re-quote, so the skew never reaches an order; warns at startup if non-zero) diff --git a/market_data.py b/market_data.py index 17efadf..5a937e5 100644 --- a/market_data.py +++ b/market_data.py @@ -151,6 +151,16 @@ def _parse_levels(self, coin: str, levels) -> Optional[MarketData]: micro_price=micro_price, ) + def get_cached_market_data(self, coin: str) -> Optional[MarketData]: + """Return the cached snapshot for *coin*, or ``None`` when not fresh. + + Unlike :meth:`get_market_data` this never falls back to a REST L2 + fetch, so callers that only want an opportunistic read (observation, + logging) cannot add API load. Returns ``None`` on a cache miss — the + caller is expected to skip rather than fetch. + """ + return self._cache.get(coin) + def update_from_ws(self, coin: str, levels) -> None: """Update the cache from a WebSocket l2Book message. diff --git a/strategies/market_making_strategy.py b/strategies/market_making_strategy.py index c42f874..1319c96 100644 --- a/strategies/market_making_strategy.py +++ b/strategies/market_making_strategy.py @@ -316,6 +316,7 @@ def __init__(self, market_data_manager, order_manager, config: Dict) -> None: coin_close_tier_overrides=self.cfg.per_coin.close_tier, close_tier_toxicity=self.cfg.close_tier_toxicity, close_tier_min_seconds=self.cfg.close.tier_min_seconds, + track_max_adverse_excursion=self.cfg.close.track_max_adverse_excursion, ) @property diff --git a/strategies/mm_config.py b/strategies/mm_config.py index 0aed92c..2189e97 100644 --- a/strategies/mm_config.py +++ b/strategies/mm_config.py @@ -303,6 +303,11 @@ class CloseConfig: # against over-shortening when dynamic age, per-coin overrides and # toxicity acceleration stack. 0 = disabled (legacy behaviour). tier_min_seconds: float = 0.0 + # Log the worst unrealized loss each position reached (mae_bps/mae_age on + # the close-event line). Observation only — no effect on close decisions + # and no extra API calls, so it defaults on. Needed to calibrate + # unrealized_loss_close_bps, which cannot be done from fills alone. + track_max_adverse_excursion: bool = True def __post_init__(self) -> None: if self.tier_min_seconds < 0: @@ -621,6 +626,9 @@ def from_legacy_dict(cls, d: Dict) -> "MMConfig": unrealized_loss_close_bps=float(d.get('unrealized_loss_close_bps', 0.0)), force_close_max_loss_bps=float(d.get('force_close_max_loss_bps', 0.0)), tier_min_seconds=float(d.get('close_tier_min_seconds', 0.0)), + track_max_adverse_excursion=bool( + d.get('track_max_adverse_excursion', True) + ), ), close_tier_toxicity=CloseTierToxicityConfig( enabled=bool(d.get('close_tier_toxicity_enabled', False)), diff --git a/strategies/mm_position_closer.py b/strategies/mm_position_closer.py index a11f96d..0a5ce63 100644 --- a/strategies/mm_position_closer.py +++ b/strategies/mm_position_closer.py @@ -62,6 +62,7 @@ def __init__( coin_close_tier_overrides: Optional[Dict[str, Tuple[float, float]]] = None, close_tier_toxicity: Optional[CloseTierToxicityConfig] = None, close_tier_min_seconds: float = 0.0, + track_max_adverse_excursion: bool = True, ) -> None: self.order_manager = order_manager self.market_data = market_data @@ -99,9 +100,18 @@ def __init__( self._tox_active: Dict[str, bool] = {} # Absolute floor (seconds) for the breakeven transition (0 = disabled) self.close_tier_min_seconds = close_tier_min_seconds + # Observation only: track the worst unrealized loss each position + # reaches while it is open, so the close-event log can report it. + self._track_mae = track_max_adverse_excursion # coin -> (entry_time, close_oid or None, close_tier) self._open_positions: Dict[str, Tuple[float, Optional[int], int]] = {} + # coin -> (max_adverse_excursion_bps, position_age_when_reached). + # Seeded to (0.0, 0.0) when a position is first tracked so a position + # that never moved against us is recorded as 0.0 rather than being + # omitted — the recovered positions are the denominator when + # calibrating the unrealized-loss close threshold. + self._mae: Dict[str, Tuple[float, float]] = {} # coin -> consecutive cycles where manage() failed to place a close order self._consecutive_close_failures: Dict[str, int] = {} # coin -> last effective_max_age seen by manage(); used so close-event @@ -142,11 +152,21 @@ def _record_close( clamped low) or by the full grace window. Optional and backward-compatible — the existing log regex ``last_tier=(\\S+)`` still matches the same group. + + ``mae_bps`` / ``mae_age`` report the worst unrealized loss the + position reached and the age at which it did. Emitted for every + close reason, including profitable maker closes, because a + threshold-based early close can only be calibrated against the + positions that drifted and then recovered. Appended at the end of + the line for the same backward-compatibility reason as ``max_age``. """ self._close_stats[reason] += 1 self._close_stats_by_coin[coin][reason] += 1 tier_name = _TIER_NAMES.get(tier, f"tier{tier}") suffix = f" max_age={effective_max_age:.0f}s" if effective_max_age is not None else "" + mae = self._mae.pop(coin, None) + if mae is not None: + suffix += f" mae_bps={mae[0]:.1f} mae_age={mae[1]:.0f}s" logger.info( f"[close-reason] {coin} reason={reason} age={age:.0f}s " f"last_tier={tier_name}{suffix}" @@ -276,6 +296,8 @@ def manage(self, coin: str, position: Dict, close_position_fn, # Register position if not tracked if coin not in self._open_positions: self._open_positions[coin] = (now, None, _TIER_NORMAL) + if self._track_mae: + self._mae[coin] = (0.0, 0.0) logger.info(f"[mm] Tracking position for {coin}: size={size:.6f} entry={entry_price:.4f}") entry_time, close_oid, current_tier = self._open_positions[coin] @@ -285,6 +307,11 @@ def manage(self, coin: str, position: Dict, close_position_fn, # on_position_closed paths that don't otherwise know it. self._last_effective_max_age[coin] = effective_max_age + # Sample the running max adverse excursion before any close decision + # below, so the reading reported with the close event includes this + # cycle. Observation only. + self._update_mae(coin, size, entry_price, age) + # Unrealized loss early close: taker close when loss exceeds threshold. # Uses the per-coin override if present, else the global threshold. unrealized_loss_threshold = self._get_unrealized_loss_bps_for_coin(coin) @@ -378,6 +405,7 @@ def manage(self, coin: str, position: Dict, close_position_fn, ) self._open_positions.pop(coin, None) self._consecutive_close_failures.pop(coin, None) + self._mae.pop(coin, None) else: self._consecutive_close_failures.pop(coin, None) @@ -408,6 +436,7 @@ def _handle_force_close( ) self._open_positions.pop(coin, None) self._consecutive_close_failures.pop(coin, None) + self._mae.pop(coin, None) return except Exception as e: logger.debug(f"[mm] Could not verify position for {coin} before force close: {e}") @@ -538,6 +567,36 @@ def _get_unrealized_loss_bps_for_coin(self, coin: str) -> float: return self._coin_unrealized_loss_overrides[bare] return self.unrealized_loss_close_bps + def _update_mae(self, coin: str, size: float, entry_price: float, age: float) -> None: + """Update the running max adverse excursion for an open position. + + Observation only — never influences a close decision. Uses the + cache-only market-data read so an enabled-by-default observation + cannot add API load: on a cache miss the sample is skipped rather + than fetched. + + ``mae_bps`` uses the same sign convention as the unrealized-loss + early close: positive means the position has moved against us. + + Sampling is bounded by the main-loop interval, so the recorded + value is a *lower bound* on the true excursion — an intra-interval + spike that retraced before the next cycle is not seen. Consumers + calibrating a threshold against this must treat counts of positions + that breached a level as under-counted. + """ + if not self._track_mae or entry_price <= 0 or size == 0: + return + md = self.market_data.get_cached_market_data(coin) + if md is None or md.mid_price <= 0: + return + if size > 0: # long: adverse when the mid falls below entry + adverse_bps = (entry_price - md.mid_price) / entry_price * 10_000 + else: # short: adverse when the mid rises above entry + adverse_bps = (md.mid_price - entry_price) / entry_price * 10_000 + prev = self._mae.get(coin) + if prev is None or adverse_bps > prev[0]: + self._mae[coin] = (adverse_bps, age) + def set_adverse_tracker(self, tracker: Any) -> None: """Register the AdverseSelectionTracker for toxicity-linked tiers. @@ -670,6 +729,7 @@ def _place_take_profit( logger.info(f"[mm] Position for {coin} already closed, skipping close order") self._open_positions.pop(coin, None) self._consecutive_close_failures.pop(coin, None) + self._mae.pop(coin, None) return False except Exception: pass # Proceed with placement — rejection is safer than missing a close diff --git a/tests/test_close_reason_logging.py b/tests/test_close_reason_logging.py index bcc819d..9d367b7 100644 --- a/tests/test_close_reason_logging.py +++ b/tests/test_close_reason_logging.py @@ -21,6 +21,9 @@ def _make_closer(max_age=120, maker_only=True, taker_fallback=None, spread_bps=10): om = MagicMock() md = MagicMock() + # Cache miss by default: manage() samples market data unconditionally + # for max-adverse-excursion tracking, which is observation-only. + md.get_cached_market_data.return_value = None md.round_size.return_value = 0.5 md.get_sz_decimals.return_value = 0 md.price_rounding_params.return_value = (0, True) diff --git a/tests/test_dynamic_close_pricing.py b/tests/test_dynamic_close_pricing.py index 720b021..912cb38 100644 --- a/tests/test_dynamic_close_pricing.py +++ b/tests/test_dynamic_close_pricing.py @@ -11,6 +11,9 @@ def _make_closer(max_age=120, taker_fallback=120, aggressive_loss_bps=1.0, """Create a PositionCloser with mocked dependencies.""" om = MagicMock() md = MagicMock() + # Cache miss by default: manage() samples market data unconditionally + # for max-adverse-excursion tracking, which is observation-only. + md.get_cached_market_data.return_value = None closer = PositionCloser( order_manager=om, market_data=md, spread_bps=10, max_position_age_seconds=max_age, diff --git a/tests/test_ghost_position_fix.py b/tests/test_ghost_position_fix.py index b928653..f761bf8 100644 --- a/tests/test_ghost_position_fix.py +++ b/tests/test_ghost_position_fix.py @@ -109,6 +109,9 @@ def test_different_dex_cache_independent(self): def _make_closer(max_age=120, maker_only=False, spread_bps=10): om = MagicMock() md = MagicMock() + # Cache miss by default: manage() samples market data unconditionally + # for max-adverse-excursion tracking, which is observation-only. + md.get_cached_market_data.return_value = None md.round_size.return_value = 0.5 closer = PositionCloser( order_manager=om, diff --git a/tests/test_mm_position_closer.py b/tests/test_mm_position_closer.py index 5d152a2..6edb84d 100644 --- a/tests/test_mm_position_closer.py +++ b/tests/test_mm_position_closer.py @@ -22,6 +22,9 @@ def _make_closer(max_age=120, maker_only=True, taker_fallback=None, spread_bps=10): om = MagicMock() md = MagicMock() + # Cache miss by default: manage() samples market data unconditionally + # for max-adverse-excursion tracking, which is observation-only. + md.get_cached_market_data.return_value = None md.round_size.return_value = 0.5 md.get_sz_decimals.return_value = 0 md.price_rounding_params.return_value = (0, True) @@ -349,6 +352,9 @@ def _make_tier_closer(max_age=120, coin_tiers=None, tox_cfg=None, tracker=None, maker_only=False): om = MagicMock() md = MagicMock() + # Cache miss by default: manage() samples market data unconditionally + # for max-adverse-excursion tracking, which is observation-only. + md.get_cached_market_data.return_value = None md.round_size.return_value = 0.5 md.price_rounding_params.return_value = (0, True) om.get_all_positions.return_value = [{'coin': 'BTC', 'szi': '1.0'}] diff --git a/tests/test_position_mae.py b/tests/test_position_mae.py new file mode 100644 index 0000000..3da61fe --- /dev/null +++ b/tests/test_position_mae.py @@ -0,0 +1,424 @@ +"""Tests for max adverse excursion (MAE) tracking on open positions. + +MAE is the worst unrealized loss a position reaches while it is open. It is +recorded for observation only -- it never influences a close decision -- and +is emitted on the close-event log line so the unrealized-loss early-close +threshold can be calibrated from production data. That calibration needs the +positions that drifted *and recovered*, which fills alone cannot show. +""" + +import logging +import re +import time + +import pytest +from unittest.mock import MagicMock + +from strategies.mm_position_closer import ( + CLOSE_REASON_EXTERNAL, + CLOSE_REASON_MAKER, + CLOSE_REASON_UNREALIZED_LOSS, + PositionCloser, +) + +# The log-parsing regexes used by downstream analysis, which the new fields +# must not break. +_BASE_RE = re.compile( + r"\[close-reason\]\s+(\S+)\s+reason=(\S+)\s+age=(\d+)s\s+last_tier=(\S+)" +) +_MAX_AGE_RE = re.compile(r"max_age=(\d+)s") +_MAE_RE = re.compile(r"mae_bps=(-?\d+\.\d+)\s+mae_age=(\d+)s") + + +def _make_closer( + unrealized_loss_close_bps: float = 0.0, + track_max_adverse_excursion: bool = True, + max_age: float = 120, +) -> tuple: + """Build a PositionCloser whose cached mid can be driven per-test.""" + om = MagicMock() + md = MagicMock() + md.round_size.return_value = 0.5 + md.get_sz_decimals.return_value = 0 + md.price_rounding_params.return_value = (0, True) + md.get_cached_market_data.return_value = None + md.get_market_data.return_value = None + closer = PositionCloser( + order_manager=om, + market_data=md, + spread_bps=10, + max_position_age_seconds=max_age, + maker_only=True, + taker_fallback_age_seconds=None, + unrealized_loss_close_bps=unrealized_loss_close_bps, + track_max_adverse_excursion=track_max_adverse_excursion, + ) + om.get_all_positions.return_value = [{'coin': 'BTC', 'szi': '1.0'}] + return closer, om, md + + +def _set_mid(md, mid: float) -> None: + """Point both market-data accessors at a mid price.""" + snapshot = MagicMock() + snapshot.mid_price = mid + snapshot.bid = mid * 0.999 + snapshot.ask = mid * 1.001 + md.get_cached_market_data.return_value = snapshot + md.get_market_data.return_value = snapshot + + +def _manage(closer, md, mid: float, size: float = 1.0, entry: float = 100.0) -> None: + """Run one manage() cycle at a given mid price.""" + _set_mid(md, mid) + closer.manage('BTC', {'size': size, 'entry_price': entry}, MagicMock()) + + +class TestMaeTracking: + """The running maximum is taken over the position's lifetime.""" + + def test_long_records_worst_drawdown(self): + """Long entry=100: mids 99.9 / 99.7 / 99.8 -> MAE is the 99.7 sample.""" + closer, om, md = _make_closer() + for mid in (99.9, 99.7, 99.8): + _manage(closer, md, mid) + mae_bps, _ = closer._mae['BTC'] + assert mae_bps == pytest.approx(30.0) # (100 - 99.7) / 100 * 10_000 + + def test_peak_survives_recovery(self): + """A position that drifts then fully recovers keeps the peak value. + + This is the case the measurement exists for: at close time the PnL + looks fine, but the position did breach a threshold on the way. + """ + closer, om, md = _make_closer() + _manage(closer, md, 99.5) # -50 bps + _manage(closer, md, 100.2) # recovered into profit + mae_bps, _ = closer._mae['BTC'] + assert mae_bps == pytest.approx(50.0) + + def test_short_sign_convention(self): + """Short is adverse when the mid rises: positive bps, same as long.""" + closer, om, md = _make_closer() + _manage(closer, md, 100.4, size=-1.0) + mae_bps, _ = closer._mae['BTC'] + assert mae_bps == pytest.approx(40.0) + + def test_never_adverse_records_zero(self): + """A position that only ever moved in our favour records 0.0, not None. + + Omitting these would bias the denominator when asking what share of + positions breached a threshold. + """ + closer, om, md = _make_closer() + _manage(closer, md, 100.5) + _manage(closer, md, 101.0) + assert closer._mae['BTC'] == (0.0, 0.0) + + def test_mae_age_is_when_the_peak_happened(self): + """mae_age distinguishes 'drifted early then recovered' from 'drifted late'.""" + closer, om, md = _make_closer() + _manage(closer, md, 100.0) + # Backdate the entry so the next sample lands at a measurable age. + entry_time, oid, tier = closer._open_positions['BTC'] + closer._open_positions['BTC'] = (entry_time - 40, oid, tier) + _manage(closer, md, 99.0) + mae_bps, mae_age = closer._mae['BTC'] + assert mae_bps == pytest.approx(100.0) + assert 39 <= mae_age <= 42 + + +class TestMaeIndependentOfEarlyClose: + """Tracking must not depend on the early-close feature being enabled.""" + + def test_tracked_when_threshold_disabled(self): + """With unrealized_loss_close_bps=0 the MAE is still recorded. + + The existing unrealized_bps arithmetic sits behind a `threshold > 0` + branch, so this is the case that would otherwise go unmeasured. + """ + closer, om, md = _make_closer(unrealized_loss_close_bps=0.0) + _manage(closer, md, 99.6) + assert closer._mae['BTC'][0] == pytest.approx(40.0) + + def test_reported_on_unrealized_loss_close(self, caplog): + """The early-close path also reports MAE.""" + closer, om, md = _make_closer(unrealized_loss_close_bps=15) + with caplog.at_level(logging.INFO): + _manage(closer, md, 99.5) + line = next(r.message for r in caplog.records if '[close-reason]' in r.message) + assert f'reason={CLOSE_REASON_UNREALIZED_LOSS}' in line + assert _MAE_RE.search(line) is not None + + +class TestMaeOnCloseEventLine: + """Emission covers every close reason, including profitable ones.""" + + def test_emitted_on_maker_close(self, caplog): + closer, om, md = _make_closer() + _manage(closer, md, 99.7) + # Simulate a resting close order so cleanup classifies it as maker. + entry_time, _, tier = closer._open_positions['BTC'] + closer._open_positions['BTC'] = (entry_time, 12345, tier) + with caplog.at_level(logging.INFO): + closer.cleanup_closed('BTC') + line = next(r.message for r in caplog.records if '[close-reason]' in r.message) + assert f'reason={CLOSE_REASON_MAKER}' in line + m = _MAE_RE.search(line) + assert m is not None + assert float(m.group(1)) == pytest.approx(30.0) + + def test_emitted_on_external_close(self, caplog): + """External closes are part of the denominator, so they report too.""" + closer, om, md = _make_closer() + _manage(closer, md, 99.8) + # No resting close order -> cleanup classifies it as an external close. + entry_time, _, tier = closer._open_positions['BTC'] + closer._open_positions['BTC'] = (entry_time, None, tier) + with caplog.at_level(logging.INFO): + closer.on_position_closed('BTC') + line = next(r.message for r in caplog.records if '[close-reason]' in r.message) + assert f'reason={CLOSE_REASON_EXTERNAL}' in line + assert _MAE_RE.search(line) is not None + + def test_zero_mae_is_logged_not_omitted(self, caplog): + closer, om, md = _make_closer() + _manage(closer, md, 100.0) + with caplog.at_level(logging.INFO): + closer.on_position_closed('BTC') + line = next(r.message for r in caplog.records if '[close-reason]' in r.message) + assert 'mae_bps=0.0' in line + + def test_existing_log_regexes_still_match(self, caplog): + """The new fields append, so prior parsers keep working unchanged.""" + closer, om, md = _make_closer() + _manage(closer, md, 99.7) + with caplog.at_level(logging.INFO): + closer.on_position_closed('BTC') + line = next(r.message for r in caplog.records if '[close-reason]' in r.message) + base = _BASE_RE.search(line) + assert base is not None + assert base.group(1) == 'BTC' + assert base.group(4) == 'normal' + assert _MAX_AGE_RE.search(line) is not None + + +class TestMaeDisabled: + """The flag turns the fields off entirely.""" + + def test_no_fields_when_disabled(self, caplog): + closer, om, md = _make_closer(track_max_adverse_excursion=False) + _manage(closer, md, 99.0) + assert 'BTC' not in closer._mae + with caplog.at_level(logging.INFO): + closer.on_position_closed('BTC') + line = next(r.message for r in caplog.records if '[close-reason]' in r.message) + assert 'mae_bps' not in line + assert _BASE_RE.search(line) is not None + + +class TestMaeEdgeCases: + def test_zero_entry_price_skipped(self): + closer, om, md = _make_closer() + _set_mid(md, 99.0) + closer.manage('BTC', {'size': 1.0, 'entry_price': 0.0}, MagicMock()) + assert closer._mae['BTC'] == (0.0, 0.0) + + def test_cache_miss_skipped(self): + """No cached snapshot -> skip the sample rather than fetch one.""" + closer, om, md = _make_closer() + md.get_cached_market_data.return_value = None + closer.manage('BTC', {'size': 1.0, 'entry_price': 100.0}, MagicMock()) + assert closer._mae['BTC'] == (0.0, 0.0) + + def test_zero_mid_skipped(self): + closer, om, md = _make_closer() + _set_mid(md, 0.0) + closer.manage('BTC', {'size': 1.0, 'entry_price': 100.0}, MagicMock()) + assert closer._mae['BTC'] == (0.0, 0.0) + + def test_state_cleared_after_close(self): + closer, om, md = _make_closer() + _manage(closer, md, 99.0) + closer.on_position_closed('BTC') + assert 'BTC' not in closer._mae + + def test_reregistration_resets_peak(self): + """A new position must not inherit the previous one's peak.""" + closer, om, md = _make_closer() + _manage(closer, md, 99.0) # MAE 100 bps + closer.on_position_closed('BTC') + _manage(closer, md, 99.95) # new position, 5 bps + assert closer._mae['BTC'][0] == pytest.approx(5.0) + + def test_ghost_clear_drops_state(self): + """The ghost-position path pops MAE even though it logs no close event.""" + closer, om, md = _make_closer() + _manage(closer, md, 99.0) + closer._open_positions.pop('BTC', None) + closer._mae.pop('BTC', None) + assert 'BTC' not in closer._mae + + +class TestMaeAddsNoApiCalls: + """The enabled-by-default observation must not increase API load.""" + + def test_tracking_adds_no_fetching_calls(self): + """Turning tracking on must not change how often the fetching + accessors are used. + + ``get_market_data`` is called by the close-order placement path and + can fall through to a REST L2 fetch on a cache miss, so the question + is not whether it is called at all but whether MAE tracking adds + calls. Comparing on/off pins that down; asserting "never called" + would only pass by accident of the placement path's behaviour. + """ + counts = {} + for track in (True, False): + closer, om, md = _make_closer(track_max_adverse_excursion=track) + for mid in (99.9, 99.5, 99.7): + _manage(closer, md, mid) + counts[track] = ( + md.get_market_data.call_count, + md.get_l2_snapshot.call_count, + ) + assert counts[True] == counts[False] + + def test_sampling_uses_the_cache_only_accessor(self): + """The samples themselves come from the non-fetching accessor.""" + closer, om, md = _make_closer() + for mid in (99.9, 99.5, 99.7): + _manage(closer, md, mid) + assert md.get_cached_market_data.call_count == 3 + md.get_l2_snapshot.assert_not_called() + + +class TestCachedMarketDataAccessor: + """MarketDataManager.get_cached_market_data must not fetch.""" + + def test_returns_cached_without_fetch(self): + from market_data import MarketDataManager + + mgr = MarketDataManager.__new__(MarketDataManager) + mgr._cache = MagicMock() + sentinel = object() + mgr._cache.get.return_value = sentinel + mgr.get_l2_snapshot = MagicMock() + + assert mgr.get_cached_market_data('BTC') is sentinel + mgr.get_l2_snapshot.assert_not_called() + + def test_returns_none_on_miss_without_fetch(self): + from market_data import MarketDataManager + + mgr = MarketDataManager.__new__(MarketDataManager) + mgr._cache = MagicMock() + mgr._cache.get.return_value = None + mgr.get_l2_snapshot = MagicMock() + + assert mgr.get_cached_market_data('BTC') is None + mgr.get_l2_snapshot.assert_not_called() + + +class TestMaeConfigWiring: + """The flag flows from the config dict through MMConfig.""" + + def test_defaults_on(self): + from strategies.mm_config import MMConfig + + cfg = MMConfig.from_legacy_dict({}) + assert cfg.close.track_max_adverse_excursion is True + + def test_can_be_disabled(self): + from strategies.mm_config import MMConfig + + cfg = MMConfig.from_legacy_dict({'track_max_adverse_excursion': False}) + assert cfg.close.track_max_adverse_excursion is False + + +class TestMaeDoesNotChangeCloseBehaviour: + """Observation only: close decisions must be identical either way.""" + + def test_no_close_triggered_by_tracking_alone(self): + """A large excursion with the threshold disabled must not close.""" + closer, om, md = _make_closer(unrealized_loss_close_bps=0.0) + close_fn = MagicMock() + _set_mid(md, 95.0) # -500 bps + closer.manage('BTC', {'size': 1.0, 'entry_price': 100.0}, close_fn) + close_fn.assert_not_called() + assert closer._mae['BTC'][0] == pytest.approx(500.0) + + def test_force_close_age_unaffected(self): + """Tracking on/off must not shift the age-based force close.""" + results = [] + for track in (True, False): + closer, om, md = _make_closer( + track_max_adverse_excursion=track, max_age=60 + ) + close_fn = MagicMock() + _set_mid(md, 99.0) + closer.manage('BTC', {'size': 1.0, 'entry_price': 100.0}, close_fn) + entry_time, oid, tier = closer._open_positions['BTC'] + closer._open_positions['BTC'] = (entry_time - 100, oid, tier) + closer.manage('BTC', {'size': 1.0, 'entry_price': 100.0}, close_fn) + results.append('BTC' not in closer._open_positions) + assert results[0] == results[1] + + +class TestMaeSamplingIsLowerBound: + """Documented limitation, asserted so it is not mistaken for exactness.""" + + def test_intra_cycle_spike_is_missed(self): + """A spike between two manage() calls leaves no trace. + + MAE is therefore a lower bound: counts of positions breaching a + level are under-counted, which is why this data can support + withholding an early close but not enabling one. + """ + closer, om, md = _make_closer() + _manage(closer, md, 99.9) # -10 bps observed + # ... mid momentarily hits 95.0 here, between cycles, unobserved ... + _manage(closer, md, 99.9) # -10 bps observed again + assert closer._mae['BTC'][0] == pytest.approx(10.0) + + def test_sampling_follows_manage_cadence(self): + """One sample per manage() call -- no independent polling.""" + closer, om, md = _make_closer() + _manage(closer, md, 99.9) + assert md.get_cached_market_data.call_count == 1 + _manage(closer, md, 99.8) + assert md.get_cached_market_data.call_count == 2 + + +class TestMaeMultiCoinIsolation: + def test_peaks_tracked_per_coin(self): + closer, om, md = _make_closer() + om.get_all_positions.return_value = [ + {'coin': 'BTC', 'szi': '1.0'}, {'coin': 'ETH', 'szi': '1.0'}, + ] + _set_mid(md, 99.0) + closer.manage('BTC', {'size': 1.0, 'entry_price': 100.0}, MagicMock()) + _set_mid(md, 99.8) + closer.manage('ETH', {'size': 1.0, 'entry_price': 100.0}, MagicMock()) + assert closer._mae['BTC'][0] == pytest.approx(100.0) + assert closer._mae['ETH'][0] == pytest.approx(20.0) + + +class TestMaeThreadSafetyShape: + """The state is a plain dict of immutable tuples (GIL-protected writes).""" + + def test_entries_are_immutable_tuples(self): + closer, om, md = _make_closer() + _manage(closer, md, 99.5) + value = closer._mae['BTC'] + assert isinstance(value, tuple) + assert len(value) == 2 + assert all(isinstance(v, float) for v in value) + + +def test_monotonic_clock_used_for_age(): + """mae_age is derived from the same monotonic clock as position age.""" + closer, om, md = _make_closer() + before = time.monotonic() + _manage(closer, md, 99.5) + _, mae_age = closer._mae['BTC'] + assert 0 <= mae_age <= time.monotonic() - before + 1 diff --git a/tests/test_reduce_only_collision.py b/tests/test_reduce_only_collision.py index 9df0779..c264563 100644 --- a/tests/test_reduce_only_collision.py +++ b/tests/test_reduce_only_collision.py @@ -17,6 +17,9 @@ def _make_closer(**kwargs): """Create a PositionCloser with mocked dependencies.""" om = MagicMock() md = MagicMock() + # Cache miss by default: manage() samples market data unconditionally + # for max-adverse-excursion tracking, which is observation-only. + md.get_cached_market_data.return_value = None # Default: get_all_positions returns a position so force-close # verification passes (existing tests assume position exists). om.get_all_positions.return_value = [{'coin': 'SP500', 'szi': '0.5'}] diff --git a/tests/test_unrealized_loss_close.py b/tests/test_unrealized_loss_close.py index e8e4e8a..614eb0f 100644 --- a/tests/test_unrealized_loss_close.py +++ b/tests/test_unrealized_loss_close.py @@ -27,6 +27,9 @@ def _make_closer( """Build a PositionCloser with mocked dependencies.""" om = MagicMock() md = MagicMock() + # Cache miss by default: manage() samples market data unconditionally + # for max-adverse-excursion tracking, which is observation-only. + md.get_cached_market_data.return_value = None md.round_size.return_value = 0.5 md.get_sz_decimals.return_value = 0 md.price_rounding_params.return_value = (0, True) diff --git a/tests/test_volatility_position_age.py b/tests/test_volatility_position_age.py index 4d471aa..217288f 100644 --- a/tests/test_volatility_position_age.py +++ b/tests/test_volatility_position_age.py @@ -32,6 +32,9 @@ def _make_closer(max_age: float = 120, maker_only: bool = False, taker_fallback: float = None, spread_bps: float = 10) -> tuple: om = MagicMock() md = MagicMock() + # Cache miss by default: manage() samples market data unconditionally + # for max-adverse-excursion tracking, which is observation-only. + md.get_cached_market_data.return_value = None md.round_size.return_value = 0.5 md.get_sz_decimals.return_value = 0 md.price_rounding_params.return_value = (0, True) From 73adc5a281ba7e547e336429cbfb4f97689f26a6 Mon Sep 17 00:00:00 2001 From: keitaj Date: Wed, 14 Oct 2026 08:03:45 +0900 Subject: [PATCH 2/2] fix: address review findings on max adverse excursion logging Two findings from the review on #171, both in the integrity of the measurement rather than its behaviour. 1. Floor the excursion at 0.0 inside `_update_mae` instead of relying on the registration seed. The `prev is None` branch stored the sample unconditionally, so a position in profit could record a negative "adverse" excursion -- verified reachable, producing (-50.0, 5.0) for a long whose mid sat above entry. The branch is live in production because `_record_close` pops the entry and can fire on the WS thread while a `manage()` call is already past the registration check. Both the docstring and the README assert the 0.0 floor; now the code guarantees it rather than inheriting it from a separate code path. 2. Replace the tautological ghost-clear test. It popped `_mae` itself and then asserted the pop had happened, so it passed regardless of production behaviour -- and it was the only test that appeared to cover the three `_mae.pop()` calls at sites that do not go through `_record_close`. All three were in fact untested; test_ghost_position_fix has no `_mae` references. Replaced with three tests that drive the real paths (repeated placement failure, and the two flat-position races before force close and before placement), plus three for the floor. Each new assertion was mutation-tested: removing the corresponding production line, or reverting the floor to the previous form, fails the matching test. Also states explicitly in the README that this parameter has no CLI flag, since every other entry in that block carries a flag reference and the omission otherwise reads as an oversight. Co-Authored-By: Claude Opus 5 --- README.md | 2 +- strategies/mm_position_closer.py | 13 ++++- tests/test_position_mae.py | 98 ++++++++++++++++++++++++++++++-- 3 files changed, 106 insertions(+), 7 deletions(-) diff --git a/README.md b/README.md index 1524357..c474d38 100644 --- a/README.md +++ b/README.md @@ -617,7 +617,7 @@ strategies: close_tier_toxicity_min_fills: 5 # --close-tier-toxicity-min-fills (min fills in window required to fire) close_tier_toxicity_floor_pct: 0.15 # --close-tier-toxicity-floor-pct (breakeven fraction floor after multiplier) unrealized_loss_close_bps: 0 # --unrealized-loss-close-bps (early taker close when unrealized loss exceeds this bps; 0 = disabled) - track_max_adverse_excursion: true # (log mae_bps/mae_age on the close-event line; observation only, no extra API calls) + track_max_adverse_excursion: true # (no CLI flag; JSON config only) log mae_bps/mae_age on the close-event line; observation only, no extra API calls bbo_mode: false # --bbo-mode (place orders at best bid/ask instead of mid ± spread) bbo_offset_bps: 0 # --bbo-offset-bps (bps behind BBO; 0 = at BBO) inventory_skew_bps: 0 # --inventory-skew-bps (NO-OP in the current single-sided flow: a coin holding a position does not re-quote, so the skew never reaches an order; warns at startup if non-zero) diff --git a/strategies/mm_position_closer.py b/strategies/mm_position_closer.py index 0a5ce63..8008217 100644 --- a/strategies/mm_position_closer.py +++ b/strategies/mm_position_closer.py @@ -576,7 +576,10 @@ def _update_mae(self, coin: str, size: float, entry_price: float, age: float) -> than fetched. ``mae_bps`` uses the same sign convention as the unrealized-loss - early close: positive means the position has moved against us. + early close: positive means the position has moved against us. The + value is floored at 0.0 rather than going negative when a position + only ever moved in our favour, so the floor does not depend on the + seeding path having run. Sampling is bounded by the main-loop interval, so the recorded value is a *lower bound* on the true excursion — an intra-interval @@ -594,7 +597,13 @@ def _update_mae(self, coin: str, size: float, entry_price: float, age: float) -> else: # short: adverse when the mid rises above entry adverse_bps = (md.mid_price - entry_price) / entry_price * 10_000 prev = self._mae.get(coin) - if prev is None or adverse_bps > prev[0]: + # Floor at 0.0 so a position that only moved in our favour cannot + # record a negative "adverse" excursion. Relying on the registration + # seed for this would break whenever _update_mae runs without it -- + # _record_close pops the entry and can fire on the WS thread while a + # manage() call is already past the registration check. + floor = prev[0] if prev is not None else 0.0 + if adverse_bps > floor: self._mae[coin] = (adverse_bps, age) def set_adverse_tracker(self, tracker: Any) -> None: diff --git a/tests/test_position_mae.py b/tests/test_position_mae.py index 3da61fe..4d4eab1 100644 --- a/tests/test_position_mae.py +++ b/tests/test_position_mae.py @@ -250,15 +250,105 @@ def test_reregistration_resets_peak(self): _manage(closer, md, 99.95) # new position, 5 bps assert closer._mae['BTC'][0] == pytest.approx(5.0) - def test_ghost_clear_drops_state(self): - """The ghost-position path pops MAE even though it logs no close event.""" + +class TestMaeClearedOnNonCloseExits: + """The three paths that drop a tracked position without logging a close + event must also drop its peak, or it would be carried into the next + position on that coin. + + These exercise the real code paths rather than mutating ``_mae`` + directly -- a test that pops the entry itself would pass even if the + production call were removed. + """ + + def test_ghost_position_clear(self): + """Repeated close-order placement failures clear the stale peak.""" closer, om, md = _make_closer() + om.create_limit_order.return_value = None # placement always fails + position = {'size': 1.0, 'entry_price': 100.0} + _set_mid(md, 99.0) + + for _ in range(5): # _GHOST_CLEAR_THRESHOLD + if 'BTC' in closer._open_positions: + entry_time = closer._open_positions['BTC'][0] + closer._open_positions['BTC'] = (entry_time, None, 0) + closer.manage('BTC', position, MagicMock()) + + assert 'BTC' not in closer._open_positions + assert 'BTC' not in closer._mae + + def test_already_closed_before_force_close(self, caplog): + """The pre-force-close flat-position check clears the peak.""" + closer, om, md = _make_closer(max_age=60) _manage(closer, md, 99.0) - closer._open_positions.pop('BTC', None) - closer._mae.pop('BTC', None) + assert closer._mae['BTC'][0] == pytest.approx(100.0) + + # Age past max_age, and let the exchange report the position as gone. + entry_time, oid, tier = closer._open_positions['BTC'] + closer._open_positions['BTC'] = (entry_time - 120, oid, tier) + om.get_all_positions.return_value = [{'coin': 'BTC', 'szi': '0'}] + + with caplog.at_level(logging.INFO): + closer.manage('BTC', {'size': 1.0, 'entry_price': 100.0}, MagicMock()) + + assert 'BTC' not in closer._mae + # No close event is logged on this path, so nothing should be emitted. + assert not [r for r in caplog.records if '[close-reason]' in r.message] + + def test_already_closed_before_placement(self): + """The pre-placement flat-position check clears the peak.""" + closer, om, md = _make_closer() + _manage(closer, md, 99.0) + assert 'BTC' in closer._mae + + # Force a re-placement attempt against a position the exchange says + # is already flat. + entry_time, _, tier = closer._open_positions['BTC'] + closer._open_positions['BTC'] = (entry_time, None, tier) + om.get_all_positions.return_value = [{'coin': 'BTC', 'szi': '0'}] + + closer.manage('BTC', {'size': 1.0, 'entry_price': 100.0}, MagicMock()) + + assert 'BTC' not in closer._open_positions assert 'BTC' not in closer._mae +class TestMaeFloor: + """The reported value never goes negative, independent of seeding.""" + + def test_profit_only_position_stays_at_zero(self): + closer, om, md = _make_closer() + _manage(closer, md, 100.8) + assert closer._mae['BTC'] == (0.0, 0.0) + + def test_floor_holds_without_the_registration_seed(self): + """_update_mae must floor on its own, not rely on the seed. + + _record_close pops the entry and can run on the WS thread while a + manage() call is already past the registration check, so the + unseeded branch is reachable in production. + """ + closer, om, md = _make_closer() + closer._open_positions['BTC'] = (time.monotonic(), None, 0) + closer._mae.pop('BTC', None) + _set_mid(md, 100.5) # long in profit + + closer._update_mae('BTC', 1.0, 100.0, 5.0) + + assert 'BTC' not in closer._mae # nothing worse than the 0.0 floor + + def test_adverse_sample_still_recorded_without_seed(self): + """The floor must not swallow a genuine adverse sample.""" + closer, om, md = _make_closer() + closer._open_positions['BTC'] = (time.monotonic(), None, 0) + closer._mae.pop('BTC', None) + _set_mid(md, 99.0) + + closer._update_mae('BTC', 1.0, 100.0, 5.0) + + assert closer._mae['BTC'][0] == pytest.approx(100.0) + + class TestMaeAddsNoApiCalls: """The enabled-by-default observation must not increase API load."""