From 57a8f65cf23a73fe50ff72e5986aa8faa1a946f9 Mon Sep 17 00:00:00 2001 From: Elmehdi Aitbrahim Date: Thu, 27 Aug 2026 17:50:24 -0400 Subject: [PATCH] feat(executor): every order is an OrderSpec, and the second Coinbase renderer is gone (#524) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The flip. The executor placed orders by handing hand-built Coinbase dicts to a pre-port client; it now builds `OrderSpec` values and reads `Preview`/ `PlaceResult`. **The bytes on the wire are unchanged, and that is verified rather than asserted** -- ten configurations rendered by the new path were compared against the PRE-FLIP renderers loaded out of git, and are byte-identical. ── ONE RENDERER, NOT TWO ────────────────────────────────────────────────────── `CoinbaseClient.preview_order`/`place_order` take a spec and render it through `keel_broker_coinbase.translate.to_order_configuration` -- the adapter's own function. `executor._bracket_order_configuration` is deleted, and with it the test #502 stage 1 shipped to pin the two byte-identical while both existed. That test's own words: "The test imports both; production code does not." There is one now, so there is nothing left to hold in agreement. `_order_configuration` becomes `_order_spec`: BUY is `MarketIOCByQuote`, SELL is `MarketIOCByBase`, the bracket is `BracketGTC`. #516's quantization is untouched and still runs before the spec is built, including its deliberate BUY/SELL asymmetry. ── THE TRAP I NEARLY WALKED INTO ────────────────────────────────────────────── Every `OrderSpec` carries an `initial_status` ClassVar, and using it for the order row's status is the obvious move and WRONG. The port's vocabulary is the venue's (`filled_or_rejected`, `open`); this column is keel's (`filled`, `pending`). `reconcile` sweeps for `pending`, so writing `open` would leave every resting order invisible to the sweep that exists to observe its fill -- a bracket recorded as `open` is a protective order keel would never look at again. `_initial_status` therefore stays, mapping `spec.kind` to KEEL's words. What went is the dict inspection (`next(iter(order_configuration), "")`), not the vocabulary. Caught by a test asserting `'filled'`, which is exactly what that test was for. ── SMALLER THINGS THE TYPES MADE OBVIOUS ────────────────────────────────────── `raw_response` stored the whole placement response as JSON so that `_native_order_id` could dig the id out later to cancel with. It stores `{"order_id": ...}` now, from `PlaceResult.broker_order_id`. No migration: both shapes answer the same `data.get("order_id")`, so old rows read unchanged. `_preview_book` already accepted `Preview | dict` and the Coinbase adapter already carried the book in `detail` -- the executor was written anticipating this -- so #350's spread gate and #332's override warning came through untouched. ── TESTS ────────────────────────────────────────────────────────────────────── ~13 fakes across five files moved to the port's signatures. The dict-shaped preview payloads are kept AS dicts at the call sites and converted by one helper: dozens of tests build a bespoke preview to exercise one degraded field, and rewriting each into a constructor would have been a bigger diff than the change it accompanies, with more chances to alter a case by accident. Gates: 4301 passed / 3 skipped, ruff clean, mypy clean across 347 files, one paper cycle run against real venue data. ── WHAT IS LEFT OF #524 ─────────────────────────────────────────────────────── `_build_broker` still constructs `CoinbaseClient` rather than resolving through `load_broker`. That is now a SMALL change -- the client and every adapter speak the same interface -- gated on two consumers that are not port methods: `assets discover`'s `list_products`, and `assets holdings`' `get_accounts`. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01T6yA5khYnJ2qzheArRToQ2 --- keel/data/cb_client.py | 124 +++++++------ keel/execution/executor.py | 179 +++++++++++------- tests/broker_coinbase/test_translate.py | 38 ++-- tests/data/test_cb_client.py | 234 +++++++++--------------- tests/execution/test_executor.py | 123 +++++++------ tests/execution/test_reconcile.py | 63 ++++--- tests/execution/test_sell_precision.py | 30 +-- tests/execution/test_size_precision.py | 18 +- tests/test_agent.py | 74 ++++---- tests/test_cli.py | 40 ++-- 10 files changed, 462 insertions(+), 461 deletions(-) diff --git a/keel/data/cb_client.py b/keel/data/cb_client.py index ed12dc1..f9d8190 100644 --- a/keel/data/cb_client.py +++ b/keel/data/cb_client.py @@ -26,10 +26,18 @@ from decimal import Decimal from typing import Any, Protocol -from keel_broker_api.results import Balance, CancelOutcome, Instrument +from keel_broker_api.orders import OrderSpec +from keel_broker_api.results import ( + Balance, + CancelOutcome, + Instrument, + PlaceResult, + Preview, +) +from keel_broker_coinbase.translate import to_order_configuration from keel_core.telemetry import log_exception, log_venue_failure -from keel.types import Candle, Granularity, Side +from keel.types import Candle, Granularity logger = logging.getLogger(__name__) @@ -298,70 +306,72 @@ def get_balances(self) -> list[Balance]: ) return balances - def preview_order(self, product_id: str, side: Side, order_configuration: dict) -> dict: - """Preview an order (no funds moved) -- returns `Decimal` money fields + any errors.""" + def preview_order(self, spec: OrderSpec) -> Preview: + """Preview an order (no funds moved), in the PORT's shape (#524). + + **One renderer, not two.** The wire configuration comes from + `keel_broker_coinbase.translate.to_order_configuration` -- the same function the adapter + uses -- rather than a dict this module builds itself. Until now the tree carried two + Coinbase order renderers, and #502 stage 1 had to ship a test pinning them byte-identical + to stop them drifting while both existed. There is one now, so there is nothing left to + hold in agreement. + + `detail` carries `best_bid`/`best_ask` as strings because that is what the port's `Preview` + declares and what `executor._preview_book` already reads off the port shape -- the spread + gate (#350) and the entry-override warning (#332) both come through it unchanged. + """ response = self._transport.preview_order( - product_id=product_id, - side=side.value if isinstance(side, Side) else side, - order_configuration=order_configuration, + product_id=spec.product_id, + side=spec.side.value, + order_configuration=to_order_configuration(spec), ) - decimal_fields = ( - "order_total", - "commission_total", - "quote_size", - "base_size", - "best_bid", - "best_ask", + return Preview( + product_id=spec.product_id, + side=spec.side, + est_base_size=Decimal(_field(response, "base_size", "0") or "0"), + est_quote_size=Decimal(_field(response, "quote_size", "0") or "0"), + est_fee=Decimal(_field(response, "commission_total", "0") or "0"), + # A real quote from the venue, never an estimate this client computed. + synthetic=False, + detail={ + key: str(value) + for key in ("best_bid", "best_ask", "order_total") + if (value := _field(response, key)) is not None + }, + errors=tuple(str(e) for e in (_field(response, "errs", []) or [])), ) - result: dict[str, Any] = {} - for key in decimal_fields: - value = _field(response, key) - if value is not None: - result[key] = Decimal(value) - result["errs"] = _field(response, "errs", []) or [] - result["warning"] = _field(response, "warning", []) or [] - return result - - def place_order(self, product_id: str, side: Side, order_configuration: dict) -> dict: - """Place a live order -- supports market/limit/stop configs, no funds-moving retries. - - Callers (the Phase-3 executor) are responsible for running rails/guards and any - confirm-mode gate *before* calling this -- `place_order` itself performs no risk - checks, it only talks to the transport and normalizes the response. A fresh - `client_order_id` is generated per call for idempotency on the Coinbase side. + + def place_order(self, spec: OrderSpec, *, idempotency_key: str | None = None) -> PlaceResult: + """Place a live order, in the PORT's shape (#524). + + Callers run rails/guards and any confirm-mode gate BEFORE calling this -- `place_order` + performs no risk checks, it only talks to the transport. + + `idempotency_key` identifies the INTENT, not the order, exactly as the port declares: + omit it and a fresh `client_order_id` is minted per attempt, which is what this method did + before the parameter existed and remains the default. Two identical orders a strategy + genuinely meant to place are two orders. """ - side_str = side.value if isinstance(side, Side) else side - client_order_id = str(uuid.uuid4()) response = self._transport.create_order( - client_order_id=client_order_id, - product_id=product_id, - side=side_str, - order_configuration=order_configuration, + client_order_id=idempotency_key or str(uuid.uuid4()), + product_id=spec.product_id, + side=spec.side.value, + order_configuration=to_order_configuration(spec), ) - success = bool(_field(response, "success", False)) success_response = _field(response, "success_response") or {} error_response = _field(response, "error_response") or {} - raw_order_configuration = _field(response, "order_configuration") - - result: dict[str, Any] = { - "success": success, - "order_id": _field(success_response, "order_id"), - "product_id": _field(success_response, "product_id", product_id), - "side": _field(success_response, "side", side_str), - "client_order_id": _field(success_response, "client_order_id", client_order_id), - "order_configuration": _decimal_map_order_configuration(raw_order_configuration), - "error": None, - } - if not success: - result["error"] = { - "error": _field(error_response, "error"), - "message": _field(error_response, "message"), - "error_details": _field(error_response, "error_details"), - "preview_failure_reason": _field(error_response, "preview_failure_reason"), - "new_order_failure_reason": _field(error_response, "new_order_failure_reason"), - } - return result + return PlaceResult( + success=success, + broker_order_id=_field(success_response, "order_id"), + reason=None + if success + else str( + _field(error_response, "message") + or _field(error_response, "error") + or "the venue refused the order without stating a reason" + ), + ) def get_order(self, order_id: str) -> dict: """Observed state of a previously placed order, normalized to `Decimal` money fields. diff --git a/keel/execution/executor.py b/keel/execution/executor.py index 8834ec0..7b44eed 100644 --- a/keel/execution/executor.py +++ b/keel/execution/executor.py @@ -102,7 +102,13 @@ from decimal import Decimal, InvalidOperation from typing import Any, Literal -from keel_broker_api.results import CancelOutcome, Preview, coerce_cancel_outcome +from keel_broker_api.orders import ( + BracketGTC, + MarketIOCByBase, + MarketIOCByQuote, + OrderSpec, +) +from keel_broker_api.results import CancelOutcome, PlaceResult, Preview, coerce_cancel_outcome from keel_core.products import quote_currency_of from keel_core.telemetry import log_event, log_exception, log_venue_failure @@ -360,7 +366,7 @@ def _base_increment_for( """The venue's finest acceptable `base_size` for `product_id`, cached, or `None` if unknown. `None` means UNKNOWN and, for a SELL, means "send the quantity unquantized" -- NOT "refuse". - See `_order_configuration`. This function therefore **never raises**: every failure path + See `_order_spec`. This function therefore **never raises**: every failure path (no broker in paper mode, a venue error, a malformed or absent field) returns `None`, and the exit proceeds exactly as it did before #516. @@ -531,7 +537,7 @@ def _build_intent( is_dca=False, rule_kind=signal.rule_name, rule_id=signal.rule_id, - # #516. Fetched here, like `available_quote` above, so `_order_configuration` stays a + # #516. Fetched here, like `available_quote` above, so `_order_spec` stays a # pure function of the intent. `None` is fine and means "send unquantized". base_increment=_base_increment_for(broker, repo, signal.product_id, now_ts), ) @@ -567,7 +573,7 @@ def _run_order( mode: str, confirm_fn: ConfirmFn | None, now_ts: int, - order_configuration: dict[str, Any] | None = None, + spec: OrderSpec | None = None, ) -> ExecutionResult: guard_result = guards.check(intent, repo, config, now_ts) if not guard_result.ok: @@ -587,7 +593,7 @@ def _run_order( reason="vetoed by guards: " + "; ".join(guard_result.violations), ) - if order_configuration is None: + if spec is None: # A size that cannot be expressed in the venue's units REFUSES THIS ORDER, and refuses # only this order (#513). `agent.run_once` does not wrap its `executor.execute` call, so # letting `SizePrecisionUnavailable` escape here would abort the whole cycle and skip @@ -595,7 +601,7 @@ def _run_order( # outage. Refuse-and-log is what every other unknown in this engine does; a raise here # would be the one that behaves differently. try: - order_configuration = _order_configuration(intent) + spec = _order_spec(intent) except SizePrecisionUnavailable as exc: log_event( logger, @@ -614,7 +620,7 @@ def _run_order( reason=f"size precision unavailable: {exc}", ) try: - preview = broker.preview_order(intent.product_id, intent.side, order_configuration) + preview = broker.preview_order(spec) except Exception: log_exception( logger, "executor.preview_failed", product=intent.product_id, side=intent.side @@ -643,7 +649,7 @@ def _run_order( # same way. So the moment the venue's own book -- the `best_ask` in the preview just # fetched, no extra call -- says the intended entry is materially off the market, say so # at WARNING, before the confirm gate and before placement. - _warn_if_market_routing_overrides_entry(intent, preview, order_configuration) + _warn_if_market_routing_overrides_entry(intent, preview, spec) # #350: THE ROUTING-TIME MAX-SPREAD ENTRY GATE. A live BUY whose book -- read from the # same preview the warning above just consumed -- is too wide (or unreadable) is refused @@ -685,7 +691,7 @@ def _run_order( order_id = repo.insert_order(_order_row(intent, mode, now_ts)) try: - place_result = broker.place_order(intent.product_id, intent.side, order_configuration) + place_result = broker.place_order(spec) except Exception: log_exception( logger, @@ -695,8 +701,8 @@ def _run_order( order_id=order_id, ) raise - success = bool(place_result.get("success")) - status = _initial_status(order_configuration) if success else "rejected" + success = place_result.success + status = _initial_status(spec) if success else "rejected" # `fee` was previously left NULL forever: nothing else in the live path ever wrote it, so # `streak.record_closed_trade` was always handed `fees=0` and every `pnl_net` was GROSS. # That defeats rail 16 precisely where it matters -- fees dominate small moves, so a trade @@ -707,13 +713,17 @@ def _run_order( # lines below (`_upgrade_to_observed_economics`) for an immediate fill, and by # `execution.reconcile` for an order that fills later. These remain the fallback when the # status endpoint is unavailable. - fee = preview.get("commission_total") if isinstance(preview, dict) else None + fee = preview.est_fee repo.update_order( order_id, status=status, actual_fill=intent.entry if status == "filled" else None, fee=fee if status == "filled" else None, - raw_response=json.dumps(place_result, default=str), + # The venue's own id for this order, which is what `_native_order_id` reads back to + # cancel it. It was `json.dumps(place_result)` -- the whole pre-port response dict -- and + # the id was dug out of that blob afterwards. `PlaceResult.broker_order_id` names it + # directly, so the column now stores the one field anything ever read from it. + raw_response=json.dumps({"order_id": place_result.broker_order_id}), updated_at=now_ts, ) @@ -728,14 +738,14 @@ def _run_order( product=intent.product_id, side=intent.side, order_id=order_id, - error=place_result.get("error"), + error=place_result.reason, ) return ExecutionResult( placed=False, order_id=order_id, vetoed_by=[], preview=preview, - reason=f"broker rejected order: {place_result.get('error')}", + reason=f"broker rejected order: {place_result.reason}", ) # `open_stop`/`open_target` are written by `place_bracket` ONLY, once the exchange has @@ -762,7 +772,7 @@ def _upgrade_to_observed_economics( broker: Any, repo: Repository, order_id: int, - place_result: dict[str, Any], + place_result: PlaceResult, now_ts: int, intent: OrderIntent | None = None, ) -> None: @@ -782,7 +792,7 @@ def _upgrade_to_observed_economics( get_order = getattr(broker, "get_order", None) if get_order is None: return - native_id = place_result.get("order_id") + native_id = place_result.broker_order_id if not native_id: return try: @@ -985,8 +995,8 @@ def _preview_best_ask(preview: Preview | dict[str, Any]) -> Decimal | None: def _warn_if_market_routing_overrides_entry( intent: OrderIntent, - preview: Preview | dict[str, Any] | None, - order_configuration: dict[str, Any] | None = None, + preview: Preview | None, + spec: OrderSpec | None = None, ) -> None: """WARNING, at routing time, when a BUY's intended entry is materially off the market. @@ -1006,7 +1016,7 @@ def _warn_if_market_routing_overrides_entry( Scoped to BUYs on the market configuration only. SELL intents (exits, brackets, stop rolls) either carry no entry condition or hand their prices to the venue verbatim, and a - caller passing its own non-market `order_configuration` -- a future resting order out of + caller passing its own non-market spec -- a future resting order out of #260's remediation plan -- is not on the override path at all. Never raises: telemetry must not be able to fail a routing. `deviation_bps` is signed, positive meaning the rule intended to enter ABOVE the market (follow-through demanded, pullback's case), negative @@ -1017,19 +1027,21 @@ def _warn_if_market_routing_overrides_entry( """ if preview is None or intent.side != Side.BUY: return - if order_configuration is None: - # Same resolution `_run_order` performs: no explicit configuration means the default - # routing, which today is always market. + if spec is None: + # Same resolution `_run_order` performs: no explicit spec means the default routing, + # which today is always market. try: - order_configuration = _order_configuration(intent) + spec = _order_spec(intent) except SizePrecisionUnavailable: # This function is a DIAGNOSTIC (it records how far the venue's book sat from the # intent). An order whose size cannot be serialised has already been refused # upstream, so there is nothing to measure and nothing to report -- and a diagnostic # must never be the thing that raises out of the order path. return - config_type = next(iter(order_configuration), "") - if not config_type.startswith("market_"): + # The KIND, off the spec, rather than the first key of a wire dict. `market_ioc_by_quote` + # and `market_ioc_by_base` are the two market kinds; a limit, stop or bracket spec is not on + # the override path. + if not spec.kind.startswith("market_"): return ref = _preview_best_ask(preview) if ref is None: @@ -1256,7 +1268,7 @@ class SizePrecisionUnavailable(RuntimeError): """ -def _order_configuration(intent: OrderIntent) -> dict[str, dict[str, str]]: +def _order_spec(intent: OrderIntent) -> OrderSpec: """Serialise the order's size at the VENUE's precision, not the engine's (#513). Everything upstream computes in full `Decimal` precision -- `sizing.size()` returns @@ -1297,8 +1309,12 @@ def _order_configuration(intent: OrderIntent) -> dict[str, dict[str, str]]: f"{intent.product_id!r} notional {intent.notional} quantizes to {notional} at " f"increment {increment} -- refusing to send a zero-size order" ) - return {"market_market_ioc": {"quote_size": str(notional)}} - return {"market_market_ioc": {"base_size": str(_sell_base_size(intent))}} + return MarketIOCByQuote( + product_id=intent.product_id, side=Side.BUY, quote_size=notional + ) + return MarketIOCByBase( + product_id=intent.product_id, side=Side.SELL, base_size=_sell_base_size(intent) + ) def _sell_base_size(intent: OrderIntent) -> Decimal: @@ -1335,12 +1351,24 @@ def _sell_base_size(intent: OrderIntent) -> Decimal: return quantized -def _initial_status(order_configuration: dict[str, Any]) -> str: - """A market (IOC) order fills immediately; a limit/stop-limit order rests as `pending` on - the exchange until a later fill event. `execution.reconcile`, run at the top of every - cycle, observes that fill and marks it `filled` with the observed price and fees.""" - config_type = next(iter(order_configuration), "") - return "filled" if config_type.startswith("market_") else "pending" +def _initial_status(spec: OrderSpec) -> str: + """A market (IOC) order fills immediately; a limit/stop-limit/bracket order rests as + `pending` on the exchange until a later fill event. `execution.reconcile`, run at the top of + every cycle, observes that fill and marks it `filled` with the observed price and fees. + + **Driven off `spec.kind`, and deliberately NOT off `spec.initial_status` (#524).** Every + `OrderSpec` carries an `initial_status` ClassVar, and reaching for it here is the obvious + move and the wrong one: the port's vocabulary is the VENUE's (`filled_or_rejected`, `open`) + and this column is KEEL's (`filled`, `pending`). They describe the same moment in different + words, and the words are not interchangeable -- `reconcile` sweeps for `pending`, so writing + `open` into this column would leave every resting order invisible to the sweep that exists to + observe its fill. A bracket recorded as `open` is a protective order keel would never look at + again. + + The dict inspection this replaced (`next(iter(order_configuration), "")`) is gone either way; + what stays is keel's own status vocabulary, mapped explicitly. + """ + return "filled" if spec.kind.startswith("market_") else "pending" class CancelUnavailable(RuntimeError): @@ -1436,8 +1464,12 @@ def _cancel_at_exchange(broker: Any, repo: Repository, order_row: dict[str, Any] def _native_order_id(order_row: dict[str, Any]) -> str | None: - """The broker-native order id for a repo order row, read back out of the `place_order` - response JSON stashed in `raw_response` -- used to cancel a specific broker order.""" + """The broker-native order id for a repo order row, read back out of `raw_response` -- used + to cancel a specific broker order. + + Rows written before #524 hold the entire pre-port placement response; rows written since hold + `{"order_id": ...}` alone. Both are read by the same `data.get("order_id")`, which is why the + narrowing needed no migration: the key that mattered is the key that was kept.""" raw = order_row.get("raw_response") if not raw: return None @@ -1448,41 +1480,46 @@ def _native_order_id(order_row: dict[str, Any]) -> str | None: return data.get("order_id") -def _bracket_order_configuration( - qty: Decimal, target: Decimal, stop: Decimal, base_increment: Decimal | None = None -) -> dict[str, dict[str, str]]: - """Coinbase's NATIVE trigger bracket: ONE order carrying both exit prices. - - `limit_price` is the take-profit and `stop_trigger_price` the stop-loss. The exchange owns - the race between them, which is the entire reason for using it: the previous design placed - two independent SELL legs and paired them client-side via `oco_sibling:` state, so a fill we - failed to observe left the other leg live and able to sell an already-closed position. That - whole failure mode does not exist here -- there is no sibling to cancel. - - It also fixes an inventory bug in that design: both legs were sized at the FULL qty, so a - 1x position was committed 2x. On spot the second leg should simply be rejected for - insufficient funds. - - `trigger_bracket_gtc` is exactly what `RESTClient.trigger_bracket_order_gtc` builds, so this - reaches it through the `place_order`/`create_order` path we already use -- no new broker API - surface, no new transport method. +def _bracket_spec( + product_id: str, + qty: Decimal, + target: Decimal, + stop: Decimal, + base_increment: Decimal | None = None, +) -> BracketGTC: + """The exit bracket as a port value: ONE order carrying both protective prices. + + **This used to render Coinbase's wire dict itself.** #502 stage 1 added `BracketGTC` to the + port and shipped a test pinning the two byte-identical, because two renderers of one order + existed and had to be kept in agreement. #524 removed the second one: the spec goes to + `CoinbaseClient.place_order`, which renders it through the adapter's own + `to_order_configuration`, and the parity test goes with the duplicate it was pinning. + + What the port adds beyond a shared renderer is REFUSAL. `BracketGTC.__post_init__` rejects a + non-positive price and a stop that does not sit on the losing side of the target -- equal legs + as firmly as inverted ones, because an equal-leg "bracket" is a stop and a target racing at + the same price. The dict this replaced checked none of that; `_roll_stop` grew its own guard + for the same hazard (#560) and now has a second line behind it. + + #516's quantization is unchanged and still happens HERE, before the spec is built: quantize + down when the increment is known, send unchanged when it is not. A bracket the venue refuses + leaves a position unprotected, so this path must never become more likely to fail than it was. """ - # 516: the protective legs carry a `base_size` too, and had the identical full-precision - # defect as a plain SELL. Same rule, same asymmetry: quantize DOWN when the increment is - # known, send unchanged when it is not. A bracket that the venue refuses leaves the position - # unprotected, so this path must never become more likely to fail than it is today. size = ( qty if base_increment is None or base_increment <= 0 else _floor_or_original(qty, base_increment) ) - return { - "trigger_bracket_gtc": { - "base_size": str(size), - "limit_price": str(target), - "stop_trigger_price": str(stop), - } - } + return BracketGTC( + product_id=product_id, + # A bracket keel places always EXITS a long: keel enters with a market IOC and protects + # afterwards, so the protective order is a SELL. `BracketGTC` derives the stop's trigger + # direction from this rather than taking it as a field. + side=Side.SELL, + base_size=size, + take_profit_price=target, + stop_trigger_price=stop, + ) def _floor_or_original(qty: Decimal, increment: Decimal) -> Decimal: @@ -1518,7 +1555,7 @@ def place_bracket( ) -> int | None: """Place the exchange-side exit bracket for an open long position, or `None` if vetoed. - ONE native trigger-bracket order (see `_bracket_order_configuration`), so the exchange owns + ONE native trigger-bracket order (see `_bracket_spec`), so the exchange owns the stop-vs-target race and the position is committed exactly once. It runs through `guards.check` like any other order (un-overridable). @@ -1548,8 +1585,8 @@ def place_bracket( "autonomous", None, now_ts, - order_configuration=_bracket_order_configuration( - qty, target, stop, _base_increment_for(broker, repo, product_id, now_ts) + spec=_bracket_spec( + product_id, qty, target, stop, _base_increment_for(broker, repo, product_id, now_ts) ), ) if not result.placed: @@ -1771,8 +1808,8 @@ def _roll_stop( "autonomous", None, now_ts, - order_configuration=_bracket_order_configuration( - qty, target, new_stop, _base_increment_for(broker, repo, product_id, now_ts) + spec=_bracket_spec( + product_id, qty, target, new_stop, _base_increment_for(broker, repo, product_id, now_ts) ), ) if not result.placed: diff --git a/tests/broker_coinbase/test_translate.py b/tests/broker_coinbase/test_translate.py index 0cb3be2..01fd8bd 100644 --- a/tests/broker_coinbase/test_translate.py +++ b/tests/broker_coinbase/test_translate.py @@ -96,26 +96,22 @@ def test_bracket_gtc_carries_no_stop_direction() -> None: assert "stop_direction" not in to_order_configuration(spec)["trigger_bracket_gtc"] -def test_bracket_gtc_is_byte_identical_to_what_the_executor_ships_today() -> None: - """Parity with the shipped, venue-accepted dict IS the contract for this kind. +# `test_bracket_gtc_is_byte_identical_to_what_the_executor_ships_today` stood here, and #524 +# deleted it along with the thing it was pinning. +# +# It existed because the tree carried TWO Coinbase order renderers: this translation, and +# `executor._bracket_order_configuration`, which built the same dict by hand for the live path. +# #502 stage 1 could not delete the second one -- the executor was not on the port yet -- so it +# pinned them byte-identical instead, and said so: "The test imports both; production code does +# not." +# +# The executor now builds a `BracketGTC` and hands it to `CoinbaseClient.place_order`, which +# renders it through THIS function. There is one renderer, so there is nothing left to hold in +# agreement, and a test comparing a function to itself would pass forever without saying anything. +# +# What the bracket's wire shape still owes is covered where it belongs: the cases below pin the +# three keys and the deliberate absence of `stop_direction`, and +# `tests/data/test_cb_client.py::test_place_order_renders_a_bracket_through_the_one_renderer` +# proves the live client sends exactly what this function returns. - `executor._bracket_order_configuration` is what Coinbase has actually been accepting on the - live path. The port's job here is to reach the same wire shape through a typed spec, not to - improve on it -- so this test pins the two together and will fail the moment either side - drifts. - The TEST imports both; production code must not. `keel_broker_coinbase` is a standalone - package that knows nothing about `keel.execution`, and the day Stage 2 switches the live - caller over, this assertion is what says the switch changed no bytes on the wire. - """ - from keel.execution.executor import _bracket_order_configuration - - qty, target, stop = Decimal("0.12345678"), Decimal("70123.45"), Decimal("60987.65") - spec = BracketGTC( - product_id="BTC-USD", - side=Side.SELL, - base_size=qty, - take_profit_price=target, - stop_trigger_price=stop, - ) - assert to_order_configuration(spec) == _bracket_order_configuration(qty, target, stop) diff --git a/tests/data/test_cb_client.py b/tests/data/test_cb_client.py index 2caec64..4f386c1 100644 --- a/tests/data/test_cb_client.py +++ b/tests/data/test_cb_client.py @@ -14,7 +14,9 @@ from typing import Any import pytest -from keel_broker_api.results import Balance, CancelOutcome, Instrument +from keel_broker_api.orders import BracketGTC, MarketIOCByQuote +from keel_broker_api.results import Balance, CancelOutcome, Instrument, PlaceResult, Preview +from keel_broker_coinbase.translate import to_order_configuration from keel_core import telemetry from keel.data.cb_client import CoinbaseClient @@ -322,184 +324,120 @@ def test_get_accounts_returns_list_of_dicts() -> None: assert all(isinstance(a, dict) for a in accounts) -# --- preview_order ------------------------------------------------------------------------ - +# --- preview_order / place_order: the PORT's shapes (#524) --------------------------------- +# +# These replace nine tests that asserted the pre-port dict contract -- `result["order_total"]`, +# `result["success"]`, `result["client_order_id"]`. That contract is gone: both methods take an +# `OrderSpec` and answer `Preview`/`PlaceResult`, and the wire configuration is rendered by +# `keel_broker_coinbase.translate.to_order_configuration` rather than built here. There is now +# ONE Coinbase order renderer in the tree; #502 stage 1 had shipped a test pinning two of them +# byte-identical, and that test goes with the duplicate it was holding in place. -def test_preview_order_maps_money_fields_to_decimal() -> None: - transport = FakeTransport(preview=_load_fixture("cb_preview_order.json")) - client = CoinbaseClient(transport) - result = client.preview_order( - "BTC-USD", - Side.BUY, - {"market_market_ioc": {"quote_size": "100.00"}}, +def _buy_spec(quote: str = "100.00") -> MarketIOCByQuote: + return MarketIOCByQuote( + product_id="BTC-USD", side=Side.BUY, quote_size=Decimal(quote) ) - assert result["order_total"] == Decimal("100.60") - assert result["commission_total"] == Decimal("0.60") - assert result["quote_size"] == Decimal("100.00") - assert result["base_size"] == Decimal("0.00152834") - assert isinstance(result["order_total"], Decimal) - assert result["errs"] == [] - - -def test_preview_order_passes_correct_params_to_transport() -> None: - transport = FakeTransport(preview=_load_fixture("cb_preview_order.json")) - client = CoinbaseClient(transport) - order_configuration = {"market_market_ioc": {"quote_size": "100.00"}} - - client.preview_order("BTC-USD", Side.BUY, order_configuration) - - assert transport.calls["preview_order"] == { - "product_id": "BTC-USD", - "side": "BUY", - "order_configuration": order_configuration, - } - -# --- place_order (Phase 3) ----------------------------------------------------------------- +def test_preview_order_answers_the_ports_type() -> None: + result = CoinbaseClient( + FakeTransport(preview=_load_fixture("cb_preview_order.json")) + ).preview_order(_buy_spec()) + assert isinstance(result, Preview) + assert result.est_fee == Decimal("0.60") + assert result.est_quote_size == Decimal("100.00") + assert result.est_base_size == Decimal("0.00152834") + assert result.errors == () + # A real quote from the venue, never an estimate this client computed. + assert result.synthetic is False -def test_place_order_market_maps_success_response() -> None: - transport = FakeTransport(placed=_load_fixture("cb_place_order_market.json")) - client = CoinbaseClient(transport) - result = client.place_order( - "BTC-USD", - Side.BUY, - {"market_market_ioc": {"quote_size": "100.00"}}, - ) +def test_preview_carries_the_book_in_detail_where_the_spread_gate_reads_it() -> None: + """#350's spread gate and #332's entry-override warning both read the book through + `executor._preview_book`, which takes it off `Preview.detail` as strings. A client that put + the book anywhere else would disable both without failing anything.""" + result = CoinbaseClient( + FakeTransport(preview=_load_fixture("cb_preview_order.json")) + ).preview_order(_buy_spec()) - assert result["success"] is True - assert result["order_id"] == "b1cd9a3b-4e5f-4a3c-9c8a-1f2e3d4c5b6a" - assert result["product_id"] == "BTC-USD" - assert result["side"] == "BUY" - assert result["client_order_id"] == "6a5e1e4a-7c8b-4d9e-9f0a-2b3c4d5e6f7a" - assert result["error"] is None - assert result["order_configuration"] == { - "market_market_ioc": {"quote_size": Decimal("100.00")} - } - assert isinstance(result["order_configuration"]["market_market_ioc"]["quote_size"], Decimal) + assert "best_bid" in result.detail + assert "best_ask" in result.detail + assert all(isinstance(v, str) for v in result.detail.values()) -def test_place_order_market_passes_market_config_through_to_transport() -> None: - transport = FakeTransport(placed=_load_fixture("cb_place_order_market.json")) - client = CoinbaseClient(transport) - order_configuration = {"market_market_ioc": {"quote_size": "100.00"}} - - client.place_order("BTC-USD", Side.BUY, order_configuration) - - call = transport.calls["create_order"] - assert call["product_id"] == "BTC-USD" - assert call["side"] == "BUY" - assert call["order_configuration"] == order_configuration - assert call["client_order_id"] # a client_order_id is always generated/passed - - -def test_place_order_limit_maps_success_response_and_decimal_fields() -> None: - transport = FakeTransport(placed=_load_fixture("cb_place_order_limit.json")) - client = CoinbaseClient(transport) - order_configuration = { - "limit_limit_gtc": { - "base_size": "0.00150000", - "limit_price": "66000.00", - "post_only": False, - } - } - - result = client.place_order("BTC-USD", Side.SELL, order_configuration) - - assert result["success"] is True - assert result["order_id"] == "c2de0b4c-5f60-4b5d-ad9b-2030415263f8" - assert result["side"] == "SELL" - limit_config = result["order_configuration"]["limit_limit_gtc"] - assert limit_config["base_size"] == Decimal("0.00150000") - assert limit_config["limit_price"] == Decimal("66000.00") - assert limit_config["post_only"] is False - assert isinstance(limit_config["base_size"], Decimal) - assert isinstance(limit_config["limit_price"], Decimal) - - call = transport.calls["create_order"] - assert call["order_configuration"] == order_configuration - assert call["side"] == "SELL" +def test_preview_renders_the_spec_through_the_adapters_translation() -> None: + """One renderer. The transport must receive exactly what the adapter would have sent.""" + transport = FakeTransport(preview=_load_fixture("cb_preview_order.json")) + spec = _buy_spec() + CoinbaseClient(transport).preview_order(spec) -def test_place_order_stop_limit_maps_decimal_fields() -> None: - transport = FakeTransport(placed=_load_fixture("cb_place_order_stop_limit.json")) - client = CoinbaseClient(transport) - order_configuration = { - "stop_limit_stop_limit_gtc": { - "base_size": "0.00150000", - "limit_price": "60000.00", - "stop_price": "61000.00", - "stop_direction": "STOP_DIRECTION_STOP_DOWN", - } + assert transport.calls["preview_order"] == { + "product_id": "BTC-USD", + "side": "BUY", + "order_configuration": to_order_configuration(spec), } - result = client.place_order("BTC-USD", Side.SELL, order_configuration) - stop_config = result["order_configuration"]["stop_limit_stop_limit_gtc"] - assert stop_config["base_size"] == Decimal("0.00150000") - assert stop_config["limit_price"] == Decimal("60000.00") - assert stop_config["stop_price"] == Decimal("61000.00") - assert stop_config["stop_direction"] == "STOP_DIRECTION_STOP_DOWN" +def test_place_order_answers_the_ports_type() -> None: + result = CoinbaseClient( + FakeTransport(placed=_load_fixture("cb_place_order_market.json")) + ).place_order(_buy_spec()) + assert isinstance(result, PlaceResult) + assert result.success is True + assert result.broker_order_id == "b1cd9a3b-4e5f-4a3c-9c8a-1f2e3d4c5b6a" + assert result.reason is None -def test_place_order_maps_error_response_when_not_successful() -> None: - transport = FakeTransport(placed=_load_fixture("cb_place_order_error.json")) - client = CoinbaseClient(transport) - result = client.place_order( - "BTC-USD", - Side.BUY, - {"market_market_ioc": {"quote_size": "100.00"}}, +def test_place_order_states_a_reason_when_the_venue_refuses() -> None: + """A refusal with no reason is the worst of both: the caller records `rejected` and the + operator learns nothing. The fallback sentence is deliberate rather than an empty string.""" + transport = FakeTransport( + placed={"success": False, "error_response": {"message": "insufficient funds"}} ) - assert result["success"] is False - assert result["order_id"] is None - assert result["error"] == { - "error": "INSUFFICIENT_FUND", - "message": "Insufficient balance in source account", - "error_details": "", - "preview_failure_reason": "PREVIEW_INSUFFICIENT_FUND", - "new_order_failure_reason": "INSUFFICIENT_FUND", - } + result = CoinbaseClient(transport).place_order(_buy_spec()) + + assert result.success is False + assert result.broker_order_id is None + assert result.reason == "insufficient funds" -def test_place_order_accepts_side_as_plain_string() -> None: +def test_place_order_mints_an_idempotency_key_per_attempt_unless_given_one() -> None: + """The port's contract: the key identifies the INTENT, not the order. Omit it and two + identical orders a strategy genuinely meant to place are two orders.""" transport = FakeTransport(placed=_load_fixture("cb_place_order_market.json")) client = CoinbaseClient(transport) - client.place_order("BTC-USD", "BUY", {"market_market_ioc": {"quote_size": "100.00"}}) - - assert transport.calls["create_order"]["side"] == "BUY" - - -def test_place_order_works_with_real_response_wrapper_types() -> None: - """`place_order` must also work when the transport returns the real typed - `CreateOrderResponse` from `coinbase-advanced-py` (not just a plain dict). - """ - from coinbase.rest.types.orders_types import CreateOrderResponse + client.place_order(_buy_spec()) + first = transport.calls["create_order"]["client_order_id"] + client.place_order(_buy_spec()) + second = transport.calls["create_order"]["client_order_id"] + assert first != second - raw = _load_fixture("cb_place_order_limit.json") - wrapped = CreateOrderResponse(dict(raw)) + client.place_order(_buy_spec(), idempotency_key="the-same-intent") + assert transport.calls["create_order"]["client_order_id"] == "the-same-intent" - class WrappedTransport: - def create_order(self, **kwargs: Any) -> CreateOrderResponse: - return wrapped - client = CoinbaseClient(WrappedTransport()) - result = client.place_order( - "BTC-USD", - Side.SELL, - {"limit_limit_gtc": {"base_size": "0.0015", "limit_price": "66000.00"}}, +def test_place_order_renders_a_bracket_through_the_one_renderer() -> None: + """The kind that had two renderers until #524, and the reason the parity test existed.""" + transport = FakeTransport(placed=_load_fixture("cb_place_order_market.json")) + spec = BracketGTC( + product_id="BTC-USD", + side=Side.SELL, + base_size=Decimal("0.01"), + take_profit_price=Decimal("53000"), + stop_trigger_price=Decimal("49000"), ) - assert result["success"] is True - assert result["order_id"] == "c2de0b4c-5f60-4b5d-ad9b-2030415263f8" - assert result["order_configuration"]["limit_limit_gtc"]["limit_price"] == Decimal( - "66000.00" - ) + CoinbaseClient(transport).place_order(spec) + + sent = transport.calls["create_order"]["order_configuration"] + assert sent == to_order_configuration(spec) + assert sent["trigger_bracket_gtc"]["stop_trigger_price"] == "49000" # --- zero network in tests ----------------------------------------------------------------- diff --git a/tests/execution/test_executor.py b/tests/execution/test_executor.py index 016c1b4..863bb0d 100644 --- a/tests/execution/test_executor.py +++ b/tests/execution/test_executor.py @@ -18,7 +18,8 @@ from typing import Any import pytest -from keel_broker_api.results import Balance +from keel_broker_api.orders import BracketGTC, LimitGTC, OrderSpec +from keel_broker_api.results import Balance, PlaceResult, Preview from keel_core.subscription import SubscriptionStatus from keel.config import ( @@ -53,6 +54,36 @@ # -- fakes ---------------------------------------------------------------------------------- +def _preview_from(spec: OrderSpec, payload: dict[str, Any]) -> Preview: + """A `Preview` from the dict shape these fakes have always described a quote in. + + Kept as a dict at the call sites deliberately: dozens of tests construct a bespoke preview to + exercise one degraded field -- a missing `best_bid`, a non-numeric `best_ask`, an `errs` list + -- and rewriting every one of them into a `Preview` constructor would have been a far larger + diff than the behaviour change it accompanies, with more chances to alter a case by accident. + This function is the one place the translation happens. + + The book goes into `detail` as STRINGS, which is what the port's `Preview` declares and what + `executor._preview_book` reads: the spread gate (#350) and the entry-override warning (#332) + both come through it, so a fake that carried the book anywhere else would silently stop + exercising two safety paths. + """ + return Preview( + product_id=spec.product_id, + side=spec.side, + est_base_size=Decimal(str(payload.get("base_size", "0"))), + est_quote_size=Decimal(str(payload.get("quote_size", "0"))), + est_fee=Decimal(str(payload.get("commission_total", "0"))), + synthetic=False, + detail={ + key: str(payload[key]) + for key in ("best_bid", "best_ask", "order_total") + if payload.get(key) is not None + }, + errors=tuple(str(e) for e in (payload.get("errs") or [])), + ) + + class FakeBroker: """Fake broker -- duck-types `CoinbaseClient.preview_order`/`.place_order`/`.cancel_order`. @@ -118,38 +149,18 @@ def get_balances(self) -> list[Balance]: Balance(currency="USDC", available=self._usdc_balance, total=self._usdc_balance), ] - def preview_order(self, product_id: str, side: Any, order_configuration: dict) -> dict: - self.preview_calls.append( - {"product_id": product_id, "side": side, "order_configuration": order_configuration} - ) - return dict(self._preview) + def preview_order(self, spec: OrderSpec) -> Preview: + self.preview_calls.append({"spec": spec}) + return _preview_from(spec, self._preview) - def place_order(self, product_id: str, side: Any, order_configuration: dict) -> dict: + def place_order(self, spec: OrderSpec, *, idempotency_key: str | None = None) -> PlaceResult: self._place_order_id_seq += 1 order_id = f"{self._place_order_id_prefix}-{self._place_order_id_seq}" self.events.append("place") - self.place_calls.append( - {"product_id": product_id, "side": side, "order_configuration": order_configuration} - ) + self.place_calls.append({"spec": spec}) if self._place_success: - return { - "success": True, - "order_id": order_id, - "product_id": product_id, - "side": side.value if isinstance(side, Side) else side, - "client_order_id": f"client-{order_id}", - "order_configuration": order_configuration, - "error": None, - } - return { - "success": False, - "order_id": None, - "product_id": product_id, - "side": side.value if isinstance(side, Side) else side, - "client_order_id": f"client-{order_id}", - "order_configuration": order_configuration, - "error": {"error": "INSUFFICIENT_FUND", "message": "no funds"}, - } + return PlaceResult(success=True, broker_order_id=order_id) + return PlaceResult(success=False, broker_order_id=None, reason="no funds") def cancel_order(self, order_id: str) -> bool: # Returns True: a CONFIRMED cancel. The real client returns bool and @@ -329,7 +340,7 @@ def _capture(preview: dict) -> bool: execute(signal, broker, repo, _config(), mode="confirm", confirm_fn=_capture, now_ts=NOW_TS) assert len(seen) == 1 - assert "order_total" in seen[0] + assert "order_total" in seen[0].detail # -- confirm mode: reject -> not placed --------------------------------------------------------- @@ -419,8 +430,8 @@ def test_rule_id_is_purely_additive_metadata_placement_and_guards_are_unchanged( assert len(broker_a.preview_calls) == len(broker_b.preview_calls) assert len(broker_a.place_calls) == len(broker_b.place_calls) assert ( - broker_a.place_calls[0]["order_configuration"] - == broker_b.place_calls[0]["order_configuration"] + broker_a.place_calls[0]["spec"] + == broker_b.place_calls[0]["spec"] ) order_a = repo.get_order(result_a.order_id) @@ -755,8 +766,8 @@ def test_live_execute_sizing_is_immune_to_reward_income_through_the_public_entry ) # The order actually sent to the venue is identical -- same sized base_size. assert ( - reward_broker.place_calls[0]["order_configuration"] - == clean_broker.place_calls[0]["order_configuration"] + reward_broker.place_calls[0]["spec"] + == clean_broker.place_calls[0]["spec"] ) @@ -874,7 +885,7 @@ def test_execute_attaches_oco_bracket_after_a_stop_target_entry_fills(repo): orders = repo.get_orders(product_id="BTC-USD") sell_orders = [o for o in orders if o["side"] == "SELL"] assert len(sell_orders) == 1 - assert "trigger_bracket_gtc" in broker.place_calls[-1]["order_configuration"] + assert isinstance(broker.place_calls[-1]["spec"], BracketGTC) assert repo.get_state("open_stop:BTC-USD") == Decimal("49000") assert repo.get_state("open_target:BTC-USD") == Decimal("53000") @@ -1392,8 +1403,8 @@ def test_a_partially_filled_entry_records_the_filled_quantity_and_warns(repo, ca assert "executor.entry_partially_filled" in caplog.text # Detect-and-surface, NOT auto-resize: the bracket is still placed for the ORDERED size # (resizing it is the amend-vs-replace decision #502 owns). - bracket = broker.place_calls[-1]["order_configuration"]["trigger_bracket_gtc"] - assert bracket["base_size"] == "1.000" + bracket = broker.place_calls[-1]["spec"] + assert bracket.base_size == Decimal("1.000") def test_a_fully_filled_entry_records_the_filled_quantity_without_warning(repo, caplog): @@ -1603,12 +1614,13 @@ def test_bracket_places_exactly_one_order_committing_the_position_once(repo): assert len(sells) == 1 assert len(broker.place_calls) == 1 - config = broker.place_calls[0]["order_configuration"] - assert "trigger_bracket_gtc" in config - leg = config["trigger_bracket_gtc"] - assert leg["base_size"] == "0.01" - assert leg["limit_price"] == "53000" # take-profit - assert leg["stop_trigger_price"] == "49000" # stop-loss + spec = broker.place_calls[0]["spec"] + assert isinstance(spec, BracketGTC) + assert spec.base_size == Decimal("0.01") + assert spec.take_profit_price == Decimal("53000") # take-profit + assert spec.stop_trigger_price == Decimal("49000") # stop-loss + # The port names the take-profit `take_profit_price`, not Coinbase's `limit_price` -- so a + # second venue's translation never starts from Coinbase's vocabulary (#521). def test_bracket_records_the_stop_for_rail_9_and_the_target_for_later_rolls(repo): @@ -1734,9 +1746,9 @@ def test_rolling_the_stop_carries_the_original_target_forward(repo): # bracket must be placed only AFTER the old one is cancelled, or the exchange would reject # it for insufficient funds (the resting bracket commits the whole position). assert broker.events == ["place", "cancel", "place"], broker.events - replacement = broker.place_calls[-1]["order_configuration"]["trigger_bracket_gtc"] - assert replacement["limit_price"] == "53000" # original target preserved - assert replacement["stop_trigger_price"] == "50000" # stop moved to break-even + replacement = broker.place_calls[-1]["spec"] + assert replacement.take_profit_price == Decimal("53000") # original target preserved + assert replacement.stop_trigger_price == Decimal("50000") # stop moved to break-even def test_a_roll_that_cannot_replace_the_bracket_screams_that_the_position_is_naked(repo, caplog): @@ -1748,11 +1760,11 @@ def __init__(self, **kw): super().__init__(**kw) self.calls = 0 - def place_order(self, product_id, side, order_configuration): + def place_order(self, spec, *, idempotency_key=None): # noqa: ANN001, ANN202 self.calls += 1 if self.calls > 1: # the original bracket places; the replacement fails - return {"success": False, "error": "INSUFFICIENT_FUND"} - return super().place_order(product_id, side, order_configuration) + return PlaceResult(success=False, broker_order_id=None, reason="INSUFFICIENT_FUND") + return super().place_order(spec, idempotency_key=idempotency_key) broker = _RejectingBroker() old_id = place_bracket( @@ -2691,7 +2703,14 @@ def test_an_explicitly_non_market_configuration_never_warns(self, caplog) -> Non market-override path -- its prices reach the venue, and this warning must not fire.""" from keel.execution.executor import _warn_if_market_routing_overrides_entry - resting = {"limit_limit_gtc": {"base_size": "0.001", "limit_price": "50300"}} + # A resting LIMIT spec, not the market routing -- the override warning is scoped to + # market orders, and a caller passing its own non-market spec is not on that path. + resting = LimitGTC( + product_id="BTC-USD", + side=Side.BUY, + base_size=Decimal("0.001"), + limit_price=Decimal("50300"), + ) with caplog.at_level(logging.WARNING): _warn_if_market_routing_overrides_entry( @@ -3146,11 +3165,11 @@ def __init__(self, **kw): super().__init__(**kw) self.calls = 0 - def place_order(self, product_id, side, order_configuration): + def place_order(self, spec, *, idempotency_key=None): # noqa: ANN001, ANN202 self.calls += 1 if self.calls > 1: # the original bracket places; the replacement fails - return {"success": False, "error": "INSUFFICIENT_FUND"} - return super().place_order(product_id, side, order_configuration) + return PlaceResult(success=False, broker_order_id=None, reason="INSUFFICIENT_FUND") + return super().place_order(spec, idempotency_key=idempotency_key) broker = _RejectingBroker() stop_id = place_bracket( diff --git a/tests/execution/test_reconcile.py b/tests/execution/test_reconcile.py index 21ff1fc..860cfa6 100644 --- a/tests/execution/test_reconcile.py +++ b/tests/execution/test_reconcile.py @@ -15,6 +15,8 @@ from typing import Any import pytest +from keel_broker_api.orders import OrderSpec +from keel_broker_api.results import Balance, PlaceResult, Preview from keel.config import Caps, Config, MarketDataConfig, MoneyMgmtConfig from keel.data.db import connect, migrate @@ -70,27 +72,28 @@ def __init__(self, orders: dict[str, dict[str, Any]] | None = None) -> None: super().__init__(orders) self.placed: list[dict[str, Any]] = [] - def get_accounts(self) -> list[dict[str, Any]]: - return [{"currency": "USDC", "available_balance": Decimal("1000000")}] - - def preview_order(self, product_id: str, side: Any, order_configuration: dict) -> dict: - return { - "order_total": Decimal("50"), - "commission_total": Decimal("0"), - "errs": [], - "warning": [], - # Both book sides, as the real venue returns them: #350's spread gate - # fails closed on a preview without them (reconcile places SELLs only, - # which the gate never touches -- this keeps the fake honest anyway). - "best_bid": Decimal("49990"), - "best_ask": Decimal("50000"), - } - - def place_order(self, product_id: str, side: Any, order_configuration: dict) -> dict: - self.placed.append( - {"product_id": product_id, "side": side, "order_configuration": order_configuration} + def get_balances(self) -> list[Balance]: + return [ + Balance(currency="USDC", available=Decimal("1000000"), total=Decimal("1000000")) + ] + + def preview_order(self, spec: OrderSpec) -> Preview: + return Preview( + product_id=spec.product_id, + side=spec.side, + est_base_size=Decimal("0"), + est_quote_size=Decimal("50"), + est_fee=Decimal("0"), + synthetic=False, + # Both book sides, as the real venue returns them: #350's spread gate fails closed on + # a preview without them (reconcile places SELLs only, which the gate never touches -- + # this keeps the fake honest anyway). + detail={"best_bid": "49990", "best_ask": "50000", "order_total": "50"}, ) - return {"success": True, "order_id": f"cb-re-{len(self.placed)}"} + + def place_order(self, spec: OrderSpec, *, idempotency_key: str | None = None) -> PlaceResult: + self.placed.append({"spec": spec}) + return PlaceResult(success=True, broker_order_id=f"cb-re-{len(self.placed)}") def _allow_orders(repo: Repository) -> None: @@ -862,9 +865,9 @@ def test_a_dead_bracket_on_a_held_position_is_replaced(repo): reconcile.reconcile_open_orders(broker, repo, _config(), now_ts=NOW) assert broker.placed, "no replacement bracket was placed" - leg = broker.placed[-1]["order_configuration"]["trigger_bracket_gtc"] - assert leg["stop_trigger_price"] == "49000" - assert leg["limit_price"] == "53000" + leg = broker.placed[-1]["spec"] + assert leg.stop_trigger_price == Decimal("49000") + assert leg.take_profit_price == Decimal("53000") # The tranche must now name the REPLACEMENT. Leaving it on the dead order is silent data # loss: the replacement's eventual fill would resolve to no tranche, take the # "exit without position context" skip, and close the position with no `trade_outcomes` row. @@ -1118,11 +1121,11 @@ class _RejectingRebracketBroker(_RebracketingBroker): """Placement reaches the exchange and comes back refused -- min-size, precision, a venue error. The reachable cause of a never-placed bracket, and the one no rail can prevent.""" - def place_order(self, product_id: str, side: Any, order_configuration: dict) -> dict: - self.placed.append( - {"product_id": product_id, "side": side, "order_configuration": order_configuration} + def place_order(self, spec: OrderSpec, *, idempotency_key: str | None = None) -> PlaceResult: + self.placed.append({"spec": spec}) + return PlaceResult( + success=False, broker_order_id=None, reason="PREVIEW_INVALID_BASE_SIZE" ) - return {"success": False, "error": "PREVIEW_INVALID_BASE_SIZE"} def _seed_unbracketed_tranche( @@ -1192,10 +1195,10 @@ def test_a_tranche_whose_bracket_was_never_placed_is_bracketed_next_cycle(repo): reconcile.reconcile_unbracketed_positions(broker, repo, _config(), now_ts=NOW) assert broker.placed, "the unprotected tranche was left without a bracket" - leg = broker.placed[-1]["order_configuration"]["trigger_bracket_gtc"] + leg = broker.placed[-1]["spec"] # The levels the ORIGINAL trade was risk-sized against, not invented ones. - assert leg["stop_trigger_price"] == "49000" - assert leg["limit_price"] == "53000" + assert leg.stop_trigger_price == Decimal("49000") + assert leg.take_profit_price == Decimal("53000") placed_id = repo.get_orders(mode="live", product_id=PRODUCT, status="pending")[-1]["id"] owner = repo.get_position_for_bracket(placed_id) diff --git a/tests/execution/test_sell_precision.py b/tests/execution/test_sell_precision.py index cb89dd0..898efbe 100644 --- a/tests/execution/test_sell_precision.py +++ b/tests/execution/test_sell_precision.py @@ -16,8 +16,8 @@ from keel.execution.executor import ( _base_increment_for, - _bracket_order_configuration, - _order_configuration, + _bracket_spec, + _order_spec, _sell_base_size, ) from keel.execution.guards import OrderIntent @@ -53,8 +53,8 @@ def test_unknown_increment_sends_unquantized_and_does_not_refuse() -> None: least sometimes works -- a round quantity is accepted -- so refusing would replace "sometimes exits" with "never exits". """ - config = _order_configuration(_sell(increment=None)) - assert config == {"market_market_ioc": {"base_size": str(MESSY_QTY)}} + config = _order_spec(_sell(increment=None)) + assert config.base_size == MESSY_QTY def test_a_buy_refuses_where_a_sell_sends() -> None: @@ -72,10 +72,10 @@ def test_a_buy_refuses_where_a_sell_sends() -> None: rule_kind="turtle_breakout", ) with pytest.raises(SizePrecisionUnavailable): - _order_configuration(buy) + _order_spec(buy) sell = _sell(increment=None) - assert _order_configuration(sell)["market_market_ioc"]["base_size"] == str(MESSY_QTY) + assert _order_spec(sell).base_size == MESSY_QTY @pytest.mark.parametrize("increment", [None, Decimal("0"), Decimal("-1")]) @@ -111,25 +111,25 @@ def test_a_quantity_smaller_than_one_increment_is_sent_unchanged() -> None: def test_bracket_quantizes_its_base_size_when_the_increment_is_known() -> None: - config = _bracket_order_configuration( - MESSY_QTY, Decimal("0.25"), Decimal("0.18"), Decimal("0.000001") + config = _bracket_spec( + "XLM-USD", MESSY_QTY, Decimal("0.25"), Decimal("0.18"), Decimal("0.000001") ) - assert config["trigger_bracket_gtc"]["base_size"] == "114.011787" + assert config.base_size == Decimal("114.011787") def test_bracket_sends_unquantized_when_the_increment_is_unknown() -> None: """A bracket the venue refuses leaves the position UNPROTECTED -- never make this stricter.""" - config = _bracket_order_configuration(MESSY_QTY, Decimal("0.25"), Decimal("0.18"), None) - assert config["trigger_bracket_gtc"]["base_size"] == str(MESSY_QTY) + config = _bracket_spec("XLM-USD", MESSY_QTY, Decimal("0.25"), Decimal("0.18"), None) + assert config.base_size == MESSY_QTY def test_bracket_prices_are_untouched() -> None: """Only the SIZE is quantized here. Prices have their own increment and are not in scope.""" - config = _bracket_order_configuration( - Decimal("1"), Decimal("0.25"), Decimal("0.18"), Decimal("0.01") + config = _bracket_spec( + "XLM-USD", Decimal("1"), Decimal("0.25"), Decimal("0.18"), Decimal("0.01") ) - assert config["trigger_bracket_gtc"]["limit_price"] == "0.25" - assert config["trigger_bracket_gtc"]["stop_trigger_price"] == "0.18" + assert config.take_profit_price == Decimal("0.25") + assert config.stop_trigger_price == Decimal("0.18") # -- the cached lookup ------------------------------------------------------------------------- diff --git a/tests/execution/test_size_precision.py b/tests/execution/test_size_precision.py index 60784c1..fefc7ae 100644 --- a/tests/execution/test_size_precision.py +++ b/tests/execution/test_size_precision.py @@ -13,7 +13,7 @@ import pytest from keel.execution import sizing -from keel.execution.executor import SizePrecisionUnavailable, _order_configuration +from keel.execution.executor import SizePrecisionUnavailable, _order_spec from keel.execution.guards import OrderIntent from keel.types import Side @@ -109,19 +109,19 @@ def test_quote_increment_answers_about_the_currency_not_the_instrument_shape() - assert sizing.quote_increment_for("BTC-PERP-USD") == Decimal("0.01") -# -- _order_configuration: the regression --------------------------------------------------- +# -- _order_spec: the regression --------------------------------------------------- def test_the_rejected_order_now_serialises_to_two_decimals() -> None: """Regression on the real payload. This exact string was answered INVALID_SIZE_PRECISION.""" - config = _order_configuration(_buy()) - assert config == {"market_market_ioc": {"quote_size": "23.00"}} + config = _order_spec(_buy()) + assert config.quote_size == Decimal("23.00") def test_serialised_quote_size_never_exceeds_the_authorised_notional() -> None: """The rails approved `notional`; the wire value must not be larger than what they passed.""" intent = _buy(notional=Decimal("23.999999999")) - sent = Decimal(_order_configuration(intent)["market_market_ioc"]["quote_size"]) + sent = Decimal(_order_spec(intent).quote_size) assert sent <= intent.notional @@ -131,18 +131,18 @@ def test_dca_style_round_notional_is_unchanged() -> None: Orders 1 and 2 sent 26 decimal places and filled, because the VALUE was exactly 50. """ intent = _buy(product_id="BTC-USD", notional=Decimal("50.00000000000000000000000000")) - assert _order_configuration(intent) == {"market_market_ioc": {"quote_size": "50.00"}} + assert _order_spec(intent).quote_size == Decimal("50.00") def test_unknown_increment_refuses_rather_than_guessing() -> None: with pytest.raises(SizePrecisionUnavailable, match="no quote increment known"): - _order_configuration(_buy(product_id="BTC-XYZ")) + _order_spec(_buy(product_id="BTC-XYZ")) def test_a_notional_that_quantizes_to_zero_is_refused() -> None: """A size rounded to nothing must never be sent as an order.""" with pytest.raises(SizePrecisionUnavailable, match="zero-size order"): - _order_configuration(_buy(notional=Decimal("0.004"))) + _order_spec(_buy(notional=Decimal("0.004"))) def test_sell_is_deliberately_untouched_pending_base_increments() -> None: @@ -162,4 +162,4 @@ def test_sell_is_deliberately_untouched_pending_base_increments() -> None: is_dca=False, rule_kind="turtle_breakout", ) - assert _order_configuration(sell) == {"market_market_ioc": {"base_size": str(intent.qty)}} + assert _order_spec(sell).base_size == intent.qty diff --git a/tests/test_agent.py b/tests/test_agent.py index ebe24b8..afbab80 100644 --- a/tests/test_agent.py +++ b/tests/test_agent.py @@ -20,7 +20,14 @@ from typing import Any import pytest -from keel_broker_api.results import Balance, MarketSchedule, SessionState +from keel_broker_api.orders import OrderSpec +from keel_broker_api.results import ( + Balance, + MarketSchedule, + PlaceResult, + Preview, + SessionState, +) from keel_core.telemetry import _FIELDS_ATTR from keel import agent @@ -64,6 +71,8 @@ def __init__(self, series: dict[tuple[str, Granularity], list[Candle]] | None = self.preview_calls: list[dict[str, Any]] = [] self.place_calls: list[dict[str, Any]] = [] self._order_seq = 0 + # The previewed fee, overridable by a subclass that means to test fee splitting. + self.commission = Decimal("0") def get_balances(self) -> list[Balance]: """Comfortable balances -- rail 13 fails closed otherwise. Both legs are funded because @@ -80,34 +89,25 @@ def get_candles( series = self._series.get((product_id, granularity), []) return [c for c in series if start <= c.ts <= end] - def preview_order(self, product_id: str, side: Any, order_configuration: dict) -> dict: - self.preview_calls.append({"product_id": product_id, "side": side}) - return { - "order_total": Decimal("50.00"), - "commission_total": Decimal("0"), - "errs": [], - "warning": [], - # Both book sides, as the real venue returns them: #350's spread gate fails - # closed on a preview without them (tests that mean a degraded/bookless - # response pass their own preview dict). - "best_bid": Decimal("99.95"), - "best_ask": Decimal("100"), - } + def preview_order(self, spec: OrderSpec) -> Preview: + self.preview_calls.append({"product_id": spec.product_id, "side": spec.side}) + return Preview( + product_id=spec.product_id, + side=spec.side, + est_base_size=Decimal("0"), + est_quote_size=Decimal("50.00"), + est_fee=self.commission, + synthetic=False, + # Both book sides, as the real venue returns them: #350's spread gate fails closed on + # a preview without them (tests that mean a degraded/bookless response build their + # own Preview). + detail={"best_bid": "99.95", "best_ask": "100", "order_total": "50.00"}, + ) - def place_order(self, product_id: str, side: Any, order_configuration: dict) -> dict: + def place_order(self, spec: OrderSpec, *, idempotency_key: str | None = None) -> PlaceResult: self._order_seq += 1 - order_id = f"broker-order-{self._order_seq}" - self.place_calls.append({"product_id": product_id, "side": side}) - side_str = side.value if isinstance(side, Side) else side - return { - "success": True, - "order_id": order_id, - "product_id": product_id, - "side": side_str, - "client_order_id": f"client-{order_id}", - "order_configuration": order_configuration, - "error": None, - } + self.place_calls.append({"product_id": spec.product_id, "side": spec.side}) + return PlaceResult(success=True, broker_order_id=f"broker-order-{self._order_seq}") def cancel_order(self, order_id: str) -> bool: return True # a CONFIRMED cancel -- see `_cancel_at_exchange` @@ -1673,10 +1673,9 @@ def test_a_rule_exit_apportions_the_exit_fee_across_tranches(repo: Repository) - ) class _FeeBroker(FakeBroker): - def preview_order(self, product_id: str, side: Any, order_configuration: dict) -> dict: - preview = super().preview_order(product_id, side, order_configuration) - preview["commission_total"] = Decimal("4.00") - return preview + def __init__(self, **kwargs: Any) -> None: + super().__init__(**kwargs) + self.commission = Decimal("4.00") repo.set_state(f"position_rule:{PRODUCT}", {"rule_name": "fake_exit", "opened_at": 1_000}) broker = _FeeBroker(series={(PRODUCT, Granularity.ONE_DAY): [_candle(0, "100")]}) @@ -2483,14 +2482,19 @@ def test_confirm_fn_sees_the_broker_preview(repo, monkeypatch): _live_ready_repo(repo) _seed_rule(repo, monkeypatch, _AlwaysEnterRule(PRODUCT), status="live") broker = FakeBroker(series={(PRODUCT, Granularity.ONE_DAY): [_candle(0, "100")]}) - seen = {} + seen: list[Preview] = [] - def _capture(preview): - seen.update(preview) + def _capture(preview): # noqa: ANN001, ANN202 + seen.append(preview) return True run_once(broker, repo, _live_config(), now_ts=90_000, confirm_fn=_capture) - assert "order_total" in seen # the preview the operator would be shown + # The operator is shown the port's `Preview` since #524, not a venue dict. `order_total` is + # still there -- it moved into `detail`, which is where the port carries the venue's own + # figures as strings. + assert len(seen) == 1 + assert isinstance(seen[0], Preview) + assert "order_total" in seen[0].detail def test_a_rail_veto_means_the_confirm_prompt_is_never_reached(repo, monkeypatch): diff --git a/tests/test_cli.py b/tests/test_cli.py index d00507b..abc6cd2 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -19,7 +19,8 @@ from typing import Any from click.testing import CliRunner -from keel_broker_api.results import SessionState +from keel_broker_api.orders import OrderSpec +from keel_broker_api.results import PlaceResult, Preview, SessionState import keel.cli as cli_module from keel import agent @@ -51,28 +52,21 @@ def get_candles(self, product_id: str, granularity: Any, start: int, end: int) - self.get_candles_calls.append((product_id, granularity, start, end)) return [] - def preview_order(self, product_id: str, side: Any, order_configuration: dict) -> dict: - return { - "order_total": Decimal("50.00"), - "commission_total": Decimal("0"), - "errs": [], - "warning": [], - # Both book sides, as the real venue returns them: #350's spread gate fails - # closed on a preview without them. - "best_bid": Decimal("99.95"), - "best_ask": Decimal("100"), - } - - def place_order(self, product_id: str, side: Any, order_configuration: dict) -> dict: - return { - "success": True, - "order_id": "fake-order-1", - "product_id": product_id, - "side": side.value if hasattr(side, "value") else side, - "client_order_id": "fake-client-1", - "order_configuration": order_configuration, - "error": None, - } + def preview_order(self, spec: OrderSpec) -> Preview: + return Preview( + product_id=spec.product_id, + side=spec.side, + est_base_size=Decimal("0"), + est_quote_size=Decimal("50.00"), + est_fee=Decimal("0"), + synthetic=False, + # Both book sides, as the real venue returns them: #350's spread gate fails closed on + # a preview without them. + detail={"best_bid": "99.95", "best_ask": "100", "order_total": "50.00"}, + ) + + def place_order(self, spec: OrderSpec, *, idempotency_key: str | None = None) -> PlaceResult: + return PlaceResult(success=True, broker_order_id="fake-order-1") def cancel_order(self, order_id: str) -> bool: return True # a CONFIRMED cancel -- see `_cancel_at_exchange`