refactor: Group MM config into dataclasses (Phase 1)

[keitaj]

Summary

• Introduce strategies/mm_config.py with four grouped dataclasses (LossStreakConfig, MicropriceConfig, VelocityGuardConfig, PerCoinOverrides) plus a MMConfig.from_legacy_dict() bridge for the existing flat strategy_config dict.
• MarketMakingStrategy.__init__ now builds self.cfg = MMConfig.from_legacy_dict(config) and assigns the existing flat instance attributes from self.cfg. Internal references and external tests are unchanged.
• Three values previously read via config.get() but never exposed via CLI/env (inventory_skew_cap, fill_rate_log_interval, dynamic_age_log_interval) become module constants in mm_config; defaults still flow through config.get() so production behavior is identical.

Why

The MM strategy __init__ currently calls config.get(...) more than 50 times for a flat parameter surface, and the related parameters are scattered across the file with no semantic grouping. This is the first step in a multi-phase refactor: it adds the dataclass infrastructure without changing CLI/env or breaking any callers, so subsequent phases can migrate internal references and fold additional groups (close, dynamic offset, dynamic age, imbalance, schedule) one at a time.

Changes

• strategies/mm_config.py (new): four config dataclasses, MMConfig root, from_legacy_dict constructor, parse_coin_overrides helper, and module constants for previously hidden values.
• strategies/market_making_strategy.py: import the new module; build self.cfg in __init__; assign the four groups' existing flat instance attributes from self.cfg; replace inline default literals with the module constants where applicable.
• tests/test_mm_config.py (new): 22 unit tests covering parser edge cases, dataclass defaults, validation, from_legacy_dict end-to-end, and module constant values.

Backward compatibility

• CLI flags and environment variables: unchanged.
• The existing flat instance attributes (self.loss_streak_limit, self._microprice_enabled, self._coin_offset_overrides, etc.) are preserved as aliases of self.cfg.<group>.<field> so call-sites and tests that bypass __init__ keep working.
• Validation error messages preserved for loss_streak_limit / loss_streak_cooldown so existing regex-based test assertions continue to match.

Test plan

• Unit tests added (tests/test_mm_config.py, 22 cases).
• flake8 passes.
• pytest passes (832 tests total, including the new 22).
• No regressions in existing tests.

🤖 Generated with Claude Code

[keitaj]

Review: refactor: Group MM config into dataclasses (Phase 1)

Solid Phase 1 scaffolding — dataclasses are well-scoped, validation moved cleanly into __post_init__, and the alias pattern keeps the call surface stable. Two should-fix items around the parser duplication and the unused velocity field, plus a couple of nits.

---

1. Duplicate coin-override parser with diverged behavior (should-fix)

strategies/mm_config.py L33-58 vs strategies/market_making_strategy.py L793-810:

After this PR, __init__ no longer calls MarketMakingStrategy._parse_coin_overrides — it goes through MMConfig.from_legacy_dict → mm_config.parse_coin_overrides. The two parsers do almost the same thing, but the new helper silently skips invalid inputs while the static method on the strategy logs [mm] Invalid coin override format / Invalid BPS value warnings. The static method is now only reachable via tests, so in production a typo in COIN_OFFSET_OVERRIDES will no longer surface a warning.

Suggestion: Either delegate the strategy method to the helper and add lightweight warning logging in mm_config.parse_coin_overrides, or remove the static method now that it has no internal callers (and update tests/test_per_coin_spread.py to import parse_coin_overrides directly). Keeping two parsers with diverged observability is the worst of both worlds.

---

2. VelocityGuardConfig populated but unused (should-fix)

strategies/mm_config.py L91-99 and from_legacy_dict L137-141:

cfg.velocity is built from the flat dict, but neither MarketMakingStrategy nor bot.py reads it — bot.py still calls self.strategy_config.get(velocity_guard_enabled, ...) etc. directly when wiring BboVelocityGuard. Including a populated-but-unread group in Phase 1 is harmless but creates a dead path that could mislead future readers (e.g. someone editing cfg.velocity.consecutive and wondering why nothing changes).

Suggestion: Either (a) wire bot.py to read from strategy.cfg.velocity in this PR, or (b) drop velocity from MMConfig for Phase 1 and add it back when bot.py is migrated. Option (a) is a small additional change; option (b) is more conservative. Whichever you pick, the docstring on VelocityGuardConfig should be updated — currently it says "Consumed by bot.py" which isnt actually true after this PR.

---

3. Missing test for non-coercible input (nit)

tests/test_mm_config.py:

TestMMConfigFromLegacyDict.test_string_to_int_coercion covers numeric strings, but theres no negative case for non-coercible input. e.g. MMConfig.from_legacy_dict({loss_streak_limit: abc}) would raise ValueError from the int(...) cast, but the contract isnt pinned down by a test.

Suggestion: Add one assertion for a non-coercible value to lock in the failure mode.

---

4. Constants test couples revisions to defaults (nit)

tests/test_mm_config.py L143-162 (TestModuleConstants):

Asserting that INVENTORY_SKEW_CAP == 3.0 etc. is essentially restating the source. It will catch accidental edits but adds noise on every intentional default change. Not a blocker — the test is cheap.

Suggestion: Optional. Consider replacing with assertions on the type and positive-finite property instead, e.g. assert isinstance(INVENTORY_SKEW_CAP, float) and INVENTORY_SKEW_CAP > 0.

---

Verdict: Changes requested

Two should-fix items (parser duplication, unused velocity group). Both are quick to address; once resolved the PR is in good shape to merge as the foundation for subsequent phases.

[keitaj]

Re-review (commit a0d8a81)

All four items from the previous review are addressed cleanly. No new issues introduced.

#	Item	Status
1	Parser duplication	✅ mm_config.parse_coin_overrides now owns the warning logging; strategy method is a thin delegate. Single source of truth restored.
2	VelocityGuardConfig unused	✅ bot.py now reads strategy.cfg.velocity.* with a flat-dict fallback for non-MM strategies. Docstring matches actual usage.
3	Missing non-coercible test	✅ Added test_non_coercible_int_raises / test_non_coercible_float_raises.
4	Brittle constants test	✅ Now checks isinstance(float) + > 0.

The fallback branch in bot.py is technically dead under current _STRATEGY_PARAMS (velocity flags are MM-only), but keeping it is sensible defensive programming since the cfg attribute is strategy-specific.

Verdict: LGTM

Phase 1 scaffolding is solid and ready to merge as the foundation for subsequent phases.