fix(router): invoke toToken.address.toLowerCase() in per-pair cache key#446
Conversation
The per-pair throttle-cache key read `toToken.address.toLowerCase`
without the call parens in all four RainSolverRouter query methods
(getMarketPrice/tryQuote/findBestRoute/getTradeParams) and in the
coupled reader at prepareRouter (round.ts). A method reference
stringifies to its source, so the key's second segment collapsed to the
constant `function toLowerCase() { [native code] }`, making the key
depend only on fromToken and collide across every toToken.
The cache is a prefetch-throttle counter (not a result cache), so the
collision never returned a stale route/quote/price, but it corrupted the
prefetch gate: pairs sharing a sell token shared one counter, so an
unrelated pair could push the gate past its skip threshold (and the
counter saturated ~2x too fast). Fixing only the writer would break the
gate, so the round.ts reader is fixed in lockstep.
Adds mutation-validated tests: distinct toTokens now yield distinct
cache entries, and a warmed pair skips prefetch via the correctly-keyed
counter. Both fail under the missing-`()` mutation.
Closes #445
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Warning Review limit reached
More reviews will be available in 51 minutes and 51 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
State what the per-pair cache key construction does now (the key includes the lowercased toToken/buyToken address, distinct per token) rather than narrating the missing-`()` mutation, the old bug, or before/after framing. Comment-only; no code or logic changes. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Resolve conflict in src/router/router.ts getMarketPrice: keep master's
`{ price: string; route?: MultiRoute }` return type and layer #446's
`toToken.address.toLowerCase()` (invoked) onto the per-pair cache key.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Resolve router.test.ts conflict: keep all three describe blocks (per-pair cache key, cache mechanism, getError classification) and correct stale comments to describe the current per-pair cache-key behavior. Production fix already landed on master via #443, so this branch now contributes tests and doc comments only. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Reviewed 80cf804: approved by maintainer; conflict resolved via merge of master (production fix already in master via #443 — this contributes the mutation-validated per-pair cache-key tests + corrected doc comments). build, unit tests, git-clean, CodeRabbit all green; the only reds are the e2e-fork suite, which is environmental (fails identically on master). Merging. |
|
@coderabbitai assess this PR size classification for the totality of the PR with the following criterias and report it in your comment: S/M/L PR Classification Guidelines:This guide helps classify merged pull requests by effort and complexity rather than just line count. The goal is to assess the difficulty and scope of changes after they have been completed. Small (S)Characteristics:
Review Effort: Would have taken 5-10 minutes Examples:
Medium (M)Characteristics:
Review Effort: Would have taken 15-30 minutes Examples:
Large (L)Characteristics:
Review Effort: Would have taken 45+ minutes Examples:
Additional Factors to ConsiderWhen deciding between sizes, also consider:
Notes:
|
What
The per-pair throttle-cache key read
toToken.address.toLowerCasewithout the call parens in all fourRainSolverRouterquery methods (getMarketPrice,tryQuote,findBestRoute,getTradeParams) and in the coupled readerprepareRouter(src/core/process/round.ts:109).A method reference stringifies to its source code, so the key's second segment became the constant
function toLowerCase() { [native code] }for every token — the key depended only onfromTokenand collided across alltoTokens.Impact (real bug, latent)
RainSolverRouter.cacheis a prefetch-throttle counter, not a result cache — the query methods always run a livePromise.alland return live results, so the collision never returned a stale route/quote/price. But the counter gatesprepareRouter(skip pool-data prefetch once a pair's counter > 3). With the key collapsed to the sell token only:(X, Z)can push the gate past its skip threshold and cause(X, Y)to skip prefetch even though it was never warmed.prepareRouter's(sell→buy)and(sell→native)calls collapse to the same key, double-incrementing the sell-token counter per round.Mitigated (not catastrophic) by the
newPoolCreatedforce-prefetch branch and the separately, correctly keyed route caches — but genuinely incorrect cross-pair behavior. Full diagnosis + probe in #445.Fix
Add the missing
()at all fourrouter.tssites and the coupledround.ts:109reader (fixing only the writer would break the gate, since the reader key would stop matching the writer key).Tests (mutation-validated)
src/router/router.test.ts— for each of the four methods: onefromToken+ two differenttoTokens yields two distinct cache entries (cache.size === 2, exact keys asserted). Collides to size 1 (FAILS) under the missing-()mutation.src/core/process/round.test.ts— a pair pre-warmed under the correctly-keyed counter skips prefetch; a differentbuyToken(samesellToken) still prefetches. FAILS under theround.tsmissing-()mutation (prefetch wrongly runs).Verified each new test passes with the fix and fails when the
()is reverted at the corresponding site.tsc+ eslint clean; full vitest unit suite green (the lonetransport.test.tsflake is an environmentalEADDRINUSE :9292port clash, passes in isolation; e2e-fork is pre-existing/environmental).Note: #443 also added tests to
src/router/router.test.ts; trivial rebase if it lands first.Closes #445
🤖 Generated with Claude Code