From d7ec8ec32a1964da4b63553bedb981562d3f58ca Mon Sep 17 00:00:00 2001 From: Elmehdi Aitbrahim Date: Sun, 9 Aug 2026 18:38:24 -0400 Subject: [PATCH 1/2] fix(brokers): read robinhood's real estimated_price shape, correct every fixture First live run against a real Robinhood credential (#216) proved the response shapes wrong in four places and the probe script wrong in a fifth (#217). F1 (blocker). `_estimated_price` read `price`, which this venue does not send; the unit price is in the column named after the side asked for (`ask`/`bid`). Every market preview against the live venue came back `est_quote_size = 0.000` with `errors` populated. There is deliberately no fallback to the other side's column -- pricing a sell off an ask overstates the proceeds of an exit -- so a row missing the requested side stays unpriced. `preview_order` now also reads the venue's own `est_fee` instead of deriving one from the account tier, and reconciles `est_total_cost` against `price x quantity` and `est_fee` per response rather than assuming either reading. All three self-consistent readings recover the same fee-exclusive notional, which is what `Preview.est_quote_size` is defined to carry; a total matching none of them is priced as sent AND reported through `Preview.errors`. `synthetic=True` and `supports_native_preview=False` are unchanged and now argued for explicitly: `/estimated_price/` prices a quantity, it does not validate an order. F2. Every money value arrives unquoted, so every fixture is now an unquoted JSON number and both fixture loaders decode with `parse_float=Decimal`, the way the transport does. This makes #194's parser change exercised rather than merely present. F3. `trading_pairs` publishes no `min_order_amount` and no `min_order_size`. Removed from the fixture; the transport docstring and README now record that a pre-flight minimum-size check (#198) has no source on this endpoint. F4. `best_bid_ask` rewritten to the observed `symbol`/`bid`/`ask`; the five invented keys are gone. F5. The probe compared a post-pagination aggregate against a raw single page, so it reported `next`/`previous` MISSING AT VENUE on all five probes and buried the real findings. `fixture_shape` normalizes the fixture to what a probe can observe -- envelope stripped, numbers parsed as `Decimal` -- and a test runs the real comparison for every probe. The three order fixtures remain unverified against the venue: observing an order object requires placing a real one. Closes #217 Co-Authored-By: Claude Opus 5 (1M context) --- packages/keel-broker-robinhood/README.md | 53 +++ .../keel_broker_robinhood/adapter.py | 336 ++++++++++++++--- .../keel_broker_robinhood/transport.py | 34 +- scripts/robinhood_smoke.py | 49 ++- tests/broker_robinhood/test_adapter.py | 337 +++++++++++++++++- tests/broker_robinhood/test_transport.py | 65 +++- tests/fixtures/rh_accounts.json | 10 +- tests/fixtures/rh_best_bid_ask.json | 8 +- tests/fixtures/rh_estimated_price.json | 8 +- tests/fixtures/rh_holdings.json | 4 +- tests/fixtures/rh_order_canceled.json | 10 +- tests/fixtures/rh_order_filled.json | 14 +- tests/fixtures/rh_order_open.json | 10 +- tests/fixtures/rh_trading_pairs.json | 7 +- tests/scripts/test_robinhood_smoke.py | 53 ++- 15 files changed, 884 insertions(+), 114 deletions(-) diff --git a/packages/keel-broker-robinhood/README.md b/packages/keel-broker-robinhood/README.md index d627e693..f0b9ba7e 100644 --- a/packages/keel-broker-robinhood/README.md +++ b/packages/keel-broker-robinhood/README.md @@ -20,6 +20,36 @@ no `time_in_force` field at all, which is why `market_ioc_base` is declared desp name saying IOC. That is a naming impedance, not a capability lie -- a market order is immediate by construction, and there is no resting-market variant here to confuse it with. +### What a preview reads, and why it is still synthetic + +A market preview prices off `GET /api/v2/crypto/trading/estimated_price/`. The row that endpoint +actually returns was confirmed against a real credential in #217: + +``` +{'symbol', 'side', 'quantity', 'timestamp', 'fee_ratio', 'est_fee', 'ask', 'est_total_cost'} +``` + +There is **no `price` field**. The unit price is in the column named after the side that was asked +for -- `ask` for a buy, `bid` for a sell. The adapter read `price` until #217, and the consequence +was total: every market preview against the real venue came back `est_quote_size = 0.000` with +`errors` populated, so confirm mode was unusable on this venue. It is read from the requested +side's column only, with no fallback to the other one: pricing a sell off an `ask` overstates the +proceeds of an exit, and a row that does not carry the requested side is treated as unpriced. + +The adapter also reads the venue's own `est_fee` instead of multiplying by the account's fee tier, +and reconciles `est_total_cost` against `price * quantity` and `est_fee` on every response. +Whether `est_total_cost` includes the fee is **not documented and not assumed**. All three +self-consistent readings (`total == notional`, `total == notional + fee`, `total == notional - +fee`) recover the same fee-exclusive notional, which is what `Preview.est_quote_size` is defined +to carry, and `Preview.detail["cost_basis"]` reports which one this response satisfied. A total +satisfying none of them is priced from the venue's number as sent *and* reported through +`Preview.errors`. + +**None of that makes the preview a broker quote.** `/estimated_price/` prices a *quantity*: it +does not validate the order, check buying power, check the account's own size bounds, or reserve +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`. + ## What does NOT work ### No candles @@ -42,6 +72,19 @@ on the live-money path, and it would do so silently. `translate.to_order_body` r `UnsupportedOrder` for `MarketIOCByQuote` with this reasoning in the message, as a second gate behind the adapter's capability declaration. +### No published minimum order size + +`GET /api/v2/crypto/trading/trading_pairs/` publishes `asset_increment`, `quote_increment` and +`max_order_size`, and **no minimum of any kind** -- neither `min_order_amount` nor +`min_order_size`. This was confirmed live across four cursor pages in #217; the fixture had +invented `min_order_amount`, and `transport.get_trading_pairs`' docstring named it as an input. + +The consequence is for the pre-flight sizing check proposed in #198: increment rounding and an +upper bound can be validated locally against this endpoint, and a **lower** bound cannot be +validated at all, because the venue never states one. An undersized order is discoverable only as +a rejection at placement. Anything designing that check must not assume a minimum is available +here. + ### No sandbox Robinhood ships no test environment for this API. Every test in this repository's @@ -50,6 +93,16 @@ exercise this adapter end to end without placing a real order with real money, w conformance suite against the fake transport is the only signal this package has before a human runs it live. +`scripts/robinhood_smoke.py` narrows that gap without placing anything: it is a read-only, +GET-only probe that compares each endpoint's live shape against the committed fixture. After the +first run of it (#217), the five READ fixtures -- `rh_accounts.json`, `rh_holdings.json`, +`rh_trading_pairs.json`, `rh_best_bid_ask.json`, `rh_estimated_price.json` -- match observed +responses. **The three order fixtures (`rh_order_open.json`, `rh_order_filled.json`, +`rh_order_canceled.json`) remain unverified against the venue**, because observing an order +object requires placing a real order, which that script refuses by construction. Their field +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* diff --git a/packages/keel-broker-robinhood/keel_broker_robinhood/adapter.py b/packages/keel-broker-robinhood/keel_broker_robinhood/adapter.py index 844a7805..f05721cc 100644 --- a/packages/keel-broker-robinhood/keel_broker_robinhood/adapter.py +++ b/packages/keel-broker-robinhood/keel_broker_robinhood/adapter.py @@ -45,7 +45,9 @@ import time import uuid +from dataclasses import dataclass from decimal import Decimal, InvalidOperation +from typing import Any from keel_broker_api.capabilities import BrokerCapabilities from keel_broker_api.orders import LimitGTC, MarketIOCByBase, OrderSpec, StopLimitGTC @@ -77,8 +79,16 @@ # There is no resting-market-order variant to be confused with. The port kind that WOULD be a # lie is the quote-sized one, and it is not declared. supported_orders=frozenset({"market_ioc_base", "limit_gtc", "stop_limit_gtc"}), - # No preview endpoint exists on this API, so every Preview this adapter returns is a number - # it computed itself and must label `synthetic=True`. + # No preview endpoint exists on this API, so every Preview this adapter returns must label + # itself `synthetic=True`. + # + # `/estimated_price/` is NOT a counter-example, and the first live run (#217) is why this + # comment now says so explicitly: that endpoint hands back `est_fee` and `est_total_cost`, + # which look exactly like a broker's own quote and are not one. It prices a QUANTITY. It does + # not validate the order, check buying power, check this account's own size bounds, or reserve + # anything -- an order it prices happily can be rejected the instant it is placed. Reading the + # venue's numbers instead of deriving our own makes a preview more ACCURATE; it does not make + # it a quote, and `Preview.synthetic` is the field that carries exactly that difference. supports_native_preview=False, synthesizes_preview=True, supports_fee_summary=True, @@ -100,6 +110,55 @@ "an execution venue for keel, and candle data must come from another source" ) +#: How far `est_total_cost` may sit from a candidate relation and still be taken as satisfying it, +#: as an absolute floor and as a fraction of the notional (one cent, or one basis point, whichever +#: is larger). +#: +#: A tolerance is required rather than fastidious: the venue rounds its own `est_fee` and +#: `est_total_cost` to some undisclosed precision, so `price * quantity + est_fee` computed here +#: at full `Decimal` precision will routinely miss the venue's rounded total by sub-cent amounts. +#: Demanding exact equality would report every healthy response as unreconciled, and a check that +#: fires on every run is a check nobody reads (see #217 F5 for what that costs). +#: +#: It is bounded in the other direction by what it must still catch: the readings it discriminates +#: between differ by a whole `est_fee`, which at this venue's ~25bp taker rate is 25x a one-basis- +#: point tolerance. A fee small enough to hide inside the tolerance is a fee too small to +#: materially mis-state the order either way. +_TOTAL_TOLERANCE_ABS = Decimal("0.01") +_TOTAL_TOLERANCE_RATIO = Decimal("0.0001") + + +@dataclass(frozen=True) +class _VenueEstimate: + """One `/estimated_price/` row, reconciled into the terms `Preview` is defined in. + + This exists because the endpoint answers with four related numbers (`ask`/`bid`, `quantity`, + `est_fee`, `est_total_cost`) and `Preview` has two slots for them, under a convention the port + fixes and the venue does not share: `est_quote_size` excludes the fee, and `est_fee` sits + beside it. Returning a bare `Decimal` price, as this used to, threw away the venue's own + statement of the fee and total and forced `preview_order` to re-derive both -- which is how a + preview ends up asserting arithmetic the venue never agreed to. + + `errors` rides along rather than being raised: every one of them is a soft failure that still + leaves a usable (if less certain) estimate, and this runs on a path the executor uses while + unwinding a position, where a raise can trap it. `preview_order` folds them into + `Preview.errors`, which is the field the port defines for exactly this. + """ + + #: The unit price from the column named after the requested side. Never from `price`, which + #: this venue does not send (#217 F1). + price: Decimal + #: Fee-EXCLUSIVE notional, in quote currency. This is what `Preview.est_quote_size` means: + #: the limit path fills it with `base_size * limit_price`, and `est_fee` is a separate field. + quote_size: Decimal + #: The venue's own `est_fee`, or `None` when it did not state a usable one for this size. + fee: Decimal | None + #: The venue's own per-order `fee_ratio`, for `detail` only -- `fee` is never derived from it. + fee_ratio: Decimal | None + #: How `quote_size` was arrived at, verbatim into `Preview.detail["cost_basis"]`. + cost_basis: str + errors: tuple[str, ...] + class RobinhoodAdapter: """Implements the `Broker` port against the Robinhood Crypto Trading API v2. @@ -236,11 +295,16 @@ def _reject_unsupported(self, spec: OrderSpec) -> None: def preview_order(self, spec: OrderSpec) -> Preview: """Synthesise a preview. Always `synthetic=True` -- there is no preview endpoint here. - Coinbase answers `preview_order` with its own quote, so approving it is approving the - venue's arithmetic. Robinhood answers nothing, so every number below is this adapter's - arithmetic, and `Preview`'s docstring is explicit that "approving an estimate must never - look identical to approving a broker's own quote". `synthetic=True` is what carries that - distinction to whatever renders the confirm gate. + ⚠️ **`/estimated_price/` is not a preview and this method must never present it as one.** + It prices a QUANTITY on one side of the book. It does not validate the order, does not + check buying power, does not check this account's own size or increment bounds, and + reserves nothing -- an order it prices happily can be rejected the instant it is placed. + Coinbase's `preview_order` is a broker quote, so approving one is approving the venue's + arithmetic about THIS order; approving a Robinhood preview is approving an estimate that + the venue has never been asked to stand behind. `Preview`'s own docstring requires those + two never to look identical, and `synthetic=True` (with `supports_native_preview=False`) + is the field that carries the difference. Reading more of the venue's numbers, as this + method now does, makes the estimate more accurate and changes nothing about that. The three fields, and how firm each one actually is: @@ -249,63 +313,97 @@ def preview_order(self, spec: OrderSpec) -> Preview: * `est_quote_size` for `limit_gtc`/`stop_limit_gtc` is `base_size * limit_price`. That is a BOUND, not a prediction: a limit order does not trade worse than its limit, so this is the most quote currency a sell can realise or the most a buy can spend. For - `market_ioc_base` there is no bound to quote, so it comes from `estimated_price` and + `market_ioc_base` there is no bound to quote, so it comes from `/estimated_price/` and is a genuine guess that the fill can and will miss. - * `est_fee` is `est_quote_size * fee_tier_status.fee_ratio`. When the account reports no - ratio it is `Decimal("0")` and `detail["fee_ratio"]` reads `"unknown"` -- a made-up - rate would be worse than a visible zero, because a plausible-looking fee is one nobody - checks. `detail["price_basis"]` names which of the two paths above produced the quote - size, so the reader can tell a bound from a guess without inferring it from the kind. + * `est_fee` is the venue's own `est_fee` when the response carries one, and only + otherwise `est_quote_size * fee_tier_status.fee_ratio`. Deriving a fee we were handed + would reproduce a number the response already contains, from an ACCOUNT-level rate + rather than the rate quoted against this order, and the two can disagree. When the + account reports no ratio either, `est_fee` is `Decimal("0")` and `detail["fee_ratio"]` + reads `"unknown"` -- a made-up rate would be worse than a visible zero, because a + plausible-looking fee is one nobody checks. + + `detail` names every basis rather than leaving it to be inferred: `price_basis` (which + endpoint or field the unit price came from), `cost_basis` (how `est_quote_size` was + arrived at, including which reading of `est_total_cost` the venue's numbers supported), + and `fee_basis` (venue-stated vs account-derived). **Every path that could not price the order populates `errors`.** A zero from a failed `estimated_price` lookup, or a zero fee from a missing ratio, renders at the confirm gate as an order that costs nothing -- which is the single most approvable thing a preview can look like, and is indistinguishable from a genuinely free order unless the preview says otherwise. `detail` is free-form text a renderer may not show; `errors` is the field the - port defines for a soft failure, so an unpriced leg has to appear there. + port defines for a soft failure, so an unpriced leg has to appear there. The same applies + to a partially-understood one: an `est_total_cost` that reconciles with nothing is not a + pricing failure, but it is not a number to put in front of a human unqualified either. **The symbol is validated on every path, including the resting-order ones.** They price off `spec.limit_price` and have no other reason to call `to_symbol`, which is exactly how `ETH-USDC` used to preview cleanly and then raise `UnsupportedOrder` at placement -- after the human had already approved it, and with the port forbidding a caller from catching that and retrying. A preview must never approve what placement will refuse. + + `GET /accounts/` is fetched only when it is actually needed -- that is, when the venue did + not state the fee itself. On the market path against a healthy response it is now not + fetched at all, which halves this method's request count on the one path the executor + calls while unwinding a position. """ self._reject_unsupported(spec) to_symbol(spec.product_id) base_size = self._base_size(spec) errors: list[str] = [] + estimate: _VenueEstimate | None = None if isinstance(spec, LimitGTC | StopLimitGTC): - price, basis = spec.limit_price, "limit_price" + price = spec.limit_price + quote_size = base_size * price + price_basis, cost_basis = "limit_price", "base_size_x_limit_price" else: - basis = "estimated_price" - estimated = self._estimated_price(spec) - if estimated is None: + price_basis = "estimated_price" + estimate = self._estimated_price(spec) + if estimate is None: price = Decimal("0") + quote_size = Decimal("0") + cost_basis = "unpriced" errors.append( "robinhood returned no usable estimated price for this order; " "est_quote_size and est_fee are NOT priced and must not be read as a cost" ) else: - price = estimated - - quote_size = base_size * price - ratio = self._fee_ratio(self._account()) - if ratio is None: - errors.append( - "robinhood reported no fee_tier_status.fee_ratio for this account; est_fee is " - "zero because the rate is UNKNOWN, not because this order trades free" - ) + price = estimate.price + quote_size = estimate.quote_size + cost_basis = estimate.cost_basis + errors.extend(estimate.errors) + + ratio: Decimal | None + if estimate is not None and estimate.fee is not None: + fee, fee_basis, ratio = estimate.fee, "venue_est_fee", estimate.fee_ratio + else: + fee_basis = "account_fee_ratio" + ratio = self._fee_ratio(self._account()) + if ratio is None: + errors.append( + "robinhood reported no fee_tier_status.fee_ratio for this account; est_fee is " + "zero because the rate is UNKNOWN, not because this order trades free" + ) + fee = Decimal("0") + else: + fee = quote_size * ratio + return Preview( product_id=spec.product_id, side=spec.side, est_base_size=base_size, est_quote_size=quote_size, - est_fee=quote_size * ratio if ratio is not None else Decimal("0"), + est_fee=fee, synthetic=True, detail={ - "price_basis": basis, - "price": str(price), + "price_basis": price_basis, + # `_render`, not `str`: `str(Decimal("1E-8"))` is `"1E-8"`, and this string is + # rendered to a human deciding whether to spend money. + "price": _render(price), + "cost_basis": cost_basis, + "fee_basis": fee_basis, "fee_ratio": str(ratio) if ratio is not None else "unknown", }, errors=tuple(errors), @@ -323,13 +421,41 @@ def _base_size(self, spec: OrderSpec) -> Decimal: return spec.base_size raise UnsupportedOrder(f"robinhood cannot size order kind {spec.kind!r} in base units") - def _estimated_price(self, spec: OrderSpec) -> Decimal | None: - """Robinhood's estimated price for this size, or `None` when it reports no usable one. - - `side` is translated, not passed through: this endpoint answers in book terms - (`bid`/`ask`), not order terms (`buy`/`sell`), and a buyer is filled from the ASK. Asking - for the wrong side of the spread would understate the cost of every buy preview -- which - is exactly the direction of error a human at a confirm gate is least likely to catch. + def _estimated_price(self, spec: OrderSpec) -> _VenueEstimate | None: + """Everything `GET /estimated_price/` states about this order, or `None` if it states no + usable price. + + ⚠️ **The unit price is read from the column named after the side that was ASKED for -- + `ask` for a buy, `bid` for a sell -- and never from `price`.** The venue sends no `price` + field. This method read one for the whole life of the package, on the strength of the + documentation, and the first live run against a real credential (#217 F1) proved every + single market preview came back `est_quote_size = 0.000` with `errors` populated: confirm + mode was unusable against this venue. The observed row is:: + + {'symbol', 'side', 'quantity', 'timestamp', + 'fee_ratio', 'est_fee', 'ask', 'est_total_cost'} + + There is deliberately NO fallback to the other side's column. If a sell is answered with + only an `ask`, pricing it off that ask overstates the proceeds of an exit -- the exact + optimistic direction `to_price_side` exists to prevent -- so a row that does not carry the + requested side is treated as unpriced instead. Silence is recoverable; a flattering number + at a confirm gate is not. + + **The venue's own `est_fee` and `est_total_cost` are read rather than derived**, but only + after checking they describe the order that was asked about. Two checks, both of which + can only be made because the venue sends four related numbers: + + 1. `quantity` must be the size that was requested. If the venue echoes a different one, + its `est_fee` and `est_total_cost` are answers about a different order; scaling them + would be precisely the "estimate that moves between the quote and the fill" this + package refuses everywhere else, so only the unit price is used and `errors` says so. + 2. `est_total_cost` must reconcile with `price * quantity`, either exactly (fee-exclusive) + or offset by `est_fee` in one direction or the other. Nothing in Robinhood's + documentation settles which of those it is, and #217 could not settle it either, so + nothing here assumes: `_reconcile_total` reads it off the numbers in each response. + A total that fits none of the three is reported through `errors` rather than quietly + priced -- see `_reconcile_total` for why all three readings recover the same + fee-exclusive notional, and why that is the number `Preview.est_quote_size` wants. **`None`, never `Decimal("0")`.** This used to answer zero for "no rows", "unparseable price", and "the venue really did quote zero" alike, and `preview_order` had no way to @@ -337,25 +463,56 @@ def _estimated_price(self, spec: OrderSpec) -> Decimal | None: costs nothing. That is not a visible nonsense; it is the most approvable thing a preview can display. `None` forces the caller to decide, and `preview_order` turns it into a populated `Preview.errors` rather than a silent zero. A quoted price of zero is treated - as unusable for the same reason: nothing on this venue costs nothing. + as unusable for the same reason: nothing on this venue costs nothing. #217 is what proved + that fix load-bearing rather than theoretical -- it is the only reason a completely + unpriced preview was survivable at all. `quantity` renders through `translate._render`, not `str()` -- `str(Decimal("1E-8"))` is `"1E-8"`, and this value goes on a query string the signature is computed over, so an exponent here is both a malformed request and a signature mismatch. """ + base_size = self._base_size(spec) + side = to_price_side(spec.side) response = self._require_transport().get_estimated_price( - symbol=to_symbol(spec.product_id), - side=to_price_side(spec.side), - quantity=_render(self._base_size(spec)), + symbol=to_symbol(spec.product_id), side=side, quantity=_render(base_size) ) rows = _results(response) if not rows: return None - try: - price = Decimal(str(_field(rows[0], "price", "0") or "0")) - except (InvalidOperation, ValueError): + row = rows[0] + + price = _decimal_or_none(_field(row, side)) + if price is None or price <= 0: return None - return price if price > 0 else None + + quantity = _decimal_or_none(_field(row, "quantity")) + if quantity is None or quantity != base_size: + quoted = "no quantity" if quantity is None else f"quantity {_render(quantity)}" + return _VenueEstimate( + price=price, + quote_size=price * base_size, + fee=None, + fee_ratio=None, + cost_basis="price_x_base_size", + errors=( + f"robinhood's estimated_price row carries {quoted} for a requested size of " + f"{_render(base_size)}; its est_fee and est_total_cost describe a different " + f"order and are NOT used -- est_quote_size is price x the requested size", + ), + ) + + fee = _positive_or_none(_decimal_or_none(_field(row, "est_fee")), allow_zero=True) + ratio = _positive_or_none(_decimal_or_none(_field(row, "fee_ratio")), allow_zero=True) + total = _positive_or_none(_decimal_or_none(_field(row, "est_total_cost"))) + quote_size, cost_basis, errors = _reconcile_total(price * quantity, total, fee) + return _VenueEstimate( + price=price, + quote_size=quote_size, + fee=fee, + fee_ratio=ratio, + cost_basis=cost_basis, + errors=errors, + ) def place_order(self, spec: OrderSpec) -> PlaceResult: """Place a live order. A fresh `client_order_id` per call; the returned `state` is read. @@ -562,6 +719,95 @@ def cancel_order(self, order_id: str) -> bool: _CANCELED = "canceled" +def _decimal_or_none(value: Any) -> Decimal | None: + """Parse one JSON leaf as a `Decimal`, or `None` if it is absent or not a number. + + `Decimal(str(value))` handles both shapes this venue produces without a branch: since #194 the + transport decodes with `parse_float=Decimal`, so an unquoted `65482.30` already arrives as a + `Decimal` (and `str()` of one round-trips exactly), while a quoted `"65482.30"` arrives as a + `str`. #217 confirmed the venue sends the unquoted form for every money field, but a `str` + remains possible on any field and costs nothing to accept. + + `None` rather than a zero default, everywhere. A zero here would flow into a preview as a real + price, a real fee, or a real cost, and this whole module is built on the principle that an + absent number and a zero number must never be the same value. + """ + if value is None or isinstance(value, bool): + return None + try: + return Decimal(str(value)) + except (InvalidOperation, ValueError, TypeError): + return None + + +def _positive_or_none(value: Decimal | None, *, allow_zero: bool = False) -> Decimal | None: + """Drop a value the venue cannot have meant. A negative fee or a zero total is not data.""" + if value is None: + return None + if value < 0 or (value == 0 and not allow_zero): + return None + return value + + +def _reconcile_total( + notional: Decimal, total: Decimal | None, fee: Decimal | None +) -> tuple[Decimal, str, tuple[str, ...]]: + """Work out what `est_total_cost` MEANS from the venue's own numbers, rather than assuming. + + Robinhood's documentation does not say whether `est_total_cost` includes `est_fee`, and the + live run behind #217 did not settle it either -- so this does not choose. The response states + `price`, `quantity`, `est_fee` and `est_total_cost`, which is one equation with one unknown, + and exactly one of three readings fits any self-consistent response: + + ============================ ===================================== ====================== + reading relation fee-exclusive notional + ============================ ===================================== ====================== + ``est_total_cost`` ``total == notional`` ``total`` + ``est_total_cost_less_...`` ``total == notional + fee`` ``total - fee`` + ``est_total_cost_plus_...`` ``total == notional - fee`` ``total + fee`` + ============================ ===================================== ====================== + + The third is not padding. A BUY's "total cost" plausibly adds the fee while a SELL's plausibly + nets it out of the proceeds, and this endpoint answers both sides -- and #217 observed only + the `ask` side, so the sell convention is genuinely unknown. + + All three recover the same fee-exclusive notional, which is the number `Preview.est_quote_size` + is defined to carry (the limit path fills it with `base_size * limit_price`, and `est_fee` is + a separate field beside it). Assigning a fee-INCLUSIVE `est_total_cost` straight into + `est_quote_size` would double-count the fee at the confirm gate: once inside the quote size and + once in `est_fee`. The venue's own arithmetic is still what is returned -- `total`, `total - + fee`, `total + fee` -- rather than the locally multiplied `notional`, so whatever precision or + rounding Robinhood applied survives. + + **A total fitting none of the three returns it unchanged AND an error.** That combination is + the point: refusing to price would degrade an exit preview over a number that is probably + right, while pricing it silently would put a cost in front of a human with an unverified + relationship to the order they are approving. The port has a field for exactly this middle + case, and it is `Preview.errors`. + """ + if total is None: + return notional, "price_x_quantity", () + + tolerance = max(_TOTAL_TOLERANCE_ABS, abs(notional) * _TOTAL_TOLERANCE_RATIO) + if abs(total - notional) <= tolerance: + return total, "est_total_cost", () + if fee is not None: + if abs(total - (notional + fee)) <= tolerance: + return total - fee, "est_total_cost_less_est_fee", () + if abs(total - (notional - fee)) <= tolerance: + return total + fee, "est_total_cost_plus_est_fee", () + return ( + total, + "est_total_cost_unreconciled", + ( + f"robinhood's est_total_cost ({total}) matches neither price x quantity " + f"({notional}) nor that notional offset by est_fee ({fee}); est_quote_size is the " + f"venue's est_total_cost exactly as sent, and whether it already includes the fee " + f"is UNVERIFIED -- do not read est_quote_size + est_fee as this order's total", + ), + ) + + def _confirms_cancel(order: object, order_id: str) -> bool: """Whether `order` is a confirmation that THIS id is cancelled. diff --git a/packages/keel-broker-robinhood/keel_broker_robinhood/transport.py b/packages/keel-broker-robinhood/keel_broker_robinhood/transport.py index 264f9222..4422bcdb 100644 --- a/packages/keel-broker-robinhood/keel_broker_robinhood/transport.py +++ b/packages/keel-broker-robinhood/keel_broker_robinhood/transport.py @@ -309,11 +309,16 @@ def _request( return None # `json.loads(..., parse_float=Decimal)`, never `response.json()`. `requests`' own decoder # parses an UNQUOTED JSON number as a `float`, and every money field on this venue -- - # price, quantity, fee -- can arrive unquoted. By the time the adapter runs its + # price, quantity, fee -- DOES arrive unquoted. By the time the adapter runs its # `Decimal(str(value))` the precision is already gone: it would faithfully preserve # whatever the float rounded to, not what Robinhood sent. Converting inside the parser is - # the only place the original digits still exist. Every fixture in this repository quotes - # its numbers, which is exactly why this could not have been caught by fixtures alone. + # the only place the original digits still exist. + # + # This was a defensive change when it landed (#194 S3) and is no longer: the first live + # run against a real credential (#217 F2) found EVERY money value unquoted, on every one + # of the five endpoints probed. The fixtures quoted all of them, which is why no test + # could have caught it -- they have since been corrected to unquoted numbers so the suite + # exercises this path rather than one the venue never produces. json_response: Any = json.loads(response.text, parse_float=Decimal) return json_response @@ -391,9 +396,19 @@ def get_trading_pairs(self, symbol: str | None = None) -> Any: down here rather than left to inference. They exist because they are the inputs the obvious next feature needs: `asset_increment`, - `quote_increment`, `min_order_amount`, and `max_order_size` are what would let this - package round a size to the venue's tick LOCALLY instead of discovering the violation as - a rejection. That work is deliberately not done here, and the reason is the same principle + `quote_increment`, and `max_order_size` are what would let this package round a size to + the venue's tick LOCALLY instead of discovering the violation as a rejection. + + ⚠️ **A minimum order size is NOT among them: this endpoint publishes none.** The rows + carry `symbol`, `asset_code`, `quote_code`, `asset_increment`, `quote_increment`, + `max_order_size`, `status` and `is_api_tradable`, and that is all -- there is no + `min_order_amount` and no `min_order_size` (#217 F3, observed live across four cursor + pages). This docstring named `min_order_amount` until that run, and the fixture invented + it, which between them gave the pre-flight minimum-size check proposed in #198 a source + that does not exist. Increment rounding and an upper bound can be checked locally against + this endpoint; a lower bound cannot be checked at all without a different source. + + That work is deliberately not done here, and the reason is the same principle that shapes `cancel_order` and `_account`: a pre-flight check that runs before every placement is also a check that runs before every EXIT, and one that raises -- or merely blocks on an extra round trip during an outage -- can trap a position it was meant to @@ -409,6 +424,13 @@ def get_trading_pairs(self, symbol: str | None = None) -> Any: def get_best_bid_ask(self, symbol: str) -> Any: # `marketdata`, not `trading` -- see `get_estimated_price` below for why these two # neighbouring endpoints genuinely sit under different namespaces in v2. + # + # Rows carry `symbol`, `bid` and `ask`. Nothing else, and specifically not the `price`, + # `buy_spread`, `sell_spread`, `ask_inclusive_of_buy_spread` or + # `bid_inclusive_of_sell_spread` the fixture invented before #217 F4 replaced it -- five + # keys, none of which this venue sends. No caller reads them today, which is the only + # reason that cost nothing; the risk was entirely in whatever got written against them + # next. return self._paginate( "/api/v2/crypto/marketdata/best_bid_ask/", params={"symbol": symbol} ) diff --git a/scripts/robinhood_smoke.py b/scripts/robinhood_smoke.py index 904568d9..e6020662 100644 --- a/scripts/robinhood_smoke.py +++ b/scripts/robinhood_smoke.py @@ -17,6 +17,14 @@ NOT a conformance suite and NOT part of the shipped wheel -- it is an operator tool, run by hand when a credential exists, and its only output is a shape report. +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. + ## Why it cannot place an order `_ReadOnly` wraps the transport's request method and raises on any method other than GET, so the @@ -50,6 +58,7 @@ import argparse import json import sys +from decimal import Decimal from pathlib import Path from typing import Any @@ -79,6 +88,18 @@ #: PEM, or a hex string -- into a precise message instead of a 401. _SEED_B64_LEN = 44 +#: The keys `RobinhoodTransport._paginate` consumes and does not pass on. +#: +#: Every probe below is a paginated read, so every probe's response reaches this script already +#: resolved into `{"results": [...]}` with the cursor stripped -- that is what `_paginate` is FOR. +#: The committed fixtures, by contrast, are single RAW pages and still carry both keys. The first +#: live run compared the two directly and reported `next` and `previous` `MISSING AT VENUE` on all +#: five probes, on a run where the venue had sent both every time. That was this script's bug, not +#: a finding, and it buried four real findings underneath ten lines of noise. A report that cries +#: wolf on every run is worse than no report at all, so the fixture is normalized to the shape a +#: probe can actually be compared against before anything is compared. +_PAGINATION_ENVELOPE_KEYS = frozenset({"next", "previous"}) + class ReadOnlyViolation(RuntimeError): """Raised when anything in this script attempts a non-GET request.""" @@ -139,6 +160,30 @@ def shape_of(value: Any) -> Any: return type(value).__name__ +def fixture_shape(path: Path) -> Any: + """The shape of a committed fixture, as a PROBE could ever observe it. + + Two normalizations, and both exist because a probe's response has already been through the + transport by the time this script sees it, while the fixture on disk has not: + + 1. **The pagination envelope is dropped.** `_paginate` resolves `next`/`previous` and hands + back `{"results": [...]}`, so comparing a post-pagination aggregate against a raw single + page reports both keys missing from a venue that sent them -- see + `_PAGINATION_ENVELOPE_KEYS`. Only a payload that actually has `results` is normalized: an + order object is left alone, so a genuine `next` field on some future endpoint would still + be compared rather than silently eaten. + 2. **Numbers are decoded with `parse_float=Decimal`,** matching `RobinhoodTransport._request` + exactly. Since #217 the fixtures quote nothing that the venue sends unquoted, so a plain + `json.loads` here would type every money field `float` while the live side reports + `Decimal` -- one `TYPE DIFFERS` line per money field per probe, which is the same + cry-wolf failure as the envelope, one layer down. + """ + payload = json.loads(path.read_text(), parse_float=Decimal) + if isinstance(payload, dict) and "results" in payload: + payload = {k: v for k, v in payload.items() if k not in _PAGINATION_ENVELOPE_KEYS} + return shape_of(payload) + + def compare_shapes(live: Any, fixture: Any, path: str = "") -> list[str]: """Return one human-readable line per structural difference, recursing into dicts. @@ -242,9 +287,7 @@ def report(results: dict[str, Any], as_json: bool) -> int: failures += 1 continue - fixture_path = FIXTURES / fixture_name - fixture_shape = shape_of(json.loads(fixture_path.read_text())) - diffs = compare_shapes(result["shape"], fixture_shape) + diffs = compare_shapes(result["shape"], fixture_shape(FIXTURES / fixture_name)) if not diffs: print(f" shape matches {fixture_name}") else: diff --git a/tests/broker_robinhood/test_adapter.py b/tests/broker_robinhood/test_adapter.py index 4373f4ae..3d37272e 100644 --- a/tests/broker_robinhood/test_adapter.py +++ b/tests/broker_robinhood/test_adapter.py @@ -26,8 +26,20 @@ def load_fixture(name: str) -> dict[str, Any]: + """Decode a fixture the way `RobinhoodTransport._request` decodes a live response. + + `parse_float=Decimal` is not a stylistic flourish here, it is the whole point of the fixture. + The first live run against a real credential (#217) established that **every money value on + this venue arrives as an UNQUOTED JSON number** -- `"ask": 65482.30`, not `"ask": "65482.30"` + -- and the transport parses those with `parse_float=Decimal` precisely so the original digits + reach the adapter instead of whatever a binary `float` rounded them to. A fixture decoded with + a plain `json.load` would hand the adapter `float`s the live path can never produce, so the + suite would be exercising a code path that does not exist in production and leaving the one + that does untested. It also silently weakens equality assertions: `Decimal(0.00000001)` is + `1.00000000000000002092256083497e-8`, which is not `Decimal("0.00000001")`. + """ with (FIXTURES_DIR / name).open() as f: - data: dict[str, Any] = json.load(f) + data: dict[str, Any] = json.load(f, parse_float=Decimal) return data @@ -271,11 +283,11 @@ def test_preview_order_market_ioc_base_prices_off_the_estimated_price_endpoint() estimated_price=load_fixture("rh_estimated_price.json"), ) adapter = RobinhoodAdapter(transport) - spec = MarketIOCByBase(product_id="BTC-USD", side=Side.SELL, base_size=Decimal("0.1")) + spec = MarketIOCByBase(product_id="BTC-USD", side=Side.BUY, base_size=Decimal("0.1")) preview = adapter.preview_order(spec) - price = Decimal(load_fixture("rh_estimated_price.json")["results"][0]["price"]) + price = Decimal(load_fixture("rh_estimated_price.json")["results"][0]["ask"]) assert preview.synthetic is True assert preview.est_base_size == Decimal("0.1") assert preview.est_quote_size == Decimal("0.1") * price @@ -283,6 +295,229 @@ def test_preview_order_market_ioc_base_prices_off_the_estimated_price_endpoint() assert transport.calls["get_estimated_price"]["symbol"] == "BTC-USD" +def test_preview_order_reads_the_ask_column_because_the_venue_sends_no_price_field() -> None: + """The #217 F1 blocker, pinned as a test: the row has no `price` key at all. + + `_estimated_price` read `_field(rows[0], "price", "0")`, which the documentation supported and + the venue does not. The first live run proved every market preview came back + `est_quote_size = 0.000` with `errors` populated -- confirm mode was unusable against this + venue, and the only reason it was survivable is the #194 S1 change that turned an unpriced + lookup into `None` rather than a silent `Decimal("0")`. + + Asserting the fixture carries no `price` key is deliberate. Without it, someone could + "fix" this by reading `price` with an `ask` fallback, the fixture would keep both keys, and + the suite would go on passing against a shape the venue never sends. + """ + row = load_fixture("rh_estimated_price.json")["results"][0] + assert "price" not in row, "the venue sends no 'price' on estimated_price -- see #217 F1" + assert "ask" in row + + transport = FakeTransport( + accounts=load_fixture("rh_accounts.json"), + estimated_price=load_fixture("rh_estimated_price.json"), + ) + spec = MarketIOCByBase(product_id="BTC-USD", side=Side.BUY, base_size=Decimal("0.1")) + + preview = RobinhoodAdapter(transport).preview_order(spec) + + assert preview.errors == (), "a fully-priced venue response must not report a pricing failure" + assert preview.est_quote_size == Decimal("0.1") * Decimal(row["ask"]) + + +def test_preview_order_reads_the_column_named_after_the_side_it_asked_for() -> None: + """A SELL is priced from `bid` and a BUY from `ask` -- never whichever column happens to + be present. + + The endpoint is asked for one side (`to_price_side`: buy -> ask, sell -> bid) and names the + price column after it. Falling back to the *other* column when the requested one is absent + would price a sell off the ask, overstating the proceeds of every exit -- the optimistic + direction `to_price_side`'s docstring exists to prevent, and the one a human at a confirm gate + is least likely to catch. So a row carrying only the wrong side is treated as UNPRICED. + """ + ask_only = load_fixture("rh_estimated_price.json") + bid_row = dict(ask_only["results"][0]) + bid_row["side"] = "bid" + bid_row["bid"] = bid_row.pop("ask") + accounts = load_fixture("rh_accounts.json") + + sell = MarketIOCByBase(product_id="BTC-USD", side=Side.SELL, base_size=Decimal("0.1")) + priced = RobinhoodAdapter( + FakeTransport(accounts=accounts, estimated_price={"results": [bid_row]}) + ).preview_order(sell) + assert priced.errors == () + assert priced.est_quote_size == Decimal("0.1") * Decimal(bid_row["bid"]) + + unpriced = RobinhoodAdapter( + FakeTransport(accounts=accounts, estimated_price=ask_only) + ).preview_order(sell) + assert unpriced.errors, "a sell priced off an ask-only row must not be reported as priced" + + +def test_preview_order_takes_the_fee_from_the_venue_rather_than_deriving_it() -> None: + """`est_fee` comes back on the row, so deriving one from the account's tier is second-hand. + + The venue states the fee it will charge for THIS quantity on THIS side. Multiplying our own + notional by `fee_tier_status.fee_ratio` reproduces a number the response already contains, and + the two can disagree -- the account tier is an account-level rate, while the row's `fee_ratio` + is the one quoted against this order. When the venue states it, the venue wins, and + `detail["fee_basis"]` records which of the two was used so a reader never has to guess. + """ + row = load_fixture("rh_estimated_price.json")["results"][0] + transport = FakeTransport( + accounts=load_fixture("rh_accounts.json"), + estimated_price=load_fixture("rh_estimated_price.json"), + ) + spec = MarketIOCByBase(product_id="BTC-USD", side=Side.BUY, base_size=Decimal("0.1")) + + preview = RobinhoodAdapter(transport).preview_order(spec) + + assert preview.est_fee == Decimal(row["est_fee"]) + assert preview.detail["fee_basis"] == "venue_est_fee" + assert preview.detail["fee_ratio"] == str(Decimal(row["fee_ratio"])) + + +def test_preview_order_splits_a_fee_inclusive_est_total_cost_back_out() -> None: + """`est_total_cost` is reconciled against the venue's own numbers, not assumed either way. + + `Preview` carries `est_quote_size` and `est_fee` as SEPARATE fields, and the limit path fills + `est_quote_size` with `base_size * limit_price` -- a notional that excludes the fee. So a + fee-INCLUSIVE `est_total_cost` assigned straight into `est_quote_size` would double-count the + fee at the confirm gate (once inside the quote size, once in `est_fee`). + + Nobody can settle from the documentation which of the two `est_total_cost` is, so nothing here + assumes: the row carries `ask`, `quantity`, `est_fee` and `est_total_cost`, which is one + equation with a single unknown. The committed fixture encodes the fee-INCLUSIVE reading + (`est_total_cost == ask * quantity + est_fee`), and if a future live run contradicts that, the + adapter reconciles the response it actually received instead of quietly mispricing it. + """ + row = load_fixture("rh_estimated_price.json")["results"][0] + notional = Decimal(row["ask"]) * Decimal(row["quantity"]) + assert Decimal(row["est_total_cost"]) == notional + Decimal(row["est_fee"]), ( + "the committed fixture must encode a self-consistent fee-inclusive total, so a live run " + "that disagrees falsifies it" + ) + + transport = FakeTransport( + accounts=load_fixture("rh_accounts.json"), + estimated_price=load_fixture("rh_estimated_price.json"), + ) + spec = MarketIOCByBase(product_id="BTC-USD", side=Side.BUY, base_size=Decimal("0.1")) + + preview = RobinhoodAdapter(transport).preview_order(spec) + + assert preview.est_quote_size == notional + assert preview.est_quote_size + preview.est_fee == Decimal(row["est_total_cost"]) + assert preview.detail["cost_basis"] == "est_total_cost_less_est_fee" + + +def test_preview_order_accepts_a_fee_exclusive_est_total_cost_unchanged() -> None: + """The other reading of the same field, and the adapter must not force one onto the other. + + If `est_total_cost` turns out to be the fee-EXCLUSIVE notional, subtracting `est_fee` from it + would understate the order by exactly one fee. The reconciliation is what tells the two apart, + and `detail["cost_basis"]` reports which relation the venue's own numbers satisfied. + """ + fixture = load_fixture("rh_estimated_price.json") + row = dict(fixture["results"][0]) + row["est_total_cost"] = Decimal(row["ask"]) * Decimal(row["quantity"]) + transport = FakeTransport( + accounts=load_fixture("rh_accounts.json"), estimated_price={"results": [row]} + ) + spec = MarketIOCByBase(product_id="BTC-USD", side=Side.BUY, base_size=Decimal("0.1")) + + preview = RobinhoodAdapter(transport).preview_order(spec) + + assert preview.errors == () + assert preview.est_quote_size == row["est_total_cost"] + assert preview.detail["cost_basis"] == "est_total_cost" + + +def test_preview_order_reports_a_total_that_reconciles_with_nothing() -> None: + """A total matching neither the notional nor the notional +/- the fee is not understood. + + This is the case that must never pass silently: the adapter has four numbers from the venue + and no interpretation of `est_total_cost` that fits them. Rendering that as an ordinary + preview would put a cost in front of a human with an unverified relationship to the order. + `Preview.errors` is the port's channel for a soft failure, so the unreconciled total surfaces + there rather than in `detail`, which a renderer is free not to show. + """ + fixture = load_fixture("rh_estimated_price.json") + row = dict(fixture["results"][0]) + row["est_total_cost"] = Decimal("9999.99") + transport = FakeTransport( + accounts=load_fixture("rh_accounts.json"), estimated_price={"results": [row]} + ) + spec = MarketIOCByBase(product_id="BTC-USD", side=Side.BUY, base_size=Decimal("0.1")) + + preview = RobinhoodAdapter(transport).preview_order(spec) + + assert preview.errors + assert any("est_total_cost" in error for error in preview.errors) + assert preview.est_quote_size == Decimal("9999.99") + assert preview.detail["cost_basis"] == "est_total_cost_unreconciled" + + +def test_preview_order_falls_back_to_price_times_quantity_without_a_total() -> None: + """No `est_total_cost` on the row is not a failure -- the price and the size still price it.""" + fixture = load_fixture("rh_estimated_price.json") + row = {k: v for k, v in fixture["results"][0].items() if k != "est_total_cost"} + transport = FakeTransport( + accounts=load_fixture("rh_accounts.json"), estimated_price={"results": [row]} + ) + spec = MarketIOCByBase(product_id="BTC-USD", side=Side.BUY, base_size=Decimal("0.1")) + + preview = RobinhoodAdapter(transport).preview_order(spec) + + assert preview.errors == () + assert preview.est_quote_size == Decimal("0.1") * Decimal(row["ask"]) + assert preview.detail["cost_basis"] == "price_x_quantity" + + +def test_preview_order_refuses_venue_totals_quoted_for_a_different_quantity() -> None: + """`est_fee` and `est_total_cost` describe the quantity the VENUE echoed, not ours. + + If the echoed `quantity` is not the size that was asked for, the row's totals are answers to a + different question and must not be read as this order's cost -- scaling them would be exactly + the "estimate that moves between the quote and the fill" this package refuses everywhere else. + The unit price still prices the order, so the preview degrades to `price * base_size` and says + why, rather than failing outright on an EXIT path. + """ + fixture = load_fixture("rh_estimated_price.json") + row = dict(fixture["results"][0]) + row["quantity"] = Decimal("0.001") + transport = FakeTransport( + accounts=load_fixture("rh_accounts.json"), estimated_price={"results": [row]} + ) + spec = MarketIOCByBase(product_id="BTC-USD", side=Side.BUY, base_size=Decimal("0.1")) + + preview = RobinhoodAdapter(transport).preview_order(spec) + + assert preview.est_quote_size == Decimal("0.1") * Decimal(row["ask"]) + assert preview.detail["cost_basis"] == "price_x_base_size" + assert preview.detail["fee_basis"] == "account_fee_ratio" + assert any("quantity" in error for error in preview.errors) + + +def test_preview_order_is_never_a_native_preview_however_much_the_venue_states() -> None: + """Reading the venue's own `est_fee`/`est_total_cost` does NOT make this a broker quote. + + `/estimated_price/` prices a QUANTITY. It does not validate the order, check buying power, + check the venue's own size bounds, or reserve anything -- an order this endpoint prices + happily can still be rejected the instant it is placed. That gap is exactly what + `Preview.synthetic` exists to carry, so it stays `True` and `supports_native_preview` stays + `False` no matter how many of the numbers came from the venue. + """ + transport = FakeTransport( + accounts=load_fixture("rh_accounts.json"), + estimated_price=load_fixture("rh_estimated_price.json"), + ) + adapter = RobinhoodAdapter(transport) + spec = MarketIOCByBase(product_id="BTC-USD", side=Side.BUY, base_size=Decimal("0.1")) + + assert adapter.capabilities().supports_native_preview is False + assert adapter.preview_order(spec).synthetic is True + + def test_place_order_market_ioc_base_sends_asset_quantity_and_no_time_in_force() -> None: """Robinhood's `market_order_config` accepts only `asset_quantity` -- there is no quote-sized market order and no `time_in_force` on this shape at all (market orders are @@ -669,7 +904,7 @@ def test_preview_order_on_a_fully_priced_order_reports_no_errors() -> None: estimated_price=load_fixture("rh_estimated_price.json"), ) adapter = RobinhoodAdapter(transport) - spec = MarketIOCByBase(product_id="BTC-USD", side=Side.SELL, base_size=Decimal("0.1")) + spec = MarketIOCByBase(product_id="BTC-USD", side=Side.BUY, base_size=Decimal("0.1")) assert adapter.preview_order(spec).errors == () @@ -749,3 +984,97 @@ def test_place_order_returns_a_domain_type() -> None: result = adapter.place_order(spec) assert isinstance(result, PlaceResult) + + +# --------------------------------------------------------------------------------------------- +# The fixtures themselves, held to the shapes the first live run actually observed (#217). +# +# These assert on `tests/fixtures/rh_*.json` rather than on adapter behaviour, which is unusual +# and deliberate. Robinhood ships no sandbox, so a fixture is the ONLY statement this repository +# makes about what the venue sends -- and #217 found three of them stating things it does not: +# an `estimated_price.price` that made every market preview unpriced, a `trading_pairs` +# minimum-order field that does not exist, and a `best_bid_ask` row that was invented outright. +# A wrong fixture is not a test-data nit here; it is a false claim about a live-money venue that +# the rest of the suite then confirms. +# --------------------------------------------------------------------------------------------- + +#: Every fixture field whose value the venue sends as an unquoted JSON number, per the #217 live +#: run. Keyed by fixture file; the paths are `results[]`-relative for the paginated reads and +#: top-level for the order objects. +_NUMERIC_FIELDS: dict[str, tuple[str, ...]] = { + "rh_accounts.json": ("buying_power",), + "rh_holdings.json": ("total_quantity", "quantity_available_for_trading"), + "rh_trading_pairs.json": ("asset_increment", "quote_increment", "max_order_size"), + "rh_best_bid_ask.json": ("bid", "ask"), + "rh_estimated_price.json": ("quantity", "fee_ratio", "est_fee", "ask", "est_total_cost"), +} + + +@pytest.mark.parametrize(("fixture_name", "fields"), sorted(_NUMERIC_FIELDS.items())) +def test_money_and_size_fixtures_are_unquoted_json_numbers( + fixture_name: str, fields: tuple[str, ...] +) -> None: + """Every money value arrives from this venue UNQUOTED, so every fixture must too. + + This is what makes #194's `parse_float=Decimal` load-bearing rather than defensive: with the + values quoted, `Decimal(str(v))` operated on a `str` that was already exact and the parser + setting was never exercised. Unquoted, the same value is a JSON number, and any decoder that + is not told otherwise routes it through a binary `float` before a `Decimal` ever sees it. + + The assertion is on the DECODED type, not on the file's bytes, because that is the property + the adapter depends on -- and it fails loudly if `load_fixture` ever loses its `parse_float`. + """ + rows = load_fixture(fixture_name)["results"] + assert rows + for field_name in fields: + value = rows[0][field_name] + assert isinstance(value, Decimal), f"{fixture_name}:{field_name} decoded as {type(value)}" + + +def test_the_account_fee_tier_status_is_numeric_throughout() -> None: + """`fee_tier_status` is the shape #216 called the most load-bearing guess in the package; the + live run corroborated every key name and contradicted every value's quoting.""" + tier = load_fixture("rh_accounts.json")["results"][0]["fee_tier_status"] + assert set(tier) == { + "fee_ratio", + "thirty_day_volume", + "next_fee_tier_ratio", + "next_fee_tier_threshold", + } + assert all(isinstance(value, Decimal) for value in tier.values()) + + +def test_trading_pairs_publishes_no_minimum_order_field() -> None: + """#217 F3: the venue sends neither `min_order_amount` nor `min_order_size`. + + The fixture invented `min_order_amount`, and `transport.get_trading_pairs`' docstring named it + as an input for the pre-flight sizing check proposed in #198. There is no source for a minimum + on this endpoint, so that half of the check has no basis -- `asset_increment`/`quote_increment` + rounding and `max_order_size` remain, a minimum does not. Keeping the invented key would let + that follow-up be written against a field that will simply be absent at runtime. + """ + pair = load_fixture("rh_trading_pairs.json")["results"][0] + assert "min_order_amount" not in pair + assert "min_order_size" not in pair + assert set(pair) == { + "symbol", + "asset_code", + "quote_code", + "asset_increment", + "quote_increment", + "max_order_size", + "status", + "is_api_tradable", + } + + +def test_best_bid_ask_carries_a_bid_and_an_ask_and_nothing_invented() -> None: + """#217 F4: the previous fixture was invented almost in full. + + It carried `price`, `buy_spread`, `sell_spread`, `ask_inclusive_of_buy_spread` and + `bid_inclusive_of_sell_spread` -- five keys, none of which the venue sends. Nothing read them, + which is why it survived; the danger was entirely in what would be written against them next. + """ + row = load_fixture("rh_best_bid_ask.json")["results"][0] + assert set(row) == {"symbol", "bid", "ask"} + assert row["bid"] < row["ask"], "a bid at or above the ask would invert every spread check" diff --git a/tests/broker_robinhood/test_transport.py b/tests/broker_robinhood/test_transport.py index 0baf8455..aaacde67 100644 --- a/tests/broker_robinhood/test_transport.py +++ b/tests/broker_robinhood/test_transport.py @@ -31,11 +31,31 @@ def _load_fixture(name: str) -> dict[str, Any]: + """A decoded fixture, for tests that only need a value out of it (an `account_number`). + + Deliberately NOT `parse_float=Decimal`, unlike `tests/broker_robinhood/test_adapter.py`'s + loader: the results of this one are handed back to `_FakeResponse(payload=...)`, which + re-serializes them with `json.dumps` -- and `json.dumps` cannot encode a `Decimal`. A test + that needs the venue's exact digits must use `_fixture_text` instead. + """ with (_FIXTURES_DIR / name).open() as f: data: dict[str, Any] = json.load(f) return data +def _fixture_text(name: str) -> str: + """A fixture's raw bytes, for replaying through the transport's own decoder. + + `_FakeResponse(text=...)` puts this string where a live `response.text` would be, so + `_request` parses it with `json.loads(..., parse_float=Decimal)` exactly as it parses the + venue. That is the only way to assert on the values a live response would actually produce: + decoding the fixture here and re-encoding it into `payload=` sends every unquoted number + through a binary `float` first, which is the precise loss `parse_float=Decimal` exists to + prevent. + """ + return (_FIXTURES_DIR / name).read_text() + + #: Throwaway Ed25519 test seed. Generated once for this file with #: `nacl.signing.SigningKey.generate()` and pasted here as a literal -- it has never been #: registered with Robinhood, or with anything else; it exists solely so `sign_payload` has a @@ -483,30 +503,39 @@ def test_trading_pairs_and_best_bid_ask_surface_their_documented_fields(http: An """The two reads the adapter does not call yet still have to return usable rows. `get_trading_pairs` and `get_best_bid_ask` are implemented but uninvoked -- see - `get_trading_pairs`' docstring for why local tick/minimum validation is a follow-up rather - than something to bolt onto the exit path. Uncalled code with unasserted fixtures is how a - method quietly rots into something that no longer works by the time the feature needing it - arrives, so this pins the fields that feature will read: `asset_increment` (the rounding - unit), `min_order_amount` / `max_order_size` (the bounds a rejected order would violate), and - the bid/ask legs a spread check would compare. - - `asset_increment` is `0.00000001` for BTC, which is the same value that makes - `translate._render` necessary -- one satoshi is a real order size on this venue, not a - rounding artefact. + `get_trading_pairs`' docstring for why local tick validation is a follow-up rather than + something to bolt onto the exit path. Uncalled code with unasserted fixtures is how a method + quietly rots into something that no longer works by the time the feature needing it arrives, + so this pins the fields that feature will read: `asset_increment` (the rounding unit), + `max_order_size` (a bound a rejected order would violate), and the bid/ask legs a spread check + would compare. + + There is deliberately **no minimum-order assertion**. The fixture used to carry + `min_order_amount` and the venue sends no such field, nor `min_order_size` (#217 F3) -- so a + pre-flight minimum check has no source on this endpoint, and asserting an invented bound here + would keep pointing #198 at one. + + The fixture's raw bytes are replayed rather than a re-serialized decode of them, so the values + reach the assertions through the same `json.loads(..., parse_float=Decimal)` the live path + uses. Round-tripping through `float` on the way in would defeat the exactness being asserted: + `asset_increment` is `0.00000001` for BTC, the same value that makes `translate._render` + necessary, and one satoshi is a real order size on this venue rather than a rounding artefact. """ - http(_FakeResponse(payload=_load_fixture("rh_trading_pairs.json"))) + http(_FakeResponse(text=_fixture_text("rh_trading_pairs.json"))) pair = _results(_transport().get_trading_pairs(symbol="BTC-USD"))[0] assert pair["symbol"] == "BTC-USD" - assert Decimal(pair["asset_increment"]) == Decimal("0.00000001") - assert Decimal(pair["min_order_amount"]) > 0 - assert Decimal(pair["max_order_size"]) > 0 + assert pair["asset_increment"] == Decimal("0.00000001") + assert isinstance(pair["asset_increment"], Decimal) + assert pair["max_order_size"] > 0 + assert "min_order_amount" not in pair + assert "min_order_size" not in pair - http(_FakeResponse(payload=_load_fixture("rh_best_bid_ask.json"))) + http(_FakeResponse(text=_fixture_text("rh_best_bid_ask.json"))) quote_row = _results(_transport().get_best_bid_ask(symbol="BTC-USD"))[0] assert quote_row["symbol"] == "BTC-USD" - bid = Decimal(quote_row["bid_inclusive_of_sell_spread"]) - ask = Decimal(quote_row["ask_inclusive_of_buy_spread"]) - assert bid < ask, "a bid at or above the ask would invert every spread-based check" + assert quote_row["bid"] < quote_row["ask"], ( + "a bid at or above the ask would invert every spread-based check" + ) # --------------------------------------------------------------------------------------------- diff --git a/tests/fixtures/rh_accounts.json b/tests/fixtures/rh_accounts.json index 67b3337b..5e307af7 100644 --- a/tests/fixtures/rh_accounts.json +++ b/tests/fixtures/rh_accounts.json @@ -5,15 +5,15 @@ { "account_number": "AB1234567890", "status": "active", - "buying_power": "1042.55", + "buying_power": 1042.55, "buying_power_currency": "USD", "account_type": "individual", "is_api_tradable": true, "fee_tier_status": { - "fee_ratio": "0.0025", - "thirty_day_volume": "12480.55", - "next_fee_tier_ratio": "0.0020", - "next_fee_tier_threshold": "50000.00" + "fee_ratio": 0.0025, + "thirty_day_volume": 12480.55, + "next_fee_tier_ratio": 0.0020, + "next_fee_tier_threshold": 50000.00 } } ] diff --git a/tests/fixtures/rh_best_bid_ask.json b/tests/fixtures/rh_best_bid_ask.json index f5013885..c4098d1a 100644 --- a/tests/fixtures/rh_best_bid_ask.json +++ b/tests/fixtures/rh_best_bid_ask.json @@ -4,12 +4,8 @@ "results": [ { "symbol": "BTC-USD", - "price": "65430.00", - "bid_inclusive_of_sell_spread": "65380.00", - "sell_spread": "0.05", - "ask_inclusive_of_buy_spread": "65480.00", - "buy_spread": "0.05", - "timestamp": "2026-08-09T12:00:00Z" + "bid": 65380.00, + "ask": 65480.00 } ] } diff --git a/tests/fixtures/rh_estimated_price.json b/tests/fixtures/rh_estimated_price.json index 5d5c2f31..f916c59c 100644 --- a/tests/fixtures/rh_estimated_price.json +++ b/tests/fixtures/rh_estimated_price.json @@ -5,8 +5,12 @@ { "symbol": "BTC-USD", "side": "ask", - "price": "65482.30", - "quantity": "0.1" + "quantity": 0.1, + "timestamp": "2026-08-09T12:00:00Z", + "fee_ratio": 0.0025, + "est_fee": 16.370575, + "ask": 65482.30, + "est_total_cost": 6564.600575 } ] } diff --git a/tests/fixtures/rh_holdings.json b/tests/fixtures/rh_holdings.json index 0fd6b987..3896b79e 100644 --- a/tests/fixtures/rh_holdings.json +++ b/tests/fixtures/rh_holdings.json @@ -5,8 +5,8 @@ { "account_number": "AB1234567890", "asset_code": "BTC", - "total_quantity": "0.53219871", - "quantity_available_for_trading": "0.50000000" + "total_quantity": 0.53219871, + "quantity_available_for_trading": 0.50000000 } ] } diff --git a/tests/fixtures/rh_order_canceled.json b/tests/fixtures/rh_order_canceled.json index c52f8d5b..10930e39 100644 --- a/tests/fixtures/rh_order_canceled.json +++ b/tests/fixtures/rh_order_canceled.json @@ -7,15 +7,15 @@ "type": "limit", "state": "canceled", "average_price": null, - "filled_asset_quantity": "0", + "filled_asset_quantity": 0, "created_at": "2026-08-09T11:40:00.000000Z", "updated_at": "2026-08-09T11:45:00.000000Z", - "fee_charged": "0", - "estimated_fee_remaining": "0", + "fee_charged": 0, + "estimated_fee_remaining": 0, "executions": [], "limit_order_config": { - "asset_quantity": "0.1", - "limit_price": "64000.00", + "asset_quantity": 0.1, + "limit_price": 64000.00, "time_in_force": "gtc" } } diff --git a/tests/fixtures/rh_order_filled.json b/tests/fixtures/rh_order_filled.json index 18c6859d..fe8a179d 100644 --- a/tests/fixtures/rh_order_filled.json +++ b/tests/fixtures/rh_order_filled.json @@ -6,20 +6,20 @@ "side": "sell", "type": "market", "state": "filled", - "average_price": "65420.75", - "filled_asset_quantity": "0.1", + "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", + "fee_charged": 1.6355, + "estimated_fee_remaining": 0, "executions": [ { - "effective_price": "65420.75", - "quantity": "0.1", + "effective_price": 65420.75, + "quantity": 0.1, "timestamp": "2026-08-09T11:55:01.000000Z" } ], "market_order_config": { - "asset_quantity": "0.1" + "asset_quantity": 0.1 } } diff --git a/tests/fixtures/rh_order_open.json b/tests/fixtures/rh_order_open.json index 290f5984..47fdfd9f 100644 --- a/tests/fixtures/rh_order_open.json +++ b/tests/fixtures/rh_order_open.json @@ -7,15 +7,15 @@ "type": "limit", "state": "open", "average_price": null, - "filled_asset_quantity": "0", + "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", + "fee_charged": 0, + "estimated_fee_remaining": 0.16, "executions": [], "limit_order_config": { - "asset_quantity": "0.1", - "limit_price": "64000.00", + "asset_quantity": 0.1, + "limit_price": 64000.00, "time_in_force": "gtc" } } diff --git a/tests/fixtures/rh_trading_pairs.json b/tests/fixtures/rh_trading_pairs.json index f6f36508..8886e2b0 100644 --- a/tests/fixtures/rh_trading_pairs.json +++ b/tests/fixtures/rh_trading_pairs.json @@ -6,10 +6,9 @@ "symbol": "BTC-USD", "asset_code": "BTC", "quote_code": "USD", - "asset_increment": "0.00000001", - "quote_increment": "0.01", - "max_order_size": "10.00000000", - "min_order_amount": "1.00", + "asset_increment": 0.00000001, + "quote_increment": 0.01, + "max_order_size": 10.00000000, "status": "tradable", "is_api_tradable": true } diff --git a/tests/scripts/test_robinhood_smoke.py b/tests/scripts/test_robinhood_smoke.py index 7e265322..1d68ca18 100644 --- a/tests/scripts/test_robinhood_smoke.py +++ b/tests/scripts/test_robinhood_smoke.py @@ -8,6 +8,7 @@ from __future__ import annotations import json +from decimal import Decimal from pathlib import Path from typing import Any @@ -18,6 +19,7 @@ ReadOnlyViolation, _ReadOnly, compare_shapes, + fixture_shape, load_credentials, run_probes, shape_of, @@ -25,6 +27,8 @@ _VALID_SEED_B64 = "A" * 44 # a base64 32-byte Ed25519 seed is 44 characters +_FIXTURES = Path(__file__).resolve().parents[1] / "fixtures" + class _StubTransport: """Records requests and replays canned payloads. Never touches the network.""" @@ -196,9 +200,54 @@ def test_a_wellformed_credential_is_returned(tmp_path: Path) -> None: def test_every_probe_names_a_fixture_that_exists() -> None: """A renamed fixture must break here, not halfway through a live run.""" - fixtures = Path(__file__).resolve().parents[1] / "fixtures" for name, fixture_name in PROBES: - assert (fixtures / fixture_name).is_file(), f"{name} points at a missing {fixture_name}" + assert (_FIXTURES / fixture_name).is_file(), f"{name} points at a missing {fixture_name}" + + +@pytest.mark.parametrize(("probe", "fixture_name"), PROBES) +def test_a_probe_response_matches_its_fixture_after_pagination( + probe: str, fixture_name: str +) -> None: + """The false positive #217 F5 reports, pinned so it cannot come back. + + Every one of the five probes goes through `RobinhoodTransport._paginate`, which resolves the + cursor and hands back `{"results": [...]}` -- `next` and `previous` are consumed there and + never reach a caller, by design. The fixtures are single RAW pages and still carry both. The + script compared the two directly, so the very first live run reported `next` and `previous` + `MISSING AT VENUE` on all five probes: five differences, on a clean run, against a venue that + sends both fields. A report that cries wolf every time is worse than no report, because the + real findings in that same run (F1 through F4) had to be read past it. + + This simulates the venue answering each fixture as its single page and asserts the comparison + the script actually performs is clean. It fails on both halves of the bug: an unstripped + envelope, and a fixture that has genuinely drifted from what the adapter reads. + """ + payload = json.loads((_FIXTURES / fixture_name).read_text(), parse_float=Decimal) + after_pagination = {"results": payload["results"]} + + diffs = compare_shapes(shape_of(after_pagination), fixture_shape(_FIXTURES / fixture_name)) + + assert diffs == [], f"{probe} reports differences against its own fixture: {diffs}" + + +def test_a_fixture_number_is_decoded_the_way_the_live_transport_decodes_it() -> None: + """Fixture money is unquoted (#217 F2) and must not be shape-reported as a `float`. + + `RobinhoodTransport._request` parses with `parse_float=Decimal`, so a live `65482.30` reaches + `shape_of` as a `Decimal`. A fixture decoded with a plain `json.loads` reaches it as a + `float`, and the script would then report `TYPE DIFFERS ... fixture='float' venue='Decimal'` + on every money field of every probe -- the same cry-wolf failure as F5, in a different place. + """ + shape = fixture_shape(_FIXTURES / "rh_estimated_price.json") + assert shape["results"][0]["ask"] == "Decimal" + + +def test_the_pagination_envelope_is_stripped_only_from_a_paginated_shape() -> None: + """`next`/`previous` are dropped because `_paginate` consumes them -- not because the keys are + unwelcome. A payload with no `results` (an order object) is left exactly as it is, so a real + key named `next` on some future endpoint would still be compared rather than silently eaten.""" + assert fixture_shape(_FIXTURES / "rh_order_open.json")["state"] == "str" + assert "next" not in fixture_shape(_FIXTURES / "rh_accounts.json") def test_a_failing_probe_does_not_abort_the_others() -> None: From 9ec331233d52928ce0c15ebf8bc4cdc3990a120a Mon Sep 17 00:00:00 2001 From: Elmehdi Aitbrahim Date: Sun, 9 Aug 2026 18:50:58 -0400 Subject: [PATCH 2/2] fix(brokers): match robinhood's mixed quoting, ask-only total, and quote timestamp Second live probe run against the branch (#217 F6-F8). F6. The venue is NOT uniformly unquoted -- it mixes, and `accounts` mixes within one object: `buying_power` is a quoted string beside an unquoted `fee_tier_status.fee_ratio`. `trading_pairs` and `best_bid_ask` quote all their money; `estimated_price` and `holdings` quote none. Reverted the six over-converted fields to strings so every fixture matches the venue field for field, and recorded the mixed quoting where it can be acted on: the transport's parser comment, `_decimal_or_none`, the README, and an executable test. The `parse_float=Decimal` + `Decimal(str(x))` pairing is what makes both forms land on the same exact number and neither half is optional. F7. `est_total_cost` is sent on the ask side only; a bid row carries `bid`, `quantity`, `fee_ratio` and `est_fee` and no total. The code already priced this correctly by falling through the reconciliation -- now it is intentional: a new `rh_estimated_price_bid.json` (a verbatim live bid row) and a test pinning that a sell prices from `bid x quantity` with the venue's `est_fee`, empty `errors`, and no `/accounts/` round trip. An error on every sell preview is an error nobody reads, so this must stay a clean answer rather than a degraded one. F8. `best_bid_ask` rows carry `timestamp`; added. Also: `rh_estimated_price.json` is now the verbatim live ask row, which settles the reconciliation empirically -- `64975.78 * 0.001 + 0.61726991 == 65.59304991`, i.e. fee-INCLUSIVE, exactly the reading that would have double-counted the fee. The relation is still derived per response, since that is one symbol, one side, one moment, and the bid side omits the field entirely. Preview tests size their specs from the fixture's echoed quantity rather than a literal, so the two cannot drift apart. Since the three order fixtures remain unverified on a venue proven to mix quoting, `get_order` is now tested against both the quoted and unquoted form of the same order and required to produce identical `Decimal`s. Co-Authored-By: Claude Opus 5 (1M context) --- packages/keel-broker-robinhood/README.md | 39 +++- .../keel_broker_robinhood/adapter.py | 58 +++-- .../keel_broker_robinhood/transport.py | 33 ++- scripts/robinhood_smoke.py | 6 + tests/broker_robinhood/test_adapter.py | 215 ++++++++++++++---- tests/broker_robinhood/test_transport.py | 23 +- tests/fixtures/rh_accounts.json | 6 +- tests/fixtures/rh_best_bid_ask.json | 5 +- tests/fixtures/rh_estimated_price.json | 12 +- tests/fixtures/rh_estimated_price_bid.json | 15 ++ tests/fixtures/rh_trading_pairs.json | 6 +- tests/scripts/test_robinhood_smoke.py | 24 +- 12 files changed, 347 insertions(+), 95 deletions(-) create mode 100644 tests/fixtures/rh_estimated_price_bid.json diff --git a/packages/keel-broker-robinhood/README.md b/packages/keel-broker-robinhood/README.md index f0b9ba7e..d2cc294d 100644 --- a/packages/keel-broker-robinhood/README.md +++ b/packages/keel-broker-robinhood/README.md @@ -38,12 +38,21 @@ proceeds of an exit, and a row that does not carry the requested side is treated The adapter also reads the venue's own `est_fee` instead of multiplying by the account's fee tier, and reconciles `est_total_cost` against `price * quantity` and `est_fee` on every response. -Whether `est_total_cost` includes the fee is **not documented and not assumed**. All three -self-consistent readings (`total == notional`, `total == notional + fee`, `total == notional - -fee`) recover the same fee-exclusive notional, which is what `Preview.est_quote_size` is defined -to carry, and `Preview.detail["cost_basis"]` reports which one this response satisfied. A total -satisfying none of them is priced from the venue's number as sent *and* reported through -`Preview.errors`. +Whether `est_total_cost` includes the fee is **not documented**. All three self-consistent +readings (`total == notional`, `total == notional + fee`, `total == notional - fee`) recover the +same fee-exclusive notional, which is what `Preview.est_quote_size` is defined to carry, and +`Preview.detail["cost_basis"]` reports which one this response satisfied. A total satisfying none +of them is priced from the venue's number as sent *and* reported through `Preview.errors`. + +A live ask-side row settles the reading empirically -- `64975.78 * 0.001 + 0.61726991 == +65.59304991`, so the total is **fee-inclusive** there, and assigning it straight into +`est_quote_size` would have double-counted the fee at the confirm gate. The relation is still +re-derived per response rather than hardcoded, because: + +**`est_total_cost` is sent on the ask side only.** A `side=bid` row carries `bid`, `quantity`, +`fee_ratio` and `est_fee` and no total at all. That is a complete answer, not a degraded one: a +sell prices from `bid * quantity` with the venue's own `est_fee` beside it, `errors` stays empty, +and `cost_basis` reads `price_x_quantity`. **None of that makes the preview a broker quote.** `/estimated_price/` prices a *quantity*: it does not validate the order, check buying power, check the account's own size bounds, or reserve @@ -72,6 +81,24 @@ on the live-money path, and it would do so silently. `translate.to_order_body` r `UnsupportedOrder` for `MarketIOCByQuote` with this reasoning in the message, as a second gate behind the adapter's capability declaration. +### This venue is not internally consistent about quoting money + +Some endpoints send money as unquoted JSON numbers and others send the same kinds of value as +quoted strings, and `accounts` does **both in the same object**: + +| | fields | +| --- | --- | +| unquoted numbers | `estimated_price.{ask,bid,quantity,fee_ratio,est_fee,est_total_cost}`, `accounts.fee_tier_status.*`, `holdings.{total_quantity,quantity_available_for_trading}` | +| quoted strings | `accounts.buying_power`, `trading_pairs.{asset_increment,quote_increment,max_order_size}`, `best_bid_ask.{bid,ask}` | + +There is therefore no venue-wide rule to code against and no field that may be assumed to be one +form or the other. Two things together make every read safe, and **both** are required: +`json.loads(..., parse_float=Decimal)` in the transport, so an unquoted number never passes +through a binary `float`; and `Decimal(str(value))` in the adapter, which is exact for a `str` and +a round-trip no-op for a `Decimal`. Do not "simplify" either into `Decimal(value)`, and do not add +an `isinstance` branch — there is nothing stable to branch on. The fixtures mirror the venue field +for field, mixed quoting included, so the suite exercises both paths. + ### No published minimum order size `GET /api/v2/crypto/trading/trading_pairs/` publishes `asset_increment`, `quote_increment` and diff --git a/packages/keel-broker-robinhood/keel_broker_robinhood/adapter.py b/packages/keel-broker-robinhood/keel_broker_robinhood/adapter.py index f05721cc..7159eb79 100644 --- a/packages/keel-broker-robinhood/keel_broker_robinhood/adapter.py +++ b/packages/keel-broker-robinhood/keel_broker_robinhood/adapter.py @@ -450,12 +450,19 @@ def _estimated_price(self, spec: OrderSpec) -> _VenueEstimate | None: would be precisely the "estimate that moves between the quote and the fill" this package refuses everywhere else, so only the unit price is used and `errors` says so. 2. `est_total_cost` must reconcile with `price * quantity`, either exactly (fee-exclusive) - or offset by `est_fee` in one direction or the other. Nothing in Robinhood's - documentation settles which of those it is, and #217 could not settle it either, so - nothing here assumes: `_reconcile_total` reads it off the numbers in each response. - A total that fits none of the three is reported through `errors` rather than quietly - priced -- see `_reconcile_total` for why all three readings recover the same - fee-exclusive notional, and why that is the number `Preview.est_quote_size` wants. + or offset by `est_fee` in one direction or the other. Robinhood's documentation settles + none of this, so nothing here assumes: `_reconcile_total` reads the relation off the + numbers in each response. A total that fits none of the three is reported through + `errors` rather than quietly priced -- see `_reconcile_total` for why all three + readings recover the same fee-exclusive notional, and why that is the number + `Preview.est_quote_size` wants. + + ⚠️ **`est_total_cost` is sent on the ASK side only** (#217 F7). The bid row carries + `bid`, `quantity`, `fee_ratio` and `est_fee` and simply has no total. That is a complete + answer, not a degraded one, and the sell path must not treat it as a failure: `price * + quantity` prices the order and the venue's own `est_fee` sits beside it, so `errors` stays + empty and `cost_basis` reads `price_x_quantity`. An exit preview that reported a problem + on every single call is an exit preview nobody would read. **`None`, never `Decimal("0")`.** This used to answer zero for "no rows", "unparseable price", and "the venue really did quote zero" alike, and `preview_order` had no way to @@ -723,10 +730,15 @@ def _decimal_or_none(value: Any) -> Decimal | None: """Parse one JSON leaf as a `Decimal`, or `None` if it is absent or not a number. `Decimal(str(value))` handles both shapes this venue produces without a branch: since #194 the - transport decodes with `parse_float=Decimal`, so an unquoted `65482.30` already arrives as a - `Decimal` (and `str()` of one round-trips exactly), while a quoted `"65482.30"` arrives as a - `str`. #217 confirmed the venue sends the unquoted form for every money field, but a `str` - remains possible on any field and costs nothing to accept. + transport decodes with `parse_float=Decimal`, so an unquoted `64975.78` already arrives as a + `Decimal` (and `str()` of one round-trips exactly), while a quoted `"64975.78"` arrives as a + `str`. + + Accepting both is not defensive breadth here, it is required: **this venue mixes the two** + (#217 F6). `estimated_price` sends every money field unquoted; `trading_pairs` and + `best_bid_ask` quote every one of theirs; and `accounts` does both at once, sending + `buying_power` quoted beside an unquoted `fee_tier_status.fee_ratio`. There is no rule to + branch on, so this does not branch. `None` rather than a zero default, everywhere. A zero here would flow into a preview as a real price, a real fee, or a real cost, and this whole module is built on the principle that an @@ -754,10 +766,10 @@ def _reconcile_total( ) -> tuple[Decimal, str, tuple[str, ...]]: """Work out what `est_total_cost` MEANS from the venue's own numbers, rather than assuming. - Robinhood's documentation does not say whether `est_total_cost` includes `est_fee`, and the - live run behind #217 did not settle it either -- so this does not choose. The response states - `price`, `quantity`, `est_fee` and `est_total_cost`, which is one equation with one unknown, - and exactly one of three readings fits any self-consistent response: + Robinhood's documentation does not say whether `est_total_cost` includes `est_fee`, so this + does not choose. The response states `price`, `quantity`, `est_fee` and `est_total_cost`, + which is one equation with one unknown, and exactly one of three readings fits any + self-consistent response: ============================ ===================================== ====================== reading relation fee-exclusive notional @@ -767,9 +779,17 @@ def _reconcile_total( ``est_total_cost_plus_...`` ``total == notional - fee`` ``total + fee`` ============================ ===================================== ====================== - The third is not padding. A BUY's "total cost" plausibly adds the fee while a SELL's plausibly - nets it out of the proceeds, and this endpoint answers both sides -- and #217 observed only - the `ask` side, so the sell convention is genuinely unknown. + A live ask-side row (#217) satisfies the SECOND reading exactly:: + + 64975.78 * 0.001 + 0.61726991 == 65.59304991 + + so `est_total_cost` is fee-INCLUSIVE there, and assigning it straight into + `Preview.est_quote_size` would have double-counted the fee at the confirm gate. That is one + symbol, one side, one moment, and the venue does not send the field on the bid side at all + (#217 F7) -- which is exactly why the relation is re-derived per response rather than being + hardcoded now that it is known once. The third reading is not padding either: a BUY's "total + cost" plausibly adds the fee while a SELL's plausibly nets it out of the proceeds, and if the + venue ever starts answering `bid` with a total, this is what will read it correctly. All three recover the same fee-exclusive notional, which is the number `Preview.est_quote_size` is defined to carry (the limit path fills it with `base_size * limit_price`, and `est_fee` is @@ -785,6 +805,10 @@ def _reconcile_total( relationship to the order they are approving. The port has a field for exactly this middle case, and it is `Preview.errors`. """ + # No total is the NORMAL bid-side answer, not an edge case: this venue sends `est_total_cost` + # on the ask side only (#217 F7). `price * quantity` with the venue's own `est_fee` beside it + # is a complete estimate, so this returns no error -- every sell preview would carry one + # otherwise, and an error on every call is an error nobody reads. if total is None: return notional, "price_x_quantity", () diff --git a/packages/keel-broker-robinhood/keel_broker_robinhood/transport.py b/packages/keel-broker-robinhood/keel_broker_robinhood/transport.py index 4422bcdb..b1e57dbb 100644 --- a/packages/keel-broker-robinhood/keel_broker_robinhood/transport.py +++ b/packages/keel-broker-robinhood/keel_broker_robinhood/transport.py @@ -308,17 +308,30 @@ def _request( if not response.content: return None # `json.loads(..., parse_float=Decimal)`, never `response.json()`. `requests`' own decoder - # parses an UNQUOTED JSON number as a `float`, and every money field on this venue -- - # price, quantity, fee -- DOES arrive unquoted. By the time the adapter runs its - # `Decimal(str(value))` the precision is already gone: it would faithfully preserve - # whatever the float rounded to, not what Robinhood sent. Converting inside the parser is - # the only place the original digits still exist. + # parses an UNQUOTED JSON number as a `float`, and money fields on this venue DO arrive + # unquoted. By the time the adapter runs its `Decimal(str(value))` the precision is + # already gone: it would faithfully preserve whatever the float rounded to, not what + # Robinhood sent. Converting inside the parser is the only place the original digits still + # exist. # - # This was a defensive change when it landed (#194 S3) and is no longer: the first live - # run against a real credential (#217 F2) found EVERY money value unquoted, on every one - # of the five endpoints probed. The fixtures quoted all of them, which is why no test - # could have caught it -- they have since been corrected to unquoted numbers so the suite - # exercises this path rather than one the venue never produces. + # This was a defensive change when it landed (#194 S3) and is no longer. + # + # ⚠️ **This venue is NOT internally consistent about quoting** (#217 F6), which is the + # part worth remembering -- it is not "Robinhood sends numbers": + # + # unquoted: estimated_price.{ask,bid,quantity,fee_ratio,est_fee,est_total_cost} + # accounts.fee_tier_status.* + # holdings.{total_quantity,quantity_available_for_trading} + # quoted: accounts.buying_power + # trading_pairs.{asset_increment,quote_increment,max_order_size} + # best_bid_ask.{bid,ask} + # + # `accounts` sends `buying_power` quoted and `fee_tier_status.fee_ratio` unquoted in the + # SAME object. So no field anywhere may be assumed to be one or the other, and no + # `isinstance` branch is safe: `parse_float=Decimal` here plus `Decimal(str(value))` in + # the adapter is what makes both forms land on the same exact number, and both halves are + # required. The fixtures now mirror the venue field for field, so the suite exercises both + # paths rather than a uniformity that does not exist. json_response: Any = json.loads(response.text, parse_float=Decimal) return json_response diff --git a/scripts/robinhood_smoke.py b/scripts/robinhood_smoke.py index e6020662..0f96ad1c 100644 --- a/scripts/robinhood_smoke.py +++ b/scripts/robinhood_smoke.py @@ -70,6 +70,12 @@ #: Read-only probes, in dependency order: `accounts` first because it resolves the account #: number every later call needs, and because it is the cheapest possible proof that the #: signature is accepted. +#: +#: `estimated_price` is probed on the ASK side only, so `tests/fixtures/rh_estimated_price_bid.json` +#: is deliberately absent from this list. The two sides do not answer with the same shape -- the +#: bid side omits `est_total_cost` entirely (#217 F7) -- so probing both would need a sixth entry +#: keyed by side rather than by endpoint. The bid shape is pinned by the adapter's tests against +#: that fixture instead; adding a side-aware probe here is a follow-up, not a nit. PROBES: tuple[tuple[str, str], ...] = ( ("accounts", "rh_accounts.json"), ("trading_pairs", "rh_trading_pairs.json"), diff --git a/tests/broker_robinhood/test_adapter.py b/tests/broker_robinhood/test_adapter.py index 3d37272e..c57e9cc4 100644 --- a/tests/broker_robinhood/test_adapter.py +++ b/tests/broker_robinhood/test_adapter.py @@ -29,20 +29,35 @@ def load_fixture(name: str) -> dict[str, Any]: """Decode a fixture the way `RobinhoodTransport._request` decodes a live response. `parse_float=Decimal` is not a stylistic flourish here, it is the whole point of the fixture. - The first live run against a real credential (#217) established that **every money value on - this venue arrives as an UNQUOTED JSON number** -- `"ask": 65482.30`, not `"ask": "65482.30"` - -- and the transport parses those with `parse_float=Decimal` precisely so the original digits + Live runs against a real credential (#217 F2/F6) established that money on this venue arrives + as an UNQUOTED JSON number on some endpoints -- `"ask": 64975.78`, not `"ask": "64975.78"` -- + and the transport parses those with `parse_float=Decimal` precisely so the original digits reach the adapter instead of whatever a binary `float` rounded them to. A fixture decoded with a plain `json.load` would hand the adapter `float`s the live path can never produce, so the suite would be exercising a code path that does not exist in production and leaving the one that does untested. It also silently weakens equality assertions: `Decimal(0.00000001)` is `1.00000000000000002092256083497e-8`, which is not `Decimal("0.00000001")`. + + The fixtures are quoted field-for-field the way the venue quotes them, which means MIXED -- + see `test_this_venue_is_not_internally_consistent_about_quoting`. Normalizing them one way or + the other would read better and would be a false claim about the API. """ with (FIXTURES_DIR / name).open() as f: data: dict[str, Any] = json.load(f, parse_float=Decimal) return data +#: The size `rh_estimated_price*.json` was quoted FOR, read back out of the fixture. +#: +#: Every preview test that drives those fixtures sizes its spec from this rather than a literal, +#: because the adapter refuses to read `est_fee`/`est_total_cost` when the venue's echoed +#: `quantity` is not the size that was requested -- the two are a matched pair, and a literal in +#: the test would silently start exercising the mismatch path the day the fixture is re-captured +#: from a live run at a different size. Both fixtures are verbatim live rows (#217 F1/F7), and +#: `0.001 BTC` is the size the probe quotes at. +_QUOTED_SIZE = Decimal("0.001") + + def _contains_key(obj: Any, key: str) -> bool: """Recursively check `obj` for `key` at any nesting depth. @@ -283,14 +298,14 @@ def test_preview_order_market_ioc_base_prices_off_the_estimated_price_endpoint() estimated_price=load_fixture("rh_estimated_price.json"), ) adapter = RobinhoodAdapter(transport) - spec = MarketIOCByBase(product_id="BTC-USD", side=Side.BUY, base_size=Decimal("0.1")) + spec = MarketIOCByBase(product_id="BTC-USD", side=Side.BUY, base_size=_QUOTED_SIZE) preview = adapter.preview_order(spec) price = Decimal(load_fixture("rh_estimated_price.json")["results"][0]["ask"]) assert preview.synthetic is True - assert preview.est_base_size == Decimal("0.1") - assert preview.est_quote_size == Decimal("0.1") * price + assert preview.est_base_size == _QUOTED_SIZE + assert preview.est_quote_size == _QUOTED_SIZE * price assert preview.detail["price_basis"] == "estimated_price" assert transport.calls["get_estimated_price"]["symbol"] == "BTC-USD" @@ -316,12 +331,12 @@ def test_preview_order_reads_the_ask_column_because_the_venue_sends_no_price_fie accounts=load_fixture("rh_accounts.json"), estimated_price=load_fixture("rh_estimated_price.json"), ) - spec = MarketIOCByBase(product_id="BTC-USD", side=Side.BUY, base_size=Decimal("0.1")) + spec = MarketIOCByBase(product_id="BTC-USD", side=Side.BUY, base_size=_QUOTED_SIZE) preview = RobinhoodAdapter(transport).preview_order(spec) assert preview.errors == (), "a fully-priced venue response must not report a pricing failure" - assert preview.est_quote_size == Decimal("0.1") * Decimal(row["ask"]) + assert preview.est_quote_size == _QUOTED_SIZE * Decimal(row["ask"]) def test_preview_order_reads_the_column_named_after_the_side_it_asked_for() -> None: @@ -335,17 +350,16 @@ def test_preview_order_reads_the_column_named_after_the_side_it_asked_for() -> N is least likely to catch. So a row carrying only the wrong side is treated as UNPRICED. """ ask_only = load_fixture("rh_estimated_price.json") - bid_row = dict(ask_only["results"][0]) - bid_row["side"] = "bid" - bid_row["bid"] = bid_row.pop("ask") + bid_only = load_fixture("rh_estimated_price_bid.json") + bid_row = bid_only["results"][0] accounts = load_fixture("rh_accounts.json") - sell = MarketIOCByBase(product_id="BTC-USD", side=Side.SELL, base_size=Decimal("0.1")) + sell = MarketIOCByBase(product_id="BTC-USD", side=Side.SELL, base_size=_QUOTED_SIZE) priced = RobinhoodAdapter( - FakeTransport(accounts=accounts, estimated_price={"results": [bid_row]}) + FakeTransport(accounts=accounts, estimated_price=bid_only) ).preview_order(sell) assert priced.errors == () - assert priced.est_quote_size == Decimal("0.1") * Decimal(bid_row["bid"]) + assert priced.est_quote_size == _QUOTED_SIZE * Decimal(bid_row["bid"]) unpriced = RobinhoodAdapter( FakeTransport(accounts=accounts, estimated_price=ask_only) @@ -353,6 +367,42 @@ def test_preview_order_reads_the_column_named_after_the_side_it_asked_for() -> N assert unpriced.errors, "a sell priced off an ask-only row must not be reported as priced" +def test_preview_order_prices_a_sell_without_the_est_total_cost_the_venue_omits() -> None: + """#217 F7: `est_total_cost` comes back on the ASK side only. The bid row has no total. + + Observed live in the same minute, same credential, same symbol:: + + side=ask -> {..., 'est_fee': ..., 'ask': ..., 'est_total_cost': ...} + side=bid -> {..., 'est_fee': ..., 'bid': ...} # no total + + That is not a degraded response and must not read as one. A sell prices from `bid * quantity` + with the venue's own `est_fee` beside it, which is a complete answer -- `errors` stays empty, + and `detail["cost_basis"]` says `price_x_quantity` rather than naming a total that was never + sent. Getting this wrong in the direction of caution would be its own failure: an exit preview + that reports an error every single time is an exit preview nobody reads. + + The asymmetry is at least coherent with what the fields mean -- "total cost" is a buyer's + concept -- but the venue documents neither field, so this is pinned as an observation. + """ + fixture = load_fixture("rh_estimated_price_bid.json") + row = fixture["results"][0] + assert "est_total_cost" not in row, "the venue sends no total on the bid side -- see #217 F7" + + transport = FakeTransport(accounts=load_fixture("rh_accounts.json"), estimated_price=fixture) + spec = MarketIOCByBase(product_id="BTC-USD", side=Side.SELL, base_size=_QUOTED_SIZE) + + preview = RobinhoodAdapter(transport).preview_order(spec) + + assert preview.errors == () + assert preview.est_quote_size == _QUOTED_SIZE * Decimal(row["bid"]) + assert preview.est_fee == Decimal(row["est_fee"]) + assert preview.detail["cost_basis"] == "price_x_quantity" + assert preview.detail["fee_basis"] == "venue_est_fee" + assert "get_accounts" not in transport.calls, ( + "the venue stated the fee, so the account round trip must be skipped" + ) + + def test_preview_order_takes_the_fee_from_the_venue_rather_than_deriving_it() -> None: """`est_fee` comes back on the row, so deriving one from the account's tier is second-hand. @@ -367,7 +417,7 @@ def test_preview_order_takes_the_fee_from_the_venue_rather_than_deriving_it() -> accounts=load_fixture("rh_accounts.json"), estimated_price=load_fixture("rh_estimated_price.json"), ) - spec = MarketIOCByBase(product_id="BTC-USD", side=Side.BUY, base_size=Decimal("0.1")) + spec = MarketIOCByBase(product_id="BTC-USD", side=Side.BUY, base_size=_QUOTED_SIZE) preview = RobinhoodAdapter(transport).preview_order(spec) @@ -384,11 +434,19 @@ def test_preview_order_splits_a_fee_inclusive_est_total_cost_back_out() -> None: fee-INCLUSIVE `est_total_cost` assigned straight into `est_quote_size` would double-count the fee at the confirm gate (once inside the quote size, once in `est_fee`). - Nobody can settle from the documentation which of the two `est_total_cost` is, so nothing here - assumes: the row carries `ask`, `quantity`, `est_fee` and `est_total_cost`, which is one - equation with a single unknown. The committed fixture encodes the fee-INCLUSIVE reading - (`est_total_cost == ask * quantity + est_fee`), and if a future live run contradicts that, the - adapter reconciles the response it actually received instead of quietly mispricing it. + Nothing here assumes which it is: the row carries `ask`, `quantity`, `est_fee` and + `est_total_cost`, which is one equation with a single unknown, and the adapter reconciles the + response it actually received. + + The fixture is a verbatim live row, and it settles the question empirically -- the venue's own + numbers satisfy the fee-INCLUSIVE relation to the last digit:: + + 64975.78 * 0.001 + 0.61726991 == 65.59304991 + + That is now an observation rather than a prior, and it is exactly the reading that would have + double-counted the fee had `est_total_cost` been assigned straight into `est_quote_size`. The + reconciliation still runs on every response: this is one symbol, one side, one moment, and the + bid side does not even send the field (#217 F7). """ row = load_fixture("rh_estimated_price.json")["results"][0] notional = Decimal(row["ask"]) * Decimal(row["quantity"]) @@ -401,7 +459,7 @@ def test_preview_order_splits_a_fee_inclusive_est_total_cost_back_out() -> None: accounts=load_fixture("rh_accounts.json"), estimated_price=load_fixture("rh_estimated_price.json"), ) - spec = MarketIOCByBase(product_id="BTC-USD", side=Side.BUY, base_size=Decimal("0.1")) + spec = MarketIOCByBase(product_id="BTC-USD", side=Side.BUY, base_size=_QUOTED_SIZE) preview = RobinhoodAdapter(transport).preview_order(spec) @@ -423,7 +481,7 @@ def test_preview_order_accepts_a_fee_exclusive_est_total_cost_unchanged() -> Non transport = FakeTransport( accounts=load_fixture("rh_accounts.json"), estimated_price={"results": [row]} ) - spec = MarketIOCByBase(product_id="BTC-USD", side=Side.BUY, base_size=Decimal("0.1")) + spec = MarketIOCByBase(product_id="BTC-USD", side=Side.BUY, base_size=_QUOTED_SIZE) preview = RobinhoodAdapter(transport).preview_order(spec) @@ -447,7 +505,7 @@ def test_preview_order_reports_a_total_that_reconciles_with_nothing() -> None: transport = FakeTransport( accounts=load_fixture("rh_accounts.json"), estimated_price={"results": [row]} ) - spec = MarketIOCByBase(product_id="BTC-USD", side=Side.BUY, base_size=Decimal("0.1")) + spec = MarketIOCByBase(product_id="BTC-USD", side=Side.BUY, base_size=_QUOTED_SIZE) preview = RobinhoodAdapter(transport).preview_order(spec) @@ -464,12 +522,12 @@ def test_preview_order_falls_back_to_price_times_quantity_without_a_total() -> N transport = FakeTransport( accounts=load_fixture("rh_accounts.json"), estimated_price={"results": [row]} ) - spec = MarketIOCByBase(product_id="BTC-USD", side=Side.BUY, base_size=Decimal("0.1")) + spec = MarketIOCByBase(product_id="BTC-USD", side=Side.BUY, base_size=_QUOTED_SIZE) preview = RobinhoodAdapter(transport).preview_order(spec) assert preview.errors == () - assert preview.est_quote_size == Decimal("0.1") * Decimal(row["ask"]) + assert preview.est_quote_size == _QUOTED_SIZE * Decimal(row["ask"]) assert preview.detail["cost_basis"] == "price_x_quantity" @@ -484,15 +542,15 @@ def test_preview_order_refuses_venue_totals_quoted_for_a_different_quantity() -> """ fixture = load_fixture("rh_estimated_price.json") row = dict(fixture["results"][0]) - row["quantity"] = Decimal("0.001") + row["quantity"] = _QUOTED_SIZE * 10 transport = FakeTransport( accounts=load_fixture("rh_accounts.json"), estimated_price={"results": [row]} ) - spec = MarketIOCByBase(product_id="BTC-USD", side=Side.BUY, base_size=Decimal("0.1")) + spec = MarketIOCByBase(product_id="BTC-USD", side=Side.BUY, base_size=_QUOTED_SIZE) preview = RobinhoodAdapter(transport).preview_order(spec) - assert preview.est_quote_size == Decimal("0.1") * Decimal(row["ask"]) + assert preview.est_quote_size == _QUOTED_SIZE * Decimal(row["ask"]) assert preview.detail["cost_basis"] == "price_x_base_size" assert preview.detail["fee_basis"] == "account_fee_ratio" assert any("quantity" in error for error in preview.errors) @@ -512,7 +570,7 @@ def test_preview_order_is_never_a_native_preview_however_much_the_venue_states() estimated_price=load_fixture("rh_estimated_price.json"), ) adapter = RobinhoodAdapter(transport) - spec = MarketIOCByBase(product_id="BTC-USD", side=Side.BUY, base_size=Decimal("0.1")) + spec = MarketIOCByBase(product_id="BTC-USD", side=Side.BUY, base_size=_QUOTED_SIZE) assert adapter.capabilities().supports_native_preview is False assert adapter.preview_order(spec).synthetic is True @@ -624,6 +682,36 @@ def test_get_order_maps_fill_quantity_average_price_and_fee() -> None: assert order.total_fees == Decimal(fixture["fee_charged"]) +def test_get_order_reads_the_same_money_whether_the_venue_quotes_it_or_not() -> None: + """The order fixtures are the three this repository has never seen live, on a venue that is + demonstrably inconsistent about quoting (#217 F6). + + Observing an order object means placing a real order, which the probe refuses by construction, + so `average_price` / `filled_asset_quantity` / `fee_charged` could arrive either way and this + package has no basis to prefer one. Rather than commit to a guess in the fixture and leave the + other half untested, this drives BOTH forms through `get_order` and requires identical + `Decimal`s out. `Decimal(str(value))` is what makes that true -- exact for a `str`, a + round-trip no-op for a `Decimal` -- and this is the test that fails if anyone "simplifies" it + to `Decimal(value)`. + """ + unquoted = load_fixture("rh_order_filled.json") + quoted = { + key: (str(value) if isinstance(value, Decimal) else value) + for key, value in unquoted.items() + } + assert quoted != unquoted, "the fixture must carry unquoted numbers for this to prove anything" + + orders = [] + for fixture in (unquoted, quoted): + transport = FakeTransport(order=fixture) + transport._issued_order_ids.add(fixture["id"]) + orders.append(RobinhoodAdapter(transport).get_order(fixture["id"])) + + assert orders[0] == orders[1] + assert orders[0].average_filled_price == Decimal("65420.75") + assert orders[0].total_fees == Decimal("1.6355") + + def test_get_order_maps_canceled_state_to_the_ports_doubled_l_spelling() -> None: """Robinhood spells it `canceled` (American, one `l`); the port spells it `CANCELLED`. This is the one place those two spellings must actually agree, or a genuinely cancelled order @@ -904,7 +992,7 @@ def test_preview_order_on_a_fully_priced_order_reports_no_errors() -> None: estimated_price=load_fixture("rh_estimated_price.json"), ) adapter = RobinhoodAdapter(transport) - spec = MarketIOCByBase(product_id="BTC-USD", side=Side.BUY, base_size=Decimal("0.1")) + spec = MarketIOCByBase(product_id="BTC-USD", side=Side.BUY, base_size=_QUOTED_SIZE) assert adapter.preview_order(spec).errors == () @@ -998,23 +1086,28 @@ def test_place_order_returns_a_domain_type() -> None: # the rest of the suite then confirms. # --------------------------------------------------------------------------------------------- -#: Every fixture field whose value the venue sends as an unquoted JSON number, per the #217 live -#: run. Keyed by fixture file; the paths are `results[]`-relative for the paginated reads and -#: top-level for the order objects. +#: Money and size fields the venue sends as UNQUOTED JSON numbers, keyed by fixture. Paths are +#: `results[]`-relative. _NUMERIC_FIELDS: dict[str, tuple[str, ...]] = { - "rh_accounts.json": ("buying_power",), "rh_holdings.json": ("total_quantity", "quantity_available_for_trading"), + "rh_estimated_price.json": ("quantity", "fee_ratio", "est_fee", "ask", "est_total_cost"), + "rh_estimated_price_bid.json": ("quantity", "fee_ratio", "est_fee", "bid"), +} + +#: Money and size fields the venue sends as QUOTED STRINGS. Same run, same credential, same +#: minute -- see `test_this_venue_is_not_internally_consistent_about_quoting`. +_QUOTED_FIELDS: dict[str, tuple[str, ...]] = { + "rh_accounts.json": ("buying_power",), "rh_trading_pairs.json": ("asset_increment", "quote_increment", "max_order_size"), "rh_best_bid_ask.json": ("bid", "ask"), - "rh_estimated_price.json": ("quantity", "fee_ratio", "est_fee", "ask", "est_total_cost"), } @pytest.mark.parametrize(("fixture_name", "fields"), sorted(_NUMERIC_FIELDS.items())) -def test_money_and_size_fixtures_are_unquoted_json_numbers( +def test_the_venues_unquoted_money_fields_decode_as_decimal( fixture_name: str, fields: tuple[str, ...] ) -> None: - """Every money value arrives from this venue UNQUOTED, so every fixture must too. + """These arrive from the venue as JSON numbers, so the fixtures must send them as numbers. This is what makes #194's `parse_float=Decimal` load-bearing rather than defensive: with the values quoted, `Decimal(str(v))` operated on a `str` that was already exact and the parser @@ -1031,9 +1124,44 @@ def test_money_and_size_fixtures_are_unquoted_json_numbers( assert isinstance(value, Decimal), f"{fixture_name}:{field_name} decoded as {type(value)}" +@pytest.mark.parametrize(("fixture_name", "fields"), sorted(_QUOTED_FIELDS.items())) +def test_the_venues_quoted_money_fields_decode_as_str( + fixture_name: str, fields: tuple[str, ...] +) -> None: + """And these arrive QUOTED, so the fixtures must not "improve" them into numbers. + + A fixture that is uniformly one or the other is easier to look at and is a lie about this + venue either way. The point of a fixture here is to be the shape the adapter will really meet. + """ + rows = load_fixture(fixture_name)["results"] + assert rows + for field_name in fields: + value = rows[0][field_name] + assert isinstance(value, str), f"{fixture_name}:{field_name} decoded as {type(value)}" + + +def test_this_venue_is_not_internally_consistent_about_quoting() -> None: + """⚠️ The finding worth carrying forward from #217 F6, stated as an executable claim. + + `buying_power` is a quoted string and `fee_tier_status.fee_ratio` is an unquoted number **in + the same JSON object**, from the same request. `trading_pairs` and `best_bid_ask` quote every + money value; `estimated_price` and `holdings` quote none of them. + + So there is no venue-wide rule to code against, and no field can be assumed to be one or the + other -- not even two fields sitting side by side. The only safe reads are the two this + package already performs: `parse_float=Decimal` in the transport, so an unquoted number never + passes through a binary `float`, and `Decimal(str(value))` in the adapter, which is exact for + a `str` and a round-trip no-op for a `Decimal`. Neither `Decimal(x)` on a raw value nor an + `isinstance` branch is safe anywhere in this package. + """ + account = load_fixture("rh_accounts.json")["results"][0] + assert isinstance(account["buying_power"], str) + assert isinstance(account["fee_tier_status"]["fee_ratio"], Decimal) + + def test_the_account_fee_tier_status_is_numeric_throughout() -> None: """`fee_tier_status` is the shape #216 called the most load-bearing guess in the package; the - live run corroborated every key name and contradicted every value's quoting.""" + live run corroborated every key name, and every one of its values is unquoted.""" tier = load_fixture("rh_accounts.json")["results"][0]["fee_tier_status"] assert set(tier) == { "fee_ratio", @@ -1074,7 +1202,14 @@ def test_best_bid_ask_carries_a_bid_and_an_ask_and_nothing_invented() -> None: It carried `price`, `buy_spread`, `sell_spread`, `ask_inclusive_of_buy_spread` and `bid_inclusive_of_sell_spread` -- five keys, none of which the venue sends. Nothing read them, which is why it survived; the danger was entirely in what would be written against them next. + + `timestamp` is the mirror-image miss (#217 F8): a field the venue DOES send that the first + correction left out. Both directions matter, which is why `compare_shapes` reports both. """ row = load_fixture("rh_best_bid_ask.json")["results"][0] - assert set(row) == {"symbol", "bid", "ask"} - assert row["bid"] < row["ask"], "a bid at or above the ask would invert every spread check" + assert set(row) == {"symbol", "timestamp", "bid", "ask"} + # `Decimal`, not a string comparison. These arrive quoted from this endpoint, and `"9" > "10"` + # lexically -- an ordering bug that only shows up once the price crosses a digit boundary. + assert Decimal(row["bid"]) < Decimal(row["ask"]), ( + "a bid at or above the ask would invert every spread check" + ) diff --git a/tests/broker_robinhood/test_transport.py b/tests/broker_robinhood/test_transport.py index aaacde67..459b98df 100644 --- a/tests/broker_robinhood/test_transport.py +++ b/tests/broker_robinhood/test_transport.py @@ -520,23 +520,38 @@ def test_trading_pairs_and_best_bid_ask_surface_their_documented_fields(http: An uses. Round-tripping through `float` on the way in would defeat the exactness being asserted: `asset_increment` is `0.00000001` for BTC, the same value that makes `translate._render` necessary, and one satoshi is a real order size on this venue rather than a rounding artefact. + + ⚠️ **Both of these endpoints QUOTE their money values, and `estimated_price` does not** + (#217 F6). That is why each value is put through `Decimal(...)` here rather than compared + directly: this venue is not internally consistent about quoting, so `str` is what these two + reads produce today and nothing about the API guarantees it stays that way. The pairing of + `parse_float=Decimal` in the transport with `Decimal(str(v))` in the adapter is what makes + both forms land on the same number, and it is the only read in this package that is safe + against a venue that mixes them. """ http(_FakeResponse(text=_fixture_text("rh_trading_pairs.json"))) pair = _results(_transport().get_trading_pairs(symbol="BTC-USD"))[0] assert pair["symbol"] == "BTC-USD" - assert pair["asset_increment"] == Decimal("0.00000001") - assert isinstance(pair["asset_increment"], Decimal) - assert pair["max_order_size"] > 0 + assert isinstance(pair["asset_increment"], str), "this endpoint quotes its numbers -- #217 F6" + assert Decimal(pair["asset_increment"]) == Decimal("0.00000001") + assert Decimal(pair["max_order_size"]) > 0 assert "min_order_amount" not in pair assert "min_order_size" not in pair http(_FakeResponse(text=_fixture_text("rh_best_bid_ask.json"))) quote_row = _results(_transport().get_best_bid_ask(symbol="BTC-USD"))[0] assert quote_row["symbol"] == "BTC-USD" - assert quote_row["bid"] < quote_row["ask"], ( + assert quote_row["timestamp"], "the venue timestamps every quote row -- #217 F8" + # `Decimal`, never a lexical comparison: these arrive quoted, and `"9" > "10"` as strings. + assert Decimal(quote_row["bid"]) < Decimal(quote_row["ask"]), ( "a bid at or above the ask would invert every spread-based check" ) + # The contrast, in one place: the same transport, the same decoder, a different endpoint. + http(_FakeResponse(text=_fixture_text("rh_estimated_price.json"))) + priced = _results(_transport().get_estimated_price("BTC-USD", "ask", "0.001"))[0] + assert isinstance(priced["ask"], Decimal), "this endpoint does NOT quote its numbers" + # --------------------------------------------------------------------------------------------- # `_paginate`: following `next` cursors across pages. diff --git a/tests/fixtures/rh_accounts.json b/tests/fixtures/rh_accounts.json index 5e307af7..b3ebf5a5 100644 --- a/tests/fixtures/rh_accounts.json +++ b/tests/fixtures/rh_accounts.json @@ -5,14 +5,14 @@ { "account_number": "AB1234567890", "status": "active", - "buying_power": 1042.55, + "buying_power": "1042.55", "buying_power_currency": "USD", "account_type": "individual", "is_api_tradable": true, "fee_tier_status": { - "fee_ratio": 0.0025, + "fee_ratio": 0.0095, "thirty_day_volume": 12480.55, - "next_fee_tier_ratio": 0.0020, + "next_fee_tier_ratio": 0.0085, "next_fee_tier_threshold": 50000.00 } } diff --git a/tests/fixtures/rh_best_bid_ask.json b/tests/fixtures/rh_best_bid_ask.json index c4098d1a..a9312e13 100644 --- a/tests/fixtures/rh_best_bid_ask.json +++ b/tests/fixtures/rh_best_bid_ask.json @@ -4,8 +4,9 @@ "results": [ { "symbol": "BTC-USD", - "bid": 65380.00, - "ask": 65480.00 + "timestamp": "2026-08-09T22:42:19.194164275Z", + "bid": "65380.00", + "ask": "65480.00" } ] } diff --git a/tests/fixtures/rh_estimated_price.json b/tests/fixtures/rh_estimated_price.json index f916c59c..65f91fba 100644 --- a/tests/fixtures/rh_estimated_price.json +++ b/tests/fixtures/rh_estimated_price.json @@ -5,12 +5,12 @@ { "symbol": "BTC-USD", "side": "ask", - "quantity": 0.1, - "timestamp": "2026-08-09T12:00:00Z", - "fee_ratio": 0.0025, - "est_fee": 16.370575, - "ask": 65482.30, - "est_total_cost": 6564.600575 + "quantity": 0.001, + "timestamp": "2026-08-09T22:42:19.194164275Z", + "fee_ratio": 0.0095, + "est_fee": 0.61726991, + "ask": 64975.78, + "est_total_cost": 65.59304991 } ] } diff --git a/tests/fixtures/rh_estimated_price_bid.json b/tests/fixtures/rh_estimated_price_bid.json new file mode 100644 index 00000000..90088e05 --- /dev/null +++ b/tests/fixtures/rh_estimated_price_bid.json @@ -0,0 +1,15 @@ +{ + "next": null, + "previous": null, + "results": [ + { + "symbol": "BTC-USD", + "side": "bid", + "quantity": 0.001, + "timestamp": "2026-08-09T22:42:19.194164275Z", + "fee_ratio": 0.0095, + "est_fee": 0.61728758, + "bid": 64977.64 + } + ] +} diff --git a/tests/fixtures/rh_trading_pairs.json b/tests/fixtures/rh_trading_pairs.json index 8886e2b0..f780ecb3 100644 --- a/tests/fixtures/rh_trading_pairs.json +++ b/tests/fixtures/rh_trading_pairs.json @@ -6,9 +6,9 @@ "symbol": "BTC-USD", "asset_code": "BTC", "quote_code": "USD", - "asset_increment": 0.00000001, - "quote_increment": 0.01, - "max_order_size": 10.00000000, + "asset_increment": "0.00000001", + "quote_increment": "0.01", + "max_order_size": "10.00000000", "status": "tradable", "is_api_tradable": true } diff --git a/tests/scripts/test_robinhood_smoke.py b/tests/scripts/test_robinhood_smoke.py index 1d68ca18..1c6fe26a 100644 --- a/tests/scripts/test_robinhood_smoke.py +++ b/tests/scripts/test_robinhood_smoke.py @@ -231,17 +231,33 @@ def test_a_probe_response_matches_its_fixture_after_pagination( def test_a_fixture_number_is_decoded_the_way_the_live_transport_decodes_it() -> None: - """Fixture money is unquoted (#217 F2) and must not be shape-reported as a `float`. + """An unquoted fixture number (#217 F2) must not be shape-reported as a `float`. - `RobinhoodTransport._request` parses with `parse_float=Decimal`, so a live `65482.30` reaches - `shape_of` as a `Decimal`. A fixture decoded with a plain `json.loads` reaches it as a + `RobinhoodTransport._request` parses with `parse_float=Decimal`, so a live unquoted `64975.78` + reaches `shape_of` as a `Decimal`. A fixture decoded with a plain `json.loads` reaches it as a `float`, and the script would then report `TYPE DIFFERS ... fixture='float' venue='Decimal'` - on every money field of every probe -- the same cry-wolf failure as F5, in a different place. + on every unquoted money field of every probe -- the same cry-wolf failure as F5, in a + different place. """ shape = fixture_shape(_FIXTURES / "rh_estimated_price.json") assert shape["results"][0]["ask"] == "Decimal" +def test_a_quoted_fixture_value_is_still_reported_as_a_string() -> None: + """The other half of #217 F6: this venue quotes SOME money and not others. + + `parse_float=Decimal` must not be mistaken for "everything becomes a `Decimal`". It converts + JSON numbers only, so a quoted `"0.00000001"` stays a `str` -- which is what `trading_pairs` + and `best_bid_ask` actually send. If this ever reported `Decimal`, the fixtures would have + been normalized to a uniformity the venue does not have, and the probe would report + `TYPE DIFFERS` against a live run. + """ + assert fixture_shape(_FIXTURES / "rh_trading_pairs.json")["results"][0]["asset_increment"] == ( + "str" + ) + assert fixture_shape(_FIXTURES / "rh_best_bid_ask.json")["results"][0]["bid"] == "str" + + def test_the_pagination_envelope_is_stripped_only_from_a_paginated_shape() -> None: """`next`/`previous` are dropped because `_paginate` consumes them -- not because the keys are unwelcome. A payload with no `results` (an order object) is left exactly as it is, so a real