Skip to content

Quote stream/only emit better results#4648

Open
fleupold wants to merge 9 commits into
mainfrom
quote_stream/only_emit_better_results
Open

Quote stream/only emit better results#4648
fleupold wants to merge 9 commits into
mainfrom
quote_stream/only_emit_better_results

Conversation

@fleupold

@fleupold fleupold commented Jul 20, 2026

Copy link
Copy Markdown
Contributor

Description

Currently we emit every quote as part of the streaming quote api. This has been flagged as unergonomic from partners that want to integrate. They are not interested in receiving errors and potentially lower quality quote updates, they only want to see an increasingly better quotes.

This PR implements this by deferring errors to the end and only emitting new results if they are better as the best one so far

Important

Inline with the non streamed endpoint, verified quotes are considered "better" than an unverified one regardless of out amount. This is because unverified quotes often suffer from overquoting (and promising a price that is "too good to be true).

Changes

  • Move streamed quoting logic into the price estimation module so it can share the ranking logic
  • Make it re-use the same ranking and error classification logic as the classic quote endpoint
  • Defer emission of errors until it's clear no Ok result can be returned

How to test

Added unit tests

@fleupold
fleupold requested a review from a team as a code owner July 20, 2026 14:52
@claude

claude Bot commented Jul 20, 2026

Copy link
Copy Markdown
Contributor

Claude finished @fleupold's task in 4m 17s —— View job


PR Review: Quote stream / only emit better results

  • Read PR diff and changed files
  • Analyze ranking / error-classification logic
  • Review for correctness, edge cases, style
  • Post inline comments and summary

The refactor is clean and the tests are well-written and readable. The consolidation of is_reasonable, unreasonable_estimates_error, and compare_quote_result between the one-shot and streaming paths is a nice improvement, and the terminal-error behavior correctly mirrors estimate.

I left 2 inline comments. The main one is worth an explicit decision:

  • Monotonicity vs. verified preference (quote.rs): under QuoteVerificationMode::Prefer (the production default), a later verified quote with a lower out_amount beats an earlier unverified one and gets emitted, so the client can see the price go down — contradicting the "monotonically improving / never a regression" promise in the doc comment and the PR's stated goal. Either soften the doc or add a non-decreasing guard, and add test coverage for the Prefer interaction (current stream tests only use MaxOutAmount/Unverified).
  • Observability gap (minor, pre-existing): the streaming path emits no PriceEstimateEvent / WinningPriceEstimateEvent, unlike the classic endpoint.

Nothing else stood out — timeout handling (full timeout per estimator vs. divided in the sequential one-shot) and ignoring usable_results_for_early_return both look intentional for a streaming API.

Note: I could not run cargo check/tests or cargo +nightly fmt in this review environment, so I did not verify compilation locally — CI should cover that.

@linear-code

linear-code Bot commented Jul 20, 2026

Copy link
Copy Markdown

BE-48

@github-actions

Copy link
Copy Markdown

Reminder: Please consider backward compatibility when modifying the API specification.
If breaking changes are unavoidable, ensure:

  • You explicitly pointed out breaking changes.
  • You communicate the changes to affected teams (at least Frontend team and SAFE team).
  • You provide proper versioning and migration mechanisms.

Caused by:

Comment thread crates/price-estimation/src/competition/quote.rs Outdated
Comment thread crates/price-estimation/src/competition/quote.rs Outdated

@jmg-duarte jmg-duarte left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, just suggestions

Comment thread crates/price-estimation/src/competition/quote.rs
Comment thread crates/price-estimation/src/competition/mod.rs Outdated
Comment thread crates/price-estimation/src/competition/quote.rs Outdated
Comment thread crates/price-estimation/src/competition/mod.rs

@MartinquaXD MartinquaXD left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change makes sense to me but runs into the inherent awkwardness of unverified vs verified quotes in the streaming context.

Comment thread crates/price-estimation/src/competition/mod.rs
Comment thread crates/price-estimation/src/competition/quote.rs Outdated
Comment thread crates/price-estimation/src/competition/quote.rs Outdated
Comment thread crates/price-estimation/src/competition/quote.rs Outdated
@fleupold

Copy link
Copy Markdown
Contributor Author

@claude can you re-review please?

@claude

claude Bot commented Jul 24, 2026

Copy link
Copy Markdown
Contributor

Claude finished @fleupold's task in 3m 28s —— View job


Re-review: Quote stream / only emit better results

Re-reviewed after the latest changes. The concerns from the previous round have been addressed well.

Resolved since last review

  • Concurrency regression (MartinquaXD): context fetching and estimator calls run concurrently again via the select loop (quote.rs:130-149), so an uncached native price no longer serializes ahead of the quotes. I checked that dropping the estimates.next() future in the Either::Left arm does not drop the in-flight estimator futures (they're owned by the FuturesUnordered, not by Next), so no results are lost. 👍
  • Prefer-mode coverage (squadgazzz): estimate_stream_only_forwards_improving_quotes now uses QuoteVerificationMode::Prefer and genuinely exercises the verified-vs-unverified branch — 100 → 200 → (error dropped) → 150 verified, proving a lower-amount verified quote supersedes a higher unverified one and errors are held back. The verified/unverified branch in compare_quote_result runs before ranking, so MaxOutAmount is fine for triggering it.
  • Monotonicity vs. verified preference: the doc comment (quote.rs:93-108) and the OpenAPI description now explicitly state a verified quote outranks an unverified one regardless of out_amount, so a later quote may report a lower amount. Documentation now matches behavior.
  • Option::is_none_or() nit applied (quote.rs:164).
  • OpenAPI changes are documentation-only (summary/description text) — no schema breaking change, so the backward-compat bot reminder doesn't apply here.

Correctness checks I ran through

  • Terminal-error path (quote.rs:184-190) mirrors one-shot estimate exactly (same is_err() || is_reasonable filter, same compare_quote_result ranking, same fallback error). best.is_none() correctly implies no reasonable quote was ever seen, so the terminal branch only ever yields an error — no double-emit, no missed first quote.
  • Buffered-then-live replay: every result is pushed exactly once into results; buffered results replay in arrival order before live draining. No duplication.
  • "Strictly better" is enforced via .is_gt(), so equal-ranked quotes aren't re-emitted.

Remaining (non-blocking)

  • Observability gap + missing #[instrument] on estimate_stream — details in the inline comment. Pre-existing, optional.

Nice refactor overall — sharing is_reasonable / unreasonable_estimates_error / compare_quote_result between the two paths keeps them honestly in sync, and the tests are readable.

Note: cargo check/tests and cargo +nightly fmt require an approval not available in this review environment, so I did not verify compilation locally — CI covers that.
· quote_stream/only_emit_better_results

Comment thread crates/price-estimation/src/competition/quote.rs
@fleupold
fleupold force-pushed the quote_stream/only_emit_better_results branch from 59a421e to 4d1d121 Compare July 24, 2026 15:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants