Skip to content

Conversation

@mathieuartu
Copy link
Contributor

@mathieuartu mathieuartu commented Nov 20, 2025

Related to: https://consensyssoftware.atlassian.net/browse/MUL-1217

Examples


Note

Introduce a unified KeyringV2 with a base wrapper and add V2 adapters for HD, Ledger, QR, and Simple keyrings, plus tests, exports, and build/config updates.

  • Keyring API (V2)
    • Add KeyringV2 interface and KeyringWrapper base with in‑memory KeyringAddressResolver (packages/keyring-api/src/api/v2/wrapper/*).
    • Export V2 modules (src/api/v2/index.ts), update README and changelog; add uuid deps.
  • New V2 Adapters
    • HdKeyringV2: wrap legacy HD keyring; supports BIP‑44 derive-index, account export, and EVM sign/decrypt utilities.
    • LedgerKeyringV2: wrap Ledger keyring; supports tx/message/EIP‑712(v4) signing.
    • QrKeyringV2: wrap QR keyring; supports tx/personal/EIP‑712(v4) signing.
    • SimpleKeyringV2: wrap simple keyring; supports random/create, hex private‑key import/export, EIP‑712(v1/v3/v4) signing.
    • Add comprehensive tests for all adapters.
  • Integration & Build
    • Wire adapters into package exports; add @metamask/keyring-api deps and TS project references across keyring packages.
    • Update Jest coverage thresholds (Ledger/Trezor) and tsconfig path mapping; README dependency graph adds edges to keyring_api.

Written by Cursor Bugbot for commit 956d921. This will update automatically on new commits. Configure here.

@mathieuartu mathieuartu self-assigned this Nov 20, 2025
@mathieuartu mathieuartu marked this pull request as ready for review November 20, 2025 16:55
@mathieuartu mathieuartu requested a review from a team as a code owner November 20, 2025 16:55
@mathieuartu mathieuartu marked this pull request as draft November 21, 2025 13:41
@mathieuartu mathieuartu changed the title feat: add keyring wrapper feat: add KeyringV2 wrapper and adapters Nov 21, 2025
@mathieuartu mathieuartu marked this pull request as ready for review November 24, 2025 12:00

// Get the updated addresses after adding new accounts
const updatedAddresses = await this.inner.getAccounts();
const targetAddress = updatedAddresses[targetIndex];
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: Creating accounts at higher groupIndex fails after account deletion

The createAccounts method conflates array indices with groupIndex values, which breaks after account deletion. When accounts are deleted, gaps appear in the groupIndex sequence but the underlying address array remains dense. The calculation accountsToAdd = targetIndex - currentCount + 1 uses the count of remaining accounts rather than the maximum existing groupIndex. Subsequently, updatedAddresses[targetIndex] accesses by array position rather than derivation index, returning undefined when targetIndex exceeds the array length. For example, after creating accounts at indices 0, 1, 2 and deleting index 1, attempting to create at groupIndex 3 will fail because the array has only 3 elements after addAccounts but the code tries to access index 3.

Additional Locations (1)

Fix in Cursor Fix in Web

HDKeyringOptions,
HDKeyringAccountSelectionOptions,
} from './hd-keyring';
export { HdKeyringV2 } from './hd-keyring-v2';
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: HdKeyringV2Options type not exported from package

The HdKeyringV2Options type is defined and exported in hd-keyring-v2.ts, but is not re-exported from the package's index.ts. Other keyring packages (Ledger, QR, Trezor) consistently export both the V2 class and the options type (e.g., export { LedgerKeyringV2, type LedgerKeyringV2Options }), but keyring-eth-hd only exports HdKeyringV2. This inconsistency prevents consumers from importing the type needed to construct the wrapper.

Fix in Cursor Fix in Web

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants