fix: stop dropping never-filled orders from the fade rate numerator - #477
Draft
claude[bot] wants to merge 1 commit into
Draft
fix: stop dropping never-filled orders from the fade rate numerator#477claude[bot] wants to merge 1 commit into
claude[bot] wants to merge 1 commit into
Conversation
claude
Bot
force-pushed
the
fix/fade-rate-null-token-drops-never-filled
branch
from
August 6, 2026 02:28
7571d72 to
3adb781
Compare
The latestRfqsV2 view projected tokenIn/tokenOut from the archivedorders side of a LEFT OUTER JOIN, and the fade query filters permissioned tokens with an unguarded LOWER(tokenIn) NOT IN (...). A never-filled order has no archivedorders row, so those columns are NULL, and NULL NOT IN (...) is NULL rather than TRUE -- the WHERE clause therefore discarded every never-filled order, which is exactly the "never filled => fade" cohort. postedorders carries its own non-nullable tokenIn/tokenOut (confirmed in data-eng-workflows posted_orders.yaml, and measured 0 NULL / 0 empty over 39,101 Dutch_V2/V3 orders in the last 21 days), so source the filtered columns from the posted-orders side. The permissioned-token exclusion keeps working as intended and now evaluates to FALSE for a genuinely permissioned order instead of NULL. fillTimestamp deliberately stays on the archived side: its NULL is the never-filled signal and it is read with IS NULL. Measured over 2026-07-15..2026-08-04: 1,166 rows dropped, all of them never-filled, cutting the fade numerator from 3,677 to 2,511 (31.7%). Co-authored-by: Cody Born <cody.born@uniswap.org> Co-Authored-By: Claude <noreply@anthropic.com>
claude
Bot
force-pushed
the
fix/fade-rate-null-token-drops-never-filled
branch
from
August 6, 2026 03:25
3adb781 to
96ae282
Compare
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
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.
Requested by Cody Born · Slack thread
Before / After
Before: a filler that wins an order and then never fills it does not count toward its fade rate at all. The order is dropped from the fade query entirely — it contributes to neither the numerator nor the denominator. Never filling is the most complete form of fading, and it was invisible.
After: it counts. A won-and-never-filled order is scored as a fade, exactly as
V2_FADE_RATE_SQLalready says it should be (WHEN fillTimestamp IS NULL THEN 1).How
The
latestRfqsV2view projectedtokenIn/tokenOutfrom thearchivedordersside of aLEFT OUTER JOIN. A never-filled order has noarchivedordersrow, so both columns come backNULL.The fade query then filters permissioned tokens with no NULL handling:
NULL NOT IN (...)evaluates toNULL, notTRUE, and aWHEREclause keeps only rows that evaluate toTRUE. So every never-filled order was silently deleted by a clause meant to exclude two tokens. Standard three-valued logic — not a Redshift quirk.postedorderscarries its own non-nullabletokenIn/tokenOut, so the fix is to source the filtered columns from the posted-orders side (latestOrdersV2) instead. The permissioned-token exclusion keeps working as intended, and now evaluates toFALSEfor a genuinely permissioned order rather thanNULL.fillTimestampdeliberately stays on the archived side — itsNULLis the never-filled signal, and it is read with an explicitIS NULL, which is NULL-safe. The test pins that asymmetry so it doesn't get "tidied up" later.Measured impact
Over 2026-07-15 → 2026-08-04 (21 complete days, BigQuery mirrors of
postedorders/archivedorders, replicating the cron's order of operations):Two things worth calling out:
NULLtokenIn, and everyNULL-tokenInorder was never-filled. Not one survived the predicate.What this does NOT fix — please don't read it as more than it is
This fix alone would not have blocked the currently-worst fillers. Riverside, Orbt, Elk Capital and Wraxyn all exceed the 12% threshold with the bug present, in the large majority of evaluation hours:
If those fillers were not being blocked in production, this bug does not explain why — the cause is elsewhere and should be chased separately.
Where the fix actually changes the outcome is the band of mid-range faders sitting near the line, whose true rate is roughly 12–25%:
That is the honest severity: the bug converted a real fade signal into invisibility for moderate faders, while the egregious ones tripped anyway.
Measurement caveat: the rolling replica behind the hours-over-threshold columns omits
fadeWindowStart(the per-filler post-block clean-slate floor), which lives in DynamoDB and wasn't reachable from the analysis environment. That over-states hours-above-threshold for any filler that was actually blocked and returned. It biases both variants identically, so the as-written-vs-corrected comparison (the flip counts) is unaffected.Scope
Deliberately one defect only. No change to
FADE_RATE_BLOCK_THRESHOLD,LAPLACE_ALPHA/LAPLACE_BETA, the 24h window,ORDERS_PER_FILLER_LIMIT, orMAX_FILLER_ADDRESSES. Other findings in this area are still under investigation and belong in their own PRs.Net diff: two identifiers in the view's projection list, two
exports so the SQL can be asserted on, and one new test file.Schema verification
Per the repo's
CLAUDE.md(Redshift schemas are owned bydata-eng-workflows, and "a column may exist but only be populated for some order types"):tokenIn/tokenOutare declaredSTRINGindata-eng-workflows→lib/spaces/uniswap_x/functions/uniswap_x_hourly_config/tables/load/posted_orders.yaml.Dutch_V2/Dutch_V3posted orders: 0 NULL, 0 empty string on the posted side.LOWER(tokenIn)and 0 onLOWER(tokenOut)across every joined row. The fix does not change which token the filter compares.Test plan
New
test/repositories/fades-repository.test.ts. Rather than string-matching the fixed spelling, it asserts the underlying invariant — every column the unguardedNOT INpredicate filters on must be projected from the non-nullable posted-orders side — by parsing the view's projection list into alias → source table. That also catches a future regression on a different column.Verified red-before / green-after: reverting just the two identifiers back to
archivedOrdersfails 2 of the 7 tests; with the fix all 7 pass.No visual surface — backend cron SQL only.
Note for whoever merges
Open PR #464 ("bound V2 fades view truncation to lookback window with deterministic ORDER BY") touches the same view and adds the same two
exports and the same test file path, for a different defect. The changes are orthogonal in substance but will conflict textually; whichever lands second needs a trivial merge (twodescribeblocks, identical export lines). I kept the export comment wording identical to #464's to make that resolution mechanical.Co-authored-by: Cody Born cody.born@uniswap.org
Generated by Claude Code