Skip to content

Commit 13773c7

Browse files
authored
fix: expose missing methods in hardware keyrings wrappers (#551)
There's some missing methods that are needed by the clients. <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Medium Risk** > Moderate risk because it adds new public surface area and changes `forgetDevice` behavior to also clear the V2 account registry, which could affect client state/caching around device resets. > > **Overview** > Exposes missing *device-management* APIs on the V2 wrapper keyrings so clients can manage paired hardware devices directly via the wrapper. > > `LedgerKeyring`, `QrKeyring`, and `TrezorKeyring` (and thus `OneKeyKeyring`) now pass through getters/methods like `hdPath`, `bridge`, paging (`getFirstPage`/`getNextPage`/`getPreviousPage`), and device reset (`forgetDevice`), plus device-specific calls (e.g. Ledger `get/setDeviceId`, `attemptMakeApp`, `getAppNameAndVersion`, Trezor `getModel`, `isUnlocked`). `forgetDevice` also clears the V2 registry to keep wrapper accounts in sync with the inner keyring, and new unit tests/changelog entries cover these additions. > > <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit 56e782c. Bugbot is set up for automated code reviews on this repo. Configure [here](https://www.cursor.com/dashboard/bugbot).</sup> <!-- /CURSOR_SUMMARY -->
1 parent 929a3b2 commit 13773c7

9 files changed

Lines changed: 491 additions & 2 deletions

File tree

packages/keyring-eth-ledger-bridge/CHANGELOG.md

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
77

88
## [Unreleased]
99

10+
### Added
11+
12+
- Expose device-management pass-throughs on the V2 `LedgerKeyring` wrapper: `hdPath` (getter), `bridge` (getter), `getDeviceId`, `setDeviceId`, `setHdPath`, `getFirstPage`, `getNextPage`, `getPreviousPage`, `forgetDevice`, `isUnlocked`, `attemptMakeApp`, `getAppNameAndVersion`. `forgetDevice` additionally clears the V2 account registry to keep it in sync with the inner keyring. ([#551](https://github.com/MetaMask/accounts/pull/551))
13+
1014
## [12.0.3]
1115

1216
### Changed

packages/keyring-eth-ledger-bridge/src/v2/ledger-keyring.test.ts

Lines changed: 99 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -852,4 +852,103 @@ describe('LedgerKeyring', () => {
852852
expect(accounts[1]?.options.entropy.groupIndex).toBe(2);
853853
});
854854
});
855+
856+
describe('device management pass-throughs', () => {
857+
it('exposes the inner keyring hdPath', () => {
858+
const { wrapper, inner } = createEmptyWrapper();
859+
860+
expect(wrapper.hdPath).toBe(inner.hdPath);
861+
});
862+
863+
it('exposes the inner keyring bridge', () => {
864+
const { wrapper, inner } = createEmptyWrapper();
865+
866+
expect(wrapper.bridge).toBe(inner.bridge);
867+
});
868+
869+
it('getDeviceId delegates to the inner keyring', () => {
870+
const { wrapper, inner } = createEmptyWrapper();
871+
inner.setDeviceId('device-abc');
872+
873+
expect(wrapper.getDeviceId()).toBe('device-abc');
874+
});
875+
876+
it('setDeviceId delegates to the inner keyring', () => {
877+
const { wrapper, inner } = createEmptyWrapper();
878+
879+
wrapper.setDeviceId('device-xyz');
880+
881+
expect(inner.getDeviceId()).toBe('device-xyz');
882+
});
883+
884+
it('setHdPath delegates to the inner keyring', () => {
885+
const { wrapper, inner } = createEmptyWrapper();
886+
const setHdPathSpy = jest.spyOn(inner, 'setHdPath');
887+
888+
wrapper.setHdPath(`m/44'/60'/0'/0`);
889+
890+
expect(setHdPathSpy).toHaveBeenCalledWith(`m/44'/60'/0'/0`);
891+
expect(inner.hdPath).toBe(`m/44'/60'/0'/0`);
892+
});
893+
894+
it('getFirstPage delegates to the inner keyring', async () => {
895+
const { wrapper, inner } = createEmptyWrapper();
896+
const page = [{ address: EXPECTED_ACCOUNTS[0], balance: 0, index: 0 }];
897+
jest.spyOn(inner, 'getFirstPage').mockResolvedValue(page);
898+
899+
expect(await wrapper.getFirstPage()).toStrictEqual(page);
900+
});
901+
902+
it('getNextPage delegates to the inner keyring', async () => {
903+
const { wrapper, inner } = createEmptyWrapper();
904+
const page = [{ address: EXPECTED_ACCOUNTS[1], balance: 0, index: 1 }];
905+
jest.spyOn(inner, 'getNextPage').mockResolvedValue(page);
906+
907+
expect(await wrapper.getNextPage()).toStrictEqual(page);
908+
});
909+
910+
it('getPreviousPage delegates to the inner keyring', async () => {
911+
const { wrapper, inner } = createEmptyWrapper();
912+
const page = [{ address: EXPECTED_ACCOUNTS[0], balance: 0, index: 0 }];
913+
jest.spyOn(inner, 'getPreviousPage').mockResolvedValue(page);
914+
915+
expect(await wrapper.getPreviousPage()).toStrictEqual(page);
916+
});
917+
918+
it('forgetDevice clears inner state and the V2 registry', async () => {
919+
const { wrapper, inner } = await createWrapperWithAccounts(2);
920+
921+
// Populate the wrapper's registry by reading accounts.
922+
const accounts = await wrapper.getAccounts();
923+
expect(accounts).toHaveLength(2);
924+
925+
await wrapper.forgetDevice();
926+
927+
expect(inner.accounts).toStrictEqual([]);
928+
expect(inner.accountDetails).toStrictEqual({});
929+
expect(await wrapper.getAccounts()).toStrictEqual([]);
930+
});
931+
932+
it('isUnlocked delegates to the inner keyring', () => {
933+
const { wrapper, inner } = createEmptyWrapper();
934+
jest.spyOn(inner, 'isUnlocked').mockReturnValue(true);
935+
936+
expect(wrapper.isUnlocked()).toBe(true);
937+
});
938+
939+
it('attemptMakeApp delegates to the inner keyring', async () => {
940+
const { wrapper, inner } = createEmptyWrapper();
941+
jest.spyOn(inner, 'attemptMakeApp').mockResolvedValue(true);
942+
943+
expect(await wrapper.attemptMakeApp()).toBe(true);
944+
});
945+
946+
it('getAppNameAndVersion delegates to the inner keyring', async () => {
947+
const { wrapper, inner } = createEmptyWrapper();
948+
const response = { appName: 'Ethereum', version: '1.10.0' };
949+
jest.spyOn(inner, 'getAppNameAndVersion').mockResolvedValue(response);
950+
951+
expect(await wrapper.getAppNameAndVersion()).toStrictEqual(response);
952+
});
953+
});
855954
});

packages/keyring-eth-ledger-bridge/src/v2/ledger-keyring.ts

Lines changed: 111 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,15 @@ import type { AccountId, EthKeyring } from '@metamask/keyring-utils';
1717
import { add0x, getChecksumAddress } from '@metamask/utils';
1818
import type { Hex } from '@metamask/utils';
1919

20-
import type { LedgerKeyring as LegacyLedgerKeyring } from '../ledger-keyring';
20+
import type {
21+
GetAppNameAndVersionResponse,
22+
LedgerBridge,
23+
LedgerBridgeOptions,
24+
} from '../ledger-bridge';
25+
import type {
26+
AccountPage,
27+
LedgerKeyring as LegacyLedgerKeyring,
28+
} from '../ledger-keyring';
2129

2230
/**
2331
* Methods supported by Ledger keyring EOA accounts.
@@ -350,4 +358,106 @@ export class LedgerKeyring
350358
this.registry.delete(accountId);
351359
});
352360
}
361+
362+
/**
363+
* @returns The current derivation path used by the inner keyring.
364+
*/
365+
get hdPath(): string {
366+
return this.inner.hdPath;
367+
}
368+
369+
/**
370+
* @returns The bridge instance used by the inner keyring to communicate
371+
* with the device.
372+
*/
373+
get bridge(): LedgerBridge<LedgerBridgeOptions> {
374+
return this.inner.bridge;
375+
}
376+
377+
/**
378+
* @returns The device ID for the paired Ledger device.
379+
*/
380+
getDeviceId(): string {
381+
return this.inner.getDeviceId();
382+
}
383+
384+
/**
385+
* Set the device ID for the paired Ledger device.
386+
*
387+
* @param deviceId - The device ID to set.
388+
*/
389+
setDeviceId(deviceId: string): void {
390+
this.inner.setDeviceId(deviceId);
391+
}
392+
393+
/**
394+
* Set the derivation path on the inner keyring.
395+
*
396+
* @param hdPath - The derivation path to set.
397+
*/
398+
setHdPath(hdPath: string): void {
399+
this.inner.setHdPath(hdPath);
400+
}
401+
402+
/**
403+
* Fetch the first page of candidate addresses from the device.
404+
*
405+
* @returns The first page of accounts.
406+
*/
407+
async getFirstPage(): Promise<AccountPage> {
408+
return this.inner.getFirstPage();
409+
}
410+
411+
/**
412+
* Fetch the next page of candidate addresses from the device.
413+
*
414+
* @returns The next page of accounts.
415+
*/
416+
async getNextPage(): Promise<AccountPage> {
417+
return this.inner.getNextPage();
418+
}
419+
420+
/**
421+
* Fetch the previous page of candidate addresses from the device.
422+
*
423+
* @returns The previous page of accounts.
424+
*/
425+
async getPreviousPage(): Promise<AccountPage> {
426+
return this.inner.getPreviousPage();
427+
}
428+
429+
/**
430+
* Clear the inner keyring's device-pairing state and accounts, and reset
431+
* the V2 account registry to keep them in sync.
432+
*/
433+
async forgetDevice(): Promise<void> {
434+
await this.withLock(async () => {
435+
this.inner.forgetDevice();
436+
this.registry.clear();
437+
});
438+
}
439+
440+
/**
441+
* @returns Whether the inner keyring has an unlocked HD key.
442+
*/
443+
isUnlocked(): boolean {
444+
return this.inner.isUnlocked();
445+
}
446+
447+
/**
448+
* Attempt to open the Ethereum app on the connected Ledger device.
449+
*
450+
* @returns Whether the app was opened.
451+
*/
452+
async attemptMakeApp(): Promise<boolean> {
453+
return this.inner.attemptMakeApp();
454+
}
455+
456+
/**
457+
* @returns The app name and version currently running on the connected
458+
* Ledger device.
459+
*/
460+
async getAppNameAndVersion(): Promise<GetAppNameAndVersionResponse> {
461+
return this.inner.getAppNameAndVersion();
462+
}
353463
}

packages/keyring-eth-qr/CHANGELOG.md

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
77

88
## [Unreleased]
99

10+
### Added
11+
12+
- Expose device-management pass-throughs on the V2 `QrKeyring` wrapper: `getName`, `getMode`, `getFirstPage`, `getNextPage`, `getPreviousPage`, `forgetDevice`. `forgetDevice` additionally clears the V2 account registry to keep it in sync with the inner keyring. ([#551](https://github.com/MetaMask/accounts/pull/551))
13+
1014
### Changed
1115

1216
- Bump `@metamask/keyring-sdk` from `^2.0.2` to `^2.1.1` ([#544](https://github.com/MetaMask/accounts/pull/544), [#546](https://github.com/MetaMask/accounts/pull/546))

packages/keyring-eth-qr/src/v2/qr-keyring.test.ts

Lines changed: 57 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -766,4 +766,61 @@ describe('QrKeyring', () => {
766766
});
767767
});
768768
});
769+
770+
describe('device management pass-throughs', () => {
771+
it('getName delegates to the inner keyring', async () => {
772+
const { wrapper, inner } = await createWrapperWithAccounts(1);
773+
774+
expect(wrapper.getName()).toBe(inner.getName());
775+
});
776+
777+
it('getMode delegates to the inner keyring', async () => {
778+
const { wrapper, inner } = await createWrapperWithAccounts(1);
779+
780+
expect(wrapper.getMode()).toBe(inner.getMode());
781+
});
782+
783+
it('getFirstPage delegates to the inner keyring', async () => {
784+
const { wrapper, inner } = createEmptyWrapper();
785+
const page = [{ address: EXPECTED_ACCOUNTS[0], index: 0 }] as Awaited<
786+
ReturnType<LegacyQrKeyring['getFirstPage']>
787+
>;
788+
jest.spyOn(inner, 'getFirstPage').mockResolvedValue(page);
789+
790+
expect(await wrapper.getFirstPage()).toStrictEqual(page);
791+
});
792+
793+
it('getNextPage delegates to the inner keyring', async () => {
794+
const { wrapper, inner } = createEmptyWrapper();
795+
const page = [{ address: EXPECTED_ACCOUNTS[1], index: 1 }] as Awaited<
796+
ReturnType<LegacyQrKeyring['getNextPage']>
797+
>;
798+
jest.spyOn(inner, 'getNextPage').mockResolvedValue(page);
799+
800+
expect(await wrapper.getNextPage()).toStrictEqual(page);
801+
});
802+
803+
it('getPreviousPage delegates to the inner keyring', async () => {
804+
const { wrapper, inner } = createEmptyWrapper();
805+
const page = [{ address: EXPECTED_ACCOUNTS[0], index: 0 }] as Awaited<
806+
ReturnType<LegacyQrKeyring['getPreviousPage']>
807+
>;
808+
jest.spyOn(inner, 'getPreviousPage').mockResolvedValue(page);
809+
810+
expect(await wrapper.getPreviousPage()).toStrictEqual(page);
811+
});
812+
813+
it('forgetDevice clears inner state and the V2 registry', async () => {
814+
const { wrapper, inner } = await createWrapperWithAccounts(2);
815+
816+
// Populate the wrapper's registry by reading accounts.
817+
const accounts = await wrapper.getAccounts();
818+
expect(accounts).toHaveLength(2);
819+
820+
await wrapper.forgetDevice();
821+
822+
expect(await inner.getAccounts()).toStrictEqual([]);
823+
expect(await wrapper.getAccounts()).toStrictEqual([]);
824+
});
825+
});
769826
});

packages/keyring-eth-qr/src/v2/qr-keyring.ts

Lines changed: 55 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@ import type { AccountId } from '@metamask/keyring-utils';
1818
import type { Hex } from '@metamask/utils';
1919

2020
import { DeviceMode } from '../device';
21+
import type { IndexedAddress } from '../device';
2122
import type { QrKeyring as LegacyQrKeyring } from '../qr-keyring';
2223

2324
/**
@@ -435,4 +436,58 @@ export class QrKeyring
435436
this.registry.delete(accountId);
436437
});
437438
}
439+
440+
/**
441+
* @returns The name of the paired device, or the keyring type if no device
442+
* is paired.
443+
*/
444+
getName(): string {
445+
return this.inner.getName();
446+
}
447+
448+
/**
449+
* @returns The current device mode (HD or Account), or `undefined` if no
450+
* device is paired.
451+
*/
452+
getMode(): DeviceMode | undefined {
453+
return this.inner.getMode();
454+
}
455+
456+
/**
457+
* Fetch the first page of candidate addresses from the device.
458+
*
459+
* @returns The first page of addresses.
460+
*/
461+
async getFirstPage(): Promise<IndexedAddress[]> {
462+
return this.inner.getFirstPage();
463+
}
464+
465+
/**
466+
* Fetch the next page of candidate addresses from the device.
467+
*
468+
* @returns The next page of addresses.
469+
*/
470+
async getNextPage(): Promise<IndexedAddress[]> {
471+
return this.inner.getNextPage();
472+
}
473+
474+
/**
475+
* Fetch the previous page of candidate addresses from the device.
476+
*
477+
* @returns The previous page of addresses.
478+
*/
479+
async getPreviousPage(): Promise<IndexedAddress[]> {
480+
return this.inner.getPreviousPage();
481+
}
482+
483+
/**
484+
* Clear the inner keyring's device-pairing state and accounts, and reset
485+
* the V2 account registry to keep them in sync.
486+
*/
487+
async forgetDevice(): Promise<void> {
488+
await this.withLock(async () => {
489+
await this.inner.forgetDevice();
490+
this.registry.clear();
491+
});
492+
}
438493
}

packages/keyring-eth-trezor/CHANGELOG.md

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
77

88
## [Unreleased]
99

10+
### Added
11+
12+
- Expose device-management pass-throughs on the V2 `TrezorKeyring` wrapper (inherited by the V2 `OneKeyKeyring`): `getModel`, `hdPath` (getter), `bridge` (getter), `setHdPath`, `getFirstPage`, `getNextPage`, `getPreviousPage`, `forgetDevice`, `isUnlocked`. `forgetDevice` additionally clears the V2 account registry to keep it in sync with the inner keyring. ([#551](https://github.com/MetaMask/accounts/pull/551))
13+
1014
### Changed
1115

1216
- Bump `@metamask/keyring-sdk` from `^2.0.2` to `^2.1.1` ([#544](https://github.com/MetaMask/accounts/pull/544), [#546](https://github.com/MetaMask/accounts/pull/546))

0 commit comments

Comments
 (0)