refactor: consolidate business logic in FmcdCore and fix balance calc…#10
refactor: consolidate business logic in FmcdCore and fix balance calc…#10
Conversation
…ulation - Fix zero balance bug by using client.get_balance() instead of only counting mint notes - Remove duplicate implementations between FmcdCore and API layers - Implement PaymentInfoResolver trait pattern for LNURL handling without coupling core to web protocols - Move all business logic (get_info, join_federation, create_invoice, pay_invoice) to FmcdCore - Make API layer a thin wrapper that delegates to FmcdCore methods - Add proper error handling and context propagation throughout
There was a problem hiding this comment.
Pull Request Overview
This PR consolidates business logic into the FmcdCore module and fixes a critical balance calculation bug. The refactoring implements a clear separation between the core business logic and the API layer, making the API a thin wrapper that delegates to the core.
- Fix zero balance bug by using
client.get_balance()instead of only counting mint notes - Consolidate all business logic (get_info, join_federation, create_invoice, pay_invoice, withdrawal, deposit) into
FmcdCore - Implement
PaymentInfoResolvertrait pattern for LNURL handling without coupling core to web protocols
Reviewed Changes
Copilot reviewed 16 out of 17 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
src/state.rs |
Remove unused import and fix multimint accessor to use Arc deref |
src/core/operations/mod.rs |
Clean up unnecessary re-exports |
src/core/mod.rs |
Major refactor: add PaymentInfoResolver trait, move business logic to FmcdCore, fix balance calculation |
src/api/rest/onchain/withdraw.rs |
Remove duplicate withdrawal logic, delegate to core |
src/api/rest/onchain/deposit_address.rs |
Remove duplicate deposit logic, delegate to core |
src/api/rest/ln/stream.rs |
Move types to core and add local InvoiceStatusUpdate |
src/api/rest/ln/status.rs |
Update import paths for moved types |
src/api/rest/ln/pay.rs |
Remove duplicate payment logic, delegate to core with resolver |
src/api/rest/ln/mod.rs |
Remove LNURL resolution logic (moved to resolver) |
src/api/rest/ln/invoice.rs |
Remove duplicate invoice logic, delegate to core |
src/api/rest/admin/mod.rs |
Remove unused helper function |
src/api/rest/admin/join.rs |
Remove duplicate join logic, delegate to core |
src/api/rest/admin/info.rs |
Remove duplicate info logic, delegate to core |
src/api/resolvers/payment.rs |
New LNURL resolver implementing PaymentInfoResolver trait |
src/api/resolvers/mod.rs |
New module for payment resolvers |
src/api/mod.rs |
Export new resolvers module |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| let mut multimint = (*self.multimint).clone(); | ||
|
|
||
| let this_federation_id = | ||
| multimint |
There was a problem hiding this comment.
Cloning the entire MultiMint for federation registration is expensive. Consider if this clone is necessary or if the operation can be performed on the Arc directly.
| multimint | |
| // Register new federation directly on Arc<MultiMint> | |
| let this_federation_id = | |
| self.multimint |
| let info = payment_info.trim(); | ||
|
|
||
| // First check if it's already a Bolt11 invoice | ||
| if let Ok(invoice) = Bolt11Invoice::from_str(info) { |
There was a problem hiding this comment.
Parsing a Bolt11Invoice twice (once here for validation, then again in the core) is inefficient. Consider returning the parsed invoice or restructuring to avoid duplicate parsing.
| network: wallet_client.get_network().to_string(), | ||
| meta: client.config().await.global.meta.clone(), | ||
| total_amount_msat: summary.total_amount(), | ||
| total_amount_msat: total_balance, |
There was a problem hiding this comment.
The balance fix uses client.get_balance() which includes all modules, but the response structure suggests it should only represent mint module data based on the comments about 'ecash notes'. This could be misleading to API consumers who expect only mint balance.
1 participant
…ulation