Add encryptor types

[mikesposito]

Description

Currently encryptor type is any, and as different clients can use different encryptor utilities, we need to establish a minimum supported (or needed) interface that the encryptor should implement.

This PR adds some types and guards to the encryptor, taking into account @metamask/browser-passworder, which is the encryptor used by the extension, and the Encryptor class used by mobile

Restrictions have also been added for cacheEncryptionKey and encryptor, as the methods that the encryptor must support depends on whether we need the encryption key to be exported or not. To do this, public class variables have been substituted with sharp vars and a check on construction has been added: an error is thrown when they are incompatible

Changes

• BREAKING: encryptor class variable is now inaccessible
• BREAKING: cacheEncryptionKey class variable is now inaccessible
• BREAKING: encryptor constructor option type has been changed to GenericEncryptor | ExportableKeyEncryptor | undefined

References

Waiting on MetaMask/browser-passworder#49

• Fixes encryptor lacks sufficient typing #295

Checklist

• I've updated the test suite for new or updated code as appropriate
• I've updated documentation for new or updated code as appropriate (note: this will usually be JSDoc)
• I've highlighted breaking changes using the "BREAKING" category above as appropriate

[mikesposito]

We can't cache the encryption key if the provided encryptor does not support it

[mikesposito]

This takes inspiration from @metamask/keyring-controller: https://github.com/MetaMask/core/blob/main/packages/keyring-controller/tests/mocks/mockEncryptor.ts

[legobeat]

Would be nice if we can get out #290 before this is merged.

[mikesposito]

These are not applicable anymore, as cacheEncryptionKey is set to true on construction (before the first createNewVaultAndKeychain > persistAllKeyrings is called), so encryptionKey and encryptionSalt will be already defined

[Gudahtt]

A few pending suggestions/questions but overall this LGTM!

[Gudahtt]

Nit: Hmm, this should always exist if encryptionKey exists. Maybe there's a way to make that more clear using types.

[mikesposito]

I changed the KeyringControllerState type to:

export type KeyringControllerState = {
  keyrings: KeyringObject[];
  isUnlocked: boolean;
} & (
  | { encryptionKey: string; encryptionSalt: string }
  | {
      encryptionKey?: never;
      encryptionSalt?: never;
    }
);

Is there a better way to represent this?

[mikesposito]

We could also enforce the right encryptor type doing something similar on KeyringControllerArgs:

export type KeyringControllerArgs = {
  keyringBuilders?: { (): Keyring<Json>; type: string }[];
  initState?: KeyringControllerPersistentState;
} & (
  | { encryptor?: ExportableKeyEncryptor; cacheEncryptionKey: true }
  | {
      encryptor?: GenericEncryptor | ExportableKeyEncryptor;
      cacheEncryptionKey: false;
    }
);

[Gudahtt]

Nice! Looks reasonable

[Gudahtt]

Nit: most of the work this function is doing (the encryption) is being thrown away here. Maybe we can come back to this an optimize this by having it just check whether the vault is outdated or not instead (e.g. expose isVaultUpdated, or inline that logic here).

[mikesposito]

Yes, that's quite bad. The best way would be to expose isVaultUpdated from browser-passworder, because I don't think there's a nice way to inline that logic here, as eth-keyring-controller would have to know the latest vault parameters (DEFAULT_DERIVATION_PARAMS in browser-passworder, which is also not exposed)

[Gudahtt]

Right, good point. Maybe something to address in a follow up PR then

[mcmire]

I had some nits as well, but this makes sense and looks good.

[Gudahtt]

LGTM!