feat(swap): cancel maker order for offline coins #2357

borngraced · 2025-02-12T23:12:55Z

fix: #2281
handle maker order matching by checking coin connection status and cancel orders it all coin servers are down.

mm2src/mm2_main/src/lp_ordermatch.rs

onur-ozkan

2 nits, LGTM otherwise

mm2src/mm2_main/src/lp_ordermatch.rs

shamardy · 2025-02-27T18:56:30Z

@borngraced We need to also broadcast cancellation message when electrum is down and broadcast order created/updated message when the electrum is back again but the fix here #2232 will make this process complicated. So I'm proposing a simpler and probably safer solution.

The proposed solution differs from what @cipig suggested in #2281 (comment). I think it makes more sense to just cancel the orders when electrums are down and the maker can place them again themselves, if electrums are down similar to what happened in a recent swap, it's not safe for the order to go back online automatically leading to a swap with an unstable electrum.

mariocynicys

Thanks!

mariocynicys · 2025-04-17T10:30:02Z

mm2src/coins/utxo/rpc_clients/electrum_rpc/connection_manager/manager.rs

+    /// Checks if there are no live server connections available.
+    pub fn is_completely_offline(&self) -> bool {
+        for server in self.read_maintained_connections().iter() {
+            if self.get_connection(server.1).is_some() {


this will always return Some. the maintained_connections set is always a subset of all_connection.

u wanna get such a connection and check connection.is_connected() (here)

@smk762 could u explain what was the test scenario in here: #2357 (comment)

i might be missing something here

ahaaa, i didn't formulate this correctly in my head. thought this will always return false (so never offline).

but if read_maintained_connections() returns an empty list, then this will return true (all electrums offline). and in fact, checking that read_maintained_connections() is not empty is enough to know that we are connected, i.e. u don't have to check connection.is_connected(), as any maintained connection must be connected (when it disconnects it gets removed from the maintained connections set).

@smk762 could u explain what was the test scenario in here: #2357 (comment)

i might be missing something here

Launch kdf,

activate coins as usual.

Create a setprice order as maker with an unattractive price so it stays on book.

Check my_orders and confirm presence of the order.

Add node/electrum url to pihole blocklist (temporarily) to mimic a lost connection. Turning off wifi for a bit would likely also work if not using pihole.

Re-check my_orders, and confirm order was auto-cancelled.

Repeat steps 1-5 for other coin types.

mariocynicys · 2025-04-17T10:33:36Z

mm2src/coins/utxo/rpc_clients/electrum_rpc/connection_manager/manager.rs

+
+    /// Checks if there are no live server connections available.
+    pub fn is_completely_offline(&self) -> bool {
+        for server in self.read_maintained_connections().iter() {


nit:
for (_id, server_address) in maintained_connections { ... }

or:
for server_address in maintained_connections.values() { ... }

mariocynicys · 2025-04-17T10:51:57Z

mm2src/mm2_main/src/lp_ordermatch/my_orders_storage.rs

        async fn save_order_in_history(&self, order: &Order) -> MyOrdersResult<()> {
            let path = my_order_history_file_path(&self.ctx, &order.uuid());
-            write_json(order, &path, USE_TMP_FILE).await?;
+            write_json(&order, &path, USE_TMP_FILE).await?;


redundant ref on order.

mariocynicys · 2025-04-17T10:53:28Z

mm2src/coins/lp_coins.rs

+            MmCoinEnum::SiaCoin(c) => c.is_offline().await,
+            #[cfg(not(target_arch = "wasm32"))]
+            MmCoinEnum::LightningCoin(c) => c.platform_coin().as_ref().rpc_client.is_offline(),
+            MmCoinEnum::Test(_) => false,


should be annotated/cfg-ed for tests only. this breaks cargo check.

done thanks.

mariocynicys · 2025-04-17T10:55:05Z

mm2src/mm2_main/src/ordermatch_tests.rs

            base_orderbook_ticker: None,
            rel_orderbook_ticker: None,
            p2p_privkey: None,
+


an extra line break;
also present in the rest of the file.

thanks done.

mariocynicys · 2025-04-17T11:00:12Z

mm2src/mm2_main/tests/mm2_tests/best_orders_tests.rs

+        let mut current_dir = std::env::current_dir().unwrap();
+        current_dir.pop();
+        current_dir.pop();
+        current_dir


could u write in comments where such current_dir() is.
like:

// this should be: DB_ROOT/x/y/zcash_db/etc.. let mut current_dir = std::env::current_dir().unwrap(); // this is: DB_ROOT/x/y/zcash_db current_dir.pop(); ...

also doesn't this depend on where from these tests are ran?
like, running the tests from the root of the komodo-defi-framework will result in a different current dir than when running from komodo-defi-framework/mm2src/mm2_main or mm2_tests? which would break the .join below?

seems like a failed merge conflict.
reverted

mariocynicys · 2025-04-17T11:10:27Z

mm2src/mm2_main/src/lp_ordermatch.rs

        if order.available_amount() >= order.min_base_vol || order.has_ongoing_matches() {
+            if order.has_ongoing_matches() {
+                continue;
+            }


has_ongoing_matches is OR-ed in the first if clause and then checked alone in the second.

we could move it out before the available_amount() check.

mariocynicys · 2025-04-17T11:14:12Z

mm2src/mm2_main/src/lp_ordermatch.rs

+    ordermatch_ctx: &OrdermatchContext,
+    order: &MakerOrder,
+) -> bool {
+    if let Ok(Some(coin)) = lp_coinfind(ctx, &order.base).await {


i think we should also cancel the order if it's not found (tho we also handle it else where i presume). and also cancel it if lp_coinfind errors (just for code simplicity, since we know it shouldn't really error).

hmmm. I believe lp_ordermatch loop will wait till a coin is active before matching etc.

and also cancel it if lp_coinfind errors (just for code simplicity, since we know it shouldn't really error).

And I do not think we want to do this since coin not found seems very different from when it is offline.

mariocynicys · 2025-04-17T11:27:55Z

if somebody is using a couple of many unreliable electrums they might lose coin connection for a couple of seconds every now and then. same for when they themselves have unreliable connections.

i would be for just stopping order rebroadcasting and rejecting any taker requests till the coin is back online. removing the orders completely will be some pain in the butt for a maker that went offline for a split second.

smk762 · 2025-05-07T02:04:41Z

if somebody is using a couple of many unreliable electrums they might lose coin connection for a couple of seconds every now and then. same for when they themselves have unreliable connections.

i would be for just stopping order rebroadcasting and rejecting any taker requests till the coin is back online. removing the orders completely will be some pain in the butt for a maker that went offline for a split second.

On the taker side, it is a pain in the butt for a user to match with an order which fails at the negotiation stage. From a guilt/punishment perspective:

A coin with unstable electrums imposes negative UX on both maker and taker, and its faults add to support team workload. These should be identified, warned and purged if not up to standard (or replaced with reliable in-house servers if the inclusion of the coin is desirable).
A maker with an unstable connection is at fault for their own failure to ensure reliable hardware/isp. If this leads to negotiation fails on a stale order, the taker suffers without contributing to the failure.
A taker with an unstable connection selects orders from the book, so cancellation upon connection loss is irrelevant. There is a chance that taker connection interruptions occur while negotiating or performing subsequent swap events, though once a swap is in progress, short connection interruptions generally wont result in a failed swap, assuming connection is active long enough for events / confirmations to be updated (and there is always the watchtowers to help also).

Based on this, IMO auto-cancel on connection loss is justified, though a brief (< 2 sec) window to recheck could be included as a compromise to cover smaller things causing an interruption

shamardy · 2025-05-07T02:19:21Z

auto-cancel on connection loss is justified, though a brief (< 2 sec) window to recheck could be included as a compromise to cover smaller things causing an interruption

Totally agree to this. Takers order cancelling is no big deal at all as they can place it again straight away and is common even in AMM dexes. For makers, they don't just place orders, they use scripts/tools/bots so auto cancellation is also fine but they should be notified (they probably check order statuses automatically by the script instead) and they will place the orders again later when electrums are back online.

borngraced · 2025-05-09T03:43:49Z

if somebody is using a couple of many unreliable electrums they might lose coin connection for a couple of seconds every now and then. same for when they themselves have unreliable connections.
i would be for just stopping order rebroadcasting and rejecting any taker requests till the coin is back online. removing the orders completely will be some pain in the butt for a maker that went offline for a split second.

On the taker side, it is a pain in the butt for a user to match with an order which fails at the negotiation stage. From a guilt/punishment perspective:
* A coin with unstable electrums imposes negative UX on both maker and taker, and its faults add to support team workload. These should be identified, warned and purged if not up to standard (or replaced with reliable in-house servers if the inclusion of the coin is desirable).

* A maker with an unstable connection is at fault for their own failure to ensure reliable hardware/isp. If this leads to negotiation fails on a stale order, the taker suffers without contributing to the failure.

* A taker with an unstable connection selects orders from the book, so cancellation upon connection loss is irrelevant. There is a chance that taker connection interruptions occur while negotiating or performing subsequent swap events, though once a swap is in progress, short connection interruptions generally wont result in a failed swap, assuming connection is active long enough for events / confirmations to be updated (and there is always the watchtowers to help also).
Based on this, IMO auto-cancel on connection loss is justified, though a brief (< 2 sec) window to recheck could be included as a compromise to cover smaller things causing an interruption

I can add retry with sleep which should solve this. cc @shamardy

smk762 · 2025-07-24T03:15:29Z

We've been merging dev into this one for a while, and with the current conflict requiring resolution in lp_ordermatch.rs, another one is on the horizon. Is it still on track to be in 2.6.0?

shamardy · 2025-07-24T17:42:40Z

We've been merging dev into this one for a while, and with the current conflict requiring resolution in lp_ordermatch.rs, another one is on the horizon. Is it still on track to be in 2.6.0?

Yes, still on track, will give it a look and fix anything myself when I start the finalization of 2.6.0-beta. For now, there will be 2.5.1-beta first for the private key export.

borngraced added 5 commits January 27, 2025 19:38

save dev state

eac9f81

fix string termination err

3618ddd

add is_active flag for MakerOrder

6a48ca2

add todos and fill is_active flag for MakerOrder initializations

d2b8cff

pause maker others from matching if coin relies on electrum conns

98f53cc

borngraced added the status: in progress label Feb 12, 2025

borngraced self-assigned this Feb 12, 2025

borngraced force-pushed the cancel-offline-electrum-orders branch from b704ba0 to 98f53cc Compare February 12, 2025 23:29

borngraced added 4 commits February 13, 2025 00:31

remove todo comment and unneeded if-else

ca7190f

remove unused

6420d94

nit

55e7f7a

Merge branch 'dev' into cancel-offline-electrum-orders

c4e458e

borngraced added status: pending review and removed status: in progress labels Feb 14, 2025

shamardy added the 2.5.0-beta label Feb 17, 2025

shamardy requested review from onur-ozkan and shamardy February 17, 2025 10:09

borngraced added 3 commits February 17, 2025 12:04

update flag to is_offline

5818720

fix cargo clippy

d58d2c0

update order log info for offline orders

029c457

onur-ozkan reviewed Feb 18, 2025

View reviewed changes

mm2src/mm2_main/src/lp_ordermatch.rs Outdated Show resolved Hide resolved

borngraced added 3 commits February 19, 2025 02:52

Merge branch 'dev' into cancel-offline-electrum-orders

2e33e90

use enum for MakerOrder status

b7ef6c3

fix clippy warnings

e18f60b

onur-ozkan reviewed Feb 19, 2025

View reviewed changes

mm2src/mm2_main/src/lp_ordermatch.rs Outdated Show resolved Hide resolved

mm2src/mm2_main/src/lp_ordermatch.rs Outdated Show resolved Hide resolved

borngraced added 3 commits February 19, 2025 13:56

rename MakerOrderStatus enum variants

44ce1a9

update docs

94c5111

Merge branch 'dev' into cancel-offline-electrum-orders

5997e23

cancel order, refactor save_my_t/maker_order

eedc9b9

borngraced dismissed smk762’s stale review via 928520e March 24, 2025 07:47

smk762 self-requested a review April 2, 2025 09:14

smk762 previously approved these changes Apr 2, 2025

View reviewed changes

Merge branch 'dev' into cancel-offline-electrum-orders

ba36f6c

mariocynicys reviewed Apr 17, 2025

View reviewed changes

fix review notes

5ef3858

borngraced dismissed smk762’s stale review via 5ef3858 April 17, 2025 11:57

borngraced added 2 commits April 17, 2025 13:15

nits

f49e339

Merge branch 'dev' into cancel-offline-electrum-orders

6287556

borngraced added 5 commits May 14, 2025 13:10

Merge branch 'dev' into cancel-offline-electrum-orders

0fa2ba8

Merge branch 'dev' into cancel-offline-electrum-orders

f505257

Merge branch 'dev' into cancel-offline-electrum-orders

18e3c87

Merge branch 'dev' into cancel-offline-electrum-orders

a4bea76

Merge branch 'dev' into cancel-offline-electrum-orders

c18e4af

shamardy added 2.6.0-beta and removed 2.5.0-beta labels Jun 16, 2025

shamardy added 3 commits July 24, 2025 20:43

merge with origin/dev

9306a25

post merge fix

7c7f370

merge with origin/dev

d75abc7

shamardy assigned shamardy and unassigned borngraced Jul 30, 2025

shamardy marked this pull request as draft July 30, 2025 14:01

shamardy removed the status: pending review label Jul 30, 2025

shamardy removed the 2.6.0-beta label Oct 6, 2025

feat(swap): cancel maker order for offline coins #2357

Are you sure you want to change the base?

feat(swap): cancel maker order for offline coins #2357

Uh oh!

Conversation

borngraced commented Feb 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

onur-ozkan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

mariocynicys left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

borngraced Apr 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mariocynicys commented Apr 17, 2025

Uh oh!

smk762 commented May 7, 2025

Uh oh!

shamardy commented May 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

borngraced commented May 9, 2025

Uh oh!

smk762 commented Jul 24, 2025

Uh oh!

shamardy commented Jul 24, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

borngraced commented Feb 12, 2025 •

edited

Loading

shamardy commented Feb 27, 2025 •

edited

Loading

borngraced Apr 17, 2025 •

edited

Loading

shamardy commented May 7, 2025 •

edited

Loading