feat(price): Phase 2 — direct backup quoters + multi-source aggregation#773
Conversation
Implements Phase 2 of docs/PRICE_PROVIDERS.md: the system becomes genuinely multi-source. Three keyless direct (PerBtc) adapters join Yadio, each fixture-tested against a captured live payload (§10.5): - coingecko: GET /simple/price over a baked fiat vs_currencies list; optional demo/pro api_key (header picked from the host), redacted from Debug/logs per §10.3. - currency_api: GET /currencies/btc.min.json; lowercase codes upper-cased (§6.6); implements `fallback_urls` — mirrors tried in order, the provider only fails when every URL fails (§7). Fallback behaviour covered by a local-axum integration test. - blockchain: GET /ticker taking `last` (mid-market) only; buy/sell are not even deserialised (§6.6/§11.6). Manager (spec §5.3, §6.5, §6.6): - Providers are polled concurrently (join_all), each fetch bounded by provider_timeout_seconds — tick wall-clock = slowest provider, not the sum. - Circuit breaker wired: Phase 0's ProviderHealth now gates polling; cooldown-skipped providers land in the new TickReport.skipped (neither success nor failure). Backoff cap fixed at 1800s (§6.5). - §6.6 fiat allowlist (new price::fiat): ISO-4217 active codes plus the non-ISO fiat Yadio really trades (IRT, GGP/IMP/JEP, MLC, XCG). Against the live Yadio feed this drops exactly BTC/XAU/XAG/XPT. VEF is deliberately excluded (pre-redenomination unit vs VES); the CoinGecko request list omits it for the same reason. Phase 2 acceptance tests (§9): median+outlier across 4 providers, lowercase/uppercase combine, official-CUP scoped out by `except` (2-source case the outlier guard can't save), non-fiat dropped by allowlist, provider-down fallback, breaker open-skip and reset-after-success. 431 tests green. Config: settings.tpl.toml gains the three §7 provider blocks (currency_api ships except = ["CUP", "MLC"]). El Toque remains rejected at startup until Phase 3. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughThis PR implements Phase 1 of a multi-source Bitcoin/fiat pricing architecture by introducing per-provider circuit breaker cooldown tracking, concurrent polling with timeouts, fiat allowlist validation, and three new price provider adapters (CoinGecko, CurrencyApi, Blockchain), while enriching tick reporting with skipped provider tracking. ChangesMulti-source Price Provider Architecture
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4efc1e915c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/price/manager.rs (1)
190-241: Add tracing spans around concurrent provider polling inPriceManager::update_all
- The concurrent path logs per-provider outcomes with
info!/warn!/debug!, but there’s notracingspan context (span!/info_span!/#[instrument]) around thejoin_allpolling, making interleaving harder to correlate.- Add a tick-level span plus per-provider child spans (keyed by
ProviderId) around the fetch/timeout so successes, errors, and timeouts are grouped cleanly.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/price/manager.rs` around lines 190 - 241, Add a tick-level tracing span around the whole polling block in PriceManager::update_all (create an info_span like info_span!("price_tick", tick = %now) or similar) and instrument the concurrent futures with per-provider child spans keyed by ProviderId inside the join_all map (create a per-provider span e.g. info_span!("provider_fetch", provider_id = %p.id) and use tracing::Instrument to .instrument that async closure or the fetched future). Ensure you import tracing::Instrument and use the span so the existing info!/warn! logs inside the match are correlated to the per-provider child span; keep the timeout wrapping and outcome handling logic unchanged.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/price/manager.rs`:
- Line 178: The timestamp `now` is captured too early (let it not be reused
after network polling); update the failure path so that `record_failure` gets a
fresh timestamp at the moment of failure rather than the earlier `now`
variable—either call Utc::now().timestamp() immediately before invoking
`record_failure` or change `record_failure` (and its callers) to compute the
current time inside itself; ensure references to `provider_timeout_seconds`,
`now`, and the `record_failure` invocation in `manager.rs` are updated so
backoff uses the real failure-time timestamp.
---
Nitpick comments:
In `@src/price/manager.rs`:
- Around line 190-241: Add a tick-level tracing span around the whole polling
block in PriceManager::update_all (create an info_span like
info_span!("price_tick", tick = %now) or similar) and instrument the concurrent
futures with per-provider child spans keyed by ProviderId inside the join_all
map (create a per-provider span e.g. info_span!("provider_fetch", provider_id =
%p.id) and use tracing::Instrument to .instrument that async closure or the
fetched future). Ensure you import tracing::Instrument and use the span so the
existing info!/warn! logs inside the match are correlated to the per-provider
child span; keep the timeout wrapping and outcome handling logic unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: dd14089c-e09e-4e3e-b4a4-bf0d992a5e0b
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (13)
Cargo.tomldocs/PRICE_PROVIDERS.mdsettings.tpl.tomlsrc/price/fiat.rssrc/price/manager.rssrc/price/mod.rssrc/price/providers/blockchain.rssrc/price/providers/coingecko.rssrc/price/providers/currency_api.rssrc/price/providers/mod.rstests/fixtures/price/blockchain_ticker.jsontests/fixtures/price/coingecko_simple_price.jsontests/fixtures/price/currency_api_btc.json
… breaker timestamps Address PR #773 review findings: - Codex P2 (valid): the outer per-provider timeout equalled the reqwest per-request timeout, so a *hanging* currency_api primary consumed the whole budget and the fallback_urls were dead code in exactly the hung case (they only worked for fast failures like connection-refused). New `poll_budget(id)` sizes the outer tokio timeout to provider_timeout_seconds × (1 + fallback_urls) + 1s slack; the per-attempt bound remains the shared reqwest client's request timeout. Adds a manager unit test for the scaling and an adapter-level axum test proving a hung primary is cut at the request timeout and the mirror still answers. - CodeRabbit Major (valid, minor impact): record_failure reused the pre-poll `now`, so breaker cooldowns were born already aged by up to a full poll budget. Failures are now stamped after polling completes. - CodeRabbit nit (declined): tracing spans around the polling — the codebase uses zero spans anywhere; per-provider outcomes already log the provider id inline, so spans here would be inconsistent house style for little correlation gain. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
@codex review |
|
Codex Review: Didn't find any major issues. 🚀 ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
![mostronatorcoder[bot]](https://avatars.githubusercontent.com/in/2922838?s=60&v=4){kind=small}
There was a problem hiding this comment.
Strict review: requesting changes.
-
src/price/fiat.rs includes
XCGin the fiat allowlist and in the "live Yadio codes survive" regression test.XCGis not an ISO-4217 fiat code and it is not documented anywhere in the spec as an intended exception, unlikeIRT,GGP,IMP,JEP, orMLC. Because Phase 2 now treats this allowlist as the gate before aggregation and Nostr publish, adding an undocumented/non-standard code here silently expands supported currencies and provenance surface. IfXCGis actually intentional, it needs spec/docs justification and provider-specific rationale; otherwise it should be removed from both the allowlist and the regression fixture expectation. -
I also verified the previously raised fallback-timeout issue and the stale timestamp issue are addressed in the current head:
poll_budget()now covers the full mirror sequence, and breaker failures are stamped with a fresh post-poll timestamp.
I am not approving with the current XCG allowlist entry unresolved.
arkanoider
left a comment
arkanoider
left a comment
There was a problem hiding this comment.
I tested with different scenarios:
- more than 1 provider working
- 2 working and one fake
- closing internet connection during retries
all seems good!
tACK!
codaMW
left a comment
codaMW
left a comment
There was a problem hiding this comment.
Tested on regtest (no LND needed for price ticks, as noted). tACK with evidence and I think the XCG hold can be resolved.
What I ran:
- cargo test --bin mostrod fiat all price/fiat tests pass, including live_yadio_codes_survive_except_non_fiat (the one that asserts the captured Yadio feed survives the allowlist except BTC/XAU/XAG/XPT).
- Full suite: 432 pass. The single failure is test_lnurl_validation_with_test_server (AddrInUse / port bind) it's in src/lightning/invoice.rs, untouched by this PR, and fails identically on upstream/main, so it's a pre-existing local-env flake, not introduced here.
- Live multi-source tick (all four providers enabled): one steady-state tick logged
price: coingecko ok (44 currencies)
price: blockchain ok (29 currencies)
price: currency_api ok (342 currencies)
price: yadio ok (127 currencies)
price: published exchange rates to Nostr (161 currencies)
Published count (161) is larger than any single provider the union-of-coverage aggregation works as specified. - Legacy path (no [price] block, just bitcoin_price_api_url) also still ticks single-source Yadio unchanged the migration path is intact.
On the XCG hold:
I think XCG actually belongs in the allowlist, and removing it would regress a live currency. XCG is a standard ISO 4217 code it was assigned by ISO 4217 Amendment 176 (numeric 532) for the Caribbean guilder (Curaçao and Sint Maarten), effective 31 March 2025, replacing ANG. So it's correctly placed in the standard KNOWN_FIAT list rather than the documented non-ISO exceptions (IRT/GGP/IMP/JEP/MLC) that's why it isn't in that exceptions comment; it doesn't need to be.
The X prefix is the likely source of confusion (it groups visually with XAU/XAG/XPT, which are correctly dropped as metals), but XCG is a real circulating fiat same category as XCD (East Caribbean dollar), XAF/XOF (CFA francs) and XPF, all already uncontested in the list.
Concretely: XCG is in the captured live Yadio fixture in this PR's own test, and it showed up in my live tick (price: XCG now has a single source ... then published). So dropping it would violate the PR's stated invariant "Phase 2 must not silently drop a currency nodes were publishing yesterday" by removing a currency the node is publishing today.
If a doc nit helps, a one-line comment next to XCG ("ISO 4217 Amendment 176, eff. 2025-03-31, Caribbean guilder, replaces ANG") would pre-empt the same question in future.
3 participants
Phase 2 of
docs/PRICE_PROVIDERS.md— direct backup quoters + multi-source aggregationPhase (spec §8): 2 — depends on Phase 1 (#753, merged). With this PR the price system becomes genuinely multi-source: three keyless direct quoters join Yadio, providers are polled concurrently behind a circuit breaker, and the §6.6 normalisation pipeline (fiat allowlist + per-provider scoping) guards the aggregate.
Providers added (each: one adapter file + one registry arm + one config block + one captured fixture, per §5.4)
coingeckoGET {url}/simple/price?ids=bitcoin&vs_currencies=…api_key(demo/pro header picked from host), redacted fromDebug/logs (§10.3)currency_apiGET {url}/currencies/btc.min.jsonfallback_urlsmirrors tried in order (§7)blockchainGET {url}/tickerlast(mid-market) only;buy/sellnot even deserialised (§6.6/§11.6)Core changes
update_allpolls all breaker-available providers in parallel; tick wall-clock = slowest single provider, each fetch bounded byprovider_timeout_seconds.ProviderHealthnow gates polling. Afterprovider_failure_thresholdconsecutive failures a provider is skipped (TickReport.skipped, logged asskipped: cooldown) for an exponentially-backed-off cooldown capped at 1800 s; one success resets it.src/price/fiat.rs): ISO-4217 active codes + the non-ISO fiat Yadio really trades (IRT,GGP/IMP/JEP,MLC,XCG). Validated against the live Yadio feed: drops exactlyBTC/XAU/XAG/XPTand nothing else.VEFdeliberately excluded (pre-redenomination unit; mixing it withVESwould price orders with a garbage rate).except = ["CUP", "MLC"], and a test proves scoping (not the outlier guard) is what saves the 2-source case.Test evidence (§12)
eth/bnbdropped by allowlist · provider-down fallback ·fallback_urlstried before failing (local-axum integration test) · breaker opens after N failures / skips without polling / resets on success.cargo fmt+clippyclean.🧪 Manual testing — step by step
Prereqs: a working
mostroddev setup (LND not required for price checks; the daemon will log price ticks regardless of trading activity).1. Multi-source happy path
settings.toml, add (or uncomment fromsettings.tpl.toml) the[price]block with all four providers enabled. For a fast feedback loop setupdate_interval_seconds = 60:RUST_LOG=mostrod=info cargo run -- -d <your-data-dir>and watch one tick. Expected log lines:ok; the published currency count is larger than any single provider (union of coverage).nak req -k 30078 -a <your mostro pubkey> <relay>): thesourcetag should readblockchain,coingecko,currency_api,yadio(alphabetical), and theBTCmap must not containETH,BNB,XAUorBTCitself (allowlist proof).2. Kill one API, watch the others carry the currency (spec §12 check)
price: coingecko error: …while the other three still reportok— USD/EUR keep updating (last-known-good never needed).provider_failure_threshold = 3failures):price: coingecko skipped: cooldown (circuit breaker open)— the dead API is no longer even polled, and the tick is not slowed down by its timeout.sourcetag dropscoingeckobut keeps the other three.coingecko okagain (breaker resets on success).3. Mirror fallback (
fallback_urls)price: currency_api mirror https://127.0.0.1:1 failed: …warn, followed byprice: currency_api ok (…)— the mirror carried the fetch, the provider is not marked failed, breaker stays closed.4. CUP stays on the informal market
CUP/BTC ÷ USD/BTC. Expected: ~400 CUP/USD (informal market — Yadio only).except = ["CUP", "MLC"]from[price.providers.currency_api]and restart. The CUP/USD ratio collapses to roughly half (~213 CUP/USD) because the official ~26 rate is now averaged in — this is exactly the corruption the shippedexceptprevents. Put theexceptback.5. Startup validation still guards Phase 3
mostrodrefuses to start withprice: provider 'eltoque' is configured (enabled) but not yet implemented in this release…. Remove the block.6. Legacy config untouched (regression)
[price]section so only the legacy[mostro].bitcoin_price_api_urlremains.price: yadio ok …), orders price normally — the migration path is untouched by this PR.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Improvements
Documentation
Chores
Tests