-
Notifications
You must be signed in to change notification settings - Fork 96
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.
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
Add counterparty_skimmed_fee_msat
field to PaymentKind::Bolt11Jit
#497
base: main
Are you sure you want to change the base?
Conversation
.. we add a field describing exactly how much the LSP withheld.
ffad4f3
to
05c77da
Compare
assert_eq!(hash, h); | ||
assert_eq!(preimage, p); | ||
assert_eq!(secret, s); | ||
assert_eq!(None, c); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: should we test with Some() as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No? This test checks that we're able to deserialize a (by now ancient) version of PaymentDetails
. As we're just adding the new field, it always has to be None
for any PaymentDetails
deserialized from the old version.
src/payment/store.rs
Outdated
@@ -346,6 +364,12 @@ pub enum PaymentKind { | |||
preimage: Option<PaymentPreimage>, | |||
/// The secret used by the payment. | |||
secret: Option<PaymentSecret>, | |||
/// The value, in thousands of a satoshi, that was deducted off of this payment as an extra |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"The value, in thousands of a satoshi" is an awkward way of saying that the value is in msat?
where msat is thousandth of a satoshi.
/// The value, in thousands of a satoshi, that was deducted off of this payment as an extra | |
/// The value, in millisatoshis (msat), that was deducted off of this payment as an extra |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mhh, well the msat
denomination is given in the variable name, no? These docs are copied from LDK, and tbh., since they are accurate, I don't see the need to change them here?
05c77da
to
6297502
Compare
Excuse the delay here, now addressed the feedback. |
Closes #495
We add a field to
Bolt11Jit
describing exactly how much the LSP withheld.Additionally, we clarify when
PaymentDetails::amount_msat
might beNone
(for now).