Fix Promises/tryUntil and improve tests (coinbase#130)

* Fix a bug in tryUntil that was causing issues in FXUtils * Augment tests
schalk · Jan 11, 2018 · 7182faf · 7182faf
1 parent 13a3324
commit 7182faf
Show file tree

Hide file tree

Showing 10 changed files with 726 additions and 505 deletions.
diff --git a/package.json b/package.json
@@ -50,7 +50,7 @@
     "@types/ws": "0.0.37",
     "bignumber.js": "4.0.2",
     "bintrees": "1.0.1",
-    "ccxt": "1.10.55",
+    "ccxt": "1.10.641",
     "commander": "2.9.0",
     "crypto": "0.0.3",
     "gdax": "https://github.com/coinbase/gdax-node.git#32360ad73517f168054585a1b0fd6ae3d7e12a77",

diff --git a/src/FXService/calculators/FailoverCalculator.ts b/src/FXService/calculators/FailoverCalculator.ts
@@ -22,6 +22,10 @@ export interface FailoverCalculatorConfig {
     logger?: Logger;
 }
 
+export interface LastRequestInfo {
+    calculator: FXRateCalculator;
+}
+
 /**
  * A simple FX rate calculator that uses a single FXProvider and return the current exchange rate from it directly.
  * If the pair is unavailable, or some other error occurs, the calculator returns null for that pair
@@ -37,7 +41,7 @@ export default class FailoverCalculator extends FXRateCalculator {
         this.logger = config.logger || ConsoleLoggerFactory();
     }
 
-    getLastRequestInfo(): any {
+    getLastRequestInfo(): LastRequestInfo {
         return { calculator: this.lastCalculatorUsed };
     }
 

diff --git a/src/utils/promises.ts b/src/utils/promises.ts
@@ -57,21 +57,20 @@ export function eachParallelAndFinish<T, U>(arr: T[], iteratorFn: (arg: T) => Pr
  * Applies iteratorFn to each element in arr until a 'true' result is returned. Rejected promises are swallowed. A false result is returned only
  * if every iteratorFn(i) returns false or an Error
  */
-export function tryUntil<T, U>(arr: T[], iteratorFn: (arg: T) => Promise<U | boolean>, index: number = 0): Promise<U | boolean> {
+export async function tryUntil<T, U>(arr: T[], iteratorFn: (arg: T) => Promise<U | boolean>, index: number = 0): Promise<U | boolean> {
     if (arr.length < 1) {
         return Promise.resolve(false);
     }
-    return iteratorFn(arr[index]).then((result: U | boolean) => {
-       if (result !== false) {
-           return Promise.resolve(result);
-       }
-       if (++index >= arr.length) {
-           return Promise.resolve(false);
-       }
-       return tryUntil(arr, iteratorFn, index);
-    }).catch(() => {
-        // Swallow the error and continue
-        const clone: T[] = arr.slice();
-        return tryUntil(clone.splice(0,1), iteratorFn);
-    });
+    const args: T[] = arr.slice();
+    while (args.length > 0) {
+        try {
+            const itemResult: U | boolean = await iteratorFn(args.shift());
+            if (itemResult !== false) {
+                return Promise.resolve(itemResult);
+            }
+        } catch (err) {
+            // Swallow the error and continue
+        }
+    }
+    return Promise.resolve(false);
 }
diff --git a/test/FXService/FailoverCalculatorTest.ts b/test/FXService/FailoverCalculatorTest.ts
@@ -25,12 +25,13 @@ const nock = require('nock');
 
 describe('FailoverCalculator', () => {
     let calculator: FailoverCalculator;
-
+    let calc1: SimpleRateCalculator;
+    let calc2: SimpleRateCalculator;
     before(() => {
         const provider1 = new CryptoProvider({ logger: NullLogger, exchange: GDAX(NullLogger) });
         const provider2 = new CryptoProvider({ logger: NullLogger, exchange: Bitfinex(NullLogger) });
-        const calc1 = new SimpleRateCalculator(provider1, NullLogger);
-        const calc2 = new SimpleRateCalculator(provider2, NullLogger);
+        calc1 = new SimpleRateCalculator(provider1, NullLogger);
+        calc2 = new SimpleRateCalculator(provider2, NullLogger);
         calculator = new FailoverCalculator({
             logger: NullLogger,
             calculators: [calc1, calc2]
@@ -61,6 +62,8 @@ describe('FailoverCalculator', () => {
             assert.equal(results[0].rate.toNumber(), 10500);
             assert.equal(results[0].from, 'BTC');
             assert.equal(results[0].to, 'USD');
+            const info = calculator.getLastRequestInfo();
+            assert.equal(info.calculator, calc1);
         });
     });
 
@@ -79,6 +82,8 @@ describe('FailoverCalculator', () => {
             assert.equal(results[0].rate.toNumber(), 500);
             assert.equal(results[0].from, 'ETH');
             assert.equal(results[0].to, 'USD');
+            const info = calculator.getLastRequestInfo();
+            assert.equal(info.calculator, calc2);
         });
     });
 
@@ -118,6 +123,41 @@ describe('FailoverCalculator', () => {
             assert.equal(results[1].rate.toNumber(), 405);
             assert.equal(results[1].from, 'ETH');
             assert.equal(results[1].to, 'USD');
+            const info = calculator.getLastRequestInfo();
+            assert.equal(info.calculator, calc2);
+        });
+    });
+
+    it('returns results from the second provider if the first errors out', () => {
+        nock('https://api.gdax.com:443')
+            .get('/products/BTC-USD/ticker')
+            .reply(500);
+        nock('https://api.bitfinex.com:443')
+            .get('/v1/pubticker/btcusd')
+            .reply(200, {
+                bid: '400.00',
+                ask: '410.00',
+                last_price: '410.0'
+            });
+        return calculator.calculateRatesFor([{ from: 'BTC', to: 'USD' }]).then((results: FXObject[]) => {
+            const rate: FXObject = results[0];
+            assert.equal(rate.rate.toNumber(), 405);
+            const info = calculator.getLastRequestInfo();
+            assert.equal(info.calculator, calc2);
+        });
+    });
+
+    it('returns null if all providers fail', () => {
+        nock('https://api.gdax.com:443')
+            .get('/products/BTC-USD/ticker')
+            .reply(500);
+        nock('https://api.bitfinex.com:443')
+            .get('/v1/pubticker/btcusd')
+            .reply(500);
+        return calculator.calculateRatesFor([{ from: 'BTC', to: 'USD' }]).then((result) => {
+            assert.equal(result[0], null);
+            const info = calculator.getLastRequestInfo();
+            assert.equal(info.calculator, calc2);
         });
     });
 });
diff --git a/test/FXService/FailoverProviderTest.ts b/test/FXService/FailoverProviderTest.ts
@@ -75,7 +75,7 @@ describe('FailoverProvider', () => {
         });
     });
 
-    it('returns spot rate from first provider is supported', () => {
+    it('returns spot rate from first provider if it is supported', () => {
         nock('https://api.gdax.com:443')
             .get('/products/BTC-USD/ticker')
             .reply(200, {
@@ -169,4 +169,18 @@ describe('FailoverProvider', () => {
             assert.equal(result.to, 'USD');
         });
     });
+
+    it('rejects promise if all providers are erroring', () => {
+        nock('https://api.gdax.com:443')
+            .get('/products/BTC-USD/ticker')
+            .reply(500);
+        nock('https://api.bitfinex.com:443')
+            .get('/v1/pubticker/btcusd')
+            .reply(500);
+        return provider.fetchCurrentRate({ from: 'BTC', to: 'USD' }).then((result: FXObject) => {
+            assert.fail(result, null, 'should reject promise');
+        }, (err: Error) => {
+            assert.ok(err instanceof EFXRateUnavailable);
+        });
+    });
 });