fix state conversion rate issue (#465)

* fix state conversion rate issue

* applying feedback

* revert tests

* adding test coverage

* adding comments, tweaking function signatures

* adjust empty string condition

src/apis/crypto-compare.test.ts

@@ -27,19 +27,6 @@ describe('CryptoCompare', () => {
     expect(conversionRate).toStrictEqual(2000.42);
   });
 
-  it('should return conversion date', async () => {
-    nock(cryptoCompareHost)
-      .get('/data/price?fsym=ETH&tsyms=CAD')
-      .reply(200, { CAD: 2000.42 });
-
-    const before = Date.now() / 1000;
-    const { conversionDate } = await fetchExchangeRate('CAD', 'ETH');
-    const after = Date.now() / 1000;
-
-    expect(conversionDate).toBeGreaterThanOrEqual(before);
-    expect(conversionDate).toBeLessThanOrEqual(after);
-  });
-
   it('should return CAD conversion rate given lower-cased currency', async () => {
     nock(cryptoCompareHost)
       .get('/data/price?fsym=ETH&tsyms=CAD')
@@ -152,4 +139,17 @@ describe('CryptoCompare', () => {
       'Invalid response for usdConversionRate: invalid',
     );
   });
+
+  it('should throw an error if either currency is invalid', async () => {
+    nock(cryptoCompareHost)
+      .get('/data/price?fsym=ETH&tsyms=EUABRT')
+      .reply(200, {
+        Response: 'Error',
+        Message: 'Market does not exist for this coin pair',
+      });
+
+    await expect(fetchExchangeRate('EUABRT', 'ETH')).rejects.toThrow(
+      'Market does not exist for this coin pair',
+    );
+  });
 });

src/apis/crypto-compare.ts

@@ -25,14 +25,27 @@ export async function fetchExchangeRate(
   nativeCurrency: string,
   includeUSDRate?: boolean,
 ): Promise<{
-  conversionDate: number;
   conversionRate: number;
   usdConversionRate: number;
 }> {
   const json = await handleFetch(
     getPricingURL(currency, nativeCurrency, includeUSDRate),
   );
+
+  /* 
+  Example expected error response (if pair is not found)
+  {
+    Response: "Error",
+    Message: "cccagg_or_exchange market does not exist for this coin pair (ETH-<NON_EXISTENT_TOKEN>)",
+    HasWarning: false,
+  }
+  */
+  if (json.Response === 'Error') {
+    throw new Error(json.Message);
+  }
+
   const conversionRate = Number(json[currency.toUpperCase()]);
+
   const usdConversionRate = Number(json.USD);
   if (!Number.isFinite(conversionRate)) {
     throw new Error(
@@ -46,7 +59,6 @@ export async function fetchExchangeRate(
   }
 
   return {
-    conversionDate: Date.now() / 1000,
     conversionRate,
     usdConversionRate,
   };

src/assets/CurrencyRateController.test.ts

@@ -37,9 +37,15 @@ function getRestrictedMessenger() {
   return messenger;
 }
 
+const getStubbedDate = () => {
+  return new Date('2019-04-07T10:20:30Z').getTime();
+};
+
 describe('CurrencyRateController', () => {
+  const realDate = Date.now;
   afterEach(() => {
     nock.cleanAll();
+    global.Date.now = realDate;
   });
 
   it('should set default state', () => {
@@ -245,4 +251,75 @@ describe('CurrencyRateController', () => {
 
     controller.destroy();
   });
+
+  it('should throw unexpected errors', async () => {
+    const cryptoCompareHost = 'https://min-api.cryptocompare.com';
+    nock(cryptoCompareHost)
+      .get('/data/price?fsym=ETH&tsyms=XYZ')
+      .reply(200, {
+        Response: 'Error',
+        Message: 'this method has been deprecated',
+      })
+      .persist();
+
+    const messenger = getRestrictedMessenger();
+    const controller = new CurrencyRateController({
+      messenger,
+      state: { currentCurrency: 'xyz' },
+    });
+
+    await expect(controller.updateExchangeRate()).rejects.toThrow(
+      'this method has been deprecated',
+    );
+    controller.destroy();
+  });
+
+  it('should catch expected errors', async () => {
+    const cryptoCompareHost = 'https://min-api.cryptocompare.com';
+    nock(cryptoCompareHost)
+      .get('/data/price?fsym=ETH&tsyms=XYZ')
+      .reply(200, {
+        Response: 'Error',
+        Message: 'market does not exist for this coin pair',
+      })
+      .persist();
+
+    const messenger = getRestrictedMessenger();
+    const controller = new CurrencyRateController({
+      messenger,
+      state: { currentCurrency: 'xyz' },
+    });
+
+    await controller.updateExchangeRate();
+    expect(controller.state.conversionRate).toBeNull();
+    controller.destroy();
+  });
+
+  it('should update conversionRates in state to null if either currentCurrency or nativeCurrency is null', async () => {
+    jest.spyOn(global.Date, 'now').mockImplementation(() => getStubbedDate());
+    const cryptoCompareHost = 'https://min-api.cryptocompare.com';
+    nock(cryptoCompareHost)
+      .get('/data/price?fsym=ETH&tsyms=XYZ')
+      .reply(200, { XYZ: 2000.42 })
+      .persist();
+
+    const messenger = getRestrictedMessenger();
+    const existingState = { currentCurrency: '', nativeCurrency: 'BNB' };
+    const controller = new CurrencyRateController({
+      messenger,
+      state: existingState,
+    });
+
+    await controller.updateExchangeRate();
+
+    expect(controller.state).toStrictEqual({
+      conversionDate: getStubbedDate() / 1000,
+      conversionRate: null,
+      currentCurrency: '',
+      nativeCurrency: 'BNB',
+      pendingCurrentCurrency: null,
+      pendingNativeCurrency: null,
+      usdConversionRate: null,
+    });
+  });
 });

src/assets/CurrencyRateController.ts

@@ -20,7 +20,7 @@ import type { RestrictedControllerMessenger } from '../ControllerMessenger';
  */
 export type CurrencyRateState = {
   conversionDate: number;
-  conversionRate: number;
+  conversionRate: number | null;
   currentCurrency: string;
   nativeCurrency: string;
   pendingCurrentCurrency: string | null;
@@ -190,35 +190,59 @@ export class CurrencyRateController extends BaseController<
   async updateExchangeRate(): Promise<CurrencyRateState | void> {
     const releaseLock = await this.mutex.acquire();
     const {
-      currentCurrency,
-      nativeCurrency,
+      currentCurrency: stateCurrentCurrency,
+      nativeCurrency: stateNativeCurrency,
       pendingCurrentCurrency,
       pendingNativeCurrency,
     } = this.state;
+
+    const conversionDate: number = Date.now() / 1000;
+    let conversionRate: number | null = null;
+    let usdConversionRate: number | null = null;
+    const currentCurrency = pendingCurrentCurrency ?? stateCurrentCurrency;
+    const nativeCurrency = pendingNativeCurrency ?? stateNativeCurrency;
+
     try {
-      const {
-        conversionDate,
-        conversionRate,
-        usdConversionRate,
-      } = await this.fetchExchangeRate(
-        pendingCurrentCurrency || currentCurrency,
-        pendingNativeCurrency || nativeCurrency,
-        this.includeUsdRate,
-      );
-      this.update(() => {
-        return {
-          conversionDate,
-          conversionRate,
-          currentCurrency: pendingCurrentCurrency || currentCurrency,
-          nativeCurrency: pendingNativeCurrency || nativeCurrency,
-          pendingCurrentCurrency: null,
-          pendingNativeCurrency: null,
-          usdConversionRate,
-        };
-      });
+      if (
+        currentCurrency &&
+        nativeCurrency &&
+        // if either currency is an empty string we can skip the comparison
+        // because it will result in an error from the api and ultimately
+        // a null conversionRate either way.
+        currentCurrency !== '' &&
+        nativeCurrency !== ''
+      ) {
+        ({ conversionRate, usdConversionRate } = await this.fetchExchangeRate(
+          currentCurrency,
+          nativeCurrency,
+          this.includeUsdRate,
+        ));
+      }
+    } catch (error) {
+      if (!error.message.includes('market does not exist for this coin pair')) {
+        throw error;
+      }
     } finally {
-      releaseLock();
+      try {
+        this.update(() => {
+          return {
+            conversionDate,
+            conversionRate,
+            // we currently allow and handle an empty string as a valid nativeCurrency
+            // in cases where a user has not entered a native ticker symbol for a custom network
+            // currentCurrency is not from user input but this protects us from unexpected changes.
+            nativeCurrency,
+            currentCurrency,
+            pendingCurrentCurrency: null,
+            pendingNativeCurrency: null,
+            usdConversionRate,
+          };
+        });
+      } finally {
+        releaseLock();
+      }
     }
+    return this.state;
   }
 }