Skip to content

feat(bitcoin-wallet-snap)!: remove assets handlers - #260

Merged
ulissesferreira merged 1 commit into
mainfrom
WPN-2012-remove-asset-handlers
Sep 4, 2026
Merged

feat(bitcoin-wallet-snap)!: remove assets handlers#260
ulissesferreira merged 1 commit into
mainfrom
WPN-2012-remove-asset-handlers

Conversation

@ulissesferreira

@ulissesferreira ulissesferreira commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

Closes WPN-2012

Why

These onAssetsLookup / onAssetsConversion / onAssetHistoricalPrice / onAssetsMarketData entry points exist so a snap can serve asset metadata and price/conversion data to the client. For Bitcoin that responsibility already lives in the client: MultichainAssetsController and MultichainAssetsRatesController source bip122 assets and fiat rates directly, so nothing calls the snap's asset handlers. They are dead code that still costs us an endowment:assets permission, a cache layer (InMemoryCache), and a price-API code path to maintain. Removing them reduces the snap's permission surface and bundle size with no change in behaviour.

Summary

  • Removes the onAssetsLookup, onAssetsConversion, onAssetHistoricalPrice, and onAssetsMarketData entry points from bitcoin-wallet-snap
  • Removes now-unused code connected to them: AssetsHandler, AssetsUseCases, InMemoryCache, ICache, and the conversionsExpirationInterval config
  • Removes the now-unused endowment:assets permission from the manifest

Breaking change

The asset handler entry points are no longer exported by the snap.

@ulissesferreira
ulissesferreira requested a review from a team as a code owner September 2, 2026 17:10
@ulissesferreira
ulissesferreira force-pushed the WPN-2012-remove-asset-handlers branch from bd45f3d to 97c3041 Compare September 2, 2026 17:31
@ulissesferreira
ulissesferreira force-pushed the WPN-2012-remove-asset-handlers branch 2 times, most recently from 0e4146b to af44f36 Compare September 4, 2026 09:01
Remove the onAssetsLookup, onAssetsConversion, onAssetHistoricalPrice,
and onAssetsMarketData entry points, along with the now-unused
AssetsHandler, AssetsUseCases, InMemoryCache, ICache, and the
endowment:assets permission.

Closes WPN-2012
@ulissesferreira
ulissesferreira force-pushed the WPN-2012-remove-asset-handlers branch from af44f36 to e432d5a Compare September 4, 2026 09:11
@sonarqubecloud

sonarqubecloud Bot commented Sep 4, 2026

Copy link
Copy Markdown

@ulissesferreira ulissesferreira changed the title feat(bitcoin-wallet-snap)!: remove asset handler entry points feat(bitcoin-wallet-snap)!: remove assets handlers Sep 4, 2026
@Battambang
Battambang requested a balanced review from Copilot September 4, 2026 12:07

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Changes recommended

Cleanup is incomplete because an unused dependency and obsolete lint suppression remain, and the changelog marker violates repository convention.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Removes the Bitcoin Wallet Snap’s asset API entry points and associated implementation.

Changes:

  • Removes four asset handlers and the endowment:assets permission.
  • Deletes related use cases, caching, metadata, and historical pricing code.
  • Updates coverage thresholds and the changelog.
File summaries
File Description
src/use-cases/index.ts Removes asset use-case export.
src/use-cases/AssetsUseCases.ts Deletes asset pricing use cases.
src/use-cases/AssetsUseCases.test.ts Deletes related tests.
src/store/InMemoryCache.ts Deletes the unused cache.
src/store/InMemoryCache.test.ts Deletes cache tests.
src/store/ICache.ts Deletes cache interfaces.
src/infra/PriceApiClientAdapter.ts Removes historical pricing support.
src/index.ts Removes asset handler initialization and exports.
src/handlers/mappings.ts Removes an unused account mapping.
src/handlers/index.ts Removes the asset handler export.
src/handlers/icons.ts Deletes asset metadata icons.
src/handlers/AssetsHandler.ts Deletes asset entry-point handling.
src/handlers/AssetsHandler.test.ts Deletes handler tests.
src/entities/rates.ts Removes historical pricing types.
src/entities/config.ts Removes conversion-expiration configuration.
src/entities/account.ts Removes an unused reverse mapping.
src/config.ts Removes the conversion-expiration value.
snap.manifest.json Removes the assets permission and updates the hash.
jest.config.mjs Updates coverage thresholds.
CHANGELOG.md Documents the breaking removal.
Review details

Suppressed comments (2)

packages/bitcoin-wallet-snap/src/use-cases/AssetsUseCases.ts:1

  • Removing this file eliminates the only source import of @metamask/slip44, but the package still declares it in packages/bitcoin-wallet-snap/package.json:56 (and its workspace lock entry remains). Remove the dependency with the workspace Yarn command so the package metadata and lockfile are updated together.
    packages/bitcoin-wallet-snap/src/store/InMemoryCache.test.ts:1
  • Deleting this test leaves its @typescript-eslint/no-explicit-any suppression behind in eslint-suppressions.json:118-122. Remove that obsolete entry so the suppression inventory no longer references a nonexistent file.
  • Files reviewed: 20/20 changed files
  • Comments generated: 1
  • Review effort level: Balanced

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.


### Removed

- **BREAKING** Remove the `onAssetsLookup`, `onAssetsConversion`, `onAssetHistoricalPrice`, and `onAssetsMarketData` asset handler entry points, along with the now-unused `AssetsHandler`, `AssetsUseCases`, `InMemoryCache`, `ICache`, and the `endowment:assets` permission ([#260](https://github.com/MetaMask/internal-snaps/pull/260))

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

What a catch. Well there are some inconsistencies where sometimes there is the colon and sometimes not but yes the convention prefers to have the colon.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

100%! I will fix it at release time

@Battambang

Copy link
Copy Markdown
Contributor

I have updated the description with the WHY part, feel to edit it if not coherent.

@ulissesferreira
ulissesferreira added this pull request to the merge queue Sep 4, 2026
Merged via the queue into main with commit 0a2e37b Sep 4, 2026
56 checks passed
@ulissesferreira
ulissesferreira deleted the WPN-2012-remove-asset-handlers branch September 4, 2026 12:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants