feat(snaps-rpc-methods)!: update usage of withKeyring to withKeyringV2Unsafe#4009
Open
hmalik88 wants to merge 5 commits into
Open
feat(snaps-rpc-methods)!: update usage of withKeyring to withKeyringV2Unsafe#4009hmalik88 wants to merge 5 commits into
withKeyring to withKeyringV2Unsafe#4009hmalik88 wants to merge 5 commits into
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4009 +/- ##
=======================================
Coverage 98.58% 98.58%
=======================================
Files 425 425
Lines 12370 12370
Branches 1949 1949
=======================================
Hits 12195 12195
Misses 175 175 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
|
||
| ### Changed | ||
|
|
||
| - Update `withKeyring` to `withKeyringV2Unsafe` ([#4009](https://github.com/MetaMask/snaps/pull/4009)) |
Member
There was a problem hiding this comment.
Suggested change
| - Update `withKeyring` to `withKeyringV2Unsafe` ([#4009](https://github.com/MetaMask/snaps/pull/4009)) | |
| - Use `withKeyringV2Unsafe` for accessing entropy ([#4009](https://github.com/MetaMask/snaps/pull/4009)) |
|
|
||
| ### Changed | ||
|
|
||
| - Update `withKeyring` to `withKeyringV2Unsafe` ([#4009](https://github.com/MetaMask/snaps/pull/4009)) |
Member
There was a problem hiding this comment.
Suggested change
| - Update `withKeyring` to `withKeyringV2Unsafe` ([#4009](https://github.com/MetaMask/snaps/pull/4009)) | |
| - **BREAKING:** Use `withKeyringV2Unsafe` for accessing entropy ([#4009](https://github.com/MetaMask/snaps/pull/4009)) |
withKeyring to withKeyringV2withKeyring to withKeyringV2Unsafe
| }; | ||
|
|
||
| // Expecting a bound function that calls KeyringController.withKeyring selecting the Snap keyring | ||
| // Expecting a bound function that calls KeyringController.withKeyringV2Unsafe selecting the Snap keyring |
Member
There was a problem hiding this comment.
I would just revert this
| 'KeyringController:withKeyring', | ||
| 'KeyringController:withKeyringV2Unsafe', | ||
| { | ||
| type: HD_KEYRING, |
Member
There was a problem hiding this comment.
Looks like type was changed, we use that for filtering, but also assert it below.
Please adjust tests as well.
Member
There was a problem hiding this comment.
Though it seems that KeyringController expects a V1 type for filtering but then returns V2 when using withKeyringV2. Unsure if that is a bug that needs fixing first?
withKeyring to withKeyringV2UnsafewithKeyring to withKeyringV2Unsafe
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR updates usage of
withKeyringto the newwithKeyringV2. The plan is to convertwithKeyringV2to bewithKeyringonce usage across the codebase has been converted to v2.Note
Medium Risk
Touches messenger paths that read HD keyring mnemonic/seed for Snap entropy RPCs; logic is unchanged but clients must expose the new action or entropy methods will fail at runtime.
Overview
This PR replaces all
KeyringController:withKeyringmessenger usage withKeyringController:withKeyringV2Unsafeas part of the broader KeyringController v2 migration.In
@metamask/snaps-rpc-methods, the shared type is renamed toKeyringControllerWithKeyringV2UnsafeAction, and entropy-related restricted methods (snap_getEntropy,snap_getBip32*,snap_getBip44Entropy) declare and call the new action.getMnemonic/getMnemonicSeedinutils.tsnow invokewithKeyringV2Unsafefor primary and alternate entropy sources. Tests and Unreleased changelog entries are updated accordingly.@metamask/snaps-simulationregisters a mock handler forwithKeyringV2Unsafeinstead ofwithKeyring.snaps-controllersonly updates a comment onWithSnapKeyringFunctionto document the v2 unsafe API.Behavior is intended to stay the same; this is primarily an integration rename so Snaps align with the new KeyringController surface before
withKeyringV2becomes the defaultwithKeyring.Reviewed by Cursor Bugbot for commit 408113b. Bugbot is set up for automated code reviews on this repo. Configure here.