Skip to content

feat: dismiss Ledger prompt after deposit signing (CONF-1715) - #34872

Draft
dan437 wants to merge 6 commits into
mainfrom
conf-1715-ledger-modal-lifecycle
Draft

feat: dismiss Ledger prompt after deposit signing (CONF-1715)#34872
dan437 wants to merge 6 commits into
mainfrom
conf-1715-ledger-modal-lifecycle

Conversation

@dan437

@dan437 dan437 commented Aug 17, 2026

Copy link
Copy Markdown
Contributor

Description

Closes the Confirm on Ledger sheet and navigates to Money home after all required Money Account deposit transactions are signed. Multi-transaction routes stay on the device prompt until the final signature.

The flow uses the core-owned requiredTransactionCount value. Ledger rejection is treated as cancellation, avoiding duplicate missing-approval warnings and developer alerts.

Depends on MetaMask/core#9897 and its package release.

Changelog

CHANGELOG entry: Changed Ledger-funded Money Account deposits to finish after the final signature and handle rejection silently

Related issues

Refs: CONF-1715

Manual testing steps

Feature: Ledger-funded Money Account deposit

  Scenario: finish after the final signature
    Given Money Account hardware deposits are enabled
    And a Ledger account is selected as the payer
    When the user confirms every required transaction on the Ledger
    Then the Confirm on Ledger sheet closes
    And the app navigates to Money home

  Scenario: keep the prompt open for multiple signatures
    Given the deposit route requires an approval and a deposit transaction
    When the user confirms only the approval on the Ledger
    Then the Confirm on Ledger sheet remains visible
    When the user confirms the deposit transaction
    Then the sheet closes
    And the app navigates to Money home

  Scenario: reject on Ledger
    Given the Confirm on Ledger sheet is visible
    When the user rejects the transaction on the Ledger
    Then the sheet closes without a missing-approval warning or developer alert

Screenshots/Recordings

N/A — requires physical Ledger validation after the core dependency is published.

Before

N/A

After

N/A

Pre-merge author checklist

  • Contributor and Mobile coding standards were considered.
  • The PR template is complete for the current draft state.
  • Focused tests cover the updated lifecycle and rejection behavior.
  • Documentation needs were considered.
  • Labels were considered and maintained by repository automation.

Performance checks (if applicable)

  • Android impact considered; physical validation is pending.
  • Power-user impact considered; multi-transaction routes are included.
  • Sentry tracing considered; no new measured operation is added.

Pre-merge reviewer checklist

  • I've manually tested the PR.
  • I confirm that this PR addresses all acceptance criteria and includes the needed test evidence.

@dan437 dan437 self-assigned this Aug 17, 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.

@metamask-ci metamask-ci Bot added the team-confirmations Push issues to confirmations team label Aug 17, 2026
@github-actions

Copy link
Copy Markdown
Contributor

⚡ Performance Test Results

ℹ️ Performance test results are currently non-blocking and will not block this PR.

All tests passed · 2 tests · 1 device

📱 Devices tested (1)

Android: Google Pixel 8 Pro (v14.0)

✅ Passed Tests (2)
Test Platform Device Duration Team Recording
Money Home after fresh wallet creation with empty balance Android Google Pixel 8 Pro (v14.0) 2.97s @mm-earn-team 📹 Watch
Money Home after importing SRP with funded balance Android Google Pixel 8 Pro (v14.0) 4.71s @mm-earn-team 📹 Watch

Branch: conf-1715-ledger-modal-lifecycle · Build: E2E · Commit: 732d158 · View full run

@github-actions

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/useTransactionPayAutoFiatSubmission.test.ts 0/115 0/262 0/393
app/components/Views/confirmations/hooks/transactions/useTransactionConfirm.test.ts 0/115 0/262 0/393

AI-detected flaky patterns

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

  • J3 — Missing jest.clearAllMocks() / resetAllMocks() (high)
    • The test file uses jest.resetAllMocks() in the beforeEach block, which is good for resetting mocks. However, it is important to ensure that all mocks are cleared between tests to avoid state leakage. The jest.clearAllMocks() should be explicitly called to clear the call history of mocks.
    • Suggested fix in app/components/Views/confirmations/hooks/pay/useTransactionPayAutoFiatSubmission.test.ts:18:
      -    beforeEach(() => {
      -      jest.resetAllMocks();
      -      onConfirmMock.mockResolvedValue(undefined);
      -      useTransactionConfirmMock.mockReturnValue({
      -        navigateOnConfirm: navigateOnConfirmMock,
      -        onConfirm: onConfirmMock,
      -      });
      -      useTransactionPayFiatPaymentMock.mockReturnValue(undefined);
      -    });
      +    beforeEach(() => {
      +      jest.clearAllMocks(); // Clear call history
      +      jest.resetAllMocks(); // Reset mock implementations
      +      onConfirmMock.mockResolvedValue(undefined);
      +      useTransactionConfirmMock.mockReturnValue({
      +        navigateOnConfirm: navigateOnConfirmMock,
      +        onConfirm: onConfirmMock,
      +      });
      +      useTransactionPayFiatPaymentMock.mockReturnValue(undefined);
      +    });

app/components/Views/confirmations/hooks/transactions/useTransactionConfirm.test.ts

  • J3 — Missing jest.clearAllMocks() / resetAllMocks() (high)
    • Similar to the previous finding, the beforeEach block resets mocks but does not clear the call history of mocks. This can lead to unexpected behavior if tests rely on the state of mocks from previous tests.
    • Suggested fix in app/components/Views/confirmations/hooks/transactions/useTransactionConfirm.test.ts:523:
      -    beforeEach(() => {
      -      jest.resetAllMocks();
      -      useParamsMock.mockReturnValue({});
      -      jest.mocked(useFiatConfirm).mockReturnValue({
      -        onFiatConfirm: onFiatConfirmMock,
      -      });
      -    });
      +    beforeEach(() => {
      +      jest.clearAllMocks(); // Clear call history
      +      jest.resetAllMocks(); // Reset mock implementations
      +      useParamsMock.mockReturnValue({});
      +      jest.mocked(useFiatConfirm).mockReturnValue({
      +        onFiatConfirm: onFiatConfirmMock,
      +      });
      +    });

This check is informational only and does not block merging.

@github-actions

Copy link
Copy Markdown
Contributor

🔍 Smart E2E Test Selection

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

E2E Test Selection:
The PR makes significant changes to the confirmation flow infrastructure:

  1. useApprovalRequest.ts - Added { ignoreMissing: true } to rejectPendingApproval. This affects ALL confirmation rejection flows, making SmokeConfirmations essential.

  2. useTransactionConfirm.ts - Extracted navigation logic into a separate navigateOnConfirm function returned from the hook, and added deferNavigation option. This refactoring affects post-confirmation navigation for ALL transaction types: perps deposits, predictions deposits, money account deposits, and standard transactions. Any regression here could break navigation after confirming transactions.

  3. useConfirmActions.ts - Added getRequiredTransactionCount logic for moneyAccountDeposit transactions, integrating with TransactionPayStrategy.Relay quotes. Added onSigningComplete, requiredTransactionCount, and transactionId to the confirm actions interface. This directly impacts Money Account deposit flows (SmokeMoney) and the Ledger hardware wallet path.

  4. useLedgerConfirm.ts - Major enhancement: added transaction signing completion watching via TransactionController:transactionStatusUpdated events for multi-transaction scenarios (like Money Account deposits via Ledger). New parameters: requiredTransactionCount, transactionId, onSigningComplete, deferNavigation. This affects Ledger hardware wallet confirmation flows (SmokeConfirmations).

  5. useMoneyAccount.ts - Minor fix: moved Logger.error/showDevErrorAlert before the rethrow in deposit setup failure path. Affects Money Account deposit error handling (SmokeMoney).

  6. middlewares.test.js - Added test for Ledger rejection error pattern (test-only, no functional impact).

Tag selection rationale:

  • SmokeConfirmations: Core confirmation hooks changed (useApprovalRequest, useConfirmActions, useTransactionConfirm, useLedgerConfirm) - all transaction/signature confirmation flows are affected
  • SmokeMoney: moneyAccountDeposit transaction type is explicitly handled in the new getRequiredTransactionCount logic and useMoneyAccount.ts error handling fix
  • SmokePerps: perpsDeposit and perpsDepositAndOrder transaction types are explicitly handled in navigateOnConfirm navigation logic
  • SmokePredictions: predictDeposit transaction type is explicitly handled in navigateOnConfirm navigation logic
  • SmokeWalletPlatform: Required by SmokePerps and SmokePredictions descriptions (Perps and Predictions are sections inside Trending tab)

Performance Test Selection:
The changes are focused on confirmation flow logic, navigation refactoring, and Ledger hardware wallet integration. None of these changes affect app launch, asset loading, onboarding, login, swap execution timing, or other performance-sensitive paths measured by the available performance test tags. No performance test files were changed.

View GitHub Actions results

@sonarqubecloud

Copy link
Copy Markdown

Quality Gate Failed Quality Gate failed

Failed conditions
61.7% Coverage on New Code (required ≥ 80%)

See analysis details on SonarQube Cloud

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

Labels

size-M team-confirmations Push issues to confirmations team

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant