From a7cf3f59766f0f578cd9d9ff168a9c1bfa36a21f Mon Sep 17 00:00:00 2001 From: Codex Date: Tue, 14 Jul 2026 23:38:16 +0200 Subject: [PATCH] =?UTF-8?q?docs(exec):=20adversarial=20best-practice=20aud?= =?UTF-8?q?it=20=E2=80=94=20web-grounded=20+=20best-of-breed=20internals?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- .../docs/EXEC_BEST_PRACTICE_AUDIT_20260714.md | 151 ++++++++++++++++++ 1 file changed, 151 insertions(+) create mode 100644 prod/docs/EXEC_BEST_PRACTICE_AUDIT_20260714.md diff --git a/prod/docs/EXEC_BEST_PRACTICE_AUDIT_20260714.md b/prod/docs/EXEC_BEST_PRACTICE_AUDIT_20260714.md new file mode 100644 index 0000000..64a7d05 --- /dev/null +++ b/prod/docs/EXEC_BEST_PRACTICE_AUDIT_20260714.md @@ -0,0 +1,151 @@ +# 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--` (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 +- Idempotency / clientOrderId: [Token Metrics](https://www.tokenmetrics.com/blog/idempotency-keys-order-placement), [Coinbase idempotency](https://docs.cdp.coinbase.com/api-reference/v2/idempotency), [George Tsiokos — idempotent orders](https://george.tsiokos.com/code/2026/idempotent-orders/) +- Float vs decimal money: [Modern Treasury — floats don't work for cents](https://www.moderntreasury.com/journal/floats-dont-work-for-storing-cents), [DZone — never use float for money](https://dzone.com/articles/never-use-float-and-double-for-monetary-calculatio), [evanjones.ca — you *can*, with rounding](https://www.evanjones.ca/floating-point-money.html) +- WS vs REST reconciliation / seqnum: [FIX vs REST vs WebSocket](https://brokeret.com/blog/fix-vs-rest-vs-websocket-trading-api-decision-matrix), [Coinbase Advanced Trade WS](https://docs.cdp.coinbase.com/coinbase-app/advanced-trade-apis/websocket/websocket-overview), [Kraken WS FAQ](https://support.kraken.com/articles/360022326871-kraken-websocket-api-frequently-asked-questions)