feat: add Invoice Key QR screen for LaChispaPOS integration#99
Open
Delgado74 wants to merge 6 commits into
Open
feat: add Invoice Key QR screen for LaChispaPOS integration#99Delgado74 wants to merge 6 commits into
Delgado74 wants to merge 6 commits into
Conversation
- Add InvoiceKeyScreen with QR code display for readKey - Use readKey with fallback to inKey (least privilege) - Add 'Invoice Key QR' option in Settings screen - Update localizations for all 7 supported languages - Make readKey required in WalletInfo model - Update WalletProvider to forward readKey when updating wallet Resolves lachispame#68
- Add invoice_key localization strings for all 7 languages - Add currency_count plural support for localized currency display - Regenerate localization files
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (8)
📒 Files selected for processing (8)
✅ Files skipped from review due to trivial changes (3)
🚧 Files skipped from review as they are similar to previous changes (5)
📝 WalkthroughWalkthroughAdds an Invoice Key QR feature: WalletInfo readKey, localized strings (7 locales), Settings navigation tile, a new InvoiceKeyScreen (QR, masked/unmasked key, copy), and WalletProvider formatting edits. ChangesInvoice Key QR Display
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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: 2
🧹 Nitpick comments (1)
lib/screens/19invoice_key_screen.dart (1)
119-121: 💤 Low valueSimplify the key selection logic for better readability.
The current nested ternary with repeated null-coalescing is harder to read than necessary. Consider extracting the
readKeycheck to a local variable:♻️ Proposed refactor
- final invoiceKey = (wallet?.readKey ?? '').isEmpty - ? (wallet?.inKey ?? '') - : (wallet?.readKey ?? ''); + final readKey = wallet?.readKey ?? ''; + final invoiceKey = readKey.isNotEmpty ? readKey : (wallet?.inKey ?? '');🤖 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/19invoice_key_screen.dart` around lines 119 - 121, The ternary expression used to compute invoiceKey is repetitive and hard to read; simplify by extracting wallet?.readKey into a local variable (e.g., final readKey = wallet?.readKey ?? '') then compute invoiceKey using that variable and fallback to wallet?.inKey (e.g., invoiceKey = readKey.isEmpty ? (wallet?.inKey ?? '') : readKey) so you avoid repeating the null-coalescing and nested ternary and improve readability around the invoiceKey computation.
🤖 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/models/wallet_info.dart`:
- Line 7: The constructor change for class WalletInfo made the field readKey
required, which breaks callers that construct WalletInfo directly; change the
WalletInfo constructor to make readKey optional with a default (e.g., empty
string) so existing code continues to compile, while leaving fromJson
(WalletInfo.fromJson) unchanged (it already falls back to ''), and ensure the
field declaration final String readKey remains compatible with the new
constructor signature.
- Line 7: The Invoice Key QR screen is preferring readKey over inKey which is
reversed; update the logic in the invoice_key_screen widget (the code that
selects the key for the QR and the security text) to prefer walletInfo.inKey
first and only fall back to walletInfo.readKey when inKey is absent, and ensure
any displayed warning text and QR payload reference the chosen key (symbols:
inKey, readKey, createInvoice(), checkInvoiceStatus()) so POS devices receive
the invoice-creation key.
---
Nitpick comments:
In `@lib/screens/19invoice_key_screen.dart`:
- Around line 119-121: The ternary expression used to compute invoiceKey is
repetitive and hard to read; simplify by extracting wallet?.readKey into a local
variable (e.g., final readKey = wallet?.readKey ?? '') then compute invoiceKey
using that variable and fallback to wallet?.inKey (e.g., invoiceKey =
readKey.isEmpty ? (wallet?.inKey ?? '') : readKey) so you avoid repeating the
null-coalescing and nested ternary and improve readability around the invoiceKey
computation.
🪄 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: ebb27c17-05a0-4947-9f7c-a6cebfff42f4
⛔ Files ignored due to path filters (8)
lib/l10n/generated/app_localizations.dartis excluded by!**/generated/**lib/l10n/generated/app_localizations_de.dartis excluded by!**/generated/**lib/l10n/generated/app_localizations_en.dartis excluded by!**/generated/**lib/l10n/generated/app_localizations_es.dartis excluded by!**/generated/**lib/l10n/generated/app_localizations_fr.dartis excluded by!**/generated/**lib/l10n/generated/app_localizations_it.dartis excluded by!**/generated/**lib/l10n/generated/app_localizations_pt.dartis excluded by!**/generated/**lib/l10n/generated/app_localizations_ru.dartis excluded by!**/generated/**
📒 Files selected for processing (11)
lib/l10n/app_de.arblib/l10n/app_en.arblib/l10n/app_es.arblib/l10n/app_fr.arblib/l10n/app_it.arblib/l10n/app_pt.arblib/l10n/app_ru.arblib/models/wallet_info.dartlib/providers/wallet_provider.dartlib/screens/17settings_screen.dartlib/screens/19invoice_key_screen.dart
| final String name; | ||
| final String adminKey; | ||
| final String inKey; | ||
| final String readKey; |
There was a problem hiding this comment.
Breaking change: making readKey required may break existing code.
The constructor parameter is marked required (line 15), but this introduces a breaking change for any code that constructs WalletInfo objects directly. While fromJson safely handles missing values with an empty-string fallback (line 28), callers using the constructor directly will fail to compile without providing readKey.
Consider making readKey optional with a default value:
final String readKey;
WalletInfo({
required this.id,
required this.name,
required this.adminKey,
required this.inKey,
this.readKey = '', // Default to empty string
required this.balanceMsat,
});This preserves backward compatibility while still supporting the new field.
Also applies to: 15-15
🤖 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/models/wallet_info.dart` at line 7, The constructor change for class
WalletInfo made the field readKey required, which breaks callers that construct
WalletInfo directly; change the WalletInfo constructor to make readKey optional
with a default (e.g., empty string) so existing code continues to compile, while
leaving fromJson (WalletInfo.fromJson) unchanged (it already falls back to ''),
and ensure the field declaration final String readKey remains compatible with
the new constructor signature.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify LNBits key permission model and check if readKey can create invoices
rg -n -C3 'readKey|inKey' --type=dart -g '!**/l10n/**'Repository: lachispame/lachispa
Length of output: 30508
🏁 Script executed:
# Search for the security warning mentioned in the review (app_en.arb:408)
find . -name "app_*.arb" -type f | head -5Repository: lachispame/lachispa
Length of output: 173
🏁 Script executed:
# Look at the invoice key screen more closely
sed -n '115,125p' lib/screens/19invoice_key_screen.dartRepository: lachispame/lachispa
Length of output: 490
🏁 Script executed:
# Check the app localization file for the security warning
rg -n "create invoices" -A2 -B2 --type=jsonRepository: lachispame/lachispa
Length of output: 212
🏁 Script executed:
# Verify what operations actually use readKey vs inKey throughout the codebase
rg -n "\.readKey|\.inKey" lib/screens/ lib/services/ --type=dart | grep -E "(createInvoice|checkInvoice|readKey)" | head -20Repository: lachispame/lachispa
Length of output: 245
🏁 Script executed:
# Check the security warning in app_en.arb around line 408
sed -n '400,420p' lib/l10n/app_en.arbRepository: lachispame/lachispa
Length of output: 1044
🏁 Script executed:
# Search for references to the invoice key screen and its purpose
rg -n "invoice.*key|InvoiceKey" lib/screens/19invoice_key_screen.dart -A5 -B5 --type=dart | head -40Repository: lachispame/lachispa
Length of output: 1400
🏁 Script executed:
# Check if readKey is used anywhere for actual invoice creation
rg -n "readKey" lib/services/ --type=dart -A2 -B2Repository: lachispame/lachispa
Length of output: 45
🏁 Script executed:
# Look at the LNBits documentation or comments about key permissions
rg -n "readkey|inkey|read.*only|invoice.*creation" -i --type=dart lib/models/ lib/services/ | head -20Repository: lachispame/lachispa
Length of output: 1657
🏁 Script executed:
# Final verification: check if readKey is ever used for any write operations
rg -n "readKey" lib/ --type=dart -A3 | grep -E "create|send|pay|invoice"Repository: lachispame/lachispa
Length of output: 527
🏁 Script executed:
# Confirm: is readKey used anywhere except in invoice_key_screen?
rg -n "readKey" lib/ --type=dart --files-with-matchesRepository: lachispame/lachispa
Length of output: 164
Prefer inKey over readKey for the Invoice Key QR screen: the keys are reversed.
The logic in invoice_key_screen.dart (lines 119–121) currently prefers readKey and falls back to inKey. This is backwards:
- LNBits key semantics:
readKeyis read-only;inKeycreates invoices - Screen purpose: "Use this QR code with LaChispaPOS or other Lightning apps to receive payments" — requires invoice creation capability
- Security warning: Correctly states "This key allows third parties to create invoices" — describing
inKey, notreadKey - All invoice operations throughout the codebase use
inKey:createInvoice(),checkInvoiceStatus(), and Lightning Address setup readKeyis unused for any write operations in the entire codebase
The current implementation exposes a read-only key to POS devices, breaking payment reception. Swap the preference: use inKey first, fall back to readKey.
🤖 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/models/wallet_info.dart` at line 7, The Invoice Key QR screen is
preferring readKey over inKey which is reversed; update the logic in the
invoice_key_screen widget (the code that selects the key for the QR and the
security text) to prefer walletInfo.inKey first and only fall back to
walletInfo.readKey when inKey is absent, and ensure any displayed warning text
and QR payload reference the chosen key (symbols: inKey, readKey,
createInvoice(), checkInvoiceStatus()) so POS devices receive the
invoice-creation key.
…-clean # Conflicts: # lib/screens/17settings_screen.dart
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Resolves #68
Description:
Adds a dedicated screen to display the invoice key (readKey) as a QR code, accessible from Settings. Enables POS integration and allows external apps to read the invoice key.
How it works:
InvoiceKeyScreenaccessible from Settings > Invoice Key QRreadKey) as a QR codeUse Cases:
Files Modified:
lib/screens/19invoice_key_screen.dart- New screen with QR code displaylib/screens/17settings_screen.dart- Added link to Invoice Key screen in Settings menuSummary by CodeRabbit
New Features
Localization