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

Finance USD conversion mechanism − no string manipulation #1180

Closed
wants to merge 5 commits into from
Closed

Finance USD conversion mechanism − no string manipulation #1180

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
b521058
Add Jest
bpierre Jun 25, 2020
c2c9a26
Move to convertAmount()
bpierre Jun 25, 2020
e07cbd8
Add comment
bpierre Jun 25, 2020
de76a63
BigInt => BN conversions
bpierre Jun 25, 2020
af337d2
Type conversion
bpierre Jun 25, 2020
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
21 changes: 16 additions & 5 deletions apps/finance/app/.babelrc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,13 +5,24 @@
{
"modules": false,
"useBuiltIns": "entry",
"core-js": 3,
"shippedProposals": true,
"corejs": 3,
sohkai marked this conversation as resolved.
Show resolved Hide resolved
"shippedProposals": true
}
],
"@babel/preset-react"
],
"plugins": [
["styled-components", { "displayName": true }],
]
"plugins": [["styled-components", { "displayName": true }]],
"env": {
"test": {
"presets": [
[
"@babel/preset-env",
{
"modules": "commonjs",
"targets": { "node": "current" }
}
]
]
}
}
}
6 changes: 5 additions & 1 deletion apps/finance/app/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,17 +27,20 @@
"@babel/preset-env": "^7.10.2",
"@babel/preset-react": "^7.10.1",
"babel-eslint": "^10.0.1",
"babel-jest": "^26.1.0",
"babel-plugin-styled-components": "^1.10.7",
"eslint": "^5.6.0",
"eslint-config-prettier": "^3.1.0",
"eslint-config-standard": "^12.0.0",
"eslint-config-standard-react": "^7.0.2",
"eslint-plugin-import": "^2.8.0",
"eslint-plugin-jest": "^23.17.1",
"eslint-plugin-node": "^7.0.1",
"eslint-plugin-prettier": "^2.7.0",
"eslint-plugin-promise": "^4.0.1",
"eslint-plugin-react": "^7.5.1",
"eslint-plugin-standard": "^4.0.0",
"jest": "^26.1.0",
"parcel-bundler": "^1.12.4",
"parcel-plugin-bundle-visualiser": "^1.2.0",
"prettier": "^1.11.1"
Expand All @@ -49,7 +52,8 @@
"build:script": "parcel build src/script.js --out-dir build/",
"watch:script": "parcel watch src/script.js --out-dir build/ --no-hmr",
"sync-assets": "copy-aragon-ui-assets -n aragon-ui ./build && rsync -rtu ./public/ ./build",
"now-build": "npm run build"
"now-build": "npm run build",
"test": "jest"
},
"browserslist": [
">2%",
Expand Down
12 changes: 3 additions & 9 deletions apps/finance/app/src/components/BalanceToken.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,8 @@ import { tokenIconUrl } from '../lib/icon-utils'
function BalanceToken({
address,
amount,
amountConverted,
compact,
convertedAmount,
decimals,
symbol,
verified,
Expand Down Expand Up @@ -82,24 +82,18 @@ function BalanceToken({
${textStyle('body2')}
`}
>
{convertedAmount.isNeg()
? '−'
: `$${formatTokenAmount(convertedAmount, decimals)}`}
{amountConverted ? `$${amountConverted}` : '−'}
</div>
</div>
</div>
)
}

BalanceToken.defaultProps = {
convertedAmount: new BN(-1),
}

BalanceToken.propTypes = {
address: PropTypes.string.isRequired,
amount: PropTypes.instanceOf(BN).isRequired,
amountConverted: PropTypes.string,
compact: PropTypes.bool.isRequired,
convertedAmount: PropTypes.instanceOf(BN),
decimals: PropTypes.instanceOf(BN).isRequired,
symbol: PropTypes.string.isRequired,
verified: PropTypes.bool.isRequired,
Expand Down
13 changes: 7 additions & 6 deletions apps/finance/app/src/components/Balances.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import React, { useMemo } from 'react'
import BN from 'bn.js'
import { Box, GU, textStyle, useTheme, useLayout } from '@aragon/ui'
import BalanceToken from './BalanceToken'
import { getConvertedAmount, useConvertRates } from '../lib/conversion-utils'
import { convertAmount, useConvertRates } from '../lib/conversion-utils'

// Prepare the balances for the BalanceToken component
function useBalanceItems(balances) {
Expand All @@ -18,9 +18,10 @@ function useBalanceItems(balances) {
return {
address,
amount,
convertedAmount: convertRates[symbol]
? getConvertedAmount(amount, convertRates[symbol], decimals)
: new BN('-1'),
amountConverted:
amount && decimals && convertRates[symbol]
? convertAmount(amount, decimals, 1 / convertRates[symbol])
Copy link
Contributor

Choose a reason for hiding this comment

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

Note that we can also get a convertRate that is > 1 here (potentially, many times greater).

: '',
decimals,
symbol,
verified,
Expand Down Expand Up @@ -84,7 +85,7 @@ function Balances({ balances }) {
({
address,
amount,
convertedAmount,
amountConverted,
decimals,
symbol,
verified,
Expand All @@ -107,7 +108,7 @@ function Balances({ balances }) {
address={address}
amount={amount}
compact={compact}
convertedAmount={convertedAmount}
amountConverted={amountConverted}
decimals={decimals}
symbol={symbol}
verified={verified}
Expand Down
72 changes: 20 additions & 52 deletions apps/finance/app/src/lib/conversion-utils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,66 +1,34 @@
import { useEffect, useRef, useState } from 'react'
import BN from 'bn.js'
import { formatTokenAmount } from '@aragon/ui'

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

const USD_DECIMALS = new BN('2')
const CONVERT_PRECISION = 9

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
// 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
/**
* Convert an amount into another one using a rate.
*
* @param {BigInt|string|number} amount amount to convert
* @param {BigInt|string|number} decimals number of decimals for the amount
* @param {string|number} rate the rate to use for the conversion
* @param {Object} [options] options passed to formatTokenAmount()
* @returns {string} the formatted amount converted
*/
export function convertAmount(amount, decimals, rate, options) {
amount = new BN(String(amount))
decimals = parseInt(String(decimals), 10)
return formatTokenAmount(
amount
.mul(new BN(10).pow(new BN(CONVERT_PRECISION)))
.mul(new BN(rate * 10 ** CONVERT_PRECISION)),
Copy link
Contributor

@sohkai sohkai Jun 25, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Hmm, not entirely sure on this; you're going to get an error if rate * 10 ** CONVERT_PRECISION becomes too large or is still an imprecise number.

decimals + CONVERT_PRECISION * 2,
options
)

// 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)

return convertedAmount
}

export function useConvertRates(symbols) {
Expand Down
16 changes: 16 additions & 0 deletions apps/finance/app/src/lib/conversion-utils.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
import BN from 'bn.js'
import { convertAmount } from './conversion-utils'

describe('convertAmount()', () => {
test('Should convert and format amounts', () => {
expect(
String(convertAmount('10663060000000000000000', 18, 0.995, { digits: 2 }))
).toBe('10,609.74')
expect(
String(convertAmount('10663060000000000000000', 18, 0.995, { digits: 3 }))
).toBe('10,609.745')
expect(
String(convertAmount('10663060000000000000000', 18, 0.995, { digits: 4 }))
).toBe('10,609.7447')
})
})