From adcc116ccae6b6cf2e9141dbe14b67aa23b77c67 Mon Sep 17 00:00:00 2001 From: Malcolm Frank Date: Sat, 1 Aug 2026 19:13:39 -0400 Subject: [PATCH] Fix DATA-RECOVERY-001 review findings: risk-gate test coverage + honest source labeling Independent review of PR #59 (DATA-RECOVERY-001) verified the trustworthy- source risk gate works correctly by hand but flagged two Medium, non-blocking findings (LA-QG-001, LA-QG-002). Addressing both here. - paperEngine.test.ts: adds regression coverage for checkTrustworthySource at its actual call sites -- processNewOrder (via submitOrder) and sweepLimitOrders -- which previously had zero automated coverage; only the standalone risk-check function and evaluateTrade's dispatch of it were tested. Each new describe block includes a positive control (a genuinely Hyperliquid-sourced order still fills) alongside the negative case, matching this file's existing ownership-test convention. - server.ts: the 1s WS-driven marketUpdate broadcast loop always labeled its payload source: 'hyperliquid', even though change24h/volume in that same payload are only as fresh as the last REST ingestion cycle, which could itself have fallen back to CoinGecko while the WS stays healthy. Now reports meta.source (the REST cycle's actual source) instead of hardcoding a label the merged-in fields don't necessarily earn. - docs/architecture/market-data.md: documents this source-attribution nuance in the broadcast loop. Full server suite: 195/195 passing (191 baseline + 4 new). Build clean. --- docs/architecture/market-data.md | 7 +- server/src/execution/paperEngine.test.ts | 144 ++++++++++++++++++++++- server/src/server.ts | 14 ++- 3 files changed, 158 insertions(+), 7 deletions(-) diff --git a/docs/architecture/market-data.md b/docs/architecture/market-data.md index 28f955b..165a611 100644 --- a/docs/architecture/market-data.md +++ b/docs/architecture/market-data.md @@ -130,7 +130,12 @@ any client UI -- that's `CHART-001`. `allMids` and merges live mid-price updates into a broadcast loop that publishes to connected clients every second, per tracked symbol. REST polling (10s prices, 60s candles) remains as the source of `change24h`/`volume`/candle history, which - the WS channel doesn't carry -- see `getLastKnownMarketMeta` below. + the WS channel doesn't carry -- see `getLastKnownMarketMeta` below. The published + `source` field on this fast path reflects the *last REST ingestion cycle's* actual + source (`meta.source`), not simply "hyperliquid" -- `price` here is always a live + Hyperliquid WS mid, but change24h/volume are only as trustworthy as that last REST + cycle, which could itself have fallen back to CoinGecko even while the WS + connection stays healthy (independent review of PR #59, finding LA-QG-002). - **Reconnect/backoff/stale-execution-gating (implemented in `DATA-RECOVERY-001`).** The WS client reconnects with exponential backoff plus jitter on close/error, and detects a silent stall (connection open but no message for 30s) via a reset-able diff --git a/server/src/execution/paperEngine.test.ts b/server/src/execution/paperEngine.test.ts index 22aa082..7b15fea 100644 --- a/server/src/execution/paperEngine.test.ts +++ b/server/src/execution/paperEngine.test.ts @@ -4,17 +4,44 @@ import { NotFoundError, ForbiddenError } from './errors'; const selectMock = vi.fn(); const updateMock = vi.fn(); +const insertMock = vi.fn(); vi.mock('../db/index', () => ({ db: { select: (...args: unknown[]) => selectMock(...args), update: (...args: unknown[]) => updateMock(...args), + insert: (...args: unknown[]) => insertMock(...args), }, })); // vitest hoists `vi.mock` above imports, so `./paperEngine` picks up the // mocked `../db/index`. -import { cancelOrder, closePosition } from './paperEngine'; +import { cancelOrder, closePosition, submitOrder, sweepLimitOrders } from './paperEngine'; + +const BASE_ORDER_ROW = { + id: 'order-1', + userId: 'user-a', + asset: 'BTC', + side: 'LONG' as const, + orderType: 'MARKET' as const, + quantity: '1', + limitPrice: null, + leverage: '1', + status: 'PENDING', +}; + +const RISK_LIMITS_ROW = { + userId: 'user-a', + maxPositionSize: '1000', + maxLeverage: '10', + maxOpenPositions: 5, + maxDailyLossPercent: '5', + killSwitchEnabled: false, +}; + +function marketRow(source: 'hyperliquid' | 'coingecko') { + return { symbol: 'BTC', price: '100', source, updatedAt: new Date() }; +} /** * Regression coverage for SEC-017: user A must never be able to cancel user @@ -31,6 +58,7 @@ describe('cancelOrder ownership', () => { beforeEach(() => { selectMock.mockReset(); updateMock.mockReset(); + insertMock.mockReset(); }); it('throws NotFoundError when the order does not exist', async () => { @@ -58,6 +86,7 @@ describe('closePosition ownership', () => { beforeEach(() => { selectMock.mockReset(); updateMock.mockReset(); + insertMock.mockReset(); }); it('throws NotFoundError when the position does not exist', async () => { @@ -89,3 +118,116 @@ describe('closePosition ownership', () => { expect(updateMock).toHaveBeenCalledTimes(1); }); }); + +/** + * Regression coverage for the trustworthy-source gate (DATA-RECOVERY-001) + * at its actual call sites in processNewOrder/sweepLimitOrders -- flagged + * by independent review of PR #59 (LA-QG-001) as untested at this level: + * checkTrustworthySource itself and evaluateTrade's dispatch of it were + * unit-tested, but nothing exercised the paperEngine.ts integration where + * market.source is actually read from a DB row and threaded through. Each + * block includes a positive control (a genuinely hyperliquid-sourced order + * still fills) alongside the negative case, for the same reason the + * ownership tests above do. + */ +describe('processNewOrder trustworthy-source gating', () => { + beforeEach(() => { + selectMock.mockReset(); + updateMock.mockReset(); + insertMock.mockReset(); + }); + + it('rejects a new MARKET order priced off a CoinGecko-fallback row', async () => { + insertMock.mockReturnValueOnce(dbChain([BASE_ORDER_ROW])); // insert(orders) + selectMock + .mockReturnValueOnce(dbChain([])) // isUserHalted -- no risk_limits row, defaults to false + .mockReturnValueOnce(dbChain([marketRow('coingecko')])) // getMarketSnapshot + .mockReturnValueOnce(dbChain([RISK_LIMITS_ROW])) // getOrCreateRiskLimits + .mockReturnValueOnce(dbChain([{ value: 0 }])); // countOpenPositions + updateMock.mockReturnValueOnce(dbChain([{ ...BASE_ORDER_ROW, status: 'REJECTED' }])); // rejectOrder -> setOrderStatus + + const result = await submitOrder('user-a', { + asset: 'BTC', + side: 'LONG', + orderType: 'MARKET', + quantity: 1, + leverage: 1, + idempotencyKey: 'key-reject', + }); + + expect(result.order.status).toBe('REJECTED'); + expect(result.fills).toEqual([]); + expect(insertMock).toHaveBeenCalledTimes(1); // only the initial order insert -- never reached fillOrder's insert(fills) + }); + + it('positive control: a genuinely Hyperliquid-sourced MARKET order still fills', async () => { + insertMock + .mockReturnValueOnce(dbChain([BASE_ORDER_ROW])) // insert(orders) + .mockReturnValueOnce(dbChain([{ id: 'fill-1', orderId: 'order-1', price: '100.05', quantity: '1' }])) // insert(fills) + .mockReturnValueOnce(dbChain([{ id: 'position-1', userId: 'user-a', asset: 'BTC' }])); // insert(positions) + selectMock + .mockReturnValueOnce(dbChain([])) // isUserHalted + .mockReturnValueOnce(dbChain([marketRow('hyperliquid')])) // getMarketSnapshot + .mockReturnValueOnce(dbChain([RISK_LIMITS_ROW])) // getOrCreateRiskLimits + .mockReturnValueOnce(dbChain([{ value: 0 }])) // countOpenPositions + .mockReturnValueOnce(dbChain([])) // getOpenPosition (direction-conflict check) + .mockReturnValueOnce(dbChain([])) // getOpenPosition again, inside fillOrder + .mockReturnValueOnce(dbChain([{ id: 'fill-1', orderId: 'order-1', price: '100.05', quantity: '1' }])); // select(fills) at the end of fillOrder + updateMock.mockReturnValueOnce(dbChain([{ ...BASE_ORDER_ROW, status: 'FILLED' }])); // setOrderStatus -> FILLED + + const result = await submitOrder('user-a', { + asset: 'BTC', + side: 'LONG', + orderType: 'MARKET', + quantity: 1, + leverage: 1, + idempotencyKey: 'key-fill', + }); + + expect(result.order.status).toBe('FILLED'); + expect(result.fills).toHaveLength(1); + }); +}); + +describe('sweepLimitOrders trustworthy-source gating', () => { + beforeEach(() => { + selectMock.mockReset(); + updateMock.mockReset(); + insertMock.mockReset(); + }); + + it('leaves a marketable resting limit order ACKNOWLEDGED (not filled) when the market is CoinGecko-fallback-sourced', async () => { + const restingOrder = { id: 'order-2', userId: 'user-a', asset: 'BTC', side: 'LONG', orderType: 'LIMIT', limitPrice: '100', status: 'ACKNOWLEDGED' }; + selectMock + .mockReturnValueOnce(dbChain([restingOrder])) // resting LIMIT/ACKNOWLEDGED orders + .mockReturnValueOnce(dbChain([marketRow('coingecko')])) // getMarketSnapshot -- price 100, limitPrice 100 -> marketable + .mockReturnValueOnce(dbChain([])); // isUserHalted + + await sweepLimitOrders(); + + // Blocked by the trustworthy-source gate before ever reaching fillOrder -- + // no fill inserted, no order-status update issued. + expect(insertMock).not.toHaveBeenCalled(); + expect(updateMock).not.toHaveBeenCalled(); + }); + + it('positive control: a genuinely Hyperliquid-sourced marketable resting limit order fills', async () => { + const restingOrder = { id: 'order-2', userId: 'user-a', asset: 'BTC', side: 'LONG', orderType: 'LIMIT', limitPrice: '100', status: 'ACKNOWLEDGED' }; + selectMock + .mockReturnValueOnce(dbChain([restingOrder])) // resting LIMIT/ACKNOWLEDGED orders + .mockReturnValueOnce(dbChain([marketRow('hyperliquid')])) // getMarketSnapshot + .mockReturnValueOnce(dbChain([])) // isUserHalted + .mockReturnValueOnce(dbChain([])) // getOpenPosition (direction-conflict check) + .mockReturnValueOnce(dbChain([])) // getOpenPosition again, inside fillOrder + .mockReturnValueOnce(dbChain([{ id: 'fill-2', orderId: 'order-2', price: '100', quantity: '1' }])); // select(fills) at the end of fillOrder + insertMock + .mockReturnValueOnce(dbChain([{ id: 'fill-2', orderId: 'order-2', price: '100', quantity: '1' }])) // insert(fills) + .mockReturnValueOnce(dbChain([{ id: 'position-2', userId: 'user-a', asset: 'BTC' }])); // insert(positions) + updateMock.mockReturnValueOnce(dbChain([{ ...restingOrder, status: 'FILLED' }])); // setOrderStatus -> FILLED + + await sweepLimitOrders(); + + expect(updateMock).toHaveBeenCalledTimes(1); + expect(insertMock).toHaveBeenCalledTimes(2); + }); +}); diff --git a/server/src/server.ts b/server/src/server.ts index 271edd3..ce8cb75 100644 --- a/server/src/server.ts +++ b/server/src/server.ts @@ -176,11 +176,15 @@ setInterval(() => { price: Number(price), change24h: Number(meta.change24h), volume: Number(meta.volume), - // A live Hyperliquid WS mid is, by definition, Hyperliquid-sourced -- - // independent of whatever the last REST cycle's source happened to - // be (e.g. if REST last fell back to CoinGecko but the WS has since - // recovered). - source: 'hyperliquid', + // `price` here is always genuinely Hyperliquid-sourced (a live WS + // mid), but change24h/volume are only as fresh as the *last REST + // ingestion cycle*, which could itself have fallen back to + // CoinGecko even while this WS connection is healthy. Labeling the + // whole row "hyperliquid" in that window would overstate the + // trustworthiness of the change24h/volume figures being merged in + // alongside it (flagged by independent review of PR #59, LA-QG-002) + // -- meta.source truthfully reflects what those fields actually are. + source: meta.source, timestamp: new Date(), }); }