Skip to content

Commit

Permalink
fix: remove call to private/internal methods from the `@metamask/keyr…
Browse files Browse the repository at this point in the history
…ing-controller` (#8975)

## **Description**

Mark has reported that ledger has mis use keyringController method in
this slack thread
https://consensys.slack.com/archives/C026NDN79JB/p1710333124579809

This PR will investigate the any ledger existing code which misuse
keyring controller methods and fix all areas contain the error.

## **Related issues**

Fixes: #8931 

## **Manual testing steps**

The menual tests should including `forget ledger devices`, `relauch app`
and `change password` to see whether ledger account still there.

## **Screenshots/Recordings**

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

### **Before**

<!-- [screenshots/recordings] -->

### **After**

https://github.com/MetaMask/metamask-mobile/assets/7315988/a5edeabc-e943-4509-996c-b460ccc751b4

<!-- [screenshots/recordings] -->

## **Pre-merge author checklist**

- [x] I’ve followed [MetaMask Coding
Standards](https://github.com/MetaMask/metamask-mobile/blob/main/.github/guidelines/CODING_GUIDELINES.md).
- [x] I've clearly explained what problem this PR is solving and how it
is solved.
- [x] I've linked related issues
- [x] I've included manual testing steps
- [x] I've included screenshots/recordings if applicable
- [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-mobile/blob/main/.github/guidelines/LABELING_GUIDELINES.md)).
Not required for external contributors.
- [x] I’ve properly set the pull request status:
  - [x] In case it's not yet "ready for review", I've set it to "draft".
- [x] In case it's "ready for review", I've changed it from "draft" to
"non-draft".

## **Pre-merge reviewer checklist**

- [x] I've manually tested the PR (e.g. pull and build branch, run the
app, test code being changed).
- [x] 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
dawnseeker8 authored Mar 26, 2024
1 parent c3646ff commit f7eedbf
Show file tree
Hide file tree
Showing 4 changed files with 22 additions and 18 deletions.
14 changes: 9 additions & 5 deletions app/core/Ledger/Ledger.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -50,8 +50,10 @@ describe('Ledger core', () => {
addNewAccountForKeyring: mockAddNewAccountForKeyring,
};

const mockSetSelectedAddress = jest.fn();
Engine.setSelectedAddress = mockSetSelectedAddress;
const mockUpdateIdentities = jest.fn();
Engine.context.PreferencesController = {
updateIdentities: mockUpdateIdentities,
};

describe('addLedgerKeyring', () => {
it('should call addNewKeyring from keyring controller', () => {
Expand Down Expand Up @@ -139,10 +141,12 @@ describe('Ledger core', () => {
expect(ledgerKeyring.forgetDevice).toHaveBeenCalled();
expect(mockGetAccounts).toHaveBeenCalled();
expect(mockPersistAllKeyrings).toHaveBeenCalled();

expect(mockSetSelectedAddress).toBeCalledWith(
mockGetAccounts.mockReturnValue([
'0x49b6FFd1BD9d1c64EEf400a64a1e4bBC33E2CAB2',
);
]);
expect(mockUpdateIdentities).toBeCalledWith([
'0x49b6FFd1BD9d1c64EEf400a64a1e4bBC33E2CAB2',
]);
});
});

Expand Down
9 changes: 4 additions & 5 deletions app/core/Ledger/Ledger.ts
Original file line number Diff line number Diff line change
Expand Up @@ -94,14 +94,13 @@ export const closeRunningAppOnLedger = async (): Promise<void> => {
* Forgets the ledger keyring's previous device specific state.
*/
export const forgetLedger = async (): Promise<void> => {
const keyringController = Engine.context.KeyringController;
const { KeyringController, PreferencesController } = Engine.context;

const keyring = await getLedgerKeyring();
keyring.forgetDevice();

const accounts: string[] = await keyringController.getAccounts();
Engine.setSelectedAddress(accounts[0]);

await keyringController.persistAllKeyrings();
await KeyringController.persistAllKeyrings();
PreferencesController.updateIdentities(await KeyringController.getAccounts());
};

/**
Expand Down
7 changes: 4 additions & 3 deletions app/core/Vault.js
Original file line number Diff line number Diff line change
Expand Up @@ -27,16 +27,17 @@ export const restoreQRKeyring = async (qrKeyring) => {
* Restores the Ledger keyring if it exists.
*/
export const restoreLedgerKeyring = async (keyring) => {
const { KeyringController } = Engine.context;
const { KeyringController, PreferencesController } = Engine.context;

if (keyring) {
try {
const serializedLedgerKeyring = await keyring.serialize();
//This will regenerate a new ledger keyring after `createNewVaultAndRestore` is called
(await getLedgerKeyring()).deserialize(serializedLedgerKeyring);

await KeyringController.persistAllKeyrings();
KeyringController.updateIdentities(await KeyringController.getAccounts());
PreferencesController.updateIdentities(
await KeyringController.getAccounts(),
);
} catch (e) {
Logger.error(
e,
Expand Down
10 changes: 5 additions & 5 deletions app/core/Vault.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,9 +11,11 @@ jest.mock('./Engine', () => ({
restoreQRKeyring: jest.fn(),
addNewKeyring: jest.fn(),
persistAllKeyrings: jest.fn(),
updateIdentities: jest.fn(),
getAccounts: jest.fn().mockReturnValue(['account']),
},
PreferencesController: {
updateIdentities: jest.fn(),
},
},
}));

Expand Down Expand Up @@ -66,7 +68,7 @@ describe('Vault', () => {

describe('restoreLedgerKeyring', () => {
it('should restore ledger keyring if it exists', async () => {
const { KeyringController } = Engine.context;
const { KeyringController, PreferencesController } = Engine.context;

const mockSerialisedKeyring = jest.fn();
const mockDeserializedKeyring = jest.fn();
Expand All @@ -79,17 +81,15 @@ describe('Vault', () => {
expect(getLedgerKeyring).toHaveBeenCalled();
expect(mockDeserializedKeyring).toHaveBeenCalled();
expect(KeyringController.persistAllKeyrings).toHaveBeenCalled();
expect(KeyringController.updateIdentities).toHaveBeenCalled();
expect(KeyringController.getAccounts).toHaveBeenCalled();
expect(PreferencesController.updateIdentities).toHaveBeenCalled();
});

it('should not restore ledger keyring if it does not exist', async () => {
const { KeyringController } = Engine.context;

await restoreLedgerKeyring();
expect(KeyringController.persistAllKeyrings).not.toHaveBeenCalled();
expect(KeyringController.updateIdentities).not.toHaveBeenCalled();
expect(KeyringController.getAccounts).not.toHaveBeenCalled();
});

it('should log error if an exception is thrown', async () => {
Expand Down

0 comments on commit f7eedbf

Please sign in to comment.