Add lastFetched PhishingController state (#986)

**Pass in initial state to PhishingController and load the initialState
from persisted state**
In Manifest V3, there's need to 

1. Persist phishingcontroller state
2. Load phishingcontroller from persistent state

Thos PR makes sure state is loaded from persistent state and also
abstracts out the checks required before updating the lists

- BREAKING:
- All the code bases using PhishingController should continue to work as
expected
- CHANGED:
- We should call initialise before calling PhishingDetector controller.

- ADDED:

- Added `maybeUpdatePhishingLists` method so that controllers don't have
to do the check logic anymore

**Checklist**

- [ ] Tests are included if applicable
- [ ] Any added code is fully documented

**Issue**

Resolves MetaMask/metamask-extension#16551

Signed-off-by: Akintayo A. Olusegun <[email protected]>

packages/phishing-controller/src/PhishingController.test.ts

@@ -69,6 +69,100 @@ describe('PhishingController', () => {
     expect(nockScope.isDone()).toBe(false);
   });
 
+  describe('maybeUpdatePhishingLists', () => {
+    let nockScope: nock.Scope;
+
+    beforeEach(() => {
+      nockScope = nock(PHISHING_CONFIG_BASE_URL)
+        .get(METAMASK_CONFIG_FILE)
+        .reply(200, {
+          blacklist: ['this-should-not-be-in-default-blacklist.com'],
+          fuzzylist: [],
+          tolerance: 0,
+          whitelist: ['this-should-not-be-in-default-whitelist.com'],
+          version: 0,
+        })
+        .get(PHISHFORT_HOTLIST_FILE)
+        .reply(200, []);
+    });
+
+    it('should not be out of date immediately after maybeUpdatePhishingLists is called', async () => {
+      const clock = sinon.useFakeTimers();
+      const controller = new PhishingController({ refreshInterval: 10 });
+      clock.tick(10);
+      expect(controller.isOutOfDate()).toBe(true);
+      await controller.maybeUpdatePhishingLists();
+      expect(controller.isOutOfDate()).toBe(false);
+
+      expect(nockScope.isDone()).toBe(true);
+    });
+
+    it('should not be out of date after maybeUpdatePhishingLists is called but before refresh interval has passed', async () => {
+      const clock = sinon.useFakeTimers();
+      const controller = new PhishingController({ refreshInterval: 10 });
+      clock.tick(10);
+      expect(controller.isOutOfDate()).toBe(true);
+      await controller.maybeUpdatePhishingLists();
+      clock.tick(5);
+      expect(controller.isOutOfDate()).toBe(false);
+      expect(nockScope.isDone()).toBe(true);
+    });
+
+    it('should still be out of date while update is in progress', async () => {
+      const clock = sinon.useFakeTimers();
+      const controller = new PhishingController({ refreshInterval: 10 });
+      clock.tick(10);
+      // do not wait
+      const maybeUpdatePhisingListPromise =
+        controller.maybeUpdatePhishingLists();
+      expect(controller.isOutOfDate()).toBe(true);
+      await maybeUpdatePhisingListPromise;
+      expect(controller.isOutOfDate()).toBe(false);
+      clock.tick(10);
+      expect(controller.isOutOfDate()).toBe(true);
+      expect(nockScope.isDone()).toBe(true);
+    });
+
+    it('should call update only if it is out of date, otherwise it should not call update', async () => {
+      const clock = sinon.useFakeTimers();
+      const controller = new PhishingController({ refreshInterval: 10 });
+      expect(controller.isOutOfDate()).toBe(false);
+      await controller.maybeUpdatePhishingLists();
+      expect(
+        controller.test('this-should-not-be-in-default-blacklist.com'),
+      ).toMatchObject({
+        result: false,
+        type: 'all',
+      });
+
+      expect(
+        controller.test('this-should-not-be-in-default-whitelist.com'),
+      ).toMatchObject({
+        result: false,
+        type: 'all',
+      });
+
+      clock.tick(10);
+      await controller.maybeUpdatePhishingLists();
+
+      expect(
+        controller.test('this-should-not-be-in-default-blacklist.com'),
+      ).toMatchObject({
+        result: true,
+        type: 'blocklist',
+      });
+
+      expect(
+        controller.test('this-should-not-be-in-default-whitelist.com'),
+      ).toMatchObject({
+        result: false,
+        type: 'allowlist',
+      });
+
+      expect(nockScope.isDone()).toBe(true);
+    });
+  });
+
   describe('isOutOfDate', () => {
     it('should not be out of date upon construction', () => {
       sinon.useFakeTimers();

packages/phishing-controller/src/PhishingController.ts

@@ -83,6 +83,7 @@ export interface PhishingConfig extends BaseConfig {
 export interface PhishingState extends BaseState {
   phishing: EthPhishingDetectConfig[];
   whitelist: string[];
+  lastFetched: number;
 }
 
 export const PHISHING_CONFIG_BASE_URL =
@@ -104,8 +105,6 @@ export class PhishingController extends BaseController<
 > {
   private detector: any;
 
-  private lastFetched = 0;
-
   #inProgressUpdate: Promise<void> | undefined;
 
   /**
@@ -140,9 +139,11 @@ export class PhishingController extends BaseController<
         },
       ],
       whitelist: [],
+      lastFetched: 0,
     };
-    this.detector = new PhishingDetector(this.defaultState.phishing);
+
     this.initialize();
+    this.detector = new PhishingDetector(this.state.phishing);
   }
 
   /**
@@ -160,7 +161,7 @@ export class PhishingController extends BaseController<
    * @returns Whether an update is needed
    */
   isOutOfDate() {
-    return Date.now() - this.lastFetched >= this.config.refreshInterval;
+    return Date.now() - this.state.lastFetched >= this.config.refreshInterval;
   }
 
   /**
@@ -215,6 +216,19 @@ export class PhishingController extends BaseController<
     }
   }
 
+  /**
+   * Conditionally update the phishing configuration.
+   *
+   * If the phishing configuration is out of date, this function will call `updatePhishingLists`
+   * to update the configuration.
+   */
+  async maybeUpdatePhishingLists() {
+    const phishingListsAreOutOfDate = this.isOutOfDate();
+    if (phishingListsAreOutOfDate) {
+      await this.updatePhishingLists();
+    }
+  }
+
   /**
    * Update the phishing configuration.
    *
@@ -238,7 +252,9 @@ export class PhishingController extends BaseController<
     } finally {
       // Set `lastFetched` even for failed requests to prevent server from being overwhelmed with
       // traffic after a network disruption.
-      this.lastFetched = Date.now();
+      this.update({
+        lastFetched: Date.now(),
+      });
     }
 
     // Correctly shaping MetaMask config.