test(KeyringController): mock encryptor upgrade #5943

matthiasgeihs · 2025-06-09T14:44:57Z

Explanation

For testing envelope encryption (#5940), we need a more versatile mock encryptor. (I.e., one that can handle multiple ciphertexts at the same time.)

Hence, here we upgrade the mock encryptor as a preparation for #5490.

References

Changelog

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 communicated my changes to consumers by updating changelogs for packages I've changed, highlighting breaking changes as necessary
I've prepared draft pull requests for clients and consumer packages to resolve any breaking changes

mikesposito · 2025-06-10T12:13:24Z

packages/keyring-controller/src/KeyringController.test.ts

@@ -2924,14 +2919,12 @@ describe('KeyringController', () => {
        it('should unlock the wallet also if encryption parameters are outdated and the vault upgrade fails', async () => {
          await withController(
            {
-              skipVaultCreation: true,


Initial vault creation was being skipped in submitPassword tests because the default withController behavior is to generate a vault and leave the controller unlocked. So when these tests get executed KeyringController will already be unlocked.

For some of these test cases, it shouldn't be a problem. Though for some of them, there's the risk of getting false positives

initially i've changed it because the withController function didn't allow to setup with encryption key. i've changed that now, so will check if i can change it back in some cases. would be good to know in which cases it's problematic to have this skipped.

I think it's potentially problematic for any test case that aims to verify how unlocking works, since the controller is already unlocked before the tests.
e.g. if a test case asserts expect(controller.state.isUnlocked).toBe(true);, that would potentially be a false positive

i've changed skipVaultCreation back where i could.

tbh, i think it might be better to simply lock the controller in cases where you need a locked controller.
the issue i had with setting up the vault like this is that it depends on the implementation of the mock controller, so everytime this is changed, you need to change the fixed vault value that is used in tests.

tbh, I think it might be better to simply lock the controller in cases where you need a locked controller.

This may work in some cases, though you'd have to use another public function to test another one, which is ok, but better to avoid it if we can make the test simpler and more independent

the issue i had with setting up the vault like this is that it depends on the implementation of the mock controller

Perhaps you are referring to the encryptor? If so, that's the reason why we set up these mock responses:

jest.spyOn(encryptor, 'decryptWithKey').mockResolvedValueOnce([ { type: KeyringTypes.hd, data: '0x123', } );

Sorry. Yes. Mock encryptor

packages/keyring-controller/src/KeyringController.test.ts

mikesposito · 2025-06-10T12:20:57Z

packages/keyring-controller/src/KeyringController.test.ts

+                // TODO actually this does trigger re-encryption. The catch is
+                // that this test is run with cacheEncryptionKey enabled, so
+                // `encryptWithKey` is being used instead of `encrypt`. Hence,
+                // the spy on `encrypt` doesn't trigger. 🤦‍♂️


The cached key is only used if:

cacheEncryptionKey is set to true

There's an actual key cached

There's a vault

The vault is already using the latest cipher configuration

This means that if any of the above is false, encryptWithDetail will be called instead of encryptWithKey, which used to call encrypt internally.

This isn't the case anymore, but only after changes in this PR.

That's not true. You can revert to main branch and debug step through the execution. You will see that it goes through the useCacheKey branch and therefore calls encryptWithKey.

In that case, can we fix the test, or remove it altogether?

Probably OK to fix in a later PR?

packages/keyring-controller/CHANGELOG.md

mikesposito · 2025-06-11T11:00:49Z

packages/keyring-controller/src/KeyringController.test.ts

            async ({ controller, initialState }) => {
-              const initialVault = controller.state.vault;
              const initialKeyrings = controller.state.keyrings;
              await controller.createNewVaultAndRestore(
                password,
                uint8ArraySeed,
              );
              expect(controller.state).not.toBe(initialState);
              expect(controller.state.vault).toBeDefined();
-              expect(controller.state.vault).toStrictEqual(initialVault);
              expect(controller.state.keyrings).toHaveLength(


Is this deletion still relevant after the mock encryptor refactor, or, do you think we can check in other ways if the new vault is the intended one?

if you add that, this is what you get

so the mnemonic and id are different. (as expected because mnenomic is randomnly generated in the first case and fixed in the second. not sure about the id, but probably similar case.)

feel free to make a suggestion how this check can be added back again (or at least the intention behind it)

Indeed, I guess we could make it match if we made this test import the same SRP that was in-place before. But I would interpret the intent to be importing a new SRP, so them not matching seems appropriate.

I do agree that matching the new vault with the initial one doesn't make sense, though what I was proposing was to leverage the new mock encryptor capabilities to check if the seed in the new vault is the one imported

packages/keyring-controller/src/KeyringController.test.ts

matthiasgeihs · 2025-06-13T07:11:37Z

rebase

Gudahtt · 2025-06-13T14:07:48Z

packages/keyring-controller/src/KeyringController.test.ts

+              skipVaultCreation: true,
+              state: {
+                vault: freshVault,
+                // @ts-expect-error we want to force the controller to have an


Nit: Hmm. I don't quite understand why we would need this here, as the encryptionSalt state is not used for anything other than detecting a mismatch between encryptionKey and vault (which is not relevant for this test, certainly because encryptionKey is not even present).

I see that it is a pre-existing widespread pattern in these tests though, so we can fix it later.

Gudahtt · 2025-06-13T14:09:56Z

packages/keyring-controller/tests/mocks/mockEncryptor.ts

+  // Generate random salt.
+
+  // return crypto.randomUUID();
+  return SALT; // TODO some tests rely on fixed salt, but wouldn't it be better to generate random value here?


Indeed, a more realistic mock encryptor would certainly have advantages. I wonder if we could take it a step further even than making the mock more 'realistic', and instead use a real encryptor that uses some very fast/insecure encryption strategies.

Gudahtt · 2025-06-13T14:21:45Z

packages/keyring-controller/src/KeyringController.test.ts

@@ -3242,7 +3268,7 @@ describe('KeyringController', () => {
          await expect(
            controller.submitEncryptionKey(
              MOCK_ENCRYPTION_KEY,
-              initialState.encryptionSalt as string,
+              initialState.encryptionSalt as string, // TODO Why need the salt here??


Nit: Answered in the other PR, but this is used because the client backs up the key and salt in session storage, and we use the salt to ensure the key matches the vault

removed in 94ad779

Gudahtt

LGTM!

tbh, not sure why the coverage changed. i've checked manually and it looks like exactly the same branches as before are not covered. still, it shows up as one less covered branch (163 vs 164 before).

- add back skipVaultCreation where it was used before - revert change to branch coverage threshold, as this went back to before levels after reverting the above

mikesposito · 2025-06-18T14:00:10Z

@matthiasgeihs are you ok with merging this one?

matthiasgeihs requested review from mikesposito and danroc June 9, 2025 16:02

matthiasgeihs force-pushed the feat/keyring/mock-encryptor branch 2 times, most recently from 2c0dbd5 to 209c58d Compare June 10, 2025 02:47

matthiasgeihs marked this pull request as ready for review June 10, 2025 02:54

matthiasgeihs requested review from a team as code owners June 10, 2025 02:54

mikesposito reviewed Jun 10, 2025

View reviewed changes

packages/keyring-controller/src/KeyringController.test.ts Outdated Show resolved Hide resolved

mikesposito reviewed Jun 10, 2025

View reviewed changes

matthiasgeihs mentioned this pull request Jun 11, 2025

vault envelope encryption #5940

Open

4 tasks

mikesposito reviewed Jun 11, 2025

View reviewed changes

mikesposito mentioned this pull request Jun 11, 2025

refactor!: drop uncached encryption support #5963

Open

4 tasks

matthiasgeihs added the no-changelog label Jun 13, 2025

matthiasgeihs force-pushed the feat/keyring/mock-encryptor branch from 0025960 to 3f83da4 Compare June 13, 2025 07:11

matthiasgeihs force-pushed the feat/keyring/mock-encryptor branch from 3f83da4 to 72d3d75 Compare June 13, 2025 11:45

Gudahtt reviewed Jun 13, 2025

View reviewed changes

Gudahtt previously approved these changes Jun 13, 2025

View reviewed changes

matthiasgeihs added 7 commits June 13, 2025 17:18

(wip) new mock encryptor

24c3bb8

cleanup

0f191cf

adjust coverage threshold

1f25ec5

tbh, not sure why the coverage changed. i've checked manually and it looks like exactly the same branches as before are not covered. still, it shows up as one less covered branch (163 vs 164 before).

update changelog

709bd54

address review comments

79a9fe4

- add back skipVaultCreation where it was used before - revert change to branch coverage threshold, as this went back to before levels after reverting the above

cleanup comment

867a525

Remove CHANGELOG entry

12f8275

matthiasgeihs force-pushed the feat/keyring/mock-encryptor branch from 72d3d75 to 12f8275 Compare June 13, 2025 15:18

remove obsolete comment

94ad779

matthiasgeihs dismissed Gudahtt’s stale review via 94ad779 June 13, 2025 15:41

Gudahtt approved these changes Jun 13, 2025

View reviewed changes

mikesposito changed the title ~~Keyring controller: mock encryptor upgrade~~ test(KeyringController): mock encryptor upgrade Jun 17, 2025

Uh oh!

test(KeyringController): mock encryptor upgrade #5943

Are you sure you want to change the base?

test(KeyringController): mock encryptor upgrade #5943

Uh oh!

Conversation

matthiasgeihs commented Jun 9, 2025 • edited by mikesposito Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Explanation

References

Changelog

Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mikesposito Jun 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

matthiasgeihs commented Jun 13, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Gudahtt left a comment

Choose a reason for hiding this comment

Uh oh!

mikesposito commented Jun 18, 2025

Uh oh!

Uh oh!

matthiasgeihs commented Jun 9, 2025 •

edited by mikesposito

Loading

mikesposito Jun 10, 2025 •

edited

Loading