feat: handle shield change payment method #38165
Open
+1,270
−213
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Collaborator
✨ Files requiring CODEOWNER review ✨🧩 @MetaMask/extension-devs (6 files, +520 -22)
📜 @MetaMask/policy-reviewers (6 files, +520 -22)
Tip Follow the policy review process outlined in the LavaMoat Policy Review Process doc before expecting an approval from Policy Reviewers. 🔗 @MetaMask/supply-chain (6 files, +520 -22)
🔐 @MetaMask/web3auth (6 files, +518 -162)
|
Collaborator
Builds ready [39b482c]
UI Startup Metrics (1245 ± 117 ms)
📊 Page Load Benchmark ResultsCurrent Commit: 📄 Localhost MetaMask Test DappSamples: 100 Summary
📈 Detailed Results
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
Collaborator
Builds ready [6a2ab08]
UI Startup Metrics (1186 ± 99 ms)
📊 Page Load Benchmark ResultsCurrent Commit: 📄 Localhost MetaMask Test DappSamples: 100 Summary
📈 Detailed Results
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
Collaborator
Builds ready [0556dca]
UI Startup Metrics (1323 ± 132 ms)
📊 Page Load Benchmark ResultsCurrent Commit: 📄 Localhost MetaMask Test DappSamples: 100 Summary
📈 Detailed Results
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
<!-- 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** <!-- 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? --> [](https://codespaces.new/MetaMask/metamask-extension/pull/38287?quickstart=1) ## **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: ## **Related issues** Fixes: ## **Manual testing steps** 1. Go to this page... 2. 3. ## **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** - [ ] 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). - [ ] I've completed the PR template to the best of my ability - [ ] I’ve included tests if applicable - [ ] I’ve documented my code using [JSDoc](https://jsdoc.app/) format if applicable - [ ] 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] > Replace payment method row buttons with design-system TextButton, simplify crypto token display, and pass supported tokens to the modal; remove related legacy styles. > > - **UI (Transaction Shield settings)**: > - **Payment method row (`payment-method-row.tsx`)**: > - Migrate from `ButtonLink`/legacy components to `@metamask/design-system-react` `TextButton`/`Text`. > - Simplify crypto display: show token symbol directly; remove `Name`/address resolution. > - Update error/warning states to `TextButton` with icon and utility classes; remove explicit icon/color props. > - Open modal via `TextButton` when crypto is selected; remove separate click handler. > - Add `tokensSupported` to `ShieldPaymentModal`; keep card option gating via `canChangePaymentMethodToCard`. > - Adjust typography variants (`TextVariant.BodyMd`) and weights. > - **Styles (`index.scss`)**: > - Remove `.warning-button:hover` rule; rely on component-level styling. > > <sup>Written by [Cursor Bugbot](https://cursor.com/dashboard?tab=bugbot) for commit 52cf388. This will update automatically on new commits. Configure [here](https://cursor.com/dashboard?tab=bugbot).</sup> <!-- /CURSOR_SUMMARY -->
Contributor
|
CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes. |
tuna1207
added
area-shield
team-shield
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
Member
Author
|
@metamaskbot update-policies |
Collaborator
Builds ready [51d7c00]
📊 Page Load Benchmark ResultsCurrent Commit: 📄 Localhost MetaMask Test DappSamples: 100 Summary
📈 Detailed Results
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
Collaborator
|
Policies updated. 🧠 Learn how: https://lavamoat.github.io/guides/policy-diff/#what-to-look-for-when-reviewing-a-policy-diff |
Collaborator
Builds ready [c8a61ea]
UI Startup Metrics (1319 ± 122 ms)
📊 Page Load Benchmark ResultsCurrent Commit: 📄 Localhost MetaMask Test DappSamples: 100 Summary
📈 Detailed Results
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
tuna1207
commented
Nov 27, 2025
| "@metamask/ppom-validator>json-rpc-random-id": true | ||
| } | ||
| }, | ||
| "@metamask/subscription-controller>@metamask/transaction-controller>@metamask/eth-block-tracker": { |
Member
Author
There was a problem hiding this comment.
added in transaction-controller 62.3.0
@MetaMask/policy-reviewers
| "@metamask/ppom-validator>json-rpc-random-id": true | ||
| } | ||
| }, | ||
| "@metamask/subscription-controller>@metamask/transaction-controller>@metamask/eth-block-tracker": { |
Member
Author
There was a problem hiding this comment.
added in transaction-controller 62.3.0
@MetaMask/policy-reviewers
| "@metamask/ppom-validator>json-rpc-random-id": true | ||
| } | ||
| }, | ||
| "@metamask/subscription-controller>@metamask/transaction-controller>@metamask/eth-block-tracker": { |
Member
Author
There was a problem hiding this comment.
added in transaction-controller 62.3.0
@MetaMask/policy-reviewers
| "@metamask/ppom-validator>json-rpc-random-id": true | ||
| } | ||
| }, | ||
| "@metamask/subscription-controller>@metamask/transaction-controller>@metamask/eth-block-tracker": { |
Member
Author
There was a problem hiding this comment.
added in transaction-controller 62.3.0
@MetaMask/policy-reviewers
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Bug: Missing error handling for crypto payment method update
The hasApiError variable includes updateSubscriptionCardPaymentMethodResult.error but omits updateSubscriptionCryptoPaymentMethodResult.error. Meanwhile, the loading variable was correctly updated to include updateSubscriptionCryptoPaymentMethodResult.pending at line 482. This inconsistency means that if the crypto payment method update fails, the error won't be displayed to the user since hasApiError won't be true and the ApiErrorHandler component won't render.
ui/pages/settings/transaction-shield-tab/transaction-shield.tsx#L223-L230
| ( | ||
| displayedShieldSubscription.paymentMethod as SubscriptionCryptoPaymentMethod | ||
| ).crypto.tokenSymbol, | ||
| ); |
There was a problem hiding this comment.
Bug: Token filter excludes same-symbol tokens across all chains
The availableTokenBalancesWithoutCurrentToken filter at lines 120-126 only checks token.symbol when filtering out the current payment token, but ignores chainId. This removes ALL tokens with the same symbol across ALL chains. For example, if the current payment method uses USDC on Ethereum, the filter excludes USDC on Polygon and other chains too, preventing users from switching to the same token type on a different chain. The filter needs to check both symbol AND chainId to only exclude the exact current token.
There was a problem hiding this comment.
Bug: Missing error state for crypto payment method update
The updateSubscriptionCryptoPaymentMethodResult.error is not included in hasApiError, even though updateSubscriptionCryptoPaymentMethodResult.pending is correctly added to loading. This means when the crypto payment method update fails, the error won't be displayed to users via the ApiErrorHandler component. All other async operation results have their .error property included in hasApiError, making this an inconsistent omission that prevents users from seeing error feedback for failed crypto payment method changes.
ui/pages/settings/transaction-shield-tab/transaction-shield.tsx#L223-L230
ui/pages/settings/transaction-shield-tab/transaction-shield.tsx#L476-L482
Collaborator
Builds ready [aeb61c5]
UI Startup Metrics (1267 ± 126 ms)
📊 Page Load Benchmark ResultsCurrent Commit: 📄 Localhost MetaMask Test DappSamples: 100 Summary
📈 Detailed Results
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
| token.symbol !== cryptoPayment.crypto.tokenSymbol && | ||
| token.chainId.toLowerCase() !== | ||
| cryptoPayment.crypto.chainId.toLowerCase(), | ||
| ); |
There was a problem hiding this comment.
Bug: Filter logic incorrectly excludes valid alternative payment tokens
The filter condition uses && (AND) when it should use || (OR). The comment states the intent is to exclude only the current payment token, but the current logic excludes tokens where the symbol is different AND the chainId is different. This means valid alternatives like USDT on the same chain (different symbol, same chainId) or USDC on a different chain (same symbol, different chainId) are incorrectly filtered out. Only tokens that differ in both symbol AND chainId are kept, which is far too restrictive. The condition should be token.symbol !== ... || token.chainId.toLowerCase() !== ... to exclude only the exact current token.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
](https://codespaces.new/MetaMask/metamask-extension/pull/38287?quickstart=1){kind=link}
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
In transaction shield settings screen when user press on payment method, show payment modal for user to choose changing payment method if available
Handle change payment method for crypto -> crypto and crypto -> card
Figma: https://www.figma.com/design/HTAO1SrmixV4ppv7qIvLoa/Metamask-Transaction-Shield?node-id=13566-64615&t=aa5GS94Ns0xYXECP-0
Changelog
CHANGELOG entry: add change payment method from crypto -> crypto / crypto -> card (in paused state only)
Related issues
Fixes:
Manual testing steps
Screenshots/Recordings
Before
After
Screen.Recording.2025-11-27.at.17.20.55.mov
Pre-merge author checklist
Pre-merge reviewer checklist
Note
Adds modal-driven payment method switching for Transaction Shield (crypto→crypto and paused crypto→card), with supporting hooks/utilities, and updates dependencies/policies accordingly.
PaymentMethodRowwith modal to change payment method; supports token selection, paused/ending-soon states, and disables card option when ineligible.transaction-shield.tsx; replace prior display logic; wire up handlers.ShieldPaymentModalto acceptdisableCardOption.useUpdateSubscriptionCryptoPaymentMethodto persist selection and trigger approval flow.useSubscriptionCryptoApprovalTransactionnow derivesnetworkClientIdfromgetNetworkConfigurationsByChainId.getIsShieldSubscriptionCanChangePaymentMethodToCardhelper.@metamask/subscription-controllerto^5.1.0and update related packages (transaction-controller,network-controller,polling-controller,gas-fee-controller,json-rpc-engine, etc.); regenerateyarn.lock.subscription-controllerpaths,klonapath changes,deep-freeze-strict, updated JSON-RPC provider/middleware).Written by Cursor Bugbot for commit e40a7c0. This will update automatically on new commits. Configure here.