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

Update controllers that rely on provider to listen to NetworkController:networkDidChange instead of NetworkController:stateChange #3610

Merged

Conversation

jiexi
Copy link
Contributor

@jiexi jiexi commented Dec 1, 2023

Explanation

Listening to NetworkController:stateChange to catch network changes doesn't guarantee that the provider proxy has also been updated to point to the new network. NetworkController:networkDidChange does however. This PR updates several controllers that rely on the provider proxy to listen to NetworkController:networkDidChange rather than NetworkController:stateChange. GasFeeController and AssetsContractController are already listening to NetworkController:networkDidChange on the extension side despite their constructor param hook names still being onNetworkStateChange (which this PR also renames to be correct)

References

Changelog

@metamask/assets-controllers

  • BREAKING: TokensController constructor params onNetworkStateChange replaced with onNetworkDidChange
  • BREAKING: AssetsContractController constructor params onNetworkStateChange replaced with onNetworkDidChange

@metamask/ens-controller

  • BREAKING: EnsController constructor params onNetworkStateChange replaced with onNetworkDidChange

@metamask/gas-fee-controller

  • BREAKING: GasFeeController will subscribe to NetworkController:networkDidChange now instead of NetworkController:stateChange using the passed in messenger param when onNetworkDidChange is not provided
  • CHANGED: GasFeeController constructor params optional onNetworkStateChange replaced with optional onNetworkDidChange

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 highlighted breaking changes using the "BREAKING" category above as appropriate

@jiexi jiexi changed the title Jl/mmp 1713/update network will change network did change usage Update controllers that rely on provider to listen to NetworkController:networkDidChange instead of NetworkController:stateChange Dec 1, 2023
@jiexi
Copy link
Contributor Author

jiexi commented Dec 1, 2023

NOTE: the TransactionController also relies on provider, but I have held off on updating that controller until after I am able to get some clarification on what the intended behavior of the current listener is.

@jiexi jiexi marked this pull request as ready for review December 1, 2023 22:21
@jiexi jiexi requested a review from a team as a code owner December 1, 2023 22:21
Copy link
Contributor

@adonesky1 adonesky1 left a comment

Choose a reason for hiding this comment

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

Looks right to me! Would love to get @mcmire or @Gudahtt to approve before we merge though.

@BelfordZ
Copy link
Contributor

BelfordZ commented Dec 4, 2023

Looks good, should get a great performance improvement from this approach. Let's make sure to test the 'infura blocked' (or anything other network change errors) - These controllers will no longer have event handlers called when there is an error in switching the network.

@jiexi jiexi requested review from mcmire and Gudahtt December 4, 2023 21:57
@jiexi
Copy link
Contributor Author

jiexi commented Dec 4, 2023

Let's make sure to test the 'infura blocked' (or anything other network change errors) - These controllers will no longer have event handlers called when there is an error in switching the network.

State updates from the network metadata flow would follow soon after a networkDidChange event. We definitely would be firing the listener one less time when migrating away from stateChange, but I don't believe this will cause any regression in functionality. Thank you for pointing this out!

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.

Makes sense to me!

@jiexi
Copy link
Contributor Author

jiexi commented Dec 6, 2023

@metamaskbot publish-preview

Copy link
Contributor

github-actions bot commented Dec 6, 2023

Preview builds have been published. See these instructions for more information about preview builds.

Expand for full list of packages and versions.
{
  "@metamask-previews/accounts-controller": "6.0.0-preview.3fbb6b41",
  "@metamask-previews/address-book-controller": "3.1.5-preview.3fbb6b41",
  "@metamask-previews/announcement-controller": "5.0.0-preview.3fbb6b41",
  "@metamask-previews/approval-controller": "5.0.0-preview.3fbb6b41",
  "@metamask-previews/assets-controllers": "20.0.0-preview.3fbb6b41",
  "@metamask-previews/base-controller": "4.0.0-preview.3fbb6b41",
  "@metamask-previews/build-utils": "1.0.0-preview.3fbb6b41",
  "@metamask-previews/composable-controller": "4.0.0-preview.3fbb6b41",
  "@metamask-previews/controller-utils": "6.1.0-preview.3fbb6b41",
  "@metamask-previews/ens-controller": "7.0.0-preview.3fbb6b41",
  "@metamask-previews/eth-json-rpc-provider": "2.3.0-preview.3fbb6b41",
  "@metamask-previews/gas-fee-controller": "11.0.0-preview.3fbb6b41",
  "@metamask-previews/json-rpc-engine": "7.3.0-preview.3fbb6b41",
  "@metamask-previews/json-rpc-middleware-stream": "6.0.0-preview.3fbb6b41",
  "@metamask-previews/keyring-controller": "10.0.0-preview.3fbb6b41",
  "@metamask-previews/logging-controller": "2.0.0-preview.3fbb6b41",
  "@metamask-previews/message-manager": "7.3.6-preview.3fbb6b41",
  "@metamask-previews/name-controller": "4.0.0-preview.3fbb6b41",
  "@metamask-previews/network-controller": "17.0.0-preview.3fbb6b41",
  "@metamask-previews/notification-controller": "4.0.0-preview.3fbb6b41",
  "@metamask-previews/permission-controller": "6.0.0-preview.3fbb6b41",
  "@metamask-previews/permission-log-controller": "0.0.0-preview.3fbb6b41",
  "@metamask-previews/phishing-controller": "8.0.0-preview.3fbb6b41",
  "@metamask-previews/polling-controller": "2.0.0-preview.3fbb6b41",
  "@metamask-previews/preferences-controller": "5.0.0-preview.3fbb6b41",
  "@metamask-previews/queued-request-controller": "0.2.0-preview.3fbb6b41",
  "@metamask-previews/rate-limit-controller": "4.0.0-preview.3fbb6b41",
  "@metamask-previews/selected-network-controller": "5.0.0-preview.3fbb6b41",
  "@metamask-previews/signature-controller": "8.0.0-preview.3fbb6b41",
  "@metamask-previews/transaction-controller": "18.3.0-preview.3fbb6b41"
}

@jiexi jiexi merged commit 64d6166 into main Dec 6, 2023
132 checks passed
@jiexi jiexi deleted the jl/mmp-1713/update-networkWillChange-networkDidChange-usage branch December 6, 2023 00:41
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