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>
This commit is contained in:
Codex
151
prod/docs/EXEC_BEST_PRACTICE_AUDIT_20260714.md
Normal file
151
prod/docs/EXEC_BEST_PRACTICE_AUDIT_20260714.md
Normal file
@@ -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-<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
|
||||
- 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)
|
||||
Reference in New Issue
Block a user