fix(exchange): harden Trading212 order watcher and fee mapping#1184
Open
bennycode wants to merge 2 commits into
Open
fix(exchange): harden Trading212 order watcher and fee mapping#1184bennycode wants to merge 2 commits into
bennycode wants to merge 2 commits into
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR hardens the Trading212 broker integration in packages/exchange by fixing several latent correctness and reliability issues discovered during the #1181 test-suite effort, focusing on order watching stability, cursor paging behavior, order-cancel safety, and fee/tax aggregation correctness.
Changes:
- Prevents
watchOrdersfrom crashing the process by guarding'error'emissions and falling back toconsole.warnwhen no listeners exist. - Fixes
watchOrderspaging/cursor behavior so fill-free or cancellation-heavy histories don’t trigger full-history re-scans and re-paging. - Corrects filled-order fee mapping by netting signed tax lines (subtracting rebates) and clamping rare net credits to zero, with regression tests.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| packages/exchange/src/broker/trading212/Trading212BrokerMapper.ts | Net signed tax lines using big.js and clamp net credits to zero for neutral Fill.fee. |
| packages/exchange/src/broker/trading212/Trading212BrokerMapper.test.ts | Adds/updates tests asserting rebate subtraction and net-credit clamping behavior. |
| packages/exchange/src/broker/trading212/Trading212Broker.ts | Hardens watchOrders error emission, improves cursor seeding/advancement, and aligns cancel filtering with list visibility via a shared predicate. |
| packages/exchange/src/broker/trading212/Trading212Broker.test.ts | Adds regression tests for cursor seeding/advancement, cancel/list symmetry, and no-listener 'error' handling. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…act' into fix/trading212-watcher-hardening
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes four latent Trading212 bugs documented during the #1181 test-suite work (
packages/exchange/src/broker/trading212/).1. Unhandled
'error'emission can crash the processScenario:
watchOrders' polling tick emits'error'on the broker EventEmitter when a poll fails.'error'is Node's reserved event — emitting it with zero listeners throws synchronously, so the first transient API failure inside the timer callback kills the whole process (which also hosts other sessions and the Telegram bot).Fix: The tick emits
'error'only whenlistenerCount('error') > 0and falls back to aconsole.warnotherwise. Polling always survives to the next tick. Listener-attached behavior is unchanged (backward compatible).2. Empty-baseline full-history paging
Scenario: The baseline cursor (
lastSeenId) was seeded from FILLED orders only. On an account whose history holds no fill yet, it stayed at 0, so every poll tick paged the ENTIRE order history — compounded by Trading212's 1 req/60s rate limit on/equity/history/orders.Fix: The cursor is seeded from the newest order of ANY status — it exists to bound paging, not to mark fills. Ticks also advance it past non-FILLED entries (cancellations/rejections) so they aren't re-scanned forever. No-replay guarantee preserved: everything in the baseline snapshot predates
watchOrdersand is never emitted.3. cancel/list asymmetry wipes manual protective stops
Scenario:
getOpenOrdersdeliberately filters out STOP/STOP_LIMIT/VALUE orders (they don't fit the neutral LIMIT/MARKET model), butcancelOpenOrderscancelled them anyway — a session's cancel-before-place silently wiped a user's manually placed stop-loss it never showed.Fix: Both methods now share one predicate (
#fitsNeutralOrderModel) with the invariant documented at its definition and referenced at the cancel site: a session must never cancel an order it cannot list.4. Fee summation uses
Math.abson tax linesScenario: The fill mapper summed
Math.absof allwalletImpact.taxes[].quantitylines. Tax lines are signed (debits negative, credits/rebates positive), so a rebate line would inflate the reported fee instead of reducing it.Fix: Lines are netted with big.js (matching the surrounding arithmetic). Since neutral
Fill.feeis a cost that downstream P&L accumulates additively (totalFees.plus(fill.fee)inBacktestExecutor), the rare net credit is clamped to zero rather than reported as a negative fee — documented at the site.Test plan
'error'listener attached and keeps polling (console.warnfallback asserted)nextPagePath; later fills still emittedcancelOpenOrdersspares STOP/STOP_LIMIT/VALUE orders and still cancels neutral-model orders of the requested ticker'0'npm testinpackages/exchange: 12 files, 153 tests passednpm run lintinpackages/exchange: 0 errorsnpm run dist(tsc) passes