Nialexsan/fix multi swapper by nialexsan · Pull Request #158 · onflow/FlowActions

nialexsan · 2026-03-27T14:57:07Z

Improve estimation calculation for quote in and quote out for multiswapper

jordanschalm · 2026-03-27T22:52:03Z

cadence/contracts/connectors/SwapConnectors.cdc

+        /// Selection policy (two-tier):
+        ///   1. Full-coverage routes (outAmount >= forDesired): prefer minimum inAmount
+        ///   2. Partial-coverage routes (outAmount < forDesired, pool capped): prefer maximum outAmount
+        /// Full-coverage always wins over partial-coverage regardless of inAmount.


Could you add this note to the type documentation too?

jordanschalm · 2026-03-27T22:55:20Z

cadence/contracts/connectors/SwapConnectors.cdc

+            for i in InclusiveRange(0, self.swappers.length - 1) {
+                let quote = (&self.swappers[i] as &{DeFiActions.Swapper})
+                    .quoteOut(forProvided: forProvided, reverse: reverse)
+                if quote.inAmount == 0.0 || quote.outAmount == 0.0 { continue }


Suggested change

if quote.inAmount == 0.0 || quote.outAmount == 0.0 { continue }

if quote.inAmount < forProvided || quote.outAmount == 0.0 { continue }

I think we also probably want to reject quotes that accept less than we're providing? If we do allow the swapper to change inAmount, we should be comparing based on the relationship between inAmount/outAmount, not just outAmount.

jordanschalm · 2026-03-27T22:57:14Z

cadence/contracts/connectors/SwapConnectors.cdc

+                if !hasBest || quote.outAmount > bestOutAmount {
+                    hasBest = true


Suggested change

if !hasBest || quote.outAmount > bestOutAmount {

hasBest = true

if quote.outAmount > bestOutAmount {

I don't 'think we need hasBest here since there is only one selection criteria

This is a real regression. quoteOut(forProvided) started exposing the child route’s smaller inAmount, while _swap still forwards the full input vault. I addressed it in #161 by restoring the final MultiSwapper quote to keep inAmount = forProvided (L222 in my PR), rather than skipping capacity-limited routes entirely like the suggested change would.

jordanschalm · 2026-03-27T23:01:13Z

cadence/contracts/connectors/SwapConnectors.cdc

+        /// The estimated amount delivered out for a provided input balance.
+        ///
+        /// Selection policy: prefer maximum outAmount across all routes.
        access(all) fun quoteOut(forProvided: UFix64, reverse: Bool): {DeFiActions.Quote} {


From the interface definition:

/// The reverse flag simply inverts inType/outType and inAmount/outAmount in the quote.
/// Interpretation:
/// - reverse=false -> I want to provide forProvided input tokens and receive quote.outAmount output tokens.
/// - reverse=true -> I want to provide quote.outAmount output tokens and receive forProvided input tokens.

If reverse=true, we should select the quote with the lowest outAmount (since that's our side of the trade), but our selection behaviour is the same either way.

jordanschalm · 2026-03-27T23:04:47Z

cadence/contracts/connectors/SwapConnectors.cdc

+                if quote.inAmount == 0.0 || quote.outAmount == 0.0 { continue }
+
+                if quote.outAmount >= forDesired {
+                    // full coverage — prefer minimum inAmount


I think the same is true here. If reverse=true, we should prefer larger quote.inAmount, since that's our side of the trade.

zhangchiqing

Looks good.

Some minor suggestions.

zhangchiqing · 2026-03-27T22:43:54Z

cadence/contracts/connectors/SwapConnectors.cdc

-            var quote = minimumAvail < maxAmount
+            let usingQuoteOut = minimumAvail < maxAmount
+            var quote = usingQuoteOut
                ? self.swapper.quoteOut(forProvided: self.source.minimumAvailable(), reverse: false)


Can we take the chance to optimize this function?

I noticed when usingQuoteOut is true, self.source.minimumAvailable() will be called twice. The first time is called in self.minimumAvailable() at L642.

This is my optimized version:

access(FungibleToken.Withdraw) fun withdrawAvailable(maxAmount: UFix64): @{FungibleToken.Vault} { if maxAmount == 0.0 { return <- DeFiActionsUtils.getEmptyVault(self.getSourceType()) } let availableIn = self.source.minimumAvailable() if availableIn == 0.0 { return <- DeFiActionsUtils.getEmptyVault(self.getSourceType()) } minimumAvail := self.swapper.quoteOut(forProvided: availableIn, reverse: false) // expect output amount as the lesser between the amount available and the maximum amount, // meaning: // - when available is less than the desired maxAmount, quote based on available amount // to avoid over-withdrawal; // - otherwise, quote based on desired maxAmount var usingQuoteOut = minimumAvail.outAmount < maxAmount var quote = usingQuoteOut ? minimumAvail : self.swapper.quoteIn(forDesired: maxAmount, reverse: false) let sourceLiquidity <- self.source.withdrawAvailable(maxAmount: quote.inAmount) if sourceLiquidity.balance == 0.0 { Burner.burn(<-sourceLiquidity) return <- DeFiActionsUtils.getEmptyVault(self.getSourceType()) } return <- self.swapper.swap(quote: quote, inVault: <-sourceLiquidity) }

zhangchiqing · 2026-03-27T23:06:58Z

cadence/contracts/connectors/SwapConnectors.cdc

+                let quote = (&self.swappers[i] as &{DeFiActions.Swapper})
+                    .quoteIn(forDesired: forDesired, reverse: reverse)


Why is the casting necessary? why not:

Suggested change

let quote = (&self.swappers[i] as &{DeFiActions.Swapper})

.quoteIn(forDesired: forDesired, reverse: reverse)

let quote = self.swappers[i].quoteIn(forDesired: forDesired, reverse: reverse)

zhangchiqing · 2026-03-28T00:26:17Z

cadence/contracts/connectors/SwapConnectors.cdc

+        /// Selection policy (two-tier):
+        ///   1. Full-coverage routes (outAmount >= forDesired): prefer minimum inAmount
+        ///   2. Partial-coverage routes (outAmount < forDesired, pool capped): prefer maximum outAmount
+        /// Full-coverage always wins over partial-coverage regardless of inAmount.


Suggested change

/// Selection policy (two-tier):

/// 1. Full-coverage routes (outAmount >= forDesired): prefer minimum inAmount

/// 2. Partial-coverage routes (outAmount < forDesired, pool capped): prefer maximum outAmount

/// Full-coverage always wins over partial-coverage regardless of inAmount.

/// Swapper quotes are divided into two groups:

/// 1. Full group: the pool has enough liquidity to fully fulfill forDesired

/// 2. Partial group: the pool can only fulfill part of forDesired

///

/// Selection policy:

/// - If any swapper is in the full group, return the one with the lowest inAmount

/// - Otherwise, return the partial group quote with the highest outAmount (best liquidity),

/// even if it doesn't have the best rate

zhangchiqing · 2026-03-28T00:37:11Z

cadence/tests/SwapConnectorsMultiSwapper_test.cdc

+    let forDesired = 10.0
+    let configs = [
+        makeConfig(priceRatio: 0.8, maxOut: 3.0),
+        makeConfig(priceRatio: 1.0, maxOut: 7.0)


Index 1 not only has higher outAmount, but also has better ratio.

We better create another case to show that index 1 is still preferred even if index 0 has better price ratio.

makeConfig(priceRatio: 0.8, maxOut: 3.0), makeConfig(priceRatio: 0.7, maxOut: 7.0)

zhangchiqing · 2026-03-28T00:40:05Z

cadence/contracts/connectors/SwapConnectors.cdc

+                if quote.inAmount == 0.0 || quote.outAmount == 0.0 { continue }
+
+                if quote.outAmount >= forDesired {
+                    // full coverage — prefer minimum inAmount


Suggested change

// full coverage — prefer minimum inAmount

// full coverage group - comparing between swappers that can full fulfill the forDesire,

// in this case we prefer the quote with minimum inAmount

zhangchiqing · 2026-03-28T00:43:48Z

cadence/contracts/connectors/SwapConnectors.cdc

+                    if !hasFull || quote.inAmount < bestInAmount {
+                        hasFull = true


Suggested change

if !hasFull || quote.inAmount < bestInAmount {

hasFull = true

if !hasFull || quote.inAmount < bestInAmount {

// when hasFull == false, we can skip the second check, because

// there is no bestInAmount yet, this quote itself will be the best temporarily

// when hasFull == true, we compare with the previously best quote, if this

// quote has lower inAmount, then it's better.

hasFull = true

zhangchiqing · 2026-03-28T00:44:41Z

cadence/contracts/connectors/SwapConnectors.cdc

+                        bestOutAmount = quote.outAmount
+                    }
+                } else if !hasFull {
+                    // partial coverage — prefer maximum outAmount (only when no full route found)


Suggested change

// partial coverage — prefer maximum outAmount (only when no full route found)

// partial coverage group (only when no full route found)

// in this case, prefer maximum outAmount

zhangchiqing · 2026-03-28T00:48:03Z

cadence/contracts/connectors/SwapConnectors.cdc

+                }
+            }
+
+            let idx = hasFull ? bestIdx : partialIdx


Suggested change

let idx = hasFull ? bestIdx : partialIdx

// now we have the best quotes in both full coverage group and partial coverage group.

// we return the best quote from full coverage group if there is,

// otherwise, return the best quote from partial coverage group.

let idx = hasFull ? bestIdx : partialIdx

liobrasil · 2026-03-28T01:26:10Z

@nialexsan I opened a small follow-up in #161 for two regressions from the current MultiSwapper quote changes:

(1) swap(nil, ...) can fail when quoteOut returns inAmount below the provided input, which is the issue @jordan called out here above
(2) SwapSource.withdrawAvailable(maxAmount) can return more than maxAmount when quoteIn returns outAmount above the target.

Please take a look.

jribbink · 2026-03-28T02:05:53Z

cadence/contracts/connectors/SwapConnectors.cdc

+            var hasFull = false
+            var bestIdx = 0
+            var bestInAmount = UFix64.max
+            var bestOutAmount = 0.0
+            var partialIdx = 0
+            var partialInAmount = 0.0
+            var partialOutAmount = 0.0


Suggested change

var hasFull = false

var bestIdx = 0

var bestInAmount = UFix64.max

var bestOutAmount = 0.0

var partialIdx = 0

var partialInAmount = 0.0

var partialOutAmount = 0.0

var hasFull = false

var bestIdx = 0

var bestInAmount = UFix64.max

var bestOutAmount = 0.0

I think that this logic can be simplified given that the policy prioritized full quotes in all places , we shouldn't need to track the best partial/full quote separately since it can just be replaced once a full quote is found.

nialexsan added 2 commits March 27, 2026 09:43

fix multiswapper quote estimation

3e75cd2

add tests

0491dd3

nialexsan mentioned this pull request Mar 27, 2026

Nialexsan/fusdev strategy onflow/FlowYieldVaults#215

Open

6 tasks

nialexsan requested review from a team and jribbink March 27, 2026 15:08

nialexsan mentioned this pull request Mar 27, 2026

Nialexsan/port multiswapper fixes #159

Draft

jordanschalm reviewed Mar 27, 2026

View reviewed changes

zhangchiqing approved these changes Mar 28, 2026

View reviewed changes

jribbink reviewed Mar 28, 2026

View reviewed changes

	if quote.inAmount == 0.0 \|\| quote.outAmount == 0.0 { continue }
	if quote.inAmount < forProvided \|\| quote.outAmount == 0.0 { continue }

		if !hasBest \|\| quote.outAmount > bestOutAmount {
		hasBest = true

	if !hasBest \|\| quote.outAmount > bestOutAmount {
	hasBest = true
	if quote.outAmount > bestOutAmount {

		let quote = (&self.swappers[i] as &{DeFiActions.Swapper})
		.quoteIn(forDesired: forDesired, reverse: reverse)

	let quote = (&self.swappers[i] as &{DeFiActions.Swapper})
	.quoteIn(forDesired: forDesired, reverse: reverse)
	let quote = self.swappers[i].quoteIn(forDesired: forDesired, reverse: reverse)

-        /// Selection policy (two-tier):
-        ///   1. Full-coverage routes (outAmount >= forDesired): prefer minimum inAmount
-        ///   2. Partial-coverage routes (outAmount < forDesired, pool capped): prefer maximum outAmount
-        /// Full-coverage always wins over partial-coverage regardless of inAmount.
+        /// Swapper quotes are divided into two groups:
+        /// 1. Full group: the pool has enough liquidity to fully fulfill forDesired
+        /// 2. Partial group: the pool can only fulfill part of forDesired
+        ///
+        /// Selection policy:
+        /// - If any swapper is in the full group, return the one with the lowest inAmount
+        /// - Otherwise, return the partial group quote with the highest outAmount (best liquidity),
+        ///   even if it doesn't have the best rate

	// full coverage — prefer minimum inAmount
	// full coverage group - comparing between swappers that can full fulfill the forDesire,
	// in this case we prefer the quote with minimum inAmount

		if !hasFull \|\| quote.inAmount < bestInAmount {
		hasFull = true

	// partial coverage — prefer maximum outAmount (only when no full route found)
	// partial coverage group (only when no full route found)
	// in this case, prefer maximum outAmount

-            let idx = hasFull ? bestIdx : partialIdx
+            // now we have the best quotes in both full coverage group and partial coverage group.
+            // we return the best quote from full coverage group if there is,
+            // otherwise, return the best quote from partial coverage group.
+            let idx = hasFull ? bestIdx : partialIdx

Conversation

nialexsan commented Mar 27, 2026

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!

liobrasil Mar 28, 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!

zhangchiqing 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

liobrasil commented Mar 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

liobrasil Mar 28, 2026 •

edited

Loading

liobrasil commented Mar 28, 2026 •

edited

Loading