fix: Use withKeyring for accessing keyring entropy for Snaps - #40296
Conversation
✨ Files requiring CODEOWNER review ✨🫰 @MetaMask/core-platform (4 files, +130 -117)
📜 @MetaMask/policy-reviewers (12 files, +208 -176)
Tip Follow the policy review process outlined in the LavaMoat Policy Review Process doc before expecting an approval from Policy Reviewers. |
Builds ready [0dba3d3]
⚡ Performance Benchmarks (1372 ± 100 ms)
🌐 Dapp Page Load BenchmarksCurrent Commit: 📄 Localhost MetaMask Test DappSamples: 100 Summary
📈 Detailed Results
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
| /** | ||
| * Gets the mnemonic of the user's primary keyring. | ||
| */ | ||
| getPrimaryKeyringMnemonic() { |
Builds ready [92a9d64]
⚡ Performance Benchmarks (1368 ± 103 ms)
🌐 Dapp Page Load BenchmarksCurrent Commit: 📄 Localhost MetaMask Test DappSamples: 100 Summary
📈 Detailed Results
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
f712bf5 to
e00dd31
Compare
Builds ready [e00dd31]
⚡ Performance Benchmarks (1374 ± 98 ms)
🌐 Dapp Page Load BenchmarksCurrent Commit: 📄 Localhost MetaMask Test DappSamples: 100 Summary
📈 Detailed Results
Bundle size diffs
|
| !hasProperty(keyrings[0], 'seed') || | ||
| !(keyrings[0].seed instanceof Uint8Array) | ||
| ) { | ||
| const { seed } = (await initMessenger.call( |
There was a problem hiding this comment.
Maybe a good opportunity to move this to a shared function.
| index: 0, | ||
| }, | ||
| async ({ keyring }) => ({ | ||
| seed: hasProperty(keyring, 'seed') ? keyring.seed : undefined, |
There was a problem hiding this comment.
Does keyring have a type or something that we could check, instead of hasProperty?
There was a problem hiding this comment.
I think we can just cast. I would use the generic parameter for withKeyring but it isn't available via the messenger.
f131d06 to
4ba2c74
Compare
|
@metamaskbot update-policies |
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
|
Policies updated. 🧠 Learn how: https://lavamoat.github.io/guides/policy-diff/#what-to-look-for-when-reviewing-a-policy-diff 👀 lavamoat/browserify/beta/policy.json changes differ from main/policy.json policy changes |
Co-authored-by: Maarten Zuidhoorn <maarten@zuidhoorn.com>
Mrtenz
left a comment
There was a problem hiding this comment.
Policies & code look good to me.
| } | ||
| }, | ||
| "@metamask/keyring-controller>@metamask/eth-hd-keyring": { | ||
| "@metamask/eth-hd-keyring": { |
There was a problem hiding this comment.
Policy changes are caused by @metamask/eth-hd-keyring now being a direct dependency. No new capabilities introduced.
Builds ready [b8717b6]
⚡ Performance Benchmarks (1353 ± 108 ms)
🌐 Dapp Page Load BenchmarksCurrent Commit: 📄 Localhost MetaMask Test DappSamples: 100 Summary
📈 Detailed Results
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
Description
Use
withKeyringinstead ofgetKeyringsByTypefor accessing the mnemonic for Snaps.withKeyringhas more guarantees that should prevent race conditions andgetKeyringsByTypeis deprecated for that same reason.Additionally, move
getMnemonicandgetMnemonicSeedto utility functions that can be re-used.Changelog
CHANGELOG entry: null
Note
Medium Risk
Touches keyring entropy retrieval paths used by Snaps; incorrect assumptions about keyring shape/types could break Snap initialization or restricted methods despite being a scoped refactor.
Overview
Refactors Snap entropy access to use
KeyringController:withKeyringinstead of the deprecatedgetKeyringsByType, reducing the chance of races when retrieving the primary HD keyring’s mnemonic/seed.The mnemonic/seed lookup logic is centralized into new reusable utilities (
controllers/permissions/snaps/utils.ts) and wired into both Snap permission specifications andSnapControllerinitialization; legacymetamask-controllerhelpers/tests for primary keyring mnemonic access are removed. Dependency/policy updates add a direct@metamask/eth-hd-keyringdependency and adjust LavaMoat policies accordingly.Written by Cursor Bugbot for commit b8717b6. This will update automatically on new commits. Configure here.