fix(core)!: calculate price impact for split-route quotes by tvinagre · Pull Request #522 · propeller-heads/fynd

tvinagre · 2026-09-04T21:16:23Z

Summary

The worker now calculates price_impact_bps for linear routes and split routes.
The calculation uses token metadata from Route and the spot price that each swap state reports for (token_in, token_out).
The calculation processes split routes as token-flow graphs. It groups swaps by input token and, for validated routes, visits the groups in producer-before-consumer order.
The calculation does not read shared MarketState.
A calculation failure does not fail the quote. The worker omits price_impact_bps, logs the reason at debug level, and increments worker_pool_price_impact_calculations_total{pool,outcome}.
worker_pool_price_impact_duration_seconds{pool} records the calculation time.
Fynd continues to call ProtocolSim::spot_price(token_in, token_out) for compatibility. Tycho implementations use different direction and fee conventions, which can add a venue-dependent fee bias to price_impact_bps.
Path Frank-Wolfe uses estimate_probe_impact only to decide whether the split search should continue and to size its next probe. The worker does not use this heuristic as the quote metric.
This breaking change removes RouteResult::with_price_impact, the RouteResult price-impact field, and its internal accessor from fynd-core.
Tests cover linear and split routes, token decimals, missing token data, spot-price failures, and metric labels.

The worker's spot-price fallback only handled linear routes, on the assumption that path_frank_wolfe was the only algorithm that splits and it reports its own impact. water_fill also splits and reports none, so every split quote shipped without price_impact_bps. The fallback now walks any validated route as a token-flow graph: each branch collection takes its share of the ideal amount at its input token and multiplies it by the swap's own spot price, so a split route gets the same definition as a linear one. The grouping the validator uses is shared so the two cannot drift. When the walk cannot price a route the worker logs why at debug level instead of silently omitting the field. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

path_frank_wolfe stamped its own price impact onto RouteResult: an input-weighted mean of per-path impacts, each measured against the marginal price left behind by the paths allocated before it. Every other route got the worker's route-level number. The two disagree on split routes, and the PFW figure understates impact on paths that share a pool. The worker's route-level walk is now the only source of price_impact_bps, so the field means one thing regardless of algorithm. RouteResult loses its price_impact field and the with_price_impact/price_impact methods. PFW keeps compute_average_price_impact for probe sizing. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

Adds the worker_pool_price_impact_duration_seconds histogram, labelled by pool like the solve and queue-wait histograms. The walk reads one spot price per swap, and a pool that derives its spot price by probing a swap pays for that probe here, so the cost is worth watching before deciding whether a cached spot price is needed. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

github-actions · 2026-09-04T21:21:16Z

Breaking API Changes (Intentional)

Breaking API changes detected and declared in the PR title.
release-plz derives the bump from commits: verify a !-marked commit is in this PR so the release PR shows a minor bump (breaking changes on 0.x.x bump the minor).

semver-checks output

Checking fynd-bench-harness v0.102.3 -> v0.102.3 (no change; assume minor)
     Checked [   0.068s] 196 checks: 196 pass, 58 skip
     Summary no semver update required
    Finished [   0.150s] fynd-bench-harness
    Checking fynd-core v0.102.3 -> v0.102.3 (no change; assume minor)
     Checked [   0.998s] 196 checks: 195 pass, 1 fail, 0 warn, 58 skip

--- failure inherent_method_missing: pub method removed or renamed ---

Description:
A publicly-visible method or associated fn is no longer available under its prior name. It may have been renamed or removed entirely.
        ref: https://doc.rust-lang.org/cargo/reference/semver.html#item-remove
       impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.50.0/src/lints/inherent_method_missing.ron

Failed in:
  RouteResult::with_price_impact, previously in file fynd-core/src/types/quote.rs:1346
  RouteResult::with_price_impact, previously in file fynd-core/src/types/quote.rs:1346

     Summary semver requires new major version: 1 major and 0 minor checks failed
    Finished [   1.750s] fynd-core
    Checking fynd-test-fixtures v0.102.3 -> v0.102.3 (no change; assume minor)
     Checked [   0.102s] 196 checks: 196 pass, 58 skip
     Summary no semver update required
    Finished [   0.159s] fynd-test-fixtures
    Checking fynd-rpc v0.102.3 -> v0.102.3 (no change; assume minor)
     Checked [   0.100s] 196 checks: 196 pass, 58 skip
     Summary no semver update required
    Finished [   0.165s] fynd-rpc
    Checking fynd-rpc-types v0.102.3 -> v0.102.3 (no change; assume minor)
     Checked [   0.136s] 196 checks: 196 pass, 58 skip
     Summary no semver update required
    Finished [   0.228s] fynd-rpc-types
    Checking fynd-client v0.102.3 -> v0.102.3 (no change; assume minor)
     Checked [   0.161s] 196 checks: 196 pass, 58 skip
     Summary no semver update required
    Finished [   0.302s] fynd-client
    Checking fynd-tools-common v0.102.3 -> v0.102.3 (no change; assume minor)
     Checked [   0.068s] 196 checks: 196 pass, 58 skip
     Summary no semver update required
    Finished [   0.118s] fynd-tools-common

tvinagre · 2026-09-06T22:18:52Z

@@ -1331,26 +1331,12 @@ pub struct RouteResult {
    net_amount_out: BigInt,
    /// Effective gas price (in wei) at the time the route was computed.
    gas_price: BigUint,


@carloszanella this was a conscious decision to remove this from the interface, since it's useless rn. Pls lmk if you think it's a bad decision so I can rollback & mark this as deprecated

tvinagre · 2026-09-06T22:26:04Z

+    pub token_out: &'a Address,
+    /// Raw units of `token_in` the swap consumed.
+    pub amount_in: f64,
+    /// Human units of `token_out` per human unit of `token_in` before the trade, fee included.


Worth noticing that the spot calculation should, by definition, include fee, but this is not guaranteed and there might be protocols that exclude the fee from the calculation, skewing the price impact calculation.
If this happens, needs to be fixed on Tycho integration, not here.

Use route-owned token metadata during price-impact calculation. Clarify the reported spot-price contract, improve diagnostics and naming, and record bounded calculation outcomes.

Use precise names and documentation for the quote metric, the Path Frank-Wolfe probe heuristic, route token metadata, errors, and telemetry.

Document all probe exit conditions and use accurate reference-output and duration terminology in comments and test messages.

Troshchk

Thank you!

Troshchk · 2026-09-08T14:35:52Z

@@ -473,7 +492,7 @@ where
                            self.route_carries_no_swaps()
                        })?
                };


This was the most concerning for me, pls, check if this is expected

[IMPL #1]

Category: bug

Description: For a buy order the worker sets amount_in_raw to route.swaps().first().amount_in(), then passes it to route_price_impact as the route's total input. A split route's first branch collection holds several swaps that all consume the input token, so .first() captures only one branch and understates the total. The walk seeds reference_human_by_token[token_in] with this understated amount, so the reference output — and the resulting price impact — is wrong. Before this change split routes took their price impact from the algorithm's path allocations, not from this value, so the diff newly routes buy split routes through it. PathFrankWolfeAlgorithm::optimize_split does not gate on order side, so a buy order can produce a split route.

Plain English: An integrator submits a buy order that the solver fills by splitting the input across several pools in parallel. The quote comes back with a price-impact figure computed as if only one of those parallel legs had been funded, so the number is inflated and does not describe the trade the integrator is actually getting. A split buy should measure impact against the full amount going into all parallel legs.

Suggestion: For the buy branch, sum the input across the route's first branch collection (the swaps whose token_in is the route input token) rather than taking the first swap. The same value feeds the quote's reported amount_in, so this is worth confirming independently of price impact.

Scope: The .first() logic is pre-existing (the diff only renamed amount_in to amount_in_raw); the diff makes it worse by feeding it into split-route price impact. I found no OrderSide::Buy coverage in fynd-core/src, so I could not confirm buy split routes occur in practice.

Troshchk · 2026-09-08T14:35:52Z

+pub(crate) fn route_price_impact(
+    route: &Route,
+    amount_in_raw: &BigUint,
+    amount_out_raw: &BigUint,


[IMPL #2]

The f64 bound inside the swap loop shares the name of the &BigUint parameter used later for PriceImpactInputs. Rename the loop binding (e.g. swap_amount_in_raw) so the raw-float leg amount and the route-level input are not the same identifier.

Troshchk · 2026-09-08T14:35:52Z

+    let mut consumed_raw_by_token: FxHashMap<&Address, f64> = FxHashMap::default();
+    for leg in legs {
+        *consumed_raw_by_token
+            .entry(leg.token_in)
+            .or_insert(0.0) += leg.amount_in_raw;
    }
-    // Reject anything that is not a simple linear chain.
-    for pair in swaps.windows(2) {
-        if pair[0].token_out() != pair[1].token_in() {
-            return None;
+
+    let mut reference_human_by_token: FxHashMap<&Address, f64> = FxHashMap::default();
+    reference_human_by_token.insert(
+        inputs.token_in,
+        raw_to_human_units(inputs.amount_in_raw, inputs.input_decimals, AmountSource::RouteInput)?,
+    );
+    let mut reference_output_human = 0.0_f64;
+
+    for (token_in, collection) in branch_collections(legs, |leg| leg.token_in) {
+        let available_reference_input_human = reference_human_by_token
+            .get(&token_in)
+            .copied()
+            .ok_or_else(|| PriceImpactError::UnfedToken(token_in.clone()))?;
+        let consumed_input_raw_total = consumed_raw_by_token
+            .get(&token_in)
+            .copied()
+            .unwrap_or_default();


[SIMPLIFY #1]

Description: spot_reference_output builds an FxHashMap<&Address, f64> of raw input summed per token in a first pass, then reads it back inside the branch_collections loop. But branch_collections(legs, |leg| leg.token_in) already groups every leg for a token into one collection, so the per-token total is just the sum over that collection. The map is a second, eager copy of the same grouping the walk already has, and the reader must reconcile two ways of grouping by token_in.

Suggestion: Delete the consumed_raw_by_token map and its build loop; compute let consumed_input_raw_total: f64 = collection.iter().map(|leg| leg.amount_in_raw).sum(); at the top of each collection iteration. Removes a map allocation, a loop, and the unwrap_or_default() lookup (~8 lines). The <= 0.0 guard and ZeroConsumedInput behavior are unchanged; nothing is lost.

Troshchk · 2026-09-08T14:35:52Z

+                    amount_out_raw,
                    gas_estimate,
                    amount_out_net_gas,
                    block_info.clone(),


[TESTS #1]

Description: The diff rewrites the worker so a failed price-impact calculation logs, counts the outcome, omits price_impact_bps, and still returns a successful quote. Only the success path is asserted (test_quote_price_impact_for_split_route, price_impact_bps() == Some(400)); no worker test drives a route whose calculation fails and asserts the quote still succeeds with price_impact_bps() == None. This is the core guarantee the change establishes.

Suggestion: Add a worker test with a mock algorithm returning a route whose token map is empty (or whose spot price errors), then assert worker.quote(...) is Ok and quote.order().price_impact_bps() is None.

Troshchk · 2026-09-08T14:35:52Z

+            .copied()
+            .unwrap_or_default();
+        if consumed_input_raw_total <= 0.0 {
+            return Err(PriceImpactError::ZeroConsumedInput(token_in));


[TESTS #2]

Description: ZeroConsumedInput (a branch collection consuming zero raw input) and NoReferenceOutput (the reference output not being positive and finite) are both reachable through price_impact_from_spot_legs, which the tests already call directly, but neither has a test.

Suggestion: Add two price_impact_from_spot_legs cases — one with a leg whose amount_in_raw is 0.0 to hit ZeroConsumedInput, and one whose legs never reach inputs.token_out to hit NoReferenceOutput — each destructuring the returned error variant.

Troshchk · 2026-09-08T14:35:52Z

+use crate::types::{quote::branch_collections, ComponentId, Route, Swap};

-use crate::{feed::market_data::MarketData, types::Route};
+/// Identifies an amount that cannot be represented as a finite `f64`.


[DOCS #1]

The doc reads "Identifies an amount that cannot be represented as a finite f64", but the enum (RouteInput/RouteOutput/SwapInput) names which amount a value is, and its Display output ("route input", …) never mentions f64. Reword to the concept, e.g. "Which amount in the price-impact calculation a value refers to."

Troshchk · 2026-09-08T14:35:52Z

+    }
+
+    #[test]
+    fn test_favorable_execution_is_negative() {


[SLOP #2]

is_negative names the assertion, not the scenario; rename test_favorable_execution_is_negative to test_favorable_execution.

Troshchk · 2026-09-08T14:35:52Z

-        let huge = "1".to_string() + &"0".repeat(400); // ~1e400 > f64::MAX
-        let pi = price_impact_from_spot_product(1.0, &bu(&huge), &bu("1"), 0, 0);
-        assert_eq!(pi, None);
+    fn test_no_slippage_sell_rate_pool_reports_zero_impact() {


[SLOP #3]

reports_zero_impact names the expected result; rename test_no_slippage_sell_rate_pool_reports_zero_impact to test_no_slippage_sell_rate_pool.

Troshchk · 2026-09-08T14:35:52Z


    #[test]
-    fn solve_duration_metric_recorded() {
+    fn quote_duration_metric_recorded() {


[SLOP #4]

The rename dropped the project's test_ prefix that its sibling test_price_impact_metrics_recorded keeps; rename quote_duration_metric_recorded to test_quote_duration_metric_recorded.

tvinagre and others added 4 commits September 4, 2026 21:08

docs(core): note the price-impact walk in the worker pool row

c1c62be

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

tvinagre changed the title ~~fix(core): compute price impact for split routes~~ fix(core)!: compute price impact for split routes Sep 6, 2026

tvinagre commented Sep 6, 2026

View reviewed changes

tvinagre marked this pull request as draft September 6, 2026 22:53

tvinagre added 2 commits September 7, 2026 01:56

perf(core): remove market reads from price impact

9df9304

Use route-owned token metadata during price-impact calculation. Clarify the reported spot-price contract, improve diagnostics and naming, and record bounded calculation outcomes.

refactor(core): clarify price impact terminology

ea46c6d

Use precise names and documentation for the quote metric, the Path Frank-Wolfe probe heuristic, route token metadata, errors, and telemetry.

tvinagre changed the title ~~fix(core)!: compute price impact for split routes~~ fix(core)!: calculate price impact for split-route quotes Sep 7, 2026

docs(core): correct price impact descriptions

702ce7d

Document all probe exit conditions and use accurate reference-output and duration terminology in comments and test messages.

tvinagre marked this pull request as ready for review September 7, 2026 13:21

Troshchk reviewed Sep 8, 2026

View reviewed changes

fix(core): handle split-route input totals

5ddcfa0

Navigation Menu

Uh oh!

fix(core)!: calculate price impact for split-route quotes - #522

fix(core)!: calculate price impact for split-route quotes#522
tvinagre wants to merge 8 commits into
mainfrom
tl/fix-split-price-impact

tvinagre commented Sep 4, 2026 •

edited

Loading

Uh oh!

github-actions Bot commented Sep 4, 2026 •

edited

Loading

Uh oh!

tvinagre Sep 6, 2026

Uh oh!

tvinagre Sep 6, 2026

Uh oh!

Troshchk left a comment •

edited

Loading

Uh oh!

Troshchk Sep 8, 2026 •

edited

Loading

Uh oh!

Troshchk Sep 8, 2026

Uh oh!

Troshchk Sep 8, 2026

Uh oh!

Troshchk Sep 8, 2026

Uh oh!

Troshchk Sep 8, 2026

Uh oh!

Troshchk Sep 8, 2026

Uh oh!

Troshchk Sep 8, 2026

Uh oh!

Troshchk Sep 8, 2026

Uh oh!

Troshchk Sep 8, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

tvinagre commented Sep 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Uh oh!

github-actions Bot commented Sep 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Breaking API Changes (Intentional)

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Troshchk left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Troshchk Sep 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

tvinagre commented Sep 4, 2026 •

edited

Loading

github-actions Bot commented Sep 4, 2026 •

edited

Loading

Troshchk left a comment •

edited

Loading

Troshchk Sep 8, 2026 •

edited

Loading