[r2r] Lightning taker swap p.o.c #1497

shamardy · 2022-10-11T15:34:26Z

#1045

Moved all lightning RPCs to coins/rpc_command/lightning [r2r] Update rust-lightning to latest version, 0_confs channels, complete btc spv, multiple fixes #1452 (comment)
Lightning RPCs now use the lightning:: namespace similar to gui_storage:: and task::
TakerFee and MakerPayment swap messages are modified to include payment instructions for the other side, in the case of lightning this payment instructions is a lightning invoice.
MakerPaymentInstructionsReceived/TakerPaymentInstructionsReceived events are added to MakerSwapEvent/TakerSwapEvent
2 new functions payment_instructions, validate_instructions are added to the SwapOps trait
Implemented the required SwapOps, MarketCoinOps, MmCoin trait functions to do a lightning taker swap, some of these functions include default or dummy values for the sake of the p.o.c. , they should be completed in upcoming sprints/PRs
test_lightning_taker_swap is the unit test for a RICK/tBTC-TEST-lightning swap, it's ignored because it requires refilling the tBTC and RICK addresses with test coins periodically. This test can be run locally to check that everything is working fine.
There is a lot of todo comments left in the code for upcoming sprints/PRs.
A detailed diagram with notes for the required changes in our current swaps implementation for lightning swaps to work can be found here https://docs.google.com/document/d/1liFL704KV9yQzeVRoKCkztNNRrL9T4ak5wQaY-pPY7U/

…to rpc_command

…x for lightning methods

…nstructions to the other side

…gCoin, copy needed parts of SecretHashAlgo from iris-swap-poc branch

…aid by maker to taker) instead of TakerPayment msg

…d_taker_payment functions

…yments, better todo comments

…ming funds)

onur-ozkan

Next review iteration 🙂

onur-ozkan · 2022-10-17T10:59:51Z

mm2src/coins/lightning.rs

+            let is_channel_id =
+                filter.channel_id.is_none() || Some(&channel_details.channel_id) == filter.channel_id.as_ref();
+
+            let is_counterparty_node_id = filter.counterparty_node_id.is_none()
+                || Some(&channel_details.counterparty_node_id) == filter.counterparty_node_id.as_ref();
+
+            let is_funding_tx = filter.funding_tx.is_none() || channel_details.funding_tx == filter.funding_tx;
+
+            let is_from_funding_value_sats =
+                Some(&channel_details.funding_tx_value_sats) >= filter.from_funding_value_sats.as_ref();
+
+            let is_to_funding_value_sats = filter.to_funding_value_sats.is_none()
+                || Some(&channel_details.funding_tx_value_sats) <= filter.to_funding_value_sats.as_ref();
+
+            let is_outbound =
+                filter.is_outbound.is_none() || Some(&channel_details.is_outbound) == filter.is_outbound.as_ref();
+
+            let is_from_balance_msat = Some(&channel_details.balance_msat) >= filter.from_balance_msat.as_ref();
+
+            let is_to_balance_msat = filter.to_balance_msat.is_none()
+                || Some(&channel_details.balance_msat) <= filter.to_balance_msat.as_ref();
+
+            let is_from_outbound_capacity_msat =
+                Some(&channel_details.outbound_capacity_msat) >= filter.from_outbound_capacity_msat.as_ref();
+
+            let is_to_outbound_capacity_msat = filter.to_outbound_capacity_msat.is_none()
+                || Some(&channel_details.outbound_capacity_msat) <= filter.to_outbound_capacity_msat.as_ref();
+
+            let is_from_inbound_capacity_msat =
+                Some(&channel_details.inbound_capacity_msat) >= filter.from_inbound_capacity_msat.as_ref();
+
+            let is_to_inbound_capacity_msat = filter.to_inbound_capacity_msat.is_none()
+                || Some(&channel_details.inbound_capacity_msat) <= filter.to_inbound_capacity_msat.as_ref();
+
+            let is_confirmed = filter.is_ready.is_none() || Some(&channel_details.is_ready) == filter.is_ready.as_ref();
+
+            let is_usable = filter.is_usable.is_none() || Some(&channel_details.is_usable) == filter.is_usable.as_ref();
+
+            let is_public = filter.is_public.is_none() || Some(&channel_details.is_public) == filter.is_public.as_ref();
+
+            is_channel_id
+                && is_counterparty_node_id
+                && is_funding_tx
+                && is_from_funding_value_sats
+                && is_to_funding_value_sats
+                && is_outbound
+                && is_from_balance_msat
+                && is_to_balance_msat
+                && is_from_outbound_capacity_msat
+                && is_to_outbound_capacity_msat
+                && is_from_inbound_capacity_msat
+                && is_to_inbound_capacity_msat
+                && is_confirmed
+                && is_usable
+                && is_public


I believe we don't need to make this much checks. In order to return true, all of them must be true as well.

So,

// Checking if channel id is none or equal if !(filter.channel_id.is_none() || Some(&channel_details.channel_id) == filter.channel_id.as_ref()) { return false; } // Checking if counterparty_node_id is none or equal if !(filter.counterparty_node_id.is_none() || Some(&channel_details.counterparty_node_id) == filter.counterparty_node_id.as_ref()) { return false; } . . . // All checks true, so return: true

should be enough

onur-ozkan · 2022-10-17T11:12:03Z

mm2src/coins/lightning.rs

+        let duration = SystemTime::now()
+            .duration_since(SystemTime::UNIX_EPOCH)
+            .expect("for the foreseeable future this shouldn't happen");


Q:
Do we need to calculate duration in UTC or in local? If this should be global duration then we have to create Duration instance using get_utc_timestamp because device time can be incorrect.

Also, you can create this as a seperated function under common module to make it reusable. We have same calculation in fn ln_utils::init_keys_manager

Do we need to calculate duration in UTC or in local?

It's calculated the same way in create_invoice_from_channelmanager which is used for generating invoices for wallet payments/operations https://github.com/KomodoPlatform/atomicDEX-API/blob/24dd76a1312c062f1c6cba234be7a44910b029af/mm2src/coins/rpc_command/lightning/generate_invoice.rs#L88 https://github.com/lightningdevkit/rust-lightning/blob/e61f3a238a70cbac87209e223b7c396108a49b97/lightning-invoice/src/utils.rs#L229-L231
The expiry is checked by the receiver/payee which is the same node that generated the invoice so it should work alright with both local or UTC I think but I would like to keep it the same as create_invoice_from_channelmanager. I will add a Todo to check this in more details later.

Also, you can create this as a seperated function under common module to make it reusable. We have same calculation in fn ln_utils::init_keys_manager

Sure, do you think get_local_duration_since_epoch is a good name for such function?

The expiry is checked by the receiver/payee which is the same node that generated the invoice so it should work alright

Seems fine then. If this goes into different nodes with different time, this calculation won't be stable. But I guess it's not possible.

Sure, do you think get_local_duration_since_epoch is a good name for such function?

I think so, yeah.

onur-ozkan · 2022-10-17T11:31:05Z

mm2src/mm2_main/src/lp_swap/taker_swap.rs

+                        self.ctx.clone(),
+                        watcher_topic(&self.r().data.taker_coin),
+                        swpmsg_watcher,
+                        600.,


Can we create const for this? Just like MAKER_PAYMENT_WAIT_TIMEOUT

onur-ozkan · 2022-10-17T11:35:26Z

mm2src/mm2_main/src/lp_swap/maker_swap.rs

+        {
+            Ok(SwapTxDataMsg::V2(PaymentDataV2 {
+                data: payment_data,
+                next_step_instructions: instructions,
+            }))
+        } else {
+            Ok(SwapTxDataMsg::V1(payment_data))
+        }


This block is duplicated in taker_swap. It would be great if you can create helper function and use it here and in taker_swap. Or at least add TODO not on top of this function and also on taker_swap::get_taker_fee_data

onur-ozkan · 2022-10-17T11:38:55Z

mm2src/mm2_main/src/lp_swap.rs

+impl Default for SecretHashAlgo {
+    fn default() -> Self { SecretHashAlgo::DHASH160 }
+}


Why do we have this trait implemented? I see it's not used anywhere.

I guess it was used in a previous commit and I forgot to delete it.

onur-ozkan · 2022-10-17T12:04:18Z

mm2src/coins/rpc_command/lightning/update_channel.rs

+            UpdateChannelError::NoSuchChannel(_) => StatusCode::NOT_FOUND,
+            UpdateChannelError::NoSuchCoin(_) => StatusCode::NOT_FOUND,


We can combine these with |

caglaryucekaya

Great work! I have a few comments and questions

caglaryucekaya · 2022-10-18T09:31:15Z

mm2src/coins/lightning.rs

@@ -171,11 +207,11 @@ impl LightningCoin {
            selfi
                .invoice_payer
                .pay_invoice(&invoice)
-                .map_to_mm(|e| SendPaymentError::PaymentError(format!("{:?}", e)))
+                .map_err(|e| PaymentError::Invoice(format!("{:?}", e)))


Can you use map_to_mm here?

Yes, this should return MmError similar to keysend. Will fix it.

caglaryucekaya · 2022-10-18T11:20:33Z

mm2src/coins/lightning.rs

+                ));
+        }
+
+        let route_hints = filter_channels(self.channel_manager.list_usable_channels(), amt_msat);


You can move this to where it is used below to make the code more clear

mm2src/mm2_main/src/lp_swap.rs

caglaryucekaya · 2022-10-19T00:35:43Z

mm2src/coins/lightning.rs

        _swap_contract_address: &Option<BytesJson>,
        _swap_unique_data: &[u8],
    ) -> TransactionFut {
-        unimplemented!()
+        let payment_hash = try_tx_fus!(payment_hash_from_slice(taker_payment_tx));


At first sight this looks like you're taking the hash of the lightning payment bytes taker_payment_tx, and that this is something different than the regular payment_hash i.e. hash of the payment_preimage. This confused me for a while. Can you change the parameter name taker_payment_tx to payment_hash? Also in other SwapOps methods.

Can you change the parameter name taker_payment_tx to payment_hash?

This can't be changed since in other SwapOps implementations the taker_payment_tx bytes are what's passed. There is no concept of payment bytes in lightning, this comment can be part of #1501

caglaryucekaya · 2022-10-19T00:49:45Z

mm2src/mm2_main/src/lp_swap/taker_swap.rs

-                        &self.p2p_privkey,
+        // Watchers cannot be used for lightning swaps for now
+        // Todo: Check if watchers can work in some cases with lightning and implement it if it's possible, the watcher will not be able to retrieve the preimage since it's retrieved through the lightning network
+        // Todo: The watcher can retrieve the preimage only if he is running a lightning node and is part of the nodes that routed the taker payment which is a very low probability event that shouldn't be considered


Good point, I will add this to todo list for watchers and think if there's a solution

I am not sure if watchers are really needed for lightning anyways since the maker should spend the taker payment straight away and propagate back the preimage. Just added this note to think about the possibilities of a malicious maker that doesn't spend the taker payment straight away and the taker goes offline, maybe combining the lightning channels watchers in the future with the swap watchers can offer a solution, the watcher can close the channel (broadcasting the latest commitment transaction) if the taker went offline without completing the swap, this will be equivalent to a taker refund but the taker will get the refund on-chain.

sergeyboyko0791

I dug deeper into the changes a little more. Here are my comments and suggestions 🙂

mm2src/coins/lightning.rs

sergeyboyko0791 · 2022-10-19T10:43:40Z

mm2src/coins/lightning.rs

+            .ok_or_else(|| TransactionErr::Plain("payment_instructions can't be None".into())));
+        let coin = self.clone();
+        let fut = async move {
+            let payment = try_tx_s!(coin.pay_invoice(invoice).await);


How about using .validate_instructions() at send_maker_payment and send_taker_payment methods instead of in maker_swap.rs and taker_swap.rs? It confuses a bit that send_maker_payment doesn't validate the input instruction.
This is not a call to the action, but a topic for discussion

How about using .validate_instructions() at send_maker_payment and send_taker_payment methods instead of in maker_swap.rs and taker_swap.rs?

I wanted the swap to fail quickly if the instructions are invalid. let's say the maker sends an invalid instructions, if validation is moved right before or inside send_taker_payment then the taker will have to wait for the confirmations of the maker payment first before failing the swap due to invalid instructions. I think the instructions should be checked if valid or not as soon as it's received in the p2p message as currently implemented :)

sergeyboyko0791 · 2022-10-19T10:48:58Z

mm2src/coins/lightning.rs

-    fn validate_taker_payment(&self, _input: ValidatePaymentInput) -> ValidatePaymentFut<()> { unimplemented!() }
+    fn validate_taker_payment(&self, input: ValidatePaymentInput) -> ValidatePaymentFut<()> {
+        let payment_hash = try_f!(payment_hash_from_slice(&input.payment_tx)
+            .map_to_mm(|e| ValidatePaymentError::TxDeserializationError(e.to_string())));


Do we need to do the following if there is a problem with the payment_tx?

// Free the htlc to allow for this inbound liquidity to be used for other inbound payments coin.channel_manager.fail_htlc_backwards(&payment_hash); payment.status = HTLCStatus::Failed; drop_mutability!(payment); coin.db .add_or_update_payment_in_db(payment) .await .error_log_with_msg("Unable to update payment information in DB!"); return MmError::err(ValidatePaymentError::WrongPaymentTx(format!( "Provided payment {} amount {:?} doesn't match required amount {}", payment_hex, amount_sent, amt_msat )));

Probably there should be a callback called from maker_swap and taker_swap that the swap is failed, and lightning will free the corresponding htlc

Probably there should be a callback called from maker_swap and taker_swap that the swap is failed, and lightning will free the corresponding htlc

I don't get what you mean by a callback? isn't the error returned in the below code enough?
https://github.com/KomodoPlatform/atomicDEX-API/blob/8b515c860faf640e04438c694561bdc8b9093ee4/mm2src/mm2_main/src/lp_swap/maker_swap.rs#L918-L925

Maybe I can edit the error message to include that lightning will free the corresponding htlc too. What do you think @sergeyboyko0791?

I meant that the following code

// Free the htlc to allow for this inbound liquidity to be used for other inbound payments coin.channel_manager.fail_htlc_backwards(&payment_hash); payment.status = HTLCStatus::Failed; drop_mutability!(payment); coin.db .add_or_update_payment_in_db(payment) .await .error_log_with_msg("Unable to update payment information in DB!"); return MmError::err(ValidatePaymentError::WrongPaymentTx(format!( "Provided payment {} amount {:?} doesn't match required amount {}", payment_hex, amount_sent, amt_msat )));

might be used on any error occurred at the validate_taker_payment, send_maker_payment etc as it changes the state of the payment to HTLCStatus::Failed. Does this even make sense?

Does this even make sense?

Yes, I understand now, thanks for the clarification :)
Let's make this part of the next sprint since I need to think about it in more details. I will add this comment to the lightning checklist.

mm2src/coins/lightning.rs

mm2src/mm2_main/src/lp_swap/maker_swap.rs

sergeyboyko0791 · 2022-10-19T11:37:15Z

mm2src/mm2_main/src/lp_swap/maker_swap.rs

        let abort_send_handle =
            broadcast_swap_message_every(self.ctx.clone(), swap_topic(&self.uuid), msg, 600., self.p2p_privkey);

        let maker_payment_wait_confirm = self.r().data.started_at + (self.r().data.lock_duration * 2) / 5;
        let f = self.maker_coin.wait_for_confirmations(
-            &self.r().maker_payment.clone().unwrap().tx_hex,
+            &self.r().maker_payment.clone().unwrap().tx_hex_or_hash(),


To be honest, this looks really weird. What if TransactionIdentifier::tx_hex will be not None, but the same value as TransactionIdentifier::tx_hash for lightning? Then we can pass TransactionIdentifier::tx_hex as it is here and in other places.

Making TransactionIdentifier::tx_hex optional also leads to huge changes in taker_swap.rs

What if TransactionIdentifier::tx_hex will be not None, but the same value as TransactionIdentifier::tx_hash for lightning?

I actually implemented it like this in the beginning but thought it might be confusing. Also these checks https://github.com/KomodoPlatform/atomicDEX-API/blob/8b515c860faf640e04438c694561bdc8b9093ee4/mm2src/mm2_main/src/lp_swap/taker_swap.rs#L1377-L1378
https://github.com/KomodoPlatform/atomicDEX-API/blob/8b515c860faf640e04438c694561bdc8b9093ee4/mm2src/mm2_main/src/lp_swap/taker_swap.rs#L1419 will be more confusing since we will check if tx_hash != tx_hex. What do you think?

Where is it required to check if tx_hash != tx_hex?

Where is it required to check if tx_hash != tx_hex?

It will be used instead of checking if tx_hex is some since it's not an Option anymore

Maybe a method can be added to SwapOps called is_watcher_supported instead of doing these checks, what do you think @caglaryucekaya @sergeyboyko0791?

I like this idea

Maybe a method can be added to SwapOps called is_watcher_supported instead of doing these checks, what do you think @caglaryucekaya @sergeyboyko0791?

Will implement it then. @caglaryucekaya I guess is_watcher_supported should be true for UTXO and QTUM only, is that correct?

Yes correct, will start to implement the rest in the next sprint.

mm2src/mm2_main/src/lp_swap/taker_swap.rs

mm2src/mm2_main/src/rpc/dispatcher/dispatcher.rs

onur-ozkan

Thank you for the fixes 🔥
Last review iteration.

mm2src/mm2_main/src/lp_swap/taker_swap.rs

onur-ozkan · 2022-10-19T14:44:17Z

mm2src/mm2_main/src/lp_swap/taker_swap.rs

+        let maybe_maker_hex = self.r().maker_payment.as_ref().unwrap().tx_hex.clone();
+        if let Some(maker_hex) = maybe_maker_hex {
+            if let Some(taker_hex) = tx_hex {
+                if self.ctx.use_watchers() {


I achieved to write this functionality recuding 1 nested block. No mandatory change required. Using this is up to you 🙂

diff --git a/mm2src/mm2_main/src/lp_swap/taker_swap.rs b/mm2src/mm2_main/src/lp_swap/taker_swap.rs index e56aabd82..ae6e3700d 100644 --- a/mm2src/mm2_main/src/lp_swap/taker_swap.rs +++ b/mm2src/mm2_main/src/lp_swap/taker_swap.rs @@ -1374,38 +1374,41 @@ impl TakerSwap { // Todo: Check if watchers can work in some cases with lightning and implement it if it's possible, the watcher will not be able to retrieve the preimage since it's retrieved through the lightning network // Todo: The watcher can retrieve the preimage only if he is running a lightning node and is part of the nodes that routed the taker payment which is a very low probability event that shouldn't be considered let maybe_maker_hex = self.r().maker_payment.as_ref().unwrap().tx_hex.clone(); - if let Some(maker_hex) = maybe_maker_hex { - if let Some(taker_hex) = tx_hex { - if self.ctx.use_watchers() { - let preimage_fut = self.taker_coin.create_taker_spends_maker_payment_preimage( - &maker_hex, - self.maker_payment_lock.load(Ordering::Relaxed) as u32, - self.r().other_maker_coin_htlc_pub.as_slice(), - &self.r().secret_hash.0, - &self.unique_swap_data()[..], - ); - match preimage_fut.compat().await.map(|p| p.tx_hex()) { - Ok(Some(preimage)) => { - let watcher_data = self.create_watcher_data(taker_hex.0, preimage.clone()); - let swpmsg_watcher = SwapWatcherMsg::TakerSwapWatcherMsg(Box::new(watcher_data)); - broadcast_swap_message( - &self.ctx, - watcher_topic(&self.r().data.taker_coin), - swpmsg_watcher, - &self.p2p_privkey, - ); - swap_events.push(TakerSwapEvent::WatcherMessageSent(Some(preimage))) - }, - Ok(None) => (), - Err(e) => error!( + match maybe_maker_hex { + Some(maker_hex) if self.ctx.use_watchers() && tx_hex.is_some() => { + let taker_hex = tx_hex.unwrap(); + + let preimage_fut = self.taker_coin.create_taker_spends_maker_payment_preimage( + &maker_hex, + self.maker_payment_lock.load(Ordering::Relaxed) as u32, + self.r().other_maker_coin_htlc_pub.as_slice(), + &self.r().secret_hash.0, + &self.unique_swap_data()[..], + ); + + match preimage_fut.compat().await.map(|p| p.tx_hex()) { + Ok(Some(preimage)) => { + let watcher_data = self.create_watcher_data(taker_hex.0, preimage.clone()); + let swpmsg_watcher = SwapWatcherMsg::TakerSwapWatcherMsg(Box::new(watcher_data)); + broadcast_swap_message( + &self.ctx, + watcher_topic(&self.r().data.taker_coin), + swpmsg_watcher, + &self.p2p_privkey, + ); + swap_events.push(TakerSwapEvent::WatcherMessageSent(Some(preimage))) + }, + Ok(None) => (), + Err(e) => error!( "The watcher message could not be sent, error creating the taker spends maker payment preimage: {}", e.get_plain_text_format() ), - } } - } - } + }, + _ => {}, + }; + Ok((Some(TakerSwapCommand::WaitForTakerPaymentSpend), swap_events)) }

looks good, thanks for the suggestion :). But this will be changed due to this #1497 (comment).

onur-ozkan · 2022-10-19T14:49:22Z

mm2src/mm2_main/src/lp_swap/maker_swap.rs

+        // If maker payment is a lightning payment the payment hash will be sent in the message
+        // It's not really needed here unlike in TakerFee msg since the hash is included in the invoice/payment_instructions but it's kept for symmetry
+        let payment_data = self.r().maker_payment.as_ref().unwrap().tx_hex_or_hash().0;
+        let hash_algo = SecretHashAlgo::SHA256;


The value itself(SecretHashAlgo::SHA256) is explaining everything. I think we can freely remove the extra allocation here.

onur-ozkan · 2022-10-19T14:51:29Z

mm2src/mm2_main/src/lp_swap.rs

+        // old message format should be deserialized to PaymentDataMsg::V1
+        let old = SwapMsgOld::MakerPayment(vec![1; 300]);
+
+        let expected = SwapMsg::MakerPayment(SwapTxDataMsg::V1(vec![1; 300]));
+
+        let serialized = rmp_serde::to_vec(&old).unwrap();
+
+        let deserialized: SwapMsg = rmp_serde::from_read_ref(serialized.as_slice()).unwrap();
+
+        assert_eq!(deserialized, expected);
+
+        // PaymentDataMsg::V1 should be deserialized to old message format
+        let v1 = SwapMsg::MakerPayment(SwapTxDataMsg::V1(vec![1; 300]));
+
+        let expected = old;
+
+        let serialized = rmp_serde::to_vec(&v1).unwrap();
+
+        let deserialized: SwapMsgOld = rmp_serde::from_read_ref(serialized.as_slice()).unwrap();
+
+        assert_eq!(deserialized, expected);
+
+        // PaymentDataMsg::V1 should be deserialized to PaymentDataMsg::V1
+        let v1 = SwapMsg::MakerPayment(SwapTxDataMsg::V1(vec![1; 300]));
+
+        let serialized = rmp_serde::to_vec(&v1).unwrap();
+
+        let deserialized: SwapMsg = rmp_serde::from_read_ref(serialized.as_slice()).unwrap();
+
+        assert_eq!(deserialized, v1);
+
+        // PaymentDataMsg::V2 should be deserialized to PaymentDataMsg::V2
+        let v2 = SwapMsg::MakerPayment(SwapTxDataMsg::V2(PaymentDataV2 {
+            data: vec![1; 300],
+            next_step_instructions: vec![1; 300],
+        }));
+
+        let serialized = rmp_serde::to_vec(&v2).unwrap();
+
+        let deserialized: SwapMsg = rmp_serde::from_read_ref(serialized.as_slice()).unwrap();
+
+        assert_eq!(deserialized, v2);
+
+        // PaymentDataMsg::V2 shouldn't be deserialized to old message format, new nodes with payment instructions can't swap with old nodes without it.
+        let v2 = SwapMsg::MakerPayment(SwapTxDataMsg::V2(PaymentDataV2 {
+            data: vec![1; 300],
+            next_step_instructions: vec![1; 300],


I guess we can create in-function const for vec![1; 300] if I am not mistaken?

vec! can't be used with constants. Created a const array instead.

onur-ozkan · 2022-10-19T14:52:54Z

mm2src/common/common.rs

@@ -937,3 +938,7 @@ impl<Id> Default for PagingOptionsEnum<Id> {

 #[inline(always)]
 pub fn get_utc_timestamp() -> i64 { Utc::now().timestamp() }
+
+pub fn get_local_duration_since_epoch() -> Result<Duration, SystemTimeError> {


Could you please inline this function?

ps: You can even provide this funcitonality as a macro.

onur-ozkan · 2022-10-19T14:58:43Z

mm2src/coins/lightning/ln_utils.rs

+        match filtered_channels.entry(channel.counterparty.node_id) {
+            Entry::Occupied(mut entry) => {
+                let current_max_capacity = entry.get().inbound_capacity_msat;
+                if channel.inbound_capacity_msat < current_max_capacity {
+                    continue;
+                }
+                entry.insert(channel);
+            },
+            Entry::Vacant(entry) => {
+                entry.insert(channel);
+            },


What about:

match filtered_channels.entry(channel.counterparty.node_id) { Entry::Occupied(entry) if channel.inbound_capacity_msat < entry.get().inbound_capacity_msat => continue, Entry::Occupied(mut entry) => entry.insert(channel), Entry::Vacant(entry) => entry.insert(channel), };

I tried to seperate different logics into different blocks.

You can also add notes on top of each pattern which will look easier to debug.

laruh

Impressive work as always!
I just have a tiny note for now.

mm2src/coins/rpc_command/lightning/connect_to_node.rs

… None, some refactors

…d is_supported_by_watchers method to SwapOps

shamardy · 2022-10-24T15:28:09Z

@sergeyboyko0791 @ozkanonur @caglaryucekaya @laruh All comments are fixed or replied to

onur-ozkan

thank you for the fixes :) LGTM

laruh

Looks good to me, thanks! 🙂

sergeyboyko0791

Awesome 🔥

artemii235

Few notes for the next iteration.

artemii235 · 2022-10-27T06:31:12Z

mm2src/coins/lightning/ln_events.rs

-                    // Free the htlc immediately if we don't have the preimage required to claim the payment
-                    // to allow for this inbound liquidity to be used for other inbound payments.
-                    self.channel_manager.fail_htlc_backwards(payment_hash);
+                    let payment_info = PaymentInfo {


Is it generally safe to not call fail_htlc_backwards here anymore? What will happen if neither fail_htlc_backwards nor claim_funds is called from the API code afterward?

Is it generally safe to not call fail_htlc_backwards here anymore?

Yes, I should call it inside the swap methods implementations when it's required. I called it here previously because before swaps we shouldn't have accepted payments that we don't know the preimage for.

What will happen if neither fail_htlc_backwards nor claim_funds is called from the API code afterward?

The HTLC will remain unsettled until the timelock expires in both cases.

artemii235 · 2022-10-27T06:33:34Z

mm2src/coins/lightning.rs

+                    // Todo: Add more validations if needed, locktime is probably the most important
+                    if amount_sent != Some(amt_msat as i64) {
+                        // Free the htlc to allow for this inbound liquidity to be used for other inbound payments
+                        coin.channel_manager.fail_htlc_backwards(&payment_hash);


I assume there might be other places to call fail_htlc_backwards in case of other swap steps failures.

Already added to checklist #1497 (comment) :)

artemii235 · 2022-10-27T06:34:16Z

mm2src/coins/lightning.rs

+        let payment_hash = payment_hash_from_slice(spend_tx).map_err(|e| e.to_string())?;
+        let payment_hex = hex::encode(payment_hash.0);
+
+        return match self.db.get_payment_from_db(payment_hash).await {


return is not required here

shamardy added 30 commits September 15, 2022 20:52

fix some ignored tests

94e6df2

move connect_to_lightning_node to rpc_command

92bd009

move open_channel to rpc_command

e18b4aa

move update_channel to rpc_command

a16cbbe

move list channels rpcs to rpc_command

df6ae9b

move get_channel_details, get_claimable_balances, trusted nodes rpcs …

48d748e

…to rpc_command

move close_channel to rpc_command

26806be

move get_payment_details, list_payments_by_filter to rpc_command

3732050

move send_payment to rpc_command

2322b25

move the rest of lightning RPCs to rpc_command, use lightning:: prefi…

b638bb7

…x for lightning methods

edit SwapMsg MakerPayment and TakerPayment to allow sending payment i…

abfb9a8

…nstructions to the other side

create_invoice_for_hash fn, impl other_side_instructions for Lightnin…

d93b834

…gCoin, copy needed parts of SecretHashAlgo from iris-swap-poc branch

check connection to nodes before generating route hints, refactors

62d5a9a

fix TakerFee msg to include PaymentDataMsg (lightning invoice to be p…

3bb77df

…aid by maker to taker) instead of TakerPayment msg

Merge remote-tracking branch 'origin/dev' into lightning-swaps-poc

5f2b459

Validate invoice received from swap messages wip

a436358

Validate invoice on maker side if maker is lightning coin

a7af8c4

Merge remote-tracking branch 'origin/dev' into lightning-swaps-poc

ba8dfc8

add optional PaymentInstructions parameter to send_maker_payment, sen…

9e67c41

…d_taker_payment functions

implement send_taker_payment and send_maker_payment for LightningCoin

2a94c23

change tx_enum_from_bytes to return none for lightning coin

ac6b7a9

Some refactors and rollbacks

e8ce302

impl more swap methods, do a unit test swap where taker is lightning

474b0a6

refactor some code by adding payment_hash_from_slice fn

2f99e1b

minor fixes and added more todo for next PR/s

38e9049

refactor lightning taker swap test

03aa40d

more refactors, broadcast_p2p_tx_msg shouldn't work with lightning pa…

9ccada1

…yments, better todo comments

add preimage of swap payment to DB after the secret is revealed (clai…

f108dfc

…ming funds)

wip

6d622f6

Final refactors

a4e970c

onur-ozkan requested changes Oct 17, 2022

View reviewed changes

shamardy changed the title ~~[r2r] Lightning taker swap p.o.c~~ [wip] Lightning taker swap p.o.c Oct 18, 2022

shamardy added 2 commits October 18, 2022 18:21

more review fixes

c49f7ea

Merge remote-tracking branch 'origin/dev' into lightning-swaps-poc

dc4b6ca

shamardy changed the title ~~[wip] Lightning taker swap p.o.c~~ [r2r] Lightning taker swap p.o.c Oct 18, 2022

caglaryucekaya reviewed Oct 19, 2022

View reviewed changes

sergeyboyko0791 suggested changes Oct 19, 2022

View reviewed changes

onur-ozkan requested changes Oct 19, 2022

View reviewed changes

shamardy changed the title ~~[r2r] Lightning taker swap p.o.c~~ [wip] Lightning taker swap p.o.c Oct 24, 2022

review fixes wip: rename SwapTxDataMsg enum variants

8b515c8

shamardy mentioned this pull request Oct 24, 2022

Lightning integration #1045

Open

24 tasks

laruh reviewed Oct 24, 2022

View reviewed changes

mm2src/coins/rpc_command/lightning/connect_to_node.rs Outdated Show resolved Hide resolved

shamardy added 2 commits October 24, 2022 16:00

review fixes wip: validate_instructions to return an error instead of…

68765b8

… None, some refactors

Review fixes: make TransactionIdentifier::tx_hex not an Option and ad…

2113b7a

…d is_supported_by_watchers method to SwapOps

shamardy changed the title ~~[wip] Lightning taker swap p.o.c~~ [r2r] Lightning taker swap p.o.c Oct 24, 2022

onur-ozkan previously approved these changes Oct 25, 2022

View reviewed changes

laruh previously approved these changes Oct 25, 2022

View reviewed changes

sergeyboyko0791 previously approved these changes Oct 25, 2022

View reviewed changes

Merge remote-tracking branch 'origin/dev' into lightning-swaps-poc

ab4ab00

shamardy dismissed stale reviews from sergeyboyko0791, laruh, and onur-ozkan via ab4ab00 October 26, 2022 09:33

shamardy added 2 commits October 26, 2022 15:12

Merge remote-tracking branch 'origin/dev' into lightning-swaps-poc

0f53219

re-add Todo that was removed by merge

bea9173

artemii235 approved these changes Oct 27, 2022

View reviewed changes

artemii235 merged commit 6fc74b9 into dev Oct 27, 2022

artemii235 deleted the lightning-swaps-poc branch October 27, 2022 06:37

shamardy mentioned this pull request Nov 29, 2022

[r2r] Lightning Swaps Locktimes + Lightning Maker Swap #1557

Merged

shamardy mentioned this pull request Feb 17, 2023

[release] 1.0.0-beta #1576

Merged

		UpdateChannelError::NoSuchChannel(_) => StatusCode::NOT_FOUND,
		UpdateChannelError::NoSuchCoin(_) => StatusCode::NOT_FOUND,

[r2r] Lightning taker swap p.o.c #1497

[r2r] Lightning taker swap p.o.c #1497

Conversation

shamardy commented Oct 11, 2022

onur-ozkan 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

caglaryucekaya left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shamardy Oct 24, 2022 • edited Loading

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

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

shamardy Oct 24, 2022 • edited Loading

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 Oct 24, 2022 • edited Loading

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

onur-ozkan 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

laruh left a comment

Choose a reason for hiding this comment

shamardy commented Oct 24, 2022

onur-ozkan left a comment

Choose a reason for hiding this comment

laruh 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

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 Oct 24, 2022 •

edited

Loading

shamardy Oct 24, 2022 •

edited

Loading

shamardy Oct 24, 2022 •

edited

Loading