-
Notifications
You must be signed in to change notification settings - Fork 5.1k
feat: import srp #30598
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: import srp #30598
Conversation
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 [3ccc9f1]
Page Load Metrics (2013 ± 132 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
Builds ready [ea82ea4]
Page Load Metrics (1862 ± 103 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
Builds ready [c3ea3b0]
Page Load Metrics (2020 ± 103 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
@@ -46,7 +46,7 @@ class AccountListPage { | |||
'[data-testid="multichain-account-menu-popover-add-watch-only-account"]'; | |||
|
|||
private readonly addHardwareWalletButton = { | |||
text: 'Add hardware wallet', | |||
text: 'Hardware wallet', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For future reviewers: Those changes are expected, and match the new designs (the "Add " prefix has now been dropped for most "add*Account" labels)
@@ -197,7 +197,7 @@ | |||
"message": "Add friends and addresses you trust" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comment for the entire file:
Since this PR also uses the new "account list" design, I've spotted some differences for some kind of accounts:
addHardwareWallet
,addNewBitcoinAccount
andaddNewBitcoinTestnetAccount
could be renamed toadd*Label
(to match the new naming convention)addNewBitcoinAccount
andaddNewBitcoinTestnetAccount
still uses the old description "Add a new ...", we could remove those (we can double-check with the designs for this)addNewAccount
is no longer used, so we can remove it IMO
ui/components/multichain/multi-srp/import-srp/import-srp.test.tsx
Outdated
Show resolved
Hide resolved
ui/components/multichain/multi-srp/import-srp/import-srp.test.tsx
Outdated
Show resolved
Hide resolved
app/scripts/metamask-controller.js
Outdated
const alreadyImportedSRP = await this.keyringController | ||
.getKeyringsByType(KeyringTypes.hd) | ||
.some((keyring) => { | ||
return ( | ||
keyring.accounts.includes(address) && | ||
keyring.type === KeyringTypes.hd | ||
Buffer.from( | ||
this._convertEnglishWordlistIndicesToCodepoints(keyring.mnemonic), | ||
).toString('utf8') === mnemonic | ||
); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this logic relies on a @deprecated
method, we could move it to the KeyringController
? That would also hide the "detailed implementation" about how mnemonics are being encoded for HD keyrings.
Or should we have a withKeyrings(keyringType)
(note the s
) that we would use similarly to withKeyring
? Just random thoughts on this.
I'd like to get your opinion (if possible) on this @mikesposito 😄 thanks!
(For now, adding a TODO
seems like a good option)
// TODO: `getKeyringsByType` is deprecated, this logic should probably be moved to the `KeyringController`.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think withKeyrings(keyringType)
would be pretty helpful.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Besides withKeyrings
we could also simply iterate through the keyrings list on KeyringController.state
and get the mnemonic with primitives we already have, like withKeyring
Though, I'm wondering if this is really needed here, since it's the controller to check for duplicates: https://github.com/MetaMask/core/blob/5b74b24875491b480a01dd5d950e04020fba7e26/packages/keyring-controller/src/KeyringController.ts#L2483
The HD keyring will always include at least one address, and if the same SRP is imported twice, the second one will throw an error on KeyrignController
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup, IMO, we should have this kind of guarantee at the controller level.
Though, currently the #checkForDuplicate
do not check anything for HD keyrings (if I read that correctly)? But I do agree that having that we should have a check there (and checking the mnemonic together would allow to have a "specific error" like KeyringControllerError.DuplicatedMnemonic
rather than KeyringControllerError.DuplicatedAccount
).
That will be also useful for mobile integration IMO.
WDYT @montelaidev?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It doesn't check the specific mnemonic that is being imported, but it throws error if there are two keyrings with the same address. I'm assuming that the same SRP will always create the same first account, and since we automatically add one when importing the SRP, that means we'll have at least one duplicated account when calling #checkForDuplicate
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As discussed together internally, the current logic of #checkForDuplicates
is not covering every cases (and not this one 🙈), here's the ticket to track this:
Builds ready [51550c4]
Page Load Metrics (1752 ± 66 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
ui/components/multichain/account-list-menu/account-list-menu.tsx
Outdated
Show resolved
Hide resolved
ui/components/multichain/multi-srp/import-srp/import-srp.test.tsx
Outdated
Show resolved
Hide resolved
ui/components/multichain/multi-srp/import-srp/import-srp.test.tsx
Outdated
Show resolved
Hide resolved
ui/components/multichain/multi-srp/import-srp/import-srp.test.tsx
Outdated
Show resolved
Hide resolved
ui/components/multichain/multi-srp/import-srp/import-srp.test.tsx
Outdated
Show resolved
Hide resolved
ui/components/multichain/multi-srp/import-srp/import-srp.test.tsx
Outdated
Show resolved
Hide resolved
app/scripts/metamask-controller.js
Outdated
const alreadyImportedSRP = await this.keyringController | ||
.getKeyringsByType(KeyringTypes.hd) | ||
.some((keyring) => { | ||
return ( | ||
keyring.accounts.includes(address) && | ||
keyring.type === KeyringTypes.hd | ||
Buffer.from( | ||
this._convertEnglishWordlistIndicesToCodepoints(keyring.mnemonic), | ||
).toString('utf8') === mnemonic | ||
); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup, IMO, we should have this kind of guarantee at the controller level.
Though, currently the #checkForDuplicate
do not check anything for HD keyrings (if I read that correctly)? But I do agree that having that we should have a check there (and checking the mnemonic together would allow to have a "specific error" like KeyringControllerError.DuplicatedMnemonic
rather than KeyringControllerError.DuplicatedAccount
).
That will be also useful for mobile integration IMO.
WDYT @montelaidev?
Yea it would be much better to do it in the controller |
@@ -0,0 +1,309 @@ | |||
import React, { useCallback, useMemo, useState } from 'react'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note for the entire file: As discussed internally with @montelaidev, this file will be refactored in a follow-up PR to avoid duplicating the logic with input-srp
.
So I will adds more comments on this new PR once this one will be merged.
See: https://github.com/MetaMask/metamask-extension/pull/30598/files#r1977094784
ui/components/multichain/multi-srp/import-srp/import-srp.test.tsx
Outdated
Show resolved
Hide resolved
ui/components/multichain/multi-srp/import-srp/import-srp.test.tsx
Outdated
Show resolved
Hide resolved
ui/components/multichain/multi-srp/import-srp/import-srp.test.tsx
Outdated
Show resolved
Hide resolved
ui/components/multichain/multi-srp/import-srp/import-srp.test.tsx
Outdated
Show resolved
Hide resolved
Co-authored-by: Charly Chevalier <[email protected]>
Co-authored-by: Charly Chevalier <[email protected]>
Builds ready [e9e193e]
Page Load Metrics (1825 ± 61 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
app/_locales/en/messages.json
Outdated
"addBitcoinAccountLabel": { | ||
"message": "Add a new Bitcoin account (Beta)" | ||
}, | ||
"addBitcoinTestnetAccountLabel": { | ||
"message": "Add a new Bitcoin account (Testnet)" | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Related to: https://github.com/MetaMask/metamask-extension/pull/30598/files#r1973162748
Are we also renaming those to Bitcoin account
?
ui/components/multichain/multi-srp/import-srp/import-srp.test.tsx
Outdated
Show resolved
Hide resolved
Tested ✅ $ jq '.metamask.internalAccounts.accounts | to_entries[] | .value.address' state.json "0x33f5be2b61b33b4423e4f15fdd9a344c0b2e3a0d"
"0x8e29b63f510f6f3714fa23c7cc0614e3a928fb16" $ jq '.metamask | {keyrings, keyringsMetadata}' state.json {
"keyrings": [
{
"type": "HD Key Tree",
"accounts": [
"0x33f5be2b61b33b4423e4f15fdd9a344c0b2e3a0d"
]
},
{
"type": "HD Key Tree",
"accounts": [
"0x8e29b63f510f6f3714fa23c7cc0614e3a928fb16"
]
}
],
"keyringsMetadata": [
{
"id": "01JNH032HRW8A1CYC9W90WDAFP",
"name": ""
},
{
"id": "01JNH053FZDK57D3SR1DMMK4R3",
"name": ""
}
]
} Found one small UI issue, the "plus" sign is bigger for Ethereum than for Solana: |
Builds ready [2d0ecd9]
Page Load Metrics (2022 ± 79 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot for all those changes, LGTM now 😄 (and already tested, see my previous comment)
Builds ready [487db25]
Page Load Metrics (1546 ± 46 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
Description
This PR introduces importing srps as a new feature. It is split from a previous pr
Related issues
Fixes:
Manual testing steps
Importing a new SRP
Import secret recovery phrase
Screenshots/Recordings
Before
After
Importing a new SRP
Screen.Recording.2025-02-14.at.17.26.32.mov
Pre-merge author checklist
Pre-merge reviewer checklist