From a218b70fcfce1d06da47873892b63999ce79a343 Mon Sep 17 00:00:00 2001 From: Elmehdi Aitbrahim Date: Sun, 9 Aug 2026 19:27:20 -0400 Subject: [PATCH] fix(brokers): sum robinhood's per-order fee_charged into a real fees_usd `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) --- packages/keel-broker-robinhood/README.md | 106 +++++++-- .../keel_broker_robinhood/adapter.py | 150 +++++++++++-- .../keel_broker_robinhood/transport.py | 42 ++++ scripts/robinhood_smoke.py | 20 +- tests/broker_robinhood/test_adapter.py | 208 +++++++++++++++++- tests/broker_robinhood/test_transport.py | 42 ++++ .../conformance/test_robinhood_conformance.py | 7 + tests/fixtures/rh_orders.json | 98 +++++++++ tests/scripts/test_robinhood_smoke.py | 12 +- 9 files changed, 639 insertions(+), 46 deletions(-) create mode 100644 tests/fixtures/rh_orders.json diff --git a/packages/keel-broker-robinhood/README.md b/packages/keel-broker-robinhood/README.md index d2cc294d..6cf3a22b 100644 --- a/packages/keel-broker-robinhood/README.md +++ b/packages/keel-broker-robinhood/README.md @@ -10,7 +10,7 @@ API v2. | Balances | Per-holding `Balance` plus one for the account's `buying_power`. | | Order status | `get_order` normalizes a Robinhood order object to `OrderStatus`. | | Cancel | `cancel_order` confirms from the venue's returned order, with one re-poll. | -| Fee summary | Rates from `fee_tier_status.fee_ratio`, `volume_usd` from `thirty_day_volume`. | +| Fee summary | Rates from `fee_tier_status.fee_ratio`, `volume_usd` from `thirty_day_volume`, `fees_usd` summed from 30 days of order history. | | Preview | Synthetic only (`synthetic=True`) -- there is no native preview endpoint. | Three order kinds are supported: `MarketIOCByBase` (market, sized in the asset), `LimitGTC` @@ -59,6 +59,71 @@ does not validate the order, check buying power, check the account's own size bo anything, so an order it prices happily can still be rejected the instant it is placed. `supports_native_preview` stays `False` and `synthetic` stays `True`. +### How `fees_usd` is built, and what it can still get wrong + +`get_fee_summary().fees_usd` used to be a hardcoded `Decimal("0")`. That was not a cosmetic gap +(#197). `FeeSummary`'s own 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*. A +constant zero can never contradict anything, so against this venue the check did not fail loudly +-- it **passed, every time, for every account**. A rail that always passes is worse than an +absent one, because it reads as coverage. + +The v2 API still publishes no account-level fees-paid total, so the number is built the only way +the API allows: `GET /api/v2/crypto/trading/orders/`, filtered to the same trailing 30 days +`thirty_day_volume` covers, with each order's `fee_charged` summed. + +Four decisions in that sentence are load-bearing: + +**The window filter is `updated_at_start`, not `created_at_start`.** Both are documented and +either would compile. A fee is charged when an execution happens, and an execution necessarily +bumps `updated_at` -- so the `updated_at_start` result set is a *superset* of the orders carrying +an in-window fee and can never omit one. `created_at_start` has no such property: a `StopLimitGTC` +resting for 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 this engine's normal case, not a corner. +Under-reporting is the false negative #197 exists to close, so between two imperfect filters the +correct one is the one that cannot under-report. + +**No `state` filter is sent, and every state is counted.** `state` is the obvious narrowing and it +is a trap: an order that partially fills and is then cancelled ends `canceled` while having been +charged a real fee on the part that executed, and filtering to `filled` would drop it. No filter is +needed anyway -- `fee_charged` is 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_remaining` is never read.** The neighbouring v2 field is the fee that *will* be +charged on an order's unfilled remainder -- explicitly conditional, explicitly an estimate. +`fees_usd` is consumed as an observation, and an estimate cannot honestly contradict anything. + +**An incomplete sweep raises rather than returning a partial sum.** `FeeSummary` has no field to +mark a total as partial (`fees_usd` is a bare `Decimal`), so a truncated sum is indistinguishable +from a complete one and would be read as an observation -- the same always-passing false negative +in a new costume. `RobinhoodTransport._paginate` already raises past `_MAX_PAGES`, and +`get_fee_summary` does not catch it. This inverts the rule `_account` and `cancel_order` follow +("a raise on the way out of a position can trap it"), and safely: `get_fee_summary` is a +reconciliation read, never a step in an unwind. + +Cost is **1 + N requests**, N being the history pages in the window: one `GET /accounts/` plus the +page walk, capped at 20. Worst case 21 per call against a 100 req/min limit with no backoff in the +transport; realistically 2 for an account trading a handful of times a month. The server-side +window filter is what stops that growing with the account's total age forever. + +Two things this still cannot get exactly right, both stated so nobody reads more into the number +than it carries: + +- **An order straddling the window edge contributes its whole fee.** `fee_charged` is an + *order*-level total, and v2's `executions[]` rows carry only `effective_price`, `quantity` and + `timestamp` -- no per-execution fee -- so a fee cannot be split at the boundary even in + principle. This over-counts, never under-counts, which is the survivable direction: an + over-count points lapse detection at a fee that was genuinely charged, just slightly earlier + than the window claims; an under-count hides one. +- **The window is not provably identical to the venue's own.** `thirty_day_volume`'s boundary is + undocumented (it may be calendar-day aligned, it may exclude today) while this window is cut + from the local clock. The two match in length and intent, not necessarily to the second. Treat + `fees_usd` and `volume_usd` as comparable magnitudes over the same nominal window; do not divide + one by the other to derive an exact effective rate. + +⚠️ **No order object, and no response from the orders LIST endpoint, has ever been observed +live** -- see "No sandbox" below. Every field name above is read from the documentation alone. + ## What does NOT work ### No candles @@ -130,13 +195,15 @@ object requires placing a real order, which that script refuses by construction. names are still read from the documentation alone, and `place_order` / `get_order` / `cancel_order` all depend on them. -### `fees_usd` is always zero - -`get_fee_summary().fees_usd` is hardcoded to `Decimal("0")`. The API exposes a fee *rate* -(`fee_tier_status.fee_ratio`) and a trailing volume figure, but no account-level total of fees -actually paid. keel's subscription-lapse detection reads `fees_usd` to notice a fee charged while -the user claims a fee-free allowance; against this venue that check cannot fire, and lapse -detection here has to fall back on the venue subscription attestation alone. +The script gained a sixth probe with #197, against the orders LIST endpoint and +`rh_orders.json`. It is a GET, so it stays inside the read-only guarantee, and it does verify the +list endpoint's path, that the signature is accepted, and the pagination envelope. **It verifies +the order OBJECT's field names only if the account happens to have order history.** On an account +that has never traded crypto here, `results` comes back empty and `compare_shapes` skips a list +whose rows are `` -- so the report prints a shape match it has not earned. An operator +reading a clean `orders` line must check whether any row came back before treating it as +corroboration. `fee_charged`'s JSON quoting -- quoted string or unquoted number, unknown at this +venue and undecided by its own docs -- is exactly what one real row would settle. ## Not wired to the live path @@ -153,15 +220,15 @@ The gaps above are capability limits -- things this venue cannot do, which the a honestly. The list below is different: these are places where wiring this adapter up **as it stands** would degrade a safety property keel already has. Phase B must trip over this section. -1. **`fees_usd` is always zero, so subscription-lapse detection is inert AND always-passing - against this venue.** This is not merely "a missing number". `FeeSummary.fees_usd` is the - field lapse detection reads to notice a fee charged while the user claims a fee-free - allowance. A constant zero can never contradict the claim, so the check does not fail - loudly -- it *passes*, every time, for every account. Anything consuming a Robinhood - `FeeSummary` must treat `fees_usd` as "not reported", never as "no fees were charged", and - the migration must decide whether an always-passing check is acceptable or whether the venue - should be excluded from that check by name. Closing it properly means paging order history - and summing per-order `fee_charged`, which needs its own rate-limit design. +1. **`fees_usd` is now summed from order history (#197), but the sum has never been checked + against a real order.** The always-zero constant is gone, so subscription-lapse detection is + no longer inert-and-always-passing here -- that item is closed. What replaces it is narrower + and must not be skipped: the sum is built from field names (`fee_charged`, `updated_at`, the + list envelope) that no live response has ever corroborated, because observing an order object + requires placing a real order. If `fee_charged` is spelled differently at the venue, every + row parses to `None` and the total silently returns to zero -- the original failure, arrived + at from a different direction. Run `scripts/robinhood_smoke.py` against an account with order + history before trusting the number, and confirm the `orders` probe actually returned a row. 2. **A fresh `client_order_id` per `place_order` call means no retry is ever deduplicated.** The uuid is minted per ATTEMPT, so a caller that retries after a timeout -- exactly when the @@ -184,7 +251,10 @@ stands** would degrade a safety property keel already has. Phase B must trip ove 4. **No rate limiting or backoff.** Robinhood allows 100 requests/minute sustained (300 burst) and this transport does not throttle or retry. Per-call account caching keeps each public - adapter method to a single `GET /accounts/`, but nothing bounds the engine's aggregate rate. + adapter method to a single `GET /accounts/`, but nothing bounds the engine's aggregate rate -- + and `get_fee_summary` is no longer a single-request method: its order-history sweep is 1 + N + requests, bounded at 21 by `_MAX_PAGES`. Whatever calls it in Phase B should call it on a + schedule, not per order. 5. **No candle source is composed.** Point 1 of "What does NOT work" means this adapter cannot be a venue's sole broker; Phase B has to decide how an engine pairs an execution venue that diff --git a/packages/keel-broker-robinhood/keel_broker_robinhood/adapter.py b/packages/keel-broker-robinhood/keel_broker_robinhood/adapter.py index 7159eb79..ab4e25fb 100644 --- a/packages/keel-broker-robinhood/keel_broker_robinhood/adapter.py +++ b/packages/keel-broker-robinhood/keel_broker_robinhood/adapter.py @@ -46,6 +46,7 @@ import time import uuid from dataclasses import dataclass +from datetime import UTC, datetime from decimal import Decimal, InvalidOperation from typing import Any @@ -127,6 +128,24 @@ _TOTAL_TOLERANCE_ABS = Decimal("0.01") _TOTAL_TOLERANCE_RATIO = Decimal("0.0001") +#: How far back `get_fee_summary` sums `fee_charged`, in seconds. +#: +#: Thirty days, and it is not a tunable: it is pinned to the window `volume_usd` already reports. +#: `FeeSummary` carries ONE `volume_window` for both figures, so a fee total covering a different +#: span than `fee_tier_status.thirty_day_volume` would be mislabelled by the very field that +#: exists to stop a caller comparing incompatible windows (see `FeeSummary`'s own docstring). Any +#: change here is a change to `volume_window`'s truthfulness, not a knob. +_FEE_WINDOW_SECONDS = 30 * 24 * 60 * 60 + +#: How the fee window's start is rendered for `updated_at_start`. +#: +#: The v2 docs specify ISO 8601 for this parameter. Seconds precision with an explicit `Z` is the +#: unambiguous form: no offset to be misread as local time, and no fractional part for the venue +#: to parse differently than we wrote it. `datetime.isoformat()` is deliberately not used -- it +#: renders UTC as `+00:00`, and that `+` on a SIGNED query string is the exact hazard +#: `transport._request` documents at `quote_via=quote` (a raw `+` decodes server-side as a space). +_WINDOW_FORMAT = "%Y-%m-%dT%H:%M:%SZ" + @dataclass(frozen=True) class _VenueEstimate: @@ -167,9 +186,10 @@ class RobinhoodAdapter: v1's cancel endpoint answers `text/plain` "Cancel request was submitted", which is an acknowledgement of a REQUEST and cannot satisfy `cancel_order`'s "return `True` only when the venue confirms the cancellation for THIS order id"; and v1 carries neither the per-order - `fee_charged` that `get_order` needs for observed economics nor the `fee_tier_status` that - `get_fee_summary` is built from. An adapter written against v1 would have to guess at all - three, and guessing is the thing the port exists to prevent. + `fee_charged` that `get_order` needs for observed economics -- and that `get_fee_summary` now + sums into a real `fees_usd` (#197) -- nor the `fee_tier_status` its rates come from. An + adapter written against v1 would have to guess at all three, and guessing is the thing the + port exists to prevent. """ def __init__(self, transport: Transport | None = None) -> None: @@ -577,7 +597,7 @@ def place_order(self, spec: OrderSpec) -> PlaceResult: return PlaceResult(success=True, broker_order_id=str(order_id)) def get_fee_summary(self) -> FeeSummary: - """Map v2's `fee_tier_status` to a `FeeSummary`. Read the `fees_usd` note below. + """Map v2's `fee_tier_status` to a `FeeSummary`, with `fees_usd` summed from order history. `volume_window` is `"trailing_30d"`, and unlike Coinbase's `"unknown"` that is a statement the docs actually support: the field is literally named `thirty_day_volume`. @@ -591,17 +611,35 @@ def get_fee_summary(self) -> FeeSummary: to both cases. The alternative, zeroing `maker_rate`, would claim resting orders trade free, which nothing supports. - ⚠️ `fees_usd` is always `Decimal("0")`, and this is a REAL GAP, not a formality. The v2 - API exposes per-order `fee_charged` but no account-level fees-paid total, and this method - has no order history to sum. `FeeSummary`'s docstring says subscription lapse detection - leans on `fees_usd` -- specifically, a fee charged while the user claims a fee-free - allowance contradicts the claim. A constant zero can never contradict anything, so - against this venue that test is inert and detection falls back to attestation alone. - Anything consuming this must treat a Robinhood `fees_usd` as "not reported", never as - "no fees were charged". Closing this properly means paging order history and summing - `fee_charged`, which is a follow-up with its own rate-limit design. + **`fees_usd` used to be a hardcoded `Decimal("0")` and is now observed (#197).** That + constant was not a cosmetic gap. `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. A constant zero can never contradict anything, so the + 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. + + The v2 API still publishes no account-level fees-paid total, so the number is built the + only way the API allows: `GET /orders/` filtered to the same trailing 30 days + `thirty_day_volume` covers, with each order's `fee_charged` summed. See `_fees_paid` for + the window, the cost, and the two places this total can be wrong. + + **One reading of the clock feeds both the window and `fetched_at`.** Calling `time.time()` + twice would let the window the fees were summed over drift away from the timestamp the + summary is reported against, for no benefit -- and a consumer comparing `fees_usd` to + `volume_usd` has no way to notice that drift. + + ⚠️ **This method can now raise, and that is deliberate.** `_fees_paid` lets a failed or + non-terminating sweep propagate rather than returning a partial sum. That inverts the rule + `_account` and `cancel_order` follow ("a raise on the way out of a position can trap it"), + and the inversion is safe for one specific reason: `get_fee_summary` is a reconciliation + read, not a step in an unwind. Nothing calls it while a position is being closed. On the + exit path a raise costs a trapped position; here it costs an error message, and the + alternative -- an under-reported total -- costs exactly the false negative this issue is + about. """ - # One `GET /accounts/` for this whole method: the account is resolved once and passed to + # One reading of the clock for the window and the reported timestamp. See the docstring. + fetched_at = int(time.time()) + # One `GET /accounts/` for the rate and volume: the account is resolved once and passed to # `_fee_ratio`, which used to fetch its own and made this two round trips for one answer. account = self._account() tier = _field(account, "fee_tier_status") or {} @@ -611,11 +649,91 @@ def get_fee_summary(self) -> FeeSummary: taker_rate=ratio, maker_rate=ratio, volume_usd=Decimal(str(_field(tier, "thirty_day_volume", "0") or "0")), - fees_usd=Decimal("0"), + fees_usd=self._fees_paid(fetched_at - _FEE_WINDOW_SECONDS), volume_window="trailing_30d", - fetched_at=int(time.time()), + fetched_at=fetched_at, ) + def _fees_paid(self, since: int) -> Decimal: + """Total `fee_charged` across every order the venue reports touched since `since`. + + This is the whole of #197's fix, and it is worth reading for what it can still get wrong + as much as for what it does. + + **Window: `updated_at`, not `created_at`, and the choice is load-bearing.** Both filters + are documented and either would compile. A fee is charged when an execution happens, and + an execution necessarily bumps `updated_at` -- so the `updated_at_start` result set is a + SUPERSET of the orders carrying an in-window fee and can never omit one. `created_at_start` + has no such property: a `StopLimitGTC` resting for forty days and filling this morning was + created outside the window and charged its fee inside it, so filtering on creation drops a + real charge. keel rests GTC brackets by design, so that is this engine's normal case + rather than a corner. Under-reporting is the false negative this issue exists to close, so + between two imperfect filters the correct one is the one that cannot under-report. + + **Every state is counted, and no `state` filter is sent.** `state` is the obvious-looking + narrowing and it is a trap: an order that partially fills and is then cancelled ends + `canceled` while having been charged a real fee on the part that executed, and `filled` + would drop it. No filter is needed anyway, because `fee_charged` is documented as "the + total fee amount that was charged for this order based on executed fills" -- the field is + already its own state filter, reading zero on an order that never traded. A state filter + layered on top could only remove real charges. + + **`estimated_fee_remaining` is never read.** The neighbouring v2 field is the fee that + will be charged on an order's UNFILLED remainder, explicitly conditional and explicitly an + estimate. `fees_usd` is consumed as an observation contradicting a fee-free claim, and an + estimate cannot honestly contradict anything. + + **Cost: 1 + N requests, where N is the number of history pages in the window.** Against a + 100 req/min sustained limit and a transport with no backoff, the bound matters: the + account read is one request and the sweep is `RobinhoodTransport._paginate`'s page walk, + capped at `_MAX_PAGES` (20). So the worst case is 21 requests per call, and the realistic + case for a keel account trading a handful of times a month is 2. The server-side window + filter is what keeps that from growing with the account's total age. + + **A sweep that cannot complete RAISES rather than returning what it collected.** + `_paginate` already raises past `_MAX_PAGES` and this method does not catch it. There is + no field on `FeeSummary` to mark a total as partial -- `fees_usd` is a bare `Decimal` -- + so a truncated sum is indistinguishable from a complete one and would be consumed as an + observation. That is the same always-passing false negative in a new costume. An exception + is visible; a confidently wrong number is not. + + ⚠️ **The one thing this cannot get exactly right: an order straddling the window edge.** + `fee_charged` is an ORDER-level total, and the v2 `executions[]` rows carry only + `effective_price`, `quantity` and `timestamp` -- no per-execution fee. So a fee cannot be + split across the boundary even in principle. An order that filled partially before `since` + and again after it contributes its WHOLE fee. That over-counts, never under-counts, which + is the survivable direction: an over-count makes lapse detection point at a fee that was + genuinely charged, merely slightly earlier than the window claims, whereas an under-count + hides one entirely. + + ⚠️ **Nor is the window provably identical to the venue's own.** `thirty_day_volume`'s + boundary is not documented -- it may be calendar-day aligned, it may exclude today -- while + this window is cut from the local clock. The two match in LENGTH and intent; they are not + guaranteed to match to the second. `fees_usd` and `volume_usd` are therefore comparable as + magnitudes over the same nominal window and must not be used to derive an exact effective + rate. + + A negative `fee_charged` is skipped rather than subtracted. Nothing documents this field + going negative, so a negative is a rebate or a venue bug -- and under both readings, + letting it net out a real charge would hide the very thing being looked for. + """ + started = datetime.fromtimestamp(since, tz=UTC).strftime(_WINDOW_FORMAT) + rows = _results(self._require_transport().get_orders(updated_at_start=started)) + + total = Decimal("0") + for row in rows: + # `_decimal_or_none` reads a quoted string and an unquoted number identically, which + # is required rather than merely tolerant here: this venue mixes the two (#217 F6) and + # `fee_charged`'s own quoting has never been observed, because observing an order + # object requires placing a real order and there is no sandbox. The v2 schema types it + # as an unquoted number and types the neighbouring `executions[].effective_price` as a + # quoted decimal string, so the documentation does not settle it either. + fee = _decimal_or_none(_field(row, "fee_charged")) + if fee is None or fee <= 0: + continue + total += fee + return total + def get_order(self, order_id: str) -> OrderStatus: """Observed state of a previously placed order, normalized to `Decimal` money fields. diff --git a/packages/keel-broker-robinhood/keel_broker_robinhood/transport.py b/packages/keel-broker-robinhood/keel_broker_robinhood/transport.py index b1e57dbb..5f0d9913 100644 --- a/packages/keel-broker-robinhood/keel_broker_robinhood/transport.py +++ b/packages/keel-broker-robinhood/keel_broker_robinhood/transport.py @@ -55,6 +55,8 @@ def get_estimated_price(self, symbol: str, side: str, quantity: str) -> Any: ... def create_order(self, body: dict[str, Any]) -> Any: ... + def get_orders(self, updated_at_start: str | None = None) -> Any: ... + def get_order(self, order_id: str) -> Any: ... def cancel_order(self, order_id: str) -> Any: ... @@ -482,6 +484,46 @@ def create_order(self, body: dict[str, Any]) -> Any: body=body, ) + def get_orders(self, updated_at_start: str | None = None) -> Any: + """List this account's orders, newest-first, with every page already concatenated. + + This exists for `adapter.get_fee_summary`, which sums each order's `fee_charged` to + produce a real `fees_usd` (#197). The v2 order LIST endpoint is the only place that total + can come from: the API publishes a fee *rate* and a trailing volume at the account level + and no fees-paid figure anywhere. + + **`updated_at_start` is a real, documented, SERVER-side filter and that is what makes the + sweep affordable.** https://docs.robinhood.com/crypto/trading/ documents this endpoint + with `account_number` (required), `cursor`, `created_at_start`, `created_at_end`, + `updated_at_start`, `updated_at_end`, `symbol`, `side`, `type` and `state`. Without a + server-side window the caller would have to page the account's ENTIRE order history on + every fee summary and discard most of it client-side, which against a 100 req/min limit + and a transport with no backoff is a cost that grows with the account's age forever. With + it, the sweep is bounded by how much the account traded in the window instead. + + Only the one filter is threaded through, deliberately. Every other documented parameter + would NARROW the result set, and this is the one caller for whom a narrower set is a + wrong answer: `state` in particular looks like the obvious filter and would drop a + partially-filled-then-cancelled order, whose fee was really charged. See + `adapter._fees_paid` for why `updated_at` rather than `created_at`. + + ⚠️ **Neither this endpoint nor any order object it returns has ever been observed live.** + `scripts/robinhood_smoke.py` can now probe it read-only, but until an operator with a real + credential runs that, the envelope shape, the page size, and every field name below the + `results` key are read from the documentation alone -- the same standing on which the + `rh_order_*.json` fixtures sit, and the same standing that #217 proved wrong four times + over on the endpoints that COULD be probed. + + `limit` is not sent: the docs' pagination section says only "some of our endpoints support + this query parameter" and directs the reader to each endpoint's own parameter list, and + this endpoint's list does not carry it. An unsupported param is not free here -- it is + signed, so a guess the venue rejects is a 401 rather than a helpful 400. + """ + return self._paginate( + "/api/v2/crypto/trading/orders/", + params={"account_number": self._account(), "updated_at_start": updated_at_start}, + ) + def get_order(self, order_id: str) -> Any: """Fetch one order; `None` only if Robinhood's 404 says this id does not exist. diff --git a/scripts/robinhood_smoke.py b/scripts/robinhood_smoke.py index 0f96ad1c..9ffeef45 100644 --- a/scripts/robinhood_smoke.py +++ b/scripts/robinhood_smoke.py @@ -20,10 +20,20 @@ The first run of it (#217) settled all three: ten requests, zero 401s, every endpoint path correct, `fee_tier_status` corroborated key for key -- and four fixture shapes wrong, one of them a live defect that left every market preview unpriced. It also produced five false positives of -its own, which `fixture_shape` below exists to prevent recurring. What it still cannot reach is -the ORDER lifecycle: `rh_order_open.json`, `rh_order_filled.json` and `rh_order_canceled.json` -describe objects that only exist once a real order has been placed, and this script refuses to -place one. Those three fixtures remain unverified against the venue. +its own, which `fixture_shape` below exists to prevent recurring. What it still cannot fully +reach is the ORDER lifecycle. The `orders` probe added for #197 stays inside the read-only +guarantee -- it is a GET, so `_ReadOnly` still enforces it at the request layer -- and it DOES +verify the list endpoint's path, that the signature is accepted, and the pagination envelope. +What it verifies only conditionally is the order OBJECT's field names: that happens **only if +the account happens to have order history**. On an account that has never traded crypto on +Robinhood, `results` comes back empty, and `compare_shapes` skips comparing a list whose rows +are `` -- so a bare account reports a shape match it has not actually earned, and an +operator reading the report needs to know that a match there is not corroboration. +`fee_charged`'s JSON quoting -- a quoted string vs an unquoted number, the exact ambiguity #197 +turns on -- is precisely what the probe would settle if a single order row came back. So +`rh_order_open.json`, `rh_order_filled.json` and `rh_order_canceled.json` remain unverified +whenever the account has no order history; placing an order is still the only way to guarantee +an observation. ## Why it cannot place an order @@ -82,6 +92,7 @@ ("best_bid_ask", "rh_best_bid_ask.json"), ("estimated_price", "rh_estimated_price.json"), ("holdings", "rh_holdings.json"), + ("orders", "rh_orders.json"), ) #: The symbol every marketdata probe is run against. BTC-USD is the one pair we can be confident @@ -267,6 +278,7 @@ def run_probes(transport: _ReadOnly, symbol: str) -> dict[str, Any]: "best_bid_ask": lambda: transport.get_best_bid_ask(symbol), "estimated_price": lambda: transport.get_estimated_price(symbol, "ask", "0.001"), "holdings": lambda: transport.get_holdings(), + "orders": lambda: transport.get_orders(), } for name, call in calls.items(): try: diff --git a/tests/broker_robinhood/test_adapter.py b/tests/broker_robinhood/test_adapter.py index c57e9cc4..24420bc8 100644 --- a/tests/broker_robinhood/test_adapter.py +++ b/tests/broker_robinhood/test_adapter.py @@ -11,6 +11,7 @@ import json import uuid +from datetime import UTC, datetime from decimal import Decimal from pathlib import Path from typing import Any @@ -94,6 +95,7 @@ def __init__( estimated_price: dict[str, Any] | None = None, placed: dict[str, Any] | None = None, order: dict[str, Any] | None = None, + orders: dict[str, Any] | None = None, ) -> None: self._accounts = accounts self._holdings = holdings @@ -102,6 +104,7 @@ def __init__( self._estimated_price = estimated_price self._placed = placed self._order = order + self._orders = orders self.calls: dict[str, dict[str, Any]] = {} #: How many times each method was actually called, keyed by method name. Kept separate #: from `self.calls` (which only remembers the latest kwargs, matching the Coinbase fake) @@ -142,6 +145,18 @@ def create_order(self, body: dict[str, Any]) -> Any: self._issued_order_ids.add(issued_id) return self._placed + def get_orders(self, updated_at_start: str | None = None) -> Any: + """The order-history list, returned WHOLE regardless of `updated_at_start`. + + Deliberately not filtered here. `updated_at_start` is a SERVER-side filter in the real + API, so an adapter that trusted a client-side reimplementation of it in this fake would + be tested against a filter that does not exist in production. What the adapter owes is + that it sends the right window, and that is asserted directly against + `calls["get_orders"]["updated_at_start"]` instead. + """ + self._record("get_orders", updated_at_start=updated_at_start) + return self._orders + def get_order(self, order_id: str) -> Any: self._record("get_order", order_id=order_id) if order_id not in self._issued_order_ids: @@ -835,17 +850,196 @@ def test_get_fee_summary_declares_a_trailing_30d_window_by_name() -> None: assert adapter.get_fee_summary().volume_window == "trailing_30d" -def test_get_fee_summary_reports_zero_fees_paid() -> None: - """Robinhood's `fee_tier_status` exposes a rate and a volume, but no account-level - fees-PAID total -- so `fees_usd` must be `Decimal("0")` rather than derived (rate * volume - would be an estimate dressed up as an observation). Subscription lapse detection reads this - field, and a nonzero value here that was never actually charged could mask a real lapse.""" - transport = FakeTransport(accounts=load_fixture("rh_accounts.json")) - adapter = RobinhoodAdapter(transport) +def _fee_summary_adapter(orders: dict[str, Any] | None = None) -> tuple[Any, RobinhoodAdapter]: + """A transport carrying the real accounts fixture plus an order history, and its adapter.""" + transport = FakeTransport( + accounts=load_fixture("rh_accounts.json"), + orders=load_fixture("rh_orders.json") if orders is None else orders, + ) + return transport, RobinhoodAdapter(transport) + + +def test_get_fee_summary_sums_fee_charged_across_the_order_history() -> None: + """`fees_usd` must be OBSERVED, not pinned at zero (#197). + + A constant zero cannot contradict anything, so subscription-lapse detection -- whose whole + job is to notice a fee charged while the user claims a fee-free allowance -- did not merely + fail to run against this venue, it silently PASSED every time. That is worse than an absent + rail, because it reads as coverage. + + The expected total is summed off the fixture rather than written as a literal, so the day + that fixture is re-captured from a live order this test still states the property (every + charged fee is counted) instead of a stale number. + """ + _, adapter = _fee_summary_adapter() + + expected = sum( + (Decimal(str(row["fee_charged"])) for row in load_fixture("rh_orders.json")["results"]), + Decimal("0"), + ) + assert expected > 0, "the fixture must carry a real charged fee or this proves nothing" + assert adapter.get_fee_summary().fees_usd == expected + + +def test_get_fee_summary_counts_a_fee_charged_on_a_canceled_order() -> None: + """Filtering the sweep to `state == "filled"` would UNDER-report, and an under-report is a + lapse-detection false negative -- exactly the failure #197 is about. + + An order that partially fills and is then cancelled ends in state `canceled` while having + been charged a real fee on the part that executed. `fee_charged` is documented as "the total + fee amount that was charged for this order based on EXECUTED FILLS", so the field is already + its own state filter: it reads zero on an order that never traded. Adding a state filter on + top of it can only remove real charges. + """ + _, adapter = _fee_summary_adapter() + + rows = load_fixture("rh_orders.json")["results"] + canceled = [r for r in rows if r["state"] == "canceled" and Decimal(str(r["fee_charged"])) > 0] + assert canceled, "the fixture must carry a charged-but-cancelled order or this proves nothing" + + filled_only = sum( + (Decimal(str(r["fee_charged"])) for r in rows if r["state"] == "filled"), Decimal("0") + ) + fees = adapter.get_fee_summary().fees_usd + assert fees > filled_only + assert fees == filled_only + sum( + (Decimal(str(r["fee_charged"])) for r in canceled), Decimal("0") + ) + + +def test_get_fee_summary_never_counts_estimated_fee_remaining() -> None: + """`estimated_fee_remaining` is a fee that has NOT been charged, and counting it would invent + a contradiction of a fee-free claim out of an order that has not traded yet. + + The v2 docs describe it as "the estimated fee amount that will be charged on the remaining + unfilled quantity", explicitly conditional and explicitly an estimate. `fees_usd` is read as + an observation, so an estimate must never reach it. + """ + _, adapter = _fee_summary_adapter() + + rows = load_fixture("rh_orders.json")["results"] + estimated = sum((Decimal(str(r["estimated_fee_remaining"])) for r in rows), Decimal("0")) + assert estimated > 0, "the fixture must carry an un-charged estimate or this proves nothing" + + charged = sum((Decimal(str(r["fee_charged"])) for r in rows), Decimal("0")) + assert adapter.get_fee_summary().fees_usd == charged + + +def test_get_fee_summary_sweeps_the_same_thirty_day_window_volume_usd_reports() -> None: + """`fees_usd` and `volume_usd` must describe the SAME window or they cannot be compared. + + `volume_usd` is the venue's own `thirty_day_volume`, so the fee sweep asks the venue for + exactly 30 days, cut from the same instant the summary reports as `fetched_at`. Equality + (rather than "roughly 30 days ago") is the assertion because the two must come from ONE + reading of the clock: taking `time.time()` twice would let the window and the timestamp it is + reported against drift apart for no reason. + """ + transport, adapter = _fee_summary_adapter() + + summary = adapter.get_fee_summary() + + sent = transport.calls["get_orders"]["updated_at_start"] + start = datetime.strptime(sent, "%Y-%m-%dT%H:%M:%SZ").replace(tzinfo=UTC) + assert int(start.timestamp()) == summary.fetched_at - 30 * 24 * 60 * 60 + assert summary.volume_window == "trailing_30d" + + +def test_get_fee_summary_filters_on_updated_at_not_created_at() -> None: + """The window is cut on `updated_at`, and the choice is load-bearing rather than arbitrary. + + A fee is charged when an execution happens, and an execution always bumps `updated_at`. So + the `updated_at_start` result set is a SUPERSET of the orders carrying an in-window fee -- + it can never omit one. `created_at_start` has no such property: a GTC stop resting for forty + days and filling today was created outside the window and charged its fee inside it, so + filtering on creation drops a real charge. Under-reporting is the false negative #197 exists + to close, so the filter that cannot under-report is the correct one. + """ + transport, adapter = _fee_summary_adapter() + + adapter.get_fee_summary() + + assert set(transport.calls["get_orders"]) == {"updated_at_start"} + + +@pytest.mark.parametrize("quoted", [True, False]) +def test_get_fee_summary_reads_fee_charged_whether_the_venue_quotes_it_or_not( + quoted: bool, +) -> None: + """`fee_charged`'s JSON quoting is UNVERIFIED, so both shapes have to land on the same number. + + This venue is not internally consistent about quoting (#217 F6) -- `accounts` sends + `buying_power` quoted beside an unquoted `fee_tier_status.fee_ratio` in the SAME object -- and + no order object has ever been observed live, because observing one requires placing a real + order and there is no sandbox. The v2 schema types `fee_charged` as an unquoted `number`, but + it types the neighbouring `executions[].effective_price` as a quoted decimal STRING, so the + documentation does not settle it either. Reading both is required, not defensive breadth. + """ + raw = "1.6355" + orders = {"results": [{"state": "filled", "fee_charged": raw if quoted else Decimal(raw)}]} + _, adapter = _fee_summary_adapter(orders) + + assert adapter.get_fee_summary().fees_usd == Decimal(raw) + + +def test_get_fee_summary_does_not_let_a_negative_fee_cancel_out_a_real_charge() -> None: + """A negative `fee_charged` is not a fee charged, and must not net a real one back to zero. + + Nothing in the v2 docs says this field can go negative, so a negative is either a rebate or a + venue bug -- and under both readings, letting it subtract would hide a charge that really did + happen from the one check that exists to notice it. + """ + orders = { + "results": [ + {"state": "filled", "fee_charged": Decimal("1.6355")}, + {"state": "filled", "fee_charged": Decimal("-5.00")}, + ] + } + _, adapter = _fee_summary_adapter(orders) + + assert adapter.get_fee_summary().fees_usd == Decimal("1.6355") + + +def test_get_fee_summary_reports_zero_only_when_the_venue_reported_no_charges() -> None: + """Zero is still a legitimate answer -- but now it is an OBSERVATION rather than a constant. + + An account that has traded nothing in thirty days genuinely paid nothing, and that zero does + contradict nothing for the right reason. The difference from the old behaviour is the whole + point of #197: this zero moves when the venue's answer moves. + """ + _, adapter = _fee_summary_adapter({"results": []}) assert adapter.get_fee_summary().fees_usd == Decimal("0") +def test_get_fee_summary_propagates_a_truncated_sweep_instead_of_under_reporting() -> None: + """An incomplete sweep must raise, because `FeeSummary` has nowhere to say "partial". + + `RobinhoodTransport._paginate` raises once a list refuses to terminate within `_MAX_PAGES`, + and `get_fee_summary` deliberately does NOT catch it. `fees_usd` is a bare `Decimal` with no + companion field for confidence, so a truncated sum is indistinguishable from a complete one + and would be read as an observation -- a silent under-report, which is the same + always-passing false negative #197 is about, merely arrived at by a different route. An + exception is visible; a confidently wrong number is not. + + This is the opposite of the rule `_account`/`cancel_order` follow, and the asymmetry is the + point: those run on the EXIT path, where a raise can trap a position. `get_fee_summary` is a + reconciliation read on no position's critical path, so failing loudly costs nothing here. + """ + + class _TruncatingTransport(FakeTransport): + def get_orders(self, updated_at_start: str | None = None) -> Any: + self._record("get_orders", updated_at_start=updated_at_start) + raise RuntimeError( + "robinhood pagination did not terminate within 20 pages following " + "'/api/v2/crypto/trading/orders/'; refusing to loop further" + ) + + adapter = RobinhoodAdapter(_TruncatingTransport(accounts=load_fixture("rh_accounts.json"))) + + with pytest.raises(RuntimeError, match="did not terminate"): + adapter.get_fee_summary() + + def test_a_transportless_adapter_refuses_network_calls_clearly() -> None: """`capabilities()` must work offline -- the engine needs it to decide whether to even wire this venue up before any credentials exist. Anything that actually needs the network must say diff --git a/tests/broker_robinhood/test_transport.py b/tests/broker_robinhood/test_transport.py index 459b98df..5acb74a1 100644 --- a/tests/broker_robinhood/test_transport.py +++ b/tests/broker_robinhood/test_transport.py @@ -438,6 +438,12 @@ def test_request_parses_unquoted_json_numbers_as_decimal_never_float(http: Any) "/api/v2/crypto/trading/estimated_price/", "quantity=0.1&side=ask&symbol=BTC-USD", ), + ( + "get_orders", + "GET", + "/api/v2/crypto/trading/orders/", + "account_number=AB1234567890&updated_at_start=2026-07-10T12%3A00%3A00Z", + ), ("create_order", "POST", "/api/v2/crypto/trading/orders/", "account_number=AB1234567890"), ( "get_order", @@ -450,6 +456,7 @@ def test_request_parses_unquoted_json_numbers_as_decimal_never_float(http: Any) _CALL_ARGS: dict[str, dict[str, Any]] = { "get_best_bid_ask": {"symbol": "BTC-USD"}, + "get_orders": {"updated_at_start": "2026-07-10T12:00:00Z"}, "get_estimated_price": {"symbol": "BTC-USD", "side": "ask", "quantity": "0.1"}, "create_order": {"body": {"symbol": "BTC-USD"}}, "get_order": {"order_id": "order-id-1"}, @@ -499,6 +506,41 @@ def account_of(call: dict[str, Any]) -> str | None: assert account_of(fetched) == account_of(created) +def test_get_orders_omits_the_window_filter_entirely_when_none_is_asked_for(http: Any) -> None: + """An absent `updated_at_start` must vanish from the query string, not ride as `"None"`. + + Every query param is SIGNED, so a stray literal `updated_at_start=None` is not a param the + venue ignores -- it is either a 400 or, worse, a filter matching nothing, which would empty + the fee sweep and report `fees_usd` as zero. That is the precise always-passing failure #197 + exists to close, reintroduced through the query string. + """ + recorder = http(_FakeResponse(payload={"results": [], "next": None})) + _transport().get_orders() + + assert _path_of(recorder.calls[0]["url"]) == ( + "/api/v2/crypto/trading/orders/?account_number=AB1234567890" + ) + _assert_signature_covers_what_was_sent(recorder.calls[0]) + + +def test_get_orders_paginates_so_the_fee_sweep_never_sees_a_page_boundary(http: Any) -> None: + """A truncated order history is a silent under-report of `fees_usd`, so `get_orders` has to + resolve every page before the adapter sees it -- and a history that refuses to terminate has + to RAISE rather than hand back what it managed to collect.""" + page_one = _FakeResponse( + payload={ + "results": [{"id": "o1", "fee_charged": 1.5}], + "next": f"{_BASE_URL}/api/v2/crypto/trading/orders/?cursor=page2", + } + ) + page_two = _FakeResponse(payload={"results": [{"id": "o2", "fee_charged": 0.25}], "next": None}) + http([page_one, page_two]) + + rows = _results(_transport().get_orders()) + + assert [row["id"] for row in rows] == ["o1", "o2"] + + def test_trading_pairs_and_best_bid_ask_surface_their_documented_fields(http: Any) -> None: """The two reads the adapter does not call yet still have to return usable rows. diff --git a/tests/conformance/test_robinhood_conformance.py b/tests/conformance/test_robinhood_conformance.py index 02803252..8b8ac05f 100644 --- a/tests/conformance/test_robinhood_conformance.py +++ b/tests/conformance/test_robinhood_conformance.py @@ -25,5 +25,12 @@ def broker(self) -> RobinhoodAdapter: estimated_price=load_fixture("rh_estimated_price.json"), placed=load_fixture("rh_order_open.json"), order=load_fixture("rh_order_open.json"), + # Wired so `test_fee_summary_matches_its_declaration` exercises the real + # order-history sweep `fees_usd` is summed from (#197). Leaving it out would let + # the conformance run assert against an EMPTY sweep -- which returns + # `Decimal("0")` and is indistinguishable from the hardcoded zero that issue + # closed, so the one suite held out as this venue's end-to-end signal would pass + # just as happily on the bug as on the fix. + orders=load_fixture("rh_orders.json"), ) ) diff --git a/tests/fixtures/rh_orders.json b/tests/fixtures/rh_orders.json new file mode 100644 index 00000000..b1c6ebd1 --- /dev/null +++ b/tests/fixtures/rh_orders.json @@ -0,0 +1,98 @@ +{ + "next": null, + "previous": null, + "results": [ + { + "id": "1a2b3c4d-5e6f-7a8b-9c0d-1e2f3a4b5c6d", + "account_number": "AB1234567890", + "symbol": "BTC-USD", + "client_order_id": "9f8e7d6c-5b4a-3c2d-1e0f-9a8b7c6d5e4f", + "side": "sell", + "type": "market", + "state": "filled", + "average_price": 65420.75, + "filled_asset_quantity": 0.1, + "created_at": "2026-08-09T11:55:00.000000Z", + "updated_at": "2026-08-09T11:55:01.000000Z", + "fee_charged": 1.6355, + "estimated_fee_remaining": 0, + "executions": [ + { + "effective_price": 65420.75, + "quantity": 0.1, + "timestamp": "2026-08-09T11:55:01.000000Z" + } + ], + "market_order_config": { + "asset_quantity": 0.1 + } + }, + { + "id": "5c6d7e8f-9a0b-1c2d-3e4f-5a6b7c8d9e0f", + "account_number": "AB1234567890", + "symbol": "BTC-USD", + "client_order_id": "2b3c4d5e-6f7a-8b9c-0d1e-2f3a4b5c6d7e", + "side": "buy", + "type": "limit", + "state": "canceled", + "average_price": 64000.0, + "filled_asset_quantity": 0.0262, + "created_at": "2026-08-09T11:40:00.000000Z", + "updated_at": "2026-08-09T11:45:00.000000Z", + "fee_charged": 0.42, + "estimated_fee_remaining": 0, + "executions": [ + { + "effective_price": 64000.0, + "quantity": 0.0262, + "timestamp": "2026-08-09T11:42:00.000000Z" + } + ], + "limit_order_config": { + "asset_quantity": 0.1, + "limit_price": 64000.0, + "time_in_force": "gtc" + } + }, + { + "id": "8e5f9c2a-1b3d-4e6f-9a2b-7c1d3e5f9a2b", + "account_number": "AB1234567890", + "symbol": "BTC-USD", + "client_order_id": "3f2e1d4c-5b6a-7c8d-9e0f-1a2b3c4d5e6f", + "side": "buy", + "type": "limit", + "state": "open", + "average_price": null, + "filled_asset_quantity": 0, + "created_at": "2026-08-09T12:00:00.000000Z", + "updated_at": "2026-08-09T12:00:00.000000Z", + "fee_charged": 0, + "estimated_fee_remaining": 0.16, + "executions": [], + "limit_order_config": { + "asset_quantity": 0.1, + "limit_price": 64000.0, + "time_in_force": "gtc" + } + }, + { + "id": "7d4e2f1a-3b5c-6d8e-0f1a-2b3c4d5e6f7a", + "account_number": "AB1234567890", + "symbol": "ETH-USD", + "client_order_id": "6a5b4c3d-2e1f-0a9b-8c7d-6e5f4a3b2c1d", + "side": "buy", + "type": "market", + "state": "failed", + "average_price": null, + "filled_asset_quantity": 0, + "created_at": "2026-08-09T10:15:00.000000Z", + "updated_at": "2026-08-09T10:15:00.000000Z", + "fee_charged": 0, + "estimated_fee_remaining": 0, + "executions": [], + "market_order_config": { + "asset_quantity": 0.5 + } + } + ] +} diff --git a/tests/scripts/test_robinhood_smoke.py b/tests/scripts/test_robinhood_smoke.py index 1c6fe26a..187d89c0 100644 --- a/tests/scripts/test_robinhood_smoke.py +++ b/tests/scripts/test_robinhood_smoke.py @@ -56,6 +56,9 @@ def get_estimated_price(self, symbol: str, side: str, quantity: str) -> Any: def get_holdings(self) -> Any: return self._request("GET", "/api/v2/crypto/trading/holdings/") + def get_orders(self, updated_at_start: str | None = None) -> Any: + return self._request("GET", "/api/v2/crypto/trading/orders/") + # --- the read-only guarantee --------------------------------------------------------------- @@ -92,9 +95,16 @@ def test_get_requests_pass_through_and_are_recorded() -> None: def test_running_every_probe_issues_only_gets() -> None: guard = _ReadOnly(_StubTransport()) - run_probes(guard, "BTC-USD") + results = run_probes(guard, "BTC-USD") assert guard.calls, "probes issued no requests at all" assert {method for method, _ in guard.calls} == {"GET"} + # Every probe must also have SUCCEEDED, not merely have issued a GET. `run_probes` records a + # failure per probe rather than aborting, so a probe calling a transport method that does not + # exist is swallowed into `{"ok": False}` -- which the read-only assertion above cannot see. + # Adding the `orders` probe (#197) hit exactly that: it was silently inert here until the stub + # grew a `get_orders`, and a probe nothing exercises is a probe that can rot unnoticed. + assert {name for name, result in results.items() if not result["ok"]} == set() + assert set(results) == {name for name, _ in PROBES} # --- shapes --------------------------------------------------------------------------------