Skip to content

CODEBASE_REVIEW.md

Latest commit

 

History

History
389 lines (246 loc) · 17.1 KB

CODEBASE_REVIEW.md

File metadata and controls

389 lines (246 loc) · 17.1 KB
Raw

GridBot Codebase Review

Review date: 2026-07-18
Scope: Entire tracked TypeScript codebase, configuration, tests, dependencies, BUILD_SPEC.md, and draft UPGRADE_SPEC.md
Review mode: Read-only audit; no implementation changes were made as part of this review

Status update (2026-07-19): all 3 critical and all 3 high findings below (#1–#6) are FIXED, with failing-test-first verification (each new test confirmed to fail against the pre-fix code, then pass) and, for #1–#5, confirmed to fail the TypeScript build entirely if reverted. See the commits: eefa11d (#6, #2, #1), 07c961c (#5), d669938 (#4), 7b29b01 (#3). The 5 medium findings (#7–#11) remain open — see each item below; none of them is a fund-safety issue and all are reasonable follow-up work, not blockers.

Executive summary

The codebase is generally well structured, strongly typed, and unusually safety-conscious. The most important risks are concentrated in fill handling, inventory accounting, user-data-stream synchronization, and rotation recovery.

The proposed work in UPGRADE_SPEC.md should not take priority over the critical correctness defects below. In particular, performance measurements, inventory drawdown, shadow comparisons, and multi-pair capital partitioning will be unreliable until partial SELLs, pending-pair inventory, fee idempotency, and user-stream gaps are handled correctly.

(2026-07-19: this recommendation was followed in reverse order of discovery — steps 17–21 of UPGRADE_SPEC.md had already been built by the time this review's findings were fully triaged; the critical/high fixes below were then completed before any further UPGRADE_SPEC work or the GitHub publish. Multi-pair (13.4) remains gated on live Phase 3 regardless.)

Verification performed

  • ./node_modules/.bin/tsc --noEmit: passed
  • node --test "dist/**/*.test.js": all 25 compiled test files passed
  • npm audit --json: zero known vulnerabilities across 87 dependencies
  • No destructive commands were run
  • No existing files were edited by this audit

Passing tests do not cover the principal defects identified below.

Confirmed problems

1. Critical — canceled partially filled SELL loses the remaining inventory

FIXED 2026-07-19 (commit eefa11d): cost/fee now prorated by executed fraction via the immutable paired BUY row; the unsold remainder is re-placed (or flagged as dust) instead of vanishing. Re-arm now waits for the lot to fully close. The identical pattern in rotate.ts's recordMissedSellCycles was fixed in the same pass.

Files and lines

  • src/engine.ts:250-254
  • src/engine.ts:290-327
  • src/risk.ts:61-85

Reasoning

A terminal CANCELED or EXPIRED execution report with executedQty > 0 is passed to #afterFill(). For a partially filled SELL, the engine then:

  1. Treats the partial execution as a completed cycle.
  2. Uses the entire paired BUY cost but only the partial SELL proceeds.
  3. Does not place another SELL for the remaining quantity.
  4. Re-arms a new BUY.
  5. Leaves the residual base asset unrepresented in open orders, pending pairs, and risk calculations.

This can produce incorrect realized PnL, hidden inventory, and additional buying while old inventory remains stranded.

Recommended fix

  • Treat SELL executions as inventory lots rather than treating every terminal order as a completed full cycle.
  • Allocate BUY cost and fees proportionally to the quantity sold.
  • On canceled or expired partial SELL, record only the completed fraction and create a replacement risk-reducing SELL, or a durable pending pair, for the remaining quantity.
  • Consider an explicit inventory-lot table.
  • Add engine and reconciliation tests for a partially filled SELL followed by cancellation.

2. Critical — pending-pair inventory bypasses risk and capital limits

FIXED 2026-07-19 (commit eefa11d): RiskMonitor.unrealized() and Engine#committedQuote() now include pending-pair inventory (symbol-scoped after finding #5's fix).

Files and lines

  • src/engine.ts:331-345
  • src/engine.ts:446-463
  • src/engine.ts:508-520
  • src/risk.ts:61-85
  • src/state.ts:123-133

Reasoning

When a BUY fills but the paired SELL cannot be placed, the inventory is represented by a pending_pairs row. However:

  • RiskMonitor.unrealized() counts only open SELL orders.
  • Engine.#committedQuote() counts only open exchange orders.
  • pending_pairs.costBasisQuote is persisted but unused by both calculations.

A pending SELL can therefore represent real held inventory while unrealized loss appears as zero and capital appears available. The engine may exceed totalQuoteCapital, and the Layer-2 unrealized-loss halt may not fire.

Recommended fix

  • Include pending SELL quantities and costBasisQuote in unrealized inventory.
  • Include pending-pair cost basis in committed capital.
  • Prevent double-counting once a pending pair becomes an open order.
  • Test that a pending SELL contributes to both capital commitment and unrealized loss.

3. Critical — trading begins before user-stream subscription and continues through gaps

FIXED 2026-07-19 (commit 7b29b01): engine.start() now blocks on the user-data stream's first confirmed subscription; every reconnect triggers a reconciliation barrier (reconcileAfterGap) before new orders resume, via a new Engine#userStreamGap pause state. Verified against a live testnet run that 'subscribed' now logs strictly before any order placement.

Files and lines

  • src/binanceWs.ts:71-74
  • src/binanceWs.ts:419-480
  • src/index.ts:366-400

Reasoning

UserDataStream.start() connects asynchronously. runSession() immediately calls engine.start() without waiting for a successful subscription response.

The engine can place orders before it can receive fills. A later user-stream disconnect triggers resubscription but no reconciliation, pause, or halt. A fill during that gap may remain unknown indefinitely while the engine continues trading.

The comment that startup reconciliation covers fills missed during a gap is only true after a process restart; reconnecting the stream does not run startup reconciliation.

Recommended fix

  • Expose a readiness promise/event that resolves only after subscription status 200.
  • Do not arm the grid until user-stream readiness is confirmed.
  • Pause all new order placement immediately on user-stream disconnect.
  • Reconcile local and exchange state after every reconnect before resuming.
  • Halt if reconciliation cannot prove consistency.
  • Test fills before initial subscription and during a reconnect gap.

4. High — rotation disposal can oversell after partial fills

FIXED 2026-07-19 (commit d669938): disposal now tracks cumulative executed quantity across attempts and recomputes the remainder each time; a cancel race is always re-verified via queryOrder, never inferred from -2011. A DisposalPartial error carries the true remainder back to executeRotation, which persists it so a resumed rotation never resubmits the original amount.

File and lines

  • src/rotate.ts:277-315

Reasoning

Every disposal attempt submits the original sellQty. If one attempt partially fills and is canceled, the following attempt submits the full amount again instead of the remaining amount.

This can:

  • attempt to sell more than the bot still holds;
  • fail repeatedly with insufficient balance;
  • sell unrelated holdings of the same base asset if present.

Additionally, Binance error -2011 during cancellation is treated as proof that the order filled, even though an unknown-order response does not establish the terminal state.

Recommended fix

  • Query and verify the terminal order state after every cancellation race.
  • Track cumulative executed quantity across attempts.
  • Recalculate remaining = target - totalExecuted, align it to stepSize, and submit only the remainder.
  • Treat unknown status as unresolved rather than filled.
  • Persist disposal order IDs and progress for idempotent recovery.
  • Add partial-fill, cancel-race, restart, and dust tests.

5. High — manual rotation can carry an old pending pair into the new symbol

FIXED 2026-07-19 (commit 07c961c): pending_pairs now has a symbol column (migrated automatically for existing DBs); every engine/risk read site filters by its own symbol. executeRotation folds any outstanding pending pair for the FROM symbol into disposal at the RECONCILED step and clears the rows.

Files and lines

  • src/rotate.ts:345-393
  • src/state.ts:123-133
  • src/engine.ts:446-463

Reasoning

Stage B rule R6 blocks rotation when pending pairs exist, but the manual rotation CLI does not enforce the same rule. executeRotation() neither resolves nor clears pending pairs.

pending_pairs has no symbol, and retry processing does not filter by symbol or generation. After rotation, the new engine session may interpret an old-symbol quantity as a SELL quantity for the new symbol.

Recommended fix

  • Refuse every rotation while pending pairs exist unless they are explicitly converted into disposal inventory.
  • Add symbol to pending_pairs.
  • Filter retry processing by current symbol and generation.
  • Reconcile or migrate pending-pair state before changing configuration.
  • Add a manual-rotation test with an outstanding pending pair.

6. High — unhandled engine failures do not await halt cleanup

FIXED 2026-07-19 (commit eefa11d): Engine#enqueue's catch handler now awaits haltNow() instead of firing it and forgetting — the serialized queue no longer resolves until cancel-all retries actually finish.

File and lines

  • src/engine.ts:134-139

Reasoning

The serialized queue catches a work failure and calls void this.haltNow(...). The halt cleanup promise is discarded, so the queued operation resolves before cancel-all retries finish. Shutdown or other control flow can proceed while exchange orders remain unmanaged.

Recommended fix

  • Await halt cleanup in the queue error path while avoiding self-enqueueing.
  • Maintain a distinct, reusable haltPromise.
  • Ensure process shutdown waits for halt cleanup or an explicit timeout.
  • Add a test where queued engine work throws and verify cancellation completes before the returned promise resolves.

7. Medium — fee accounting is not idempotent

Files and lines

  • src/engine.ts:231-254
  • src/state.ts:223-247
  • src/binanceWs.ts:326-344

Reasoning

Every execution report adds its commission to stored fee totals. No trade ID or event ID is persisted. A duplicate execution event therefore adds the same commission again.

The existing duplicate-fill test verifies that pairing and cycle creation are not duplicated, but it does not verify fee totals.

Recommended fix

  • Preserve Binance’s trade ID from execution reports.
  • Store processed fills with a unique key such as (symbol, orderId, tradeId).
  • Apply quantity and fee changes transactionally only for unseen fills.
  • Test duplicate partial-fill and final-fill events.

8. Medium — unknown exchange statuses become EXPIRED

File and lines

  • src/reconcile.ts:58-65

Reasoning

Any unrecognized exchange status is silently converted to EXPIRED. Contrary to the nearby comment, the report does not necessarily expose that assumption as an unresolved mismatch.

A new, malformed, or unexpected status could therefore be treated as safely terminal.

Recommended fix

  • Make status conversion return a validated result.
  • Add unknown statuses to report.unresolved.
  • Refuse startup rather than making a terminal-state assumption.

9. Medium — sparse scans can satisfy continuous-unhealthy persistence

Files and lines

  • src/scan.ts:68-81
  • src/scan.ts:184

Reasoning

unhealthyStreakMs() measures from the oldest consecutive unhealthy scan in the available rows without checking observation gaps. Two unhealthy scans 24 hours apart, with no evidence between them, can satisfy a 24-hour continuous-persistence rule.

Recommended fix

  • Require scan coverage with a maximum permitted gap, such as 1.5 × scanIntervalHours.
  • Break the streak when observations are missing beyond that tolerance.
  • Add tests for sparse scans and scheduler downtime.

10. Medium — manual rotations consume the Stage B rotation budget

Files and lines

  • src/scan.ts:185-186
  • src/autopilot.ts:81-85
  • src/rotate.ts:205-208

Reasoning

R4 states that only Stage B rotations count and manual rotations are exempt. In practice, all rotations write the same rotation event and the scanner counts all of them.

Recommended fix

  • Persist a rotation origin such as manual, stage_b, or rerange.
  • Count only stage_b events for the Stage B budget.
  • Test that a manual rotation does not consume R4.

11. Medium — scanner networking lacks timeout, retry, and rate-limit handling

Files and lines

  • src/scan.ts:35-58
  • src/scan.ts:109-144

Reasoning

The scanner uses raw fetch() without a timeout or retry policy and bypasses the REST client’s rate-limit tracking. One request can hang indefinitely, and four workers can create request bursts. The incumbent’s klines are also fetched twice.

Recommended fix

  • Reuse a public-only form of BinanceRest.
  • Add request timeouts, bounded retries with jitter, and 429 handling.
  • Cache the incumbent candle result.
  • Make concurrency and expected API weight explicit.

Missing tests to prioritize

  1. Canceled partially filled SELL: proportional cycle plus replacement for remaining inventory.
  2. Pending SELL included in capital and unrealized-loss calculations.
  3. User-stream subscription must complete before any order placement.
  4. Fill during user-stream disconnect followed by reconnect and reconciliation.
  5. Rotation disposal across partial fills and cancellation races.
  6. Manual rotation with an outstanding pending pair.
  7. Duplicate execution event does not duplicate fees.
  8. Unknown exchange status causes reconciliation failure.
  9. Sparse scans do not satisfy continuous-unhealthy persistence.
  10. Manual rotations do not consume the Stage B budget.

The existing chaos suite tests DB/exchange order-state consistency well, but it does not model user-stream blind windows, pending-pair capital, partial disposal, or residual inventory from partial SELLs.

Optional improvements

These are not confirmed correctness failures:

  • Add a SQLite busy_timeout in src/state.ts:165-169 so concurrent report and scan processes are less likely to fail immediately on lock contention.
  • Add database uniqueness constraints for cycles.sellClientOrderId and pending-pair origins.
  • Consider buffered logging in src/logger.ts:39-47 if event volume becomes material.
  • Use atomic temporary-file-plus-rename writes for setup output and rotation configuration changes.
  • Validate REST and WebSocket numeric response fields as finite positive values before strategy use.
  • Bound and validate Retry-After in src/binanceRest.ts:155-164.
  • Consider fixed-point or decimal arithmetic for all monetary accounting if the project expands beyond micro-size validation.

Dependency review

Current live registry results:

  • npm audit: zero known vulnerabilities.
  • better-sqlite3 12.11.1: current.
  • ws 8.21.0: patch update available to 8.21.1.
  • react 18.3.1 and ink 5.2.1: newer major versions exist, but upgrading is optional and carries migration risk.
  • @types/node, @types/react, and TypeScript have newer major versions; these are not security-driven upgrades.

Recommendation: consider the ws patch after tests, but defer React, Ink, and TypeScript major upgrades until the trading-state defects are fixed.

Relationship to UPGRADE_SPEC.md

The measurement ledger, lab, and backtester are directionally sensible. However, build steps 17–23 should follow the critical correctness work in this report.

Before trusting the proposed metrics or adding multi-pair support, the system needs:

  1. Correct partial-SELL inventory accounting.
  2. Pending-pair inclusion in risk and capital.
  3. User-stream readiness and reconnect reconciliation.
  4. Idempotent fill and fee processing.
  5. Resumable, quantity-correct rotation disposal.

Multi-pair support would multiply the consequences of the current inventory and stream-synchronization defects.

Suggested Claude Code follow-up

Claude Code should independently:

  1. Trace each confirmed problem through production and test code.
  2. Challenge the severity and identify any invariant that already prevents the scenario.
  3. Construct a minimal failing test for each accepted problem before proposing implementation changes.
  4. Determine whether partial SELL handling needs an inventory-lot schema migration.
  5. Design one reconciliation barrier shared by startup and user-stream reconnect.
  6. Review Binance’s current official Spot API documentation for execution-report fields, trade IDs, subscription lifecycle, and cancel-order error semantics.
  7. Produce a staged remediation plan that fixes correctness before implementing UPGRADE_SPEC.md.

Workspace note

At the end of the original audit:

  • UPGRADE_SPEC.md was untracked.
  • src/setup.ts was modified by a concurrent or external change during the review.
  • The reviewer did not make that src/setup.ts change.