Skip to content

Commit 759b92e

Browse files
montelaidevmetamaskbot
montelaidev
and
metamaskbot
authored
fix: bump accounts controller and migration to fix undefined selectedAccount (MetaMask#26573)
<!-- 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 bumps the `AccountsController` and introduces a new migration. The `updateAccounts` methods from the `AccountsController` now checks if the selectedAccount is undefined and recovers from this. The migration updates the selectedAccount values that are not defined. <!-- 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? --> [![Open in GitHub Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/26573?quickstart=1) ## **Related issues** Fixes: MetaMask#26377 ## **Manual testing steps** 1. Go to this page... 2. 3. ## **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 Extension Coding Standards](https://github.com/MetaMask/metamask-extension/blob/develop/.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-extension/blob/develop/.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: MetaMask Bot <[email protected]>
1 parent d81d69b commit 759b92e

File tree

18 files changed

+333
-41
lines changed

18 files changed

+333
-41
lines changed

‎app/scripts/lib/transaction/util.test.ts

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@ import {
1818
} from '../../../../shared/constants/security-provider';
1919
import { SecurityAlertResponse } from '../ppom/types';
2020
import { flushPromises } from '../../../../test/lib/timer-helpers';
21+
import { createMockInternalAccount } from '../../../../test/jest/mocks';
2122
import {
2223
AddDappTransactionRequest,
2324
AddTransactionOptions,
@@ -40,6 +41,9 @@ jest.mock('uuid', () => {
4041
const SECURITY_ALERT_ID_MOCK = '123';
4142

4243
const INTERNAL_ACCOUNT_ADDRESS = '0xec1adf982415d2ef5ec55899b9bfb8bc0f29251b';
44+
const INTERNAL_ACCOUNT = createMockInternalAccount({
45+
address: INTERNAL_ACCOUNT_ADDRESS,
46+
});
4347

4448
const TRANSACTION_PARAMS_MOCK: TransactionParams = {
4549
from: '0x1',
@@ -484,7 +488,7 @@ describe('Transaction Utils', () => {
484488
...sendRequest,
485489
securityAlertsEnabled: false,
486490
chainId: '0x1',
487-
internalAccounts: [{ address: INTERNAL_ACCOUNT_ADDRESS }],
491+
internalAccounts: [INTERNAL_ACCOUNT],
488492
});
489493

490494
expect(

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

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -72,6 +72,7 @@ function expectedInternalAccount(
7272
type: 'HD Key Tree',
7373
},
7474
lastSelected: lastSelected ? expect.any(Number) : undefined,
75+
importTime: 0,
7576
},
7677
options: {},
7778
methods: ETH_EOA_METHODS,

‎app/scripts/migrations/105.ts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -97,6 +97,7 @@ function createInternalAccountsForAccountsController(
9797
metadata: {
9898
name: identity.name,
9999
lastSelected: identity.lastSelected ?? undefined,
100+
importTime: 0,
100101
keyring: {
101102
// This is default HD Key Tree type because the keyring is encrypted
102103
// during migration, the type will get updated when the during the

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

Lines changed: 69 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,69 @@
1+
import { AccountsControllerState } from '@metamask/accounts-controller';
2+
import { createMockInternalAccount } from '../../../test/jest/mocks';
3+
import { migrate, version } from './126';
4+
5+
const oldVersion = 125;
6+
7+
const mockInternalAccount = createMockInternalAccount();
8+
const mockAccountsControllerState: AccountsControllerState = {
9+
internalAccounts: {
10+
accounts: {
11+
[mockInternalAccount.id]: mockInternalAccount,
12+
},
13+
selectedAccount: mockInternalAccount.id,
14+
},
15+
};
16+
17+
describe('migration #126', () => {
18+
afterEach(() => jest.resetAllMocks());
19+
20+
it('updates the version metadata', async () => {
21+
const oldStorage = {
22+
meta: { version: oldVersion },
23+
data: {
24+
AccountsController: mockAccountsControllerState,
25+
},
26+
};
27+
28+
const newStorage = await migrate(oldStorage);
29+
expect(newStorage.meta).toStrictEqual({ version });
30+
});
31+
32+
it('updates selected account if it is not found in the list of accounts', async () => {
33+
const oldStorage = {
34+
meta: { version: oldVersion },
35+
data: {
36+
AccountsController: {
37+
...mockAccountsControllerState,
38+
internalAccounts: {
39+
...mockAccountsControllerState.internalAccounts,
40+
selectedAccount: 'unknown id',
41+
},
42+
},
43+
},
44+
};
45+
46+
const newStorage = await migrate(oldStorage);
47+
const {
48+
internalAccounts: { selectedAccount },
49+
} = newStorage.data.AccountsController as AccountsControllerState;
50+
expect(selectedAccount).toStrictEqual(mockInternalAccount.id);
51+
expect(newStorage.data.AccountsController).toStrictEqual(
52+
mockAccountsControllerState,
53+
);
54+
});
55+
56+
it('does nothing if the selectedAccount is found in the list of accounts', async () => {
57+
const oldStorage = {
58+
meta: { version: oldVersion },
59+
data: {
60+
AccountsController: mockAccountsControllerState,
61+
},
62+
};
63+
64+
const newStorage = await migrate(oldStorage);
65+
expect(newStorage.data.AccountsController).toStrictEqual(
66+
mockAccountsControllerState,
67+
);
68+
});
69+
});

‎app/scripts/migrations/126.ts

Lines changed: 49 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,49 @@
1+
import { AccountsControllerState } from '@metamask/accounts-controller';
2+
import { cloneDeep } from 'lodash';
3+
4+
type VersionedData = {
5+
meta: { version: number };
6+
data: Record<string, unknown>;
7+
};
8+
9+
export const version = 126;
10+
11+
/**
12+
* This migration removes depreciated `Txcontroller` key if it is present in state.
13+
*
14+
* @param originalVersionedData - Versioned MetaMask extension state, exactly
15+
* what we persist to dist.
16+
* @param originalVersionedData.meta - State metadata.
17+
* @param originalVersionedData.meta.version - The current state version.
18+
* @param originalVersionedData.data - The persisted MetaMask state, keyed by
19+
* controller.
20+
* @returns Updated versioned MetaMask extension state.
21+
*/
22+
export async function migrate(
23+
originalVersionedData: VersionedData,
24+
): Promise<VersionedData> {
25+
const versionedData = cloneDeep(originalVersionedData);
26+
versionedData.meta.version = version;
27+
transformState(versionedData.data);
28+
return versionedData;
29+
}
30+
31+
function transformState(state: Record<string, unknown>) {
32+
const accountsControllerState = state?.AccountsController as
33+
| AccountsControllerState
34+
| undefined;
35+
36+
if (
37+
accountsControllerState &&
38+
Object.values(accountsControllerState?.internalAccounts.accounts).length >
39+
0 &&
40+
!accountsControllerState?.internalAccounts.accounts[
41+
accountsControllerState?.internalAccounts.selectedAccount
42+
]
43+
) {
44+
accountsControllerState.internalAccounts.selectedAccount = Object.values(
45+
accountsControllerState?.internalAccounts.accounts,
46+
)[0].id;
47+
}
48+
return state;
49+
}

‎app/scripts/migrations/index.js

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -142,6 +142,7 @@ const migrations = [
142142
require('./123'),
143143
require('./124'),
144144
require('./125'),
145+
require('./126'),
145146
];
146147

147148
export default migrations;

‎lavamoat/browserify/beta/policy.json

Lines changed: 33 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -790,10 +790,10 @@
790790
"@ethereumjs/tx>@ethereumjs/util": true,
791791
"@ethereumjs/tx>ethereum-cryptography": true,
792792
"@metamask/accounts-controller>@metamask/base-controller": true,
793+
"@metamask/accounts-controller>@metamask/utils": true,
793794
"@metamask/eth-snap-keyring": true,
794795
"@metamask/keyring-api": true,
795796
"@metamask/keyring-controller": true,
796-
"@metamask/utils": true,
797797
"uuid": true
798798
}
799799
},
@@ -805,6 +805,21 @@
805805
"immer": true
806806
}
807807
},
808+
"@metamask/accounts-controller>@metamask/utils": {
809+
"globals": {
810+
"TextDecoder": true,
811+
"TextEncoder": true
812+
},
813+
"packages": {
814+
"@metamask/utils>@metamask/superstruct": true,
815+
"@metamask/utils>@scure/base": true,
816+
"@metamask/utils>pony-cause": true,
817+
"@noble/hashes": true,
818+
"browserify>buffer": true,
819+
"nock>debug": true,
820+
"semver": true
821+
}
822+
},
808823
"@metamask/address-book-controller": {
809824
"packages": {
810825
"@metamask/address-book-controller>@metamask/controller-utils": true,
@@ -1610,10 +1625,25 @@
16101625
"URL": true
16111626
},
16121627
"packages": {
1628+
"@metamask/keyring-api>@metamask/utils": true,
16131629
"@metamask/keyring-api>bech32": true,
16141630
"@metamask/keyring-api>uuid": true,
1615-
"@metamask/utils": true,
1616-
"superstruct": true
1631+
"@metamask/utils>@metamask/superstruct": true
1632+
}
1633+
},
1634+
"@metamask/keyring-api>@metamask/utils": {
1635+
"globals": {
1636+
"TextDecoder": true,
1637+
"TextEncoder": true
1638+
},
1639+
"packages": {
1640+
"@metamask/utils>@metamask/superstruct": true,
1641+
"@metamask/utils>@scure/base": true,
1642+
"@metamask/utils>pony-cause": true,
1643+
"@noble/hashes": true,
1644+
"browserify>buffer": true,
1645+
"nock>debug": true,
1646+
"semver": true
16171647
}
16181648
},
16191649
"@metamask/keyring-api>uuid": {

‎lavamoat/browserify/flask/policy.json

Lines changed: 33 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -790,10 +790,10 @@
790790
"@ethereumjs/tx>@ethereumjs/util": true,
791791
"@ethereumjs/tx>ethereum-cryptography": true,
792792
"@metamask/accounts-controller>@metamask/base-controller": true,
793+
"@metamask/accounts-controller>@metamask/utils": true,
793794
"@metamask/eth-snap-keyring": true,
794795
"@metamask/keyring-api": true,
795796
"@metamask/keyring-controller": true,
796-
"@metamask/utils": true,
797797
"uuid": true
798798
}
799799
},
@@ -805,6 +805,21 @@
805805
"immer": true
806806
}
807807
},
808+
"@metamask/accounts-controller>@metamask/utils": {
809+
"globals": {
810+
"TextDecoder": true,
811+
"TextEncoder": true
812+
},
813+
"packages": {
814+
"@metamask/utils>@metamask/superstruct": true,
815+
"@metamask/utils>@scure/base": true,
816+
"@metamask/utils>pony-cause": true,
817+
"@noble/hashes": true,
818+
"browserify>buffer": true,
819+
"nock>debug": true,
820+
"semver": true
821+
}
822+
},
808823
"@metamask/address-book-controller": {
809824
"packages": {
810825
"@metamask/address-book-controller>@metamask/controller-utils": true,
@@ -1610,10 +1625,25 @@
16101625
"URL": true
16111626
},
16121627
"packages": {
1628+
"@metamask/keyring-api>@metamask/utils": true,
16131629
"@metamask/keyring-api>bech32": true,
16141630
"@metamask/keyring-api>uuid": true,
1615-
"@metamask/utils": true,
1616-
"superstruct": true
1631+
"@metamask/utils>@metamask/superstruct": true
1632+
}
1633+
},
1634+
"@metamask/keyring-api>@metamask/utils": {
1635+
"globals": {
1636+
"TextDecoder": true,
1637+
"TextEncoder": true
1638+
},
1639+
"packages": {
1640+
"@metamask/utils>@metamask/superstruct": true,
1641+
"@metamask/utils>@scure/base": true,
1642+
"@metamask/utils>pony-cause": true,
1643+
"@noble/hashes": true,
1644+
"browserify>buffer": true,
1645+
"nock>debug": true,
1646+
"semver": true
16171647
}
16181648
},
16191649
"@metamask/keyring-api>uuid": {

‎lavamoat/browserify/main/policy.json

Lines changed: 33 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -790,10 +790,10 @@
790790
"@ethereumjs/tx>@ethereumjs/util": true,
791791
"@ethereumjs/tx>ethereum-cryptography": true,
792792
"@metamask/accounts-controller>@metamask/base-controller": true,
793+
"@metamask/accounts-controller>@metamask/utils": true,
793794
"@metamask/eth-snap-keyring": true,
794795
"@metamask/keyring-api": true,
795796
"@metamask/keyring-controller": true,
796-
"@metamask/utils": true,
797797
"uuid": true
798798
}
799799
},
@@ -805,6 +805,21 @@
805805
"immer": true
806806
}
807807
},
808+
"@metamask/accounts-controller>@metamask/utils": {
809+
"globals": {
810+
"TextDecoder": true,
811+
"TextEncoder": true
812+
},
813+
"packages": {
814+
"@metamask/utils>@metamask/superstruct": true,
815+
"@metamask/utils>@scure/base": true,
816+
"@metamask/utils>pony-cause": true,
817+
"@noble/hashes": true,
818+
"browserify>buffer": true,
819+
"nock>debug": true,
820+
"semver": true
821+
}
822+
},
808823
"@metamask/address-book-controller": {
809824
"packages": {
810825
"@metamask/address-book-controller>@metamask/controller-utils": true,
@@ -1610,10 +1625,25 @@
16101625
"URL": true
16111626
},
16121627
"packages": {
1628+
"@metamask/keyring-api>@metamask/utils": true,
16131629
"@metamask/keyring-api>bech32": true,
16141630
"@metamask/keyring-api>uuid": true,
1615-
"@metamask/utils": true,
1616-
"superstruct": true
1631+
"@metamask/utils>@metamask/superstruct": true
1632+
}
1633+
},
1634+
"@metamask/keyring-api>@metamask/utils": {
1635+
"globals": {
1636+
"TextDecoder": true,
1637+
"TextEncoder": true
1638+
},
1639+
"packages": {
1640+
"@metamask/utils>@metamask/superstruct": true,
1641+
"@metamask/utils>@scure/base": true,
1642+
"@metamask/utils>pony-cause": true,
1643+
"@noble/hashes": true,
1644+
"browserify>buffer": true,
1645+
"nock>debug": true,
1646+
"semver": true
16171647
}
16181648
},
16191649
"@metamask/keyring-api>uuid": {

0 commit comments

Comments
 (0)