refactor: consolidate business logic in FmcdCore and fix balance calc… #12
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 refactors the codebase to consolidate business logic in FmcdCore and fix a zero balance calculation bug. The main purpose is to move all core business operations from the API layer into FmcdCore, making the API layer a thin wrapper that delegates to core methods.
Key changes include:
- Fix balance calculation by using
client.get_balance()instead of only counting mint notes - Implement
PaymentInfoResolvertrait pattern for LNURL handling without coupling core to web protocols - Move business logic methods (
get_info,join_federation,create_invoice,pay_invoice,create_deposit_address,withdraw_onchain) toFmcdCore
Reviewed Changes
Copilot reviewed 16 out of 17 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
src/state.rs |
Remove unused import and fix dereference pattern for multimint accessor |
src/core/operations/mod.rs |
Clean up duplicate re-exports |
src/core/mod.rs |
Add PaymentInfoResolver trait and consolidate business logic methods in FmcdCore |
src/api/rest/onchain/withdraw.rs |
Convert to thin wrapper delegating to FmcdCore.withdraw_onchain() |
src/api/rest/onchain/deposit_address.rs |
Convert to thin wrapper delegating to FmcdCore.create_deposit_address() |
src/api/rest/ln/pay.rs |
Convert to thin wrapper using resolver pattern for payment info resolution |
src/api/rest/ln/invoice.rs |
Convert to thin wrapper delegating to FmcdCore.create_invoice() |
src/api/rest/admin/join.rs |
Convert to thin wrapper delegating to FmcdCore.join_federation() |
src/api/rest/admin/info.rs |
Convert to thin wrapper delegating to FmcdCore.get_info() |
src/api/resolvers/payment.rs |
New LNURL resolver implementation for payment info resolution |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| /// Main entry point for library consumers | ||
| pub struct FmcdCore { | ||
| pub multimint: MultiMint, | ||
| pub multimint: Arc<MultiMint>, |
There was a problem hiding this comment.
The multimint field changed from MultiMint to Arc<MultiMint> but line 390 clones the dereferenced Arc ((*self.multimint).clone()) which defeats the purpose of using Arc for cheap cloning. This should be self.multimint.clone() to get a cheap Arc clone instead of an expensive MultiMint clone.
| 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 comment on line 141-142 states this field represents 'ecash notes in the mint module' but the implementation now uses client.get_balance() which includes all module balances. The comment should be updated to reflect that this now represents the total balance across all modules.
| pub async fn handle_ws(state: AppState, v: Value) -> Result<Value, AppError> { | ||
| let req = serde_json::from_value::<LnPayRequest>(v) | ||
| .map_err(|e| AppError::validation_error(format!("Invalid request: {}", e)))?; | ||
| let req = serde_json::from_value::<LnPayRequest>(v)?; |
There was a problem hiding this comment.
The error handling was simplified from a detailed validation error to using the ? operator, which changes the error type and message. This should maintain the same error handling pattern: .map_err(|e| AppError::validation_error(format!(\"Invalid request: {}\", e)))?
| let req = serde_json::from_value::<LnPayRequest>(v)?; | |
| let req = serde_json::from_value::<LnPayRequest>(v) | |
| .map_err(|e| AppError::validation_error(format!("Invalid request: {}", e)))?; |
| // Convenience accessors for backward compatibility | ||
| pub fn multimint(&self) -> &MultiMint { | ||
| &self.core.multimint | ||
| &*self.core.multimint |
There was a problem hiding this comment.
This dereference pattern &* is unnecessary and confusing since multimint is now an Arc<MultiMint>. This should be &self.core.multimint which will automatically deref the Arc to &MultiMint.
| &*self.core.multimint | |
| &self.core.multimint |
1 participant
…ulation