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.

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

Refactor NetworkController to BaseControllerV2 #903

Merged
merged 16 commits into from
Oct 3, 2022

Conversation

BelfordZ
Copy link
Contributor

@BelfordZ BelfordZ commented Sep 7, 2022

NetworkController update to extend from BaseControllerV2

swaps out the base controller, implements messenger, fixes all the tests associated

Description

  • BREAKING:

    • NetworkController constructor now takes a ControllerMessenger
    • subscriptions should be made on the messenger, not the controller
    • listen for providerChange if you want to receive updates once lookupNetwork has completed
  • CHANGED:

    • NetworkController uses BaseControllerV2

Checklist

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

Issue

#902

@BelfordZ BelfordZ requested a review from a team as a code owner September 7, 2022 20:32
@BelfordZ BelfordZ marked this pull request as draft September 7, 2022 20:32
@BelfordZ BelfordZ force-pushed the refactor-network-controller-v2 branch 3 times, most recently from a1c0977 to 5fd5125 Compare September 8, 2022 00:02
@BelfordZ BelfordZ force-pushed the refactor-network-controller-v2 branch 2 times, most recently from 9dae659 to 3e332e4 Compare September 12, 2022 20:00
@BelfordZ BelfordZ force-pushed the refactor-network-controller-v2 branch from 815b372 to c4144f9 Compare September 22, 2022 20:26
@BelfordZ BelfordZ changed the title WIP Refactor NetworkController to BaseControllerV2 Refactor NetworkController to BaseControllerV2 Sep 22, 2022
@BelfordZ BelfordZ force-pushed the refactor-network-controller-v2 branch from c4144f9 to 3e44e92 Compare September 22, 2022 20:29
@BelfordZ BelfordZ marked this pull request as ready for review September 22, 2022 20:30
@BelfordZ BelfordZ force-pushed the refactor-network-controller-v2 branch 5 times, most recently from 7d7ccc8 to 4c820f9 Compare September 26, 2022 19:07
Copy link
Contributor

@mcmire mcmire left a comment

Choose a reason for hiding this comment

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

Hey! Thanks for the PR. One thing for the future is that it might be nice if the test refactors could go in a different PR as I had to sort through those changes a bit, but they all make sense. Most of my comments below are fairly minor or are questions.

src/network/NetworkController.ts Outdated Show resolved Hide resolved
src/network/NetworkController.ts Outdated Show resolved Hide resolved
src/network/NetworkController.ts Outdated Show resolved Hide resolved
src/network/NetworkController.ts Outdated Show resolved Hide resolved
src/network/NetworkController.ts Outdated Show resolved Hide resolved
src/assets/TokenListController.ts Show resolved Hide resolved
src/assets/TokenRatesController.test.ts Outdated Show resolved Hide resolved
src/gas/GasFeeController.ts Show resolved Hide resolved
src/assets/AssetsContractController.test.ts Show resolved Hide resolved
src/network/NetworkController.test.ts Outdated Show resolved Hide resolved
@BelfordZ BelfordZ requested a review from Gudahtt September 29, 2022 23:24
shanejonas and others added 2 commits September 29, 2022 16:29
Fixed tests to pass

Added test isolation of race condition

Remove node-version file

Fixed TokenListController test

Fixed broken tests

Fix network controller to no longer use settimeout

Fixed test cleanup in a variety of cases

Remove tsfmt file

Update collectibles controller tests to minimize refactoring

Change infura api keys to fake ones in tests where possible
@BelfordZ BelfordZ force-pushed the refactor-network-controller-v2 branch from f47b678 to bdc7690 Compare September 29, 2022 23:29
@mcmire
Copy link
Contributor

mcmire commented Oct 3, 2022

Looked through again and this looks good pending the changes mentioned above.

Copy link
Contributor

@mcmire mcmire left a comment

Choose a reason for hiding this comment

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

LGTM. Looking forward to more updates!

@BelfordZ BelfordZ merged commit 739d100 into main Oct 3, 2022
@BelfordZ BelfordZ deleted the refactor-network-controller-v2 branch October 3, 2022 22:13
Gudahtt added a commit that referenced this pull request Nov 8, 2022
This fixes a deadlock accidentally introduced as part of #903. That PR
introduced an early exit in the `net_version` callback that did not
release the lock.

A `try...finally` block has been introduced to ensures the lock is
always released.
@Gudahtt Gudahtt mentioned this pull request Nov 8, 2022
2 tasks
Gudahtt added a commit that referenced this pull request Nov 8, 2022
This fixes a deadlock accidentally introduced as part of #903. That PR
introduced an early exit in the `net_version` callback that did not
release the lock.

A `try...finally` block has been introduced to ensures the lock is
always released.
Gudahtt added a commit that referenced this pull request Nov 8, 2022
This fixes a deadlock accidentally introduced as part of #903. That PR
introduced an early exit in the `net_version` callback that did not
release the lock.

A `try...finally` block has been introduced to ensures the lock is
always released.
gantunesr pushed a commit that referenced this pull request Dec 8, 2022
* Refactor NetworkController to BaseControllerV2

Change onNetworkStateChange to use messaging system & use providerChange event where possible.

Co-authored-by: Shane Jonas <[email protected]>
gantunesr pushed a commit that referenced this pull request Dec 8, 2022
This fixes a deadlock accidentally introduced as part of #903. That PR
introduced an early exit in the `net_version` callback that did not
release the lock.

A `try...finally` block has been introduced to ensures the lock is
always released.
MajorLift pushed a commit that referenced this pull request Oct 11, 2023
* Refactor NetworkController to BaseControllerV2

Change onNetworkStateChange to use messaging system & use providerChange event where possible.

Co-authored-by: Shane Jonas <[email protected]>
MajorLift pushed a commit that referenced this pull request Oct 11, 2023
This fixes a deadlock accidentally introduced as part of #903. That PR
introduced an early exit in the `net_version` callback that did not
release the lock.

A `try...finally` block has been introduced to ensures the lock is
always released.
MajorLift pushed a commit that referenced this pull request Oct 11, 2023
* Refactor NetworkController to BaseControllerV2

Change onNetworkStateChange to use messaging system & use providerChange event where possible.

Co-authored-by: Shane Jonas <[email protected]>
MajorLift pushed a commit that referenced this pull request Oct 11, 2023
This fixes a deadlock accidentally introduced as part of #903. That PR
introduced an early exit in the `net_version` callback that did not
release the lock.

A `try...finally` block has been introduced to ensures the lock is
always released.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants