From 14c1c78e88fedd242c8856c5293685ef02b6dfe5 Mon Sep 17 00:00:00 2001 From: Niranjana Binoy <43930900+NiranjanaBinoy@users.noreply.github.com> Date: Fri, 2 Dec 2022 13:24:18 -0500 Subject: [PATCH 01/14] updating disable option in token rates controller --- packages/assets-controllers/src/TokenRatesController.ts | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/packages/assets-controllers/src/TokenRatesController.ts b/packages/assets-controllers/src/TokenRatesController.ts index 33048873c06..5f3cbe14c89 100644 --- a/packages/assets-controllers/src/TokenRatesController.ts +++ b/packages/assets-controllers/src/TokenRatesController.ts @@ -191,7 +191,7 @@ export class TokenRatesController extends BaseController< ) { super(config, state); this.defaultConfig = { - disabled: true, + disabled: false, interval: 3 * 60 * 1000, nativeCurrency: 'eth', chainId: '', @@ -203,7 +203,9 @@ export class TokenRatesController extends BaseController< contractExchangeRates: {}, }; this.initialize(); - this.configure({ disabled: false }, false, false); + if(config?.disabled === true){ + this.configure({ disabled: true }, false, false); + } onTokensStateChange(({ tokens, detectedTokens }) => { this.configure({ tokens: [...tokens, ...detectedTokens] }); }); From 3c62c4197db8086107bf3a8c430a04fd282140b3 Mon Sep 17 00:00:00 2001 From: Dan J Miller Date: Fri, 2 Dec 2022 18:01:20 -0330 Subject: [PATCH 02/14] Allow network requests from instances of the currencyRateController to be enabled or not --- .../src/CurrencyRateController.test.ts | 38 +++++++++++++++++++ .../src/CurrencyRateController.ts | 9 +++++ 2 files changed, 47 insertions(+) diff --git a/packages/assets-controllers/src/CurrencyRateController.test.ts b/packages/assets-controllers/src/CurrencyRateController.test.ts index 0e744d32f4d..d77673890b4 100644 --- a/packages/assets-controllers/src/CurrencyRateController.test.ts +++ b/packages/assets-controllers/src/CurrencyRateController.test.ts @@ -277,6 +277,44 @@ describe('CurrencyRateController', () => { controller.destroy(); }); + it('should fetch exchange rates after calling setNativeCurrency if start has been called', async () => { + const fetchExchangeRateStub = sinon.stub().resolves({}); + const messenger = getRestrictedMessenger(); + const controller = new CurrencyRateController({ + includeUsdRate: true, + fetchExchangeRate: fetchExchangeRateStub, + messenger, + }); + + await controller.setNativeCurrency('XYZ'); + + expect( + fetchExchangeRateStub.alwaysCalledWithExactly('usd', 'XYZ', true), + ).toBe(true); + + controller.destroy(); + }); + + it('should NOT fetch exchange rates after calling setNativeCurrency if stop has been called', async () => { + const fetchExchangeRateStub = sinon.stub().resolves({}); + const messenger = getRestrictedMessenger(); + const controller = new CurrencyRateController({ + includeUsdRate: true, + fetchExchangeRate: fetchExchangeRateStub, + messenger, + }); + controller.stop(); + + await controller.setNativeCurrency('XYZ'); + + expect( + fetchExchangeRateStub.notCalled, + ).toBe(true); + + controller.destroy(); + }); + + it('should throw unexpected errors', async () => { const cryptoCompareHost = 'https://min-api.cryptocompare.com'; nock(cryptoCompareHost) diff --git a/packages/assets-controllers/src/CurrencyRateController.ts b/packages/assets-controllers/src/CurrencyRateController.ts index 3bb8d4a31b6..1aa900b0ca1 100644 --- a/packages/assets-controllers/src/CurrencyRateController.ts +++ b/packages/assets-controllers/src/CurrencyRateController.ts @@ -90,6 +90,8 @@ export class CurrencyRateController extends BaseControllerV2< private includeUsdRate; + #enabled; + /** * Creates a CurrencyRateController instance. * @@ -99,6 +101,7 @@ export class CurrencyRateController extends BaseControllerV2< * @param options.messenger - A reference to the messaging system. * @param options.state - Initial state to set on this controller. * @param options.fetchExchangeRate - Fetches the exchange rate from an external API. This option is primarily meant for use in unit tests. + * @param options.enabled - A boolean that controls whether or not network requests can be made by the controller. */ constructor({ includeUsdRate = false, @@ -122,12 +125,14 @@ export class CurrencyRateController extends BaseControllerV2< this.includeUsdRate = includeUsdRate; this.intervalDelay = interval; this.fetchExchangeRate = fetchExchangeRate; + this.#enabled = true; } /** * Start polling for the currency rate. */ async start() { + this.#enabled = true; await this.startPolling(); } @@ -135,6 +140,7 @@ export class CurrencyRateController extends BaseControllerV2< * Stop polling for the currency rate. */ stop() { + this.#enabled = false; this.stopPolling(); } @@ -196,6 +202,9 @@ export class CurrencyRateController extends BaseControllerV2< * @returns The controller state. */ async updateExchangeRate(): Promise { + if (!this.#enabled) { + return; + } const releaseLock = await this.mutex.acquire(); const { currentCurrency: stateCurrentCurrency, From 427b82fceb9dcdf9d297ece5b6223e5f992506fb Mon Sep 17 00:00:00 2001 From: Dan J Miller Date: Fri, 2 Dec 2022 18:12:14 -0330 Subject: [PATCH 03/14] Lint fix --- .../assets-controllers/src/CurrencyRateController.test.ts | 5 +---- packages/assets-controllers/src/CurrencyRateController.ts | 5 ++--- packages/assets-controllers/src/TokenRatesController.ts | 3 ++- 3 files changed, 5 insertions(+), 8 deletions(-) diff --git a/packages/assets-controllers/src/CurrencyRateController.test.ts b/packages/assets-controllers/src/CurrencyRateController.test.ts index d77673890b4..6a66e8d8d51 100644 --- a/packages/assets-controllers/src/CurrencyRateController.test.ts +++ b/packages/assets-controllers/src/CurrencyRateController.test.ts @@ -307,14 +307,11 @@ describe('CurrencyRateController', () => { await controller.setNativeCurrency('XYZ'); - expect( - fetchExchangeRateStub.notCalled, - ).toBe(true); + expect(fetchExchangeRateStub.notCalled).toBe(true); controller.destroy(); }); - it('should throw unexpected errors', async () => { const cryptoCompareHost = 'https://min-api.cryptocompare.com'; nock(cryptoCompareHost) diff --git a/packages/assets-controllers/src/CurrencyRateController.ts b/packages/assets-controllers/src/CurrencyRateController.ts index 1aa900b0ca1..18ae68bef84 100644 --- a/packages/assets-controllers/src/CurrencyRateController.ts +++ b/packages/assets-controllers/src/CurrencyRateController.ts @@ -90,7 +90,7 @@ export class CurrencyRateController extends BaseControllerV2< private includeUsdRate; - #enabled; + #enabled; // A boolean that controls whether or not network requests can be made by the controller. /** * Creates a CurrencyRateController instance. @@ -101,7 +101,6 @@ export class CurrencyRateController extends BaseControllerV2< * @param options.messenger - A reference to the messaging system. * @param options.state - Initial state to set on this controller. * @param options.fetchExchangeRate - Fetches the exchange rate from an external API. This option is primarily meant for use in unit tests. - * @param options.enabled - A boolean that controls whether or not network requests can be made by the controller. */ constructor({ includeUsdRate = false, @@ -203,7 +202,7 @@ export class CurrencyRateController extends BaseControllerV2< */ async updateExchangeRate(): Promise { if (!this.#enabled) { - return; + return null; } const releaseLock = await this.mutex.acquire(); const { diff --git a/packages/assets-controllers/src/TokenRatesController.ts b/packages/assets-controllers/src/TokenRatesController.ts index 5f3cbe14c89..36dbc3cca2b 100644 --- a/packages/assets-controllers/src/TokenRatesController.ts +++ b/packages/assets-controllers/src/TokenRatesController.ts @@ -203,9 +203,10 @@ export class TokenRatesController extends BaseController< contractExchangeRates: {}, }; this.initialize(); - if(config?.disabled === true){ + if (config?.disabled === true) { this.configure({ disabled: true }, false, false); } + onTokensStateChange(({ tokens, detectedTokens }) => { this.configure({ tokens: [...tokens, ...detectedTokens] }); }); From 88c2d566529ce448121bd24f1b87673243f220a4 Mon Sep 17 00:00:00 2001 From: Dan J Miller Date: Fri, 2 Dec 2022 20:53:16 -0330 Subject: [PATCH 04/14] Update packages/assets-controllers/src/CurrencyRateController.test.ts Co-authored-by: Elliot Winkler --- packages/assets-controllers/src/CurrencyRateController.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/assets-controllers/src/CurrencyRateController.test.ts b/packages/assets-controllers/src/CurrencyRateController.test.ts index 6a66e8d8d51..0b4f28d0f75 100644 --- a/packages/assets-controllers/src/CurrencyRateController.test.ts +++ b/packages/assets-controllers/src/CurrencyRateController.test.ts @@ -277,7 +277,7 @@ describe('CurrencyRateController', () => { controller.destroy(); }); - it('should fetch exchange rates after calling setNativeCurrency if start has been called', async () => { + it('should fetch exchange rates after calling setNativeCurrency by default', async () => { const fetchExchangeRateStub = sinon.stub().resolves({}); const messenger = getRestrictedMessenger(); const controller = new CurrencyRateController({ From d11621405485019e1ca1339e9a77a320f46aa251 Mon Sep 17 00:00:00 2001 From: Dan J Miller Date: Fri, 2 Dec 2022 20:53:45 -0330 Subject: [PATCH 05/14] Update packages/assets-controllers/src/TokenRatesController.ts Co-authored-by: Elliot Winkler --- packages/assets-controllers/src/TokenRatesController.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/assets-controllers/src/TokenRatesController.ts b/packages/assets-controllers/src/TokenRatesController.ts index 36dbc3cca2b..90f1bce112f 100644 --- a/packages/assets-controllers/src/TokenRatesController.ts +++ b/packages/assets-controllers/src/TokenRatesController.ts @@ -203,7 +203,7 @@ export class TokenRatesController extends BaseController< contractExchangeRates: {}, }; this.initialize(); - if (config?.disabled === true) { + if (config?.disabled) { this.configure({ disabled: true }, false, false); } From 3723d5b76d6c8de7e3c7baac3d8c85c7405995c3 Mon Sep 17 00:00:00 2001 From: Dan Miller Date: Fri, 2 Dec 2022 21:14:38 -0330 Subject: [PATCH 06/14] Set enabled to true by default --- .../src/CurrencyRateController.test.ts | 19 ++++++++++++------- .../src/CurrencyRateController.ts | 4 ++-- 2 files changed, 14 insertions(+), 9 deletions(-) diff --git a/packages/assets-controllers/src/CurrencyRateController.test.ts b/packages/assets-controllers/src/CurrencyRateController.test.ts index 0b4f28d0f75..de1b9f7a2ef 100644 --- a/packages/assets-controllers/src/CurrencyRateController.test.ts +++ b/packages/assets-controllers/src/CurrencyRateController.test.ts @@ -169,6 +169,7 @@ describe('CurrencyRateController', () => { fetchExchangeRate: fetchExchangeRateStub, messenger, }); + controller.start(); expect(controller.state.conversionRate).toStrictEqual(0); await controller.updateExchangeRate(); expect(controller.state.conversionRate).toStrictEqual(10); @@ -198,7 +199,7 @@ describe('CurrencyRateController', () => { fetchExchangeRate: fetchExchangeRateStub, messenger, }); - + controller.start(); await controller.setNativeCurrency('DAI'); await controller.updateExchangeRate(); expect(controller.state.conversionRate).toStrictEqual(1); @@ -217,6 +218,7 @@ describe('CurrencyRateController', () => { fetchExchangeRate: fetchExchangeRateStub, messenger, }); + controller.start(); expect(controller.state.conversionRate).toStrictEqual(0); await controller.setCurrentCurrency('CAD'); expect(controller.state.conversionRate).toStrictEqual(10); @@ -232,6 +234,7 @@ describe('CurrencyRateController', () => { fetchExchangeRate: fetchExchangeRateStub, messenger, }); + controller.start(); expect(controller.state.conversionRate).toStrictEqual(0); await controller.setNativeCurrency('xDAI'); expect(controller.state.conversionRate).toStrictEqual(10); @@ -248,7 +251,7 @@ describe('CurrencyRateController', () => { messenger, state: { currentCurrency: 'xyz' }, }); - + controller.start(); await controller.updateExchangeRate(); expect( @@ -269,7 +272,7 @@ describe('CurrencyRateController', () => { messenger, state: { currentCurrency: 'xyz' }, }); - + controller.start(); await controller.updateExchangeRate(); expect(controller.state.conversionRate).toStrictEqual(2000.42); @@ -286,6 +289,7 @@ describe('CurrencyRateController', () => { messenger, }); + controller.start(); await controller.setNativeCurrency('XYZ'); expect( @@ -303,8 +307,9 @@ describe('CurrencyRateController', () => { fetchExchangeRate: fetchExchangeRateStub, messenger, }); + controller.start(); controller.stop(); - + fetchExchangeRateStub.resetHistory(); await controller.setNativeCurrency('XYZ'); expect(fetchExchangeRateStub.notCalled).toBe(true); @@ -327,7 +332,7 @@ describe('CurrencyRateController', () => { messenger, state: { currentCurrency: 'xyz' }, }); - + controller.start(); await expect(controller.updateExchangeRate()).rejects.toThrow( 'this method has been deprecated', ); @@ -349,7 +354,7 @@ describe('CurrencyRateController', () => { messenger, state: { currentCurrency: 'xyz' }, }); - + controller.start(); await controller.updateExchangeRate(); expect(controller.state.conversionRate).toBeNull(); controller.destroy(); @@ -369,7 +374,7 @@ describe('CurrencyRateController', () => { messenger, state: existingState, }); - + controller.start(); await controller.updateExchangeRate(); expect(controller.state).toStrictEqual({ diff --git a/packages/assets-controllers/src/CurrencyRateController.ts b/packages/assets-controllers/src/CurrencyRateController.ts index 18ae68bef84..76268394220 100644 --- a/packages/assets-controllers/src/CurrencyRateController.ts +++ b/packages/assets-controllers/src/CurrencyRateController.ts @@ -124,7 +124,7 @@ export class CurrencyRateController extends BaseControllerV2< this.includeUsdRate = includeUsdRate; this.intervalDelay = interval; this.fetchExchangeRate = fetchExchangeRate; - this.#enabled = true; + this.#enabled = false; } /** @@ -202,7 +202,7 @@ export class CurrencyRateController extends BaseControllerV2< */ async updateExchangeRate(): Promise { if (!this.#enabled) { - return null; + return this.state; } const releaseLock = await this.mutex.acquire(); const { From 60d11f48984104005b45d9c5c3d7d2fb837d6f67 Mon Sep 17 00:00:00 2001 From: Dan J Miller Date: Mon, 5 Dec 2022 07:41:21 -0330 Subject: [PATCH 07/14] Update packages/assets-controllers/src/CurrencyRateController.test.ts Co-authored-by: legobeat <109787230+legobeat@users.noreply.github.com> --- .../src/CurrencyRateController.test.ts | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/packages/assets-controllers/src/CurrencyRateController.test.ts b/packages/assets-controllers/src/CurrencyRateController.test.ts index de1b9f7a2ef..c96faa285fe 100644 --- a/packages/assets-controllers/src/CurrencyRateController.test.ts +++ b/packages/assets-controllers/src/CurrencyRateController.test.ts @@ -298,7 +298,21 @@ describe('CurrencyRateController', () => { controller.destroy(); }); + it('should NOT fetch exchange rates after calling setNativeCurrency if start has not been called', async () => { + const fetchExchangeRateStub = sinon.stub().resolves({}); + const messenger = getRestrictedMessenger(); + const controller = new CurrencyRateController({ + includeUsdRate: true, + fetchExchangeRate: fetchExchangeRateStub, + messenger, + }); + fetchExchangeRateStub.resetHistory(); + await controller.setNativeCurrency('XYZ'); + + expect(fetchExchangeRateStub.notCalled).toBe(true); + controller.destroy(); + }); it('should NOT fetch exchange rates after calling setNativeCurrency if stop has been called', async () => { const fetchExchangeRateStub = sinon.stub().resolves({}); const messenger = getRestrictedMessenger(); From 3536d03a9c121e5d6757ddfea25a3fb8dd6f96e3 Mon Sep 17 00:00:00 2001 From: Pedro Figueiredo Date: Mon, 5 Dec 2022 11:24:01 +0000 Subject: [PATCH 08/14] fix linting --- .../src/CurrencyRateController.test.ts | 151 ++++++++++++------ .../src/CurrencyRateController.ts | 15 +- 2 files changed, 116 insertions(+), 50 deletions(-) diff --git a/packages/assets-controllers/src/CurrencyRateController.test.ts b/packages/assets-controllers/src/CurrencyRateController.test.ts index c96faa285fe..6af97838963 100644 --- a/packages/assets-controllers/src/CurrencyRateController.test.ts +++ b/packages/assets-controllers/src/CurrencyRateController.test.ts @@ -34,8 +34,23 @@ const getStubbedDate = () => { return new Date('2019-04-07T10:20:30Z').getTime(); }; +/** + * Resolve all pending promises. + * This method is used for async tests that use fake timers. + * See https://stackoverflow.com/a/58716087 and https://jestjs.io/docs/timer-mocks. + */ +function flushPromises(): Promise { + return new Promise(jest.requireActual('timers').setImmediate); +} + describe('CurrencyRateController', () => { + beforeEach(() => { + jest.useFakeTimers(); + }); + afterEach(() => { + jest.useRealTimers(); + nock.cleanAll(); sinon.restore(); }); @@ -91,33 +106,40 @@ describe('CurrencyRateController', () => { messenger, }); - await new Promise((resolve) => setTimeout(() => resolve(), 150)); + jest.advanceTimersByTime(200); + await flushPromises(); + expect(fetchExchangeRateStub.called).toBe(false); controller.destroy(); }); it('should poll and update rate in the right interval', async () => { - const fetchExchangeRateStub = sinon.stub(); + const fetchExchangeRateStub = jest.fn(); const messenger = getRestrictedMessenger(); const controller = new CurrencyRateController({ interval: 100, fetchExchangeRate: fetchExchangeRateStub, messenger, }); + await controller.start(); - await new Promise((resolve) => setTimeout(() => resolve(), 1)); - expect(fetchExchangeRateStub.called).toBe(true); - expect(fetchExchangeRateStub.calledTwice).toBe(false); - await new Promise((resolve) => setTimeout(() => resolve(), 150)); - expect(fetchExchangeRateStub.calledTwice).toBe(true); + jest.advanceTimersByTime(99); + await flushPromises(); + + expect(fetchExchangeRateStub).toHaveBeenCalledTimes(1); + + jest.advanceTimersByTime(1); + await flushPromises(); + + expect(fetchExchangeRateStub).toHaveBeenCalledTimes(2); controller.destroy(); }); it('should not poll after being stopped', async () => { - const fetchExchangeRateStub = sinon.stub(); + const fetchExchangeRateStub = jest.fn(); const messenger = getRestrictedMessenger(); const controller = new CurrencyRateController({ interval: 100, @@ -128,17 +150,19 @@ describe('CurrencyRateController', () => { controller.stop(); // called once upon initial start - expect(fetchExchangeRateStub.called).toBe(true); - expect(fetchExchangeRateStub.calledTwice).toBe(false); + expect(fetchExchangeRateStub).toHaveBeenCalledTimes(1); + + jest.advanceTimersByTime(150); + await flushPromises(); - await new Promise((resolve) => setTimeout(() => resolve(), 150)); - expect(fetchExchangeRateStub.calledTwice).toBe(false); + expect(fetchExchangeRateStub).toHaveBeenCalledTimes(1); controller.destroy(); }); it('should poll correctly after being started, stopped, and started again', async () => { - const fetchExchangeRateStub = sinon.stub(); + const fetchExchangeRateStub = jest.fn(); + const messenger = getRestrictedMessenger(); const controller = new CurrencyRateController({ interval: 100, @@ -149,16 +173,19 @@ describe('CurrencyRateController', () => { controller.stop(); // called once upon initial start - expect(fetchExchangeRateStub.called).toBe(true); - expect(fetchExchangeRateStub.calledTwice).toBe(false); + expect(fetchExchangeRateStub).toHaveBeenCalledTimes(1); - controller.start(); + await controller.start(); - await new Promise((resolve) => setTimeout(() => resolve(), 1)); - expect(fetchExchangeRateStub.calledTwice).toBe(true); - expect(fetchExchangeRateStub.calledThrice).toBe(false); - await new Promise((resolve) => setTimeout(() => resolve(), 150)); - expect(fetchExchangeRateStub.calledThrice).toBe(true); + jest.advanceTimersByTime(1); + await flushPromises(); + + expect(fetchExchangeRateStub).toHaveBeenCalledTimes(2); + + jest.advanceTimersByTime(99); + await flushPromises(); + + expect(fetchExchangeRateStub).toHaveBeenCalledTimes(3); }); it('should update exchange rate', async () => { @@ -169,9 +196,11 @@ describe('CurrencyRateController', () => { fetchExchangeRate: fetchExchangeRateStub, messenger, }); - controller.start(); + expect(controller.state.conversionRate).toStrictEqual(0); - await controller.updateExchangeRate(); + + await controller.start(); + expect(controller.state.conversionRate).toStrictEqual(10); controller.destroy(); @@ -199,12 +228,16 @@ describe('CurrencyRateController', () => { fetchExchangeRate: fetchExchangeRateStub, messenger, }); - controller.start(); + + expect(controller.state.conversionRate).toStrictEqual(0); + + await controller.start(); await controller.setNativeCurrency('DAI'); - await controller.updateExchangeRate(); + expect(controller.state.conversionRate).toStrictEqual(1); - await controller.updateExchangeRate(); + await controller.setNativeCurrency(TESTNET_TICKER_SYMBOLS.RINKEBY); + expect(controller.state.conversionRate).toStrictEqual(10); controller.destroy(); @@ -218,10 +251,16 @@ describe('CurrencyRateController', () => { fetchExchangeRate: fetchExchangeRateStub, messenger, }); - controller.start(); - expect(controller.state.conversionRate).toStrictEqual(0); + + expect(controller.state.currentCurrency).toStrictEqual('usd'); + + await controller.start(); + + expect(controller.state.currentCurrency).toStrictEqual('usd'); + await controller.setCurrentCurrency('CAD'); - expect(controller.state.conversionRate).toStrictEqual(10); + + expect(controller.state.currentCurrency).toStrictEqual('CAD'); controller.destroy(); }); @@ -234,10 +273,16 @@ describe('CurrencyRateController', () => { fetchExchangeRate: fetchExchangeRateStub, messenger, }); - controller.start(); - expect(controller.state.conversionRate).toStrictEqual(0); + + expect(controller.state.nativeCurrency).toStrictEqual('ETH'); + + await controller.start(); + + expect(controller.state.nativeCurrency).toStrictEqual('ETH'); + await controller.setNativeCurrency('xDAI'); - expect(controller.state.conversionRate).toStrictEqual(10); + + expect(controller.state.nativeCurrency).toStrictEqual('xDAI'); controller.destroy(); }); @@ -251,8 +296,7 @@ describe('CurrencyRateController', () => { messenger, state: { currentCurrency: 'xyz' }, }); - controller.start(); - await controller.updateExchangeRate(); + await controller.start(); expect( fetchExchangeRateStub.alwaysCalledWithExactly('xyz', 'ETH', true), @@ -272,8 +316,7 @@ describe('CurrencyRateController', () => { messenger, state: { currentCurrency: 'xyz' }, }); - controller.start(); - await controller.updateExchangeRate(); + await controller.start(); expect(controller.state.conversionRate).toStrictEqual(2000.42); @@ -289,15 +332,25 @@ describe('CurrencyRateController', () => { messenger, }); - controller.start(); + await controller.start(); await controller.setNativeCurrency('XYZ'); - expect( - fetchExchangeRateStub.alwaysCalledWithExactly('usd', 'XYZ', true), - ).toBe(true); + expect(fetchExchangeRateStub.calledTwice).toBe(true); + expect(fetchExchangeRateStub.getCall(0).args).toStrictEqual([ + 'usd', + 'ETH', + true, + ]); + + expect(fetchExchangeRateStub.getCall(1).args).toStrictEqual([ + 'usd', + 'XYZ', + true, + ]); controller.destroy(); }); + it('should NOT fetch exchange rates after calling setNativeCurrency if start has not been called', async () => { const fetchExchangeRateStub = sinon.stub().resolves({}); const messenger = getRestrictedMessenger(); @@ -313,6 +366,7 @@ describe('CurrencyRateController', () => { controller.destroy(); }); + it('should NOT fetch exchange rates after calling setNativeCurrency if stop has been called', async () => { const fetchExchangeRateStub = sinon.stub().resolves({}); const messenger = getRestrictedMessenger(); @@ -321,7 +375,7 @@ describe('CurrencyRateController', () => { fetchExchangeRate: fetchExchangeRateStub, messenger, }); - controller.start(); + await controller.start(); controller.stop(); fetchExchangeRateStub.resetHistory(); await controller.setNativeCurrency('XYZ'); @@ -346,10 +400,13 @@ describe('CurrencyRateController', () => { messenger, state: { currentCurrency: 'xyz' }, }); - controller.start(); + + await controller.start(); + await expect(controller.updateExchangeRate()).rejects.toThrow( 'this method has been deprecated', ); + controller.destroy(); }); @@ -368,9 +425,11 @@ describe('CurrencyRateController', () => { messenger, state: { currentCurrency: 'xyz' }, }); - controller.start(); - await controller.updateExchangeRate(); + + await controller.start(); + expect(controller.state.conversionRate).toBeNull(); + controller.destroy(); }); @@ -388,8 +447,8 @@ describe('CurrencyRateController', () => { messenger, state: existingState, }); - controller.start(); - await controller.updateExchangeRate(); + + await controller.start(); expect(controller.state).toStrictEqual({ conversionDate: null, diff --git a/packages/assets-controllers/src/CurrencyRateController.ts b/packages/assets-controllers/src/CurrencyRateController.ts index 76268394220..2608d17f1ba 100644 --- a/packages/assets-controllers/src/CurrencyRateController.ts +++ b/packages/assets-controllers/src/CurrencyRateController.ts @@ -132,6 +132,7 @@ export class CurrencyRateController extends BaseControllerV2< */ async start() { this.#enabled = true; + await this.startPolling(); } @@ -140,6 +141,7 @@ export class CurrencyRateController extends BaseControllerV2< */ stop() { this.#enabled = false; + this.stopPolling(); } @@ -189,9 +191,11 @@ export class CurrencyRateController extends BaseControllerV2< private async startPolling(): Promise { this.stopPolling(); // TODO: Expose polling currency rate update errors - await safelyExecute(() => this.updateExchangeRate()); + + await safelyExecute(async () => await this.updateExchangeRate()); + this.intervalId = setInterval(async () => { - await safelyExecute(() => this.updateExchangeRate()); + await safelyExecute(async () => await this.updateExchangeRate()); }, this.intervalDelay); } @@ -235,11 +239,14 @@ export class CurrencyRateController extends BaseControllerV2< currentCurrency !== '' && nativeCurrency !== '' ) { - ({ conversionRate, usdConversionRate } = await this.fetchExchangeRate( + const fetchExchangeRateResponse = await this.fetchExchangeRate( currentCurrency, nativeCurrencyForExchangeRate, this.includeUsdRate, - )); + ); + + conversionRate = fetchExchangeRateResponse.conversionRate; + usdConversionRate = fetchExchangeRateResponse.usdConversionRate; conversionDate = Date.now() / 1000; } } catch (error) { From 42ba38e3389bf6fe0e486e292cb2ef95424f203f Mon Sep 17 00:00:00 2001 From: Pedro Figueiredo Date: Mon, 5 Dec 2022 16:06:24 +0000 Subject: [PATCH 09/14] fix tests --- .../src/CurrencyRateController.test.ts | 76 ++++++++++--------- 1 file changed, 40 insertions(+), 36 deletions(-) diff --git a/packages/assets-controllers/src/CurrencyRateController.test.ts b/packages/assets-controllers/src/CurrencyRateController.test.ts index 6af97838963..55271c5219d 100644 --- a/packages/assets-controllers/src/CurrencyRateController.test.ts +++ b/packages/assets-controllers/src/CurrencyRateController.test.ts @@ -1,4 +1,3 @@ -import * as sinon from 'sinon'; import nock from 'nock'; import { ControllerMessenger } from '@metamask/base-controller'; import { TESTNET_TICKER_SYMBOLS } from '@metamask/controller-utils'; @@ -52,16 +51,12 @@ describe('CurrencyRateController', () => { jest.useRealTimers(); nock.cleanAll(); - sinon.restore(); }); it('should set default state', () => { - const fetchExchangeRateStub = sinon.stub(); const messenger = getRestrictedMessenger(); - const controller = new CurrencyRateController({ - fetchExchangeRate: fetchExchangeRateStub, - messenger, - }); + const controller = new CurrencyRateController({ messenger }); + expect(controller.state).toStrictEqual({ conversionDate: 0, conversionRate: 0, @@ -76,14 +71,13 @@ describe('CurrencyRateController', () => { }); it('should initialize with initial state', () => { - const fetchExchangeRateStub = sinon.stub(); const messenger = getRestrictedMessenger(); const existingState = { currentCurrency: 'rep' }; const controller = new CurrencyRateController({ - fetchExchangeRate: fetchExchangeRateStub, messenger, state: existingState, }); + expect(controller.state).toStrictEqual({ conversionDate: 0, conversionRate: 0, @@ -98,7 +92,7 @@ describe('CurrencyRateController', () => { }); it('should not poll before being started', async () => { - const fetchExchangeRateStub = sinon.stub(); + const fetchExchangeRateStub = jest.fn(); const messenger = getRestrictedMessenger(); const controller = new CurrencyRateController({ interval: 100, @@ -109,7 +103,7 @@ describe('CurrencyRateController', () => { jest.advanceTimersByTime(200); await flushPromises(); - expect(fetchExchangeRateStub.called).toBe(false); + expect(fetchExchangeRateStub).not.toHaveBeenCalled(); controller.destroy(); }); @@ -146,6 +140,7 @@ describe('CurrencyRateController', () => { fetchExchangeRate: fetchExchangeRateStub, messenger, }); + await controller.start(); controller.stop(); @@ -189,7 +184,9 @@ describe('CurrencyRateController', () => { }); it('should update exchange rate', async () => { - const fetchExchangeRateStub = sinon.stub().resolves({ conversionRate: 10 }); + const fetchExchangeRateStub = jest + .fn() + .mockResolvedValue({ conversionRate: 10 }); const messenger = getRestrictedMessenger(); const controller = new CurrencyRateController({ interval: 10, @@ -244,7 +241,9 @@ describe('CurrencyRateController', () => { }); it('should update current currency', async () => { - const fetchExchangeRateStub = sinon.stub().resolves({ conversionRate: 10 }); + const fetchExchangeRateStub = jest + .fn() + .mockResolvedValue({ conversionRate: 10 }); const messenger = getRestrictedMessenger(); const controller = new CurrencyRateController({ interval: 10, @@ -266,7 +265,9 @@ describe('CurrencyRateController', () => { }); it('should update native currency', async () => { - const fetchExchangeRateStub = sinon.stub().resolves({ conversionRate: 10 }); + const fetchExchangeRateStub = jest + .fn() + .mockResolvedValue({ conversionRate: 10 }); const messenger = getRestrictedMessenger(); const controller = new CurrencyRateController({ interval: 10, @@ -288,7 +289,7 @@ describe('CurrencyRateController', () => { }); it('should add usd rate to state when includeUsdRate is configured true', async () => { - const fetchExchangeRateStub = sinon.stub().resolves({}); + const fetchExchangeRateStub = jest.fn().mockResolvedValue({}); const messenger = getRestrictedMessenger(); const controller = new CurrencyRateController({ includeUsdRate: true, @@ -298,9 +299,10 @@ describe('CurrencyRateController', () => { }); await controller.start(); - expect( - fetchExchangeRateStub.alwaysCalledWithExactly('xyz', 'ETH', true), - ).toBe(true); + expect(fetchExchangeRateStub).toHaveBeenCalledTimes(1); + expect(fetchExchangeRateStub.mock.calls).toMatchObject([ + ['xyz', 'ETH', true], + ]); controller.destroy(); }); @@ -324,7 +326,7 @@ describe('CurrencyRateController', () => { }); it('should fetch exchange rates after calling setNativeCurrency by default', async () => { - const fetchExchangeRateStub = sinon.stub().resolves({}); + const fetchExchangeRateStub = jest.fn().mockResolvedValue({}); const messenger = getRestrictedMessenger(); const controller = new CurrencyRateController({ includeUsdRate: true, @@ -333,54 +335,56 @@ describe('CurrencyRateController', () => { }); await controller.start(); - await controller.setNativeCurrency('XYZ'); - expect(fetchExchangeRateStub.calledTwice).toBe(true); - expect(fetchExchangeRateStub.getCall(0).args).toStrictEqual([ - 'usd', - 'ETH', - true, - ]); + expect(fetchExchangeRateStub).toHaveBeenCalledTimes(1); - expect(fetchExchangeRateStub.getCall(1).args).toStrictEqual([ - 'usd', - 'XYZ', - true, + await controller.setNativeCurrency('XYZ'); + + expect(fetchExchangeRateStub).toHaveBeenCalledTimes(2); + expect(fetchExchangeRateStub.mock.calls).toMatchObject([ + ['usd', 'ETH', true], + ['usd', 'XYZ', true], ]); controller.destroy(); }); it('should NOT fetch exchange rates after calling setNativeCurrency if start has not been called', async () => { - const fetchExchangeRateStub = sinon.stub().resolves({}); + const fetchExchangeRateStub = jest.fn().mockResolvedValue({}); + const messenger = getRestrictedMessenger(); const controller = new CurrencyRateController({ includeUsdRate: true, fetchExchangeRate: fetchExchangeRateStub, messenger, }); - fetchExchangeRateStub.resetHistory(); + await controller.setNativeCurrency('XYZ'); - expect(fetchExchangeRateStub.notCalled).toBe(true); + expect(fetchExchangeRateStub).toHaveBeenCalledTimes(0); controller.destroy(); }); it('should NOT fetch exchange rates after calling setNativeCurrency if stop has been called', async () => { - const fetchExchangeRateStub = sinon.stub().resolves({}); + const fetchExchangeRateStub = jest.fn().mockResolvedValue({}); + const messenger = getRestrictedMessenger(); const controller = new CurrencyRateController({ includeUsdRate: true, fetchExchangeRate: fetchExchangeRateStub, messenger, }); + expect(fetchExchangeRateStub).toHaveBeenCalledTimes(0); + await controller.start(); controller.stop(); - fetchExchangeRateStub.resetHistory(); + + expect(fetchExchangeRateStub).toHaveBeenCalledTimes(1); + await controller.setNativeCurrency('XYZ'); - expect(fetchExchangeRateStub.notCalled).toBe(true); + expect(fetchExchangeRateStub).toHaveBeenCalledTimes(1); controller.destroy(); }); From 68b0a4b51dea596e4eac18d2e16d0d0375154ab4 Mon Sep 17 00:00:00 2001 From: Niranjana Binoy <43930900+NiranjanaBinoy@users.noreply.github.com> Date: Mon, 5 Dec 2022 12:33:13 -0500 Subject: [PATCH 10/14] Update packages/assets-controllers/src/CurrencyRateController.test.ts - test name as per review comment --- packages/assets-controllers/src/CurrencyRateController.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/assets-controllers/src/CurrencyRateController.test.ts b/packages/assets-controllers/src/CurrencyRateController.test.ts index 55271c5219d..a201120272e 100644 --- a/packages/assets-controllers/src/CurrencyRateController.test.ts +++ b/packages/assets-controllers/src/CurrencyRateController.test.ts @@ -325,7 +325,7 @@ describe('CurrencyRateController', () => { controller.destroy(); }); - it('should fetch exchange rates after calling setNativeCurrency by default', async () => { + it('should fetch exchange rates after starting and again after calling setNativeCurrency', async () => { const fetchExchangeRateStub = jest.fn().mockResolvedValue({}); const messenger = getRestrictedMessenger(); const controller = new CurrencyRateController({ From 5b20d55400c668af4a94f5046cc83336c31915a3 Mon Sep 17 00:00:00 2001 From: Pedro Figueiredo Date: Mon, 5 Dec 2022 19:21:43 +0000 Subject: [PATCH 11/14] adjust thresholds --- packages/assets-controllers/jest.config.js | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/packages/assets-controllers/jest.config.js b/packages/assets-controllers/jest.config.js index cb53c822520..215aad04a06 100644 --- a/packages/assets-controllers/jest.config.js +++ b/packages/assets-controllers/jest.config.js @@ -16,10 +16,10 @@ module.exports = merge(baseConfig, { // An object that configures minimum threshold enforcement for coverage results coverageThreshold: { global: { - branches: 88.66, - functions: 96.45, - lines: 96.55, - statements: 96.63, + branches: 87.77, + functions: 96.15, + lines: 95.28, + statements: 95.4, }, }, From cc1e8a3e0ba4dcc49fdb906115a1555c8f1cbce9 Mon Sep 17 00:00:00 2001 From: Dan J Miller Date: Mon, 5 Dec 2022 16:23:24 -0330 Subject: [PATCH 12/14] Update packages/assets-controllers/src/CurrencyRateController.ts Co-authored-by: Elliot Winkler --- packages/assets-controllers/src/CurrencyRateController.ts | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/packages/assets-controllers/src/CurrencyRateController.ts b/packages/assets-controllers/src/CurrencyRateController.ts index 2608d17f1ba..5b4921f0a62 100644 --- a/packages/assets-controllers/src/CurrencyRateController.ts +++ b/packages/assets-controllers/src/CurrencyRateController.ts @@ -90,7 +90,10 @@ export class CurrencyRateController extends BaseControllerV2< private includeUsdRate; - #enabled; // A boolean that controls whether or not network requests can be made by the controller. + /** + * A boolean that controls whether or not network requests can be made by the controller + */ + #enabled; /** * Creates a CurrencyRateController instance. From fa82463fcf13907c8bb9a2444aaef7db524a4d9c Mon Sep 17 00:00:00 2001 From: Dan J Miller Date: Mon, 5 Dec 2022 16:23:34 -0330 Subject: [PATCH 13/14] Update packages/assets-controllers/src/CurrencyRateController.ts Co-authored-by: Elliot Winkler --- packages/assets-controllers/src/CurrencyRateController.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/packages/assets-controllers/src/CurrencyRateController.ts b/packages/assets-controllers/src/CurrencyRateController.ts index 5b4921f0a62..28d36ba66c1 100644 --- a/packages/assets-controllers/src/CurrencyRateController.ts +++ b/packages/assets-controllers/src/CurrencyRateController.ts @@ -209,6 +209,7 @@ export class CurrencyRateController extends BaseControllerV2< */ async updateExchangeRate(): Promise { if (!this.#enabled) { + console.info('[CurrencyRateController] Not updating exchange rate since network requests have been disabled'); return this.state; } const releaseLock = await this.mutex.acquire(); From aed0fd010abd5e4099287eda174b538dd87df23e Mon Sep 17 00:00:00 2001 From: Dan J Miller Date: Mon, 5 Dec 2022 16:34:09 -0330 Subject: [PATCH 14/14] Lint fix --- packages/assets-controllers/src/CurrencyRateController.ts | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/packages/assets-controllers/src/CurrencyRateController.ts b/packages/assets-controllers/src/CurrencyRateController.ts index 28d36ba66c1..0256224e0d2 100644 --- a/packages/assets-controllers/src/CurrencyRateController.ts +++ b/packages/assets-controllers/src/CurrencyRateController.ts @@ -209,7 +209,9 @@ export class CurrencyRateController extends BaseControllerV2< */ async updateExchangeRate(): Promise { if (!this.#enabled) { - console.info('[CurrencyRateController] Not updating exchange rate since network requests have been disabled'); + console.info( + '[CurrencyRateController] Not updating exchange rate since network requests have been disabled', + ); return this.state; } const releaseLock = await this.mutex.acquire();