diff --git a/packages/keel-broker-robinhood/README.md b/packages/keel-broker-robinhood/README.md index d627e693..d2cc294d 100644 --- a/packages/keel-broker-robinhood/README.md +++ b/packages/keel-broker-robinhood/README.md @@ -20,6 +20,45 @@ 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**. 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 +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 +81,37 @@ 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 +`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 +120,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..7159eb79 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,48 @@ 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. 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 @@ -337,25 +470,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 +726,112 @@ 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 `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 + 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`, 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`` + ============================ ===================================== ====================== + + 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 + 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`. + """ + # 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", () + + 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..b1e57dbb 100644 --- a/packages/keel-broker-robinhood/keel_broker_robinhood/transport.py +++ b/packages/keel-broker-robinhood/keel_broker_robinhood/transport.py @@ -308,12 +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 -- can 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. + # 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. + # + # ⚠️ **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 @@ -391,9 +409,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 +437,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..0f96ad1c 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 @@ -61,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"), @@ -79,6 +94,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 +166,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 +293,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..c57e9cc4 100644 --- a/tests/broker_robinhood/test_adapter.py +++ b/tests/broker_robinhood/test_adapter.py @@ -26,11 +26,38 @@ 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. + 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) + 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. @@ -271,18 +298,284 @@ 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=_QUOTED_SIZE) 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 + 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" +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=_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 == _QUOTED_SIZE * 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_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=_QUOTED_SIZE) + priced = RobinhoodAdapter( + FakeTransport(accounts=accounts, estimated_price=bid_only) + ).preview_order(sell) + assert priced.errors == () + assert priced.est_quote_size == _QUOTED_SIZE * 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_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. + + 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=_QUOTED_SIZE) + + 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`). + + 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"]) + 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=_QUOTED_SIZE) + + 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=_QUOTED_SIZE) + + 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=_QUOTED_SIZE) + + 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=_QUOTED_SIZE) + + preview = RobinhoodAdapter(transport).preview_order(spec) + + assert preview.errors == () + assert preview.est_quote_size == _QUOTED_SIZE * 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"] = _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=_QUOTED_SIZE) + + preview = RobinhoodAdapter(transport).preview_order(spec) + + 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) + + +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=_QUOTED_SIZE) + + 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 @@ -389,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 @@ -669,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.SELL, base_size=Decimal("0.1")) + spec = MarketIOCByBase(product_id="BTC-USD", side=Side.BUY, base_size=_QUOTED_SIZE) assert adapter.preview_order(spec).errors == () @@ -749,3 +1072,144 @@ 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. +# --------------------------------------------------------------------------------------------- + +#: 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_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"), +} + + +@pytest.mark.parametrize(("fixture_name", "fields"), sorted(_NUMERIC_FIELDS.items())) +def test_the_venues_unquoted_money_fields_decode_as_decimal( + fixture_name: str, fields: tuple[str, ...] +) -> None: + """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 + 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)}" + + +@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 every one of its values is unquoted.""" + 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. + + `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", "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 0baf8455..459b98df 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,54 @@ 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. + + ⚠️ **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(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 isinstance(pair["asset_increment"], str), "this endpoint quotes its numbers -- #217 F6" assert Decimal(pair["asset_increment"]) == Decimal("0.00000001") - assert Decimal(pair["min_order_amount"]) > 0 assert Decimal(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["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" # --------------------------------------------------------------------------------------------- diff --git a/tests/fixtures/rh_accounts.json b/tests/fixtures/rh_accounts.json index 67b3337b..b3ebf5a5 100644 --- a/tests/fixtures/rh_accounts.json +++ b/tests/fixtures/rh_accounts.json @@ -10,10 +10,10 @@ "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.0095, + "thirty_day_volume": 12480.55, + "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 f5013885..a9312e13 100644 --- a/tests/fixtures/rh_best_bid_ask.json +++ b/tests/fixtures/rh_best_bid_ask.json @@ -4,12 +4,9 @@ "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" + "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 5d5c2f31..65f91fba 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.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_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..f780ecb3 100644 --- a/tests/fixtures/rh_trading_pairs.json +++ b/tests/fixtures/rh_trading_pairs.json @@ -9,7 +9,6 @@ "asset_increment": "0.00000001", "quote_increment": "0.01", "max_order_size": "10.00000000", - "min_order_amount": "1.00", "status": "tradable", "is_api_tradable": true } diff --git a/tests/scripts/test_robinhood_smoke.py b/tests/scripts/test_robinhood_smoke.py index 7e265322..1c6fe26a 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,70 @@ 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: + """An unquoted fixture number (#217 F2) must not be shape-reported as a `float`. + + `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 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 + 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: