Skip to content

Commit 8897c2c

Browse files
montelaidevsethkfmangantunesr
authored
fix: restore snap and imported srps when changing password (#15237)
<!-- Please submit this PR as a draft initially. Do not mark it as "Ready for review" until the template has been completely filled out, and PR status checks have passed at least once. --> ## **Description** This PR adds the restoration of first party snap accounts and imported srps when changing password. Changes: 1. added `restoreImportedSrp` `restoreSnapAccounts` to Vault.js and updated the change password logic to use these. 2. Added new tests to Vault 3. Fix deadlock that could happen when awaiting a promise in MultichainSnapClient. <!-- Write a short description of the changes included in this pull request, also include relevant motivation and context. Have in mind the following questions: 1. What is the reason for the change? 4. What is the improvement/solution? --> ## **Related issues** Fixes: #15258 Fixes: #15233 Resolves: https://consensyssoftware.atlassian.net/browse/MMMULTISRP-208?atlOrigin=eyJpIjoiMmM0MDMzNDRlYzdlNDJmZTg3ODNhMzUzNzlkOWZmZDIiLCJwIjoiaiJ9 ## **Manual testing steps** 1. Using a fresh wallet. 2. Create a new solana account 3. Import a private key 5. Import a qr hardware wallet 6. import a ledger wallet 7. Go to settings --> security 8. Go through change password 9. Login to the wallet and see that accounts are still there. ## **Screenshots/Recordings** <!-- If applicable, add screenshots and/or recordings to visualize the before and after of your change. --> ### **Before** <!-- [screenshots/recordings] --> ### **After** <!-- [screenshots/recordings] --> ## **Pre-merge author checklist** - [x] I’ve followed [MetaMask Contributor Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask Mobile Coding Standards](https://github.com/MetaMask/metamask-mobile/blob/main/.github/guidelines/CODING_GUIDELINES.md). - [x] I've completed the PR template to the best of my ability - [x] I’ve included tests if applicable - [x] I’ve documented my code using [JSDoc](https://jsdoc.app/) format if applicable - [x] I’ve applied the right labels on the PR (see [labeling guidelines](https://github.com/MetaMask/metamask-mobile/blob/main/.github/guidelines/LABELING_GUIDELINES.md)). Not required for external contributors. ## **Pre-merge reviewer checklist** - [ ] I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed). - [ ] I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots. --------- Co-authored-by: sethkfman <[email protected]> Co-authored-by: Gustavo Antunes <[email protected]>
1 parent 025bfdb commit 8897c2c

File tree

5 files changed

+602
-89
lines changed

5 files changed

+602
-89
lines changed

app/core/SnapKeyring/MultichainWalletSnapClient.test.ts

Lines changed: 24 additions & 43 deletions
Original file line numberDiff line numberDiff line change
@@ -6,18 +6,22 @@ import {
66
WalletClientType,
77
} from './MultichainWalletSnapClient';
88
import { CaipChainId, SnapId } from '@metamask/snaps-sdk';
9-
import { KeyringTypes } from '@metamask/keyring-controller';
109
import Engine from '../Engine';
1110
import { Sender } from '@metamask/keyring-snap-client';
1211
import { SnapKeyring } from '@metamask/eth-snap-keyring';
1312
import { BtcScope, SolScope } from '@metamask/keyring-api';
1413
import { BITCOIN_WALLET_SNAP_ID } from './BitcoinWalletSnap';
1514

15+
const mockSnapKeyring = {
16+
createAccount: jest.fn(),
17+
discoverAccounts: jest.fn(),
18+
};
19+
1620
jest.mock('../Engine', () => ({
1721
controllerMessenger: {
1822
call: jest.fn(),
1923
},
20-
getSnapKeyring: jest.fn(),
24+
getSnapKeyring: () => mockSnapKeyring,
2125
context: {
2226
AccountsController: {
2327
getNextAvailableAccountName: jest.fn().mockReturnValue('Snap Account 1'),
@@ -95,32 +99,29 @@ describe('MultichainWalletSnapClient', () => {
9599

96100
describe('withSnapKeyring', () => {
97101
it('calls the callback with the keyring', async () => {
98-
const mockCallback = jest.fn();
99-
const mockKeyring = { test: 'keyring' };
100-
101-
(Engine.controllerMessenger.call as jest.Mock).mockImplementationOnce(
102-
async (_, __, callback) => {
103-
await callback({ keyring: mockKeyring });
104-
},
105-
);
102+
const mockOptions = {};
103+
const mockCallback = async (keyring: SnapKeyring) => {
104+
await keyring.createAccount(
105+
mockSnapId,
106+
mockOptions,
107+
mockSnapKeyringOptions,
108+
);
109+
};
106110

107111
await client.testWithSnapKeyring(mockCallback);
108112

109-
expect(Engine.controllerMessenger.call).toHaveBeenCalledWith(
110-
'KeyringController:withKeyring',
111-
{ type: KeyringTypes.snap },
112-
expect.any(Function),
113+
expect(mockSnapKeyring.createAccount).toHaveBeenCalledWith(
114+
mockSnapId,
115+
mockOptions,
116+
mockSnapKeyringOptions,
113117
);
114-
expect(mockCallback).toHaveBeenCalledWith(mockKeyring);
115118
});
116119

117120
it('handles errors from the controller messenger', async () => {
118121
const mockError = new Error('Test error');
119-
(Engine.controllerMessenger.call as jest.Mock).mockRejectedValueOnce(
120-
mockError,
121-
);
122+
const mockCallback = jest.fn().mockRejectedValueOnce(mockError);
122123

123-
await expect(client.testWithSnapKeyring(jest.fn())).rejects.toThrow(
124+
await expect(client.testWithSnapKeyring(mockCallback)).rejects.toThrow(
124125
'Test error',
125126
);
126127
});
@@ -134,19 +135,9 @@ describe('MultichainWalletSnapClient', () => {
134135
entropySource: 'test-entropy',
135136
};
136137

137-
const mockKeyring = {
138-
createAccount: jest.fn(),
139-
};
140-
141-
(Engine.controllerMessenger.call as jest.Mock).mockImplementationOnce(
142-
async (_, __, callback) => {
143-
await callback({ keyring: mockKeyring });
144-
},
145-
);
146-
147138
await client.createAccount(mockOptions);
148139

149-
expect(mockKeyring.createAccount).toHaveBeenCalledWith(
140+
expect(mockSnapKeyring.createAccount).toHaveBeenCalledWith(
150141
mockSnapId,
151142
mockOptions,
152143
mockSnapKeyringOptions,
@@ -213,8 +204,8 @@ describe('MultichainWalletSnapClient', () => {
213204

214205
await client.addDiscoveredAccounts(mockEntropySource);
215206

216-
expect(mockKeyring.createAccount).toHaveBeenCalledTimes(1);
217-
expect(mockKeyring.createAccount).toHaveBeenCalledWith(
207+
expect(mockSnapKeyring.createAccount).toHaveBeenCalledTimes(1);
208+
expect(mockSnapKeyring.createAccount).toHaveBeenCalledWith(
218209
mockSnapId,
219210
{
220211
derivationPath: mockDiscoveredAccounts[0].derivationPath,
@@ -301,20 +292,10 @@ describe('Wallet Client Implementations', () => {
301292
entropySource: 'test-entropy',
302293
};
303294

304-
const mockKeyring = {
305-
createAccount: jest.fn(),
306-
};
307-
308-
(Engine.controllerMessenger.call as jest.Mock).mockImplementationOnce(
309-
async (_, __, callback) => {
310-
await callback({ keyring: mockKeyring });
311-
},
312-
);
313-
314295
const bitcoinClient = new BitcoinWalletSnapClient(mockSnapKeyringOptions);
315296
await bitcoinClient.createAccount(mockOptions);
316297

317-
expect(mockKeyring.createAccount).toHaveBeenCalledWith(
298+
expect(mockSnapKeyring.createAccount).toHaveBeenCalledWith(
318299
BITCOIN_WALLET_SNAP_ID,
319300
{ ...mockOptions, synchronize: true },
320301
mockSnapKeyringOptions,

app/core/SnapKeyring/MultichainWalletSnapClient.ts

Lines changed: 5 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,6 @@ import {
1313
SolanaWalletSnapSender,
1414
} from './SolanaWalletSnap';
1515
import Engine from '../Engine';
16-
import { KeyringTypes } from '@metamask/keyring-controller';
1716
import { SnapKeyring } from '@metamask/eth-snap-keyring';
1817
import { store } from '../../store';
1918
import { startPerformanceTrace } from '../redux/slices/performance';
@@ -88,16 +87,11 @@ export abstract class MultichainWalletSnapClient {
8887
*
8988
*/
9089
protected async withSnapKeyring(
91-
callback: (keyring: SnapKeyring) => Promise<void>,
90+
callback: (keyring: SnapKeyring) => Promise<unknown>,
9291
) {
93-
const controllerMessenger = Engine.controllerMessenger;
94-
await Engine.getSnapKeyring();
92+
const snapKeyring = (await Engine.getSnapKeyring()) as SnapKeyring;
9593

96-
return await controllerMessenger.call(
97-
'KeyringController:withKeyring',
98-
{ type: KeyringTypes.snap },
99-
async ({ keyring }) => await callback(keyring as unknown as SnapKeyring),
100-
);
94+
return await callback(snapKeyring);
10195
}
10296

10397
/**
@@ -127,7 +121,7 @@ export abstract class MultichainWalletSnapClient {
127121
);
128122

129123
return await this.withSnapKeyring(async (keyring) => {
130-
(keyring as unknown as SnapKeyring).createAccount(
124+
await keyring.createAccount(
131125
this.snapId,
132126
{
133127
...options,
@@ -212,7 +206,7 @@ export abstract class MultichainWalletSnapClient {
212206
await this.withSnapKeyring(async (keyring) => {
213207
const results = await Promise.allSettled(
214208
discoveredAccounts.map(async (account) => {
215-
keyring.createAccount(
209+
await keyring.createAccount(
216210
this.snapId,
217211
{
218212
derivationPath: account.derivationPath,

app/core/Vault.js

Lines changed: 150 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,16 @@ import Engine from './Engine';
22
import Logger from '../util/Logger';
33
import { KeyringTypes } from '@metamask/keyring-controller';
44
import { withLedgerKeyring } from './Ledger/Ledger';
5+
import {
6+
MultichainWalletSnapFactory,
7+
WalletClientType,
8+
} from './SnapKeyring/MultichainWalletSnapClient';
9+
import {
10+
BtcAccountType,
11+
BtcScope,
12+
SolAccountType,
13+
SolScope,
14+
} from '@metamask/keyring-api';
515

616
/**
717
* Restore the given serialized QR keyring.
@@ -36,14 +46,72 @@ export const restoreLedgerKeyring = async (serializedLedgerKeyring) => {
3646
}
3747
};
3848

49+
export const restoreImportedSrp = async (seedPhrase, numberOfAccounts) => {
50+
const { KeyringController } = Engine.context;
51+
try {
52+
const { id: keyringId } = await KeyringController.addNewKeyring(
53+
KeyringTypes.hd,
54+
{
55+
mnemonic: seedPhrase,
56+
},
57+
);
58+
59+
for (let i = 0; i < numberOfAccounts; i++) {
60+
await KeyringController.withKeyring(
61+
{ id: keyringId },
62+
async ({ keyring }) => await keyring.addAccounts(1),
63+
);
64+
}
65+
66+
return keyringId;
67+
} catch (e) {
68+
Logger.error(
69+
e,
70+
'error while trying to restore imported srp accounts on recreate vault',
71+
);
72+
}
73+
};
74+
75+
export const restoreSnapAccounts = async (accountType, entropySource) => {
76+
let walletClientType;
77+
let scope;
78+
switch (accountType) {
79+
case SolAccountType.DataAccount: {
80+
walletClientType = WalletClientType.Solana;
81+
scope = SolScope.Mainnet;
82+
break;
83+
}
84+
case BtcAccountType.P2wpkh: {
85+
walletClientType = WalletClientType.Bitcoin;
86+
scope = BtcScope.Mainnet;
87+
break;
88+
}
89+
default:
90+
throw new Error('Unsupported account type');
91+
}
92+
93+
try {
94+
const client = MultichainWalletSnapFactory.createClient(walletClientType);
95+
await client.createAccount({
96+
entropySource,
97+
scope,
98+
});
99+
} catch (e) {
100+
Logger.error(
101+
e,
102+
'error while trying to restore snap accounts on recreate vault',
103+
);
104+
}
105+
};
106+
39107
/**
40108
* Returns current vault seed phrase
41109
* It does it using an empty password or a password set by the user
42110
* depending on the state the app is currently in
43111
*/
44-
export const getSeedPhrase = async (password = '') => {
112+
export const getSeedPhrase = async (password = '', keyringId) => {
45113
const { KeyringController } = Engine.context;
46-
return await KeyringController.exportSeedPhrase(password);
114+
return await KeyringController.exportSeedPhrase(password, keyringId);
47115
};
48116

49117
/**
@@ -58,8 +126,35 @@ export const recreateVaultWithNewPassword = async (
58126
newPassword,
59127
selectedAddress,
60128
) => {
61-
const { KeyringController } = Engine.context;
62-
const seedPhrase = await getSeedPhrase(password);
129+
const { KeyringController, AccountsController } = Engine.context;
130+
const hdKeyringsWithMetadata = KeyringController.state.keyrings
131+
.map((keyring, _index) => ({
132+
...keyring,
133+
///: BEGIN:ONLY_INCLUDE_IF(multi-srp)
134+
metadata: KeyringController.state.keyringsMetadata?.[_index] || {},
135+
///: END:ONLY_INCLUDE_IF
136+
}))
137+
.filter((keyring) => keyring.type === KeyringTypes.hd);
138+
139+
const seedPhrases = await Promise.all(
140+
hdKeyringsWithMetadata.map(async (keyring) => {
141+
try {
142+
return await getSeedPhrase(password, keyring.metadata.id);
143+
} catch (e) {
144+
Logger.error(
145+
e,
146+
'error while trying to get seed phrase on recreate vault',
147+
);
148+
return null;
149+
}
150+
}),
151+
);
152+
const [primaryKeyringSeedPhrase, ...otherSeedPhrases] = seedPhrases;
153+
if (!primaryKeyringSeedPhrase) {
154+
throw new Error('error while trying to get seed phrase on recreate vault');
155+
}
156+
157+
// START: Getting accounts to be reimported
63158

64159
let importedAccounts = [];
65160
try {
@@ -83,9 +178,19 @@ export const recreateVaultWithNewPassword = async (
83178
);
84179
}
85180

181+
const firstPartySnapAccounts =
182+
AccountsController.listMultichainAccounts().filter(
183+
(account) => account.options?.entropySource,
184+
);
185+
86186
// Get props to restore vault
87-
const hdKeyring = KeyringController.state.keyrings[0];
88-
const existingAccountCount = hdKeyring.accounts.length;
187+
const hdKeyringsAccountCount = hdKeyringsWithMetadata.map(
188+
(keyring) => keyring.accounts.length,
189+
);
190+
const [primaryKeyringAccountCount, ...otherKeyringAccountCounts] =
191+
hdKeyringsAccountCount;
192+
193+
// END: Getting accounts to be reimported
89194

90195
const serializedLedgerKeyring = hasKeyringType(
91196
KeyringController.state,
@@ -101,7 +206,14 @@ export const recreateVaultWithNewPassword = async (
101206
: undefined;
102207

103208
// Recreate keyring with password given to this method
104-
await KeyringController.createNewVaultAndRestore(newPassword, seedPhrase);
209+
await KeyringController.createNewVaultAndRestore(
210+
newPassword,
211+
primaryKeyringSeedPhrase,
212+
);
213+
const [newPrimaryKeyringMetadata] = KeyringController.state.keyringsMetadata;
214+
const newPrimaryKeyringId = newPrimaryKeyringMetadata.id;
215+
216+
// START: Restoring keyrings
105217

106218
if (serializedQrKeyring !== undefined) {
107219
await restoreQRKeyring(serializedQrKeyring);
@@ -111,7 +223,7 @@ export const recreateVaultWithNewPassword = async (
111223
}
112224

113225
// Create previous accounts again
114-
for (let i = 0; i < existingAccountCount - 1; i++) {
226+
for (let i = 0; i < primaryKeyringAccountCount - 1; i++) {
115227
await KeyringController.addNewAccount();
116228
}
117229

@@ -125,6 +237,36 @@ export const recreateVaultWithNewPassword = async (
125237
} catch (e) {
126238
Logger.error(e, 'error while trying to import accounts on recreate vault');
127239
}
240+
241+
// recreate import srp accounts
242+
const importedSrpKeyringIds = [];
243+
///: BEGIN:ONLY_INCLUDE_IF(multi-srp)
244+
for (const [index, otherSeedPhrase] of otherSeedPhrases.entries()) {
245+
const importedSrpKeyring = await restoreImportedSrp(
246+
otherSeedPhrase,
247+
otherKeyringAccountCounts[index],
248+
);
249+
importedSrpKeyringIds.push(importedSrpKeyring);
250+
}
251+
///: END:ONLY_INCLUDE_IF(multi-srp)
252+
253+
const newHdKeyringIds = [newPrimaryKeyringId, ...importedSrpKeyringIds];
254+
// map old keyring id to new keyring id
255+
const keyringIdMap = new Map();
256+
for (const [index, keyring] of hdKeyringsWithMetadata.entries()) {
257+
keyringIdMap.set(keyring.metadata.id, newHdKeyringIds[index]);
258+
}
259+
260+
// recreate snap accounts
261+
for (const snapAccount of firstPartySnapAccounts) {
262+
await restoreSnapAccounts(
263+
snapAccount.type,
264+
keyringIdMap.get(snapAccount.options.entropySource),
265+
);
266+
}
267+
268+
// END: Restoring keyrings
269+
128270
const recreatedKeyrings = KeyringController.state.keyrings;
129271
// Reselect previous selected account if still available
130272
for (const keyring of recreatedKeyrings) {

0 commit comments

Comments
 (0)