diff --git a/packages/keel-broker-alpaca/keel_broker_alpaca/adapter.py b/packages/keel-broker-alpaca/keel_broker_alpaca/adapter.py index e7f9abb..9c2aad8 100644 --- a/packages/keel-broker-alpaca/keel_broker_alpaca/adapter.py +++ b/packages/keel-broker-alpaca/keel_broker_alpaca/adapter.py @@ -53,6 +53,7 @@ Balance, CancelOutcome, FeeSummary, + Instrument, MarketSchedule, OrderStatus, PlaceResult, @@ -363,6 +364,24 @@ def _reject_unsupported(self, spec: OrderSpec) -> None: f"(supported: {', '.join(sorted(_CAPABILITIES.supported_orders))})" ) + def get_instrument(self, product_id: str) -> Instrument | None: + """Not written yet, and that is a statement about this ADAPTER, not about Alpaca. + + Alpaca has an assets endpoint (`/v2/assets/{symbol}`) carrying exactly this -- whether a + symbol is fractionable and its minimum trade increment. This adapter's `Transport` + protocol does not declare it, so there is nothing here to read it through, and adding one + is a change to the transport contract rather than a line in this method. + + `NotImplementedError` rather than `None`. `None` is the port's word for "this venue does + not list that product", and answering it here would tell a caller a symbol is unlisted + when the truth is that nobody has written the lookup -- which on the executor's path + means silently skipping quantization for every equity. + """ + raise NotImplementedError( + "keel-broker-alpaca has no product-catalogue read: Transport declares no assets " + "endpoint. See #524." + ) + def preview_order(self, spec: OrderSpec) -> Preview: """Synthesize a preview. Always `synthetic=True` -- Alpaca has no preview endpoint, so no number below is a quote the venue stands behind. diff --git a/packages/keel-broker-api/keel_broker_api/conformance/suite.py b/packages/keel-broker-api/keel_broker_api/conformance/suite.py index ae0e59f..a8c886a 100644 --- a/packages/keel-broker-api/keel_broker_api/conformance/suite.py +++ b/packages/keel-broker-api/keel_broker_api/conformance/suite.py @@ -43,6 +43,7 @@ def broker(self) -> MyVenueAdapter: Balance, CancelOutcome, FeeSummary, + Instrument, MarketSchedule, OrderStatus, PlaceResult, @@ -328,6 +329,48 @@ def test_fee_summary_matches_its_declaration(self) -> None: assert isinstance(summary.volume_usd, Decimal) assert isinstance(summary.fees_usd, Decimal) + def test_get_instrument_answers_the_port_type_or_declares_it_is_unwritten(self) -> None: + """`Instrument | None`, or an explicit `NotImplementedError` -- never a venue dict. + + There is no capability flag gating this one, unlike `get_fee_summary`, and that is + deliberate: a product catalogue is not an optional venue FEATURE, it is something every + venue has and some adapters have not been taught to read yet. `NotImplementedError` says + which of those it is. `None` must not be used for it -- `None` is this method's word for + "this venue does not list that product", and an adapter answering it for an unwritten + lookup would tell `executor._base_increment_for` a symbol is unlisted when the truth is + that nobody wrote the read, which on the live path means silently skipping quantization. + + The value is checked rather than only its type: a zero or negative increment is what a + caller divides and quantizes against, so it must never cross the port at all. + """ + broker = self.broker() + try: + instrument = broker.get_instrument("BTC-USD") + except NotImplementedError: + return + + if instrument is None: + return + assert isinstance(instrument, Instrument), ( + f"get_instrument returned {type(instrument).__name__}, not the port's Instrument" + ) + assert isinstance(instrument.base_increment, Decimal) + assert instrument.base_increment > 0, "a non-positive increment must never cross the port" + assert instrument.product_id == "BTC-USD", ( + "the Instrument must describe the product that was asked for" + ) + + def test_get_instrument_may_answer_none_for_a_product_the_venue_does_not_list(self) -> None: + """An id that no venue lists. `None` or `NotImplementedError` are both correct; an + exception of any other kind is not, because a product id reaching this method comes from + an operator's allowlist and being absent is ordinary.""" + broker = self.broker() + try: + instrument = broker.get_instrument("NOT-LISTED") + except NotImplementedError: + return + assert instrument is None or isinstance(instrument, Instrument) + # --- no broker-native type crosses the port -------------------------------------------- def test_get_balances_returns_only_domain_types(self) -> None: diff --git a/packages/keel-broker-api/keel_broker_api/port.py b/packages/keel-broker-api/keel_broker_api/port.py index 92c6fb0..e4cd294 100644 --- a/packages/keel-broker-api/keel_broker_api/port.py +++ b/packages/keel-broker-api/keel_broker_api/port.py @@ -13,6 +13,7 @@ Balance, CancelOutcome, FeeSummary, + Instrument, MarketSchedule, OrderStatus, PlaceResult, @@ -128,6 +129,23 @@ def get_candles( def get_balances(self) -> list[Balance]: ... + def get_instrument(self, product_id: str) -> Instrument | None: + """One product's venue-imposed granularity, or `None` if this venue does not list it. + + **`None` is an answer, not a failure, and that is why this differs from `get_order`.** An + order id is one the caller was handed by this venue, so its absence is a genuine + inconsistency worth raising on. A product id arrives from an operator's config allowlist + and may simply not be listed here -- a perfectly ordinary fact about a venue, and the one + a multi-venue deployment most needs to be able to ask without catching an exception. + + Raise for anything that is a real failure: an unreachable venue, a refused credential, a + response that cannot be parsed. `executor._base_increment_for` treats every one of those + the same way it treats `None` -- unknown, send the quantity unquantized, never refuse the + exit -- but that is the CALLER's policy about its own path, not a licence for an adapter + to swallow errors on its behalf. + """ + ... + def preview_order(self, spec: OrderSpec) -> Preview: ... def place_order(self, spec: OrderSpec, *, idempotency_key: str | None = None) -> PlaceResult: diff --git a/packages/keel-broker-api/keel_broker_api/results.py b/packages/keel-broker-api/keel_broker_api/results.py index 94602de..d5e95a0 100644 --- a/packages/keel-broker-api/keel_broker_api/results.py +++ b/packages/keel-broker-api/keel_broker_api/results.py @@ -134,6 +134,43 @@ class Balance: total: Decimal +@dataclass(frozen=True) +class Instrument: + """One tradeable product's venue-imposed granularity. + + **Why this exists at all.** `executor._base_increment_for` (#516) needs the finest `base_size` + a venue will accept, and reads it today by calling `broker.list_products()` and picking + through raw dicts for `product_id` and `base_increment`. That is the pre-port + `CoinbaseClient`'s shape, and it is one of the two gaps #524 names as the reason the live path + cannot move onto the port: the port had no catalog read at all. + + **One product, not the catalogue, and that is the caller's own argument.** `list_products` + returns every product the venue lists -- about 900 on Coinbase -- and + `_base_increment_for` caches exactly ONE of them per miss, because `Repository.set_state` + commits per call and caching all of them would mean ~900 fsyncs inside the order-placement + path. A port method shaped like the caller's need lets an adapter ask the venue for one + product where the venue supports that, and filter locally where it does not. + + **Only `base_increment`, for now.** Quote-side granularity and minimum sizes are the same + class of fact and would sit here naturally, but nothing reads them yet, and a field no caller + reads is a field no test meaningfully checks. + """ + + product_id: str + #: The venue's finest acceptable `base_size` for this product. Always positive: an adapter + #: that cannot obtain a usable value returns `None` from `get_instrument` rather than + #: constructing an Instrument carrying zero, which a caller would quantize against and get + #: a division error or a silent zero size. + base_increment: Decimal + + def __post_init__(self) -> None: + if self.base_increment <= 0: + raise ValueError( + f"base_increment must be positive, got {self.base_increment} for " + f"{self.product_id}" + ) + + @dataclass(frozen=True) class Preview: """What the human approves at the confirm gate (`executor.py:311`). @@ -204,6 +241,7 @@ def __post_init__(self) -> None: __all__ = [ "Balance", + "Instrument", "CancelOutcome", "FeeSummary", "MarketSchedule", diff --git a/packages/keel-broker-coinbase/keel_broker_coinbase/adapter.py b/packages/keel-broker-coinbase/keel_broker_coinbase/adapter.py index 48fc15a..865ae45 100644 --- a/packages/keel-broker-coinbase/keel_broker_coinbase/adapter.py +++ b/packages/keel-broker-coinbase/keel_broker_coinbase/adapter.py @@ -26,6 +26,7 @@ Balance, CancelOutcome, FeeSummary, + Instrument, MarketSchedule, OrderStatus, PlaceResult, @@ -138,6 +139,33 @@ def _reject_unsupported(self, spec: OrderSpec) -> None: if spec.kind not in _CAPABILITIES.supported_orders: raise UnsupportedOrder(f"coinbase does not support order kind {spec.kind!r}") + def get_instrument(self, product_id: str) -> Instrument | None: + """One product's `base_increment`, read from Coinbase's per-product endpoint. + + `get_product` rather than `get_products`: the caller + (`executor._base_increment_for`) needs ONE product and caches ONE, and asking the venue + for the whole catalogue -- about 900 rows -- inside the order-placement path to use a + single field of it is the wrong shape. The transport has carried `get_product` since + before this method existed. + + `None` for a product this venue does not list, or whose `base_increment` is missing, + unparseable or non-positive. All four are the same fact to a caller -- no usable + granularity -- and none of them is an error worth raising on: a product id comes from an + operator's allowlist and may simply not be listed here. + """ + response = self._require_transport().get_product(product_id) + raw = _field(response, "product", response) + increment = _field(raw, "base_increment") + if increment is None: + return None + try: + value = Decimal(str(increment)) + except (ArithmeticError, TypeError, ValueError): + return None + if value <= 0: + return None + return Instrument(product_id=product_id, base_increment=value) + def preview_order(self, spec: OrderSpec) -> Preview: """Preview via Coinbase's own endpoint -- hence `synthetic=False`.""" self._reject_unsupported(spec) diff --git a/packages/keel-broker-fake/keel_broker_fake/adapter.py b/packages/keel-broker-fake/keel_broker_fake/adapter.py index 4106a95..aaf1322 100644 --- a/packages/keel-broker-fake/keel_broker_fake/adapter.py +++ b/packages/keel-broker-fake/keel_broker_fake/adapter.py @@ -34,6 +34,7 @@ Balance, CancelOutcome, FeeSummary, + Instrument, MarketSchedule, OrderStatus, PlaceResult, @@ -146,6 +147,17 @@ def get_balances(self) -> list[Balance]: Balance(currency="BTC", available=Decimal("0.5"), total=Decimal("0.75")), ] + def get_instrument(self, product_id: str) -> Instrument | None: + """A fixed satoshi-grained increment for anything, and `None` for one reserved id. + + The reserved id is the point: `None` is a documented answer of this port method ("this + venue does not list it"), and a fake that could only ever answer with an Instrument would + let the conformance suite pass without any adapter ever exercising the absent case. + """ + if product_id == "NOT-LISTED": + return None + return Instrument(product_id=product_id, base_increment=Decimal("0.00000001")) + def preview_order(self, spec: OrderSpec) -> Preview: """This venue has no preview endpoint and does not estimate one. diff --git a/packages/keel-broker-kraken/keel_broker_kraken/adapter.py b/packages/keel-broker-kraken/keel_broker_kraken/adapter.py index 42cdc3a..55be7d6 100644 --- a/packages/keel-broker-kraken/keel_broker_kraken/adapter.py +++ b/packages/keel-broker-kraken/keel_broker_kraken/adapter.py @@ -33,6 +33,7 @@ Balance, CancelOutcome, FeeSummary, + Instrument, MarketSchedule, OrderStatus, PlaceResult, @@ -95,6 +96,10 @@ def get_balances(self) -> list[Balance]: """Not implemented: the stub reads no account and holds no credentials.""" raise NotImplementedError(_STUB_MESSAGE) + def get_instrument(self, product_id: str) -> Instrument | None: + """Not implemented: the stub reads no catalogue.""" + raise NotImplementedError(_STUB_MESSAGE) + def preview_order(self, spec: OrderSpec) -> Preview: """Not implemented: the stub previews nothing (`can_preview` is False in `capabilities()`, so callers are told before they try).""" diff --git a/packages/keel-broker-robinhood/keel_broker_robinhood/adapter.py b/packages/keel-broker-robinhood/keel_broker_robinhood/adapter.py index 69fecaf..b31a5e1 100644 --- a/packages/keel-broker-robinhood/keel_broker_robinhood/adapter.py +++ b/packages/keel-broker-robinhood/keel_broker_robinhood/adapter.py @@ -67,6 +67,7 @@ Balance, CancelOutcome, FeeSummary, + Instrument, MarketSchedule, OrderStatus, PlaceResult, @@ -482,6 +483,31 @@ def _reject_unsupported(self, spec: OrderSpec) -> None: f"(supported: {', '.join(sorted(_CAPABILITIES.supported_orders))})" ) + def get_instrument(self, product_id: str) -> Instrument | None: + """One pair's minimum order increment, from `trading_pairs/`. + + Robinhood reports `min_order_size` for a pair, which is the same fact + `base_increment` names on Coinbase: the finest base quantity the venue will accept. The + port's field keeps keel's name for it, not the venue's, exactly as `BracketGTC` keeps + `take_profit_price` rather than Coinbase's `limit_price`. + + `None` for an unlisted symbol or an unusable value -- see the port's docstring for why + that is an answer rather than an error. + """ + response = self._require_transport().get_trading_pairs(symbol=product_id) + for raw in _results(response): + if _field(raw, "symbol") != product_id: + continue + size = _field(raw, "min_order_size") + if size is None: + return None + try: + value = Decimal(str(size)) + except (ArithmeticError, TypeError, ValueError): + return None + return Instrument(product_id=product_id, base_increment=value) if value > 0 else None + return None + def preview_order(self, spec: OrderSpec) -> Preview: """Synthesise a preview. Always `synthetic=True` -- there is no preview endpoint here. diff --git a/tests/broker_coinbase/test_adapter.py b/tests/broker_coinbase/test_adapter.py index 7103f07..f0a2652 100644 --- a/tests/broker_coinbase/test_adapter.py +++ b/tests/broker_coinbase/test_adapter.py @@ -46,6 +46,7 @@ def __init__( placed: dict[str, Any] | None = None, summary: dict[str, Any] | None = None, order: dict[str, Any] | None = None, + product: dict[str, Any] | None = None, ) -> None: self._candles = candles self._accounts = accounts @@ -53,6 +54,7 @@ def __init__( self._placed = placed self._summary = summary self._order = order + self._product = product self.calls: dict[str, dict[str, Any]] = {} # Ids this transport has actually issued via `create_order`, so `cancel_orders` can tell # a genuine order apart from one the suite's unknown-id test made up -- the same @@ -71,7 +73,8 @@ def get_candles( return self._candles def get_product(self, product_id: str, **kwargs: Any) -> Any: - return {} + self.calls["get_product"] = {"product_id": product_id} + return {} if self._product is None else self._product def get_accounts(self, **kwargs: Any) -> Any: self.calls["get_accounts"] = {} @@ -402,3 +405,57 @@ def cancel_orders(self, order_ids: list[str], **kwargs: Any) -> Any: outcome = adapter.cancel_order("abc") assert outcome is CancelOutcome.UNKNOWN assert not outcome.settled + + +# -- the product catalogue (#524) ------------------------------------------------------------ + + +def test_get_instrument_reads_the_base_increment_from_the_per_product_endpoint() -> None: + """`get_product`, not `get_products`. + + The caller (`executor._base_increment_for`) needs ONE product and caches ONE; fetching the + whole ~900-row catalogue inside the order-placement path to use a single field of it is the + wrong shape, and Coinbase exposes the per-product read this uses instead. + """ + transport = FakeTransport(product={"product_id": "BTC-USD", "base_increment": "0.00000001"}) + adapter = CoinbaseAdapter(transport) + + instrument = adapter.get_instrument("BTC-USD") + + assert instrument is not None + assert instrument.product_id == "BTC-USD" + assert instrument.base_increment == Decimal("0.00000001") + assert transport.calls["get_product"] == {"product_id": "BTC-USD"} + + +def test_get_instrument_unwraps_a_product_envelope() -> None: + """Coinbase returns the product both bare and wrapped in `{"product": {...}}` depending on + the endpoint and the client version; both must read the same.""" + adapter = CoinbaseAdapter( + FakeTransport(product={"product": {"product_id": "ETH-USD", "base_increment": "0.0001"}}) + ) + instrument = adapter.get_instrument("ETH-USD") + assert instrument is not None and instrument.base_increment == Decimal("0.0001") + + +@pytest.mark.parametrize( + "payload", + [ + {}, + {"product_id": "BTC-USD"}, + {"product_id": "BTC-USD", "base_increment": None}, + {"product_id": "BTC-USD", "base_increment": "not-a-number"}, + {"product_id": "BTC-USD", "base_increment": "0"}, + {"product_id": "BTC-USD", "base_increment": "-1"}, + ], +) +def test_get_instrument_answers_none_for_anything_unusable(payload: dict[str, Any]) -> None: + """Missing, unparseable, zero and negative are one fact to a caller: no usable granularity. + + Zero and negative matter most. `executor._order_configuration` quantizes a SELL against this + value, so a zero crossing the port is a division error or a silent zero size on the exit + path -- which is why `Instrument.__post_init__` refuses it too, and why this returns `None` + rather than constructing one. + """ + adapter = CoinbaseAdapter(FakeTransport(product=payload)) + assert adapter.get_instrument("BTC-USD") is None