From c3eb272af9c279a59c247010b88d56d1adbe2fc6 Mon Sep 17 00:00:00 2001 From: Erik Marks <25517051+rekmarks@users.noreply.github.com> Date: Wed, 2 Dec 2020 08:49:49 -0800 Subject: [PATCH] Use method middleware for watchAsset (#9943) * Use method middleware for watchAsset * Update validation error messages * Make addSuggestedERC20Asset private * Remove redundant check in _handleWatchAssetERC20 --- app/scripts/controllers/preferences.js | 121 ++++++++---------- app/scripts/lib/enums.js | 2 + .../createMethodMiddleware.js | 4 +- .../rpc-method-middleware/handlers/index.js | 3 +- .../handlers/log-web3-usage.js | 2 +- .../handlers/watch-asset.js | 40 ++++++ app/scripts/metamask-controller.js | 9 +- .../preferences-controller-test.js | 65 ++++------ 8 files changed, 127 insertions(+), 119 deletions(-) create mode 100644 app/scripts/lib/rpc-method-middleware/handlers/watch-asset.js diff --git a/app/scripts/controllers/preferences.js b/app/scripts/controllers/preferences.js index a3c6fec92..c7bc88f0b 100644 --- a/app/scripts/controllers/preferences.js +++ b/app/scripts/controllers/preferences.js @@ -1,12 +1,12 @@ import { strict as assert } from 'assert' import ObservableStore from 'obs-store' +import { ethErrors } from 'eth-json-rpc-errors' import { normalize as normalizeAddress } from 'eth-sig-util' import { isValidAddress, sha3, bufferToHex } from 'ethereumjs-util' import ethers from 'ethers' import log from 'loglevel' import { isPrefixedFormattedHexString } from '../lib/util' import { LISTED_CONTRACT_ADDRESSES } from '../../../shared/constants/tokens' -import { addInternalMethodPrefix } from './permissions' import { NETWORK_TYPE_TO_ID_MAP } from './network/enums' export default class PreferencesController { @@ -171,22 +171,6 @@ export default class PreferencesController { return this.store.getState().assetImages } - addSuggestedERC20Asset(tokenOpts) { - this._validateERC20AssetParams(tokenOpts) - const suggested = this.getSuggestedTokens() - const { rawAddress, symbol, decimals, image } = tokenOpts - const address = normalizeAddress(rawAddress) - const newEntry = { - address, - symbol, - decimals, - image, - unlisted: !LISTED_CONTRACT_ADDRESSES.includes(address.toLowerCase()), - } - suggested[address] = newEntry - this.store.updateState({ suggestedTokens: suggested }) - } - /** * Add new methodData to state, to avoid requesting this information again through Infura * @@ -200,37 +184,21 @@ export default class PreferencesController { } /** - * RPC engine middleware for requesting new asset added + * wallet_watchAsset request handler. * - * @param {any} req - * @param {any} res - * @param {Function} next - * @param {Function} end + * @param {Object} req - The watchAsset JSON-RPC request object. */ - async requestWatchAsset(req, res, next, end) { - if ( - req.method === 'metamask_watchAsset' || - req.method === addInternalMethodPrefix('watchAsset') - ) { - const { type, options } = req.params - switch (type) { - case 'ERC20': { - const result = await this._handleWatchAssetERC20(options) - if (result instanceof Error) { - end(result) - } else { - res.result = result - end() - } - return - } - default: - end(new Error(`Asset of type ${type} not supported`)) - return - } - } + async requestWatchAsset(req) { + const { type, options } = req.params - next() + switch (type) { + case 'ERC20': + return await this._handleWatchAssetERC20(options) + default: + throw ethErrors.rpc.invalidParams( + `Asset of type "${type}" not supported.`, + ) + } } /** @@ -775,21 +743,17 @@ export default class PreferencesController { * */ async _handleWatchAssetERC20(tokenMetadata) { - const { address, symbol, decimals, image } = tokenMetadata - const rawAddress = address - try { - this._validateERC20AssetParams({ rawAddress, symbol, decimals }) - } catch (err) { - return err - } - const tokenOpts = { rawAddress, decimals, symbol, image } - this.addSuggestedERC20Asset(tokenOpts) - return this.openPopup().then(() => { - const tokenAddresses = this.getTokens().filter( - (token) => token.address === normalizeAddress(rawAddress), - ) - return tokenAddresses.length > 0 - }) + this._validateERC20AssetParams(tokenMetadata) + + const address = normalizeAddress(tokenMetadata.address) + const { symbol, decimals, image } = tokenMetadata + this._addSuggestedERC20Asset(address, symbol, decimals, image) + + await this.openPopup() + const tokenAddresses = this.getTokens().filter( + (token) => token.address === address, + ) + return tokenAddresses.length > 0 } /** @@ -800,26 +764,41 @@ export default class PreferencesController { * doesn't fulfill requirements * */ - _validateERC20AssetParams({ rawAddress, symbol, decimals } = {}) { - if (!rawAddress || !symbol || typeof decimals === 'undefined') { - throw new Error( - `Cannot suggest token without address, symbol, and decimals`, + _validateERC20AssetParams({ address, symbol, decimals }) { + if (!address || !symbol || typeof decimals === 'undefined') { + throw ethErrors.rpc.invalidParams( + `Must specify address, symbol, and decimals.`, ) } if (typeof symbol !== 'string') { - throw new Error(`Invalid symbol: not a string`) + throw ethErrors.rpc.invalidParams(`Invalid symbol: not a string.`) } - if (symbol.length > 6) { - throw new Error(`Invalid symbol ${symbol} more than six characters`) + if (!(symbol.length < 7)) { + throw ethErrors.rpc.invalidParams( + `Invalid symbol "${symbol}": longer than 6 characters.`, + ) } const numDecimals = parseInt(decimals, 10) if (isNaN(numDecimals) || numDecimals > 36 || numDecimals < 0) { - throw new Error( - `Invalid decimals ${decimals} must be at least 0, and not over 36`, + throw ethErrors.rpc.invalidParams( + `Invalid decimals "${decimals}": must be 0 <= 36.`, ) } - if (!isValidAddress(rawAddress)) { - throw new Error(`Invalid address ${rawAddress}`) + if (!isValidAddress(address)) { + throw ethErrors.rpc.invalidParams(`Invalid address "${address}".`) } } + + _addSuggestedERC20Asset(address, symbol, decimals, image) { + const newEntry = { + address, + symbol, + decimals, + image, + unlisted: !LISTED_CONTRACT_ADDRESSES.includes(address), + } + const suggested = this.getSuggestedTokens() + suggested[address] = newEntry + this.store.updateState({ suggestedTokens: suggested }) + } } diff --git a/app/scripts/lib/enums.js b/app/scripts/lib/enums.js index 543e8db36..d93030f2c 100644 --- a/app/scripts/lib/enums.js +++ b/app/scripts/lib/enums.js @@ -25,6 +25,8 @@ const MESSAGE_TYPE = { ETH_SIGN_TYPED_DATA: 'eth_signTypedData', LOG_WEB3_USAGE: 'metamask_logInjectedWeb3Usage', PERSONAL_SIGN: 'personal_sign', + WATCH_ASSET: 'wallet_watchAsset', + WATCH_ASSET_LEGACY: 'metamask_watchAsset', } export { diff --git a/app/scripts/lib/rpc-method-middleware/createMethodMiddleware.js b/app/scripts/lib/rpc-method-middleware/createMethodMiddleware.js index fff0afb16..b87047cd2 100644 --- a/app/scripts/lib/rpc-method-middleware/createMethodMiddleware.js +++ b/app/scripts/lib/rpc-method-middleware/createMethodMiddleware.js @@ -1,7 +1,9 @@ import handlers from './handlers' const handlerMap = handlers.reduce((map, handler) => { - map.set(handler.methodName, handler.implementation) + for (const methodName of handler.methodNames) { + map.set(methodName, handler.implementation) + } return map }, new Map()) diff --git a/app/scripts/lib/rpc-method-middleware/handlers/index.js b/app/scripts/lib/rpc-method-middleware/handlers/index.js index bc87cb309..74e26b675 100644 --- a/app/scripts/lib/rpc-method-middleware/handlers/index.js +++ b/app/scripts/lib/rpc-method-middleware/handlers/index.js @@ -1,4 +1,5 @@ import logWeb3Usage from './log-web3-usage' +import watchAsset from './watch-asset' -const handlers = [logWeb3Usage] +const handlers = [logWeb3Usage, watchAsset] export default handlers diff --git a/app/scripts/lib/rpc-method-middleware/handlers/log-web3-usage.js b/app/scripts/lib/rpc-method-middleware/handlers/log-web3-usage.js index 290c52b94..44bb3a2ca 100644 --- a/app/scripts/lib/rpc-method-middleware/handlers/log-web3-usage.js +++ b/app/scripts/lib/rpc-method-middleware/handlers/log-web3-usage.js @@ -8,7 +8,7 @@ import { MESSAGE_TYPE } from '../../enums' */ const logWeb3Usage = { - methodName: MESSAGE_TYPE.LOG_WEB3_USAGE, + methodNames: [MESSAGE_TYPE.LOG_WEB3_USAGE], implementation: logWeb3UsageHandler, } export default logWeb3Usage diff --git a/app/scripts/lib/rpc-method-middleware/handlers/watch-asset.js b/app/scripts/lib/rpc-method-middleware/handlers/watch-asset.js new file mode 100644 index 000000000..1c5f4c1db --- /dev/null +++ b/app/scripts/lib/rpc-method-middleware/handlers/watch-asset.js @@ -0,0 +1,40 @@ +import { MESSAGE_TYPE } from '../../enums' + +const watchAsset = { + methodNames: [MESSAGE_TYPE.WATCH_ASSET, MESSAGE_TYPE.WATCH_ASSET_LEGACY], + implementation: watchAssetHandler, +} +export default watchAsset + +/** + * @typedef {Object} WatchAssetOptions + * @property {Function} handleWatchAssetRequest - The wallet_watchAsset method implementation. + */ + +/** + * @typedef {Object} WatchAssetParam + * @property {string} type - The type of the asset to watch. + * @property {Object} options - Watch options for the asset. + */ + +/** + * @param {import('json-rpc-engine').JsonRpcRequest} req - The JSON-RPC request object. + * @param {import('json-rpc-engine').JsonRpcResponse} res - The JSON-RPC response object. + * @param {Function} _next - The json-rpc-engine 'next' callback. + * @param {Function} end - The json-rpc-engine 'end' callback. + * @param {WatchAssetOptions} options + */ +async function watchAssetHandler( + req, + res, + _next, + end, + { handleWatchAssetRequest }, +) { + try { + res.result = await handleWatchAssetRequest(req) + return end() + } catch (error) { + return end(error) + } +} diff --git a/app/scripts/metamask-controller.js b/app/scripts/metamask-controller.js index 5a4e69aaf..bbd2c9d0f 100644 --- a/app/scripts/metamask-controller.js +++ b/app/scripts/metamask-controller.js @@ -1968,6 +1968,9 @@ export default class MetamaskController extends EventEmitter { createMethodMiddleware({ origin, sendMetrics: this.trackMetaMetricsEvent, + handleWatchAssetRequest: this.preferencesController.requestWatchAsset.bind( + this.preferencesController, + ), }), ) // filter and subscription polyfills @@ -1979,12 +1982,6 @@ export default class MetamaskController extends EventEmitter { this.permissionsController.createMiddleware({ origin, extensionId }), ) } - // watch asset - engine.push( - this.preferencesController.requestWatchAsset.bind( - this.preferencesController, - ), - ) // forward to metamask primary provider engine.push(providerAsMiddleware(provider)) return engine diff --git a/test/unit/app/controllers/preferences-controller-test.js b/test/unit/app/controllers/preferences-controller-test.js index e041bb582..6a831c3e7 100644 --- a/test/unit/app/controllers/preferences-controller-test.js +++ b/test/unit/app/controllers/preferences-controller-test.js @@ -2,7 +2,6 @@ import assert from 'assert' import ObservableStore from 'obs-store' import sinon from 'sinon' import PreferencesController from '../../../../app/scripts/controllers/preferences' -import { addInternalMethodPrefix } from '../../../../app/scripts/controllers/permissions' describe('preferences controller', function () { let preferencesController @@ -405,53 +404,41 @@ describe('preferences controller', function () { }) describe('on watchAsset', function () { - let stubHandleWatchAssetERC20, asy, req, res + let req, stubHandleWatchAssetERC20 const sandbox = sinon.createSandbox() beforeEach(function () { - req = { params: {} } - res = {} - asy = { next: sandbox.spy(), end: sandbox.spy() } + req = { method: 'wallet_watchAsset', params: {} } stubHandleWatchAssetERC20 = sandbox.stub( preferencesController, '_handleWatchAssetERC20', ) }) + after(function () { sandbox.restore() }) - it('shouldn not do anything if method not corresponds', async function () { - req.method = 'metamask' - await preferencesController.requestWatchAsset(req, res, asy.next, asy.end) - sandbox.assert.notCalled(asy.end) - sandbox.assert.called(asy.next) + it('should error if passed no type', async function () { + await assert.rejects( + () => preferencesController.requestWatchAsset(req), + { message: 'Asset of type "undefined" not supported.' }, + 'should have errored', + ) }) - it('should do something if method is supported', async function () { - req.method = 'metamask_watchAsset' + + it('should error if method is not supported', async function () { req.params.type = 'someasset' - await preferencesController.requestWatchAsset(req, res, asy.next, asy.end) - sandbox.assert.called(asy.end) - sandbox.assert.notCalled(asy.next) - req.method = addInternalMethodPrefix('watchAsset') - req.params.type = 'someasset' - await preferencesController.requestWatchAsset(req, res, asy.next, asy.end) - sandbox.assert.calledTwice(asy.end) - sandbox.assert.notCalled(asy.next) + await assert.rejects( + () => preferencesController.requestWatchAsset(req), + { message: 'Asset of type "someasset" not supported.' }, + 'should have errored', + ) }) - it('should through error if method is supported but asset type is not', async function () { - req.method = 'metamask_watchAsset' - req.params.type = 'someasset' - await preferencesController.requestWatchAsset(req, res, asy.next, asy.end) - sandbox.assert.called(asy.end) - sandbox.assert.notCalled(stubHandleWatchAssetERC20) - sandbox.assert.notCalled(asy.next) - assert.deepEqual(res, {}) - }) - it('should trigger handle add asset if type supported', async function () { - req.method = 'metamask_watchAsset' + + it('should handle ERC20 type', async function () { req.params.type = 'ERC20' - await preferencesController.requestWatchAsset(req, res, asy.next, asy.end) + await preferencesController.requestWatchAsset(req) sandbox.assert.called(stubHandleWatchAssetERC20) }) }) @@ -527,7 +514,7 @@ describe('preferences controller', function () { assert.doesNotThrow(() => validate({ - rawAddress: '0xd26114cd6EE289AccF82350c8d8487fedB8A0C07', + address: '0xd26114cd6EE289AccF82350c8d8487fedB8A0C07', symbol: 'ABC', decimals: 0, }), @@ -539,7 +526,7 @@ describe('preferences controller', function () { assert.throws( () => validate({ - rawAddress: '0xd26114cd6EE289AccF82350c8d8487fedB8A0C07', + address: '0xd26114cd6EE289AccF82350c8d8487fedB8A0C07', decimals: 0, }), 'missing symbol should fail', @@ -547,7 +534,7 @@ describe('preferences controller', function () { assert.throws( () => validate({ - rawAddress: '0xd26114cd6EE289AccF82350c8d8487fedB8A0C07', + address: '0xd26114cd6EE289AccF82350c8d8487fedB8A0C07', symbol: 'ABC', }), 'missing decimals should fail', @@ -555,7 +542,7 @@ describe('preferences controller', function () { assert.throws( () => validate({ - rawAddress: '0xd26114cd6EE289AccF82350c8d8487fedB8A0C07', + address: '0xd26114cd6EE289AccF82350c8d8487fedB8A0C07', symbol: 'ABCDEFGHI', decimals: 0, }), @@ -564,7 +551,7 @@ describe('preferences controller', function () { assert.throws( () => validate({ - rawAddress: '0xd26114cd6EE289AccF82350c8d8487fedB8A0C07', + address: '0xd26114cd6EE289AccF82350c8d8487fedB8A0C07', symbol: 'ABC', decimals: -1, }), @@ -573,14 +560,14 @@ describe('preferences controller', function () { assert.throws( () => validate({ - rawAddress: '0xd26114cd6EE289AccF82350c8d8487fedB8A0C07', + address: '0xd26114cd6EE289AccF82350c8d8487fedB8A0C07', symbol: 'ABC', decimals: 38, }), 'decimals > 36 should fail', ) assert.throws( - () => validate({ rawAddress: '0x123', symbol: 'ABC', decimals: 0 }), + () => validate({ address: '0x123', symbol: 'ABC', decimals: 0 }), 'invalid address should fail', ) })