From f0d932c61384ca656a0e3ba87f30d1ca22f03e4a Mon Sep 17 00:00:00 2001 From: joshuakrueger-dfx Date: Thu, 4 Jun 2026 09:24:34 +0200 Subject: [PATCH] =?UTF-8?q?fix(sell-bitbox):=20make=20deposit=20retry=20id?= =?UTF-8?q?empotent=20=E2=80=94=20never=20re-broadcast=20a=20sent=20tx?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit _broadcastDepositAndConfirm broadcast the deposit tx and then confirmed the payment in one try/catch, emitting SellBitboxDepositRetry on ANY failure. When the broadcast succeeded but the confirmation failed, retryDeposit re-ran the whole helper and re-broadcast the already-on-chain deposit transaction — a duplicate send and a perpetual retry loop. Split broadcast from confirm and carry the resulting txHash in the retry state: - broadcast failure → retry may broadcast again (broadcastTxHash == null) - confirm failure after a successful broadcast → retry confirms ONLY using the stored txHash, never re-broadcasting (broadcastTxHash != null) Regressions: sell_bitbox_cubit_test.dart (confirm-only retry asserts verifyNever(broadcastTransaction); broadcast-failure retry still broadcasts) + sell_bitbox_state_test.dart (broadcastTxHash in props). Issue #657 — Part 4, finding BB1 (HIGH, financial). --- .../sell_bitbox/cubit/sell_bitbox_cubit.dart | 50 +++++++--- .../sell_bitbox/cubit/sell_bitbox_state.dart | 15 ++- .../cubit/sell_bitbox_state_test.dart | 12 ++- .../sell_bitbox/sell_bitbox_cubit_test.dart | 92 ++++++++++++++++++- 4 files changed, 147 insertions(+), 22 deletions(-) diff --git a/lib/screens/sell_bitbox/cubit/sell_bitbox_cubit.dart b/lib/screens/sell_bitbox/cubit/sell_bitbox_cubit.dart index 9d9043f7..0c6127d2 100644 --- a/lib/screens/sell_bitbox/cubit/sell_bitbox_cubit.dart +++ b/lib/screens/sell_bitbox/cubit/sell_bitbox_cubit.dart @@ -148,19 +148,21 @@ class SellBitboxCubit extends Cubit { final retryState = state; if (retryState is! SellBitboxDepositRetry) return; - try { - emit(SellBitboxDepositing()); - await _broadcastDepositAndConfirm( + emit(SellBitboxDepositing()); + final txHash = retryState.broadcastTxHash; + if (txHash != null) { + // The deposit transaction is already on-chain — only the payment + // confirmation failed. Confirm only; re-broadcasting an already-sent tx + // is what caused the perpetual retry loop (issue #657 P4 BB1). + await _confirmDeposit( retryState.signedSwapTransaction, retryState.signedDepositTransaction, + txHash, ); - } catch (e) { - emit( - SellBitboxDepositRetry( - retryState.signedSwapTransaction, - retryState.signedDepositTransaction, - e.toString(), - ), + } else { + await _broadcastDepositAndConfirm( + retryState.signedSwapTransaction, + retryState.signedDepositTransaction, ); } } @@ -169,15 +171,39 @@ class SellBitboxCubit extends Cubit { BroadcastTransactionRequestDto signedSwap, BroadcastTransactionRequestDto signedDeposit, ) async { + final String txHash; try { - final txHash = await _sellService.broadcastTransaction( + txHash = await _sellService.broadcastTransaction( _paymentInfo.id, signedDeposit, ); + } catch (e) { + // The broadcast itself failed: the tx is not on-chain, so a retry may + // safely broadcast again (broadcastTxHash stays null). + emit(SellBitboxDepositRetry(signedSwap, signedDeposit, e.toString())); + return; + } + // From here the deposit tx is broadcast; confirmation must be idempotent. + await _confirmDeposit(signedSwap, signedDeposit, txHash); + } + + Future _confirmDeposit( + BroadcastTransactionRequestDto signedSwap, + BroadcastTransactionRequestDto signedDeposit, + String txHash, + ) async { + try { await _sellService.confirmPaymentWithTxHash(_paymentInfo, txHash); emit(SellBitboxSuccess()); } catch (e) { - emit(SellBitboxDepositRetry(signedSwap, signedDeposit, e.toString())); + // Confirmation failed AFTER a successful broadcast: carry the txHash so a + // retry confirms the existing tx instead of broadcasting a duplicate. + emit(SellBitboxDepositRetry( + signedSwap, + signedDeposit, + e.toString(), + broadcastTxHash: txHash, + )); } } diff --git a/lib/screens/sell_bitbox/cubit/sell_bitbox_state.dart b/lib/screens/sell_bitbox/cubit/sell_bitbox_state.dart index c6828dc2..938657a2 100644 --- a/lib/screens/sell_bitbox/cubit/sell_bitbox_state.dart +++ b/lib/screens/sell_bitbox/cubit/sell_bitbox_state.dart @@ -47,20 +47,27 @@ class SellBitboxAwaitingDepositConfirm extends SellBitboxDepositState { class SellBitboxDepositing extends SellBitboxDepositState {} -// Swap was broadcast successfully but deposit broadcast failed +// The swap was broadcast; the deposit step then failed. `broadcastTxHash` tells +// retry which step to resume: null = the deposit was never broadcast (safe to +// broadcast), non-null = the deposit is already on-chain and only the payment +// confirmation failed, so retry must confirm ONLY and never re-broadcast it +// (issue #657 P4 BB1). class SellBitboxDepositRetry extends SellBitboxDepositState { final BroadcastTransactionRequestDto signedSwapTransaction; final BroadcastTransactionRequestDto signedDepositTransaction; + final String? broadcastTxHash; final String errorMessage; SellBitboxDepositRetry( this.signedSwapTransaction, this.signedDepositTransaction, - this.errorMessage, - ); + this.errorMessage, { + this.broadcastTxHash, + }); @override - List get props => [signedSwapTransaction, signedDepositTransaction, errorMessage]; + List get props => + [signedSwapTransaction, signedDepositTransaction, broadcastTxHash, errorMessage]; } class SellBitboxSuccess extends SellBitboxDepositState {} diff --git a/test/screens/sell_bitbox/cubit/sell_bitbox_state_test.dart b/test/screens/sell_bitbox/cubit/sell_bitbox_state_test.dart index 23e1e269..90949bec 100644 --- a/test/screens/sell_bitbox/cubit/sell_bitbox_state_test.dart +++ b/test/screens/sell_bitbox/cubit/sell_bitbox_state_test.dart @@ -140,7 +140,7 @@ void main() { expect(a, equals(b)); expect(a.hashCode, b.hashCode); - expect(a.props, [broadcastA, broadcastB, 'boom']); + expect(a.props, [broadcastA, broadcastB, null, 'boom']); }); test('different error messages are unequal', () { @@ -148,6 +148,16 @@ void main() { final b = SellBitboxDepositRetry(broadcastA, broadcastB, 'other'); expect(a, isNot(equals(b))); }); + + test('different broadcastTxHash values are unequal (issue #657 P4 BB1)', () { + // null = deposit not yet on-chain (retry may broadcast); non-null = + // already broadcast (retry must confirm only). + final a = SellBitboxDepositRetry(broadcastA, broadcastB, 'boom'); + final b = SellBitboxDepositRetry(broadcastA, broadcastB, 'boom', + broadcastTxHash: '0xtxhash'); + expect(a, isNot(equals(b))); + expect(b.props, [broadcastA, broadcastB, '0xtxhash', 'boom']); + }); }); group('SellBitboxError', () { diff --git a/test/screens/sell_bitbox/sell_bitbox_cubit_test.dart b/test/screens/sell_bitbox/sell_bitbox_cubit_test.dart index 616cb46f..55fc326d 100644 --- a/test/screens/sell_bitbox/sell_bitbox_cubit_test.dart +++ b/test/screens/sell_bitbox/sell_bitbox_cubit_test.dart @@ -586,11 +586,93 @@ void main() { }); }); - // retryDeposit's outer catch is reached only if _broadcastDepositAndConfirm - // itself raises — which in practice means an emit() against a closed cubit - // (the inner branch already swallows all broadcast/confirm errors into a - // DepositRetry state). Pin that defensive path so a regression that turned - // the helper async-throwy would surface here, not in prod. + group('retryDeposit idempotency (issue #657 P4 BB1)', () { + test( + 'a retry after broadcast-succeeded/confirm-failed confirms only and ' + 'must NOT re-broadcast the on-chain deposit tx', + () async { + final creds = FakeBitboxCredentials(); + when(() => account.primaryAddress).thenReturn(creds); + when(() => sellService.createUnsignedTransactions(any())).thenAnswer( + (_) async => const RealUnitUnsignedTransactionsRequestDto( + swap: '0xdeadbeef', + deposit: '0xcafebabe', + ), + ); + // Both broadcasts (swap + deposit) succeed … + when(() => sellService.broadcastTransaction(any(), any())) + .thenAnswer((_) async => '0xtxhash'); + // … but the payment confirmation fails once, then succeeds. + var confirms = 0; + when(() => sellService.confirmPaymentWithTxHash(any(), any())) + .thenAnswer((_) async { + confirms++; + if (confirms == 1) throw Exception('confirm backend down'); + }); + + final cubit = build(); + await cubit.stream.firstWhere((s) => s is SellBitboxEthReady); + await cubit.proceedToSwap(); + await cubit.confirmSwap(); + await cubit.confirmDeposit(); + + // The deposit IS on-chain; the retry state must carry its txHash. + final retry = cubit.state; + expect(retry, isA()); + expect((retry as SellBitboxDepositRetry).broadcastTxHash, '0xtxhash'); + verify(() => sellService.broadcastTransaction(any(), any())).called(2); + verify(() => sellService.confirmPaymentWithTxHash(any(), any())).called(1); + + await cubit.retryDeposit(); + + // Success via confirm-only — not a single additional broadcast (the + // old behaviour re-broadcast the already-sent tx, looping forever). + expect(cubit.state, isA()); + verifyNever(() => sellService.broadcastTransaction(any(), any())); + verify(() => sellService.confirmPaymentWithTxHash(any(), any())).called(1); + }, + ); + + test('a retry after a FAILED broadcast may safely broadcast again', () async { + final creds = FakeBitboxCredentials(); + when(() => account.primaryAddress).thenReturn(creds); + when(() => sellService.createUnsignedTransactions(any())).thenAnswer( + (_) async => const RealUnitUnsignedTransactionsRequestDto( + swap: '0xdeadbeef', + deposit: '0xcafebabe', + ), + ); + // Swap broadcast succeeds, the first deposit broadcast fails, the + // re-broadcast on retry succeeds. + var broadcasts = 0; + when(() => sellService.broadcastTransaction(any(), any())).thenAnswer((_) async { + broadcasts++; + if (broadcasts == 2) throw Exception('mempool hiccup'); + return '0xok'; + }); + when(() => sellService.confirmPaymentWithTxHash(any(), any())) + .thenAnswer((_) async {}); + + final cubit = build(); + await cubit.stream.firstWhere((s) => s is SellBitboxEthReady); + await cubit.proceedToSwap(); + await cubit.confirmSwap(); + await cubit.confirmDeposit(); + + // Deposit broadcast failed → nothing on-chain → no txHash carried. + final retry = cubit.state; + expect(retry, isA()); + expect((retry as SellBitboxDepositRetry).broadcastTxHash, isNull); + + await cubit.retryDeposit(); + + expect(cubit.state, isA()); + }); + }); + + // A StateError from an emit() against a closed cubit must surface to the + // caller (retryDeposit performs no emit-gating of its own); pin that the + // closed-cubit path throws instead of silently corrupting state. group('retryDeposit defensive outer-catch', () { test( 'StateError from emit-after-close is caught and the cubit is left intact',