fix(legacy_swap): check for existing maker/taker payment before timeout by shamardy · Pull Request #2283 · GLEECBTC/komodo-defi-framework

shamardy · 2024-11-27T19:46:26Z

This PR moves payment existence check in maker_payment/send_taker_payment before timeout validation and skips timeout if payment is already sent, as the taker swap should proceed to waiting for maker to spend the taker payment.

Should fix #2136 , but we are still not aware why the electrum request doesn't timeout.

Move payment existence check before timeout validation and skip timeout if payment is already sent, as we should proceed to waiting for maker to spend the taker payment.

laruh

Thanks for the fix!
Changes are well-structured and code in general is good to me, I have non mandatory suggestion

mm2src/mm2_main/src/lp_swap/taker_swap.rs

dimxy · 2024-11-28T10:29:23Z

So this actually fixes the situation (for taker case):
Taker sent the payment but logged no event for that. Then went offline for a long time. When Taker restarts he finds the timeout has gone and fails the swap in a state in which he does not refund (but he can refund)?

cipig · 2024-11-28T11:02:45Z

When Taker restarts he finds the timeout has gone and fails the swap in a state in which he does not refund (but he can refund)?

not sure if i understood the question, but taker can only refund if he removes the TakerPaymentTransactionFailed event from his swap JSON

      {
         "event" : {
            "data" : {
               "error" : "taker_swap:1488] Timeout 1717611160 > 1717610301"
            },
            "type" : "TakerPaymentTransactionFailed"
         },
         "timestamp" : 1717611160346
      },

and replaces it with a TakerPaymentSent event and also adds the correct txid and txhash from that particular swap to the event... like this

      {
         "event" : {
            "data" : {
               "tx_hash" : "84fa6b8d50aff9a727c61b8f1bf33564c8958d120669da33fa385d45f90f6da0",
               "tx_hex" : "0400008085202f8901a33d00490a3d9e3cbccbb394013c91d4f80dc1f2a19191632a29747d2f439301010000006b48304502210094e97cb9999aa77357f29f23a8e0bf778dbbf238523a92875992b7ea8af303e502204e1554dd35d6cbdebe3f5780609163473aa9b918b7213bfbe018dd672a9a12cd0121028a38bafdd6c1aa3ec34c480d6ae9383bee54eef7c115e51a5aeab699734ff055ffffffff03008589dc0000000017a914ea8dcc7b4ea177f7a2c999d7a603aab8d6308b2f870000000000000000166a14718d7a526e8d14f45b5795db8b864924b0cd90c6d2bee2f60d0000001976a914d89058fbcce7402fb342dffadf04bd32dba29abe88ac89a16066000000000000000000000000000000"
            },
            "type" : "TakerPaymentSent"
         },
         "timestamp" : 1717611160346
      },

this was the situation from before this change...

dimxy · 2024-11-28T11:39:27Z

I am trying to work out how taker came to situation that he actually sent taker payment but did not log any event about this (to memorise this, it's not clear from the issue description)

cipig · 2024-11-28T11:44:07Z

I am trying to work out how taker came to situation that he actually sent taker payment but did not log any event about this (to memorise this, it's not clear from the issue description)

That's the big question, indeed. I first thought the electrums returned a timeout, but sent the tx anyway. But that's not the case, else we would have an error from electrum there in the event and not simply taker_swap:1488] Timeout 1717611160 > 1717610301. Will show the affected user this, maybe he can tell us more about the "circumstances".

mariocynicys · 2024-11-28T12:51:04Z

That's the big question, indeed. I first thought the electrums returned a timeout, but sent the tx anyway. But that's not the case, else we would have an error from electrum there in the event and not simply taker_swap:1488] Timeout 1717611160 > 1717610301. Will show the affected user this, maybe he can tell us more about the "circumstances".

if the process is killed after this line https://github.com/KomodoPlatform/komodo-defi-framework/blob/f401497aee55ee8152f96955bd40dd147b8ab88f/mm2src/mm2_main/src/lp_swap/taker_swap.rs#L1581

but before the swap event is written, this could happen.

mariocynicys

tiny notes. LGTM otherwise

mm2src/mm2_main/src/lp_swap/maker_swap.rs

shamardy · 2024-12-06T16:48:47Z

@mariocynicys @laruh please review the refactor next week.

mm2src/mm2_main/src/lp_swap/taker_swap.rs

dimxy · 2024-12-10T17:35:26Z

mm2src/mm2_main/src/lp_swap/taker_swap.rs

+            &self.unique_swap_data()[..],
+        );
+
+        let time_lock = match std::env::var("USE_TEST_LOCKTIME") {


Should we not allow this test locktime only if enabled by a cfg feature?
(I guess we tend to make all test code cfg enabled)

Removed USE_TEST_LOCKTIME occurrences in swaps code completely, and from the whole codebase. Took me some time to make the tests work again but this should make the legacy swap code cleaner. We should look into removing all other env vars in swaps code when we have the time.

dimxy · 2024-12-11T09:15:44Z

I added a note why the swap failed on send taker payment and manual swap file editing was required.
This PR fixes this issue: even if TakerPaymentSent was not saved in the file, the fix checks that the payment tx in fact was sent and the swap continues.

(PS. I guess, there may be similar issues in other swap steps due to the event not saved)

LGTM

laruh

LGTM!
I have last non blocking suggestion

mm2src/mm2_main/src/lp_swap/taker_swap.rs

* dev: (35 commits) fix(crypto): allow non bip39 mnemonics storage (GLEECBTC#2312) fix(legacy_swap): check for existing maker/taker payment before timeout (GLEECBTC#2283) feat(tendermint): validators RPC (GLEECBTC#2310) chore(CI): validate Cargo lock file (GLEECBTC#2309) test(P2P): add test for peer time sync validation (GLEECBTC#2304) fix mm2_p2p dev build (GLEECBTC#2311) update Cargo.lock (GLEECBTC#2308) chore(CI): unlock wasm-pack version (GLEECBTC#2307) add `wasm` feature on WASM for timed-map (GLEECBTC#2306) replace broken rpc link (GLEECBTC#2305) chore(eth-websocket): remove some unnecessary wrappers (GLEECBTC#2291) improvement(CI): switch to proper rust caching (GLEECBTC#2303) fix(wasm): add test-ext-api feature to mm2_main and mm2_bin_lib tomls (GLEECBTC#2295) chore(ci): Update docker build for wasm (GLEECBTC#2294) chore(p2p): follow-up nits (GLEECBTC#2302) feat(p2p): ensure time synchronization in the network (GLEECBTC#2255) bump libp2p (GLEECBTC#2296) chore(adex-cli): use "Komodo DeFi Framework" name in adex_cli (GLEECBTC#2290) chore(ctx): replace gstuff constructible with oncelock (GLEECBTC#2267) don't rely on core (GLEECBTC#2289) ...

…ut (#2283) This commit moves payment existence check in maker_payment/send_taker_payment before timeout validation and skips timeout if payment is already sent, as the taker swap should proceed to waiting for maker to spend the taker payment.

fix(taker_swap): check for existing payment before timeout

38d5bde

Move payment existence check before timeout validation and skip timeout if payment is already sent, as we should proceed to waiting for maker to spend the taker payment.

shamardy added the in progress label Nov 27, 2024

fix(maker_swap): implement the same fix

cedebad

shamardy changed the title ~~fix(legacy_taker_swap): check for existing payment before timeout~~ fix(legacy_swap): check for existing maker/taker payment before timeout Nov 27, 2024

shamardy added under review and removed in progress labels Nov 28, 2024

laruh reviewed Nov 28, 2024

View reviewed changes

mm2src/mm2_main/src/lp_swap/taker_swap.rs Show resolved Hide resolved

mariocynicys reviewed Nov 28, 2024

View reviewed changes

mm2src/mm2_main/src/lp_swap/maker_swap.rs Outdated Show resolved Hide resolved

mm2src/mm2_main/src/lp_swap/maker_swap.rs Outdated Show resolved Hide resolved

shamardy added 5 commits December 4, 2024 00:54

review fixes: make some code comments better

c039ebd

refactor: extract taker payment watcher reward setup to a function

61f8357

refactor: extract maker payment watcher reward setup to a function

81f4e83

refactor: extract taker payment watcher logic to a function

199b135

one last small refactor

f6471b5

mariocynicys reviewed Dec 6, 2024

View reviewed changes

mm2src/mm2_main/src/lp_swap/taker_swap.rs Show resolved Hide resolved

shamardy added the 2.3.0-beta label Dec 9, 2024

add some doc comments

5eca01d

dimxy reviewed Dec 10, 2024

View reviewed changes

laruh previously approved these changes Dec 11, 2024

View reviewed changes

mm2src/mm2_main/src/lp_swap/taker_swap.rs Show resolved Hide resolved

dimxy self-requested a review December 11, 2024 11:33

remove reliance on USE_TEST_LOCKTIME env var

557ebe2

shamardy dismissed laruh’s stale review via 557ebe2 December 12, 2024 14:59

onur-ozkan added status: pending review and removed under review labels Dec 16, 2024

dimxy mentioned this pull request Dec 27, 2024

fix(legacy-refunds): support going the spending path on refund #2280

Draft

shamardy added 2.4.0-beta and removed 2.3.0-beta labels Jan 9, 2025

rename setup_maker_watcher_reward to setup_watcher_reward

6568a6b

shamardy merged commit 07416d1 into dev Jan 9, 2025

shamardy deleted the fix-check-payment-before-timeout branch January 9, 2025 13:41

shamardy mentioned this pull request Apr 16, 2025

chore(release): v2.4.0-beta #2345

Merged

shamardy mentioned this pull request Jul 4, 2025

failed swap can't be refunded #2136

Closed

Conversation

shamardy commented Nov 27, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

laruh left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dimxy commented Nov 28, 2024

Uh oh!

cipig commented Nov 28, 2024

Uh oh!

dimxy commented Nov 28, 2024

Uh oh!

cipig commented Nov 28, 2024

Uh oh!

mariocynicys commented Nov 28, 2024

Uh oh!

mariocynicys left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

shamardy commented Dec 6, 2024

Uh oh!

Uh oh!

dimxy Dec 10, 2024

Choose a reason for hiding this comment

Uh oh!

shamardy Dec 12, 2024

Choose a reason for hiding this comment

Uh oh!

dimxy commented Dec 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

laruh left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

shamardy commented Nov 27, 2024 •

edited

Loading

dimxy commented Dec 11, 2024 •

edited

Loading