Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions app/scripts/metamask-controller.js
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Copy link
Contributor

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

Copy link
Contributor Author

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!

// 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();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to init the AccountTreeController before syncing?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, let me rework this PR and see if we can do the same than on mobile (which is running all this "async"/non-awaited)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I made it async now, so this will happen after all initialization!

///: 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(
Expand Down
18 changes: 18 additions & 0 deletions app/scripts/metamask-controller.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -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();
});
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: Race condition in async test verification

The test uses waitForAllPromises() which only waits for one process.nextTick, but the resyncAndAlignAccounts function contains two sequential await statements. This creates a race condition where the test assertions may execute before both resyncAccounts and alignWallets complete, causing intermittent test failures. The fire-and-forget pattern with void means the function execution isn't guaranteed to finish before the assertions run.

Fix in Cursor Fix in Web

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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');
Expand Down
Loading