Repository navigation
feat(bitcoin): use shared fetch policy helpers - #530
Conversation
|
@metamaskbot publish-preview |
|
Preview builds have been published. Learn how to use preview builds in other projects. Expand for full list of packages and versions. |
There was a problem hiding this comment.
🔵 Needs a closer look
Broadcast retries and string-based error classification need human validation against the real BDK WASM client because the adapter tests mock that boundary.
No open issues 🎉
What changed in this PR
Adopts the shared fetch-policy helpers in the Bitcoin wallet snap, aligning its network handling with the Solana and Tron integrations.
Changes:
- Adds per-network Esplora policies and a price API policy, replacing BDK’s internal retries.
- Retries signed transaction broadcasts, recognizes duplicate submissions, and recreates scan requests for each attempt.
- Adds adapter tests, raises coverage thresholds, and configures the performance shim required by the shared policy.
| File | Description |
|---|---|
yarn.lock |
Records the Bitcoin snap’s webpack dependency. |
packages/bitcoin-wallet-snap/src/infra/PriceApiClientAdapter.ts |
Uses the shared fetch policy for price requests. |
packages/bitcoin-wallet-snap/src/infra/PriceApiClientAdapter.test.ts |
Tests retries, non-transient failures, and malformed JSON. |
packages/bitcoin-wallet-snap/src/infra/EsploraClientAdapter.ts |
Adds network policies, BDK error mapping, and duplicate broadcast handling. |
packages/bitcoin-wallet-snap/src/infra/EsploraClientAdapter.test.ts |
Tests retry behavior, request recreation, and broadcast handling. |
packages/bitcoin-wallet-snap/src/index.ts |
Passes the logger to both network adapters. |
packages/bitcoin-wallet-snap/src/entities/config.ts |
Removes the obsolete retry configuration field. |
packages/bitcoin-wallet-snap/src/config.ts |
Removes the retry setting from validation and defaults. |
packages/bitcoin-wallet-snap/src/config.test.ts |
Updates configuration fixtures and expectations. |
packages/bitcoin-wallet-snap/snap.config.ts |
Provides the performance shim through webpack. |
packages/bitcoin-wallet-snap/package.json |
Pins webpack as a development dependency. |
packages/bitcoin-wallet-snap/jest.config.js |
Raises coverage thresholds. |
packages/bitcoin-wallet-snap/CHANGELOG.md |
Documents retries and duplicate broadcast handling. |
eslint-suppressions.json |
Removes suppressions now handled in the snap configuration. |
Review effort: Balanced
…ts on retry without a null-pointer error
|
There was a problem hiding this comment.
Completely unrelated remark but eventually I would love to see us use objects to pass parameters to constructors so we have a better way to handle optional ones
…aMask#47621) <!-- Please submit this PR as a draft initially. Do not mark it as "Ready for review" until the template has been completely filled out, and PR status checks have passed at least once. --> ## **Description** # Bitcoin [3.3.0] ### Changed - Retry transient Bitcoin indexer, price API, and signed transaction broadcast failures with the shared fetch policy, and treat duplicate broadcasts as successful. ([MetaMask#530](MetaMask/internal-snaps#530)) ### Fixed - Fix formatting of small Bitcoin balances to avoid scientific notation. ([MetaMask#543](MetaMask/internal-snaps#543)) # Tron [4.3.1] ### Fixed - Allow transaction confirmation after remote simulation failures while retaining security warnings and checks for locally invalid transactions. ([MetaMask#552](MetaMask/internal-snaps#552)) <!-- Write a short description of the changes included in this pull request, also include relevant motivation and context. Have in mind the following questions: 1. What is the reason for the change? 2. What is the improvement/solution? --> ## **Changelog** <!-- If this PR is not End-User-Facing and should not show up in the CHANGELOG, you can choose to either: 1. Write `CHANGELOG entry: null` 2. Label with `no-changelog` If this PR is End-User-Facing, please write a short User-Facing description in the past tense like: `CHANGELOG entry: Added a new tab for users to see their NFTs` `CHANGELOG entry: Fixed a bug that was causing some NFTs to flicker` (This helps the Release Engineer do their job more quickly and accurately) --> CHANGELOG entry: null ## **Related issues** n/a ## **Manual testing steps** n/a ## **Screenshots/Recordings** <!-- If applicable, add screenshots and/or recordings to visualize the before and after of your change. --> ### **Before** <!-- [screenshots/recordings] --> ### **After** <!-- [screenshots/recordings] --> ## **Pre-merge author checklist** - [X] I've followed [MetaMask Contributor Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask Extension Coding Standards](https://github.com/MetaMask/metamask-extension/blob/main/.github/guidelines/CODING_GUIDELINES.md). - [X] I've completed the PR template to the best of my ability - [X] I’ve included tests if applicable - [X] I’ve documented my code using [JSDoc](https://jsdoc.app/) format if applicable - [X] I’ve applied the right labels on the PR (see [labeling guidelines](https://github.com/MetaMask/metamask-extension/blob/main/.github/guidelines/LABELING_GUIDELINES.md)). Not required for external contributors. ## **Pre-merge reviewer checklist** - [ ] I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed). - [ ] I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots. <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Medium Risk** > Patch bumps change Bitcoin broadcast/retry behavior and Tron confirmation when remote simulation fails, both on transaction-critical paths. > > **Overview** > Bumps **`@metamask/bitcoin-wallet-snap`** to **3.3.0** and **`@metamask/tron-wallet-snap`** to **4.3.1** in `package.json`, with matching **`yarn.lock`** resolutions. No extension source changes—behavior comes from the new snap builds. > > With **Bitcoin 3.3.0**, users get more resilient indexer/price/broadcast calls (shared fetch retries, duplicate broadcasts treated as success) and corrected display of very small balances (no scientific notation). > > With **Tron 4.3.1**, confirmation can proceed when **remote** simulation fails, while locally invalid transactions still block approval and security warnings remain. > > <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit 4bd1b01. Bugbot is set up for automated code reviews on this repo. Configure [here](https://www.cursor.com/dashboard/bugbot).</sup> <!-- /CURSOR_SUMMARY -->
## Explanation # Bitcoin [3.3.0] ### Changed - Retry transient Bitcoin indexer, price API, and signed transaction broadcast failures with the shared fetch policy, and treat duplicate broadcasts as successful. ([MetaMask#530](MetaMask#530)) ### Fixed - Fix formatting of small Bitcoin balances to avoid scientific notation. ([MetaMask#543](MetaMask#543)) # Tron [4.3.1] ### Fixed - Allow transaction confirmation after remote simulation failures while retaining security warnings and checks for locally invalid transactions. ([MetaMask#552](MetaMask#552)) <!-- Thanks for your contribution! Take a moment to answer these questions so that reviewers have the information they need to properly understand your changes: * What is the current state of things and why does it need to change? * What is the solution your changes offer and how does it work? * Are there any changes whose purpose might not obvious to those unfamiliar with the domain? * If your primary goal was to update one package but you found you had to update another one along the way, why did you do so? * If you had to upgrade a dependency, why did you do so? --> ## References <!-- Are there any issues that this pull request is tied to? Are there other links that reviewers should consult to understand these changes better? Are there client or consumer pull requests to adopt any breaking changes? For example: * Fixes #12345 * Related to #67890 --> ## 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](https://github.com/MetaMask/internal-snaps/tree/main/docs/processes/updating-changelogs.md) - [ ] I've introduced [breaking changes](https://github.com/MetaMask/internal-snaps/tree/main/docs/processes/breaking-changes.md) in this PR and have prepared draft pull requests for clients and consumer packages to resolve them
…taMask#39134) <!-- Please submit this PR as a draft initially. Do not mark it as "Ready for review" until this PR meets the canonical Definition of Ready For Review in `docs/readme/ready-for-review.md`. --> ## **Description** Bumps the Bitcoin, Solana, Stellar, and Tron wallet snaps to their latest published versions: - `@metamask/bitcoin-wallet-snap`: `^3.2.0` → `^3.3.0` - `@metamask/solana-wallet-snap`: `^7.2.0` → `^7.3.0` - `@metamask/stellar-wallet-snap`: `^2.0.0` → `^2.1.0` - `@metamask/tron-wallet-snap`: `^4.3.0` → `^4.4.0` All are backwards-compatible minor updates; no application code changes are required. ### @metamask/bitcoin-wallet-snap 3.3.0 - Retry transient Bitcoin indexer, price API, and signed transaction broadcast failures with the shared fetch policy, and treat duplicate broadcasts as successful ([MetaMask#530](MetaMask/internal-snaps#530)). - Fix formatting of small Bitcoin balances to avoid scientific notation ([MetaMask#543](MetaMask/internal-snaps#543)). ### @metamask/solana-wallet-snap 7.3.0 - Remove the temporary fallback that injected `https://metamask.io` as the request origin when `keyring_submitRequest` was called without one ([MetaMask#533](MetaMask/internal-snaps#533)). ### @metamask/stellar-wallet-snap 2.1.0 - `confirmSend` now throws when signing or submission fails after the user confirms, instead of returning a JSON error response ([MetaMask#550](MetaMask/internal-snaps#550)). - Bump `@stellar/stellar-sdk` from `^17.2.1` to `^17.4.0` ([MetaMask#545](MetaMask/internal-snaps#545)). - Retry transient Token API, Price API, Security Alerts API, RPC, and Horizon failures with bounded timeouts and circuit breaking ([MetaMask#534](MetaMask/internal-snaps#534), [MetaMask#551](MetaMask/internal-snaps#551)). - Fix `onAmountInput` to accept the optional destination as `toAddress` instead of `to` ([MetaMask#558](MetaMask/internal-snaps#558)). ### @metamask/tron-wallet-snap 4.4.0 - Support `VoteWitnessContract` and `AccountPermissionUpdateContract` transactions by upgrading TronWeb to 6.5.1 ([MetaMask#561](MetaMask/internal-snaps#561)). - Align transaction security warning copy and scan-loading behavior with user preferences ([MetaMask#557](MetaMask/internal-snaps#557)). ## **Changelog** CHANGELOG entry: Updated the Solana, Stellar, and Tron wallet snaps to their latest versions. ## **Related issues** Fixes: N/A ## **Manual testing steps** ```gherkin Feature: Bitcoin and non-EVM wallet flows Scenario: user views account assets Given the user has Bitcoin, Solana, Stellar, and Tron accounts with assets When user opens the wallet and switches between those networks Then the assets and balances load successfully Scenario: user sends funds on each network Given the user has funds on Bitcoin, Solana, Stellar, and Tron When user sends funds on each network Then the fee estimate and confirmation are displayed correctly And the transaction is broadcast and completes successfully ``` ## **Screenshots/Recordings** ### **Before** N/A ### **After** N/A ## **Pre-merge author checklist** - [x] I've followed [MetaMask Contributor Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask Mobile Coding Standards](https://github.com/MetaMask/metamask-mobile/blob/main/.github/guidelines/CODING_GUIDELINES.md). - [x] I've completed the PR template to the best of my ability - [x] I've included tests if applicable - [x] I've documented my code using [JSDoc](https://jsdoc.app/) format if applicable - [x] I've applied the right labels on the PR (see [labeling guidelines](https://github.com/MetaMask/metamask-mobile/blob/main/.github/guidelines/LABELING_GUIDELINES.md)). Not required for external contributors. #### Performance checks (if applicable) - [x] I've tested on Android - Ideally on a mid-range device; emulator is acceptable - [x] I've tested with a power user scenario - Use these [power-user SRPs](https://consensyssoftware.atlassian.net/wiki/spaces/TL1/pages/edit-v2/401401446401?draftShareId=9d77e1e1-4bdc-4be1-9ebb-ccd916988d93) to import wallets with many accounts and tokens - [x] I've instrumented key operations with Sentry traces for production performance metrics - See [`trace()`](/app/util/trace.ts) for usage and [`addToken`](/app/components/Views/AddAsset/components/AddCustomToken/AddCustomToken.tsx#L274) for an example For performance guidelines and tooling, see the [Performance Guide](https://consensyssoftware.atlassian.net/wiki/spaces/TL1/pages/400085549067/Performance+Guide+for+Engineers). ## **Pre-merge reviewer checklist** - [ ] I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed). - [ ] I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots.



Explanation
Applies the shared retry and circuit-breaker fetch policy (
createFetchPolicy,executeWithFetchPolicy, andfetchWithPolicyfrom@metamask/snap-networks-utils) to@metamask/bitcoin-wallet-snap, following #472, #478, and #492.Changes
EsploraClientAdapter):FetchPolicyper Bitcoin network (esplora_bitcoin,esplora_testnet,esplora_testnet4,esplora_signet,esplora_regtest) so an outage on one network cannot open the circuit breaker for another.EsploraClientretry loop (maxRetries = 0) and removes the obsoletechain.maxRetriesconfig so retries are not stacked on top of the shared policy.fullScan,sync, andgetFeeEstimatesthroughexecuteWithFetchPolicy, recreatingaccount.startFullScan()andaccount.startSync()inside each attempt (since the WASM client consumes the request pointer per call) and applying the walletUpdateonly once after the policy succeeds.getTransactionSenders(GET /tx/:txid) throughfetchWithPolicy, keeping JSON parsing outside the retry boundary and preserving failed-lookup cache eviction.toFetchPolicyErrormapper that extracts HTTP status codes fromHttpResponse { status: ... }and maps transportReqwest(...)errors toNetworkError, while letting response decode (kind: Decode), parsing, and wallet-state errors pass through without retrying.PriceApiClientAdapter):spotPricesthroughfetchWithPolicywith aprice_apipolicy and parses JSON outside the policy while preserving theExternalServiceErrorboundary.EsploraClientAdapter.broadcast):txid([sha256d(serialized_tx_without_witness)]). BecauseEsploraClient.broadcast(transaction)borrows the already-signedTransactionby reference without rebuilding or re-signing it, every retry submits the exact same transaction bytes andtxid. Re-broadcasting cannot create a second payment or double-spend; nodes treat it as the same transaction.Transaction already in block chain,already in mempool,txn-already-in-mempool, ortxn-already-known).isAlreadyBroadcastErrortreats those responses as a successful broadcast.@metamask/snap-networks-utils/performance-shimglobal via webpack'sProvidePlugininsnap.config.tsand declareswebpack(5.107.2, matching@metamask/snaps-cli) as a dev dependency.References
Checklist