Skip to content
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

fix: estimated execution price #5308

Merged
merged 17 commits into from
Jan 23, 2025
Merged
Changes from 1 commit
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
Prev Previous commit
Next Next commit
fix: price error handling (#5327)
* fix: memoize and error handle market price calculation on ExecutionPriceUpdater

* fix: error handling SpotPriceUpdater

* fix: avoid division by 0

* fix: don't be stupid and create invalid fractions

* fix: use console.error for errors instead of debug

* fix: numerator and denominator are inverted in the price constructor

* fix: skip est. exec. price display when it's 0

* fix: use est. exec. price calculated locally
alfetopito authored Jan 23, 2025
commit 888189d5855884ff6c86b746e1195a61e7228121
Original file line number Diff line number Diff line change
@@ -105,8 +105,11 @@ export function SpotPricesUpdater(): null {
})
} catch (e) {
console.error(
`Failed to update spot prices for ${inputCurrency.address} and ${outputCurrency.address}`,
{ inputPrice, outputPrice },
`[SpotPricesUpdater] Failed to calculate spot price for ${inputCurrency.address} and ${outputCurrency.address}`,
inputPrice.price.numerator.toString(),
inputPrice.price.denominator.toString(),
outputPrice.price.numerator.toString(),
outputPrice.price.denominator.toString(),
e,
)
}
6 changes: 4 additions & 2 deletions apps/cowswap-frontend/src/legacy/state/orders/utils.ts
Original file line number Diff line number Diff line change
@@ -319,7 +319,7 @@ export function getEstimatedExecutionPrice(

// When fee > amount, return 0 price
if (!remainingSellAmount.greaterThan(ZERO_FRACTION)) {
return new Price(inputToken, outputToken, '0', '0')
return new Price(inputToken, outputToken, '1', '0')
}

const feeWithMargin = feeAmount.add(feeAmount.multiply(EXECUTION_PRICE_FEE_COEFFICIENT))
@@ -355,7 +355,9 @@ export function getEstimatedExecutionPrice(

// Just in case when the denominator is <= 0 after subtracting the fee
if (!denominator.greaterThan(ZERO_FRACTION)) {
return new Price(inputToken, outputToken, '0', '0')
// numerator and denominator are inverted!!!
// https://github.com/Uniswap/sdk-core/blob/9997e88/src/entities/fractions/price.ts#L26
return new Price(inputToken, outputToken, '1', '0')
}

/**
Original file line number Diff line number Diff line change
@@ -9,7 +9,7 @@ import { useLimitOrdersDerivedState } from 'modules/limitOrders/hooks/useLimitOr
import { executionPriceAtom } from 'modules/limitOrders/state/executionPriceAtom'
import { limitRateAtom } from 'modules/limitOrders/state/limitRateAtom'

import { useSafeEffect } from 'common/hooks/useSafeMemo'
import { useSafeEffect, useSafeMemo } from 'common/hooks/useSafeMemo'

import { limitOrdersSettingsAtom } from '../../state/limitOrdersSettingsAtom'

@@ -23,12 +23,21 @@ export function ExecutionPriceUpdater() {
const inputToken = inputCurrencyAmount?.currency && getWrappedToken(inputCurrencyAmount.currency)
const outputToken = outputCurrencyAmount?.currency && getWrappedToken(outputCurrencyAmount.currency)

const marketPrice =
marketRate &&
!marketRate.equalTo('0') &&
inputToken &&
outputToken &&
FractionUtils.toPrice(marketRate, inputToken, outputToken)
const marketPrice = useSafeMemo(() => {
try {
if (marketRate && !marketRate.equalTo('0') && inputToken && outputToken) {
return FractionUtils.toPrice(marketRate, inputToken, outputToken)
}
} catch (e) {
console.error(
`[ExecutionPriceUpdater] Failed to parse the market price for ${inputToken?.address} and ${outputToken?.address}`,
marketRate?.numerator.toString(),
marketRate?.denominator.toString(),
e,
)
}
return null
}, [marketRate, inputToken, outputToken])

const fee = feeAmount?.quotient.toString()

Original file line number Diff line number Diff line change
@@ -340,7 +340,7 @@ export function OrderRow({
return '-'
}

if (prices && estimatedExecutionPrice) {
if (estimatedExecutionPrice && !estimatedExecutionPrice.equalTo(ZERO_FRACTION)) {
return (
<styledEl.ExecuteCellWrapper>
{!isUnfillable &&
@@ -497,7 +497,7 @@ export function OrderRow({
if (nextScheduledOrder) {
// For scheduled orders, use the execution price if available, otherwise use the estimated price from props
const nextOrderExecutionPrice =
nextScheduledOrder.executionData.executedPrice || prices?.estimatedExecutionPrice
nextScheduledOrder.executionData.executedPrice || estimatedExecutionPrice
const nextOrderPriceDiffs = nextOrderExecutionPrice
? calculatePriceDifference({
referencePrice: spotPrice,
6 changes: 6 additions & 0 deletions libs/common-utils/src/fractionUtils.test.ts
Original file line number Diff line number Diff line change
@@ -76,5 +76,11 @@ describe('Fraction utils', () => {
expect(JSBI.toNumber(simplified.numerator)).toBe(1)
expect(JSBI.toNumber(simplified.denominator)).toBe(3)
})
it('should avoid division by 0', () => {
const fraction = new Fraction(JSBI.BigInt(0), JSBI.BigInt(0))
const simplified = FractionUtils.simplify(fraction)
expect(JSBI.toNumber(simplified.numerator)).toBe(0)
expect(JSBI.toNumber(simplified.denominator)).toBe(1)
})
})
})
5 changes: 5 additions & 0 deletions libs/common-utils/src/fractionUtils.ts
Original file line number Diff line number Diff line change
@@ -193,6 +193,11 @@ function reduce(fraction: Fraction): Fraction {
let numerator = fraction.numerator
let denominator = fraction.denominator
let rest: JSBI

if (JSBI.equal(denominator, ZERO)) {
return new Fraction(JSBI.BigInt(0), JSBI.BigInt(1))
}

while (JSBI.notEqual(denominator, ZERO)) {
rest = JSBI.remainder(numerator, denominator)
numerator = denominator