feat(multichain-account-service): re-sync multichain account and wallets on account events #6165
Conversation
…ets on account events
|
@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. |
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
| // This new account might be a new multichain account, and the wallet might not | ||
| // know it yet, so we need to force-sync here. | ||
| wallet.sync(); |
There was a problem hiding this comment.
Not sure if I follow what is happening here. If it is a new account then the wallet should not now it, I don't understand why it is a "might".
And the sync operation seems quite expensive to be executed for every new account added, at least the second part of it,
for (const [groupIndex, multichainAccount] of this.#accounts.entries()) {
multichainAccount.sync();
// Clean up old multichain accounts.
if (!multichainAccount.hasAccounts()) {
this.#accounts.delete(groupIndex);
}
}There was a problem hiding this comment.
For the moment, we don't control the account creation through the multichain account "SDK".
So you might have the following scenario:
- You have "Wallet 1"
- It has 1 "Multichain Account 1" (index 0)
- You now create a new EVM account for the next index (index 1)
- This will trigger a
:accountAddedevent - Currently "Wallet 1" lives in memory and it does not know about this new account (index 1) yet
- Thus, the service
syncthe appropriate wallet (by checking theoptions.entropy.id):- To let it know that a new account got added
- So the wallet can check all accounts (from its providers) and create (a
MultichainAccountinstance) for the missing "Multichain Account 2" (index 1)
- So the wallet can check all accounts (from its providers) and create (a
- To let it know that a new account got added
I initially had another idea where AccountProvider would implement a sort pub/sub pattern to let the MultichainAccountWallet or MultichainAccount instances that something got added:
But the implementation felt more complex, and I opted for the "manual sync" + controller events instead (at least, for the moment).
And the sync operation seems quite expensive to be executed for every new account added
I'd say, it's ok for the moment. We can always change this if needed (it won't change the public API part). Also, we might not use events anymore once we introduce operations to create "a multichain account" entirely through the SDK/account providers.
There was a problem hiding this comment.
That being said, I can maybe improve this 🤔
Given that we only filter for Bip44Account, we can do this:
// `account` is a `Bip44Account`.
const wallet = this.#wallets.get(
toMultichainAccountWalletId(account.options.entropy.id),
);
if (wallet) {
let multichainAccount = wallet.getMultichainAccount(
account.options.entropy.groupIndex
);
if (multichainAccount) {
// This new account is part of an existing multichain account, let it
// re-sync with its providers.
multichainAccount.sync();
} else {
// This new account is a new multichain account, let the wallet know
// it has to re-sync with its providers.
wallet.sync();
multichainAccount = wallet.getMultichainAccount(
account.options.entropy.groupIndex,
);
}
if (multichainAccount) {
// Same here, this account should have been already grouped in that
// multichain account.
this.#reverse.set(account.id, {
wallet,
multichainAccount,
});
}
}Thus we avoid doing a wallet.sync unnecessarily.
There was a problem hiding this comment.
Agreed on the sync method feeling like overkill for a single account added. Wondering if there's a more surgical way of syncing this new account?
There was a problem hiding this comment.
Yup, delegating the synchronization on the wallet and multichain accounts themselves might be better indeed.
I do have a draft PR for this, but this makes the responsibility of the "account providers". So for now, I'd just keep the sync method until we find a better pattern for this. That being said, I do agree that syncing the entire wallet for 1 account can be avoided, so I'll change the logic for now.
packages/multichain-account-service/src/MultichainAccountService.ts
Outdated
Show resolved
Hide resolved
packages/multichain-account-service/src/MultichainAccountService.ts
Outdated
Show resolved
Hide resolved
| if (found) { | ||
| const { wallet } = found; | ||
|
|
||
| wallet.sync(); |
There was a problem hiding this comment.
Same sentiment as above re: adding an account.
There was a problem hiding this comment.
I did refactor this, lemme know what you think. (I still believe we could do it in a more surgical way, but that should be ok for now).
| return new MockAccountBuilder(account); | ||
| } | ||
|
|
||
| withId(id: InternalAccount['id']) { |
There was a problem hiding this comment.
Can this be Bip44Account<InternalAccount>['id']?
There was a problem hiding this comment.
Since we're dealing with InternalAccount in the tests, I'll keep this as InternalAccount['id'] (Though, I made the change for other InternalAccount['id'], cause I also think it's better that way!) Thanks
Co-authored-by: Hassan Malik <[email protected]>
Co-authored-by: Hassan Malik <[email protected]>
Explanation
Re-sync multichain accounts and wallets upon
AccountsControllerevents.References
N/A
Changelog
N/A
Checklist