Improve Add Token disclosure for SAC

[leofelix077]

Closes #2826

Small PR to adjust the flow for adding SAC tokens to make it a 2 step flow on the Add token + Confirm Transaction for clarity

and for SEP-41 it continues being a direct approval

⚠️ - Some UI issues like double padding or double spacing were also added as part of this PR to make it more consistent

e.g. "before" screenshot

Videos: 1 SAC verified / 1 SEP-41 unverified

Screen.Recording.2026-06-24.at.17.45.53.mov

Screen.Recording.2026-06-24.at.17.46.56.mov

[github-actions]

PR Preview build is ready: https://github.com/stellar/freighter/releases/tag/untagged-1ea4e3149826d09d43f1 (SDF collaborators only — install instructions in the release description)

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: aea39a3184

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 0b9b951deb

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: e00510aa7d

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 1a1539b9dc

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

[Copilot]

Pull request overview

This PR updates the extension popup’s Add Token flow to treat SAC (classic-asset) tokens as a 2-step flow (Add Token → Change Trust review/submit), while keeping SEP-41 tokens as a direct approval path. It also refactors the prior “SAC implies changeTrust” logic out of useSetupAddTokenFlow and into the UI routing, and adds/updates tests and E2E stubs to cover the new behavior.

Changes:

• Route SAC adds through ChangeTrustInternal (with fee disclosure/prop wiring) before resolving the dApp request; keep SEP-41 as a direct confirm.
• Extend ChangeTrustInternal/Submit UI to better support “standalone full-height” usage and avoid misleading close-tab prompts in this context.
• Add new unit tests + expand E2E coverage/stubs for SAC vs SEP-41 add-token scenarios.

Reviewed changes

Copilot reviewed 16 out of 17 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
extension/src/popup/views/AddToken/styles.scss	Removes scroll overflow on the AddToken wrapper styling.
extension/src/popup/views/AddToken/index.tsx	Implements SAC 2-step routing into ChangeTrustInternal, fee display, and “Continue” CTA for SAC.
extension/src/popup/views/AddToken/tests/AddToken.test.tsx	Adds routing/unit coverage for SAC vs SEP-41 behavior in AddToken.
extension/src/popup/locales/pt/translation.json	Adds translations for “Auto-lock timer”, “Auto-Lock Timer”, and “Token address”.
extension/src/popup/locales/en/translation.json	Adds English strings for “Auto-lock timer”, “Auto-Lock Timer”, and “Token address”.
extension/src/popup/helpers/useSetupAddTokenFlow.ts	Removes SAC trustline submission from the “approve add token” helper; now always dispatches addToken+metrics.
extension/src/popup/helpers/useChangeTrustline.ts	Deletes the no-longer-used trustline hook.
extension/src/popup/components/WarningMessages/styles.scss	Adds styling for the asset-list banner variant (.ScanAssetList).
extension/src/popup/components/WarningMessages/index.tsx	Updates AssetListWarning styling class + icon.
extension/src/popup/components/manageAssets/ManageAssetRows/ChangeTrustInternal/SubmitTx/index.tsx	Adds onClose + hideCloseTabHint behavior; routes Done differently on success vs failure.
extension/src/popup/components/manageAssets/ManageAssetRows/ChangeTrustInternal/styles.scss	Adds “standalone/full-height” layout rules and scroll behavior for the external Add Token flow.
extension/src/popup/components/manageAssets/ManageAssetRows/ChangeTrustInternal/index.tsx	Adds onSuccess, initialFee, isFullHeight; passes fee into XDR build and adjusts padding for submit step.
extension/src/popup/components/manageAssets/ManageAssetRows/ChangeTrustInternal/hooks/useChangeTrustData.tsx	Makes change-trust data reload when key inputs (including fee) change.
extension/src/popup/components/tests/ChangeTrustInternal.test.tsx	Adds coverage for onSuccess vs onCancel behavior and fee initialization/unit correctness.
extension/e2e-tests/integration-tests/freighterApiIntegration.test.ts	Adds/updates E2E coverage for verified/unverified SEP-41 and SAC add-token flows, including SAC-specific stub injection.
extension/e2e-tests/helpers/stubs.ts	Adds SAC-specific stubs and a helper to mark a contractId as “verified” via asset-list stubbing.

---

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

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 5e51fd73e6

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

[leofelix077]

side-fix to unify the translations and add the PT translation for it. the prettier was adding them back before push. was missing from the other PR that was merged related to the auto lock

[piyalbasu]

Add-SAC-token: verified-list path can silently skip the trustline

Suggestion / latent edge — not a merge blocker. Reachable only when an asset-list entry for a real classic-asset SAC is imperfect (missing/normalized issuer or code), so it won't show up in the happy-path or the current tests — but when it does hit, the failure is silent (token added, no trustline) rather than a visible error.

TL;DR: When an add-token request resolves a token via a verified asset list, the decision to create a trustline rests entirely on re-deriving the asset's contract from the list's code/issuer fields. The authoritative on-chain "is this a Stellar Asset Contract?" check is not run on that path. So if the list entry for a genuine classic-asset SAC is missing or has a slightly-off issuer/code, the token is added to the wallet but no trustline is created — and the user can't actually hold or receive that asset, with no error shown.

---

Detailed explanation (for agents)

Root cause: two different SAC tests, and on the verified-list path only the weaker one runs.

There are two SAC determinations in this flow:

• isSacContractExecutable — authoritative, on-chain. It reads the contract instance's executable type (executable.switch().name === contractExecutableStellarAsset().name), or on standard networks queries the indexer /is-sac-contract/{id} endpoint. Answers "is contract X a Stellar Asset Contract?"
  

  freighter/@shared/helpers/soroban/token.ts

  Lines 102 to 151 in 6255393

  	export const isSacContractExecutable = async (
  	contractId: string,
  	networkDetails: NetworkDetails,
  	signal?: AbortSignal,
  	) => {
  	if (isCustomNetwork(networkDetails)) {
  	// verify the contract executable in the instance entry
  	// The SAC has a unique contract executable type
  	if (!networkDetails.sorobanRpcUrl) {
  	throw new SorobanRpcNotSupportedError();
  	}
  	const server = buildSorobanServer(
  	networkDetails.sorobanRpcUrl || "",
  	networkDetails.networkPassphrase,
  	);
  	const instance = new Contract(contractId).getFootprint();
  	const ledgerKeyContractCode = instance.toXDR("base64");
  	const { entries } = await server.getLedgerEntries(
  	xdr.LedgerKey.fromXDR(ledgerKeyContractCode, "base64"),
  	);
  	if (entries && entries.length) {
  	const parsed = entries[0].val;
  	const executable = parsed.contractData().val().instance().executable();
  	return (
  	executable.switch().name ===
  	xdr.ContractExecutableType.contractExecutableStellarAsset().name
  	);
  	}
  	throw new Error("Contract not found in the ledger entries");
  	}
  	try {
  	const url = new URL(
  	`${INDEXER_URL}/is-sac-contract/${contractId}?network=${networkDetails.network}`,
  	);
  	const response = await fetch(url.href, { signal });
  	const data = await response.json();
  	if (!response.ok) {
  	throw new Error(data);
  	}
  	return data.isSacContract;
  	} catch (e) {

• isAssetSac → isSacContract — derived. It computes new Asset(code, issuer).contractId(passphrase) === contract. Answers the stronger question "is contract X the SAC of the specific (code, issuer) I have in hand?"
  

  freighter/extension/src/popup/helpers/soroban.ts

  Lines 861 to 920 in 6255393

  	export const isSacContract = (
  	name: string,
  	contractId: string,
  	networkPassphrase: string,
  	) => {
  	const Sdk = getSdk(networkPassphrase);
  	if (name.includes(":")) {
  	try {
  	return (
  	new Sdk.Asset(...(name.split(":") as [string, string])).contractId(
  	networkPassphrase,
  	) === contractId
  	);
  	} catch (error) {
  	return false;
  	}
  	}
  	return false;
  	};
  	/**
  	* Determines if an asset is a Stellar Asset Contract (SAC).
  	* SAC assets include:
  	* 1. The native XLM contract
  	* 2. Classic Stellar assets that have been wrapped as Soroban contracts
  	*
  	* @param asset - Asset details
  	* @param networkDetails - Network configuration details
  	* @returns true if the asset is a SAC, false otherwise
  	*/
  	export const isAssetSac = ({
  	asset,
  	networkDetails,
  	}: {
  	asset: {
  	code: string;
  	issuer: string | undefined;
  	contract: string | undefined;
  	};
  	networkDetails: NetworkDetails;
  	}): boolean => {
  	if (!asset.contract) {
  	return false;
  	}
  	const nativeContract = getNativeContractDetails(networkDetails);
  	// Check if it's the native XLM contract
  	if (asset.contract === nativeContract.contract) {
  	return true;
  	}
  	// Check if it's a classic asset wrapper (SAC)
  	const canonicalName = getCanonicalFromAsset(asset.code, asset.issuer);
  	return isSacContract(
  	canonicalName,
  	asset.contract,
  	networkDetails.networkPassphrase,
  	);

On the manual lookup path these agree, because code/issuer come from the contract's own symbol()/name(), which a real SAC derives from the asset it wraps — so the re-derivation round-trips deterministically, and isSacContractExecutable is consulted up front:

freighter/extension/src/popup/helpers/useTokenLookup.ts

Lines 89 to 110 in 6255393

	const isSacContract = await isSacContractExecutable(
	contractId,
	networkDetails,
	);
	if (!tokenDetailsResponse) {
	setAssetRows([]);
	} else {
	const issuer = isSacContract
	? tokenDetailsResponse.name.split(":")[1] || ""
	: contractId; // get the issuer name, if applicable ,
	const scannedAsset = await scanAsset(
	`${tokenDetailsResponse.symbol}-${issuer}`,
	networkDetails,
	);
	setAssetRows([
	{
	code: tokenDetailsResponse.symbol,
	contract: contractId,
	issuer,
	domain: "",
	name: tokenDetailsResponse.name,

On the verified-list path, isSacContractExecutable is never called. code/issuer come straight from curated JSON, with a fallback to the C-address when a field is absent:

freighter/extension/src/popup/helpers/useTokenLookup.ts

Lines 132 to 140 in 6255393

	setAssetRows(
	verifiedTokens.map((record: VerifiedTokenRecord) => ({
	code: record.code || record.contract,
	issuer: record.issuer || record.contract,
	image: record.icon,
	domain: record.domain,
	contract: record.contract,
	})),
	);

Routing then rests solely on isAssetSac against that curated data:

freighter/extension/src/popup/views/AddToken/index.tsx

Lines 315 to 322 in 6255393

	const isSac = isAssetSac({
	asset: {
	code: assetCode,
	issuer: assetIssuer,
	contract: assetCurrency?.contract,
	},
	networkDetails: state.data.settings.networkDetails,
	});

freighter/extension/src/popup/views/AddToken/index.tsx

Line 562 in 6255393

isSac ? setShowTrustlineReview(true) : handleApprove()

So for a genuine classic-asset SAC whose list entry is imperfect, isAssetSac returns false and the Confirm button takes the handleApprove() branch — addToken with no changeTrust op. Failure modes:

• Missing issuer → issuer = record.contract (a C… address) → new Sdk.Asset(code, "C…") throws → catch returns false:
  

  freighter/extension/src/popup/helpers/soroban.ts

  Lines 866 to 878 in 6255393

  	const Sdk = getSdk(networkPassphrase);
  	if (name.includes(":")) {
  	try {
  	return (
  	new Sdk.Asset(...(name.split(":") as [string, string])).contractId(
  	networkPassphrase,
  	) === contractId
  	);
  	} catch (error) {
  	return false;
  	}
  	}

• Code/issuer casing or normalization drift vs. the on-ledger asset → derived contract id ≠ record.contract → false.

isSacContractExecutable cannot rescue either case because it isn't invoked on this path.

Secondary inconsistency: the two functions key on different network fields — isSacContractExecutable builds its indexer URL from networkDetails.network, while isAssetSac derives the contract id from networkDetails.networkPassphrase. Consistent on standard networks; on a misconfigured custom network where the two drift, they're answering about different networks.

Deterministic repro: drive AddToken with a stubbed useTokenLookup whose verified-list branch returns a record for a real classic-asset SAC but with issuer omitted (so issuer falls back to the C-address) — or with a code that differs in case from the on-ledger asset. Assert that isAssetSac returns false and Confirm routes to handleApprove() (no showTrustlineReview, no changeTrust submitted). The existing e2e tests only cover a cleanly-derived SAC_CONTRACT_ID, i.e. the case that always matches.

Suggested fixes (increasing depth):

1. Test the gap: add an AddToken test for the issuer-less / C-address-fallback and code-drift verified-list inputs, pinning the intended routing so a regression is visible.
2. Close the path: consult isSacContractExecutable on the verified-list path too (it's already imported), and/or when isSacContractExecutable says SAC but isAssetSac says not, log/telemeter the disagreement instead of silently dropping the trustline — so this becomes observable rather than a silent no-trustline add.
3. Reconcile network keys: have both SAC checks key off the same network identity (passphrase) to remove the custom-network drift vector.

[piyalbasu]

Add-SAC-token: a failed submit tells the dApp the user rejected, with no way to retry

Suggestion / UX + correctness — not a merge blocker. Happens on any transient submit failure (network blip, tx_bad_seq, fee too low) during the SAC trustline step, which is a normal real-world occurrence.

TL;DR: If the trustline transaction fails to submit, the only thing the user can do is dismiss — and dismissing tells the dApp "the user rejected this request," even though the user wanted the token and the submit just failed. There's no retry. Ideally a transient failure should let the user retry in place, and the dApp should only ever hear "user rejected" when the user actually rejects.

Steps to reproduce:

1. From a dApp, request to add a SAC token; approve it in the popup so the trustline review appears.
2. Cause the submit to fail (e.g. go offline, or have Horizon return a transient error like tx_insufficient_fee).
3. The screen shows "Failed!" with only a Done button — no Retry.
4. Click Done → the popup closes and the dApp's addToken() promise rejects with "The user rejected this request," plus a reject metric is emitted.

---

Detailed explanation (for agents)

Root cause. In the failure state (isFail = state.state === RequestState.ERROR) the footer renders only the Done button — there is no retry control, and "View transaction" is success-only:

freighter/extension/src/popup/components/manageAssets/ManageAssetRows/ChangeTrustInternal/SubmitTx/index.tsx

Lines 223 to 247 in 6255393

	{(isSuccess || isFail) && (
	<div className="SubmitTransaction__Footer__Done">
	<Button
	size="lg"
	isFullWidth
	isRounded
	variant="secondary"
	isLoading={
	resetChangeTrustDataState.state === RequestState.LOADING
	}
	onClick={async (e) => {
	e.preventDefault();
	await resetChangeTrustData({
	isHardwareWallet,
	isSuccess,
	});
	if (isSuccess) {
	onSuccess();
	} else {
	onClose();
	}
	}}
	>
	{t("Done")}
	</Button>

Done branches if (isSuccess) onSuccess() else onClose(), so on failure it calls onClose(). In the SAC add-token flow onClose is wired to rejectAndClose:

freighter/extension/src/popup/views/AddToken/index.tsx

Lines 394 to 395 in 6255393

	onSuccess={handleSacSuccess}
	onClose={rejectAndClose}

rejectAndClose emits the reject metric and dispatches rejectToken:

freighter/extension/src/popup/helpers/useSetupAddTokenFlow.ts

Lines 40 to 43 in 6255393

	const rejectAndClose = () => {
	emitMetric(METRIC_NAMES.tokenRejectApi);
	dispatch(rejectTokenFn({ uuid }));
	window.close();

rejectToken (and the window-close fallback) resolve the dApp request with FreighterApiDeclinedError — "The user rejected this request." (code -4):

freighter/@shared/api/helpers/extensionMessaging.ts

Lines 94 to 103 in 6255393

	export const FreighterApiInternalError = {
	code: -1,
	message:
	"The wallet encountered an internal error. Please try again or contact the wallet if the problem persists.",
	};
	export const FreighterApiDeclinedError = {
	code: -4,
	message: "The user rejected this request.",
	};

So a failed submit and a deliberate user rejection are indistinguishable to the dApp, and the user has no way to retry. Note the same rejectAndClose is also correctly used for the explicit reject button (AddToken/index.tsx:549) — that usage is right; the problem is reusing it for the failed-submit dismissal.

Also relevant: the window-close fallback rejectOnWindowClose returns FreighterApiDeclinedError on any close (popupMessageListener.ts:130), so simply closing after a failure also reads as a rejection.

Suggested fix (matches the desired behavior):

1. Retry in place. On isFail, render a Retry button that re-invokes the submit (fetchData from useGetChangeTrust, the same call the mount effect makes) so a transient network/Horizon failure can be retried without leaving the popup.

2. Reserve "user rejected" for actual rejection. Don't route the failed-submit dismissal through rejectAndClose. The declined signal (FreighterApiDeclinedError, code -4) and the tokenRejectApi metric should fire only from the explicit reject/cancel/back paths.

3. Give the dApp the right error. For a failed-then-dismissed submit, resolve the request with the existing FreighterApiInternalError (code -1, "The wallet encountered an internal error. Please try again…") instead of the declined error — it already exists right beside the declined one:
   

   freighter/@shared/api/helpers/extensionMessaging.ts

   Lines 94 to 103 in 6255393

   	export const FreighterApiInternalError = {
   	code: -1,
   	message:
   	"The wallet encountered an internal error. Please try again or contact the wallet if the problem persists.",
   	};
   	export const FreighterApiDeclinedError = {
   	code: -4,
   	message: "The user rejected this request.",
   	};

   
   To fully close the gap, the window-close fallback should also return the internal error (not declined) once a submit has failed, so closing the window post-failure doesn't read as a rejection either.

[piyalbasu]

Add-SAC-token: success handler can leave the popup open and the dApp hanging

Suggestion / correctness — not a merge blocker. Only triggers if the addToken dispatch itself throws, which is uncommon — but the failure mode is silent (popup stuck open, dApp promise never resolves, no failure metric).

TL;DR: The SAC add-token success handler has no error handling, while the equivalent handler for the other token type does. If adding the token fails at the last step, the popup stays open, the dApp's request never resolves, and the failure isn't recorded — whereas the sibling path cleanly closes and records the failure.

---

Detailed explanation (for agents)

Root cause. handleSacSuccess runs the add/metric/close sequence with no try/catch:

freighter/extension/src/popup/views/AddToken/index.tsx

Lines 123 to 128 in 6255393

	const handleSacSuccess = async () => {
	await dispatch(addToken({ uuid }));
	await emitMetric(METRIC_NAMES.tokenAddedApi);
	window.close();
	};

The SEP-41 path uses addTokenAndClose, which wraps the identical sequence in try/catch, emits tokenFailedApi on error, and calls window.close() unconditionally (so the popup always closes and the request always resolves):

freighter/extension/src/popup/helpers/useSetupAddTokenFlow.ts

Lines 46 to 54 in 6255393

	const addTokenAndClose = async () => {
	try {
	await dispatch(addTokenFn({ uuid }));
	await emitMetric(METRIC_NAMES.tokenAddedApi);
	} catch (e) {
	console.error(e);
	await emitMetric(METRIC_NAMES.tokenFailedApi);
	}
	window.close();

dispatch(addToken({ uuid })) is a createAsyncThunk backed by an async background-messaging round-trip, so it can reject. In handleSacSuccess a rejection propagates out of the handler: emitMetric(tokenAddedApi) and window.close() never run → popup stays open, the dApp's addToken() promise hangs, and tokenFailedApi is never emitted. The two success paths are asymmetric for no apparent reason.

Suggested fix: have the SAC branch reuse addTokenAndClose (or expose it from useSetupAddTokenFlow) instead of re-implementing the sequence — that collapses the two paths into one and inherits the try/catch + tokenFailedApi + unconditional close. Minimal alternative: wrap handleSacSuccess in the same try/catch and always close.

[piyalbasu]

Deleting useChangeTrustline orphaned the add/remove-asset metrics

Suggestion / analytics regression — not a merge blocker. Affects more than this feature: every in-app Manage-Assets add/remove-trustline plus the dApp add-token trustline now emits zero add/remove-asset analytics.

TL;DR: This PR removes the only code that emitted the "add asset" / "remove asset" analytics events. After it merges, those two metrics fire from nowhere, so any dashboard or funnel built on them silently drops to zero. There's no compile error to catch it — worth confirming this is intentional, and if not, re-wiring the events.

---

Detailed explanation (for agents)

Root cause. The deleted useChangeTrustline hook was the sole emitter of METRIC_NAMES.manageAssetAddAsset / manageAssetRemoveAsset (it picked between them based on add-vs-remove at base useChangeTrustline.ts:120-121):

freighter/extension/src/popup/helpers/useChangeTrustline.ts

Lines 118 to 122 in 2cca02a

	emitMetric(
	addTrustline
	? METRIC_NAMES.manageAssetAddAsset
	: METRIC_NAMES.manageAssetRemoveAsset,
	{ code: assetCode, issuer: assetIssuer },

At PR head, the only remaining references to either metric across extension/src are the constant definitions themselves — no emitMetric call site survives:

freighter/extension/src/popup/constants/metricsNames.ts

Lines 101 to 104 in 6255393

	manageAssetAddAsset: "manage asset: add asset",
	manageAssetAddToken: "manage asset: add token",
	manageAssetAddUnsafeAsset: "manage asset: add unsafe asset",
	manageAssetRemoveAsset: "manage asset: remove asset",

The replacement flow (ChangeTrustInternal → SubmitTx) emits no add/remove-asset metric, and the SAC add-token success only emits tokenAddedApi. So both the in-app Manage-Assets add/remove-trustline path and the dApp add-token trustline path now produce zero manage asset: add/remove asset events. Because the constants still exist, nothing fails to compile.

Suggested fix: if these metrics are still wanted, emit them from the new submit path (e.g. in SubmitTx on a successful changeTrust, choosing add vs remove from addTrustline). If the metrics are intentionally retired, delete the constants so the dashboards' data source is unambiguously gone rather than silently dark.

[piyalbasu]

SAC trustline review re-does work AddToken already did (redundant network calls + a stale lint suppression)

Suggestion / efficiency + maintainability — not a merge blocker. Several redundant network round-trips on the SAC review screen, plus one of them re-fires on every fee-editor change.

TL;DR: When the SAC add-token review opens, it re-fetches data the previous screen already had — it scans the asset with Blockaid a second time and re-fetches network fees a second time, because neither result is passed down. On top of that, editing the fee in the review re-runs the whole data fetch (including the Blockaid scan and an account load) even though only the fee changed. None of this is user-visible beyond extra latency/requests, but it's easy to avoid by threading the already-fetched values through.

---

Detailed explanation (for agents)

These four share one root cause: ChangeTrustInternal / useChangeTrustData re-derive things AddToken already computed, because nothing is passed down.

(a) Fee edit re-runs the full data fetch. The data-fetch effect went from mount-once ([]) to a 10-dep array that includes recommendedFee:

freighter/extension/src/popup/components/manageAssets/ManageAssetRows/ChangeTrustInternal/hooks/useChangeTrustData.tsx

Lines 122 to 136 in 6255393

	useEffect(() => {
	fetchData();
	// eslint-disable-next-line react-hooks/exhaustive-deps
	}, [
	addTrustline,
	asset.code,
	asset.contract,
	asset.domain,
	asset.issuer,
	blockaidOverrideState,
	networkDetails.networkPassphrase,
	networkDetails.networkUrl,
	publicKey,
	recommendedFee,
	]);

recommendedFee is fed the editable fee state (ChangeTrustInternal/index.tsx:108), so each fee edit re-runs fetchData, which re-issues checkForSuspiciousAsset (account load), scanAsset (Blockaid), and getManageAssetXDR — but only getManageAssetXDR depends on the fee. Split the XDR rebuild (keyed on fee) from the asset scan / suspicious-asset fetch (keyed on asset identity).

(b) Stale eslint-disable. The // eslint-disable-next-line react-hooks/exhaustive-deps on line 124 was appropriate for the old mount-once []. It now sits above a load-bearing 10-dep array, and fetchData (recreated every render, unmemoized) is not in the deps (it's only in the returned object). So a maintainer who later captures a new value inside fetchData gets no lint warning to add it. Either useCallback-wrap fetchData and depend on [fetchData], or keep it intentionally mount-only with an explicit comment rather than a blanket disable.

(c) Duplicate Blockaid scan. AddToken already scans the asset and stores the result:

freighter/extension/src/popup/views/AddToken/index.tsx

Lines 173 to 195 in 6255393

	const getBlockaidData = async () => {
	const [scannedAsset, overrideState] = await Promise.all([
	scanAsset(`${assetCode}-${assetIssuer}`, networkDetails),
	getBlockaidOverrideState().catch(() => null),
	]);
	// Show Blockaid warning if suspicious, malicious, or unable to scan (including debug override)
	if (
	scannedAsset &&
	(isAssetSuspicious(scannedAsset, overrideState) ||
	isAssetMalicious(scannedAsset, overrideState) ||
	shouldTreatAssetAsUnableToScan(
	scannedAsset,
	overrideState,
	networkDetails,
	))
	) {
	setBlockaidData(scannedAsset);
	setIsMaliciousAsset(isAssetMalicious(scannedAsset, overrideState));
	}
	};
	getBlockaidData();

When the SAC review mounts, useChangeTrustData scans the identical ${code}-${issuer} key again (useChangeTrustData.tsx:87); scanAsset is an uncached fetchJson, and the result isn't threaded into ChangeTrustInternal. Pass blockaidData down as a prop (or cache by code-issuer).

(d) Duplicate feeStats(). AddToken calls useNetworkFees() and passes the derived fee down as initialFee:

freighter/extension/src/popup/views/AddToken/index.tsx

Line 119 in 6255393

const { recommendedFee } = useNetworkFees();

ChangeTrustInternal calls useNetworkFees() again on mount, issuing a second server.feeStats() round-trip purely to populate the Fee pane:

freighter/extension/src/popup/components/manageAssets/ManageAssetRows/ChangeTrustInternal/index.tsx

Line 93 in 6255393

const { recommendedFee } = useNetworkFees();

Pass the parent's recommendedFee down alongside initialFee and skip the child's refetch on this path.

Net: threading blockaidData + recommendedFee from AddToken into ChangeTrustInternal, and splitting the fee-only XDR rebuild from the asset scan, removes the duplicate scan, the duplicate fee fetch, and the per-fee-edit re-scan in one move.

[piyalbasu]

A few things I noticed while manual testing:

1. Just to see what would happen, I tried adding CAS3J7GYLGXMF6TDJBBYYSE3HQ6BBSMLNUQ34T6TZMYMW2EVH34XOWMA which is the XLM SAC address on Mainnet. I got a forever loading spinner with some runtime errors in the JS

2. If you try to add a token that a user already has a trustline for, it still submits a tx. We should guard against this as it does charge a fee. I'd be okay with addressing this as a fast follow as it's a bit out of scope for what we're addressing here

3. Minor: There's some crowding happening in this menu. Not sure if this can be fixed in this PR or if this should be its own ticket as it may affect some other screens:

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 75668d8cf5

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

[chatgpt-codex-connector]

Send a failure response for failed SAC submits

When the external SAC trustline submit reaches the Failed screen, this onClose path calls rejectAndClose, which dispatches the generic reject handler; the background then receives undefined and maps the dApp addToken result to FreighterApiDeclinedError. Fresh evidence in this revision is that freighterApiMessageListener now has an explicit AddTokenResponse apiError branch for failed-then-dismissed submits, but this wiring never uses it, so dApps cannot distinguish a network/signing failure from the user declining the add-token request and metrics are also recorded as a rejection.

Useful? React with 👍 / 👎.

[leofelix077]

@piyalbasu adjusted some of the issues. like the SAC for XLM not loading, there was a hydration issue / race in some cases, so the pub key and network were empty. should work consistently now

• added the promise handling for the hanging promises. hadn't added it before was it seems like ver edge case / does not affect the final outcome (in the sense of the transaction going through). also adjusted the return codes for failed / rejected to return the proper codes

not to create a separate ticket just for a small ui change, I increased the space a bit for the settings

Screen.Recording.2026-06-30.at.11.25.47.mov

Trustline check for already added token

added an error message if trustline is already added and handling of the return message to the dapp on transaction success, not on done

Screen.Recording.2026-06-30.at.14.00.51.mov

[chatgpt-codex-connector]

Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits.
Credits must be used to enable repository wide code reviews.

[piyalbasu]

Add-SAC-token: a successful trustline can still report "user rejected" to the dApp (residual race)

Suggestion / correctness — not a merge blocker. Reachable only on a fast Done-click, but the outcome is the worst kind of wrong: the dApp is told the user declined a trustline that actually succeeded on-chain.

TL;DR: This is the un-closed remainder of the concern Codex raised earlier (resolve add-token from submit completion, not from a window-close). The loading-time "Close" case was fixed by resolving off the transaction success — but that resolution is fire-and-forget, and the "Done" button still closes the window. If the user clicks Done before the token-add round-trip finishes, the window-close path wins and the dApp hears "user rejected" for a trustline that succeeded.

Steps to reproduce:

1. From a dApp, request to add a SAC token and approve it so the trustline review appears.
2. Submit the trustline; wait for the "Success!" screen.
3. Click Done quickly (before the background token-add finishes).
4. The dApp's addToken() promise rejects with "The user rejected this request" (code -4) even though the trustline succeeded.

---

Detailed explanation (for agents)

Root cause. The dApp request is resolved off the transaction-success callback, but the call is not awaited and races the window close.

handleTrustlineTransactionSuccess fires addTokenAndClose() fire-and-forget:

freighter/extension/src/popup/views/AddToken/index.tsx

Lines 182 to 189 in 5b9161c

	// The dApp request resolves off the actual trustline transaction (fired by
	// SubmitTx's onTransactionSuccess), not the Done button — a successful
	// trustline must never report "user rejected" to the dApp. addTokenAndClose
	// records the token and resolves the request; storage is best-effort, so a
	// storage hiccup can't decline a transaction the user already approved.
	const handleTrustlineTransactionSuccess = () => {
	void addTokenAndClose();
	};

onTransactionSuccess runs it the moment the tx succeeds — i.e. at the exact moment the Done button appears — and onSuccess is wired to window.close():

freighter/extension/src/popup/views/AddToken/index.tsx

Lines 562 to 582 in 5b9161c

	if (showTrustlineReview && isSac && assetCurrency) {
	return (
	<ChangeTrustInternal
	asset={{
	code: assetCode,
	issuer: assetIssuer,
	image: assetIcon || null,
	domain: assetDomain || null,
	contract: assetCurrency.contract,
	}}
	addTrustline
	publicKey={state.data.account.publicKey}
	networkDetails={state.data.settings.networkDetails}
	onCancel={() => setShowTrustlineReview(false)}
	onTransactionSuccess={handleTrustlineTransactionSuccess}
	onSuccess={() => window.close()}
	onClose={() => setShowTrustlineReview(false)}
	initialFee={displayFee}
	isFullHeight
	/>
	);

In SubmitTransaction, the success effect calls onTransactionSuccess(), and the Done button calls onSuccess() (→ window.close()):

freighter/extension/src/popup/components/manageAssets/ManageAssetRows/ChangeTrustInternal/SubmitTx/index.tsx

Lines 177 to 191 in 5b9161c

	useEffect(() => {
	if (!isSuccess) {
	return;
	}
	if (isTrustlineSubmit) {
	emitMetric(
	addTrustline
	? METRIC_NAMES.manageAssetAddAsset
	: METRIC_NAMES.manageAssetRemoveAsset,
	{ code: asset.code, issuer: asset.issuer },
	);
	}
	onTransactionSuccess?.();
	// eslint-disable-next-line react-hooks/exhaustive-deps
	}, [isSuccess]);

freighter/extension/src/popup/components/manageAssets/ManageAssetRows/ChangeTrustInternal/SubmitTx/index.tsx

Lines 288 to 299 in 5b9161c

	onClick={async (e) => {
	e.preventDefault();
	await resetChangeTrustData({
	isHardwareWallet,
	isSuccess,
	});
	if (isSuccess) {
	onSuccess();
	} else {
	onClose();
	}
	}}

In the background, two resolvers compete and makeSafeResolve lets only the first win: rejectOnWindowClose (declined) vs the ADD_TOKEN response(true) (contractId):

freighter/extension/src/background/messageListener/freighterApiMessageListener.ts

Lines 303 to 354 in 5b9161c

	return new Promise((resolve) => {
	const safeResolve = makeSafeResolve(resolve);
	if (popup === undefined) {
	safeResolve({
	apiError: FreighterApiInternalError,
	});
	} else if (popup === null) {
	setTimeout(
	() =>
	safeResolve({
	apiError: FreighterApiDeclinedError,
	}),
	QUEUE_ITEM_TTL_MS,
	);
	} else {
	rejectOnWindowClose(popup.id!, safeResolve, {
	apiError: FreighterApiDeclinedError,
	});
	}
	const response = (addTokenResponse: AddTokenResponse) => {
	if (addTokenResponse === true) {
	safeResolve({
	contractId,
	});
	return;
	}
	// A failed-then-dismissed submit resolves with an explicit error
	// (internal, code -1) so the dApp can tell it apart from a user
	// rejection (declined, code -4).
	if (
	addTokenResponse &&
	typeof addTokenResponse === "object" &&
	"apiError" in addTokenResponse
	) {
	safeResolve({
	apiError: addTokenResponse.apiError,
	});
	return;
	}
	safeResolve({
	apiError: FreighterApiDeclinedError,
	});
	};
	(responseQueue as ResponseQueue<AddTokenResponse>).push({
	response,
	uuid,
	createdAt: Date.now(),
	});

addTokenAndClose is a createAsyncThunk → background-messaging round-trip → localStore.setItem → response(true), so it has real async latency:

freighter/extension/src/popup/helpers/useSetupAddTokenFlow.ts

Lines 76 to 99 in 5b9161c

	const addTokenAndClose = async () => {
	setIsTokenAdded(false);
	setSubmitError("");
	try {
	const addTokenResp = await dispatch(addTokenFn({ uuid }));
	const rejectedMessage = getThunkErrorMessage(addTokenResp);
	if (rejectedMessage) {
	await emitMetric(METRIC_NAMES.tokenFailedApi);
	setSubmitError(rejectedMessage);
	return false;
	}
	await emitMetric(METRIC_NAMES.tokenAddedApi);
	setIsTokenAdded(true);
	} catch (e) {
	console.error(e);
	await emitMetric(METRIC_NAMES.tokenFailedApi);
	setSubmitError("Failed to add token. Please retry or cancel.");
	return false;
	}
	return true;
	};

If window.close() fires before that round-trip completes, rejectOnWindowClose resolves first with FreighterApiDeclinedError and the later response(true) is dropped by makeSafeResolve — the exact invariant the code comment at index.tsx:182-186 says must never happen.

Deterministic repro. In a unit/integration test, render the SAC success state, invoke the Done handler synchronously in the same tick that onTransactionSuccess fired (before flushing the addTokenAndClose promise), and assert the dApp promise resolved to FreighterApiDeclinedError rather than { contractId }.

Suggested fixes (increasing depth):

1. Cheap guard: have Done await the in-flight add before closing (disable/spinner the Done button until addTokenAndClose resolves), so the success resolution always precedes window.close().
2. Root cause: once a trustline submit has succeeded, make the window-close fallback resolve with success/contractId (or suppress rejectOnWindowClose) rather than FreighterApiDeclinedError, so no close ordering can turn an approved+successful trustline into a rejection.

[piyalbasu]

The ADD_TOKEN_FAILURE / apiError failure-response plumbing is entirely unreachable

Suggestion / dead code + unshipped feature — not a merge blocker. Corroborates and extends Codex's failed-SAC-submit finding: confirming it's real, and noting the entire failure-signal mechanism (not just the one call site) is wired to nothing.

TL;DR: This PR adds a whole path for telling a dApp "the add-token failed internally" (distinct from "the user declined") — a new message type, a new response shape, and a new branch to handle it. But nothing ever sends that message or produces that response shape, so the branch can never run. On any failed-then-dismissed SAC submit the dApp still gets a plain "user rejected," and the new code ships dark.

---

Detailed explanation (for agents)

Root cause. Three new pieces were added but never connected end-to-end:

1. The enum + message type:
   

   freighter/@shared/constants/services.ts

   Line 18 in 5b9161c

   ADD_TOKEN_FAILURE = "ADD_TOKEN_FAILURE",

   
   

   freighter/@shared/api/types/message-request.ts

   Lines 226 to 229 in 5b9161c

   	export interface AddTokenFailureMessage extends BaseMessage {
   	type: SERVICE_TYPES.ADD_TOKEN_FAILURE;
   	uuid: string;
   	}

2. The apiError variant of AddTokenResponse and the listener branch that resolves with it:
   

   freighter/@shared/api/types/message-request.ts

   Lines 50 to 52 in 5b9161c

   	export type AddTokenResponse =
   	| boolean
   	| { apiError: { code: number; message: string } };

   
   

   freighter/extension/src/background/messageListener/freighterApiMessageListener.ts

   Lines 331 to 343 in 5b9161c

   	// A failed-then-dismissed submit resolves with an explicit error
   	// (internal, code -1) so the dApp can tell it apart from a user
   	// rejection (declined, code -4).
   	if (
   	addTokenResponse &&
   	typeof addTokenResponse === "object" &&
   	"apiError" in addTokenResponse
   	) {
   	safeResolve({
   	apiError: addTokenResponse.apiError,
   	});
   	return;
   	}

Why it's unreachable:

• The responseQueue callback is only ever invoked with a boolean — handlers/addToken.ts calls tokenResponse.response(!response.error), never an object:
  

  freighter/extension/src/background/messageListener/handlers/addToken.ts

  Lines 62 to 70 in 5b9161c

  	if (tokenResponse && typeof tokenResponse.response === "function") {
  	tokenResponse.response(!response.error);
  	if (response.error) {
  	return { error: response.error };
  	}
  	return {};
  	}

  
  So the typeof addTokenResponse === "object" && "apiError" in addTokenResponse branch is dead.
• SERVICE_TYPES.ADD_TOKEN_FAILURE / AddTokenFailureMessage have no dispatcher and no handler — a repo-wide search finds them only at their definitions; popupMessageListener.ts has no case for it.

Net: a failed-then-dismissed SAC submit still routes through rejectAndClose / rejectOnWindowClose → FreighterApiDeclinedError (code -4), which is exactly the gap Codex's discussion_r3499356192 describes. The plumbing added here was presumably intended to fix it but was never hooked up.

Suggested fixes (increasing depth):

1. Wire it: on a failed-then-dismissed submit, dispatch ADD_TOKEN_FAILURE (with uuid), add a popupMessageListener case that pulls the queued response and calls it with { apiError: FreighterApiInternalError }, and make the window-close fallback return internal-error (not declined) once a submit has failed.
2. Or remove it: if this is out of scope for now, delete the enum value, AddTokenFailureMessage, the apiError response variant, and the listener branch so no dead surface ships — and track the dApp-distinguishable-error work as a follow-up.

[piyalbasu]

Add-token: a storage-write failure reports success to the dApp on the SEP-41 path

Suggestion / correctness regression — not a merge blocker. Narrow (only when the local storage write itself throws), but on that path the dApp is told the token was added when nothing was persisted.

TL;DR: When persisting the added token fails, the code now swallows the error and returns success — justified by "the trustline already succeeded." That reasoning holds for SAC/classic assets (which submit a trustline first), but SEP-41 tokens have no trustline: the storage write is the whole operation. So a failed write on the SEP-41 path tells the dApp "added" while the token silently isn't there on reload. Before this PR that case returned an error.

---

Detailed explanation (for agents)

Root cause. The catch now returns a success-shaped result ({ accountTokenIdList }, no error) when localStore.setItem throws:

freighter/extension/src/background/messageListener/helpers/add-token-contract-id.ts

Lines 41 to 55 in 5b9161c

	try {
	accountTokenIdList.push(tokenId);
	await localStore.setItem(TOKEN_ID_LIST, {
	...tokenIdsByNetwork,
	[network]: {
	...tokenIdList,
	[keyId]: accountTokenIdList,
	},
	});
	} catch (error) {
	console.error(error);
	// Do not reject a dApp addToken request after the user has already
	// approved and submitted a successful trustline transaction.
	return { accountTokenIdList };
	}

The comment justifies this for the case where "the user has already approved and submitted a successful trustline transaction" — but the same helper serves the SEP-41 direct-approval path, which submits no trustline. handleApprove → addTokenAndClose → ADD_TOKEN reaches this helper with no prior transaction:

freighter/extension/src/popup/views/AddToken/index.tsx

Lines 788 to 802 in 5b9161c

	onClick={() => {
	if (isTokenAdded) {
	window.close();
	return;
	}
	clearSubmitError();
	if (hasExistingTrustline || isTrustlineCheckLoading) {
	return;
	}
	if (isSac) {
	setShowTrustlineReview(true);
	return;
	}
	handleApprove();
	}}

freighter/extension/src/popup/helpers/useSetupAddTokenFlow.ts

Lines 76 to 111 in 5b9161c

	const addTokenAndClose = async () => {
	setIsTokenAdded(false);
	setSubmitError("");
	try {
	const addTokenResp = await dispatch(addTokenFn({ uuid }));
	const rejectedMessage = getThunkErrorMessage(addTokenResp);
	if (rejectedMessage) {
	await emitMetric(METRIC_NAMES.tokenFailedApi);
	setSubmitError(rejectedMessage);
	return false;
	}
	await emitMetric(METRIC_NAMES.tokenAddedApi);
	setIsTokenAdded(true);
	} catch (e) {
	console.error(e);
	await emitMetric(METRIC_NAMES.tokenFailedApi);
	setSubmitError("Failed to add token. Please retry or cancel.");
	return false;
	}
	return true;
	};
	const handleApprove = async () => {
	setIsConfirming(true);
	if (hasPrivateKey) {
	await addTokenAndClose();
	} else {
	setIsPasswordRequired(true);
	}
	setIsConfirming(false);
	};

The handler then maps a missing error to a success signal for the dApp:

freighter/extension/src/background/messageListener/handlers/addToken.ts

Lines 62 to 70 in 5b9161c

	if (tokenResponse && typeof tokenResponse.response === "function") {
	tokenResponse.response(!response.error);
	if (response.error) {
	return { error: response.error };
	}
	return {};
	}

So on the SEP-41 path, if setItem throws, addTokenWithContractId returns success → response(true) → the dApp's addToken() resolves with { contractId }, but the pushed entry only ever lived in the in-memory accountTokenIdList and was never persisted; it's gone on next load. Pre-PR the catch returned { error: "Failed to subscribe to token details" }, so this is a behavior regression for the no-trustline case.

Deterministic repro. Unit-test addTokenWithContractId for a non-SAC (SEP-41) contract with localStore.setItem stubbed to throw; assert the return omits error (current) vs. should surface an error so the dApp isn't told success. (The added test does not return an error when persisting fails after a successful trustline pins the SAC-justified behavior but doesn't distinguish the no-trustline path.)

Suggested fix. Gate the error-swallow on whether a trustline transaction actually occurred — e.g. pass a flag (or split the helper) so the SAC/classic post-trustline path swallows a persistence failure (to avoid declining an on-chain success), while the SEP-41 path surfaces the setItem failure as an error to the dApp.

[leofelix077]

@piyalbasu taking a look at the new comments now. will report back soon