Phishing controller test method should be async and it should not poll

[BelfordZ]

PR Title

problems:

1. Constructor starts polling, which is async. Starting async actions in a contructor makes testing hard.
2. test depends on state.whitelist which is set async via poll > updatePhishingLists. It is a sync function, which creates race conditions.

solution:

1. make test, have it update the phishing list if necessary
2. remove polling

Description

• BREAKING:

  • phishingController.test now returns a Promise<EthPhishingDetectResult>
  • instead of calling poll with a new interval to change the interval, use phishingController.setRefreshInterval or phishingController.configure({ refreshInterval: 123 }) (configure will be deprecated when we switch to using BaseController V2)

• FIXED:

  • race conditions in Phishing controller tests

Checklist

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

Issue

Resolves #???

[mcmire]

Thanks for doing this! Left some comments below — mostly minor, but did want to see if we could make lastFetched private instead of public.

[BelfordZ]

@mcmire I've updated bypass to no longer be async & no longer call fetchIfNecessary

[mcmire]

This looks good to me!

[BelfordZ]

@mcmire bump!

[mcmire]

LGTM!

[Gudahtt]

Hmm. This might significantly slow down the phishing check when a page loads. The check is already made asynchronously, with the suspicious page sometimes loading partially before we have a chance to redirect to the warning page.

I'll test this out to see what it looks like in practice. Maybe there are ways to check sooner in the navigation process.

[Gudahtt]

In testing, the slowdown is noticeable but barely. It doesn't seem appreciably different from before as far as I can tell. Before and after, there is a flash of the page rendering before redirect.

It'll be a bit difficult to manage this on the extension side because we normally check the phishing list just prior to setting up the RPC pipeline. If this is async, we'll need to continue to setup the pipeline without waiting (so we don't drop requests send during page load), but we won't want to process these requests until after the result returns. But that's fine, we can do that.

[Gudahtt]

LGTM!

[Gudahtt]

I went ahead with merging this myself so that it could be included in the v31 release