From 119f686f0d2361fc18d7eda7134b679986d62360 Mon Sep 17 00:00:00 2001 From: Erik Marks <25517051+rekmarks@users.noreply.github.com> Date: Fri, 17 Apr 2020 15:04:45 -0700 Subject: [PATCH] Roll back to production API; fixup (#29) * disable unit tests * roll back to production API; un-deprecate sendAsync --- .circleci/config.yml | 28 +++---- .eslintrc.js | 4 +- index.js | 181 ++++++++++++++----------------------------- src/messages.js | 13 ++-- src/utils.js | 62 ++++----------- test/utils.js | 72 ----------------- 6 files changed, 99 insertions(+), 261 deletions(-) delete mode 100644 test/utils.js diff --git a/.circleci/config.yml b/.circleci/config.yml index 1f484e31..3d4f8440 100644 --- a/.circleci/config.yml +++ b/.circleci/config.yml @@ -7,13 +7,13 @@ workflows: - test-lint: requires: - prep-deps - - test-unit: - requires: - - prep-deps + # - test-unit: + # requires: + # - prep-deps - all-tests-pass: requires: - test-lint - - test-unit + # - test-unit jobs: prep-deps: @@ -46,16 +46,16 @@ jobs: name: Lint command: yarn lint - test-unit: - docker: - - image: circleci/node:10 - steps: - - checkout - - attach_workspace: - at: . - - run: - name: Unit tests - command: yarn test + # test-unit: + # docker: + # - image: circleci/node:10 + # steps: + # - checkout + # - attach_workspace: + # at: . + # - run: + # name: Unit tests + # command: yarn test all-tests-pass: docker: diff --git a/.eslintrc.js b/.eslintrc.js index f93e0504..a03c73bd 100644 --- a/.eslintrc.js +++ b/.eslintrc.js @@ -22,7 +22,9 @@ module.exports = { parserOptions: { ecmaVersion: 2018, }, - rules: {}, + rules: { + 'require-await': 'off', + }, ignorePatterns: [ '!.eslintrc.js', 'node_modules/', diff --git a/index.js b/index.js index d7b8ac29..24a1d209 100644 --- a/index.js +++ b/index.js @@ -15,22 +15,11 @@ const { sendSiteMetadata } = require('./src/siteMetadata') const { createErrorMiddleware, EMITTED_NOTIFICATIONS, + getRpcPromiseCallback, logStreamDisconnectWarning, - makeThenable, NOOP, } = require('./src/utils') -// resolve response.result, reject errors -const getRpcPromiseCallback = (resolve, reject) => (error, response) => { - if (error || response.error) { - reject(error || response.error) - } else { - Array.isArray(response) - ? resolve(response) - : resolve(response.result) - } -} - module.exports = class MetamaskInpageProvider extends SafeEventEmitter { constructor (connectionStream, shouldSendMetadata = true) { @@ -45,8 +34,8 @@ module.exports = class MetamaskInpageProvider extends SafeEventEmitter { enable: false, experimentalMethods: false, isConnected: false, - sendAsync: false, - // TODO:deprecate:2020-Q1 + send: false, + // TODO:deprecation:remove autoReload: false, sendSync: false, }, @@ -65,8 +54,8 @@ module.exports = class MetamaskInpageProvider extends SafeEventEmitter { // bind functions (to prevent e.g. web3@1.x from making unbound calls) this._handleAccountsChanged = this._handleAccountsChanged.bind(this) this._handleDisconnect = this._handleDisconnect.bind(this) - this._sendAsync = this._sendAsync.bind(this) - this._sendSync = this._sendSync.bind(this) + this._rpcRequest = this._rpcRequest.bind(this) + this._legacySend = this._legacySend.bind(this) this.enable = this.enable.bind(this) this.send = this.send.bind(this) this.sendAsync = this.sendAsync.bind(this) @@ -91,7 +80,7 @@ module.exports = class MetamaskInpageProvider extends SafeEventEmitter { if (this._state.isUnlocked) { // this will get the exposed accounts, if any try { - this._sendAsync( + this._rpcRequest( { method: 'eth_accounts', params: [] }, NOOP, true, // indicating that eth_accounts _should_ update accounts @@ -107,7 +96,7 @@ module.exports = class MetamaskInpageProvider extends SafeEventEmitter { if ('chainId' in state && state.chainId !== this.chainId) { this.chainId = state.chainId this.emit('chainChanged', this.chainId) - this.emit('chainIdChanged', this.chainId) // TODO:deprecate:2020-Q1 + this.emit('chainIdChanged', this.chainId) // TODO:deprecation:remove } // Emit networkChanged event on network change @@ -172,15 +161,15 @@ module.exports = class MetamaskInpageProvider extends SafeEventEmitter { // indicate that we've connected, for EIP-1193 compliance setTimeout(() => this.emit('connect')) - // TODO:deprecate:2020-Q1 + // TODO:deprecation:remove this._web3Ref = undefined - // TODO:deprecate:2020-Q1 + // TODO:deprecation:remove // give the dapps control of a refresh they can toggle this off on the window.ethereum // this will be default true so it does not break any old apps. this.autoRefreshOnNetworkChange = true - // TODO:deprecate:2020-Q1 + // TODO:deprecation:remove // wait a second to attempt to send this, so that the warning can be silenced // moved this here because there's another warning in .enable() discouraging // the use thereof per EIP 1102 @@ -193,7 +182,7 @@ module.exports = class MetamaskInpageProvider extends SafeEventEmitter { } /** - * Deprecated. + * DEPRECATED. * Returns whether the inpage provider is connected to MetaMask. */ isConnected () { @@ -206,92 +195,47 @@ module.exports = class MetamaskInpageProvider extends SafeEventEmitter { } /** - * Sends an RPC request to MetaMask. Resolves to the result of the method call. - * May reject with an error that must be caught by the caller. + * DEPRECATED + * Sends an RPC request to MetaMask. + * Many different return types, which is why this method should not be used. * - * @param {(string|Object)} methodOrPayload - The method name, or the RPC request object. - * @param {Array} [params] - If given a method name, the method's parameters. - * @returns {Promise} - A promise resolving to the result of the method call. + * @param {(string | Object)} methodOrPayload - The method name, or the RPC request object. + * @param {Array | Function} [callbackOrArgs] - If given a method name, the method's parameters. + * @returns {unknown} - The method result, or a JSON RPC response object. */ - send (methodOrPayload, params) { - - // preserve original params for later error if necessary - let _params = params - - // construct payload object - let payload - if ( - methodOrPayload && - typeof methodOrPayload === 'object' && - !Array.isArray(methodOrPayload) - ) { - - // TODO:deprecate:2020-Q1 - // handle send(object, callback), an alias for sendAsync(object, callback) - if (typeof _params === 'function') { - return this._sendAsync(methodOrPayload, _params) - } + send (methodOrPayload, callbackOrArgs) { - payload = methodOrPayload - - // TODO:deprecate:2020-Q1 - // backwards compatibility: "synchronous" methods - if (!_params && [ - 'eth_accounts', - 'eth_coinbase', - 'eth_uninstallFilter', - 'net_version', - ].includes(payload.method)) { - return this._sendSync(payload) - } - } else if ( - typeof methodOrPayload === 'string' && - typeof _params !== 'function' - ) { - - // wrap params in array out of kindness - // params have to be an array per EIP 1193, even though JSON RPC - // allows objects - if (_params === undefined) { - _params = [] - } else if (!Array.isArray(_params)) { - _params = [_params] - } - - payload = { - method: methodOrPayload, - params: _params, - } + if (!this._state.sentWarnings.send) { + log.warn(messages.warnings.sendDeprecation) + this._state.sentWarnings.send = true } - // typecheck payload and payload.params if ( - !payload || - typeof payload !== 'object' || - Array.isArray(payload) || - !Array.isArray(_params) + typeof methodOrPayload === 'string' && + (!callbackOrArgs || Array.isArray(callbackOrArgs)) ) { - throw ethErrors.rpc.invalidRequest({ - message: messages.errors.invalidParams(), - data: [methodOrPayload, params], + return new Promise((resolve, reject) => { + try { + this._rpcRequest( + { method: methodOrPayload, params: callbackOrArgs }, + getRpcPromiseCallback(resolve, reject), + ) + } catch (error) { + reject(error) + } }) + } else if ( + typeof methodOrPayload === 'object' && + typeof callbackOrArgs === 'function' + ) { + return this._rpcRequest(methodOrPayload, callbackOrArgs) } - - return new Promise((resolve, reject) => { - try { - this._sendAsync( - payload, - getRpcPromiseCallback(resolve, reject), - ) - } catch (error) { - reject(error) - } - }) + return this._legacySend(methodOrPayload) } /** - * Deprecated. - * Equivalent to: ethereum.send('eth_requestAccounts') + * DEPRECATED. + * Equivalent to: ethereum.request('eth_requestAccounts') * * @returns {Promise>} - A promise that resolves to an array of addresses. */ @@ -301,9 +245,10 @@ module.exports = class MetamaskInpageProvider extends SafeEventEmitter { log.warn(messages.warnings.enableDeprecation) this._state.sentWarnings.enable = true } + return new Promise((resolve, reject) => { try { - this._sendAsync( + this._rpcRequest( { method: 'eth_requestAccounts', params: [] }, getRpcPromiseCallback(resolve, reject), ) @@ -313,27 +258,23 @@ module.exports = class MetamaskInpageProvider extends SafeEventEmitter { }) } + // TODO:deprecation deprecate this method in favor of 'request' /** - * Deprecated. - * Backwards compatibility. ethereum.send() with callback. + * Backwards compatibility; ethereum.request() with JSON-RPC request object + * and a callback. * * @param {Object} payload - The RPC request object. * @param {Function} callback - The callback function. */ sendAsync (payload, cb) { - - if (!this._state.sentWarnings.sendAsync) { - log.warn(messages.warnings.sendAsyncDeprecation) - this._state.sentWarnings.sendAsync = true - } - this._sendAsync(payload, cb) + this._rpcRequest(payload, cb) } /** - * TODO:deprecate:2020-Q1 + * TODO:deprecation:remove * Internal backwards compatibility method. */ - _sendSync (payload) { + _legacySend (payload) { if (!this._state.sentWarnings.sendSync) { log.warn(messages.warnings.sendSyncDeprecation) @@ -352,7 +293,7 @@ module.exports = class MetamaskInpageProvider extends SafeEventEmitter { break case 'eth_uninstallFilter': - this._sendAsync(payload, NOOP) + this._rpcRequest(payload, NOOP) result = true break @@ -364,12 +305,11 @@ module.exports = class MetamaskInpageProvider extends SafeEventEmitter { throw new Error(messages.errors.unsupportedSync(payload.method)) } - // looks like a plain object, but behaves like a Promise if someone calls .then on it :evil_laugh: - return makeThenable({ + return { id: payload.id, jsonrpc: payload.jsonrpc, result, - }, 'result') + } } /** @@ -377,12 +317,12 @@ module.exports = class MetamaskInpageProvider extends SafeEventEmitter { * Also remap ids inbound and outbound. * * @param {Object} payload - The RPC request object. - * @param {Function} userCallback - The caller's callback. + * @param {Function} callback - The consumer's callback. * @param {boolean} isInternal - Whether the request is internal. */ - _sendAsync (payload, userCallback, isInternal = false) { + _rpcRequest (payload, callback, isInternal = false) { - let cb = userCallback + let cb = callback if (!Array.isArray(payload)) { @@ -402,11 +342,10 @@ module.exports = class MetamaskInpageProvider extends SafeEventEmitter { payload.method === 'eth_accounts', isInternal, ) - userCallback(err, res) + callback(err, res) } } } - this._rpcEngine.handle(payload, cb) } @@ -462,7 +401,7 @@ module.exports = class MetamaskInpageProvider extends SafeEventEmitter { this.selectedAddress = _accounts[0] || null } - // TODO:deprecate:2020-Q1 + // TODO:deprecation:remove // handle web3 if (this._web3Ref) { this._web3Ref.defaultAccount = this.selectedAddress @@ -513,7 +452,7 @@ function getExperimentalApi (instance) { return new Promise((resolve, reject) => { try { - instance._sendAsync( + instance._rpcRequest( requests, getRpcPromiseCallback(resolve, reject), ) @@ -523,9 +462,9 @@ function getExperimentalApi (instance) { }) }, - // TODO:deprecate:2020-Q1 isEnabled, isApproved + // TODO:deprecation:remove isEnabled, isApproved /** - * Deprecated. Will be removed in Q1 2020. + * DEPRECATED. Scheduled for removal. * Synchronously determines if this domain is currently enabled, with a potential false negative if called to soon * * @returns {boolean} - returns true if this domain is currently enabled @@ -535,7 +474,7 @@ function getExperimentalApi (instance) { }, /** - * Deprecated. Will be removed in Q1 2020. + * DEPRECATED. Scheduled for removal. * Asynchronously determines if this domain is currently enabled * * @returns {Promise} - Promise resolving to true if this domain is currently enabled diff --git a/src/messages.js b/src/messages.js index 15fa6dac..ed386414 100644 --- a/src/messages.js +++ b/src/messages.js @@ -1,17 +1,16 @@ module.exports = { errors: { - invalidParams: () => `MetaMask: Invalid request parameters. Please use ethereum.send(method: string, params: Array). For more details, see: https://eips.ethereum.org/EIPS/eip-1193`, + invalidParams: () => `MetaMask: Invalid request parameters. Please use ethereum.sendAsync(request: Object, callback: Function).`, sendSiteMetadata: () => `MetaMask: Failed to send site metadata. This is an internal error, please report this bug.`, - unsupportedSync: (method) => `MetaMask: The MetaMask Web3 object does not support synchronous methods like ${method} without a callback parameter.`, // TODO:deprecate:2020-Q1 + unsupportedSync: (method) => `MetaMask: The MetaMask Web3 object does not support synchronous methods like ${method} without a callback parameter.`, // TODO:deprecation:remove }, warnings: { - // TODO:deprecate:2020-Q1 - autoReloadDeprecation: `MetaMask: MetaMask will stop reloading pages on network change in Q1 2020. For more information, see: https://medium.com/metamask/no-longer-reloading-pages-on-network-change-fbf041942b44 \nSet 'ethereum.autoRefreshOnNetworkChange' to 'false' to silence this warning: https://metamask.github.io/metamask-docs/API_Reference/Ethereum_Provider#ethereum.autorefreshonnetworkchange`, - sendSyncDeprecation: `MetaMask: 'ethereum.send(...)' will return result-resolving Promises for all methods starting in Q1 2020. For more information, see: https://medium.com/metamask/deprecating-synchronous-provider-methods-82f0edbc874b`, + // TODO:deprecation:remove + autoReloadDeprecation: `MetaMask: MetaMask will stop reloading pages on network change in Q2 2020. For more information, see: https://medium.com/metamask/no-longer-reloading-pages-on-network-change-fbf041942b44 \nSet 'ethereum.autoRefreshOnNetworkChange' to 'false' to silence this warning: https://metamask.github.io/metamask-docs/API_Reference/Ethereum_Provider#ethereum.autorefreshonnetworkchange`, // deprecated stuff yet to be scheduled for removal - enableDeprecation: `MetaMask: 'ethereum.enable()' is deprecated and may be removed in the future. Please use "ethereum.send('eth_requestAccounts')" instead. For more information, see: https://eips.ethereum.org/EIPS/eip-1102`, + enableDeprecation: `MetaMask: 'ethereum.enable()' is deprecated and may be removed in the future. Please use "ethereum.sendAsync({ method: 'eth_requestAccounts' })" instead.`, isConnectedDeprecation: `MetaMask: 'ethereum.isConnected()' is deprecated and may be removed in the future. Please listen for the relevant events instead. For more information, see: https://eips.ethereum.org/EIPS/eip-1193`, - sendAsyncDeprecation: `MetaMask: 'ethereum.sendAsync(...)' is deprecated and may be removed in the future. Please use 'ethereum.send(method: string, params: Array)' instead. For more information, see: https://eips.ethereum.org/EIPS/eip-1193`, + sendDeprecation: `MetaMask: 'ethereum.send(...)' is deprecated and may be removed in the future. Please use 'ethereum.sendAsync({ method: string, params: Array | Object }, callback: Function)' instead.`, // misc experimentalMethods: `MetaMask: 'ethereum._metamask' exposes non-standard, experimental methods. They may be removed or changed without warning.`, }, diff --git a/src/utils.js b/src/utils.js index 0de05b2e..ddb60efa 100644 --- a/src/utils.js +++ b/src/utils.js @@ -3,11 +3,7 @@ const log = require('loglevel') const { ethErrors, serializeError } = require('eth-json-rpc-errors') const SafeEventEmitter = require('safe-event-emitter') -/** - * Middleware configuration object - * - * @typedef {Object} MiddlewareConfig - */ +// utility functions /** * json-rpc-engine middleware that both logs standard and non-standard error @@ -38,6 +34,17 @@ function createErrorMiddleware () { } } +// resolve response.result, reject errors +const getRpcPromiseCallback = (resolve, reject) => (error, response) => { + if (error || response.error) { + reject(error || response.error) + } else { + Array.isArray(response) + ? resolve(response) + : resolve(response.result) + } +} + /** * Logs a stream disconnection error. Emits an 'error' if bound to an * EventEmitter that has listeners for the 'error' event. @@ -58,56 +65,19 @@ function logStreamDisconnectWarning (remoteLabel, err) { } } -/** - * TODO:deprecate:2020-Q1 - * Adds hidden "then" and "catch" properties to the given object. When returned - * from a function, the given object will appear unchanged. If, however, the - * caller expects a Promise, it will behave like a Promise that resolves to - * the value of the indicated property. - * - * @param {Object} obj - The object to make thenable. - * @param {string} prop - The property whose value the object's then function resolves to. - * @returns {Object} - The secretly thenable object. - */ -function makeThenable (obj, prop) { - - // don't do anything to Promises - if (obj instanceof Promise) { - return obj - } - - const defineOpts = { - configurable: true, writable: true, enumerable: false, - } - - // strange wrapping of Promise functions to fully emulate .then behavior, - // specifically Promise chaining - // there may be a simpler way of doing it, but this works - const thenFunction = (consumerResolve, consumerCatch) => { - return Promise.resolve().then(() => consumerResolve(obj[prop]), consumerCatch) - } - - Object.defineProperty(obj, 'then', { ...defineOpts, value: thenFunction }) - - // the Promise will never fail in our usage, so just make a no-op "catch" - Object.defineProperty(obj, 'catch', { ...defineOpts, value: Promise.prototype.catch }) - - Object.defineProperty(obj, 'finally', { ...defineOpts, value: Promise.prototype.finally }) +// eslint-disable-next-line no-empty-function +const NOOP = () => {} - return obj -} +// constants const EMITTED_NOTIFICATIONS = [ 'eth_subscription', // per eth-json-rpc-filters/subscriptionManager ] -// eslint-disable-next-line no-empty-function -const NOOP = () => {} - module.exports = { createErrorMiddleware, EMITTED_NOTIFICATIONS, + getRpcPromiseCallback, logStreamDisconnectWarning, - makeThenable, NOOP, } diff --git a/test/utils.js b/test/utils.js deleted file mode 100644 index c7c6b604..00000000 --- a/test/utils.js +++ /dev/null @@ -1,72 +0,0 @@ - -const test = require('tape') - -const { makeThenable } = require('../src/utils') - -test('makeThenable objects are Promise ducks', async (t) => { - - const target = 'foo' - const responseObject = { - result: target, - jsonrpc: '2.0', - id: 2512950, - } - - const customThenable = (obj) => makeThenable(obj, 'result') - const promiseThenable = (obj) => Promise.resolve(obj.result) - - const customRes = await runThenableTests(customThenable) - const promiseRes = await runThenableTests(promiseThenable) - - await Promise.all(Object.entries(customRes).map(async ([k, v1]) => { - - const v2 = promiseRes[k] - - if (k === 'funcRes') { - t.deepEqual(v1, responseObject, 'makeThenable direct return is the target object') - t.ok(Boolean(v1.then), 'makeThenable direct return has hidden "then" property') - t.ok(v2 instanceof Promise, 'promiseThenable direct return is a Promise') - const v2res = await v2 - t.ok(v2res === target, 'promiseThenable direct return resolves to target value') - } else if (v1 instanceof Promise) { - t.ok(v2 instanceof Promise, 'value1 instanceof Promise -> value2 instanceof Promise') - const r1 = await v1 - const r2 = await v2 - t.deepEqual(r1, r2, 'promises resolve to the same values') - } else { - t.deepEqual(v1, v2, 'values are equal') - } - })) - - const response = customThenable({ ...responseObject }) - const stringResponse = JSON.stringify(response, null, 2) - t.comment(`serialized thenable response:\n${stringResponse}`) - t.deepEqual(JSON.parse(stringResponse), response, 'serializing and deserializing preserves response without "then"') - - t.end() - - async function runThenableTests (func) { - - const results = {} - - results.funcRes = func({ ...responseObject }) - - await func({ ...responseObject }).then((res) => { - results.res1then1 = res - }) - - const chainRes = await func({ ...responseObject }).then((res) => { - results.res2then1 = res - return res - }) - .then((res) => { - results.res2then2 = res - return res - }) - results.chainRes = chainRes - - results.asyncRes = await func({ ...responseObject }) - - return results - } -})