refactor(multichain-account-service)!: replace withKeyring usage with withKeyringV2#8491
refactor(multichain-account-service)!: replace withKeyring usage with withKeyringV2#8491
withKeyring usage with withKeyringV2#8491Conversation
…t service types and base provider
hmalik88
changed the title
withKeyring usage with withKeyringV2withKeyring usage with withKeyringV2
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit e442d90. Configure here.
| // Activity found — account stays. Return the address so we can | ||
| // look it up in the AccountsController after the callback persists. | ||
| return address; | ||
| }, |
There was a problem hiding this comment.
Discovery holds keyring mutex during network I/O
Medium Severity
The refactored discoverAccounts now performs the entire create → RPC check → delete flow inside a single withKeyringV2 callback. Since withKeyringV2 holds the KeyringController's exclusive mutex (#persistOrRollback → #withControllerLock), the RPC call to #getTransactionCount — which retries up to maxAttempts times with timeoutMs and backOffMs delays — now blocks all other keyring operations (signing, account creation, switching, etc.) for potentially several seconds. The prior implementation performed the RPC call outside withKeyring, acquiring the mutex only briefly for the peek and create steps.
Reviewed by Cursor Bugbot for commit e442d90. Configure here.
There was a problem hiding this comment.
The prior implementation peeked at addresses via keyring.root (V1 internal state) outside the lock. V2 keyrings don't expose HD internals, so we create the account inside the callback to discover the address, and delete it if inactive. Both must happen in the same callback so that #persistOrRollback sees zero net state change, as a result this holds the lock during the rpc call
|
|
||
| ### Changed | ||
|
|
||
| - **BREAKING:** Replace `KeyringController:withKeyring` with `KeyringController:withKeyringV2` across all account providers ([#8491](https://github.com/MetaMask/core/pull/8491)) |
There was a problem hiding this comment.
| - **BREAKING:** Replace `KeyringController:withKeyring` with `KeyringController:withKeyringV2` across all account providers ([#8491](https://github.com/MetaMask/core/pull/8491)) | |
| - **BREAKING:** Replace `KeyringController:withKeyring` with `KeyringController:withKeyringV2` for the EVM account provider ([#8491](https://github.com/MetaMask/core/pull/8491)) |
|
|
||
| protected async withKeyring<SelectedKeyring, CallbackResult = void>( | ||
| selector: KeyringSelector, | ||
| selector: KeyringSelectorV2, |
There was a problem hiding this comment.
I wonder if we should have introduced a withKeyringV2 here too? This way we can add a constraint with SelectedKeyring extends KeyringV2?
The old one would still remain and be used by the SnapAccountProvider for now until we have proper support for SnapKeyringV2.
WDYT?
| return { | ||
| ...actual, | ||
| publicToAddress: jest.fn(), | ||
| }; | ||
| }); | ||
|
|
||
| function mockNextDiscoveryAddress(address: string): void { | ||
| jest.mocked(publicToAddress).mockReturnValue(createBytes(address as Hex)); | ||
| id: account.id, | ||
| address: account.address, | ||
| type: account.type, | ||
| options: account.options, | ||
| methods: account.methods, | ||
| scopes: account.scopes ?? [], | ||
| } as unknown as KeyringAccount; |
There was a problem hiding this comment.
An alternative without casting would be to use:
const {
metadata, // `InternalAccount` is just a `KeyringAccount` + `metadata.
...keyringAccount;
};
return keyringAccount;| class MockEthKeyring implements EthKeyring { | ||
| readonly type = 'MockEthKeyring'; | ||
| // Internal test-only state — not part of the Keyring interface. | ||
| readonly accounts: InternalAccount[]; |
There was a problem hiding this comment.
Then we could just use KeyringAccount right away no?
| const newAccountsIndex = this.accounts.length; | ||
| createAccounts = jest | ||
| .fn() | ||
| .mockImplementation((options: Record<string, unknown>) => { |
There was a problem hiding this comment.
We can properly type this with CreateAccountOptions (from the keyring-api)
| // The account was created to peek at the address, then deleted | ||
| // because there was no on-chain activity. | ||
| expect(keyring.deleteAccount).toHaveBeenCalledTimes(1); |
There was a problem hiding this comment.
Oh, I completely forgot about this, and actually we might have to tweak this a bit...
We don't want to create an account + remove it after, this cause some weird glitch where a group would be created out of this account and be removed 1s after...
So maybe we need to re-export the same getters than the HdKeyring v1 to be able to derive the address ourselves for now 🤔 (or add a peekAddress inside the v2 wrapper for now.
NOTE: We might need to re-export root and other getters, cause I think we use them on the clients for the Snap platform 😅
WDYT?
2 participants
Explanation
Changes
Base provider & types
AllowedActionsto useKeyringControllerWithKeyringV2ActionBaseBip44AccountProvider.withKeyringnow callsKeyringController:withKeyringV2withKeyringSelectorV2EvmAccountProvider
KeyringV2methods:getAccounts()returnsKeyringAccount[],createAccounts()replacesaddAccounts()bip44:derive-indexsinceHdKeyringV2does not supportbip44:derive-index-rangediscoverAccountsreplaces the V1keyring.rootpeek with a create → check → delete-if-no-activity pattern, preserving the same net behavior of not leaving unused accounts in theHD keyring
#getAddressFromGroupIndex(relied on V1 HD keyring internals)SnapAccountProvider
RestrictedSnapKeyringescape-hatch pattern that leaked keyring references outside thewithKeyringmutexwithSnapnow runs operations inside thewithKeyringV2scopecreateBip44Accountsalways useskeyring.createAccounts()directly, removing the V1/V2 branching based onbatchedconfigresyncAccountsuseskeyring.deleteAccount(id)instead ofkeyring.removeAccount(address)Snap subclasses (BTC, SOL, TRX)
createAccountV1overrides — V2 snaps handle chain-specific account creation internallyReferences
N/a
Checklist
Note
Medium Risk
Touches account creation/discovery paths and keyring mutex/persistence boundaries; regressions could lead to missed discovery results or unintended account/keyring state changes, though changes are scoped to provider/keyring integration.
Overview
BREAKING: Migrates multichain account providers’ keyring access from
KeyringController:withKeyringtoKeyringController:withKeyringV2, updating shared types/messenger delegation and the base providerwithKeyringhelper to useKeyringSelectorV2.Updates
EvmAccountProviderto use V2 keyring methods (getAccounts()returningKeyringAccount[],createAccounts(),deleteAccount()), changes range creation to loop per-index (since V2 HD keyrings don’t support range creation), and rewrites discovery to do create→RPC activity check→delete-if-inactive within a singlewithKeyringV2callback to ensure no net keyring state change when no activity is found.Keeps Snap-based providers on V1 keyring behavior for now by overriding
withKeyringto callKeyringController:withKeyring, while still accepting V2 selectors; tests are updated accordingly and the changelog documents the breaking API shift.Reviewed by Cursor Bugbot for commit 15f7986. Bugbot is set up for automated code reviews on this repo. Configure here.