fix(lore-0165): publish peg assets in price_usd_series via a zero-weight arm - #188
Merged
Merged
Conversation
…ght arm USDC is our top-preference quote, so canonicalisation makes it the quote on essentially every pair and the base of almost nothing. price_usd_series* emit one row per BASE asset, so USDC got no row at all: 0 of BE's 1,433 USDC-legged pools were priceable in any window, 67.8% of every never-priced pool they hold. Both grains now UNION a zero-weight placeholder keyed on the QUOTE leg before the GROUP BY. Adding 0 to both numerator and denominator cannot perturb a weighted average, so the placeholder only creates the group key and flags it, and precedence falls out of the arithmetic: a non-peg asset reduces to the historical expression byte-identically, a quote-only peg asset gets the fallback, and a peg asset that also trades keeps its market value. Rejected three simpler-looking shapes, documented at the view: appending a $1 row after the GROUP BY emits duplicate keys and silently doubles BE's aggregates; letting the peg arm own peg identities flattens USDT's 102 priceable pools to $1; the anti-join costs two FINAL scans because ClickHouse substitutes CTEs textually. Deviation from the settled design: the guard is countIf(is_peg = 0) = 0, not sum(w) = 0. The latter would convert a genuine NULL - a peg asset whose only candles carry volume_base = 0 - into a fabricated $1, moving the pre-existing nullIf edge the ACs require be left alone. Adds the `method` provenance column (traded/peg, oracle reserved for 0168) so a measured 1.0000 is never confusable with a fallback 1.0000, and the $1 is documented at the view as a placeholder that contradicts our own oracle-tier candles. Audit: usd_reference/_1h are clean (they join both legs), identity_by_contract is N/A, and current_price_usd is confirmed in code to share the base-only defect - scoped out, it is a refreshable-MV rebuild. Tests: new three-case regression over both grains, confirmed to fail without the fix. 11 unit + 15 CH integration tests green on the 26.3.10.60 prod pin. NOT DEPLOYED - views.sql needs a privileged applier on ch-prod-01.
…MIN as 'traded' Code review caught the one deliberate deviation and it was wrong. The guard is now the originally specified sum(w) = 0. My justification asserted the historical expression "yields NULL via nullIf". It does not, and I did not test it. close_usd is a NON-NULLABLE Decimal(38,14), so CAST strips the Nullable nullIf introduces and a zero denominator lands as Decimal128::MIN (-1701411834604692317316873.03) rather than NULL. The countIf guard therefore published that value, flagged method='traded', for a peg asset whose only candles carry zero volume - a catastrophic negative number labelled as measured, in the column BE multiplies into TVL. Two more review findings fixed. The "no NULL introduced" assertion was vacuous - IS NULL on a non-Nullable column is structurally always 0, which is why it never caught this - and now asserts countIf(toFloat64(close_usd) <= 0) = 0. The 1h fixture was hoisted above the loop where it had been a precondition disguised as an iteration step. Two review claims corrected rather than accepted. Its reproduction fixture (a single USDT/USDC candle) fails under BOTH guards, because USDT is a base only there so arm B emits no placeholder and max(is_peg) = 0; sum(w) = 0 only wins where the peg asset is also a quote leg. Both fixtures were measured before changing the guard, and the new test uses the one that discriminates. The residual - any asset whose only priced candles carry zero volume - is pre-existing and not peg-specific, so it is spawned as 0171 rather than widened into this task: the fix needs a contract decision with BE, omit the row versus substitute an unweighted statistic. Also corrected two overstated comments: arm B is a full second FINAL pass, not a cheap narrow projection; and arm B is not subject to the close_usd > 0 predicate, so a peg identity can read status='ok' where usd_reference is empty - intended, but it means the 12.3 discriminator is not universal. 11 unit + 16 CH integration tests green on the 26.3.10.60 pin. The new zero-volume test confirmed to fail with countIf and pass with sum(w) = 0.
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.
price_usd_series*emit one row per base asset. USDC is our top-preference quote, so canonicalisation makes it the quote on essentially every pair and the base of almost nothing — it got no row at all. 0 of BE's 1,433 USDC-legged pools were priceable in any window: 67.8% of every never-priced pool they hold.The control that proves the mechanism: USDC at the canonical issuer is 0/1,433 priceable; USDC at 56 other issuers is 228/233 (97.9%). Same asset code, quote preference the sole variable.
The fix
Both grains now
UNIONa zero-weight placeholder keyed on the quote leg, before theGROUP BY. Adding 0 to both numerator and denominator cannot perturb a weighted average, so the placeholder's only jobs are to create the group key and flag it. Precedence then falls out of the arithmetic rather than being coded as a rule:Three simpler-looking shapes, rejected and documented at the view
$1row after theGROUP BYemits two rows for one key wherever a peg asset also trades. BE joins on(identity, bucket)→ duplicate keys silently double every downstream aggregate.$1— a regression dressed as a fix.FINALscans, because ClickHouse substitutes CTEs textually.One deviation from the settled design
The guard is
countIf(is_peg = 0) = 0, not the specifiedsum(w) = 0. They agree everywhere the design enumerated, and differ in one case: a peg asset whose only candles carryvolume_base = 0has traded rows summing to zero weight, where the historical expression yieldsNULL.sum(w) = 0would convert that into a fabricated$1— moving the pre-existingnullIfedge that the ACs require be left untouched in either direction.Provenance
Adds
method('traded'/'peg', with'oracle'reserved for 0168), appended last. Without it, once 0168 lands a real rate a consumer cannot tell a measured1.0000from a fallback1.0000— theclose_usd = 0mistake in a new surface. The$1is documented at the view as a placeholder that contradicts our own candles (the oracle tier already pricesTF/USDCatclose × 0.9993).Audit of the sibling surfaces
usd_reference/_1h— clean, they join base and quote.identity_by_contract— N/A, reads no candles.current_price_usd— 🔴 confirmed in code to share the base-only defect:current.sqlgroups byasset_idandquote_asset_idappears nowhere in the MV. Scoped out — it is a refreshable-MV DROP + recreate, the operation that wiped the coarse tables in 0095. Prod measurement query is in the task file.Tests
New three-case regression over both grains, confirmed to fail without the fix two ways: on the missing
methodcolumn, and — the behavioural proof — the pre-existing count assertion going 3 → 4, which is USDC appearing for the first time. That assertion change is intentional and annotated.11 unit + 15 Docker-gated CH integration tests green on the 26.3.10.60 prod pin (the whole
prices-clickhousesuite).fmtclean; the 2 clippy warnings are pre-existing ondevelop, verified by stashing.views.sqlneeds a privileged applier on ch-prod-01 —CREATE OR REPLACE VIEWrequires aDROP VIEWgrant unconditionally on 26.3.10.60, which no scoped runtime user has. This merges as a repo change and takes effect on an operator apply. Verification queries — including the USDT non-regression check and its rollback trigger — are in the task file's new Deploy section.This does not fix
GET /assets/{USDC}/ohlcv— that is a different code path, tracked as 0170.