Skip to content

fix: promote _STRATEGY_PARAMS / _COMMON_PARAMS to module level#150

Merged
keitaj merged 1 commit into
mainfrom
fix/strategy-params-module-level
May 4, 2026
Merged

fix: promote _STRATEGY_PARAMS / _COMMON_PARAMS to module level#150
keitaj merged 1 commit into
mainfrom
fix/strategy-params-module-level

Conversation

@keitaj
Copy link
Copy Markdown
Owner

@keitaj keitaj commented May 4, 2026

Summary

  • PR fix: wire JSON risk namespace through to Config (close PR #148 gap) #149 left _STRATEGY_PARAMS and _COMMON_PARAMS as locals inside main(), so validation.strategy_validator.known_market_making_keys() raised ImportError whenever the JSON-config typo detector ran (i.e. on every --config invocation).
  • This PR promotes both lists to module scope (right after _RISK_PARAMS) and adds a regression test class that pins the contract.

Why this matters

Without this fix the migration tool added in the farming repo (scripts/env_to_json.py) would generate a working JSON config that crashes the bot the first time it tries to enumerate known MM keys. Only the typo-warning path was affected; without --config (i.e. the existing CLI/env path) the bug was latent.

Changes

  • bot.py — move _COMMON_PARAMS and _STRATEGY_PARAMS out of main() to module level. main() now carries a short comment pointing at the module-level definitions; call-sites for _collect_params / argparse.Namespace are unchanged.
  • tests/test_strategy_validator.py — new TestKnownMarketMakingKeys class with 6 regression tests that exercise the function and assert the three name lists are importable from bot at module scope.

Test plan

  • flake8 . --max-line-length=120 passes
  • pytest tests/ -q passes (1050 / 1050)
  • End-to-end round-trip verified: scripts/env_to_json.py .env.farming | bot's load_json_configs returns 68 keys with no ImportError and no spurious typo warnings
  • No CLI/env regression — existing CLI invocation path is byte-identical

🤖 Generated with Claude Code

PR #149 promoted ``_RISK_PARAMS`` to module scope so
``validation.strategy_validator.known_market_making_keys`` could
introspect risk-guardrail names lazily. The other two lists
(``_STRATEGY_PARAMS``, ``_COMMON_PARAMS``) were left as locals inside
``main()``, so the lazy import in ``known_market_making_keys`` would
raise ``ImportError`` at runtime as soon as the JSON config typo
detector tried to enumerate known MM keys.

This PR moves both lists out of ``main()`` to module scope right
after ``_RISK_PARAMS``. ``main()`` keeps a short comment pointing
readers at the new location; the surrounding ``_collect_params`` /
``argparse.Namespace`` call-sites are unchanged.

A regression test class is added to ``test_strategy_validator.py`` —
it pins the contract that all three name lists remain at module
level (any future demotion will fail the import-style assertions).
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: promote _STRATEGY_PARAMS / _COMMON_PARAMS to module level

Tight, surgical fix for the latent ImportError in known_market_making_keys that PR #149 left behind. Pure code-motion (delete locals, add module-level definitions) plus a regression-test class that pins the contract from both sides — direct import and through the public function. End-to-end round-trip with the migration tool was verified before opening the PR. No risk-guardrail or order-logic changes.


1. Comment wording in main() ─ slight mismatch (nit)

bot.py L1388-1393:

The comment says "the local references below preserve the existing call-site shape of _collect_params" but after the move the call sites just dereference module globals — there are no "local references" anymore. Minor wording inconsistency that could mislead a future reader into thinking shadowing is still in play.

Suggestion:

# ``_COMMON_PARAMS`` and ``_STRATEGY_PARAMS`` are defined at module
# level (top of this file) so the JSON config layer's typo detector
# (``validation.strategy_validator.known_market_making_keys``) can
# introspect them without circular-import gymnastics. The call sites
# below reference the module-level definitions directly.

Non-blocking.


2. Consider an end-to-end typo-warning test (nit)

tests/test_strategy_validator.py:

The new TestKnownMarketMakingKeys covers the symptom (function returns / direct import). One additional integration-style test that drives load_json_configs with a deliberate typo and asserts the warning fires would catch the whole failure mode (loader → validator → bot module) in a single check. Nice-to-have, not required — the current six tests already prevent the specific regression that motivated this PR.


Verdict: LGTM

No blockers, no should-fix. Two nits at most. The contract-pinning tests in TestKnownMarketMakingKeys::test_bot_module_exports_param_lists are exactly what was needed — the next time someone is tempted to re-localise these lists, CI will stop them.

@keitaj keitaj merged commit e47d2ae into main May 4, 2026
6 checks passed
@keitaj keitaj deleted the fix/strategy-params-module-level branch May 4, 2026 02:47
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