fix(brokers): close the review findings on the robinhood adapter

[eaitbrahim]

Follow-ups to #192, which merged before these review fixes landed. Every finding below was
raised against that PR; the adapter shipped to main with all four blockers live, so this is the
catch-up. Diff is fix-only — the package itself is already on main via the squash.

Not merging — that is the user's call.

Blockers

B1 — str(Decimal) emitted scientific notation into the order body

str(Decimal("0.00000001")) == "1E-8", and BTC's asset_increment is exactly 0.00000001 per
this repo's own tests/fixtures/rh_trading_pairs.json. One satoshi is therefore the smallest
order this venue accepts, not an edge case — it is the size a dust-sized exit produces. Robinhood's
asset_quantity / limit_price / stop_price have no exponent form, so that body is malformed.

The failure mode is the quiet kind: a rejected exit leaves a position open while the engine
records it closed, and a rejected stop-limit leaves a position unprotected while local state
says there is a stop. Every money and size field now renders through translate._render
(format(d, "f")), which is positional at every magnitude and neither rounds nor truncates.

The test that should have caught this asserted the property over 0.123456789 and 64000.10 —
two values that cannot trigger the exponent form. It is now parametrized over values that do
(0.00000001, 0.000000012345, 1E+2 → 100), plus a structural assertion that no rendered
field ever contains an e. Confirmed failing against the old implementation before fixing.

B2 — place_order reported success for orders the venue rejected

The old code checked only that an id came back, on the stated reasoning that "a placement that
comes back at all came back as an order." That is false here: Robinhood answers a rejected order
on the happy HTTP path — 200, with a real order object whose state reads failed.

A StopLimitGTC answered {"id": "...", "state": "failed"} was recorded as a protective stop that
does not exist at the venue. failed and canceled now return PlaceResult(success=False, ...)
with the state named in reason, and broker_order_id=None matching CoinbaseAdapter's failure
path.

An unrecognised state still reports success, deliberately — the asymmetry with get_order is
argued in the docstring: 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 success hands back the id and
lets reconciliation poll (where an unknown state maps to PENDING and stays under observation).

B3 — estimated_price namespace: the reviewer was wrong, the code was right

Resolved from the primary source, not by guessing: https://docs.robinhood.com/crypto/trading/.
The v2 API genuinely splits these two neighbouring reads across namespaces —

get/api/v2/crypto/trading/estimated_price/
get/api/v2/crypto/marketdata/best_bid_ask/

— which is exactly what transport.py already did. The instinct to "fix" it is understandable
because v1 is the consistent one (/api/v1/crypto/marketdata/estimated_price/). Required
query params confirmed as symbol, side (bid/ask/both), quantity, all three required.

No behaviour change. The path is now pinned by a test and anchored to the doc URL with the verbatim
quoted paths in a comment, so this cannot be re-litigated.

B4 — get_order omitted account_number; cancel_order correctly does not

get_order now sends the same account_number query param create_order sends, per Robinhood's
own v2 sample client. Omitting it risks a 404 — and a 404 here is quiet corruption, 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.

Partially rejecting the finding: the review asked to make all three consistent. Robinhood
documents post/api/v2/crypto/trading/orders/{id}/cancel/ with a path parameter only and no
query-parameter section, and their own v2 sample omits it there while passing it for place/fetch.
Adding an undocumented param would be a guess, and every query byte is signed — so a guess the
venue rejects is a 401 on the cancel path. Left as-is, with the reasoning written down.

Should-fix

#	Finding	Resolution
S1	Silent-zero previews	Preview.errors populated on every unpriced path; _estimated_price returns None rather than Decimal("0") so a pricing failure is distinguishable from a free order at the confirm gate
S2	preview_order approved symbols place_order refuses	to_symbol validated on every preview path — ETH-USDC no longer previews cleanly then raises after the human approved
S3	Float laundering	json.loads(..., parse_float=Decimal); test uses unquoted numeric money, which all eight existing fixtures quote — the reason this was untestable before
S4	Transport untested	Real coverage against a fake HTTP layer: signature/wire byte-identity (verified cryptographically, not by inspection), the 404-vs-raise split, pagination + _MAX_PAGES, _account caching, and the literal endpoint path of every method
S5	Hand-built query string	Percent-encoded via urlencode(..., quote_via=quote, safe=""), with signed and sent bytes still byte-identical
S6	cancel_order could raise on the exit path	Fails safe to False. This codebase's own principle — "a raise on the exit path can trap a position" — with why in the docstring
S7	Idempotency tradeoff undocumented	Behaviour kept; now in the README's must-fix-before-wiring list, not only a docstring

Nits

• Per-call account caching: _fee_ratio takes the account as a parameter, so get_fee_summary and
  preview_order each make one GET /accounts/ instead of two-plus. Deliberately not
  memoized on the instance — get_balances reads buying_power off the same payload and a stale
  one would misreport available capital.
• _paginate cursor hardening: a non-string next no longer raises AttributeError out of a read
  the adapter cannot expect to fail that way, and an absolute URL on another host stops pagination
  rather than being concatenated onto the base URL.
• get_trading_pairs / get_best_bid_ask documented as deliberately uncalled, with the reason
  pre-flight validation is a follow-up rather than a nit fix (a check that runs before every
  placement also runs before every exit). Their fixtures are now asserted against.

README

Added a "Must fix BEFORE wiring this to the live path" section so the Phase B migration trips
over it: the fees_usd hole makes subscription-lapse detection not merely inert but
always-passing against this venue; the per-call client_order_id permits duplicate orders on
retry; and keel/cli.py's _interactive_confirm takes a raw dict and iterates .items(), so it
has nowhere to display Preview.synthetic (or Preview.errors) — this adapter's synthetic-preview
flag is invisible at the confirm gate until that path is migrated.

Gates

$ uv run ruff check keel tests packages
All checks passed!

$ uv run mypy
Success: no issues found in 212 source files

$ uv run pytest -q -rs
SKIPPED [1] packages/keel-broker-api/keel_broker_api/conformance/suite.py:254: adapter serves no granularity the suite could exercise
2268 passed, 1 skipped in 31.33s

Baseline before these changes was 2206 passed, 1 skipped. Skip count unchanged and it is the same
skip — the conformance candles probe. mypy strict still covers keel_broker_robinhood.*
(pyproject.toml untouched). No .github/workflows/, config*.yaml, or .db touched.

🤖 Generated with Claude Code

---

Closes #196.

Follow-ups deliberately NOT in this PR, tracked separately: #197 (fees_usd always 0), #198 (prerequisites before wiring to the live path), #199 (confirm gate cannot display Preview.synthetic).

Context: follow-up to #192, which merged before these fixes landed.