fix(rails): rail 13 must guard the currency the order actually spends - #121
Merged
Conversation
Rail 13 guards config.quote_currency while an order spends its PRODUCT's quote leg -- which can make the rail PASS an order the account cannot fund. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Rail 13 fetched the balance of config.quote_currency (USDC) and compared it to
the notional -- but a BTC-USD order spends USD. The rail guarded a balance the
order never touches, with two failure modes:
false veto (observed live): $49 USD, $0.25 USDC, a $5 BTC-USD order -> vetoed
false pass (the serious one): ample USDC, no USD -> the rail APPROVES an order
the account cannot fund. That is exactly the 'never draw from a
linked bank/ACH source' case rail 13 exists to prevent. A rail that
can pass when it should veto is worse than no rail, because it is
trusted.
The currency an order spends is a property of the PRODUCT, never of global
config. New keel_core.products.quote_currency_of() derives it; the executor
fetches that balance; the rail names that currency in its violation (telling an
operator to fund USDC for a USD-settled order sends them to the wrong place);
an unresolvable product id yields None, which the rail already vetoes on.
Two consequences of the same root cause, fixed with it:
- MarketFacts.quotable_in_settlement_currency was 'endswith(-quote) or
bool(candles)'. Every screened product is -USD while quote was USDC, so it
always fell through to bool(candles) -- one of four admission criteria was
re-checking 'do we have bars'. The fallback is gone; the check is now real.
- quote_currency defaults to USD (config, templates, Config, DiscoveryPolicy)
because that is what this deployment actually trades. With the settlement
check real, USDC would reject every -USD asset.
Also: stablecoins join fiat in the holdings exclusion -- cash held between
positions is not a position. The operator's working config moves to a
gitignored config.local.yaml; the repo config.yaml is the shipped template and
should not carry one machine's live settings.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
eaitbrahim
added
bug
…11 equity Independent review found four blocking defects in the first cut: B1 THE TESTS DID NOT TEST THE FIX. _config() inherits the new USD default, so 'config.quote_currency=USDC' was false in the tests asserting it, and reverting the executor line left the suite green -- the regression barrier was the default flip, not the product derivation. Now passed explicitly, and mutation-checked: reverting the line fails all three. B2 A default change does not change configs on disk. _history_product and _default_sim_products hardcoded -USD while the (now real) settlement check compared against config, so every existing quote_currency: USDC deployment silently rejected every asset on an unfixable settlement failure. Both now derive from the configured currency, so the worst case is an honest 'no local history, run keel fetch'. Verified against a legacy config. B3 The go-live pre-flight told the operator to hold USDC -- precisely the currency that now produces the false veto this PR exists to fix. Corrected, along with README/operator-runbook claims that buys route through USDC. B4 Rail 11's equity still read only config.quote_currency, and this PR unblocks the trading that corrupts it: under-read equity latches a monotonic HWM and arms the total-drawdown breaker permanently on a flat account. Equity now sums settled cash across every quote leg in play. Also: 'settlement' removed from the data-derived suppression set (it no longer touches candles, so suppressing it would hide a real failure), executor docstring, case-insensitive discover comparison, and restored coverage for 'no account exists for the required currency'. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… tautology
Second review round. No blocking defects were found; these are its non-blocking
findings, and two of them matter.
THE FIXES HAD NO TESTS. Reverting _history_product to a hardcoded -USD, or
equity's all()->any(), or discover's .upper(), all left the suite green -- the
same gap the first round caught for rail 13 itself. All four are now mutation-
checked and CAUGHT.
THE SETTLEMENT CRITERION IS TAUTOLOGICAL FOR PRODUCTS WE DERIVE, and my B2 fix
made it so: _history_product builds {asset}-{quote} and the screen then asserts
they match. Rather than claim it is doing work it isn't, the check is documented
for what it actually guards -- an EXTERNALLY supplied product (--products, or a
future venue/LLM-sourced list) whose quote leg would need a cross to settle,
which is the §65.7 case. A test pins that BTC-EUR under USD settlement fails it,
so the criterion is not dead.
Also: monitor duplicated the -USD expression instead of calling the helper, so
under a non-USD config it cached candles nothing else read; a comment still
described a fallback this branch deleted; help text promised -USD regardless of
configuration; and equity's no-FX-conversion assumption is now an explicit
documented bound rather than an unstated one.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…atters Third review round, all non-blocking. The operative stale comment was the one I had NOT fixed. The version above the constant was corrected last round, but the one at the suppression site still described the "quotable_in_settlement_currency degenerates to bool(candles)" fallback that this very PR deletes -- and that is the comment a maintainer reads before re-adding "settlement" to _DATA_DERIVED_FAILURES. No test can catch that (a derived product can never fail settlement), so the comment now says so explicitly. The duplicated paragraph is removed. Equity's currency scan covered only the rule products, while the valuation loop covers those plus held_products(). A rule retired while its position is still open leaves that position marked to market with its funding currency unseen -- an under-read, and the HWM is monotonic, so it arms rail 11 permanently. Now scanned symmetrically, and mutation-checked. Also: the veto-message assertion strips the rail's lowercase tag, which is the only reason the "USDC not in message" check passed. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Found during the first supervised live-order attempt. It looked like three separate quirks; it is one design defect with a live-money safety hole in it.
The defect
The codebase conflates two different things:
BTC-USDspends USD);config.quote_currency— one global setting (defaultUSDC).Nothing derived the first from the product, so rail 13 guarded a balance the order never touches:
BTC-USDorder → vetoed. Annoying, safe.The fix
One rule: the currency an order spends is a property of the PRODUCT, never of global config.
keel_core.products.quote_currency_of(product_id)→ the leg after the last-, uppercased;Nonefor anything unresolvable.None, which it already vetoes on.Two consequences of the same root cause
The screen's settlement criterion was vacuous.
quotable_in_settlement_currency = product.endswith(f"-{quote}") or bool(candles). Every screened product is-USDwhilequotewasUSDC, so it always fell through tobool(candles)— meaningrequire_settlement_quotere-checked "do we have bars", which the history criterion already covers. One of four admission criteria did nothing. The fallback is removed; the check is now real. (This was recorded as a known weakness in PR #120 and deliberately deferred — this is that follow-up.)quote_currencynow defaults toUSD(repo config, both templates,Config,DiscoveryPolicy), because that is what this deployment actually trades — products, cached history, rules and the simulator are all-USD. With the settlement check real, leaving itUSDCwould reject every asset. A default has to describe reality.Also
config.local.yaml.config.yamlis the source of the shipped wheel template and should never carry one machine's live settings — I had committed mine by accident withgit add -A.Verification
1314 tests pass, ruff clean. New:
quote_currency_ofunit tests, and four executor regressions — the headline one asserts that ample configured-currency balance does not fund a differently-quoted product, plus the mirror case, malformed-id fail-closed, and that the message names the right currency.The old tests encoded the bug:
FakeBrokerreturned only aUSDCaccount while every signal wasBTC-USD. It now models per-currency balances so the mismatch cases can be set independently.Known gap, recorded not fixed
Nothing yet rejects an order whose product quote leg differs from
config.quote_currency. The screen rejects such an asset at admission, but a live-seeded rule bypasses admission. Worth a follow-up rail once the live path is proven — adding a new veto the night before a live test is the wrong sequencing.Design:
docs/superpowers/specs/2026-07-22-quote-currency-mismatch-design.md.