From 03a43cce64038c185f1b3b8ef5dcf7f21817f21d Mon Sep 17 00:00:00 2001 From: Esteban Mino Date: Tue, 2 Feb 2021 15:28:57 -0300 Subject: [PATCH 1/7] fix --- package.json | 1 - src/assets/AccountTrackerController.ts | 31 ++++++++++++++++++++------ src/util.ts | 29 ++++++++++++++++++++++++ yarn.lock | 10 --------- 4 files changed, 53 insertions(+), 18 deletions(-) diff --git a/package.json b/package.json index 6c8d1d8802..663dcf411e 100644 --- a/package.json +++ b/package.json @@ -47,7 +47,6 @@ "eth-sig-util": "^3.0.0", "ethereumjs-util": "^6.1.0", "ethereumjs-wallet": "^0.6.4", - "ethjs-query": "^0.3.8", "human-standard-collectible-abi": "^1.0.2", "human-standard-token-abi": "^2.0.0", "isomorphic-fetch": "^3.0.0", diff --git a/src/assets/AccountTrackerController.ts b/src/assets/AccountTrackerController.ts index 2cd3734936..11a44f3757 100644 --- a/src/assets/AccountTrackerController.ts +++ b/src/assets/AccountTrackerController.ts @@ -1,8 +1,9 @@ import BaseController, { BaseConfig, BaseState } from '../BaseController'; import PreferencesController from '../user/PreferencesController'; -import { BNToHex, safelyExecute } from '../util'; +import { BNToHex, safelyExecuteWithTimeout } from '../util'; -const EthjsQuery = require('ethjs-query'); +const EthQuery = require('eth-query'); +const { Mutex } = require('await-semaphore'); /** * @type AccountInformation @@ -42,7 +43,9 @@ export interface AccountTrackerState extends BaseState { * Controller that tracks information for all accounts in the current keychain */ export class AccountTrackerController extends BaseController { - private ethjsQuery: any; + private ethQuery: any; + + private mutex = new Mutex(); private handle?: NodeJS.Timer; @@ -64,6 +67,18 @@ export class AccountTrackerController extends BaseController { + return new Promise((resolve, reject) => { + this.ethQuery[method](...args, (error: Error, result: any) => { + if (error) { + reject(error); + return; + } + resolve(result); + }); + }); + } + /** * Name of this controller used during composition */ @@ -95,7 +110,7 @@ export class AccountTrackerController extends BaseController { + const releaseLock = await this.mutex.acquire(); interval && this.configure({ interval }, false, false); this.handle && clearTimeout(this.handle); - await safelyExecute(() => this.refresh()); + await this.refresh(); this.handle = setTimeout(() => { + releaseLock(); this.poll(this.config.interval); }, this.config.interval); } @@ -130,8 +147,8 @@ export class AccountTrackerController extends BaseController { - const balance = await this.ethjsQuery.getBalance(address); + await safelyExecuteWithTimeout(async () => { + const balance = await this.query('getBalance', [address]); accounts[address] = { balance: BNToHex(balance) }; this.update({ accounts: { ...accounts } }); }); diff --git a/src/util.ts b/src/util.ts index 81a990ce29..f069ad95c2 100644 --- a/src/util.ts +++ b/src/util.ts @@ -215,6 +215,34 @@ export async function safelyExecute(operation: () => Promise, logError = fa } } +/** + * Execute and return an asynchronous operation with a timeout + * + * @param operation - Function returning a Promise + * @param logError - Determines if the error should be logged + * @param retry - Function called if an error is caught + * @param timeout - Timeout to fail the operation + * @returns - Promise resolving to the result of the async operation + */ +export async function safelyExecuteWithTimeout(operation: () => Promise, logError = false, timeout = 500, retry?: (error: Error) => void) { + try { + return await Promise.race([ + operation(), + new Promise((_, reject) => + setTimeout(() => { + reject(new Error('timeout')); + }, timeout), + ), + ]); + } catch (error) { + /* istanbul ignore next */ + if (logError) { + console.error(error); + } + retry && retry(error); + } +} + /** * Validates a Transaction object for required properties and throws in * the event of any validation error. @@ -455,6 +483,7 @@ export default { isSmartContractCode, normalizeTransaction, safelyExecute, + safelyExecuteWithTimeout, successfulFetch, timeoutFetch, validateTokenToWatch, diff --git a/yarn.lock b/yarn.lock index e43545c1d8..bf330f6701 100644 --- a/yarn.lock +++ b/yarn.lock @@ -2808,16 +2808,6 @@ ethjs-query@0.3.7: ethjs-rpc "0.2.0" promise-to-callback "^1.0.0" -ethjs-query@^0.3.8: - version "0.3.8" - resolved "https://registry.yarnpkg.com/ethjs-query/-/ethjs-query-0.3.8.tgz#aa5af02887bdd5f3c78b3256d0f22ffd5d357490" - integrity sha512-/J5JydqrOzU8O7VBOwZKUWXxHDGr46VqNjBCJgBVNNda+tv7Xc8Y2uJc6aMHHVbeN3YOQ7YRElgIc0q1CI02lQ== - dependencies: - babel-runtime "^6.26.0" - ethjs-format "0.2.7" - ethjs-rpc "0.2.0" - promise-to-callback "^1.0.0" - ethjs-rpc@0.2.0: version "0.2.0" resolved "https://registry.yarnpkg.com/ethjs-rpc/-/ethjs-rpc-0.2.0.tgz#3d0011e32cfff156ed6147818c6fb8f801701b4c" From 85b086b7457a2021ddc3ac4c91f7727f4e8ec4e3 Mon Sep 17 00:00:00 2001 From: Esteban Mino Date: Tue, 2 Feb 2021 16:39:04 -0300 Subject: [PATCH 2/7] test --- src/assets/AccountTrackerController.ts | 16 +------ src/transaction/TransactionController.ts | 33 +++++--------- src/util.ts | 22 +++++++++ tests/TransactionController.test.ts | 1 + tests/util.test.ts | 58 ++++++++++++++++++++++++ 5 files changed, 94 insertions(+), 36 deletions(-) diff --git a/src/assets/AccountTrackerController.ts b/src/assets/AccountTrackerController.ts index 11a44f3757..d378669fe6 100644 --- a/src/assets/AccountTrackerController.ts +++ b/src/assets/AccountTrackerController.ts @@ -1,6 +1,6 @@ import BaseController, { BaseConfig, BaseState } from '../BaseController'; import PreferencesController from '../user/PreferencesController'; -import { BNToHex, safelyExecuteWithTimeout } from '../util'; +import { BNToHex, query, safelyExecuteWithTimeout } from '../util'; const EthQuery = require('eth-query'); const { Mutex } = require('await-semaphore'); @@ -67,18 +67,6 @@ export class AccountTrackerController extends BaseController { - return new Promise((resolve, reject) => { - this.ethQuery[method](...args, (error: Error, result: any) => { - if (error) { - reject(error); - return; - } - resolve(result); - }); - }); - } - /** * Name of this controller used during composition */ @@ -148,7 +136,7 @@ export class AccountTrackerController extends BaseController { - const balance = await this.query('getBalance', [address]); + const balance = await query(this.ethQuery, 'getBalance', [address]); accounts[address] = { balance: BNToHex(balance) }; this.update({ accounts: { ...accounts } }); }); diff --git a/src/transaction/TransactionController.ts b/src/transaction/TransactionController.ts index 404e52fc8c..33bb86e304 100644 --- a/src/transaction/TransactionController.ts +++ b/src/transaction/TransactionController.ts @@ -13,6 +13,7 @@ import { validateTransaction, isSmartContractCode, handleTransactionFetch, + query, } from '../util'; const MethodRegistry = require('eth-method-registry'); @@ -240,18 +241,6 @@ export class TransactionController extends BaseController { - return new Promise((resolve, reject) => { - this.ethQuery[method](...args, (error: Error, result: any) => { - if (error) { - reject(error); - return; - } - resolve(result); - }); - }); - } - private async registryLookup(fourBytePrefix: string): Promise { const registryMethod = await this.registry.lookup(fourBytePrefix); const parsedRegistryMethod = this.registry.parse(registryMethod); @@ -490,7 +479,7 @@ export class TransactionController extends BaseController { if (meta.status === 'submitted' && meta.networkID === currentNetworkID) { - const txObj = await this.query('getTransactionByHash', [meta.transactionHash]); + const txObj = await query(this.ethQuery, 'getTransactionByHash', [meta.transactionHash]); /* istanbul ignore else */ if (txObj && txObj.blockNumber) { transactions[index].status = 'confirmed'; @@ -806,7 +795,7 @@ export class TransactionController extends BaseController { + return new Promise((resolve, reject) => { + ethQuery[method](...args, (error: Error, result: any) => { + if (error) { + reject(error); + return; + } + resolve(result); + }); + }); +} + export default { BNToHex, fractionBN, + query, getBuyURL, handleFetch, hexToBN, diff --git a/tests/TransactionController.test.ts b/tests/TransactionController.test.ts index 5f4e81bcc5..3733d62502 100644 --- a/tests/TransactionController.test.ts +++ b/tests/TransactionController.test.ts @@ -12,6 +12,7 @@ jest.mock('eth-query', () => jest.fn().mockImplementation(() => { return { estimateGas: (_transaction: any, callback: any) => { + console.log('MOCK', callback); if (mockFlags.estimateGas) { callback(new Error(mockFlags.estimateGas)); return; diff --git a/tests/util.test.ts b/tests/util.test.ts index dfcc20d309..09f2c194e1 100644 --- a/tests/util.test.ts +++ b/tests/util.test.ts @@ -7,6 +7,46 @@ const { BN } = require('ethereumjs-util'); const SOME_API = 'https://someapi.com'; const SOME_FAILING_API = 'https://somefailingapi.com'; +const HttpProvider = require('ethjs-provider-http'); +const EthQuery = require('eth-query'); + +const mockFlags: { [key: string]: any } = { + estimateGas: null, + gasPrice: null, +}; +const PROVIDER = new HttpProvider('https://ropsten.infura.io/v3/341eacb578dd44a1a049cbc5f6fd4035'); + +jest.mock('eth-query', () => + jest.fn().mockImplementation(() => { + return { + estimateGas: (_transaction: any, callback: any) => { + callback(undefined, '0x0'); + }, + gasPrice: (callback: any) => { + if (mockFlags.gasPrice) { + callback(new Error(mockFlags.gasPrice)); + return; + } + callback(undefined, '0x0'); + }, + getBlockByNumber: (_blocknumber: any, _fetchTxs: boolean, callback: any) => { + callback(undefined, { gasLimit: '0x0' }); + }, + getCode: (_to: any, callback: any) => { + callback(undefined, '0x0'); + }, + getTransactionByHash: (_hash: any, callback: any) => { + callback(undefined, { blockNumber: '0x1' }); + }, + getTransactionCount: (_from: any, _to: any, callback: any) => { + callback(undefined, '0x0'); + }, + sendRawTransaction: (_transaction: any, callback: any) => { + callback(undefined, '1337'); + }, + }; + }), +); describe('util', () => { beforeEach(() => { @@ -577,4 +617,22 @@ describe('util', () => { expect(invalid).toBeNull(); }); }); + + describe('query', () => { + it('should query and resolve', async () => { + const ethQuery = new EthQuery(PROVIDER); + const gasPrice = await util.query(ethQuery, 'gasPrice', []); + expect(gasPrice).toEqual('0x0'); + }); + + it('should query and reject if error', async () => { + const ethQuery = new EthQuery(PROVIDER); + mockFlags.gasPrice = 'Uh oh'; + try { + await util.query(ethQuery, 'gasPrice', []); + } catch (error) { + expect(error.message).toContain('Uh oh'); + } + }); + }); }); From b1e68495dabca8e86085bb3f4030444dd77102f3 Mon Sep 17 00:00:00 2001 From: Esteban Mino Date: Tue, 2 Feb 2021 16:41:24 -0300 Subject: [PATCH 3/7] test --- src/util.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/src/util.ts b/src/util.ts index e6b369cea9..b374e5791c 100644 --- a/src/util.ts +++ b/src/util.ts @@ -239,6 +239,7 @@ export async function safelyExecuteWithTimeout(operation: () => Promise, lo if (logError) { console.error(error); } + /* istanbul ignore next */ retry && retry(error); } } From 69cf6b312ea25e6cce0815975150c01c39b3f353 Mon Sep 17 00:00:00 2001 From: Esteban Mino Date: Tue, 2 Feb 2021 16:47:33 -0300 Subject: [PATCH 4/7] prettier --- src/util.ts | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/src/util.ts b/src/util.ts index b374e5791c..5895d536fd 100644 --- a/src/util.ts +++ b/src/util.ts @@ -224,7 +224,12 @@ export async function safelyExecute(operation: () => Promise, logError = fa * @param timeout - Timeout to fail the operation * @returns - Promise resolving to the result of the async operation */ -export async function safelyExecuteWithTimeout(operation: () => Promise, logError = false, timeout = 500, retry?: (error: Error) => void) { +export async function safelyExecuteWithTimeout( + operation: () => Promise, + logError = false, + timeout = 500, + retry?: (error: Error) => void, +) { try { return await Promise.race([ operation(), From dea7d9a008ceff5b7d180340d4beb33620509e81 Mon Sep 17 00:00:00 2001 From: Esteban Mino Date: Tue, 2 Feb 2021 16:56:57 -0300 Subject: [PATCH 5/7] log --- tests/TransactionController.test.ts | 1 - tests/util.test.ts | 2 +- 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/tests/TransactionController.test.ts b/tests/TransactionController.test.ts index 3733d62502..5f4e81bcc5 100644 --- a/tests/TransactionController.test.ts +++ b/tests/TransactionController.test.ts @@ -12,7 +12,6 @@ jest.mock('eth-query', () => jest.fn().mockImplementation(() => { return { estimateGas: (_transaction: any, callback: any) => { - console.log('MOCK', callback); if (mockFlags.estimateGas) { callback(new Error(mockFlags.estimateGas)); return; diff --git a/tests/util.test.ts b/tests/util.test.ts index 09f2c194e1..919796c160 100644 --- a/tests/util.test.ts +++ b/tests/util.test.ts @@ -629,7 +629,7 @@ describe('util', () => { const ethQuery = new EthQuery(PROVIDER); mockFlags.gasPrice = 'Uh oh'; try { - await util.query(ethQuery, 'gasPrice', []); + await util.query(ethQuery, 'gasPrice', []); } catch (error) { expect(error.message).toContain('Uh oh'); } From f3a0c41e135bdeb422feb2a97bd70ca98cff66f0 Mon Sep 17 00:00:00 2001 From: Esteban Mino Date: Tue, 2 Feb 2021 17:15:50 -0300 Subject: [PATCH 6/7] safelyExecuteWithTimeouttest --- src/util.ts | 3 --- tests/util.test.ts | 25 +++++++++++++++++++++++++ 2 files changed, 25 insertions(+), 3 deletions(-) diff --git a/src/util.ts b/src/util.ts index 5895d536fd..f300d4bcf2 100644 --- a/src/util.ts +++ b/src/util.ts @@ -228,7 +228,6 @@ export async function safelyExecuteWithTimeout( operation: () => Promise, logError = false, timeout = 500, - retry?: (error: Error) => void, ) { try { return await Promise.race([ @@ -244,8 +243,6 @@ export async function safelyExecuteWithTimeout( if (logError) { console.error(error); } - /* istanbul ignore next */ - retry && retry(error); } } diff --git a/tests/util.test.ts b/tests/util.test.ts index 919796c160..4e0c1612a5 100644 --- a/tests/util.test.ts +++ b/tests/util.test.ts @@ -119,6 +119,31 @@ describe('util', () => { }); }); + describe('safelyExecuteWithTimeout', () => { + it('should swallow errors', async () => { + await util.safelyExecuteWithTimeout(() => { + throw new Error('ahh'); + }); + }); + + it('should resolve', async () => { + const response = await util.safelyExecuteWithTimeout(() => { + return new Promise((res) => setTimeout(() => res('response'), 200)); + }); + expect(response).toEqual('response'); + }); + + it('should timeout', () => { + try { + util.safelyExecuteWithTimeout(() => { + return new Promise((res) => setTimeout(res, 800)); + }); + } catch (e) { + expect(e.message).toContain('timeout'); + } + }); + }); + describe('validateTransaction', () => { it('should throw if no from address', () => { expect(() => util.validateTransaction({} as any)).toThrow(); From 527304b0957e0b540f13fea4d57c4db9392c2fb6 Mon Sep 17 00:00:00 2001 From: Esteban Mino Date: Tue, 2 Feb 2021 17:17:52 -0300 Subject: [PATCH 7/7] format --- src/util.ts | 6 +----- tests/util.test.ts | 8 ++++---- 2 files changed, 5 insertions(+), 9 deletions(-) diff --git a/src/util.ts b/src/util.ts index f300d4bcf2..5af4e371a6 100644 --- a/src/util.ts +++ b/src/util.ts @@ -224,11 +224,7 @@ export async function safelyExecute(operation: () => Promise, logError = fa * @param timeout - Timeout to fail the operation * @returns - Promise resolving to the result of the async operation */ -export async function safelyExecuteWithTimeout( - operation: () => Promise, - logError = false, - timeout = 500, -) { +export async function safelyExecuteWithTimeout(operation: () => Promise, logError = false, timeout = 500) { try { return await Promise.race([ operation(), diff --git a/tests/util.test.ts b/tests/util.test.ts index 4e0c1612a5..12a27e6df1 100644 --- a/tests/util.test.ts +++ b/tests/util.test.ts @@ -127,10 +127,10 @@ describe('util', () => { }); it('should resolve', async () => { - const response = await util.safelyExecuteWithTimeout(() => { - return new Promise((res) => setTimeout(() => res('response'), 200)); - }); - expect(response).toEqual('response'); + const response = await util.safelyExecuteWithTimeout(() => { + return new Promise((res) => setTimeout(() => res('response'), 200)); + }); + expect(response).toEqual('response'); }); it('should timeout', () => {