Skip to content

Commit 0a6e91c

Browse files
ccharlyccharly
authored
fix(accounts-controller): fire :accountAdded before :selectedAccountChange on KeyringController:stateChange (#6567)
## Explanation We were firing `:selectedAccountChange` before firing `:accountAdded` which does have undesired side-effect in the `AccountTreeController`. Mainly because the tree will try to get the associated selected account group, but this account group does not exist yet (since it's using `:accountAdded` to create the account groups). This is more like a temporary solution, and `#update` could be slightly refactored to a more robust solution later on. ## References N/A ## Checklist - [x] I've updated the test suite for new or updated code as appropriate - [x] I've updated documentation (JSDoc, Markdown, etc.) for new or updated code as appropriate - [ ] I've communicated my changes to consumers by [updating changelogs for packages I've changed](https://github.com/MetaMask/core/tree/main/docs/contributing.md#updating-changelogs), highlighting breaking changes as necessary - [ ] I've prepared draft pull requests for clients and consumer packages to resolve any breaking changes
1 parent 29a7354 commit 0a6e91c

File tree

3 files changed

+124
-49
lines changed

3 files changed

+124
-49
lines changed

‎packages/accounts-controller/CHANGELOG.md‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,11 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
1818
- Bump `@metamask/keyring-internal-api` from `^8.1.0` to `^9.0.0` ([#6560](https://github.com/MetaMask/core/pull/6560))
1919
- Bump `@metamask/eth-snap-keyring` from `^16.1.0` to `^17.0.0` ([#6560](https://github.com/MetaMask/core/pull/6560))
2020

21+
### Fixed
22+
23+
- Now publish `:accountAdded` before `:selectedAccountChange` on `KeyringController:stateChange` ([#6567](https://github.com/MetaMask/core/pull/6567))
24+
- This was preventing the `AccountTreeController` to properly create its account group before trying to select it.
25+
2126
## [33.0.0]
2227

2328
### Changed

‎packages/accounts-controller/src/AccountsController.test.ts‎

Lines changed: 56 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1634,6 +1634,62 @@ describe('AccountsController', () => {
16341634
expect(selectedAccount.id).toStrictEqual(expectedSelectedId);
16351635
},
16361636
);
1637+
1638+
it('fires :accountAdded before :selectedAccountChange', async () => {
1639+
const messenger = buildMessenger();
1640+
1641+
mockUUIDWithNormalAccounts([mockAccount, mockAccount2]);
1642+
1643+
const { accountsController } = setupAccountsController({
1644+
initialState: {
1645+
internalAccounts: {
1646+
accounts: {},
1647+
selectedAccount: '',
1648+
},
1649+
},
1650+
messenger,
1651+
});
1652+
1653+
const mockNewKeyringState = {
1654+
isUnlocked: true,
1655+
keyrings: [
1656+
{
1657+
type: KeyringTypes.hd,
1658+
accounts: [mockAccount.address],
1659+
metadata: {
1660+
id: 'mock-id',
1661+
name: 'mock-name',
1662+
},
1663+
},
1664+
],
1665+
};
1666+
1667+
const mockEventsOrder = jest.fn();
1668+
1669+
messenger.subscribe('AccountsController:accountAdded', () => {
1670+
mockEventsOrder('AccountsController:accountAdded');
1671+
});
1672+
messenger.subscribe('AccountsController:selectedAccountChange', () => {
1673+
mockEventsOrder('AccountsController:selectedAccountChange');
1674+
});
1675+
1676+
expect(accountsController.getSelectedAccount()).toBe(EMPTY_ACCOUNT);
1677+
1678+
messenger.publish(
1679+
'KeyringController:stateChange',
1680+
mockNewKeyringState,
1681+
[],
1682+
);
1683+
1684+
expect(mockEventsOrder).toHaveBeenNthCalledWith(
1685+
1,
1686+
'AccountsController:accountAdded',
1687+
);
1688+
expect(mockEventsOrder).toHaveBeenNthCalledWith(
1689+
2,
1690+
'AccountsController:selectedAccountChange',
1691+
);
1692+
});
16371693
});
16381694

16391695
describe('onSnapKeyringEvents', () => {

‎packages/accounts-controller/src/AccountsController.ts‎

Lines changed: 63 additions & 49 deletions
Original file line numberDiff line numberDiff line change
@@ -833,63 +833,71 @@ export class AccountsController extends BaseController<
833833
added: [] as InternalAccount[],
834834
};
835835

836-
this.#update((state) => {
837-
const { internalAccounts } = state;
836+
this.#update(
837+
(state) => {
838+
const { internalAccounts } = state;
838839

839-
for (const patch of [patches.snap, patches.normal]) {
840-
for (const account of patch.removed) {
841-
delete internalAccounts.accounts[account.id];
840+
for (const patch of [patches.snap, patches.normal]) {
841+
for (const account of patch.removed) {
842+
delete internalAccounts.accounts[account.id];
842843

843-
diff.removed.push(account.id);
844-
}
845-
846-
for (const added of patch.added) {
847-
const account = this.#getInternalAccountFromAddressAndType(
848-
added.address,
849-
added.keyring,
850-
);
844+
diff.removed.push(account.id);
845+
}
851846

852-
if (account) {
853-
// Re-compute the list of accounts everytime, so we can make sure new names
854-
// are also considered.
855-
const accounts = Object.values(
856-
internalAccounts.accounts,
857-
) as InternalAccount[];
858-
859-
// Get next account name available for this given keyring.
860-
const name = this.getNextAvailableAccountName(
861-
account.metadata.keyring.type,
862-
accounts,
847+
for (const added of patch.added) {
848+
const account = this.#getInternalAccountFromAddressAndType(
849+
added.address,
850+
added.keyring,
863851
);
864852

865-
// If it's the first account, we need to select it.
866-
const lastSelected =
867-
accounts.length === 0 ? this.#getLastSelectedIndex() : 0;
868-
869-
internalAccounts.accounts[account.id] = {
870-
...account,
871-
metadata: {
872-
...account.metadata,
873-
name,
874-
importTime: Date.now(),
875-
lastSelected,
876-
},
877-
};
878-
879-
diff.added.push(internalAccounts.accounts[account.id]);
853+
if (account) {
854+
// Re-compute the list of accounts everytime, so we can make sure new names
855+
// are also considered.
856+
const accounts = Object.values(
857+
internalAccounts.accounts,
858+
) as InternalAccount[];
859+
860+
// Get next account name available for this given keyring.
861+
const name = this.getNextAvailableAccountName(
862+
account.metadata.keyring.type,
863+
accounts,
864+
);
865+
866+
// If it's the first account, we need to select it.
867+
const lastSelected =
868+
accounts.length === 0 ? this.#getLastSelectedIndex() : 0;
869+
870+
internalAccounts.accounts[account.id] = {
871+
...account,
872+
metadata: {
873+
...account.metadata,
874+
name,
875+
importTime: Date.now(),
876+
lastSelected,
877+
},
878+
};
879+
880+
diff.added.push(internalAccounts.accounts[account.id]);
881+
}
880882
}
881883
}
882-
}
883-
});
884-
885-
// Now publish events
886-
for (const id of diff.removed) {
887-
this.messagingSystem.publish('AccountsController:accountRemoved', id);
888-
}
884+
},
885+
// Will get executed after the update, but before re-selecting an account in case
886+
// the current one is not valid anymore.
887+
() => {
888+
// Now publish events
889+
for (const id of diff.removed) {
890+
this.messagingSystem.publish('AccountsController:accountRemoved', id);
891+
}
889892

890-
for (const account of diff.added) {
891-
this.messagingSystem.publish('AccountsController:accountAdded', account);
892-
}
893+
for (const account of diff.added) {
894+
this.messagingSystem.publish(
895+
'AccountsController:accountAdded',
896+
account,
897+
);
898+
}
899+
},
900+
);
893901

894902
// NOTE: Since we also track "updated" accounts with our patches, we could fire a new event
895903
// like `accountUpdated` (we would still need to check if anything really changed on the account).
@@ -899,9 +907,12 @@ export class AccountsController extends BaseController<
899907
* Update the state and fixup the currently selected account.
900908
*
901909
* @param callback - Callback for updating state, passed a draft state object.
910+
* @param beforeAutoSelectAccount - Callback to be executed before auto-selecting an account
911+
* if the current one is no longer available.
902912
*/
903913
#update(
904914
callback: (state: WritableDraft<AccountsControllerStrictState>) => void,
915+
beforeAutoSelectAccount?: () => void,
905916
) {
906917
// The currently selected account might get deleted during the update, so keep track
907918
// of it before doing any change.
@@ -932,6 +943,9 @@ export class AccountsController extends BaseController<
932943
}
933944
});
934945

946+
// We might want to do some pre-work before selecting a new account.
947+
beforeAutoSelectAccount?.();
948+
935949
// Now, we compare the newly selected account, and we send event if different.
936950
const { selectedAccount } = this.state.internalAccounts;
937951
if (selectedAccount && selectedAccount !== previouslySelectedAccount) {

0 commit comments

Comments
 (0)