Use createNewVaultAndRestore from core KeyringController
#19816
Merged
Conversation
This file contains 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
|
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. |
Builds ready [8314b41]
Page Load Metrics (1654 ± 37 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
Codecov Report
@@ Coverage Diff @@
## develop #19816 +/- ##
===========================================
- Coverage 68.84% 68.69% -0.15%
===========================================
Files 993 990 -3
Lines 38258 38083 -175
Branches 10248 10201 -47
===========================================
- Hits 26338 26160 -178
- Misses 11920 11923 +3
|
mikesposito
force-pushed
the
refactor/use-core-keyring-controller
branch
5 times, most recently
from
c65c6b9 to
2d584d7
Compare
|
Depends on #19659 |
mikesposito
force-pushed
the
refactor/use-core-keyring-controller
branch
2 times, most recently
from
95313e3 to
1567ae0
Compare
mikesposito
force-pushed
the
refactor/use-create-valut-and-restore-from-core
branch
from
8314b41 to
e23ac8a
Compare
Builds ready [e23ac8a]
Page Load Metrics (1758 ± 70 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
mcmire
reviewed
Jul 31, 2023
app/scripts/metamask-controller.js
Outdated
Comment on lines
2951
to
2988
| /** | ||
| * Get a Uint8Array mnemonic from Buffer. | ||
| * | ||
| * @param {Buffer} mnemonic - The mnemonic phrase as a Buffer | ||
| * @returns {Uint8Array} The mnemonic phrase as a Uint8Array | ||
| */ | ||
| _bufferToUint8Array(mnemonic) { | ||
| const indices = mnemonic | ||
| .toString() | ||
| .split(' ') | ||
| .map((word) => wordlist.indexOf(word)); | ||
| return new Uint8Array(new Uint16Array(indices).buffer); | ||
| } |
There was a problem hiding this comment.
This seems like almost the reverse of: #19817 (comment)
What do you think about:
Suggested change
| /** | |
| * Get a Uint8Array mnemonic from Buffer. | |
| * | |
| * @param {Buffer} mnemonic - The mnemonic phrase as a Buffer | |
| * @returns {Uint8Array} The mnemonic phrase as a Uint8Array | |
| */ | |
| _bufferToUint8Array(mnemonic) { | |
| const indices = mnemonic | |
| .toString() | |
| .split(' ') | |
| .map((word) => wordlist.indexOf(word)); | |
| return new Uint8Array(new Uint16Array(indices).buffer); | |
| } | |
| /** | |
| * Encodes a BIP-39 mnemonic as the indices of words in the English BIP-39 wordlist. | |
| * | |
| * @param {Buffer} mnenonic - The BIP-39 mnemonic. | |
| * @returns {Buffer} The Unicode code points for the seed phrase formed from the words in the wordlist. | |
| */ | |
| _convertMnemonicToWordlistIndices(mnemonic) { | |
| const indices = mnemonic | |
| .toString() | |
| .split(' ') | |
| .map((word) => wordlist.indexOf(word)); | |
| return new Uint8Array(indices); | |
| } |
It might be good to relocate this and the other function to some kind of utility file as they're related operations, but this seems fine for now.
There was a problem hiding this comment.
Makes sense! I just applied your suggestion.
Do you think that makes sense to merge the two PRs and then move the two methods to another file in a later PR?
There was a problem hiding this comment.
Uhm, it looks like this doesn't work. I noticed that also in @metamask/eth-hd-keyring we convert indices to Uint16Array before converting them finally to Uint8Array.
I'm not exactly sure why this is needed though
mikesposito
force-pushed
the
refactor/use-create-valut-and-restore-from-core
branch
from
4e5b9a9 to
fc64a7b
Compare
@mcmire We already have test coverage for this method, but I've just refactored the tests to mock |
mikesposito
force-pushed
the
refactor/use-create-valut-and-restore-from-core
branch
from
b672917 to
28be6d0
Compare
Builds ready [28be6d0]
Page Load Metrics (1689 ± 63 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
mikesposito
force-pushed
the
refactor/use-create-valut-and-restore-from-core
branch
from
28be6d0 to
ae61eeb
Compare
Builds ready [ae61eeb]
Page Load Metrics (1484 ± 37 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
mikesposito
force-pushed
the
refactor/use-create-valut-and-restore-from-core
branch
from
ae61eeb to
e8db201
Compare
Builds ready [e8db201]
Page Load Metrics (1466 ± 35 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
mikesposito
deleted the
refactor/use-create-valut-and-restore-from-core
branch
metamaskbot
added
the
release-10.36.0
Gudahtt
added
release-11.1.0
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.
Explanation
With this PR the extension starts using
createNewVaultAndRestorefrom coreKeyringController, instead of callingEthKeyringControllerdirectly.createNewVaultAndRestorefrom core KeyringController #18769Screenshots/Screencaps
Before
After
Manual Testing Steps
Pre-merge author checklist
Pre-merge reviewer checklist
If further QA is required (e.g. new feature, complex testing steps, large refactor), add the
Extension QA Boardlabel.In this case, a QA Engineer approval will be be required.