fix(getUsdPrice): don't stop of first failure - #254
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Team Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe change distinguishes CoinGecko failures from missing prices, centralizes repository fallback handling, and verifies that proxy failures are not cached. ChangesUSD repository failure handling
Estimated code review effort: 3 (Moderate) | ~25 minutes Merge Risk: ⚪ Minimal · up to The PR improves price-source fallback behavior and prevents upstream failures from being cached as missing prices; no actionable merge-blocking risk remains. Sequence Diagram(s)sequenceDiagram
participant Client
participant UsdRepositoryFallback
participant Repository
participant logger
Client->>UsdRepositoryFallback: request USD price
UsdRepositoryFallback->>Repository: query repository
Repository-->>UsdRepositoryFallback: price, null, or error
UsdRepositoryFallback->>logger: log fallback event
UsdRepositoryFallback->>Repository: query next repository when needed
UsdRepositoryFallback-->>Client: return price, null, or error
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Full details: Title checkExplanation The title clearly identifies the main change: getUsdPrice now continues fallback processing after the first repository failure. It contains a minor grammatical error, but remains specific and understandable. Full details: Docstring CoverageExplanation No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 5 files. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
|
@coderabbitai review |
7d6697d to
e79e194
Compare
e79e194 to
944b674
Compare
2 participants
Summary
Another part of https://linear.app/cowswap/issue/FE-449/address-coingecko-fallback-issues-in-bff
Don't return a 500 on first 500 found. Try all repositories instead as one might have a price.
Details
Make fallback work on upstream errors, not just null, and stop caching upstream errors as "no price". Implemented together, since neither works alone: fixing only the fallback is inert because Coingecko failures never reach it as throws, and fixing only Coingecko would turn its errors straight into 500s.
UsdRepositoryFallback had a bare await per repository, so a throw aborted the loop and escaped as a 500 instead of trying the next source. A throw is now treated like a null and the next source is tried.
Both Coingecko response handlers short-circuited before throwIfUnsuccessful: a 429 or 5xx carries no price field, matched the !data?.[key]?.usd condition and returned null. That null was cached for the 30 minute NULL TTL, hid the outage behind a 404 long after recovery, and made the fallback log line count failures as unknown tokens. Only a 404 is now treated as an answer.
When no price is found and any source failed, the error is rethrown rather than reported as no price. Reason below in the notes: a 404 makes the frontend blacklist the token for the whole session, so an outage must never look like one.
UsdRepositoryCache needed no change, since cacheValue already sits after the await and a throw skips it. That was implicit and is now asserted in tests.
Watch after deploy: this deliberately converts a class of silent 404s into 5xx. Volume is unknown precisely because these failures have been recorded as nulls until now.
Testing
Summary by CodeRabbit