chore: Update `@metamask/keyring-controller` to v16

[Gudahtt]

Description

The package @metamask/keyring-controller has been updated from v13 to v16. See here for the changelog: https://github.com/MetaMask/core/blob/%40metamask/keyring-controller%4016.0.0/packages/keyring-controller/CHANGELOG.md#1600

Breaking KeyringController changes

Most of the changes did not require any code changes in mobile. The breaking changes in v14 were mainly type changes that didn't break anything for us, and the changes in v15 only impacted the extension (it changed the meaning of the optional accountCount parameter to addNewAccount, which mobile doesn't use).

The main breaking change is that as of v16, a number of methods no longer return a copy of the KeyringController state. We were never relying on this return value, but we had to adjust how a few of these methods were called or mocked to accommodate this change.

withKeyring method

v16 of the KeyringController introduced a new method for performing custom keyring operations, called withKeyring. This is meant as an alternative approach to using getKeyringForAccount or getKeyringsByType to get a keyring instance, then using that keyring instance directly.

The withKeyring method helps with getting the keyring instance, and it ensures that the KeyringController is locked while the operation is performed, and it ensures that the KeyringController state is updated when the operation is finished. This gets rid of a bunch of boilerplate in our QR and Ledger integrations, and adds additional protection against race conditions and non-atomic state changes.

It's not strictly required as part of this update, but this was implemented in the same PR because I was concerned that the other changes in v16 might reveal some pre-existing race conditions that using withKeyring would prevent.

Peer dependency warnings

This update introduces three new peerDependencies warnings, each of which I have audited and determined are safe to temporarily ignore.

• warning "@metamask/keyring-controller > @metamask/[email protected]" has incorrect peer dependency "@metamask/providers@>=15 <17".
  • The @metamask/keyring-api package uses types from the @metamask/providers package as part of the dapp client. The dapp client is not used by the KeyringController.
• warning " > @metamask/[email protected]" has incorrect peer dependency "@metamask/keyring-controller@^13.0.0".
  • The @metamask/accounts-controller package uses the KeyringTypes type from @metamask/keyring-controller. This type has not changed between v13 and v16.
• warning " > @metamask/[email protected]" has incorrect peer dependency "@metamask/keyring-controller@^13.0.0".
  • The @metamask/preferences-controller package uses the KeyringController state and stateChange event. These have not changed between v13 and v16.

Related issues

Fixes https://github.com/MetaMask/accounts-planning/issues/441

Manual testing steps

Affected workflows may include:

• Lock
• Unlock
• Upgrade app
• Add account
• Remove account
• Connect hardware wallet
• All hardware wallet interactions

Regression testing in these areas would be recommended. I would also advise doing some regression testing in these areas, then restart your device, then unlock it again (to ensure the keyring data was correctly persisted).

Screenshots/Recordings

Before

N/A

After

TODO

Pre-merge author checklist

• [x] I’ve followed MetaMask Coding Standards.
• [ ] I've completed the PR template to the best of my ability
• [x] I’ve included tests if applicable
• [x] I’ve documented my code using JSDoc format if applicable
• [x] I’ve applied the right labels on the PR (see labeling guidelines). Not required for external contributors.

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.

[socket-security[bot]]

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

Package	New capabilities	Transitives	Size	Publisher
npm/@metamask/[email protected]	Transitive: filesystem, network	+31	5.37 MB	metamaskbot

View full report↗︎

[socket-security[bot]]

👍 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.

Ignoring: npm/@metamask/[email protected], npm/@metamask/[email protected]

View full report↗︎

Next steps

Take a deeper look at the dependency

Take a moment to review the security alert above. Review the linked package source code to understand the potential risk. Ensure the package is not malicious before proceeding. If you're unsure how to proceed, reach out to your security team or ask the Socket team for help at support [AT] socket [DOT] dev.

Remove the package

If you happen to install a dependency that Socket reports as Known Malware you should immediately remove it and select a different dependency. For other alert types, you may may wish to investigate alternative packages or consider if there are other ways to mitigate the specific risk posed by the dependency.

Mark a package as acceptable risk

To ignore an alert, reply with a comment starting with @SocketSecurity ignore followed by a space separated list of ecosystem/package-name@version specifiers. e.g. @SocketSecurity ignore npm/[email protected] or ignore all packages with @SocketSecurity ignore-all

[Gudahtt]

This is close to done, I think I need to adopt withKeyring before this is completely ready though.

[github-actions[bot]]

Bitrise

❌❌❌ pr_smoke_e2e_pipeline failed on Bitrise! ❌❌❌

Commit hash: a5534c833892d9bc26253d9784dbef8c187003a1 Build link: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/34d6d0b1-a132-4799-96d0-019f3fe60805

[!NOTE]

• You can kick off another pr_smoke_e2e_pipeline on Bitrise by removing and re-applying the Run Smoke E2E label on the pull request

[Gudahtt]

@SocketSecurity ignore npm/@spruceid/[email protected]

New author seems OK, nothing concerning in the recent published packages by this person. They appear to be a member of the same team, an active maintainer.

[gantunesr]

We can get this PR in QA once it's approved @Gudahtt

[github-actions[bot]]

Bitrise

❌❌❌ pr_smoke_e2e_pipeline failed on Bitrise! ❌❌❌

Commit hash: 16831ca4de6fb2a3492e8cf00b739118a4f04714 Build link: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/e836ffc3-1524-4641-9a6c-2e63fa017633

[!NOTE]

• You can kick off another pr_smoke_e2e_pipeline on Bitrise by removing and re-applying the Run Smoke E2E label on the pull request

[github-actions[bot]]

Bitrise

✅✅✅ pr_smoke_e2e_pipeline passed on Bitrise! ✅✅✅

Commit hash: 45e7c22f426e016d4d48589ad57ae7c49bbed059 Build link: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/25726a99-afe0-4c98-b8af-0d3115cd0a99

[!NOTE]

• You can kick off another pr_smoke_e2e_pipeline on Bitrise by removing and re-applying the Run Smoke E2E label on the pull request

[Gudahtt]

Oops, I meant to add a recording before marking this as ready for review. I can't test the hardware wallet stuff, but I can show that I've tested HD account and imported account stuff at least (I have already manually tested that).

I'll fix these conflicts and post that recording later today

[github-actions[bot]]

Bitrise

✅✅✅ pr_smoke_e2e_pipeline passed on Bitrise! ✅✅✅

Commit hash: 58c377ad4690a448d17bbf315f1a1882b6a5ab89 Build link: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/c7717c08-abb3-4503-8f47-6ff6f5e2d389

[!NOTE]

• You can kick off another pr_smoke_e2e_pipeline on Bitrise by removing and re-applying the Run Smoke E2E label on the pull request

[Gudahtt]

Cool to see that mjs is the compiled code from ecmascript files cjs is for common js

Curious question, why it creates a chunk for js and a chunk for mjs?

Our new build tooling ships both formats, CommonJS and ECMAScript modules. Different tools use one or the other - we need CommonJS for compatibility, and we need ECMAScript modules for supporting some newer tools and features (e.g. tree-shaking, to remove unused modules from our bundle)

[plasmacorral]

I've conducted migration tests on the QA build of this branch from the following sources:

• QA build of version 7.12.0 (build 1202), initially believed to be the release-approved version but later identified as RC 1.
• QA build of version 7.20.1 (build 1314).

The testing was performed across four different physical devices:

• iPhone Xs running iOS 17.4.1
• iPhone 13 mini running iOS 15.6.1
• Samsung a515f running Android 12 (security patch dated Jan 1, 2024)
• Pixel 5a running Android 14 (security patch dated Apr 5, 2024)

Using the following hardware wallets:

• LNX running 2.2.3 with Eth App 1.10.4
• Keystone v3 with fw 1.3.6

Observations:

Issues:

1. Ledger Transaction Issue: Encountered an inability to transact using Ledger across all devices. This was tested using a test dApp, as well as attempts to sign into Opensea and create a Safe via defisaver.com. Instead of the expected Looking for device message, an Unexpected error occurred message was displayed. This issue was not present in the QA build of the main commit (80fb30f938b555aed3870857e3432c608872743a), suggesting a potential regression in this build. This is likely a blocker.

2. Private Key Import Error: Attempts to import a private key resulted in a Something went wrong message, although the account did appear in the account list. This issue was consistent across iOS and Android devices. Notably, importing a private key via QR code was successful in the QA build of the main commit (80fb30f938b555aed3870857e3432c608872743a) and with build 1326 from TestFlight, despite a previously reported invalid deeplink error.

3. QR Account Labels Issue: An existing issue with QR account labels was observed, which was not introduced in this build. This issue was likely overlooked within feature QA of PR 9318 and has been reported in a new issue here.

Positive Outcomes:

• Successful migration testing on all 4 devices with HD, Imported, Ledger, and QR accounts, preserving all accounts and custom labels.
• Ability to add and remove Imported and Hardware accounts post-migration.
• Functionality to add HD accounts and rename accounts post-migration was intact.
• Password change, SRP reveal, and private key reveal functionalities worked as expected on all devices.
• Transactions using Keystone for SIWE, Sign permit, and typed data, as well as deployment of ERC20, ERC712, and ERC1155 contracts were successful.
• Post-testing device reboots did not affect the presence of accounts or custom labels upon unlocking.

Recommendation:

For future iterations, incorporating a custom build number could facilitate a broader range of migration path tests, especially considering the current build (1317) limits testing migration from version 7.22.0 (builds >1321), which could affect a significant user base upon release.

[mikesposito]

The import account flow still uses the old return that used to include the mem state: (app/util/address/index.js)

The function importAccountFromPrivateKey:

  const { importedAccountAddress } =
    await KeyringController.importAccountWithStrategy('privateKey', [pkey]);

Should be, instead:

  const importedAccountAddress =
    await KeyringController.importAccountWithStrategy('privateKey', [pkey]);

This would explain the error during private key import

[mikesposito]

Nit (here or another PR): we could make this function to recreate primary accounts with balance "without update" better with withKeyring

[mikesposito]

Ledger Transaction Issue: Encountered an inability to transact using Ledger across all devices.

@plasmacorral could you confirm that the Ledger device connection (to add an account) was successful, thus indicating a problem related to transaction/message signing and not to the general ability to connect to the device?

[plasmacorral]

Ledger Transaction Issue: Encountered an inability to transact using Ledger across all devices.

@plasmacorral could you confirm that the Ledger device connection (to add an account) was successful, thus indicating a problem related to transaction/message signing and not to the general ability to connect to the device?

Yes, I can. Observed no problems in pairing. The issue occurs when looking for the device to sign. To quickly see this on a zero balance address, try a v4 typed data signature with the test dapp.

[mikesposito]

@SocketSecurity ignore npm/@metamask/[email protected]

This is our package

[github-actions[bot]]

Bitrise

✅✅✅ pr_smoke_e2e_pipeline passed on Bitrise! ✅✅✅

Commit hash: ce5ac0244d9e18e6afeaf4c16b66855bcc834a39 Build link: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/806c3b95-07eb-47a4-9ecf-de6935a9fa2e

[!NOTE]

• You can kick off another pr_smoke_e2e_pipeline on Bitrise by removing and re-applying the Run Smoke E2E label on the pull request

[github-actions[bot]]

Bitrise

✅✅✅ pr_smoke_e2e_pipeline passed on Bitrise! ✅✅✅

Commit hash: c5bd3db4ad2f12e03b21f1367c99ca1967b8b5a8 Build link: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/eac30078-0d8d-44b1-a62a-d8181c37704b

[!NOTE]

• You can kick off another pr_smoke_e2e_pipeline on Bitrise by removing and re-applying the Run Smoke E2E label on the pull request

[mikesposito]

I'm unable to test this locally, but 135e572 should fix the issue with Ledger (1) and c5bd3db should fix the error when importing an account (2)

QR Account Labels Issue: An existing issue with QR account labels was observed, which was not introduced in this build.

I assume we want to fix (3) separately instead

[github-actions[bot]]

CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes.

[github-actions[bot]]

Bitrise

✅✅✅ pr_smoke_e2e_pipeline passed on Bitrise! ✅✅✅

Commit hash: 75fbf52aca9d9ea6f39b89264462c9d89707575e Build link: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/bf0fb9c7-2b9d-4674-bbc1-7f915b4bde17

[!NOTE]

• You can kick off another pr_smoke_e2e_pipeline on Bitrise by removing and re-applying the Run Smoke E2E label on the pull request

[vivek-consensys]

@mikesposito @gantunesr I am unable to verify the personal sign using test dapp and Ledger Nano X/Keystone devices on iPhone 13 (iOS 14.3.1), which was tested using the bitrise build. The verify button does not result in any of the recovery results appearing:-

[Gudahtt]

Thanks @vivek-consensys ! It looks like that bug is on main though, not introduced by this PR. It's fixed by this PR: #9627

[vivek-consensys]

@Gudahtt I am facing a few issues:-

• QR accounts are being duplicated when closing the app and reopening
• When changing the password, the hardware accounts are removed.

https://github.com/MetaMask/metamask-mobile/assets/106310394/150084a5-bbfe-40e6-a34c-4be0b113a041

[mikesposito]

QR accounts are being duplicated when closing the app and reopening

Hi @vivek-consensys, looking at your screen recording it looks like QR accounts are not being duplicated during login (app restart). Were they duplicated in another step (e.g. QR Sync/sign), or is it just happening inconsistently?

[Gudahtt]

When changing the password, the hardware accounts are removed.

The latest commit should fix this problem

[vivek-consensys]

When changing the password, the hardware accounts are removed.

The latest commit should fix this problem

This has been fixed using latest build, screen recording shows:

https://github.com/MetaMask/metamask-mobile/assets/106310394/88e90101-caec-4029-a394-470cf50c81bd

[vivek-consensys]

2 issues found:-

1. When updating the account name, then changing password, the updated account name is not reflected

https://github.com/MetaMask/metamask-mobile/assets/106310394/641cdb79-dcf1-47c7-a9c0-b358bffedc47

2. When the user clicks on Add hardware wallet -> QR-based -> Continue -> Only this time (camera permission) -> once the camera opens go back to home and add QR hardware wallet -> The camera won't open with no errors

https://github.com/MetaMask/metamask-mobile/assets/106310394/159bec03-9b1c-4ee0-9c16-c8568f754786

[Gudahtt]

Thanks @vivek-consensys ! Could you verify whether those bugs exist already on main? The connection between that behavior and this PR is not clear to me. We don't change anything close to the hardware wallet connect flow in the UI, and the KeyringController doesn't store those account labels.