Skip to content

Conversation

danroc
Copy link
Contributor

@danroc danroc commented Oct 17, 2025

Explanation

Currently, OnAssetConversion and OnAssetMarketData requests are batched per account, which can lead to duplicate requests when an asset is present in multiple accounts.

This PR resolves this issue by batching the requests per Snap for all assets managed by each Snap.

References

Checklist

  • I've updated the test suite for new or updated code as appropriate
  • I've updated documentation (JSDoc, Markdown, etc.) for new or updated code as appropriate
  • I've communicated my changes to consumers by updating changelogs for packages I've changed, highlighting breaking changes as necessary
  • I've prepared draft pull requests for clients and consumer packages to resolve any breaking changes

Note

Batch OnAssetsConversion and OnAssetsMarketData per Snap across accounts with deduplicated assets; refactor controller, update tests, and note in changelog.

  • MultichainAssetsRatesController:
    • Batch requests per Snap by aggregating assets across non‑EVM accounts via #addAssetsToSnapIdMap, deduplicating assets before calling Snaps.
    • New helpers to fetch data: #getConversionRates, #getMarketData, and consolidated #getUpdatedRatesFor combining both; added #getCaipCurrentCurrency.
    • Refactor #handleSnapRequest with typed overloads; state merge via #applyUpdatedRates now keyed by CaipAssetType.
    • updateAssetsRates and #updateAssetsRatesForNewAssets now build a Snap‑ID→assets map and issue a single batched call per Snap.
  • Tests:
    • Update to reflect batched behavior and CAIP currency keys (swift:0/iso4217:USD); add multi‑Snap scenarios and revised spies/mocks.
  • Changelog:
    • Note batching of OnAssetConversion and OnAssetsMarketData requests to non‑EVM account Snaps.

Written by Cursor Bugbot for commit fd1be26. This will update automatically on new commits. Configure here.

@danroc danroc force-pushed the dr/batch-rate-conversion branch from a706278 to 468843a Compare October 17, 2025 01:09
@danroc danroc marked this pull request as ready for review October 17, 2025 01:16
@danroc danroc requested review from a team as code owners October 17, 2025 01:16
cursor[bot]

This comment was marked as outdated.

@danroc
Copy link
Contributor Author

danroc commented Oct 17, 2025

@metamaskbot publish-preview

@danroc danroc self-assigned this Oct 17, 2025
@danroc danroc added the area-performance Issues relating to slowness of app, cpu usage, and/or blank screens. label Oct 17, 2025
// Apply these updated rates to controller state
this.#applyUpdatedRates(rates);
for (const asset of this.#getAssetsForAccount(account.id)) {
assetToSnapID.set(asset, snapId as SnapId);
Copy link
Member

Choose a reason for hiding this comment

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

Maybe simpler to construct the Snap ID -> Asset List mapping here instead of constructing this mapping and then constructing the reverse mapping in the next function call?

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 in two steps mainly because it also dedupes the assets. If I build the snapId -> assets[] mapping here I need to do another pass to remove the deplicates of each entry.

Copy link
Member

@FrederikBolding FrederikBolding Oct 17, 2025

Choose a reason for hiding this comment

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

We could use a Snap ID -> Asset Set for that perhaps? Up to you!

cursor[bot]

This comment was marked as outdated.

Comment on lines -340 to -342
if (assets?.length === 0) {
continue;
}
Copy link
Member

@FrederikBolding FrederikBolding Oct 17, 2025

Choose a reason for hiding this comment

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

Should we keep this? So we don't create sets for Snaps that don't have any assets

Copy link
Contributor Author

@danroc danroc Oct 17, 2025

Choose a reason for hiding this comment

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

Indeed, I added some checks to prevent adding empty sets to the map, or calling the Snap when the list of assets is empty.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area-performance Issues relating to slowness of app, cpu usage, and/or blank screens. team-assets

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants