Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: add HD keyring check to persistAllKeyrings #4168

Merged
merged 4 commits into from
Apr 18, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 11 additions & 3 deletions packages/keyring-controller/src/KeyringController.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1111,12 +1111,20 @@ describe('KeyringController', () => {
* If there is only HD Key Tree keyring with 1 account and removeAccount is called passing that account
* It deletes keyring object also from state - not sure if this is correct behavior.
* https://github.com/MetaMask/core/issues/801
*
* Update: The behaviour is now modified to never remove the HD keyring as a preventive and temporal solution to the current race
* condition case we have been seeing lately. https://github.com/MetaMask/mobile-planning/issues/1507
* Enforcing this behaviour is not a 100% correct and it should be responsibility of the consumer to handle the accounts
* and keyrings in a way that it matches the expected behaviour.
*/
it('should remove HD Key Tree keyring from state when single account associated with it is deleted', async () => {
it('should not remove HD Key Tree keyring nor the single account from state', async () => {
await withController(async ({ controller, initialState }) => {
const account = initialState.keyrings[0].accounts[0] as Hex;
await controller.removeAccount(account);
expect(controller.state.keyrings).toHaveLength(0);
await expect(controller.removeAccount(account)).rejects.toThrow(
KeyringControllerError.NoHdKeyring,
);
expect(controller.state.keyrings).toHaveLength(1);
expect(controller.state.keyrings[0].accounts).toHaveLength(1);
});
});

Expand Down
6 changes: 6 additions & 0 deletions packages/keyring-controller/src/KeyringController.ts
Original file line number Diff line number Diff line change
Expand Up @@ -953,6 +953,12 @@ export class KeyringController extends BaseController<

serializedKeyrings.push(...this.#unsupportedKeyrings);

if (
!serializedKeyrings.some((keyring) => keyring.type === KeyringTypes.hd)
) {
throw new Error(KeyringControllerError.NoHdKeyring);
}

const updatedState: Partial<KeyringControllerState> = {};

if (this.#cacheEncryptionKey) {
Expand Down
1 change: 1 addition & 0 deletions packages/keyring-controller/src/constants.ts
Original file line number Diff line number Diff line change
Expand Up @@ -26,4 +26,5 @@ export enum KeyringControllerError {
ExpiredCredentials = 'KeyringController - Encryption key and salt provided are expired',
NoKeyringBuilder = 'KeyringController - No keyringBuilder found for keyring',
DataType = 'KeyringController - Incorrect data type provided',
NoHdKeyring = 'KeyringController - No HD Keyring found',
}
Loading