feat: fee-aware round-trip profit helper + SMA crossover that won't sell at a loss by bennycode · Pull Request #1188 · bennycode/trading-signals

bennycode · 2026-07-11T08:00:31Z

Why

The SMA crossover bleeds money on trading fees: it sells on every bearish cross, including round trips where the sell price barely differs from the buy price — a "win" on raw price that a maker+taker fee round trip quietly turns into a loss. There was no helper anywhere to check whether a sale actually nets a profit after fees.

What

Four commits, bottom-up:

1. `feat(exchange)` — round-trip profit helper

calculateRoundTripProfit({entryPrice, exitPrice, quantity, entryFeeRate, exitFeeRate}) → { netProfit, netProfitPercent, entryCost, exitProceeds, entryFee, exitFee, breakEvenPrice, isProfitable }, plus isProfitableAfterFees(...). Fee-inclusive on both legs; also reports the break-even sell price. Pure util in @typedtrader/exchange, 10 tests.

2. `feat(trading-strategies)` — `FeeAwareSmaCrossoverStrategy`

Extends SmaCrossoverStrategy and gates only the SELL: on a bearish cross it forwards the SELL only when the round trip nets a profit after the taker fee on both legs — otherwise it holds. Buys pass through unchanged. Registered + exported.

3. `feat(trading-signals-docs)` — backtest page

Adds "SMA Crossover (Fee-Aware)" to the backtest UI to compare head-to-head with the plain crossover.

4. `refactor(trading-strategies)` — position tracking on the base `Strategy`

Moves fill-driven position tracking into the root Strategy: a base onFill maintains positionSize and a cost-basis-averaged averageEntryPrice, persisted under a reserved state key. Any strategy can now measure an exit against its entry without its own bookkeeping.

FeeAwareSmaCrossoverStrategy drops its own last-buy tracking and reads averageEntryPrice from the base (correct under partial fills, not just all-in/out).
ProtectedStrategy and TrailingStopStrategy now override onFill with a super.onFill() call so base tracking stays accurate (they keep their own state too, for now — a later cleanup could collapse the duplication).
Adds Strategy.test.ts covering buys, partial sells, full exit, and persistence.

Trade-off (documented in the class)

Skipping unprofitable exits means it can hold through a downtrend rather than take a small loss. Pair with a stop-loss for a hard floor. This is a fee-bleed fix, not a risk-management change.

Verification

exchange: 10 helper tests. trading-strategies: full suite 269 green (incl. new base tracking). Docs tsc clean. Lint clean across all packages.

Add calculateRoundTripProfit() and isProfitableAfterFees(): given an entry price, exit price, quantity, and the buy/sell fee rates, they report whether a sale actually nets a profit after both fees — plus the break-even sell price, net profit, and net profit percent. A naive exit>entry check misses trades that a round trip of fees turns into a loss, which is how a high-churn strategy bleeds money. Not wired into any strategy yet.

Extends SmaCrossoverStrategy and gates the SELL on profitability using the new calculateRoundTripProfit() helper. Since the strategy is all-in / all-out, it just remembers the last buy price (via onFill) and, on a bearish cross, only forwards the SELL when selling at the current price nets a profit after the taker fee on both legs — otherwise it holds. Buys pass through unchanged. Wired into StrategyRegistry and the barrel. Tests show it holds a losing round trip that the plain crossover would sell, and still sells profitable ones. Trade-off (documented): it will hold through a downtrend rather than take a small loss, so pair it with a stop-loss for a hard floor.

…ktest page Add the fee-aware SMA crossover to strategyDefinitions (default fast SMA5@1m vs slow SMA10@2m), a 'fee-aware-sma-crossover' StrategyId, and a createStrategy case built via FeeAwareSmaCrossoverSchema.parse(config). Lets you compare it head to head with the plain SMA Crossover on the bundled datasets and see the fee bleed disappear.

…trategy Add fill tracking to the root Strategy: a base onFill records the most recent BUY and SELL price, exposed as lastBuyPrice / lastSellPrice (Big | undefined) and persisted under a reserved state key. Any strategy can measure an exit against its entry without bookkeeping of its own — no cost-basis averaging, just the latest fill per side. - FeeAwareSmaCrossoverStrategy reads lastBuyPrice from the base (all-in/all-out, so the last buy is the current entry) - ProtectedStrategy and TrailingStopStrategy override onFill with a super.onFill() call so base tracking stays accurate (they keep their own state too) - Base Strategy always keeps a state object; restored fills are preserved - Adds Strategy.test.ts covering last buy/sell tracking and persistence

bennycode · 2026-07-12T09:52:05Z


+const LAST_FILLS_STATE_KEY = '__lastFills__';
+
+/** Most recent fill price per side, persisted alongside the strategy's own state under a reserved key. */


@claude comment is repetitive

Dropped the repetitive type/field comments on LastFills — 671c62c.

bennycode · 2026-07-12T09:52:14Z

+
+/** Most recent fill price per side, persisted alongside the strategy's own state under a reserved key. */
+type LastFills = {
+  /** Price of the most recent BUY fill, as a Big string. `null` until the first buy. */


@claude comment is repetitive

Dropped — the field name says it — 671c62c.

bennycode · 2026-07-12T09:52:17Z

+type LastFills = {
+  /** Price of the most recent BUY fill, as a Big string. `null` until the first buy. */
+  lastBuyPrice: string | null;
+  /** Price of the most recent SELL fill, as a Big string. `null` until the first sell. */


@claude comment is repetitive

Dropped — 671c62c.

bennycode · 2026-07-12T09:53:34Z

 import type {OrderAdvice, TradingSessionState, TradingSessionStrategy} from '../trader/index.js';
 import type {MarketType} from './MarketType.js';

+const LAST_FILLS_STATE_KEY = '__lastFills__';


@claude prefix this with the strategy name instead of doing __

Renamed to strategyLastFills (readable, name-prefixed) instead of the __ convention — 671c62c.

bennycode · 2026-07-12T09:54:35Z


+  /**
+   * Records the most recent BUY / SELL fill price, exposed as {@link lastBuyPrice} /
+   * {@link lastSellPrice}, so a strategy can measure an exit against its entry without


@claude don't link classes and variables here all the time, it's prone when refactoring. Keep comments read-able for people and make them less technical.

Removed the {@link} refs and rewrote the comments as plain prose — 671c62c.

bennycode · 2026-07-12T09:55:12Z

+   * Records the most recent BUY / SELL fill price, exposed as {@link lastBuyPrice} /
+   * {@link lastSellPrice}, so a strategy can measure an exit against its entry without
+   * bookkeeping of its own. Subclasses that override `onFill` MUST call
+   * `super.onFill(fill, state)` to keep this accurate.


@claude can we enforce with an eslint rule that super is being called when overwriting?

There's no built-in ESLint rule for 'must call super in an override' (only framework-specific plugins like React lifecycle rules), so it'd need a custom rule. Rather than enforce the super call, I removed the need for it: onFill now records the last buy/sell price and then calls a processFill hook that subclasses override — same template-method pattern as onCandle/processCandle. Base tracking always runs and there's nothing to forget. ProtectedStrategy/TrailingStop/Scalp now override processFill. Done in 671c62c.

bennycode · 2026-07-12T09:55:34Z

+    this.#setLastFills(patch);
+  }
+
+  /** Price of the most recent BUY fill, or `undefined` if there hasn't been one. */


@claude comment is repetitive

Removed the getter doc (name + return type already say it) — 671c62c.

bennycode · 2026-07-12T09:55:39Z

+    return price === null ? undefined : new Big(price);
+  }
+
+  /** Price of the most recent SELL fill, or `undefined` if there hasn't been one. */


@claude comment is repetitive

Removed — 671c62c.

bennycode · 2026-07-12T09:55:59Z

+
+  #setLastFills(patch: Partial<LastFills>): void {
+    /*
+     * Reassigning the reserved top-level key trips the state Proxy's set trap, which fires


@claude comment is repetitive

Trimmed to a one-line note about why the whole key is replaced — 671c62c.

- Rename the reserved state key from '__lastFills__' to 'strategyLastFills' (readable, name-prefixed rather than the __ convention) - Trim repetitive doc comments and drop {@link} cross-references; keep the remaining comments plain and about intent - Replace the 'subclasses must call super.onFill()' footgun with a template method: onFill records the last buy/sell price and then calls processFill, the hook subclasses override — so base tracking can never be skipped, no eslint rule needed. ProtectedStrategy/TrailingStop/Scalp now override processFill (Scalp still chains super.processFill for cost-basis tracking) - Update strategy-protected README and Strategy.test.ts accordingly

…estoreState Extend the pattern from onFill to the other two 'override + must call super' spots: - onCandle now delegates the decision to a decideAdvice hook (default: processCandle) while keeping the lastBatchedCandle / latestAdvice bookkeeping in the base. ProtectedStrategy overrides decideAdvice for its kill-switch gate instead of reimplementing onCandle, so the bookkeeping can't drift. - restoreState now stores the snapshot in the base and calls a hydrateState hook. BuyOnce/Protected/Scalp/TrailingStop override hydrateState; nobody overrides restoreState anymore, so base state restoration can't be skipped. Subclasses of ProtectedStrategy chain super.hydrateState for its guard-state restore. - Strategy.test.ts gains hooks coverage

bennycode · 2026-07-12T10:20:48Z

Followed the template-method approach through to the other two 'override + must call super' spots (the same footgun behind the onFill eslint question):

onCandle → base keeps the lastBatchedCandle/latestAdvice bookkeeping and delegates to a decideAdvice hook. ProtectedStrategy overrides decideAdvice for its kill-switch gate instead of reimplementing onCandle.
restoreState → base stores the snapshot and calls a hydrateState hook. Nobody overrides restoreState anymore, so base state restoration can't be skipped; ProtectedStrategy subclasses chain super.hydrateState for guard-state restore.

Full suite 270 green, tsc + lint clean. cc77b1f

…dStrategy Per review preference for no averaging anywhere. ProtectedStrategy no longer accumulates totalCostBasis/totalPositionSize; its guards now measure from the base strategy's last buy price and the live base balance (matching TrailingStop). - Remove the processFill cost-basis tracker and the totalCostBasis/totalPositionSize state; add seedEntryPrice for seedFromBalance (attaching to an externally-opened position with no buy fill of its own) - Guards read state.baseBalance for quantity and hasSellablePosition for the position check; entry = lastBuyPrice ?? seedEntryPrice - Base Strategy now restores its last buy/sell price on restoreState, so guards that key off lastBuyPrice survive a restart - Behavior change: multi-tranche entries measure from the last buy, not a weighted average (identical for all-in/all-out strategies) - Rewrite ProtectedStrategy tests (live balance + last buy) and fix the BuyAndHold alias tests to pass a held balance

bennycode · 2026-07-12T11:51:35Z

@@ -130,6 +130,14 @@ export const strategyDefinitions: StrategyDefinition[] = [
    schema: SmaCrossoverSchema,
    getDefaultConfig: () => ({fastPeriod: 5, fastTimeframe: '1m', slowPeriod: 10, slowTimeframe: '2m'}),


@claude let's rename "Timeframe" into "Resolution" - across the codebase for SmaCrossover strategy.

bennycode added 3 commits July 11, 2026 09:42

github-actions Bot added the type: feature ✨ label Jul 11, 2026

bennycode force-pushed the feat/round-trip-profit-helper branch from e3f29e2 to 6f1d332 Compare July 11, 2026 13:00

bennycode commented Jul 12, 2026

View reviewed changes

bennycode force-pushed the feat/round-trip-profit-helper branch from a8e8760 to 671c62c Compare July 12, 2026 10:03

bennycode commented Jul 12, 2026

View reviewed changes


		const LAST_FILLS_STATE_KEY = '__lastFills__';

		/** Most recent fill price per side, persisted alongside the strategy's own state under a reserved key. */

		@@ -130,6 +130,14 @@ export const strategyDefinitions: StrategyDefinition[] = [
		schema: SmaCrossoverSchema,
		getDefaultConfig: () => ({fastPeriod: 5, fastTimeframe: '1m', slowPeriod: 10, slowTimeframe: '2m'}),

Uh oh!

Conversation

bennycode commented Jul 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Why

What

1. feat(exchange) — round-trip profit helper

2. feat(trading-strategies) — FeeAwareSmaCrossoverStrategy

3. feat(trading-signals-docs) — backtest page

4. refactor(trading-strategies) — position tracking on the base Strategy

Trade-off (documented in the class)

Verification

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bennycode commented Jul 12, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

bennycode commented Jul 11, 2026 •

edited

Loading

1. `feat(exchange)` — round-trip profit helper

2. `feat(trading-strategies)` — `FeeAwareSmaCrossoverStrategy`

3. `feat(trading-signals-docs)` — backtest page

4. `refactor(trading-strategies)` — position tracking on the base `Strategy`