Skip to content

Commit 3c6a71d

Browse files
authored
Merge pull request #2293 from SatoshiPortal/1895-sanitize-all-user-facing-error-messages
refactor(errors): establish user-facing error sanitization pattern (#1895)
2 parents 8ecad6f + c0cb31f commit 3c6a71d

267 files changed

Lines changed: 10434 additions & 5033 deletions

File tree

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

AGENTS.md

Lines changed: 28 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -57,7 +57,7 @@ Hard rules:
5757
8. **Shared value objects go in `lib/core/primitives/`** (planned per [FEATURES.md](FEATURES.md) — folder doesn't exist yet at audit time, and `Secret`/`Address`/`Amount`/`Fingerprint` are not yet extracted as primitives). When you create a value object that more than one feature would reasonably use, put it in `lib/core/primitives/` from the start — that's how the primitives layer gets built. Before creating one, grep `lib/` for an existing class with the same intent.
5858
9. **Prefer rich domain models** — methods that enforce invariants, not anemic DTOs mirroring DB rows. (Note: [Fowler's anemic-domain anti-pattern](https://www.martinfowler.com/bliki/AnemicDomainModel.html) is widely cited but [not universal](https://blog.inf.ed.ac.uk/sapm/2014/02/04/the-anaemic-domain-model-is-no-anti-pattern-its-a-solid-design/). This is our preference, aligned with ARCHITECTURE.md's "Anemic Domain Models" pitfall.)
5959
10. **No raw colors.** Always pull from the theme. If the user describes a color in plain language, pick the closest theme color that works in both light and dark mode. Don't edit theme files unprompted.
60-
11. **Errors — one sealed family per feature.** Define a sealed `<feature>_error.dart` in `domain/`. Map foreign errors at every boundary; never leak another layer's or feature's error type. **Each error exposes a `toTranslated(BuildContext)`** (dominant convention, 36 uses — `sell_error.dart`, `bitbox_errors.dart`, `replace_by_fee/errors.dart`; not `toTranslation`/`String get message`) returning a localized, user-safe message via `AppLocalizations` (the `context.loc` extension; see the UI Kit "no hardcoded user-facing strings" rule) — never the raw exception or dev detail, which stays in logs. The `sealed` switch makes a missing user message a compile error. **Call it UI-side only** — it takes a `BuildContext`; the bloc holds the `<Feature>Error` in state, the widget renders `error.toTranslated(context)`. A `BuildContext` in `presentation/` is a smell. **The end user never sees a dev string:** the catch-all variant (`unexpected`/`unknown`) returns a **generic localized** message, never the raw `message` — `unexpected: (message) => message` leaks dev detail; log it, show a generic string. Prefer returning a `Result`/sealed outcome for expected, recoverable errors at the repository boundary (`throw` only for programmer errors) — officially "a recommendation, but not a requirement" ([data-layer case study](https://docs.flutter.dev/app-architecture/case-study/data-layer)); don't mass-migrate existing exception code. **Legacy (don't replicate):** some hexagonal features split errors per layer (`domain_errors.dart` / `application_errors.dart` / `presentation_errors.dart`), and `fund_exchange` uses feature-prefixed singular (`fund_exchange_<layer>_error.dart`). Converge to one family per feature over time.
60+
11. **Failures — one sealed family per feature, `*Failure` not `*Error`.** Three words, kept apart: **`Exception`** = thrown infra (data layer, caught at the boundary); **`Error`** = a `dart:core` programmer bug, never caught (→ Sentry); **`Failure`** = a modeled, recoverable **value** in `domain/`. Define a sealed `<Feature>Failure extends Failure` in `domain/<feature>_failure.dart`, **Flutter-free**; cross-cutting modes (network, storage-locked, not-found, timeout, auth, device, insufficient-funds) come from the shared `sealed CoreFailure` in `lib/core/failures/` (alongside the `Failure` base; the legacy `lib/core/errors/` is the graveyard until emptied). Map foreign errors at the boundary; never leak another layer's or feature's type. **Translation is a presentation extension, not a method on the failure:** `presentation/<feature>_failure_l10n.dart` exposes `toTranslated(BuildContext)` — the only place `context.loc` / `flutter` touches a failure (keeps `domain/`+`data/` Flutter-free); the `sealed` switch makes a missing user message a compile error. **The end user never sees a dev string:** the catch-all variant returns a **generic localized** message (`oopsSomethingWentWrong`), never the raw `logMessage` (logged at the boundary, for us only) — `unexpected: (m) => m` leaks dev detail. **Propagation:** repositories return `Result<T, F extends Failure>` (variants `Ok`/`Err`, generic over `F` so consumers never cast, `@useResult`; helpers `fold`/`map`/`mapErr`); `throw` only for `dart:core` bugs ([Flutter Result](https://docs.flutter.dev/app-architecture/design-patterns/result)). The **repository** is the one `try/catch` boundary (or the feature **use-case** when wrapping a shared core repo that still throws); use-cases forward/compose `Result`s; the bloc `switch`es and holds the typed `<Feature>Failure` in state (no `BuildContext` in `presentation/` logic); the UI renders `failure.toTranslated(context)`. **Migration (#1895) is sanctioned and staged** — bring a feature fully in line when you touch it, one feature per PR. **Legacy (don't replicate, converge per-feature):** `BullException` + hardcoded English; `<Feature>Error` naming; `toTranslated` *on* the error; per-layer splits (`domain_errors.dart`/`application_errors.dart`/…); `fund_exchange`'s feature-prefixed singular.
6161
12. **Acyclic feature graph.** Check [FEATURES.md](FEATURES.md) before adding a dependency.
6262
13. **Keep the dependency graph live.** Any PR that adds a feature, removes a feature, or changes which other features it consumes via `public/` facades **must** update the mermaid graph in [FEATURES.md](FEATURES.md) in the same commit. The graph is documentation only if it matches the code — if you change one without the other, both become useless.
6363
14. **Folders justify their existence — files don't justify folders.** A tiny piece of code is one file with a role suffix, not a folder of one file:
@@ -69,7 +69,7 @@ Hard rules:
6969

7070
**Exception (melos migration):** `packages/` and `features/` are intentionally pre-created with `.gitkeep` as reserved workspace homes for the in-progress monorepo migration — do not remove them or treat them as a rule-#14 violation. See the Monorepo / melos section.
7171

72-
15. **Enforce with the compiler, not hope.** Repository contracts → `abstract interface class` (forbids `extends`, forces `implements`); lock finer capabilities with `@Deprecated.implement()` / `.instantiate()` / `.extend()` (Dart 3.10+). Error families and multi-case states → `sealed` (a missing `switch` case is a compile error). `Result`-returning repo methods → annotate `@useResult` (`package:meta`) so a discarded result warns. Cross-feature and `data/`-from-`presentation/` bans → a rule in the first-party analyzer plugin system (Dart 3.10+, the supported successor to `custom_lint`); under melos, package boundaries + the `implementation_imports` lint make them hard errors. All available in the current toolchain (Flutter 3.41/3.44 bundle Dart 3.11/3.12) — no point-release dependency; nothing new in 3.41/3.44 is required.
72+
15. **Enforce with the compiler, not hope.** Repository contracts → `abstract interface class` (forbids `extends`, forces `implements`); lock finer capabilities with `@Deprecated.implement()` / `.instantiate()` / `.extend()` (Dart 3.10+). Failure families and multi-case states → `sealed` (a missing `switch` case is a compile error). `Result`-returning repo methods → annotate `@useResult` (`package:meta`) so a discarded result warns. Cross-feature and `data/`-from-`presentation/` bans → a rule in the first-party analyzer plugin system (Dart 3.10+, the supported successor to `custom_lint`); under melos, package boundaries + the `implementation_imports` lint make them hard errors. All available in the current toolchain (Flutter 3.41/3.44 bundle Dart 3.11/3.12) — no point-release dependency; nothing new in 3.41/3.44 is required.
7373

7474
When a request would break these rules, explain why and propose a compliant alternative. Don't silently comply.
7575

@@ -114,7 +114,7 @@ Codified from a sweep of the actual codebase. ARCHITECTURE.md is silent on most
114114
| Event | `<feature>_event.dart` | `send_event.dart` |
115115
| Facade | `<feature>_facade.dart` | `labels_facade.dart` |
116116
| Locator | `<feature>_locator.dart` | `wallet_locator.dart` |
117-
| Errors | per rule #11 | |
117+
| Failure (family) | `<feature>_failure.dart` (domain) + `<feature>_failure_l10n.dart` (presentation) | per rule #11 |
118118

119119
**Classes**`PascalCase`, matching local quirks:
120120

@@ -129,10 +129,34 @@ Codified from a sweep of the actual codebase. ARCHITECTURE.md is silent on most
129129
- **State / Event**: `<Feature>State` / `<Feature>Event`, sealed with freezed when there are multiple cases.
130130
- **Facade**: `<Feature>Facade` (`LabelsFacade`).
131131
- **Watcher** (when introduced): `<Subject>Watcher`.
132-
- **Error**: one sealed `<Feature>Error` family per feature (see rule #11).
132+
- **Failure**: one sealed `<Feature>Failure` family per feature in `domain/<feature>_failure.dart` (base `Failure`); never `<Feature>Error``Error` is reserved for `dart:core` bugs. Translation extension `<Feature>FailureL10n` in `presentation/`. See rule #11.
133133

134134
When the same role exists with two names in the codebase, use the dominant one and flag the outlier as legacy. Don't rename outliers in unrelated PRs — that breaks atomic commits.
135135

136+
### Member ordering
137+
138+
Inside a class, declare members in this order, with a blank line between each group:
139+
140+
1. **Instance fields** — the object's state. Reading these first tells you *what the thing is*.
141+
2. **Constructor(s)** — how it's built, on top of the fields you just read.
142+
3. **Methods** — its behaviour, including `@override`s.
143+
144+
```dart
145+
class Amount {
146+
final BigInt sats;
147+
148+
Amount(this.sats) {
149+
if (sats < BigInt.zero) throw ArgumentError('Amount cannot be negative');
150+
}
151+
152+
bool get isDust => sats < BigInt.from(546);
153+
}
154+
```
155+
156+
This is fields-first on purpose: our domain, value, and error types are defined by their state, so surfacing it first reads most naturally (and matches the field-first habit of most other languages). It is a soft convention — the analyzer does not enforce member order, so keep it consistent by hand and in review.
157+
158+
**Do not enable the `sort_constructors_first` lint.** It enforces the *opposite* order (constructor before fields); turning it on would fight this convention and churn every class. Widgets, whose constructor is effectively their public API, may keep the constructor first if that reads better there — don't reformat existing widgets to satisfy this rule.
159+
136160
## UI Kit — reuse first, build the kit as you go
137161

138162
The codebase is migrating toward a real UI Kit. The skeleton already lives in [`lib/core/widgets/`](lib/core/widgets/) (`buttons/`, `cards/`, `inputs/`, `lists/`, `dropdown/`, `bottom_sheet/`, `text/`, `selectors/`, …) and theme tokens in [`lib/core/themes/`](lib/core/themes/). Today the same widgets get re-implemented per feature (`_SaveButton`, `_ConfirmButton`, `_BottomButtons` …) — stop adding to that pile.

0 commit comments

Comments
 (0)