Subscribe to QRKeyring store and publish updates

[mikesposito]

Explanation

Currently, the extension is directly subscribing to the QRKeyring store, to determine whether to open the modal for syncing and signing.

This raises concerns when the instance of the QRKeyring will be held by KeyringController instead of the client because it will be tricky to handle subscribe and unsubscribe in the keyring's lifecycle, risking to have multiple subscriptions to the same QRKeyring instance or even no subscriptions at all!

This PR gives this responsibility to KeyringController.
KeyringController will subscribe to the QRKeyring instance in these methods:

• submitPassword - because here we unlock the vault and potentially find a QRKeyring (even in case of re-construction from backup)
• submitEncryptionKey - same as above
• getOrAddQRKeyring - because here we explicitly create a new QRKeyring if it doesn't exist
• addNewKeyring(type) - when type is of a QRKeyring then it will return getOrAddQRKeyring, to avoid creating multiple keyring instances (and multiple subscriptions)

KeyringController will unsubscribe from the QRKeyring instance (if there's one ) when calling setLocked().

Moreover, KeyringController:qrKeyringStateChange event is now available on the messenger

References

• Related to Keyring consolidation: Use QRKeyring from core KeyringController metamask-extension#18776
• Related to Use QRKeyring from KeyringController metamask-extension#20502

Changelog

@metamask/keyring-controller

• BREAKING: addNewKeyring(type) return type changed from Promise<Keyring<Json>> to Promise<unknown>
  • When calling with QRKeyring type the keyring instance is retrieved or created (no multiple QRKeyring instances possible)
• ADDED: Add getQRKeyring(): QRKeyring | undefined method
• ADDED: Add KeyringController:qrKeyringStateChange messenger event
  The event emits updates from the internal QRKeyring instance, if there's one

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]

We shouldn't assume that we have just one QRKeyring as calling twice this expression:

this.addNewKeyring(KeyringTypes.qr)

would end up creating two different QRKeyring instances

[mikesposito]

The real problem with having multiple QRKeyring instances would be that the event listeners would publish through the same messenger event, making impossible from a client to establish which specific QRKeyring emitted that state update.

[mikesposito]

A possible solution for this would be to subscribe to QRKeyring during addNewAccount only for the first keyring added: this would force a client that wants to use more than one QRKeyring instance to implement a subscribing solution on its side.

[mikesposito]

Or, as we don't explicitly support multiple keyrings of the same type (yet), we could simply throw an error when calling addNewKeyring if there's a keyring of that type already (with the exception of SimpleKeyring, but that one is created through importAccountWithStrategy).

[mikesposito]

Ops, I actually forgot that I added this in addNewKeyring to prevent exactly this 🤦 :

if (type === KeyringTypes.qr) {
   return this.getOrAddQRKeyring();
}

So the behavior in this PR is that if the addNewKeyring is called with a QR type, then the QRKeyring is retrieved or created

[mikesposito]

Not strictly required by this PR, but makes more sense since the keyring created could be of any type (for example, a QRKeyring)

[kanthesha]

Can we have a base class/interface for the Keyring, then having a child classes with specific Keyring types for example QRKering.

[mikesposito]

We have a type that should be implemented by all Keyrings: https://github.com/MetaMask/utils/blob/4589c755e3005024c9071dd0c23c933335c3efa4/src/keyring.ts#L47

But that's not yet the case, so we still have to handle their API differences. In this specific case, it is more correct to say that we don't know what we are returning, instead of assuming that we are returning a Keyring<Json - also because we don't restrict the type of keyrings that can be created through this controller

[mikesposito]

We do this to avoid this issue: https://github.com/MetaMask/core/pull/1702/files#r1334384936

[Gudahtt]

LGTM!