feat: ldk-node serializable types#434
Conversation
b867493 to
84714ad
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR migrates from custom domain model types to serializable types from the updated ldk-node library, eliminating the need for manual type conversions. The change simplifies the codebase by using ldk-node's native types directly instead of maintaining separate domain models.
Key changes:
- Replaced custom
LnPeer,BalanceDetails, andLightningBalancedomain models with ldk-node's nativePeerDetails,BalanceDetails, andLightningBalancetypes - Added extension functions for
PeerDetailsto provide convenient access patterns and parsing functionality - Updated all references throughout the codebase including services, repositories, ViewModels, and UI components
Reviewed Changes
Copilot reviewed 22 out of 22 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| gradle/libs.versions.toml | Updated ldk-node dependency version to v0.6.2-rc.4 |
| app/src/main/java/to/bitkit/models/LnPeer.kt | Removed custom LnPeer domain model class |
| app/src/main/java/to/bitkit/models/BalanceState.kt | Removed custom BalanceDetails and LightningBalance domain models with mapping functions |
| app/src/main/java/to/bitkit/ext/PeerDetails.kt | Added extension functions for PeerDetails parsing and property access |
| app/src/main/java/to/bitkit/services/LightningService.kt | Updated to use ldk-node types directly instead of domain model conversion |
| app/src/main/java/to/bitkit/repositories/LightningRepo.kt | Updated method signatures and state management to use ldk-node types |
| Multiple ViewModels and UI files | Updated type references from domain models to ldk-node types |
| Multiple test files | Updated imports and mock objects to use ldk-node types |
Comments suppressed due to low confidence (1)
app/src/main/java/to/bitkit/ui/screens/wallets/HomeScreen.kt:1
- This code block appears to be duplicated and should be part of the extracted
Widgetscomposable function. The structure suggests this was moved but some trailing code wasn't properly cleaned up during the refactoring.
package to.bitkit.ui.screens.wallets
3d8ab81 to
fc060f4
Compare
Pull request was converted to draft
fc060f4 to
6d528ab
Compare
a0d006e to
1e3c044
Compare
feat: receive QR code quiet zone and polish
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 32 out of 32 changed files in this pull request and generated 4 comments.
Comments suppressed due to low confidence (1)
app/src/main/java/to/bitkit/services/LightningService.kt:1
- [nitpick] Logging peers with '$peer' relies on PeerDetails.toString(); using peer.uri would produce a consistent, readable 'nodeId@host:port' format matching other areas. Suggest: Logger.info("Connected to trusted peer: ${peer.uri}").
package to.bitkit.services
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
feat: bitkit-core serializable types
jvsena42
left a comment
There was a problem hiding this comment.
Tested channel operations and LN node peer URI
|
@jvsena42 need one final re-approval after last merge with master. |
This PR uses the new serializable types generated in ldk-node PR:
Preview
regtest.mp4
QA Notes
Since this PR changes the balance calculation to use the
ldk-nodetypes directly instead of the previous setup with mapping them to a domain model, a regression test for transfer-to-spending & back to savings is recommended, alongside the main core test to build & run, which verified for no compilation or runtime issues.Then, we also need to check the 2 impacted areas:
LnPeertype with direct use ofPeerDetailsTests: