Skip to content

Commit 93d9f3f

Browse files
Cherry pick 7857b82 (Solana account discovery on SRP import from #15614) (#15870)
- Solana account discovery was not working on the RC due to the discovery logic not being included in the correct code fences. I have fixed this by changing the incorrect code fence to `///: BEGIN:ONLY_INCLUDE_IF(solana)` - This PR also adds a small fix that was discovered after fixing account discovery. The issue was that once a Solana account was discovered, it would be set as the default account. Now, when a solana account is discovered, we do not automatically set it as the selected account which is the same behaviour we have on extension. I took this code directly from [extension](https://github.com/MetaMask/metamask-extension/blob/726bcad098f67b7223e90be0885290399b6fae95/app/scripts/lib/snap-keyring/snap-keyring.ts#L326-L332). Fixes: https://consensyssoftware.atlassian.net/browse/MUL-208 1. load branch 2. import any SRP 3. complete onboarding 4. Notice: A solana account should be auto created for the SRP. If the account had a balance, it should be correct. If not, it should have a zero balance. 5. Optional, import a second SRP 6. a new solana account under that SRP should also be created. https://github.com/user-attachments/assets/a9c22c59-f4d4-44cd-bf52-11d6d4f88b49 <!-- [screenshots/recordings] --> https://github.com/user-attachments/assets/9074c40b-6178-4bf3-9dd6-aad6fcd6a63f - [ ] 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). - [ ] I've completed the PR template to the best of my ability - [ ] I’ve included tests if applicable - [ ] I’ve documented my code using [JSDoc](https://jsdoc.app/) format if applicable - [ ] 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. - [ ] 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: Curtis David <Curtis.David7@gmail.com>
1 parent df94299 commit 93d9f3f

4 files changed

Lines changed: 142 additions & 33 deletions

File tree

app/actions/multiSrp/index.ts

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,7 @@ import { wordlist } from '@metamask/scure-bip39/dist/wordlists/english';
33
import ExtendedKeyringTypes from '../../constants/keyringTypes';
44
import Engine from '../../core/Engine';
55
import { KeyringSelector } from '@metamask/keyring-controller';
6-
///: BEGIN:ONLY_INCLUDE_IF(beta)
6+
///: BEGIN:ONLY_INCLUDE_IF(solana)
77
import {
88
MultichainWalletSnapFactory,
99
WalletClientType,
@@ -64,7 +64,7 @@ export async function importNewSecretRecoveryPhrase(mnemonic: string) {
6464
async ({ keyring }) => keyring.getAccounts(),
6565
);
6666

67-
///: BEGIN:ONLY_INCLUDE_IF(beta)
67+
///: BEGIN:ONLY_INCLUDE_IF(solana)
6868
const multichainClient = MultichainWalletSnapFactory.createClient(
6969
WalletClientType.Solana,
7070
);

app/core/Authentication/Authentication.ts

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -31,12 +31,12 @@ import NavigationService from '../NavigationService';
3131
import Routes from '../../constants/navigation/Routes';
3232
import { TraceName, TraceOperation, endTrace, trace } from '../../util/trace';
3333
import ReduxService from '../redux';
34-
///: BEGIN:ONLY_INCLUDE_IF(beta)
34+
///: BEGIN:ONLY_INCLUDE_IF(solana)
3535
import {
3636
MultichainWalletSnapFactory,
3737
WalletClientType,
3838
} from '../SnapKeyring/MultichainWalletSnapClient';
39-
///: END:ONLY_INCLUDE_IF(beta)
39+
///: END:ONLY_INCLUDE_IF
4040

4141
/**
4242
* Holds auth data used to determine auth configuration
@@ -91,7 +91,7 @@ class AuthenticationService {
9191
const { KeyringController }: any = Engine.context;
9292
if (clearEngine) await Engine.resetState();
9393
await KeyringController.createNewVaultAndRestore(password, parsedSeed);
94-
///: BEGIN:ONLY_INCLUDE_IF(beta)
94+
///: BEGIN:ONLY_INCLUDE_IF(solana)
9595
const primaryHdKeyringId =
9696
Engine.context.KeyringController.state.keyrings[0].metadata.id;
9797
const client = MultichainWalletSnapFactory.createClient(
@@ -101,7 +101,7 @@ class AuthenticationService {
101101
},
102102
);
103103
await client.addDiscoveredAccounts(primaryHdKeyringId);
104-
///: END:ONLY_INCLUDE_IF(beta)
104+
///: END:ONLY_INCLUDE_IF
105105
password = this.wipeSensitiveData();
106106
parsedSeed = this.wipeSensitiveData();
107107
};
@@ -119,7 +119,7 @@ class AuthenticationService {
119119
await Engine.resetState();
120120
await KeyringController.createNewVaultAndKeychain(password);
121121

122-
///: BEGIN:ONLY_INCLUDE_IF(beta)
122+
///: BEGIN:ONLY_INCLUDE_IF(solana)
123123
const primaryHdKeyringId =
124124
Engine.context.KeyringController.state.keyrings[0].metadata.id;
125125
const client = MultichainWalletSnapFactory.createClient(
@@ -129,7 +129,7 @@ class AuthenticationService {
129129
},
130130
);
131131
await client.addDiscoveredAccounts(primaryHdKeyringId);
132-
///: END:ONLY_INCLUDE_IF(beta)
132+
///: END:ONLY_INCLUDE_IF
133133
password = this.wipeSensitiveData();
134134
};
135135

app/core/SnapKeyring/SnapKeyring.test.ts

Lines changed: 116 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -29,7 +29,6 @@ const mockSetSelectedAccount = jest.fn();
2929
const mockRemoveAccountHelper = jest.fn();
3030
const mockGetAccountByAddress = jest.fn();
3131
const mockSetAccountName = jest.fn();
32-
const mockSetAccountNameAndSelectAccount = jest.fn();
3332
const mockSnapControllerHandleRequest = jest.fn();
3433
const mockListMultichainAccounts = jest.fn();
3534

@@ -85,7 +84,6 @@ const createControllerMessenger = ({
8584
'AccountsController:getAccountByAddress',
8685
'AccountsController:listMultichainAccounts',
8786
'AccountsController:setAccountName',
88-
'AccountsController:setAccountNameAndSelectAccount',
8987
],
9088
allowedEvents: [],
9189
});
@@ -110,8 +108,6 @@ const createControllerMessenger = ({
110108
return mockSetSelectedAccount(params);
111109
case 'AccountsController:setAccountName':
112110
return mockSetAccountName.mockReturnValue(null)(params);
113-
case 'AccountsController:setAccountNameAndSelectAccount':
114-
return mockSetAccountNameAndSelectAccount.mockReturnValue(null)(params);
115111
case 'AccountsController:listMultichainAccounts':
116112
return mockListMultichainAccounts.mockReturnValue([])();
117113
case 'SnapController:handleRequest':
@@ -221,12 +217,16 @@ describe('Snap Keyring Methods', () => {
221217
]);
222218
expect(mockPersisKeyringHelper).toHaveBeenCalledTimes(1);
223219
expect(mockGetAccounts).toHaveBeenCalledTimes(1);
224-
expect(mockSetAccountNameAndSelectAccount).not.toHaveBeenCalled();
225220
expect(mockEndFlow).toHaveBeenCalledWith([{ id: mockFlowId }]);
226221

227222
// Wait for any pending promises (including the account finalization which tracks the event)
228223
await waitForAllPromises();
229224

225+
// Verify that setSelectedAccount is called but setAccountName is not called
226+
expect(mockSetSelectedAccount).toHaveBeenCalledTimes(1);
227+
expect(mockSetSelectedAccount).toHaveBeenCalledWith([mockAccount.id]);
228+
expect(mockSetAccountName).not.toHaveBeenCalled();
229+
230230
// Verify trackSnapAccountEvent was called for successful account creation
231231
expect(trackSnapAccountEvent).toHaveBeenCalled();
232232
});
@@ -260,17 +260,21 @@ describe('Snap Keyring Methods', () => {
260260
true,
261261
]);
262262
expect(mockGetAccounts).toHaveBeenCalledTimes(1);
263-
expect(mockSetAccountNameAndSelectAccount).toHaveBeenCalledTimes(1);
264-
expect(mockSetAccountNameAndSelectAccount).toHaveBeenCalledWith([
263+
264+
// Wait for any pending promises (including the account finalization which tracks the event)
265+
await waitForAllPromises();
266+
267+
// Verify that setSelectedAccount and setAccountName are called separately
268+
expect(mockSetSelectedAccount).toHaveBeenCalledTimes(1);
269+
expect(mockSetSelectedAccount).toHaveBeenCalledWith([mockAccount.id]);
270+
expect(mockSetAccountName).toHaveBeenCalledTimes(1);
271+
expect(mockSetAccountName).toHaveBeenCalledWith([
265272
mockAccount.id,
266273
mockNameSuggestion,
267274
]);
268275
expect(mockEndFlow).toHaveBeenCalledTimes(2);
269276
expect(mockEndFlow).toHaveBeenCalledWith([{ id: mockFlowId }]);
270277

271-
// Wait for any pending promises (including the account finalization which tracks the event)
272-
await waitForAllPromises();
273-
274278
// Verify trackSnapAccountEvent was called
275279
expect(trackSnapAccountEvent).toHaveBeenCalled();
276280
});
@@ -402,8 +406,15 @@ describe('Snap Keyring Methods', () => {
402406

403407
// Verify that the account was created and named
404408
expect(mockPersisKeyringHelper).toHaveBeenCalledTimes(1);
405-
expect(mockSetAccountNameAndSelectAccount).toHaveBeenCalledTimes(1);
406-
expect(mockSetAccountNameAndSelectAccount).toHaveBeenCalledWith([
409+
410+
// Wait for any pending promises (including the account finalization)
411+
await waitForAllPromises();
412+
413+
// Verify that setSelectedAccount and setAccountName are called separately
414+
expect(mockSetSelectedAccount).toHaveBeenCalledTimes(1);
415+
expect(mockSetSelectedAccount).toHaveBeenCalledWith([mockAccount.id]);
416+
expect(mockSetAccountName).toHaveBeenCalledTimes(1);
417+
expect(mockSetAccountName).toHaveBeenCalledWith([
407418
mockAccount.id,
408419
mockNameSuggestion,
409420
]);
@@ -437,8 +448,15 @@ describe('Snap Keyring Methods', () => {
437448

438449
// Verify that the account was created and named
439450
expect(mockPersisKeyringHelper).toHaveBeenCalledTimes(1);
440-
expect(mockSetAccountNameAndSelectAccount).toHaveBeenCalledTimes(1);
441-
expect(mockSetAccountNameAndSelectAccount).toHaveBeenCalledWith([
451+
452+
// Wait for any pending promises (including the account finalization)
453+
await waitForAllPromises();
454+
455+
// Verify that setSelectedAccount and setAccountName are called separately
456+
expect(mockSetSelectedAccount).toHaveBeenCalledTimes(1);
457+
expect(mockSetSelectedAccount).toHaveBeenCalledWith([mockAccount.id]);
458+
expect(mockSetAccountName).toHaveBeenCalledTimes(1);
459+
expect(mockSetAccountName).toHaveBeenCalledWith([
442460
mockAccount.id,
443461
mockNameSuggestion,
444462
]);
@@ -484,8 +502,90 @@ describe('Snap Keyring Methods', () => {
484502

485503
// Verify that the account was created and named
486504
expect(mockPersisKeyringHelper).toHaveBeenCalledTimes(1);
487-
expect(mockSetAccountNameAndSelectAccount).toHaveBeenCalledTimes(1);
488-
expect(mockSetAccountNameAndSelectAccount).toHaveBeenCalledWith([
505+
506+
// Wait for any pending promises (including the account finalization)
507+
await waitForAllPromises();
508+
509+
// Verify that setSelectedAccount and setAccountName are called separately
510+
expect(mockSetSelectedAccount).toHaveBeenCalledTimes(1);
511+
expect(mockSetSelectedAccount).toHaveBeenCalledWith([mockAccount.id]);
512+
expect(mockSetAccountName).toHaveBeenCalledTimes(1);
513+
expect(mockSetAccountName).toHaveBeenCalledWith([
514+
mockAccount.id,
515+
mockNameSuggestion,
516+
]);
517+
});
518+
519+
it('always sets selected account for both preinstalled and non-preinstalled snaps with default options', async () => {
520+
// Test with preinstalled snap first
521+
(isSnapPreinstalled as jest.Mock).mockReturnValue(true);
522+
523+
const mockNameSuggestion = 'suggested name';
524+
mockAddRequest.mockReturnValueOnce({
525+
success: true,
526+
name: mockNameSuggestion,
527+
});
528+
529+
const builder = createSnapKeyringBuilder();
530+
await builder().handleKeyringSnapMessage(mockSnapId, {
531+
method: KeyringEvent.AccountCreated,
532+
params: {
533+
account: mockAccount,
534+
displayConfirmation: false,
535+
accountNameSuggestion: mockNameSuggestion,
536+
},
537+
});
538+
539+
// Wait for any pending promises (including the account finalization)
540+
await waitForAllPromises();
541+
542+
// Verify that both setSelectedAccount and setAccountName are called for preinstalled snap
543+
expect(mockSetSelectedAccount).toHaveBeenCalledTimes(1);
544+
expect(mockSetSelectedAccount).toHaveBeenCalledWith([mockAccount.id]);
545+
expect(mockSetAccountName).toHaveBeenCalledTimes(1);
546+
expect(mockSetAccountName).toHaveBeenCalledWith([
547+
mockAccount.id,
548+
mockNameSuggestion,
549+
]);
550+
551+
// Reset mocks for second test and set them up again
552+
mockSetSelectedAccount.mockReset();
553+
mockSetAccountName.mockReset();
554+
mockAddRequest.mockReset();
555+
mockPersisKeyringHelper.mockReset();
556+
mockStartFlow.mockReset();
557+
mockEndFlow.mockReset();
558+
mockGetAccounts.mockReset();
559+
560+
// Set up mocks for second test
561+
mockAddRequest.mockReturnValueOnce({
562+
success: true,
563+
name: mockNameSuggestion,
564+
});
565+
mockStartFlow.mockReturnValue({ id: mockFlowId });
566+
mockEndFlow.mockReturnValue(true);
567+
mockGetAccounts.mockResolvedValue([]);
568+
569+
// Test with non-preinstalled snap
570+
(isSnapPreinstalled as jest.Mock).mockReturnValue(false);
571+
572+
await builder().handleKeyringSnapMessage(mockSnapId, {
573+
method: KeyringEvent.AccountCreated,
574+
params: {
575+
account: mockAccount,
576+
displayConfirmation: false,
577+
accountNameSuggestion: mockNameSuggestion,
578+
},
579+
});
580+
581+
// Wait for any pending promises (including the account finalization)
582+
await waitForAllPromises();
583+
584+
// Verify that both setSelectedAccount and setAccountName are called for non-preinstalled snap too
585+
expect(mockSetSelectedAccount).toHaveBeenCalledTimes(1);
586+
expect(mockSetSelectedAccount).toHaveBeenCalledWith([mockAccount.id]);
587+
expect(mockSetAccountName).toHaveBeenCalledTimes(1);
588+
expect(mockSetAccountName).toHaveBeenCalledWith([
489589
mockAccount.id,
490590
mockNameSuggestion,
491591
]);

app/core/SnapKeyring/SnapKeyring.ts

Lines changed: 18 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -16,9 +16,7 @@ import { getTraceTags } from '../../util/sentry/tags';
1616
import { store } from '../../store';
1717
import { MetaMetricsEvents } from '../../core/Analytics/MetaMetrics.events';
1818
import { trackSnapAccountEvent } from '../Analytics/helpers/SnapKeyring/trackSnapAccountEvent';
19-
import {
20-
endPerformanceTrace,
21-
} from '../../core/redux/slices/performance';
19+
import { endPerformanceTrace } from '../../core/redux/slices/performance';
2220
import { PerformanceEventNames } from '../redux/slices/performance/constants';
2321

2422
/**
@@ -148,11 +146,13 @@ class SnapKeyringImpl implements SnapKeyringCallbacks {
148146
private async addAccountFinalize({
149147
address: _address,
150148
snapId,
149+
skipSetSelectedAccountStep,
151150
accountName,
152151
onceSaved,
153152
}: {
154153
address: string;
155154
snapId: SnapId;
155+
skipSetSelectedAccountStep: boolean;
156156
onceSaved: Promise<string>;
157157
accountName?: string;
158158
}) {
@@ -172,17 +172,20 @@ class SnapKeyringImpl implements SnapKeyringCallbacks {
172172
// state, so we can safely uses this state to run post-processing.
173173
// (e.g. renaming the account, select the account, etc...)
174174

175-
// Set the selected account to the new account
176-
if (accountName) {
175+
if (!skipSetSelectedAccountStep) {
176+
// Set the selected account to the new account
177177
this.#messenger.call(
178-
'AccountsController:setAccountNameAndSelectAccount',
178+
'AccountsController:setSelectedAccount',
179179
accountId,
180-
accountName,
181180
);
182-
} else {
181+
}
182+
183+
if (accountName) {
184+
// Set the account name if one is provided
183185
this.#messenger.call(
184-
'AccountsController:setSelectedAccount',
186+
'AccountsController:setAccountName',
185187
accountId,
188+
accountName,
186189
);
187190
}
188191

@@ -220,6 +223,7 @@ class SnapKeyringImpl implements SnapKeyringCallbacks {
220223
accountNameSuggestion: string = '',
221224
{
222225
displayAccountNameSuggestion,
226+
setSelectedAccount,
223227
}: SnapKeyringInternalOptions = getDefaultInternalOptions(),
224228
) {
225229
assertIsValidSnapId(snapId);
@@ -237,13 +241,18 @@ class SnapKeyringImpl implements SnapKeyringCallbacks {
237241
skipAccountNameSuggestionDialog,
238242
});
239243

244+
// Only pre-installed Snaps can skip the account from being selected.
245+
const skipSetSelectedAccountStep =
246+
isSnapPreinstalled(snapId) && !setSelectedAccount;
247+
240248
// The second part is about selecting the newly created account and showing some other
241249
// confirmation dialogs (or error dialogs if anything goes wrong while persisting the account
242250
// into the state.
243251
// eslint-disable-next-line no-void
244252
void this.addAccountFinalize({
245253
address,
246254
snapId,
255+
skipSetSelectedAccountStep,
247256
onceSaved,
248257
accountName,
249258
});

0 commit comments

Comments
 (0)