Swap and bridge quote is loaded twice

[elena-zh]

1 case: when I pick a buy token in a destination chain, the selection works very slow causing double quotes loading for previous token and for a newly selected.
See this video with steps and an outcome:
https://www.loom.com/share/751c71560a8a4936aee02fb6ca48bd57

Expected: load a token into a buy field faster?

2nd case: Safe (implemented in #6618 : the 2nd quote is caused AddData change after the 1st quote is loaded.

To reproduce:

1. Open the app inside Safe
2. Specify a cross-chain swap and wait till the quote is loaded: it is loaded twice.

Expected: load the quote once.

[crutch12]

It you disable auto slippage, you shouldn't catch that problem.

Why - useTradeQuotePolling + useQuoteParams + useTradeQuoteManager

pollQuoteRef.current calls twice because of how useQuoteParams works

/**
 * Fetch the quote instantly once the quote params are changed
 */
useLayoutEffect(() => {
  /**
   * Quote params are not supposed to be changed once confirm screen is open
   * So, we should not refetch quote
   */
  if (isConfirmOpen) return
  if (pollQuoteRef.current(true)) {
    resetQuoteCounter()
  }
}, [isConfirmOpen, isQuoteUpdatePossible, quoteParams, resetQuoteCounter])

On the first call useQuoteParams returns last saved slippageBps value, derived from useSmartSlippageFromQuote

const slippage = tradeQuote?.quote?.quoteResults.suggestedSlippageBps ?? null

But then we immediattely call tradeQuoteManager.setLoading and it resets last saved slippage value and updates quoteParams

So we call pollQuoteRef.current again with default slippageBips value (50)

Looks like we also totally ignore auto slippage value when calculate swap prices 🫠

My suggestions how to fix it

• (worse) don't fully reset tradeQuote?.quote when call tradeQuoteManager.setLoading, reuse previous value
• (better) fetch /slippageTolerance before /quote, and use it in quoteParams

But these suggestions require some review from core team (whom could we mention, @elena-zh?)

Proof:

Another problem (not that bad) - doQuotePolling

This method simultaneously calls fetchQuote with priceQuality: PriceQuality.FAST / PriceQuality.OPTIMAL

// Don't fetch fast quote in confirm screen and in bridging mode
if (fastQuote && !isConfirmOpen && !isBridging) {
  fetchQuote({ hasParamsChanged, priceQuality: PriceQuality.FAST, fetchStartTimestamp })
}
fetchQuote({ hasParamsChanged, priceQuality: PriceQuality.OPTIMAL, fetchStartTimestamp })

So we always have two /quote calls. But looks like its intended behaviour, just to show user approximate price at first.

[elena-zh]

Hey @crutch12 , thanks for reviewing it and proposing suggestions.

But these suggestions require some review from core team (whom could we mention, @elena-zh?)

I'm a part of the core team, so no need to mention anybody as I can see your comments and can react on them.
I've sent your suggestions to the team's review.

[shoom3301]

This method simultaneously calls fetchQuote with priceQuality: PriceQuality.FAST / PriceQuality.OPTIMAL

This is by design. We request both in parallel, FAST comes faster to display a price asap, but this price cannot be used to trade, it's just a measure to display something to a user. OPTIMAL is supposed to be used in order signing.

[shoom3301]

@crutch12 thanks a lot for such a deep investigation!
I got your point about slippage in useQuoteParams, indeed, that's a problem.
I agree with your proposal: don't fully reset tradeQuote?.quote. But we should make sure that UI don't display stale data once another quote fetch is started.

(better) fetch /slippageTolerance before /quote, and use it in quoteParams

This happens on the SDK side, and we do call slippageTolerance before quote. I don't think this is related to this bug.

[crutch12]

@elena-zh sorry, didn't want to offend you, I meant someone of developers who could investigate the code logic 🙏

@shoom3301 looks like SDK calls /slippageTolerance only after /quote
https://github.com/cowprotocol/cow-sdk/blob/349f758ea15d358e737fb50abdbfaa56fe617316/packages/trading/src/getQuote.ts#L140

And it's really strange, because it always leads to extra fetch:

1. Swap page loaded, tradeQuote?.quote?.quoteResults.suggestedSlippageBps is empty
2. First /quote fetch with default slippageBips value -> first /slippageTolerance fetch -> set tradeQuote?.quote
3. Hooray, now we have suggestedSlippageBps. So we should call /quote again to get the real result 🫠

Here is a dumb workaround just to preserve previous value, but it may cause even more problems:

// useSmartSlippageFromQuote.ts

  const slippage = tradeQuote?.quote?.quoteResults.suggestedSlippageBps ?? null
+ const prevSlippage = usePrevious(slippage)

  if (typeof slippage === 'number') {
    if (slippage < slippageBottomCap) return null
    if (slippage > slippageTopCap) return null
  }

+ if (slippage === null) {
+   return prevSlippage
+ }

[crutch12]

I think we shouldn't use suggestedSlippageBps at all. We should separate /slippageTolerance and /quote requests.

1. call /slippageTolerance for provided assets.
2. call /quote with prefetched slippage to get the final result

[crutch12]

Also there is a problem in useQuoteParams

// Slippage value for quote params:
// - User slippage: always include (re-quotes when user changes it)
// - Smart slippage: only include after quote loads to prevent re-fetch loop and this will only re-fetch when user switches to auto-slippage mode
const slippageBps =
  tradeSlippage.type === 'user'
    ? tradeSlippage.value
    : tradeSlippage.type === 'smart' && !isQuoteLoading
      ? tradeSlippage.value
      : undefined

This code prevents re-fetch loop only due to !isQuoteLoading check.
If I remove !isQuoteLoading check, slippageBps will be updated on every quote update.

So even if we fix tradeQuote?.quote?.quoteResults.suggestedSlippageBps, we have to come up here with some workaround too.
Looks like we should update suggestedSlippageBps value only on user interaction/polling interval (via hasParamsChanged === true ?)

But I believe that if we separate /quote and /slippageTolerance logic, we will get rid of re-fetch loop problems at all and remove current workarounds

[crutch12]

Another /quote problem: with PriceQuality.FAST, SDK always returns default suggestedSlippageBps value 🫠