fix(trading): include fees in cash risk checks

CHANGELOG.md

@@ -23,6 +23,9 @@ This project follows [Semantic Versioning](https://semver.org/spec/v2.0.0.html).
 
 ## Unreleased
 
+### Changed
+- `ExchangeClient` now exposes a conservative pre-trade fee estimate, and canonical fills require an explicit fee. Buy-side risk checks validate order inputs, fee estimates, and the returned fill before portfolio accounting. This is a breaking interface change intended for the next minor release.
+
 ### Added
 - Initial fork from upstream [Franklin (brcc) 3.21.9](https://github.com/BlockRunAI/Franklin/tree/v3.21.9).
 - New CLI binary name `franklin-trading`, new npm package name `@blockrun/franklin-trading`.

src/trading/engine.ts

@@ -13,9 +13,31 @@
  */
 
 import type { ExchangeClient } from './mock-exchange.js';
-import type { Portfolio } from './portfolio.js';
+import type { Fill, Portfolio } from './portfolio.js';
 import type { RiskEngine } from './risk.js';
 
+const FEE_COMPARISON_TOLERANCE_USD = 1e-9;
+
+function errorMessage(error: unknown): string {
+  return error instanceof Error ? error.message : String(error);
+}
+
+function assertAcceptableBuyFill(fill: Fill, symbol: string, estimatedFeeUsd: number): void {
+  if (fill.symbol !== symbol || fill.side !== 'buy') {
+    throw new Error(
+      `Exchange returned mismatched fill: expected buy ${symbol}, received ${fill.side} ${fill.symbol}`,
+    );
+  }
+  if (!Number.isFinite(fill.feeUsd) || fill.feeUsd < 0) {
+    throw new Error(`Exchange returned invalid fill fee: ${fill.feeUsd}`);
+  }
+  if (fill.feeUsd > estimatedFeeUsd + FEE_COMPARISON_TOLERANCE_USD) {
+    throw new Error(
+      `Exchange fill fee $${fill.feeUsd} exceeds the approved estimate $${estimatedFeeUsd}`,
+    );
+  }
+}
+
 export interface OpenPositionRequest {
   symbol: string;
   qty: number;
@@ -43,30 +65,63 @@ export class TradingEngine {
 
   async openPosition(req: OpenPositionRequest): Promise<Outcome> {
     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,
+    };
+    // Reject malformed or obviously over-limit orders before a future
+    // adapter can spend money or rate-limit capacity on a fee quote.
+    const localDecision = risk.check(portfolio, { ...order, feeUsd: 0 });
+    if (!localDecision.allowed) {
+      return { status: 'blocked', reason: localDecision.reason ?? 'blocked by risk engine' };
+    }
+
+    let feeUsd: number;
+    try {
+      feeUsd = await exchange.estimateFee(order);
+    } catch (error) {
+      return {
+        status: 'blocked',
+        reason: `Unable to estimate exchange fee: ${errorMessage(error)}`,
+      };
+    }
+    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,
+    const fill = await exchange.placeOrder(order);
+    assertAcceptableBuyFill(fill, order.symbol, feeUsd);
+
+    // Re-evaluate the canonical fill before mutating accounting. This also
+    // catches unexpected quantity/price changes that would breach cash or
+    // exposure limits even when the fee stayed below its ceiling.
+    const fillDecision = risk.check(portfolio, {
+      symbol: fill.symbol,
       side: 'buy',
-      qty: req.qty,
-      priceUsd: req.priceUsd,
+      qty: fill.qty,
+      priceUsd: fill.priceUsd,
+      feeUsd: fill.feeUsd,
     });
+    if (!fillDecision.allowed) {
+      throw new Error(
+        `Exchange fill violates pre-trade risk: ${fillDecision.reason ?? 'blocked by risk engine'}`,
+      );
+    }
+
     portfolio.applyFill(fill);
     return {
       status: 'filled',
       fill: {
         symbol: fill.symbol,
         qty: fill.qty,
         priceUsd: fill.priceUsd,
-        feeUsd: fill.feeUsd ?? 0,
+        feeUsd: fill.feeUsd,
       },
     };
   }
@@ -95,7 +150,7 @@ export class TradingEngine {
         symbol: fill.symbol,
         qty: fill.qty,
         priceUsd: fill.priceUsd,
-        feeUsd: fill.feeUsd ?? 0,
+        feeUsd: fill.feeUsd,
       },
     };
   }

src/trading/fees.ts

@@ -0,0 +1,14 @@
+/** Calculate a fee from order notional and basis points. */
+export function bpsFee(order: { qty: number; priceUsd: number }, feeBps: number): number {
+  if (!Number.isFinite(order.qty) || order.qty <= 0) {
+    throw new RangeError(`Invalid order quantity: ${order.qty}`);
+  }
+  if (!Number.isFinite(order.priceUsd) || order.priceUsd <= 0) {
+    throw new RangeError(`Invalid order price: ${order.priceUsd}`);
+  }
+  if (!Number.isFinite(feeBps) || feeBps < 0) {
+    throw new RangeError(`Invalid fee rate: ${feeBps} bps`);
+  }
+
+  return (order.qty * order.priceUsd * feeBps) / 10_000;
+}

src/trading/live-exchange.ts

@@ -14,8 +14,9 @@
  * 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';
+import { bpsFee } from './fees.js';
 
 /** Subset of src/trading/data.ts's PriceData that we actually consume. */
 export interface PricingClientResponse {
@@ -52,20 +53,17 @@ export class LiveExchange implements ExchangeClient {
     }
   }
 
-  async placeOrder(order: {
-    symbol: string;
-    side: Side;
-    qty: number;
-    priceUsd: number;
-  }): Promise<Fill> {
-    const notional = order.qty * order.priceUsd;
-    const feeUsd = (notional * this.opts.feeBps) / 10_000;
+  estimateFee(order: ExchangeOrder): number {
+    return bpsFee(order, this.opts.feeBps);
+  }
+
+  async placeOrder(order: ExchangeOrder): Promise<Fill> {
     return {
       symbol: order.symbol,
       side: order.side,
       qty: order.qty,
       priceUsd: order.priceUsd,
-      feeUsd,
+      feeUsd: this.estimateFee(order),
     };
   }
 }

src/trading/mock-exchange.ts

@@ -12,14 +12,20 @@
  */
 
 import type { Fill, Side } from './portfolio.js';
+import { bpsFee } from './fees.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<Fill>;
+  // Return a conservative fee ceiling before an order reaches the venue.
+  // A fill must never charge more than the estimate returned for its order.
+  estimateFee(order: ExchangeOrder): number | Promise<number>;
+  placeOrder(order: ExchangeOrder): Promise<Fill>;
   // Live mark-price for portfolio valuation. Real adapters hit the ticker
   // endpoint; MockExchange reads from its config.
   getPrice(symbol: string): Promise<number | null>;
@@ -44,23 +50,20 @@ export class MockExchange implements ExchangeClient {
     this.prices[symbol] = priceUsd;
   }
 
-  async placeOrder(order: {
-    symbol: string;
-    side: Side;
-    qty: number;
-    priceUsd: number;
-  }): Promise<Fill> {
+  estimateFee(order: ExchangeOrder): number {
+    return bpsFee(order, this.feeBps);
+  }
+
+  async placeOrder(order: ExchangeOrder): Promise<Fill> {
     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),
     };
   }
 

src/trading/portfolio.ts

@@ -20,7 +20,7 @@ export interface Fill {
   side: Side;
   qty: number;
   priceUsd: number;
-  feeUsd?: number;
+  feeUsd: number;
 }
 
 export interface Position {
@@ -76,7 +76,17 @@ export class Portfolio {
   }
 
   applyFill(fill: Fill): void {
-    const fee = fill.feeUsd ?? 0;
+    if (!Number.isFinite(fill.qty) || fill.qty <= 0) {
+      throw new RangeError(`Invalid fill quantity: ${fill.qty}`);
+    }
+    if (!Number.isFinite(fill.priceUsd) || fill.priceUsd <= 0) {
+      throw new RangeError(`Invalid fill price: ${fill.priceUsd}`);
+    }
+    if (!Number.isFinite(fill.feeUsd) || fill.feeUsd < 0) {
+      throw new RangeError(`Invalid fill fee: ${fill.feeUsd}`);
+    }
+
+    const fee = fill.feeUsd;
     const notional = fill.qty * fill.priceUsd;
 
     if (fill.side === 'buy') {

src/trading/risk.ts

@@ -13,20 +13,23 @@
  * cap could otherwise trap the agent in a losing position it wants to exit.
  */
 
-import type { Portfolio, Side } from './portfolio.js';
+import type { Portfolio } from './portfolio.js';
 
 export interface RiskConfig {
   maxPositionUsd: number;
   maxTotalExposureUsd: number;
 }
 
-export interface OrderRequest {
+interface BaseOrderRequest {
   symbol: string;
-  side: Side;
   qty: number;
   priceUsd: number;
 }
 
+export type OrderRequest =
+  | (BaseOrderRequest & { side: 'buy'; feeUsd: number })
+  | (BaseOrderRequest & { side: 'sell'; feeUsd?: never });
+
 export interface RiskDecision {
   allowed: boolean;
   reason?: string;
@@ -36,6 +39,17 @@ export class RiskEngine {
   constructor(private config: RiskConfig) {}
 
   check(portfolio: Portfolio, order: OrderRequest): RiskDecision {
+    if (!Number.isFinite(order.qty) || order.qty <= 0) {
+      return { allowed: false, reason: `Invalid order quantity: ${order.qty}` };
+    }
+    if (!Number.isFinite(order.priceUsd) || order.priceUsd <= 0) {
+      return { allowed: false, reason: `Invalid order price: ${order.priceUsd}` };
+    }
+    const notional = order.qty * order.priceUsd;
+    if (!Number.isFinite(notional) || notional <= 0) {
+      return { allowed: false, reason: `Invalid order notional: ${notional}` };
+    }
+
     // Sells of existing positions are always permitted; exposure caps are
     // entry-side only, and Portfolio.applyFill enforces that we don't sell
     // more than we hold.
@@ -47,12 +61,19 @@ export class RiskEngine {
       return { allowed: true };
     }
 
-    const notional = order.qty * order.priceUsd;
+    const feeUsd = order.feeUsd;
+    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`,
       };
     }