fix(brokers): sum robinhood's per-order fee_charged into a real fees_usd - #222
Merged
Conversation
`RobinhoodAdapter.get_fee_summary()` returned `fees_usd=Decimal("0")`
unconditionally. `FeeSummary`'s docstring names subscription-lapse
detection as its consumer, and the contradiction it looks for is a fee
charged while the user claims a fee-free allowance. Pinned at zero, that
check did not error against this venue -- it silently PASSED, for every
account, every time. A rail that always passes is worse than an absent
one, because it reads as coverage.
`fees_usd` is now summed from `GET /api/v2/crypto/trading/orders/`,
filtered to the same trailing 30 days `thirty_day_volume` covers.
Four decisions, each of which could have reintroduced the bug:
* **`updated_at_start`, not `created_at_start`.** Both filters are
documented server-side. A fee is charged when an execution happens and
an execution necessarily bumps `updated_at`, so that result set is a
SUPERSET of the orders carrying an in-window fee and can never omit
one. `created_at_start` drops a GTC bracket that rested past the window
edge and filled inside it -- keel's normal case, not a corner.
* **No `state` filter; every state counted.** A partially-filled-then-
cancelled order ends `canceled` having been charged a real fee, so
filtering to `filled` under-reports. `fee_charged` is documented as the
fee charged based on executed fills, so the field is already its own
state filter.
* **`estimated_fee_remaining` is never read.** It is an estimate of a fee
not yet charged, and `fees_usd` is consumed as an observation.
* **An incomplete sweep raises rather than returning a partial sum.**
`FeeSummary` has no field to mark a total partial, so a truncated sum
would be read as complete -- the same false negative in a new costume.
Cost is 1 + N requests, bounded at 21 by the transport's existing
`_MAX_PAGES`; realistically 2. The server-side window filter is what
stops it growing with the account's total age.
Two residual inaccuracies are documented rather than papered over: an
order straddling the window edge contributes its whole fee (v2's
`executions[]` carry no per-execution fee, so it cannot be split even in
principle -- this over-counts, never under-counts), and the venue's own
`thirty_day_volume` boundary is undocumented, so the windows match in
length and intent but not provably to the second.
`fee_charged`'s JSON quoting is unverified -- no order object has ever
been observed live, and the v2 schema types it unquoted while typing the
neighbouring `executions[].effective_price` quoted -- so both shapes are
read and both are tested. `scripts/robinhood_smoke.py` gains a read-only
`orders` probe so an operator can settle it without placing an order,
and the README records that a clean probe on an account with no history
is a match it has not earned.
Closes #197. Refs #198.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This was referenced Aug 11, 2026
eaitbrahim
added a commit
that referenced
this pull request
Aug 11, 2026
…the CTS scoring fix (#241) A minor bump, not a patch, for three reasons that each require operator action or change behaviour the deployment is currently relying on. SCHEMA. `SCHEMA_VERSION` goes 9 -> 10 (#223). Both deployed databases are at 9 and must be migrated before this build can use them. BEHAVIOUR REQUIRING OPERATOR ACTION. #223 adds a second attested claim -- what CONTRACT a venue listing is, not only what the underlying asset is. It fails closed with no backfill, deliberately, so after this lands `keel assets screen` REJECTS every product with `instrument_wrapper: UNATTESTED` until `keel assets attest-instrument` is run once per product. Live trading is unaffected: rail 1 gates buys on `config.allowlist`, not on the screen. LIVE SCORING CHANGED. #227 fixed `is_round_number`, which returned True for every 2dp-quoted price and so handed BTC/ETH/PAXG a free CTS point on every bar. Scores on those three assets are genuinely lower under this build than under 0.5.7. Also ships: the Robinhood crypto adapter behind the broker port (#216/#218/#222/#229, not wired to the live path), the TUI activity feed (#235/#237), the CTS factor collinearity study (#224), `Preview.synthetic` at the confirm gate (#221), rail 9 seeing a bracket's own stop (#212), and CI gating merges on the `test` check (#234/#238). Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
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.
RobinhoodAdapter.get_fee_summary()returnedfees_usd=Decimal("0")unconditionally.FeeSummary's docstring names subscription-lapse detection as its consumer, and the contradiction it looks for is a fee charged while the user claims a fee-free allowance. Pinned at zero, that check did not error against this venue — it silently passed, for every account, every time. A rail that always passes is worse than an absent one, because it reads as coverage.fees_usdis now summed fromGET /api/v2/crypto/trading/orders/, filtered to the same trailing 30 daysthirty_day_volumecovers.Window: the filter exists, and which one matters
I fetched https://docs.robinhood.com/crypto/trading/ rather than guessing. The page is a Next.js SPA with an empty
__NEXT_DATA__; the real source is a complete OpenAPI 3.0.1 spec embedded in a JS chunk, which I extracted and read directly. The list endpoint documentsaccount_number(required),cursor,created_at_start,created_at_end,updated_at_start,updated_at_end,symbol,side,type,state. Server-side time filters exist, so no client-side sweep-and-discard is needed.updated_at_start, notcreated_at_start. A fee is charged when an execution happens, and an execution necessarily bumpsupdated_at— so that result set is a superset of the orders carrying an in-window fee and can never omit one.created_at_starthas no such property: aStopLimitGTCresting forty days and filling this morning was created outside the window and charged its fee inside it. keel rests GTC brackets by design, so that is the normal case, not a corner. Under-reporting is the false negative this issue is about, so between two imperfect filters the correct one is the one that cannot under-report.Decisions that could each have reintroduced the bug
statefilter; every state counted. A partially-filled-then-cancelled order endscanceledhaving been charged a real fee, so filtering tofilledunder-reports. No filter is needed anyway —fee_chargedis documented as the fee charged based on executed fills, so the field is already its own state filter, reading zero on an order that never traded.estimated_fee_remainingis never read. It is an estimate of a fee not yet charged;fees_usdis consumed as an observation.FeeSummaryhas no field to mark a total partial (fees_usdis a bareDecimal), so a truncated sum is indistinguishable from a complete one — the same always-passing false negative in a new costume. The transport's existing_MAX_PAGESalready raises, andget_fee_summarydoes not catch it. This inverts the_account/cancel_orderrule ("a raise on the way out of a position can trap it") and safely:get_fee_summaryis a reconciliation read, never a step in an unwind.Cost
1 + N requests, N being history pages in the window: one
GET /accounts/plus the page walk, capped at 20. Worst case 21 against a 100 req/min limit with no backoff; realistically 2. The server-side window filter is what stops it growing with the account's total age forever. No bound below_MAX_PAGESwas added — a tighter cap would truncate silently, and truncation is the failure this issue is about.Two residual inaccuracies, documented not papered over
fee_chargedis order-level and v2'sexecutions[]rows carry onlyeffective_price/quantity/timestamp— no per-execution fee — so a fee cannot be split at the boundary even in principle. This over-counts, never under-counts: an over-count points lapse detection at a fee genuinely charged, just slightly earlier than claimed; an under-count hides one.thirty_day_volume's boundary is undocumented; ours is cut from the local clock. They match in length and intent, not to the second. Comparable as magnitudes; do not divide one by the other for an exact effective rate.Unverifiable without placing a real order
fee_charged's JSON quoting has never been observed — and the docs do not settle it, typingfee_chargedas an unquotednumberwhile typing the neighbouringexecutions[].effective_priceas a quoted decimal string. Both shapes are read and both are tested. No order object and no orders-list response has ever been seen live.scripts/robinhood_smoke.pygains a read-onlyordersprobe so this can be settled without placing an order. It verifies the path, signature and pagination envelope unconditionally, but the order-object field names only if the account has history — on a bare accountcompare_shapesskips an<empty>list and prints a match it has not earned. Both the script docstring and the README say so.Gates
TDD: 12 tests written failing first, then implemented.
Closes #197. Refs #198 (its prerequisite item 1 is closed by this and rewritten to name the narrower remaining risk; item 4's request-count claim is corrected).