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

[laruh]

related to #2175 (comment)

[laruh]

Second problem solving approach

If we wouldn't like to do changes in legacy taker swap events/logic and increase backward incompatibility risks, there is an alternative. We can add maker payment spend confirmation right after send_taker_spends_maker_payment in spend_maker_payment taker swap operation.

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

Lines 1787 to 1812 in 5c47ef0

	async fn spend_maker_payment(&self) -> Result<(Option<TakerSwapCommand>, Vec<TakerSwapEvent>), String> {
	#[cfg(any(test, feature = "run-docker-tests"))]
	if self.fail_at == Some(FailAt::MakerPaymentSpend) {
	return Ok((Some(TakerSwapCommand::Finish), vec![
	TakerSwapEvent::MakerPaymentSpendFailed("Explicit test failure".into()),
	]));
	} else if self.fail_at == Some(FailAt::MakerPaymentSpendPanic) {
	panic!("Taker panicked unexpectedly at maker payment spend");
	}
	let other_maker_coin_htlc_pub = self.r().other_maker_coin_htlc_pub;
	let secret = self.r().secret;
	let taker_spends_payment_args = SpendPaymentArgs {
	other_payment_tx: &self.r().maker_payment.clone().unwrap().tx_hex,
	time_lock: self.maker_payment_lock.load(Ordering::Relaxed),
	other_pubkey: &*other_maker_coin_htlc_pub,
	secret: &secret.0,
	secret_hash: &self.r().secret_hash.clone().0,
	swap_contract_address: &self.r().data.maker_coin_swap_contract_address.clone(),
	swap_unique_data: &self.unique_swap_data(),
	watcher_reward: self.r().watcher_reward,
	};
	let maybe_spend_tx = self
	.maker_coin
	.send_taker_spends_maker_payment(taker_spends_payment_args)
	.await;

If confirmation failed, then also return swap finish command and MakerPaymentSpendFailed event

                return Ok((Some(TakerSwapCommand::Finish), vec![
                    TakerSwapEvent::MakerPaymentSpendFailed(ERRL!("{}", err.get_plain_text_format()).into()),
                ]));

MakerPaymentSpendFailed is not actually correct event in the case of failed maker payment spend confirmation, but in this way we dont do changes in Taker commands and events logic.

In comparison, Maker has taker payment spend confirmation events in MakerSwapEvent:

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

Lines 1646 to 1648 in 5c47ef0

	TakerPaymentSpendConfirmStarted,
	TakerPaymentSpendConfirmed,
	TakerPaymentSpendConfirmFailed(SwapError),

[laruh]

Solved with second approach in this pr #2206

UPD: opened again as we decided to test both approaches #2206 (comment)

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

This pr approach could be preferable

[shamardy]

@laruh this is still draft, should I review it? I want to check it so that I can close the other PR if there are no problems with the approach here.

[laruh]

@laruh this is still draft, should I review it? I want to check it so that I can close the other PR if there are no problems with the approach here.

Its almost r2r, I just wanted to fix these notes #2206 (review) in other approach and cherry pick commit to this branch, as both branches would have the same review notes. (already fixed this b2446b9, just confirmation number is left)

[laruh]

@shamardy its r2r. added confirmations here 2c119be

[laruh]

@shamardy when we wait for taker payment spend on maker side, we use false for requires_nota.
Shouldnt we do the same on taker side, when they wait for maker payment spend?

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

Lines 1143 to 1153 in b9ba150

	async fn confirm_taker_payment_spend(&self) -> Result<(Option<MakerSwapCommand>, Vec<MakerSwapEvent>), String> {
	// we should wait for only one confirmation to make sure our spend transaction is not failed
	let confirmations = std::cmp::min(1, self.r().data.taker_payment_confirmations);
	let requires_nota = false;
	let confirm_taker_payment_spend_input = ConfirmPaymentInput {
	payment_tx: self.r().taker_payment_spend.clone().unwrap().tx_hex.0,
	confirmations,
	requires_nota,
	wait_until: self.wait_refund_until(),
	check_every: WAIT_CONFIRM_INTERVAL_SEC,
	};

[shamardy]

when we wait for taker payment spend on maker side, we use false for requires_nota.
Shouldnt we do the same on taker side, when they wait for maker payment spend?

Agree, since we use 1 confirmation at most. I was wrong to suggest otherwise in a previous review.

[laruh]

fixed 165ddfe

[shamardy]

Thanks for the fix! I have a few comments.

[shamardy]

For old saved swaps, we should still check MakerPaymentSpent as below

// MakerPaymentSpent was the last success event but a new step `MakerPaymentSpendConfirmed` was added after it
// For backward compatibility (old saved swaps) we need to check for MakerPaymentSpent but no MakerPaymentSpendConfirmFailed
TakerSwapEvent::MakerPaymentSpent(_) => {
    if !self
        .events
        .iter()
        .any(|event| matches!(event.event, TakerSwapEvent::MakerPaymentSpendConfirmFailed(_)))
    {
        return false;
    }
},

So a finished swap that has MakerPaymentSpent but no MakerPaymentSpendConfirmFailed is not recoverable as it was considered successful. What do you think?

[laruh]

Did this aed8d4c

[dimxy]

I guess version field would be useful in TakerSavedSwap and MakerSavedSwap to simplify such logic

[shamardy]

I guess version field would be useful in TakerSavedSwap and MakerSavedSwap to simplify such logic

If I understand this correctly, you are suggesting a new version field for saved swaps not to be confused for swap version for p2p messages. right?

[dimxy]

Yes, I added a version field in PR#2112, it's a protocol version (for a node to understand what features a remote peer supports)
Here we changed the swap format, I think it's good to have a version field in it, to increment it whenever we change its structure, so when we read old swaps we may understand what features they supported and process accordingly

[shamardy]

This is an example of a successful finished swap before this PR where this is the last event before finished

{
    "timestamp": 1588244078870,
    "event": {
        "type": "MakerPaymentSpent",
        "data": {
            "tx_hex": "0400008085202f8901dfbf5ad1339c6f9478c4917cd3d90ac56bfdc2e75c57c94db20bed853f0b4c9a00000000d848304502210092535c081325ba5261699d7cfd4c503fb6125dde86389b83f40f3e2c006039bb022063cfd72aa15558dee874cac08b22dbcf11d3f06c8e48b0ddaf75b86887d604410120dedf3c8dcfff9ee2787b4bf211f960fd044fdab7fa8e922ef613a0115848a498004c6b630434ebaa5eb1752102987d5f82205a55d789616f470ae9df48537f050ee050d501aa316651642a0a4dac6782012088a9149304bce3196f344b2a22dc99db406e95ab6f3107882102859a80b83941e4e8ff2f511080e3ea5021db4ba95caec30eb37864e71ae73521ac68ffffffff0130e07648170000001976a914d24e799df360da3ca3158d63b89ffaff27722c1588ac5ea0aa5e000000000000000000000000000000",
            "tx_hash": "caea128b1c85a88abd5924e512780ee18952dadc217b0c06f4b2820eb71d03bc",
            "from": [
                "bYp9ncp3V7FYsymipriVbdd3QL72hK9hio"
            ],
            "to": [
                "RUTBzLtJNTn89Wkb6oZocbesKrjBDTRMrC"
            ],
            "total_amount": "999.99999",
            "spent_by_me": "0",
            "received_by_me": "999.99998",
            "my_balance_change": "999.99998",
            "block_height": 0,
            "timestamp": 0,
            "fee_details": {
                "amount": "0.00001"
            },
            "coin": "MYCOIN",
            "internal_id": "caea128b1c85a88abd5924e512780ee18952dadc217b0c06f4b2820eb71d03bc"
        }
    }
}

It's fine to leave it as is as it's backward compatible, but we can also add the new event to it. What do you think?

[laruh]

added "MakerPaymentSpendConfirmed" event ed91569

[shamardy]

The below is related to this change in get_command_based_on_maker_or_watcher_activity.

I think we should return TakerSwapCommand::ConfirmMakerPaymentSpend instead of TakerSwapCommand::Finish here

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

Line 145 in 1c3c935

Ok(TakerSwapCommand::Finish)

[shamardy]

c.c. @dimxy

[dimxy]

Seems so.
search_for_swap_tx_spend_other fn may return unconfirmed txns.
So we need (with the created event MakerPaymentSpentByWatcher) to go to ConfirmMakerPaymentSpend command.
I think it's better to fix also get_command() to ensure MakerPaymentSpentByWatcher event triggers ConfirmMakerPaymentSpend.
And good to have a new swap watcher test for this to ensure the flow works okay

[laruh]

I think it's better to fix also get_command() to ensure MakerPaymentSpentByWatcher event triggers ConfirmMakerPaymentSpend.

yep, if we want to return TakerSwapCommand::ConfirmMakerPaymentSpend in check_maker_payment_spend_and_add_event, then we also need to cover confirmation step in get_command for MakerPaymentSpentByWatcher

[laruh]

I think it's better to fix also get_command() to ensure MakerPaymentSpentByWatcher event triggers ConfirmMakerPaymentSpend.

yep, if we want to return TakerSwapCommand::ConfirmMakerPaymentSpend in check_maker_payment_spend_and_add_event, then we also need to cover confirmation step in get_command for MakerPaymentSpentByWatcher

Hmm it means that we also need to update fn is_recoverable according to the same note as here #2199 (comment)

[laruh]

@shamardy one moment about watchers, do we actually use this feature in production? Im not sure that it is fully functional in prod.

If nope, we shouldn't change MakerPaymentSpentByWatcher handle in legacy get_command()function

[shamardy]

yep, if we want to return TakerSwapCommand::ConfirmMakerPaymentSpend in check_maker_payment_spend_and_add_event, then we also need to cover confirmation step in get_command for MakerPaymentSpentByWatcher

It will be done here

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

Lines 135 to 145 in 1c3c935

	let event = TakerSwapEvent::MakerPaymentSpentByWatcher(tx_ident);
	let to_save = TakerSavedEvent {
	timestamp: now_ms(),
	event,
	};
	swap.apply_event(to_save.event.clone());
	saved.events.push(to_save);
	let new_swap = SavedSwap::Taker(saved);
	try_s!(new_swap.save_to_db(ctx).await);
	info!("{}", MAKER_PAYMENT_SPENT_BY_WATCHER_LOG);
	Ok(TakerSwapCommand::Finish)

by this change #2199 (comment)

Hmm it means that we also need to update fn is_recoverable according to the same note as here #2199 (comment)

Yes, I suppose.

one moment about watchers, do we actually use this feature in production? Im not sure that it is fully functional in prod.
If nope, we shouldn't change MakerPaymentSpentByWatcher handle in legacy get_command()function

It's used for utxo <-> utxo swap in production. We need to collect data about it though to know how many users used it etc.. , we should leave a comment about this to @KomodoPlatform/qa

[shamardy]

We might need to handle it here too

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

Line 187 in aed8d4c

TakerSwapEvent::MakerPaymentSpentByWatcher(_) => Some(TakerSwapCommand::Finish),

[laruh]

It's used for utxo <-> utxo swap in production.

Got it, then I also update MakerPaymentSpentByWatcher cases

[laruh]

We might need to handle it here too

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

Line 187 in aed8d4c

TakerSwapEvent::MakerPaymentSpentByWatcher(_) => Some(TakerSwapCommand::Finish),

yep, its what dimxy was referencing about

[dimxy]

Can maker_payment_confirmations be 0?
Should 'confirmations' be also 0 in this case?

[laruh]

Can maker_payment_confirmations be 0?

seems like yes. I took confirm_taker_payment_spend from maker_swap.rs as an example

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

Lines 1143 to 1153 in 0073fe8

	async fn confirm_taker_payment_spend(&self) -> Result<(Option<MakerSwapCommand>, Vec<MakerSwapEvent>), String> {
	// we should wait for only one confirmation to make sure our spend transaction is not failed
	let confirmations = std::cmp::min(1, self.r().data.taker_payment_confirmations);
	let requires_nota = false;
	let confirm_taker_payment_spend_input = ConfirmPaymentInput {
	payment_tx: self.r().taker_payment_spend.clone().unwrap().tx_hex.0,
	confirmations,
	requires_nota,
	wait_until: self.wait_refund_until(),
	check_every: WAIT_CONFIRM_INTERVAL_SEC,
	};

std::cmp::min ensures that at least one confirmation is required unless taker_payment_confirmations is explicitly set to a value less than 1.

Should 'confirmations' be also 0 in this case?

Previously confirmations value always was 1, but was changed in this PR

https://github.com/KomodoPlatform/komodo-defi-framework/pull/971/files#diff-b837d781bf1a7f0fb7828ac91d7b96f2f75328bb14f5cd47c2dd316f837a2574R796-R797

@shamardy could you clarify please, why return minimum between 1 and the value of taker_payment_confirmations was added previously? From what I see, by this we decided to allow to skip payment spend confirmation step?

[shamardy]

Yes, we allow the user to use 0 confirmations if they want since 0 confirmations will be used in a more important step before that if it was set

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

Lines 982 to 997 in 1461212

	async fn validate_taker_payment(&self) -> Result<(Option<MakerSwapCommand>, Vec<MakerSwapEvent>), String> {
	let wait_taker_payment = taker_payment_spend_deadline(self.r().data.started_at, self.r().data.lock_duration);
	let confirmations = self.r().data.taker_payment_confirmations;
	let taker_coin_swap_contract_address = self.r().data.taker_coin_swap_contract_address.clone();
	let confirm_taker_payment_input = ConfirmPaymentInput {
	payment_tx: self.r().taker_payment.clone().unwrap().tx_hex.0,
	confirmations,
	requires_nota: self.r().data.taker_payment_requires_nota.unwrap_or(false),
	wait_until: wait_taker_payment,
	check_every: WAIT_CONFIRM_INTERVAL_SEC,
	};
	let wait_f = self
	.taker_coin
	.wait_for_confirmations(confirm_taker_payment_input)
	.compat();

[laruh]

Ok, I will update we should wait for only one confirmation... comment then, to make it clear that we also allow 0 confirmations