Skip to content

Commit

Permalink
fix: Duplicate accounts (#9943)
Browse files Browse the repository at this point in the history 
<!--
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.
-->

This PR addresses the issue of duplicate accounts showing up in the
wallet. This bug was introduced when [the accounts controller was
integrated](#8759) but
was only visible to users in [this
PR](#9733) when we
started rendering account information in the accounts list. The is that
when we ran migration `036.ts`, we simply added the all of the
identities from the preferences controller into the AccountsController.
However the accounts controller also consumes addresses from the
KeyringController and those addresses are in lowercase. The result would
be two accounts with the same address, one in checksum format and one in
lowercase format. This would result in duplicates when rendering the UI.

This PR addresses this issue in two ways...
1. for users who are using <= v7.19.0
- I have patched migration 36 to ensure that the addresses that are
added are lowercase.
2. For users who have are using version >= 7.20.0 and are already
effected by this bug
- I have created a new migration to deduplicate the accounts in the
accounts controller and ensure that we are only storing lowercase
addresses in state.

Fixes: #9823

- [Current
branch](https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/c68d80ea-dbaa-4e81-9018-fe9757cab397)
-
[7.18.0](https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/011ac0b3-b1d8-4e53-9595-ea0f739d4d10)
-
[7.24.0](https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/a5c54c50-8382-45ad-b203-687b66273c4e)

1. Install
[7.18.0](https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/011ac0b3-b1d8-4e53-9595-ea0f739d4d10)
and import a wallet with multiple addresses already
2. Install
[7.24.0](https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/a5c54c50-8382-45ad-b203-687b66273c4e)
and upgrade app.
3. EXPECTED: You should see duplicate accounts when you open the
accounts list. If you had 5 accounts previously there will be should now
be 10.
4. Install [the current branch QA
build](https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/c68d80ea-dbaa-4e81-9018-fe9757cab397)
and upgrade.
5. EXPECT: The duplicate accounts should be removed (back to 5 accounts
from 10)
6. EXPECT: The previously selected address should still be selected
7. EXPECT: The account names should be in tact and in chronological
order if they are the default account name.

this occurs for users who have not upgraded from 7.19.0 yet
1. Install
[7.18.0](https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/011ac0b3-b1d8-4e53-9595-ea0f739d4d10)
and import a wallet with multiple addresses already
2. Install [the current branch QA
build](https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/c68d80ea-dbaa-4e81-9018-fe9757cab397)
and upgrade.
3. EXPECT: All of the account should appear in the accounts list without
duplicates
4. EXPECT: The previously selected address should be the same
5. EXPECT: The account names should be the same

<!-- If applicable, add screenshots and/or recordings to visualize the
before and after of your change. -->

7.18.0

<img
src="https://github.com/MetaMask/metamask-mobile/assets/22918444/3ee9b1f3-f042-414b-9ce4-82cad7d791cf"
width="200" height="400">

After Upgrading to 7.24.0

<img
src="https://github.com/MetaMask/metamask-mobile/assets/22918444/47b5ff3d-d2d0-4373-b8a0-ae0007b19b7b"
width="200" height="400">

Upgrading to this branch

<img
src="https://github.com/MetaMask/metamask-mobile/assets/22918444/9ed8d80a-ef3c-40a6-9152-e661a1ecdd25"
width="200" height="400">

- [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
- [ ] 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.

- [x] 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.
  • Loading branch information
@owencraston
owencraston committed Jun 12, 2024
1 parent b1de3ab commit 6097d75
Show file tree
Hide file tree
Showing 6 changed files with 743 additions and 70 deletions.
95 changes: 45 additions & 50 deletions app/store/migrations/036.test.ts
View file
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import { v4 as uuid } from 'uuid';
import { EthMethod, InternalAccount } from '@metamask/keyring-api';
import migrate, { sha256FromAddress, Identity } from './036';
import migrate, { Identity } from './036';
import { captureException } from '@sentry/react-native';
import { getUUIDFromAddressOfNormalAccount } from '@metamask/accounts-controller';

jest.mock('@sentry/react-native', () => ({
captureException: jest.fn(),
Expand All @@ -11,12 +11,6 @@ const mockedCaptureException = jest.mocked(captureException);
const MOCK_ADDRESS = '0x0';
const MOCK_ADDRESS_2 = '0x1';

async function addressToUUID(address: string): Promise<string> {
return uuid({
random: await sha256FromAddress(address),
});
}

interface Identities {
[key: string]: Identity;
}
Expand Down Expand Up @@ -47,14 +41,14 @@ function createMockPreferenceControllerState(
return state;
}

async function expectedInternalAccount(
function expectedInternalAccount(
address: string,
nickname: string,
lastSelected?: number,
): Promise<InternalAccount> {
): InternalAccount {
return {
address,
id: await addressToUUID(address),
id: getUUIDFromAddressOfNormalAccount(address),
metadata: {
name: nickname,
keyring: {
Expand Down Expand Up @@ -97,58 +91,58 @@ describe('Migration #036', () => {
beforeEach(() => {
mockedCaptureException.mockReset();
});
it('should throw if state.engine is not defined', async () => {
const newState = await migrate({});
it('should throw if state.engine is not defined', () => {
const newState = migrate({});
expect(newState).toStrictEqual({});
expect(mockedCaptureException).toHaveBeenCalledWith(expect.any(Error));
expect(mockedCaptureException.mock.calls[0][0].message).toBe(
`Migration 36: Invalid root engine state: 'undefined'`,
);
});
it('should throw if state.engine.backgroundState is not defined', async () => {
it('should throw if state.engine.backgroundState is not defined', () => {
const oldState = {
engine: {
backgroundState: undefined,
},
};
const newState = await migrate(oldState);
const newState = migrate(oldState);
expect(newState).toStrictEqual(oldState);
expect(mockedCaptureException).toHaveBeenCalledWith(expect.any(Error));
expect(mockedCaptureException.mock.calls[0][0].message).toBe(
`Migration 36: Invalid root engine backgroundState: 'undefined'`,
);
});
it('should throw if state.engine.backgroundState.PreferencesController is not defined', async () => {
it('should throw if state.engine.backgroundState.PreferencesController is not defined', () => {
const oldState = {
engine: {
backgroundState: {
PreferencesController: undefined,
},
},
};
const newState = await migrate(oldState);
const newState = migrate(oldState);
expect(newState).toStrictEqual(oldState);
expect(mockedCaptureException).toHaveBeenCalledWith(expect.any(Error));
expect(mockedCaptureException.mock.calls[0][0].message).toBe(
`Migration 36: Invalid PreferencesController state: 'undefined'`,
);
});
it('should throw if state.engine.backgroundState.PreferencesController.identities is not defined', async () => {
it('should throw if state.engine.backgroundState.PreferencesController.identities is not defined', () => {
const oldState = {
engine: {
backgroundState: {
PreferencesController: {},
},
},
};
const newState = await migrate(oldState);
const newState = migrate(oldState);
expect(newState).toStrictEqual(oldState);
expect(mockedCaptureException).toHaveBeenCalledWith(expect.any(Error));
expect(mockedCaptureException.mock.calls[0][0].message).toBe(
`Migration 36: Missing identities property from PreferencesController: 'object'`,
);
});
it('creates default state for accounts controller', async () => {
it('creates default state for accounts controller', () => {
const oldState = createMockState({
identities: {
[MOCK_ADDRESS]: {
Expand All @@ -159,10 +153,10 @@ describe('Migration #036', () => {
},
selectedAddress: MOCK_ADDRESS,
});
const newState = await migrate(oldState);
const newState = migrate(oldState);

const expectedUUID = await addressToUUID(MOCK_ADDRESS);
const resultInternalAccount = await expectedInternalAccount(
const expectedUUID = getUUIDFromAddressOfNormalAccount(MOCK_ADDRESS);
const resultInternalAccount = expectedInternalAccount(
MOCK_ADDRESS,
'Account 1',
);
Expand Down Expand Up @@ -194,8 +188,8 @@ describe('Migration #036', () => {
});

describe('createInternalAccountsForAccountsController', () => {
it('should create the identities into AccountsController as internal accounts', async () => {
const expectedUUID = await addressToUUID(MOCK_ADDRESS);
it('should create the identities into AccountsController as internal accounts', () => {
const expectedUUID = getUUIDFromAddressOfNormalAccount(MOCK_ADDRESS);
const oldState = createMockState({
identities: {
[MOCK_ADDRESS]: {
Expand All @@ -207,15 +201,15 @@ describe('Migration #036', () => {
selectedAddress: MOCK_ADDRESS,
});

const newState = await migrate(oldState);
const newState = migrate(oldState);

expect(newState).toStrictEqual({
engine: {
backgroundState: {
AccountsController: {
internalAccounts: {
accounts: {
[expectedUUID]: await expectedInternalAccount(
[expectedUUID]: expectedInternalAccount(
MOCK_ADDRESS,
`Account 1`,
),
Expand All @@ -229,23 +223,23 @@ describe('Migration #036', () => {
});
});

it('should keep the same name from the identities', async () => {
const expectedUUID = await addressToUUID(MOCK_ADDRESS);
it('should keep the same name from the identities', () => {
const expectedUUID = getUUIDFromAddressOfNormalAccount(MOCK_ADDRESS);
const oldState = createMockState(
createMockPreferenceControllerState(
[{ name: 'a random name', address: MOCK_ADDRESS }],
MOCK_ADDRESS,
),
);
const newState = await migrate(oldState);
const newState = migrate(oldState);
expect(newState).toStrictEqual({
engine: {
backgroundState: {
PreferencesController: expect.any(Object),
AccountsController: {
internalAccounts: {
accounts: {
[expectedUUID]: await expectedInternalAccount(
[expectedUUID]: expectedInternalAccount(
MOCK_ADDRESS,
`a random name`,
),
Expand All @@ -258,28 +252,28 @@ describe('Migration #036', () => {
});
});

it('should be able to handle multiple identities', async () => {
const expectedUUID = await addressToUUID(MOCK_ADDRESS);
const expectedUUID2 = await addressToUUID(MOCK_ADDRESS_2);
it('should be able to handle multiple identities', () => {
const expectedUUID = getUUIDFromAddressOfNormalAccount(MOCK_ADDRESS);
const expectedUUID2 = getUUIDFromAddressOfNormalAccount(MOCK_ADDRESS_2);
const oldState = createMockState({
identities: {
[MOCK_ADDRESS]: { name: 'Account 1', address: MOCK_ADDRESS },
[MOCK_ADDRESS_2]: { name: 'Account 2', address: MOCK_ADDRESS_2 },
},
selectedAddress: MOCK_ADDRESS_2,
});
const newState = await migrate(oldState);
const newState = migrate(oldState);
expect(newState).toStrictEqual({
engine: {
backgroundState: {
AccountsController: {
internalAccounts: {
accounts: {
[expectedUUID]: await expectedInternalAccount(
[expectedUUID]: expectedInternalAccount(
MOCK_ADDRESS,
`Account 1`,
),
[expectedUUID2]: await expectedInternalAccount(
[expectedUUID2]: expectedInternalAccount(
MOCK_ADDRESS_2,
`Account 2`,
),
Expand All @@ -293,12 +287,12 @@ describe('Migration #036', () => {
});
});

it('should handle empty identities and create default AccountsController with no internal accounts', async () => {
it('should handle empty identities and create default AccountsController with no internal accounts', () => {
const oldState = createMockState({
// Simulate `identities` being an empty object
identities: {},
});
const newState = await migrate(oldState);
const newState = migrate(oldState);

expect(mockedCaptureException).toHaveBeenCalledWith(expect.any(Error));

Expand All @@ -319,30 +313,31 @@ describe('Migration #036', () => {
});

describe('createSelectedAccountForAccountsController', () => {
it('should select the same account as the selected address', async () => {
it('should select the same account as the selected address', () => {
const oldState = createMockState(
createMockPreferenceControllerState(
[{ name: 'a random name', address: MOCK_ADDRESS }],
MOCK_ADDRESS,
),
);
const newState = await migrate(oldState);
const newState = migrate(oldState);
expect(newState).toStrictEqual({
engine: {
backgroundState: {
PreferencesController: expect.any(Object),
AccountsController: {
internalAccounts: {
accounts: expect.any(Object),
selectedAccount: await addressToUUID(MOCK_ADDRESS),
selectedAccount:
getUUIDFromAddressOfNormalAccount(MOCK_ADDRESS),
},
},
},
},
});
});

it("should leave selectedAccount as empty if there aren't any selectedAddress", async () => {
it("should leave selectedAccount as empty if there aren't any selectedAddress", () => {
const oldState = {
engine: {
backgroundState: {
Expand All @@ -353,7 +348,7 @@ describe('Migration #036', () => {
},
},
};
const newState = await migrate(oldState);
const newState = migrate(oldState);
expect(newState).toStrictEqual({
engine: {
backgroundState: {
Expand All @@ -368,7 +363,7 @@ describe('Migration #036', () => {
},
});
});
it('should select the first account as the selected account if selectedAddress is undefined, and update PreferencesController accordingly', async () => {
it('should select the first account as the selected account if selectedAddress is undefined, and update PreferencesController accordingly', () => {
const identities = [
{ name: 'Account 1', address: MOCK_ADDRESS },
{ name: 'Account 2', address: MOCK_ADDRESS_2 },
Expand All @@ -377,8 +372,8 @@ describe('Migration #036', () => {
const oldState = createMockState(
createMockPreferenceControllerState(identities, undefined),
);
const expectedUUID = await addressToUUID(MOCK_ADDRESS);
const expectedUUID2 = await addressToUUID(MOCK_ADDRESS_2);
const expectedUUID = getUUIDFromAddressOfNormalAccount(MOCK_ADDRESS);
const expectedUUID2 = getUUIDFromAddressOfNormalAccount(MOCK_ADDRESS_2);

expect(oldState).toStrictEqual({
engine: {
Expand All @@ -402,7 +397,7 @@ describe('Migration #036', () => {
},
});

const newState = await migrate(oldState);
const newState = migrate(oldState);

expect(mockedCaptureException).toHaveBeenCalledWith(expect.any(Error));

Expand All @@ -428,11 +423,11 @@ describe('Migration #036', () => {
AccountsController: {
internalAccounts: {
accounts: {
[expectedUUID]: await expectedInternalAccount(
[expectedUUID]: expectedInternalAccount(
MOCK_ADDRESS,
`Account 1`,
),
[expectedUUID2]: await expectedInternalAccount(
[expectedUUID2]: expectedInternalAccount(
MOCK_ADDRESS_2,
`Account 2`,
),
Expand Down
27 changes: 8 additions & 19 deletions app/store/migrations/036.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,18 +5,16 @@ import {
} from '@metamask/keyring-api';
import { isObject, hasProperty } from '@metamask/utils';
import { captureException } from '@sentry/react-native';
import { v4 as uuid } from 'uuid';
import { NativeModules } from 'react-native';
const Aes = NativeModules.Aes;
import { getUUIDFromAddressOfNormalAccount } from '@metamask/accounts-controller';
import { KeyringTypes } from '@metamask/keyring-controller';

export interface Identity {
name: string;
address: string;
lastSelected?: number;
}

export default async function migrate(stateAsync: unknown) {
const state = await stateAsync;
export default function migrate(state: unknown) {
if (!isObject(state)) {
captureException(
new Error(`Migration 36: Invalid root state: '${typeof state}'`),
Expand Down Expand Up @@ -65,20 +63,12 @@ export default async function migrate(stateAsync: unknown) {
);
return state;
}

createDefaultAccountsController(state);
await createInternalAccountsForAccountsController(state);
createInternalAccountsForAccountsController(state);
createSelectedAccountForAccountsController(state);
return state;
}

export const sha256FromAddress = async (
address: string,
): Promise<ArrayLike<number>> => {
const sha256: string = await Aes.sha256(address);
return Buffer.from(sha256).slice(0, 16);
};

function createDefaultAccountsController(state: Record<string, any>) {
state.engine.backgroundState.AccountsController = {
internalAccounts: {
Expand All @@ -88,7 +78,7 @@ function createDefaultAccountsController(state: Record<string, any>) {
};
}

async function createInternalAccountsForAccountsController(
function createInternalAccountsForAccountsController(
state: Record<string, any>,
) {
const identities: {
Expand All @@ -105,9 +95,8 @@ async function createInternalAccountsForAccountsController(
const accounts: Record<string, InternalAccount> = {};

for (const identity of Object.values(identities)) {
const expectedId = uuid({
random: await sha256FromAddress(identity.address),
});
const lowerCaseAddress = identity.address.toLocaleLowerCase();
const expectedId = getUUIDFromAddressOfNormalAccount(lowerCaseAddress);

accounts[expectedId] = {
address: identity.address,
Expand All @@ -120,7 +109,7 @@ async function createInternalAccountsForAccountsController(
// This is default HD Key Tree type because the keyring is encrypted
// during migration, the type will get updated when the during the
// initial updateAccounts call.
type: 'HD Key Tree',
type: KeyringTypes.hd,
},
},
methods: [
Expand Down
Loading

0 comments on commit 6097d75

Please sign in to comment.