Nialexsan/fix multi swapper

[nialexsan]

Improve estimation calculation for quote in and quote out for multiswapper

[jordanschalm]

Could you add this note to the type documentation too?

[jordanschalm]

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.

[nialexsan]

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

[jordanschalm]

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

[liobrasil]

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]

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.

[nialexsan]

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

[jordanschalm]

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

[nialexsan]

comment above

[zhangchiqing]

Looks good.

Some minor suggestions.

[zhangchiqing]

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]

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)

[nialexsan]

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

[zhangchiqing]

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]

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]

@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]

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.