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

[tnull]

We recently started tracking onchain payments in the store. It turns out the calculated amount was slightly off as in the outbound case it would include the paid fees. Here, we fix this minor bug and start tracking the fees separately.

[jkczyz]

Any reason we track fees for on-chain payments but not for lightning payments? I suppose we could for outbound lightning payments, but IIUC we can't know for inbound payments.

Also, should we use sats here instead of msats? Or is this to align with msats used in PaymentDetails?

[tnull]

Any reason we track fees for on-chain payments but not for lightning payments? I suppose we could for outbound lightning payments, but IIUC we can't know for inbound payments.

Hmm, you mean adding this as an optional field one layer above at PaymentDetails directly? I guess we could, but note that the field will only get updated for outbound payments once they succeeded. Now pushed a fixup in that direction, let me know if you find that preferable.

Also, should we use sats here instead of msats? Or is this to align with msats used in PaymentDetails?

Yes, originally it was to align the units, but if we don't make it a PaymentDetails field we could consider switching to sat I guess.

[jkczyz]

LGTM. Seems it would be useful to include.

[tnull]

Alright. Should I squash then? Will try to separate the amount calculation fix into a dedicated commit.

[tnull]

Went ahead and did so.

[tnull]

Force-pushed with the following changes:

> git diff-tree -U2 f429ee99 b4df52ce
diff --git a/tests/integration_tests_rust.rs b/tests/integration_tests_rust.rs
index b1d95dc1..19e7806d 100644
--- a/tests/integration_tests_rust.rs
+++ b/tests/integration_tests_rust.rs
@@ -357,7 +357,32 @@ fn onchain_send_receive() {
        let txid =
                node_b.onchain_payment().send_to_address(&addr_a, amount_to_send_sats, None).unwrap();
-       generate_blocks_and_wait(&bitcoind.client, &electrsd.client, 6);
        wait_for_tx(&electrsd.client, txid);
+       node_a.sync_wallets().unwrap();
+       node_b.sync_wallets().unwrap();
+
+       let payment_id = PaymentId(txid.to_byte_array());
+       let payment_a = node_a.payment(&payment_id).unwrap();
+       assert_eq!(payment_a.status, PaymentStatus::Pending);
+       match payment_a.kind {
+               PaymentKind::Onchain { status, .. } => {
+                       assert!(matches!(status, ConfirmationStatus::Unconfirmed));
+               },
+               _ => panic!("Unexpected payment kind"),
+       }
+       assert!(payment_a.fee_paid_msat > Some(0));
+       let payment_b = node_b.payment(&payment_id).unwrap();
+       assert_eq!(payment_b.status, PaymentStatus::Pending);
+       match payment_a.kind {
+               PaymentKind::Onchain { status, .. } => {
+                       assert!(matches!(status, ConfirmationStatus::Unconfirmed));
+               },
+               _ => panic!("Unexpected payment kind"),
+       }
+       assert!(payment_b.fee_paid_msat > Some(0));
+       assert_eq!(payment_a.amount_msat, Some(amount_to_send_sats * 1000));
+       assert_eq!(payment_a.amount_msat, payment_b.amount_msat);
+       assert_eq!(payment_a.fee_paid_msat, payment_b.fee_paid_msat);

+       generate_blocks_and_wait(&bitcoind.client, &electrsd.client, 6);
        node_a.sync_wallets().unwrap();
        node_b.sync_wallets().unwrap();
@@ -377,8 +402,5 @@ fn onchain_send_receive() {
        assert_eq!(node_b_payments.len(), 3);

-       let payment_id = PaymentId(txid.to_byte_array());
        let payment_a = node_a.payment(&payment_id).unwrap();
-       assert_eq!(payment_a.amount_msat, Some(amount_to_send_sats * 1000));
-       assert!(payment_a.fee_paid_msat > Some(0));
        match payment_a.kind {
                PaymentKind::Onchain { txid: _txid, status } => {
@@ -390,6 +412,4 @@ fn onchain_send_receive() {

        let payment_b = node_a.payment(&payment_id).unwrap();
-       assert_eq!(payment_b.amount_msat, Some(amount_to_send_sats * 1000));
-       assert!(payment_b.fee_paid_msat > Some(0));
        match payment_b.kind {
                PaymentKind::Onchain { txid: _txid, status } => {

[jkczyz]

Could you assert it is None in expect_payment_received_event?

[tnull]

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)?

[jkczyz]

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?

[tnull]

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