From 9193397eef098a9907e3ef3fcd437ad36fa80756 Mon Sep 17 00:00:00 2001 From: Mark Stacey Date: Tue, 28 Jan 2020 22:49:32 -0400 Subject: [PATCH] Allow editing max spend limit (#7919) In the case where the initial spend limit for the `approve` function was set to the maximum amount, editing this value would result in the new limit being silently ignored. The transaction would be submitted with the original max spend limit. This occurred because function to generate the new custom data would look for the expected spend limit in the existing data, then bail if it was not found (and in these cases, it was never found). The reason the value was not found is that it was erroneously being converted to a `Number`. A JavaScript `Number` is not precise enough to represent larger spend limits, so it would give the wrong hex value (after rounding had taken place in the conversion to a floating-point number). The data string is now updated without relying upon the original token value; the new value is inserted after the `spender` argument instead, as the remainder of the `data` string is guaranteed to be the original limit. Additionally, the conversion to a `Number` is now omitted so that the custom spend limit is encoded correctly. Fixes #7915 --- .../edit-approval-permission.component.js | 20 +++---- .../confirm-approve-content.component.js | 8 +-- .../confirm-approve.component.js | 14 ++--- .../confirm-approve.container.js | 3 +- .../confirm-approve/confirm-approve.util.js | 53 +++++++++---------- 5 files changed, 46 insertions(+), 52 deletions(-) diff --git a/ui/app/components/app/modals/edit-approval-permission/edit-approval-permission.component.js b/ui/app/components/app/modals/edit-approval-permission/edit-approval-permission.component.js index d8b23a251925..b492010eabaa 100644 --- a/ui/app/components/app/modals/edit-approval-permission/edit-approval-permission.component.js +++ b/ui/app/components/app/modals/edit-approval-permission/edit-approval-permission.component.js @@ -4,6 +4,7 @@ import Modal from '../../modal' import Identicon from '../../../ui/identicon' import TextField from '../../../ui/text-field' import classnames from 'classnames' +import BigNumber from 'bignumber.js' export default class EditApprovalPermission extends PureComponent { static propTypes = { @@ -60,7 +61,7 @@ export default class EditApprovalPermission extends PureComponent {
{t('balance')}
- {`${tokenBalance} ${tokenSymbol}`} + {`${Number(tokenBalance).toPrecision(9)} ${tokenSymbol}`}
@@ -93,15 +94,17 @@ export default class EditApprovalPermission extends PureComponent { 'edit-approval-permission__edit-section__option-label--selected': selectedOptionIsUnlimited, })} > - {tokenAmount < tokenBalance - ? t('proposedApprovalLimit') - : t('unlimited')} + { + (new BigNumber(tokenAmount)).lessThan(new BigNumber(tokenBalance)) + ? t('proposedApprovalLimit') + : t('unlimited') + }
{t('spendLimitRequestedBy', [origin])}
-
- {`${tokenAmount} ${tokenSymbol}`} +
+ {`${Number(tokenAmount)} ${tokenSymbol}`}
@@ -139,9 +142,8 @@ export default class EditApprovalPermission extends PureComponent { { + placeholder={ `${Number(customTokenAmount || tokenAmount)} ${tokenSymbol}` } + onChange={(event) => { this.setState({ customSpendLimit: event.target.value }) if (selectedOptionIsUnlimited) { this.setState({ selectedOptionIsUnlimited: false }) diff --git a/ui/app/pages/confirm-approve/confirm-approve-content/confirm-approve-content.component.js b/ui/app/pages/confirm-approve/confirm-approve-content/confirm-approve-content.component.js index 114fe166d39c..291849c88551 100644 --- a/ui/app/pages/confirm-approve/confirm-approve-content/confirm-approve-content.component.js +++ b/ui/app/pages/confirm-approve/confirm-approve-content/confirm-approve-content.component.js @@ -113,12 +113,8 @@ export default class ConfirmApproveContent extends Component { {t('accessAndSpendNotice', [origin])}
-
- {t('amountWithColon')} -
-
- {`${customTokenAmount || tokenAmount} ${tokenSymbol}`} -
+
{ t('amountWithColon') }
+
{ `${Number(customTokenAmount || tokenAmount)} ${tokenSymbol}` }
diff --git a/ui/app/pages/confirm-approve/confirm-approve.component.js b/ui/app/pages/confirm-approve/confirm-approve.component.js index 58c5dc706c68..bf3af94a8ef7 100644 --- a/ui/app/pages/confirm-approve/confirm-approve.component.js +++ b/ui/app/pages/confirm-approve/confirm-approve.component.js @@ -13,7 +13,7 @@ export default class ConfirmApprove extends Component { static propTypes = { tokenAddress: PropTypes.string, toAddress: PropTypes.string, - tokenAmount: PropTypes.number, + tokenAmount: PropTypes.string, tokenSymbol: PropTypes.string, fiatTransactionTotal: PropTypes.string, ethTransactionTotal: PropTypes.string, @@ -31,7 +31,7 @@ export default class ConfirmApprove extends Component { } static defaultProps = { - tokenAmount: 0, + tokenAmount: '0', } state = { @@ -67,7 +67,7 @@ export default class ConfirmApprove extends Component { } = this.props const { customPermissionAmount } = this.state - const tokensText = `${tokenAmount} ${tokenSymbol}` + const tokensText = `${Number(tokenAmount)} ${tokenSymbol}` let tokenBalance @@ -75,18 +75,18 @@ export default class ConfirmApprove extends Component { tokenBalance = tokenTrackerBalance.map(balance => (balance ? Number(calcTokenAmount(tokenTrackerBalance, decimals)).toPrecision( - 9 + 10 ) : '') ) } else { tokenBalance = tokenTrackerBalance - ? Number(calcTokenAmount(tokenTrackerBalance, decimals)).toPrecision(9) + ? Number(calcTokenAmount(tokenTrackerBalance, decimals)).toPrecision(10) : '' } const customData = customPermissionAmount - ? getCustomTxParamsData(data, { customPermissionAmount, tokenAmount, decimals }) + ? getCustomTxParamsData(data, { customPermissionAmount, decimals }) : null return ( @@ -102,7 +102,7 @@ export default class ConfirmApprove extends Component { this.setState({ customPermissionAmount: newAmount }) }} customTokenAmount={String(customPermissionAmount)} - tokenAmount={String(tokenAmount)} + tokenAmount={tokenAmount} origin={origin} tokenSymbol={tokenSymbol} tokenBalance={tokenBalance} diff --git a/ui/app/pages/confirm-approve/confirm-approve.container.js b/ui/app/pages/confirm-approve/confirm-approve.container.js index e1773f1b91f4..92223142f29c 100644 --- a/ui/app/pages/confirm-approve/confirm-approve.container.js +++ b/ui/app/pages/confirm-approve/confirm-approve.container.js @@ -56,8 +56,7 @@ const mapStateToProps = (state, ownProps) => { const tokenData = getTokenData(data) const tokenValue = tokenData && getTokenValue(tokenData.params) const toAddress = tokenData && getTokenToAddress(tokenData.params) - const tokenAmount = - tokenData && calcTokenAmount(tokenValue, decimals).toNumber() + const tokenAmount = tokenData && calcTokenAmount(tokenValue, decimals).toString(10) const contractExchangeRate = contractExchangeRateSelector(state) const { origin } = transaction diff --git a/ui/app/pages/confirm-approve/confirm-approve.util.js b/ui/app/pages/confirm-approve/confirm-approve.util.js index 4ebef1b05cdd..0318c6bed03f 100644 --- a/ui/app/pages/confirm-approve/confirm-approve.util.js +++ b/ui/app/pages/confirm-approve/confirm-approve.util.js @@ -1,36 +1,33 @@ import { decimalToHex } from '../../helpers/utils/conversions.util' import { calcTokenValue } from '../../helpers/utils/token-util.js' +import { getTokenData } from '../../helpers/utils/transactions.util' -export function getCustomTxParamsData ( - data, - { customPermissionAmount, tokenAmount, decimals } -) { - if (customPermissionAmount) { - const tokenValue = decimalToHex(calcTokenValue(tokenAmount, decimals)) +export function getCustomTxParamsData (data, { customPermissionAmount, decimals }) { + const tokenData = getTokenData(data) - const re = new RegExp('(^.+)' + tokenValue + '$') - const matches = re.exec(data) - - if (!matches || !matches[1]) { - return data - } - let dataWithoutCurrentAmount = matches[1] - const customPermissionValue = decimalToHex( - calcTokenValue(Number(customPermissionAmount), decimals) - ) + if (!tokenData) { + throw new Error('Invalid data') + } else if (tokenData.name !== 'approve') { + throw new Error(`Invalid data; should be 'approve' method, but instead is '${tokenData.name}'`) + } + let spender = tokenData.params[0].value + if (spender.startsWith('0x')) { + spender = spender.substring(2) + } + const [signature, tokenValue] = data.split(spender) - const differenceInLengths = customPermissionValue.length - tokenValue.length - const zeroModifier = dataWithoutCurrentAmount.length - differenceInLengths - if (differenceInLengths > 0) { - dataWithoutCurrentAmount = dataWithoutCurrentAmount.slice(0, zeroModifier) - } else if (differenceInLengths < 0) { - dataWithoutCurrentAmount = dataWithoutCurrentAmount.padEnd( - zeroModifier, - 0 - ) - } + if (!signature || !tokenValue) { + throw new Error('Invalid data') + } else if (tokenValue.length !== 64) { + throw new Error('Invalid token value; should be exactly 64 hex digits long (u256)') + } - const customTxParamsData = dataWithoutCurrentAmount + customPermissionValue - return customTxParamsData + let customPermissionValue = decimalToHex(calcTokenValue(customPermissionAmount, decimals)) + if (customPermissionValue.length > 64) { + throw new Error('Custom value is larger than u256') } + + customPermissionValue = customPermissionValue.padStart(tokenValue.length, '0') + const customTxParamsData = `${signature}${spender}${customPermissionValue}` + return customTxParamsData }