fix(priceOracle): circuit breaker false-trips on unsupported-asset lookups - #141
Merged
prodbycorne merged 7 commits intoAug 21, 2026
Conversation
…e's unsupported-asset check Lets callers check whether CoinGecko can serve a given asset before ever invoking the circuit-breaker-wrapped fetchPrice — see SmartDropLabs#130. STELLAR_COINGECKO_MAP only maps XLM, so isSupported('USDC') is false while isSupported('XLM') is true, matching fetchPrice's existing '!coinId' branch exactly.
…lveMarket Lets callers check whether CoinMarketCap can serve a given asset/issuer before ever invoking the circuit-breaker-wrapped fetchPrice — see SmartDropLabs#130. Reuses the existing resolveMarket helper so isSupported can never drift from fetchPrice's own notion of 'supported' (an XLM-style no-issuer asset, a configured asset:issuer pair like USDC, or nothing in assetIssuerMap at all).
Lets callers check whether the Stellar DEX can serve a given asset before ever invoking the circuit-breaker-wrapped fetchPrice — see SmartDropLabs#130. Matches fetchPrice's existing '!issuer && normalizedCode !== XLM' check exactly: true for XLM regardless of issuer, true for any other asset with an issuer, false for a non-XLM asset with none.
…esn't support the asset fetchFromAllSources now checks source.isSupported(assetCode, issuer) before ever calling source.breaker.call(...). This is the actual fix for SmartDropLabs#130: previously every source's CircuitBreaker instance is shared across all assets, so a source being asked about even one unsupported asset (e.g. CoinGecko queried for USDC, which STELLAR_COINGECKO_MAP never mapped) would eventually trip that source's breaker OPEN for every other asset it does support, purely from a normal 'unsupported asset' null being misread as a source failure. A genuine null for an asset a source is supposed to support still goes through breaker.call() and still counts toward the failure threshold, unchanged.
…ce's breaker Adds isSupported mocks to the existing source-module mocks (defaulting to true so prior tests keep exercising the breaker-wrapped fetch path unchanged) and a resetCircuitBreakers() call to beforeEach so SmartDropLabs#130's deliberately-tripped breaker tests never leak state into other tests. Three new tests: a source is skipped entirely (fetch never called) for an asset it doesn't support; 5 repeated lookups for a CoinGecko-unsupported asset never move its breaker off 'closed', and XLM (CoinGecko-supported) keeps succeeding via it throughout; and a source still trips 'open' after failureThreshold genuine null returns for an asset it does support, with fetch correctly skipped afterward. Verified the first two fail against the pre-fix fetchFromAllSources (fetch called 1 and 2 times respectively) and pass against the fix.
…rce's circuit breaker The existing CIRCUIT_BREAKER_* env var docs didn't mention that these breakers are shared per-source across every watched asset — worth spelling out now that SmartDropLabs#130's fix makes 'unsupported asset' and 'source failure' distinct, so operators tuning CIRCUIT_BREAKER_FAILURE_THRESHOLD understand what actually counts toward it.
…ller's isSupported obligation Adds a JSDoc comment on call() spelling out that null/undefined is treated as a failure, and that callers must pre-filter permanently-unsupported requests before ever calling call() rather than letting them surface as null — that conflation is exactly what SmartDropLabs#130 was. Points at priceOracle.js's isSupported gate as the reference implementation so a future caller wrapping a new shared resource in this breaker doesn't repeat the bug.
Contributor
|
clean code |
4 tasks
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
2 participants
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.
Summary
CircuitBreaker.call()treats anynull/undefinedreturn as a failure. Every price source'sfetchPrice()legitimately returnsnullfor the common, permanent, non-error case of "this source doesn't support this asset" — not just for actual failures. Since each source'sCircuitBreakeris shared across every asset (one breaker per source, not per source-per-asset), a source being asked about even one unsupported asset (e.g. CoinGecko queried for USDC —STELLAR_COINGECKO_MAPonly maps XLM, and USDC is a real, first-class configured asset perconfig.coinmarketcap.assetIssuerMap) will eventually trip that source's breaker OPEN, taking it offline for every other asset it does support.Fixes it the way the issue calls "the cleanest fix": each source now exposes
isSupported(assetCode, issuer), andpriceOracle.js'sfetchFromAllSourceschecks it before ever invoking the circuit-breaker-wrappedfetchPrice— an unsupported lookup never reaches the breaker at all. A genuinenullfor an asset a source is supposed to support still goes throughbreaker.call()and still counts toward the failure threshold, unchanged.Changes (7 commits)
feat(coingecko): addisSupported(assetCode), extracted fromfetchPrice's existing unsupported-asset checkfeat(coinmarketcap): addisSupported(assetCode, issuer), reusing the existingresolveMarkethelper so it can't drift fromfetchPrice's own notion of "supported"feat(stellarDex): addisSupported(assetCode, issuer), matchingfetchPrice's existing issuer-required checkfix(priceOracle): the actual fix —fetchFromAllSourcesskips a source's breaker entirely whenisSupportedreturns falsetest(priceOracle): regression tests for the false-trip (verified they fail against the pre-fix code) plus a test confirming genuine failures still trip the breakerdocs: clarify in the README that unsupported-asset lookups don't count towardCIRCUIT_BREAKER_FAILURE_THRESHOLDdocs(circuitBreaker): document the null-means-failure contract oncall()so a future caller wrapping a new shared resource doesn't repeat this bugAcceptance criteria
failureThresholdgenuine failures for an asset it supportscoingecko.js,coinmarketcap.js, andstellarDex.jsTest plan
npx jest— 382/382 passing across 39 suites, run against local Redis matching CI's service containernpx @redocly/cli lint openapi.yaml— passes (untouched by this change)priceOracle.test.jsfail against the pre-fixfetchFromAllSources(CoinGecko's mocked fetch gets called 1 and 2 times respectively when it should never be called) and pass against the fixCloses #130