Integrate KeyringController v10
#17056
Merged
+447
−500
Conversation
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
Contributor
|
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. |
adonesky1
force-pushed
the
integrate-keyringcontroller-v9
branch
5 times, most recently
from
45bad6b to
85cde3b
Compare
adonesky1
force-pushed
the
integrate-keyringcontroller-v9
branch
from
1790b5e to
7bf6ca2
Compare
Collaborator
adonesky1
commented
Jan 6, 2023
| const recoveredIndices = Array.from( | ||
| new Uint16Array(new Uint8Array(keyring.mnemonic).buffer), | ||
| ); | ||
| return recoveredIndices.map((i) => englishWordlist[i]).join(' '); |
Contributor
Author
There was a problem hiding this comment.
getPrimaryKeyringMnemonic is used by Snaps which still passes mnemonics around as strings. They will be refactoring to pass mnemonics as Uint8Arrays but until then we must recover and return the string version from the Uint8Array format that is now on the keyring value returned by the KeyringController as of this latest version bump.
adonesky1
commented
Jan 6, 2023
app/scripts/metamask-controller.js
Outdated
| throw new Error('Primary keyring mnemonic unavailable.'); | ||
| } | ||
| return keyring.mnemonic; | ||
| return uint8ArrayMnemonicToString(keyring.mnemonic); |
Contributor
Author
There was a problem hiding this comment.
getPrimaryKeyringMnemonic is used by Snaps which still passes mnemonics around as strings. They will be refactoring to pass mnemonics as Uint8Arrays but until then we must recover and return the string version from the Uint8Array format that is now on the keyring value returned by the KeyringController on line 2508 as of this latest version bump.
adonesky1
commented
Jan 7, 2023
Collaborator
Builds ready [d11ff38]
Page Load Metrics (1361 ± 91 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
highlights:storybook
|
adonesky1
commented
Jan 9, 2023
Comment on lines
25
to
34
| const keyringBuilder = keyringController.getKeyringBuilderForType( | ||
| KEYRING_TYPES.HD_KEY_TREE, | ||
| ); | ||
| const keyring = keyringBuilder(); | ||
| const opts = { | ||
| mnemonic: seedPhrase, | ||
| numberOfAccounts: createdAccounts.length, | ||
| }; | ||
|
|
||
| const keyring = new Keyring(opts); | ||
| await keyring.deserialize(opts); |
Contributor
Author
There was a problem hiding this comment.
The instantiation pattern for keyrings has changed.
Changelog entry: https://github.com/MetaMask/KeyringController/blob/main/CHANGELOG.md?plain=1#L16
PR: MetaMask/KeyringController#163
adonesky1
changed the title
KeyringController v9
adonesky1
commented
Jan 9, 2023
Comment on lines
+210
to
+215
| const pubAddressHexArr = await simpleKeyrings[0].getAccounts(); | ||
| const privKeyHex = await simpleKeyrings[0].exportAccount( | ||
| pubAddressHexArr[0], | ||
| ); |
Contributor
Author
There was a problem hiding this comment.
This is the new pattern for getting public and private keys for an account.
adonesky1
commented
Jan 9, 2023
Comment on lines
+516
to
+533
| const mockMnemonic = | ||
| 'above mercy benefit hospital call oval domain student sphere interest argue shock'; | ||
| const mnemonicIndices = mockMnemonic | ||
| .split(' ') | ||
| .map((word) => englishWordlist.indexOf(word)); | ||
| const uint8ArrayMnemonic = new Uint8Array( | ||
| new Uint16Array(mnemonicIndices).buffer, | ||
| ); | ||
|
|
||
| const mockHDKeyring = { | ||
| type: 'HD Key Tree', | ||
| mnemonic: uint8ArrayMnemonic, | ||
| }; | ||
| sinon | ||
| .stub(metamaskController.keyringController, 'getKeyringsByType') | ||
| .returns([mockHDKeyring]); |
Contributor
Author
There was a problem hiding this comment.
This mocks the way mnemonics are stored on HD keyrings - as Uint8Arrays of the indices of the words relative to the english wordlist - as of MetaMask/eth-hd-keyring#67. This test then demonstrates that even though the mnemonic is stored in this way, the getPrimaryKeyringMnemonic method on MetamaskController returns the mnemonic in it's string form, since this is required by Snaps for now.
adonesky1
commented
Jan 9, 2023
| @@ -1,4 +1,4 @@ | |||
| import KeyringController from 'eth-keyring-controller'; | |||
| import { KeyringController } from 'eth-keyring-controller'; | |||
Contributor
Author
There was a problem hiding this comment.
KeyringController is now a named export: https://github.com/MetaMask/KeyringController/blob/main/CHANGELOG.md?plain=1#L17
adonesky1
commented
Jan 9, 2023
Comment on lines
+604
to
+657
| const additionalKeyringTypes = [ | ||
| TrezorKeyring, | ||
| LedgerBridgeKeyring, | ||
| LatticeKeyring, | ||
| QRHardwareKeyring, | ||
| ]; | ||
| additionalKeyrings = additionalKeyringTypes.map((keyringType) => | ||
| keyringBuilderFactory(keyringType), | ||
| ); |
Contributor
Author
There was a problem hiding this comment.
The KeyringController constructor now takes keyring builder functions rather than classes.
keyringBuilderFactory takes the class types and makes them into these builder functions.
jpuri
reviewed
Jan 12, 2023
app/scripts/lib/util.js
Outdated
| ); | ||
|
|
||
| return recoveredIndices.map((i) => englishWordlist[i]).join(' '); | ||
| } |
Contributor
There was a problem hiding this comment.
Hey @adonesky1 : just curious, how is above change related to KeyringController ?
Contributor
Author
There was a problem hiding this comment.
So this helper function is used here. The mnemonic returned on the keyring by this.keyringController.getKeyringsByType(KEYRING_TYPES.HD_KEY_TREE) is now a Uint8Array rather than a plain text string, but the getPrimaryKeyringMnemonic function that is used by snaps still expects it as a string so we need this function to convert it back.
adonesky1
force-pushed
the
integrate-keyringcontroller-v9
branch
from
d11ff38 to
7210370
Compare
Collaborator
Builds ready [ee8b6ee]
Page Load Metrics (1270 ± 50 ms)
Bundle size diffs [🚀 Bundle size reduced!]
highlights:storybook
|
Codecov Report
@@ Coverage Diff @@
## develop #17056 +/- ##
===========================================
+ Coverage 59.53% 59.55% +0.02%
===========================================
Files 936 936
Lines 35959 35959
Branches 9213 9212 -1
===========================================
+ Hits 21407 21413 +6
+ Misses 14552 14546 -6
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
adonesky1
force-pushed
the
integrate-keyringcontroller-v9
branch
from
6d441e1 to
060a7ca
Compare
Member
|
In reviewing this, I've noticed that the format of the mnemonic saved in the vault has changed with this update. This change was missed from the changelog as well. The format isn't especially great either, in that it's unconventional (it's essentially an array that was casted to an object - why not use an array instead?). This format change means we'll have to be careful when releasing this, because if we roll back whatever release this lands in, vault decryption will fail. So we can't roll this change back. Is this format change worth the trouble? We could switch back to the old one. |
This was referenced Jan 17, 2023
adonesky1
force-pushed
the
integrate-keyringcontroller-v9
branch
2 times, most recently
from
68ef7c6 to
c673a4e
Compare
Collaborator
Builds ready [432b7b2]
Page Load Metrics (1457 ± 149 ms)
Bundle size diffs [🚀 Bundle size reduced!]
|
Contributor
Author
@Gudahtt is aware of this but for broader visibility for anyone else reviewing this PR, this specific concern has been addressed with the updates made to |
adonesky1
force-pushed
the
integrate-keyringcontroller-v9
branch
from
432b7b2 to
8cc60b2
Compare
Collaborator
Builds ready [8cc60b2]
Page Load Metrics (1253 ± 101 ms)
Bundle size diffs [🚀 Bundle size reduced!]
|
Gudahtt
changed the title
KeyringController v9KeyringController v10
jpuri
approved these changes
Jan 20, 2023
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
Integrates
@metamask/scure-bip39andeth-keyring-controllerv9 and adapts accordinglyContext for changes are provided with inline comments 😄