From 368cb0af7c4c4ca6870bf3c848c81c304c17dfb7 Mon Sep 17 00:00:00 2001 From: Elmehdi Aitbrahim Date: Sun, 9 Aug 2026 08:11:22 -0400 Subject: [PATCH] fix(brokers): close the review findings on the robinhood adapter Follow-ups to #192, which merged before these review fixes landed. BLOCKERS * `str(Decimal)` emitted scientific notation into the order body. `str(Decimal("0.00000001"))` is `"1E-8"`, and BTC's `asset_increment` is exactly `0.00000001` -- so one satoshi is the smallest order this venue accepts and the size a dust-sized exit produces. Every money and size field now renders through `translate._render` (`format(d, "f")`). A rejected exit leaves a position open while the engine records it closed; a rejected stop-limit leaves a position unprotected while local state says otherwise. The test that missed this asserted the property over two values that cannot trigger the exponent form; it is now parametrized over values that do. * `place_order` returned `success=True` for orders the venue rejected. Robinhood answers a rejected order on the happy HTTP path -- 200, with `"state": "failed"` -- so an `id` alone is not evidence the order is live. `failed`/`canceled` now return `success=False`. An unrecognised state still reports success, deliberately: reporting failure for a live order invites a duplicate placement, which has no recovery. * `get_order` omitted the `account_number` query param `create_order` sends. A 404 here becomes `None`, which the adapter turns into a terminal FAILED with zeroed money for a live resting order -- corrupting reconciliation rather than failing loudly. `cancel_order` deliberately still sends none: Robinhood documents that endpoint with a path param only. * The `estimated_price` namespace was challenged in review and is CORRECT as written. Verified against https://docs.robinhood.com/crypto/trading/: v2 really does split these two reads, `get/api/v2/crypto/trading/estimated_price/` beside `get/api/v2/crypto/marketdata/best_bid_ask/`. The asymmetry is real (v1 is the consistent one), so it is now pinned by a test and anchored to the doc in a comment. SHOULD-FIX * `Preview.errors` is populated on every path that could not price an order. A pricing failure previously rendered at the confirm gate as an order that costs nothing. * `preview_order` validates the symbol on every path, so it can no longer approve a symbol `place_order` will refuse with `UnsupportedOrder` after the human has already said yes. * The transport parses JSON with `parse_float=Decimal`; unquoted numeric money became `float` before any `Decimal` saw it. Every fixture quotes its numbers, which is why this was untested. * Query strings are percent-encoded, with the signed and sent bytes still identical. * `cancel_order` fails safe to `False` instead of raising -- a raise on the exit path can trap a position, which is this codebase's own stated principle. * `RobinhoodTransport` gains real coverage against a fake HTTP layer: signature/wire byte-identity, the 404-vs-raise split, pagination and its `_MAX_PAGES` bound, `_account` caching, and the literal endpoint path of every method. Also: per-call account caching (one `GET /accounts/` per public method), `_paginate` cursor hardening (non-string cursors, off-host URLs), and a README "must fix before wiring" section covering the always-passing `fees_usd` lapse check, the un-deduplicated `client_order_id`, and `Preview.synthetic` having nowhere to render at today's CLI confirm gate. Co-Authored-By: Claude Opus 5 (1M context) --- packages/keel-broker-robinhood/README.md | 43 ++ .../keel_broker_robinhood/adapter.py | 182 ++++-- .../keel_broker_robinhood/translate.py | 52 +- .../keel_broker_robinhood/transport.py | 132 ++++- tests/broker_robinhood/test_adapter.py | 230 ++++++++ tests/broker_robinhood/test_translate.py | 135 ++++- tests/broker_robinhood/test_transport.py | 550 +++++++++++++++++- 7 files changed, 1262 insertions(+), 62 deletions(-) diff --git a/packages/keel-broker-robinhood/README.md b/packages/keel-broker-robinhood/README.md index f1389482..d627e693 100644 --- a/packages/keel-broker-robinhood/README.md +++ b/packages/keel-broker-robinhood/README.md @@ -67,6 +67,49 @@ Installing this package registers `robinhood` as a discoverable broker plugin (s currently constructs or calls a `RobinhoodAdapter`. This is deliberate: the adapter is built and tested ahead of the migration that will use it, not wired in early. +### Must fix BEFORE wiring this to the live path + +The gaps above are capability limits -- things this venue cannot do, which the adapter refuses +honestly. The list below is different: these are places where wiring this adapter up **as it +stands** would degrade a safety property keel already has. Phase B must trip over this section. + +1. **`fees_usd` is always zero, so subscription-lapse detection is inert AND always-passing + against this venue.** This is not merely "a missing number". `FeeSummary.fees_usd` is the + field lapse detection reads to notice a fee charged while the user claims a fee-free + allowance. A constant zero can never contradict the claim, so the check does not fail + loudly -- it *passes*, every time, for every account. Anything consuming a Robinhood + `FeeSummary` must treat `fees_usd` as "not reported", never as "no fees were charged", and + the migration must decide whether an always-passing check is acceptable or whether the venue + should be excluded from that check by name. Closing it properly means paging order history + and summing per-order `fee_charged`, which needs its own rate-limit design. + +2. **A fresh `client_order_id` per `place_order` call means no retry is ever deduplicated.** + The uuid is minted per ATTEMPT, so a caller that retries after a timeout -- exactly when the + first request may already have reached the venue -- places a **second live order**. Robinhood + has nothing to match the retry against, because the id differs. The current behaviour is the + right default for the opposite hazard (an id minted per spec would silently collapse two + orders a strategy genuinely meant to place twice), and the port has no "retry of" concept to + disambiguate the two. So this is a documented tradeoff, not a solved problem: whatever calls + `place_order` in Phase B must not retry placement blindly. + +3. **`Preview.synthetic` is invisible at the confirm gate on today's CLI path.** + `keel/cli.py`'s `_interactive_confirm` takes a raw `dict` and renders it by iterating + `.items()` -- it has no `Preview` field to read and nowhere to display `synthetic`. Every + preview this adapter produces is `synthetic=True` (there is no native preview endpoint here), + and `Preview`'s own docstring requires that "approving an estimate must never look identical + to approving a broker's own quote". Until that CLI path is migrated to the port's `Preview` + type, approving a Robinhood estimate looks exactly like approving a Coinbase quote. The same + applies to `Preview.errors`, which this adapter populates whenever it could not price an + order -- an unpriced preview currently renders as a normal one. + +4. **No rate limiting or backoff.** Robinhood allows 100 requests/minute sustained (300 burst) + and this transport does not throttle or retry. Per-call account caching keeps each public + adapter method to a single `GET /accounts/`, but nothing bounds the engine's aggregate rate. + +5. **No candle source is composed.** Point 1 of "What does NOT work" means this adapter cannot + be a venue's sole broker; Phase B has to decide how an engine pairs an execution venue that + serves no bars with a separate market-data source. + ## Credentials Robinhood authenticates with an Ed25519 keypair, not an API secret. Robinhood's API credential diff --git a/packages/keel-broker-robinhood/keel_broker_robinhood/adapter.py b/packages/keel-broker-robinhood/keel_broker_robinhood/adapter.py index fee8b408..844a7805 100644 --- a/packages/keel-broker-robinhood/keel_broker_robinhood/adapter.py +++ b/packages/keel-broker-robinhood/keel_broker_robinhood/adapter.py @@ -54,6 +54,7 @@ from keel_core.types import Candle, Granularity from keel_broker_robinhood.translate import ( + _render, to_order_body, to_port_status, to_price_side, @@ -191,15 +192,25 @@ def _account(self) -> object: accounts = _results(self._require_transport().get_accounts()) return accounts[0] if accounts else {} - def _fee_ratio(self) -> Decimal | None: + def _fee_ratio(self, account: object) -> Decimal | None: """The account's fee ratio, or `None` when the venue did not report one. `None` is distinct from `Decimal("0")` on purpose and the two must not be collapsed: zero is a claim that this account trades free, and nothing in the payload supports that claim when the field is simply absent. Callers turn `None` into a zero fee ESTIMATE only - where they also label the estimate's basis as unknown. + where they also label the estimate's basis as unknown AND say so in `Preview.errors`. + + `account` is a PARAMETER rather than a `self._account()` call inside this method, which + is what keeps a single public call to one `GET /accounts/` round trip. It previously + fetched its own, so `get_fee_summary` -- which also needs the account for + `thirty_day_volume` -- spent two requests to answer one question, and `preview_order` + spent one more on top of its `estimated_price` call. Robinhood allows 100 requests/minute + sustained and this transport does not throttle, so duplicated reads are not free. The + account is deliberately NOT memoized on the instance: `get_balances` reads `buying_power` + off the same payload, and a stale buying power would misreport available capital to + anything sizing an order from it. Per call, not per adapter. """ - tier = _field(self._account(), "fee_tier_status") or {} + tier = _field(account, "fee_tier_status") or {} raw = _field(tier, "fee_ratio") if raw is None: return None @@ -245,17 +256,46 @@ def preview_order(self, spec: OrderSpec) -> Preview: 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. + + **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. + + **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. """ self._reject_unsupported(spec) + to_symbol(spec.product_id) base_size = self._base_size(spec) + errors: list[str] = [] if isinstance(spec, LimitGTC | StopLimitGTC): price, basis = spec.limit_price, "limit_price" else: - price, basis = self._estimated_price(spec), "estimated_price" + basis = "estimated_price" + estimated = self._estimated_price(spec) + if estimated is None: + price = Decimal("0") + 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() + 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" + ) return Preview( product_id=spec.product_id, side=spec.side, @@ -268,6 +308,7 @@ def preview_order(self, spec: OrderSpec) -> Preview: "price": str(price), "fee_ratio": str(ratio) if ratio is not None else "unknown", }, + errors=tuple(errors), ) def _base_size(self, spec: OrderSpec) -> Decimal: @@ -282,44 +323,67 @@ 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: - """Robinhood's estimated price for this size, or `Decimal("0")` if it reports none. + 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. - A zero on a missing price is a visible nonsense that surfaces as a zero-cost preview, - rather than an exception thrown mid-confirm; `errors` on `Preview` is the port's channel - for a soft failure and the price basis in `detail` says where the number came from. + **`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 + tell them apart -- so a pricing FAILURE rendered at the confirm gate as an order that + 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. + + `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. """ response = self._require_transport().get_estimated_price( symbol=to_symbol(spec.product_id), side=to_price_side(spec.side), - quantity=str(self._base_size(spec)), + quantity=_render(self._base_size(spec)), ) rows = _results(response) if not rows: - return Decimal("0") + return None try: - return Decimal(str(_field(rows[0], "price", "0") or "0")) + price = Decimal(str(_field(rows[0], "price", "0") or "0")) except (InvalidOperation, ValueError): - return Decimal("0") + return None + return price if price > 0 else None def place_order(self, spec: OrderSpec) -> PlaceResult: - """Place a live order. A fresh `client_order_id` per call gives Robinhood idempotency. - - The uuid is required by the API, not optional as it is on some venues, and it must be - fresh per ATTEMPT rather than per spec: reusing one across a retry is how a caller asks - the venue to deduplicate, and generating one per spec would silently deduplicate two - orders a strategy genuinely meant to place twice. - - Robinhood signals failure with an HTTP error rather than the success/error envelope - Coinbase returns, so a placement that comes back at all came back as an order. The one - thing still worth checking is that it carries an `id`: a `PlaceResult(success=True, - broker_order_id=None)` would be an order nobody can later reconcile or cancel, which is - worse than a reported failure. + """Place a live order. A fresh `client_order_id` per call; the returned `state` is read. + + ⚠️ **A fresh uuid per call means NO placement retry is ever deduplicated.** The id is + required by this API, and minting one per ATTEMPT is what stops two orders a strategy + genuinely meant to place twice from collapsing into one. The cost of that choice is the + opposite failure: a caller that retries after a timeout -- where the first request may + well have reached the venue -- places a SECOND live order, because the retry carries a + different id and Robinhood has nothing to match it against. Neither default is safe in + both directions and the port has no "retry of" concept to disambiguate them, so the + behaviour is documented rather than silently chosen (README, "must fix before wiring"). + + **Two things are checked, not one.** An `id` must come back -- a `success=True` with no + id is an order nobody can later reconcile or cancel. But the `id` alone is not evidence + the order is live, and treating it as such was the bug this docstring used to justify: + Robinhood does NOT signal every rejection with an HTTP error. It answers a rejected order + on the happy path, 200, with a real order object whose `state` reads `failed`. So a + protective `StopLimitGTC` could return `{"id": ..., "state": "failed"}` and be recorded as + a resting stop that does not exist at the venue -- the position it protects running naked, + with nothing to contradict the belief until the stop fails to fire. + + Only the states Robinhood documents as not-live (`_REJECTED_PLACEMENT_STATES`) are + rejections. An UNRECOGNISED state resolves to success, which is the opposite of what + `get_order` does with one, and the asymmetry is deliberate: reporting failure for an order + that is actually live invites the caller to place it again, and a duplicate live order has + no recovery, whereas reporting success hands back the id and lets reconciliation poll -- + where `to_port_status` maps the same unknown state to `PENDING` and keeps it observed. """ self._reject_unsupported(spec) body = to_order_body(spec, client_order_id=str(uuid.uuid4())) @@ -332,6 +396,20 @@ def place_order(self, spec: OrderSpec) -> PlaceResult: broker_order_id=None, reason="robinhood accepted the request but returned no order id", ) + state = str(_field(response, "state", "") or "") + if state in _REJECTED_PLACEMENT_STATES: + # `broker_order_id` is None here, matching `CoinbaseAdapter.place_order`'s failure + # path: a caller that reads a non-None id as "there is a live order to manage" must + # not be handed one for an order that is not live. The id rides in `reason` so it + # survives for debugging without being mistaken for a handle on a resting order. + return PlaceResult( + success=False, + broker_order_id=None, + reason=( + f"robinhood returned order {order_id} in state {state!r}: the venue rejected " + f"this order, so it is not resting and must not be recorded as placed" + ), + ) return PlaceResult(success=True, broker_order_id=str(order_id)) def get_fee_summary(self) -> FeeSummary: @@ -359,9 +437,11 @@ def get_fee_summary(self) -> FeeSummary: "no fees were charged". Closing this properly means paging order history and summing `fee_charged`, which is a follow-up with its own rate-limit design. """ + # One `GET /accounts/` for this whole method: the account is resolved once and passed to + # `_fee_ratio`, which used to fetch its own and made this two round trips for one answer. account = self._account() tier = _field(account, "fee_tier_status") or {} - ratio = self._fee_ratio() or Decimal("0") + ratio = self._fee_ratio(account) or Decimal("0") return FeeSummary( venue=_VENUE, taker_rate=ratio, @@ -424,21 +504,57 @@ def cancel_order(self, order_id: str) -> bool: An id the venue never issued returns `False` rather than raising, per the port docstring: absence of a refusal is not a confirmation, and neither is a 404. + + **A venue error also returns `False` rather than propagating.** The transport raises for + every failure that is not a 404 -- a 5xx, a timeout, a dropped connection -- and this + method runs on the EXIT path, where `executor._cancel_at_exchange` calls it while + unwinding a position. This adapter already writes the rule down at `_account`: "a raise on + the way out of a position can trap it." An exception escaping here can abort an unwind + partway through and leave both the position and the resting orders it was clearing live, + which is strictly worse than a `False`. + + Nothing is claimed by swallowing it. The port's contract is already "`True` ONLY when the + venue CONFIRMS", and an exception is definitionally not a confirmation -- so `False` is + the same answer this method would give for any other unconfirmed cancel, and it keeps the + caller believing the order may still be resting, which is the belief that keeps it + watching. The failure is not hidden either: the order stays in local state as + possibly-live and the next reconciliation poll re-reads it from the venue. """ transport = self._require_transport() - response = transport.cancel_order(order_id) - if response is None: + try: + response = transport.cancel_order(order_id) + if response is None: + return False + if _confirms_cancel(response, order_id): + return True + + polled = transport.get_order(order_id) + # Intentionally broad. The transport raises whatever the HTTP stack raises -- a + # `requests` exception, a socket timeout, a JSON decode error -- and narrowing this to a + # guessed list would let the one unguessed type escape onto the exit path, which is the + # exact failure this catch exists to prevent. See the docstring for why `False` claims + # nothing the port does not already treat as "unconfirmed". + except Exception: return False - if _confirms_cancel(response, order_id): - return True - - polled = transport.get_order(order_id) if polled is None: return False return _confirms_cancel(polled, order_id) +#: Robinhood order states that mean a just-placed order is NOT resting at the venue. +#: +#: Robinhood answers a rejected order on the HAPPY HTTP path -- 200, with a real order object +#: whose `state` says it never became live -- so `place_order` cannot infer success from the +#: absence of an HTTP error. Both spellings here come from the v2 docs' order-state enum +#: (https://docs.robinhood.com/crypto/trading/, "Get Orders" `state` filter: `open`, `canceled`, +#: `filled`, `failed`, `pending`), and Robinhood's `canceled` is the American single-`l` spelling +#: -- the port's `CANCELLED` must never be compared against raw venue JSON. +#: +#: Deliberately NOT a denylist of everything unfamiliar: an unrecognised state resolves to +#: success. See `place_order` for why that asymmetry with `get_order` is the safe direction. +_REJECTED_PLACEMENT_STATES: frozenset[str] = frozenset({"failed", "canceled"}) + #: Robinhood's own spelling of the terminal cancelled state. Compared against raw venue JSON, #: so it is the venue's single-`l` spelling and NOT the port's `"CANCELLED"` -- the two meet #: only in `translate.STATE_TO_PORT_STATUS`, and reading the port's spelling here would silently diff --git a/packages/keel-broker-robinhood/keel_broker_robinhood/translate.py b/packages/keel-broker-robinhood/keel_broker_robinhood/translate.py index bccb9414..dcb0392b 100644 --- a/packages/keel-broker-robinhood/keel_broker_robinhood/translate.py +++ b/packages/keel-broker-robinhood/keel_broker_robinhood/translate.py @@ -12,12 +12,14 @@ port spells it `CANCELLED`. `STATE_TO_PORT_STATUS` is the one place those two spellings meet, so nobody downstream has to remember which venue uses which. -Decimals render via `str()` everywhere in this module so an order's size, price, or stop can -never be perturbed by a float round-trip on the live-money path. +Money and size values render through `_render`, never `str()` and never `float`. Keeping them as +`Decimal` end to end is only half the requirement -- see `_render`'s docstring for why `str()` on +a `Decimal` is itself unsafe here, which is the half that is easy to miss. """ from __future__ import annotations +from decimal import Decimal from typing import Any, assert_never from keel_broker_api.orders import ( @@ -56,6 +58,35 @@ } +def _render(value: Decimal) -> str: + """Render a money or size `Decimal` positionally, for a JSON field that has no exponent form. + + **`str()` is wrong here, and it is wrong in the one case that matters most.** `str(Decimal)` + follows the decimal spec's `to-scientific-string`: once a value's adjusted exponent leaves a + narrow band it switches to scientific notation. `str(Decimal("0.00000001"))` is `"1E-8"`, not + `"0.00000001"`. Robinhood's `asset_quantity` / `limit_price` / `stop_price` are decimal + strings with no exponent form at all, so that renders a MALFORMED ORDER BODY. + + This is not a hypothetical magnitude. BTC's `asset_increment` is exactly `0.00000001` (see + `tests/fixtures/rh_trading_pairs.json`), so one satoshi is the smallest order this venue + accepts -- precisely the size a dust-sized exit produces. The failure it causes is the bad + kind: a rejected EXIT leaves a position open while the engine records it closed, and a + rejected STOP-LIMIT leaves a position with no protective stop at the venue while local state + says there is one. Neither surfaces as a wrong number a human might notice; both surface as a + venue rejection at the exact moment the order mattered. + + `format(value, "f")` is the fixed-point renderer: positional at every magnitude, no exponent + ever, and -- unlike `f"{value:.8f}"` or any quantize-based approach -- it neither truncates + nor rounds, so the string still carries the caller's exact value. It preserves trailing zeros + (`Decimal("64000.10")` stays `"64000.10"`), which is correct: those digits are the caller's + stated precision, and Robinhood parses the value numerically regardless. + + The float ban the rest of this module observes still applies and is a separate concern: + `float` loses precision, while `str()` keeps the precision and mangles the SHAPE. + """ + return format(value, "f") + + def to_symbol(product_id: str) -> str: """Render a keel product id as Robinhood's `symbol`, refusing anything not settled in USD. @@ -101,8 +132,9 @@ def to_price_side(side: Side) -> str: def to_order_body(spec: OrderSpec, *, client_order_id: str) -> dict[str, Any]: """Render `spec` as the JSON body for `POST /api/v2/crypto/trading/orders/`. - Decimals render via `str()`, never float, so no order size, limit price, or stop price can be - perturbed by a float round-trip between here and the wire. + Every money and size field renders through `_render` (`format(d, "f")`), never `str()` and + never `float`. See `_render`'s docstring: `str(Decimal)` emits scientific notation at small + magnitudes, and `"1E-8"` in an `asset_quantity` is a malformed body, not a rounding nuisance. """ match spec: case MarketIOCByQuote(): @@ -124,7 +156,7 @@ def to_order_body(spec: OrderSpec, *, client_order_id: str) -> dict[str, Any]: "client_order_id": client_order_id, "side": to_side(spec.side), "type": "market", - "market_order_config": {"asset_quantity": str(spec.base_size)}, + "market_order_config": {"asset_quantity": _render(spec.base_size)}, } case LimitGTC(): return { @@ -133,8 +165,8 @@ def to_order_body(spec: OrderSpec, *, client_order_id: str) -> dict[str, Any]: "side": to_side(spec.side), "type": "limit", "limit_order_config": { - "asset_quantity": str(spec.base_size), - "limit_price": str(spec.limit_price), + "asset_quantity": _render(spec.base_size), + "limit_price": _render(spec.limit_price), "time_in_force": TIME_IN_FORCE, }, } @@ -145,9 +177,9 @@ def to_order_body(spec: OrderSpec, *, client_order_id: str) -> dict[str, Any]: "side": to_side(spec.side), "type": "stop_limit", "stop_limit_order_config": { - "asset_quantity": str(spec.base_size), - "limit_price": str(spec.limit_price), - "stop_price": str(spec.stop_price), + "asset_quantity": _render(spec.base_size), + "limit_price": _render(spec.limit_price), + "stop_price": _render(spec.stop_price), "time_in_force": TIME_IN_FORCE, }, } diff --git a/packages/keel-broker-robinhood/keel_broker_robinhood/transport.py b/packages/keel-broker-robinhood/keel_broker_robinhood/transport.py index 0d63c26f..264f9222 100644 --- a/packages/keel-broker-robinhood/keel_broker_robinhood/transport.py +++ b/packages/keel-broker-robinhood/keel_broker_robinhood/transport.py @@ -27,7 +27,9 @@ import base64 import json import time +from decimal import Decimal from typing import Any, Protocol +from urllib.parse import quote, urlencode class Transport(Protocol): @@ -265,7 +267,21 @@ def _request( if params: # Sorted for a deterministic string: dict insertion order is an implementation detail # that must not silently change what gets signed from one call to the next. - query = "?" + "&".join(f"{k}={v}" for k, v in sorted(params.items()) if v is not None) + # + # `quote_via=quote`, NOT the `urlencode` default of `quote_plus`: `quote_plus` encodes + # a space as `+`, and `+` is itself a character that must survive round-tripping here. + # A raw `+` on the wire decodes server-side as a SPACE, so a value like `1E+2` would + # be verified against the signature as `1E+2` and then parsed as `1E 2` -- the + # signature check passes and the venue acts on a different value than was signed. + # (`translate._render` now keeps exponent forms out of sizes and prices, so that + # specific pairing is closed at the source too; this is the second gate.) + # + # `safe=""` so nothing is left unencoded on the assumption it is harmless. + query = "?" + urlencode( + [(k, v) for k, v in sorted(params.items()) if v is not None], + quote_via=quote, + safe="", + ) full_path = f"{path}{query}" body_str = "" if body is None else json.dumps(body) @@ -291,7 +307,14 @@ def _request( response.raise_for_status() if not response.content: return None - json_response: Any = response.json() + # `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. + json_response: Any = json.loads(response.text, parse_float=Decimal) return json_response def _paginate(self, path: str, params: dict[str, Any] | None = None) -> dict[str, Any]: @@ -317,19 +340,41 @@ def _paginate(self, path: str, params: dict[str, Any] | None = None) -> dict[str ) page = self._request("GET", next_path, params=next_params) results.extend(_results(page)) - cursor = _field(page, "next") + next_path = self._next_path(_field(page, "next")) # After the first page, `next` is already a full absolute URL Robinhood hands back -- # not a path this transport built -- so params are folded into it already and must # not be re-appended on the next iteration. - if cursor: - if cursor.startswith(self._base_url): - cursor = cursor[len(self._base_url) :] - next_path = cursor - next_params = None - else: - next_path = None + next_params = None return {"results": results} + def _next_path(self, cursor: Any) -> str | None: + """Turn a `next` cursor into a same-host path, or `None` to stop paginating. + + `cursor` is typed `Any` because it comes straight out of a JSON payload, and this method + exists to stop that `Any` from becoming a crash or, worse, a request to somewhere else. + Two hazards, both of which are server-controlled input: + + 1. **A non-string `next`.** A `null` is already handled as "stop", but a number, a list, + or an object would previously reach `.startswith` and raise `AttributeError` out of a + read the adapter has no reason to expect can fail that way. Anything that is not a + string is treated as "no further pages": one truncated list is a far better outcome + than an exception surfacing from `get_holdings` mid-reconciliation. + 2. **An absolute URL on a DIFFERENT host.** The old code stripped `self._base_url` only + when the cursor started with it, and otherwise used the value as a path -- so a + `next` of `https://evil.example/x` would be concatenated onto `self._base_url` and + requested as `https://trading.robinhood.com/https://evil.example/x`. That is a + malformed request rather than a leak, but this transport signs every request with the + account's credentials, so the rule worth enforcing is simple and absolute: a cursor + either points at this venue or pagination stops. It is never followed off-host. + """ + if not isinstance(cursor, str) or not cursor: + return None + if cursor.startswith(self._base_url): + return cursor[len(self._base_url) :] + if cursor.startswith("/"): + return cursor + return None + def get_accounts(self) -> Any: return self._paginate("/api/v2/crypto/trading/accounts/") @@ -339,15 +384,53 @@ def get_holdings(self) -> Any: ) def get_trading_pairs(self, symbol: str | None = None) -> Any: + """The venue's per-pair trading rules. **Deliberately not called by the adapter yet.** + + This method and `get_best_bid_ask` are the only two on this transport that `adapter.py` + never invokes, which is a fair thing to challenge in review, so the reason is written + 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 + 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 + protect. Sizing validation must therefore be designed to degrade to "place it anyway" + rather than bolted on as a gate, and that design is a follow-up, not a nit fix. + + They are exercised by the transport tests (endpoint path, signature, response shape), so + they are not untested code -- only uncalled code, on purpose, with a named successor. + """ params = {"symbol": symbol} if symbol is not None else None return self._paginate("/api/v2/crypto/trading/trading_pairs/", params=params) 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. return self._paginate( "/api/v2/crypto/marketdata/best_bid_ask/", params={"symbol": symbol} ) def get_estimated_price(self, symbol: str, side: str, quantity: str) -> Any: + # ⚠️ `trading`, NOT `marketdata`. This looks like a copy-paste error next to + # `get_best_bid_ask` above and it is not -- Robinhood's v2 API really does split these + # two market-data reads across two namespaces. Verified against the primary source, + # https://docs.robinhood.com/crypto/trading/, which lists them verbatim as: + # + # get/api/v2/crypto/trading/estimated_price/ + # get/api/v2/crypto/marketdata/best_bid_ask/ + # + # The v1 API is the consistent one (`/api/v1/crypto/marketdata/estimated_price/`), which + # is very likely where the instinct to "fix" this path comes from. Do not change it to + # `marketdata` on symmetry grounds: a wrong path here is a 404, `_request` turns a 404 + # into `None`, and `_estimated_price` then reports an unpriced preview -- a silent + # degradation of the confirm gate rather than an error anyone would notice. + # + # Required query params, per the same page: `symbol`, `side` (`bid`/`ask`/`both`), and + # `quantity` -- all three marked required. + # # Not paginated in practice (one symbol, one side, one quantity produces at most a # handful of rows), but routed through `_paginate` anyway so its shape matches every # other read here and the adapter never has to special-case one endpoint. @@ -372,8 +455,23 @@ def get_order(self, order_id: str) -> Any: a terminal FAILED status one layer up. Reporting FAILED because of a network blip rather than because Robinhood actually said "no such order" would tell the engine a resting order is gone when it is still live on the venue. + + `account_number` rides on the query string, exactly as `create_order` sends it. + Robinhood's own v2 sample client (https://docs.robinhood.com/crypto/trading/, "Making + your first API call") builds this call as: + + params = {"account_number": account_number} + path = f"/api/v2/crypto/trading/orders/{order_id}/{query_params}" + + Omitting it risks a 404 -- and a 404 on THIS method is the quiet corruption described + above, not a loud failure: `_request` turns it into `None`, and `adapter.get_order` turns + that into a terminal FAILED with zeroed money for an order still resting at the venue. """ - return self._request("GET", f"/api/v2/crypto/trading/orders/{order_id}/") + return self._request( + "GET", + f"/api/v2/crypto/trading/orders/{order_id}/", + params={"account_number": self._account()}, + ) def cancel_order(self, order_id: str) -> Any: """Cancel one order; `None` only if Robinhood's 404 says this id does not exist. @@ -383,6 +481,18 @@ def cancel_order(self, order_id: str) -> Any: else -- including a successful response whose `state` is not `"canceled"` -- as evidence to re-poll, precisely because a resting order that failed to cancel is still live money and must never be reported as gone on the strength of a raised exception it didn't get. + + Unlike `create_order` and `get_order`, this endpoint sends NO `account_number`, and that + asymmetry is deliberate rather than an oversight. Robinhood's v2 reference documents this + endpoint with a path parameter `id` and no query-parameter section at all -- + `post/api/v2/crypto/trading/orders/{id}/cancel/`, per + https://docs.robinhood.com/crypto/trading/ -- + and their own v2 sample client builds it as a bare + `f"/api/v2/crypto/trading/orders/{order_id}/cancel/"` with no params -- while the same + sample DOES pass `account_number` for placing and fetching. Adding an undocumented param + here for the sake of looking consistent would be a guess, and every extra byte on the + query string is also signed, so a guess that the venue rejects is a 401 on the cancel + path. The order id alone identifies the order. """ return self._request("POST", f"/api/v2/crypto/trading/orders/{order_id}/cancel/") diff --git a/tests/broker_robinhood/test_adapter.py b/tests/broker_robinhood/test_adapter.py index 4ce88ccd..4373f4ae 100644 --- a/tests/broker_robinhood/test_adapter.py +++ b/tests/broker_robinhood/test_adapter.py @@ -151,6 +151,41 @@ def cancel_order(self, order_id: str) -> Any: return order +class _RaisingCancelTransport(FakeTransport): + """A transport whose `cancel_order` raises the way a 5xx or a dropped connection does. + + `RobinhoodTransport._request` deliberately raises for every failure that is not a 404, so this + is the realistic shape of a venue outage during a cancel -- and `cancel_order` runs on the + EXIT path, where a raise can trap a position. + """ + + def cancel_order(self, order_id: str) -> Any: + self._record("cancel_order", order_id=order_id) + raise RuntimeError("503 Server Error: Service Unavailable") + + +class _RaisingRepollTransport(FakeTransport): + """Cancel answers ambiguously (still `open`), and the mandatory re-poll is what blows up.""" + + def cancel_order(self, order_id: str) -> Any: + self._record("cancel_order", order_id=order_id) + order = dict(self._order or {}) + order["id"] = order_id + order["state"] = "open" + return order + + def get_order(self, order_id: str) -> Any: + self._record("get_order", order_id=order_id) + raise RuntimeError("503 Server Error: Service Unavailable") + + +def _placed_with_state(state: str) -> dict[str, Any]: + """An order-placement response carrying `state`, otherwise shaped like a real one.""" + placed = load_fixture("rh_order_open.json") + placed["state"] = state + return placed + + def test_capabilities_declare_robinhood() -> None: """The engine gates live spend on these declarations, so each clause here is a promise the rest of the suite must keep: no native preview, a synthesized one instead, fee summaries @@ -509,6 +544,201 @@ def test_entry_point_discovery_finds_the_robinhood_adapter() -> None: assert load_broker("robinhood").__name__ == "RobinhoodAdapter" +@pytest.mark.parametrize("state", ["failed", "canceled"]) +def test_place_order_reports_failure_when_the_venue_rejected_the_order(state: str) -> None: + """An HTTP 200 carrying `"state": "failed"` is a REJECTION, not a placement. + + Robinhood answers a rejected order on the happy HTTP path -- 200, with a real order object + whose `state` says it never became live. Checking only that an `id` came back therefore + reports `success=True` for an order the venue refused. The concrete failure: a protective + `StopLimitGTC` answered `{"id": ..., "state": "failed"}` makes the engine record a stop that + does not exist at the venue, so the position it believes is protected is running naked, and + nothing will contradict that belief until the stop fails to fire. + + `canceled` is treated the same way. A placement that comes back already cancelled is equally + not a resting order, and recording it as one has the identical consequence. + + `broker_order_id` is `None` on this path, matching `CoinbaseAdapter.place_order`: a caller + that reads a non-`None` id as "there is a live order to manage" must not be handed one for an + order that is not live. The id is named in `reason` instead, so it survives for debugging. + """ + transport = FakeTransport(placed=_placed_with_state(state)) + adapter = RobinhoodAdapter(transport) + spec = StopLimitGTC( + product_id="BTC-USD", + side=Side.SELL, + base_size=Decimal("0.1"), + stop_price=Decimal("60000"), + limit_price=Decimal("59900"), + ) + + result = adapter.place_order(spec) + + assert result.success is False + assert result.broker_order_id is None + assert result.reason is not None + assert state in result.reason + + +@pytest.mark.parametrize("state", ["open", "filled", "pending", "partially_filled"]) +def test_place_order_reports_success_for_a_state_that_is_or_may_become_live(state: str) -> None: + """The mirror of the rejection test: a live or in-flight state must still be a success. + + `partially_filled` is in this list on purpose -- it appears in Robinhood's v1 state enum but + not v2's, and a partially filled order is unambiguously a real order that was placed. + """ + transport = FakeTransport(placed=_placed_with_state(state)) + adapter = RobinhoodAdapter(transport) + spec = MarketIOCByBase(product_id="BTC-USD", side=Side.SELL, base_size=Decimal("0.1")) + + result = adapter.place_order(spec) + + assert result.success is True + assert result.broker_order_id == load_fixture("rh_order_open.json")["id"] + + +def test_place_order_treats_an_unrecognised_state_as_placed_not_rejected() -> None: + """An unknown `state` means the adapter does not know the outcome -- and here, unlike + `get_order`, "I don't know" must resolve to SUCCESS rather than failure. + + The asymmetry is deliberate and follows the consequences. Reporting `success=False` for an + order that is actually live invites the caller to place it again, and a duplicate live order + is unrecoverable. Reporting `success=True` hands back the id, and reconciliation then polls + `get_order`, which maps the same unrecognised state to `PENDING` and keeps the order under + observation until the venue says something the adapter understands. Only the two states + Robinhood documents as not-live (`failed`, `canceled`) are treated as rejections. + """ + transport = FakeTransport(placed=_placed_with_state("a_future_state_this_adapter_predates")) + adapter = RobinhoodAdapter(transport) + spec = MarketIOCByBase(product_id="BTC-USD", side=Side.SELL, base_size=Decimal("0.1")) + + result = adapter.place_order(spec) + + assert result.success is True + assert result.broker_order_id is not None + + +def test_preview_order_reports_an_error_when_the_venue_returned_no_price() -> None: + """A preview that could not be priced must SAY so, not render as a free order. + + `_estimated_price` falls back to zero when the endpoint answers nothing, and a zero price + makes `est_quote_size` and `est_fee` both zero. At the human confirm gate that is + indistinguishable from an order that genuinely costs nothing -- the single most approvable + thing a preview can look like. `Preview.errors` is the port's channel for exactly this, and + leaving it empty is what makes the failure invisible. + """ + transport = FakeTransport( + accounts=load_fixture("rh_accounts.json"), estimated_price={"results": []} + ) + adapter = RobinhoodAdapter(transport) + spec = MarketIOCByBase(product_id="BTC-USD", side=Side.SELL, base_size=Decimal("0.1")) + + preview = adapter.preview_order(spec) + + assert preview.errors, "an unpriced preview must not come back with empty errors" + assert any("price" in e.lower() for e in preview.errors) + + +def test_preview_order_reports_an_error_when_the_account_reports_no_fee_ratio() -> None: + """`est_fee` of zero, from a missing `fee_ratio`, is a claim this account trades free. + + `_fee_ratio` already distinguishes `None` from `Decimal("0")` precisely so this claim is never + made by accident, and `detail["fee_ratio"]` says `"unknown"`. But `detail` is free-form text a + renderer may not show, while `errors` is the field the port defines for a soft failure -- so + the unpriced fee has to surface there too. + """ + accounts = load_fixture("rh_accounts.json") + del accounts["results"][0]["fee_tier_status"] + transport = FakeTransport(accounts=accounts) + adapter = RobinhoodAdapter(transport) + spec = LimitGTC( + product_id="BTC-USD", side=Side.SELL, base_size=Decimal("0.1"), limit_price=Decimal("65000") + ) + + preview = adapter.preview_order(spec) + + assert preview.errors + assert any("fee" in e.lower() for e in preview.errors) + assert preview.detail["fee_ratio"] == "unknown" + + +def test_preview_order_on_a_fully_priced_order_reports_no_errors() -> None: + """The control case: `errors` must stay empty when everything priced, or it means nothing.""" + 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.SELL, base_size=Decimal("0.1")) + + assert adapter.preview_order(spec).errors == () + + +@pytest.mark.parametrize("kind", ["limit", "stop_limit"]) +def test_preview_order_refuses_a_non_usd_symbol_on_every_path(kind: str) -> None: + """`preview_order` must not approve a symbol `place_order` will refuse. + + The resting-order paths price off `spec.limit_price` and never call `to_symbol`, so + `ETH-USDC` previews cleanly and then raises `UnsupportedOrder` at placement. That ordering is + the problem: the human has already approved at the confirm gate by then, and the port + explicitly forbids catching `UnsupportedOrder` and retrying with a different spec -- so the + approved order simply cannot be placed. Validating the symbol on every preview path moves the + refusal to before the human is asked. + """ + spec: LimitGTC | StopLimitGTC + if kind == "limit": + spec = LimitGTC( + product_id="ETH-USDC", + side=Side.SELL, + base_size=Decimal("1"), + limit_price=Decimal("3000"), + ) + else: + spec = StopLimitGTC( + product_id="ETH-USDC", + side=Side.SELL, + base_size=Decimal("1"), + stop_price=Decimal("2900"), + limit_price=Decimal("2890"), + ) + adapter = RobinhoodAdapter(FakeTransport(accounts=load_fixture("rh_accounts.json"))) + + with pytest.raises(UnsupportedOrder, match="USDC"): + adapter.preview_order(spec) + + +def test_cancel_order_returns_false_instead_of_raising_when_the_venue_errors() -> None: + """A 5xx during a cancel must fail safe to `False`, never propagate out of this method. + + This adapter already writes the rule down at `_account`: "a raise on the way out of a position + can trap it". `cancel_order` is the method most exposed to it -- `executor._cancel_at_exchange` + calls it while unwinding, and an exception there can abort the unwind partway through, leaving + the position AND the resting orders it was trying to clear both live. + + `False` is the honest answer regardless of what went wrong, because the port's contract is + already "`True` ONLY when the venue CONFIRMS" -- and an exception is definitionally not a + confirmation. Nothing is claimed here that was not observed; the caller keeps believing the + order may still be resting, which is the belief that keeps it watching. + """ + fixture = load_fixture("rh_order_open.json") + transport = _RaisingCancelTransport(order=fixture) + transport._issued_order_ids.add(fixture["id"]) + adapter = RobinhoodAdapter(transport) + + assert adapter.cancel_order(fixture["id"]) is False + + +def test_cancel_order_returns_false_when_the_mandatory_re_poll_raises() -> None: + """Same rule, one layer deeper: the re-poll is on the exit path too and cannot be allowed to + escape as an exception either.""" + fixture = load_fixture("rh_order_open.json") + transport = _RaisingRepollTransport(order=fixture) + transport._issued_order_ids.add(fixture["id"]) + adapter = RobinhoodAdapter(transport) + + assert adapter.cancel_order(fixture["id"]) is False + + def test_place_order_returns_a_domain_type() -> None: """No Robinhood-native order object may cross the port -- a caller must only ever see `PlaceResult`.""" diff --git a/tests/broker_robinhood/test_translate.py b/tests/broker_robinhood/test_translate.py index ab07b378..b1437ec6 100644 --- a/tests/broker_robinhood/test_translate.py +++ b/tests/broker_robinhood/test_translate.py @@ -105,19 +105,140 @@ def test_stop_limit_gtc_body() -> None: } -def test_decimals_are_rendered_as_exact_strings_not_floats() -> None: - """A float round-trip here would silently change an order's size, price, or stop on the - live-money path.""" +#: Decimal inputs paired with the exact positional text Robinhood's JSON order body must carry. +#: +#: `str(Decimal)` is NOT a positional renderer. It switches to scientific notation whenever the +#: value's adjusted exponent leaves a narrow band, so `str(Decimal("0.00000001"))` is `"1E-8"` -- +#: and Robinhood's order body has no exponent form. This is not a contrived edge case: BTC's +#: `asset_increment` is exactly `0.00000001` (see `tests/fixtures/rh_trading_pairs.json`), which +#: makes one satoshi the SMALLEST ORDER THIS VENUE ACCEPTS and therefore a size a dust-sized exit +#: genuinely produces. `format(d, "f")` renders positionally at every magnitude. +#: +#: The first three entries all render wrongly under `str()`; the last two already rendered +#: correctly and are kept so this parametrisation also proves the fix does not perturb the +#: ordinary case. +_EXPONENT_HAZARDS: list[tuple[str, str]] = [ + # One satoshi -- the venue's own minimum increment for BTC. `str()` gives "1E-8". + ("0.00000001", "0.00000001"), + # Sub-satoshi precision, to prove the fix is about the exponent form and not about a single + # magic value. `str()` gives "1.2345E-8". + ("0.000000012345", "0.000000012345"), + # A value carrying a POSITIVE exponent. `Decimal("1E+2")` is 100, and `str()` gives "1E+2". + # Nobody writes this literal, but arithmetic inside the engine -- a size divided by a price + # and re-multiplied, say -- produces exactly this shape without anyone intending it. + ("1E+2", "100"), + ("0.123456789", "0.123456789"), + ("64000.10", "64000.10"), +] + + +def _rendered_values(body: dict[str, object]) -> list[str]: + """Every string leaf inside `body`'s `*_order_config` block. + + Reaching into the config block rather than naming fields one at a time means a money or size + field added to a body later is covered by the exponent assertion below automatically, instead + of silently escaping it until a dust-sized order is rejected on the live-money path. + """ + config = next(v for k, v in body.items() if k.endswith("_order_config")) + assert isinstance(config, dict) + return [v for v in config.values() if isinstance(v, str)] + + +@pytest.mark.parametrize(("raw", "expected"), _EXPONENT_HAZARDS) +def test_market_order_size_renders_positionally_never_in_scientific_notation( + raw: str, expected: str +) -> None: + """A size rendered as `"1E-8"` is a malformed order body, not a rounding nuisance. + + Robinhood's `asset_quantity` is parsed as a decimal string with no exponent form. A dust-sized + exit -- one satoshi, which is exactly BTC's `asset_increment` -- would go out as `"1E-8"` and + be rejected, and a rejected EXIT is a position that stays open when the engine believes it + closed. `str()` cannot be trusted to render a `Decimal` positionally; `format(d, "f")` can. + """ + spec = MarketIOCByBase(product_id="BTC-USD", side=Side.SELL, base_size=Decimal(raw)) + body = to_order_body(spec, client_order_id="c1") + + assert body["market_order_config"] == {"asset_quantity": expected} + + +@pytest.mark.parametrize(("raw", "expected"), _EXPONENT_HAZARDS) +def test_limit_order_size_and_price_render_positionally(raw: str, expected: str) -> None: + """Both the size and the limit price ride through the same renderer, so both must be pinned. + + A limit PRICE in exponent form is the more dangerous of the two: rejected outright it is + merely a failed take-profit, but it is also the field a venue is most likely to parse + leniently and differently than intended.""" spec = LimitGTC( product_id="BTC-USD", side=Side.BUY, - base_size=Decimal("0.123456789"), - limit_price=Decimal("64000.10"), + base_size=Decimal(raw), + limit_price=Decimal(raw), + ) + body = to_order_body(spec, client_order_id="c1") + + assert body["limit_order_config"] == { + "asset_quantity": expected, + "limit_price": expected, + "time_in_force": "gtc", + } + + +@pytest.mark.parametrize(("raw", "expected"), _EXPONENT_HAZARDS) +def test_stop_limit_order_size_price_and_stop_render_positionally( + raw: str, expected: str +) -> None: + """The protective-stop path, which is the one that must never be malformed. + + A stop-limit rejected for a malformed `stop_price` leaves a position with NO protective stop + at the venue while the engine's local state records one. That is the worst shape this bug can + take, so the stop leg gets its own assertion rather than riding on the limit test.""" + spec = StopLimitGTC( + product_id="BTC-USD", + side=Side.SELL, + base_size=Decimal(raw), + stop_price=Decimal(raw), + limit_price=Decimal(raw), ) body = to_order_body(spec, client_order_id="c1") - assert body["limit_order_config"]["asset_quantity"] == "0.123456789" - assert body["limit_order_config"]["limit_price"] == "64000.10" + assert body["stop_limit_order_config"] == { + "asset_quantity": expected, + "limit_price": expected, + "stop_price": expected, + "time_in_force": "gtc", + } + + +@pytest.mark.parametrize(("raw", "_expected"), _EXPONENT_HAZARDS) +def test_no_rendered_order_field_ever_contains_an_exponent_marker( + raw: str, _expected: str +) -> None: + """The property itself, asserted structurally across all three body shapes. + + The tests above pin exact strings, which is what catches a regression precisely. This one + catches a money or size field added to a body LATER that quietly reintroduces `str()` -- it + reads every string leaf of the config block and refuses any `e`/`E`, which no legitimate + positional decimal rendering ever contains. + """ + value = Decimal(raw) + specs = [ + MarketIOCByBase(product_id="BTC-USD", side=Side.SELL, base_size=value), + LimitGTC( + product_id="BTC-USD", side=Side.BUY, base_size=value, limit_price=value + ), + StopLimitGTC( + product_id="BTC-USD", + side=Side.SELL, + base_size=value, + stop_price=value, + limit_price=value, + ), + ] + for spec in specs: + for rendered in _rendered_values(to_order_body(spec, client_order_id="c1")): + assert "e" not in rendered.lower(), ( + f"{spec.kind} rendered {rendered!r} with an exponent" + ) def test_market_ioc_by_quote_is_refused_with_the_real_reason() -> None: diff --git a/tests/broker_robinhood/test_transport.py b/tests/broker_robinhood/test_transport.py index 6697c52f..0baf8455 100644 --- a/tests/broker_robinhood/test_transport.py +++ b/tests/broker_robinhood/test_transport.py @@ -10,10 +10,31 @@ from __future__ import annotations import base64 +import json +from decimal import Decimal +from pathlib import Path from typing import Any +from urllib.parse import parse_qsl, urlsplit import nacl.signing -from keel_broker_robinhood.transport import _field, _results, build_headers, sign_payload +import pytest +from keel_broker_robinhood.transport import ( + _MAX_PAGES, + RobinhoodTransport, + _field, + _results, + build_headers, + sign_payload, +) + +_FIXTURES_DIR = Path(__file__).parent.parent / "fixtures" + + +def _load_fixture(name: str) -> dict[str, Any]: + with (_FIXTURES_DIR / name).open() as f: + data: dict[str, Any] = json.load(f) + return data + #: Throwaway Ed25519 test seed. Generated once for this file with #: `nacl.signing.SigningKey.generate()` and pasted here as a literal -- it has never been @@ -147,3 +168,530 @@ def test_results_tolerates_none() -> None: empty list rather than raising keeps every caller of `_results` from having to special-case an unset fixture or a genuinely empty response.""" assert _results(None) == [] + + +# --------------------------------------------------------------------------------------------- +# The network-backed transport, exercised against a fake HTTP layer. +# +# `RobinhoodTransport` is the half of this package that actually talks to a live-money venue, and +# it was previously untested end to end -- only the pure helpers above had coverage. That gap is +# what let a wrong endpoint path ship: nothing asserted which URL any method builds. Everything +# below therefore drives the REAL transport and fakes only `requests.request`, so the code under +# test is the code that runs in production, minus the socket. +# +# Robinhood ships no sandbox, so there is no lower-stakes environment to point these at. A real +# network call from this file would be a real order. +# --------------------------------------------------------------------------------------------- + +_BASE_URL = "https://trading.robinhood.com" + + +class _FakeResponse: + """The slice of `requests.Response` that `_request` actually touches.""" + + def __init__( + self, status_code: int = 200, payload: Any = None, text: str | None = None + ) -> None: + self.status_code = status_code + if text is not None: + self.text = text + elif payload is None: + self.text = "" + else: + self.text = json.dumps(payload) + self.content = self.text.encode() + + def json(self) -> Any: + return json.loads(self.text) + + def raise_for_status(self) -> None: + if self.status_code >= 400: + raise RuntimeError(f"{self.status_code} Error for url") + + +class _RecordingHTTP: + """Stands in for `requests.request`, recording every call and replaying canned responses. + + Responses may be a single `_FakeResponse` (returned for every call), a list (returned in + order, with the last one repeating), or a callable taking `(method, url, data)`. + """ + + def __init__(self, responses: Any) -> None: + self.calls: list[dict[str, Any]] = [] + self._responses = responses + + def __call__( + self, + method: str, + url: str, + headers: dict[str, str] | None = None, + data: Any = None, + timeout: float | None = None, + ) -> _FakeResponse: + self.calls.append( + {"method": method, "url": url, "headers": headers, "data": data, "timeout": timeout} + ) + if callable(self._responses): + result: _FakeResponse = self._responses(method, url, data) + return result + if isinstance(self._responses, list): + index = min(len(self.calls) - 1, len(self._responses) - 1) + response: _FakeResponse = self._responses[index] + return response + single: _FakeResponse = self._responses + return single + + +@pytest.fixture +def http(monkeypatch: pytest.MonkeyPatch) -> Any: + """Install a `_RecordingHTTP` over `requests.request` and hand back the installer. + + `transport._request` does `import requests` at call time, so patching the attribute on the + real module is what the deferred import will see. + """ + import requests + + def install(responses: Any) -> _RecordingHTTP: + recorder = _RecordingHTTP(responses) + monkeypatch.setattr(requests, "request", recorder) + return recorder + + return install + + +def _transport(**kwargs: Any) -> RobinhoodTransport: + """A transport with a real signing key and a pre-set account number. + + `account_number` is supplied by default so a test asserting one endpoint's path is not also + forced to absorb the `GET /accounts/` resolution round trip. The tests that care about that + resolution construct their own without it. + """ + kwargs.setdefault("account_number", "AB1234567890") + return RobinhoodTransport( + api_key="rh-api-key-1", private_key_b64=_TEST_SEED_B64, **kwargs + ) + + +def _path_of(url: str) -> str: + """The path + query of a recorded URL -- exactly the string that must have been signed.""" + split = urlsplit(url) + return split.path + (f"?{split.query}" if split.query else "") + + +def _assert_signature_covers_what_was_sent(call: dict[str, Any]) -> None: + """Verify the recorded request's signature against the bytes actually put on the wire. + + This is the single most important property in this module and the one with the least + forgiving failure mode. Robinhood recomputes the signature server-side from the request it + RECEIVED, so any divergence between the string signed and the string sent is a 401 that is + indistinguishable from a bad key, a revoked credential, or a stale clock -- there is no error + body that says "your query string differed". Rather than asserting the two are equal by + inspection, this re-derives the canonical message from the recorded URL and body and verifies + it cryptographically, which is the same check Robinhood's server performs. + """ + headers = call["headers"] + body_sent = call["data"] or "" + if isinstance(body_sent, bytes): + body_sent = body_sent.decode() + message = ( + f"{headers['x-api-key']}{headers['x-timestamp']}" + f"{_path_of(call['url'])}{call['method'].upper()}{body_sent}" + ).encode() + signature = base64.b64decode(headers["x-signature"]) + _verify_key().verify(message, signature) + + +def test_request_signs_exactly_the_path_and_body_it_sends(http: Any) -> None: + """A GET with query params: the signature must cover the query string too.""" + recorder = http(_FakeResponse(payload={"results": []})) + _transport().get_estimated_price(symbol="BTC-USD", side="ask", quantity="0.1") + + _assert_signature_covers_what_was_sent(recorder.calls[0]) + + +def test_request_signs_the_exact_body_bytes_it_sends(http: Any) -> None: + """A POST body is serialized ONCE and that same string is both signed and sent. + + Handing `requests` the dict via its `json=` kwarg would re-serialize it, and nothing + guarantees the re-serialization is byte-identical to what was signed -- a different key order + or separator spacing is enough to invalidate the signature. + """ + recorder = http(_FakeResponse(payload={"id": "abc", "state": "open"})) + body = {"symbol": "BTC-USD", "client_order_id": "c1", "side": "sell", "type": "market"} + _transport().create_order(body) + + call = recorder.calls[0] + _assert_signature_covers_what_was_sent(call) + assert json.loads(call["data"]) == body + + +def test_request_returns_none_for_a_404_and_raises_for_every_other_error(http: Any) -> None: + """The 404/everything-else split is the load-bearing safety property of this transport. + + `None` means "the venue does not recognise this id" and NOTHING else, because + `adapter.get_order` turns a `None` into a terminal `FAILED` status with zeroed money fields. + If a 500 or a timeout were laundered into `None` too, a transient blip while polling a live + resting order would report that order as dead -- and the engine could re-enter a position + whose original order is still working at the venue. + """ + http(_FakeResponse(404)) + assert _transport().get_order("no-such-id") is None + + for status in (401, 429, 500, 503): + http(_FakeResponse(status)) + with pytest.raises(RuntimeError): + _transport().get_order("some-id") + + +def test_request_returns_none_for_an_empty_body(http: Any) -> None: + http(_FakeResponse(200, text="")) + assert _transport().get_order("some-id") is None + + +def test_request_percent_encodes_query_values_and_signs_the_encoded_form(http: Any) -> None: + """Query values are percent-encoded, and the encoded string is what gets signed AND sent. + + The hand-rolled `f"{k}={v}"` join this replaces put raw values on the wire. A `+` in a value + is the sharp edge: it is the URL encoding of a SPACE, so a server decodes `1E+2` as `1E 2` -- + meaning the value Robinhood verifies the signature over and the value it then parses are + different strings. That is not a theoretical pairing with the `Decimal` rendering bug either; + `str(Decimal("1E+2"))` is literally `"1E+2"`. + + Byte-identity is the part that must survive the fix: encoding for the wire but signing the + raw form (or vice versa) would trade a parsing bug for a permanent 401. + """ + recorder = http(_FakeResponse(payload={"results": []})) + _transport().get_estimated_price(symbol="BTC-USD", side="ask", quantity="1E+2") + + call = recorder.calls[0] + assert "quantity=1E%2B2" in call["url"], call["url"] + # Decoding what was sent must give back the value that was asked for, not a space-mangled one. + assert dict(parse_qsl(urlsplit(call["url"]).query))["quantity"] == "1E+2" + _assert_signature_covers_what_was_sent(call) + + +def test_request_parses_unquoted_json_numbers_as_decimal_never_float(http: Any) -> None: + """Money must not pass through `float`, even for one instant. + + Every fixture in this repository quotes its numeric values, which is why this went untested: + with `"price": "65482.30"` the value is already a `str` and `Decimal(str(v))` is exact. But + nothing in Robinhood's API guarantees quoting, and `response.json()` parses an UNQUOTED + number as a `float` -- so `65482.30` unquoted becomes a float, and the adapter's + `Decimal(str(...))` then faithfully preserves whatever the float rounded to. The precision is + already gone by the time any `Decimal` is constructed. + + `parse_float=Decimal` moves the conversion to the parser, where the original digits are still + available. + """ + http(_FakeResponse(text='{"results": [{"price": 65482.30, "quantity": 0.1}]}')) + response = _transport().get_estimated_price(symbol="BTC-USD", side="ask", quantity="0.1") + + row = _results(response)[0] + assert isinstance(row["price"], Decimal), f"got {type(row['price'])}" + assert row["price"] == Decimal("65482.30") + assert isinstance(row["quantity"], Decimal) + # The exactness this buys, stated as the property that actually matters. + assert row["price"] * 3 == Decimal("196446.90") + + +#: Every transport method, with the EXACT v2 path it must request. +#: +#: Quoted from https://docs.robinhood.com/crypto/trading/ (the single-page v2 reference). These +#: are asserted literally, and that literalness is the point: nothing previously asserted any +#: endpoint path, which is precisely how a wrong namespace could ship unnoticed. Note that +#: `estimated_price` sits under `/trading/` while `best_bid_ask` sits under `/marketdata/` -- +#: that asymmetry is REAL in Robinhood's v2 API and is not a bug in this transport (see +#: `RobinhoodTransport.get_estimated_price`'s comment for the verbatim doc lines). +_ENDPOINTS: list[tuple[str, str, str, str]] = [ + ("get_accounts", "GET", "/api/v2/crypto/trading/accounts/", ""), + ( + "get_holdings", + "GET", + "/api/v2/crypto/trading/holdings/", + "account_number=AB1234567890", + ), + ("get_trading_pairs", "GET", "/api/v2/crypto/trading/trading_pairs/", ""), + ("get_best_bid_ask", "GET", "/api/v2/crypto/marketdata/best_bid_ask/", "symbol=BTC-USD"), + ( + "get_estimated_price", + "GET", + "/api/v2/crypto/trading/estimated_price/", + "quantity=0.1&side=ask&symbol=BTC-USD", + ), + ("create_order", "POST", "/api/v2/crypto/trading/orders/", "account_number=AB1234567890"), + ( + "get_order", + "GET", + "/api/v2/crypto/trading/orders/order-id-1/", + "account_number=AB1234567890", + ), + ("cancel_order", "POST", "/api/v2/crypto/trading/orders/order-id-1/cancel/", ""), +] + +_CALL_ARGS: dict[str, dict[str, Any]] = { + "get_best_bid_ask": {"symbol": "BTC-USD"}, + "get_estimated_price": {"symbol": "BTC-USD", "side": "ask", "quantity": "0.1"}, + "create_order": {"body": {"symbol": "BTC-USD"}}, + "get_order": {"order_id": "order-id-1"}, + "cancel_order": {"order_id": "order-id-1"}, +} + + +@pytest.mark.parametrize(("method_name", "verb", "path", "query"), _ENDPOINTS) +def test_every_endpoint_requests_its_documented_v2_path( + http: Any, method_name: str, verb: str, path: str, query: str +) -> None: + """Pin the literal URL and HTTP verb of every method that talks to the venue. + + A wrong path is not a loud failure. It is a 404, which `_request` converts to `None`, which + `adapter.get_order` converts into a terminal `FAILED` status with zeroed money for an order + that is alive and resting at the venue -- corrupting reconciliation rather than crashing. + """ + recorder = http(_FakeResponse(payload={"results": [], "next": None})) + getattr(_transport(), method_name)(**_CALL_ARGS.get(method_name, {})) + + call = recorder.calls[0] + assert call["method"] == verb + assert call["url"] == f"{_BASE_URL}{path}" + (f"?{query}" if query else "") + _assert_signature_covers_what_was_sent(call) + + +def test_get_order_sends_the_account_number_that_create_order_sends(http: Any) -> None: + """`account_number` is required on v2's order endpoints, and `get_order` must not omit it. + + Robinhood's own v2 sample client builds this path as + `f"/api/v2/crypto/trading/orders/{order_id}/{query_params}"` with + `params = {"account_number": account_number}` -- the same query param `create_order` already + sends. Omitting it risks a 404, and a 404 here is the quiet failure described above: a live + resting order reported as terminally FAILED with zeroed money, which reconciliation then acts + on. This test asserts the two endpoints agree rather than trusting them to drift together. + """ + recorder = http(_FakeResponse(payload={"id": "order-id-1", "state": "open"})) + transport = _transport() + transport.create_order({"symbol": "BTC-USD"}) + transport.get_order("order-id-1") + + def account_of(call: dict[str, Any]) -> str | None: + return dict(parse_qsl(urlsplit(call["url"]).query)).get("account_number") + + created, fetched = recorder.calls[0], recorder.calls[1] + assert account_of(created) == "AB1234567890" + assert account_of(fetched) == account_of(created) + + +def test_trading_pairs_and_best_bid_ask_surface_their_documented_fields(http: Any) -> None: + """The two reads the adapter does not call yet still have to return usable rows. + + `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. + """ + http(_FakeResponse(payload=_load_fixture("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 + + http(_FakeResponse(payload=_load_fixture("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" + + +# --------------------------------------------------------------------------------------------- +# `_paginate`: following `next` cursors across pages. +# --------------------------------------------------------------------------------------------- + +_HOLDINGS_PATH = "/api/v2/crypto/trading/holdings/" + + +def test_paginate_follows_a_next_cursor_and_concatenates_both_pages(http: Any) -> None: + """A holdings or orders list that spans two pages must not silently report only page one. + + Robinhood's `next` on page one is a full absolute URL, not a path this transport built, so + the second request has to be derived from that cursor rather than re-using the first + request's params. Getting this wrong either drops every row past the first page (an account's + real holdings under-reported, feeding a wrong balance into risk checks) or -- if the base URL + is naively concatenated onto the cursor instead of stripped from it -- sends a malformed + request to `https://trading.robinhood.com/https://trading.robinhood.com/...` that 404s and + is silently swallowed by `_request`, which reads exactly the same as "only one page existed." + """ + page1 = { + "results": [{"id": "h1"}], + "next": f"{_BASE_URL}{_HOLDINGS_PATH}?cursor=page2", + "previous": None, + } + page2 = {"results": [{"id": "h2"}], "next": None, "previous": f"{_BASE_URL}{_HOLDINGS_PATH}"} + recorder = http([_FakeResponse(payload=page1), _FakeResponse(payload=page2)]) + transport = _transport() + + response = transport._paginate(_HOLDINGS_PATH, params={"account_number": "AB1234567890"}) + + assert _results(response) == [{"id": "h1"}, {"id": "h2"}] + assert len(recorder.calls) == 2 + assert recorder.calls[0]["url"] == f"{_BASE_URL}{_HOLDINGS_PATH}?account_number=AB1234567890" + # The second request's URL is the cursor with the base URL stripped and re-applied once -- + # not the base URL doubled, and not the first page's params tacked back on. + assert recorder.calls[1]["url"] == f"{_BASE_URL}{_HOLDINGS_PATH}?cursor=page2" + assert "account_number" not in recorder.calls[1]["url"] + + +def test_paginate_raises_rather_than_loop_forever_on_a_self_referencing_cursor(http: Any) -> None: + """`next` is server-controlled input, and a Robinhood bug that hands back a cursor pointing at + itself must not be able to hang this transport indefinitely against a live-money credential. + A `get_holdings()` or `get_order` poll that never returns would hold the credential's + request budget hostage and stall whatever reconciliation loop called it -- the `_MAX_PAGES` + cap exists precisely so that failure mode becomes a loud `RuntimeError` instead of a silent + hang. This pins that the cap actually fires, and that it fires at the documented bound rather + than after some larger, undocumented number of requests. + """ + self_referencing_next = f"{_BASE_URL}{_HOLDINGS_PATH}?cursor=loop" + page = {"results": [{"id": "h"}], "next": self_referencing_next, "previous": None} + recorder = http(_FakeResponse(payload=page)) + transport = _transport() + + with pytest.raises(RuntimeError, match="did not terminate"): + transport._paginate(_HOLDINGS_PATH, params={"account_number": "AB1234567890"}) + + assert len(recorder.calls) == _MAX_PAGES + + +# --------------------------------------------------------------------------------------------- +# `_next_path`: hardening against hostile or degenerate `next` values. +# --------------------------------------------------------------------------------------------- + +_HOSTILE_NEXT_CURSORS: list[Any] = [ + pytest.param(12345, id="non-string-int"), + pytest.param(["not", "a", "string"], id="non-string-list"), + pytest.param({"not": "a string"}, id="non-string-dict"), + pytest.param("", id="empty-string"), + pytest.param("https://evil.example/api/x", id="off-host-absolute-url"), + pytest.param("api/v2/crypto/trading/holdings/?cursor=2", id="relative-not-absolute-path"), +] + + +@pytest.mark.parametrize("cursor", _HOSTILE_NEXT_CURSORS) +def test_next_path_stops_cleanly_on_a_hostile_or_degenerate_cursor( + http: Any, cursor: Any +) -> None: + """Every one of these cursor shapes is something Robinhood's server -- or a bug in it -- + could hand back, and none of them may be allowed to crash `get_holdings`/`get_order` mid + reconciliation or, worse, cause this transport to sign and send the account's live credentials + to a URL on someone else's host. A non-string `next` used to reach `.startswith` and raise + `AttributeError` straight out of a read the adapter had no reason to expect could fail that + way; an absolute URL on a different host used to be concatenated onto the base URL and + requested rather than refused. The safe behavior for all of these is identical: stop + paginating, return what was already fetched, and never issue the extra request. + """ + page1 = {"results": [{"id": "h1"}], "next": cursor, "previous": None} + recorder = http(_FakeResponse(payload=page1)) + transport = _transport() + + response = transport._paginate(_HOLDINGS_PATH, params={"account_number": "AB1234567890"}) + + assert _results(response) == [{"id": "h1"}] + assert len(recorder.calls) == 1 + assert recorder.calls[0]["url"] == f"{_BASE_URL}{_HOLDINGS_PATH}?account_number=AB1234567890" + + +_GOOD_NEXT_CURSORS: list[Any] = [ + pytest.param(f"{_BASE_URL}{_HOLDINGS_PATH}?cursor=page2", id="full-same-host-absolute-url"), + pytest.param(f"{_HOLDINGS_PATH}?cursor=page2", id="bare-absolute-path"), +] + + +@pytest.mark.parametrize("cursor", _GOOD_NEXT_CURSORS) +def test_next_path_follows_the_two_legitimate_cursor_shapes(http: Any, cursor: str) -> None: + """The hardening in `_next_path` must reject hostile input without becoming so strict that it + also rejects the two cursor shapes Robinhood actually sends: a full same-host URL and a bare + `/api/v2/...` path. A regression here would look identical to the `_MAX_PAGES` bug from the + caller's side -- pagination silently stops one page early -- except it would trigger on every + real multi-page account instead of only in a server-bug edge case, quietly under-reporting + holdings or order history on the very first account with more than one page. + """ + page1 = {"results": [{"id": "h1"}], "next": cursor, "previous": None} + page2 = {"results": [{"id": "h2"}], "next": None, "previous": None} + recorder = http([_FakeResponse(payload=page1), _FakeResponse(payload=page2)]) + transport = _transport() + + response = transport._paginate(_HOLDINGS_PATH, params={"account_number": "AB1234567890"}) + + assert _results(response) == [{"id": "h1"}, {"id": "h2"}] + assert len(recorder.calls) == 2 + expected_second_url = cursor if cursor.startswith(_BASE_URL) else f"{_BASE_URL}{cursor}" + assert recorder.calls[1]["url"] == expected_second_url + + +# --------------------------------------------------------------------------------------------- +# `_account()`: resolution from `GET /accounts/` and caching. +# --------------------------------------------------------------------------------------------- + + +def test_account_resolves_from_the_first_accounts_row_and_then_caches_it(http: Any) -> None: + """When no `account_number` is configured up front, this transport must resolve one exactly + once and reuse it for the rest of its life. Every write and every account-scoped read signs + and sends `account_number` on the query string, so re-resolving it on every call would not + just waste a request -- if the account list ever changed shape between calls (a second account + added, ordering shifted), a later call could silently start signing and trading against a + different account number than the one the caller has been observing. Caching after the first + resolution is what makes "resolve once" also mean "stays fixed for this transport's lifetime." + """ + accounts_fixture = _load_fixture("rh_accounts.json") + expected_account_number = accounts_fixture["results"][0]["account_number"] + recorder = http(_FakeResponse(payload=accounts_fixture)) + transport = _transport(account_number=None) + + resolved = transport._account() + + assert resolved == expected_account_number + assert len(recorder.calls) == 1 + assert recorder.calls[0]["method"] == "GET" + assert _path_of(recorder.calls[0]["url"]) == "/api/v2/crypto/trading/accounts/" + + resolved_again = transport._account() + + assert resolved_again == expected_account_number + # A second call needing the account must not issue a second `/accounts/` request. + assert len(recorder.calls) == 1 + + +def test_account_raises_when_accounts_endpoint_returns_no_rows(http: Any) -> None: + """A credential with zero accounts is not a state this transport can trade in, and it must + fail loudly at resolution time rather than proceed with `account_number=None` (or some other + falsy placeholder) silently baked into every subsequent signed request -- which would turn + into a stream of confusing 401s or 404s with no indication the actual problem was an empty + accounts list. + """ + http(_FakeResponse(payload={"results": [], "next": None, "previous": None})) + transport = _transport(account_number=None) + + with pytest.raises(RuntimeError, match="no accounts"): + transport._account() + + +def test_account_raises_when_the_first_row_has_no_account_number_field(http: Any) -> None: + """A malformed or unexpectedly-shaped accounts row must fail resolution outright rather than + resolve to `None` (via `_field`'s default) and let that flow silently into every subsequent + query string as the literal string `"None"` -- which would sign and send a syntactically valid + but semantically meaningless `account_number`, and every trade or holdings read after it would + fail against an account the venue has never heard of. + """ + accounts_fixture = _load_fixture("rh_accounts.json") + first_row = accounts_fixture["results"][0] + malformed_row = {k: v for k, v in first_row.items() if k != "account_number"} + http(_FakeResponse(payload={"results": [malformed_row], "next": None, "previous": None})) + transport = _transport(account_number=None) + + with pytest.raises(RuntimeError, match="account_number"): + transport._account()