Skip to content

refactor(transactions): speed up transfer progress and transaction details - #2668

Merged
ethicnology merged 4 commits into
feat/exchange-order-swapsfrom
transaction-details-loading
Aug 13, 2026
Merged

refactor(transactions): speed up transfer progress and transaction details#2668
ethicnology merged 4 commits into
feat/exchange-order-swapsfrom
transaction-details-loading

Conversation

@wired-pasteque

Copy link
Copy Markdown
Collaborator

Speeds up transfer progress and transaction details by running wallet sync in the background and loading wallet information in parallel.

Screen.Recording.2026-08-12.at.3.07.03.PM.mov

@wired-pasteque wired-pasteque self-assigned this Aug 12, 2026
@ethicnology

Copy link
Copy Markdown
Member

@claude review cette PR please

@claude

This comment was marked as outdated.

@wired-pasteque

Copy link
Copy Markdown
Collaborator Author

@claude, check the fixes

@claude

claude Bot commented Aug 12, 2026

Copy link
Copy Markdown

Claude finished @wired-pasteque's task in 6m 2s —— View job


Review: the fixes check out ✅

I re-reviewed the full PR diff (4 commits vs feat/exchange-order-swaps), with focus on the latest fix commit 771c4b6f0 ("keep transfer details accurate after sync"). The fixes are sound and well tested. Details below.

  • Gather context (prior review, diff vs feat/exchange-order-swaps)
  • Review the fix commits
  • Run relevant tests — not possible: Flutter/Dart is not installed on this runner, so I verified by close reading instead
  • Post review verdict

What the fixes address, and my verification

1. Wallet transaction stays live after sync (transaction_details_cubit.dart) — The new _watchOrderSwapWalletTransaction (line 190) correctly re-arms a WatchWalletTransactionByTxIdUsecase subscription on the canonical txid, with a generation counter guarding the await cancel() gap so concurrent re-arms can't double-subscribe (I traced the interleavings — latest call wins, no leaked duplicate subscription). The listener guards both isClosed and a stale-txid check before emitting. Since the watcher only fires on walletSyncFinishedStream (no initial emission), the one-shot fetch in _loadOrderSwapDetails plus the transfer bloc's post-broadcast background sync correctly cover the "tx not yet in DB" window.

2. Recovery from a failed initial load_handleOrderSwapUpdate (line 164) now reloads when the state has no matching order swap instead of silently returning, and _loadOrderSwapDetails clears err/notFoundError on success. The new test "recovers a failed initial load from an order update" covers exactly this.

3. Counterpart wallet row direction (transaction.dart:183)isReceivingWallet(walletId, {isCounterpart}) fixes the fallback for the counterpart row (isOutgoing instead of isIncoming), which restores the pre-existing behavior for ordinary between-wallet transfers while keeping correct semantics for internal swaps and chain swaps. The truth table checks out for all combinations I traced (internal order swap, external in/out order swap, chain swap, plain transfer).

4. ParallelWaitError unwrapping_firstParallelError surfaces the first underlying error instead of the opaque wrapper; the test asserts same(failure).

5. Transfer bloc (transfer_bloc.dart) — The order-swap branch now emits txId in the same emit as the broadcast result and returns early; the finally still resets isConfirming, and post-broadcast syncs are background + error-swallowed, so a sync failure can no longer masquerade as a broadcast failure. Sequentializing the two same-chain syncs via _syncWalletsAfterBroadcast is a good call.

Minor observations (non-blocking)

  1. refresh() doesn't re-arm the wallet-tx watcherinitByOrderSwapLocalId sets _reload = () => _loadDetailsByOrderSwapLocalId(localId) (transaction_details_cubit.dart:148). If the initial load failed (watcher never armed) and the user pull-to-refreshes successfully, the wallet transaction won't live-update on subsequent syncs until the next order-swap event happens to re-arm it. Wrapping the reload to also call _watchOrderSwapWalletTransaction(state.transaction?.orderSwap) would close that gap. Fix this →

  2. Dropped diagnostics in the sync-failure log_syncWalletAfterBroadcast (transfer_bloc.dart:1533) changed from log.warning(msg, error: error, trace: stackTrace) to catch (_) with a bare message. log.warning does accept error:/trace: (logger.dart:257), so this loses the failure cause for no gain — worth restoring.

  3. Narrow close race in _watchOrderSwapWalletTransaction — if close() lands exactly inside the await _walletTransactionSubscription?.cancel() at line 198, the method can resume with isClosed still false and create a subscription that close() never cancels. Emits are still guarded by isClosed, so the impact is only a dangling listener on the sync stream; same exposure exists in the pre-existing payjoin watchers, so fine to leave as-is.

  4. _firstParallelError cast (transaction_details_cubit.dart:300) — the hard cast to a 3-tuple is correct for the only .wait in the try block today, but if another ParallelWaitError shape ever escapes that scope the cast throws inside the catch handler. An is-check fallback to the raw error would future-proof it. Pure nit.

Test coverage

The added tests are strong: txId emitted before sync completes, atomic parallel loading (state stays loading until all three futures resolve, verified with Completers), update-preserves-details without refetch, stale canonical tx cleared, replacement txid watched, failed-load recovery, watcher retry after stream error, and error unwrapping. Nothing important looks untested.

Verdict: LGTM. The observations above are all minor and none block merging.

@ethicnology
ethicnology force-pushed the feat/exchange-order-swaps branch from 545f724 to 9755ebc Compare August 13, 2026 01:26
@ethicnology
ethicnology force-pushed the transaction-details-loading branch from 771c4b6 to 3e48eea Compare August 13, 2026 11:54
@ethicnology
ethicnology merged commit 5b4b140 into feat/exchange-order-swaps Aug 13, 2026
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