Skip to content

refactor(payjoin): UX updates from clean-install testing - #2509

Merged
ethicnology merged 15 commits into
developfrom
payjoin-ux-minor-updates
Jul 30, 2026
Merged

refactor(payjoin): UX updates from clean-install testing#2509
ethicnology merged 15 commits into
developfrom
payjoin-ux-minor-updates

Conversation

@i5hi

@i5hi i5hi commented Jul 25, 2026

Copy link
Copy Markdown
Collaborator
  1. Autoswap announcement: copy direction corrected (Liquid → Bitcoin) and only shown when the Liquid balance actually reaches the trigger threshold (reuses AutoSwap.passedRequiredBalance)
  2. Payjoin eligibility no longer requires a confirmed balance — unconfirmed UTXOs are contributable (BDK listUnspent), so payjoin activates immediately on funding
  3. Payjoin announcement removed from home — payjoin education lives in the disclaimer pop-up and settings
  4. Receive screen: payjoin control moved from the TopBar chip to a toggle row under the address
  5. Payjoin settings: disclaimers become a one-time pop-up on first enable (re-openable via a settings row); minimum amount, session expiry and servers now live under a collapsed Advanced section
  6. Send confirmation: payjoin shown as a ticked details row (like self-transfer), replacing the info card

@claude

claude Bot commented Jul 25, 2026

Copy link
Copy Markdown

Claude finished @i5hi's task in 6m 54s —— View job


Code review — payjoin UX updates

  • Read repo guidelines and gather PR diff
  • Review announcements changes (autoswap trigger, payjoin removal)
  • Review receive flow changes (payjoin toggle, unconfirmed balance eligibility)
  • Review payjoin settings (disclaimer dialog, datasource, advanced section)
  • Review send confirmation changes
  • Verify findings and post review

Note: I can't submit formal inline PR review comments (I only update this single comment), so findings are anchored by file:line below. I also couldn't run the analyzer/tests here (no Flutter SDK on this runner) — relying on CI for that.

Findings

1. One-time disclaimer can be permanently skipped without ever being shown — lib/features/settings/ui/widgets/payjoin_disclaimer_dialog.dart:22-28 (low, correctness)
showIfNeverShown writes the "shown" flag before displaying the dialog. If context.mounted is false after the async readDisclaimerShown() (user flips the toggle and immediately navigates back), the method returns after the write — the disclaimer is marked shown but never displayed, and it's a one-time safety disclosure. Suggest showing first and writing the flag afterwards (guard against double-show with a simple in-flight boolean if needed). Fix this →

2. Unconfirmed-input "worst case" is understated; consider preferring confirmed UTXOs — lib/features/receive/presentation/bloc/receive_bloc.dart:141-146 (medium, risk note on a deliberate decision)
The new comment says the worst case is "a strict sender rejects a proposal spending an unconfirmed input and the payment falls back to a normal broadcast." There's a worse case: a non-strict sender signs and broadcasts the payjoin tx spending the receiver's unconfirmed input; if that input's parent tx is later RBF-replaced/double-spent, the payjoin tx is invalidated after both sides consider the payment done — the sender keeps their funds and the receiver waits forever, with nothing automatically rebroadcasting the original fallback. I verified the claim that BDK's listUnspent includes unconfirmed outputs (lib/core/wallet/data/datasources/bdk_wallet_datasource.dart:369-388, confirmations = 0), and _filterAvailableUtxos (lib/core/payjoin/data/repository/payjoin_repository_impl.dart:1137-1145) only filters payjoin-locked UTXOs — no confirmation preference. A cheap mitigation that keeps the immediate-activation UX: sort/prefer confirmed UTXOs when building input pairs, so only genuinely unconfirmed-only wallets take the new risk. At minimum, the bloc comment should state the real worst case.

3. Autoswap announcement mixes current-environment balance with mainnet-pinned autoswap settings — lib/features/announcements/domain/usecases/get_visible_announcements_usecase.dart:31-48 (low)
GetWalletsUsecase filters wallets by the current environment (lib/core/wallet/domain/usecases/get_wallets_usecase.dart:22-30), but autoswap settings come from the mainnet Boltz repo instance (lib/core/swaps/swaps_locator.dart:203-209) and execution hardcodes Environment.mainnet (lib/core/swaps/domain/usecases/auto_swap_execution_usecase.dart:41-43). In testnet mode the card's visibility is driven by the testnet Liquid balance against mainnet trigger settings. Related: GetWalletsUsecase.execute throws NoWalletsFoundException on an empty result, which turns the whole announcements evaluation into an Err (everything hidden) instead of treating the balance as zero — harmless while the catalog has one entry, but a footgun as it grows. Consider tolerating the empty-wallets case in the usecase.

4. Stale comment — lib/features/receive/presentation/bloc/receive_state.dart:287-289 (nit)
The canPayjoin comment still says "ReceiveBloc only creates a session for a wallet with a confirmed balance to contribute," which this PR changed to total balance.

5. sendPayjoinWillBeAttempted is now unreferenced (nit)
After removing the send-screen InfoCard, no code references this string; the entry remains in localization/app_en.arb:14929. Remove it unless keeping deliberately.

6. Raw Material Switch vs BBSwitch (nit)
The new receive tile uses Flutter's Switch (lib/features/receive/ui/widgets/receive_payjoin_toggle_button.dart:63), while the payjoin settings screen uses BBSwitch — minor visual inconsistency between the two entry points of the same global setting.

Verified correct

  • Autoswap copy direction: execution creates SwapType.liquidToBitcoin swaps sweeping the default Liquid wallet, so "Your Liquid is automatically swapped to Bitcoin" is the right fix.
  • Trigger gating: AutoSwap.passedRequiredBalance checks enabled too (lib/core/swaps/domain/entity/auto_swap.dart:24-26), so the single signal correctly covers both conditions.
  • Payjoin announcement removal: AnnouncementId.payjoinPrivacy retained; l10n and navigation mappings stay exhaustive, so persisted dismissals still map cleanly. Dropping WatchPayjoinEnabledChangesUsecase from the announcements cubit is safe — it's still registered and used by the receive feature.
  • Receive toggle relocation: the tile self-gates on isPayjoinToggleable and the awaiting-funds hint stays consistent with the new hasUtxos rule; no double-dispatch between the row tap and the switch.
  • Send confirm row: isPayjoin follows the existing isToSelf private-named-param pattern; the other CommonOnchainSendInfoSection call sites (Liquid send, swap confirm) correctly default to false.
  • Tests were updated coherently with the behavior changes, including flipping the unconfirmed-balance regression pin into a positive test.

Overall: no blocking bugs found. #2 is the one I'd act on (comment accuracy + confirmed-first input preference), #1 is a small real edge worth the reorder.

@i5hi

i5hi commented Jul 26, 2026

Copy link
Copy Markdown
Collaborator Author

Several issues with UI. Manually testing and updating.

@i5hi

i5hi commented Jul 26, 2026

Copy link
Copy Markdown
Collaborator Author
simulator_screenshot_45B51DDA-4FD2-4A88-9BBA-FAFF40673571 simulator_screenshot_175A21C4-F1D7-4D10-BC5F-491B1D41CBB2

@i5hi

i5hi commented Jul 26, 2026

Copy link
Copy Markdown
Collaborator Author
simulator_screenshot_DCD547F3-60CF-4F21-AB1C-FF68720CA456

@i5hi

i5hi commented Jul 26, 2026

Copy link
Copy Markdown
Collaborator Author
simulator_screenshot_4CD8FE9C-7B58-4F0E-AB07-DAFEB054CA2E

@i5hi

i5hi commented Jul 26, 2026

Copy link
Copy Markdown
Collaborator Author

@claude Please re-review

@claude

claude Bot commented Jul 26, 2026

Copy link
Copy Markdown

Claude finished @i5hi's task in 6m 16s —— View job


Re-review — commits baa9241 + 5603ebe (manual-testing round)

  • Gather context (new commits since last review, full diff vs develop)
  • Check whether prior review findings were addressed
  • Review new/changed code from the manual-testing round
  • Post re-review

Verdict: no blocking bugs. CI (Analyze and Test) is green on the latest commit, and the screenshots match the code. The new round is a solid cleanup — the router guard in particular correctly replaces the deleted ReceiveAmountBottomSheet workaround. A few small items below, plus four carried-over nits from the previous review that are still present.

New findings

1. Wallet-dropdown selection can silently revert when opening the amount page — lib/features/receive/ui/receive_router.dart:151-154 (low, pre-existing behavior, not a regression)
The new guard compares the route's extra wallet against bloc.state.wallet. Scenario: enter bitcoin receive from a wallet detail page (extra = W1), switch to W2 via the on-screen dropdown (which dispatches ReceiveBitcoinStarted(W2) directly), then tap the Additional Information tile. The push re-runs the parent pageBuilder with the original extra W1, the guard sees W2 != W1, and re-fires ReceiveBitcoinStarted(W1) — reverting the dropdown choice and wiping any amount/message. Pre-PR this was strictly worse (unconditional restart every re-run), so this is still an improvement; flagging so the interaction is a known trade-off. A fix would be to only restart on type mismatch (like the lightning/liquid routes) and let the dropdown own within-flow wallet switches. Fix this →

2. receiveNotePlaceholder is now orphaned (nit)
The old QR-screen note tile was its last consumer; nothing in lib/ references it anymore. Same situation as sendPayjoinWillBeAttempted from the previous review (localization/app_en.arb) — both can go in one sweep.

3. ReceiveInfoDetails.wallet is now dead — lib/features/receive/ui/screens/receive_qr_screen.dart:237-241 (nit)
The rewritten build no longer reads the field; the constructor param and the ReceiveInfoDetails(wallet: wallet) call site in ReceiveQrPage can be dropped.

4. Entered-unit suffix can drift from what was actually entered (nit)receive_qr_screen.dart:245-252
_enteredUnitSuffix reads inputAmountCurrencyCode/formattedConfirmedAmountFiat via context.read, keyed off the current input currency. If the user confirms an amount in EUR, re-opens the amount page, switches the currency picker (which clears inputAmount but keeps confirmedAmountSat) and pops back without confirming, the suffix shows the new currency's conversion, not the entered one. Niche; fine to leave.

Carried-over from the previous review (still present)

  • Disclaimer marked shown before it's displayedpayjoin_disclaimer_dialog.dart:22-28: writeDisclaimerShown() still precedes show(); an unmounted context after the async read permanently skips the one-time disclosure.
  • Understated worst case for unconfirmed payjoin inputsreceive_bloc.dart comment: the real worst case is a payjoin tx spending the receiver's unconfirmed input being invalidated by an RBF of its parent after both sides consider the payment done. Confirmed-first UTXO preference in _filterAvailableUtxos remains a cheap mitigation.
  • Stale commentreceive_state.dart:287-291: canPayjoin's doc still says "a wallet with a confirmed balance to contribute"; the adjacent hasUtxos comment (line 323) now says the opposite, correctly.
  • Raw Material Switch vs BBSwitchreceive_payjoin_toggle_button.dart:59 still uses Flutter's Switch while the settings screen uses BBSwitch for the same global setting.

Verified correct in this round

  • Router guard (receive_router.dart:147-155): correctly prevents the pop-back restart that wiped amount/message — this is what made deleting ReceiveAmountBottomSheet safe, since the sheet existed precisely to avoid route pushes re-running the pageBuilder (and the historical-txs auto-redirect that restart caused). Fresh entry still works: a new bloc's type is null, so ReceiveBitcoinStarted fires.
  • ReceiveNoteChangedReceiveNoteSaved pairing on the amount screen: _onNoteChanged emits synchronously before _onNoteSaved reads state.note, and _onNoteSaved safely no-ops for lightning and for null addresses (receive_bloc.dart:713-745) — so adding it to the amount screen (where lightning has no invoice yet) is safe, and it fixes notes never reaching the labels store from that page.
  • Advanced settings extraction: PayjoinAdvancedSettingsScreen is a faithful move of the old fields — debounce, bounds validation, empty-is-typing handling, _persist logging, and the controller/value: mirror all preserved; the new route is registered in settings_router.dart and only reachable when payjoin is enabled.
  • Unified "Additional Information" tile: amount always shown in BTC (matching what the BIP21 carries) with the entered unit as a suffix; fiatCurrencyCode/exchangeRate track the entered fiat currency (receive_bloc.dart:589-627), so the ~ fiat suffix is in the right currency. Liquid vs bitcoin amount routes are dispatched correctly despite sharing the amount path.
  • BullRadius.xxs (2px) radius unification and the QR/tile sizing match the design-token comment and the screenshots.
  • Label sheet inset fix (label_entry_bottom_sheet.dart): max(viewInsets.bottom, viewPadding.bottom) is the right formula — keyboard inset when open, home-indicator inset when closed, never both.
  • Lightning naming: "Note" on lightning (invoice description) vs "Message For Sender" on bitcoin/liquid (BIP21 message=) is applied consistently on the amount screen and summary tile.

--- · Branch: payjoin-ux-minor-updates

@i5hi

i5hi commented Jul 27, 2026

Copy link
Copy Markdown
Collaborator Author

@claude please review again

@claude

claude Bot commented Jul 27, 2026

Copy link
Copy Markdown

Claude finished @i5hi's task in 3m 2s —— View job


Re-review — commits f8378ac + 7714b32 (review-findings round)

  • Gather context (new commits since last review: f8378ac, 7714b32)
  • Verify prior review findings were addressed
  • Review new/changed code from this round
  • Post re-review

Verdict: no blocking bugs. CI (Analyze and Test) is green on the latest commit. Every finding from the previous two reviews has been addressed — with one wrinkle: the updated receive_bloc.dart comment now describes a mitigation that was never actually implemented.

Findings

1. Comment claims a confirmed-first UTXO preference that doesn't exist in code — lib/features/receive/presentation/bloc/receive_bloc.dart:145-149 (low, but misleading)
The updated doc comment ends with "_filterAvailableUtxos preferring confirmed UTXOs when available is the cheap mitigation" — present tense, reading as existing behavior. But _filterAvailableUtxos (lib/core/payjoin/data/repository/payjoin_repository_impl.dart:1137-1146) was not touched in this round: it still only filters payjoin-locked UTXOs, with no confirmation preference. Downstream, _contributeInputs (lib/core/payjoin/data/datasources/pdk_payjoin_datasource.dart:566-590) hands all candidates to PDK's tryPreservingPrivacy, which selects one input on privacy heuristics alone — so an unconfirmed UTXO can be chosen even when confirmed ones are available, and list ordering wouldn't help. Two ways to resolve:

  • Implement it (small): in _filterAvailableUtxos, when any candidate has confirmations > 0 (the field exists on WalletUtxoModel), return only those — unconfirmed-only wallets still activate immediately, everyone else avoids the RBF-invalidation risk the comment describes.
  • Or reword the comment to "would be the cheap mitigation" so it reads as a known follow-up rather than current behavior.

Fix this →

2. Removed l10n keys linger in translated .arb files (nit)
receiveNotePlaceholder and sendPayjoinWillBeAttempted were removed from localization/app_en.arb but remain in ~30 locale files (app_fr.arb, app_de.arb, …). Extra keys in non-template files are silently ignored by gen-l10n, so this is harmless — only worth a sweep if the translation pipeline doesn't prune them automatically.

3. Import ordering — lib/features/receive/ui/widgets/receive_payjoin_toggle_button.dart:1-2 (nit)
The new bb_switch.dart import was inserted above themes/app_theme.dart, breaking alphabetical order. The analyzer doesn't enforce directives_ordering here (CI is green), so purely cosmetic.

Prior findings — all verified fixed

  • Disclaimer ordering (payjoin_disclaimer_dialog.dart:22-31): writeDisclaimerShown() now runs after show(context) completes, with a comment explaining why — the unmounted-context path can no longer permanently skip the one-time disclosure. The suggested in-flight double-show guard wasn't added, but the dialog is modal (the switch is unreachable while it's up), so the only remaining window is a rapid off/on before the async read resolves — negligible.
  • Router guard (receive_router.dart:146-156): now restarts on type mismatch only, matching the lightning/liquid routes — the dropdown-revert-on-pop-back interaction is fixed, and fresh entry still works since a new bloc's type is null.
  • BBSwitch consistency: the receive tile now uses BBSwitch, which gained an optional materialTapTargetSize pass-through for the dense tile placement — clean, non-breaking addition.
  • Dead code: ReceiveInfoDetails.wallet param and its call-site argument removed.
  • Orphaned English l10n keys: both removed from app_en.arb (see nit 2 for the locale-file leftovers).
  • Stale comments: receive_state.dart:288 now says "a balance to contribute (unconfirmed counts…)", and the receive_bloc.dart eligibility comment now states the real RBF-invalidation worst case accurately (modulo finding 1's phrasing).

Also reviewed this round

  • New BIP21 composition tests (test/features/receive/presentation/bloc/receive_state_test.dart:121-193): good contract coverage — amount always denominated in BTC regardless of entry unit, note travels as message=, payjoin pj= params merge on top of (not instead of) amount/message, and the liquid variant checks scheme + assetid. These pin exactly the user-facing QR/clipboard string this PR's "Additional Information" rework produces.

Summary: this round faithfully closes out both prior reviews. The only thing I'd act on is finding 1 — either implement the confirmed-first filter (a few lines, and the comment becomes true) or soften the comment's tense. Findings 2–3 are take-or-leave nits.

--- · Branch: payjoin-ux-minor-updates ·

@ethicnology
ethicnology force-pushed the payjoin-ux-minor-updates branch from 0900a34 to 007fe2f Compare July 27, 2026 16:50
i5hi and others added 8 commits July 27, 2026 15:15
The disclaimer dialog resolved a datasource from the service locator inside
a widget, so the ui layer talked straight to data (AGENTS.md rules #2/#6),
and the receive feature reached into settings/ui/widgets for it (rule #1).

The flag now travels the normal chain: a PayjoinDisclaimerRepository
interface in domain, a SharedPreferences implementation in data, two
usecases, and SettingsCubit holding the state. One boolean does not justify
a datasource forwarding two calls to a repository forwarding them back, so
it collapses into the repository. The dialog becomes passive and moves to
core/widgets, which also drops the cross-feature import: both call sites
show it, then record it — never the other way round.
The seven keys this branch adds (disclaimer title and settings row, advanced
settings title, the receive Additional Information tile and its placeholder,
the message-for-sender label, the send-confirm payjoin row) were en-only.

Short labels, not disclosure prose: PAYJOIN_FEATURES_PLAN F9 keeps the two
disclaimer explanations on the translator pipeline, and those are untouched
here. "Payjoin" stays untranslated in every locale, as the reviewed corpus
already does (transactionStatusPayjoinAborted). Machine-authored — worth a
native pass whenever the locale is next reviewed.
@ethicnology
ethicnology force-pushed the payjoin-ux-minor-updates branch from ec94f58 to 796513b Compare July 27, 2026 19:23
@ethicnology

Copy link
Copy Markdown
Member

Payjoin lifecycle

  • Centralized Payjoin enable/disable and disclaimer handling in the settings use case.
  • Made the one-time disclaimer explicitly accepted and non-dismissible before enabling Payjoin.
  • Added typed failures and localized feedback when toggling Payjoin fails.
  • Safely cleans up active receiver sessions when Payjoin is disabled: idle endpoints are removed, received requests fall back to the original transaction, and committed proposals remain active.
  • Removes expired unused receiver endpoints instead of showing ghost 0 sats / pending transactions.
  • Added synchronization around receiver creation, manual fallback, and observed fallback handling.

Receive and settings UX

  • Keeps the Receive and Settings screens synchronized when Payjoin is toggled elsewhere.
  • Reacts immediately to changes in the minimum Payjoin amount.
  • Prevents duplicate toggle actions while an update is running.
  • Persists valid debounced advanced-setting edits when leaving the screen.
  • Routes completed Payjoins directly to transaction details.
  • Allows clearing a receive message and removes its existing address label.

Transaction display

  • Hides an aborted Payjoin session until its original wallet transaction is available, avoiding a temporary duplicate or empty transaction row.
  • Preserves genuinely pending Payjoin sessions in transaction history.

Additional review fixes

  • Generalized the checked status row on the send confirmation screen.
  • Prevented the auto-swap announcement from appearing while its warning state is active.
  • Updated Payjoin disclosure wording in English and French.

@ethicnology ethicnology changed the title Payjoin UX updates from clean-install testing refactor(payjoin): UX updates from clean-install testing Jul 28, 2026
@ethicnology
ethicnology merged commit ad6c1b5 into develop Jul 30, 2026
3 checks passed
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