From 4584ec0080e0383692d4eee519d82e956c19fabe Mon Sep 17 00:00:00 2001 From: Elmehdi Aitbrahim Date: Wed, 26 Aug 2026 19:50:44 -0400 Subject: [PATCH] feat(executor): the quote balance comes from the port's shape, and only `available` counts MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #524's second blocker. Like the first, this changes no live behaviour -- the same numbers reach rail 13 by the same path -- and it removes the dual-shape probe the issue lists as part of the payoff. ── ONE QUESTION, ONE ANSWER ─────────────────────────────────────────────────── `_fetch_available_quote` called `broker.get_accounts()` and then probed each row TWICE -- dict key or attribute, `currency` or `.currency`, `available_balance` or `.available_balance` -- because it did not know whether it held the pre-port `CoinbaseClient` or a port adapter. The port answers `list[Balance]`; the client answered venue-shaped dicts. `CoinbaseClient.get_balances()` now answers in the port's type as well, computing `total` as `available + hold` exactly as `keel_broker_coinbase.adapter` does -- Coinbase exposes no single "total" field, and two implementations of one word must not disagree while both exist. The executor asks one question and the fork is gone. When `_build_broker` finally resolves through `load_broker`, this path needs no further change. `get_accounts` stays for `keel assets holdings` (`cli.py:557`), which reads it through `gather_holdings`. Moving that is the flip's business, not this blocker's. ── A GAP THE MIGRATION EXPOSED, AND THE TEST THAT NOW HOLDS IT ──────────────── `Balance` carries `available` AND `total`, which the dict shape did not distinguish. Swapping `.available` for `.total` in `_fetch_available_quote` PASSED THE ENTIRE SUITE -- verified by making that change -- because every fake in the repository sets the two equal. They are not the same number. `available` is what the venue will let an order draw on; `total` includes funds on hold: settling proceeds, collateral behind a resting order. Reading `total` would let rail 13 pass an order the account cannot fund -- precisely the failure the rail exists to prevent -- and silently, because both are plausible balances. Pinned now with a balance whose figures differ (100 available of 1000 total), plus the case-insensitive currency match and the no-account-for-this-currency case that `None` means. The mutation that passed before fails now. Gates: 4298 passed / 3 skipped, ruff clean, mypy clean across 347 files. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01T6yA5khYnJ2qzheArRToQ2 --- keel/data/cb_client.py | 38 ++++++++++++++- keel/execution/executor.py | 30 +++++------- tests/data/test_cb_client.py | 59 ++++++++++++++++++++++- tests/execution/test_executor.py | 83 ++++++++++++++++++++++++++------ tests/test_agent.py | 44 ++++++++--------- 5 files changed, 196 insertions(+), 58 deletions(-) diff --git a/keel/data/cb_client.py b/keel/data/cb_client.py index 9b32f53..e624d8f 100644 --- a/keel/data/cb_client.py +++ b/keel/data/cb_client.py @@ -26,7 +26,7 @@ from decimal import Decimal from typing import Any, Protocol -from keel_broker_api.results import CancelOutcome +from keel_broker_api.results import Balance, CancelOutcome from keel_core.telemetry import log_exception, log_venue_failure from keel.types import Candle, Granularity, Side @@ -233,6 +233,42 @@ def get_accounts(self) -> list[dict]: ) return accounts + def get_balances(self) -> list[Balance]: + """The same read as `get_accounts`, in the PORT's shape (#524). + + This client predates `keel-broker-api` and its `get_accounts` returns venue-shaped dicts + (`available_balance`, plus `uuid`/`default`/`active` nobody reads). The port's answer is + `list[Balance]` -- `currency`, `available`, `total` -- and `executor._fetch_available_quote` + had to probe for BOTH shapes, dict key or attribute, because it did not know which kind of + broker it held. + + Teaching this client the port's shape removes that fork without flipping anything: the + executor now asks one question, and the answer is the same type whether it is talking to + this pre-port client or to a real adapter. When `_build_broker` finally resolves through + `load_broker`, this path needs no further change. + + `total` is `available + hold`, matching `keel_broker_coinbase.adapter.get_balances` + exactly -- Coinbase exposes no single "total" field, and the two implementations must not + disagree about what the word means while both exist. + """ + try: + response = self._transport.get_accounts() + except Exception: + log_venue_failure(logger, "cb_client.accounts_fetch_failed") + raise + balances: list[Balance] = [] + for raw in _field(response, "accounts", []) or []: + available = Decimal(_field(_field(raw, "available_balance") or {}, "value", "0")) + hold = Decimal(_field(_field(raw, "hold") or {}, "value", "0")) + balances.append( + Balance( + currency=str(_field(raw, "currency", "")), + available=available, + total=available + hold, + ) + ) + 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.""" response = self._transport.preview_order( diff --git a/keel/execution/executor.py b/keel/execution/executor.py index 18fb9bf..5ff4d24 100644 --- a/keel/execution/executor.py +++ b/keel/execution/executor.py @@ -63,7 +63,7 @@ `trade_outcomes` row so rail 16 does not count a scaled-out winner as a loss). **USDC-funding balance (rail 13, Issue #59).** For a BUY `_build_intent` fetches the live -available balance of the PRODUCT's quote leg from `broker.get_accounts()` and hands +available balance of the PRODUCT's quote leg from `broker.get_balances()` and hands it to `guards.check` via `OrderIntent.available_quote` -- guards itself has no broker access, by design. This happens *before* `guards.check` runs (the balance is an input to the rail, not something guarded itself), so it is the one broker call this module makes ahead of the guard @@ -411,7 +411,7 @@ def _coerce_increment(raw: object) -> Decimal | None: def _fetch_available_quote(broker: Any, quote_currency: str | None) -> Decimal | None: - """Live available balance of `quote_currency` from `broker.get_accounts()`. + """Live available balance of `quote_currency`, from the port's `get_balances()`. `quote_currency` is the **product's own settlement leg** (`BTC-USD` -> `USD`), not `config.quote_currency`: the currency an order spends is a property of the product. Checking @@ -432,7 +432,7 @@ def _fetch_available_quote(broker: Any, quote_currency: str | None) -> Decimal | # that is expected and already handled (rail 13 is skipped offline). return None try: - accounts = broker.get_accounts() + balances = broker.get_balances() except Exception: # `log_venue_failure`, not `log_exception`: an unreachable venue outside a trade cycle # is a dashboard balance refresh on a sleeping laptop, and this line is the SECOND @@ -444,22 +444,14 @@ def _fetch_available_quote(broker: Any, quote_currency: str | None) -> Decimal | log_venue_failure(logger, "executor.quote_fetch_failed", quote_currency=quote_currency) return None - for account in accounts or []: - currency = ( - account.get("currency") - if isinstance(account, dict) - else getattr(account, "currency", None) - ) - if (currency or "").upper() != quote_currency.upper(): - continue - balance = ( - account.get("available_balance") - if isinstance(account, dict) - else getattr(account, "available_balance", None) - ) - if balance is None: - return None - return balance if isinstance(balance, Decimal) else Decimal(str(balance)) + # ONE shape, since #524. This probed for two -- a dict key or an attribute, `currency` or + # `.currency`, `available_balance` or `.available_balance` -- because it did not know whether + # it held the pre-port `CoinbaseClient` or a port adapter. `CoinbaseClient.get_balances` now + # answers in the port's `Balance` type as well, so there is one question and one answer, and + # the fork that existed only to bridge them is gone. + for balance in balances or []: + if balance.currency.upper() == quote_currency.upper(): + return balance.available return None diff --git a/tests/data/test_cb_client.py b/tests/data/test_cb_client.py index e55f079..2ae19bf 100644 --- a/tests/data/test_cb_client.py +++ b/tests/data/test_cb_client.py @@ -14,7 +14,7 @@ from typing import Any import pytest -from keel_broker_api.results import CancelOutcome +from keel_broker_api.results import Balance, CancelOutcome from keel_core import telemetry from keel.data.cb_client import CoinbaseClient @@ -205,6 +205,63 @@ def test_get_spot_returns_decimal() -> None: assert transport.calls["get_product"] == {"product_id": "BTC-USD"} +# --- get_balances (the port's shape, #524) ------------------------------------------------ + + +def test_get_balances_answers_the_ports_type() -> None: + """`Balance`, not this client's account dicts. + + The point of the method: `executor._fetch_available_quote` used to probe for a dict key OR an + attribute because it did not know whether it held this pre-port client or a real adapter. + Answering in the port's own type removes the fork -- one question, one shape, whichever kind + of broker is on the other end. + """ + client = CoinbaseClient(FakeTransport(accounts=_load_fixture("cb_accounts.json"))) + + balances = client.get_balances() + + assert all(isinstance(b, Balance) for b in balances) + btc = next(b for b in balances if b.currency == "BTC") + assert btc.available == Decimal("0.53219871") + usd = next(b for b in balances if b.currency == "USD") + assert usd.available == Decimal("1042.55") + + +def test_get_balances_totals_available_plus_hold_like_the_adapter_does() -> None: + """Coinbase exposes no single "total" field, so both implementations compute it -- and they + must not disagree about what the word means while both exist. + `keel_broker_coinbase.adapter.get_balances` sums `available_balance` and `hold`; so does this. + """ + transport = FakeTransport( + accounts={ + "accounts": [ + { + "currency": "USD", + "available_balance": {"value": "100.25"}, + "hold": {"value": "9.75"}, + } + ] + } + ) + + balance = CoinbaseClient(transport).get_balances()[0] + + assert balance.available == Decimal("100.25") + assert balance.total == Decimal("110.00") + + +def test_get_balances_reraises_an_unreachable_venue() -> None: + """Rail 13 fails closed on the exception itself, so this must not swallow it -- the same + contract `get_accounts` keeps.""" + + class _Down: + def get_accounts(self, **_: object) -> object: + raise ConnectionError("venue unreachable") + + with pytest.raises(ConnectionError): + CoinbaseClient(_Down()).get_balances() # type: ignore[arg-type] + + # --- get_accounts ------------------------------------------------------------------------- diff --git a/tests/execution/test_executor.py b/tests/execution/test_executor.py index f17d09c..016c1b4 100644 --- a/tests/execution/test_executor.py +++ b/tests/execution/test_executor.py @@ -18,6 +18,7 @@ from typing import Any import pytest +from keel_broker_api.results import Balance from keel_core.subscription import SubscriptionStatus from keel.config import ( @@ -95,20 +96,26 @@ def __init__( # Ordered log of exchange interactions -- lets a test assert SEQUENCE, not just that a # call happened. `_roll_stop` must cancel before it places. self.events: list[str] = [] - self.get_accounts_calls = 0 + self.get_balances_calls = 0 - def get_accounts(self) -> list[dict[str, Any]]: - self.get_accounts_calls += 1 + def get_balances(self) -> list[Balance]: + """The port's shape since #524. + + The "no balance known" case is an EMPTY LIST rather than a row carrying `None`. That is + not a shortcut around `Balance` requiring a `Decimal`: in the port's model a currency + with no account simply is not in the list, and `_fetch_available_quote` returns `None` + for it either way -- by falling off the loop instead of by testing a null field. + """ + self.get_balances_calls += 1 if self._balances is not None: - return [{"currency": c, "available_balance": b} for c, b in self._balances.items()] - if self._usdc_balance is None: return [ - {"currency": "USD", "available_balance": None}, - {"currency": "USDC", "available_balance": None}, + Balance(currency=c, available=b, total=b) for c, b in self._balances.items() ] + if self._usdc_balance is None: + return [] return [ - {"currency": "USD", "available_balance": self._usdc_balance}, - {"currency": "USDC", "available_balance": self._usdc_balance}, + Balance(currency="USD", available=self._usdc_balance, total=self._usdc_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: @@ -487,8 +494,8 @@ class _BrokerAccountsError: or placing. """ - def get_accounts(self) -> list[dict[str, Any]]: - raise ConnectionError("simulated broker outage fetching accounts") + def get_balances(self) -> list[Balance]: + raise ConnectionError("simulated broker outage fetching balances") def preview_order(self, *args: Any, **kwargs: Any) -> Any: raise AssertionError("preview_order must not be called when the balance fetch failed") @@ -504,7 +511,7 @@ def test_execute_fetches_the_available_quote_balance_for_a_buy_and_places(repo): result = execute(signal, broker, repo, _config(), mode="autonomous", now_ts=NOW_TS) assert result.placed is True - assert broker.get_accounts_calls == 1 + assert broker.get_balances_calls == 1 def test_broker_balance_fetch_error_vetoes_the_buy_before_preview_or_place(repo): @@ -551,7 +558,7 @@ def test_exit_signal_never_fetches_a_balance(repo): result = execute(signal, broker, repo, _config(), mode="autonomous", now_ts=NOW_TS) assert result.placed is True - assert broker.get_accounts_calls == 0 + assert broker.get_balances_calls == 0 # -- autonomous mode: compliant -> placed without a prompt @@ -2211,12 +2218,12 @@ def test_no_account_at_all_for_the_required_currency_fails_closed(repo): class _UnreachableBroker: - """A broker whose `get_accounts` raises as an offline HTTP stack does.""" + """A broker whose `get_balances` raises as an offline HTTP stack does.""" def __init__(self, exc: BaseException) -> None: self._exc = exc - def get_accounts(self) -> list[dict]: + def get_balances(self) -> list[Balance]: raise self._exc @@ -2233,6 +2240,52 @@ def _quote_failure_payload(caplog, exc: BaseException) -> tuple[Decimal | None, return result, json.loads(formatter.format(records[0])) +def test_the_quote_read_takes_available_and_never_total() -> None: + """**Rail 13 is about SPENDABLE funds, and `Balance` carries two numbers that differ.** + + `available` is what the venue will let an order draw on; `total` includes funds on hold -- + settling proceeds, collateral behind a resting order. Reading `total` would let rail 13 pass + an order the account cannot actually fund, which is the precise failure the rail exists to + prevent, and it would do it silently because both numbers are plausible balances. + + Pinned with a balance whose two figures DIFFER. Every other fake in this file sets them + equal, so before this test existed, swapping `.available` for `.total` in + `_fetch_available_quote` passed the entire suite -- verified by making that change. + """ + from keel.execution.executor import _fetch_available_quote + + class _OnHold: + def get_balances(self) -> list[Balance]: + # 900 of the 1000 is on hold: spendable is 100. + return [Balance(currency="USD", available=Decimal("100"), total=Decimal("1000"))] + + assert _fetch_available_quote(_OnHold(), "USD") == Decimal("100") + + +def test_the_quote_read_matches_the_currency_case_insensitively() -> None: + """Venues disagree about casing and the product's quote leg is derived from a product id. + A `usd` row must satisfy a `USD` question -- the same comparison `gather_holdings` makes.""" + from keel.execution.executor import _fetch_available_quote + + class _Lowercase: + def get_balances(self) -> list[Balance]: + return [Balance(currency="usd", available=Decimal("42"), total=Decimal("42"))] + + assert _fetch_available_quote(_Lowercase(), "USD") == Decimal("42") + + +def test_the_quote_read_answers_none_when_the_currency_has_no_account() -> None: + """`None` means UNKNOWN and rail 13 fails closed on it. In the port's model a currency with + no account simply is not in the list -- there is no row carrying a null balance to inspect.""" + from keel.execution.executor import _fetch_available_quote + + class _NoUsd: + def get_balances(self) -> list[Balance]: + return [Balance(currency="EUR", available=Decimal("500"), total=Decimal("500"))] + + assert _fetch_available_quote(_NoUsd(), "USD") is None + + def test_quote_fetch_logs_an_unreachable_venue_as_a_warning(caplog) -> None: exc = type("ConnectionError", (Exception,), {})("api.coinbase.com unreachable") diff --git a/tests/test_agent.py b/tests/test_agent.py index ff65a67..ebe24b8 100644 --- a/tests/test_agent.py +++ b/tests/test_agent.py @@ -20,7 +20,7 @@ from typing import Any import pytest -from keel_broker_api.results import MarketSchedule, SessionState +from keel_broker_api.results import Balance, MarketSchedule, SessionState from keel_core.telemetry import _FIELDS_ATTR from keel import agent @@ -65,12 +65,12 @@ def __init__(self, series: dict[tuple[str, Granularity], list[Candle]] | None = self.place_calls: list[dict[str, Any]] = [] self._order_seq = 0 - def get_accounts(self) -> list[dict[str, Any]]: + def get_balances(self) -> list[Balance]: """Comfortable balances -- rail 13 fails closed otherwise. Both legs are funded because rail 13 checks the PRODUCT's quote leg (BTC-USD spends USD), not config.quote_currency.""" return [ - {"currency": "USD", "available_balance": Decimal("1000000")}, - {"currency": "USDC", "available_balance": Decimal("1000000")}, + Balance(currency="USD", available=Decimal("1000000"), total=Decimal("1000000")), + Balance(currency="USDC", available=Decimal("1000000"), total=Decimal("1000000")), ] def get_candles( @@ -1290,12 +1290,12 @@ def test_run_once_skips_the_drawdown_update_when_the_quote_balance_is_unreadable high-water mark PERMANENTLY -- an HWM cannot be walked back, so an under-read arms the breaker on a phantom drawdown forever after. Skip and keep last cycle's scalars instead.""" - class _BrokenAccountsBroker(FakeBroker): - def get_accounts(self) -> list[dict[str, Any]]: + class _BrokenBalancesBroker(FakeBroker): + def get_balances(self) -> list[Balance]: raise RuntimeError("broker down") series = {(PRODUCT, Granularity.ONE_DAY): [_candle(1_000 + i * 86_400) for i in range(30)]} - broker = _BrokenAccountsBroker(series=series) + broker = _BrokenBalancesBroker(series=series) run_once(broker, repo, _config(), now_ts=1_000 + 29 * 86_400) @@ -1314,12 +1314,12 @@ def test_paper_to_live_flip_clears_stale_scalars_even_when_broker_unreadable( repo.set_state("drawdown_total_pct", Decimal("0.9")) repo.set_state("drawdown_weekly_pct", Decimal("0.5")) - class _BrokenAccountsBroker(FakeBroker): - def get_accounts(self) -> list[dict[str, Any]]: + class _BrokenBalancesBroker(FakeBroker): + def get_balances(self) -> list[Balance]: raise RuntimeError("broker down") series = {(PRODUCT, Granularity.ONE_DAY): [_candle(1_000 + i * 86_400) for i in range(30)]} - broker = _BrokenAccountsBroker(series=series) + broker = _BrokenBalancesBroker(series=series) run_once(broker, repo, _config(), now_ts=1_000 + 29 * 86_400) @@ -1343,8 +1343,8 @@ def __init__(self, **kwargs: Any) -> None: super().__init__(**kwargs) self.balance = Decimal("10000") - def get_accounts(self) -> list[dict[str, Any]]: - return [{"currency": "USD", "available_balance": self.balance}] + def get_balances(self) -> list[Balance]: + return [Balance(currency="USD", available=self.balance, total=self.balance)] series = {(PRODUCT, Granularity.ONE_DAY): [_candle(1_000 + i * 86_400) for i in range(30)]} broker = _DecliningBroker(series=series) @@ -1739,7 +1739,7 @@ def place_order(self, *a, **k): def preview_order(self, *a, **k): raise AssertionError("paper mode previewed an order") - def get_accounts(self, *a, **k): + def get_balances(self, *a, **k): raise AssertionError("paper mode read account state") @@ -1898,7 +1898,7 @@ class _NullBalanceBroker(FakeBroker): (and therefore the paper seed's real-equity attempt) must return `None` here, forcing the config fallback rather than a phantom balance.""" - def get_accounts(self) -> list[dict[str, Any]]: + def get_balances(self) -> list[Balance]: return [] @@ -2601,10 +2601,10 @@ def test_equity_counts_settled_cash_in_EVERY_quote_leg_being_traded(repo): """ class TwoCurrencyBroker: - def get_accounts(self): + def get_balances(self): return [ - {"currency": "USD", "available_balance": Decimal("1000")}, - {"currency": "USDC", "available_balance": Decimal("7")}, + Balance(currency="USD", available=Decimal("1000"), total=Decimal("1000")), + Balance(currency="USDC", available=Decimal("7"), total=Decimal("7")), ] equity = agent._mark_to_market_equity(repo, TwoCurrencyBroker(), ["BTC-USD"], {}, "USDC") @@ -2620,8 +2620,8 @@ def test_equity_is_a_total_when_only_SOME_currencies_are_readable(repo): would return None on a perfectly ordinary account and stall rail 11's equity tracking.""" class OnlyUsd: - def get_accounts(self): - return [{"currency": "USD", "available_balance": Decimal("1000")}] + def get_balances(self): + return [Balance(currency="USD", available=Decimal("1000"), total=Decimal("1000"))] equity = agent._mark_to_market_equity(repo, OnlyUsd(), ["BTC-USD"], {}, "USDC") assert equity == Decimal("1000"), f"expected a total, got {equity!r}" @@ -2629,7 +2629,7 @@ def get_accounts(self): def test_equity_is_None_only_when_NOTHING_is_readable(repo): class NoAccounts: - def get_accounts(self): + def get_balances(self): return [] assert agent._mark_to_market_equity(repo, NoAccounts(), ["BTC-USD"], {}, "USDC") is None @@ -2641,8 +2641,8 @@ def test_equity_finds_cash_for_a_HELD_product_whose_rule_was_retired(repo, monke an under-read, and the HWM never falls.""" class EurOnly: - def get_accounts(self): - return [{"currency": "EUR", "available_balance": Decimal("500")}] + def get_balances(self): + return [Balance(currency="EUR", available=Decimal("500"), total=Decimal("500"))] monkeypatch.setattr(repo, "held_products", lambda: ["BTC-EUR"]) equity = agent._mark_to_market_equity(repo, EurOnly(), [], {}, "USD")