LSPS5 implementation

[martinsaposnic]

A complete implementation for LSPS5 (spec defined here https://github.com/lightning/blips/pull/55)

Reviewing commit by commit is recommended (~40% of the added lines are tests)

Notes:

• Persistence of the state will come on a future PR ~- Will rebase from https://github.com/lightningdevkit/rust-lightning/pull/3509 once that PR is merged~ DONE

[ldk-reviews-bot]

👋 Thanks for assigning @TheBlueMatt 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.

[martinsaposnic]

This is a huge PR, but it wasn’t obvious to me how to split it in a way that would still make sense (I did split it into small commits to make it easier to review.). I’m open to suggestions if you have ideas on how this could be structured differently.

[codecov[bot]]

Codecov Report

Attention: Patch coverage is 86.38596% with 194 lines in your changes missing coverage. Please review.

Project coverage is 88.92%. Comparing base (65d518b) to head (18fe164). Report is 13 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning-liquidity/src/lsps5/msgs.rs	76.84%	81 Missing and 16 partials :warning:
lightning-liquidity/src/lsps5/client.rs	90.59%	30 Missing and 3 partials :warning:
lightning-liquidity/src/lsps5/service.rs	91.16%	20 Missing and 5 partials :warning:
lightning-liquidity/src/lsps0/ser.rs	79.16%	3 Missing and 17 partials :warning:
lightning-liquidity/src/lsps5/url_utils.rs	94.87%	1 Missing and 5 partials :warning:
lightning-liquidity/src/manager.rs	93.84%	2 Missing and 2 partials :warning:
lightning-liquidity/src/lsps5/validator.rs	95.83%	0 Missing and 3 partials :warning:
lightning/src/util/test_utils.rs	50.00%	3 Missing :warning:
lightning/src/ln/blinded_payment_tests.rs	0.00%	2 Missing :warning:
lightning-liquidity/src/lsps0/msgs.rs	0.00%	1 Missing :warning:

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3662      +/-   ##
==========================================
- Coverage   88.95%   88.92%   -0.04%     
==========================================
  Files         168      173       +5     
  Lines      121974   123393    +1419     
  Branches   121974   123393    +1419     
==========================================
+ Hits       108502   109723    +1221     
- Misses      11072    11216     +144     
- Partials     2400     2454      +54     

Flag	Coverage Δ
fuzzing	22.31% <2.12%> (-0.38%)	:arrow_down:
tests	88.74% <86.38%> (-0.04%)	:arrow_down:

Flags with carried forward coverage won't be shown. Click here to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

:rocket: New features to boost your workflow:

• :snowflake: Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[ldk-reviews-bot]

👋 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.

[tnull]

This is a huge PR, but it wasn’t obvious to me how to split it in a way that would still make sense (I did split it into small commits to make it easier to review.). I’m open to suggestions if you have ideas on how this could be structured differently.

Thanks for asking! I'm totally fine to keep this (with its current scope) in a single PR, as long as we keep the commit history pretty clean to allow continuing review to happen commit-by-commit. To this end, please make sure add any fixup commits clearly marked (e.g. via a f or fixup prefix in the commit message header) directly under the commit they belong to, so they can cleanly be squashed into the respective feature commits in-between review rounds.

[tnull]

Btw, I'm not sure if you're familiar with the previous attempt of implementing LSPS5: https://github.com/lightningdevkit/rust-lightning/pull/3499

Given this is a clean slate, not sure how much there is to learn, but still might be worth a look. Also not sure if @johncantrell97 would be interested in reviewing this PR, too, as he's familiar with the codebase and LSPS5.

[martinsaposnic]

@tnull, ready for the next review round!

• Removed reqwest and tokio.
• HttpClient is now a generic trait, allowing users to pass any implementation.
• Removed std.
• Small refactor on client and service to be able to delete some silly / unnecessary code.
• Ran formatting on every commit.
• All new changes are in commits prefixed with fixup:.

CI is failing because of the usage of the url crate (which I guess does not support rust 1.63?), which I need for validating the webhook URL. A new commit will come addressing that shortly

[ldk-reviews-bot]

🔔 1st Reminder

Hey @tnull @valentinewallace! 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.

[ldk-reviews-bot]

🔔 1st Reminder

Hey @tnull @valentinewallace! 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.

[martinsaposnic]

FYI I fixed the Rust 1.63.0 build failures by downgrading the url crate to 1.7.2 in this commit.

I'm not thrilled about using such an old version (5+ years). It works, but maybe not ideal.

A few other options:

• I could write a small URL validator that just handles what we need for the webhook validation. It can bring other bugs / issues but eliminates the dependency headache.
• Use feature flags in Cargo.toml
• Bump the MSRV for lightning-liquidity

I can implement any of these alternatives if you think it's worth it. Let me know what you think!

[tnull]

FYI I fixed the Rust 1.63.0 build failures by downgrading the url crate to 1.7.2 in this commit.

I'm not thrilled about using such an old version (5+ years). It works, but maybe not ideal.

A few other options:

* I could write a small URL validator that just handles what we need for the webhook validation. It can bring other bugs / issues but eliminates the dependency headache.

* Use feature flags in Cargo.toml

* Bump the MSRV for lightning-liquidity

I can implement any of these alternatives if you think it's worth it. Let me know what you think!

Yes, let's please also drop the url dependency and just write the few checks we need to run on the URLs ourselves. I'd rather avoid introducing feature flags and have LSPS5 available to all users. Bumping MSRV is really no option currently, as we're looking to expose lightning-liquidity in our language bindings, which currently relies on said 1.63 MSRV.

[martinsaposnic]

Hey @tnull, thank you so much for reviewing and answering my questions!

I just pushed some updates, addressing all your comments, including:

• All new crates added before are now deleted
• Correct notification signature
• URL validation is now handled by us (not an external crate)
• Generic time trait for user-provided implementations (with a default std implementation)
• Added tests with spec examples and test vectors (more tests are coming, so we are sure the PR is following the spec byte-for-byte)

This should be ready for the next round of review.

Thanks again!

[martinsaposnic]

Thanks for the changes so far! This is definitely moving in the right direction quickly.

I’ve added more high-level comments and will get into the details in the next round. Feel free to squash the fixups!

Thanks for the review @tnull!

I've addressed all new comments and pushed them as fixup: commits.

Key changes:

• http_client has been removed. Instead, there's now a SendWebhookNotifications event that informs the user which URLs to call, leaving the actual POST requests to them.
• No more panics or unsafe unwraps.
• TimeProvider can now be injected via an alternative constructor.

CI is taking forever, so I may need to push a few fixes if anything breaks, but this should be ready for review again!

A couple of things I know will get comments:

1. I'm using fromRFC3339 and toRFC3339 from LSPSDateTime, but since timestamps are handled as Duration, I had to introduce a function to convert Duration into LSPSDateTime:

/// Returns the number of seconds since the Unix epoch.
pub fn new_from_duration(duration: Duration) -> Self {
    Self(DateTime::from_timestamp(duration.as_secs() as i64, duration.subsec_nanos()).unwrap())
}

It works, but it’s not the cleanest approach—open to suggestions.

2. The client receives app_name and webhook_url as plain strings, which are then wrapped in LSPS5AppName and LSPS5WebhookUrl via a new method that validates them.

When the event is triggered, the service picks it up and receives already-validated LSPS5AppName and LSPS5WebhookUrl, but it calls validate again to ensure nothing changed.

This pattern—new calls validate, and then validate runs again later—works, but feels a bit redundant. Curious to hear thoughts on whether this is a good approach.

Looking forward to your feedback!

[martinsaposnic]

Hey @tnull, thanks for the review! Apologies for pinging you earlier when the CI was failing.

I've addressed your last comments in fixup: commits, and the CI is now passing.

[ldk-reviews-bot]

🔔 1st Reminder

Hey @tnull! 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.

[martinsaposnic]

@tnull thanks for the review!

I've addressed the comments in fixup commits and rebased the branch. This should be ready for review again.

[ldk-reviews-bot]

🔔 1st Reminder

Hey @tnull! 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.

[martinsaposnic]

@tnull thanks a lot for the review!

All review comments have been addressed in fixup commits. Let me know what you think!

Side note. There are a few things I’m still not fully convinced about:

• I made the requested changes to url_utils, but now I’m unsure if this util is something we should keep long-term. I have a (not fully working) draft that takes inspiration from the url crate and builds a minimal, no-std-friendly version tailored to our needs. I’d love your opinion on whether that direction makes sense and is worth pursuing, or if the current version is good enough as-is.
• I added a property-based test for url_utils as an example (very cool!). I have a few more drafted and will push them once they’re ready.
• The UntrustedString wrapper made LSPSAppName and LSPSWebhookUrl more awkward to use (e.g., requiring .0.0 to access the underlying string in some places). I made some adjustments to reduce that friction, but I’m still exploring whether there’s a cleaner solution.
• I introduced an LSPS5Error that requires .into() conversions to and from LSPSResponseError when entering and leaving the LSPS5 context. I’m not sure if this is the best approach, but if it is, we might want to apply the same pattern across the other LSPS modules.

[ldk-reviews-bot]

🔔 1st Reminder

Hey @tnull! 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.

[ldk-reviews-bot]

🔔 2nd Reminder

Hey @tnull! 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.

[martinsaposnic]

@tnull rebase done!

[martinsaposnic]

@tnull thanks for the review.

This should be ready to review again

Updates include:

• I improved the documentation a lot
• New LSPS5Error with a separation between spec defined errors and client errors
• Tests are improved to cover more stuff

[ldk-reviews-bot]

🔔 1st Reminder

Hey @tnull! 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.

[tnull]

This should be ready to review again

Cool, thank you! I think this is ready for more eyes now! Given the size of this PR, I'll let the bot assign a second reviewer and let @johncantrell97 weigh in whenever he gets to it.

[ldk-reviews-bot]

✅ Added second reviewer: @wpaulino

[ldk-reviews-bot]

✅ Added second reviewer: @TheBlueMatt

[ldk-reviews-bot]

🔔 1st Reminder

Hey @TheBlueMatt! 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.

[martinsaposnic]

Thank you for your review @TheBlueMatt. I've addressed the comments in fixup commits

[ldk-reviews-bot]

🔔 1st Reminder

Hey @tnull @TheBlueMatt! 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.

[ldk-reviews-bot]

🔔 1st Reminder

Hey @tnull @TheBlueMatt! 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.