Skip to content

Skip unfunded-destination warning for Soroban on Search address screen#2778

Merged
JakeUrban merged 2 commits into
masterfrom
plan-do-review/issue-2777
May 15, 2026
Merged

Skip unfunded-destination warning for Soroban on Search address screen#2778
JakeUrban merged 2 commits into
masterfrom
plan-do-review/issue-2777

Conversation

@JakeUrban
Copy link
Copy Markdown
Contributor

Problem

PR #2720 removed the misleading "Blockaid unfunded destination" warning for
pure Soroban custom tokens on the transaction Review step. The same
warning still appears one step earlier on the Search address screen
(extension/src/popup/components/send/SendTo), which is what issue #2777
tracks. Freighter Mobile already addresses both screens.

Root cause

Two surfaces independently decide whether to show the unfunded-destination
warning:

  • Search address (SendTo/index.tsx:282-285) renders
    <AccountDoesntExistWarning /> based purely on !destinationBalances.isFunded,
    with no asset-side check at all.
  • Review (SendAmount/hooks/useSimulateTxData.tsx:94-123
    getExpectedToFailReason) checks assetCanonical !== "native" and
    isContractId(issuer), but doesn't consider isCollectible or
    contract destinations.

The exported helper shouldAccountDoesntExistWarning(isFunded, assetID, amount)
in SendTo/index.tsx:51-58 is dead code (verified via repo-wide grep) and
encodes a third, incomplete version of the rule.

Proposed change

1. New shared helper

Create extension/src/popup/helpers/sendWarnings.ts:

import { isContractId } from "popup/helpers/soroban";

/**
 * Returns true when the "destination is unfunded" warning rule applies to
 * this send — i.e. the asset transfers via a classic Stellar payment op AND
 * the destination would be a classic G-account.
 *
 *  - Native XLM, credit_alphanum4/12, and SAC-wrapped classic assets all
 *    use a classic payment op that fails to an unfunded G-account (with
 *    the special case of native ≥ 1 XLM, which succeeds via create-account
 *    — that quantitative check is left to the caller; this helper only
 *    answers the qualitative "should the rule even fire?" question).
 *  - Pure Soroban custom tokens (issuer is a C-address) and collectibles
 *    transfer via contract invocation; the classic destination ledger is
 *    never touched.
 *  - Contract (C...) destinations have no classic account at all — their
 *    "balance" lives in the token contract's storage.
 */
export const shouldCheckUnfundedDestinationWarning = ({
  assetCanonical,
  destination,
  isCollectible,
}: {
  assetCanonical: string;
  destination: string;
  isCollectible: boolean;
}): boolean => {
  if (isCollectible) return false;
  if (destination && isContractId(destination)) return false;

  if (assetCanonical && assetCanonical !== "native") {
    const [, issuer] = assetCanonical.split(":");
    if (issuer && isContractId(issuer)) return false;
  }

  return true;
};

isContractId is the same predicate already used in
getExpectedToFailReason, so the two surfaces will agree by construction.

The qualitative/quantitative split (helper answers "does the rule apply?";
caller decides "is the destination actually unfunded enough to fail?") is
deliberate. The native ≥ 1 XLM create-account threshold lives where the
amount is finalized — getExpectedToFailReason's native branch — and the
SendTo screen never needs it (asset is fixed, amount is not yet known).
This addresses the naming concern by making the helper's semantics
unambiguous: it is a precondition gate, not a "this send will fail"
predicate.

2. Use the helper in SendTo/index.tsx

  • Pull asset and isCollectible from the existing transactionDataSelector
    (already imported and used for destination/federationAddress).
  • Replace the inline !isFunded check with a single compound
    helper call (see Tests, below, for why the === false and the
    qualitative gate are co-located):
{shouldShowAccountDoesntExistWarning({
  assetCanonical: asset,
  destination: sendDataState.data.validatedAddress,
  isCollectible,
  isFunded: sendDataState.data.destinationBalances?.isFunded,
}) && <AccountDoesntExistWarning />}

validatedAddress is the right thing to pass: federation domains are
resolved to G-addresses there, and useSendToData only fetches
destinationBalances when the resolved address is non-contract anyway —
but passing it through the helper keeps the rule self-consistent at the
read site.

Note the change from !destinationBalances.isFunded to destinationBalances?.isFunded === false: isFunded is boolean | null (@shared/api/types/backend-api.ts:10-13), and Review-step logic only warns on === false (useSimulateTxData.tsx:105-123). Aligning Search with that strict check means an unknown/null funding state (e.g. mid-fetch or fetch failure) no longer triggers a misleading warning. (Critic round 2.)

  • Delete the dead shouldAccountDoesntExistWarning export and its base
    reserve constant. They are unused (grep confirms). Leaving a stale,
    near-correct helper next to the new one is a footgun for the next
    person tracing this logic. The min-1-XLM check it encoded for native
    sends already lives in getExpectedToFailReason's native branch.

3. Refactor getExpectedToFailReason — in-place, no API surface change

Per critic feedback (round 1, item 1): do not thread new args through
popup/views/Send/index.tsx. The hook already has what it needs:

  • useSimulateTxData already accepts destination as a hook arg
    (useSimulateTxData.tsx:394-405).
  • useSimulateTxData already destructures from transactionDataSelector
    (useSimulateTxData.tsx:410-412); add isCollectible to that destructure.
  • The single getExpectedToFailReason(...) call inside the hook
    (useSimulateTxData.tsx:472-477) gains two locally-scoped parameters
    (destination, isCollectible).
  • Send/index.tsx is untouched; the hook signature is untouched.

Inside getExpectedToFailReason, replace the bespoke
assetCanonical !== "native" + issuer parsing with:

if (
  isDestinationFunded !== false ||
  !shouldCheckUnfundedDestinationWarning({
    assetCanonical,
    destination,
    isCollectible,
  })
) {
  return null;
}
// native min-1-XLM branch unchanged

4. Tests

Unit tests for shouldCheckUnfundedDestinationWarning covering:

  • Native XLM → true
  • credit_alphanum4 with G-issuer → true
  • SAC-wrapped classic (G-issuer) → true
  • Pure Soroban custom token (C-issuer) → false
  • isCollectible: true (regardless of asset) → false
  • Contract (C...) destination (regardless of asset) → false
  • Empty assetCanonical (initial-state edge case before
    useSendQueryParams resolves) → defaults to "treat as classic" →
    document why this is safe (the warning would only render once
    destinationBalances loads, which gates on a real address, and the
    default redux value for asset is "native" per
    transactionSubmission.ts:488, so empty-string is not a real
    runtime path; covered as a defensive case).

SendTo warning gate as a unit-testable helper. The
extension/src/popup/views/__tests__/SendPayment.test.tsx integration
suite is currently describe.skip("Send", ...) (line 128, verified
round 3) — anything added there would not run. Rather than unskip an
unrelated suite or stand up an ad-hoc integration harness for one
boolean, extract the wrapping === false + helper composition into a
second pure helper alongside shouldCheckUnfundedDestinationWarning
in sendWarnings.ts:

export const shouldShowAccountDoesntExistWarning = ({
  assetCanonical,
  destination,
  isCollectible,
  isFunded,
}: {
  assetCanonical: string;
  destination: string;
  isCollectible: boolean;
  isFunded: boolean | null | undefined;
}): boolean =>
  isFunded === false &&
  shouldCheckUnfundedDestinationWarning({
    assetCanonical,
    destination,
    isCollectible,
  });

The SendTo JSX then collapses to:

{shouldShowAccountDoesntExistWarning({
  assetCanonical: asset,
  destination: sendDataState.data.validatedAddress,
  isCollectible,
  isFunded: sendDataState.data.destinationBalances?.isFunded,
}) && <AccountDoesntExistWarning />}

This puts the strict-=== false rule in the same module as the
qualitative gate (one place to update) and makes the integration cases
pure unit tests in extension/src/popup/helpers/__tests__/sendWarnings.test.ts:

  • Default state (asset: "native", isCollectible: false,
    isFunded: false) + non-contract G-destination → true.
  • Pure Soroban asset (asset: "TOKEN:CABC...XYZ",
    isCollectible: false, isFunded: false) + G-destination → false.
  • Collectible state (isCollectible: true, asset: "native"
    reproduces the early-return query-param hydration in
    useSendQueryParams.ts:64-85 where saveIsCollectible(true) runs
    but the asset destructure path is short-circuited) +
    isFunded: false → false.
  • Contract destination (destination: "C...") regardless of asset +
    isFunded: false → false. (Also documents that
    useSendToData.tsx:92-93 already skips balance fetch for contract
    destinations, so this is belt-and-braces.)
  • Unknown funding state (isFunded: null, e.g. mid-fetch or backend
    fallback) with classic asset + non-contract destination → false
    (regression guard for the strict-=== false change, per critic
    round 2).
  • isFunded: undefined (no balances loaded yet) with classic asset →
    false (defensive, matches the optional-chaining at the call site).
  • isFunded: true with classic asset → false (sanity).

Update useSimulateTxData.test.ts's existing
getExpectedToFailReason cases to pass the new destination /
isCollectible params, and add a collectible case (collectible flag
true with classic asset / unfunded G-destination → returns null).

5. Out of scope (file as follow-ups if confirmed)

  • Equivalent screens for Swap/Soroswap, account-merge, and the QR-scan
    recipient flow — none called out in Skip unfunded-destination warning for Soroban on Search address screen #2777, each touches separate UI.
  • Aligning extension's helper with mobile's buildUnfundedContext shape
    (which feeds Blockaid result merging on mobile). Extension's Review
    step uses a simpler "expected to fail reason" string; cross-platform
    convergence is a separate refactor.
  • Promoting the helper into @shared/helpers/ for cross-submodule
    reuse. Mobile's helper is shaped differently (returns
    UnfundedDestinationContext); a shared module would have to bridge
    both shapes, which is out of scope for a one-screen bug fix.

Verification

Per freighter/AGENTS.md (also stored in agent memory): from repo root
run yarn test:ci and yarn build:extension. Manually exercise the
SendTo screen in the extension dev build with (a) a classic asset →
unfunded G-address (warning shown) and (b) a pure Soroban custom token →
unfunded G-address (warning hidden).


Closes #2777

PR #2720 removed the misleading "Blockaid unfunded destination" warning
for pure Soroban custom tokens on the transaction Review step. The same
warning still appeared one step earlier on the Search address screen
(SendTo). This change consolidates the rule into a single helper so
both surfaces agree by construction.

- Add popup/helpers/sendWarnings.ts with two pure helpers:
  - shouldCheckUnfundedDestinationWarning (qualitative gate: classic
    asset + classic destination + non-collectible)
  - shouldShowAccountDoesntExistWarning (compound: gate + strict
    isFunded === false, matching Review-step semantics where isFunded
    is boolean | null)
- SendTo/index.tsx: pull asset/isCollectible from transactionDataSelector
  and use the compound helper; delete the dead
  shouldAccountDoesntExistWarning export and unused baseReserve.
- useSimulateTxData.tsx: refactor getExpectedToFailReason in-place to
  delegate the qualitative gate to the shared helper; thread destination
  and isCollectible (already available in the hook) into the single
  call site without widening the hook's public API.
- Tests:
  - New sendWarnings.test.ts covers native, classic, SAC, pure Soroban,
    collectible, contract destination, null/undefined funding, and
    the early-return query-param hydration edge case.
  - useSimulateTxData.test.ts updated for the new params and gains
    collectible / contract-destination cases.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings May 12, 2026 18:54
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR aligns the “unfunded destination” warning behavior between the Send flow’s Search address (SendTo) step and the Review step by introducing a shared predicate that disables the warning for Soroban-only transfers (contract-issuer assets), collectibles, and contract destinations.

Changes:

  • Added shared helpers in popup/helpers/sendWarnings.ts to centralize when the unfunded-destination warning rule should apply, plus a SendTo-specific compound predicate that requires isFunded === false.
  • Updated SendTo to use the shared helper and to avoid warning on unknown funding state (null/undefined).
  • Refactored getExpectedToFailReason to delegate the qualitative gate to the shared helper and updated/expanded unit tests accordingly.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
extension/src/popup/helpers/sendWarnings.ts New shared predicates to gate unfunded-destination warnings consistently across screens.
extension/src/popup/helpers/tests/sendWarnings.test.ts Unit tests covering classic vs Soroban/collectible/contract-destination cases and funding-state semantics.
extension/src/popup/components/send/SendTo/index.tsx Uses the new helper to suppress misleading warnings on Search address step and aligns strict isFunded === false behavior.
extension/src/popup/components/send/SendAmount/hooks/useSimulateTxData.tsx Refactors getExpectedToFailReason to use the shared qualitative gate and accounts for collectibles/destination contract IDs.
extension/src/popup/components/send/SendAmount/hooks/tests/useSimulateTxData.test.ts Updates existing tests for new params and adds coverage for collectible + contract-destination behavior.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@@ -0,0 +1,71 @@
import { isContractId } from "popup/helpers/soroban";
Addressed Copilot PR review suggestion: sendWarnings.ts only uses the
isContractId predicate, so importing from the lightweight @shared source
directly avoids pulling in the larger popup/helpers/soroban module.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copy link
Copy Markdown
Collaborator

@leofelix077 leofelix077 left a comment

Choose a reason for hiding this comment

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

Verified behavior, unit tests and other E2E tests for regression. All look fine

There's one E2E test that is failing, but also failing on master, due to wrong mocks (Send token payment to C address), but works fine when doing manually. Checking it separately

xlm-and-collectibles.mov
custom-token.mov
Screen.Recording.2026-05-15.at.12.57.01.mov

@JakeUrban JakeUrban merged commit d8ee94c into master May 15, 2026
9 checks passed
@JakeUrban JakeUrban deleted the plan-do-review/issue-2777 branch May 15, 2026 16:29
CassioMG added a commit that referenced this pull request May 15, 2026
#2778) (#2781)

* Skip unfunded-destination warning for Soroban on Search address screen

PR #2720 removed the misleading "Blockaid unfunded destination" warning
for pure Soroban custom tokens on the transaction Review step. The same
warning still appeared one step earlier on the Search address screen
(SendTo). This change consolidates the rule into a single helper so
both surfaces agree by construction.

- Add popup/helpers/sendWarnings.ts with two pure helpers:
  - shouldCheckUnfundedDestinationWarning (qualitative gate: classic
    asset + classic destination + non-collectible)
  - shouldShowAccountDoesntExistWarning (compound: gate + strict
    isFunded === false, matching Review-step semantics where isFunded
    is boolean | null)
- SendTo/index.tsx: pull asset/isCollectible from transactionDataSelector
  and use the compound helper; delete the dead
  shouldAccountDoesntExistWarning export and unused baseReserve.
- useSimulateTxData.tsx: refactor getExpectedToFailReason in-place to
  delegate the qualitative gate to the shared helper; thread destination
  and isCollectible (already available in the hook) into the single
  call site without widening the hook's public API.
- Tests:
  - New sendWarnings.test.ts covers native, classic, SAC, pure Soroban,
    collectible, contract destination, null/undefined funding, and
    the early-return query-param hydration edge case.
  - useSimulateTxData.test.ts updated for the new params and gains
    collectible / contract-destination cases.



* Import isContractId from @shared/api/helpers/soroban in sendWarnings.ts

Addressed Copilot PR review suggestion: sendWarnings.ts only uses the
isContractId predicate, so importing from the lightweight @shared source
directly avoids pulling in the larger popup/helpers/soroban module.



---------

Co-authored-by: Jake Urban <10968980+JakeUrban@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
CassioMG added a commit that referenced this pull request May 15, 2026
* v5.41.0

* Trigger CI

* Skip unfunded-destination warning for Soroban on Search address screen (#2778) (#2781)

* Skip unfunded-destination warning for Soroban on Search address screen

PR #2720 removed the misleading "Blockaid unfunded destination" warning
for pure Soroban custom tokens on the transaction Review step. The same
warning still appeared one step earlier on the Search address screen
(SendTo). This change consolidates the rule into a single helper so
both surfaces agree by construction.

- Add popup/helpers/sendWarnings.ts with two pure helpers:
  - shouldCheckUnfundedDestinationWarning (qualitative gate: classic
    asset + classic destination + non-collectible)
  - shouldShowAccountDoesntExistWarning (compound: gate + strict
    isFunded === false, matching Review-step semantics where isFunded
    is boolean | null)
- SendTo/index.tsx: pull asset/isCollectible from transactionDataSelector
  and use the compound helper; delete the dead
  shouldAccountDoesntExistWarning export and unused baseReserve.
- useSimulateTxData.tsx: refactor getExpectedToFailReason in-place to
  delegate the qualitative gate to the shared helper; thread destination
  and isCollectible (already available in the hook) into the single
  call site without widening the hook's public API.
- Tests:
  - New sendWarnings.test.ts covers native, classic, SAC, pure Soroban,
    collectible, contract destination, null/undefined funding, and
    the early-return query-param hydration edge case.
  - useSimulateTxData.test.ts updated for the new params and gains
    collectible / contract-destination cases.



* Import isContractId from @shared/api/helpers/soroban in sendWarnings.ts

Addressed Copilot PR review suggestion: sendWarnings.ts only uses the
isContractId predicate, so importing from the lightweight @shared source
directly avoids pulling in the larger popup/helpers/soroban module.



---------

Co-authored-by: Jake Urban <10968980+JakeUrban@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

---------

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: Cássio Marcos Goulart <cassiomgoulart@gmail.com>
Co-authored-by: Cássio Marcos Goulart <3228151+CassioMG@users.noreply.github.com>
Co-authored-by: Jake Urban <10968980+JakeUrban@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Skip unfunded-destination warning for Soroban on Search address screen

4 participants