Skip to content

Add fees value for recent payment details#4674

Open
randomlogin wants to merge 1 commit into
lightningdevkit:mainfrom
randomlogin:expose-fees-in-payment-details
Open

Add fees value for recent payment details#4674
randomlogin wants to merge 1 commit into
lightningdevkit:mainfrom
randomlogin:expose-fees-in-payment-details

Conversation

@randomlogin

Copy link
Copy Markdown
Contributor

Previously it was not possible to obtain fees of pending/fulfilled payments.
Also we can add and populate the fee field for RecentPaymentDetails::Abandoned, but I doubt that it is useful, as the payment was not made.

I ran cargo fmt, so unrelated parts of code were changed.

Removed unnecessary clarification "in msat" for total_msat field.

@ldk-reviews-bot

ldk-reviews-bot commented Jun 10, 2026

Copy link
Copy Markdown

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

@ldk-claude-review-bot

ldk-claude-review-bot commented Jun 10, 2026

Copy link
Copy Markdown
Collaborator

I re-examined the full data flow and all construction/match sites:

  • mark_fulfilled (outbound_payment.rs:304) and claim_htlc (2275) both derive the fee from get_pending_fee_msat() on the still-Retryable/Abandoned payment, with nothing mutating state between the PaymentSent push and mark_fulfilled at 2285 — so Fulfilled.fee_paid_msat and PaymentSent.fee_paid_msat are guaranteed identical. mark_fulfilled has only one caller.
  • get_pending_fee_msat now also covers the Fulfilled arm so the value survives MPP second-path/onchain re-claims and abandon→succeed transitions.
  • TLV (7, fee_paid_msat, option) for Fulfilled does not collide (existing: 0,1,3,5).
  • Version wording "prior to 0.3.0" matches the current crate version (0.3.0+git).
  • All other RecentPaymentDetails::{Pending,Fulfilled} sites (e.g. payment_tests.rs:1788, offers_tests.rs expect_recent_payment!) use .. or pattern-only matching, so no compile breakage.

No issues found.

@ldk-reviews-bot ldk-reviews-bot requested a review from wpaulino June 10, 2026 14:10
Comment thread lightning/src/ln/channelmanager.rs Outdated
bool,
) -> (Option<MonitorUpdateCompletionAction>, Option<RAAMonitorUpdateBlockingAction>),
)
-> (Option<MonitorUpdateCompletionAction>, Option<RAAMonitorUpdateBlockingAction>),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

These should go away if you format with 1.75

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

fixed

Comment thread lightning/src/ln/channelmanager.rs Outdated
/// Total routing fees paid for this payment, as also reported via the `fee_paid_msat`
/// field of [`Event::PaymentSent`].
///
/// `None` for payments fulfilled before this field was introduced.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We should note the specific version

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

added specific version (0.3.0)

@ldk-reviews-bot

Copy link
Copy Markdown

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

Introduce fields `pending_fee_msat` for `RecentPaymentDetails::Pending`
and `fee_paid_msat` for `RecentPaymentDetails::Fulfilled`.
@randomlogin randomlogin force-pushed the expose-fees-in-payment-details branch from 95ece4a to af19ed5 Compare June 10, 2026 21:51
@randomlogin randomlogin requested a review from wpaulino June 10, 2026 21:53
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