-
Notifications
You must be signed in to change notification settings - Fork 37
Update simln-lib docs #241
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Let me know when this needs a first pass! |
Yep, thanks for checking in, have been moving slowly on this one sadly but I'll tag you when this is ready for a review. |
simln-lib/README.md
Outdated
sim-ln = "0.1.0" | ||
``` | ||
|
||
### Basic Example |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should I take this out for now and add it when the polar file is also ready? This example won't work if it was copy pasted as is, for example, some of tokio needs to be added in as a dependency. Some of the node configuration can also be tricky.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's go ahead and take this out for now. Not particularly because we want a polar file, but because an example that won't compile isn't great - perhaps we can just link to sim-cli for an example of how to use it in the meantime?
@carlaKC This one is ready for a review :) Let me know what you think, happy to change/remove anything if it seems redundant. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've been very nitpicky because we're doing a big docs overhaul, but looking very good overall!
simln-lib/README.md
Outdated
@@ -0,0 +1,13 @@ | |||
# sim-ln | |||
|
|||
A Lightning Network simulation library that enables testing and analysis of payment routing behavior in a controlled environment. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: in a controlled environment / on any lightning network.
We could theoretically run on signet where you don't control all the nodes.
simln-lib/README.md
Outdated
- Create simulated Lightning Network topologies | ||
- Test payment routing strategies | ||
- Analyze network behavior under different conditions | ||
- Simulate real Lightning node implementations (LND, c-lightning, Eclair) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can remove this last line until we have the ability to run with simulated mode fully in simln
simln-lib/README.md
Outdated
|
||
sim-ln provides a framework for simulating Lightning Network payment routing behavior, allowing developers and researchers to: | ||
|
||
- Create simulated Lightning Network topologies |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This sounds to me like sim-ln allows you to spin up different channel graphs, which isn't the case. Could we change this bullet point to something about specific or random traffic patterns?
simln-lib/src/lib.rs
Outdated
#[derive(Serialize, Debug, Clone)] | ||
pub enum NodeId { | ||
/// The node's public key |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: here and a few other places in the PR, end comments with a full stop.
sim-cli/src/parsing.rs
Outdated
let clients_clone = clients.clone(); | ||
let get_node = move |pk: &PublicKey| async move { | ||
if let Some(c) = clients_clone.values().next() { | ||
let get_node = async |pk: &PublicKey| -> Result<NodeInfo, LightningError> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, this can be squashed with the previous commit
simln-lib/README.md
Outdated
sim-ln = "0.1.0" | ||
``` | ||
|
||
### Basic Example |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's go ahead and take this out for now. Not particularly because we want a polar file, but because an example that won't compile isn't great - perhaps we can just link to sim-cli for an example of how to use it in the meantime?
simln-lib/src/lib.rs
Outdated
/// for payments to be sent to. The selection can be based on different strategies: | ||
/// - Predefined destinations (for deterministic payment patterns) | ||
/// - Random selection weighted by node capacity (for simulating network activity) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we should have details about implementations in trait documentation, likewise below for PaymentGenerator
simln-lib/src/lib.rs
Outdated
/// # Returns | ||
/// | ||
/// * `Some(u64)` - The exact number of payments to make |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this adds much, and this won't always be Some
6e56367
to
0095a23
Compare
Hey @carlaKC, I added these to the top of the docs file but this means we woud need to add in docs for all public functions in the lightning implementations. Not sure if I have done this incorrectly, or we want to add docs for those in this PR as well?
|
My comment definitely could have been clearer - I meant that we'll want to turn them on in the future (not this PR), so might as well get our docs conventions in line now. Let's remove it for now and come back to it in a follow up 👍
This is being correctly enforced, but we can handle in a follow up. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, feel free to squash this all down to one commit (if it's easier) or go ahead and squash fixups.
Only high level comment is that the line wrapping looks a little off - can update to 120 across the board (non-blocking).
As for enforcing docs, let's just go with #![deny(rustdoc::broken_intra_doc_links)]
(and add it to sim-cli/src/lib.rs
as well).
- Generate specific or random traffic patterns on a provided Lightning graph. | ||
- Test payment routing strategies. | ||
- Analyze network behavior under different conditions. | ||
## Usage |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: missing newline above Usage
simln-lib/src/lib.rs
Outdated
@@ -1,3 +1,6 @@ | |||
#![deny(rustdoc::broken_intra_doc_links)] | |||
#![deny(missing_docs)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we remove missing_docs
I believe we should be able to keep #![deny(rustdoc::broken_intra_doc_links)]
as-is to make sure code references are correct.
simln-lib/src/lib.rs
Outdated
/// - Features: The node's supported protocol features and capabilities, used to determine compatibility | ||
/// and available functionality. In CLN, features are stored as big-endian bytes, while in LND they are | ||
/// stored as a set of feature flags converted to little-endian bytes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: I don't think that we need information about LND/CLN on this level, can remove.
When we get around to adding comments to the individual LN impls, that would be a good place to make note of these type of implementation details.
simln-lib/src/lib.rs
Outdated
pub fn not_dispatched() -> Self { | ||
PaymentResult { | ||
htlc_count: 0, | ||
payment_outcome: PaymentOutcome::NotDispatched, | ||
} | ||
} | ||
|
||
/// Creates a new PaymentResult indicating that tracking the payment failed. | ||
/// This is used when the payment was dispatched but the system was unable to | ||
/// determine its final outcome (e.g., due to connection issues or timeouts) with [`PaymentOutcome::TrackPaymentFailed`]. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: line wrapping is off here
8a2a18d
to
168a921
Compare
Add ValueOrRange logs Add NodeInfo logs Add PaymentResult docs Add PaymentOutcome docs Fix up url and add simulation logs Remove spurious change Add basic usage Update doc for WriteResults Add Simulation and Lightning error docs Add DestinationGenerator and PaymentGenerator docs quick tidy up of a few doc comments Fix up formatting and some language Fix up NodeInfo and NodeFeatures comments PR Fixes Fix line wrapping to 120 chars
Thanks for the review @carlaKC! I've taken out |
Documentation Updates for simln-lib
Changes
lib.rs
README.md
Testing
This is what it looks like when I run

cargo doc --open
locally.Note
Working on a usage example with Polar. Current gRPC issue documented here.