Skip to content
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

Fix amount calculation and track paid fees in PaymentKind::Onchain #468

Merged
merged 2 commits into from
Feb 22, 2025
Merged

Fix amount calculation and track paid fees in PaymentKind::Onchain #468

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a38b180
Fix amount calculation for tracked onchain payments
tnull Feb 13, 2025
0b7af80
Track paid fees in `PaymentDetails`
tnull Feb 13, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions bindings/ldk_node.udl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -412,6 +412,7 @@ dictionary PaymentDetails {
PaymentId id;
PaymentKind kind;
u64? amount_msat;
u64? fee_paid_msat;
PaymentDirection direction;
PaymentStatus status;
u64 latest_update_timestamp;
Expand Down
3 changes: 3 additions & 0 deletions src/event.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -725,6 +725,7 @@ where
payment_id,
kind,
Some(amount_msat),
None,
PaymentDirection::Inbound,
PaymentStatus::Pending,
);
Expand Down Expand Up @@ -765,6 +766,7 @@ where
payment_id,
kind,
Some(amount_msat),
None,
PaymentDirection::Inbound,
PaymentStatus::Pending,
);
Expand Down Expand Up @@ -931,6 +933,7 @@ where
let update = PaymentDetailsUpdate {
hash: Some(Some(payment_hash)),
preimage: Some(Some(payment_preimage)),
fee_paid_msat: Some(fee_paid_msat),
status: Some(PaymentStatus::Succeeded),
..PaymentDetailsUpdate::new(payment_id)
};
Expand Down
6 changes: 6 additions & 0 deletions src/payment/bolt11.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -160,6 +160,7 @@ impl Bolt11Payment {
payment_id,
kind,
invoice.amount_milli_satoshis(),
None,
PaymentDirection::Outbound,
PaymentStatus::Pending,
);
Expand All @@ -182,6 +183,7 @@ impl Bolt11Payment {
payment_id,
kind,
invoice.amount_milli_satoshis(),
None,
PaymentDirection::Outbound,
PaymentStatus::Failed,
);
Expand Down Expand Up @@ -293,6 +295,7 @@ impl Bolt11Payment {
payment_id,
kind,
Some(amount_msat),
None,
PaymentDirection::Outbound,
PaymentStatus::Pending,
);
Expand All @@ -315,6 +318,7 @@ impl Bolt11Payment {
payment_id,
kind,
Some(amount_msat),
None,
PaymentDirection::Outbound,
PaymentStatus::Failed,
);
Expand Down Expand Up @@ -531,6 +535,7 @@ impl Bolt11Payment {
id,
kind,
amount_msat,
None,
PaymentDirection::Inbound,
PaymentStatus::Pending,
);
Expand Down Expand Up @@ -666,6 +671,7 @@ impl Bolt11Payment {
id,
kind,
amount_msat,
None,
PaymentDirection::Inbound,
PaymentStatus::Pending,
);
Expand Down
6 changes: 6 additions & 0 deletions src/payment/bolt12.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,6 +113,7 @@ impl Bolt12Payment {
payment_id,
kind,
Some(offer_amount_msat),
None,
PaymentDirection::Outbound,
PaymentStatus::Pending,
);
Expand All @@ -137,6 +138,7 @@ impl Bolt12Payment {
payment_id,
kind,
Some(offer_amount_msat),
None,
PaymentDirection::Outbound,
PaymentStatus::Failed,
);
Expand Down Expand Up @@ -217,6 +219,7 @@ impl Bolt12Payment {
payment_id,
kind,
Some(amount_msat),
None,
PaymentDirection::Outbound,
PaymentStatus::Pending,
);
Expand All @@ -241,6 +244,7 @@ impl Bolt12Payment {
payment_id,
kind,
Some(amount_msat),
None,
PaymentDirection::Outbound,
PaymentStatus::Failed,
);
Expand Down Expand Up @@ -338,6 +342,7 @@ impl Bolt12Payment {
payment_id,
kind,
Some(refund.amount_msats()),
None,
PaymentDirection::Inbound,
PaymentStatus::Pending,
);
Expand Down Expand Up @@ -402,6 +407,7 @@ impl Bolt12Payment {
payment_id,
kind,
Some(amount_msat),
None,
PaymentDirection::Outbound,
PaymentStatus::Pending,
);
Expand Down
2 changes: 2 additions & 0 deletions src/payment/spontaneous.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -140,6 +140,7 @@ impl SpontaneousPayment {
payment_id,
kind,
Some(amount_msat),
None,
PaymentDirection::Outbound,
PaymentStatus::Pending,
);
Expand All @@ -161,6 +162,7 @@ impl SpontaneousPayment {
payment_id,
kind,
Some(amount_msat),
None,
PaymentDirection::Outbound,
PaymentStatus::Failed,
);
Expand Down
42 changes: 36 additions & 6 deletions src/payment/store.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,13 @@ pub struct PaymentDetails {
pub kind: PaymentKind,
/// The amount transferred.
pub amount_msat: Option<u64>,
/// The fees that were paid for this payment.
///
/// For Lightning payments, this will only be updated for outbound payments once they
/// succeeded.
///
/// Will be `None` for Lightning payments made with LDK Node v0.4.x and earlier.
pub fee_paid_msat: Option<u64>,
/// The direction of the payment.
pub direction: PaymentDirection,
/// The status of the payment.
Expand All @@ -52,14 +59,14 @@ pub struct PaymentDetails {

impl PaymentDetails {
pub(crate) fn new(
id: PaymentId, kind: PaymentKind, amount_msat: Option<u64>, direction: PaymentDirection,
status: PaymentStatus,
id: PaymentId, kind: PaymentKind, amount_msat: Option<u64>, fee_paid_msat: Option<u64>,
direction: PaymentDirection, status: PaymentStatus,
) -> Self {
let latest_update_timestamp = SystemTime::now()
.duration_since(UNIX_EPOCH)
.unwrap_or(Duration::from_secs(0))
.as_secs();
Self { id, kind, amount_msat, direction, status, latest_update_timestamp }
Self { id, kind, amount_msat, fee_paid_msat, direction, status, latest_update_timestamp }
}

pub(crate) fn update(&mut self, update: &PaymentDetailsUpdate) -> bool {
Expand Down Expand Up @@ -154,6 +161,10 @@ impl PaymentDetails {
update_if_necessary!(self.amount_msat, amount_opt);
}

if let Some(fee_paid_msat_opt) = update.fee_paid_msat {
update_if_necessary!(self.fee_paid_msat, fee_paid_msat_opt);
}

if let Some(status) = update.status {
update_if_necessary!(self.status, status);
}
Expand Down Expand Up @@ -192,6 +203,7 @@ impl Writeable for PaymentDetails {
(4, None::<Option<PaymentSecret>>, required),
(5, self.latest_update_timestamp, required),
(6, self.amount_msat, required),
(7, self.fee_paid_msat, option),
(8, self.direction, required),
(10, self.status, required)
});
Expand All @@ -213,6 +225,7 @@ impl Readable for PaymentDetails {
(4, secret, required),
(5, latest_update_timestamp, (default_value, unix_time_secs)),
(6, amount_msat, required),
(7, fee_paid_msat, option),
(8, direction, required),
(10, status, required)
});
Expand Down Expand Up @@ -253,7 +266,15 @@ impl Readable for PaymentDetails {
}
};

Ok(PaymentDetails { id, kind, amount_msat, direction, status, latest_update_timestamp })
Ok(PaymentDetails {
id,
kind,
amount_msat,
fee_paid_msat,
direction,
status,
latest_update_timestamp,
})
}
}

Expand Down Expand Up @@ -479,6 +500,7 @@ pub(crate) struct PaymentDetailsUpdate {
pub preimage: Option<Option<PaymentPreimage>>,
pub secret: Option<Option<PaymentSecret>>,
pub amount_msat: Option<Option<u64>>,
pub fee_paid_msat: Option<Option<u64>>,
pub direction: Option<PaymentDirection>,
pub status: Option<PaymentStatus>,
pub confirmation_status: Option<ConfirmationStatus>,
Expand All @@ -492,6 +514,7 @@ impl PaymentDetailsUpdate {
preimage: None,
secret: None,
amount_msat: None,
fee_paid_msat: None,
direction: None,
status: None,
confirmation_status: None,
Expand Down Expand Up @@ -521,6 +544,7 @@ impl From<&PaymentDetails> for PaymentDetailsUpdate {
preimage: Some(preimage),
secret: Some(secret),
amount_msat: Some(value.amount_msat),
fee_paid_msat: Some(value.fee_paid_msat),
direction: Some(value.direction),
status: Some(value.status),
confirmation_status,
Expand Down Expand Up @@ -708,8 +732,14 @@ mod tests {
.is_err());

let kind = PaymentKind::Bolt11 { hash, preimage: None, secret: None };
let payment =
PaymentDetails::new(id, kind, None, PaymentDirection::Inbound, PaymentStatus::Pending);
let payment = PaymentDetails::new(
id,
kind,
None,
None,
PaymentDirection::Inbound,
PaymentStatus::Pending,
);

assert_eq!(Ok(false), payment_store.insert(payment.clone()));
assert!(payment_store.get(&id).is_some());
Expand Down
27 changes: 15 additions & 12 deletions src/wallet/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,6 @@ use bitcoin::{

use std::ops::Deref;
use std::sync::{Arc, Mutex};
use std::time::{Duration, SystemTime, UNIX_EPOCH};

pub(crate) enum OnchainSendAmount {
ExactRetainingReserve { amount_sats: u64, cur_anchor_reserve_sats: u64 },
Expand Down Expand Up @@ -146,11 +145,6 @@ where
fn update_payment_store<'a>(
&self, locked_wallet: &'a mut PersistedWallet<KVStoreWalletPersister>,
) -> Result<(), Error> {
let latest_update_timestamp = SystemTime::now()
.duration_since(UNIX_EPOCH)
.unwrap_or(Duration::from_secs(0))
.as_secs();

for wtx in locked_wallet.transactions() {
let id = PaymentId(wtx.tx_node.txid.to_byte_array());
let txid = wtx.tx_node.txid;
Expand Down Expand Up @@ -187,25 +181,34 @@ where
// here to determine the `PaymentKind`, but that's not really satisfactory, so
// we're punting on it until we can come up with a better solution.
let kind = crate::payment::PaymentKind::Onchain { txid, status: confirmation_status };
let fee = locked_wallet.calculate_fee(&wtx.tx_node.tx).unwrap_or(Amount::ZERO);
let (sent, received) = locked_wallet.sent_and_received(&wtx.tx_node.tx);
let (direction, amount_msat) = if sent > received {
let direction = PaymentDirection::Outbound;
let amount_msat = Some(sent.to_sat().saturating_sub(received.to_sat()) * 1000);
let amount_msat = Some(
sent.to_sat().saturating_sub(fee.to_sat()).saturating_sub(received.to_sat())
* 1000,
);
(direction, amount_msat)
} else {
let direction = PaymentDirection::Inbound;
let amount_msat = Some(received.to_sat().saturating_sub(sent.to_sat()) * 1000);
let amount_msat = Some(
received.to_sat().saturating_sub(sent.to_sat().saturating_sub(fee.to_sat()))
* 1000,
);
(direction, amount_msat)
};

let payment = PaymentDetails {
let fee_paid_msat = Some(fee.to_sat() * 1000);

let payment = PaymentDetails::new(
id,
kind,
amount_msat,
fee_paid_msat,
direction,
status: payment_status,
latest_update_timestamp,
};
payment_status,
);

self.payment_store.insert_or_update(&payment)?;
}
Expand Down
6 changes: 6 additions & 0 deletions tests/common/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,10 @@ macro_rules! expect_payment_received_event {
ref e @ Event::PaymentReceived { payment_id, amount_msat, .. } => {
println!("{} got event {:?}", $node.node_id(), e);
assert_eq!(amount_msat, $amount_msat);
let payment = $node.payment(&payment_id.unwrap()).unwrap();
if !matches!(payment.kind, PaymentKind::Onchain { .. }) {
assert_eq!(payment.fee_paid_msat, None);
}
$node.event_handled();
payment_id
},
Expand Down Expand Up @@ -148,6 +152,8 @@ macro_rules! expect_payment_successful_event {
if let Some(fee_msat) = $fee_paid_msat {
assert_eq!(fee_paid_msat, fee_msat);
}
let payment = $node.payment(&$payment_id.unwrap()).unwrap();
assert_eq!(payment.fee_paid_msat, fee_paid_msat);
Comment on lines +155 to +156
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you assert it is None in expect_payment_received_event?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hmm, as mentioned above, it won't be None for onchain payments? Do you think this is confusing? Should we hard-code it to be None in the inbound case, even though we know the transactions fees there, too (in contrast to Lightning)?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, sorry. I think that I assumed expect_payment_successful_event was only used for lightning payments. We could condition the assertion on the PaymentKind not being on-chain, maybe?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Alright, now amended with the following changes:

> git diff-tree -U2  b4df52ce 0b7af80a
diff --git a/tests/common/mod.rs b/tests/common/mod.rs
index ab0aa7c1..2ad73682 100644
--- a/tests/common/mod.rs
+++ b/tests/common/mod.rs
@@ -104,4 +104,8 @@ macro_rules! expect_payment_received_event {
                                println!("{} got event {:?}", $node.node_id(), e);
                                assert_eq!(amount_msat, $amount_msat);
+                               let payment = $node.payment(&payment_id.unwrap()).unwrap();
+                               if !matches!(payment.kind, PaymentKind::Onchain { .. }) {
+                                       assert_eq!(payment.fee_paid_msat, None);
+                               }
                                $node.event_handled();
                                payment_id

assert_eq!(payment_id, $payment_id);
$node.event_handled();
},
Expand Down
Loading