Files
sentiment-engine/prod/docs/EXEC_BEST_PRACTICE_AUDIT_20260714.md
Codex a7cf3f5976 docs(exec): adversarial best-practice audit — web-grounded + best-of-breed internals
Weird-pass review of pink_direct.py + exec_router.py + unified spec vs industry practice.
Ranked findings, each grounded (cited sources + nautilus/FIX/OMS internals):
- H1 float money math (PINK) — against consensus; ALREADY fixed by rebuild's Decimal contract
- H2 crash-recovery state in /tmp — durable+atomic path needed
- H3 shared VST account + symbol-membership ownership — sub-account isolation is the fix
- H4 clientOrderId must be unique PER ATTEMPT — gap in my contract.py (raw request_id seed)
- H5 WS-primary without explicit seqnum gap detection — add forced REST resync
- M1 bare except-pass swallows; M2 dead-man-stop orphan hard-gate; M3 L673 sign bug
Plus a balance section: where PINK/spec are AT/ABOVE best practice (don't 'fix' these).
Net: spec sound (deviations are additions); real deviations are PINK-side; float already fixed.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
2026-07-14 23:38:16 +02:00

11 KiB
Raw Blame History

Execution layer — adversarial best-practice audit (the "weird pass")

From: Fable, 2026-07-14. Scope: pink_direct.py + exec_router.py + SPEC_UNIFIED_EXEC_LAYER_20260714.md vs industry / best-of-breed execution practice. Method: current-web grounding (cited) + best-of-breed internals (nautilus_trader — which this repo vendors — FIX ClOrdID discipline, professional OMS drop-copy patterns, Hummingbot in-flight-order tracking). Ranked most-severe first. Nothing here is a BLUE/PINK edit order — it is the port's correctness checklist + escalations for the operator.


HIGH

H1 — float for all money/price/size math (PINK). Against near-universal consensus.

pink_direct.py casts everything to float: _slot_to_position_dict (L55-78), capital, size, PnL, fees throughout. IEEE-754 cannot represent most decimal money exactly; error accumulates across many fills — and PnL/fee/capital is precisely where verification is expensive (doctrine anti-pattern #4). Industry consensus is blunt: never use float/double for monetary math; use arbitrary-precision decimal or integer minor units (rounding every op can be made exact, but a decimal type is the safe default). The irony: this system sits on nautilus_trader, which ships fixed-precision Price/Quantity/Money value types — and pink_direct.py casts them straight to raw float at the boundary.

  • Mitigation PINK already has: the K≈E reconcile gate re-anchors capital to exchange truth on every balance-bearing ACCOUNT_UPDATE (L726-732), so drift is bounded and caught, not unbounded. That is a backstop, not correctness.
  • Verdict: real deviation, bounded in practice. The Unified spec + contract.py already fix it — everything is Decimal at the boundary (contract §2, done). The port must NOT re-inherit float. This finding validates the rebuild direction.

MEDIUM-HIGH

H2 — crash-recovery state lives in /tmp. Against durable-state practice.

_KERNEL_STATE_PATH = /tmp/.pink_kernel_state.json (L87) holds crash recovery + fee calibration + session-to-session continuity. /tmp is volatile (cleared on reboot / tmpwatch), world-traversable, and racy. For a subsystem whose entire purpose is surviving a crash, persisting its recovery tape to the one directory guaranteed to be wiped is self-defeating. Best practice: a durable app-state dir ($XDG_STATE_HOME, /var/lib/...), 0600 perms, atomic write (tmp-file + rename) so a crash mid-write can't corrupt it. The append-only-log school goes further: the event log is the source of truth, snapshots merely accelerate recovery. Port: exec_unified persists working-order state to a durable, atomically-written path — never /tmp.

H3 — shared VST account + symbol-membership fill ownership. Architectural foot-gun.

_fill_is_ours (L588-611) attributes fills on an account shared by PINK / PRODGREEN / BLUE / manual, using symbol membership because BingX WS does not echo our clientOrderId. Best practice is unambiguous: isolate strategies by sub-account (or dedicated API key) — never multiplex independent trading systems on one account; margin and fills cross- contaminate. Symbol membership is fragile precisely where it matters: two systems trading the same symbol on the shared account misattribute each other's fills. The reseed-on- ACCOUNT_UPDATE bounds the damage, but the root (shared account) is the deviation.

  • Escalation: the real bug is the venue not echoing clientOrderId on WS. That should be pushed to BingX and/or worked around with a per-strategy sub-account. This is the correct long-term fix, not a smarter membership heuristic.

H4 — clientOrderId idempotency on retry (SPEC + my contract.py gap).

Industry rule (and FIX ClOrdID law): a unique key per ATTEMPT, never reused; a retry after an INDETERMINATE submit needs a new venue id (exchanges reject duplicate clientOrderId) while preserving linkage to the parent (FIX OrigClOrdID). TTL of the dedup key must exceed the retry window.

  • PINK does this right: retry builds new_tid, intent_id=new_tid (L1210-1212) — fresh id per retry.
  • My contract.py does NOT yet: client_order_id_seed returns the raw request_id (contract.py). If two attempts derive the same venue id, the retry-after-INDETERMINATE collides and is rejected — the exact failure the idempotency is meant to prevent. Fix: the dialect must mint u-<request_id>-<attempt_n> (fresh per attempt, parent-linked) AND enforce venue charset/length limits (BingX caps clientOrderId length). Add an attempt counter to the working-order state; the seed alone is insufficient.

H5 — WS-primary / REST-failover without explicit sequence-gap detection.

PINK starts the WS account stream as primary with poll failover inside it (L386-389). Best practice: WS is a delta channel; the REST snapshot is the reconciliation source of truth; seqnum gaps must be detected and force a REST resync; the append-only event log is the ultimate truth.

  • PINK partially aligns: pump_venue_events reconciles via venue.reconcile() and the kernel dedups by seen_event_ids / _last_settled_pnl (L1236-1237) — that IS event-log- style idempotent application. Good.
  • Gap: no visible WS sequence-number gap detection; the SNAPSHOT-burst barrier is still a spec TODO (§19). The 5 s own-fill hot-window (L1055, L1323) is a time-based proxy for "WS may lag REST" — a heuristic where seqnum + REST-snapshot reconciliation is the rigorous mechanism. Port: add explicit order-stream gap detection → forced REST resync; keep the hot-window as belt-and-braces, not the primary guard.

MEDIUM

M1 — bare except Exception: pass swallowing. The project's own named house-demon.

Multiple silent swallows: venue back-ref (L351), own-fill symbol seed (L370), asset-picker observe (L1428), and others. Some are correctly fail-safe with logging ("Hz read failure must never affect trading", L881 — that one logs). The intent is right (telemetry/Hz failure must never touch trading), but a bare pass in a hot loop hides the bug that eventually bites — and the repo's TESTING_DOCTRINE explicitly names "silent failure is the house specialty." Best practice: catch the specific exception, always log at ≥debug with context. Port: fail-safe guards keep the guard, lose the silence — log every swallow.

M2 — dead-man's-stop orphan + flip risk (already spec-flagged; make it a HARD gate).

Attaching STOP_MARKET at ~2× software SL (spec §6/§10) is best practice for silent-death survival — but two known foot-guns: (a) the attached stop must auto-cancel on position close or it orphans (the proto-PINK hell the spec §6 already flags); (b) it must be reduceOnly/closePosition so a fire cannot open a reverse position. Verdict: the spec flags (a) as a VST-verify TODO — good, but it must be a hard pre-live gate with a mutation-tested assertion, not a checklist line. ProtectiveSpec.reduce_only=True (my contract) covers (b); keep it enforced.

M3 — live sign inconsistency pink_direct.py:673 (# negative = rebate).

Contradicts today's Q1 finding (BingX commission negative = debit/cost). If bingx_user_stream passes the raw venue sign into event.fee, the K-fold books a cost as income. Bounded by the K≈E gate + abs() in calibration (L549), so likely not bleeding — but it is a real sign inconsistency. Not fixing (BLUE/PINK untouchable); the port's §12 friction telemetry must get the sign right at its own boundary. Already logged in the port inventory §4.2.

LOW

L1 — hardcoded cadences / magic numbers (1 s TTL, 5 s window, 15 % dev, 1e-9).

Best practice + the spec's own NFR: named constants with provenance, cadence injected. PINK partially violates; the port fixes it (_constants.py + injected clock). Minor.

L2 — "GTX is the ONLY certified fill-improvement technique" — conservative, not wrong.

Industry also uses pegged/midpoint/iceberg/adaptive-limit. The spec is honestly scoping to the only empirically-certified technique for THIS venue/history — disciplined, not a deviation. The tier ladder (T2+) leaves room for the others. No action.


Where PINK / the spec are AT or ABOVE best practice (balance — do not "fix" these)

  • "Unknown is never flat" / INDETERMINATE triage (spec §5, L5 doctrine): matches best- practice execution-truth handling — never synthesize a REJECT; let the E-feed FILL settle. This is better than many retail bots, which optimistically assume failure = flat.
  • Idempotent replace = cancel-confirm → place (no blind amend) (spec §5): FIX-grade.
  • Telemetry off the exec path, lossless side-lane (spec §12, b46ebd2 lesson): textbook — no I/O in the hot loop.
  • Fail-safe requote gate; venue position is the truth (_exec_safe_to_requote, L1049): fail-closed on ambiguity is exactly right ("a skipped entry is safe, a doubled one is not").
  • Three price truths never conflated (mark = geometry, OB-top = execution, last = parity; spec §9): matches how professional risk/exec systems separate mark vs trade price.
  • seen_event_ids idempotent event application (L1236): the append-only-log dedup discipline the sources call the ultimate source of truth.
  • New contract.py uses Decimal end-to-end: directly fixes H1 for the port.

Net verdict

The spec is sound; its deviations (H4 clientOrderId-per-attempt, H5 seqnum gaps, M2 hard-gate the stop) are additions, not corrections. The real deviations are PINK-side (H1 float, H2 /tmp, H3 shared account, M1 silent swallows) — and the single most important one (float money) is already fixed by the rebuild's Decimal contract. The audit thus argues for the port, and hands it a concrete checklist.

Sources