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.

Sign up for GitHub

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

fix: convert active rate to fraction to calculate opposite amount #5322

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e63c3bb
fix: convert active rate to fraction to calculate opposite amount
shoom3301 Jan 22, 2025
7f7a81f
test: explain problem with multiplying price
shoom3301 Jan 23, 2025
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
4 changes: 2 additions & 2 deletions apps/cowswap-frontend/src/modules/limitOrders/hooks/useUpdateCurrencyAmount.ts
View file
Open in desktop
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
9 changes: 5 additions & 4 deletions apps/cowswap-frontend/src/utils/orderUtils/calculateAmountForRate.ts
View file
Open in desktop
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 •
edited
Loading

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