feat: add multichain assets controller #5138
Conversation
|
@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. |
| symbol: 'SOL', | ||
| native: true, | ||
| fungible: true, | ||
| iconBase64: |
There was a problem hiding this comment.
shouldn't this be an url? We are getting the url from the static api
There was a problem hiding this comment.
I thought so too, it was defined as iconBase64 in this document which i think is different than a url.
There was a problem hiding this comment.
IMO, this is better to use a base64 here, in case the logo we want to use is not hosted "anywhere".
Also, using "untrusted URLs" seems always """risky""" (I don't have good example for now) 🙈
| // Mock End To be removed once the above is implemented | ||
|
|
||
| // Identify the correct snap that has the right endowment:assets permission | ||
| const currentAssetChain = assets[0].split('/')[0]; |
There was a problem hiding this comment.
We should use parseCaipAssetType here (from @metamask/utils), that would be something like this:
| const currentAssetChain = assets[0].split('/')[0]; | |
| const { chainId } = parseCaipChainId(asset); |
And we can even deconstruct more than that, but if you need the CAIP-2 chain ID here, then chainId is what we need.
| async #handleOnAccountAdded(account: InternalAccount) { | ||
| if (!this.#isNonEvmAccount(account)) { | ||
| // Nothing to do here for EVM accounts | ||
| return; | ||
| } | ||
|
|
||
| // Get assets list | ||
| if (account.metadata.snap) { | ||
| const assets = await this.#getAssets( | ||
| account.id, | ||
| account.metadata.snap.id, | ||
| ); | ||
| const assetsWithoutMetadata = assets.filter( | ||
| (asset) => !this.state.metadata[asset], | ||
| ); | ||
| const snaps = this.#getAllSnaps(); | ||
|
|
||
| const permissions = snaps.map((snap) => | ||
| this.#getSnapsPermissions(snap.id), | ||
| ); | ||
|
|
||
| // Mock start To be removed once the above is implemented | ||
| permissions.forEach((singlePermission) => { | ||
| (singlePermission as unknown as AssetEndowment) = { | ||
| ...singlePermission, | ||
| 'endowment:assets': { | ||
| scopes: ['bip122:000000000019d6689c085ae165831e93'], | ||
| }, | ||
| }; | ||
| }); | ||
| (permissions[0] as unknown as AssetEndowment) = { | ||
| ...permissions[0], | ||
| 'endowment:assets': { | ||
| scopes: ['solana:EtWTRABZaYq6iMfeYKouRu166VU2xqa1'], | ||
| }, | ||
| }; | ||
| // Mock End To be removed once the above is implemented | ||
|
|
||
| // Identify the correct snap that has the right endowment:assets permission | ||
| // TODO: create a mapping of the assets to the snapId based on the permission['endowment:assets']?.scopes from permissions array and the assets[0].split('/')[0] from assets array | ||
| const mapAssetsToSnapId = new Map<CaipAssetType, string[]>(); | ||
| assets.forEach((asset) => { | ||
| const snapIds: string[] = []; | ||
| permissions.forEach((permission: AssetEndowment, index: number) => { | ||
| if ( | ||
| permission['endowment:assets']?.scopes.includes(asset.split('/')[0]) | ||
| ) { | ||
| snapIds.push(snaps[index].id); | ||
| } | ||
| }); | ||
| mapAssetsToSnapId.set(asset, snapIds); | ||
| }); | ||
| // should take the first snapId from the mapAssetsToSnapId and use it to get the assets | ||
|
|
||
| // call the snap to get the metadata | ||
| if (assetsWithoutMetadata.length > 0) { | ||
| const metadata = await this.#getMetadata(assetsWithoutMetadata); | ||
|
|
||
| const newMetadata = { | ||
| ...this.state.metadata, | ||
| ...metadata.assets, | ||
| }; | ||
| this.update((state) => { | ||
| state.metadata = newMetadata; | ||
| }); | ||
| } | ||
| this.update((state) => { | ||
| state.allNonEvmTokens[account.id] = assets; | ||
| }); | ||
| } | ||
| } |
There was a problem hiding this comment.
As discussed together, I do think we should architecture this controller slightly differently.
I believe that having a in-memory mapping of CAIP-2 chain IDs <> Assets Snaps could be a good idea.
This would allow to easily access relevant Snaps for a given scope (CAIP-2 chain ID).
Here's what I have in mind (playground).
Here's the code:
class MultichainAssetsController {
/// Mapping of CAIP-2 Chain ID to Asset Snaps.
#snaps: Record<CaipChainId, Snap[]>;
#assets: Record<CaipChainId, AssetMetadata>;
constructor() {
this.#snaps = this.#getAssetSnaps();
this.#assets = {};
// TODO: Register to SnapController events when the Snaps list is updated (installed/removed, etc...)
}
updateAssetsMetadata(assets: CaipAssetType[]) {
for (const asset of assets) {
// TODO: Do we need to refresh those metadata somehow? Maybe invalidate them based on some timing? diff?
if (this.#assets[asset]) {
// We have metadata already, skipping.
continue;
}
// Extract CAIP-2 chain ID from asset type.
const { chainId } = parseCaipAssetType(asset); // @metamask/utils
// Now fetch metadata from the associated asset Snaps:
// Pick only the first one, we ignore the other Snaps if there are multiple candidates for now.
const [snap] = this.#snaps[chainId];
if (snap) {
this.#assets[asset] = dispatchOnAssetLookup(snap);
}
}
}
handleAccountAssetListUpdated(assets: CaipAssetType[]) {
this.updateAssetsMetadata(assets);
}
handleAccountAdded(account: InternalAccount) {
// TODO: Check non-EVM account.
this.updateAssetsMetadata(account.assets);
}
// Creates a mapping of CAIP-2 Chain ID to Asset Snaps.
#getAssetSnaps() {
const snaps: Record<CaipChainId, Snap[]> = {};
for (const snap of getSnaps().filter((snap) => this.#isAssetSnap(snap))) {
for (const scope of snap.scopes) {
if (!snaps[scope]) {
snaps[scope] = [];
}
snaps[scope].push(snap)
}
}
return snaps;
}
#isAssetSnap(snap: Snap) {
// TODO: Check permissions here.
...
}
}Do you have any opinion on this @danroc?
There was a problem hiding this comment.
Thanks @ccharly, indeed I think it's a good approach
There was a problem hiding this comment.
Thanks @ccharly ! I wonder if its necessary to do the this.#snaps = this.#getAssetSnaps(); inside the constructor;
Im thinking if i do it as part of handleOnAccountAdded; it will avoid making subscriptions to o SnapController events when the Snaps list is updated (installed/removed, etc...) making the controller simpler
There was a problem hiding this comment.
Yes, we can start with this idea @sahar-fehri, my point was to avoid re-listing Snaps every time there's an incoming events.
Re-computing the "Snaps mapping" for every events, seems unnecessary to me. That's why I had the #getAssetSnaps in the constructor to get the initial mapping, and afterward, we would update the mapping based on the SnapController events.
But for now, we can start with a simple approach first! I would still keep the mapping as it is though, WDYT?
There was a problem hiding this comment.
Regarding this comment:
// TODO: Do we need to refresh those metadata somehow? Maybe invalidate them based on some timing? diff?
Agree, I do not expect the token metadata to be refreshed that often too.
For non-evm tokens; the one part of token metadata that users would usually update is the token logo.
And we refetch the tokenList once a day from token-api.
In our case; we can refresh the metadata once a day for all assets we have in state.
There was a problem hiding this comment.
Hey @danroc @ccharly @aganglada 👋 what do you guys think about this ☝️
The notification we will get from the notification system is notify:accountAssetListUpdated which as we've discussed in the past should tell us about the new acquired tokens. We could update all asset metadata once we get notified. However, if a user never gets new tokens for any reason, his asset metadata might stay stale. (outdated logo for exp)
We can make the MultichainAssetsController extends StaticIntervalPollingController and we refetch all asset metadata for all assets once a day. Should be enough. What do you guys think?
There was a problem hiding this comment.
I'd be okay with this approach and 👍 for the StaticIntervalPollingConttroller too. As long as the interval is "big enough", that should be ok.
packages/assets-controllers/src/MultichainAssetsController/MultichainAssetsController.ts
Outdated
Show resolved
Hide resolved
| export function parseCaipAssetType(asset: CaipAssetType): CaipChainId { | ||
| return asset.split('/')[0] as CaipChainId; | ||
| } |
There was a problem hiding this comment.
Oh sorry, this wasn't clear during my last review, but we do have this helper already 😅
I'd use this one (from @metamask/utils):
So I guess we could get rid of this one in favor of the other one?
| const tmpAssets: Record<CaipChainId, CaipAssetType[]> = {}; | ||
| for (const asset of assets) { | ||
| const chainId = parseCaipAssetType( | ||
| asset as `${string}:${string}/${string}:${string}`, |
There was a problem hiding this comment.
This type cast feels unnecessary? We're already using CaipAssetType at that point, no?
| const assetsWithoutMetadata: CaipAssetType[] = assets.filter( | ||
| (asset) => !this.state.metadata[asset], | ||
| ); | ||
|
|
||
| // call the snap to get the metadata | ||
| if (assetsWithoutMetadata.length > 0) { | ||
| // check if for every asset in assetsWithoutMetadata there is a snap in snaps by chainId else call getAssetSnaps | ||
| if ( | ||
| !assetsWithoutMetadata.every((asset: CaipAssetType) => { | ||
| const chainId = parseCaipAssetType( | ||
| asset as `${string}:${string}/${string}:${string}`, | ||
| ); | ||
| console.log('this.snaps', this.#snaps); | ||
| return this.#snaps[chainId as CaipChainId]?.length > 0; | ||
| }) | ||
| ) { | ||
| this.#snaps = this.#getAssetSnaps(); | ||
| } |
There was a problem hiding this comment.
WDYT about moving this logic in another method to avoid too much logic here?
Could be something like:
#refreshAssetsSnapsFor(asset: CaipAssetType[]) {
const assetsWithoutMetadata: CaipAssetType[] = assets.filter(
(asset) => !this.state.metadata[asset],
);
// call the snap to get the metadata
if (assetsWithoutMetadata.length > 0) {
// check if for every asset in assetsWithoutMetadata there is a snap in snaps by chainId else call getAssetSnaps
if (
!assetsWithoutMetadata.every((asset: CaipAssetType) => {
const chainId = parseCaipAssetType(
asset as `${string}:${string}/${string}:${string}`,
);
console.log('this.snaps', this.#snaps);
return this.#snaps[chainId as CaipChainId]?.length > 0;
})
) {
this.#snaps = this.#getAssetSnaps();
}
}This way we could re-use this method elsewhere if we want to refresh the Snaps list using any assets list?
| asset as `${string}:${string}/${string}:${string}`, | ||
| ); | ||
| console.log('this.snaps', this.#snaps); | ||
| return this.#snaps[chainId as CaipChainId]?.length > 0; |
There was a problem hiding this comment.
We're also having this pattern when using this notation:
const [snap] = this.#snaps[chainId as CaipChainId];So IMO, that would be great to have another method for this like:
#getAssetSnapFor(scope: CaipChainId): Snap {
const [snap] = this.#snaps[scope];
return snap; // Will be undefined if there's no Snaps candidate for this scope.
}And this would just become:
| return this.#snaps[chainId as CaipChainId]?.length > 0; | |
| return Boolean(this.#getAssetSnapFor(chainId)); |
Or using !== undefined if you prefer.
| } | ||
| } | ||
|
|
||
| async updateAssetsMetadata(assets: CaipAssetType[]) { |
| } | ||
|
|
||
| async updateAssetsMetadata(assets: CaipAssetType[]) { | ||
| const tmpAssets: Record<CaipChainId, CaipAssetType[]> = {}; |
There was a problem hiding this comment.
Just to be a bit more explicit. I would also rename tmpAssets to assetsByScope, WDYT?
| const tmpAssets: Record<CaipChainId, CaipAssetType[]> = {}; | |
| // Creates a mapping of scope to their respective assets list. | |
| const assetsByScope: Record<CaipChainId, CaipAssetType[]> = {}; |
| const assetsForChain = tmpAssets[chainId as CaipChainId]; | ||
| // Now fetch metadata from the associated asset Snaps: | ||
| // Pick only the first one, we ignore the other Snaps if there are multiple candidates for now. | ||
| const [snap] = this.#snaps[chainId as CaipChainId]; |
There was a problem hiding this comment.
We could re-use the new #getAssetSnapFor method (from my other comment)
Explanation
References
Changelog
@metamask/package-a@metamask/package-bChecklist