Skip to content

Fix SEP-41/SAC add-token regressions (feeds into #2869)#2885

Open
leofelix077 wants to merge 2 commits into
stellar:feat/addtoken-sac-changetrust-reviewfrom
leofelix077:feat/addtoken-sac-changetrust-review
Open

Fix SEP-41/SAC add-token regressions (feeds into #2869)#2885
leofelix077 wants to merge 2 commits into
stellar:feat/addtoken-sac-changetrust-reviewfrom
leofelix077:feat/addtoken-sac-changetrust-review

Conversation

@leofelix077

Copy link
Copy Markdown
Contributor

Summary

I lost write access to stellar/freighter (now read-only), so I can no longer push directly to my PR branch feat/addtoken-sac-changetrust-review (used by #2869). This PR targets that branch directly (not master) so a maintainer can merge it in without needing to grant me access back.

Contains one commit addressing Piyal's latest review round on #2869:

  • Fix a residual race where a successful SAC trustline could still report "user rejected" to the dApp if Done was clicked before the add-token round-trip finished
  • Remove dead/unreachable ADD_TOKEN_FAILURE plumbing
  • Fix a SEP-41 storage-write failure incorrectly reporting success to the dApp
  • Make SEP-41 a true one-step flow (submit + auto-close, matching pre-SAC-review behavior) instead of a second "Done" click
  • Scope the "already has a trustline" message to SAC only (SEP-41 has no trustline)
  • Allow re-adding SEP-41 tokens multiple times (local-only, no cost) instead of blocking on a prior add
  • New Playwright e2e regression test + updated unit tests

Test plan

  • tsc --noEmit clean
  • Unit tests pass (yarn jest)
  • e2e tests pass (yarn test:e2e), including a new deterministic regression test for the Done/race issue

🤖 Generated with Claude Code

Copilot AI review requested due to automatic review settings July 2, 2026 15:10
@chatgpt-codex-connector

Copy link
Copy Markdown

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.

Copilot AI 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.

Pull request overview

This PR fixes regressions in the extension’s “Add Token” flow introduced during the SAC disclosure work (feeds into #2869), focusing on correct dApp resolution semantics and restoring the intended 1-step SEP-41 vs 2-step SAC UX.

Changes:

  • Fixes the SAC “Done” race by awaiting the in-flight add-token resolution before closing the popup.
  • Makes SEP-41 add-token a true one-step flow (Confirm submits + auto-closes on success) and allows re-adding SEP-41 tokens repeatedly (local-only).
  • Removes dead ADD_TOKEN_FAILURE plumbing, tightens background error surfacing for SEP-41 persistence failures, and adds/updates unit + Playwright regression tests.

Reviewed changes

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

Show a summary per file
File Description
extension/src/popup/views/AddToken/index.tsx SAC Done now awaits add-token round-trip; SEP-41 no longer blocks on prior local add; trustline messaging scoped to SAC.
extension/src/popup/views/AddToken/tests/AddToken.test.tsx Updates unit coverage for SAC Done semantics and SAC/SEP-41 duplicate handling.
extension/src/popup/locales/pt/translation.json Removes now-unused success strings.
extension/src/popup/locales/en/translation.json Removes now-unused success strings.
extension/src/popup/helpers/useSetupAddTokenFlow.ts Removes isTokenAdded; SEP-41 approve closes only on success.
extension/src/popup/helpers/tests/useSetupAddTokenFlow.test.ts Updates tests for one-step close-on-success behavior and fixes confirmPassword mocking.
extension/src/background/messageListener/helpers/add-token-contract-id.ts Persistence failures now surface an error (caller decides whether to swallow).
extension/src/background/messageListener/helpers/tests/add-token-contract-id.test.ts Updates expectations to ensure persistence failures are surfaced.
extension/src/background/messageListener/handlers/addToken.ts Swallows persistence errors only for SAC (trustline-backed), surfaces for SEP-41.
extension/src/background/messageListener/freighterApiMessageListener.ts Simplifies addToken response handling to boolean success/failure.
extension/src/background/messageListener/tests/addToken.test.ts Adds coverage for SEP-41 persist-failure surfacing vs SAC persist-failure swallowing.
extension/e2e-tests/integration-tests/freighterApiIntegration.test.ts Updates SEP-41 to expect auto-close and adds deterministic SAC Done/race regression test.
@shared/constants/services.ts Removes ADD_TOKEN_FAILURE.
@shared/api/types/message-request.ts Removes AddTokenFailureMessage and narrows AddTokenResponse to boolean.

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

Comment thread extension/src/popup/views/AddToken/index.tsx
Comment thread extension/src/background/messageListener/handlers/addToken.ts Outdated
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.

2 participants