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: remove global network from signature controller #4797

Merged
merged 9 commits into from
Oct 24, 2024
Merged

feat: remove global network from signature controller #4797

merged 9 commits into from
Oct 24, 2024
+131 −59

Conversation

matthewwalsh0
Copy link
Member

@matthewwalsh0 matthewwalsh0 commented Oct 15, 2024 •
edited
Loading

Explanation

Remove all global network usage from the signature controller to support multi-chain in the clients.

Specifically:

  • Remove getCurrentChainId constructor callback from SignatureController.
  • Determine chainId using networkClientId from request argument and NetworkController messenger action.
  • Persist chainId and networkClientId in state.

References

Fixes #3498

Extension Adoption
Mobile Adoption

Changelog

@metamask/signature-controller

  • BREAKING: Make request argument required in newUnsignedPersonalMessage and newUnsignedTypedMessage methods.
  • ADDED: Add chainId and networkClientId to SignatureRequest and LegacyMessage types.
  • CHANGED: Throw if no networkClientId provided or if chainId cannot be determined.
  • REMOVED: Remove getCurrentChainId and getAllState callbacks from constructor options.

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
  • I've prepared draft pull requests for clients and consumer packages to resolve any breaking changes

@matthewwalsh0 matthewwalsh0 requested review from a team as code owners October 15, 2024 20:47
@matthewwalsh0
Copy link
Member Author

@metamaskbot publish-preview

@github-actions GitHub Actions
Copy link
Contributor

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": "18.2.2-preview-2c5a4f93",
  "@metamask-previews/address-book-controller": "6.0.1-preview-2c5a4f93",
  "@metamask-previews/announcement-controller": "7.0.1-preview-2c5a4f93",
  "@metamask-previews/approval-controller": "7.1.0-preview-2c5a4f93",
  "@metamask-previews/assets-controllers": "38.3.0-preview-2c5a4f93",
  "@metamask-previews/base-controller": "7.0.1-preview-2c5a4f93",
  "@metamask-previews/build-utils": "3.0.1-preview-2c5a4f93",
  "@metamask-previews/chain-controller": "0.1.3-preview-2c5a4f93",
  "@metamask-previews/composable-controller": "9.0.1-preview-2c5a4f93",
  "@metamask-previews/controller-utils": "11.3.0-preview-2c5a4f93",
  "@metamask-previews/ens-controller": "14.0.1-preview-2c5a4f93",
  "@metamask-previews/eth-json-rpc-provider": "4.1.4-preview-2c5a4f93",
  "@metamask-previews/gas-fee-controller": "20.0.1-preview-2c5a4f93",
  "@metamask-previews/json-rpc-engine": "9.0.3-preview-2c5a4f93",
  "@metamask-previews/json-rpc-middleware-stream": "8.0.3-preview-2c5a4f93",
  "@metamask-previews/keyring-controller": "17.2.2-preview-2c5a4f93",
  "@metamask-previews/logging-controller": "6.0.1-preview-2c5a4f93",
  "@metamask-previews/message-manager": "10.1.1-preview-2c5a4f93",
  "@metamask-previews/name-controller": "8.0.1-preview-2c5a4f93",
  "@metamask-previews/network-controller": "21.0.1-preview-2c5a4f93",
  "@metamask-previews/notification-controller": "7.0.0-preview-2c5a4f93",
  "@metamask-previews/notification-services-controller": "0.9.0-preview-2c5a4f93",
  "@metamask-previews/permission-controller": "11.0.2-preview-2c5a4f93",
  "@metamask-previews/permission-log-controller": "3.0.1-preview-2c5a4f93",
  "@metamask-previews/phishing-controller": "12.0.3-preview-2c5a4f93",
  "@metamask-previews/polling-controller": "10.0.1-preview-2c5a4f93",
  "@metamask-previews/preferences-controller": "13.0.3-preview-2c5a4f93",
  "@metamask-previews/profile-sync-controller": "0.9.7-preview-2c5a4f93",
  "@metamask-previews/queued-request-controller": "5.1.0-preview-2c5a4f93",
  "@metamask-previews/rate-limit-controller": "6.0.1-preview-2c5a4f93",
  "@metamask-previews/selected-network-controller": "18.0.1-preview-2c5a4f93",
  "@metamask-previews/signature-controller": "19.1.0-preview-2c5a4f93",
  "@metamask-previews/transaction-controller": "37.2.0-preview-2c5a4f93",
  "@metamask-previews/user-operation-controller": "15.0.1-preview-2c5a4f93"
}

Base automatically changed from refactor/signature-controller-remove-message-managers to main October 16, 2024 13:15
@matthewwalsh0 matthewwalsh0 force-pushed the feat/signature-controller-no-global-chain-id branch from 2c5a4f9 to 6d497e3 Compare October 17, 2024 21:56
@matthewwalsh0
Copy link
Member Author

@metamaskbot publish-preview

@github-actions GitHub Actions
Copy link
Contributor

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": "18.2.2-preview-1800a170",
  "@metamask-previews/address-book-controller": "6.0.1-preview-1800a170",
  "@metamask-previews/announcement-controller": "7.0.1-preview-1800a170",
  "@metamask-previews/approval-controller": "7.1.0-preview-1800a170",
  "@metamask-previews/assets-controllers": "39.0.0-preview-1800a170",
  "@metamask-previews/base-controller": "7.0.1-preview-1800a170",
  "@metamask-previews/build-utils": "3.0.1-preview-1800a170",
  "@metamask-previews/chain-controller": "0.1.3-preview-1800a170",
  "@metamask-previews/composable-controller": "9.0.1-preview-1800a170",
  "@metamask-previews/controller-utils": "11.3.0-preview-1800a170",
  "@metamask-previews/ens-controller": "14.0.1-preview-1800a170",
  "@metamask-previews/eth-json-rpc-provider": "4.1.5-preview-1800a170",
  "@metamask-previews/gas-fee-controller": "21.0.0-preview-1800a170",
  "@metamask-previews/json-rpc-engine": "10.0.0-preview-1800a170",
  "@metamask-previews/json-rpc-middleware-stream": "8.0.4-preview-1800a170",
  "@metamask-previews/keyring-controller": "17.3.0-preview-1800a170",
  "@metamask-previews/logging-controller": "6.0.1-preview-1800a170",
  "@metamask-previews/message-manager": "11.0.0-preview-1800a170",
  "@metamask-previews/multichain": "0.0.0-preview-1800a170",
  "@metamask-previews/name-controller": "8.0.1-preview-1800a170",
  "@metamask-previews/network-controller": "21.1.0-preview-1800a170",
  "@metamask-previews/notification-controller": "7.0.0-preview-1800a170",
  "@metamask-previews/notification-services-controller": "0.11.0-preview-1800a170",
  "@metamask-previews/permission-controller": "11.0.2-preview-1800a170",
  "@metamask-previews/permission-log-controller": "3.0.1-preview-1800a170",
  "@metamask-previews/phishing-controller": "12.0.3-preview-1800a170",
  "@metamask-previews/polling-controller": "11.0.0-preview-1800a170",
  "@metamask-previews/preferences-controller": "13.1.0-preview-1800a170",
  "@metamask-previews/profile-sync-controller": "0.9.7-preview-1800a170",
  "@metamask-previews/queued-request-controller": "5.1.0-preview-1800a170",
  "@metamask-previews/rate-limit-controller": "6.0.1-preview-1800a170",
  "@metamask-previews/selected-network-controller": "18.0.2-preview-1800a170",
  "@metamask-previews/signature-controller": "20.1.0-preview-1800a170",
  "@metamask-previews/transaction-controller": "37.3.0-preview-1800a170",
  "@metamask-previews/user-operation-controller": "16.0.0-preview-1800a170"
}

cryptodev-2s
cryptodev-2s previously approved these changes Oct 23, 2024
View reviewed changes
Copy link
Contributor

@cryptodev-2s cryptodev-2s left a comment

Choose a reason for hiding this comment

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

LGTM!

@matthewwalsh0
Copy link
Member Author

@metamaskbot publish-preview

@github-actions GitHub Actions
Copy link
Contributor

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": "18.2.2-preview-c77aebdb",
  "@metamask-previews/address-book-controller": "6.0.1-preview-c77aebdb",
  "@metamask-previews/announcement-controller": "7.0.1-preview-c77aebdb",
  "@metamask-previews/approval-controller": "7.1.0-preview-c77aebdb",
  "@metamask-previews/assets-controllers": "39.0.0-preview-c77aebdb",
  "@metamask-previews/base-controller": "7.0.1-preview-c77aebdb",
  "@metamask-previews/build-utils": "3.0.1-preview-c77aebdb",
  "@metamask-previews/chain-controller": "0.1.3-preview-c77aebdb",
  "@metamask-previews/composable-controller": "9.0.1-preview-c77aebdb",
  "@metamask-previews/controller-utils": "11.3.0-preview-c77aebdb",
  "@metamask-previews/ens-controller": "14.0.1-preview-c77aebdb",
  "@metamask-previews/eth-json-rpc-provider": "4.1.5-preview-c77aebdb",
  "@metamask-previews/gas-fee-controller": "21.0.0-preview-c77aebdb",
  "@metamask-previews/json-rpc-engine": "10.0.0-preview-c77aebdb",
  "@metamask-previews/json-rpc-middleware-stream": "8.0.4-preview-c77aebdb",
  "@metamask-previews/keyring-controller": "17.3.0-preview-c77aebdb",
  "@metamask-previews/logging-controller": "6.0.1-preview-c77aebdb",
  "@metamask-previews/message-manager": "11.0.0-preview-c77aebdb",
  "@metamask-previews/multichain": "0.0.0-preview-c77aebdb",
  "@metamask-previews/name-controller": "8.0.1-preview-c77aebdb",
  "@metamask-previews/network-controller": "21.1.0-preview-c77aebdb",
  "@metamask-previews/notification-controller": "7.0.0-preview-c77aebdb",
  "@metamask-previews/notification-services-controller": "0.11.0-preview-c77aebdb",
  "@metamask-previews/permission-controller": "11.0.2-preview-c77aebdb",
  "@metamask-previews/permission-log-controller": "3.0.1-preview-c77aebdb",
  "@metamask-previews/phishing-controller": "12.0.3-preview-c77aebdb",
  "@metamask-previews/polling-controller": "11.0.0-preview-c77aebdb",
  "@metamask-previews/preferences-controller": "13.1.0-preview-c77aebdb",
  "@metamask-previews/profile-sync-controller": "0.9.7-preview-c77aebdb",
  "@metamask-previews/queued-request-controller": "5.1.0-preview-c77aebdb",
  "@metamask-previews/rate-limit-controller": "6.0.1-preview-c77aebdb",
  "@metamask-previews/selected-network-controller": "18.0.2-preview-c77aebdb",
  "@metamask-previews/signature-controller": "20.1.0-preview-c77aebdb",
  "@metamask-previews/transaction-controller": "37.3.0-preview-c77aebdb",
  "@metamask-previews/user-operation-controller": "16.0.0-preview-c77aebdb"
}

jpuri
jpuri approved these changes Oct 24, 2024
View reviewed changes
Copy link
Contributor

@jpuri jpuri 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 👍

@matthewwalsh0 matthewwalsh0 merged commit 63c38b7 into main Oct 24, 2024
119 checks passed
@matthewwalsh0 matthewwalsh0 deleted the feat/signature-controller-no-global-chain-id branch October 24, 2024 16:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jpuri jpuri jpuri approved these changes

@cryptodev-2s cryptodev-2s cryptodev-2s left review comments

Assignees
No one assigned
Labels
team-confirmations
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@matthewwalsh0 @jpuri @cryptodev-2s