feat(brokers): report robinhood's own sizing bounds on the preview - #418
Merged
Conversation
`trading_pairs` carries `min_order_amount`, `asset_increment` and `max_order_size`, and nothing read them -- `get_trading_pairs` was one of two transport methods the adapter never called. A sub-minimum or off-increment order found out at the venue. `preview_order` now reads them and reports what they say through `Preview.errors`, alongside the bounds themselves in `Preview.detail`, so the human at the confirm gate sees the number an order was measured against rather than a bare sentence. THE DENOMINATIONS ARE NOT THE SAME, and that is the part that bites. Established live on 2026-08-19 across all 89 pairs: * `min_order_amount` is QUOTE currency. All 63 pairs that carry it report `0.1`, from BTC at ~$68,000 to DOGE at ~$0.07 -- a constant cannot be a base-denominated minimum across six orders of magnitude of unit price. It is a venue-wide $0.10 floor. The obvious implementation, `base_size >= min_order_amount`, would reject every BTC order under 0.1 BTC (~$6,800), the exit path included -- a far worse failure than the missing check. * `max_order_size` and `asset_increment` are BASE. `max_order_size` varies per asset (20 BTC, 6,500,000 DOGE) and only lands on a comparable notional ceiling read that way. The names carry it once pointed out: amount is quote, size is base. REPORTED, NEVER ENFORCED. `place_order` does not call this and is pinned by test not to. Two reasons, and the second decides it: every order this adapter can place is an exit or a protective leg, since entries are `MarketIOCByQuote` which this venue cannot express at all; and nobody has ever watched this venue reject an out-of-bounds order, because no order has ever been placed against it (#412). Refusing locally would be a guess, and a guess that refuses an order the venue would have ACCEPTED is a new failure this package invented. The off-increment note says outright that the outcome is unobserved rather than predicting one. Enforcement can be revisited once #412 has seen a real rejection. Three absences stay absences rather than collapsing to a passing check: a pair with no `min_order_amount` (26 of 89, #230) says so; a `trading_pairs` call that fails or returns nothing says the bounds were NOT checked; and an unpriced market order is not accused of being below a minimum, because its zero `quote_size` is an absence and not a number. `symbol=` is passed so the venue filters. #230 is the standing lesson: the probe that read `results[0]` off the unfiltered 89-row response got BILL-USD and concluded the venue publishes no minimum at all. Not cached, deliberately -- a remembered bound is one that can go stale, and a preview approving a spend against a stale ceiling asserts something it did not check. Cost is one request per confirm-gate call. Test fakes that answer `accounts` and `estimated_price` but not `trading_pairs` are incomplete venues, not neutral ones, so the six pricing tests asserting no errors now supply it; the ones that mean "the venue did not answer" leave it out and assert the note. Refs #410. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
eaitbrahim
added a commit
that referenced
this pull request
Aug 20, 2026
Minor, not patch. Three things since v0.9.3 change what an implementor or a deployment can rely on: * A NEW DISTRIBUTION. `keel-broker-alpaca` (#382, #384) plus the paper-equities profile that selects it (#386), so a deployment can now be US equities via the broker port rather than crypto only. * THE PORT CONTRACT MOVED TWICE. `market_clock`/`market_schedule` made venues session-aware (#385), and `place_order` gained `idempotency_key` (#419). Both carry defaults so no CALLER breaks, but a third-party adapter that does not accept them is no longer a `Broker` -- the conformance suite now says so. That is exactly the kind of change a patch bump must not hide. * THE OPERATOR CONSOLE. The TUI became keel's console across #399-#408, and `keel update` (#415/#417) makes a deployment self-updating. Every pinned sibling moves with it. The four production distributions are required `==` at this exact version (`RELEASING.md`, "Release assets"), so a bump that missed one would install a mixed set -- the `keel-trader 0.5.7` against `keel-core 0.5.5` failure `keel versions` exists to catch, and which `~/keel` actually ran across two releases. Also in this window, on the Robinhood adapter: the best_bid_ask fixture corrected against the live venue (#414), a credential guard that catches the error it only claimed to (#416), pre-flight sizing reported on the preview (#418), transport backoff (#420), and the fenced one-order probe (#421). Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
First half of #410. Reported, never enforced — see the boundary section below for why that split is the design and not a shortcut.
The gap
trading_pairscarriesmin_order_amount,asset_incrementandmax_order_size, and nothing read them —get_trading_pairswas one of two transport methods the adapter never called. A sub-minimum or off-increment order found out at the venue.preview_ordernow reads them, reports what they say throughPreview.errors, and puts the bounds themselves inPreview.detail, so the human at the confirm gate sees the number an order was measured against rather than a bare sentence.The denominations are not the same
This is the part that bites, established live on 2026-08-19 across all 89 pairs:
min_order_amountAll 63 pairs that carry the field report the same
0.1. A constant cannot be a base-denominated minimum across six orders of magnitude of unit price — it is a venue-wide $0.10 floor in quote currency.The obvious implementation,
base_size >= min_order_amount, would reject every BTC order under 0.1 BTC (~$6,800), the exit path included — a far worse failure than the missing check this issue exists to add.max_order_sizeandasset_incrementgo the other way, both base:max_order_sizevaries per asset (20 BTC, 6,500,000 DOGE) and only lands on a comparable notional ceiling ($1.37M, $474k) read that way. The names carry it once pointed out — amount is quote, size is base — which is why_PairRulessays so at the top anddetaillabels each key with its denomination.The boundary: reported, never enforced
place_orderdoes not call this, andtest_place_order_does_not_consult_the_sizing_boundspins that it does not. Two reasons, and the second decides it:MarketIOCByQuote, which this venue cannot express at all. A check that refuses on those paths can strand a position or leave one running without its stop.The off-increment note says outright that the outcome is unobserved rather than predicting one. Enforcement can be revisited once #412 has seen a real rejection.
Absences stay absences
Three cases that must not collapse into a passing check, one test each:
min_order_amount(26 of 89, The probe only inspects results[0], and it cost us a real field — min_order_amount was removed on a false negative #230) → says the minimum was not checked,detailreadsunknown, not0trading_pairscall that fails or returns nothing → says the bounds were not checkedquote_sizeis an absence, not a numbersymbol=is passed so the venue filters. #230 is the standing lesson: the probe that readresults[0]off the unfiltered 89-row response got BILL-USD and concluded the venue publishes no minimum at all.Not cached, deliberately — a remembered bound can go stale, and a preview approving a spend against a stale ceiling asserts something it did not check. Cost is one request per confirm-gate call, which is once per human decision, not a loop.
Test fakes
Six pricing tests asserted
errors == ()with fakes that answeraccountsandestimated_pricebut nottrading_pairs. That is an incomplete venue, not a neutral one, so they now supply it via_pairs(); the tests that genuinely mean "the venue did not answer" leave it out and assert the resulting note.packages/*from a git worktree: the venv's editable installs resolve to the main checkout, soPYTHONPATHhas to put the worktree's packages first or you are testingmain's adapter.keel/andscripts/resolve from cwd and are unaffected. This ran green against the unmodified package before I noticed.Full suite: 3662 passed, 3 skipped, with the worktree packages on the path.
ruff checkandmypyclean.🤖 Generated with Claude Code