From ead992f4da4296bccfc826d9ab738dda78d9bfbe Mon Sep 17 00:00:00 2001 From: Elmehdi Aitbrahim Date: Wed, 16 Sep 2026 09:59:14 -0400 Subject: [PATCH 1/2] fix(execution): quantize bracket prices to the venue tick, in opposite directions Sizes were quantized (#513/#516); prices were not. `_bracket_spec` floored `base_size` and passed `stop`/`target` through untouched, so the rule's ATR arithmetic reached the wire at fourteen decimal places and Coinbase rejected it: 2026-09-16 03:44Z, keel-live.db order id 5 executor.order_rejected PAXG-USD SELL error='Too many decimals in order price' Every protective bracket was refused, which is why every position in that deployment carried `bracket_order_id = NULL` -- the stop-loss and take-profit machinery has never fired in live, and nothing distinguished that from DCA's legitimate bracket-lessness. Direction is the substance. `quantize_down` is documented as safe FOR SIZES -- rounding one up spends more than the rails authorised -- and that reasoning does not transfer: a long's protective stop rounded DOWN sits further from price and silently widens the loss the position was sized against. So the stop rounds UP and the target DOWN, each toward the safer answer, which means toward each other. `quantize_up` is a separate function rather than a flag on the existing one, because the two carry different arguments and a flag invites picking the wrong one. Because they move toward each other, a coarse tick can invert a pair that was valid before it. That is `BracketPricesUnplaceable` -- raised rather than sent. `_bracket_spec` was an inline ARGUMENT to `_run_order`, so anything it raised escaped `place_bracket` past the `if not result.placed` recovery: the shape that stranded a filled position in #799. It is now built in a `try`, and a bracket that cannot be BUILT takes the same path as one the venue REFUSES -- levels to `unbracketed:`, a WARNING, and a retry next cycle. `Instrument.quote_increment` is the field its own docstring anticipated ("would sit here naturally, but nothing reads them yet"). It rides the existing per-product fetch and the existing cache record, so no extra venue round-trip enters the order path. Unknown stays unknown: prices go unrounded, never refused, because refusing leaves a filled position with no stop at all. Verified by mutation: reusing `quantize_down` for the stop -- the fix that still puts prices on the tick -- is killed by three tests. Refs #802. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01AoGRoExgHVCsWDHT3ej8mD --- keel/execution/executor.py | 99 +++++++++- keel/execution/sizing.py | 27 +++ .../keel_broker_api/results.py | 13 +- .../keel_broker_coinbase/adapter.py | 26 ++- tests/broker_coinbase/test_adapter.py | 45 +++++ tests/execution/test_price_precision.py | 180 ++++++++++++++++++ 6 files changed, 382 insertions(+), 8 deletions(-) create mode 100644 tests/execution/test_price_precision.py diff --git a/keel/execution/executor.py b/keel/execution/executor.py index 74fe4cef..9309aad4 100644 --- a/keel/execution/executor.py +++ b/keel/execution/executor.py @@ -418,10 +418,43 @@ def _base_increment_for( if instrument is None: return None increment = instrument.base_increment - repo.set_state(key, {"increment": str(increment), "fetched_at": now_ts}) + # Both increments ride ONE fetch and ONE cached record. Splitting them would double the + # venue round-trips inside the order-placement path -- the latency the per-product read in + # `get_instrument` exists to avoid -- for two fields of the same response (#802). + record: dict[str, object] = {"increment": str(increment), "fetched_at": now_ts} + if instrument.quote_increment is not None: + record["quote_increment"] = str(instrument.quote_increment) + repo.set_state(key, record) return increment +def _price_increment_for( + broker: Any, repo: Repository, product_id: str, now_ts: int +) -> Decimal | None: + """The venue's PRICE tick for `product_id`, or `None` if unknown -- never raises. + + Reads the record `_base_increment_for` writes, and warms it through that function on a miss + so the two never issue separate fetches. `None` means UNKNOWN: prices go on the wire + unrounded, exactly as they did before #802. That is deliberately NOT a refusal -- refusing + here would leave a filled position with no stop at all, which is the outcome #799 documents. + + A record written before #802 carries no `quote_increment` key and reads as unknown until it + expires, which is correct: nothing knew the tick when it was written. + """ + key = f"{BASE_INCREMENT_PREFIX}{product_id}" + cached = repo.get_state(key) + if isinstance(cached, dict): + fetched_at = cached.get("fetched_at") + if isinstance(fetched_at, int) and now_ts - fetched_at < BASE_INCREMENT_TTL_SEC: + return _coerce_increment(cached.get("quote_increment")) + + _base_increment_for(broker, repo, product_id, now_ts) + refreshed = repo.get_state(key) + if isinstance(refreshed, dict): + return _coerce_increment(refreshed.get("quote_increment")) + return None + + def _coerce_increment(raw: object) -> Decimal | None: """A positive `Decimal` from the venue's string, or `None` -- never raises.""" if raw is None: @@ -1785,6 +1818,17 @@ def _order_row( ) +class BracketPricesUnplaceable(RuntimeError): + """A bracket's two prices cannot be expressed on the venue's tick (#802). + + Raised only when quantization COLLAPSES the pair -- rounding moves a long's stop up and its + target down, toward each other, so a coarse enough tick can invert a pair that was valid + before it. `BracketGTC.__post_init__` refuses the inverted pair; this names why, and gives + `place_bracket` something to catch so the failure takes the unbracketed-retry path rather + than escaping the call (the shape that stranded a position in #799). + """ + + class SizePrecisionUnavailable(RuntimeError): """No quote increment is known for this product, so no size can be safely serialised (#513). @@ -2016,6 +2060,7 @@ def _bracket_spec( target: Decimal, stop: Decimal, base_increment: Decimal | None = None, + price_increment: Decimal | None = None, ) -> BracketGTC: """The exit bracket as a port value: ONE order carrying both protective prices. @@ -2034,12 +2079,31 @@ def _bracket_spec( #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. + + **#802 gave the same treatment to the two PRICES, and in OPPOSITE directions.** Sending them + at the engine's precision is what had every bracket rejected ("Too many decimals in order + price"), so the tick applies here too -- but `quantize_down`'s reasoning is about SIZES + (rounding one up spends more than the rails authorised) and does not transfer. A long's + protective stop rounded DOWN sits further from price and widens the loss the position was + sized against; its target rounded UP becomes less reachable. So the stop rounds UP and the + target rounds DOWN -- each toward the safer answer, which means toward each other. + + Because they move toward each other, a coarse tick can invert a pair that was valid before + it. That is `BracketPricesUnplaceable`, raised rather than sent, and caught by `place_bracket`. """ size = ( qty if base_increment is None or base_increment <= 0 else _floor_or_original(qty, base_increment) ) + if price_increment is not None and price_increment > 0: + stop = sizing.quantize_up(stop, price_increment) + target = sizing.quantize_down(target, price_increment) + if stop >= target: + raise BracketPricesUnplaceable( + f"{product_id!r}: at tick {price_increment} the stop quantizes to {stop} and the " + f"target to {target}, which is not a bracket -- refusing to send it" + ) return BracketGTC( product_id=product_id, # A bracket keel places always EXITS a long: keel enters with a market IOC and protects @@ -2115,6 +2179,35 @@ def place_bracket( rule_kind=rule_name, available_base=held, ) + # Built BEFORE the call, and inside a try, deliberately. As an inline argument to + # `_run_order` any exception from here escaped `place_bracket` entirely -- past the + # `if not result.placed` recovery below -- which is exactly how #799 stranded a filled + # position: the entry was already on the books and the bookkeeping never ran. A bracket that + # cannot be BUILT is the same event as a bracket the venue REFUSES, and takes the same path. + try: + spec = _bracket_spec( + product_id, + qty, + target, + stop, + _base_increment_for(broker, repo, product_id, now_ts), + _price_increment_for(broker, repo, product_id, now_ts), + ) + except (BracketPricesUnplaceable, ValueError) as exc: + repo.set_state( + f"{UNBRACKETED_PREFIX}{product_id}", + {"stop": stop, "target": target, "qty": qty}, + ) + log_event( + logger, + logging.WARNING, + "executor.bracket_not_placed", + product=product_id, + reason=f"the bracket could not be expressed for this venue: {exc}", + vetoed_by=[], + ) + return None + result = _run_order( intent, broker, @@ -2123,9 +2216,7 @@ def place_bracket( "autonomous", None, now_ts, - spec=_bracket_spec( - product_id, qty, target, stop, _base_increment_for(broker, repo, product_id, now_ts) - ), + spec=spec, ) if not result.placed: # The entry has ALREADY filled by the time we get here, so this is a real position with diff --git a/keel/execution/sizing.py b/keel/execution/sizing.py index cd316b32..a29f2e41 100644 --- a/keel/execution/sizing.py +++ b/keel/execution/sizing.py @@ -72,6 +72,33 @@ def quantize_down(value: Decimal, increment: Decimal) -> Decimal: return stepped.quantize(scale if isinstance(exponent, int) and exponent < 0 else Decimal(1)) +def quantize_up(value: Decimal, increment: Decimal) -> Decimal: + """`value` rounded UP to a multiple of `increment`. + + The twin of `quantize_down`, and the direction is again the whole point -- but the reasoning + is NOT the same reasoning, so the two must not be collapsed into one helper with a flag that + callers pick idly. `quantize_down` is safe for a SIZE because rounding a size up spends more + than `guards.check` authorised. This one exists for a PROTECTIVE STOP on a long, where the + hazard runs the other way: rounding the stop DOWN moves it further from price and widens the + loss the position was sized against, silently, on every bracket. + + A value already on the increment is returned unchanged rather than bumped a step higher. + + Presentation follows `quantize_down` exactly -- never scientific notation, because `str()` of + the result is what goes on the wire (#513, and #802 for the price leg). + """ + if increment <= 0: + return value + # `Decimal.__floordiv__` TRUNCATES toward zero rather than flooring, so the usual + # `-((-v) // i * i)` ceiling trick silently returns the FLOOR here. Step up explicitly. + stepped = (value // increment) * increment + if stepped < value: + stepped += increment + scale = increment.normalize() + exponent = scale.as_tuple().exponent + return stepped.quantize(scale if isinstance(exponent, int) and exponent < 0 else Decimal(1)) + + def quote_increment_for(product_id: str) -> Decimal | None: """The venue's finest acceptable `quote_size` for `product_id`, or `None` if unknown. diff --git a/packages/keel-broker-api/keel_broker_api/results.py b/packages/keel-broker-api/keel_broker_api/results.py index af61443c..8975e739 100644 --- a/packages/keel-broker-api/keel_broker_api/results.py +++ b/packages/keel-broker-api/keel_broker_api/results.py @@ -152,9 +152,15 @@ class Instrument: 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. + **`quote_increment` was added when a caller appeared (#802).** The paragraph here used to + say quote-side granularity "would sit here naturally, but nothing reads them yet, and a field + no caller reads is a field no test meaningfully checks." `executor._bracket_spec` now reads + it: a bracket carries two PRICES, and sending them at the engine's precision had every + protective bracket rejected with "Too many decimals in order price". Minimum sizes are still + absent for the original reason -- nothing reads them. + + `None` means the venue did not report one, and callers must treat that as UNKNOWN rather + than as "no rounding needed". """ product_id: str @@ -163,6 +169,7 @@ class of fact and would sit here naturally, but nothing reads them yet, and a fi #: constructing an Instrument carrying zero, which a caller would quantize against and get #: a division error or a silent zero size. base_increment: Decimal + quote_increment: Decimal | None = None def __post_init__(self) -> None: if self.base_increment <= 0: diff --git a/packages/keel-broker-coinbase/keel_broker_coinbase/adapter.py b/packages/keel-broker-coinbase/keel_broker_coinbase/adapter.py index 369dd516..b7f437de 100644 --- a/packages/keel-broker-coinbase/keel_broker_coinbase/adapter.py +++ b/packages/keel-broker-coinbase/keel_broker_coinbase/adapter.py @@ -268,7 +268,16 @@ def get_instrument(self, product_id: str) -> Instrument | None: return None if value <= 0: return None - return Instrument(product_id=product_id, base_increment=value) + # `quote_increment` is the PRICE tick, and it is optional in a way `base_increment` is + # not: a product missing it is still tradeable by size, so its absence returns an + # Instrument with `quote_increment=None` rather than no Instrument at all. The caller + # (`executor._bracket_spec`, #802) treats None as UNKNOWN and sends prices unrounded, + # which is what it did for every product before this field existed. + return Instrument( + product_id=product_id, + base_increment=value, + quote_increment=_positive_decimal_or_none(_field(raw, "quote_increment")), + ) def list_products(self, product_type: str = "SPOT") -> list[dict[str, Any]]: """Every tradable product on the venue, as plain dicts. READ-ONLY market metadata. @@ -535,3 +544,18 @@ def _cancel_outcome_from_success(success: object) -> CancelOutcome: __all__ = ["CoinbaseAdapter"] + + +def _positive_decimal_or_none(raw: object) -> Decimal | None: + """A positive `Decimal` from the venue's string, or `None` -- never raises. + + Mirrors `executor._coerce_increment`; kept here rather than imported because the adapter + package must not depend on the engine. + """ + if raw is None: + return None + try: + value = Decimal(str(raw)) + except ArithmeticError, TypeError, ValueError: + return None + return value if value > 0 else None diff --git a/tests/broker_coinbase/test_adapter.py b/tests/broker_coinbase/test_adapter.py index 389e08f3..d2764547 100644 --- a/tests/broker_coinbase/test_adapter.py +++ b/tests/broker_coinbase/test_adapter.py @@ -873,3 +873,48 @@ def test_preview_order_tolerates_an_empty_numeric_field(field: str) -> None: "quote_size": "est_quote_size", "commission_total": "est_fee", } + + +def test_get_instrument_reads_the_quote_increment_too() -> None: + """The PRICE tick, alongside the size one (#802). + + `Instrument` carried only `base_increment` while nothing read the quote side. A bracket + carries two PRICES, and sending them at the engine's precision had Coinbase reject every one + ("Too many decimals in order price"), so `executor._bracket_spec` now reads this. + """ + adapter = CoinbaseAdapter( + FakeTransport( + product={ + "product_id": "PAXG-USD", + "base_increment": "0.00000001", + "quote_increment": "0.01", + } + ) + ) + + instrument = adapter.get_instrument("PAXG-USD") + + assert instrument is not None + assert instrument.base_increment == Decimal("0.00000001") + assert instrument.quote_increment == Decimal("0.01") + + +@pytest.mark.parametrize("bad", [None, "", "abc", "0", "-0.01"]) +def test_a_missing_or_unusable_quote_increment_still_yields_an_instrument(bad: object) -> None: + """Absent/unusable price tick is UNKNOWN, not fatal. + + The product is still tradeable by size, so the read must not collapse to `None` and take the + `base_increment` down with it -- that would make every bracket send an unquantized SIZE as + well, which is the #513 failure this adapter already fixes. The caller treats + `quote_increment=None` as "send prices unrounded", exactly as it behaved before the field. + """ + product: dict[str, object] = {"product_id": "BTC-USD", "base_increment": "0.00000001"} + if bad is not None: + product["quote_increment"] = bad + adapter = CoinbaseAdapter(FakeTransport(product=product)) + + instrument = adapter.get_instrument("BTC-USD") + + assert instrument is not None, "an unusable price tick must not lose the size tick" + assert instrument.base_increment == Decimal("0.00000001") + assert instrument.quote_increment is None diff --git a/tests/execution/test_price_precision.py b/tests/execution/test_price_precision.py new file mode 100644 index 00000000..aa6ea1c2 --- /dev/null +++ b/tests/execution/test_price_precision.py @@ -0,0 +1,180 @@ +"""Order PRICES must be serialised at the venue's tick, not the engine's (#802). + +The sibling of `test_size_precision.py`. That file covers the SIZE leg (#513, the XLM entry +rejected `INVALID_SIZE_PRECISION`); this one covers the PRICE leg, and the live failure is the +same shape one rung along: + + 2026-09-16 03:44 UTC, keel-live.db order id 5 + executor.order_rejected PAXG-USD SELL error='Too many decimals in order price' + +`_bracket_spec` quantized `base_size` and passed `stop`/`target` through untouched, so the +fourteen digits of the rule's ATR arithmetic went on the wire. Every protective bracket was +rejected, which is why every position in that deployment carried `bracket_order_id = NULL`. + +Direction is the substance here, not a detail. `quantize_down` is documented as the safe +direction for a SIZE (rounding up spends more than the rails authorised). That reasoning does +NOT transfer to a protective stop: rounding a long's stop DOWN widens the loss it was sized +against. A stop rounds toward safety, a target toward reachability, and they are therefore +rounded in OPPOSITE directions. +""" + +from __future__ import annotations + +from decimal import Decimal + +import pytest + +from keel.execution import executor, sizing +from keel.execution.executor import BracketPricesUnplaceable, _bracket_spec, place_bracket +from keel.types import Side +from tests.execution.test_executor import NOW_TS, _config, _seed_open_position, repo # noqa: F401 +from tests.execution.test_sell_clamp import HeldBroker + +#: The exact levels Coinbase rejected on 2026-09-16 (keel-live.db, order id 5, position 3). +LIVE_STOP = Decimal("4521.76390215979454") +LIVE_TARGET = Decimal("5582.02658704123276") +CENT = Decimal("0.01") + + +def test_quantize_up_rounds_to_a_multiple_and_never_down() -> None: + assert sizing.quantize_up(Decimal("4521.76390215979454"), CENT) == Decimal("4521.77") + # Already on the tick: unchanged, NOT bumped a tick higher. + assert sizing.quantize_up(Decimal("4521.77"), CENT) == Decimal("4521.77") + + +def test_quantize_up_never_emits_scientific_notation() -> None: + """`str()` of the result goes on the wire, so `5E+1` is a rejected order (`quantize_down`'s + own docstring; the up-rounding twin must not reintroduce what that one was written to avoid).""" + assert str(sizing.quantize_up(Decimal("50"), Decimal("10"))) == "50" + assert str(sizing.quantize_up(Decimal("0.001"), CENT)) == "0.01" + + +def test_bracket_spec_quantizes_both_prices_to_the_tick() -> None: + """The live rejection, reproduced and fixed: neither price may reach the venue unrounded.""" + spec = _bracket_spec( + "PAXG-USD", + Decimal("0.01320427"), + LIVE_TARGET, + LIVE_STOP, + base_increment=Decimal("0.00000001"), + price_increment=CENT, + ) + + assert spec.stop_trigger_price == Decimal("4521.77") + assert spec.take_profit_price == Decimal("5582.02") + # What actually goes on the wire is the string form. + assert str(spec.stop_trigger_price) == "4521.77" + assert str(spec.take_profit_price) == "5582.02" + + +def test_the_two_prices_round_in_opposite_directions() -> None: + """A long's stop rounds UP (toward safety) and its target DOWN (toward reachability). + + This is the assertion that rejects the tempting mutation: reusing `quantize_down` for both, + which passes a naive "is it on the tick?" check while quietly widening every stop-loss. + Both live values round strictly away from `quantize_down`'s answer for the stop. + """ + spec = _bracket_spec("PAXG-USD", Decimal("1"), LIVE_TARGET, LIVE_STOP, price_increment=CENT) + + assert spec.stop_trigger_price > LIVE_STOP, "a long's stop must not be widened by rounding" + assert spec.take_profit_price < LIVE_TARGET, "a long's target must not be raised by rounding" + assert spec.stop_trigger_price != sizing.quantize_down(LIVE_STOP, CENT) + + +def test_an_unknown_tick_sends_the_prices_unchanged() -> None: + """Unknown means unknown. `_base_increment_for`'s contract for the size leg is "send + unquantized, never refuse the exit", and the price leg must not be stricter -- refusing here + would leave a filled position with no stop at all, which is the outcome #799 documents.""" + spec = _bracket_spec("PAXG-USD", Decimal("1"), LIVE_TARGET, LIVE_STOP, price_increment=None) + + assert spec.stop_trigger_price == LIVE_STOP + assert spec.take_profit_price == LIVE_TARGET + + +def test_a_tick_that_collapses_the_pair_is_refused_not_sent() -> None: + """Rounding moves the two prices TOWARD each other, so a coarse tick can invert a pair that + was valid before it. `BracketGTC` already refuses an inverted-or-equal pair; this asserts the + refusal is raised as the executor's own precision error, so `place_bracket` routes it through + the unbracketed-retry path rather than letting it escape (the #799 failure shape).""" + with pytest.raises(BracketPricesUnplaceable): + _bracket_spec( + "FAKE-USD", + Decimal("1"), + Decimal("100.009"), # -> 100.00 + Decimal("100.001"), # -> 100.01, now ABOVE the target + price_increment=CENT, + ) + + +def test_the_spec_is_still_a_sell_bracket_after_quantization() -> None: + spec = _bracket_spec("PAXG-USD", Decimal("1"), LIVE_TARGET, LIVE_STOP, price_increment=CENT) + assert spec.side is Side.SELL + + +# -- the call site: a bracket that cannot be BUILT must not escape `place_bracket` ------------- + + +def test_place_bracket_records_the_retry_when_the_pair_cannot_be_expressed(repo): # noqa: F811 + """A bracket that cannot be BUILT is the same event as one the venue REFUSES. + + `_bracket_spec` used to be an inline argument to `_run_order`, so anything it raised left + `place_bracket` past the `if not result.placed` recovery -- the shape that stranded a filled + PAXG position for three weeks in #799. The entry is already on the books by the time this + runs, so the levels must reach `unbracketed:` for the sweep to retry from, and the call must + return `None` rather than raise. + + Deleting the `try` around the spec build, or narrowing its `except`, fails this test. + """ + _seed_open_position(repo, "BTC-USD", Decimal("1.0"), Decimal("50000")) + repo.set_state( + f"{executor.BASE_INCREMENT_PREFIX}BTC-USD", + {"increment": "0.00000001", "quote_increment": "0.01", "fetched_at": NOW_TS}, + ) + broker = HeldBroker("BTC", available=Decimal("1.0"), total=Decimal("1.0")) + + # A tick that collapses the pair: stop -> 100.01, target -> 100.00. + result = place_bracket( + broker, + repo, + _config(), + "BTC-USD", + Decimal("1.0"), + Decimal("100.001"), # stop -- `place_bracket` takes stop BEFORE target + Decimal("100.009"), # target + "turtle_breakout", + NOW_TS, + ) + + assert result is None + assert broker.place_calls == [], "a bracket that cannot be expressed must not be sent" + retry = repo.get_state(f"{executor.UNBRACKETED_PREFIX}BTC-USD") + assert retry is not None, "the sweep has nothing to retry from -- the position stays naked" + assert retry["stop"] == Decimal("100.001"), "the retry must hold the RULE's levels, unrounded" + assert retry["target"] == Decimal("100.009") + + +def test_place_bracket_sends_prices_on_the_tick(repo): # noqa: F811 + """The live rejection, end to end: what reaches the broker carries cents, not + fourteen digits.""" + _seed_open_position(repo, "PAXG-USD", Decimal("0.0132"), Decimal("4673.23")) + repo.set_state( + f"{executor.BASE_INCREMENT_PREFIX}PAXG-USD", + {"increment": "0.00000001", "quote_increment": "0.01", "fetched_at": NOW_TS}, + ) + broker = HeldBroker("PAXG", available=Decimal("0.0132"), total=Decimal("0.0132")) + + place_bracket( + broker, + repo, + _config(), + "PAXG-USD", + Decimal("0.0132"), + LIVE_STOP, # `place_bracket` takes stop BEFORE target + LIVE_TARGET, + "turtle_breakout", + NOW_TS, + ) + + spec = broker.place_calls[-1]["spec"] + assert spec.stop_trigger_price == Decimal("4521.77") + assert spec.take_profit_price == Decimal("5582.02") From f3c40350689e8c8b3840e76a7a071337bebdd11e Mon Sep 17 00:00:00 2001 From: Elmehdi Aitbrahim Date: Wed, 16 Sep 2026 10:23:04 -0400 Subject: [PATCH 2/2] fix(execution): quantize the ROLLED stop too -- review found the second call site Self-review of #804 found `_bracket_spec` has TWO callers and this PR patched one. `_roll_stop` -- the trailing ratchet, the protective path that fires most often -- kept sending prices at full Decimal precision, so #802 would have stayed alive on it while `place_bracket` looked fixed. It also shares the #799 escape shape, and worse: `_roll_stop` CANCELS the old bracket before building the replacement, so an exception there leaves the position naked AND skips the CRITICAL that exists to announce exactly that. The spec is now built in a try, and a replacement that cannot be BUILT logs the same CRITICAL as one the venue REJECTS, leaving the `unbracketed:` record standing for the sweep. Tests added for the gap the review also found: `_price_increment_for` had no direct coverage. Five cases -- warm cache, one-fetch-for-both, a pre-#802 record with no quote_increment key, a venue reporting no tick, and no broker at all (paper mode). NOT changed, recorded so it is not re-attempted: the `except (A, B) as exc` parentheses look inconsistent with `keel/proposer.py:53`'s bare PEP 758 form, but the unparenthesized syntax is illegal WITH an `as` binding -- "multiple exception types must be parenthesized when using 'as'". The two forms are not interchangeable. Refs #802. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01AoGRoExgHVCsWDHT3ej8mD --- keel/execution/executor.py | 35 +++++++- tests/execution/test_price_precision.py | 114 ++++++++++++++++++++++++ 2 files changed, 146 insertions(+), 3 deletions(-) diff --git a/keel/execution/executor.py b/keel/execution/executor.py index 9309aad4..8ffe21f3 100644 --- a/keel/execution/executor.py +++ b/keel/execution/executor.py @@ -2684,6 +2684,37 @@ def _roll_stop( rule_kind=rule_name, available_base=held, ) + # Built BEFORE the call and inside a try, for the same reason `place_bracket` is -- and more + # urgently. The old bracket is ALREADY CANCELLED by the time we get here, so an exception + # escaping this line leaves the position naked AND skips the CRITICAL below, which is + # strictly worse than the #799 shape it shares. A replacement that cannot be BUILT is the + # same event as one the venue REJECTS, and takes the same path. + try: + spec = _bracket_spec( + product_id, + qty, + target, + new_stop, + _base_increment_for(broker, repo, product_id, now_ts), + _price_increment_for(broker, repo, product_id, now_ts), + ) + except (BracketPricesUnplaceable, ValueError) as exc: + log_event( + logger, + logging.CRITICAL, + "executor.position_unprotected", + product=product_id, + reason=f"the replacement bracket could not be expressed for this venue: {exc}", + attempted_stop=new_stop, + cancelled_order_id=old_stop_order_id, + detail=( + "the previous bracket was cancelled and its replacement could not be BUILT -- " + "this position currently has no protective stop at the exchange. The " + "unbracketed record is retained so the next cycle's sweep re-places it." + ), + ) + return None + result = _run_order( intent, broker, @@ -2692,9 +2723,7 @@ def _roll_stop( "autonomous", None, now_ts, - spec=_bracket_spec( - product_id, qty, target, new_stop, _base_increment_for(broker, repo, product_id, now_ts) - ), + spec=spec, ) if not result.placed: # The old bracket is already cancelled, so the position is NAKED right now. The diff --git a/tests/execution/test_price_precision.py b/tests/execution/test_price_precision.py index aa6ea1c2..683cb0de 100644 --- a/tests/execution/test_price_precision.py +++ b/tests/execution/test_price_precision.py @@ -178,3 +178,117 @@ def test_place_bracket_sends_prices_on_the_tick(repo): # noqa: F811 spec = broker.place_calls[-1]["spec"] assert spec.stop_trigger_price == Decimal("4521.77") assert spec.take_profit_price == Decimal("5582.02") + + +# -- the ratchet path, which is the one that matters most --------------------------------------- + + +def test_a_rolled_stop_is_also_quantized(repo): # noqa: F811 + """`_roll_stop` is the SECOND `_bracket_spec` call site, and it was missed once already. + + It is the trailing ratchet -- the thing that tightens a stop as a trade runs -- so leaving it + unquantized would keep #802 alive on the protective path that fires most often, while + `place_bracket` looked fixed. + """ + repo.set_state( + f"{executor.BASE_INCREMENT_PREFIX}BTC-USD", + {"increment": "0.00000001", "quote_increment": "0.01", "fetched_at": NOW_TS}, + ) + broker = HeldBroker("BTC", available=Decimal("0.01"), total=Decimal("0.01")) + stop_id = place_bracket( + broker, + repo, + _config(), + product_id="BTC-USD", + qty=Decimal("0.01"), + stop=Decimal("49000"), + target=Decimal("53000"), + rule_name="pullback_continuation", + now_ts=NOW_TS, + ) + + executor.roll_to_break_even( + broker, + repo, + _config(), + product_id="BTC-USD", + old_stop_order_id=stop_id, + entry_price=Decimal("50000.004999"), # a break-even stop off the tick + qty=Decimal("0.01"), + rule_name="pullback_continuation", + now_ts=NOW_TS + 100, + ) + + rolled = broker.place_calls[-1]["spec"] + assert rolled.stop_trigger_price == Decimal("50000.01"), "the rolled stop reached the tick" + assert str(rolled.stop_trigger_price) == "50000.01" + + +# -- _price_increment_for ---------------------------------------------------------------------- + + +class _InstrumentBroker: + """A broker that answers `get_instrument` and counts how often it is asked.""" + + def __init__(self, quote_increment: str | None = "0.01") -> None: + self.calls = 0 + self._quote_increment = quote_increment + + def get_instrument(self, product_id: str): # noqa: ANN201 + from keel_broker_api.results import Instrument + + self.calls += 1 + return Instrument( + product_id=product_id, + base_increment=Decimal("0.00000001"), + quote_increment=( + None if self._quote_increment is None else Decimal(self._quote_increment) + ), + ) + + +def test_price_increment_is_read_from_the_cached_record(repo): # noqa: F811 + repo.set_state( + f"{executor.BASE_INCREMENT_PREFIX}PAXG-USD", + {"increment": "0.00000001", "quote_increment": "0.01", "fetched_at": NOW_TS}, + ) + broker = _InstrumentBroker() + + assert executor._price_increment_for(broker, repo, "PAXG-USD", NOW_TS) == CENT + assert broker.calls == 0, "a warm cache must not reach the venue inside the order path" + + +def test_both_increments_share_one_fetch(repo): # noqa: F811 + """The per-product read exists to keep ONE venue round-trip in the order path. Asking for the + price tick after the size one must not add a second.""" + broker = _InstrumentBroker() + + executor._base_increment_for(broker, repo, "PAXG-USD", NOW_TS) + assert executor._price_increment_for(broker, repo, "PAXG-USD", NOW_TS) == CENT + assert broker.calls == 1 + + +def test_a_record_without_a_quote_increment_reads_unknown(repo): # noqa: F811 + """A cache record written before #802 carries no `quote_increment` key. Unknown is correct -- + nothing knew the tick when it was written -- and unknown must not become a guessed 0.01.""" + repo.set_state( + f"{executor.BASE_INCREMENT_PREFIX}PAXG-USD", + {"increment": "0.00000001", "fetched_at": NOW_TS}, + ) + + assert executor._price_increment_for(_InstrumentBroker(), repo, "PAXG-USD", NOW_TS) is None + + +def test_a_venue_that_reports_no_tick_reads_unknown(repo): # noqa: F811 + assert ( + executor._price_increment_for( + _InstrumentBroker(quote_increment=None), repo, "X-USD", NOW_TS + ) + is None + ) + + +def test_no_broker_reads_unknown_rather_than_raising(repo): # noqa: F811 + """Paper mode passes no broker. `_base_increment_for` is documented as never raising, and + this must not be the function that reintroduces one into the order path.""" + assert executor._price_increment_for(None, repo, "PAXG-USD", NOW_TS) is None