-
Notifications
You must be signed in to change notification settings - Fork 405
Document better optional features #1514
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
Conversation
Codecov Report
@@ Coverage Diff @@
## main #1514 +/- ##
==========================================
- Coverage 90.93% 90.91% -0.02%
==========================================
Files 80 80
Lines 43469 43469
Branches 43469 43469
==========================================
- Hits 39527 39520 -7
- Misses 3942 3949 +7
Continue to review full report at Codecov.
|
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.
Thanks! This already needs rebase for the ZeroConf feature lol. sorry about that.
lightning/src/ln/features.rs
Outdated
//! - `DataLossProtect` - requires/supports that a node, which has somehow fallen behind (e.g. has been restored from old backup), | ||
//! can detect that it has fallen behind | ||
//! (see [BOLT-2](https://github.com/lightning/bolts/blob/master/02-peer-protocol.md) for more information). | ||
//! - `set_data_loss_protect_optional` - sets this feature to optional. |
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.
Hmm, do we need to list the methods? In general I think users should never change them manually.
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.
Sounds good - I will remove them.
lightning/src/ln/features.rs
Outdated
@@ -18,6 +18,81 @@ | |||
//! the term "supports" is used in reference to a particular set of [`Features`]. That is, a node | |||
//! supports a feature if it advertises the feature (as either required or optional) to its peers. | |||
//! And the implementation can interpret a feature if the feature is known to it. | |||
//! | |||
//! The following features are currently supported in the LDK: | |||
//! - `DataLossProtect` - requires/supports that a node, which has somehow fallen behind (e.g. has been restored from old backup), |
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.
We should mention somewhere what we require vs support. I think currently we may only require StaticRemoteKey, but in the near future we'll probably require VariableLengthOnion too.
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 will make two lists: one for required features (StaticRemoteKey and VariableLengthOnion) and one for supported features (everything else).
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.
Thanks for having a look! Just a few suggestions/nits/remarks.
lightning/src/ln/features.rs
Outdated
//! (see [BOLT-3](https://github.com/lightning/bolts/blob/master/03-transactions.md) for more information). | ||
//! | ||
//! The following features are currently supported in the LDK: | ||
//! - `DataLossProtect` - requires/supports that a node, which has somehow fallen behind (e.g. has been restored from old backup), |
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.
//! - `DataLossProtect` - requires/supports that a node, which has somehow fallen behind (e.g. has been restored from old backup), | |
//! - `DataLossProtect` - requires/supports that a node which has somehow fallen behind, e.g., has been restored from an old backup, |
lightning/src/ln/features.rs
Outdated
//! - `ShutdownAnySegwit` - requires/supports that future segwit versions are allowed in `shutdown` | ||
//! (see [BOLT-2](https://github.com/lightning/bolts/blob/master/02-peer-protocol.md#closing-initiation-shutdown) for more information). | ||
//! - `ChannelType` - node supports the channel_type field in open/accept | ||
//! (see [BOLT-2](https://github.com/lightning/bolts/blob/master/02-peer-protocol.md#closing-initiation-shutdown) for more information). |
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 this link to BOLT 2 and the following two should not link to the closing-initiation-shutdown
subsection.
lightning/src/ln/features.rs
Outdated
//! - `SCIDPrivacy` - supply channel aliases for routing | ||
//! (see [BOLT-2](https://github.com/lightning/bolts/blob/master/02-peer-protocol.md#closing-initiation-shutdown) for more information). | ||
//! - `Keysend` - send funds to a node without an invoice | ||
//! (see [BOLT-11](https://github.com/lightning/bolts/blob/master/11-payment-encoding.md) for more information). |
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.
Pointing users towards this URL may be misleading since BOLT 11 does AFAIK not contain any further information on keysend payments?
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 you are right - I will link to the Keysend
feature assignment proposal.
Closes #438.
Issue: Post #428 merge, would be great to document FeatureContext* setters (lightning/src/ln/msgs.rs), I don't think optional features are documented anyway in the current codebase beyond references to BOLT.
Changes: Added documentation in
features.rs
on what the currently-supported features do in the LDK based on BOLT-9 feature flags.