Add lightning node data to info event #408
Conversation
WalkthroughThe pull request introduces enhancements to the Lightning Network node information retrieval and representation across multiple files. A new Changes
Sequence DiagramsequenceDiagram
participant Main as Main Function
participant LndConnector as LND Connector
participant LnStatus as LN Status
participant Static as LN_STATUS
Main->>LndConnector: Create instance
Main->>LndConnector: get_node_info()
LndConnector-->>Main: GetInfoResponse
Main->>LnStatus: from_get_info_response()
LnStatus-->>Main: LnStatus instance
Main->>Static: Store LnStatus
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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: 2
🧹 Nitpick comments (2)
src/nip33.rs (1)
186-213: Good layering of LN node details, but consider fallback for empty fields.
The code properly tags LN-related fields like version and alias. For improved robustness, you could set fallback values (e.g.,"unknown") if any field (such asnode_alias) is absent or empty.src/lightning/mod.rs (1)
255-263:get_node_infomethod is straightforward but watch for large error messages.
Mapping errors directly toMostroError::LnNodeErroris fine, though you might want to sanitize or shorten the original error messages for user readability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/lightning/mod.rs(2 hunks)src/main.rs(3 hunks)src/nip33.rs(3 hunks)src/scheduler.rs(2 hunks)
🔇 Additional comments (9)
src/nip33.rs (2)
137-137: Use of ln_status parameter for LN tags is a solid approach.
Switching from a mostro_pubkey parameter to a full LnStatus object clarifies that the function intends to incorporate multiple LN-related attributes into the tag set. This change aligns with the new LnStatus struct usage in other parts of the code.
147-147: Confirm environment variable presence and handle missing values.
Referencing env!("GIT_HASH") can raise an error at compile time if the variable is not defined. Ensure build scripts or environment setups are properly handling it.
src/lightning/mod.rs (3)
14-14: Import statements are well-scoped.
The addition of GetInfoRequest and GetInfoResponse is needed for the new retrieval functionality. No issues noted.
19-19: Ordering import is properly used for fee comparison logic.
No action needed since std::cmp::Ordering is already used for comparing amounts.
266-288: LnStatus struct is well-defined.
Storing chain info and URIs in vectors is beneficial for future expansions. Just confirm all fields in GetInfoResponse are relevant to avoid clutter.
src/main.rs (2)
19-19: LnStatus import recognized.
This import centralizes LN status for downstream usage. No issues noted.
36-36: Good use of OnceLock for LN status.
Ensures the LN status is set exactly once. This pattern is safer than global mutable statics.
src/scheduler.rs (2)
8-8: Static LN status import is consistent with updated design.
Importing LN_STATUS here centralizes LN node data retrieval. No issues noted.
78-79: Calls info_to_tags using LN data, ensuring real-time LN details.
This is a great improvement, providing LN node context in the published info.
| let ln_status = ln_client.get_node_info().await?; | ||
| let ln_status = LnStatus::from_get_info_response(ln_status); | ||
| if LN_STATUS.set(ln_status).is_err() { | ||
| panic!("No connection to LND node - shutting down Mostro!"); | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ensure fallback if LN node is unreachable.
Currently, a panic is triggered if the LN status fails to set. Consider a more graceful fallback if node connectivity fails, e.g., partial operation mode.
| @@ -69,13 +70,13 @@ async fn job_info_event_send() { | |||
| Err(e) => return error!("{e}"), | |||
| }; | |||
| let interval = Settings::get_mostro().publish_mostro_info_interval as u64; | |||
|
|
|||
| let ln_status = LN_STATUS.get().unwrap(); | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Unwrapping LN_STATUS can panic if LN status was never set.
Consider logging or gracefully handling missing LN status rather than unwrap(), as the node may be offline.
Fix #333
Summary by CodeRabbit
New Features
Bug Fixes
Documentation