Skip to content

fix(confirmations): abort send deeplink when chain is missing - #34836

Draft
joaoloureirop wants to merge 1 commit into
mainfrom
fix/22847_send-deeplink-missing-network
Draft

fix(confirmations): abort send deeplink when chain is missing#34836
joaoloureirop wants to merge 1 commit into
mainfrom
fix/22847_send-deeplink-missing-network

Conversation

@joaoloureirop

Copy link
Copy Markdown
Contributor

Description

The redesigned send path fell back to the selected network when findNetworkClientIdByChainId failed, so a send deeplink could open a confirmation on the wrong chain.

getNetworkClientIdForChainId now throws when the requested chain is not in the wallet. The existing “network not found” alert is shown and no transaction is added.

Changelog

CHANGELOG entry: Fixed send deeplinks using the wrong network when the requested chain is not in the wallet

Related issues

Fixes: #22847

Manual testing steps

Feature: send deeplink missing chain

  Scenario: send deeplink for a chain not in the wallet
    Given the wallet does not have chain 10 configured
    When the user opens a send deeplink with chainId 10
    Then a network not found alert is shown
    And no send confirmation is opened on the selected network

Screenshots/Recordings

Before

N/A

After

N/A

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.

Throw instead of falling back to the selected network so a send
deeplink cannot confirm on the wrong chain.

Co-authored-by: Cursor <cursoragent@cursor.com>
@joaoloureirop joaoloureirop self-assigned this Aug 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

Copy link
Copy Markdown
Contributor

🔍 Smart E2E Test Selection

  • Selected E2E tags: SmokeConfirmations
  • Selected Performance tags: None (no tests recommended)
  • Risk Level: medium
  • AI Confidence: 72%
click to see 🤖 AI reasoning details

E2E Test Selection:
The PR modifies app/components/Views/confirmations/utils/deeplink.ts with two key behavioral changes:

  1. Error-throwing instead of silent fallback: getNetworkClientIdForChainId() now throws Unable to find network with chain id ${chainId} instead of falling back to getGlobalNetworkClientId() (the currently selected network). This prevents transactions from being silently sent on the wrong network when a deeplink specifies an unsupported chain ID.

  2. finally block for flag reset: isAddingDeeplinkTransaction is now always reset via finally, fixing a potential bug where the flag could get stuck if an error occurred.

The other two changed files are unit tests validating these new behaviors.

Impact assessment:

  • The changes affect the ethereum: URL deeplink flow for sending ETH/ERC-20 tokens
  • handleEthereumUrl.ts already catches errors from addTransactionForDeeplink and shows a "network not found" alert - the new error propagation integrates cleanly with this existing error handling
  • No E2E smoke tests directly test the ethereum: deeplink URL scheme (confirmed by searching tests/smoke/ and tests/smoke-appium/)
  • The change is in the confirmations/transaction-sending path, so SmokeConfirmations is the most relevant tag to validate the transaction flow still works correctly for normal cases (where the chain IS found)

Why SmokeConfirmations: The deeplink utility is part of the transaction confirmation flow. Running SmokeConfirmations validates that normal transaction sending (where the network IS found) still works correctly after this refactor. The error path is covered by unit tests.

No performance impact: These are logic/error-handling changes with no performance implications.

Performance Test Selection:
The changes are purely behavioral/error-handling logic in the deeplink transaction flow. No performance-sensitive code paths (rendering, data fetching, app launch, etc.) are affected. No performance test tags are warranted.

View GitHub Actions results

@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/utils/deeplink.test.ts 0/136 0/179 0/374
app/core/DeeplinkManager/handlers/legacy/__tests__/handleEthereumUrl.test.ts 0/136 0/179 0/374

AI-detected flaky patterns

app/components/Views/confirmations/utils/deeplink.test.ts

  • J3 — Missing jest.clearAllMocks() / jest.resetAllMocks() (high)
    • The test file uses mocks but does not ensure that the mock state is cleared between tests. This can lead to shared state between tests, causing intermittent failures.
    • Suggested fix in app/components/Views/confirmations/utils/deeplink.test.ts:1:
      -jest.mock('../../../../core/Engine', () => ({
      -  context: {
      -    RemoteFeatureFlagController: {
      -      state: {
      -        remoteFeatureFlags: {},
      -      },
      -    },
      -    AccountsController: {
      -      getSelectedAccount: jest.fn(),
      -    },
      -    NetworkController: {
      -      state: {
      -        selectedNetworkClientId: 'mainnet',
      -        networkConfigurationsByChainId: {
      -          '0x1': {
      -            defaultRpcEndpointIndex: 0,
      -            rpcEndpoints: [{ networkClientId: 'mainnet' }],
      -          },
      -          '0x22': {
      -            defaultRpcEndpointIndex: 0,
      -            rpcEndpoints: [{ networkClientId: 'another-network' }],
      -          },
      -        },
      -      },
      -      findNetworkClientIdByChainId: jest.fn(),
      -    },
      -    TransactionController: {
      -      addTransaction: jest.fn(),
      -    },
      -  },
      -}));
      +beforeEach(() => {
      +  jest.clearAllMocks();
      +});

app/core/DeeplinkManager/handlers/legacy/__tests__/handleEthereumUrl.test.ts

  • J3 — Missing jest.clearAllMocks() / jest.resetAllMocks() (high)
    • The test file uses mocks but does not ensure that the mock state is cleared between tests. This can lead to shared state between tests, causing intermittent failures.
    • Suggested fix in app/core/DeeplinkManager/handlers/legacy/__tests__/handleEthereumUrl.test.ts:1:
      -jest.mock('../../../../../../locales/i18n', () => ({
      -  strings: jest.fn((key) => key),
      -}));
      +beforeEach(() => {
      +  jest.clearAllMocks();
      +});
  • J9 — Module-level mutable let bindings not reset in beforeEach (high)

This check is informational only and does not block merging.

@sonarqubecloud

Copy link
Copy Markdown

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

Labels

size-M team-mobile-platform Mobile Platform team

Projects

Status: Needs dev review

1 participant