Expose amount forwarded in PaymentForwarded event
#2136
Conversation
marctyndel
force-pushed
the
2023-03-paymentforwarded-expose-amount-forwarded
branch
from
6823642
to
3ad0f09
Compare
lightning/src/ln/channelmanager.rs
Outdated
| @@ -4151,6 +4151,7 @@ where | |||
| claim_from_onchain_tx: from_onchain, | |||
| prev_channel_id, | |||
| next_channel_id, | |||
| amount_forwarded_msat: forwarded_htlc_value_msat, | |||
lightning/src/events/mod.rs
Outdated
| @@ -603,6 +603,9 @@ pub enum Event { | |||
| /// If this is `true`, the forwarded HTLC was claimed by our counterparty via an on-chain | |||
| /// transaction. | |||
| claim_from_onchain_tx: bool, | |||
| /// The final amount forwarded, in milli-satoshis, after the fee is deducted. | |||
| /// The caveat described above the `fee_earned_msat` field applies here as well. | |||
| amount_forwarded_msat: Option<u64>, | |||
There was a problem hiding this comment.
Is there a name that more clearly indicates this is the outbound amount we forwarded downstream, rather than the amount we received? I think this is fine just want to make sure we don't see anything obviously better, I don't have any ideas, though.
There was a problem hiding this comment.
Something like amount_outbound_forwarded_msat?
There was a problem hiding this comment.
maybe outgoing_amount_forwarded_msat? 🤷
There was a problem hiding this comment.
Up to you @marctyndel.
There was a problem hiding this comment.
Going with a blend of the above: outbound_amount_forwarded_msat (because HTLCIntercepted, also uses the word "outbound"...I do see "outgoing" is used elsewhere but at a glance not in anything exposed)
lightning/src/events/mod.rs
Outdated
| @@ -603,6 +603,9 @@ pub enum Event { | |||
| /// If this is `true`, the forwarded HTLC was claimed by our counterparty via an on-chain | |||
| /// transaction. | |||
| claim_from_onchain_tx: bool, | |||
| /// The final amount forwarded, in milli-satoshis, after the fee is deducted. | |||
| /// The caveat described above the `fee_earned_msat` field applies here as well. | |||
| amount_forwarded_msat: Option<u64>, | |||
There was a problem hiding this comment.
maybe outgoing_amount_forwarded_msat? 🤷
|
Feel free to squash the fixup commit(s) down when you next push. |
marctyndel
force-pushed
the
2023-03-paymentforwarded-expose-amount-forwarded
branch
from
6b3b0c9
to
cf3c211
Compare
Codecov ReportPatch coverage:
📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more Additional details and impacted files@@ Coverage Diff @@
## main #2136 +/- ##
==========================================
- Coverage 91.40% 91.37% -0.04%
==========================================
Files 102 102
Lines 49736 49568 -168
Branches 49736 49568 -168
==========================================
- Hits 45461 45291 -170
- Misses 4275 4277 +2
... and 7 files with indirect coverage changes Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report in Codecov by Sentry. |
| @@ -1780,7 +1780,7 @@ macro_rules! expect_payment_forwarded { | |||
| let events = $node.node.get_and_clear_pending_events(); | |||
| assert_eq!(events.len(), 1); | |||
| match events[0] { | |||
| Event::PaymentForwarded { fee_earned_msat, prev_channel_id, claim_from_onchain_tx, next_channel_id } => { | |||
| Event::PaymentForwarded { fee_earned_msat, prev_channel_id, claim_from_onchain_tx, next_channel_id, .. } => { | |||
There was a problem hiding this comment.
Question: for this test macro, if I add the new field, I get stuck trying to update the mid_update_fulfill_dance macro, because I can't see how to get the amount. I notice that fee is obtained there in a perhaps indirect way, by getting the forwarding_fee_base_msat from the channel config (meaning the test is run with 0 proportional fees), which suggests getting the amount from the HTLC might not straightforward here? Or I might be missing something.
There was a problem hiding this comment.
Hmm, yea, you could do a few different things - you could add a new macro variant (ie the top level ($node,,, thing you can just add another one with a new parameter), you could indeed try to deduce the correct amount based on the fee fetched from the channel (though a few tests will fail that), or you could just not worry about it. The code here is simple enough and there are at least a few tests which exercise it, so I'm admittedly not super worried about ensuring we have Maximal Test Coverage of it.
There was a problem hiding this comment.
Then if everyone's okay with it I'll go with the "just not worry about it" option
marctyndel
force-pushed
the
2023-03-paymentforwarded-expose-amount-forwarded
branch
from
cf3c211
to
ee2cb8e
Compare
This PR adds the amount forwarded to
PaymentForwardedevents.We'd like this value for analytics. At the moment I've been computing it using
fee_earned_msatand the outbound channel's forwarding policy, but this introduces rounding error (and I think could occasionally be incorrect if the policy was recently updated but not yet propagated).