Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add lastFetched PhishingController state #986

Merged
merged 9 commits into from
Nov 29, 2022
Merged

Add lastFetched PhishingController state #986

merged 9 commits into from
Nov 29, 2022

Conversation

segun
Copy link
Contributor

@segun segun commented Nov 22, 2022

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

@segun segun requested a review from a team as a code owner November 22, 2022 22:01
@segun segun self-assigned this Nov 22, 2022
digiwand
digiwand previously approved these changes Nov 23, 2022
View reviewed changes
@segun segun force-pushed the dev-olu-phishing-controller-use-internal-state branch from cdec915 to 96cc0ca Compare November 23, 2022 19:41
Gudahtt
Gudahtt reviewed Nov 23, 2022
View reviewed changes
Copy link
Member

@Gudahtt Gudahtt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These changes make sense! The title doesn't seem accurate though, since this controller already accepted initial state. Maybe we could title it "Add lastFetched to PhishingController state` instead, or something like that.

packages/phishing-controller/src/PhishingController.ts Show resolved Hide resolved
packages/phishing-controller/src/PhishingController.test.ts Outdated Show resolved Hide resolved
packages/phishing-controller/src/PhishingController.test.ts Outdated Show resolved Hide resolved
packages/phishing-controller/src/PhishingController.test.ts Outdated Show resolved Hide resolved
Gudahtt
Gudahtt previously approved these changes Nov 25, 2022
View reviewed changes
Copy link
Member

@Gudahtt Gudahtt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Some additional console errors introduced here that we should cleanup, but we can do that in a follow-up PR since it's a pre-existing problem.

packages/phishing-controller/src/PhishingController.test.ts Show resolved Hide resolved
digiwand
digiwand previously approved these changes Nov 25, 2022
View reviewed changes
@segun segun dismissed stale reviews from digiwand and Gudahtt via 660f0c8 November 25, 2022 20:17
@segun segun mentioned this pull request Nov 29, 2022
Merged
8 tasks
Gudahtt
Gudahtt approved these changes Nov 29, 2022
View reviewed changes
Copy link
Member

@Gudahtt Gudahtt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@segun segun requested a review from digiwand November 29, 2022 13:50
@segun segun changed the title Pass in initial state to PhishingController Add lastFetched PhishingController state Nov 29, 2022
@segun
Abstract out maybeUpdatePhishingLists so that we don't have to do the
4a13d1b 
checks in metamask-controller.

Signed-off-by: Akintayo A. Olusegun <akintayo.segun@gmail.com>
@segun
Remove unneccesary spaces.
6872737 
Signed-off-by: Akintayo A. Olusegun <akintayo.segun@gmail.com>
@segun
Lint fixes
f0d323d 
Signed-off-by: Akintayo A. Olusegun <akintayo.segun@gmail.com>
@segun
Tests for maybeUpdatePhishingList
d9636be 
Signed-off-by: Akintayo A. Olusegun <akintayo.segun@gmail.com>
@segun
Do not call maybeUpdatePhishingLists twice, call once and await the
a82453d 
result later

Signed-off-by: Akintayo A. Olusegun <akintayo.segun@gmail.com>
@segun
Do not test controller method directly, use nock
cb88fa6 
Signed-off-by: Akintayo A. Olusegun <akintayo.segun@gmail.com>
@segun
Add jsdoc
317747a 
Signed-off-by: Akintayo A. Olusegun <akintayo.segun@gmail.com>
@segun
change some wordings
0f0ddd7 
Signed-off-by: Akintayo A. Olusegun <akintayo.segun@gmail.com>
@segun
Put nock in describe to fix console.log errors
af4b9b0 
Signed-off-by: Akintayo A. Olusegun <akintayo.segun@gmail.com>
@segun segun force-pushed the dev-olu-phishing-controller-use-internal-state branch from 660f0c8 to af4b9b0 Compare November 29, 2022 17:09
@segun segun merged commit 5f1f69d into main Nov 29, 2022
@segun segun deleted the dev-olu-phishing-controller-use-internal-state branch November 29, 2022 17:52
gantunesr pushed a commit that referenced this pull request Dec 8, 2022
@segun @gantunesr
Add lastFetched PhishingController state (#986)
bcd8a69 
**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 <akintayo.segun@gmail.com>
MajorLift pushed a commit that referenced this pull request Oct 11, 2023
@segun @MajorLift
Add lastFetched PhishingController state (#986)
1c344b9 
**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 <akintayo.segun@gmail.com>
MajorLift pushed a commit that referenced this pull request Oct 11, 2023
@segun @MajorLift
Add lastFetched PhishingController state (#986)
9a74b39 
**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 <akintayo.segun@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@darkwing darkwing darkwing approved these changes

@Gudahtt Gudahtt Gudahtt approved these changes

@digiwand digiwand Awaiting requested review from digiwand

Assignees

@segun segun

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Bug]: [MV3] Loading a detected phishing page for the first time does not show the phishing page warning
4 participants
@segun @darkwing @Gudahtt @digiwand