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: convert active rate to fraction to calculate opposite amount #5322

Merged
merged 2 commits into from
Jan 23, 2025
Merged
Show file tree
Hide file tree
Changes from all 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 @@ -37,8 +37,8 @@ export function useUpdateCurrencyAmount() {
outputCurrency,
})

const newInputAmount = (field as Field) === Field.INPUT ? amount : calculatedAmount
const newOutputAmount = (field as Field) === Field.OUTPUT ? amount : calculatedAmount
const newInputAmount = field === Field.INPUT ? amount : calculatedAmount
const newOutputAmount = field === Field.OUTPUT ? amount : calculatedAmount

const update: Partial<Writeable<LimitOrdersRawState>> = {
orderKind,
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,64 @@
import { USDC_MAINNET, WETH_MAINNET } from '@cowprotocol/common-const'
import { CurrencyAmount, Fraction, Price } from '@uniswap/sdk-core'

import JSBI from 'jsbi'

describe('calculateAmountForRate', () => {
it('When multiply an amount to a price instead of fraction, then the result will be zero', () => {
const priceAsFraction = new Fraction('320778000000000000000000000000000000', '99995600000000000000000000000000') // 3207.921148
const price = new Price({
baseAmount: CurrencyAmount.fromRawAmount(WETH_MAINNET, '1000000000000000000'),
quoteAmount: CurrencyAmount.fromRawAmount(USDC_MAINNET, '3208940687'),
})
const value = CurrencyAmount.fromRawAmount(WETH_MAINNET, '1000000')

const resultAfterFraction = value.multiply(priceAsFraction).quotient.toString()
const resultAfterPrice = value.multiply(price).quotient.toString()

expect(resultAfterFraction).toBe('3207921148') // 3207.921148
expect(resultAfterPrice).toBe('0')
})

it('When multiply an amount to a price numerator and divide to denominator, then the result will be zero', () => {
const price = new Price({
baseAmount: CurrencyAmount.fromRawAmount(WETH_MAINNET, '1000000000000000000'),
quoteAmount: CurrencyAmount.fromRawAmount(USDC_MAINNET, '3208940687'),
})
const value = CurrencyAmount.fromRawAmount(WETH_MAINNET, '1000000')

expect(value.numerator.toString()).toBe('1000000')
expect(value.denominator.toString()).toBe('1')

expect(price.numerator.toString()).toBe('3208940687')
expect(price.denominator.toString()).toBe('1000000000000000000')
Comment on lines +29 to +33
Copy link
Collaborator

Choose a reason for hiding this comment

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

That's testing the Uniswap lib methods

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, I know :)
This is a demo in order to answer on Anxos question


/**
* The code bellow is the same if we multiply CurrencyAmount to Price
* I just revealed it to demonstrate a problem
* When we do this multiplication, we get a new Fraction where numerator and denominator have different length
*/
expect(JSBI.multiply(value.numerator, price.numerator).toString()).toBe('3208940687000000')
expect(JSBI.multiply(value.denominator, price.denominator).toString()).toBe('1000000000000000000')

const multiplied = new Fraction(
JSBI.multiply(value.numerator, price.numerator),
JSBI.multiply(value.denominator, price.denominator),
)

/**
* This is still valid fraction, and it does make sense, but let's see what we get bellow
*/
expect(multiplied.toFixed(10)).toBe('0.0032089407')
Copy link
Collaborator

Choose a reason for hiding this comment

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

That's true that it doesn't work as expected because the Price instances are decimals aware.
That's why we shouldn't use multiply or prices as if they are regular fractions. We should use quote instead for them https://docs.uniswap.org/sdk/core/reference/classes/Price#quote


expect(multiplied.numerator.toString()).toBe('3208940687000000') // length 16
expect(multiplied.denominator.toString()).toBe('1000000000000000000') // length 19

/**
* Inside of JSBI.divide() it compares number lengths using __absoluteCompare() and if they are different it just returns zero
* *** It happens, because the Price was constructed from CurrencyAmounts with different decimals
* *** To avoid that FractionUtils.fromPrice() in order to normalize the Fraction
*/
expect((JSBI as any).__absoluteCompare(multiplied.numerator, multiplied.denominator)).toBe(-1)
expect(JSBI.divide(multiplied.numerator, multiplied.denominator).toString()).toBe('0')
})
})
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { FractionUtils } from '@cowprotocol/common-utils'
import { Currency, CurrencyAmount, Fraction } from '@uniswap/sdk-core'
import { Currency, CurrencyAmount, Fraction, Price } from '@uniswap/sdk-core'

import { Field } from 'legacy/state/types'

Expand Down Expand Up @@ -28,15 +28,16 @@ export function calculateAmountForRate({
const parsedValue = FractionUtils.adjustDecimalsAtoms(
amount,
field === Field.INPUT ? inputDecimals : outputDecimals,
field === Field.INPUT ? outputDecimals : inputDecimals
field === Field.INPUT ? outputDecimals : inputDecimals,
)
const activeRateAsFraction = activeRate instanceof Price ? FractionUtils.fromPrice(activeRate) : activeRate
Copy link
Contributor

Choose a reason for hiding this comment

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

The definition of Price is:

export declare class Price<TBase extends Currency, TQuote extends Currency> extends Fraction {

So a price is a valid fraction, why do we need specific handling?

Would be the problem that we use in the math the quotient instead of multiplying by numerator and dividing by denominator for the INPUT case, and the inverse of that for the OUTPUT case

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@anxolin thanks for the question!
I've added a test with an explnation how it works:
7f7a81f

Please let me know if it doesn't answer on your question.

Copy link
Contributor

@anxolin anxolin Jan 28, 2025

Choose a reason for hiding this comment

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

I see.

I yout test, you multiply 1000000 weis of Ether (1e-12 Ether) by the price of ether --> 3208940687 weis of USDC for 1e18 weis of WETH --> 3208.940687 USDC/ETH

If you multiply you get 3208940687000000 / 1e18 ---> 0.003208940687

Because JSBI is a BigInt library, it will return 0 when you try to divide the numerator and denominator (quotient)

So maybe the issue, is we "expect" the price to be in units and it's in wei, but this is not how it stores the information inside. However, I believe it does the conversion you seek inside the Price implementation, IF ONLY it was a public method https://github.com/Uniswap/sdk-core/blob/main/src/entities/fractions/price.ts#L77

Copy link
Contributor

Choose a reason for hiding this comment

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

As far as I can tell by their implementation of the Price, its correct. It makes sure you multiply using the right units. So here when you multiply weiWETH value for a price in weiUSD/weiWETH, you naturally get weiUSDC.

So I think multiplying directly should be actually safer. You just need to not use later the quotient without adjusting the units

Basically the function I share above (adjustedForDecimals) I think should convert a fraction with units of weiUSD/weiWETH into USD/WETH


if (field === Field.INPUT) {
return CurrencyAmount.fromRawAmount(outputCurrency, parsedValue.multiply(activeRate).quotient)
return CurrencyAmount.fromRawAmount(outputCurrency, parsedValue.multiply(activeRateAsFraction).quotient)
}

if (field === Field.OUTPUT) {
return CurrencyAmount.fromRawAmount(inputCurrency, parsedValue.divide(activeRate).quotient)
return CurrencyAmount.fromRawAmount(inputCurrency, parsedValue.divide(activeRateAsFraction).quotient)
}

return null
Expand Down
Loading