Skip to content

feat: add JSON config file as a layered input source#148

Merged
keitaj merged 1 commit into
mainfrom
feat/json-config-layer
May 4, 2026
Merged

feat: add JSON config file as a layered input source#148
keitaj merged 1 commit into
mainfrom
feat/json-config-layer

Conversation

@keitaj
Copy link
Copy Markdown
Owner

@keitaj keitaj commented May 4, 2026

Summary

  • Add --config <path.json> (repeatable) and $BOT_CONFIG env var as a new input layer that slots between dataclass defaults and CLI/env overrides. Layering precedence: CLI / env > JSON > dataclass defaults.
  • JSON is opt-in: with no flag and no env var the loader is never invoked, behaviour is identical to prior releases.
  • Files accept either flat or nested form (or mix). Nested form auto-flattens by underscore-joining the path under known namespaces (market_making, risk), so downstream code is unchanged.

Why

Adding the Forager feature exposed a long-standing issue: a single new strategy parameter touches roughly a dozen files (CLI flag, default-config dict, _STRATEGY_PARAMS registration, validator, .env.farming, three start scripts, oplog.sh, README in two places). The boilerplate had grown to roughly the same size as the feature itself. This PR begins the migration toward a declarative JSON config that can express the same data in a single, version-controllable file — without breaking any existing surface.

Designed in docs/design-doc/20260504_json_config_layer.md.

Changes

  • json_config_loader.py (new): public load_json_configs(paths, strategy_name, known_keys) + ConfigError. Standard library only (json, pathlib). Walks nested dicts under known namespaces and emits flat keys; unknown namespaces are kept with a prefix so the validator can warn.
  • bot.py: adds --config argparse (repeatable), env-var fallback ($BOT_CONFIG), and a json_overrides kwarg to HyperliquidBot.__init__ that slots into the existing merge step. CLI / env paths flow exactly as before.
  • validation/strategy_validator.py: exposes known_market_making_keys() for typo detection.
  • examples/config.example.json: full nested template covering Forager, Refresh tolerance, auto-exclude, dynamic-offset / dynamic-age, schedule, microprice, velocity, per-coin overrides.
  • README.md: new ### JSON config layer section under ## Trading Strategies describing precedence, the nested form, fail-safe behaviours, and pointer to the example.

Test plan

  • Unit tests added: 33 new cases across tests/test_json_config_loader.py (21) and tests/test_bot_config_layering.py (12)
  • Covers flat / nested / mixed forms; three-file layering; missing-file fail-safe (warn-and-skip); malformed-JSON abort path; typo warnings; underscore-concat path semantics
  • Pins CLI > JSON > defaults precedence end-to-end (including the realistic case where CLI flips a feature off that JSON had enabled)
  • flake8 . --max-line-length=120 passes
  • pytest tests/ -q passes (1039 tests, 0 failures)
  • No regressions in existing tests
  • No external dependencies added

🤖 Generated with Claude Code

Add ``--config <path.json>`` (repeatable) and ``$BOT_CONFIG`` env var
as a new input layer between dataclass defaults and CLI/env overrides.
Layering precedence:

    CLI / env  >  JSON  >  dataclass defaults

JSON is opt-in: with no flag and no env var the loader is never
invoked, behaviour matches prior releases exactly. Backward compatible
all the way down — every existing CLI flag, env var, ``.env.farming``
entry, ``start_farming*.sh`` argument, and ``oplog.sh`` log keep
working.

The loader (``json_config_loader.py``) accepts both flat and nested
forms in the same file. Nested form auto-flattens by
underscore-joining the path under known namespaces (``market_making``,
``risk``):

    {"market_making": {"refresh": {"tolerance_bp": 1}}}
        →  {"refresh_tolerance_bp": 1}

so downstream code (``bot.py``, ``MMConfig.from_legacy_dict``) is
unchanged. The flat namespace is the source of truth.

* New module ``json_config_loader.py`` with public ``load_json_configs``
  and ``ConfigError``. Standard library only (``json``, ``pathlib``).
* ``bot.py`` adds ``--config`` argparse, env-var fallback, and a
  ``json_overrides`` kwarg to ``HyperliquidBot.__init__`` that slots
  into the existing config-merge step.
* ``validation/strategy_validator.py`` exposes
  ``known_market_making_keys()`` for typo detection. Unknown keys
  produce a warning but still flow through to the validator.
* ``examples/config.example.json`` ships a full nested template.
* ``README.md`` documents the new flag, env var, precedence, and
  format.
* New tests:
  - ``tests/test_json_config_loader.py`` (21 cases) covers flat /
    nested / mixed forms, three-file layering, missing-file fail-safe,
    malformed-JSON ``SystemExit``, typo warnings, and the internal
    underscore-concat semantic.
  - ``tests/test_bot_config_layering.py`` (12 cases) pins
    CLI > JSON > defaults precedence end-to-end.

Designed in ``docs/design-doc/20260504_json_config_layer.md``. Targets
the CLI flag bloat that became acute around the Forager rollout
(1 feature → 12 file changes); future features can declare their
parameters once in the dataclass and ship a JSON example, dropping
several boilerplate touchpoints over time.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Copy link
Copy Markdown
Owner Author

@keitaj keitaj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review: JSON config file as a layered input source

The implementation is clean — minimal stdlib-only loader, opt-in entry path, comprehensive failure-mode tests, and proper layering precedence. One should-fix on a real gap between the example config and what's actually wired up, plus a few nits.


1. risk namespace in JSON is silently ignored — example config misleads (should-fix)

examples/config.example.json and README.md both advertise a risk namespace:

{ "risk": { "daily_loss_limit": 200 } }

The loader correctly flattens this into {"daily_loss_limit": 200}, but that key only flows into strategy_config (the merged dict consumed by MarketMakingStrategy). Risk parameters in bot.py come from a separate code path (bot.py L1374-1385) that reads args.{param} directly and assigns to the Config class:

for param in _RISK_PARAMS:
    val = getattr(args, param, None)
    if val is not None:
        setattr(Config, param.upper(), val)

So daily_loss_limit: 200 from JSON ends up as a no-op key in strategy_configRiskManager never sees it. Worse, known_market_making_keys() doesn't include risk params, so the typo detector also flags it as an "Unknown key" while the value is silently ignored. Operators would reasonably expect their JSON risk block to take effect.

Two options to resolve:

  • (a) Document the gap — strip risk from _KNOWN_NAMESPACES and the example, add a README note that risk params are CLI/env-only in this Phase. Smaller, safer for this PR.
  • (b) Wire it through — after loading JSON, iterate the flattened _RISK_PARAMS keys and set Config.{KEY.upper()} if absent on args. Requires careful precedence handling (args.{param} should still beat JSON).

Option (a) is enough for this PR; option (b) is a natural follow-up once Phase 4 (VPS test rollout) starts. Either way, the current state where the example shows a feature that doesn't work is the issue to fix.


2. strategy_name argument to _to_flat is unused (nit)

json_config_loader.py L102:

def _to_flat(data: Dict[str, Any], strategy_name: str) -> Dict[str, Any]:
    ...
    for top_key, top_value in data.items():
        if top_key in _KNOWN_NAMESPACES and isinstance(top_value, dict):

The function never references strategy_name; namespace recognition uses the hardcoded _KNOWN_NAMESPACES tuple. Future-extensibility intent is reasonable (other strategies could add their own JSON namespace), but until then the argument is dead.

Suggestion: drop it from the signature or document why it's there:

def _to_flat(data: Dict[str, Any], strategy_name: str) -> Dict[str, Any]:
    """...
    Parameters
    ----------
    strategy_name :
        Reserved for future per-strategy namespace recognition. Currently
        unused; ``_KNOWN_NAMESPACES`` is the source of truth.
    """

3. Hardcoded "extras" set in known_market_making_keys() will drift (nit)

validation/strategy_validator.py L329-339:

The set of keys not in _STRATEGY_PARAMS but read via config.get in MarketMakingStrategy (close_immediately, maker_only, max_positions, etc.) is hand-maintained. Future commits adding similar opt-in flags could miss this list.

Two improvements possible:

  • Extract the special-case keys from a single declarative list shared with bot.py's _collect_params boolean handling.
  • Or: instead of a closed set, derive known_keys from a dataclass-driven introspection (MMConfig walks the field tree). Out-of-scope for this PR but worth a TODO comment.

The current state is fine for typo-detection — false-positives aren't critical — but the maintenance burden is real.


4. Lazy import is fragile in theory (nit)

validation/strategy_validator.py L321:

def known_market_making_keys() -> set:
    from bot import _STRATEGY_PARAMS, _COMMON_PARAMS

The lazy import dodges a circular dependency (bot.py imports from this module at top level). It works because known_market_making_keys() is only invoked from bot.py's main() after module load completes. A future refactor that calls it at module load (e.g., a validator.__init__ cache) would explode with ImportError.

Suggestion: add a guard comment that documents the constraint, or move _STRATEGY_PARAMS / _COMMON_PARAMS to a separate module that both bot.py and validation/strategy_validator.py can import without cycling.


Verdict: LGTM

No blockers. One should-fix on the risk namespace gap (the implementation says yes but the runtime says no). Three nits on cleanliness / future-proofing. The should-fix is easily addressable in a follow-up — option (a) is a 5-minute documentation fix and option (b) is a small, well-scoped extension. Both can land after the merge as a small PR rather than blocking this one.

🤖 Generated with Claude Code

@keitaj keitaj merged commit c5813c4 into main May 4, 2026
6 checks passed
@keitaj keitaj deleted the feat/json-config-layer branch May 4, 2026 02:12
keitaj added a commit that referenced this pull request May 4, 2026
…149)

The previous JSON config layer (#148) flattened ``{"risk": {...}}`` into
flat keys correctly, but those values only flowed into ``strategy_config``
— the strategy-level merged dict — not into the ``Config`` class that
``RiskManager`` actually reads from. Operators putting
``"daily_loss_limit": 200`` in their JSON would see no effect on the
risk guardrails.

This change closes that gap:

* Promote ``_RISK_PARAMS`` from ``main()`` to module level so the JSON
  helper and the CLI override loop share the list.
* Add ``_apply_json_risk_overrides(json_overrides, args)``: for each
  risk param present in JSON and *not* already supplied via CLI args,
  write the value to ``Config.{KEY.upper()}``. Pop the key from
  ``json_overrides`` so it does not leak into ``strategy_config``
  (which the strategy would not know what to do with) or trigger the
  typo detector.
* Call the helper right after ``load_json_configs`` and before the
  existing CLI risk loop. CLI > JSON precedence is preserved because
  the CLI loop runs last and unconditionally writes ``args.{param}``
  when present.
* ``known_market_making_keys()`` now includes ``_RISK_PARAMS`` so the
  JSON loader's typo detector treats risk keys as known when they
  appear at flat-top-level (e.g., a user mixing flat + nested forms).
* README clarifies that the ``risk`` namespace targets ``Config``,
  not ``strategy_config``, and that CLI flags still beat JSON.

5 new test cases pin the behaviour: JSON sets Config when CLI is
unset; CLI beats JSON; multiple risk params apply together; empty /
None inputs are no-ops; unknown risk-shaped keys pass through
untouched. Tests snapshot and restore the relevant ``Config`` class
attributes so they stay isolated from each other and from the wider
suite.

Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant