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

+        /// 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

+            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.

this change will invalidate partial swapper that provides better quote, like this:
swapper 1 takes 5 inTokens in out of 10 inTokens and returns 4 outTokens
swapper 2 takes all 10 inTokens and returns 3 outTokens

in this case using partial swapper 1 is better

I agree with your example, but I think this point still stands:

If we do allow the swapper to change inAmount, we should be comparing based on the relationship between inAmount/outAmount, not just outAmount.

Otherwise we can have a scenario like:

Swapper 1 takes 1/10 inTokens and returns 9 outTokens

Swapper 2 takes 9/10 inTokens and returns 9 outTokens

(we would randomly pick between swapper 1/2)

handle it here:
be6b5d5#diff-0c8e664fdac94affe5d8ece010362b0853a46e8d9bc29e10fa46d436cced1439R218-R221

I don't think that handles all cases.

What about:

we ask quoteOut(forProvided: 10, reverse: false)

quote 1: swap 100000000000000 inToken for 10 outToken

quote 2: swap 10 inToken for 9 outToken

We'd take quote 1, even though quote 2 is a better price and we only want to trade 10 tokens anyway.

I don't think it makes sense to prioritize maximizing outAmount for this function: we want the best price to trade forProvided input tokens, not the most output tokens. I also don't think we should optimize inAmount (make this symmetrical with quoteIn) for obvious reasons.

In general I don't see a way to make this conceptually symmetrical with quoteIn's current behaviour, without poorly representing the user's likely interests. I'd suggest:

first select quotes with inAmount==forProvided

then select the highest outAmount among those quotes

otherwise select the quote with the best price, regardless of inAmount

with optimizing only for ratio, I'd argue that swapping 9 out of 10 inToken for 8.9 outToken is better than swapping 0.1 out of 10 inToken for 0.1 outToken

I added inAmount guard, that it should not be more than forProvided

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

+                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

+        /// 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.

I believe this is a disconnect between the interface description and actual implementation
in every implemented swapper the reverse flag inverts inType and outType, not the actual inAmount and outAmount

we can either change the interface definition, which is cheap,
or align the implementation in at least five swappers, which will require some time

Does this look right for the interface definition?

// quoteOut /// Interpretation: /// - reverse=false -> I want to provide `forProvided` `swapper.inType()` tokens and receive `quote.outAmount` `swapper.outType()` tokens. /// - reverse=true -> I want to provide `forProvided` `swapper.outType()` tokens and receive `quote.outAmount` `swapper.inType()` tokens. // quoteIn /// Interpretation: /// - reverse=false -> I want to provide `quote.inAmount` `swapper.inType()` tokens and receive `forDesired` `swapper.outType()` tokens. /// - reverse=true -> I want to provide `quote.inAmount` `swapper.outType()` tokens and receive `forDesired` `swapper.inType()` tokens.

yes, that looks correct

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

+                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.

comment above

zhangchiqing

Looks good.

Some minor suggestions.

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

-            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) }

I'll add a new issue to apply the optimizations

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

+                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)

cadence assumes that self.swappers could be an empty array and it required explicit casting

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

+    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:48:03Z

+                }
+            }
+
+            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

+            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.

Co-authored-by: Leo Zhang <zhangchiqing@gmail.com>

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

Merged

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

nialexsan and others added 3 commits March 28, 2026 22:08

address PR comments

ef24c22

Apply suggestions from code review

34b33ff

Co-authored-by: Leo Zhang <zhangchiqing@gmail.com>

optimize for inAmount after outAmount

be6b5d5

nialexsan mentioned this pull request Mar 30, 2026

apply withdraw optimizations #162

Open

add inAmount guard

15901cc

nialexsan merged commit 7be92be into v0 Mar 31, 2026
3 checks passed

nialexsan deleted the nialexsan/fix-multi-swapper branch March 31, 2026 02:14

	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)

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

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

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!

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

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!

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