Skip to content
Open
Show file tree
Hide file tree
Changes from 8 commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ export function PriceImpactIndicator({ priceImpactParams, isBridging = false }:

return (
<span>
{priceImpact ? (
{priceImpact && !priceImpactLoading ? (
<PriceImpactWrapper priceImpact$={priceImpact} isBridging$={isBridging}>
{' '}
<HoverTooltip
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import { CurrencyAmount, Token } from '@cowprotocol/currency'
import { act, renderHook } from '@testing-library/react'

import { useDerivedTradeState } from 'modules/trade'
import { useTradeQuote } from 'modules/tradeQuote'
import { useTradeUsdAmounts } from 'modules/usdAmount'

import { useFiatValuePriceImpact } from './useFiatValuePriceImpact'
Expand All @@ -17,6 +18,10 @@ jest.mock('modules/trade', () => ({
useDerivedTradeState: jest.fn(),
}))

jest.mock('modules/tradeQuote', () => ({
useTradeQuote: jest.fn(),
}))

jest.mock('modules/usdAmount', () => ({
useTradeUsdAmounts: jest.fn(),
}))
Expand All @@ -32,6 +37,7 @@ jest.mock('./logger', () => ({

const mockedUseDerivedTradeState = useDerivedTradeState as jest.MockedFunction<typeof useDerivedTradeState>
const mockedUseTradeUsdAmounts = useTradeUsdAmounts as jest.MockedFunction<typeof useTradeUsdAmounts>
const mockedUseTradeQuote = useTradeQuote as jest.MockedFunction<typeof useTradeQuote>

function createToken(symbol: string, address: string): Token {
return new Token(ChainId.SEPOLIA, address, 18, symbol, symbol)
Expand All @@ -52,6 +58,11 @@ describe('useFiatValuePriceImpact', () => {
inputCurrencyAmount: CurrencyAmount.fromRawAmount(inputToken, 1),
outputCurrencyAmount: CurrencyAmount.fromRawAmount(outputToken, 1),
} as ReturnType<typeof useDerivedTradeState>)

mockedUseTradeQuote.mockReturnValue({
isLoading: false,
hasParamsChanged: false,
} as ReturnType<typeof useTradeQuote>)
})

afterEach(() => {
Expand Down Expand Up @@ -158,4 +169,37 @@ describe('useFiatValuePriceImpact', () => {

expect(result.current).toEqual({ priceImpact: undefined, isLoading: false })
})

it('restarts the loading timeout when a new quote begins after timing out', () => {
mockedUseTradeUsdAmounts.mockReturnValue({
inputAmount: { value: null, isLoading: true },
outputAmount: { value: null, isLoading: true },
})

const { result, rerender } = renderHook(() => useFiatValuePriceImpact())

expect(result.current).toEqual({ priceImpact: undefined, isLoading: true })

act(() => {
jest.advanceTimersByTime(15_000)
})

// Stale value stays suppressed only until the safety-valve timeout fires
expect(result.current).toEqual({ priceImpact: undefined, isLoading: false })

// A fresh quote for the same token pair must re-arm the timeout and suppress the stale value again
mockedUseTradeQuote.mockReturnValue({
isLoading: true,
hasParamsChanged: true,
} as ReturnType<typeof useTradeQuote>)
rerender()

expect(result.current).toEqual({ priceImpact: undefined, isLoading: true })

act(() => {
jest.advanceTimersByTime(15_000)
})

expect(result.current).toEqual({ priceImpact: undefined, isLoading: false })
})
})
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import { Fraction, Percent } from '@cowprotocol/currency'
import ms from 'ms.macro'

import { useDerivedTradeState } from 'modules/trade'
import { useTradeQuote } from 'modules/tradeQuote'
import { useTradeUsdAmounts } from 'modules/usdAmount'

import { useSafeMemo } from 'common/hooks/useSafeMemo'
Expand All @@ -31,9 +32,19 @@ export function useFiatValuePriceImpact(): { priceImpact: Percent | undefined; i
outputAmount: { value: fiatValueOutput, isLoading: outputIsLoading },
} = useTradeUsdAmounts(inputCurrencyAmount, outputCurrencyAmount, inputToken, outputToken)

const isLoading = inputIsLoading || outputIsLoading
const { isLoading: isQuoteLoading, hasParamsChanged: quoteParamsChanged } = useTradeQuote()

// Trade-quote signals indicate the current output amount is stale (token just changed
// or a fresh quote is in flight). Compute price impact only once the quote catches up,
// otherwise we'd display a huge nonsense % derived from mismatched in/out amounts.
const isLoading = inputIsLoading || outputIsLoading || isQuoteLoading || quoteParamsChanged
Comment thread
coderabbitai[bot] marked this conversation as resolved.
const [hasLoadingTimedOut, setHasLoadingTimedOut] = useState(false)

// Restart the safety-valve timeout on a token-pair change OR whenever a new quote begins
// (`quoteParamsChanged`). Keying it only off the token pair left `hasLoadingTimedOut` stuck
// true after the first 15s, so the stale-value suppression below never fired again for later
// same-pair repricing. Plain loading flicker (unchanged params) intentionally does not restart
// it, so a stuck quote still times out.
useEffect(() => {
logPriceImpact.debug(`Price impact timeout reset`)
setHasLoadingTimedOut(false)
Expand All @@ -45,18 +56,26 @@ export function useFiatValuePriceImpact(): { priceImpact: Percent | undefined; i
}, PRICE_IMPACT_LOADING_TIMEOUT)

return () => clearTimeout(timeoutId)
}, [isTradeSetUp, inputToken, outputToken])
}, [isTradeSetUp, inputToken, outputToken, quoteParamsChanged])

return useSafeMemo(() => {
// Don't calculate price impact if trade is not set up (both trade assets are not set)
if (!isTradeSetUp) return null

const stillLoading = isLoading && !hasLoadingTimedOut

// While a fresh quote is loading, don't expose the stale value at all — consumers
// hide the percentage when `priceImpact` is undefined, leaving just the spinner.
if (stillLoading) {
return { priceImpact: undefined, isLoading: true }

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

While a fresh quote is loading, don't expose the stale value at all —

This never happens because after the first quote is being loaded, the second quote (the fresh quote as you say) has hasLoadingTimedOut === true so stillLoading is never true again, after the first quote.

So this code and comment are incorrect

}

const priceImpact = computeFiatValuePriceImpact(
fiatValueInput ? FractionUtils.fractionLikeToFraction(fiatValueInput) : null,
fiatValueOutput ? FractionUtils.fractionLikeToFraction(fiatValueOutput) : null,
)

return { priceImpact, isLoading: isLoading && !hasLoadingTimedOut }
return { priceImpact, isLoading: false }
}, [isTradeSetUp, fiatValueInput, fiatValueOutput, isLoading, hasLoadingTimedOut])
}

Expand Down
Loading