From 41ac7062764584dba26bb006bf723b0627da2cbe Mon Sep 17 00:00:00 2001 From: Enrique Ortiz Date: Thu, 25 Jun 2020 14:53:34 -0400 Subject: [PATCH] Fix finance USD conversion mechanism (#1177) * Fix finance USD conversion mechanism * Add Jest * Simplify conversion function * Add some tests for the conversion utility Co-authored-by: Pierre Bertet --- apps/finance/app/.babelrc | 21 ++++- apps/finance/app/package.json | 6 +- apps/finance/app/src/components/Balances.js | 92 ++++--------------- .../app/src/components/useConvertRates.js | 61 ++++++++++++ apps/finance/app/src/lib/conversion-utils.js | 16 ++++ .../app/src/lib/conversion-utils.test.js | 29 ++++++ 6 files changed, 146 insertions(+), 79 deletions(-) create mode 100644 apps/finance/app/src/components/useConvertRates.js create mode 100644 apps/finance/app/src/lib/conversion-utils.js create mode 100644 apps/finance/app/src/lib/conversion-utils.test.js 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%", diff --git a/apps/finance/app/src/components/Balances.js b/apps/finance/app/src/components/Balances.js index e5bc81057d..096d915e4b 100644 --- a/apps/finance/app/src/components/Balances.js +++ b/apps/finance/app/src/components/Balances.js @@ -1,67 +1,9 @@ -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 } from '../lib/conversion-utils' +import { useConvertRates } from './useConvertRates' // Prepare the balances for the BalanceToken component function useBalanceItems(balances) { @@ -72,18 +14,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/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 new file mode 100644 index 0000000000..0478fc9aec --- /dev/null +++ b/apps/finance/app/src/lib/conversion-utils.js @@ -0,0 +1,16 @@ +import BN from 'bn.js' + +export function getConvertedAmount(amount, convertRate) { + const [whole = '', dec = ''] = convertRate.toString().split('.') + // Remove any trailing zeros from the decimal part + const parsedDec = dec.replace(/0*$/, '') + // Construct the final rate, and remove any leading zeros + const rate = `${whole}${parsedDec}`.replace(/^0*/, '') + + // 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 amount.mul(new BN('10').pow(carryAmount)).div(new BN(rate)) +} 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() + }) +})