fix: prevent cleared Bolt11 invoices from appearing in transaction history#111
fix: prevent cleared Bolt11 invoices from appearing in transaction history#111Delgado74 wants to merge 4 commits into
Conversation
…g transactions When a user generates a Bolt11 invoice from the receive screen and presses 'Clear invoice', the invoice remained visible in the transaction history because _clearInvoice() only cleared local UI state without cancelling the pending invoice on the LNBits server. Add cancelInvoice() method to InvoiceService that attempts DELETE on LNBits API endpoints. If the server does not support cancellation, the payment hash is stored in a shared set and filtered from the history screen locally.
…story When a user generated a Bolt11 invoice on the receive screen and then pressed 'Clear invoice', or when the 10-minute monitoring timeout expired, or when navigating away, the invoice remained visible in the transaction history. This happened because the invoice was only removed from local UI state but was never cancelled on the LNBits server, and no local record of the cleared invoice was kept. Key changes: - Add cancelInvoice() to InvoiceService: attempts DELETE on LNBits API endpoints to cancel the pending invoice server-side - Add ClearedInvoiceStore: persists cleared invoice payment hashes to SharedPreferences as JSON, surviving app restarts and crashes - Add _discardInvoice() helper: used by both clear and timeout paths to ensure the hash is always recorded before async cancellation - Update HistoryScreen filter: excludes pending transactions whose payment hash is in the cleared set All discard paths (_clearInvoice, timeout, dispose) now record the hash synchronously before async cancellation is attempted, eliminating race conditions between clearing and history navigation.
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughAdds a persisted ClearedInvoiceStore, loads it at app startup, records cleared/discarded invoice hashes from ReceiveScreen (and attempts server cancellation), and filters those hashes out of HistoryScreen results. ChangesCleared Invoice Tracking and Cancellation
|App startup initialization |Invoice cancellation API |ReceiveScreen invoice tracking and cancellation |HistoryScreen cleared invoice filtering Sequence DiagramsequenceDiagram
participant User
participant ReceiveScreen
participant ClearedInvoiceStore
participant InvoiceService
participant LNBits
participant HistoryScreen
User->>ReceiveScreen: Generate invoice
ReceiveScreen->>ReceiveScreen: set _generatedInvoice
User->>ReceiveScreen: Clear / Discard / Timeout
ReceiveScreen->>ClearedInvoiceStore: add(paymentHash)
ReceiveScreen->>InvoiceService: _tryCancelInvoiceOnServer(paymentHash) (async)
InvoiceService->>LNBits: DELETE /api/v1/payments/{paymentHash}
alt Server cancellation succeeds
LNBits-->>InvoiceService: 200/204
else Server cancellation fails
InvoiceService->>LNBits: DELETE /api/v1/wallet/payment/{paymentHash}
end
User->>HistoryScreen: View transaction history
HistoryScreen->>ClearedInvoiceStore: contains(paymentHash)?
ClearedInvoiceStore-->>HistoryScreen: exclude from display
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@lib/screens/9receive_screen.dart`:
- Around line 129-131: The discard/dispose paths currently only record cleared
invoices locally via
ClearedInvoiceStore.instance.add(_generatedInvoice!.paymentHash) (seen where
_generatedInvoice is handled) but do not attempt server cancellation; update
those paths (the discard and dispose flows that call
ClearedInvoiceStore.instance.add and any similar local-only branches) to also
fire a best-effort, unawaited cancellation by calling
unawaited(_tryCancelInvoiceOnServer(hash)) with the same payment hash (e.g., use
_generatedInvoice!.paymentHash or the local hash variable) so the upstream
pending invoice is best-effort cancelled while preserving the existing local
recording.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6886fec3-b482-4f11-9b19-6de8f9e4abec
📒 Files selected for processing (5)
lib/main.dartlib/screens/7history_screen.dartlib/screens/9receive_screen.dartlib/services/cleared_invoice_store.dartlib/services/invoice_service.dart
CodeRabbit review found that _discardInvoice() and dispose() only recorded the cleared invoice hash locally but did not fire a best-effort server cancellation. This could leave pending invoices active on LNBits even when the user navigated away or the monitoring timeout fired. Add unawaited(_tryCancelInvoiceOnServer(hash)) to both paths so the DELETE attempt is always made regardless of how the invoice is discarded.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/screens/9receive_screen.dart (1)
122-136:⚠️ Potential issue | 🔴 Critical | ⚡ Quick win
context.readfromdispose()is unsafe andwallet.inKeyis the wrong credential for server cancellation.The call to
_tryCancelInvoiceOnServerfromdispose()(line 133) invokescontext.read<WalletProvider>()andcontext.read<AuthProvider>()(lines 824–825), which is unsafe during widget teardown. Per Flutter provider documentation, this can throw "Looking up a deactivated widget's ancestor is unsafe," and the error will be silently swallowed by the emptycatch (_)at line 836, leaving invoices un-cancelled with no signal.Additionally, the method passes
wallet.inKeyas theadminKeyparameter tocancelInvoice()(line 828). SinceWalletInfohas bothadminKeyandinKeyfields, and only the admin key can perform DELETE operations on the LNBits API, this will fail to cancel invoices on the server. This same bug appears increateInvoice()andcheckInvoiceStatus()calls.Fix: Cache
serverUrl,wallet, and auth data after invoice generation into_Statefields and pass them directly to_tryCancelInvoiceOnServerinstead of resolving fromcontext. Usewallet.adminKeyinstead ofwallet.inKeyfor all server operations.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/screens/9receive_screen.dart` around lines 122 - 136, The dispose() call is unsafe because _tryCancelInvoiceOnServer currently resolves WalletProvider/AuthProvider from context and uses wallet.inKey; change the flow to cache serverUrl, WalletInfo, and auth credentials into State fields when you generate the invoice (e.g., where _generatedInvoice is set), then have dispose() call unawaited(_tryCancelInvoiceOnServer(cachedServerUrl, cachedWallet, cachedAuth, ...)) using those cached fields; also update all server calls (createInvoice(), checkInvoiceStatus(), _tryCancelInvoiceOnServer) to use wallet.adminKey instead of wallet.inKey so DELETE/auth operations use the admin credential. Ensure _tryCancelInvoiceOnServer signature and calls are updated to accept the cached values rather than reading from context.
🧹 Nitpick comments (1)
lib/screens/9receive_screen.dart (1)
790-820: ⚡ Quick winDe-duplicate
_clearInvoiceand_discardInvoice.The bodies are identical except for the trailing snackbar. Have
_clearInvoicedelegate to_discardInvoiceand then show the snackbar — keeps the cleanup/recording logic in one place so future tweaks (e.g., capturing the server credentials suggested above) only need to land once.♻️ Proposed refactor
void _clearInvoice() { - _invoicePaymentTimer?.cancel(); - _invoicePaymentTimeoutTimer?.cancel(); - - if (_generatedInvoice != null) { - final hash = _generatedInvoice!.paymentHash; - ClearedInvoiceStore.instance.add(hash); - unawaited(_tryCancelInvoiceOnServer(hash)); - } - - setState(() { - _generatedInvoice = null; - }); + _discardInvoice(); _showAccentSnackBar( icon: Icons.check_circle, message: AppLocalizations.of(context)!.invoice_cleared_message, ); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/screens/9receive_screen.dart` around lines 790 - 820, Refactor to remove duplicate cleanup logic by making _discardInvoice contain the shared cancellation/clearing/recording logic (cancel _invoicePaymentTimer and _invoicePaymentTimeoutTimer, add paymentHash to ClearedInvoiceStore, call unawaited(_tryCancelInvoiceOnServer(hash)), and setState to clear _generatedInvoice), then have _clearInvoice simply call _discardInvoice() and afterwards call _showAccentSnackBar(...) to display the success message; update references to paymentHash, ClearedInvoiceStore.instance, _tryCancelInvoiceOnServer, and setState in those functions to ensure behavior is identical after delegating.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@lib/screens/9receive_screen.dart`:
- Around line 130-134: The call to ClearedInvoiceStore.instance.add(hash)
returns a Future and must be wrapped with unawaited(...) to avoid dropping the
Future and to match the surrounding pattern (_tryCancelInvoiceOnServer is
already unawaited); replace direct calls to
ClearedInvoiceStore.instance.add(hash) with
unawaited(ClearedInvoiceStore.instance.add(hash)) at all three call sites in
this file (the branch guarded by _generatedInvoice != null and the other two
occurrences referenced in the review) so persistence I/O errors are not silently
discarded.
- Around line 822-837: In _tryCancelInvoiceOnServer replace the read-only
invoice key with the wallet's admin key when calling
InvoiceService.cancelInvoice: pass wallet.adminKey (not wallet.inKey) as the
adminKey argument to ensure the LNBits DELETE endpoint is authorized; locate the
call to cancelInvoice inside _tryCancelInvoiceOnServer and update the adminKey
parameter to use wallet.adminKey.
---
Outside diff comments:
In `@lib/screens/9receive_screen.dart`:
- Around line 122-136: The dispose() call is unsafe because
_tryCancelInvoiceOnServer currently resolves WalletProvider/AuthProvider from
context and uses wallet.inKey; change the flow to cache serverUrl, WalletInfo,
and auth credentials into State fields when you generate the invoice (e.g.,
where _generatedInvoice is set), then have dispose() call
unawaited(_tryCancelInvoiceOnServer(cachedServerUrl, cachedWallet, cachedAuth,
...)) using those cached fields; also update all server calls (createInvoice(),
checkInvoiceStatus(), _tryCancelInvoiceOnServer) to use wallet.adminKey instead
of wallet.inKey so DELETE/auth operations use the admin credential. Ensure
_tryCancelInvoiceOnServer signature and calls are updated to accept the cached
values rather than reading from context.
---
Nitpick comments:
In `@lib/screens/9receive_screen.dart`:
- Around line 790-820: Refactor to remove duplicate cleanup logic by making
_discardInvoice contain the shared cancellation/clearing/recording logic (cancel
_invoicePaymentTimer and _invoicePaymentTimeoutTimer, add paymentHash to
ClearedInvoiceStore, call unawaited(_tryCancelInvoiceOnServer(hash)), and
setState to clear _generatedInvoice), then have _clearInvoice simply call
_discardInvoice() and afterwards call _showAccentSnackBar(...) to display the
success message; update references to paymentHash, ClearedInvoiceStore.instance,
_tryCancelInvoiceOnServer, and setState in those functions to ensure behavior is
identical after delegating.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: dd069189-1d7f-4f56-8660-e129b4533a28
📒 Files selected for processing (1)
lib/screens/9receive_screen.dart
| if (_generatedInvoice != null) { | ||
| final hash = _generatedInvoice!.paymentHash; | ||
| ClearedInvoiceStore.instance.add(hash); | ||
| unawaited(_tryCancelInvoiceOnServer(hash)); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm ClearedInvoiceStore.add signature
fd -t f 'cleared_invoice_store.dart' | xargs catRepository: lachispame/lachispa
Length of output: 1204
🏁 Script executed:
#!/bin/bash
# Find the receive screen file
fd -t f 'receive_screen.dart' -o '.*receive.*\.dart'Repository: lachispame/lachispa
Length of output: 217
🏁 Script executed:
#!/bin/bash
# Find the receive screen file correctly
fd 'receive' --type fRepository: lachispame/lachispa
Length of output: 129
🏁 Script executed:
#!/bin/bash
# Check line count and then examine the specific lines
wc -l lib/screens/9receive_screen.dart
echo "---"
# Extract the three relevant sections
echo "Lines 128-136:"
sed -n '128,136p' lib/screens/9receive_screen.dart
echo "---"
echo "Lines 791-799:"
sed -n '791,799p' lib/screens/9receive_screen.dart
echo "---"
echo "Lines 810-818:"
sed -n '810,818p' lib/screens/9receive_screen.dartRepository: lachispame/lachispa
Length of output: 1042
Wrap ClearedInvoiceStore.instance.add(hash) with unawaited() at all three call sites.
ClearedInvoiceStore.add() returns Future<void> and performs async SharedPreferences I/O. The current code at lines 132, 796, and 814 drops the returned Future without unawaited(), which violates the unawaited_futures lint and silently discards persistence errors. Wrap each call in unawaited(...) for consistency with the adjacent _tryCancelInvoiceOnServer call.
♻️ Proposed change (apply at all three sites)
- ClearedInvoiceStore.instance.add(hash);
+ unawaited(ClearedInvoiceStore.instance.add(hash));
unawaited(_tryCancelInvoiceOnServer(hash));Also applies to: 796, 814
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@lib/screens/9receive_screen.dart` around lines 130 - 134, The call to
ClearedInvoiceStore.instance.add(hash) returns a Future and must be wrapped with
unawaited(...) to avoid dropping the Future and to match the surrounding pattern
(_tryCancelInvoiceOnServer is already unawaited); replace direct calls to
ClearedInvoiceStore.instance.add(hash) with
unawaited(ClearedInvoiceStore.instance.add(hash)) at all three call sites in
this file (the branch guarded by _generatedInvoice != null and the other two
occurrences referenced in the review) so persistence I/O errors are not silently
discarded.
| Future<void> _tryCancelInvoiceOnServer(String paymentHash) async { | ||
| try { | ||
| final walletProvider = context.read<WalletProvider>(); | ||
| final authProvider = context.read<AuthProvider>(); | ||
| final serverUrl = authProvider.sessionData?.serverUrl; | ||
| final wallet = walletProvider.primaryWallet; | ||
|
|
||
| if (serverUrl == null || wallet == null) return; | ||
|
|
||
| await _invoiceService.cancelInvoice( | ||
| serverUrl: serverUrl, | ||
| adminKey: wallet.inKey, | ||
| paymentHash: paymentHash, | ||
| ); | ||
| } catch (_) {} | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect InvoiceService.cancelInvoice to confirm which key the endpoint expects
fd -t f 'invoice_service.dart' | xargs catRepository: lachispame/lachispa
Length of output: 50375
🏁 Script executed:
rg -A 20 'Future.*cancelInvoice' lib/services/Repository: lachispame/lachispa
Length of output: 1376
🏁 Script executed:
rg -A 40 'Future<bool> cancelInvoice' lib/services/invoice_service.dartRepository: lachispame/lachispa
Length of output: 1355
🏁 Script executed:
rg -B 5 -A 15 '_tryCancelInvoiceOnServer' lib/screens/9receive_screen.dartRepository: lachispame/lachispa
Length of output: 2176
🏁 Script executed:
fd -t f 'wallet' lib/models/ | xargs grep -l 'class.*Wallet' | head -1 | xargs grep -A 30 'class.*Wallet' | grep -E '(inKey|adminKey)'Repository: lachispame/lachispa
Length of output: 313
Pass wallet.adminKey instead of wallet.inKey to cancelInvoice.
_tryCancelInvoiceOnServer at lines 822–837 passes wallet.inKey as adminKey to InvoiceService.cancelInvoice, but the LNBits DELETE endpoint for cancelling invoices requires the wallet's admin key, not the read-only invoice key. Using inKey causes a 401/403 response, which the empty catch (_) {} silently swallows. The invoice then remains live on the server while appearing cancelled locally.
Change line 833 to:
adminKey: wallet.adminKey,
The wallet model already provides both keys separately (see line 144: setAuthHeaders(wallet.inKey, wallet.adminKey)).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@lib/screens/9receive_screen.dart` around lines 822 - 837, In
_tryCancelInvoiceOnServer replace the read-only invoice key with the wallet's
admin key when calling InvoiceService.cancelInvoice: pass wallet.adminKey (not
wallet.inKey) as the adminKey argument to ensure the LNBits DELETE endpoint is
authorized; locate the call to cancelInvoice inside _tryCancelInvoiceOnServer
and update the adminKey parameter to use wallet.adminKey.
…-removes-from-history
Delgado74
force-pushed
the
fix/clear-invoice-removes-from-history
branch
from
7079d97 to
52471dd
Compare
1 participant
Description
Fixes #107. When a user generates a Bolt11 invoice from the receive screen and clears it (via the "Clear invoice" button, 10-minute timeout, or navigating away), the invoice should not appear in the transaction history. Previously, it remained visible because the invoice was only removed from local UI state without any cancellation on the LNBits server or local record of the cleared invoice.
Changes
Why additional modifications were needed beyond issue #107 proposals
The original issue proposed four options (A-D), with Option D (hybrid: server cancel + local filtering) as the recommended approach. During implementation, three additional problems were discovered that required going beyond the original proposal:
**Race condition in **: The original Option D added the payment hash to the cleared set only inside the async method — AFTER an HTTP call to LNBits. If the user navigated to the history screen before this async operation completed, the hash wasn’t in the set yet, and the invoice appeared.
Timeout and dispose were not covered: The 10-minute invoice monitoring timeout handler and the widget’s method (when the user navigates away) both cleared locally but never recorded the hash anywhere. These paths left pending invoices visible in history.
No persistence across app restarts: The original proposals only kept the cleared hashes in an in-memory . If the app was closed and reopened (or crashed), the set was empty and ALL previously cleared invoices reappeared in history.
To solve all three, we added:
lib/services/cleared_invoice_store.dart): a singleton that persists cleared payment hashes to as JSON, surviving app restarts and crashes.9receive_screen.dart): a synchronous helper that records the hash to the persistent store before any async operation. Used by all three discard paths (clear button, timeout, dispose).Files changed
lib/services/cleared_invoice_store.dartlib/services/invoice_service.dartcancelInvoice()method attempting DELETE on LNBits API endpointslib/screens/9receive_screen.dart_discardInvoice()helper. Updated_clearInvoice(), timeout handler, anddispose()to synchronously record hash. Simplified_tryCancelInvoiceOnServer()lib/screens/7history_screen.dart_filteredTransactionsnow excludes pending transactions whose hash is inClearedInvoiceStorelib/main.dartClearedInvoiceStoreon app startupSummary by CodeRabbit
New Features
Bug Fixes