[keyring-controller] Add withKeyring method

[mikesposito]

Explanation

Part of KeyringController responsibilies is ensuring each operation is mutually exclusive and atomic, updating keyring instances and the vault (or rolling them back) in the same mutex lock.

However, the ability of clients to have direct access to a keyring instance represents a loophole, as with the current implementation they don’t have to comply with the rules enforced by the controller: we should provide a way for clients to interact with a keyring instance through safeguards provided by KeyringController.

The current behavior is this one:

1. Client obtains a keyring instance through getKeyringForAccount
2. Client interacts with the instance
3. Client calls persistAllKeyrings

We should, instead, have something like this:

1. Client calls a withKeyring method, passing a keyring selector and a callback
2. KeyringController selects the keyring instance and calls the callback with it, after locking the controller operation mutex
3. Client interacts with the keyring instance safely, inside the callback
4. KeyringController, after the callback execution, internally updates the vault or rolls back changes in case of error, and then releases the mutex lock

References

• Fixes [keyring-controller] Provide a safe way to interact with keyring instances #4198
• Related to [keyring-controller] Atomic Operations #4192

Changelog

@metamask/keyring-controller

• ADDED: Added withKeyring method
• DEPRECATED: Deprecated persistAllKeyrings method
  • Use withKeyring instead

Checklist

• I've updated the test suite for new or updated code as appropriate
• I've updated documentation (JSDoc, Markdown, etc.) for new or updated code as appropriate
• I've highlighted breaking changes using the "BREAKING" category above as appropriate

[mikesposito]

The KeyringController:withKeyring action will be added in #4226.

As there are some issues when using action handlers with generic types, we can add the withKeyring method while we find a solution for the action in a separate PR.

[Gudahtt]

Good idea 👍

[Gudahtt]

LGTM! Just a few nits left

[gantunesr]

Great improvement!!

[gantunesr]

This comment is beyond the scope of the PR

How should we ensure the correct execution of an operation when we have multiple keyrings with the same address/account? @Gudahtt, and I believe others, have already pointed this particular issue regarding the removeAccount method

[mikesposito]

Currently every time a new keyring is added (or deserialized from vault) this.#checkForDuplicates is called, but not when adding a new account on a keyring.

This means that a user would be able to add an account that is already on another keyring without issues, but it would then get errors when unlocking the wallet, because the keyrings would be added along with all the accounts already, and checkForDuplicates in #newKeyring would throw.

Perhaps we could check for duplicates also when adding new accounts? it would solve the root issue also regarding removeAccount as well

[Gudahtt]

Perhaps we could check for duplicates also when adding new accounts?

This sounds like a good solution for now, for as long as we're using the address as the unique identifier. Eventually we'll be moving away from that idea hopefully

[mikesposito]

Perhaps we could check for duplicates also when adding new accounts? it would solve the root issue also regarding removeAccount as well

Hmm, I'm thinking that this could lead to interesting scenarios as well: if a user imports an account which can also be derived with e.g. index + 1 of the HD keyring, it would end up in a situation where no more HD accounts can be added until the imported one is removed, and the user would not receive clear errors or warnings about that.

Same applies for Snaps: I'm wondering if a Snap could potentially block account additions on other keyrings, or the other way around

[gantunesr]

for as long as we're using the address as the unique identifier. Eventually we'll be moving away from that idea hopefully

The accounts-controller is introducing the concept of account ID (an UUID) in the clients, it would be good to also use this identifier inside the keyring-controller

I'm wondering if a Snap could potentially block account additions on other keyrings, or the other way around

Yes, this is a possible scenario, I think the long term solution is to use the regular IDs instead of addresses as the identifier. It's a good UX to be able to sign with different keyrings if the user wants to so I don't think we should block the account duplication at a keyring-controller but simplify it at a UI/UX level