-
Notifications
You must be signed in to change notification settings - Fork 159
fix(swap): fix quote double loading and prevent re-fetch quote loop (#6675) #6771
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.
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
base: develop
Are you sure you want to change the base?
Changes from 2 commits
eac18d7
9024f60
112d487
2f3417b
bcbb688
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,11 +1,13 @@ | ||
| import { useAtom, useAtomValue } from 'jotai' | ||
| import { useLayoutEffect, useRef } from 'react' | ||
| import { useEffect, useLayoutEffect, useRef } from 'react' | ||
|
|
||
| import { useIsOnline, useIsWindowVisible, usePrevious } from '@cowprotocol/common-hooks' | ||
| import { useIsOnline, useIsWindowVisible, usePrevious, useSyncedRef } from '@cowprotocol/common-hooks' | ||
| import { getCurrencyAddress } from '@cowprotocol/common-utils' | ||
|
|
||
| import ms from 'ms.macro' | ||
|
|
||
| import { useIsSmartSlippageApplied } from 'modules/tradeSlippage/hooks/useIsSmartSlippageApplied' | ||
|
|
||
| import { usePollQuoteCallback } from './usePollQuoteCallback' | ||
| import { useQuoteParams } from './useQuoteParams' | ||
| import { useTradeQuote } from './useTradeQuote' | ||
|
|
@@ -17,10 +19,13 @@ import { tradeQuoteCounterAtom } from '../state/tradeQuoteCounterAtom' | |
| import { tradeQuoteInputAtom } from '../state/tradeQuoteInputAtom' | ||
| import { TradeQuotePollingParameters } from '../types' | ||
| import { isQuoteExpired } from '../utils/quoteDeadline' | ||
| import { checkOnlySlippageBpsChanged } from '../utils/quoteParamsChanges' | ||
|
|
||
| const ONE_SEC = 1000 | ||
| const QUOTE_VALIDATION_INTERVAL = ms`2s` | ||
| const QUOTE_SLIPPAGE_CHANGE_THROTTLE_INTERVAL = ms`1.5s` | ||
|
|
||
| // eslint-disable-next-line max-lines-per-function | ||
| export function useTradeQuotePolling(quotePollingParams: TradeQuotePollingParameters): null { | ||
| const { isConfirmOpen, isQuoteUpdatePossible } = quotePollingParams | ||
|
|
||
|
|
@@ -50,6 +55,13 @@ export function useTradeQuotePolling(quotePollingParams: TradeQuotePollingParame | |
| // eslint-disable-next-line react-hooks/refs | ||
| pollQuoteRef.current = pollQuote | ||
|
|
||
| const prevQuoteParamsRef = useRef(quoteParams) | ||
| useEffect(() => { | ||
| prevQuoteParamsRef.current = quoteParams | ||
| }, [quoteParams]) | ||
|
|
||
| const isSmartSlippageApplied = useSyncedRef(useIsSmartSlippageApplied()) | ||
|
|
||
| /** | ||
| * Reset quote when window is not visible or sell amount has been cleared | ||
| */ | ||
|
|
@@ -75,10 +87,29 @@ export function useTradeQuotePolling(quotePollingParams: TradeQuotePollingParame | |
| */ | ||
| if (isConfirmOpen) return | ||
|
|
||
| if (isSmartSlippageApplied.current) { | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe it should be checked inside |
||
| const onlySlippageBpsChanged = checkOnlySlippageBpsChanged( | ||
| quoteParams, | ||
| prevQuoteParamsRef.current, | ||
| tradeQuoteRef.current, | ||
| ) | ||
|
|
||
| if (onlySlippageBpsChanged) { | ||
| const quoteTimestampDiff = tradeQuoteRef.current.localQuoteTimestamp | ||
| ? Date.now() - tradeQuoteRef.current.localQuoteTimestamp | ||
| : undefined | ||
| // in "smart" slippage mode slippageBps updates on every fetch /quote response | ||
| // so we should throttle duplicated additional requests caused by following slippageBps updates to prevent re-fetch loop (#6675) | ||
| if (typeof quoteTimestampDiff === 'number' && quoteTimestampDiff < QUOTE_SLIPPAGE_CHANGE_THROTTLE_INTERVAL) { | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Another suggestion: instead of throttling by time, we can compare new/prev values and if the difference is less than N% - skip following (ofc only for "smart" slippage mode) But... sometimes |
||
| return | ||
| } | ||
| } | ||
| } | ||
|
|
||
| if (pollQuoteRef.current(true)) { | ||
| resetQuoteCounter() | ||
| } | ||
| }, [isConfirmOpen, isQuoteUpdatePossible, quoteParams, resetQuoteCounter]) | ||
| }, [isConfirmOpen, isQuoteUpdatePossible, isSmartSlippageApplied, quoteParams, resetQuoteCounter]) | ||
|
|
||
| /** | ||
| * Update quote once a QUOTE_POLLING_INTERVAL | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -18,6 +18,8 @@ export interface TradeQuoteState { | |||||||||
| hasParamsChanged: boolean | ||||||||||
| isLoading: boolean | ||||||||||
| localQuoteTimestamp: number | null | ||||||||||
| // cached slippageBps from quote response | ||||||||||
| suggestedSlippageBps: number | null | ||||||||||
| } | ||||||||||
|
|
||||||||||
| export const DEFAULT_TRADE_QUOTE_STATE: TradeQuoteState = { | ||||||||||
|
|
@@ -29,32 +31,41 @@ export const DEFAULT_TRADE_QUOTE_STATE: TradeQuoteState = { | |||||||||
| hasParamsChanged: false, | ||||||||||
| isLoading: false, | ||||||||||
| localQuoteTimestamp: null, | ||||||||||
| suggestedSlippageBps: null, | ||||||||||
| } | ||||||||||
|
|
||||||||||
| export const tradeQuotesAtom = atom<Record<SellTokenAddress, TradeQuoteState | undefined>>({}) | ||||||||||
|
|
||||||||||
| export const updateTradeQuoteAtom = atom( | ||||||||||
| null, | ||||||||||
| (get, set, _sellTokenAddress: SellTokenAddress, nextState: Partial<TradeQuoteState>) => { | ||||||||||
| // eslint-disable-next-line complexity | ||||||||||
| set(tradeQuotesAtom, () => { | ||||||||||
| const sellTokenAddress = _sellTokenAddress.toLowerCase() | ||||||||||
| const prevState = get(tradeQuotesAtom) | ||||||||||
| const prevQuote = prevState[sellTokenAddress] || DEFAULT_TRADE_QUOTE_STATE | ||||||||||
|
|
||||||||||
| const fastPriceQuality = nextState.fetchParams?.priceQuality === PriceQuality.FAST | ||||||||||
|
|
||||||||||
| // Don't update state if Fast quote finished after Optimal quote | ||||||||||
| if ( | ||||||||||
| prevQuote.fetchParams?.fetchStartTimestamp === nextState.fetchParams?.fetchStartTimestamp && | ||||||||||
| nextState.quote && | ||||||||||
| nextState.fetchParams?.priceQuality === PriceQuality.FAST | ||||||||||
| fastPriceQuality | ||||||||||
| ) { | ||||||||||
| return { ...prevState } | ||||||||||
| } | ||||||||||
|
|
||||||||||
| const quote = typeof nextState.quote === 'undefined' ? prevQuote.quote : nextState.quote | ||||||||||
|
|
||||||||||
| const update: TradeQuoteState = { | ||||||||||
| ...prevQuote, | ||||||||||
| ...nextState, | ||||||||||
| quote: typeof nextState.quote === 'undefined' ? prevQuote.quote : nextState.quote, | ||||||||||
| localQuoteTimestamp: nextState.quote ? Math.ceil(Date.now() / 1000) : null, | ||||||||||
| quote, | ||||||||||
| localQuoteTimestamp: nextState.quote ? Date.now() : null, | ||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fix timestamp preservation when quote is preserved. When Example scenario:
🔎 Proposed fix- localQuoteTimestamp: nextState.quote ? Date.now() : null,
+ localQuoteTimestamp: typeof nextState.quote === 'undefined'
+ ? prevQuote.localQuoteTimestamp
+ : (nextState.quote ? Date.now() : null),📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||
| // sdk return default suggestedSlippageBps value for PriceQuality.FAST, should ignore it | ||||||||||
| suggestedSlippageBps: | ||||||||||
| quote && !fastPriceQuality ? quote.quoteResults.suggestedSlippageBps : prevQuote.suggestedSlippageBps, | ||||||||||
| } | ||||||||||
|
|
||||||||||
| return { | ||||||||||
|
|
||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,21 @@ | ||
| import type { QuoteBridgeRequest } from '@cowprotocol/sdk-bridging' | ||
|
|
||
| import deepEqual from 'fast-deep-equal' | ||
|
|
||
| import type { TradeQuoteState } from '../state/tradeQuoteAtom' | ||
|
|
||
| export function checkOnlySlippageBpsChanged( | ||
| quoteParams: QuoteBridgeRequest | undefined, | ||
| prevQuoteParams: QuoteBridgeRequest | undefined, | ||
| tradeQuote: TradeQuoteState, | ||
| ): boolean { | ||
| const onlySlippageBpsChanged = | ||
| !tradeQuote.isLoading && | ||
| quoteParams?.slippageBps !== prevQuoteParams?.slippageBps && | ||
| deepEqual( | ||
| { ...quoteParams, slippageBps: undefined, signer: undefined }, | ||
| { ...prevQuoteParams, slippageBps: undefined, signer: undefined }, | ||
| ) | ||
|
|
||
| return onlySlippageBpsChanged | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,23 @@ | ||
| import { useMemo, useRef } from 'react' | ||
|
|
||
| /** | ||
| * Like `useRef`, but it returns immutable ref that contains actual value. | ||
| * | ||
| * @param value | ||
| * @see https://github.com/react-hookz/web/blob/master/src/useSyncedRef/index.ts | ||
| */ | ||
| export function useSyncedRef<T>(value: T): { readonly current: T } { | ||
| const ref = useRef(value) | ||
|
|
||
| ref.current = value | ||
|
|
||
| return useMemo( | ||
| () => | ||
| Object.freeze({ | ||
| get current() { | ||
| return ref.current | ||
| }, | ||
| }), | ||
| [], | ||
| ) | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion | 🟠 Major
Remove debug comment before merging.
The commented-out line 58 is for testing the re-fetch loop scenario. This debug code should be removed before merging to prevent accidental activation in production.
🔎 Proposed cleanup
log(`Retrieved slippage tolerance from API: ${data.slippageBps} BPS`) - return { - slippageBps: data.slippageBps, - // slippageBps: data.slippageBps + Math.floor(Math.random() * 25), // uncomment to test smart slippage re-fetch quote loop problem (fixed) (#6675) - } + return { slippageBps: data.slippageBps }📝 Committable suggestion
🤖 Prompt for AI Agents