Handle unsuccessful fetch (#157)

* Add 'isomorphic-fetch' import to index

This ensures that 'isomorphic-fetch' is imported before anything else.

Previously 'isomorphic-fetch' was imported at the beginning of each
separate module that used 'fetch', which was needlessly verbose and
easy to forget to do (it was missing from one module already).

This would normally break the tests, as the tests don't import modules
via the main index file. This was resolved by adding a `setupTests`
file that imports `isomorphic-fetch` before running any tests.

* Handle unsuccessful fetch attempts

The `fetch` API will throw an exception if a network error is
encountered during a `fetch` invocation, but an unsuccessful response
will not. We should never assume that the response from `fetch` is
successful without checking first. We can't even assume that
successfully calling `.json()` on a request implies that it succeeded,
because failed responses can pass information in the response body as
well.

A `successfulFetch` utility method has been added that verifies the
fetch response was successful, and throws an error if it was not.
This method is now used for the other two `fetch` utility methods, and
each instance of `fetch` has been updated to use one of these methods.

New tests have been added for `successfulFetch`. `fetch-mock` was
updated to make it easier to specify the status of responses from calls
to the mock fetch instance.

package.json

@@ -33,7 +33,8 @@
         "lines": 100,
         "statements": 100
       }
-    }
+    },
+    "setupFiles": ["./tests/setupTests.ts"]
   },
   "prettier": {
     "arrowParens": "always",
@@ -51,14 +52,14 @@
     ]
   },
   "devDependencies": {
-    "@types/fetch-mock": "^6.0.3",
+    "@types/fetch-mock": "^7.3.1",
     "@types/jest": "^22.2.3",
     "@types/node": "^10.1.4",
     "@types/sinon": "^4.3.3",
     "@types/web3": "^1.0.6",
     "@types/xtend": "^4.0.2",
     "ethjs-provider-http": "^0.1.6",
-    "fetch-mock": "^6.4.3",
+    "fetch-mock": "^7.3.9",
     "husky": "^0.14.3",
     "jest": "^24.9.0",
     "jsdom": "11.11.0",

src/assets/AssetsContractController.ts

@@ -1,4 +1,3 @@
-import 'isomorphic-fetch';
 import BaseController, { BaseConfig, BaseState } from '../BaseController';
 
 const BN = require('ethereumjs-util').BN;

src/assets/AssetsController.ts

@@ -1,4 +1,3 @@
-import 'isomorphic-fetch';
 import { toChecksumAddress } from 'ethereumjs-util';
 import BaseController, { BaseConfig, BaseState } from '../BaseController';
 import PreferencesController from '../user/PreferencesController';

src/assets/AssetsDetectionController.ts

@@ -1,4 +1,3 @@
-import 'isomorphic-fetch';
 import { toChecksumAddress } from 'ethereumjs-util';
 import BaseController, { BaseConfig, BaseState } from '../BaseController';
 import AssetsController from './AssetsController';

src/assets/CurrencyRateController.ts

@@ -1,7 +1,6 @@
-import 'isomorphic-fetch';
 const Mutex = require('await-semaphore').Mutex;
 import BaseController, { BaseConfig, BaseState } from '../BaseController';
-import { safelyExecute } from '../util';
+import { safelyExecute, handleFetch } from '../util';
 
 /**
  * @type CurrencyRateConfig
@@ -124,8 +123,7 @@ export class CurrencyRateController extends BaseController<CurrencyRateConfig, C
 	 * @returns - Promise resolving to exchange rate for given currency
 	 */
 	async fetchExchangeRate(currency: string, nativeCurrency = this.activeNativeCurrency): Promise<CurrencyRateState> {
-		const response = await fetch(this.getPricingURL(currency, nativeCurrency));
-		const json = await response.json();
+		const json = await handleFetch(this.getPricingURL(currency, nativeCurrency));
 		return {
 			conversionDate: Date.now() / 1000,
 			conversionRate: Number(json[currency.toUpperCase()]),

src/assets/TokenBalancesController.ts

@@ -1,4 +1,3 @@
-import 'isomorphic-fetch';
 import BaseController, { BaseConfig, BaseState } from '../BaseController';
 import AssetsController from './AssetsController';
 import { Token } from './TokenRatesController';

src/assets/TokenRatesController.ts

@@ -1,8 +1,7 @@
-import 'isomorphic-fetch';
 import { toChecksumAddress } from 'ethereumjs-util';
 import BaseController, { BaseConfig, BaseState } from '../BaseController';
 import AssetsController from './AssetsController';
-import { safelyExecute } from '../util';
+import { safelyExecute, handleFetch } from '../util';
 import CurrencyRateController from './CurrencyRateController';
 
 /**
@@ -132,9 +131,7 @@ export class TokenRatesController extends BaseController<TokenRatesConfig, Token
 	 * @returns - Promise resolving to exchange rates for given pairs
 	 */
 	async fetchExchangeRate(query: string): Promise<CoinGeckoResponse> {
-		const response = await fetch(this.getPricingURL(query));
-		const json = await response.json();
-		return json;
+		return handleFetch(this.getPricingURL(query));
 	}
 
 	/**

src/index.ts

@@ -1,3 +1,4 @@
+import 'isomorphic-fetch';
 import * as util from './util';
 
 export * from './assets/AccountTrackerController';

src/keyring/KeyringController.ts

@@ -1,4 +1,3 @@
-import 'isomorphic-fetch';
 import { toChecksumAddress } from 'ethereumjs-util';
 import BaseController, { BaseConfig, BaseState, Listener } from '../BaseController';
 import PreferencesController from '../user/PreferencesController';

src/network/NetworkStatusController.ts

@@ -1,6 +1,5 @@
-import 'isomorphic-fetch';
 import BaseController, { BaseConfig, BaseState } from '../BaseController';
-import { safelyExecute } from '../util';
+import { safelyExecute, handleFetch } from '../util';
 
 /**
  * Network status code string
@@ -98,8 +97,7 @@ export class NetworkStatusController extends BaseController<NetworkStatusConfig,
 	 */
 	async updateInfuraStatus(): Promise<NetworkStatus> {
 		try {
-			const response = await fetch('https://api.infura.io/v1/status/metamask');
-			const json = await response.json();
+			const json = await handleFetch('https://api.infura.io/v1/status/metamask');
 			return json && json.mainnet ? json : /* istanbul ignore next */ DOWN_NETWORK_STATUS;
 		} catch (error) {
 			/* istanbul ignore next */

src/third-party/PhishingController.ts

@@ -1,6 +1,5 @@
-import 'isomorphic-fetch';
 import BaseController, { BaseConfig, BaseState } from '../BaseController';
-import { safelyExecute } from '../util';
+import { safelyExecute, handleFetch } from '../util';
 
 const DEFAULT_PHISHING_RESPONSE = require('eth-phishing-detect/src/config.json');
 const PhishingDetector = require('eth-phishing-detect/src/detector');
@@ -127,8 +126,7 @@ export class PhishingController extends BaseController<PhishingConfig, PhishingS
 			return;
 		}
 
-		const response = await fetch('https://api.infura.io/v2/blacklist');
-		const phishing = await response.json();
+		const phishing = await handleFetch('https://api.infura.io/v2/blacklist');
 		this.detector = new PhishingDetector(phishing);
 		phishing && this.update({ phishing });
 	}

src/third-party/ShapeShiftController.ts

@@ -1,6 +1,5 @@
-import 'isomorphic-fetch';
 import BaseController, { BaseConfig, BaseState } from '../BaseController';
-import { safelyExecute } from '../util';
+import { safelyExecute, handleFetch } from '../util';
 
 /**
  * @type ShapeShiftTransaction
@@ -70,8 +69,7 @@ export class ShapeShiftController extends BaseController<ShapeShiftConfig, Shape
 
 	private async updateTransaction(transaction: ShapeShiftTransaction) {
 		return safelyExecute(async () => {
-			const response = await fetch(this.getUpdateURL(transaction));
-			transaction.response = await response.json();
+			transaction.response = await handleFetch(this.getUpdateURL(transaction));
 			if (transaction.response && transaction.response.status === 'complete') {
 				transaction.time = Date.now();
 			}

src/transaction/TransactionController.ts

@@ -5,6 +5,7 @@ import NetworkController from '../network/NetworkController';
 import {
 	BNToHex,
 	fractionBN,
+	handleFetch,
 	hexToBN,
 	normalizeTransaction,
 	safelyExecute,
@@ -634,8 +635,7 @@ export class TransactionController extends BaseController<TransactionConfig, Tra
 		if (fromBlock) {
 			url += `&startBlock=${fromBlock}`;
 		}
-		const response = await fetch(url);
-		const parsedResponse = await response.json();
+		const parsedResponse = await handleFetch(url);
 		/* istanbul ignore else */
 		if (parsedResponse.status !== '0' && parsedResponse.result.length > 0) {
 			const remoteTxList: { [key: string]: number } = {};

src/util.ts

@@ -288,6 +288,21 @@ export function isSmartContractCode(code: string) {
 	return smartContractCode;
 }
 
+/**
+ * Execute fetch and verify that the response was successful
+ *
+ * @param request - Request information
+ * @param options - Options
+ * @returns - Promise resolving to the fetch response
+ */
+export async function successfulFetch(request: string, options?: RequestInit) {
+	const response = await fetch(request, options);
+	if (!response.ok) {
+		throw new Error(`Fetch failed with status '${response.status}' for request '${request}'`);
+	}
+	return response;
+}
+
 /**
  * Execute fetch and return object response
  *
@@ -296,7 +311,7 @@ export function isSmartContractCode(code: string) {
  * @returns - Promise resolving to the result object of fetch
  */
 export async function handleFetch(request: string, options?: RequestInit) {
-	const response = await fetch(request, options);
+	const response = await successfulFetch(request, options);
 	const object = await response.json();
 	return object;
 }
@@ -312,7 +327,7 @@ export async function handleFetch(request: string, options?: RequestInit) {
  */
 export async function timeoutFetch(url: string, options?: RequestInit, timeout: number = 500): Promise<Response> {
 	return Promise.race([
-		fetch(url, options),
+		successfulFetch(url, options),
 		new Promise<Response>((_, reject) =>
 			setTimeout(() => {
 				reject(new Error('timeout'));
@@ -354,6 +369,7 @@ export default {
 	isSmartContractCode,
 	normalizeTransaction,
 	safelyExecute,
+	successfulFetch,
 	timeoutFetch,
 	validateTokenToWatch,
 	validateTransaction,

tests/CurrencyRateController.test.ts

@@ -1,15 +1,18 @@
+import 'isomorphic-fetch';
 import { stub } from 'sinon';
+import * as fetchMock from 'fetch-mock';
 import CurrencyRateController from '../src/assets/CurrencyRateController';
 
 describe('CurrencyRateController', () => {
 	beforeEach(() => {
-		const mock = stub(window, 'fetch');
-		mock.resolves({
-			json: () => ({ USD: 1337 })
-		});
-		mock.restore();
+		fetchMock
+			.reset()
+			.mock('*', () => new Response(JSON.stringify({ USD: 1337 })))
+			.spy();
 	});
 
+	afterEach(fetchMock.reset);
+
 	it('should set default state', () => {
 		const controller = new CurrencyRateController();
 		expect(controller.state).toEqual({
@@ -79,12 +82,7 @@ describe('CurrencyRateController', () => {
 	it('should use default base asset', async () => {
 		const nativeCurrency = 'FOO';
 		const controller = new CurrencyRateController({ nativeCurrency });
-		const mock = stub(window, 'fetch');
-		mock.resolves({
-			json: () => ({ USD: 1337 })
-		});
 		await controller.fetchExchangeRate('usd');
-		mock.restore();
-		expect(mock.getCall(0).args[0]).toContain(nativeCurrency);
+		expect(fetchMock.calls()[0][0]).toContain(nativeCurrency);
 	});
 });

tests/setupTests.ts

@@ -0,0 +1 @@
+import 'isomorphic-fetch';

tests/util.test.ts

@@ -1,10 +1,17 @@
+import 'isomorphic-fetch';
+import * as fetchMock from 'fetch-mock';
+
 import * as util from '../src/util';
-import { mock } from 'fetch-mock';
 
 const { BN } = require('ethereumjs-util');
 const SOME_API = 'https://someapi.com';
+const SOME_FAILING_API = 'https://somefailingapi.com';
 
 describe('util', () => {
+	beforeEach(() => {
+		fetchMock.reset();
+	});
+
 	it('BNToHex', () => {
 		expect(util.BNToHex(new BN('1337'))).toBe('0x539');
 	});
@@ -426,15 +433,42 @@ describe('util', () => {
 		});
 	});
 
+	describe('successfulFetch', () => {
+		beforeEach(() => {
+			fetchMock
+				.mock(SOME_API, new Response(JSON.stringify({ foo: 'bar' }), { status: 200 }))
+				.mock(SOME_FAILING_API, new Response('response', { status: 500 }));
+		});
+
+		it('should return successful fetch response', async () => {
+			const res = await util.successfulFetch(SOME_API);
+			const parsed = await res.json();
+			expect(parsed).toEqual({ foo: 'bar' });
+		});
+
+		it('should throw error for an unsuccessful fetch', async () => {
+			let error;
+			try {
+				await util.successfulFetch(SOME_FAILING_API);
+			} catch (e) {
+				error = e;
+			}
+			expect(error.message).toBe(`Fetch failed with status '500' for request '${SOME_FAILING_API}'`);
+		});
+	});
+
 	describe('timeoutFetch', () => {
 		const delay = (time: number) => {
 			return new Promise((resolve) => {
 				setTimeout(resolve, time);
 			});
 		};
-		mock(SOME_API, () => {
-			return delay(300).then(() => {
-				return JSON.stringify({});
+
+		beforeEach(() => {
+			fetchMock.mock(SOME_API, () => {
+				return delay(300).then(() => {
+					return JSON.stringify({});
+				});
 			});
 		});