[r2r] Lightning Refunds #1592

shamardy · 2022-12-28T16:40:01Z

#1045

Add 4 new methods to SwapOps traits. These methods are related to auto refunding of swap payments and early release of swap payments by the other side of the swap.
Implemented check_if_my_payment_sent, search_for_swap_tx_spend_my, search_for_swap_tx_spend_other SwapOps methods for LightningCoin.
Added MakerPaymentInstructionsReceived to MAKER_SUCCESS_EVENTS, TakerPaymentInstructionsReceived to TAKER_SUCCESS_EVENTS.
Refactored LightningDB methods to reduce calls to DB.

…wapOps functions

… trait

…nt secret from payment info and db

…ymentInstructionsReceived to MAKER_SUCCESS_EVENTS

…rt, recreate_swap_data

shamardy · 2022-12-29T09:25:36Z

@artemii235 @ozkanonur I think the new is_auto_refundable and wait_for_htlc_refund methods
https://github.com/KomodoPlatform/atomicDEX-API/blob/8df164c96106a6c16dc9939f453980b5a3261f64/mm2src/coins/lp_coins.rs#L719-L727
should be implemented for TendermintCoin/TendermintToken to return MakerPaymentRefunded/TakerPaymentRefunded events instead of MakerPaymentRefundFailed/TakerPaymentRefundFailed events and for the early release of maker funds to work in the future for a lightning/tendermint swap
https://github.com/KomodoPlatform/atomicDEX-API/blob/8df164c96106a6c16dc9939f453980b5a3261f64/mm2src/mm2_main/src/lp_swap/taker_swap.rs#L1711-L1720

artemii235

Thanks for the PR! It's a first review iteration.

mm2src/mm2_main/src/lp_swap/taker_swap.rs

mm2src/mm2_main/src/lp_swap/maker_swap.rs

artemii235 · 2023-01-06T12:11:21Z

mm2src/coins/lightning.rs

    }

+    // Todo: need to also check on-chain spending


Do you plan to do it on next iteration? 🙂

Yes, this should be done in next sprint. This can only happen when the channel the payment is sent/received over is closed before the swap finishes, so this will be done alongside claiming swap or refunding on-chain for closed channels.
I believe it might be easier to implement this if Event::ChannelClosed from rust-lightning side provides info about unsettled HTLCs in the channel closing transaction.https://github.com/KomodoPlatform/atomicDEX-API/blob/ae5daf7c082b3493555057a7c7559e6d5d211893/mm2src/coins/lightning/ln_events.rs#L85 But if this can't be done easy from rust-lightning side I believe it can be done on mm2 straight away. Just need to check the rust-lightning code in details for unsettled HTLCs and make sure I don't miss anything before implementing this, that's why I moved it to next sprint.

mm2src/coins/lp_coins.rs

sergeyboyko0791

Good job!
First review iteration

sergeyboyko0791 · 2023-01-09T13:34:28Z

mm2src/coins/lightning.rs

@@ -741,6 +785,74 @@ impl SwapOps for LightningCoin {
        }
    }

+    fn can_be_released(&self) -> bool { true }


In my opinion, can_be_released is not clear enough. Could you please give it another name?
At least, can_release_payment or can_fail_htlc_backwards

can_be_released is removed after implementing this suggestion #1592 (comment) by @artemii235

sergeyboyko0791 · 2023-01-09T13:38:33Z

mm2src/coins/lightning/ln_events.rs

+                    PaymentInfo::new(payment_hash, PaymentType::InboundPayment, "keysend".into(), amt_msat);
+                payment_info.preimage = Some(preimage);
+                payment_info.status = HTLCStatus::Received;


How about adding PaymentInfo::with_preimage_and_status?

sergeyboyko0791 · 2023-01-09T13:38:50Z

mm2src/coins/lightning/ln_events.rs

+                        "Swap Payment".into(),
+                        amt_msat,
+                    );
+                    payment_info.status = HTLCStatus::Received;


How about adding PaymentInfo::with_status?

sergeyboyko0791 · 2023-01-09T13:45:23Z

mm2src/coins/lightning/ln_sql.rs

            amount_msat,
            fee_paid_msat,
            is_outbound,
            status,
            created_at,
            last_updated
        ) VALUES (
-            ?1, ?2, ?3, ?4, ?5, ?6, ?7, ?8, ?9, ?10, ?11
+            ?1, ?2, ?3, ?4, ?5, ?6, ?7, ?8, ?9, ?10


How about giving a name to each SQL parameter like it's done for starts_swaps:
https://github.com/KomodoPlatform/atomicDEX-API/blob/728b20db56c25eb35a00cc1e922267bb4736923f/mm2src/mm2_main/src/database/stats_swaps.rs#L135-L150

sergeyboyko0791 · 2023-01-09T13:46:29Z

mm2src/coins/lightning/ln_sql.rs

+            let params = [
+                &preimage as &dyn ToSql,
+                &last_updated as &dyn ToSql,
+                &payment_hash as &dyn ToSql,
+            ];


Please consider using the rusqlite::params or db_common::owned_named_params macros

…TakerSwapOps

…fo impl

…l_sql, used params macro where applicable

shamardy · 2023-01-10T16:17:08Z

@artemii235 @sergeyboyko0791 this is ready for next review iteration

sergeyboyko0791

Next review iteration

mm2src/coins/lightning.rs

sergeyboyko0791 · 2023-01-10T21:08:11Z

mm2src/mm2_main/src/lp_swap/maker_swap.rs

@@ -1079,7 +1086,28 @@ impl MakerSwap {
            ]));
        }

+        let taker_payment = self.r().taker_payment.clone();
+        if let Some(payment) = &taker_payment {
+            if let Err(e) = self.taker_coin.on_maker_payment_refund_start(&payment.tx_hex).await {


This is really confusing because TakerSwapOps::on_maker_payment_refund_start is called on the Maker side if only the taker payment has been sent.
Shouldn't TakerSwapOps be used on the Taker side only?

Also I'm really not sure if MakerSwapOps and TakerSwapOps names are suitable as the coins traits.
What if we have MakerSwapMakerCoin, MakerSwapTakerCoin, TakerSwapMakerCoin, TakerSwapTakerCoin.

What if we have MakerSwapMakerCoin, MakerSwapTakerCoin, TakerSwapMakerCoin, TakerSwapTakerCoin

I like the idea, it will remove a lot of the confusion. We need @artemii235 opinion on this too before proceeding since he suggested this #1592 (comment).

Yeah, this looks more clear than Maker/TakerSwapOps, so I agree 🙂

Only MakerSwapTakerCoin, TakerSwapMakerCoin are needed in this PR. Will open an issue for the refactor/spliting of SwapOps #1592 (comment) to the 4 above traits.

Will open an issue for the refactor/spliting of SwapOps #1592 (comment) to the 4 above traits.

#1610

Consider this as solved!

mm2src/mm2_main/src/lp_swap/maker_swap.rs

mm2src/mm2_main/src/lp_swap/taker_swap.rs

sergeyboyko0791 · 2023-01-10T21:27:05Z

mm2src/mm2_main/src/lp_swap/taker_swap.rs

+        // Since taker payment is refunded successfully, maker payment can be released for lightning and similar protocols.
+        // # Important: New code that leads to refund failure shouldn't be added below this code block.
+        if let Err(e) = self.maker_coin.on_taker_payment_refund_success(&maker_payment).await {
+            error!("Error {} on calling on_taker_payment_refund!", e)
+        }


Should we do the same if self.taker_coin.is_auto_refundable()?

Yes, @sergeyboyko0791 @artemii235 do you think it's worth adding 2 new swap commands TakerSwapCommand::OnRefundStart , TakerSwapCommand::OnRefundSuccess and the same for maker_swap? This will help us avoid a lot of confusion in the future.

Yeah, looks like it's worth doing it. As these are commands, I would name them PrepareForRefund and FinalizeRefund or something similar 🙂

PrepareForRefund and FinalizeRefund are good enough names, couldn't come up with better names myself :)

I really like this idea.

do you think it's worth adding 2 new swap commands TakerSwapCommand::OnRefundStart , TakerSwapCommand::OnRefundSuccess and the same for maker_swap? This will help us avoid a lot of confusion in the future.

Consider this as solved, but will not resolve so Artem can see this conversation at the next review iteration

mm2src/mm2_main/src/lp_swap/taker_swap.rs

mm2src/mm2_main/src/lp_swap/maker_swap.rs

…mentation for LightningCoin

…_auto_refundable in maker_swap's recover_funds

…pMakerCoin/MakerSwapTakerCoin

…Refund, TakerSwapCommand::FinalizeTakerPaymentRefund

…nd, MakerSwapCommand::FinalizeMakerPaymentRefund

shamardy · 2023-01-12T19:27:08Z

@artemii235 @sergeyboyko0791 this is ready for another review!

mm2src/coins/lightning.rs

mm2src/coins/lp_coins.rs

sergeyboyko0791 · 2023-01-13T11:45:06Z

mm2src/mm2_main/src/lp_swap/taker_swap.rs

+        // Since taker payment is refunded successfully, maker payment can be released for lightning and similar protocols.
+        // # Important: New code that leads to refund failure shouldn't be added below this code block.
+        if let Err(e) = self.maker_coin.on_taker_payment_refund_success(&maker_payment).await {
+            error!("Error {} on calling on_taker_payment_refund!", e)
+        }


I really like this idea.

do you think it's worth adding 2 new swap commands TakerSwapCommand::OnRefundStart , TakerSwapCommand::OnRefundSuccess and the same for maker_swap? This will help us avoid a lot of confusion in the future.

Consider this as solved, but will not resolve so Artem can see this conversation at the next review iteration

sergeyboyko0791

Thank you for the fixes! Just a few questions

sergeyboyko0791

Awesome!

artemii235

🔥

shamardy added 14 commits December 12, 2022 21:47

lightning refunds wip: add is_auto_refundable, wait_for_htlc_refund S…

3e907ac

…wapOps functions

lightning refunds wip: move fail_htlc_backwards to the right place

3a6c61d

lightning refunds wip: add update_payment_status_in_db to LightningDB…

daf5da2

… trait

add update_payment_to_received_in_db method to LightningDB

66e6f80

add update_payment_to_sent_in_db method to LightningDB

2a5aa63

change add_or_update_payment_in_db to add_payment_to_db, remove payme…

420e763

…nt secret from payment info and db

edit docs/notes related to fail_htlc_backwards

e384af3

Merge remote-tracking branch 'origin/dev' into ln-refunds

6067f03

fix typo: rename if_my_payment_spent_args to if_my_payment_sent_args

0353a81

impl check_if_my_payment_sent for LightningCoin

84d19b1

add TakerPaymentInstructionsReceived to TAKER_SUCCESS_EVENTS, MakerPa…

e61e76b

…ymentInstructionsReceived to MAKER_SUCCESS_EVENTS

implemented recover_funds for lightning, revised lightning swap resta…

ab68e26

…rt, recreate_swap_data

Remove unneeded todos

fbc2da1

Merge remote-tracking branch 'origin/dev' into ln-refunds

8df164c

shamardy changed the title ~~[wip] Lightning Refunds~~ [r2r] Lightning Refunds Dec 28, 2022

shamardy requested review from artemii235, caglaryucekaya and sergeyboyko0791 December 28, 2022 16:56

artemii235 requested changes Jan 6, 2023

View reviewed changes

sergeyboyko0791 suggested changes Jan 9, 2023

View reviewed changes

shamardy changed the title ~~[r2r] Lightning Refunds~~ [wip] Lightning Refunds Jan 9, 2023

shamardy added 8 commits January 9, 2023 18:25

review fixes wip: add MakerSwapOps and TakerSwapOps

f133368

review fixes wip: update deny.toml

445a6ee

review fixes wip: revert deny.toml changes

1f370bb

Merge remote-tracking branch 'origin/dev' into ln-refunds

72b3467

review fixes wip: add on_start/success trait methods to MakerSwapOps/…

ae5daf7

…TakerSwapOps

review fixes wip: fix wait_for_htlc_refund to not return TransactionFut

d555e09

review fixes wip: add with_preimage, with_status methods to PaymentIn…

98b5798

…fo impl

review fixes wip: use owned_named_params for insert_payment_sql

ee12d6b

first review fixes complete: use owned_named_params for insert_channe…

a35db06

…l_sql, used params macro where applicable

shamardy changed the title ~~[wip] Lightning Refunds~~ [r2r] Lightning Refunds Jan 10, 2023

sergeyboyko0791 suggested changes Jan 10, 2023

View reviewed changes

shamardy changed the title ~~[r2r] Lightning Refunds~~ [wip] Lightning Refunds Jan 11, 2023

shamardy added 2 commits January 11, 2023 12:42

second review fixes wip: use repeatable in wait_for_htlc_refund imple…

733aca1

…mentation for LightningCoin

second review fixes wip: remove white spaces in recover_funds, fix is…

3279aa6

…_auto_refundable in maker_swap's recover_funds

shamardy changed the title ~~[wip] Lightning Refunds~~ [r2r] Lightning Refunds Jan 11, 2023

shamardy added 2 commits January 12, 2023 10:39

Merge remote-tracking branch 'origin/dev' into ln-refunds

87f5fce

second review fixes wip: rename MakerSwapOps/TakerSwapOps to TakerSwa…

cbe3d48

…pMakerCoin/MakerSwapTakerCoin

shamardy mentioned this pull request Jan 12, 2023

Split SwapOps to multiple traits #1610

Open

shamardy added 2 commits January 12, 2023 20:42

second review fixes wip: Add TakerSwapCommand::PrepareForTakerPayment…

b329de3

…Refund, TakerSwapCommand::FinalizeTakerPaymentRefund

second review fixes: Add MakerSwapCommand::PrepareForMakerPaymentRefu…

76632df

…nd, MakerSwapCommand::FinalizeMakerPaymentRefund

sergeyboyko0791 reviewed Jan 13, 2023

View reviewed changes

more review fixes

0d2b7da

sergeyboyko0791 approved these changes Jan 13, 2023

View reviewed changes

artemii235 approved these changes Jan 13, 2023

View reviewed changes

artemii235 merged commit e2f9046 into dev Jan 13, 2023

artemii235 deleted the ln-refunds branch January 13, 2023 15:13

cipig mentioned this pull request Jan 31, 2023

successful swaps have is_success 0 in MM2.db #1637

Closed

shamardy mentioned this pull request Jan 31, 2023

Lightning integration #1045

Open

24 tasks

shamardy mentioned this pull request Feb 17, 2023

[release] 1.0.0-beta #1576

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[r2r] Lightning Refunds #1592

[r2r] Lightning Refunds #1592

shamardy commented Dec 28, 2022

shamardy commented Dec 29, 2022

artemii235 left a comment

artemii235 Jan 6, 2023

shamardy Jan 10, 2023 •

edited

Loading

sergeyboyko0791 left a comment

sergeyboyko0791 Jan 9, 2023

shamardy Jan 9, 2023

sergeyboyko0791 Jan 9, 2023

sergeyboyko0791 Jan 9, 2023

sergeyboyko0791 Jan 9, 2023

sergeyboyko0791 Jan 9, 2023

shamardy commented Jan 10, 2023

sergeyboyko0791 left a comment

sergeyboyko0791 Jan 10, 2023

shamardy Jan 11, 2023

artemii235 Jan 12, 2023

shamardy Jan 12, 2023

shamardy Jan 12, 2023

sergeyboyko0791 Jan 13, 2023

sergeyboyko0791 Jan 10, 2023

shamardy Jan 11, 2023 •

edited

Loading

artemii235 Jan 12, 2023 •

edited

Loading

shamardy Jan 12, 2023

sergeyboyko0791 Jan 13, 2023

shamardy commented Jan 12, 2023

sergeyboyko0791 Jan 13, 2023

sergeyboyko0791 left a comment

sergeyboyko0791 left a comment

artemii235 left a comment

[r2r] Lightning Refunds #1592

[r2r] Lightning Refunds #1592

Conversation

shamardy commented Dec 28, 2022

shamardy commented Dec 29, 2022

artemii235 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shamardy Jan 10, 2023 • edited Loading

Choose a reason for hiding this comment

sergeyboyko0791 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shamardy commented Jan 10, 2023

sergeyboyko0791 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shamardy Jan 11, 2023 • edited Loading

Choose a reason for hiding this comment

artemii235 Jan 12, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shamardy commented Jan 12, 2023

Choose a reason for hiding this comment

sergeyboyko0791 left a comment

Choose a reason for hiding this comment

sergeyboyko0791 left a comment

Choose a reason for hiding this comment

artemii235 left a comment

Choose a reason for hiding this comment

shamardy Jan 10, 2023 •

edited

Loading

shamardy Jan 11, 2023 •

edited

Loading

artemii235 Jan 12, 2023 •

edited

Loading