Skip to content

refactor(sell): sanitize user-facing error messages - #2384

Open
wired-pasteque wants to merge 15 commits into
developfrom
refactor-sell-errors
Open

refactor(sell): sanitize user-facing error messages#2384
wired-pasteque wants to merge 15 commits into
developfrom
refactor-sell-errors

Conversation

@wired-pasteque

Copy link
Copy Markdown
Collaborator

No description provided.

@wired-pasteque wired-pasteque self-assigned this Jun 29, 2026
@wired-pasteque

This comment was marked as outdated.

@claude

This comment was marked as outdated.

@wired-pasteque
wired-pasteque changed the base branch from 1895-sanitize-all-user-facing-error-messages to develop July 7, 2026 06:10
@wired-pasteque wired-pasteque linked an issue Jul 7, 2026 that may be closed by this pull request
59 tasks
@wired-pasteque
wired-pasteque marked this pull request as ready for review July 7, 2026 06:55
@ethicnology

Copy link
Copy Markdown
Member

conflict management required @wired-pasteque

@ethicnology

Copy link
Copy Markdown
Member

Conflict management required @wired-pasteque

Wrap the throwing shared-core GetWalletUtxosUsecase in a sell-owned
use-case returning Result<List<WalletUtxo>, SellFailure>.
Owns the pre-order estimation (rate, required amount, sufficient-balance
rule, absolute fee) and maps every infra throw to a sealed SellFailure.
Recomputes the absolute payin fee on coin-selection / RBF changes,
returning Result<int, SellFailure>.
Loads the exchange user summary and bitcoin unit for the amount-input
screen, mapping infra throws to a sealed SellFailure.
Build+sign+broadcast the payin as a single boundary. Derives the txid
before broadcast and treats labelling as a best-effort post-broadcast
side-effect, so a bookkeeping failure never demotes a completed payin to
a SellSendPaymentFailure (no double-spend retry). The raw exception is
never forwarded to logs or failure state (type + trace only). Txid
derivation is injectable so the money path is unit-testable.
Fetches the latest sell order while polling, mapping both an infra throw
and an unexpected order type to a sealed SellFailure.
Move create/refresh use-cases into domain/usecases/ and rewire SellBloc
to switch on Ok/Err from the eight sell use-cases instead of running its
own try/catch. The presentation layer now holds zero try/catch; the data
boundary is the use-case, as the convention requires.

This activates two behaviour fixes carried by the new use-cases:
- payin is treated as succeeded once broadcast succeeds (labelling
  failure no longer reported as a payment failure);
- order-polling failures surface to the user instead of being swallowed.
The key was added to 23 locales when the sell send-payment failure was
introduced; complete the remaining four so every language is translated.
Replace a hardcoded 'Loading...' with the existing sellLoadingGeneric
key, and rename the private _SellError error-card widget to
_SellErrorCard to avoid confusion with the SellFailure family.
@wired-pasteque

Copy link
Copy Markdown
Collaborator Author

@claude, review the fixes related to issue #1895

@claude

This comment was marked as outdated.

@wired-pasteque

Copy link
Copy Markdown
Collaborator Author

@claude, check the fixes

@claude

claude Bot commented Aug 3, 2026

Copy link
Copy Markdown

Claude finished @wired-pasteque's task in 5m 33s —— View job


Review of the #1895 fixes ✅

  • Gather context (issue Sanitize All User-Facing Error Messages #1895, PR diff vs origin/develop, latest fix commits)
  • Review new sell use-cases and SellFailure error sanitization
  • Review bloc/UI wiring and localization changes
  • Review tests
  • Post review feedback

Verdict

The fixes hold up. The core goal of #1895 — no raw exception text ever reaching the UI — is now enforced structurally: every sell error crosses the domain boundary as a sealed SellFailure, and the only path to a user-visible string is SellFailureL10n.toTranslated (lib/features/sell/presentation/sell_failure_l10n.dart:7), which maps exclusively to localized keys. The old leak paths on develop (e.g. SellError.unexpected(message: e.toString()) rendered verbatim by toTranslated) are gone, and the unexpected: (message) => message arm was removed from sell_error.dart.

The latest fix commits, checked individually

a78be2f (poll UX, broadcast diagnosability, dead fee field)

  • Poll/refresh errors are now swallowed-and-logged instead of painted on screen, and successful polls clear a stale error (sell_bloc.dart:257, sell_bloc.dart:349). This is the right call for a 5-second background timer — a transient blip no longer leaves a persistent error banner.
  • Logging restored to full error: e instead of e.runtimeType (confirm_sell_payin_usecase.dart:109), so Sentry/log diagnosability is back while the user-facing failure still carries only the runtime type. The test at confirm_sell_payin_usecase_test.dart:186 explicitly asserts the raw exception text never reaches logMessage.
  • The dead updatedAbsoluteFees return value and the CalculateBitcoinAbsoluteFeesUsecase dependency were removed cleanly from the confirm use-case, locator, and tests — no leftovers found.

8d3e1b03d (l10n) — verified all 27 .arb locales now contain sellErrorSendPayment, sellLoadingGeneric, sellErrorFeesNotCalculated, sellErrorLoadUtxos, and the {amount} placeholder in sellBelowMinAmountError / sellAboveMaxAmountError / sellInsufficientBalanceError. Removed keys (sellOrderNotFoundError, sellErrorPrepareTransaction, sellErrorRecalculateFees, etc.) have no remaining references in lib/.

82e82891d / 6038dbce6 (use-case routing) — all eight use-cases return Result<_, SellFailure>, log at the boundary, and the bloc no longer touches repositories or exceptions directly. Locator wiring matches. The branch is also 0 commits behind origin/develop, so the earlier merge-conflict issue appears resolved.

Improvements worth calling out

  • ConfirmSellPayinUsecase now derives the txid before broadcasting and treats labelling as best-effort (confirm_sell_payin_usecase.dart:82-84,113-115). On develop, a labelling throw after a successful broadcast surfaced as a user-facing error even though the money had moved — an invitation to a double-spend retry. The new ordering plus the dedicated test (confirm_sell_payin_usecase_test.dart:141) closes that.
  • Failure display was added to the initial screen (sell_screen.dart), which previously dropped StartSell errors silently.

Minor observations (non-blocking, mostly pre-existing)

  1. Polling timer never stops after a confirm-driven success. _onSendPaymentConfirmed's Ok branch transitions to the success state without _stopPolling() (sell_bloc.dart:310-318), so the 5s timer keeps firing no-op SellPollOrderStatus events until the bloc closes (_onPollOrderStatus early-returns on non-payment states without cancelling). Same behavior existed on develop, but since a78be2f was about poll UX, adding _stopPolling() there would be a cheap win. Fix this →
  2. Success emit uses state captured before the broadcast. In _onSendPaymentConfirmed, the success state is built from sellPaymentState captured at handler entry (sell_bloc.dart:313-317); any sellOrder updates emitted by polls during the broadcast + 5s window are discarded. Pre-existing pattern, harmless in practice, but worth knowing.
  3. Detached doc comments. Several new use-cases have a blank line between the /// doc and the class declaration (e.g. confirm_sell_payin_usecase.dart:33-35, estimate_sell_payin_fees_usecase.dart:17-22), which detaches the comment from the class for dartdoc/IDE hover. Deleting the blank line fixes it.
  4. EstimateSellPayinFeesUsecase's balance check (estimate_sell_payin_fees_usecase.dart:63) doesn't account for fees, so an exact-balance wallet passes the check and later fails with the generic "fees not calculated" message. Pre-existing behavior, just noting it survived the refactor.

CI (Analyze and Test) was still in progress at review time: run 30841148861. Generated freezed/l10n files are gitignored and rebuilt in CI, so the SellErrorSellFailure state change should regenerate cleanly.

--- · Branch

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.

Sanitize All User-Facing Error Messages

2 participants