From 67303b7865b2671d4ec7c7caa71d3e80fda684de Mon Sep 17 00:00:00 2001 From: David Walsh Date: Fri, 13 Nov 2020 00:08:18 -0600 Subject: [PATCH] Fix BigNumber issues (#9860) * Document where we need BigNumber-related changes * Fix 1 unit test * Debug progress * Add required values for each upstream usage of getBigNumber * Switch to base 10 * Address feedback --- .../ui/token-input/token-input.component.js | 2 ++ ui/app/helpers/utils/confirm-tx.util.js | 12 ++++++---- ui/app/helpers/utils/conversion-util.js | 23 +++++++++++++++++++ ui/app/helpers/utils/conversion-util.test.js | 15 +++++++++--- ui/app/helpers/utils/token-util.js | 4 ++++ ui/app/helpers/utils/transactions.util.js | 6 +++-- .../amount-max-button.utils.js | 3 +++ 7 files changed, 56 insertions(+), 9 deletions(-) diff --git a/ui/app/components/ui/token-input/token-input.component.js b/ui/app/components/ui/token-input/token-input.component.js index bb802db86..4d85ed4a5 100644 --- a/ui/app/components/ui/token-input/token-input.component.js +++ b/ui/app/components/ui/token-input/token-input.component.js @@ -80,6 +80,8 @@ export default class TokenInput extends PureComponent { const multiplier = Math.pow(10, Number(decimals || 0)) const hexValue = multiplyCurrencies(decimalValue || 0, multiplier, { + multiplicandBase: 10, + multiplierBase: 10, toNumericBase: 'hex', }) diff --git a/ui/app/helpers/utils/confirm-tx.util.js b/ui/app/helpers/utils/confirm-tx.util.js index 5d5f7b9c6..8ff39e498 100644 --- a/ui/app/helpers/utils/confirm-tx.util.js +++ b/ui/app/helpers/utils/confirm-tx.util.js @@ -39,19 +39,23 @@ export function getHexGasTotal({ gasLimit, gasPrice }) { } export function addEth(...args) { - return args.reduce((acc, base) => { - return addCurrencies(acc, base, { + return args.reduce((acc, ethAmount) => { + return addCurrencies(acc, ethAmount, { toNumericBase: 'dec', numberOfDecimals: 6, + aBase: 10, + bBase: 10, }) }) } export function addFiat(...args) { - return args.reduce((acc, base) => { - return addCurrencies(acc, base, { + return args.reduce((acc, fiatAmount) => { + return addCurrencies(acc, fiatAmount, { toNumericBase: 'dec', numberOfDecimals: 2, + aBase: 10, + bBase: 10, }) }) } diff --git a/ui/app/helpers/utils/conversion-util.js b/ui/app/helpers/utils/conversion-util.js index 94a322bfe..ca6bad62b 100644 --- a/ui/app/helpers/utils/conversion-util.js +++ b/ui/app/helpers/utils/conversion-util.js @@ -54,6 +54,11 @@ const baseChange = { BN: (n) => new BN(n.toString(16)), } +// Utility function for checking base types +const isValidBase = (base) => { + return Number.isInteger(base) && base > 1 +} + /** * Defines the base type of numeric value * @typedef {('hex' | 'dec' | 'BN')} NumericBase @@ -162,6 +167,10 @@ const conversionUtil = ( }) const getBigNumber = (value, base) => { + if (!isValidBase(base)) { + throw new Error('Must specificy valid base') + } + // We don't include 'number' here, because BigNumber will throw if passed // a number primitive it considers unsafe. if (typeof value === 'string' || value instanceof BigNumber) { @@ -173,6 +182,11 @@ const getBigNumber = (value, base) => { const addCurrencies = (a, b, options = {}) => { const { aBase, bBase, ...conversionOptions } = options + + if (!isValidBase(aBase) || !isValidBase(bBase)) { + throw new Error('Must specify valid aBase and bBase') + } + const value = getBigNumber(a, aBase).add(getBigNumber(b, bBase)) return converter({ @@ -183,6 +197,11 @@ const addCurrencies = (a, b, options = {}) => { const subtractCurrencies = (a, b, options = {}) => { const { aBase, bBase, ...conversionOptions } = options + + if (!isValidBase(aBase) || !isValidBase(bBase)) { + throw new Error('Must specify valid aBase and bBase') + } + const value = getBigNumber(a, aBase).minus(getBigNumber(b, bBase)) return converter({ @@ -194,6 +213,10 @@ const subtractCurrencies = (a, b, options = {}) => { const multiplyCurrencies = (a, b, options = {}) => { const { multiplicandBase, multiplierBase, ...conversionOptions } = options + if (!isValidBase(multiplicandBase) || !isValidBase(multiplierBase)) { + throw new Error('Must specify valid multiplicandBase and multiplierBase') + } + const value = getBigNumber(a, multiplicandBase).times( getBigNumber(b, multiplierBase), ) diff --git a/ui/app/helpers/utils/conversion-util.test.js b/ui/app/helpers/utils/conversion-util.test.js index 22436977a..34a7a5308 100644 --- a/ui/app/helpers/utils/conversion-util.test.js +++ b/ui/app/helpers/utils/conversion-util.test.js @@ -5,17 +5,26 @@ import { addCurrencies, conversionUtil } from './conversion-util' describe('conversion utils', function () { describe('addCurrencies()', function () { it('add whole numbers', function () { - const result = addCurrencies(3, 9) + const result = addCurrencies(3, 9, { + aBase: 10, + bBase: 10, + }) assert.equal(result.toNumber(), 12) }) it('add decimals', function () { - const result = addCurrencies(1.3, 1.9) + const result = addCurrencies(1.3, 1.9, { + aBase: 10, + bBase: 10, + }) assert.equal(result.toNumber(), 3.2) }) it('add repeating decimals', function () { - const result = addCurrencies(1 / 3, 1 / 9) + const result = addCurrencies(1 / 3, 1 / 9, { + aBase: 10, + bBase: 10, + }) assert.equal(result.toNumber(), 0.4444444444444444) }) }) diff --git a/ui/app/helpers/utils/token-util.js b/ui/app/helpers/utils/token-util.js index 804b58b90..fa581f910 100644 --- a/ui/app/helpers/utils/token-util.js +++ b/ui/app/helpers/utils/token-util.js @@ -217,6 +217,10 @@ export function getTokenFiatAmount( const currentTokenToFiatRate = multiplyCurrencies( contractExchangeRate, conversionRate, + { + multiplicandBase: 10, + multiplierBase: 10, + }, ) const currentTokenInFiat = conversionUtil(tokenAmount, { fromNumericBase: 'dec', diff --git a/ui/app/helpers/utils/transactions.util.js b/ui/app/helpers/utils/transactions.util.js index 99e3bdf63..6dda958cc 100644 --- a/ui/app/helpers/utils/transactions.util.js +++ b/ui/app/helpers/utils/transactions.util.js @@ -152,9 +152,11 @@ export async function isSmartContractAddress(address) { } export function sumHexes(...args) { - const total = args.reduce((acc, base) => { - return addCurrencies(acc, base, { + const total = args.reduce((acc, hexAmount) => { + return addCurrencies(acc, hexAmount, { toNumericBase: 'hex', + aBase: 16, + bBase: 16, }) }) diff --git a/ui/app/pages/send/send-content/send-amount-row/amount-max-button/amount-max-button.utils.js b/ui/app/pages/send/send-content/send-amount-row/amount-max-button/amount-max-button.utils.js index 4c053bd96..39045a78e 100644 --- a/ui/app/pages/send/send-content/send-amount-row/amount-max-button/amount-max-button.utils.js +++ b/ui/app/pages/send/send-content/send-amount-row/amount-max-button/amount-max-button.utils.js @@ -12,8 +12,11 @@ export function calcMaxAmount({ balance, gasTotal, sendToken, tokenBalance }) { ? multiplyCurrencies(tokenBalance, multiplier, { toNumericBase: 'hex', multiplicandBase: 16, + multiplierBase: 10, }) : subtractCurrencies(addHexPrefix(balance), addHexPrefix(gasTotal), { toNumericBase: 'hex', + aBase: 16, + bBase: 16, }) }