feat: add QrKeyringV2 wrapper
#411
Conversation
mathieuartu
changed the title
QrKeyringV2 wrapperQrKeyringV2 wrapper
mathieuartu
changed the title
QrKeyringV2 wrapperQrKeyringV2 wrapper
mathieuartu
changed the title
QrKeyringV2 wrapperQrKeyringV2 wrapper
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| entropy: { | ||
| type: KeyringAccountEntropyTypeOption.Mnemonic, | ||
| id: this.entropySource, | ||
| groupIndex: addressIndex, |
There was a problem hiding this comment.
I think the addressIndex only make sense for DeviceMode.HD? Since for DeviceMode.ACCOUNT we're defaulting to the arrayIndex, this won't really match the real account/address index used during the derivation?
This might also be pretty hard to go from derivation path -> index if the "QR device" can use any arbitrary path. So I wonder if DeviceMode.ACCOUNT should be treated as private key here? 🤔
NOTE: I might have misunderstood the legacy QR keyring, so correct me if I'm wrong 😅
There was a problem hiding this comment.
I'm a bit confused here as well. I'll set up something internally so we can share knowledge around this area 😅
| const currentAccounts = await this.getAccounts(); | ||
| const existingAccount = currentAccounts.find( | ||
| (account) => account.options.entropy.groupIndex === targetIndex, | ||
| ); |
There was a problem hiding this comment.
Following-up my previous questions, we would have to consider only BIP-44 accounts in this case.
| this.inner.setAccountToUnlock(targetIndex); | ||
| const [newAddress] = await this.inner.addAccounts(1); |
There was a problem hiding this comment.
Question: (Might be a bit too much) but, I wonder if we should reset the #accountToUnlock back to what it was here? Like if for some reason the previous value had to be kept, then, our implementation side effect could have an impact on that? 🤔
Basically I'm thinking about something like:
const previousAccountToUnlock = this.inner.getAccountToUnlock();
let newAddress;
try {
this.inner.setAccountToUnlock(targetIndex);
[newAddress] = await this.inner.addAccounts(1);
} finally {
this.inner.setAccountToUnlock(previousAccountToUnlock);
}But even this, is not entirely concurrent-safe (if another addAccounts happens in parallel 🙃).
There was a problem hiding this comment.
We can discuss this internally, but I think the legacy implementation is a bit fragile regarding this logic anyway, like, if you use setAccountToUnlock(n) and addAccounts(2), this will try to create the same account twice (since #accountToUnlock is not cleared after creating an account 😅)
The legacy logic should probably be fixed to only accept addAccounts(1) if #accountToUnlock is set!
But our wrapper implementation is good otherwise!
| } | ||
|
|
||
| // Derive the account at the specified index | ||
| this.inner.setAccountToUnlock(targetIndex); |
There was a problem hiding this comment.
Missing validation for groupIndex allows invalid derivation paths
Medium Severity
The createAccounts method accepts options.groupIndex without validating it's a non-negative integer. Negative numbers, NaN, Infinity, or non-integers (like 1.5) are passed directly to setAccountToUnlock and ultimately used to construct a BIP-32 derivation path (e.g., m/0/-1 or m/0/NaN). The underlying hdkey library will fail with a confusing error or potentially produce incorrect behavior when attempting to derive from these invalid paths.
Related to: https://consensyssoftware.atlassian.net/browse/MUL-1320
Examples
Note
Adds a V2 wrapper around the legacy QR keyring to expose accounts via the unified API and support controlled account creation.
QrKeyringV2(extendsEthKeyringWrapper) implementingKeyringV2with capabilities for EOA and BIP-44 derive-indexgetAccounts,createAccounts(HD-only, validates entropy source),deleteAccount,serialize/deserialize; treats Account-mode as private-key accountsQrKeyringgainsgetPathFromAddressused for HD derivation metadataQrKeyringV2from package; add deps on@metamask/keyring-apiand@metamask/account-api; update tsconfig references and README graphWritten by Cursor Bugbot for commit a5fa59d. This will update automatically on new commits. Configure here.