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

chore: bump @metamask/{keyring, profile-sync}-controller #30637

Merged
merged 22 commits into from
Mar 7, 2025

Conversation

mikesposito
Copy link
Member

@mikesposito mikesposito commented Feb 28, 2025

Description

These packages are being bumped to their latest version:

@metamask/keyring-controller

  • BREAKING: addNewKeyring method now returns Promise<KeyringMetadata> instead of Promise<unknown> (#5372)
    • Consumers can use the returned KeyringMetadata.id to access the created keyring instance via withKeyring.
  • BREAKING: withKeyring method now requires a callback argument of type ({ keyring: SelectedKeyring; metadata: KeyringMetadata }) => Promise<CallbackResult> (#5372)
  • Bump @metamask/keyring-internal-api from ^4.0.3 to ^5.0.0 (#5405)
  • Bump @metamask/eth-hd-keyring from ^10.0.0 to ^11.0.0 (#5405)
  • Bump @metamask/eth-simple-keyring from ^8.1.0 to ^9.0.0 (#5405)

@metamask/profile-sync-controller

  • Bump @metamask/keyring-internal-api from ^4.0.3 to ^5.0.0 (#5405)

@metamask/eth-ledger-bridge-keyring

  • BREAKING: LedgerKeyring now implements the Keyring type (#194)
    • The class does not extend EventEmitter anymore.
    • The LedgerKeyring.accounts class variable is now a readonly Hex[] array.
    • The addAccounts method signature has been changed:
      • An amount number parameter is now required to specify the number of accounts to add.
      • The method now returns a promise resolving to an array of Hex addresses.
    • The unlock method now returns Promise<Hex>.
    • The getAccounts method now returns Promise<Hex[]>.
    • The deserialize method now requires a LedgerKeyringSerializedState typed parameter.
    • The signTransaction method now accepts an Hex typed value as the address parameter.
    • The signMessage method now accepts an Hex typed value as the withAccount parameter.
    • The signPersonalMessage method now accepts an Hex typed value as the withAccount parameter.
    • The signTypedData method now accepts an Hex typed value as the withAccount parameter.
    • The unlockAccountByAddress method now accepts an Hex typed value as the address parameter.

Open in GitHub Codespaces

Related issues

Fixes:

Manual testing steps

  1. Go to this page...

Screenshots/Recordings

Before

After

Pre-merge author checklist

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.

@mikesposito

This comment was marked as outdated.

Copy link

socket-security bot commented Feb 28, 2025

New and updated dependencies detected. Learn more about Socket for GitHub ↗︎

Package New capabilities Transitives Size Publisher
npm/@metamask/[email protected]11.0.0 None 0 143 kB metamaskbot
npm/@metamask/[email protected]9.0.0 None +1 454 kB metamaskbot
npm/@metamask/[email protected]9.0.0 None 0 79 kB metamaskbot
npm/@metamask/[email protected]6.1.1 None 0 165 kB metamaskbot
npm/@metamask/[email protected]20.0.0 None +1 67.3 kB
npm/@metamask/[email protected]9.0.0 None 0 0 B

View full report↗︎

Copy link

socket-security bot commented Feb 28, 2025

👍 Dependency issues cleared. Learn more about Socket for GitHub ↗︎

This PR previously contained dependency changes with security issues that have been resolved, removed, or ignored.

View full report↗︎

@metamaskbot
Copy link
Collaborator

Policies updated.
👀 Please review the diff for suspicious new powers.

🧠 Learn how: https://lavamoat.github.io/guides/policy-diff/#what-to-look-for-when-reviewing-a-policy-diff

@mikesposito mikesposito force-pushed the mikesposito/bump-keyring-controller branch from 4c87f28 to c4a8c9d Compare February 28, 2025 13:34
@mikesposito
Copy link
Member Author

@metamaskbot update-policies

@metamaskbot
Copy link
Collaborator

Policies updated.
👀 Please review the diff for suspicious new powers.

🧠 Learn how: https://lavamoat.github.io/guides/policy-diff/#what-to-look-for-when-reviewing-a-policy-diff

@mikesposito mikesposito changed the title chore: bump @metamask/keyring-controller chore: bump @metamask/{keyring, profile-sync}-controller Mar 3, 2025
@mikesposito
Copy link
Member Author

@SocketSecurity ignore npm/[email protected]

mikesposito added a commit to MetaMask/core that referenced this pull request Mar 3, 2025
## Explanation

<!--
Thanks for your contribution! Take a moment to answer these questions so
that reviewers have the information they need to properly understand
your changes:

* What is the current state of things and why does it need to change?
* What is the solution your changes offer and how does it work?
* Are there any changes whose purpose might not obvious to those
unfamiliar with the domain?
* If your primary goal was to update one package but you found you had
to update another one along the way, why did you do so?
* If you had to upgrade a dependency, why did you do so?
-->

This PR bumps these packages across the core controllers:
- `@metamask/eth-simple-keyring`
- `@metamask/eth-hd-keyring`
- `@metamask/keyring-internal-api`

The package is being updated on Extension by this PR:
MetaMask/metamask-extension#30637

## References

<!--
Are there any issues that this pull request is tied to?
Are there other links that reviewers should consult to understand these
changes better?
Are there client or consumer pull requests to adopt any breaking
changes?

For example:

* Fixes #12345
* Related to #67890
-->

## Changelog

<!--
If you're making any consumer-facing changes, list those changes here as
if you were updating a changelog, using the template below as a guide.

(CATEGORY is one of BREAKING, ADDED, CHANGED, DEPRECATED, REMOVED, or
FIXED. For security-related issues, follow the Security Advisory
process.)

Please take care to name the exact pieces of the API you've added or
changed (e.g. types, interfaces, functions, or methods).

If there are any breaking changes, make sure to offer a solution for
consumers to follow once they upgrade to the changes.

Finally, if you're only making changes to development scripts or tests,
you may replace the template below with "None".
-->

### `@metamask/keyring-controller`

- **CHANGED**: Bump `@metamask/eth-simple-keyring` from `^8.1.0` to
`^9.0.0` ([#5405](#5405))
- **CHANGED**: Bump `@metamask/eth-hd-keyring` from `^10.0.0` to
`^11.0.0` ([#5405](#5405))
- **CHANGED**: Bump `@metamask/keyring-internal-api` from `^4.0.3` to
`^5.0.0` ([#5405](#5405))

### `@metamask/accounts-controller`

- **CHANGED**: Bump `@metamask/keyring-internal-api` from `^4.0.3` to
`^5.0.0` ([#5405](#5405))

### `@metamask/profile-sync-controller`

- **CHANGED**: Bump `@metamask/keyring-internal-api` from `^4.0.3` to
`^5.0.0` ([#5405](#5405))

### `@metamask/assets-controller`

- **CHANGED**: Bump `@metamask/keyring-internal-api` from `^4.0.3` to
`^5.0.0` ([#5405](#5405))

### `@metamask/multichain-transaction-controller`

- **CHANGED**: Bump `@metamask/keyring-internal-api` from `^4.0.3` to
`^5.0.0` ([#5405](#5405))

## 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
@mikesposito
Copy link
Member Author

@metamaskbot update-policies

@mikesposito mikesposito requested review from a team as code owners March 5, 2025 09:47
mathieuartu
mathieuartu previously approved these changes Mar 5, 2025
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.

Looking good! Thanks for doing that, was following closely this PR as you know 😛

ccharly
ccharly previously approved these changes Mar 5, 2025
Copy link
Contributor

@ccharly ccharly left a comment

Choose a reason for hiding this comment

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

Code LGTM (not manually tested).

@mikesposito mikesposito dismissed stale reviews from ccharly and mathieuartu via 70e7886 March 5, 2025 13:57
@metamaskbot
Copy link
Collaborator

Builds ready [3aad368]
Page Load Metrics (1543 ± 44 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint43218481487258124
domContentLoaded1416178615208440
load1435184615439144
domInteractive24113342110
backgroundConnect106430189
firstReactRender1467282110
getState45113157
initialActions00000
loadScripts1022137711157737
setupStore75313136
uiStartup16122084174110249
Bundle size diffs [🚀 Bundle size reduced!]
  • background: -1.77 KiB (-0.03%)
  • ui: 0 Bytes (0.00%)
  • common: 796 Bytes (0.01%)

mathieuartu added a commit to MetaMask/core that referenced this pull request Mar 5, 2025
## Explanation

This PR makes the necessary changes so that `UserStorageController` and
`AuthenticationController` consumes the profile sync SDK.
That means that there's no longer controller specific logic nor services
related to authentication and user storage.
Test coverages has also been increased to almost 100% for all things
related to authentication and user storage, for controller & SDK.

## References

Related to: https://consensyssoftware.atlassian.net/browse/IDENTITY-48
Extension test drive PR:
MetaMask/metamask-extension#30681

Note that when bumping the version on the extension and mobile, some
changes will be mandatory:
- Updating the mock storage key for both identity and notifications E2E
tests constants
- Updating `sessionData` shapes everywhere
- Add a migration to manage `sessionData` shape changes so that it is
reset for users still using the previous shape
- Bumping `KeyringController` to `^19.2.1` if not done already, and take
care of the breaking changes this version adds on extension
- This will most likely be fixed with
MetaMask/metamask-extension#30637

## Changelog

<!--
If you're making any consumer-facing changes, list those changes here as
if you were updating a changelog, using the template below as a guide.

(CATEGORY is one of BREAKING, ADDED, CHANGED, DEPRECATED, REMOVED, or
FIXED. For security-related issues, follow the Security Advisory
process.)

Please take care to name the exact pieces of the API you've added or
changed (e.g. types, interfaces, functions, or methods).

If there are any breaking changes, make sure to offer a solution for
consumers to follow once they upgrade to the changes.

Finally, if you're only making changes to development scripts or tests,
you may replace the template below with "None".
-->

### `@metamask/profile-sync-controller`

- **CHANGED**: `UserStorageController` and `AuthenticationController`
now use the SDK under the hood.
- **CHANGED**(**BREAKING**): `AuthenticationController` state entry
`sessionData` has changed shape to fully reflect the `LoginResponse` SDK
type.
- **CHANGED**(**BREAKING**): `UserStorageController` cannot use the
`'AuthenticationController:performSignOut'` action anymore.

### `@metamask/notification-services-controller`

- **CHANGED**: Change import for mock access token (only related to
tests)

## Checklist

- [x] I've updated the test suite for new or updated code as appropriate
- [x] I've updated documentation (JSDoc, Markdown, etc.) for new or
updated code as appropriate
- [x] I've highlighted breaking changes using the "BREAKING" category
above as appropriate
- [x] I've prepared draft pull requests for clients and consumer
packages to resolve any breaking changes
@vivek-consensys
Copy link

LGTM!

@mikesposito mikesposito added this pull request to the merge queue Mar 6, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Mar 6, 2025
@mikesposito mikesposito added this pull request to the merge queue Mar 6, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Mar 6, 2025
@mikesposito mikesposito enabled auto-merge March 7, 2025 10:08
@mikesposito mikesposito added this pull request to the merge queue Mar 7, 2025
@metamaskbot
Copy link
Collaborator

Builds ready [ea84681]
Page Load Metrics (1718 ± 56 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint15142001171811957
domContentLoaded15041973168111656
load15162001171811756
domInteractive2595432110
backgroundConnect1290442211
firstReactRender1468312110
getState66315168
initialActions01000
loadScripts11091506125610048
setupStore75514147
uiStartup16942203193913263
Bundle size diffs [🚀 Bundle size reduced!]
  • background: -1.77 KiB (-0.03%)
  • ui: 0 Bytes (0.00%)
  • common: 796 Bytes (0.01%)

Merged via the queue into main with commit 74d4186 Mar 7, 2025
74 checks passed
@mikesposito mikesposito deleted the mikesposito/bump-keyring-controller branch March 7, 2025 11:15
@github-actions github-actions bot locked and limited conversation to collaborators Mar 7, 2025
@metamaskbot metamaskbot added the release-12.15.0 Issue or pull request that will be included in release 12.15.0 label Mar 7, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
QA Passed release-12.15.0 Issue or pull request that will be included in release 12.15.0 team-wallet-framework
Projects
Archived in project
6 participants