From baea1b57fb064a9e7a04a6b0e8b54501d61b5cc6 Mon Sep 17 00:00:00 2001 From: Erik Marks <25517051+rekmarks@users.noreply.github.com> Date: Thu, 3 Dec 2020 18:15:59 -0800 Subject: [PATCH] Update transaction params validation (#9992) * Update transaction params validation * fixup! Update transaction params validation * Update to/data error message * fixup! Update to/data error message --- app/scripts/controllers/transactions/index.js | 4 +- .../controllers/transactions/lib/util.js | 30 +++++++++++---- .../transactions/tx-controller-test.js | 7 +++- .../controllers/transactions/tx-utils-test.js | 38 ++++++++++++++++--- 4 files changed, 62 insertions(+), 17 deletions(-) diff --git a/app/scripts/controllers/transactions/index.js b/app/scripts/controllers/transactions/index.js index 0c6f3c6ac..4a4488fc1 100644 --- a/app/scripts/controllers/transactions/index.js +++ b/app/scripts/controllers/transactions/index.js @@ -827,9 +827,9 @@ export default class TransactionController extends EventEmitter { ].find((methodName) => methodName === name && name.toLowerCase()) let result - if (txParams.data && tokenMethodName) { + if (data && tokenMethodName) { result = tokenMethodName - } else if (txParams.data && !to) { + } else if (data && !to) { result = TRANSACTION_CATEGORIES.DEPLOY_CONTRACT } diff --git a/app/scripts/controllers/transactions/lib/util.js b/app/scripts/controllers/transactions/lib/util.js index 758d70121..b33d3029f 100644 --- a/app/scripts/controllers/transactions/lib/util.js +++ b/app/scripts/controllers/transactions/lib/util.js @@ -1,4 +1,5 @@ import { isValidAddress } from 'ethereumjs-util' +import { ethErrors } from 'eth-json-rpc-errors' import { addHexPrefix } from '../../../lib/util' import { TRANSACTION_STATUSES } from '../../../../../shared/constants/transaction' @@ -37,19 +38,30 @@ export function normalizeTxParams(txParams, lowerCase = true) { * @throws {Error} if the tx params contains invalid fields */ export function validateTxParams(txParams) { + if (!txParams || typeof txParams !== 'object' || Array.isArray(txParams)) { + throw ethErrors.rpc.invalidParams( + 'Invalid transaction params: must be an object.', + ) + } + if (!txParams.to && !txParams.data) { + throw ethErrors.rpc.invalidParams( + 'Invalid transaction params: must specify "data" for contract deployments, or "to" (and optionally "data") for all other types of transactions.', + ) + } + validateFrom(txParams) validateRecipient(txParams) if ('value' in txParams) { const value = txParams.value.toString() if (value.includes('-')) { - throw new Error( - `Invalid transaction value of ${txParams.value} not a positive number.`, + throw ethErrors.rpc.invalidParams( + `Invalid transaction value "${txParams.value}": not a positive number.`, ) } if (value.includes('.')) { - throw new Error( - `Invalid transaction value of ${txParams.value} number must be in wei`, + throw ethErrors.rpc.invalidParams( + `Invalid transaction value of "${txParams.value}": number must be in wei.`, ) } } @@ -62,10 +74,12 @@ export function validateTxParams(txParams) { */ export function validateFrom(txParams) { if (!(typeof txParams.from === 'string')) { - throw new Error(`Invalid from address ${txParams.from} not a string`) + throw ethErrors.rpc.invalidParams( + `Invalid "from" address "${txParams.from}": not a string.`, + ) } if (!isValidAddress(txParams.from)) { - throw new Error('Invalid from address') + throw ethErrors.rpc.invalidParams('Invalid "from" address.') } } @@ -80,10 +94,10 @@ export function validateRecipient(txParams) { if (txParams.data) { delete txParams.to } else { - throw new Error('Invalid recipient address') + throw ethErrors.rpc.invalidParams('Invalid "to" address.') } } else if (txParams.to !== undefined && !isValidAddress(txParams.to)) { - throw new Error('Invalid recipient address') + throw ethErrors.rpc.invalidParams('Invalid "to" address.') } return txParams } diff --git a/test/unit/app/controllers/transactions/tx-controller-test.js b/test/unit/app/controllers/transactions/tx-controller-test.js index f3426d5f9..0e39f791d 100644 --- a/test/unit/app/controllers/transactions/tx-controller-test.js +++ b/test/unit/app/controllers/transactions/tx-controller-test.js @@ -276,6 +276,7 @@ describe('Transaction Controller', function () { describe('#addUnapprovedTransaction', function () { const selectedAddress = '0x1678a085c290ebd122dc42cba69373b5953b831d' + const recipientAddress = '0xc42edfcc21ed14dda456aa0756c153f7985d8813' let getSelectedAddress, getPermittedAccounts beforeEach(function () { @@ -295,6 +296,7 @@ describe('Transaction Controller', function () { it('should add an unapproved transaction and return a valid txMeta', async function () { const txMeta = await txController.addUnapprovedTransaction({ from: selectedAddress, + to: recipientAddress, }) assert.ok('id' in txMeta, 'should have a id') assert.ok('time' in txMeta, 'should have a time stamp') @@ -321,7 +323,10 @@ describe('Transaction Controller', function () { done() }) txController - .addUnapprovedTransaction({ from: selectedAddress }) + .addUnapprovedTransaction({ + from: selectedAddress, + to: recipientAddress, + }) .catch(done) }) diff --git a/test/unit/app/controllers/transactions/tx-utils-test.js b/test/unit/app/controllers/transactions/tx-utils-test.js index 02c5171c7..77add3f62 100644 --- a/test/unit/app/controllers/transactions/tx-utils-test.js +++ b/test/unit/app/controllers/transactions/tx-utils-test.js @@ -6,21 +6,47 @@ describe('txUtils', function () { it('does not throw for positive values', function () { const sample = { from: '0x1678a085c290ebd122dc42cba69373b5953b831d', + to: '0xc42edfcc21ed14dda456aa0756c153f7985d8813', value: '0x01', } txUtils.validateTxParams(sample) }) - it('returns error for negative values', function () { + it('throws for invalid params value', function () { + assert.throws(() => txUtils.validateTxParams(), { + message: 'Invalid transaction params: must be an object.', + }) + assert.throws(() => txUtils.validateTxParams(null), { + message: 'Invalid transaction params: must be an object.', + }) + assert.throws(() => txUtils.validateTxParams(true), { + message: 'Invalid transaction params: must be an object.', + }) + assert.throws(() => txUtils.validateTxParams([]), { + message: 'Invalid transaction params: must be an object.', + }) + }) + + it('throws for missing "to" and "data"', function () { const sample = { from: '0x1678a085c290ebd122dc42cba69373b5953b831d', - value: '-0x01', + value: '0x01', } - try { - txUtils.validateTxParams(sample) - } catch (err) { - assert.ok(err, 'error') + assert.throws(() => txUtils.validateTxParams(sample), { + message: + 'Invalid transaction params: must specify "data" for contract deployments, or "to" (and optionally "data") for all other types of transactions.', + }) + }) + + it('throws for negative values', function () { + const sample = { + from: '0x1678a085c290ebd122dc42cba69373b5953b831d', + to: '0xc42edfcc21ed14dda456aa0756c153f7985d8813', + value: '-0x01', } + assert.throws(() => txUtils.validateTxParams(sample), { + message: 'Invalid transaction value "-0x01": not a positive number.', + }) }) })