Skip to content

Commit

Permalink
fix: bump accounts controller and migration to fix undefined selected…
Browse files Browse the repository at this point in the history
…Account (#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: #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 <metamaskbot@users.noreply.github.com>
  • Loading branch information
montelaidev and metamaskbot authored Aug 22, 2024
1 parent d81d69b commit 759b92e
Show file tree
Hide file tree
Showing 18 changed files with 333 additions and 41 deletions.
6 changes: 5 additions & 1 deletion app/scripts/lib/transaction/util.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import {
} from '../../../../shared/constants/security-provider';
import { SecurityAlertResponse } from '../ppom/types';
import { flushPromises } from '../../../../test/lib/timer-helpers';
import { createMockInternalAccount } from '../../../../test/jest/mocks';
import {
AddDappTransactionRequest,
AddTransactionOptions,
Expand All @@ -40,6 +41,9 @@ jest.mock('uuid', () => {
const SECURITY_ALERT_ID_MOCK = '123';

const INTERNAL_ACCOUNT_ADDRESS = '0xec1adf982415d2ef5ec55899b9bfb8bc0f29251b';
const INTERNAL_ACCOUNT = createMockInternalAccount({
address: INTERNAL_ACCOUNT_ADDRESS,
});

const TRANSACTION_PARAMS_MOCK: TransactionParams = {
from: '0x1',
Expand Down Expand Up @@ -484,7 +488,7 @@ describe('Transaction Utils', () => {
...sendRequest,
securityAlertsEnabled: false,
chainId: '0x1',
internalAccounts: [{ address: INTERNAL_ACCOUNT_ADDRESS }],
internalAccounts: [INTERNAL_ACCOUNT],
});

expect(
Expand Down
1 change: 1 addition & 0 deletions app/scripts/migrations/105.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,7 @@ function expectedInternalAccount(
type: 'HD Key Tree',
},
lastSelected: lastSelected ? expect.any(Number) : undefined,
importTime: 0,
},
options: {},
methods: ETH_EOA_METHODS,
Expand Down
1 change: 1 addition & 0 deletions app/scripts/migrations/105.ts
Original file line number Diff line number Diff line change
Expand Up @@ -97,6 +97,7 @@ function createInternalAccountsForAccountsController(
metadata: {
name: identity.name,
lastSelected: identity.lastSelected ?? undefined,
importTime: 0,
keyring: {
// This is default HD Key Tree type because the keyring is encrypted
// during migration, the type will get updated when the during the
Expand Down
69 changes: 69 additions & 0 deletions app/scripts/migrations/126.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,69 @@
import { AccountsControllerState } from '@metamask/accounts-controller';
import { createMockInternalAccount } from '../../../test/jest/mocks';
import { migrate, version } from './126';

const oldVersion = 125;

const mockInternalAccount = createMockInternalAccount();
const mockAccountsControllerState: AccountsControllerState = {
internalAccounts: {
accounts: {
[mockInternalAccount.id]: mockInternalAccount,
},
selectedAccount: mockInternalAccount.id,
},
};

describe('migration #126', () => {
afterEach(() => jest.resetAllMocks());

it('updates the version metadata', async () => {
const oldStorage = {
meta: { version: oldVersion },
data: {
AccountsController: mockAccountsControllerState,
},
};

const newStorage = await migrate(oldStorage);
expect(newStorage.meta).toStrictEqual({ version });
});

it('updates selected account if it is not found in the list of accounts', async () => {
const oldStorage = {
meta: { version: oldVersion },
data: {
AccountsController: {
...mockAccountsControllerState,
internalAccounts: {
...mockAccountsControllerState.internalAccounts,
selectedAccount: 'unknown id',
},
},
},
};

const newStorage = await migrate(oldStorage);
const {
internalAccounts: { selectedAccount },
} = newStorage.data.AccountsController as AccountsControllerState;
expect(selectedAccount).toStrictEqual(mockInternalAccount.id);
expect(newStorage.data.AccountsController).toStrictEqual(
mockAccountsControllerState,
);
});

it('does nothing if the selectedAccount is found in the list of accounts', async () => {
const oldStorage = {
meta: { version: oldVersion },
data: {
AccountsController: mockAccountsControllerState,
},
};

const newStorage = await migrate(oldStorage);
expect(newStorage.data.AccountsController).toStrictEqual(
mockAccountsControllerState,
);
});
});
49 changes: 49 additions & 0 deletions app/scripts/migrations/126.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
import { AccountsControllerState } from '@metamask/accounts-controller';
import { cloneDeep } from 'lodash';

type VersionedData = {
meta: { version: number };
data: Record<string, unknown>;
};

export const version = 126;

/**
* This migration removes depreciated `Txcontroller` key if it is present in state.
*
* @param originalVersionedData - Versioned MetaMask extension state, exactly
* what we persist to dist.
* @param originalVersionedData.meta - State metadata.
* @param originalVersionedData.meta.version - The current state version.
* @param originalVersionedData.data - The persisted MetaMask state, keyed by
* controller.
* @returns Updated versioned MetaMask extension state.
*/
export async function migrate(
originalVersionedData: VersionedData,
): Promise<VersionedData> {
const versionedData = cloneDeep(originalVersionedData);
versionedData.meta.version = version;
transformState(versionedData.data);
return versionedData;
}

function transformState(state: Record<string, unknown>) {
const accountsControllerState = state?.AccountsController as
| AccountsControllerState
| undefined;

if (
accountsControllerState &&
Object.values(accountsControllerState?.internalAccounts.accounts).length >
0 &&
!accountsControllerState?.internalAccounts.accounts[
accountsControllerState?.internalAccounts.selectedAccount
]
) {
accountsControllerState.internalAccounts.selectedAccount = Object.values(
accountsControllerState?.internalAccounts.accounts,
)[0].id;
}
return state;
}
1 change: 1 addition & 0 deletions app/scripts/migrations/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -142,6 +142,7 @@ const migrations = [
require('./123'),
require('./124'),
require('./125'),
require('./126'),
];

export default migrations;
36 changes: 33 additions & 3 deletions lavamoat/browserify/beta/policy.json
Original file line number Diff line number Diff line change
Expand Up @@ -790,10 +790,10 @@
"@ethereumjs/tx>@ethereumjs/util": true,
"@ethereumjs/tx>ethereum-cryptography": true,
"@metamask/accounts-controller>@metamask/base-controller": true,
"@metamask/accounts-controller>@metamask/utils": true,
"@metamask/eth-snap-keyring": true,
"@metamask/keyring-api": true,
"@metamask/keyring-controller": true,
"@metamask/utils": true,
"uuid": true
}
},
Expand All @@ -805,6 +805,21 @@
"immer": true
}
},
"@metamask/accounts-controller>@metamask/utils": {
"globals": {
"TextDecoder": true,
"TextEncoder": true
},
"packages": {
"@metamask/utils>@metamask/superstruct": true,
"@metamask/utils>@scure/base": true,
"@metamask/utils>pony-cause": true,
"@noble/hashes": true,
"browserify>buffer": true,
"nock>debug": true,
"semver": true
}
},
"@metamask/address-book-controller": {
"packages": {
"@metamask/address-book-controller>@metamask/controller-utils": true,
Expand Down Expand Up @@ -1610,10 +1625,25 @@
"URL": true
},
"packages": {
"@metamask/keyring-api>@metamask/utils": true,
"@metamask/keyring-api>bech32": true,
"@metamask/keyring-api>uuid": true,
"@metamask/utils": true,
"superstruct": true
"@metamask/utils>@metamask/superstruct": true
}
},
"@metamask/keyring-api>@metamask/utils": {
"globals": {
"TextDecoder": true,
"TextEncoder": true
},
"packages": {
"@metamask/utils>@metamask/superstruct": true,
"@metamask/utils>@scure/base": true,
"@metamask/utils>pony-cause": true,
"@noble/hashes": true,
"browserify>buffer": true,
"nock>debug": true,
"semver": true
}
},
"@metamask/keyring-api>uuid": {
Expand Down
36 changes: 33 additions & 3 deletions lavamoat/browserify/flask/policy.json
Original file line number Diff line number Diff line change
Expand Up @@ -790,10 +790,10 @@
"@ethereumjs/tx>@ethereumjs/util": true,
"@ethereumjs/tx>ethereum-cryptography": true,
"@metamask/accounts-controller>@metamask/base-controller": true,
"@metamask/accounts-controller>@metamask/utils": true,
"@metamask/eth-snap-keyring": true,
"@metamask/keyring-api": true,
"@metamask/keyring-controller": true,
"@metamask/utils": true,
"uuid": true
}
},
Expand All @@ -805,6 +805,21 @@
"immer": true
}
},
"@metamask/accounts-controller>@metamask/utils": {
"globals": {
"TextDecoder": true,
"TextEncoder": true
},
"packages": {
"@metamask/utils>@metamask/superstruct": true,
"@metamask/utils>@scure/base": true,
"@metamask/utils>pony-cause": true,
"@noble/hashes": true,
"browserify>buffer": true,
"nock>debug": true,
"semver": true
}
},
"@metamask/address-book-controller": {
"packages": {
"@metamask/address-book-controller>@metamask/controller-utils": true,
Expand Down Expand Up @@ -1610,10 +1625,25 @@
"URL": true
},
"packages": {
"@metamask/keyring-api>@metamask/utils": true,
"@metamask/keyring-api>bech32": true,
"@metamask/keyring-api>uuid": true,
"@metamask/utils": true,
"superstruct": true
"@metamask/utils>@metamask/superstruct": true
}
},
"@metamask/keyring-api>@metamask/utils": {
"globals": {
"TextDecoder": true,
"TextEncoder": true
},
"packages": {
"@metamask/utils>@metamask/superstruct": true,
"@metamask/utils>@scure/base": true,
"@metamask/utils>pony-cause": true,
"@noble/hashes": true,
"browserify>buffer": true,
"nock>debug": true,
"semver": true
}
},
"@metamask/keyring-api>uuid": {
Expand Down
36 changes: 33 additions & 3 deletions lavamoat/browserify/main/policy.json
Original file line number Diff line number Diff line change
Expand Up @@ -790,10 +790,10 @@
"@ethereumjs/tx>@ethereumjs/util": true,
"@ethereumjs/tx>ethereum-cryptography": true,
"@metamask/accounts-controller>@metamask/base-controller": true,
"@metamask/accounts-controller>@metamask/utils": true,
"@metamask/eth-snap-keyring": true,
"@metamask/keyring-api": true,
"@metamask/keyring-controller": true,
"@metamask/utils": true,
"uuid": true
}
},
Expand All @@ -805,6 +805,21 @@
"immer": true
}
},
"@metamask/accounts-controller>@metamask/utils": {
"globals": {
"TextDecoder": true,
"TextEncoder": true
},
"packages": {
"@metamask/utils>@metamask/superstruct": true,
"@metamask/utils>@scure/base": true,
"@metamask/utils>pony-cause": true,
"@noble/hashes": true,
"browserify>buffer": true,
"nock>debug": true,
"semver": true
}
},
"@metamask/address-book-controller": {
"packages": {
"@metamask/address-book-controller>@metamask/controller-utils": true,
Expand Down Expand Up @@ -1610,10 +1625,25 @@
"URL": true
},
"packages": {
"@metamask/keyring-api>@metamask/utils": true,
"@metamask/keyring-api>bech32": true,
"@metamask/keyring-api>uuid": true,
"@metamask/utils": true,
"superstruct": true
"@metamask/utils>@metamask/superstruct": true
}
},
"@metamask/keyring-api>@metamask/utils": {
"globals": {
"TextDecoder": true,
"TextEncoder": true
},
"packages": {
"@metamask/utils>@metamask/superstruct": true,
"@metamask/utils>@scure/base": true,
"@metamask/utils>pony-cause": true,
"@noble/hashes": true,
"browserify>buffer": true,
"nock>debug": true,
"semver": true
}
},
"@metamask/keyring-api>uuid": {
Expand Down
Loading

0 comments on commit 759b92e

Please sign in to comment.