Skip to content

Sort LSPS2 opening fee params menu according to spec requirements #3840

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
Merged
merged 1 commit into from
Jun 16, 2025
Merged

Sort LSPS2 opening fee params menu according to spec requirements #3840

merged 1 commit into from
Jun 16, 2025

Conversation

@martinsaposnic
Copy link
Contributor

@martinsaposnic martinsaposnic commented Jun 10, 2025

Closes #3462

The LSPS2 specification requires that the opening_fee_params_menu array be sorted in a specific order to simplify fee display for clients, but the current implementation returns parameters in an arbitrary order.

The LSPS2 spec:

The LSP, when ordering the opening_fee_params_menu array, MUST order by the following rules:

  • The 0th item MAY have any parameters.
  • Each succeeding item MUST, compared to the previous item, obey any one of the following:
    • Have a larger min_fee_msat, and equal proportional.
    • Have a larger proportional, and equal min_fee_msat.
    • Have a larger min_fee_msat, AND larger proportional.

This PR implements the sorting as described in the spec by sorting first by min_fee_msat, then by proportional for equal values.

The test creates an unsorted array of fee parameters and verifies that the returned menu follows all three ordering rules specified in the LSPS2 specification.

@ldk-reviews-bot
Copy link

ldk-reviews-bot commented Jun 10, 2025 •
edited
Loading

👋 Thanks for assigning @tnull 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
Copy link

codecov bot commented Jun 10, 2025 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.83%. Comparing base (b8673f3) to head (16390e6).
Report is 64 commits behind head on main.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #3840      +/-   ##
==========================================
- Coverage   89.88%   89.83%   -0.05%     
==========================================
  Files         162      162              
  Lines      130415   130422       +7     
  Branches   130415   130422       +7     
==========================================
- Hits       117217   117165      -52     
- Misses      10509    10552      +43     
- Partials     2689     2705      +16

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

@tnull tnull self-requested a review June 11, 2025 08:33
tnull
tnull previously approved these changes Jun 11, 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

tnull
tnull approved these changes Jun 12, 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

Minor nit: please avoid rebasing on main if there are no conflicts, as it makes comparing the changes easier, especially if you force-push.

@ldk-reviews-bot
Copy link

ldk-reviews-bot commented Jun 12, 2025

🔔 1st Reminder

Hey @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
Copy link

ldk-reviews-bot commented Jun 14, 2025

🔔 2nd Reminder

Hey @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
Copy link

ldk-reviews-bot commented Jun 16, 2025

🔔 3rd Reminder

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

valentinewallace
valentinewallace approved these changes Jun 16, 2025
View reviewed changes
Copy link
Contributor

@valentinewallace valentinewallace left a comment

Choose a reason for hiding this comment

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

LGTM, test fails as expected on main

@valentinewallace valentinewallace merged commit 662cd77 into lightningdevkit:main Jun 16, 2025
27 of 28 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@tnull tnull tnull approved these changes

@valentinewallace valentinewallace valentinewallace 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.

LSPS2: Order opening fee params by increasing fee rates

4 participants

@martinsaposnic @ldk-reviews-bot @tnull @valentinewallace