Skip to content
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.

Sign up for GitHub

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

Jump to bottom

make blinded path public #3677

Merged
merged 1 commit into from
Mar 25, 2025
Merged

make blinded path public #3677

merged 1 commit into from
Mar 25, 2025

Conversation

JssDWt
Copy link
Contributor

@JssDWt JssDWt commented Mar 20, 2025 •
edited
Loading

What do I want: Create custom responses to onion messages in a core lightning plugin. The core lightning hook provides all the fields necessary to fill the blinded path. All I need is the BlindedPath struct to be publicly accessible.

Usage example here: https://github.com/michael1011/bolt12-client/blob/730c27d259b9da689ea41f8cd5ce5338ce9927ab/src/bolt12.rs#L136-L156

@ldk-reviews-bot
Copy link

ldk-reviews-bot commented Mar 20, 2025 •
edited
Loading

I've assigned @wpaulino as a reviewer!
I'll wait for their review and will help manage the review process.
Once they submit their review, I'll check if a second reviewer would be helpful.

@codecov Codecov
Copy link

codecov bot commented Mar 20, 2025 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.94%. Comparing base (ebdbee0) to head (8ce438c).
Report is 19 commits behind head on main.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #3677      +/-   ##
==========================================
+ Coverage   89.25%   89.94%   +0.69%     
==========================================
  Files         155      155              
  Lines      119954   124186    +4232     
  Branches   119954   124186    +4232     
==========================================
+ Hits       107064   111703    +4639     
+ Misses      10277     9908     -369     
+ Partials     2613     2575      -38

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@ldk-reviews-bot
Copy link

🔔 1st Reminder

Hey @wpaulino! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

wpaulino
wpaulino reviewed Mar 24, 2025
View reviewed changes
Copy link
Contributor

@wpaulino wpaulino left a comment

Choose a reason for hiding this comment

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

Would it be enough for your use case to expose BlindedMessagePath::from_raw instead? It's currently test-only.

@ldk-reviews-bot
Copy link

👋 The first review has been submitted!

Do you think this PR is ready for a second reviewer? If so, click here to assign a second reviewer.

@JssDWt JssDWt force-pushed the jssdwt-make-blinded-path-public branch from 1dc18c2 to 9c10693 Compare March 24, 2025 21:54
@JssDWt
Copy link
Contributor Author

JssDWt commented Mar 24, 2025

Would it be enough for your use case to expose BlindedMessagePath::from_raw instead? It's currently test-only.

Yes, that will work. Updated the PR to expose that.

@wpaulino
Copy link
Contributor

Great! CI is failing since we need docs on all public APIs. Maybe also rename from_raw to from_blinded_path or something else if you have a better suggestion?

@JssDWt JssDWt force-pushed the jssdwt-make-blinded-path-public branch from 9c10693 to 307b197 Compare March 25, 2025 10:48
@JssDWt
Copy link
Contributor Author

JssDWt commented Mar 25, 2025

Great! CI is failing since we need docs on all public APIs. Maybe also rename from_raw to from_blinded_path or something else if you have a better suggestion?

I like from_blinded_path. Renamed the function and updated the documentation comment.

tnull
tnull previously approved these changes Mar 25, 2025
View reviewed changes
Copy link
Contributor

@tnull tnull left a comment

Choose a reason for hiding this comment

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

LGTM

shaavan
shaavan reviewed Mar 25, 2025
View reviewed changes
Copy link
Member

@shaavan shaavan left a comment

Choose a reason for hiding this comment

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

Thanks! The changes look great.

The CI is failing because there's still one instance of from_raw usage that hasn't been updated in this PR:

https://github.com/JssDWt/rust-lightning/blob/jssdwt-make-blinded-path-public/lightning/src/offers/static_invoice.rs#L782

@JssDWt
rename from_raw to from_blinded_path
8ce438c 
This adds the option to initialize a `BlindedMessagePath` from its
constituent parts. Useful when you need to reconstruct a blinded path
from previously serialized components. The `from_blinded_path` function
is made public.
@JssDWt JssDWt force-pushed the jssdwt-make-blinded-path-public branch from 307b197 to 8ce438c Compare March 25, 2025 14:57
@JssDWt
Copy link
Contributor Author

JssDWt commented Mar 25, 2025

The CI is failing because there's still one instance of from_raw usage that hasn't been updated in this PR:

Ah, my editor didn't catch that. Updated again.

TheBlueMatt
TheBlueMatt approved these changes Mar 25, 2025
View reviewed changes
Copy link
Collaborator

@TheBlueMatt TheBlueMatt left a comment

Choose a reason for hiding this comment

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

Sure

@TheBlueMatt TheBlueMatt merged commit 030a784 into lightningdevkit:main Mar 25, 2025
26 of 27 checks passed
tnull
tnull reviewed Mar 25, 2025
View reviewed changes
Copy link
Contributor

@tnull tnull left a comment

Choose a reason for hiding this comment

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

Post-merge ACK

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tnull tnull tnull left review comments

@shaavan shaavan shaavan left review comments

@TheBlueMatt TheBlueMatt TheBlueMatt approved these changes

@wpaulino wpaulino wpaulino approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@JssDWt @ldk-reviews-bot @wpaulino @tnull @TheBlueMatt @shaavan