-
-
Notifications
You must be signed in to change notification settings - Fork 6
feat(account-api): add reverse mapping + AccountProvider
events
#320
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
base: main
Are you sure you want to change the base?
Conversation
AccountProvider
events
@@ -46,6 +46,7 @@ | |||
"test:watch": "jest --watch" | |||
}, | |||
"dependencies": { | |||
"@metamask/base-controller": "^8.0.1", |
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.
Required for the Messenger
, but we will change that to @metamask/messenger
once available.
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.
Bug: Account Retrieval Method Returns Incorrect Type
The abstract method AccountProvider.getAccount
is incorrectly typed to return Bip44Account<Account>
. It should return Bip44Account<Account> | undefined
. This forces implementations to either throw an error or use unsafe type casting (as seen in MockAccountProvider
) when an account with the given ID is not found. This can lead to runtime crashes in MultichainAccount.getAccount
and MultichainAccount.getAccounts
if their internal account ID lists become out of sync with the provider's actual state, causing them to request non-existent accounts from the provider.
packages/account-api/src/api/provider.ts#L34-L35
accounts/packages/account-api/src/api/provider.ts
Lines 34 to 35 in d968dd7
*/ | |
abstract getAccount(id: Bip44Account<Account>['id']): Bip44Account<Account>; |
packages/account-api/src/api/multichain/account.ts#L120-L128
accounts/packages/account-api/src/api/multichain/account.ts
Lines 120 to 128 in d968dd7
allAccounts = allAccounts.concat( | |
accounts | |
.map((id) => provider.getAccount(id)) | |
.filter( | |
(account) => | |
account.options.entropy.id === this.wallet.entropySource && | |
account.options.entropy.groupIndex === this.index, | |
), | |
); |
packages/account-api/src/api/multichain/account.ts#L146-L149
accounts/packages/account-api/src/api/multichain/account.ts
Lines 146 to 149 in d968dd7
return undefined; | |
} | |
return provider.getAccount(id); | |
} |
packages/account-api/src/index.test.ts#L159-L167
accounts/packages/account-api/src/index.test.ts
Lines 159 to 167 in d968dd7
getAccount = jest | |
.fn() | |
.mockImplementation((id: InternalAccount['id']): InternalAccount => { | |
// Assuming this never fails. | |
return this.#accounts.find( | |
(account) => account.id === id, | |
) as InternalAccount; | |
}); |
Bug: Multichain Account Retrieval Returns Incorrect Accounts
The MultichainAccount.getAccount()
method can return accounts that do not belong to the specific multichain account. This is due to the reverse
map, used by getAccount()
, being populated in the constructor with all accounts from providers, instead of only those matching the multichain account's entropy.id
and entropy.groupIndex
. Consequently, getAccount()
retrieves accounts based solely on ID presence in this unfiltered map, without validating their entropy
properties, a check correctly performed by getAccounts()
.
packages/account-api/src/api/multichain/account.ts#L139-L149
accounts/packages/account-api/src/api/multichain/account.ts
Lines 139 to 149 in d968dd7
*/ | |
getAccount(id: Account['id']): Bip44Account<Account> | undefined { | |
const provider = this.#reverse.get(id); | |
// If there's nothing in the map, it means we tried to get an account | |
// that does not belong to this multichain account. | |
if (!provider) { | |
return undefined; | |
} | |
return provider.getAccount(id); | |
} |
packages/account-api/src/api/multichain/account.ts#L57-L72
accounts/packages/account-api/src/api/multichain/account.ts
Lines 57 to 72 in d968dd7
for (const provider of providers) { | |
// We only use IDs to always fetch the latest version of accounts. | |
const accounts = provider.getAccounts().map((account) => account.id); | |
this.#accounts.push({ | |
provider, | |
accounts, | |
}); | |
// Reverse-mapping for fast indexing. | |
for (const id of accounts) { | |
this.#reverse.set(id, provider); | |
} | |
} | |
} |
Was this report helpful? Give feedback by reacting with 👍 or 👎
Implement reverse-mapping + event mechanism for the
AccountProvider
.This also solve the issue that a wallet might not know when a new multichain account got added to the wallet.
For now, we omitted any use of
:accountRemoved
event since we don't plan to remove those accounts for the moment.