Conversation
36 tickers (16 crypto + 20 xStocks) mapped to CoinGecko ids, single server-side batched fetch per TTL (replaces per-visitor CG calls in the bridge frontend; covers the xStocks family the frontend map never had). CORS-open; stale-on-error keeps serving the last good map. CG_API_KEY env optional.
|
🔍 OpenCodeReview found 4 issue(s) in this PR.
📄
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| if (this.isStale()) { | ||
| void this.refresh(); | ||
| } | ||
| if (this.inFlight) await this.inFlight.catch(() => {}); |
There was a problem hiding this comment.
There is no backoff on failed refreshes: fetchedAt is only updated on success, so during a CoinGecko outage every incoming request sees isStale() and joins/starts a refresh. Once a failed fetch finishes, the very next request immediately retries — sustained outage means one 15s-timeout upstream call per request, and each request awaits the in-flight fetch, adding up to 15s latency. Consider a lastAttemptAt-based throttle (e.g. min retry interval) or a negative cache for failures.
Suggestion:
| if (this.isStale()) { | |
| void this.refresh(); | |
| } | |
| if (this.inFlight) await this.inFlight.catch(() => {}); | |
| if (this.isStale() && Date.now() - this.lastAttemptAt > MIN_RETRY_MS) { | |
| void this.refresh(); | |
| } |
…elCase, node pin - backoff: lastAttemptAt + MIN_RETRY_MS gates request-path retries, so a CG outage costs at most one upstream call per 30s instead of one per request - GET /prices caps the ?tickers filter at the covered ticker count - /prices/coverage: prices_stale → pricesStale (camelCase consistency) - drop the tickerToPriceId() non-null assertion (iterate map entries) - PricesService implements OnModuleInit (hook drift now caught by tsc) - .nvmrc pins 20.18; CI reads node-version-file: .nvmrc (single source)
| const apiKey = this.config.get<string>('cg.apiKey', ''); | ||
| const headers: Record<string, string> = { accept: 'application/json' }; | ||
| if (apiKey) headers['x-cg-demo-api-key'] = String(apiKey); |
There was a problem hiding this comment.
The config comment declares CG_API_KEY as an 'Optional CoinGecko demo/pro key', but this code always sends it as x-cg-demo-api-key against the free api.coingecko.com host. A pro-tier key (which requires the pro-api.coingecko.com host and the x-cg-pro-api-key header) will be silently misused — requests will look anonymous and get demo-tier rate limits or 401s. Either restrict the config to a demo key (update the comment/validation) or branch on key type to set the correct host and header.
Suggestion:
| const apiKey = this.config.get<string>('cg.apiKey', ''); | |
| const headers: Record<string, string> = { accept: 'application/json' }; | |
| if (apiKey) headers['x-cg-demo-api-key'] = String(apiKey); | |
| const apiKey = this.config.get<string>('cg.apiKey', ''); | |
| const headers: Record<string, string> = { accept: 'application/json' }; | |
| if (apiKey) headers['x-cg-demo-api-key'] = String(apiKey); // demo-tier only |
| if (Object.keys(next).length === 0) { | ||
| throw new Error('CoinGecko returned no usable prices'); | ||
| } | ||
| this.prices = next; |
There was a problem hiding this comment.
An upstream response covering only a subset of ids silently replaces the entire price map. On a partial CG response (rate-limited batch truncation, an id temporarily delisted, etc.), all missing tickers are dropped from what every client sees until a later fully-successful refresh, and the only signal is a debug log. Consider merging next into this.prices for ids still present, or at least warn when coverage drops below a threshold (e.g. next size vs BRIDGE_TICKERS.length).
Suggestion:
| if (Object.keys(next).length === 0) { | |
| throw new Error('CoinGecko returned no usable prices'); | |
| } | |
| this.prices = next; | |
| const coverage = Object.keys(next).length; | |
| if (coverage < BRIDGE_TICKERS.length) { | |
| this.logger.warn( | |
| `partial CoinGecko coverage: ${coverage}/${BRIDGE_TICKERS.length} ids returned; merging with previous map`, | |
| ); | |
| } | |
| if (coverage > 0) { | |
| this.prices = { ...this.prices, ...next }; | |
| this.fetchedAt = Date.now(); | |
| } |
1 participant
Covers 36/36 bridgeable tickers from bridge.mintlayer.org
agents-config: 16 crypto/DeFi (already known ids) + 20 xStocks tokenized equities the frontend never had prices for (waaplx→apple-xstock … wxomx→exxon-mobil-xstock, ids verified against CoinGecko live).GET /prices(optional?tickers=filter) —{ticker: usd}, CORS-openGET /prices/coverage— ticker→id map + stalenessCG_API_KEYenv optional; stale-on-error keeps serving the last good map