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

@@ -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 {Object} req - The watchAsset JSON-RPC request object.
-   * @param {any} res
-   * @param {Function} next
-   * @param {Function} end
    */
-  async requestWatchAsset(req, res, next, end) {
+  async requestWatchAsset(req) {
-    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
-      }
-    }
 
-    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
+    this._validateERC20AssetParams(tokenMetadata)
-    const rawAddress = address
+
-    try {
+    const address = normalizeAddress(tokenMetadata.address)
-      this._validateERC20AssetParams({ rawAddress, symbol, decimals })
+    const { symbol, decimals, image } = tokenMetadata
-    } catch (err) {
+    this._addSuggestedERC20Asset(address, symbol, decimals, image)
-      return err
+
-    }
+    await this.openPopup()
-    const tokenOpts = { rawAddress, decimals, symbol, image }
-    this.addSuggestedERC20Asset(tokenOpts)
-    return this.openPopup().then(() => {
     const tokenAddresses = this.getTokens().filter(
-        (token) => token.address === normalizeAddress(rawAddress),
+      (token) => token.address === address,
     )
     return tokenAddresses.length > 0
-    })
   }
 
   /**
@@ -800,26 +764,41 @@ export default class PreferencesController {
    * doesn't fulfill requirements
    *
    */
-  _validateERC20AssetParams({ rawAddress, symbol, decimals } = {}) {
+  _validateERC20AssetParams({ address, symbol, decimals }) {
-    if (!rawAddress || !symbol || typeof decimals === 'undefined') {
+    if (!address || !symbol || typeof decimals === 'undefined') {
-      throw new Error(
+      throw ethErrors.rpc.invalidParams(
-        `Cannot suggest token without address, symbol, and decimals`,
+        `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) {
+    if (!(symbol.length < 7)) {
-      throw new Error(`Invalid symbol ${symbol} more than six characters`)
+      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(
+      throw ethErrors.rpc.invalidParams(
-        `Invalid decimals ${decimals} must be at least 0, and not over 36`,
+        `Invalid decimals "${decimals}": must be 0 <= 36.`,
       )
     }
-    if (!isValidAddress(rawAddress)) {
+    if (!isValidAddress(address)) {
-      throw new Error(`Invalid address ${rawAddress}`)
+      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 })
+  }
 }

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 {

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())
 

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

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

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<WatchAssetParam>} req - The JSON-RPC request object.
+ * @param {import('json-rpc-engine').JsonRpcResponse<true>} 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)
+  }
+}

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

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: {} }
+      req = { method: 'wallet_watchAsset', params: {} }
-      res = {}
-      asy = { next: sandbox.spy(), end: sandbox.spy() }
       stubHandleWatchAssetERC20 = sandbox.stub(
         preferencesController,
         '_handleWatchAssetERC20',
       )
     })
+
     after(function () {
       sandbox.restore()
     })
 
-    it('shouldn not do anything if method not corresponds', async function () {
+    it('should error if passed no type', async function () {
-      req.method = 'metamask'
+      await assert.rejects(
-      await preferencesController.requestWatchAsset(req, res, asy.next, asy.end)
+        () => preferencesController.requestWatchAsset(req),
-      sandbox.assert.notCalled(asy.end)
+        { message: 'Asset of type "undefined" not supported.' },
-      sandbox.assert.called(asy.next)
+        '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)
+      await assert.rejects(
-      sandbox.assert.called(asy.end)
+        () => preferencesController.requestWatchAsset(req),
-      sandbox.assert.notCalled(asy.next)
+        { message: 'Asset of type "someasset" not supported.' },
-      req.method = addInternalMethodPrefix('watchAsset')
+        'should have errored',
-      req.params.type = 'someasset'
+      )
-      await preferencesController.requestWatchAsset(req, res, asy.next, asy.end)
-      sandbox.assert.calledTwice(asy.end)
-      sandbox.assert.notCalled(asy.next)
     })
-    it('should through error if method is supported but asset type is not', async function () {
+
-      req.method = 'metamask_watchAsset'
+    it('should handle ERC20 type', async function () {
-      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'
       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',
       )
     })