perf(keyring-controller): do not fire unnecessary :stageChange in withKeyring
#5732
Conversation
ccharly
force-pushed
the
perf/with-keyring
branch
from
ba27fd6 to
f603e07
Compare
|
@metamaskbot publish-preview |
|
Preview builds have been published. See these instructions for more information about preview builds. Expand for full list of packages and versions. |
| @@ -1551,8 +1552,9 @@ export class KeyringController extends BaseController< | |||
| ): Promise<CallbackResult> { | |||
| this.#assertIsUnlocked(); | |||
|
|
|||
| return this.#persistOrRollback(async () => { | |||
| return this.#withRollback(async () => { | |||
There was a problem hiding this comment.
Thoughts on moving the check in #persistOrRollback instead of using #withRollback here? it would avoid giving to withKeyring the responsibility to update the vault directly
There was a problem hiding this comment.
Yes, that was my initial idea but the thing is I need to have a reference of the keyring to be able to serialize it.
So this would probably require to move the "keyring selector" to that method?
I also thought of adding a new persistKeyringOrRollback to not pollute the existing persistOrRollback.
Any preferences?
There was a problem hiding this comment.
We wouldn't check a single keyring in #persistOrRollback (as some operations need to interact with multiple keyrings) - we would have to use this.#getSerializedKeyrings() before and after executing the callback passed to #persistOrRollback
There was a problem hiding this comment.
That makes sense yes, I just went with the "fastest option" (being, just serializing + comparing what we need in that case)
But having this in #persistOrRollback is an option too, just potentially a bit slower, but maybe we wouldn't notice the difference.
| // This is a new keyring, so we force the vault update in that case. | ||
| forceUpdate = true; |
There was a problem hiding this comment.
As the withKeyring overload that creates a new keyring is deprecated (createIfMissing), thoughts on removing it completely so that we don't need to manage this case?
There was a problem hiding this comment.
I can definitely get rid of this yes. I thought we were still relying on this behavior in some of the clients, and given that the keyring is not yet on the vault at that point, I needed a way to force call the #updateVault method.
There was a problem hiding this comment.
I think that it is probably used in a couple of places, but I don't see blockers from using addNewKeyring explicitly
There was a problem hiding this comment.
As discussed internally, if we don't support this anymore, we probably wants to remove the deprecated method altogether, given that, without the forceUpdate logic, the createIfMissing option won't work anymore, so I'd just remove it entirely (but we could do that in another PR and keep the force update logic for now)
| it('should update the vault if the keyring is being updated', async () => { | ||
| await withController( | ||
| { keyringBuilders: [keyringBuilderFactory(MockMutableKeyring)] }, | ||
| async ({ controller, messenger }) => { | ||
| const selector = { type: MockMutableKeyring.type }; | ||
|
|
||
| const mockStateChange = jest.fn(); | ||
| messenger.subscribe( | ||
| 'KeyringController:stateChange', | ||
| mockStateChange, | ||
| ); | ||
|
|
||
| await controller.withKeyring( | ||
| selector, | ||
| async ({ keyring }) => { | ||
| (keyring as MockMutableKeyring).update(); // Update the keyring state. | ||
| }, | ||
| { | ||
| createIfMissing: true, | ||
| }, | ||
| ); | ||
|
|
||
| expect(mockStateChange).toHaveBeenCalled(); | ||
| expect(controller.state.keyrings).toHaveLength(2); | ||
| }, | ||
| ); | ||
| }); |
There was a problem hiding this comment.
nit: We can probably accomplish the same result without adding a MockMutableKeyring and by looking at what happens to the vault specifically through the encryptor (also without using the deprecated variant of withKeyring):
| it('should update the vault if the keyring is being updated', async () => { | |
| await withController( | |
| { keyringBuilders: [keyringBuilderFactory(MockMutableKeyring)] }, | |
| async ({ controller, messenger }) => { | |
| const selector = { type: MockMutableKeyring.type }; | |
| const mockStateChange = jest.fn(); | |
| messenger.subscribe( | |
| 'KeyringController:stateChange', | |
| mockStateChange, | |
| ); | |
| await controller.withKeyring( | |
| selector, | |
| async ({ keyring }) => { | |
| (keyring as MockMutableKeyring).update(); // Update the keyring state. | |
| }, | |
| { | |
| createIfMissing: true, | |
| }, | |
| ); | |
| expect(mockStateChange).toHaveBeenCalled(); | |
| expect(controller.state.keyrings).toHaveLength(2); | |
| }, | |
| ); | |
| }); | |
| it('should update the vault if the keyring is being updated', async () => { | |
| stubKeyringClassWithAccount(MockKeyring, '0x1234'); | |
| await withController( | |
| { keyringBuilders: [keyringBuilderFactory(MockKeyring)] }, | |
| async ({ controller, encryptor }) => { | |
| await controller.addNewKeyring(MockKeyring.type); | |
| const encryptSpy = jest.spyOn(encryptor, 'encrypt'); | |
| jest.spyOn(MockKeyring.prototype, 'serialize').mockResolvedValue({ | |
| foo: 'bar', | |
| }); | |
| await controller.withKeyring( | |
| { type: MockKeyring.type }, | |
| async () => { | |
| // No-op | |
| }, | |
| ); | |
| expect(encryptSpy).not.toHaveBeenCalled(); | |
| }, | |
| ); | |
| }); |
There was a problem hiding this comment.
I did something similar to your suggestion but kept my use of the messenger since I think it's more "explicit" for my use-case: 676415f
Explanation
We were always calling
#updateVaultwhen usingwithKeyringeven when the keyring was not mutated.This PR now compare the states after the operation execution and see if the keyring got updated or not and decide to call
#updateVaultonly if needed.Testing PR:
References
N/A
Changelog
N/A
Checklist