Get Balance #6
Get Balance #6
Conversation
WalkthroughThis update enhances the balance handling and node information retrieval across multiple API implementations. The CLN and LND modules have introduced new structures for handling channel and balance information. The Phoenixd module implements a new balance retrieval flow—fetching data from a Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant PhoenixdAPI as Phoenixd API
participant NodeEndpoint as Node Info Endpoint
participant BalanceEndpoint as Balance Endpoint
Client->>PhoenixdAPI: Call get_info()
PhoenixdAPI->>NodeEndpoint: GET request to info_url
NodeEndpoint-->>PhoenixdAPI: Return node info
PhoenixdAPI->>BalanceEndpoint: GET request to balance_url
BalanceEndpoint-->>PhoenixdAPI: Return balance info
PhoenixdAPI->>Client: Return combined NodeInfo
Poem
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🔭 Outside diff range comments (1)
crates/lni/phoenixd/api.rs (1)
6-43: 💡 Verification agent🧩 Analysis chain
Update TypeScript interface to include new balance fields
The NodeInfo struct has been updated with balance fields, but the corresponding TypeScript interface in
bindings/lni_nodejs/index.d.tsneeds to be updated to include these new fields.Run this script to verify the TypeScript interface:
🏁 Script executed:
#!/bin/bash # Check if the TypeScript interface includes the new balance fields cat bindings/lni_nodejs/index.d.ts | grep -A 10 "export interface NodeInfo"Length of output: 280
Action Required: Update the TypeScript NodeInfo Interface
The Rust implementation of
NodeInfonow includes additional balance fields (send_balance_msat,receive_balance_msat,fee_credit_balance_msat,pending_balance_msat, andinactive_balance_msat), but these are missing from the TypeScript interface inbindings/lni_nodejs/index.d.ts. Please update the interface to include these new balance properties. For example, consider adding the following fields (using the camelCase convention to match existing properties):
sendBalanceMsat: number;receiveBalanceMsat: number;feeCreditBalanceMsat: number;pendingBalanceMsat: number;inactiveBalanceMsat: number;Ensure that the updated interface aligns with how the Rust backend populates these values.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
crates/lni/cln/api.rs(1 hunks)crates/lni/lnd/api.rs(1 hunks)crates/lni/phoenixd/api.rs(2 hunks)crates/lni/phoenixd/types.rs(1 hunks)crates/lni/types.rs(1 hunks)
🧰 Additional context used
🧬 Code Definitions (2)
crates/lni/phoenixd/types.rs (1)
bindings/lni_nodejs/index.d.ts (1)
Channel(78-96)
crates/lni/phoenixd/api.rs (2)
bindings/lni_nodejs/index.d.ts (3)
NodeInfo(50-57)PayInvoiceParams(199-210)PayInvoiceResponse(157-161)crates/lni/phoenixd/lib.rs (1)
new(24-29)
🔇 Additional comments (12)
crates/lni/cln/api.rs (2)
27-29: Informative comments for future implementationThese comments provide valuable context for understanding how balance information could be retrieved using the
/v1/listfundsendpoint. The link to the Zeus implementation is particularly helpful as a reference.
38-38: Good use of default initializationUsing
..Default::default()ensures that any unspecified fields in theNodeInfostruct (including the newly added balance fields intypes.rs) will be initialized with their default values. This is a good practice for maintaining robustness as the struct evolves.crates/lni/phoenixd/types.rs (3)
11-11: Appropriate addition of channels to InfoResponseAdding the
channelsfield toInfoResponseproperly extends the struct to include channel information returned by the API.
14-28: Well-structured Channel implementationThe new
Channelstruct appropriately models lightning channel data with proper field naming and serialization mappings. The fields align well with theChannelinterface defined in the Node.js bindings.
30-36: Good GetBalanceResponse implementationThis new struct provides a clear structure for parsing responses from the balance endpoint, with appropriately named fields and correct serialization attributes.
crates/lni/lnd/api.rs (2)
41-45: Informative comments for future implementationThese comments provide valuable context about how balance information can be retrieved using the
/v1/balance/channelsendpoint, with a helpful link to the LND API documentation.
53-53: Good use of default initializationUsing
..Default::default()ensures that any unspecified fields in theNodeInfostruct (including the newly added balance fields intypes.rs) will be initialized with their default values. This is a good practice for maintaining robustness as the struct evolves.crates/lni/types.rs (2)
29-33: Well-designed balance fields for NodeInfoThe addition of these balance-related fields to the
NodeInfostruct provides a comprehensive representation of different balance types. This aligns with the PR objective of enhancing balance handling.
35-50: Appropriate Default implementationThe implementation of
DefaultforNodeInfois well-structured and ensures all fields are properly initialized with sensible default values. This complements the use of..Default::default()in the API implementations.crates/lni/phoenixd/api.rs (3)
6-6: Good addition of GetBalanceResponse importAdding the import for GetBalanceResponse aligns with the implementation of the balance retrieval functionality mentioned in the TODO at line 12.
17-17: Good refactoring: Creating a dedicated info_url variableCreating a new variable for the info URL instead of reusing the url parameter improves clarity and maintainability.
12-12: Good job implementing the get_balance TODOThe implementation now fetches balance information from the
/getbalanceendpoint, addressing this TODO comment.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
bindings/lni_uniffi/src/lni.udl(1 hunks)crates/lni/lnd/api.rs(2 hunks)crates/lni/lnd/types.rs(1 hunks)crates/lni/phoenixd/api.rs(2 hunks)crates/lni/types.rs(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/lni/phoenixd/api.rs
🧰 Additional context used
🧬 Code Definitions (2)
crates/lni/lnd/types.rs (1)
bindings/lni_nodejs/index.d.ts (1)
BalancesResponse(165-168)
crates/lni/types.rs (2)
bindings/lni_nodejs/index.d.ts (1)
NodeInfo(50-57)crates/lni/database.rs (1)
new(38-56)
🔇 Additional comments (9)
bindings/lni_uniffi/src/lni.udl (1)
171-177: Fields added match new functionality for retrieving balance data.These new fields in the
NodeInfodictionary align with the corresponding Rust struct changes incrates/lni/types.rs. They provide comprehensive balance information, including send/receive capacities, unsettled balances, and pending operations.crates/lni/lnd/types.rs (2)
224-235: Well-structured newBalancesResponsetype for LND data.The structure correctly maps to the LND API response format and includes all necessary fields for parsing channel balance data. The fields follow the same pattern as other response structures in this file.
237-241: Good use of optional fields for Amount structure.Using
Option<String>for thesatandmsatfields is appropriate since these values may not always be present in the API response.crates/lni/lnd/api.rs (2)
2-4: Added import for the newBalancesResponsetype.The import is correctly placed with other related types.
41-49: API call for retrieving balance information.Good documentation of the endpoint that's being used, with clear comments explaining the purpose.
crates/lni/types.rs (4)
29-36: Good documentation for new balance fields.The comments clearly explain each field's purpose, making it easy for developers to understand how these values should be used. The Phoenix-specific comment about fee credits is especially helpful.
37-54: Appropriate implementation of Default for NodeInfo.Adding the Default implementation is a good practice, ensuring all fields have sensible initial values. This makes it easier to initialize NodeInfo instances and use the spread operator pattern (
..Default::default()) for partial updates.
287-288: Comment formatting improvement for fee_limit_msat.The comment now clarifies that fee_limit_msat and fee_limit_percentage are mutually exclusive options.
304-305: Comment formatting improvement for timeout_seconds.Small formatting adjustment to the comment aligns with the codebase style.
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
crates/lni/cln/api.rs (1)
14-82: 🛠️ Refactor suggestionAdd error handling for API calls
The
get_infofunction usesunwrap()at multiple points, which could cause the program to panic if an API call fails. Consider implementing proper error handling.- let response = client - .post(&req_url) - .header("Content-Type", "application/json") - .send() - .unwrap(); - let response_text = response.text().unwrap(); + let response = client + .post(&req_url) + .header("Content-Type", "application/json") + .send() + .map_err(|e| ApiError::Api { + reason: format!("Failed to send getinfo request: {}", e) + })?; + let response_text = response.text() + .map_err(|e| ApiError::Api { + reason: format!("Failed to read getinfo response: {}", e) + })?; // Similar changes needed for the funds API call (lines 29-35)Similarly, implement proper error handling for the
listfundsAPI call.
🧹 Nitpick comments (1)
crates/lni/cln/api.rs (1)
45-64: Consider using match statement for channel state handlingThe current implementation uses multiple if-else statements to categorize channels based on their state. Consider refactoring to use a match statement for better maintainability.
- for channel in channels.channels.iter() { - if channel.state == "CHANNELD_NORMAL" && channel.connected { - // Active channels - local_balance += channel.our_amount_msat; - remote_balance += channel.amount_msat - channel.our_amount_msat; - } else if channel.state == "CHANNELD_NORMAL" && !channel.connected { - // Unsettled channels (previously inactive) - unsettled_send_balance_msat += channel.our_amount_msat; - unsettled_receive_balance_msat += channel.amount_msat - channel.our_amount_msat; - } else if channel.state == "CHANNELD_AWAITING_LOCKIN" - || channel.state == "DUALOPEND_AWAITING_LOCKIN" - || channel.state == "DUALOPEND_OPEN_INIT" - || channel.state == "DUALOPEND_OPEN_COMMITTED" - || channel.state == "DUALOPEND_OPEN_COMMIT_READY" - || channel.state == "OPENINGD" { - // Pending open channels - pending_open_send_balance += channel.our_amount_msat; - pending_open_receive_balance += channel.amount_msat - channel.our_amount_msat; - } - } + for channel in channels.channels.iter() { + let remote_amount = channel.amount_msat - channel.our_amount_msat; + + match (channel.state.as_str(), channel.connected) { + ("CHANNELD_NORMAL", true) => { + // Active channels + local_balance += channel.our_amount_msat; + remote_balance += remote_amount; + }, + ("CHANNELD_NORMAL", false) => { + // Unsettled channels (previously inactive) + unsettled_send_balance_msat += channel.our_amount_msat; + unsettled_receive_balance_msat += remote_amount; + }, + ("CHANNELD_AWAITING_LOCKIN", _) | + ("DUALOPEND_AWAITING_LOCKIN", _) | + ("DUALOPEND_OPEN_INIT", _) | + ("DUALOPEND_OPEN_COMMITTED", _) | + ("DUALOPEND_OPEN_COMMIT_READY", _) | + ("OPENINGD", _) => { + // Pending open channels + pending_open_send_balance += channel.our_amount_msat; + pending_open_receive_balance += remote_amount; + }, + _ => {} // Ignoring other channel states + } + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
crates/lni/cln/api.rs(2 hunks)crates/lni/cln/types.rs(1 hunks)
🔇 Additional comments (7)
crates/lni/cln/types.rs (2)
91-97: Well-structured wrapper for channel dataThe
ChannelWrapperstruct is correctly designed to deserialize the API response from thelistfundsendpoint. The#[serde(skip)]attribute on theoutputsfield ensures that only relevant channel data is processed.
99-110: Channel struct models channel data appropriatelyThe
Channelstruct contains all the necessary fields to represent a Lightning Network channel, including connection status, state, balances, and funding information. The field types match the expected data formats.crates/lni/cln/api.rs (5)
2-2: Appropriate import of the new typeThe import has been correctly updated to include the new
ChannelWrappertype.
27-36: Good implementation of balance retrievalThe code correctly implements a new API call to the
listfundsendpoint to retrieve channel data, following the same pattern as the existinggetinfocall.
38-43: Well-structured balance trackingThe code initializes separate variables to track different types of balances, making the accounting logic clear and maintainable.
73-79: Correctly updates NodeInfo with balance informationThe code properly updates the
NodeInfostruct with the calculated balances and uses the..Default::default()syntax to handle any fields not explicitly set.
36-36:❓ Verification inconclusive
Validate JSON response before parsing
The code parses the funds response directly without validating if it's a valid JSON or contains error messages from the API.
🏁 Script executed:
#!/bin/bash # Check the structure of listfunds response curl -s https://api.github.com/repos/ElementsProject/lightning/contents/doc/schemas/listfunds.schema.json | jq -r '.content' | base64 -d | jqLength of output: 238
Attention: Validate JSON Response Before Parsing
The current implementation in
crates/lni/cln/api.rson line 36 directly parses the API response usingserde_json::from_str(&funds_response_text)?without explicit validation. This approach doesn’t check whether the response is valid JSON or if it contains error messages from the API.
- The initial check using the GitHub API for the JSON schema did not produce output because the environment lacks the
base64command.- Without a valid automated check, it’s unclear if the API response might return error messages or malformed JSON that could cause runtime errors.
Please manually verify that the JSON response from the funds API is always valid or add additional validation/error-handling logic to gracefully manage unexpected responses.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
bindings/lni_nodejs/index.d.ts (1)
57-63: Good addition of balance fields to NodeInfoThe new balance fields provide comprehensive tracking of funds in different states, which aligns with the PR objective of enhancing balance handling. This will give users better visibility into their node's financial state.
I notice that while most fields use "Msat" suffix to indicate millsatoshi units, the
pendingOpenSendBalanceandpendingOpenReceiveBalanceproperties don't follow this naming convention.Consider renaming these fields to maintain consistency:
sendBalanceMsat: number receiveBalanceMsat: number feeCreditBalanceMsat: number unsettledSendBalanceMsat: number unsettledReceiveBalanceMsat: number - pendingOpenSendBalance: number - pendingOpenReceiveBalance: number + pendingOpenSendBalanceMsat: number + pendingOpenReceiveBalanceMsat: number
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
bindings/lni_nodejs/index.d.ts(1 hunks)bindings/lni_nodejs/main.mjs(1 hunks)
🔇 Additional comments (1)
bindings/lni_nodejs/main.mjs (1)
145-147: Test configuration updateThis change enables the CLN implementation test while disabling the general test function. This aligns with the PR focus on enhancing balance handling, as it allows for testing the newly added balance fields in the NodeInfo object returned by the CLN implementation.
Summary by CodeRabbit