feat(price): Phase 4 — unify live quote path on cache + enforce staleness#783
feat(price): Phase 4 — unify live quote path on cache + enforce staleness#783grunch wants to merge 1 commit into
Conversation
…ness Completes Phase 4 of docs/PRICE_PROVIDERS.md. Live path → cache: - `get_market_quote` is now a synchronous cache read, `sats = (fiat_amount / get_price(ccy)) × 1e8` with the premium applied, instead of a live Yadio `/convert` call per take. No network I/O on the take path. - Delete the dead live-path code: `retries_yadio_request`, `yadio_base_url`/`select_yadio_base_url`/`normalize_base_url`, `MAX_RETRY`, `FiatNames`, and the `Yadio`/`Request` response model (`src/models.rs` removed). With the live path gone, the legacy `[mostro].bitcoin_price_api_url` now feeds only legacy synthesis. - `get_market_amount_and_fee` becomes sync and returns `MostroError` so callers can distinguish the stale case. Staleness enforcement (spec §6.4): - `PriceManager::get_price` returns `ServiceError::PriceTooStale` for a value older than `max_price_staleness_seconds` (still warning once on the transition) instead of serving it. - Order create (`app/order.rs`) and market-priced takes (`app/take_buy.rs`, `app/take_sell.rs`) map it onto a user-facing `CantDoReason::PriceTooStale`. Depends on the new error variants in mostro-core 0.13.1 (MostroP2P/mostro-core#153). Tests: new `stale_price_is_refused_but_fresh_is_served`; full suite green (488 tests), clippy clean. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015NvSncWrjedLiAtAV4DTJR
WalkthroughPhase 4 of the multi-source BTC/fiat price rollout: ChangesPhase 4 Price Staleness Enforcement
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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: 2397684c18
ℹ️ 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".
| // If the order amount is zero, calculate the market price in sats | ||
| if order.has_no_amount() { | ||
| match get_market_amount_and_fee(order.fiat_amount, &order.fiat_code, order.premium).await { | ||
| match get_market_amount_and_fee(order.fiat_amount, &order.fiat_code, order.premium) { |
There was a problem hiding this comment.
When taker bonds are enabled, this call snapshots order.amount and fees before creating the bond; request_taker_bond stores those values in the bond's taker_* columns, and promote_taker_context_to_order later copies them when the invoice is accepted. If the taker pays the bond after max_price_staleness_seconds has elapsed, the trade still proceeds with that old market quote instead of returning PriceTooStale, so bonded market-priced takes can bypass the Phase 4 staleness guarantee. Revalidate/reprice before promoting the bond, or expire/reject stale bond snapshots.
Useful? React with 👍 / 👎.
![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/util.rs (1)
1071-1081: ⚡ Quick winAdd
///docs forget_market_amount_and_feeThis public helper returns a tuple with positional semantics; add a short doc comment describing tuple order and error behavior.
As per coding guidelines: "Document non-obvious public APIs with
///doc comments."🤖 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/util.rs` around lines 1071 - 1081, Add `///` doc comments to the public function `get_market_amount_and_fee` to document its non-obvious behavior. Document each parameter (fiat_amount, fiat_code, and premium), clearly explain the return tuple's positional semantics (first element is satoshi amount, second element is the fee), and document the error conditions that may result in MostroError. Place the doc comments immediately before the function signature.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`:
- Around line 401-417: The `warned_stale` state is currently shared between two
different warning paths: the within-TTL stale warning in the `observe_freshness`
method (around line 448) and the past-TTL warning in the current block (around
line 401). This causes the past-TTL transition warning to be suppressed if the
within-TTL stale warning has already been issued for the same key. To fix this,
separate the warning state by creating two distinct warning trackers instead of
sharing one `warned_stale` variable - one specifically for tracking within-TTL
stale warnings and another for tracking past-TTL refused warnings. Update the
`mark_warned` calls to use the appropriate warning state based on the context.
---
Nitpick comments:
In `@src/util.rs`:
- Around line 1071-1081: Add `///` doc comments to the public function
`get_market_amount_and_fee` to document its non-obvious behavior. Document each
parameter (fiat_amount, fiat_code, and premium), clearly explain the return
tuple's positional semantics (first element is satoshi amount, second element is
the fee), and document the error conditions that may result in MostroError.
Place the doc comments immediately before the function signature.
🪄 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: fa0736ae-9e26-4d39-82b2-e0665d10ac42
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (9)
Cargo.tomldocs/PRICE_PROVIDERS.mdsrc/app/order.rssrc/app/take_buy.rssrc/app/take_sell.rssrc/main.rssrc/models.rssrc/price/manager.rssrc/util.rs
💤 Files with no reviewable changes (2)
- src/models.rs
- src/main.rs
| if self.mark_warned(&self.warned_stale, &key) { | ||
| let age = self | ||
| .store | ||
| .snapshot(currency) | ||
| .map(|e| now.saturating_sub(e.as_of)); | ||
| match age { | ||
| Some(age) => warn!( | ||
| "price: {} is past the staleness window ({}s old) — refusing", | ||
| currency, age | ||
| ); | ||
| ), | ||
| None => warn!( | ||
| "price: {} is past the staleness window — refusing", | ||
| currency | ||
| ), | ||
| } | ||
| Ok(entry.value) | ||
| } else { | ||
| // Should not happen: TooStale means an entry exists, | ||
| // but tolerate the race in case the entry was wiped | ||
| // between get and snapshot. | ||
| Err(MostroError::MostroInternalErr(ServiceError::NoAPIResponse)) | ||
| } | ||
| Err(MostroError::MostroInternalErr(ServiceError::PriceTooStale)) |
There was a problem hiding this comment.
Past-TTL transition warning can be suppressed after a prior within-TTL stale read
warned_stale is shared between the past-TTL path (Line 401) and observe_freshness (Line 448). If the key is already inserted by the within-TTL stale warning, the PriceError::TooStale branch will skip its transition warning.
Consider separate warning state for “within-TTL stale” vs “past-TTL refused”.
🤖 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 401 - 417, The `warned_stale` state is
currently shared between two different warning paths: the within-TTL stale
warning in the `observe_freshness` method (around line 448) and the past-TTL
warning in the current block (around line 401). This causes the past-TTL
transition warning to be suppressed if the within-TTL stale warning has already
been issued for the same key. To fix this, separate the warning state by
creating two distinct warning trackers instead of sharing one `warned_stale`
variable - one specifically for tracking within-TTL stale warnings and another
for tracking past-TTL refused warnings. Update the `mark_warned` calls to use
the appropriate warning state based on the context.
codaMW
left a comment
codaMW
left a comment
There was a problem hiding this comment.
Reviewed on regtest against mostro-core 0.13.1. The core Phase 4 change is sound, I verified the mechanics, demonstrated the staleness enforcement live, and dug into the two open bot findings. One (the bonded-take bypass) is real and I was able to reproduce it end-to-end; details below. Not blocking from my side, but worth a decision on scope before merge.
Mechanics
• Builds clean against mostro-core 0.13.1 (Rust 1.93); cargo clippy --all-targets --all-features -- -D warnings clean.
• cargo test: 487 pass. The one failure is test_lnurl_validation_with_test_server (AddrInUse), a pre-existing port-bind flake unrelated to this PR (fails on main too).
• The new stale_price_is_refused_but_fresh_is_served unit test passes.
Staleness enforcement verified live. With a [price] block (max_price_staleness_seconds set low) and the cache aged past the window, the daemon refused market-priced requests with price: is past the staleness window (s old) refusing and returned CantDoReason::PriceTooStale. So the core guarantee works at runtime, not just in the unit test.
Config gate worth flagging. Phase 4 enforcement only engages when a [price] block is present; on a legacy config (no [price]), the daemon runs the legacy synthesis path and market orders proceed regardless of cache age (confirmed at runtime orders on a no-[price] config were not refused). Suggest documenting that Phase 4 protection requires migrating to a [price] block, or consider a sensible default guard for legacy configs.
On the Codex P2 finding (bonded takes bypassing staleness) confirmed by code trace AND reproduced end-to-end. Code path: take_buy runs the staleness check in get_market_amount_and_fee (take_buy.rs:122), then request_taker_bond snapshots the quote into bond.taker_amount (bond/flow.rs:183). When the bond locks, promote_taker_context_to_order copies that snapshot into the order (order.amount = bond.taker_amount, bond/flow.rs ~1069) with no re-pricing or staleness re-check. Reproduced with two funded regtest LND nodes + a mobile-app taker, apply_to = take, max_price_staleness_seconds = 60:
- Took a market-priced order while the price was fresh -> bond snapshotted taker_amount = 33196.
- Blocked the price provider so the cache aged past the window (direct market takes were refused with price_too_stale throughout the stale period).
- ~10 min into the stale window, paid the bond hold invoice -> daemon logged Bond … locked, promoted the order to waiting-payment with amount = 33196, and issued the trade invoice for that amount, no staleness re-check, despite the price being stale the entire time. So a bonded market take completes on a pre-TTL quote that the direct path refuses. Scope: only bites with bonds enabled and a delayed bond payment, and the fix (revalidate/expire the snapshot before promoting) belongs to the bond flow rather than this price PR. I'd treat it as a tracked follow-up, not a blocker but the Phase 4 guarantee should probably be documented as direct path only until the bonded path re-checks.
On the CodeRabbit warned_stale finding confirmed, minor/logging-only. The past-TTL refuse path (manager.rs:401) and the within-TTL stale path (manager.rs:448) both call mark_warned(&self.warned_stale, key) on the same set, so a within-TTL warning already recorded for a key suppresses the past-TTL refusing log line. The refusal itself still fires (the Err(PriceTooStale) returns regardless) only the operator warning can be swallowed. Non-blocking; separate trackers would fix it.
Housekeeping: the branch currently shows merge conflicts against main needs a rebase before merge.
Net: the Phase 4 core is solid and I'm comfortable with it landing after the rebase. The bonded-take staleness gap is real but reasonably a follow-up rather than a blocker for this PR. Nice deletion of the whole live-Yadio retry path.
2 participants
Phase 4 of
docs/PRICE_PROVIDERS.mdUnifies the live market-quote path onto the multi-source cache and turns on staleness (TTL) enforcement. Depends on mostro-core 0.13.1 (MostroP2P/mostro-core#153), which adds
ServiceError::PriceTooStaleandCantDoReason::PriceTooStale.What changed
Live path → cache
get_market_quote(util.rs) is now a synchronous cache read —sats = (fiat_amount / get_price(ccy)) × 1e8with the premium applied — instead of a live Yadio/convertcall (with 4 retries + blockingthread::sleep) on every take. No network I/O on the take path.retries_yadio_request,yadio_base_url/select_yadio_base_url/normalize_base_url,MAX_RETRY,FiatNames, and theYadio/Requestresponse model (src/models.rsdeleted). With the live path gone, the legacy[mostro].bitcoin_price_api_urlnow feeds only legacy synthesis.get_market_amount_and_feeis now sync and returnsMostroErrorso callers can distinguish the stale case.Staleness enforcement (spec §6.4)
PriceManager::get_pricereturnsServiceError::PriceTooStalefor a value older thanmax_price_staleness_seconds(still warning once on the transition), instead of serving stale data.app/order.rs) and market-priced takes (app/take_buy.rs,app/take_sell.rs) map it onto a user-facingCantDoReason::PriceTooStale.Test evidence
stale_price_is_refused_but_fresh_is_served: past-TTL →PriceTooStale, within-TTL → served, never-seen currency →NoAPIResponse.clippy --all-targets --all-features -D warningsclean,cargo fmtapplied.How to test manually
Prereqs: an LND/regtest mostrod setup as usual, with a
[price]section enabled insettings.toml.1. Happy path — market-priced order prices from the cache (no live
/convert)mostrod. Wait for the first price tick (price: ... ok (N currencies)in the logs).0) in a fiat likeUSD, and take it.GET .../convert/...request is made during create/take (the old live call is gone). The only price HTTP traffic is the scheduler's periodic provider polls.2. Staleness refusal — past the TTL
settings.toml:mostrod, let it fetch once, then block provider refreshes so the cached value ages out — e.g. disable the providers or cut network to the price APIs (firewall/airplane) while keeping the daemon running.> 30swith no fresh tick, try to create a market-priced order (or take one) in a currency that aggregated earlier.CantDoReason::PriceTooStale(client shows a "price too stale" cant-do), not a priced order.price: <CCY> is past the staleness window (<n>s old) — refusingonce (one-shot warning), not per read.3. Recovery — fresh tick re-arms
4. Currency with no data
NoAPIResponse→ existing behaviour), distinct from the stale path.🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Documentation