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

feat: add NetworkController NetworkConfiguration actions and events #4698

Merged
merged 13 commits into from
Nov 29, 2024

Conversation

Prithpal-Sooriya
Copy link
Contributor

@Prithpal-Sooriya Prithpal-Sooriya commented Sep 13, 2024

Explanation

This adds and exposes some new actions and events inside the network controller.

Events:

  • NetworkController:networkRemoved

Actions

  • NetworkController:updateNetwork
  • NetworkController:addNetwork
  • NetworkController:removeNetwork

These will be used for the network syncing feature (see references)

References

https://consensyssoftware.atlassian.net/browse/NOTIFY-1089

Changelog

@metamask/network-controller

  • ADDED: (BREAKING) add new event NetworkController:networkRemoved
  • ADDED: (BREAKING) add new actions NetworkController:addNetwork; NetworkController:removeNetwork; NetworkController:updateNetwork

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

@Prithpal-Sooriya Prithpal-Sooriya added the team-notifications Notification Team changes. https://github.com/orgs/MetaMask/teams/notifications label Sep 13, 2024
@Prithpal-Sooriya Prithpal-Sooriya force-pushed the NOTIFY-1089/add-network-events-actions branch from 5c85bb9 to 2226588 Compare September 13, 2024 14:01
actions and events for adding, updating, and removing a network configuration
@Prithpal-Sooriya Prithpal-Sooriya force-pushed the NOTIFY-1089/add-network-events-actions branch from 75361eb to 27c18aa Compare November 15, 2024 14:41
@Prithpal-Sooriya Prithpal-Sooriya marked this pull request as ready for review November 15, 2024 14:44
@Prithpal-Sooriya Prithpal-Sooriya requested review from a team as code owners November 15, 2024 14:44
mathieuartu
mathieuartu previously approved these changes Nov 20, 2024
Copy link
Contributor

@mathieuartu mathieuartu left a comment

Choose a reason for hiding this comment

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

LGTM!!

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.

One missing test, but otherwise this looks good to me.

@mcmire
Copy link
Contributor

mcmire commented Nov 27, 2024

@Prithpal-Sooriya The addition of the actions/events will be breaking changes to the NetworkController (since clients will need to amend the allowlists they pass to the NetworkController messenger). That's fine, but can you update the PR description to reflect this? Typically we just prepend the changelog entries that refer to breaking changes with **BREAKING:**. This way we know to bump network-controller by a major when the time comes to release these changes.

@Prithpal-Sooriya
Copy link
Contributor Author

Prithpal-Sooriya commented Nov 27, 2024

@Prithpal-Sooriya The addition of the actions/events will be breaking changes to the NetworkController (since clients will need to amend the allowlists they pass to the NetworkController messenger). That's fine, but can you update the PR description to reflect this? Typically we just prepend the changelog entries that refer to breaking changes with **BREAKING:**. This way we know to bump network-controller by a major when the time comes to release these changes.

@mcmire Done.

Do additions (new actions/events) count as breaking? Wouldn't clients/consumers of this controller need to update their allowedActions/events only if they do call/subscribe to them

@mcmire
Copy link
Contributor

mcmire commented Nov 27, 2024

@Prithpal-Sooriya Good point! If the controller calls new actions or subscribes to new events outside of itself, then those actions/events would need to be added to the controller messenger's allowlists, and the client would have to do that. But in this case since the actions/events being added are internal to NetworkController, you are right, it wouldn't cause anything to break immediately. So maybe it's inappropriate to mark these changes as breaking. Sorry for the confusion.

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.

Sorry one more thing!

mathieuartu
mathieuartu previously approved these changes Nov 28, 2024
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.

Looks good!

@Prithpal-Sooriya Prithpal-Sooriya enabled auto-merge (squash) November 29, 2024 10:54
@Prithpal-Sooriya Prithpal-Sooriya merged commit 9857fad into main Nov 29, 2024
123 checks passed
@Prithpal-Sooriya Prithpal-Sooriya deleted the NOTIFY-1089/add-network-events-actions branch November 29, 2024 10:58
@Gudahtt
Copy link
Member

Gudahtt commented Dec 2, 2024

These changes aren't breaking, right? There are no new allowed actions/events here. No restricted messenger changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
team-notifications Notification Team changes. https://github.com/orgs/MetaMask/teams/notifications
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants