Fix bug that allowed for multiple fetch requests instantiate from get…

…GasFeeEstimatesAndStartPolling (#586) * Ensure that the gas fee controller does not make new network requests while awaiting for a previous getGasFeeEstimatesAndStartPolling call * Use a jest function for handling events from nock calls in GasFeeController.test.ts * Improve unit test description'
MetaMask · Oct 11, 2023 · eca999b · eca999b
1 parent ce94444
commit eca999b
Show file tree

Hide file tree

Showing 2 changed files with 74 additions and 27 deletions.
diff --git a/src/gas/GasFeeController.test.ts b/src/gas/GasFeeController.test.ts
@@ -33,6 +33,8 @@ describe('GasFeeController', () => {
   let getCurrentNetworkLegacyGasAPICompatibility: jest.Mock<boolean>;
   let getIsEIP1559Compatible: jest.Mock<Promise<boolean>>;
   let getChainId: jest.Mock<`0x${string}` | `${number}` | number>;
+  let mockGasFeeRequest: any;
+  const mockRequestHandler = jest.fn();
 
   beforeAll(() => {
     nock.disableNetConnect();
@@ -50,7 +52,7 @@ describe('GasFeeController', () => {
     getIsEIP1559Compatible = jest
       .fn()
       .mockImplementation(() => Promise.resolve(true));
-    nock(TEST_GAS_FEE_API.replace('<chain_id>', '1'))
+    mockGasFeeRequest = nock(TEST_GAS_FEE_API.replace('<chain_id>', '1'))
       .get(/.+/u)
       .reply(200, {
         low: {
@@ -74,6 +76,7 @@ describe('GasFeeController', () => {
         estimatedBaseFee: '28',
       })
       .persist();
+    mockGasFeeRequest.on('request', mockRequestHandler);
 
     nock(TEST_LEGACY_FEE_API.replace('<chain_id>', '0x1'))
       .get(/.+/u)
@@ -99,25 +102,76 @@ describe('GasFeeController', () => {
 
   afterEach(() => {
     nock.cleanAll();
+    jest.clearAllMocks();
     gasFeeController.destroy();
   });
 
   it('should initialize', async () => {
     expect(gasFeeController.name).toBe(name);
   });
 
-  it('should getGasFeeEstimatesAndStartPolling', async () => {
-    expect(gasFeeController.state.gasFeeEstimates).toStrictEqual({});
-    const result = await gasFeeController.getGasFeeEstimatesAndStartPolling(
-      undefined,
-    );
-    expect(result).toHaveLength(36);
-    expect(gasFeeController.state.gasFeeEstimates).toHaveProperty('low');
-    expect(gasFeeController.state.gasFeeEstimates).toHaveProperty('medium');
-    expect(gasFeeController.state.gasFeeEstimates).toHaveProperty('high');
-    expect(gasFeeController.state.gasFeeEstimates).toHaveProperty(
-      'estimatedBaseFee',
-    );
+  describe('getGasFeeEstimatesAndStartPolling', () => {
+    it('should fetch estimates and start polling', async () => {
+      expect(gasFeeController.state.gasFeeEstimates).toStrictEqual({});
+      const result = await gasFeeController.getGasFeeEstimatesAndStartPolling(
+        undefined,
+      );
+      expect(result).toHaveLength(36);
+      expect(gasFeeController.state.gasFeeEstimates).toHaveProperty('low');
+      expect(gasFeeController.state.gasFeeEstimates).toHaveProperty('medium');
+      expect(gasFeeController.state.gasFeeEstimates).toHaveProperty('high');
+      expect(gasFeeController.state.gasFeeEstimates).toHaveProperty(
+        'estimatedBaseFee',
+      );
+    });
+
+    it('should not fetch estimates if the controller is already polling, and should still return the passed token', async () => {
+      const pollToken = 'token';
+
+      const firstCallPromise = gasFeeController.getGasFeeEstimatesAndStartPolling(
+        undefined,
+      );
+      const secondCallPromise = gasFeeController.getGasFeeEstimatesAndStartPolling(
+        pollToken,
+      );
+
+      const result1 = await firstCallPromise;
+      const result2 = await secondCallPromise;
+
+      expect(mockRequestHandler).toHaveBeenCalledTimes(1);
+      expect(result1).toHaveLength(36);
+      expect(result2).toStrictEqual(pollToken);
+    });
+
+    it('should cause the fetching new estimates if called after the poll tokens are cleared, and then should not cause additional new fetches when subsequently called', async () => {
+      const pollToken = 'token';
+
+      const firstCallPromise = gasFeeController.getGasFeeEstimatesAndStartPolling(
+        undefined,
+      );
+      const secondCallPromise = gasFeeController.getGasFeeEstimatesAndStartPolling(
+        pollToken,
+      );
+
+      await firstCallPromise;
+      await secondCallPromise;
+
+      expect(mockRequestHandler).toHaveBeenCalledTimes(1);
+
+      gasFeeController.stopPolling();
+
+      const result3 = await gasFeeController.getGasFeeEstimatesAndStartPolling(
+        undefined,
+      );
+      expect(result3).toHaveLength(36);
+      expect(mockRequestHandler).toHaveBeenCalledTimes(2);
+
+      const result4 = await gasFeeController.getGasFeeEstimatesAndStartPolling(
+        undefined,
+      );
+      expect(result4).toHaveLength(36);
+      expect(mockRequestHandler).toHaveBeenCalledTimes(2);
+    });
   });
 
   describe('when on any network supporting legacy gas estimation api', () => {

diff --git a/src/gas/GasFeeController.ts b/src/gas/GasFeeController.ts
@@ -323,13 +323,14 @@ export class GasFeeController extends BaseController<
   async getGasFeeEstimatesAndStartPolling(
     pollToken: string | undefined,
   ): Promise<string> {
-    if (this.pollTokens.size === 0) {
-      await this._fetchGasFeeEstimateData();
-    }
-
     const _pollToken = pollToken || random();
 
-    this._startPolling(_pollToken);
+    this.pollTokens.add(_pollToken);
+
+    if (this.pollTokens.size === 1) {
+      await this._fetchGasFeeEstimateData();
+      this._poll();
+    }
 
     return _pollToken;
   }
@@ -444,15 +445,7 @@ export class GasFeeController extends BaseController<
     this.stopPolling();
   }
 
-  // should take a token, so we know that we are only counting once for each open transaction
-  private async _startPolling(pollToken: string) {
-    if (this.pollTokens.size === 0) {
-      this._poll();
-    }
-    this.pollTokens.add(pollToken);
-  }
-
-  private async _poll() {
+  private _poll() {
     if (this.intervalId) {
       clearInterval(this.intervalId);
     }