Skip to content

Add methods to set invoice features in Features objects. #836

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

Merged

Conversation

valentinewallace
Copy link
Contributor

I was struggling to get the Vec<u5> from lightning-invoice into an our-features-compatible Vec<u8>. This was much easier. Lmk if it makes sense.

@codecov
Copy link

codecov bot commented Mar 9, 2021

Codecov Report

Merging #836 (e0c8ec5) into main (52edab7) will increase coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #836      +/-   ##
==========================================
+ Coverage   90.67%   90.68%   +0.01%     
==========================================
  Files          50       50              
  Lines       26828    26843      +15     
==========================================
+ Hits        24326    24343      +17     
+ Misses       2502     2500       -2     
Impacted Files Coverage Δ
lightning/src/ln/features.rs 98.91% <100.00%> (+0.06%) ⬆️
lightning/src/ln/functional_tests.rs 97.10% <0.00%> (+0.03%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 52edab7...e0c8ec5. Read the comment docs.

@jkczyz
Copy link
Contributor

jkczyz commented Mar 9, 2021

I don't have a problem with this approach. @TheBlueMatt Any reason not to expose these?

@TheBlueMatt
Copy link
Collaborator

Hmm, our features historically have always been "don't export the manipulation bits, just allows creation via known or empty. I'm open to changing it, though it would be nice to do something more full-featured than piecemeal.

@jkczyz
Copy link
Contributor

jkczyz commented Mar 9, 2021

Hmm, our features historically have always been "don't export the manipulation bits, just allows creation via known or empty. I'm open to changing it, though it would be nice to do something more full-featured than piecemeal.

I tend to like a fluent-style API for this (consuming rather than mutating) similar to what I defined in a test:

https://github.com/rust-bitcoin/rust-lightning/blob/ad09a2f00413b50481975a08aa1315f8ca57291b/lightning/src/ln/features.rs#L673

@TheBlueMatt
Copy link
Collaborator

TheBlueMatt commented Mar 9, 2021

I tend to like a fluent-style API for this (consuming rather than mutating) similar to what I defined in a test:

Yea, definitely makes for cleaner rust code. I need to figure out how to properly map such things in the bindings, though, presumably via a constructor that takes a parameter for each possible function call. Gonna be hard with lightning-invoice. If it makes sense, would be nice to avoid it for a bit longer, but it may need to happen eventually anyway.

@valentinewallace
Copy link
Contributor Author

valentinewallace commented Mar 11, 2021

Wtf. Why does this keep closing.

Edit: oh my changes mysteriously vanished.. fixing

Comment on lines 272 to 274
define_feature!(1, DataLossProtect, [InitContext, NodeContext],
"Feature flags for `option_data_loss_protect`.");
"Feature flags for `option_data_loss_protect`.", set_supports_data_loss_protect,
set_requires_data_loss_protect);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It kinda sucks that we have to pass separate names for these. Especially considering we'd have to pass two more if we want to be consistent and define the existing supports_ and requires_ predicates using macros, too. Not sure if there is a better solution, though.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think technically you could do a procedural macro for this, but I don't know how to use them and I don't really want to find out.

@valentinewallace valentinewallace force-pushed the invoice-features-methods branch from a6434fc to be512c2 Compare March 11, 2021 17:43
@valentinewallace valentinewallace force-pushed the invoice-features-methods branch 2 times, most recently from 1a2b881 to 0138c87 Compare March 12, 2021 17:13
@valentinewallace valentinewallace force-pushed the invoice-features-methods branch from 0138c87 to e0c8ec5 Compare March 12, 2021 17:58
@TheBlueMatt TheBlueMatt merged commit 2cb5b1a into lightningdevkit:main Mar 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants