Skip to content

refactor(dca): sanitize user-facing error messages - #2537

Draft
wired-pasteque wants to merge 5 commits into
developfrom
refactor-errors-dca
Draft

refactor(dca): sanitize user-facing error messages#2537
wired-pasteque wants to merge 5 commits into
developfrom
refactor-errors-dca

Conversation

@wired-pasteque

Copy link
Copy Markdown
Collaborator

No description provided.

@wired-pasteque wired-pasteque self-assigned this Jul 29, 2026
@claude

This comment was marked as outdated.

@wired-pasteque

Copy link
Copy Markdown
Collaborator Author

@claude, review fixes that are related to #1895

@claude

claude Bot commented Jul 29, 2026

Copy link
Copy Markdown

Claude finished @wired-pasteque's task in 3m 13s —— View job


Review: fixes for issue #1895 (error-message sanitization follow-ups)

  • Gather context (fix commits since last review; couldn't fetch issue Sanitize All User-Facing Error Messages #1895 body — gh/web access not permitted in this runner, reviewed against the PR's stated goal of sanitizing user-facing DCA errors)
  • Review receive-address failure sanitization fix
  • Review buy-limits loading removal
  • Review retry path for failed account loading
  • Review l10n usage and tests
  • Post review findings

Three of the four findings from the previous review are now properly fixed, and CI (Analyze and Test) is green on the latest commit. The one remaining item is the duplicate-order risk, which was flagged as a pre-existing issue — details below in case it's intended for a follow-up.

Fixed and verified

1. logMessage sanitization (55df4598) — DcaReceiveAddressFailure now carries e.runtimeType.toString() instead of e.toString() (lib/features/dca/domain/usecases/set_dca_usecase.dart:81), so wallet identifiers can no longer leak through consumers that log failure.logMessage. The test now backs the guarantee with real assertions: the sentinel wallet id must be absent from logMessage and the value must equal 'WalletNotFound' (test/features/dca/domain/usecases/set_dca_usecase_test.dart:153-154). I confirmed this assertion is sound: with freezed 3.x (pubspec.yaml pins freezed: ^3.2.3), WalletError.notFound constructs the concrete generated class WalletNotFound, so runtimeType.toString() is exactly that — and the green CI run confirms it.

2. Misleading buy-limits failure (8ff2071c) — resolved by removing the buy-limits fetch entirely rather than remapping the failure. I verified buyLimits was genuinely dead: nothing in lib/ consumed DcaStartData.buyLimits (the only remaining getBuyLimits references are the core exchange repository/datasource, used elsewhere). The locator registration, usecase constructor, and tests were all updated consistently.

3. No retry path (7303df1f) — _onStarted now emits DcaState.initial() at the top of the handler (lib/features/dca/presentation/dca_bloc.dart:31), clearing any stale failure, and DcaScreen was restructured around a state switch: spinner while loading, sanitized error plus a Retry button on failure (_DcaInitialContent in lib/features/dca/ui/screens/dca_screen.dart), and the form only once data is loaded — a nice side benefit over the old always-visible form. The Retry button reuses the existing retry l10n key, which is present in all 27 locale files. The new bloc test covers the full failure → retry → success sequence, including the intermediate cleared-failure state.

Still open

Duplicate-order risk when the preference save fails after order creationlib/features/dca/domain/usecases/set_dca_usecase.dart:102-111. Unchanged from the previous review: if createDca succeeds but _saveUserPreferencesUsecase.execute(dcaEnabled: true) throws, the usecase returns Err(DcaUnexpectedFailure(...)), the confirmation screen re-enables the Confirm button, and tapping it again creates a second live DCA order. Not a regression (same behavior on develop), so fine to defer — but worth tracking. Since the order is already live at that point, logging the failure and returning Ok(dca) (or a distinct partial-success failure) would be safer. Fix this →

Optional hardening (nitpick)

DcaStarted is registered without an event transformer (dca_bloc.dart:21), and bloc's default processes events concurrently. A rapid double-tap on Retry could run two _startDcaUsecase.execute() calls whose results race — worst case the screen lands on a failure state even though one load succeeded. The window is tiny (the button disappears as soon as the first handler emits), so this is purely optional: on<DcaStarted>(_onStarted, transformer: restartable()) (or droppable()) from bloc_concurrency would close it.


@ethicnology

Copy link
Copy Markdown
Member

Conflict management required plz @wired-pasteque

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