fix(legacy-swap): wait for confirmation of taker spend maker payment #2206

laruh · 2024-08-29T08:22:38Z

fixes this issue #2175 (comment)
Currently in legacy swap protocol we dont check transaction "taker spend maker payment" confirmation and just mark Swap on taker side as successfully finished, while on maker side we have taker payment spend confirmation check.

Current PR is a solution approach suggested here #2199 (comment)

… method in taker_swap.rs

laruh · 2024-08-29T08:28:17Z

@cipig is it possible for you to test recreate the issue and test the solution?

cipig

taker now waits for one confirmation on makerpaymentspend
if EVM tx is reverted (out of gas), swap is marked as failed and recoverable
if you click on "Recover Funds" in Desktop app, it shows success in spending maker payment, even though that tx is reverted too (still out of gas)... this is fine, else we would need to wait here too for a confirmation before showing a result, which can take some time on slow UTXO chains
after fixing the problem (increased gas limit) and clicking on "Recover Funds" taker successfully spends maker payment

laruh · 2024-08-30T08:37:40Z

if you click on "Recover Funds" in Desktop app, it shows success in spending maker payment, even though that tx is reverted too (still out of gas)

The thing is that legacy taker swap doesnt have spending maker payment confirmed topic, as maker. But it is correct to see maker payment spend tx was successful, as we actually successfully broadcasted tx to blockchain.

On maker I believe GUI follows backend events: spend taker payment success-> spend taker payment confirmed or failed.

we would need to wait here too for a confirmation before showing a result, which can take some time on slow UTXO chains

On utxo case it will be same, if taker tx spend maker payment was broadcasted, it will be marked as successful. But then taker will have to wait for this tx confirmation, just couldn't see confirmation started->confirmed, as we dont have it on taker. And yes it could take some time to see Finished.

shamardy · 2024-09-05T01:03:47Z

I suppose this also fixes the below issue reported by @cipig on DM

i had a swap that shows up as successful on both maker and taker, but the makerpaymentspend tx is nowhere to be found:

     {
        "event" : {
           "data" : {
              "tx_hash" : "afd0259281f374d4eae427cef11f29ed6b587cc79d93f8eeee92351370ffe79a",
              "tx_hex" : "01f9016b81898218008506fc23ac1b83014c08949130b257d37a52e52f21054c4da3450c72f595ce80b8a402ed292bb525820e3d1fb63302da962154d96091447add2ec0b21c1a5c47b667126b9e79000000000000000000000000000000000000000000000000609fcf31656c00000b8f2c9a27f2dc7c80b5c5af45be319d5b5fb507ad418b5f9a558ba51ac512ea000000000000000000000000e5417af564e4bfda1c483642db72007871397896000000000000000000000000df38dd108bab50da564092ad0cd739c4634d963cf85bf85994e5417af564e4bfda1c483642db72007871397896f842a041587256da2d7733d0860c4b69f4f40974a681485f4398a35b4f3cfff44f9abca0b899ff6e1bfd9838a15242d3f328080cc178c6f40679e41fae6092c1ddeeca5580a0eaeabf5991c1e83822b11a9b6053497a93196aa243ff24e3d334513c4ba35e99a07de06f821b89efeea1fa3037828ac43738f890378b9f668b04f67252ef8efae1"
           },
           "type" : "MakerPaymentSpent"
        },
        "timestamp" : 1724170849368
     },

the above tx does not exist on chain: https://polygonscan.com/tx/0xafd0259281f374d4eae427cef11f29ed6b587cc79d93f8eeee92351370ffe79a
unfortunately mm2 log does not contain any error either, just:

20 16:20:49, mm2_main::lp_swap::taker_swap:1869] INFO Maker payment spend tx afd0259281f374d4eae427cef11f29ed6b587cc79d93f8eeee92351370ffe79a
· 2024-08-20 16:20:49 +0000 [swap uuid=b9c6df00-3b58-449e-95b0-01abc8333a31] Finished

The reason was that the RPC probably didn't broadcast the transaction to the network, this fixes such issues as taker will now wait for the transaction that spends the maker payment to confirm.

shamardy

Thanks for the fix! 2 minor comments :)

shamardy · 2024-09-05T00:51:07Z

mm2src/mm2_main/src/lp_swap/taker_swap.rs

+        let confirm_maker_payment_spend_input = ConfirmPaymentInput {
+            payment_tx: transaction.tx_hex(),
+            confirmations,
+            requires_nota: false,


Shouldn't we use maker_payment_requires_nota here with unwrap_or(false)?

yep, thanks for the catch
done 9827b54

shamardy · 2024-09-05T01:05:29Z

mm2src/mm2_main/src/lp_swap/taker_swap.rs

+                ),
+            ]));
+        }
+        info!("Maker payment spend confirmed");


You can add the number of confirmations in the logs too, might be useful who knows.

shamardy · 2024-09-05T03:48:36Z

@laruh one problem with adding confirmation to SpendMakerPayment step is that GUI will not have the tx hash to show to user until tx is confirmed, users like to check tx on chain while it's awaiting confirmations (I do it myself). I understand the problem with backward compatibility but I added steps before to the events (TakerPaymentInstructionsReceived for instance) without backward compatibility issues, so we should give it a try. By adding confirmation steps, users can also know when they can spend the received funds as not all chains require confirmations to spend the received funds and takers can spend them once SpendMakerPayment is fired and the balance appears.

if you click on "Recover Funds" in Desktop app, it shows success in spending maker payment, even though that tx is reverted too (still out of gas)... this is fine, else we would need to wait here too for a confirmation before showing a result, which can take some time on slow UTXO chains

About this, I think we should at least check that transaction is not reverted (for EVM only, so this should not be done in swaps code), this will not take time and doesn't require waiting for confirmations.

For TPU/Swaps v2, I guess you will fix this in another PR, right? You can fix it here as well as it's not related to ETH TPU. This is where we need to check for confirmations instead of changing state to completed straight away.

komodo-defi-framework/mm2src/mm2_main/src/lp_swap/taker_swap_v2.rs

Lines 2189 to 2198 in 7723b50

    
           #[async_trait] 
        
           impl<MakerCoin: MmCoin + MakerCoinSwapOpsV2, TakerCoin: MmCoin + TakerCoinSwapOpsV2> State 
        
               for MakerPaymentSpent<MakerCoin, TakerCoin> 
        
           { 
        
               type StateMachine = TakerSwapStateMachine<MakerCoin, TakerCoin>; 
        
               async fn on_changed(self: Box<Self>, state_machine: &mut Self::StateMachine) -> StateResult<Self::StateMachine> { 
        
                   Self::change_state(Completed::new(), state_machine).await 
        
               } 
        
           }

laruh · 2024-09-05T05:45:49Z

@shamardy thanks for the feedback

I understand the problem with backward compatibility but I added steps before to the events (TakerPaymentInstructionsReceived for instance) without backward compatibility issues, so we should give it a try. By adding confirmation steps, users can also know when they can spend the received funds as not all chains require confirmations to spend the received funds and takers can spend them once SpendMakerPayment is fired and the balance appears.

Do you mean we should come back to this approach https://github.com/KomodoPlatform/komodo-defi-framework/pull/2199/files with additional "MakerPaymentSpendConfirmed" event?

For TPU/Swaps v2, I guess you will fix this in another PR, right? You can fix it here as well as it's not related to ETH TPU. This is where we need to check for confirmations instead of changing state to completed straight away.

I actually wanted to add new state with new wait_for_maker_payment_spend function or integrate this method to the currently existed swap step.

In TPUV2 there is the same issue as in legacy protocol, that we have step to confirm that taker payment was spent (wait_for_taker_payment_spend)

komodo-defi-framework/mm2src/mm2_main/src/lp_swap/taker_swap_v2.rs

Lines 1686 to 1691 in f73b5de

    
           let taker_payment_spend = match state_machine 
        
               .taker_coin 
        
               .wait_for_taker_payment_spend( 
        
                   &self.taker_payment, 
        
                   self.taker_coin_start_block, 
        
                   state_machine.taker_payment_locktime(),

But we dont wait for maker payment was spend. So added wait_for_maker_payment_spend in maker swap ops trait
https://github.com/KomodoPlatform/komodo-defi-framework/pull/2211/files#diff-21afdd8d37a9aa80dbf375468ad6f5f494436c419d27b5ff443425dfba5f10faR1810-R1816

Note: I didnt add it to swap v2 states yet.

I suppose we could call wait_for_maker_payment_spend here.

komodo-defi-framework/mm2src/mm2_main/src/lp_swap/taker_swap_v2.rs

Lines 2189 to 2198 in 7723b50

    
           #[async_trait] 
        
           impl<MakerCoin: MmCoin + MakerCoinSwapOpsV2, TakerCoin: MmCoin + TakerCoinSwapOpsV2> State 
        
               for MakerPaymentSpent<MakerCoin, TakerCoin> 
        
           { 
        
               type StateMachine = TakerSwapStateMachine<MakerCoin, TakerCoin>; 
        
               async fn on_changed(self: Box<Self>, state_machine: &mut Self::StateMachine) -> StateResult<Self::StateMachine> { 
        
                   Self::change_state(Completed::new(), state_machine).await 
        
               } 
        
           }

But on the one hand its a bit awkward, that we will have both wait_for_taker_payment_spend and wait_for_maker_payment_spend in taker_swap_v2.rs. On the other hand, maker (maker_swap_v2.rs) finish their swap steps earlier than taker, so we have to wait for maker payment spend in taker_swap_v2.rs

This is where we need to check for confirmations instead of changing state to completed straight away.

Do you really want to call wait_for_confirmations on MakerPaymentSpent in v2 swap? I noticed that in TPU V2 protocol when we wait for taker payment spend we use a bit different approach from legacy wait_for_confirmations function in UTXO and ETH. Thats why I decided we could do the same in wait_for_maker_payment_spend implementation. I mean we can check not actually confirmations but outputs for UTXO coins and Payment State for ETH.

laruh · 2024-09-05T05:51:07Z

if you click on "Recover Funds" in Desktop app, it shows success in spending maker payment, even though that tx is reverted too (still out of gas)... this is fine, else we would need to wait here too for a confirmation before showing a result, which can take some time on slow UTXO chains

About this, I think we should at least check that transaction is not reverted (for EVM only, so this should not be done in swaps code), this will not take time and doesn't require waiting for confirmations.

@shamardy but you can check was it reverted or not during waiting for confirmations. You broadcast transaction to spend payment, if it was successfully broadcasted you will see log info info!("Maker payment spend tx {:02x}", tx_hash);

shamardy · 2024-09-06T03:38:07Z

I guess questions here #2206 (comment) have been resolved in DM.

About this #2206 (comment)

but you can check was it reverted or not during waiting for confirmations. You broadcast transaction to spend payment, if it was successfully broadcasted you will see log info info!("Maker payment spend tx {:02x}", tx_hash);

Original comment from cipi was about recover funds, which is trying another transaction where we also don't check if it was reverted or not. It's a different case and can be worked on in another PR but this issue #2175 (comment) shouldn't be closed until it's resolved.

Code reference:

komodo-defi-framework/mm2src/mm2_main/src/lp_swap/taker_swap.rs

Lines 2177 to 2212 in 17a5fab

    
               let maybe_spend_tx = self 
        
                   .maker_coin 
        
                   .send_taker_spends_maker_payment(SpendPaymentArgs { 
        
                       other_payment_tx: &maker_payment, 
        
                       time_lock: self.maker_payment_lock.load(Ordering::Relaxed), 
        
                       other_pubkey: other_maker_coin_htlc_pub.as_slice(), 
        
                       secret: &secret, 
        
                       secret_hash: &secret_hash, 
        
                       swap_contract_address: &maker_coin_swap_contract_address, 
        
                       swap_unique_data: &unique_data, 
        
                       watcher_reward, 
        
                   }) 
        
                   .await; 
        
               let transaction = match maybe_spend_tx { 
        
                   Ok(t) => t, 
        
                   Err(err) => { 
        
                       if let Some(tx) = err.get_tx() { 
        
                           broadcast_p2p_tx_msg( 
        
                               &self.ctx, 
        
                               tx_helper_topic(self.maker_coin.ticker()), 
        
                               &tx, 
        
                               &self.p2p_privkey, 
        
                           ); 
        
                       } 
        
                       return ERR!("{}", err.get_plain_text_format()); 
        
                   }, 
        
               }; 
        
               return Ok(RecoveredSwap { 
        
                   action: RecoveredSwapAction::SpentOtherPayment, 
        
                   coin: self.maker_coin.ticker().to_string(), 
        
                   transaction, 
        
               }); 
        
           }

komodo-defi-framework/mm2src/mm2_main/src/lp_swap/taker_swap.rs

Lines 2248 to 2251 in 17a5fab

    
           let maybe_spend_tx = self 
        
               .maker_coin 
        
               .send_taker_spends_maker_payment(taker_spends_payment_args) 
        
               .await;

komodo-defi-framework/mm2src/mm2_main/src/lp_swap/taker_swap.rs

Lines 2290 to 2300 in 17a5fab

    
           let fut = self.taker_coin.send_taker_refunds_payment(RefundPaymentArgs { 
        
               payment_tx: &taker_payment, 
        
               time_lock: taker_payment_lock, 
        
               other_pubkey: other_taker_coin_htlc_pub.as_slice(), 
        
               tx_type_with_secret_hash: SwapTxTypeWithSecretHash::TakerOrMakerPayment { 
        
                   maker_secret_hash: secret_hash.as_slice(), 
        
               }, 
        
               swap_contract_address: &taker_coin_swap_contract_address, 
        
               swap_unique_data: &unique_data, 
        
               watcher_reward, 
        
           });

So basically for any broadcasted ethereum transaction, we have to make sure it's not reverted. One way is to add a check at the end of sign_and_send_transaction, by getting the receipt

komodo-defi-framework/mm2src/coins/eth.rs

Line 3606 in 8b1170d

    
           pub fn sign_and_send_transaction(&self, value: U256, action: Action, data: Vec<u8>, gas: U256) -> EthTxFut {

but this is equivalent to getting one confirmation as a transaction executed is a transaction mined and we already added confirmation step to swaps in #2199. Let's handle this case in a different PR as it needs more thinking, also handling dropped transactions are needed as we shouldn't increase nonce in such cases.

…-spend-maker-confirmation

dimxy · 2024-09-24T17:48:19Z

mm2src/mm2_main/src/lp_swap/taker_swap.rs

+        // we should wait for only one confirmation to make sure spend maker payment transaction is not failed
+        let confirm_maker_payment_spend_input = ConfirmPaymentInput {
+            payment_tx: transaction.tx_hex(),
+            confirmations: std::cmp::min(1, self.r().data.maker_payment_confirmations),


Is there any reason to involve 'maker_payment_confirmations' if we always want exactly 1 conf?
(And what would be if maker_payment_confirmations set to 0? Like if someone wants to run the whole swap with no confs at all?)

dimxy

LGTM

dimxy · 2024-09-24T18:20:07Z

So basically for any broadcasted ethereum transaction, we have to make sure it's not reverted.

If we do not want to wait until the payment spending tx is confirmed, we may just try to read it with eth_getTransactionByHash rpc which returns the tx at least in 'pending' state (I guess we also need to ensure we read the tx from the same eth node or it may not have time to propagate to other nodes yet). I believe the tx in the pending state does not guarantee it won't be reverted but ensures it is valid and the sender has sufficient funds to pay for it.

borngraced

Great work! only two notes

borngraced · 2024-09-24T18:41:36Z

mm2src/coins/eth.rs

@@ -2257,7 +2257,7 @@ impl MarketCoinOps for EthCoin {
        }
    }

-    fn wait_for_confirmations(&self, input: ConfirmPaymentInput) -> Box<dyn Future<Item = (), Error = String> + Send> {
+    fn wait_for_confirmations(&self, input: ConfirmPaymentInput) -> Box<dyn Future<Item = u64, Error = String> + Send> {


why not u8 or u16?

too keep consistence with the field pub confirmations: u64,

borngraced · 2024-09-24T18:46:12Z

mm2src/coins/tendermint/tendermint_coin.rs

@@ -2581,7 +2581,8 @@ impl MarketCoinOps for TendermintCoin {

                if let Some(tx_status_code) = tx_status_code {
                    return match tx_status_code {
-                        cosmrs::tendermint::abci::Code::Ok => Ok(()),
+                        // Tendermint uses Byzantine Fault Tolerance (BFT), achieve instant finality once a block is committed.
+                        cosmrs::tendermint::abci::Code::Ok => Ok(1u64),


I think it's better this function wait_for_confirmations return Optional u8 or 16 and we can return None for tendermint. cc @onur-ozkan

borngraced · 2024-09-24T18:50:35Z

I spot some duplicated changes with this PR #2199? @laruh

shamardy · 2024-09-25T00:33:09Z

I spot some duplicated changes with this PR #2199? @laruh

Yeah, we will probably use this #2199 already reviewing it.

shamardy · 2024-09-25T00:34:44Z

@laruh please close this PR so no else wastes time reviewing it. If we find we need this we will re-open it and close the other one, but looking at the other one there are no backward compatibility issues found yet :)

laruh · 2024-09-25T06:08:24Z

reason: see this #2206 (comment) pls

laruh · 2024-09-25T06:16:56Z

I spot some duplicated changes with this PR #2199? @laruh

Correct, these PRs contain code duplications in the context of calling wait_for_confirmations function. The main difference is that this PR #2199 handles confirmations as a standalone event in taker_swap.rs

call wait_for_confirmations after send_taker_spends_maker_payment…

17a5fab

… method in taker_swap.rs

laruh added the under review label Aug 29, 2024

laruh requested review from cipig and shamardy August 29, 2024 08:26

cipig approved these changes Aug 29, 2024

View reviewed changes

cipig mentioned this pull request Aug 29, 2024

failed swaps are marked as successful on maker and not refunded #2175

Closed

laruh mentioned this pull request Aug 30, 2024

fix(legacy-swap): taker failed spend maker payment marked as failed #2199

Open

laruh changed the title ~~fix(legacy-swap) wait for confirmation of taker spend maker payment~~ fix(legacy-swap): wait for confirmation of taker spend maker payment Aug 30, 2024

shamardy reviewed Sep 5, 2024

View reviewed changes

laruh added 2 commits September 6, 2024 15:42

provide maker_payment_requires_nota to requires_nota

9827b54

provide number of confirmations in wait_for_confirmations

98afd06

laruh force-pushed the legacy-swap-wait-taker-spend-maker-confirmation branch from ee0ab4c to 98afd06 Compare September 8, 2024 02:09

Merge remote-tracking branch 'origin/dev' into legacy-swap-wait-taker…

d846616

…-spend-maker-confirmation

dimxy reviewed Sep 24, 2024

View reviewed changes

borngraced requested changes Sep 24, 2024

View reviewed changes

laruh closed this Sep 25, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(legacy-swap): wait for confirmation of taker spend maker payment #2206

fix(legacy-swap): wait for confirmation of taker spend maker payment #2206

laruh commented Aug 29, 2024 •

edited

Loading

laruh commented Aug 29, 2024

cipig left a comment

laruh commented Aug 30, 2024

shamardy commented Sep 5, 2024 •

edited

Loading

shamardy left a comment

shamardy Sep 5, 2024

laruh Sep 8, 2024

shamardy Sep 5, 2024

shamardy commented Sep 5, 2024

laruh commented Sep 5, 2024 •

edited

Loading

laruh commented Sep 5, 2024

shamardy commented Sep 6, 2024

dimxy Sep 24, 2024

dimxy left a comment

dimxy commented Sep 24, 2024

borngraced left a comment

borngraced Sep 24, 2024

laruh Sep 25, 2024

borngraced Sep 24, 2024

borngraced commented Sep 24, 2024

shamardy commented Sep 25, 2024

shamardy commented Sep 25, 2024

laruh commented Sep 25, 2024

laruh commented Sep 25, 2024 •

edited

Loading

fix(legacy-swap): wait for confirmation of taker spend maker payment #2206

fix(legacy-swap): wait for confirmation of taker spend maker payment #2206

Conversation

laruh commented Aug 29, 2024 • edited Loading

laruh commented Aug 29, 2024

cipig left a comment

Choose a reason for hiding this comment

laruh commented Aug 30, 2024

shamardy commented Sep 5, 2024 • edited Loading

shamardy left a comment

Choose a reason for hiding this comment

shamardy Sep 5, 2024

Choose a reason for hiding this comment

laruh Sep 8, 2024

Choose a reason for hiding this comment

shamardy Sep 5, 2024

Choose a reason for hiding this comment

shamardy commented Sep 5, 2024

laruh commented Sep 5, 2024 • edited Loading

laruh commented Sep 5, 2024

shamardy commented Sep 6, 2024

dimxy Sep 24, 2024

Choose a reason for hiding this comment

dimxy left a comment

Choose a reason for hiding this comment

dimxy commented Sep 24, 2024

borngraced left a comment

Choose a reason for hiding this comment

borngraced Sep 24, 2024

Choose a reason for hiding this comment

laruh Sep 25, 2024

Choose a reason for hiding this comment

borngraced Sep 24, 2024

Choose a reason for hiding this comment

borngraced commented Sep 24, 2024

shamardy commented Sep 25, 2024

shamardy commented Sep 25, 2024

laruh commented Sep 25, 2024

laruh commented Sep 25, 2024 • edited Loading

laruh commented Aug 29, 2024 •

edited

Loading

shamardy commented Sep 5, 2024 •

edited

Loading

laruh commented Sep 5, 2024 •

edited

Loading

laruh commented Sep 25, 2024 •

edited

Loading