Skip to content

Commit ee729bf

Browse files
mikespositoGudahtt
authored and
DDDDDanica
committed
fix: cp-12.18.0 discard duplicate accounts on unlock (#32621)
<!-- 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. --> <!-- 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? 2. What is the improvement/solution? --> Updating `@metamask/keyring-controller` to `^22.0.0`: ```markdown - **BREAKING** `keyringsMetadata` has been removed from the controller state ([#5725](MetaMask/core#5725)) - The metadata is now stored in each keyring object in the `state.keyrings` array. - When updating to this version, we recommend removing the `keyringsMetadata` state and all state referencing a keyring ID with a migration. New metadata will be generated for each keyring automatically after the update. - Keyrings with duplicate accounts are skipped as unsupported on unlock ([#5775](MetaMask/core#5775)) - Prevent emitting `:stateChange` from `withKeyring` unnecessarily ([#5732](MetaMask/core#5732)) ``` and `@metamask/accounts-controller` to `^29.0.0`: ```markdown - **BREAKING:** bump `@metamask/keyring-controller` peer dependency to `^22.0.0` ([#5802](MetaMask/core#5802)) - Add new `setAccountNameAndSelectAccount` action ([#5714](MetaMask/core#5714)) - Add `entropySource` and `derivationPath` to EVM HD account options ([#5618](MetaMask/core#5618)) - **BREAKING:** Bump `@metamask/snaps-controllers` peer dependency from `^9.19.0` to `^11.0.0` ([#5639](MetaMask/core#5639)) - **BREAKING:** Bump `@metamask/providers` peer dependency from `^18.1.0` to `^21.0.0` ([#5639](MetaMask/core#5639)) - Bump `@metamask/base-controller` from `^8.0.0` to `^8.0.1` ([#5722](MetaMask/core#5722)) - Bump `@metamask/snaps-sdk` from `^6.17.1` to `^6.22.0` ([#5639](MetaMask/core#5639)) - Bump `@metamask/snaps-utils` from `^8.10.0` to `^9.2.0` ([#5639](MetaMask/core#5639)) - Bump `@metamask/eth-snap-keyring` from `^12.0.0` to `^12.1.1` ([#5565](MetaMask/core#5565)) - Bump `@metamask/keyring-api` from `^17.2.0` to `^17.4.0` ([#5565](MetaMask/core#5565)) - Bump `@metamask/keyring-internal-api` from `^6.0.0` to `^6.0.1` ([#5565](MetaMask/core#5565)) - Do not fire events during `update` blocks ([#5555](MetaMask/core#5555)) - Prevent unnecessary state updates when updating `InternalAccount.metadata.snap` ([#5735](MetaMask/core#5735)) ``` [![Open in GitHub Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/32621?quickstart=1) Fixes: #32935 1.1. Switch to the `v11.7.3` branch, build locally, and install the extension from `chrome://extensions` 1.2. Onboard with an SRP of which we know the second account (`child guilt hollow arrive average popular nasty soon summer like scheme diary pill country rapid`) 1.3. Import an account that is part of the mnemonic ( `0x80842b7e3cfb1118e86a427cdec418e3b4179ef5bbbfd71c02a76349831c8a8b` which is the account at index 2 of the above SRP) 1.4. Add a new account on the main HD 1.5. Switch to `Version-v12.17.1` branch, and refresh the extension in `chrome://extensions` 1.6. Unlock the wallet, you should see duplicates in your accounts list and you won't be able to add new accounts 2.1. Switch to this branch, build locally, and refresh the extension in `chrome://extensions` 2.2 Unlock the wallet, you shouldn't see duplicate accounts anymore, and you should be able to add new accounts <!-- If applicable, add screenshots and/or recordings to visualize the before and after of your change. --> <!-- [screenshots/recordings] --> <!-- [screenshots/recordings] --> - [ ] I've followed [MetaMask Contributor Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask Extension Coding Standards](https://github.com/MetaMask/metamask-extension/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-extension/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: Mark Stacey <[email protected]>
1 parent 1187ad2 commit ee729bf

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

41 files changed

+720
-274
lines changed

‎.storybook/test-data.js

Lines changed: 8 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -1254,20 +1254,18 @@ const state = {
12541254
'0x64a845a5b02460acf8a3d84503b0d68d028b4bb4',
12551255
'0xb19ac54efa18cc3a14a5b821bfec73d284bf0c5e',
12561256
],
1257+
metadata: {
1258+
id: '01JN08SYECPZHFHB3K0J1NHJ4H',
1259+
name: '',
1260+
},
12571261
},
12581262
{
12591263
type: KeyringType.ledger,
12601264
accounts: ['0x9d0ba4ddac06032527b140912ec808ab9451b788'],
1261-
},
1262-
],
1263-
keyringsMetadata: [
1264-
{
1265-
id: '01JN08SYECPZHFHB3K0J1NHJ4H',
1266-
name: '',
1267-
},
1268-
{
1269-
id: '01JN08T38HEXPYQX2HKP1FCRMZ',
1270-
name: '',
1265+
metadata: {
1266+
id: '01JN08T38HEXPYQX2HKP1FCRMZ',
1267+
name: '',
1268+
},
12711269
},
12721270
],
12731271
...mockNetworkState(

‎app/scripts/controllers/mmi-controller.test.ts

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -177,7 +177,6 @@ describe('MMIController', function () {
177177
state: {
178178
keyrings: [],
179179
isUnlocked: true,
180-
keyringsMetadata: [],
181180
},
182181
encryptor: {
183182
encrypt(_, object) {

‎app/scripts/controllers/preferences-controller.test.ts

Lines changed: 20 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -152,12 +152,10 @@ describe('preferences controller', () => {
152152
{
153153
type: 'HD Key Tree',
154154
accounts: [firstAddress, secondAddress],
155-
},
156-
],
157-
keyringsMetadata: [
158-
{
159-
id: '01JKDGGBRE3DGZA7N1PZJSQK4W',
160-
name: '',
155+
metadata: {
156+
id: '01JKDGGBRE3DGZA7N1PZJSQK4W',
157+
name: '',
158+
},
161159
},
162160
],
163161
},
@@ -203,12 +201,10 @@ describe('preferences controller', () => {
203201
{
204202
type: 'HD Key Tree',
205203
accounts: [firstAddress, secondAddress],
206-
},
207-
],
208-
keyringsMetadata: [
209-
{
210-
id: '01JKDGGBRE3DGZA7N1PZJSQK4W',
211-
name: '',
204+
metadata: {
205+
id: '01JKDGGBRE3DGZA7N1PZJSQK4W',
206+
name: '',
207+
},
212208
},
213209
],
214210
},
@@ -260,12 +256,10 @@ describe('preferences controller', () => {
260256
{
261257
type: 'HD Key Tree',
262258
accounts: [firstAddress, secondAddress],
263-
},
264-
],
265-
keyringsMetadata: [
266-
{
267-
id: '01JKDGGBRE3DGZA7N1PZJSQK4W',
268-
name: '',
259+
metadata: {
260+
id: '01JKDGGBRE3DGZA7N1PZJSQK4W',
261+
name: '',
262+
},
269263
},
270264
],
271265
},
@@ -301,12 +295,10 @@ describe('preferences controller', () => {
301295
{
302296
type: 'HD Key Tree',
303297
accounts: [firstAddress, secondAddress],
304-
},
305-
],
306-
keyringsMetadata: [
307-
{
308-
id: '01JKDGGBRE3DGZA7N1PZJSQK4W',
309-
name: '',
298+
metadata: {
299+
id: '01JKDGGBRE3DGZA7N1PZJSQK4W',
300+
name: '',
301+
},
310302
},
311303
],
312304
},
@@ -511,12 +503,10 @@ describe('preferences controller', () => {
511503
{
512504
type: 'HD Key Tree',
513505
accounts: [firstAddress, secondAddress],
514-
},
515-
],
516-
keyringsMetadata: [
517-
{
518-
id: '01JKDGGBRE3DGZA7N1PZJSQK4W',
519-
name: '',
506+
metadata: {
507+
id: '01JKDGGBRE3DGZA7N1PZJSQK4W',
508+
name: '',
509+
},
520510
},
521511
],
522512
},

‎app/scripts/metamask-controller.actions.test.js

Lines changed: 11 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -211,18 +211,19 @@ describe('MetaMaskController', function () {
211211
const result2 = metamaskController.keyringController.state;
212212

213213
// On restore, a new keyring metadata is generated.
214-
expect(result1.keyringsMetadata[0].id).toBe(mockULIDs[0]);
215-
expect(result2).toStrictEqual(
216-
expect.objectContaining({
217-
...result1,
218-
keyringsMetadata: [
219-
{
214+
expect(result1.keyrings[0].metadata.id).toBe(mockULIDs[0]);
215+
expect(result2).toStrictEqual({
216+
...result1,
217+
keyrings: [
218+
{
219+
...result1.keyrings[0],
220+
metadata: {
221+
...result1.keyrings[0].metadata,
220222
id: mockULIDs[1],
221-
name: '',
222223
},
223-
],
224-
}),
225-
);
224+
},
225+
],
226+
});
226227
});
227228
});
228229

‎app/scripts/metamask-controller.js

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -7025,8 +7025,8 @@ export default class MetamaskController extends EventEmitter {
70257025
.map((keyring, index) => {
70267026
if (keyring.type === KeyringTypes.hd) {
70277027
return {
7028-
id: state.keyringsMetadata[index].id,
7029-
name: state.keyringsMetadata[index].name,
7028+
id: keyring.metadata.id,
7029+
name: keyring.metadata.name,
70307030
type: 'mnemonic',
70317031
primary: index === 0,
70327032
};

‎app/scripts/metamask-controller.test.js

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -3989,10 +3989,9 @@ describe('MetaMaskController', () => {
39893989
metamaskController.keyringController.state.keyrings;
39903990

39913991
const newlyAddedKeyringId =
3992-
metamaskController.keyringController.state.keyringsMetadata[
3993-
metamaskController.keyringController.state.keyringsMetadata.length -
3994-
2 // -1 for the snap keyring, -1 for the newly added keyring
3995-
].id;
3992+
metamaskController.keyringController.state.keyrings[
3993+
metamaskController.keyringController.state.keyrings.length - 2 // -1 for the snap keyring, -1 for the newly added keyring
3994+
].metadata.id;
39963995

39973996
const newSRP = Buffer.from(
39983997
await metamaskController.getSeedPhrase(password, newlyAddedKeyringId),

‎app/scripts/migrations/156.1.test.ts

Lines changed: 101 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,101 @@
1+
import { migrate } from './156.1';
2+
3+
const expectedVersion = 156.1;
4+
const previousVersion = 156;
5+
6+
describe(`migration #${expectedVersion}`, () => {
7+
it('does nothing if state has no KeyringController property', async () => {
8+
const oldVersionedData = {
9+
meta: { version: previousVersion },
10+
data: {},
11+
};
12+
const expectedVersionedData = {
13+
meta: { version: expectedVersion },
14+
data: oldVersionedData.data,
15+
};
16+
17+
const newVersionedData = await migrate(oldVersionedData);
18+
19+
expect(newVersionedData).toStrictEqual(expectedVersionedData);
20+
});
21+
22+
it('does nothing if state.KeyringController is not an object', async () => {
23+
const oldVersionedData = {
24+
meta: { version: previousVersion },
25+
data: {
26+
KeyringController: 'not-an-object',
27+
},
28+
};
29+
const expectedVersionedData = {
30+
meta: { version: expectedVersion },
31+
data: oldVersionedData.data,
32+
};
33+
34+
const newVersionedData = await migrate(oldVersionedData);
35+
36+
expect(newVersionedData).toStrictEqual(expectedVersionedData);
37+
});
38+
39+
it('does nothing if state.KeyringController has no keyringsMetadata property', async () => {
40+
const oldVersionedData = {
41+
meta: { version: previousVersion },
42+
data: {
43+
KeyringController: {
44+
vault: 'vault',
45+
keyrings: [
46+
{
47+
type: 'HD Key Tree',
48+
accounts: ['0x1234567890abcdef'],
49+
},
50+
],
51+
},
52+
},
53+
};
54+
const expectedVersionedData = {
55+
meta: { version: expectedVersion },
56+
data: oldVersionedData.data,
57+
};
58+
59+
const newVersionedData = await migrate(oldVersionedData);
60+
61+
expect(newVersionedData).toStrictEqual(expectedVersionedData);
62+
});
63+
64+
it('removes keyringsMetadata from KeyringController state', async () => {
65+
const oldVersionedData = {
66+
meta: { version: previousVersion },
67+
data: {
68+
KeyringController: {
69+
vault: 'vault',
70+
keyrings: [
71+
{
72+
type: 'HD Key Tree',
73+
accounts: ['0x1234567890abcdef'],
74+
},
75+
],
76+
keyringsMetadata: {
77+
someKey: 'someValue',
78+
},
79+
},
80+
},
81+
};
82+
const expectedVersionedData = {
83+
meta: { version: expectedVersion },
84+
data: {
85+
KeyringController: {
86+
vault: 'vault',
87+
keyrings: [
88+
{
89+
type: 'HD Key Tree',
90+
accounts: ['0x1234567890abcdef'],
91+
},
92+
],
93+
},
94+
},
95+
};
96+
97+
const newVersionedData = await migrate(oldVersionedData);
98+
99+
expect(newVersionedData).toStrictEqual(expectedVersionedData);
100+
});
101+
});

‎app/scripts/migrations/156.1.ts

Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,37 @@
1+
import { hasProperty } from '@metamask/utils';
2+
import { cloneDeep, isObject } from 'lodash';
3+
4+
export const version = 156.1;
5+
6+
/**
7+
* Remove `keyringsMetadata` from `KeyringController` state.
8+
*
9+
* @param originalVersionedData - Versioned MetaMask extension state, exactly what we persist to dist.
10+
* @param originalVersionedData.meta - State metadata.
11+
* @param originalVersionedData.meta.version - The current state version.
12+
* @param originalVersionedData.data - The persisted MetaMask state, keyed by controller.
13+
* @returns Updated versioned MetaMask extension state.
14+
*/
15+
export async function migrate(originalVersionedData: {
16+
meta: { version: number };
17+
data: Record<string, unknown>;
18+
}) {
19+
const versionedData = cloneDeep(originalVersionedData);
20+
versionedData.meta.version = version;
21+
versionedData.data = transformState(versionedData.data);
22+
return versionedData;
23+
}
24+
25+
function transformState(state: Record<string, unknown>) {
26+
const newState = state;
27+
28+
if (
29+
hasProperty(newState, 'KeyringController') &&
30+
isObject(newState.KeyringController) &&
31+
hasProperty(newState.KeyringController, 'keyringsMetadata')
32+
) {
33+
delete newState.KeyringController.keyringsMetadata;
34+
}
35+
36+
return newState;
37+
}

‎app/scripts/migrations/index.js

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -185,6 +185,7 @@ const migrations = [
185185
require('./154'),
186186
require('./155'),
187187
require('./156'),
188+
require('./156.1'),
188189
];
189190

190191
export default migrations;

‎lavamoat/browserify/beta/policy.json

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1671,8 +1671,7 @@
16711671
},
16721672
"@metamask/keyring-controller": {
16731673
"globals": {
1674-
"console.error": true,
1675-
"console.log": true
1674+
"console.error": true
16761675
},
16771676
"packages": {
16781677
"@ethereumjs/tx>@ethereumjs/util": true,
@@ -1684,6 +1683,7 @@
16841683
"@metamask/utils": true,
16851684
"@metamask/name-controller>async-mutex": true,
16861685
"@metamask/keyring-controller>ethereumjs-wallet": true,
1686+
"lodash": true,
16871687
"@metamask/keyring-controller>ulid": true
16881688
}
16891689
},

‎lavamoat/browserify/flask/policy.json

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1671,8 +1671,7 @@
16711671
},
16721672
"@metamask/keyring-controller": {
16731673
"globals": {
1674-
"console.error": true,
1675-
"console.log": true
1674+
"console.error": true
16761675
},
16771676
"packages": {
16781677
"@ethereumjs/tx>@ethereumjs/util": true,
@@ -1684,6 +1683,7 @@
16841683
"@metamask/utils": true,
16851684
"@metamask/name-controller>async-mutex": true,
16861685
"@metamask/keyring-controller>ethereumjs-wallet": true,
1686+
"lodash": true,
16871687
"@metamask/keyring-controller>ulid": true
16881688
}
16891689
},

‎lavamoat/browserify/main/policy.json

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1671,8 +1671,7 @@
16711671
},
16721672
"@metamask/keyring-controller": {
16731673
"globals": {
1674-
"console.error": true,
1675-
"console.log": true
1674+
"console.error": true
16761675
},
16771676
"packages": {
16781677
"@ethereumjs/tx>@ethereumjs/util": true,
@@ -1684,6 +1683,7 @@
16841683
"@metamask/utils": true,
16851684
"@metamask/name-controller>async-mutex": true,
16861685
"@metamask/keyring-controller>ethereumjs-wallet": true,
1686+
"lodash": true,
16871687
"@metamask/keyring-controller>ulid": true
16881688
}
16891689
},

‎lavamoat/browserify/mmi/policy.json

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1802,8 +1802,7 @@
18021802
},
18031803
"@metamask/keyring-controller": {
18041804
"globals": {
1805-
"console.error": true,
1806-
"console.log": true
1805+
"console.error": true
18071806
},
18081807
"packages": {
18091808
"@ethereumjs/tx>@ethereumjs/util": true,
@@ -1815,6 +1814,7 @@
18151814
"@metamask/utils": true,
18161815
"@metamask/name-controller>async-mutex": true,
18171816
"@metamask/keyring-controller>ethereumjs-wallet": true,
1817+
"lodash": true,
18181818
"@metamask/keyring-controller>ulid": true
18191819
}
18201820
},

0 commit comments

Comments
 (0)