Feat/phishing controller hotlist timestamp #5621

0xOhm · 2025-04-09T18:29:57Z

Explanation

What is the current state of things and why does it need to change? This needed to change in order to make sure we dont drop updates
What is the solution your changes offer and how does it work? We introduced a timestamp on the phishing detection endpoint to ensure we have the correct timestamp

References

Are there any issues that this pull request is tied to? Issue
Are there other links that reviewers should consult to understand these changes better? Michael Hahn and Jacob Lebowitz

Changelog

Checklist

I've updated the test suite for new or updated code as appropriate
I've updated documentation (JSDoc, Markdown, etc.) for new or updated code as appropriate
I've communicated my changes to consumers by updating changelogs for packages I've changed, highlighting breaking changes as necessary
I've prepared draft pull requests for clients and consumer packages to resolve any breaking changes

…w type update for c2Blocklist having lastFetchedAt being a number now rather than string Signed-off-by: Ohm <[email protected]>

Signed-off-by: Ohm <[email protected]>

…shingControllerHotlistTimestamp

packages/phishing-controller/src/utils.ts

packages/controller-utils/src/util.ts

packages/controller-utils/src/util.test.ts

cryptotavares · 2025-04-09T21:31:57Z

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

+      });
+
+      // Mock a successful response with lastFetchedAt at the root level similar to the example
+      nock(PHISHING_CONFIG_BASE_URL)


With nock we also want to ensure that the request was actually done and the mock was hit. You can do that by using the isDone (https://github.com/nock/nock?tab=readme-ov-file#isdone) expecation from nock:

const scope = nock(...); ... expect(scope.isDone()).toBe(true);

cryptotavares · 2025-04-09T21:37:25Z

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

+
+      // Verify the timestamp wasn't changed
+      expect(controller.state.hotlistLastSuccessTimestamp).toBe(1744213500);
+    });


Here it seems that we are actually doing 3 tests:

mock successfull response with lastFetchedAt

mock successfull response without lastFetchedAt

mock failed response

Each of those should be its own test (an it descriptor for each case). You can read more about testing best practices in the contributor docs:

Overall best practices: https://github.com/nock/nock?tab=readme-ov-file#isdone

Unit test best practices: https://github.com/MetaMask/contributor-docs/blob/main/docs/testing/unit-testing.md

packages/phishing-controller/src/PhishingController.ts

eslint-warning-thresholds.json

…shingControllerHotlistTimestamp

Signed-off-by: Ohm <[email protected]>

…ags, reduced linting errors across other files Signed-off-by: Ohm <[email protected]>

Signed-off-by: Ohm <[email protected]>

0xOhm · 2025-04-09T23:58:24Z

packages/phishing-controller/src/PhishingController.ts

@@ -148,10 +148,16 @@ export type HotlistDiff = {
  isRemoval?: boolean;
 };

-// TODO: Either fix this lint violation or explain why it's necessary to ignore.
-// eslint-disable-next-line @typescript-eslint/naming-convention
+/**


here is explicitly the new JSDoc tags

mindofmar · 2025-04-10T16:30:17Z

packages/phishing-controller/src/PhishingController.ts

@@ -792,7 +812,7 @@ export class PhishingController extends BaseController<
    const newPhishingLists = this.state.phishingLists.map((phishingList) => {
      const updatedList = applyDiffs(
        phishingList,
-        [],
+        [], // Proper Hotlist object with empty data array


no need for this comment

mindofmar · 2025-04-10T16:32:38Z

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

          eth_phishing_detect_config: {
            allowlist: [allowlistedHostname],
            blocklist: [],
            fuzzylist: [],
          },
-          // eslint-disable-next-line @typescript-eslint/naming-convention


why removing these throughout the entire file?

mindofmar · 2025-04-10T16:42:06Z

packages/phishing-controller/src/PhishingController.ts

@@ -714,19 +724,20 @@ export class PhishingController extends BaseController<
   * this function that prevents redundant configuration updates.
   */
  async #updateHotlist() {
+    if (this.state.hotlistLastSuccessTimestamp === 0) {


Why does the updateHotlist() function need to update the stalelist?

My concern is that in the controller's default state, this value is set to 0. AFAIK, the controller should already be fetching the stalelist in that scenario. But then updateHotlist would then be called, potentially causing a duplicate request to update the stalelist. Let me know if I am missing something here

Should run this locally to ensure there are not duplicate stalelist requests being made since that is a heavy request

Yes, I agree with @mindofmar.. not sure why we are calling updateStalelist in this scenario. If the stale list is out of date, the updateHotlist method will not be called. So not sure why we are doing this.

The idea was to force a Stalelist refresh on update to this version.

packages/phishing-controller/src/PhishingController.ts

…st updates Signed-off-by: Ohm <[email protected]>

cryptotavares · 2025-04-11T10:09:53Z

packages/phishing-controller/src/PhishingController.ts

@@ -673,6 +683,7 @@ export class PhishingController extends BaseController<
      this.update((draftState) => {
        draftState.stalelistLastFetched = timeNow;
        draftState.hotlistLastFetched = timeNow;
+        draftState.hotlistLastSuccessTimestamp = timeNow;


Before we were relying on the staleList lastUpdated field for querying the hotList and now we are replacing it with the hotlistLastSuccessTimestamp. However as you can see here, the lastUpdated is a property of the staleList response while the hotlistLastSuccessTimestamp is computed in the client. This can cause multiple issues:

The staleList might have been computed sometime into the past. We should not assume that the list returned represents the latest available values at the time that the client receives it.

Using a computed property in the client, will not allow us to capture any items added to the list in between the request being handled by the API and the client computing the timestamp value.

I might be missing something, but I don't think that this implementation is quite right. I believe it should change to something like:

draftState.hotlistLastSuccessTimestamp = hotlistDiffsResponse?.lastFetchedAt || this.state.hotlistLastSuccessTimestamp

cryptotavares · 2025-04-11T10:13:19Z

packages/phishing-controller/src/PhishingController.ts

      hotlistResponse = await this.#queryConfig<DataResultWrapper<Hotlist>>(
-        `${METAMASK_HOTLIST_DIFF_URL}/${lastDiffTimestamp}`,
+        `${METAMASK_HOTLIST_DIFF_URL}/${this.state.hotlistLastSuccessTimestamp}`,


What if the hotlistLastSuccessTimestamp is still set to 0? Shouldn't we use the state.phishingLists[].lastUpdated?

cryptotavares · 2025-04-11T10:18:26Z

packages/phishing-controller/src/PhishingController.ts

+    this.update((draftState) => {
+      // We've already verified that hotlistResponse is not null above
+      draftState.hotlistLastSuccessTimestamp =
+        hotlistResponse?.lastFetchedAt ||
+        draftState.hotlistLastSuccessTimestamp;
+    });


Should we prevent updating it to the same value as before? (there's no value in that operation). We could update the code here to something like:

if (hotlistResponse?.lastFetchedAt) { this.update((draftState) => { draftState.hotlistLastSuccessTimestamp = hotlistResponse.lastFetchedAt; }); }

Also I don't think that we need the code comments added. You can drop those as the code is quite readable here.

0xOhm added 3 commits April 9, 2025 12:49

adding changes + tests, still need to manually test extension with ne…

936cf7f

…w type update for c2Blocklist having lastFetchedAt being a number now rather than string Signed-off-by: Ohm <[email protected]>

updated to accomdate for linting errors

e7526cf

Signed-off-by: Ohm <[email protected]>

Merge branch 'main' of https://github.com/MetaMask/core into feat/Phi…

ca4b82c

…shingControllerHotlistTimestamp

0xOhm requested review from a team as code owners April 9, 2025 18:29

mindofmar reviewed Apr 9, 2025

View reviewed changes

packages/phishing-controller/src/utils.ts Outdated Show resolved Hide resolved

mindofmar reviewed Apr 9, 2025

View reviewed changes

packages/phishing-controller/src/utils.ts Outdated Show resolved Hide resolved

cryptotavares reviewed Apr 9, 2025

View reviewed changes

packages/controller-utils/src/util.ts Outdated Show resolved Hide resolved

cryptotavares reviewed Apr 9, 2025

View reviewed changes

packages/controller-utils/src/util.test.ts Outdated Show resolved Hide resolved

cryptotavares reviewed Apr 9, 2025

View reviewed changes

packages/phishing-controller/src/PhishingController.ts Outdated Show resolved Hide resolved

cryptotavares reviewed Apr 9, 2025

View reviewed changes

eslint-warning-thresholds.json Outdated Show resolved Hide resolved

cryptotavares reviewed Apr 9, 2025

View reviewed changes

eslint-warning-thresholds.json Show resolved Hide resolved

0xOhm added 6 commits April 9, 2025 17:25

Merge branch 'main' of https://github.com/MetaMask/core into feat/Phi…

f3ac88e

…shingControllerHotlistTimestamp

adding support for hotlistLastSuccessTimestamp

9c7d143

Signed-off-by: Ohm <[email protected]>

addressing unneeded linting code edits

668f58c

Signed-off-by: Ohm <[email protected]>

reverting changes to eslint-warning-thresholds.json to match main

5ac4a32

Signed-off-by: Ohm <[email protected]>

increasing jsdoc tag names are a todo was resolved and required new t…

9afbb76

…ags, reduced linting errors across other files Signed-off-by: Ohm <[email protected]>

reverting controller-utils/src/util.ts back to original state

88d3acc

Signed-off-by: Ohm <[email protected]>

0xOhm commented Apr 9, 2025

View reviewed changes

mindofmar reviewed Apr 10, 2025

View reviewed changes

packages/phishing-controller/src/PhishingController.ts Outdated Show resolved Hide resolved

ensured timestamp for HotlistLastSuccessTimestamp is set post staleli…

8404a9f

…st updates Signed-off-by: Ohm <[email protected]>

cryptotavares reviewed Apr 11, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feat/phishing controller hotlist timestamp #5621

Feat/phishing controller hotlist timestamp #5621

0xOhm commented Apr 9, 2025

cryptotavares Apr 9, 2025

cryptotavares Apr 9, 2025

0xOhm Apr 9, 2025

mindofmar Apr 10, 2025

mindofmar Apr 10, 2025

mindofmar Apr 10, 2025

mindofmar Apr 10, 2025

cryptotavares Apr 11, 2025

0xOhm Apr 14, 2025

cryptotavares Apr 11, 2025

cryptotavares Apr 11, 2025

cryptotavares Apr 11, 2025

Feat/phishing controller hotlist timestamp #5621

Are you sure you want to change the base?

Feat/phishing controller hotlist timestamp #5621

Conversation

0xOhm commented Apr 9, 2025

Explanation

References

Changelog

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment