ChannelManager documentation refresh
#2704
Conversation
Codecov ReportAttention: Patch coverage is
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## main #2704 +/- ##
==========================================
+ Coverage 89.46% 90.96% +1.49%
==========================================
Files 117 117
Lines 95189 107474 +12285
Branches 95189 107474 +12285
==========================================
+ Hits 85162 97761 +12599
+ Misses 7803 7589 -214
+ Partials 2224 2124 -100 ☔ View full report in Codecov by Sentry. |
|
Concept ACK. It looks good so far, code snippets inline will be very helpful IMO. |
jkczyz
force-pushed
the
2023-11-channelmanager-docs
branch
from
ee5f94c to
7ab1216
Compare
|
Latest push fills in the " As mentioned earlier, we may want to move portions of this to module-level docs instead of having it all at the struct-level. |
jkczyz
force-pushed
the
2023-11-channelmanager-docs
branch
from
7ab1216 to
58cbb59
Compare
lightning/src/ln/channelmanager.rs
Outdated
| /// | ||
| /// Implements [`ChannelMessageHandler`], handling the multi-channel parts and passing things through | ||
| /// to individual Channels. | ||
| /// [`ChannelManager`] is parameterized by a number of components, which in aggregate facilitate |
There was a problem hiding this comment.
Not really convinced "it has parameters" is the second most important thing to say about a ChannelManager, should this be below the manager-vs-monitor section, at least?
There was a problem hiding this comment.
Yeah, this goes back to how we want to breakdown the organization of these docs across modules and structs as mentioned in the PR description. The draft has everything on ChannelManager, but my feeling is we should minimize those docs and move the sectioned breakdown to higher-level docs, likely on a module akin to how Iterator is documented in the std library.
So I can see moving everything after this list to the module-level, linking to it from here. I'm fine with also putting a short mention of ChannelMonitor here and linking to the particular module-level section doc that goes into detail.
There was a problem hiding this comment.
I do think the number of foundational components needed to get a ChannelManager up a running is worth noting. I've found it does help devs with their mental models and how things connect together
There was a problem hiding this comment.
Yip. Agree with @ConorOkus on knowing what it makes use of upfront. At least in my experience it helps to know what it is (including what it's composed of) first and then more deeply go into what it does and how, then all the other caveats to take note of.
There was a problem hiding this comment.
@dunxen Any opinion on whether most of the docs should move to the module-level as seems to be common in Rust standard library docs?
There was a problem hiding this comment.
For me as a dev, It would be helpful to expand the first sentence.
Firstly, why is it 2 things: state machine and payment management? I always refer to it to just state machine, but then maybe I am wrong here.
Secondly, as a state machine it would be helpful to know the possible states and actions that change the state, where is the state being held, saved, loaded?
Lastly, what logic does this state machine implement? What logic goes into the parameters and what is in the state machine and why.
There was a problem hiding this comment.
Thanks for the feedback! I'm not sure exactly how to address these yet, but wanted to respond in case it helps you or others see a way to improve these docs. We'll likely land this as-is but I'm definitely open to improvements in follow-ups. Contributions are more than welcome, too.
For me as a dev, It would be helpful to expand the first sentence. Firstly, why is it 2 things: state machine and payment management? I always refer to it to just state machine, but then maybe I am wrong here.
While there may be some refactoring that would allow us to have a cleaner separation of channel state machine from payment management, they are intrinsically linked as payment-related messages cause channel state transitions. Some of which may result in payment completion, failure, retry, or timeout.
Secondly, as a state machine it would be helpful to know the possible states and actions that change the state, where is the state being held, saved, loaded?
That seems more like internal developer docs. The goal of ChannelManager is to abstract away the details of the state machine from developers. That said, there are probably higher-level states exposed in ChannelDetails and RecentPaymentDetails that could be useful to better document there. I believe we are at least linking to those docs here albeit indirectly through methods that return those types.
Lastly, what logic does this state machine implement? What logic goes into the parameters and what is in the state machine and why.
The Lightning BOLT specification. :) Or at least some of it. Kidding aside, I think we want to err on the side of brevity here, but documentation on the linked parameterized types are in many instances ripe for going into further details, if they haven't already. I'll be the first to admit our docs need some work. Hopefully this is just a first step.
There was a problem hiding this comment.
Secondly, as a state machine it would be helpful to know the possible states and actions that change the state, where is the state being held, saved, loaded?
That seems more like internal developer docs. The goal of
ChannelManageris to abstract away the details of the state machine from developers. That said, there are probably higher-level states exposed inChannelDetailsandRecentPaymentDetailsthat could be useful to better document there. I believe we are at least linking to those docs here albeit indirectly through methods that return those types.
A further point here is how such transitions result in Event generation. We do go into that in the examples, but yes those could possibly use some higher-level docs as it relates to state transitions.
There was a problem hiding this comment.
My original point here was a bit less about whether this should be on ChannelManager or the module, but rather that we should try to sort things in the docs in importance-order. While I agree we should list the things that parameterize ChannelManager, I'm just not sure it should go second. Its not all that critical, however.
That said, I do agree the first sentence could be a bit more user-relevant, and maybe get a second sentence in the first paragraph. Maybe the "Channelmanager...facilitate sending, forwarding, and receiving payments through lightning channels" should move to the first sentence, and the second paragraph intro can just be "ChannelManager is parameterized"?
There was a problem hiding this comment.
That said, I do agree the first sentence could be a bit more user-relevant, and maybe get a second sentence in the first paragraph. Maybe the "Channelmanager...facilitate sending, forwarding, and receiving payments through lightning channels" should move to the first sentence, and the second paragraph intro can just be "ChannelManager is parameterized"?
Ah, ok, that sounds good. See latest push.
lightning/src/ln/channelmanager.rs
Outdated
| /// | ||
| /// An implementation of [`chain::Watch`], such as [`ChainMonitor`], is responsible for aggregating | ||
| /// these [`ChannelMonitor`]s and applying any [`ChannelMonitorUpdate`]s to them. It then monitors | ||
| /// for any pertinent on-chain activity, broadcasting claims as needed. |
There was a problem hiding this comment.
I like "enforcement" for what the monitor does.
jkczyz
force-pushed
the
2023-11-channelmanager-docs
branch
from
58cbb59 to
7f9f25c
Compare
lightning/src/ln/channelmanager.rs
Outdated
| /// | ||
| /// Implements [`ChannelMessageHandler`], handling the multi-channel parts and passing things through | ||
| /// to individual Channels. | ||
| /// [`ChannelManager`] is parameterized by a number of components, which in aggregate facilitate |
There was a problem hiding this comment.
Yeah, this goes back to how we want to breakdown the organization of these docs across modules and structs as mentioned in the PR description. The draft has everything on ChannelManager, but my feeling is we should minimize those docs and move the sectioned breakdown to higher-level docs, likely on a module akin to how Iterator is documented in the std library.
So I can see moving everything after this list to the module-level, linking to it from here. I'm fine with also putting a short mention of ChannelMonitor here and linking to the particular module-level section doc that goes into detail.
lightning/src/ln/channelmanager.rs
Outdated
| /// | ||
| /// An implementation of [`chain::Watch`], such as [`ChainMonitor`], is responsible for aggregating | ||
| /// these [`ChannelMonitor`]s and applying any [`ChannelMonitorUpdate`]s to them. It then monitors | ||
| /// for any pertinent on-chain activity, broadcasting claims as needed. |
|
Is there a way to generate preview links for these at all? |
Not that I'm immediately aware of or can see from a glance through https://github.com/marketplace. Locally, you can run |
jkczyz
force-pushed
the
2023-11-channelmanager-docs
branch
from
7f9f25c to
bb5384e
Compare
WalkthroughThe Changes
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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
jkczyz
force-pushed
the
2023-11-channelmanager-docs
branch
from
bb5384e to
956229d
Compare
|
Latest push includes docs for |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 2
Configuration used: CodeRabbit UI
Files selected for processing (1)
- lightning/src/ln/channelmanager.rs (4 hunks)
Additional comments: 23
lightning/src/ln/channelmanager.rs (23)
- 1071-1071: The documentation for
ChannelManagerhas been expanded to provide a comprehensive overview of its responsibilities and parameterization.- 1073-1086: The detailed breakdown of the components that parameterize
ChannelManageris informative and enhances understanding of its operation.- 1087-1093: The explanation of
ChannelManager's implementation of various traits and the events it generates is clear and useful for developers.- 1094-1107: The distinction between off-chain management by
ChannelManagerand on-chain enforcement byChannelMonitoris well articulated.- 1109-1177: The "Initialization" section with code examples is a valuable addition for developers to understand the setup process for
ChannelManager.- 1181-1193: The "Operation" section clearly outlines the necessary steps for
ChannelManagerto function properly.- 1200-1221: The "Channels" section provides a good overview of channel management and usage within
ChannelManager.- 1223-1277: The "Opening Channels" section with code examples is comprehensive and demonstrates the process effectively.
- 1284-1325: The "Accepting Channels" section provides clarity on how inbound channels are handled, with a good example of conditional acceptance.
- 1328-1360: The "Closing Channels" section explains the two methods of closing channels and the associated events, which is crucial for channel lifecycle management.
- 1363-1370: The "Payments" section begins well by outlining the responsibilities of
ChannelManagerin payment processing.- 1372-1375: The explanation of payment tracking and retry logic is clear and highlights the importance of managing payment state.
- 1377-1384: The "BOLT 11 Invoices" section references the
lightning-invoicecrate and provides guidance on creating invoices compatible withChannelManager.- 1386-1432: The documentation on handling
Event::PaymentClaimableand claiming funds is detailed and instructive.- 1434-1480: The "Paying an Invoice" section with code examples is thorough and demonstrates the use of
send_paymenteffectively.- 1483-1526: The "BOLT 12 Offers" section explains the creation and payment of offers, which is a more advanced feature that is well documented here.
- 1580-1634: The "BOLT 12 Refunds" section provides a clear explanation of the refund process and the state management required.
- 1637-1672: The documentation on requesting refund payments is clear and provides a good understanding of the process.
- 1695-1695: The documentation on
ChannelUpdatemessages and the conditions under which they are generated is a useful addition for understanding channel liveness tracking.- 1703-1703: The "DoS Mitigation" section is important for understanding the limitations imposed by
ChannelManagerto prevent denial-of-service attacks.- 1714-1714: The introduction of type aliases such as
SimpleArcChannelManagerandSimpleRefChannelManageris a good practice for conciseness and clarity.- 1722-1751: The cross-references to various traits, methods, and external crates are comprehensive and improve the navigability of the documentation.
- 9172-9174: The method
get_and_clear_needs_persistenceis well documented, and the reference to the associatedFutureis helpful for asynchronous operation.
lightning/src/ln/channelmanager.rs
Outdated
| /// # } | ||
| /// ``` | ||
| /// | ||
| /// TODO: Consider re-writing ChannelManagerReadArgs docs and moving here. |
There was a problem hiding this comment.
The TODO comment about re-writing ChannelManagerReadArgs docs suggests that this section is still a work in progress.
Would you like me to assist with re-writing the ChannelManagerReadArgs documentation or open a GitHub issue to track this task?
There was a problem hiding this comment.
Sure, take a stab at it!
There was a problem hiding this comment.
Based on the provided output, I have the necessary context to begin re-writing the documentation for ChannelManagerReadArgs. I will now draft a revised version of the documentation that aims to be clear and comprehensive.
Here's a draft for the revised documentation of ChannelManagerReadArgs:
/// Arguments required to (re)construct a [`ChannelManager`] during deserialization.
///
/// This struct encapsulates the dependencies needed by a [`ChannelManager`] to operate on a
/// Lightning network. It is used when deserializing a [`ChannelManager`] to provide it with
/// the necessary interfaces and configuration.
///
/// # Fields
///
/// - `entropy_source`: A cryptographically secure source of entropy used for random number generation.
/// - `node_signer`: A signer responsible for node-scoped cryptographic operations, such as signing channel announcements.
/// - `signer_provider`: Provides access to signers for specific channels, enabling the [`ChannelManager`] to sign channel-related messages and transactions.
/// - `fee_estimator`: Estimates fees for on-chain transactions. The [`ChannelManager`] uses this to set appropriate fees when broadcasting transactions.
/// - `chain_monitor`: Monitors the blockchain for activity relevant to the lightning channels, such as tracking confirmations and broadcasting justice transactions if necessary.
///
/// # Usage
///
/// When deserializing a [`ChannelManager`], create an instance of this struct with references to the implementations of the required traits. After deserialization, the [`ChannelManager`] will use these provided instances to interact with the blockchain, sign messages, and perform other operational tasks.
///
/// Note that no calls to `fee_estimator` or `chain_monitor` will be made during deserialization. It is assumed that you have deserialized [`ChannelMonitor`] instances separately and will add them to your `chain_monitor` after deserializing the [`ChannelManager`].
///
/// [`ChannelManager`]: lightning::ln::channelmanager::ChannelManager
/// [`ChannelMonitor`]: lightning::chain::channelmonitor::ChannelMonitor
Please review this draft and let me know if it aligns with the project's documentation standards and if there are any additional details or changes you would like to incorporate.
Analysis chain
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Find the file that contains the definition of `ChannelManagerReadArgs` and extract its documentation.
rg --type rust "struct ChannelManagerReadArgs" --context 15
Length of output: 2686
Script:
#!/bin/bash
# Extract the full documentation block for `ChannelManagerReadArgs`.
rg --type rust -A 30 "pub struct ChannelManagerReadArgs" lightning/src/ln/channelmanager.rs
Length of output: 1344
There was a problem hiding this comment.
Could you wrap at 100 characters so it's easier to read?
There was a problem hiding this comment.
/// Arguments required to (re)construct a [`ChannelManager`] during deserialization.
///
/// This struct encapsulates the dependencies needed by a [`ChannelManager`] to operate on a
/// Lightning network. It is used when deserializing a [`ChannelManager`] to provide it with
/// the necessary interfaces and configuration.
///
/// # Fields
///
/// - `entropy_source`: A cryptographically secure source of entropy used for random number
/// generation.
/// - `node_signer`: A signer responsible for node-scoped cryptographic operations, such as
/// signing channel announcements.
/// - `signer_provider`: Provides access to signers for specific channels, enabling the
/// [`ChannelManager`] to sign channel-related messages and transactions.
/// - `fee_estimator`: Estimates fees for on-chain transactions. The [`ChannelManager`] uses
/// this to set appropriate fees when broadcasting transactions.
/// - `chain_monitor`: Monitors the blockchain for activity relevant to the lightning channels,
/// such as tracking confirmations and broadcasting justice transactions if necessary.
///
/// # Usage
///
/// When deserializing a [`ChannelManager`], create an instance of this struct with references
/// to the implementations of the required traits. After deserialization, the [`ChannelManager`]
/// will use these provided instances to interact with the blockchain, sign messages, and
/// perform other operational tasks.
///
/// Note that no calls to `fee_estimator` or `chain_monitor` will be made during deserialization.
/// It is assumed that you have deserialized [`ChannelMonitor`] instances separately and will
/// add them to your `chain_monitor` after deserializing the [`ChannelManager`].
///
/// [`ChannelManager`]: lightning::ln::channelmanager::ChannelManager
/// [`ChannelMonitor`]: lightning::chain::channelmonitor::ChannelMonitor
@jkczyz, I've wrapped the documentation at 100 characters. Please let me know if this meets your requirements or if there are any further adjustments needed.
| /// # } | ||
| /// ``` | ||
| /// | ||
| /// # Persistence |
There was a problem hiding this comment.
The "Persistence" section is brief and could benefit from more detail on the persistence process and its importance.
Consider expanding the "Persistence" section to include more details on how and when to persist ChannelManager state to disk.
lightning/src/ln/channelmanager.rs
Outdated
| /// This division of off-chain management and on-chain enforcement is what allows for interesting | ||
| /// node setups. See the [`chain`] module for further details. |
There was a problem hiding this comment.
I don't see in chain where we describe such "interesting" setups, maybe we should just say something like "allows for the creation of custom and/or redundant watchtowers, as well as separation of expensive off-chain enforcement structs onto separate hosts"? Also, at least,
| /// This division of off-chain management and on-chain enforcement is what allows for interesting | |
| /// node setups. See the [`chain`] module for further details. | |
| /// This division of off-chain management and on-chain enforcement allows for interesting | |
| /// node setups. See the [`chain`] module for further details. |
There was a problem hiding this comment.
Re-phrased a bit with a little more succinct wording of what you suggested.
| /// PaymentPurpose::InvoicePayment { payment_preimage: None, .. } => { | ||
| /// println!("Unknown payment hash: {}", payment_hash); | ||
| /// }, | ||
| /// // ... |
There was a problem hiding this comment.
Also funky indentation here, but also what is the ...?
There was a problem hiding this comment.
The catchall case (_ => {},) is hidden from the docs, so that's just signifying that the event handler would need to handle other events.
jkczyz
force-pushed
the
2023-11-channelmanager-docs
branch
from
956229d to
59fa1ff
Compare
| /// PaymentPurpose::InvoicePayment { payment_preimage: None, .. } => { | ||
| /// println!("Unknown payment hash: {}", payment_hash); | ||
| /// }, | ||
| /// // ... |
There was a problem hiding this comment.
The catchall case (_ => {},) is hidden from the docs, so that's just signifying that the event handler would need to handle other events.
lightning/src/ln/channelmanager.rs
Outdated
| /// This division of off-chain management and on-chain enforcement is what allows for interesting | ||
| /// node setups. See the [`chain`] module for further details. |
There was a problem hiding this comment.
Re-phrased a bit with a little more succinct wording of what you suggested.
lightning/src/ln/channelmanager.rs
Outdated
| /// Rather than using a plain `ChannelManager`, it is preferable to use either a [`SimpleArcChannelManager`] | ||
| /// a [`SimpleRefChannelManager`], for conciseness. See their documentation for more details, but | ||
| /// essentially you should default to using a [`SimpleRefChannelManager`], and use a | ||
| /// [`SimpleArcChannelManager`] when you require a `ChannelManager` with a static lifetime, such as when | ||
| /// you're using lightning-net-tokio. | ||
| /// | ||
| /// [`ChainMonitor`]: crate::chain::chainmonitor::ChainMonitor | ||
| /// [`MessageHandler`]: crate::ln::peer_handler::MessageHandler | ||
| /// [`OnionMessenger`]: crate::onion_message::OnionMessenger |
There was a problem hiding this comment.
Note, this will need to be updated once rebased since the re-export was removed.
lightning/src/ln/channelmanager.rs
Outdated
| /// | ||
| /// Implements [`ChannelMessageHandler`], handling the multi-channel parts and passing things through | ||
| /// to individual Channels. | ||
| /// [`ChannelManager`] is parameterized by a number of components, which in aggregate facilitate |
There was a problem hiding this comment.
Yip. Agree with @ConorOkus on knowing what it makes use of upfront. At least in my experience it helps to know what it is (including what it's composed of) first and then more deeply go into what it does and how, then all the other caveats to take note of.
|
CI just needs attention but LGTM |
|
Feel free to squash with some clarification for #2704 (comment) and I think we can basically land this. |
ChannelManager docs aren't very approachable as they consist of a large wall of texts without much direction. As a first step of improvement, add sections to help delineate the existing text and make it easier to scan.
jkczyz
force-pushed
the
2023-11-channelmanager-docs
branch
from
59fa1ff to
80d9358
Compare
Squashed and rebased to fix the doc linking. Regarding the linked comment, happy to find some better wording if you prefer. I was tying to keep it brief in the first sentence, while the second sentences sorta links channels and payments by saying "... sending, forwarding, and receiving payments through lightning channels". |
jkczyz
force-pushed
the
2023-11-channelmanager-docs
branch
from
80d9358 to
3b956b8
Compare
|
CI is sad (maybe a doctest failure?) |
jkczyz
force-pushed
the
2023-11-channelmanager-docs
branch
from
3b956b8 to
63a64e5
Compare
Yep, missing a parameter in |
|
Looks like doctests are still failing in no-std because of the reference to |
jkczyz
force-pushed
the
2023-11-channelmanager-docs
branch
from
63a64e5 to
e6bb711
Compare
|
Ugh, looks like doctests are now failing on |
jkczyz
force-pushed
the
2023-11-channelmanager-docs
branch
from
e6bb711 to
bdf97d6
Compare
Should be fixed |
jkczyz
force-pushed
the
2023-11-channelmanager-docs
branch
from
bdf97d6 to
bd25357
Compare
jkczyz
force-pushed
the
2023-11-channelmanager-docs
branch
from
bd25357 to
63f0147
Compare
jkczyz
force-pushed
the
2023-11-channelmanager-docs
branch
from
63f0147 to
998fffb
Compare
ChannelManager's documentation could use a little love. It needs:ChainMonitor/ChannelMonitors)All this needs to be organized in a way that is approachable, discoverable, and doesn't overwhelm the user. Some documentation may need to be consolidated or placed at a higher-level so it is not missed.
This PR re-writes
ChannelManager's summary, adds sections to delineate the existing docs, and proposes new sections. It still has a ways to go. One consideration is how to split the documentation betweenChannelManager, its methods, the higher-levelchannelmanagermodule docs (yet to be re-written), and even higher-level modules. Or if the module layout should be flattened a bit to foster better discovery.Feedback encouraged!