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

feat: Delay network controller provider initialization #23005

Draft
wants to merge 23 commits into
base: develop
Choose a base branch
from
Draft

feat: Delay network controller provider initialization #23005

wants to merge 23 commits into from
+153 −54

Conversation

pedronfigueiredo
Copy link
Contributor

@pedronfigueiredo pedronfigueiredo commented Feb 16, 2024 •
edited by dbrans

Description

We want to have finer control over the initialization of the network controller inside the metamask controller. For additional background and reasoning, see the tickets linked below.

This PR also depends on 2 PRs (#274 on the smart transactions controller repo and #4004 on the core repo) being merged and the corresponding packages released first. Once that is done, I will update the dependencies on this PR.

Related issues

Fixes: #2114 on the planning repo (For more details see also #881)

See: PR #274 on the smart transactions controller repo and PR #4004 on the core repo.

Manual testing steps

  • Checkout develop.
  • Run yarn && yarn dist.
  • When onboarding the extension, you should see the requests on the background console.
  • Checkout this branch.
  • Clone the SmartTransactionController repository.
  • Checkout this PR and build.
  • Clone the Core repository.
  • Checkout this PR and build.
  • Add these two lines to the resolutions section of the package.json.
    "@metamask/transaction-controller@^23.1.0": "file:../core/packages/transaction-controller",
    "@metamask/smart-transactions-controller@^6.2.2": "file:../smart-transactions-controller",
  • You may need to run yarn && yarn lavamoat:auto.
  • Run yarn && yarn dist.
  • No requests to the RPC node will be made before the advanced settings during onboarding.

Screenshots/Recordings

N/A

Pre-merge author checklist

  • I’ve followed MetaMask Coding Standards.
  • I've clearly explained what problem this PR is solving and how it is solved.
  • I've linked related issues
  • I've included manual testing steps
  • I've included screenshots/recordings if applicable
  • I’ve included tests if applicable
  • I’ve documented my code using JSDoc format if applicable
  • I’ve applied the right labels on the PR (see labeling guidelines). Not required for external contributors.
  • I’ve properly set the pull request status:
    • In case it's not yet "ready for review", I've set it to "draft".
    • In case it's "ready for review", I've changed it from "draft" to "non-draft".

Pre-merge reviewer checklist

  • I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed).
  • I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots.

@pedronfigueiredo pedronfigueiredo self-assigned this Feb 16, 2024
@pedronfigueiredo pedronfigueiredo requested a review from a team as a code owner February 16, 2024 15:21
@github-actions GitHub Actions
Copy link
Contributor

CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes.

@danjm danjm marked this pull request as draft February 16, 2024 18:52
@pedronfigueiredo pedronfigueiredo changed the title Delay network controller initialization Delay network controller provider initialization Feb 19, 2024
This was referenced Feb 21, 2024
Merged
Closed
@pedronfigueiredo pedronfigueiredo added the DO-NOT-MERGE Pull requests that should not be merged label Feb 22, 2024
@pedronfigueiredo pedronfigueiredo marked this pull request as ready for review February 22, 2024 12:41
@gauthierpetetin gauthierpetetin added the needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) label Feb 22, 2024
@pedronfigueiredo pedronfigueiredo changed the base branch from feat-881 to develop March 5, 2024 11:27
@pedronfigueiredo pedronfigueiredo requested review from a team as code owners March 5, 2024 11:41
@pedronfigueiredo pedronfigueiredo changed the title Delay network controller provider initialization feat: Delay network controller provider initialization Mar 8, 2024
@legobeat legobeat mentioned this pull request Mar 13, 2024
Closed
@pedronfigueiredo pedronfigueiredo mentioned this pull request Mar 21, 2024
Merged
13 tasks
pedronfigueiredo added a commit that referenced this pull request Mar 26, 2024
@pedronfigueiredo @danjm
fix: Prevent rpc request before onboarding is completed (#23622)
c947630 
## **Description**

The temporary solution is to:

- Prevent two network controller requests by:
- Patch the network controller so that the lookup on
initializeProvidercan be avoided.
- Call that initializeProvider with the lookup: false argument right
after the network controller is constructed
- Call await this.networkController.lookupNetwork(); at the end of the
metamask controller if the onboarding has been completed
- Call await this.networkController.lookupNetwork(); once the onboarding
complete state changes
- Prevent one accounts tracker request by:
- Moving the blockTracker event listener from the constructor of the
controller to the start function

The long term solution will be
[#23005](#23005)
(currently blocked by [#4004 on
core](MetaMask/core#4004).


## **Related issues**

Fixes: MetaMask/MetaMask-planning#881

## **Manual testing steps**

1. Add `await new Promise((resolve) => setTimeout(resolve, 10000));` to
the top of initialize on the background.js file
2. Build the app
3. Load extension and immediately open the network tab on the background
console before the app initializes
4. No requests should happen

## **Screenshots/Recordings**

<!-- If applicable, add screenshots and/or recordings to visualize the
before and after of your change. -->

### **Before**




https://github.com/MetaMask/metamask-extension/assets/7499938/73513327-90fb-4e9f-8af2-aa41c731b647





### **After**



https://github.com/MetaMask/metamask-extension/assets/7499938/d6dee1c6-fa13-48c8-9fe2-520e3d43d0b3



## **Pre-merge author checklist**

- [ ] I’ve followed [MetaMask Coding
Standards](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/CODING_GUIDELINES.md).
- [ ] I've clearly explained what problem this PR is solving and how it
is solved.
- [ ] I've linked related issues
- [ ] I've included manual testing steps
- [ ] I've included screenshots/recordings if applicable
- [ ] I’ve included tests if applicable
- [ ] I’ve documented my code using [JSDoc](https://jsdoc.app/) format
if applicable
- [ ] I’ve applied the right labels on the PR (see [labeling
guidelines](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/LABELING_GUIDELINES.md)).
Not required for external contributors.
- [ ] I’ve properly set the pull request status:
  - [ ] In case it's not yet "ready for review", I've set it to "draft".
- [ ] In case it's "ready for review", I've changed it from "draft" to
"non-draft".

## **Pre-merge reviewer checklist**

- [ ] I've manually tested the PR (e.g. pull and build branch, run the
app, test code being changed).
- [ ] I confirm that this PR addresses all acceptance criteria described
in the ticket it closes and includes the necessary testing evidence such
as recordings and or screenshots.

---------

Co-authored-by: Dan J Miller <danjm.com@gmail.com>
pedronfigueiredo added a commit that referenced this pull request Mar 26, 2024
@pedronfigueiredo @danjm
fix: Prevent rpc request before onboarding is completed (#23622)
f315975 
## **Description**

The temporary solution is to:

- Prevent two network controller requests by:
- Patch the network controller so that the lookup on
initializeProvidercan be avoided.
- Call that initializeProvider with the lookup: false argument right
after the network controller is constructed
- Call await this.networkController.lookupNetwork(); at the end of the
metamask controller if the onboarding has been completed
- Call await this.networkController.lookupNetwork(); once the onboarding
complete state changes
- Prevent one accounts tracker request by:
- Moving the blockTracker event listener from the constructor of the
controller to the start function

The long term solution will be
[#23005](#23005)
(currently blocked by [#4004 on
core](MetaMask/core#4004).


## **Related issues**

Fixes: MetaMask/MetaMask-planning#881

## **Manual testing steps**

1. Add `await new Promise((resolve) => setTimeout(resolve, 10000));` to
the top of initialize on the background.js file
2. Build the app
3. Load extension and immediately open the network tab on the background
console before the app initializes
4. No requests should happen

## **Screenshots/Recordings**

<!-- If applicable, add screenshots and/or recordings to visualize the
before and after of your change. -->

### **Before**




https://github.com/MetaMask/metamask-extension/assets/7499938/73513327-90fb-4e9f-8af2-aa41c731b647





### **After**



https://github.com/MetaMask/metamask-extension/assets/7499938/d6dee1c6-fa13-48c8-9fe2-520e3d43d0b3



## **Pre-merge author checklist**

- [ ] I’ve followed [MetaMask Coding
Standards](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/CODING_GUIDELINES.md).
- [ ] I've clearly explained what problem this PR is solving and how it
is solved.
- [ ] I've linked related issues
- [ ] I've included manual testing steps
- [ ] I've included screenshots/recordings if applicable
- [ ] I’ve included tests if applicable
- [ ] I’ve documented my code using [JSDoc](https://jsdoc.app/) format
if applicable
- [ ] I’ve applied the right labels on the PR (see [labeling
guidelines](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/LABELING_GUIDELINES.md)).
Not required for external contributors.
- [ ] I’ve properly set the pull request status:
  - [ ] In case it's not yet "ready for review", I've set it to "draft".
- [ ] In case it's "ready for review", I've changed it from "draft" to
"non-draft".

## **Pre-merge reviewer checklist**

- [ ] I've manually tested the PR (e.g. pull and build branch, run the
app, test code being changed).
- [ ] I confirm that this PR addresses all acceptance criteria described
in the ticket it closes and includes the necessary testing evidence such
as recordings and or screenshots.

---------

Co-authored-by: Dan J Miller <danjm.com@gmail.com>
@pedronfigueiredo pedronfigueiredo mentioned this pull request Mar 26, 2024
Merged
20 tasks
pedronfigueiredo added a commit that referenced this pull request Mar 26, 2024
@pedronfigueiredo @danjm
cherry-pick: Prevent rpc request before onboarding is completed (#23622…
f9be841 
…) (#23730)

## **Description**

The temporary solution is to:

- Prevent two network controller requests by:
- Patch the network controller so that the lookup on
initializeProvidercan be avoided.
- Call that initializeProvider with the lookup: false argument right
after the network controller is constructed
- Call await this.networkController.lookupNetwork(); at the end of the
metamask controller if the onboarding has been completed
- Call await this.networkController.lookupNetwork(); once the onboarding
complete state changes
- Prevent one accounts tracker request by:
- Moving the blockTracker event listener from the constructor of the
controller to the start function

The long term solution will be
[#23005](#23005)
(currently blocked by [#4004 on
core](MetaMask/core#4004).


## **Related issues**

Fixes: MetaMask/MetaMask-planning#881

## **Manual testing steps**

1. Add `await new Promise((resolve) => setTimeout(resolve, 10000));` to
the top of initialize on the background.js file
2. Build the app
3. Load extension and immediately open the network tab on the background
console before the app initializes
4. No requests should happen

## **Screenshots/Recordings**

<!-- If applicable, add screenshots and/or recordings to visualize the
before and after of your change. -->

### **Before**





https://github.com/MetaMask/metamask-extension/assets/7499938/73513327-90fb-4e9f-8af2-aa41c731b647





### **After**




https://github.com/MetaMask/metamask-extension/assets/7499938/d6dee1c6-fa13-48c8-9fe2-520e3d43d0b3



## **Pre-merge author checklist**

- [ ] I’ve followed [MetaMask Coding
Standards](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/CODING_GUIDELINES.md).
- [ ] I've clearly explained what problem this PR is solving and how it
is solved.
- [ ] I've linked related issues
- [ ] I've included manual testing steps
- [ ] I've included screenshots/recordings if applicable
- [ ] I’ve included tests if applicable
- [ ] I’ve documented my code using [JSDoc](https://jsdoc.app/) format
if applicable
- [ ] I’ve applied the right labels on the PR (see [labeling
guidelines](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/LABELING_GUIDELINES.md)).
Not required for external contributors.
- [ ] I’ve properly set the pull request status:
  - [ ] In case it's not yet "ready for review", I've set it to "draft".
- [ ] In case it's "ready for review", I've changed it from "draft" to
"non-draft".

## **Pre-merge reviewer checklist**

- [ ] I've manually tested the PR (e.g. pull and build branch, run the
app, test code being changed).
- [ ] I confirm that this PR addresses all acceptance criteria described
in the ticket it closes and includes the necessary testing evidence such
as recordings and or screenshots.

---------


<!--
Please submit this PR as a draft initially.
Do not mark it as "Ready for review" until the template has been
completely filled out, and PR status checks have passed at least once.
-->

## **Description**

<!--
Write a short description of the changes included in this pull request,
also include relevant motivation and context. Have in mind the following
questions:
1. What is the reason for the change?
2. What is the improvement/solution?
-->

[![Open in GitHub
Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/23730?quickstart=1)

## **Related issues**

Fixes:

## **Manual testing steps**

1. Go to this page...
2.
3.

## **Screenshots/Recordings**

<!-- If applicable, add screenshots and/or recordings to visualize the
before and after of your change. -->

### **Before**

<!-- [screenshots/recordings] -->

### **After**

<!-- [screenshots/recordings] -->

## **Pre-merge author checklist**

- [ ] I’ve followed [MetaMask Coding
Standards](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/CODING_GUIDELINES.md).
- [ ] I've completed the PR template to the best of my ability
- [ ] I’ve included tests if applicable
- [ ] I’ve documented my code using [JSDoc](https://jsdoc.app/) format
if applicable
- [ ] I’ve applied the right labels on the PR (see [labeling
guidelines](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/LABELING_GUIDELINES.md)).
Not required for external contributors.

## **Pre-merge reviewer checklist**

- [ ] I've manually tested the PR (e.g. pull and build branch, run the
app, test code being changed).
- [ ] I confirm that this PR addresses all acceptance criteria described
in the ticket it closes and includes the necessary testing evidence such
as recordings and or screenshots.

Co-authored-by: Dan J Miller <danjm.com@gmail.com>
@pedronfigueiredo pedronfigueiredo mentioned this pull request May 3, 2024
Open
@github-actions GitHub Actions
Copy link
Contributor

This PR has been automatically marked as stale because it has not had recent activity in the last 60 days. It will be closed in 14 days. Thank you for your contributions.

@github-actions github-actions bot added the stale issues and PRs marked as stale label May 10, 2024
@legobeat legobeat marked this pull request as draft May 21, 2024 16:04
@github-actions github-actions bot removed the stale issues and PRs marked as stale label May 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@pedronfigueiredo pedronfigueiredo

Labels
DO-NOT-MERGE Pull requests that should not be merged needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) team-extension-platform
Projects
PR review queue
Status: Needs more work from the author
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@pedronfigueiredo @gauthierpetetin @legobeat