feat: Graph performance improvements by carloszanella · Pull Request #414 · propeller-heads/fynd

carloszanella · 2026-08-10T16:43:26Z

I added a comment going over the changes: https://propeller-heads.atlassian.net/wiki/x/AQA15w

There is one larger commit: feat: Implement MostLiquid V2, designed with token sequences

The others should be easy to review in isolation, so I suggest reviewing one commit at a time

Benchmark results:

Baseline:

New solve times

louise-poole

Did a quick look through this morning - these are the first things I've noticed. Nothing else jumped out at me beyond this though. Will do a proper review next week

louise-poole · 2026-08-13T10:07:11Z

+        // Only the winner is built into swaps: that is what copies a component and a pool state per
+        // leg, and every other candidate would have thrown them away.
+        let best = match best_route {
+            Some((token_path, solved)) => {
+                let route = Self::build_route(&ctx, token_path, &solved)?;
+                if let Err(e) = route.validate() {
+                    trace!(error = %e, "best route failed validation");
+                    report.validation_failures += 1;
+                    None
+                } else {
+                    let amount_out = swap_on_route(&route, token_prices.as_deref(), &gas_price);
+                    Some(RouteResult::new(route, amount_out, gas_price.clone()))
+                }
+            }


And if the winner fails to build/validate? We should fallback to trying the next best route no?

that's a valid question. I looked into this and the potential validation reasons. They are:

swaps are empty: if the "best" is an empty list of swaps then we have no graph. it would've failed earlier

split validation: does not apply

The other two are: DisconnectedSwaps and UnsupportedCycle: from what I checked they are not possible by design on the algorithm and they would also imply a bug in graph logic

I can add a loop around this if you prefer. I made it like this given the above and the extra code to "solve the next best" which would be unreachable if I understood it correctly

louise-poole · 2026-08-13T10:10:08Z

-        // Track state overrides for components we've already swapped through.
-        let mut state_overrides: HashMap<&ComponentId, Box<dyn ProtocolSim>> = HashMap::new();


Why did you drop the state overrides? Sometimes a pool is re-used at different hops (thinking mainly for 3 token pools etc) and without these overrides we'll simulate the second hop on that pool as if it wasn't swapped on already which is incorrect.

I judged it is not worth the extra code plus performance impact of cloning things and cache misses. I added some extra code to filter paths where the same pool shows up twice

louise-poole · 2026-08-13T10:43:04Z

+    /// Sets whether a pair's simulation results are reused across one order's routes.
+    ///
+    /// On, a pair is asked once and the answer serves every route crossing it. Off, every route
+    /// asks every pool on the pair at the amount reaching it, which is exact but much slower.
+    pub fn with_cache_pair_swaps(mut self, enabled: bool) -> Self {
+        self.cache_pair_swaps = enabled;
+        self
    }


This looks related to the simulations overrides I mentioned already, but looks unused?

atm unused. Id like to expose this at the config level at some point. removing this code for now

It is much faster than stdlib hashing with no downsides for fynd usecase

Use the input parameter to loop through instead of the whole market

This graph collapses all edges between different token pairs into a single edge and tracks a mapping of tokens to pools between them for easy access. This makes traversing the graph much simpler and faster, and allows you to later combine the edge data to form the expanded list of paths

This allows for this data to stay on the heap in most cases and reduces drastically the cost of copying data every time a solver needs a snapshot of the market.

These were cloned and are big structs for cloning at every order request.

Main methods to list token paths and expand them into pool paths between two tokens. add tests.

Search, ranking and simulation all worked on concrete pool paths. With deep and densely connected market graphs solving orders in an algorithm that was supposed to be fast became slow and expensive. Using the new topology graph, I made a redesign of the core logic of the algorithm making it the fastest and most reliable algorithm in Fynd up to depth=4. Here's a summary of changes: - Interfaces with new topology graph - When solving token paths, each edge containing multiple pools is collapsed into a single best pool by swapping on them all and comparing net amounts - Added optional order-lifetime caching for these edges, in case the same pair A/B shows up in multiple token paths and/or at different hops. - Dont drop pools that dont have pool depth data anymore. Simulate all.

Before bellman ford would simulate all paths going out of token_in independent of whether they actually lead to the token out. Now there is a filter for reaching token_out at build_context. This gave it a nice speed increase. The side effect is before BF would not respect max_depth parameter, and now it does. bonus: Dont clone token objects, use the Arc method

This doenst require callers owning the strings which they might also borrow from market data. The data is not consumed in the methods so it is fine to not pass by value.

The expected-output baseline was recorded with a deeper hop budget than the repo's config allows. Seven of its scenarios route through three or four swaps, and `worker_pools.toml` ships a single pool at two hops, so those routes could not be produced at all: USDC_to_DAI_1000000, WETH_to_LINK_500 and UNI_to_LINK_150000 came out 3%, 7% and 14% below their recorded amounts and failed the 1% regression check. The harness now includes a config next to itself, with one Bellman-Ford pool at three hops. The project default is unchanged, and so is the copy of it inlined as DEFAULT_WORKER_POOLS_TOML in fynd-rpc. Note this costs the suite an implicit check: it no longer solves with the config the project ships, so the two can drift apart. Regenerating the baseline at two hops would restore that, at the cost of a baseline that exercises no route longer than two swaps. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

carloszanella · 2026-08-14T09:02:03Z

I force pushed after your comments just to rebase with latest main, didn't change any of the commits of this PR in the force push. all new changes you haven't reviewed are outside the force-push changes

These maps are keyed by pool name or token address and are built and read on the quote path. Neither needs SipHash's collision resistance — the keys come from the operator's own config and from the market, not from anything a caller controls — and the rest of the crate already reaches for `FxHashMap` in the same situation. This was required after rebasing with main

The module used both words for the same thing: `SwapOutcome` and `SolvedRoute.legs` sat next to `LegNotTradable` and a doc comment calling the same step a hop. A reader has to work out that they mean one concept before they can read anything else. Settles on hop, since that is what the rest of the crate says and what the type describing one step now says too: `SwapOutcome` -> `HopResult`, `legs` -> `hops`, `LegNotTradable` -> `HopNotTradable`, and the prose follows.

…g it `record` did two jobs: it constructed the `HopResult` to return and, as a side effect, wrote it into the cache. So the only way to build one was to go through the cache, and the doc had to explain that an off cache still returns a result it did not store.

The market lock checked `gas_price().is_none()` and returned `DataNotFound { kind: "gas price" }`, then the line that actually wants the value built the identical error again with `ok_or`. Two sites, one condition, and nothing between them can change the answer.

Abstract some of this method into its parts, making the code much more readable

A pool an earlier hop swapped through was offered to a later one, which priced the second swap as if the first had not moved the pool. The sequence then reported an output it could not deliver. It shows up on multi-token pools, where one component can serve both A -> B and B -> C, and on circular sequences, which cross the same pair in both directions. `solve_token_path` now records the pools a sequence has crossed and does not offer them again. Where that bites, the pool is picked directly rather than through `PoolSwapsCache`, and the cache is neither read nor written for that hop: what it holds was chosen over every pool, and the best of a narrowed field is not the answer the next sequence to reach this pair at this amount should be handed.

louise-poole · 2026-08-17T09:33:35Z

+
+    /// Every token path from `from` to `to` within the filter's hop bounds.
+    ///
+    /// Empty when there is no route. Check [`TopologyGraph::expand_path`] for expanded pool paths.


Personally I'm not such a fan of the [ ] in our doc strings. It breaks things up a bit for me as a human reader. The '`' characters are enough to indicate code is being referenced.

In the IDE it allows you to jump to the code definition or even read the docs of the function as a tool tip. Not sure if you knew that?

louise-poole · 2026-08-17T09:36:02Z

+    pub fn expand_path(
+        &self,
+        token_path: &[NodeIndex],
+        max_paths: Option<usize>,


Many of this places this is called use None here. I might have missed it, bit I didn't actually spot any callsites that set this value. Is it necessary to keep this logic here that no-one uses?

I will review this but I think I am using it in the waterfill PR

louise-poole · 2026-08-17T09:37:17Z

+        if filter.min_hops == 0 || filter.min_hops > filter.max_hops {
+            return Vec::new();
+        }


Would it not be better to error on a misconfiguration like this? Returning an empty vec just silently swallows a configuration error.

louise-poole · 2026-08-17T09:44:16Z

+            // The pool was simulated but not at this amount. We assume that amounts dont move
+            // so much so we assume the same winner wins again at a slightly different amount.


I feel this might be a risky assumption. Maybe we should buffer by how much it can differ? The top pool for a $0.1 trade can differ greatly to the top pool for a $100k trade.

Oh wait - we're caching the simulations per-order no? Then my comment above doesn't apply. Ignore this 😅

carloszanella force-pushed the cz/feat/improve-graph-r37 branch from 5e4cf85 to fec67cc Compare August 10, 2026 16:45

carloszanella force-pushed the cz/feat/algorithms-benchmark branch from ce9836c to 34f367c Compare August 10, 2026 16:52

carloszanella force-pushed the cz/feat/improve-graph-r37 branch 3 times, most recently from 39e779b to 5dfc585 Compare August 13, 2026 07:36

louise-poole reviewed Aug 13, 2026

View reviewed changes

carloszanella and others added 11 commits August 14, 2026 09:59

feat: Use FxHash at all hashed fields

75ea368

It is much faster than stdlib hashing with no downsides for fynd usecase

perf: Extract subset loops optimized

7b2624a

Use the input parameter to loop through instead of the whole market

feat: Replace path data types with SmallVec

4a7424b

This allows for this data to stay on the heap in most cases and reduces drastically the cost of copying data every time a solver needs a snapshot of the market.

feat: Use shared pointer (ARC) for component and token data

20b67e8

These were cloned and are big structs for cloning at every order request.

feat: Implement topology graph search methods

b23372c

Main methods to list token paths and expand them into pool paths between two tokens. add tests.

feat: Allow querying market data with reference IDs

8bed02b

This doenst require callers owning the strings which they might also borrow from market data. The data is not consumed in the methods so it is fine to not pass by value.

chore: peripherals that needed updating after recent changes

488a7b5

Base automatically changed from cz/feat/algorithms-benchmark to main August 14, 2026 09:01

carloszanella force-pushed the cz/feat/improve-graph-r37 branch from 5dfc585 to 0648090 Compare August 14, 2026 09:01

carloszanella added 7 commits August 14, 2026 15:49

refactor(most_liquid): Split find_best_route into its four stages

972090a

Abstract some of this method into its parts, making the code much more readable

docs: Improve some AI docs

f518fc1

carloszanella force-pushed the cz/feat/improve-graph-r37 branch from cb3e4d3 to f518fc1 Compare August 14, 2026 14:53

louise-poole approved these changes Aug 17, 2026

View reviewed changes

feat: Keep public API backwards compatible

fada11d

Merge branch 'main' into cz/feat/improve-graph-r37

f23b879

carloszanella enabled auto-merge August 17, 2026 22:23

carloszanella added 3 commits August 17, 2026 23:31

fix: Update callsite after merging main

d606fb8

feat: Add error tracking on benchmark

3fd39fe

chore: FMT

8946a13

carloszanella merged commit c6ac4ec into main Aug 17, 2026
18 checks passed

carloszanella deleted the cz/feat/improve-graph-r37 branch August 17, 2026 22:36

		// Track state overrides for components we've already swapped through.
		let mut state_overrides: HashMap<&ComponentId, Box<dyn ProtocolSim>> = HashMap::new();

		// The pool was simulated but not at this amount. We assume that amounts dont move
		// so much so we assume the same winner wins again at a slightly different amount.

Uh oh!

Conversation

carloszanella commented Aug 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Benchmark results:

Uh oh!

louise-poole left a comment

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!

carloszanella commented Aug 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

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!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

carloszanella commented Aug 10, 2026 •

edited

Loading

carloszanella commented Aug 14, 2026 •

edited

Loading