Skip to content

refactor(engine): split giant fill functions and consolidate one-shot provenance flags into an explicit state model #227

Description

@luisleo526

Problem

The broker/fill core has grown by accretion: every TradingView quirk lands as another one-shot boolean on BacktestEngine plus a branch inside an already huge function. Measured at 95aeb9c:

Metric Value
src/engine_fills.cpp length 9,084 lines
BacktestEngine::apply_filled_order_to_state (single function) ~1,915 lines
BacktestEngine::sort_orders_by_fill_phase ~685 lines
BacktestEngine::process_margin_call ~543 lines
include/pineforge/engine.hpp class BacktestEngine (from line 1101) ~4,100 header lines, ~208 member fields

Examples of the flag pattern (all in engine.hpp, @broker-state block):

  • opening_affordability_pending_ / opening_affordability_eligible_
  • commissioned_all_in_market_long_opening_affordability_
  • opening_affordability_default_long_reversal_
  • close_then_short_opening_requires_adverse_retry_

Each is documented by a 5–15 line comment describing a narrow admission shape and which other flags it must not be combined with. The comments are excellent, but they are a symptom: the invariants live in prose, not in types.

Why it matters

  • Any refactor touching fills has a high regression surface; the test suite protects behavior but not a model, so it cannot tell a maintainer which combination of flags is legal.
  • Onboarding cost for external contributors is prohibitive, which undercuts the Apache-2.0 positioning.
  • The broker_state_hash coverage check already treats these fields as a flat bag, which will keep growing.

Proposal

  1. Split apply_filled_order_to_state by fill phase (entry open / add / partial close / full close / reversal / margin-call) into named member functions with a small FillContext struct passed through, keeping behavior bit-identical and verifying with the existing ctest + corpus sweep.
  2. Consolidate the affordability provenance flags into one OpeningAffordabilityEvent struct (or a small enum-tagged variant) with an explicit lifecycle: queued → eligible → consumed. Illegal combinations become unrepresentable.
  3. Add a short docs/pages/fill-model.md (or extend lifecycle.md) with a phase diagram, so the state model has a canonical description outside the header comments.
  4. Do this in behavior-preserving steps, one PR per phase, each gated by scripts/run_corpus.sh with zero CSV drift.

Acceptance

  • No function in src/engine_*.cpp exceeds ~400 lines.
  • All affordability-related booleans replaced by a single struct with documented transitions.
  • ctest and the full corpus sweep unchanged (4,189/4,190 CSV byte-identical baseline as in README round 39).

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    enhancementNew feature or request

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions