Fix SEP-41/SAC add-token regressions (feeds into #2869)#2885
Conversation
…idation of trustline add for SAC
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
There was a problem hiding this comment.
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_FAILUREplumbing, 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.
piyalbasu
left a comment
There was a problem hiding this comment.
These changes look good. I'm merging into the main feature branch and will be doing a more thorough review
b314de9
into
stellar:feat/addtoken-sac-changetrust-review
Summary
I lost write access to
stellar/freighter(now read-only), so I can no longer push directly to my PR branchfeat/addtoken-sac-changetrust-review(used by #2869). This PR targets that branch directly (notmaster) 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:
ADD_TOKEN_FAILUREplumbingTest plan
tsc --noEmitcleanyarn jest)yarn test:e2e), including a new deterministic regression test for the Done/race issue🤖 Generated with Claude Code