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 finance USD conversion mechanism #1177

Merged
merged 5 commits into from
Jun 25, 2020
Merged
Show file tree
Hide file tree
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
91 changes: 18 additions & 73 deletions apps/finance/app/src/components/Balances.js
Original file line number Diff line number Diff line change
@@ -1,67 +1,8 @@
import React, { useEffect, useMemo, useRef, useState } from 'react'
import React, { useMemo } from 'react'
import BN from 'bn.js'
import { Box, GU, textStyle, useTheme, useLayout } from '@aragon/ui'
import BalanceToken from './BalanceToken'

const CONVERT_API_RETRY_DELAY = 2 * 1000
const CONVERT_API_RETRY_DELAY_MAX = 60 * 1000

function convertRatesUrl(symbolsQuery) {
return `https://min-api.cryptocompare.com/data/price?fsym=USD&tsyms=${symbolsQuery}`
}

function useConvertRates(symbols) {
const [rates, setRates] = useState({})
const retryDelay = useRef(CONVERT_API_RETRY_DELAY)

const symbolsQuery = symbols.join(',')

useEffect(() => {
let cancelled = false
let retryTimer = null

const update = async () => {
if (!symbolsQuery) {
setRates({})
return
}

try {
const response = await fetch(convertRatesUrl(symbolsQuery))
const rates = await response.json()
if (!cancelled) {
setRates(rates)
retryDelay.current = CONVERT_API_RETRY_DELAY
}
} catch (err) {
// The !cancelled check is needed in case:
// 1. The fetch() request is ongoing.
// 2. The component gets unmounted.
// 3. An error gets thrown.
//
// Assuming the fetch() request keeps throwing, it would create new
// requests even though the useEffect() got cancelled.
if (!cancelled) {
// Add more delay after every failed attempt
retryDelay.current = Math.min(
CONVERT_API_RETRY_DELAY_MAX,
retryDelay.current * 1.2
)
retryTimer = setTimeout(update, retryDelay.current)
}
}
}
update()

return () => {
cancelled = true
clearTimeout(retryTimer)
retryDelay.current = CONVERT_API_RETRY_DELAY
}
}, [symbolsQuery])

return rates
}
import { getConvertedAmount, useConvertRates } from '../lib/conversion-utils'

// Prepare the balances for the BalanceToken component
function useBalanceItems(balances) {
Expand All @@ -72,18 +13,22 @@ function useBalanceItems(balances) {
const convertRates = useConvertRates(verifiedSymbols)

const balanceItems = useMemo(() => {
return balances.map(({ address, amount, decimals, symbol, verified }) => ({
address,
amount,
convertedAmount: convertRates[symbol]
? amount.divn(convertRates[symbol])
: new BN(-1),
decimals,
symbol,
verified,
}))
}, [balances, convertRates])

return balances.map(
({ address, amount, decimals, symbol, verified }) => {
return {
address,
amount,
convertedAmount: convertRates[symbol]
? getConvertedAmount(amount, convertRates[symbol], decimals)
: new BN('-1'),
decimals,
symbol,
verified,
}
},
[balances, convertRates]
)
})
return balanceItems
}

Expand Down
117 changes: 117 additions & 0 deletions apps/finance/app/src/lib/conversion-utils.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,117 @@
import { useEffect, useRef, useState } from 'react'
import BN from 'bn.js'

const CONVERT_API_RETRY_DELAY = 2 * 1000
const CONVERT_API_RETRY_DELAY_MAX = 60 * 1000

const USD_DECIMALS = new BN('2')

function convertRatesUrl(symbolsQuery) {
return `https://min-api.cryptocompare.com/data/price?fsym=USD&tsyms=${symbolsQuery}`
}

function formatConvertRate(convertRate, decimals) {
const [whole = '', dec = ''] = convertRate.split('.')
const parsedWhole = whole.replace(/^0*/, '')
const parsedDec = dec.replace(/0*$/, '')
// parsedWhole could be empty,
// so in this case, we wanna remove leading zeros.
const fullyParsedDec = parsedWhole ? parsedDec : parsedDec.replace(/^0*/, '')

// Even if we remove leading zeroes from the decimal
// part, we want to count as if we "shifted" them
const decimalsToShift = decimals.sub(new BN(parsedDec.length.toString()))
// Apart from always considering the USD decimals (2),
// if there's the strange case that the above is negative,
// we take it as a carry as we know we already shifted to far,
// and will compensate by shifting the token amount by this much
const carryAmount =
decimalsToShift.toNumber() < 0
? decimalsToShift.add(USD_DECIMALS)
: USD_DECIMALS
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll admit I haven't found practical examples, but I'm just making sure! 😃

// The remaining total amount to shift through bn.js to avoid overflow.
const amountToShift = new BN('10').pow(
decimalsToShift.toNumber() > 0 ? decimalsToShift : new BN('0')
)

// Finish shifting the whole number through BN.js to avoid overflow,
return [
new BN(`${parsedWhole}${fullyParsedDec}`).mul(amountToShift),
carryAmount,
]
}

export function getConvertedAmount(amount, convertRate, decimals) {
const [formattedConvertRate, carryAmount] = formatConvertRate(
convertRate.toString(),
decimals
)

// Get the actual precision we need to re-add when calculations are over
const precisionTarget = new BN('10').pow(decimals.sub(USD_DECIMALS))
const convertedAmount = amount
// Shift the amount to take into account the USD decimals
// + any leftover
.mul(new BN('10').pow(carryAmount))
// Actually convert to an USD rate
.div(formattedConvertRate)
// Return it to its original precision
// Note that we don't have to subtract the "extra carry"
// as it's undone during the division
.mul(precisionTarget)
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure if we need to consider any USD decimals here, as our "USD" converted amount is still in the original token's decimals after the conversion.

Wondering if we can simplify everything down to:

const [whole = '', dec = ''] = convertRate.split('.')
const parsedDec = dec.replace(/0*$/, '')
const carryAmount = parsedDec.length
const rate = `${whole}${parsedDec}`.replace(/^0*/, '')

return amount
  .mul(new BN('10').pow(carryAmount))
  .div(new BN(rate))

As we don't need to use the decimals at all.

Copy link
Contributor

@bpierre bpierre Jun 25, 2020

Choose a reason for hiding this comment

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

I think we could completely cut the string manipulations by multiplying the two values and relying on formatTokenAmount(). What do you guys think?

PR: #1180

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My only worry with the above is overflow issues on many tokens due to the ceiling for safe numbers in Javascript (Number.MAX_SAFE_INTEGER) is around 9*10^15, having 3 decimals less than most tokens, which have 18. This is really the only reason why I considered doing this string manipulation, rather than just shifting the decimals by normal math.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, the loss of precision with Number is exactly why are using BN.js, and the proposed solution is taking care of the token decimals… but I realize now that rate itself, once scaled up, could also easily become too large :-(

I am wondering if it would be easier to implement TokenAmount.convert() directly? We would use a mix of string splitting for the rate, and BigInt calculations otherwise.

Copy link
Contributor Author

@Evalir Evalir Jun 25, 2020

Choose a reason for hiding this comment

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

could also easily become too large :-(

Yeah, and the solutions are not pretty :( In general these conversions are a headache when maintaining them in BigInts.

I am wondering if it would be easier to implement TokenAmount.convert() directly? We would use a mix of string splitting for the rate, and BigInt calculations otherwise.

I think it could be good! it's also pretty high leverage as doing these conversions is both hard to get 100% right, and at the same time extremely common to do. Should we wait for .convert() to be implemented, or should we merge and then upgrade once it's done? Asking this mainly because it's breaking Aave's finance app.

Copy link
Contributor

Choose a reason for hiding this comment

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

I was thinking that we could add it to TokenAmount directly and use it from here, but we could merge this first to make sure it gets fixed quick yes.

I added the commit with the Jest configuration.

What would you think of:

  1. Perhaps simplify a bit the process, following @sohkai’s comment.
  2. Add some tests to make sure everything works fine.
  3. Merge it until TokenAmount.format() is ready?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good to me! Let's do that.


return convertedAmount
}

export function useConvertRates(symbols) {
Evalir marked this conversation as resolved.
Show resolved Hide resolved
const [rates, setRates] = useState({})
const retryDelay = useRef(CONVERT_API_RETRY_DELAY)

const symbolsQuery = symbols.join(',')

useEffect(() => {
let cancelled = false
let retryTimer = null

const update = async () => {
if (!symbolsQuery) {
setRates({})
return
}

try {
const response = await fetch(convertRatesUrl(symbolsQuery))
const rates = await response.json()
if (!cancelled) {
setRates(rates)
retryDelay.current = CONVERT_API_RETRY_DELAY
}
} catch (err) {
// The !cancelled check is needed in case:
// 1. The fetch() request is ongoing.
// 2. The component gets unmounted.
// 3. An error gets thrown.
//
// Assuming the fetch() request keeps throwing, it would create new
// requests even though the useEffect() got cancelled.
if (!cancelled) {
// Add more delay after every failed attempt
retryDelay.current = Math.min(
CONVERT_API_RETRY_DELAY_MAX,
retryDelay.current * 1.2
)
retryTimer = setTimeout(update, retryDelay.current)
}
}
}
update()

return () => {
cancelled = true
clearTimeout(retryTimer)
retryDelay.current = CONVERT_API_RETRY_DELAY
}
}, [symbolsQuery])

return rates
}