-
Notifications
You must be signed in to change notification settings - Fork 5.4k
fix: add resync mechanism to sync Snap accounts states with client accounts cp-13.11.0 #37987
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -5207,12 +5207,20 @@ export default class MetamaskController extends EventEmitter { | |
| } | ||
|
|
||
| await this.accountsController.updateAccounts(); | ||
|
|
||
| ///: BEGIN:ONLY_INCLUDE_IF(multichain) | ||
| // Init multichain accounts after creating internal accounts. | ||
| await this.multichainAccountService.init(); | ||
| // READ THIS CAREFULLY: | ||
| // There is is/was a bug with Snap accounts that can be desynchronized (Solana). To | ||
| // automatically "fix" this corrupted state, we run this method which will re-sync | ||
| // MetaMask accounts and Snap accounts upon login. | ||
| await this.multichainAccountService.resyncAccounts(); | ||
|
||
| ///: END:ONLY_INCLUDE_IF | ||
|
|
||
| // Force account-tree refresh after all accounts have been updated. | ||
| this.accountTreeController.init(); | ||
|
|
||
| // TODO: Move this logic to the SnapKeyring directly. | ||
| // Forward selected accounts to the Snap keyring, so each Snaps can fetch those accounts. | ||
| await this.forwardSelectedAccountGroupToSnapKeyring( | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -728,6 +728,24 @@ describe('MetaMaskController', () => { | |
| }); | ||
| }); | ||
|
|
||
| describe('#submitPasswordOrEncryptionKey', () => { | ||
| const password = 'a-fake-password'; | ||
|
|
||
| it('should call multichainAccountService.resyncAccounts when submitPasswordOrEncryptionKey is called', async () => { | ||
| const mockResyncAccounts = jest.fn(); | ||
|
|
||
| metamaskController.multichainAccountService = { | ||
| init: jest.fn(), | ||
| resyncAccounts: mockResyncAccounts, | ||
| }; | ||
|
|
||
| await metamaskController.createNewVaultAndRestore(password, TEST_SEED); | ||
| await metamaskController.submitPasswordOrEncryptionKey({ password }); | ||
|
|
||
| expect(mockResyncAccounts).toHaveBeenCalled(); | ||
| }); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Bug: Race condition in async test verificationThe test uses
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's ok for now, I'll keep this simple. |
||
| }); | ||
|
|
||
| describe('setLocked', () => { | ||
| it('should lock KeyringController', async () => { | ||
| await metamaskController.createNewVaultAndKeychain('password'); | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we add a reference to the bug in the comments
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did add the reference now, even if this PR is more like a "mitigation" rather than a true fix, but still worth IMO!