refactor(errors): establish user-facing error sanitization pattern (#1895) - #2293
Merged
Conversation
59 tasks
This comment was marked as outdated.
This comment was marked as outdated.
ethicnology
force-pushed
the
1895-sanitize-all-user-facing-error-messages
branch
from
June 18, 2026 07:02
b83bad3 to
4b853f9
Compare
59 tasks
Replace raw e.toString() in state and UI with a sealed ImportWatchOnlyError family whose variants own their localized toTranslated(BuildContext). Foreign import failures are mapped and logged at the usecase boundary; the catch-all shows a generic message. Also renames the usecase call() to execute() to match convention.
Replace the hardcoded-English BullException errors and the Exception?-typed state with sealed BroadcastSignedTxError and TransactionReviewError families that own their localized toTranslated. Scan/NFC/broadcast/parent-fetch failures are mapped and logged at the owning layer; the UI no longer renders state.error.toString() or a raw catch-all message.
LabelError becomes a sealed family with per-variant toTranslated. The unexpected catch-all now returns a generic localized message instead of the raw exception text, and the label usecases log the technical reason at the mapping site.
Add the new import_watch_only and broadcast_signed_tx error keys plus the regenerated labelErrorUnsupportedType and broadcast failure copy across all 27 locales. Remove the retired labelErrorUnexpected, the stale {type} placeholder, and the orphaned coreScreensUnexpectedError. Non-en/fr translations are AI-generated and pending native-speaker review.
- Result is now generic over its failure type (Result<T, F extends Failure>) so consumers no longer cast (drops `failure as PinCodeError`); add fold/map/mapErr helpers. - Move the Failure base to lib/core/failures/ (Flutter-free). - Rename PinCodeError to a pure sealed PinCodeFailure in domain/; Error is reserved for dart:core bugs. - Move toTranslated into a presentation extension (pin_code_failure_x.dart) so domain and data stay Flutter-free. - Repository maps exceptions to failures at the boundary; the bloc switches on Result and stores the typed failure in state.
Establish the sanitized error standard for #1895 across both rulebooks. ARCHITECTURE.md (Error handling section, glossary, feature template, checklist, enforcement table) and AGENTS.md (rule #11, naming, files table, rule #15) now specify: - Three distinct kinds: Exception (thrown infra, caught at the boundary), Error (dart:core bug, never caught), Failure (modeled recoverable value in domain/). Name domain failures <Feature>Failure, never <Feature>Error. - Result<T, F extends Failure> (Ok/Err, generic over F, @useResult, fold/map/mapErr); throw only for dart:core bugs. - Failures are Flutter-free in domain/; translation is a presentation extension (<feature>_failure_x.dart), never a method on the failure. - Repository is the one try/catch boundary (or the feature use-case when wrapping a shared core repo); bloc switches and holds the typed failure. - Cross-cutting modes via shared CoreFailure in lib/core/failures/. - #1895 migration is sanctioned and staged; existing BullException, *Error naming, on-error translation and throw-based code is legacy-to-converge.
- Rename ImportWatchOnlyError to a pure sealed ImportWatchOnlyFailure in domain/; Error is reserved for dart:core bugs. - Move toTranslated into a presentation extension (import_watch_only_failure_l10n.dart) so domain and data stay Flutter-free. - Use-cases wrap the throwing core WalletRepository, map the raw reason to ImportFailedFailure at the boundary, and return Result<Wallet, F>. - The cubit switches on Result and stores the typed failure in state; no exception text reaches the UI. - Update the descriptor use-case test to assert the sanitized failure on bad input.
- Replace the two error families with pure sealed BroadcastSignedTxFailure and TransactionReviewFailure in domain/; Error is reserved for dart:core bugs. - Move toTranslated into presentation extensions (*_failure_l10n.dart) so domain stays Flutter-free. - BuildReviewableTransactionUsecase maps the foreign TransactionPortError at the boundary and returns Result; add its unit test. - State carries the typed failure; the cubits switch on Result. The broadcast cubit keeps try/catch only as the data boundary for direct QR/NFC/launchUrl calls. - Remove the legacy broadcast_signed_tx_error.dart and domain_errors.dart.
- Rename LabelError to a pure sealed LabelFailure in domain/; move toTranslated into a presentation extension (label_failure_l10n.dart). - The four facade use-cases map the throwing repository port at the boundary and return Result; add their unit tests. - Facade keeps reads best-effort (degrade to empty + log, never throw) and exposes Result on writes (store/trash), so the ~15 wallet read sites are untouched. - Fix the real leak: the BIP329 cubit no longer renders raw 'Export failed: $e'; it logs the reason and emits a sanitized LabelUnexpectedFailure. - transaction_details_cubit switches on the write Results (required for the facade signature change to compile).
- Rename pin_code_failure_x.dart to pin_code_failure_l10n.dart so the translation-extension file name states intent; sets the naming the rest of the rollout follows.
- Update the translation-extension references from <feature>_failure_x.dart to <feature>_failure_l10n.dart across the architecture and agent docs, matching the shipped code. - Refresh the member-ordering example off the legacy UnexpectedLabelError / toTranslated-on-the-error pattern that the sanitization rollout replaces.
- Remove LabelNotFoundFailure, UnsupportedLabelTypeFailure and SystemLabelCannotBeDeletedFailure — modeled and translated but never constructed anywhere. - Remove their now-orphaned l10n keys (labelErrorNotFound, labelErrorUnsupportedType, labelErrorSystemCannotDelete) from the arb files. - LabelUnexpectedFailure, the catch-all that is actually produced, is the only remaining variant.
- Add ParseWatchOnlyInputUsecase that wraps the throwing satoshifier parser at the boundary and returns Result<WatchOnlyWalletEntity, ImportWatchOnlyFailure>, mapping a parse error to InvalidFormatFailure. - The cubit injects it and switches on the Result, so parsePastedInput no longer holds a try/catch. - Add a use-case test asserting the sanitized failure on unparseable input.
- Assert ImportWatchOnlyXpubUsecase maps a foreign repository failure to ImportFailedFailure without leaking the raw exception, and returns Ok on success.
- Remove ImportWatchOnlyUnexpectedFailure: it was declared and translated but never constructed (the use-cases map every throw to ImportFailedFailure), consistent with dropping the unused LabelFailure variants. - ImportFailedFailure is import's effective catch-all.
- Best-effort reads no longer log twice: the facade drops its read-fold log.warning since the use-case already logs the failure once with the stack trace. - deleteTransactionNote returns its Result so the labels table item can surface a sanitized message on a failed delete instead of silently keeping the note. - Export label_failure_l10n from the facade so consumers translate LabelFailure; drop the now-redundant direct import in labels_widget.
- Replace the fabricated LabelFetchFailure example (and its contrived toString) with the Amount value object already used in ARCHITECTURE.md. Failures are field+constructor only now, so they don't illustrate the methods group; a value object shows fields -> constructor -> method without inventing a fake type.
…rors refactor(bip85_entropy): sanitize errors with sealed failures and a Result boundary
…-errors refactor(mempool_settings): sanitize errors with sealed failures and a Result boundary
…rors refactor(all_seed_view): sanitize user-facing error messages
Resolved conflicts:
- lib/core/fees/data/fees_datasource.dart: kept the Result-based active mempool server lookup (.fold) from this branch, since GetActiveMempoolServerUsecase returns Result<MempoolServer, MempoolFailure> here.
- 27 localization/app_*.arb files: kept this branch's sanitized error strings (no raw {reason}/{error} placeholders) and added develop's 166 new keys (coins/UTXO, bitbox bluetooth, logs viewer, labels, etc.).
- test/core_test/fees/fees_datasource_test.dart (new in develop): wrapped the settings-repo mock in Ok() to match the Result-based fetchByNetwork signature.
Regenerated l10n + build_runner outputs. flutter analyze clean; full test suite (483 tests) passes.
The Result migration destructured the usecase result as `mnemonic`, shadowing the outer default-wallet mnemonic the verification re-derivation relies on. Rename to `resultMnemonic` (as the sibling test already does) so the direct BIP85 derivation uses the same seed the usecase derives from.
…user-facing-error-messages # Conflicts: # lib/features/import_watch_only_wallet/presentation/scan_watch_only_screen.dart
…errors refactor(import_mnemonic): sanitize user-facing error messages
…rrors refactor(replace_by_fee): sanitize user-facing error messages
ethicnology
marked this pull request as ready for review
June 25, 2026 17:02
i5hi
approved these changes
Jun 27, 2026
wired-pasteque
added a commit
that referenced
this pull request
Aug 24, 2026
Four points where the ledger implementation drifted from the standard in #2293: The failure family moves to domain/ and the exception family to data/, matching the layer each word belongs to — a Failure is a domain value, an Exception is thrown infra. The catch-all now returns the shared oopsSomethingWentWrong instead of a ledger-specific "unknown error" string, so the redundant key is gone from all 27 locales. ConnectionTypeNotInitialized no longer maps to the generic failure. The transports are nullable and initialized during scan, so this fires on a reachable path and means "no connection available" — which is actionable, unlike "Oops, something went wrong". The APDU status word is only read when it is labelled (0x6985, sw=6985). The previous pattern matched any four hex-ish characters, so "timeout after 6985 ms" was reported to the user as "you rejected the operation on the device". Also drops the last `dynamic` from the operation seam and closes the repository contract with `abstract interface class`.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
refactor(errors): establish the user-facing error sanitization pattern (#1895)
This PR is the anchor for #1895 — Sanitize All User-Facing Error Messages. It
fixes the leak in the first features and defines the error standard the rest of
the codebase converges to. The standard, snippets, and rollout are below.
Why
User-facing flows rendered raw exception text into the UI —
e.toString(), noderejection reasons, BDK/Electrum/NFC internals. Both a UX problem and an
information leak. Root causes: a legacy
BullExceptionbase whose hardcodedEnglish message reached users; widespread
catch (e) { emit(error: e.toString()) };~219 exception classes and ~171
throwsites with no enforced "never reaches theUI" boundary.
The standard
Three words, three jobs — kept strictly apart:
ExceptionError(dart:core)Failuredomain/Err, translated by the UIError propagation by layer
Failure/Result.try/catches. Catches the foreignException, logs the raw reason (log.*/ Sentry), maps it to a domainFailure, returnsResult<T, F>. (When a feature wraps a shared core repo that still throws, this mapping happens in the feature's use-case instead — the first layer the feature owns.)try/catch(unless it is the owning boundary above). Forwards a singleResult, or composes several with an explicit short-circuit. ReturnsResult<T, F>.Resultwith an exhaustiveswitch.Ok→ success state;Err→ store the typedFailurein state. Notry/catch, no cast.failure.toTranslated(context). Never sees raw text, never switches on the type.A domain
Failurein the bloc is not a leak: dependencies point inward(presentation → domain → data), so presentation consuming a domain type is
correct — same as a bloc holding a
Wallet. The leak we prevent is a dataexception reaching the bloc, which the repository's mapping stops.
File layout (failures in domain, translations in presentation)
The repository constructs the failure, so its type lives where the data layer can
reach it —
domain/— and stays Flutter-free. Translation needsBuildContext,so it lives in a presentation extension, the only place that imports Flutter.
domain/anddata/stay Flutter-free and unit-testable without bindings.Exhaustiveness is still compiler-enforced: a
switchover a sealed type fails tocompile when a variant is added, wherever the switch lives.
Building blocks
1.
Result<T, F>— generic over the failure type (core)@useResult(package:meta) onResult-returning methods so a discarded result warns.2.
Failurebase + sharedCoreFailure(pure)Failureis an openabstractbase (it spans every feature). Cross-cutting modes— recurring across the audit (network, storage-locked, not-found, timeout, auth,
device, insufficient-funds) — live once in a
sealedCoreFailure, composed byfeatures instead of redefined 48 times.
3. Feature failure — pure, in
domain/4. Translation — presentation extension (the only Flutter import)
5. Repository — the boundary (catch → log raw → map →
Result)6. Use-case — forward, or compose with short-circuit (no
try/catch)7. Bloc — exhaustive
switch, no cast; domain failure stored in state8. UI — translate only
Conventions (mandatory)
failure.toTranslated(context)only.e.toString()/e.messageis forlog.*/ Sentry exclusively.try/catchlives only at the data boundary — the feature's repository, or its use-case when wrapping a shared core repo. Above it:switch.sealed, one closed family per feature, declared indomain/<feature>_failure.dart, Flutter-free. Cross-cutting modes come fromCoreFailure.<Feature>Failure; baseFailure.Erroris reserved fordart:corebugs only — never a domain family.Exceptiononly for thrown infra.presentation/<feature>_failure_l10n.dart) — the only placeBuildContext/flutterappears for failures.<Feature>UnexpectedFailure) carriesString? logMessage— logs only; the raw reason is logged at the boundary, the field is secondary (equality/debug). Keep the typeString?consistently.AGENTS.md).Result<T, F>is generic over the failure type (noascasts); annotate returning methods@useResult.Rollout plan (#1895)
This is a sanctioned, staged migration — it supersedes the prior "don't
mass-migrate exception code" guidance for the scope of #1895.
Do not migrate the high-fan-out core repos first (WalletRepository ~30
dependents, Exchange ~19, Blockchain/Swap ~12, Payjoin ~12) — that turns
incremental into big-bang. Adapt at the feature boundary: a feature's own
repo/use-case wraps the still-throwing core call locally. Core repos move last.
Sequencing
Failurebase, genericResult<T, F>+fold/map/mapErr, theCoreFailureset.status_check,legacy_seed_view,psbt_flow,all_seed_view,address_view,bip85_entropy,dca,electrum_settings,import_mnemonic,mempool_settings,onboarding,receive,sell,send,swap.ledger. (labelsis done — see below.)send,swap,wallet.Core / package failures (melos phase — not in #1895's feature sweep)
lib/coreis shared infrastructure with nopresentation/— and post-melos itbecomes packages, which by rule never export a bloc or screen. So failures that
originate in core split two ways, and translation can't live in core.
Where they live:
lib/core/wallet,lib/core/exchange, …) — that module owns its family in its own domain, exactly like a feature:lib/core/<domain>/domain/<domain>_failure.dart(WalletFailure,ExchangeFailure, …).sealed CoreFailureinlib/core/failures/(failure.dartbase +core_failure.dart).How they're translated (core has no
BuildContext):<Feature>FailureviamapErr, then its presentation extension translates. Core stays Flutter-free; a core failure never reaches a bloc untranslated.CoreFailureL10nextension in a presentation-capable spot every consumer imports (todaylib/core/widgets/; post-melos the app or a ui package). One definition avoids copying the sameswitchinto N features — and the rare co-import ambiguity when two same-member extensions land in one file. Theswitchstays exhaustive-checked because allCoreFailurevariants live in one library.Melos-forward: the rule survives the core→packages move unchanged — a domain/data package has no presentation, so whoever owns the
BuildContext(the app, a feature, or a ui package) owns the extension; the package failure stays pure.Scope: lands with the core→packages migration, not in #1895. This PR and the
feature sweep sanitize feature-facing errors now; core/package failure families
plus the shared extension are the next phase.
Feature inventory (audit summary)
coinsis empty. Effort is a fast-scan estimate — verify the L-tier before scheduling.Already sealed + translated (finish, don't rebuild):
pin_code,withdraw,buy,recoverbull,recoverbull_google_drive,replace_by_fee,fund_exchange,pay,broadcast_signed_tx,import_watch_only,labels—plus core-level
bitbox,ledger(sealed but missing the translation layer).Top three (deep chains + many variants):
swap(15+ variants, 5–7 callchain),
send(8+ variants, ~7 steps),wallet(12+ variants, 20+ use-cases).broadcast_signed_txis the reference this PR ships and is now complete: thelegacy
errors.dartis gone, the domain/presentation split and the*Failurerename are done, and the cubit emits typed sanitized failures. It keeps
try/catchonly as the data boundary for direct QR/NFC/SDK scanning, which the standard
permits (no use-case sits beneath those calls).
What this PR lands
broadcast_signed_tx,import_watch_only,labelsfully migrated to thestandard: sealed
<Feature>Failureindomain/(pure Dart), translation in apresentation/<feature>_failure_l10n.dartextension, andResult<T, F>propagation (use-cases/repos return it; cubits
switch). The real leaks areremoved —
transaction_review_view'scoreScreensUnexpectedError(message ?? 'unknown')and the BIP329 labels cubit's raw
'Export failed: $e'.Exception?/Stringto the concrete sealedFailure.import_watch_only: the inlinesatoshifierparse is extracted intoParseWatchOnlyInputUsecasereturningResult, so the cubit holds notry/catch.labels: the facade returnsResulton writes (store/trash) and staysbest-effort on reads (degrade to empty + log) so the ~15 core wallet read sites
are untouched; unused
LabelFailurevariants pruned.pin_code: the failure extension is renamed topin_code_failure_l10n.dart(naming convergence; pin_code itself was already on the standard).
*Error*keys; removed dead keys(
coreScreensUnexpectedError,labelErrorUnexpected, and the pruned labelvariants' keys).
docs(agents + architecture): member-ordering convention, the_failure_l10nnaming, and the Result/Failure standard.
descriptor/xpub/parse, broadcast
build_reviewable, all four labels use-cases).Convergence note. These three features are now fully on the standard —
<Feature>Failurenaming, thedomain/+presentation/split, andResult<T, F>propagation — and serve as the reference implementation the rest of the rollout
copies. They no longer use the legacy
<Feature>Error/toTranslated-on-the-error/
throw-based shape. The NFC collapse (PushTxNoNdefRecordsError/PushTxNoUriError/PushTxMissingFragmentParamsError→ oneInvalidPushTxFailure)is a deliberate granularity trade for sanitization.