Skip to content

Nialexsan/fix multi swapper #158

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
nialexsan merged 6 commits into from
Mar 31, 2026
Merged

Nialexsan/fix multi swapper #158

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
3e75cd2
fix multiswapper quote estimation
nialexsan Mar 27, 2026
0491dd3
add tests
nialexsan Mar 27, 2026
ef24c22
address PR comments
nialexsan Mar 29, 2026
34b33ff
Apply suggestions from code review
nialexsan Mar 29, 2026
be6b5d5
optimize for inAmount after outAmount
nialexsan Mar 30, 2026
15901cc
add inAmount guard
nialexsan Mar 31, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
107 changes: 78 additions & 29 deletions cadence/contracts/connectors/SwapConnectors.cdc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -146,26 +146,92 @@ access(all) contract SwapConnectors {
access(all) view fun outType(): Type {
return self.outVault
}
/// The estimated amount required to provide a Vault with the desired output balance
/// The estimated amount required to provide a Vault with the desired output balance.
///
/// 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
access(all) fun quoteIn(forDesired: UFix64, reverse: Bool): {DeFiActions.Quote} {
let estimate = self._estimate(amount: forDesired, out: false, reverse: reverse)
var hasFull = false
var bestIdx = 0
var bestInAmount = UFix64.max
var bestOutAmount = 0.0

for i in InclusiveRange(0, self.swappers.length - 1) {
let quote = (&self.swappers[i] as &{DeFiActions.Swapper})
.quoteIn(forDesired: forDesired, reverse: reverse)
Comment on lines +166 to +167
Copy link
Copy Markdown
Member

@zhangchiqing zhangchiqing Mar 27, 2026

Choose a reason for hiding this comment

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

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)

Copy link
Copy Markdown
Contributor Author

@nialexsan nialexsan Mar 29, 2026

Choose a reason for hiding this comment

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

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

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

if quote.outAmount >= forDesired {
// 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 {
// 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
Comment thread
nialexsan marked this conversation as resolved.
bestIdx = i
bestInAmount = quote.inAmount
bestOutAmount = quote.outAmount
}
} else if !hasFull {
// partial coverage group (only when no full route found)
// in this case, prefer maximum outAmount
if quote.outAmount > bestOutAmount {
bestIdx = i
bestInAmount = quote.inAmount
bestOutAmount = quote.outAmount
}
}
}

return MultiSwapperQuote(
inType: reverse ? self.outType() : self.inType(),
outType: reverse ? self.inType() : self.outType(),
inAmount: estimate[1],
outAmount: forDesired,
swapperIndex: Int(estimate[0])
inAmount: bestInAmount,
outAmount: bestOutAmount,
swapperIndex: bestIdx
)
}
/// The estimated amount delivered out for a provided input balance
/// The estimated amount delivered out for a provided input balance.
///
/// Selection policy: among routes where inAmount <= forProvided, prefer the highest outAmount,
/// then the lowest inAmount as a tiebreaker. Routes where inAmount > forProvided are excluded —
/// they cannot be executed with the provided input and would produce misleadingly high outAmounts.
access(all) fun quoteOut(forProvided: UFix64, reverse: Bool): {DeFiActions.Quote} {
Copy link
Copy Markdown
Member

@jordanschalm jordanschalm Mar 27, 2026

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Contributor Author

@nialexsan nialexsan Mar 29, 2026

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Member

@jordanschalm jordanschalm Mar 30, 2026

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Contributor Author

@nialexsan nialexsan Mar 30, 2026

Choose a reason for hiding this comment

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

yes, that looks correct

Copy link
Copy Markdown
Member

@jordanschalm jordanschalm Mar 30, 2026

Choose a reason for hiding this comment

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

#163

let estimate = self._estimate(amount: forProvided, out: true, reverse: reverse)
var bestIdx = 0
var bestInAmount = 0.0
var bestOutAmount = 0.0

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 }
Copy link
Copy Markdown
Member

@jordanschalm jordanschalm Mar 27, 2026

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Contributor Author

@nialexsan nialexsan Mar 29, 2026

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Member

@jordanschalm jordanschalm Mar 30, 2026

Choose a reason for hiding this comment

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

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)

Copy link
Copy Markdown
Contributor Author

@nialexsan nialexsan Mar 30, 2026

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Member

@jordanschalm jordanschalm Mar 30, 2026

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Contributor Author

@nialexsan nialexsan Mar 31, 2026

Choose a reason for hiding this comment

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

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

if quote.inAmount > forProvided { continue }

if quote.outAmount > bestOutAmount {
bestIdx = i
bestInAmount = quote.inAmount
bestOutAmount = quote.outAmount
} else if quote.outAmount == bestOutAmount && quote.inAmount < bestInAmount {
bestIdx = i
bestInAmount = quote.inAmount
bestOutAmount = quote.outAmount
}
}

return MultiSwapperQuote(
inType: reverse ? self.outType() : self.inType(),
outType: reverse ? self.inType() : self.outType(),
inAmount: forProvided,
outAmount: estimate[1],
swapperIndex: Int(estimate[0])
inAmount: bestInAmount,
outAmount: bestOutAmount,
swapperIndex: bestIdx
)
}
/// Performs a swap taking a Vault of type inVault, outputting a resulting outVault. Implementations may choose
Expand All @@ -183,24 +249,6 @@ access(all) contract SwapConnectors {
access(all) fun swapBack(quote: {DeFiActions.Quote}?, residual: @{FungibleToken.Vault}): @{FungibleToken.Vault} {
return <-self._swap(quote: quote, from: <-residual, reverse: true)
}
/// Returns the the index of the optimal Swapper (result[0]) and the associated amountOut or amountIn (result[0])
/// as a UFix64 array
access(self) fun _estimate(amount: UFix64, out: Bool, reverse: Bool): [UFix64; 2] {
var res: [UFix64; 2] = out ? [0.0, 0.0] : [0.0, UFix64.max] // maximizing for out, minimizing for in
for i in InclusiveRange(0, self.swappers.length - 1) {
let swapper = &self.swappers[i] as &{DeFiActions.Swapper}
// call the appropriate estimator
let estimate = out
? swapper.quoteOut(forProvided: amount, reverse: reverse).outAmount
: swapper.quoteIn(forDesired: amount, reverse: reverse).inAmount
// estimates of 0.0 are presumed to be graceful failures - skip them
if estimate > 0.0 && (out ? res[1] < estimate : estimate < res[1]) {
// take minimum for in, maximum for out
res = [UFix64(i), estimate]
}
}
return res
}
/// Swaps the provided Vault in the defined direction. If the quote is not a MultiSwapperQuote, a new quote is
/// requested and the current optimal Swapper used to fulfill the swap.
access(self) fun _swap(quote: {DeFiActions.Quote}?, from: @{FungibleToken.Vault}, reverse: Bool): @{FungibleToken.Vault} {
Expand Down Expand Up @@ -607,7 +655,8 @@ access(all) contract SwapConnectors {
}

// expect output amount as the lesser between the amount available and the maximum amount
var quote = minimumAvail < maxAmount
let usingQuoteOut = minimumAvail < maxAmount
var quote = usingQuoteOut
? self.swapper.quoteOut(forProvided: self.source.minimumAvailable(), reverse: false)
Copy link
Copy Markdown
Member

@zhangchiqing zhangchiqing Mar 27, 2026

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Contributor Author

@nialexsan nialexsan Mar 30, 2026

Choose a reason for hiding this comment

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

I'll add a new issue to apply the optimizations

: self.swapper.quoteIn(forDesired: maxAmount, reverse: false)

Expand Down
1 change: 0 additions & 1 deletion cadence/contracts/connectors/evm/UniswapV3SwapConnectors.cdc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -682,7 +682,6 @@ access(all) contract UniswapV3SwapConnectors {
// optional dev guards
let _chkIn = EVMAbiHelpers.abiUInt256(evmAmountIn)
let _chkMin = EVMAbiHelpers.abiUInt256(minOutUint)
//panic("path: \(EVMAbiHelpers.toHex(pathBytes.value)), amountIn: \(evmAmountIn.toString()), amountOutMin: \(minOutUint.toString())")
assert(_chkIn.length == 32, message: "amountIn not 32 bytes")
assert(_chkMin.length == 32, message: "amountOutMin not 32 bytes")

Expand Down
Loading
Loading