Skip to content

fix(pay): always emit mm_pay_payment_token_list_size on confirmation events - #33253

Merged
jpuri merged 7 commits into
mainfrom
fix/pay-token-list-size-always-emit
Jul 27, 2026
Merged

fix(pay): always emit mm_pay_payment_token_list_size on confirmation events#33253
jpuri merged 7 commits into
mainfrom
fix/pay-token-list-size-always-emit

Conversation

@jpuri

@jpuri jpuri commented Jul 14, 2026

Copy link
Copy Markdown
Contributor

Description

  1. Move mm_pay_payment_token_list_size outside the if(payToken) guard so it is written regardless of whether a payment token was selected.
  2. Fix: mm_pay_payment_method_selected missing on 18.5% of Transaction Finalized events for money_account_deposit

Changelog

CHANGELOG entry:

Related issues

Fixes: https://consensyssoftware.atlassian.net/browse/CONF-1636
Fixes: https://consensyssoftware.atlassian.net/browse/CONF-1634

Manual testing steps

NA

Screenshots/Recordings

NA

Pre-merge author checklist

Performance checks (if applicable)

  • I've tested on Android
    • Ideally on a mid-range device; emulator is acceptable
  • I've tested with a power user scenario
    • Use these power-user SRPs to import wallets with many accounts and tokens
  • I've instrumented key operations with Sentry traces for production performance metrics

For performance guidelines and tooling, see the Performance Guide.

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.

Note

Low Risk
Changes are limited to analytics property assembly and UI metric hooks; no payment execution or auth paths are modified.

Overview
MetaMask Pay confirmation and finalized-transaction analytics are tightened so key properties are present even when the user has not picked a pay token or when controller metadata is incomplete.

On the confirmation UI hook, mm_pay_payment_token_list_size is now set whenever payment-method metrics are updated (alongside mm_pay_payment_method_available), instead of only when a pay token is already selected. Tests assert list sizes of 5 and 0 in the scenarios that previously omitted the field.

In addPayTypeProperties (transaction-controller metrics), baseline mm_pay, mm_pay_payment_method_selected, and mm_pay_use_case are derived for recognized Pay transaction types even when metamaskPay lacks both chainId and tokenAddress, with optional mm_pay_chain_selected / mm_pay_token_selected from controller state or token lookup. Non–Pay-type transactions still emit no Pay properties unless full metamaskPay metadata is present. This addresses missing mm_pay_payment_method_selected on a slice of Transaction Finalized events (e.g. money_account_deposit).

Reviewed by Cursor Bugbot for commit 1ef0205. Bugbot is set up for automated code reviews on this repo. Configure here.

…events

Move mm_pay_payment_token_list_size outside the if(payToken) guard so
it is written regardless of whether a payment token was selected.

Previously the property was only set when payToken was truthy, causing
it to be absent from Transaction Rejected (and other outcome events)
when no eligible ERC-20 tokens existed. Mixpanel could not distinguish
'user had zero tokens' from 'property was never written'.

Now the property is always present with the actual count of available
(non-disabled) tokens, defaulting to 0 when none are eligible.
@jpuri
jpuri requested a review from a team as a code owner July 14, 2026 10:58
@jpuri jpuri added team-confirmations Push issues to confirmations team no-changelog no-changelog Indicates no external facing user changes, therefore no changelog documentation needed labels Jul 14, 2026
@github-actions

Copy link
Copy Markdown
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.

@github-actions github-actions Bot added size-XS risk:low AI analysis: low risk labels Jul 14, 2026
…ns with incomplete metamaskPay

When a money_account_deposit (or other PAY_TYPE) transaction fails
before the pay flow populates metamaskPay with chainId/tokenAddress,
addPayTypeProperties returned early without setting any mm_pay_*
properties. This caused mm_pay_payment_method_selected to be absent
from 18.5% of Transaction Finalized events for money_account_deposit.

Add addPayTypeBaselineProperties fallback for PAY_TYPE transactions
when metamaskPay is incomplete. Derives what it can from the
transaction type and TransactionPayController state:
- mm_pay, mm_pay_use_case, mm_pay_payment_method_selected (always)
- mm_pay_chain_selected (when metamaskPay.chainId exists)
- mm_pay_token_selected (when controller has paymentToken)
- fiat method override (when controller has fiatPayment data)

Non-PAY_TYPE transactions without metamaskPay still return empty
properties (unchanged).
@github-actions github-actions Bot added size-M and removed size-XS labels Jul 14, 2026
@github-actions github-actions Bot added risk:medium AI analysis: medium risk and removed risk:low AI analysis: low risk labels Jul 14, 2026
@github-actions

github-actions Bot commented Jul 14, 2026

Copy link
Copy Markdown
Contributor

🧪 Flaky unit test detection

Run history flaky detection

View recent run history

Historical failure rate is a hint, not proof — review each suggestion in context. See the flaky-test-detection skill for the full pattern reference and manual audit workflow.

Failures / runs sampled per window:

File 7d 15d 30d
app/components/Views/confirmations/hooks/pay/useTransactionPayMetrics.test.ts 0/146 0/197 0/305

AI-detected flaky patterns

app/components/Views/confirmations/hooks/pay/useTransactionPayMetrics.test.ts

  • J6 — Arbitrary setTimeout/sleep used as a synchronization barrier (high)
    • This test (and ~20 similar tests in the file) relies on await act(async () => noop()) immediately after runHook() (which invokes renderHookWithProvider) to act as a synchronization barrier. This allows time for the hook's internal useEffect (which calls updateConfirmationMetric) to execute before the assertion. This is exactly the zero-delay flush anti-pattern highlighted in J6 (see skill: even setTimeout(resolve, 0) is non-deterministic under CI load). The beforeEach correctly uses jest.resetAllMocks(), no waitFor is present, no fake timers, no module-level lets, and no spyOn without restore. The second test file uses proper mocking of Date.now with restoreAllMocks in afterEach and has no J1-J10 violations. Historical data shows zero failures for both files so hint not used. This pattern risks intermittent CI failures when effect timing varies.
    • Suggested fix in app/components/Views/confirmations/hooks/pay/useTransactionPayMetrics.test.ts:
      -    runHook();
      -
      -    await act(async () => noop());
      -
      -    expect(updateConfirmationMetricMock).toHaveBeenCalledWith({
      -      id: transactionIdMock,
      -      params: {
      -        properties: {
      -          mm_pay_payment_method_available: ['crypto'],
      -          mm_pay_payment_token_list_size: 5,
      -        },
      -        sensitiveProperties: {},
      -      },
      -    });
      -  });
      -
      +    runHook();
      +
      +    await waitFor(() => {
      +      expect(updateConfirmationMetricMock).toHaveBeenCalledWith({
      +        id: transactionIdMock,
      +        params: {
      +          properties: {
      +            mm_pay_payment_method_available: ['crypto'],
      +            mm_pay_payment_token_list_size: 5,
      +          },
      +          sensitiveProperties: {},
      +        },
      +      });
      +    });
      +  });
      +

This check is informational only and does not block merging.

@matthewwalsh0
matthewwalsh0 self-requested a review July 16, 2026 14:54
* selection). Derives what it can from the transaction type and
* TransactionPayController state without requiring chainId/tokenAddress.
*/
function addPayTypeBaselineProperties(

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Rather than duplicating lots of identical logic, could we support this inline above by removing the early return?

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.

PR is updated to address this.

@cursor cursor Bot 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.

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit f5e3701. Configure here.

@jpuri
jpuri requested a review from matthewwalsh0 July 22, 2026 06:15
@jpuri
jpuri enabled auto-merge July 22, 2026 10:17
@github-actions

Copy link
Copy Markdown
Contributor

🔍 Smart E2E Test Selection

  • Selected E2E tags: SmokeConfirmations, SmokePerps, SmokePredictions, SmokeMoney
  • Selected Performance tags: None (no tests recommended)
  • Risk Level: low
  • AI Confidence: 82%
click to see 🤖 AI reasoning details

E2E Test Selection:
The changes are confined to analytics/metrics tracking for MetaMask Pay flows:

  1. metamask-pay.ts: Fixes metrics property collection logic for PAY_TYPE transactions. Key changes:

    • Early return guard if mm_pay already set (prevents duplicate processing)
    • PAY_TYPE transactions (perpsDeposit, predictWithdraw, moneyAccountDeposit, moneyAccountWithdraw) now get baseline metrics even without metamaskPay.chainId/tokenAddress
    • Null-safe state access
    • Conditional property assignment for mm_pay_chain_selected and mm_pay_token_selected
  2. useTransactionPayMetrics.ts: Moves mm_pay_payment_token_list_size outside a conditional block so it's always included in analytics events regardless of payment method state.

These are metrics-only changes with no impact on transaction execution, UI rendering, or user-facing behavior. However, they affect the analytics events fired during:

  • Transaction confirmations (SmokeConfirmations) - the core confirmation flow
  • Perps deposit flows (SmokePerps)
  • Predictions deposit/withdraw flows (SmokePredictions)
  • Money account deposit/withdraw flows (SmokeMoney)

Per tag descriptions: SmokePerps, SmokePredictions, and SmokeMoney all require SmokeConfirmations (on-chain transactions). SmokePerps also requires SmokeWalletPlatform (Trending section). SmokePredictions also requires SmokeWalletPlatform.

Risk is LOW because:

  • No functional logic changes, only metrics collection
  • Changes are additive (more data collected, not less)
  • Unit tests updated to verify new behavior
  • No shared components (TabBar, navigation, modals) affected

Performance Test Selection:
The changes are purely analytics/metrics tracking modifications with no impact on rendering performance, asset loading, app launch, or any performance-sensitive flows. No performance test tags are warranted.

View GitHub Actions results

@sonarqubecloud

Copy link
Copy Markdown

@jpuri
jpuri added this pull request to the merge queue Jul 27, 2026
@github-merge-queue
github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Jul 27, 2026
@jpuri
jpuri added this pull request to the merge queue Jul 27, 2026
Merged via the queue into main with commit 957cfab Jul 27, 2026
194 checks passed
@jpuri
jpuri deleted the fix/pay-token-list-size-always-emit branch July 27, 2026 15:23
@github-actions github-actions Bot locked and limited conversation to collaborators Jul 27, 2026
@metamask-ci metamask-ci Bot added the release-8.6.0 Issue or pull request that will be included in release 8.6.0 label Jul 27, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

no-changelog no-changelog Indicates no external facing user changes, therefore no changelog documentation needed release-8.6.0 Issue or pull request that will be included in release 8.6.0 risk:medium AI analysis: medium risk size-M team-confirmations Push issues to confirmations team

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants