-
Notifications
You must be signed in to change notification settings - Fork 103
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
Changes from all commits
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 |
---|---|---|
@@ -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') | ||
|
||
/** | ||
* 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') | ||
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. That's true that it doesn't work as expected because the Price instances are decimals aware. |
||
|
||
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' | ||
|
||
|
@@ -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 | ||
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. The definition of Price is:
So a price is a valid fraction, why do we need specific handling? Would be the problem that we use in the math the 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. 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. 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 ( 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 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. 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 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 ( |
||
|
||
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 | ||
|
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.
That's testing the Uniswap lib methods
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.
Yeah, I know :)
This is a demo in order to answer on Anxos question