From 932c6e4076cd64ee821604de89d68d1a024c3f3d Mon Sep 17 00:00:00 2001 From: Enrique Ortiz Date: Tue, 23 Jun 2020 21:48:12 -0400 Subject: [PATCH 1/4] Fix finance USD conversion mechanism --- apps/finance/app/src/components/Balances.js | 91 +++------------ apps/finance/app/src/lib/conversion-utils.js | 117 +++++++++++++++++++ 2 files changed, 135 insertions(+), 73 deletions(-) create mode 100644 apps/finance/app/src/lib/conversion-utils.js diff --git a/apps/finance/app/src/components/Balances.js b/apps/finance/app/src/components/Balances.js index e5bc81057d..b3e0ba9dc7 100644 --- a/apps/finance/app/src/components/Balances.js +++ b/apps/finance/app/src/components/Balances.js @@ -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) { @@ -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 } diff --git a/apps/finance/app/src/lib/conversion-utils.js b/apps/finance/app/src/lib/conversion-utils.js new file mode 100644 index 0000000000..3aea0a4b65 --- /dev/null +++ b/apps/finance/app/src/lib/conversion-utils.js @@ -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 + // 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) + + return convertedAmount +} + +export 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 +} From a9f9313d3bb4539c15b95f59863f3db7d8813623 Mon Sep 17 00:00:00 2001 From: Pierre Bertet Date: Thu, 25 Jun 2020 09:59:38 +0100 Subject: [PATCH 2/4] Add Jest --- apps/finance/app/.babelrc | 21 ++++++++++++++++----- apps/finance/app/package.json | 6 +++++- 2 files changed, 21 insertions(+), 6 deletions(-) diff --git a/apps/finance/app/.babelrc b/apps/finance/app/.babelrc index 7adcae8d5f..7d24d1da6a 100644 --- a/apps/finance/app/.babelrc +++ b/apps/finance/app/.babelrc @@ -5,13 +5,24 @@ { "modules": false, "useBuiltIns": "entry", - "core-js": 3, - "shippedProposals": true, + "corejs": 3, + "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" } + } + ] + ] + } + } } diff --git a/apps/finance/app/package.json b/apps/finance/app/package.json index 76fcf3d5fb..7a52d6b8f3 100644 --- a/apps/finance/app/package.json +++ b/apps/finance/app/package.json @@ -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" @@ -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%", From 94d4be6d96ee4ed5701d91a58f4f11dbc20fbc03 Mon Sep 17 00:00:00 2001 From: Enrique Ortiz Date: Thu, 25 Jun 2020 13:52:48 -0400 Subject: [PATCH 3/4] Simplify conversion function --- apps/finance/app/src/components/Balances.js | 3 +- .../app/src/components/useConvertRates.js | 61 +++++++++ apps/finance/app/src/lib/conversion-utils.js | 121 ++---------------- 3 files changed, 73 insertions(+), 112 deletions(-) create mode 100644 apps/finance/app/src/components/useConvertRates.js diff --git a/apps/finance/app/src/components/Balances.js b/apps/finance/app/src/components/Balances.js index b3e0ba9dc7..096d915e4b 100644 --- a/apps/finance/app/src/components/Balances.js +++ b/apps/finance/app/src/components/Balances.js @@ -2,7 +2,8 @@ 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 { getConvertedAmount } from '../lib/conversion-utils' +import { useConvertRates } from './useConvertRates' // Prepare the balances for the BalanceToken component function useBalanceItems(balances) { diff --git a/apps/finance/app/src/components/useConvertRates.js b/apps/finance/app/src/components/useConvertRates.js new file mode 100644 index 0000000000..7e337bcb03 --- /dev/null +++ b/apps/finance/app/src/components/useConvertRates.js @@ -0,0 +1,61 @@ +import { useEffect, useState, useRef } from 'react' + +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}` +} + +export 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 +} diff --git a/apps/finance/app/src/lib/conversion-utils.js b/apps/finance/app/src/lib/conversion-utils.js index 3aea0a4b65..0478fc9aec 100644 --- a/apps/finance/app/src/lib/conversion-utils.js +++ b/apps/finance/app/src/lib/conversion-utils.js @@ -1,117 +1,16 @@ -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*/, '') +export function getConvertedAmount(amount, convertRate) { + const [whole = '', dec = ''] = convertRate.toString().split('.') + // Remove any trailing zeros from the decimal part 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 - ) - - // 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) { - 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() + // Construct the final rate, and remove any leading zeros + const rate = `${whole}${parsedDec}`.replace(/^0*/, '') - return () => { - cancelled = true - clearTimeout(retryTimer) - retryDelay.current = CONVERT_API_RETRY_DELAY - } - }, [symbolsQuery]) + // Number of decimals to shift the amount of the token passed in, + // resulting from converting the rate to a number without any decimal + // places + const carryAmount = new BN(parsedDec.length.toString()) - return rates + return amount.mul(new BN('10').pow(carryAmount)).div(new BN(rate)) } From d36ec33aaf11c737ae4b3c2eba77a8d5356be07b Mon Sep 17 00:00:00 2001 From: Enrique Ortiz Date: Thu, 25 Jun 2020 14:52:08 -0400 Subject: [PATCH 4/4] Add some tests for the conversion utility --- .../app/src/lib/conversion-utils.test.js | 29 +++++++++++++++++++ 1 file changed, 29 insertions(+) create mode 100644 apps/finance/app/src/lib/conversion-utils.test.js diff --git a/apps/finance/app/src/lib/conversion-utils.test.js b/apps/finance/app/src/lib/conversion-utils.test.js new file mode 100644 index 0000000000..fdd6690285 --- /dev/null +++ b/apps/finance/app/src/lib/conversion-utils.test.js @@ -0,0 +1,29 @@ +import BN from 'bn.js' +import { getConvertedAmount } from './conversion-utils' + +const ONE_ETH = new BN('10').pow(new BN('18')) + +describe('getConvertedAmount tests', () => { + test('Converts amounts correctly', () => { + expect(getConvertedAmount(new BN('1'), 1).toString()).toEqual('1') + expect(getConvertedAmount(new BN(ONE_ETH), 1).toString()).toEqual( + ONE_ETH.toString() + ) + expect(getConvertedAmount(new BN('1'), 0.5).toString()).toEqual('2') + expect(getConvertedAmount(new BN('1'), 0.25).toString()).toEqual('4') + expect(getConvertedAmount(new BN('1'), 0.125).toString()).toEqual('8') + + expect(getConvertedAmount(new BN('100'), 50).toString()).toEqual('2') + // This is the exact case that broke the previous implementation, + // which is AAVE's amount of WBTC + the exchange rate at a certain + // hour on 2020-06-24 + expect( + getConvertedAmount(new BN('1145054'), 0.00009248).toString() + ).toEqual('12381639273') + }) + + test('Throws on invalid inputs', () => { + expect(() => getConvertedAmount(new BN('1'), 0)).toThrow() + expect(() => getConvertedAmount('1000', 0)).toThrow() + }) +})