From 3bc3e7141519f12cac37dfaa57e4bdcb332e0839 Mon Sep 17 00:00:00 2001 From: ECD5A Date: Sun, 26 Jul 2026 09:35:58 +0500 Subject: [PATCH] fix(trading): include fees in cash risk checks --- src/trading/engine.ts | 16 ++++++++-------- src/trading/live-exchange.ts | 19 ++++++++---------- src/trading/mock-exchange.ts | 32 ++++++++++++++++--------------- src/trading/risk.ts | 13 +++++++++++-- test/local.mjs | 37 ++++++++++++++++++++++++++++++++++++ 5 files changed, 81 insertions(+), 36 deletions(-) diff --git a/src/trading/engine.ts b/src/trading/engine.ts index 7f974b1..bde6e45 100644 --- a/src/trading/engine.ts +++ b/src/trading/engine.ts @@ -43,22 +43,22 @@ export class TradingEngine { async openPosition(req: OpenPositionRequest): Promise { const { portfolio, risk, exchange } = this.deps; - const decision = risk.check(portfolio, { + const order = { symbol: req.symbol, - side: 'buy', + side: 'buy' as const, qty: req.qty, priceUsd: req.priceUsd, + }; + const feeUsd = await exchange.estimateFee(order); + const decision = risk.check(portfolio, { + ...order, + feeUsd, }); if (!decision.allowed) { return { status: 'blocked', reason: decision.reason ?? 'blocked by risk engine' }; } - const fill = await exchange.placeOrder({ - symbol: req.symbol, - side: 'buy', - qty: req.qty, - priceUsd: req.priceUsd, - }); + const fill = await exchange.placeOrder(order); portfolio.applyFill(fill); return { status: 'filled', diff --git a/src/trading/live-exchange.ts b/src/trading/live-exchange.ts index e5f5945..f5a7265 100644 --- a/src/trading/live-exchange.ts +++ b/src/trading/live-exchange.ts @@ -14,8 +14,8 @@ * can validate behavior without hitting CoinGecko. */ -import type { ExchangeClient } from './mock-exchange.js'; -import type { Fill, Side } from './portfolio.js'; +import type { ExchangeClient, ExchangeOrder } from './mock-exchange.js'; +import type { Fill } from './portfolio.js'; /** Subset of src/trading/data.ts's PriceData that we actually consume. */ export interface PricingClientResponse { @@ -52,20 +52,17 @@ export class LiveExchange implements ExchangeClient { } } - async placeOrder(order: { - symbol: string; - side: Side; - qty: number; - priceUsd: number; - }): Promise { - const notional = order.qty * order.priceUsd; - const feeUsd = (notional * this.opts.feeBps) / 10_000; + estimateFee(order: ExchangeOrder): number { + return (order.qty * order.priceUsd * this.opts.feeBps) / 10_000; + } + + async placeOrder(order: ExchangeOrder): Promise { return { symbol: order.symbol, side: order.side, qty: order.qty, priceUsd: order.priceUsd, - feeUsd, + feeUsd: this.estimateFee(order), }; } } diff --git a/src/trading/mock-exchange.ts b/src/trading/mock-exchange.ts index 4262beb..ce8953e 100644 --- a/src/trading/mock-exchange.ts +++ b/src/trading/mock-exchange.ts @@ -13,13 +13,18 @@ import type { Fill, Side } from './portfolio.js'; +export interface ExchangeOrder { + symbol: string; + side: Side; + qty: number; + priceUsd: number; +} + export interface ExchangeClient { - placeOrder(order: { - symbol: string; - side: Side; - qty: number; - priceUsd: number; - }): Promise; + // Risk checks need the expected fee before an order reaches the venue. + // Implementations must use the same fee model for estimation and fills. + estimateFee(order: ExchangeOrder): number | Promise; + placeOrder(order: ExchangeOrder): Promise; // Live mark-price for portfolio valuation. Real adapters hit the ticker // endpoint; MockExchange reads from its config. getPrice(symbol: string): Promise; @@ -44,23 +49,20 @@ export class MockExchange implements ExchangeClient { this.prices[symbol] = priceUsd; } - async placeOrder(order: { - symbol: string; - side: Side; - qty: number; - priceUsd: number; - }): Promise { + estimateFee(order: ExchangeOrder): number { + return (order.qty * order.priceUsd * this.feeBps) / 10_000; + } + + async placeOrder(order: ExchangeOrder): Promise { if (!(order.symbol in this.prices)) { throw new Error(`MockExchange has no quote for ${order.symbol}`); } - const notional = order.qty * order.priceUsd; - const feeUsd = (notional * this.feeBps) / 10_000; return { symbol: order.symbol, side: order.side, qty: order.qty, priceUsd: order.priceUsd, - feeUsd, + feeUsd: this.estimateFee(order), }; } diff --git a/src/trading/risk.ts b/src/trading/risk.ts index 365507c..c7f8874 100644 --- a/src/trading/risk.ts +++ b/src/trading/risk.ts @@ -25,6 +25,7 @@ export interface OrderRequest { side: Side; qty: number; priceUsd: number; + feeUsd?: number; } export interface RiskDecision { @@ -48,11 +49,19 @@ export class RiskEngine { } const notional = order.qty * order.priceUsd; + const feeUsd = order.feeUsd ?? 0; + if (!Number.isFinite(feeUsd) || feeUsd < 0) { + return { + allowed: false, + reason: `Invalid estimated fee: ${feeUsd}`, + }; + } + const cashRequired = notional + feeUsd; - if (notional > portfolio.cashUsd) { + if (cashRequired > portfolio.cashUsd) { return { allowed: false, - reason: `Insufficient cash: order needs $${notional.toFixed(2)} but only $${portfolio.cashUsd.toFixed(2)} available`, + reason: `Insufficient cash: order needs $${cashRequired.toFixed(2)} including $${feeUsd.toFixed(2)} estimated fee but only $${portfolio.cashUsd.toFixed(2)} available`, }; } diff --git a/test/local.mjs b/test/local.mjs index f7b93b4..50f0559 100644 --- a/test/local.mjs +++ b/test/local.mjs @@ -3101,6 +3101,24 @@ test('RiskEngine: rejects buy that exceeds available cash regardless of caps', a assert.match(decision.reason ?? '', /insufficient cash/i); }); +test('RiskEngine: includes the estimated exchange fee in the buy cash check', async () => { + const { RiskEngine } = await import('../dist/trading/risk.js'); + const { Portfolio } = await import('../dist/trading/portfolio.js'); + const pf = new Portfolio({ startingCashUsd: 100 }); + const risk = new RiskEngine({ maxPositionUsd: 10_000, maxTotalExposureUsd: 10_000 }); + + const decision = risk.check(pf, { + symbol: 'BTC', + side: 'buy', + qty: 0.001, + priceUsd: 100_000, + feeUsd: 0.10, + }); + assert.equal(decision.allowed, false); + assert.match(decision.reason ?? '', /insufficient cash/i); + assert.match(decision.reason ?? '', /\$100\.10/); +}); + test('RiskEngine: sell is allowed even when caps are exceeded, as long as position exists', async () => { const { RiskEngine } = await import('../dist/trading/risk.js'); const { Portfolio } = await import('../dist/trading/portfolio.js'); @@ -3387,6 +3405,24 @@ test('TradingEngine: executes a compliant order through risk → exchange → po assert.ok(Math.abs(portfolio.cashUsd - (1000 - 140.14)) < 1e-9); }); +test('TradingEngine: blocks a buy when notional fits cash but notional plus fee does not', async () => { + const { TradingEngine } = await import('../dist/trading/engine.js'); + const { Portfolio } = await import('../dist/trading/portfolio.js'); + const { RiskEngine } = await import('../dist/trading/risk.js'); + const { MockExchange } = await import('../dist/trading/mock-exchange.js'); + + const portfolio = new Portfolio({ startingCashUsd: 100 }); + const risk = new RiskEngine({ maxPositionUsd: 10_000, maxTotalExposureUsd: 10_000 }); + const exchange = new MockExchange({ prices: { BTC: 100_000 }, feeBps: 10 }); + const engine = new TradingEngine({ portfolio, risk, exchange }); + + const outcome = await engine.openPosition({ symbol: 'BTC', qty: 0.001, priceUsd: 100_000 }); + assert.equal(outcome.status, 'blocked'); + assert.match(outcome.reason ?? '', /insufficient cash/i); + assert.equal(portfolio.getPosition('BTC'), undefined); + assert.equal(portfolio.cashUsd, 100); +}); + test('TradingEngine: blocks order that violates risk and does NOT touch the exchange', async () => { const { TradingEngine } = await import('../dist/trading/engine.js'); const { Portfolio } = await import('../dist/trading/portfolio.js'); @@ -3394,6 +3430,7 @@ test('TradingEngine: blocks order that violates risk and does NOT touch the exch let placed = 0; const fakeExchange = { + estimateFee() { return 0; }, async placeOrder() { placed++; throw new Error('should never be called'); }, async getPrice() { return null; }, };