feat(price): Phase 1 — Yadio provider + PriceManager wiring#753
Open
grunch wants to merge 4 commits into
Open
Conversation
…ource parity)
Second atomic PR of the multi-source price rollout
(docs/PRICE_PROVIDERS.md §9, Phase 1). Wires Phase 0's foundation into the
daemon at single-source parity: Yadio behind the new abstraction, the rest
of the daemon untouched outside the call sites.
Provider:
- src/price/providers/yadio.rs: YadioProvider via GET {url}/exrates/BTC,
lenient Option<f64> parse (drops `null` / non-finite rates, preserving
the db99f94 fix), parse() helper unit-tested against a captured fixture.
- tests/fixtures/price/yadio_btc.json: captured payload exercising
USD/EUR/ARS/CUP plus the `BGN: null` regression case.
Manager:
- src/price/manager.rs: PriceManager owning Vec<Box<dyn PriceProvider>> +
PriceStore + reqwest::Client. update_all() polls each provider with its
own tokio::time::timeout, runs aggregate_tick (applying per-provider
only/except scoping at the boundary so aggregate.rs stays generic), and
writes the store; failed providers contribute nothing so prior values
survive as last-known-good (§6.4). get_price() logs a single warn! when
a value ages past one update interval but still returns it — Phase 1
never refuses an order that would have priced today (§9, enforcement
lands in Phase 4). build_provider() is the single designated
extension point (§5.4 Step 3); unknown ids in config are warn-and-skip
(forward-compat); an enabled-but-unimplemented adapter fails startup.
- Nostr publishing preserved with the legacy {"BTC": {ccy: value}}
wrapper; the `source` tag becomes the contributing provider list
(deterministically sorted), so today it's "yadio" and Phase 2 widens
it without changing the schema.
- Process-wide PriceManager::global() singleton via OnceLock; installed
in main right after settings_init().
Wiring:
- scheduler::job_update_bitcoin_prices now drives PriceManager::update_all;
interval comes from [price].update_interval_seconds, MIN_INTERVAL guard
preserved.
- util::get_bitcoin_price reads through PriceManager.
- src/bitcoin_price.rs shrunk to the shim required by §9 Phase 1
(BitcoinPriceManager::get_price delegates to PriceManager); the rest
retires in Phase 5.
Config migration (§10.1):
- When [price] is absent, synthesise_legacy_price_settings() builds a
single yadio provider from bitcoin_price_api_url +
exchange_rates_update_interval_seconds + publish_exchange_rates_to_nostr,
so existing settings.toml files keep working byte-for-byte.
- settings.tpl.toml documents the new [price] block as opt-in and marks
bitcoin_price_api_url deprecated.
Tests (+11, 365 total green):
- single-yadio tick matches today, yadio-down keeps prior values, no
providers → NoAPIResponse (§9 Phase 1 acceptance criteria).
- only-scoping enforced at the manager boundary (§6.6).
- from_settings rejects enabled-but-not-implemented (CoinGecko etc.),
ignores unknown ids, skips disabled.
- legacy migration validates; deterministic source-tag ordering.
- Yadio fixture parse + null/non-finite drop + trailing-slash URL +
parse-error surfacing.
cargo fmt, cargo clippy --all-targets --all-features, cargo test all
green.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
|
Warning Review limit reached
More reviews will be available in 12 minutes and 13 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ 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: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughPhase 1 price subsystem: adds a process-global PriceManager with multi-provider aggregation and optional Nostr publishing, a Yadio provider adapter and fixtures, startup installation and legacy settings synthesis, and replaces the old BitcoinPriceManager with a compatibility shim. ChangesPrice Subsystem Phase 1 Implementation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 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 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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/scheduler.rs (1)
565-565: ⚡ Quick winUse the update report to surface provider failures.
At Line 565,
_reportis dropped. Please emit at least a warn/error from the returned report when no providers succeed (or when failures spike), so price-source outages are visible from scheduler logs.🤖 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/scheduler.rs` at line 565, The update report returned by manager.update_all() is currently ignored (assigned to _report); change this so you capture the returned report (e.g., report) and inspect its success/failure metrics and per-provider results, then emit a log.warn or log.error from the scheduler when no providers succeeded or when failure counts spike; locate the call to manager.update_all() and the binding of _report, read fields on the returned Report (or its methods) to determine total successes/failures and provider-level failures, and add conditional logging that surfaces which providers failed and summary counts so price-source outages are visible in scheduler logs.
🤖 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`:
- Around line 211-213: The Nostr `source` tag is being built from
report.successes (used in publish_rates_to_nostr and sources_to_tag) which can
include providers filtered out or excluded by aggregation; change the pipeline
so provider identities are those that actually contributed to the final
aggregates: modify aggregate_tick (or the aggregation code) to return
contributor metadata (e.g., a Vec of provider IDs or embed a contributors field
in the aggregate type), update callers (publish_rates_to_nostr and any place
building sources_to_tag) to use that returned contributor list instead of
report.successes, and adjust sources_to_tag to accept and format the actual
contributors; ensure function signatures (aggregate_tick,
publish_rates_to_nostr, sources_to_tag) are updated consistently and tests
updated accordingly.
- Around line 273-285: The TooStale branch currently logs on every call
(PriceError::TooStale in the match), which can spam logs; change this to a
one-shot warning by tracking whether we've already warned for that currency
(e.g., add a per-manager or per-store tracking structure like a HashSet/flag for
warned currencies), check that flag before calling warn! in the
Err(PriceError::TooStale) branch (use self.store.snapshot(currency) as now),
emit warn! only if not already warned, and set the flag after warning; also
clear/unset that flag when a fresh entry is served (where
self.store.snapshot(currency) returns a fresh entry) so future staleness can
warn again.
---
Nitpick comments:
In `@src/scheduler.rs`:
- Line 565: The update report returned by manager.update_all() is currently
ignored (assigned to _report); change this so you capture the returned report
(e.g., report) and inspect its success/failure metrics and per-provider results,
then emit a log.warn or log.error from the scheduler when no providers succeeded
or when failure counts spike; locate the call to manager.update_all() and the
binding of _report, read fields on the returned Report (or its methods) to
determine total successes/failures and provider-level failures, and add
conditional logging that surfaces which providers failed and summary counts so
price-source outages are visible in scheduler logs.
🪄 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: 8c9b5471-d76c-4688-9fa5-ec90d7c76aa6
📒 Files selected for processing (10)
settings.tpl.tomlsrc/bitcoin_price.rssrc/main.rssrc/price/manager.rssrc/price/mod.rssrc/price/providers/mod.rssrc/price/providers/yadio.rssrc/scheduler.rssrc/util.rstests/fixtures/price/yadio_btc.json
…r outage log Addresses the three review findings on #753. 1. Nostr `source` tag now reflects **actual contributors**, not the broader "polled successfully" list. A provider scoped out by `only`/`except` lands in `report.successes` (it did poll OK, so the Phase 2 circuit breaker stays happy) but **not** in the new `report.contributors`, so the kind-30078 `source` tag never names a provider that didn't move the aggregate. Contributors are computed at the manager boundary from the post-scope quote maps; tracking outlier-rejected individual quotes would require pairing every `Quote` with a `ProviderId` through the pure `aggregate_tick`, which is out of scope for Phase 1 and noted as a Phase 2 follow-up. 2. `Err(PriceError::TooStale)` in `get_price` no longer logs on every call. The single `warned_currencies` HashMap (which also collided `Stale` and `SingleSource` flags for the same currency) is split into two independent `HashSet`s — `warned_stale` and `warned_single_source` — so neither flag clobbers the other. The TooStale branch now warns at most once between fresh reads; a fresh `Ok` read clears `warned_stale` so a later regression past the TTL warns again. 3. The scheduler no longer drops the `TickReport`. PriceManager already logs each provider's outcome per tick, so the scheduler only surfaces the outage condition that ops cares about: an `error!` when **every** provider failed (the store is reading last-known-good across the board), and a `warn!` summary on partial outages. Tests (+2, 367 total): - `scoped_out_provider_is_success_but_not_contributor`: a provider whose only quote is filtered by `only` appears in `report.successes` but not in `report.contributors`. - `stale_warning_is_one_shot_then_re_arms_on_fresh_read`: 10 reads against a stale value populate `warned_stale` with one entry; a fresh tick + read clears it so future regressions warn again. cargo fmt, cargo clippy --all-targets --all-features, cargo test all green. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
|
Actionable comments posted: 0 |
![ermeme[bot]](https://avatars.githubusercontent.com/in/3690068?s=60&v=4){kind=small}
There was a problem hiding this comment.
Requesting changes: the source metadata is built from providers that merely returned non-empty quotes before aggregation. That means a provider whose quotes are later discarded as outliers can still be advertised as a contributor in the Nostr source tag. Please derive contributors from the quotes that actually survive aggregation, or have aggregate_tick return provenance so the tag reflects reality.
Addresses the Hermes review on #753: the prior fix computed contributors at the manager boundary as "post-scope non-empty quotes", which still claimed a provider as contributor when `combine`'s outlier filter dropped its value. The Nostr `source` tag advertised providers that had no effect on the aggregate. Threads provenance through the pure-function aggregation core instead: - `aggregate_tick` now takes `&[(ProviderId, ProviderQuotes)]`. Direct (PerBtc) quotes and resolved PerBase quotes carry their source id through the pipeline. - `AggregateResult` gains `contributors: Vec<ProviderId>` — sorted, deduplicated, and populated by the new `kept_contributors` helper, which mirrors `combine`'s "kept" predicate exactly: - n≤2: every clean provider contributes, - n≥3: only providers whose value is within `outlier_pct` percent of the median contribute, - bimodal even-length fallback: every clean provider stays (no single source is demonstrably the outlier; `combine` falls back to the median itself). - Fiat-cross resolution attributes the resolved candidate to the fiat-cross provider only. Anchor contributors (the direct quoters whose values built the USD/BTC anchor, say) are an intermediate of the cross math, not upstreams of the cross currency. - Manager derives the tick-wide Nostr `source` tag as the union of every per-currency contributor list — using a BTreeSet so the result is deterministic without re-sorting in `sources_to_tag`. The manager-boundary "non-empty post-scope" heuristic is gone. - `ProviderId` gains `Ord` / `PartialOrd` so the sorted contributor lists are stable. Tests (+3, 370 total): - `aggregate_tick_outlier_drops_provider_from_contributors`: three providers, one outlier (75_000 against a 50_000/50_200 median) — the outlier appears in `sources=3` but NOT in `contributors`. - `aggregate_tick_bimodal_fallback_keeps_all_clean_contributors`: four values across two clusters land in the bimodal fallback; all four providers stay contributors. - `aggregate_tick_non_finite_value_drops_provider_from_contributors`: a NaN-emitting provider is dropped from the cleaned set, not advertised as a contributor. Existing aggregate_tick tests assert on the new contributors field; the unions-partial-coverage test pins the CUP contributor list to [Yadio (direct), ElToque (fiat-cross)] — confirming the anchor's own contributors are NOT propagated into the resolved-currency tag. cargo fmt, cargo clippy --all-targets --all-features, cargo test all green. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/price/aggregate.rs (1)
470-493: ⚡ Quick winAssert the exact contributor list in the bimodal fallback test.
len() == 4won't catch a regression where this path returns the right set in nondeterministic order. Sincecontributorsis part of the contract now, it's worth pinning the full sorted vector here too.🧪 Tighten the assertion
approx(out["USD"].value, 51.0); // (2+100)/2 - // All four clean providers survive the bimodal fallback. - assert_eq!(out["USD"].contributors.len(), 4); + // All four clean providers survive the bimodal fallback, in deterministic order. + let mut expected = vec![ + ProviderId::Yadio, + ProviderId::CoinGecko, + ProviderId::CurrencyApi, + ProviderId::Blockchain, + ]; + expected.sort(); + assert_eq!(out["USD"].contributors, expected);🤖 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/aggregate.rs` around lines 470 - 493, The test aggregate_tick_bimodal_fallback_keeps_all_clean_contributors should assert the exact contributor list rather than just contributors.len(); replace the loose length check with a deterministic equality check of the sorted contributors vector so nondeterministic ordering can't hide regressions: extract the provider identifiers from out["USD"].contributors, sort them deterministically (e.g. by provider enum/name), and assert they equal the expected list [ProviderId::Yadio, ProviderId::CoinGecko, ProviderId::CurrencyApi, ProviderId::Blockchain]; update the test around aggregate_tick and the contributors field to perform that exact sorted equality check.
🤖 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.
Nitpick comments:
In `@src/price/aggregate.rs`:
- Around line 470-493: The test
aggregate_tick_bimodal_fallback_keeps_all_clean_contributors should assert the
exact contributor list rather than just contributors.len(); replace the loose
length check with a deterministic equality check of the sorted contributors
vector so nondeterministic ordering can't hide regressions: extract the provider
identifiers from out["USD"].contributors, sort them deterministically (e.g. by
provider enum/name), and assert they equal the expected list [ProviderId::Yadio,
ProviderId::CoinGecko, ProviderId::CurrencyApi, ProviderId::Blockchain]; update
the test around aggregate_tick and the contributors field to perform that exact
sorted equality check.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: cffa729e-e165-4367-99f6-43c14cec6b3f
📒 Files selected for processing (4)
src/price/aggregate.rssrc/price/manager.rssrc/price/provider.rssrc/price/store.rs
✅ Files skipped from review due to trivial changes (2)
- src/price/provider.rs
- src/price/store.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- src/price/manager.rs
Replace the loose `.len() == 4` check with the deterministic sorted-list assertion. `kept_contributors` -> `dedup_sort` orders by the derived `Ord` on `ProviderId` (which follows enum-variant declaration order: Yadio, CoinGecko, CurrencyApi, Blockchain, ElToque), so the expected result is fully determined. A future refactor that perturbs ordering or silently drops a contributor will now be caught here rather than slipping past the count. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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
1 participant
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
Second atomic PR of the multi-source price rollout (
docs/PRICE_PROVIDERS.md§9, Phase 1). Builds on Phase 0 (#747).PriceManagerowns the provider registry + store + HTTP client; the scheduler'sjob_update_bitcoin_pricesnow drivesPriceManager::update_all, andutil::get_bitcoin_pricereads through it.src/price/providers/yadio.rs(GET{url}/exrates/BTC, lenientOption<f64>parse sonullrates are dropped — preserves the d3f5bfa fix). Captured-payload fixture + parse tests.[price]is absent we synthesise a single-yadio config from[mostro].bitcoin_price_api_url+exchange_rates_update_interval_seconds+publish_exchange_rates_to_nostr, so existingsettings.tomlfiles keep working byte-for-byte.src/bitcoin_price.rsshrunk to the §9 Phase 1 shim (BitcoinPriceManager::get_pricedelegates toPriceManager); 423 lines removed, the rest retires in Phase 5.get_pricelogs a singlewarn!when a value ages past one update interval but still returns it — Phase 1 never refuses an order that would have priced today. Enforcement turns on in Phase 4.What is NOT in this PR
get_market_quoteunification + staleness enforcement — Phase 4.bitcoin_price.rsremoval — Phase 5.Single designated extension point (§5.4 Step 3)
build_provideris the single match arm a new provider touches; the aggregation core, the store, and the scheduler stay untouched. An enabled-but-unimplemented provider fails startup; an unknown id is warn-and-skipped (forward-compat with newer config from an older binary).Test evidence
NoAPIResponsematching today's "no data yet".only/exceptscoping (§6.6) enforced at the manager boundary.BGN: nullregression case.source-tag ordering across ticks.cargo fmt,cargo clippy --all-targets --all-features,cargo test(365 passing) all green.Test plan
cargo fmt --check && cargo clippy --all-targets --all-features -- -D warnings && cargo testgreen on CIsettings.toml(no[price]block) — confirm logs showPriceManager installed,price: yadio ok (N currencies)each tick, and orders that exerciseget_bitcoin_price(market-priced create) succeed[price.providers.yadio]block — same behaviouriptablesblock on api.yadio.io) — confirm the store keeps serving last-known-good values,warn!fires once the value ages past one update interval, and orders still succeed (Phase 1 logs but does not refuse)sourcetagyadio(now derived from the contributing-providers list, not hard-coded)🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Refactor
Bug Fixes / Improvements