-
Notifications
You must be signed in to change notification settings - Fork 95
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(legacy-swap): taker failed spend maker payment marked as failed #2199
base: dev
Are you sure you want to change the base?
Conversation
…d_taker_expected.json
…sts, iris_nimda_rick_taker_swap.json
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 komodo-defi-framework/mm2src/mm2_main/src/lp_swap/taker_swap.rs Lines 1787 to 1812 in 5c47ef0
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()),
]));
In comparison, Maker has taker payment spend confirmation events in komodo-defi-framework/mm2src/mm2_main/src/lp_swap/maker_swap.rs Lines 1646 to 1648 in 5c47ef0
|
Solved with second approach in this pr #2206 UPD: opened again as we decided to test both approaches #2206 (comment)
This pr approach could be preferable |
…d::ConfirmMakerPaymentSpend)
…d => Ok(command),
…and_add_event(ctx, swap, saved).await in taker restart
…pCommand::Finish),
…TS in for_tests.rs
@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) |
let confirm_maker_payment_spend_input = ConfirmPaymentInput { | ||
payment_tx: self.r().maker_payment_spend.clone().unwrap().tx_hex.0, | ||
confirmations: std::cmp::min(1, self.r().data.maker_payment_confirmations), | ||
requires_nota: self.r().data.maker_payment_requires_nota.unwrap_or(false), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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, | |
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed 165ddfe
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the fix! I have a few comments.
@@ -267,7 +272,7 @@ impl TakerSavedSwap { | |||
| TakerSwapEvent::MakerPaymentValidateFailed(_) | |||
| TakerSwapEvent::TakerPaymentRefunded(_) | |||
| TakerSwapEvent::TakerPaymentRefundedByWatcher(_) | |||
| TakerSwapEvent::MakerPaymentSpent(_) | |||
| TakerSwapEvent::MakerPaymentSpendConfirmed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did this aed8d4c
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess version field would be useful in TakerSavedSwap and MakerSavedSwap to simplify such logic
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const UNFINISHED_TAKER_SWAP: &str = r#"{"type":"Taker","uuid":"5acb0e63-8b26-469e-81df-7dd9e4a9ad15","events":[{"timestamp":1588244036253,"event":{"type":"Started","data":{"taker_coin":"MYCOIN1","maker_coin":"MYCOIN","maker":"987d5f82205a55d789616f470ae9df48537f050ee050d501aa316651642a0a4d","my_persistent_pub":"02859a80b83941e4e8ff2f511080e3ea5021db4ba95caec30eb37864e71ae73521","lock_duration":7800,"maker_amount":"999.99999","taker_amount":"999.99999","maker_payment_confirmations":1,"maker_payment_requires_nota":false,"taker_payment_confirmations":1,"taker_payment_requires_nota":false,"taker_payment_lock":1588251836,"uuid":"5acb0e63-8b26-469e-81df-7dd9e4a9ad15","started_at":1588244036,"maker_payment_wait":1588247156,"maker_coin_start_block":12,"taker_coin_start_block":11}}},{"timestamp":1588244038239,"event":{"type":"Negotiated","data":{"maker_payment_locktime":1588259636,"maker_pubkey":"02987d5f82205a55d789616f470ae9df48537f050ee050d501aa316651642a0a4d","secret_hash":"9304bce3196f344b2a22dc99db406e95ab6f3107"}}},{"timestamp":1588244038271,"event":{"type":"TakerFeeSent","data":{"tx_hex":"0400008085202f8901bdde9bca02870787441f6068e4c2a869a3aac3d1d0925f6a6e27874343544d0a010000006a47304402206694a794693b55fbe8205cb1cbb992d92fa2a9a851763ad1bf1628c16deaf73e02203cfff465504bdd6c51e8fbd45dd2e1187142fdc29e82f36f577616d9d6097d7a012102859a80b83941e4e8ff2f511080e3ea5021db4ba95caec30eb37864e71ae73521ffffffff02dfceab07000000001976a914ca1e04745e8ca0c60d8c5881531d51bec470743f88ac39fd41892e0000001976a914d24e799df360da3ca3158d63b89ffaff27722c1588ac46aeaa5e000000000000000000000000000000","tx_hash":"7118d7484d1cbfdd6126673a848a228856f8748d946f6a9a440c90f0d62e27c6","from":["RUTBzLtJNTn89Wkb6oZocbesKrjBDTRMrC"],"to":["RThtXup6Zo7LZAi8kRWgjAyi1s4u6U9Cpf","RUTBzLtJNTn89Wkb6oZocbesKrjBDTRMrC"],"total_amount":"2000","spent_by_me":"2000","received_by_me":"1998.71298873","my_balance_change":"-1.28701127","block_height":0,"timestamp":0,"fee_details":{"amount":"0.00001"},"coin":"MYCOIN1","internal_id":"7118d7484d1cbfdd6126673a848a228856f8748d946f6a9a440c90f0d62e27c6"}}}],"maker_amount":"999.99999","maker_coin":"MYCOIN","taker_amount":"999.99999","taker_coin":"MYCOIN1","gui":"nogui","mm_version":"UNKNOWN","success_events":["Started","Negotiated","TakerFeeSent","MakerPaymentReceived","MakerPaymentWaitConfirmStarted","MakerPaymentValidatedAndConfirmed","TakerPaymentSent","TakerPaymentSpent","MakerPaymentSpent","Finished"],"error_events":["StartFailed","NegotiateFailed","TakerFeeSendFailed","MakerPaymentValidateFailed","MakerPaymentWaitConfirmFailed","TakerPaymentTransactionFailed","TakerPaymentWaitConfirmFailed","TakerPaymentDataSendFailed","TakerPaymentWaitForSpendFailed","MakerPaymentSpendFailed","TakerPaymentWaitRefundStarted","TakerPaymentRefunded","TakerPaymentRefundFailed"]}"#; | ||
const FINISHED_TAKER_SWAP: &str = r#"{"type":"Taker","uuid":"5acb0e63-8b26-469e-81df-7dd9e4a9ad15","events":[{"timestamp":1588244036253,"event":{"type":"Started","data":{"taker_coin":"MYCOIN1","maker_coin":"MYCOIN","maker":"987d5f82205a55d789616f470ae9df48537f050ee050d501aa316651642a0a4d","my_persistent_pub":"02859a80b83941e4e8ff2f511080e3ea5021db4ba95caec30eb37864e71ae73521","lock_duration":7800,"maker_amount":"999.99999","taker_amount":"999.99999","maker_payment_confirmations":1,"maker_payment_requires_nota":false,"taker_payment_confirmations":1,"taker_payment_requires_nota":false,"taker_payment_lock":1588251836,"uuid":"5acb0e63-8b26-469e-81df-7dd9e4a9ad15","started_at":1588244036,"maker_payment_wait":1588247156,"maker_coin_start_block":12,"taker_coin_start_block":11}}},{"timestamp":1588244038239,"event":{"type":"Negotiated","data":{"maker_payment_locktime":1588259636,"maker_pubkey":"02987d5f82205a55d789616f470ae9df48537f050ee050d501aa316651642a0a4d","secret_hash":"9304bce3196f344b2a22dc99db406e95ab6f3107"}}},{"timestamp":1588244038271,"event":{"type":"TakerFeeSent","data":{"tx_hex":"0400008085202f8901bdde9bca02870787441f6068e4c2a869a3aac3d1d0925f6a6e27874343544d0a010000006a47304402206694a794693b55fbe8205cb1cbb992d92fa2a9a851763ad1bf1628c16deaf73e02203cfff465504bdd6c51e8fbd45dd2e1187142fdc29e82f36f577616d9d6097d7a012102859a80b83941e4e8ff2f511080e3ea5021db4ba95caec30eb37864e71ae73521ffffffff02dfceab07000000001976a914ca1e04745e8ca0c60d8c5881531d51bec470743f88ac39fd41892e0000001976a914d24e799df360da3ca3158d63b89ffaff27722c1588ac46aeaa5e000000000000000000000000000000","tx_hash":"7118d7484d1cbfdd6126673a848a228856f8748d946f6a9a440c90f0d62e27c6","from":["RUTBzLtJNTn89Wkb6oZocbesKrjBDTRMrC"],"to":["RThtXup6Zo7LZAi8kRWgjAyi1s4u6U9Cpf","RUTBzLtJNTn89Wkb6oZocbesKrjBDTRMrC"],"total_amount":"2000","spent_by_me":"2000","received_by_me":"1998.71298873","my_balance_change":"-1.28701127","block_height":0,"timestamp":0,"fee_details":{"amount":"0.00001"},"coin":"MYCOIN1","internal_id":"7118d7484d1cbfdd6126673a848a228856f8748d946f6a9a440c90f0d62e27c6"}}},{"timestamp":1588244038698,"event":{"type":"MakerPaymentReceived","data":{"tx_hex":"0400008085202f890163cc0aceb3b432f84c1407991a0389b74b7842f030aa261203aa0b4b9a9a15fd010000006b483045022100f3239794b7b0e1c75aae084a65535270f154231ce86a4739976ff69eeff1ebd402206c0aeb28b7b4b2e77e98b3c3efd9a20d85c2cd0627acc3f45d577c0e88c34fb4012102987d5f82205a55d789616f470ae9df48537f050ee050d501aa316651642a0a4dffffffff0118e476481700000017a914dc4ed4686174503b85374bfb0aefe07a9fb37bcf8746aeaa5e000000000000000000000000000000","tx_hash":"9a4c0b3f85ed0bb24dc9575ce7c2fd6bc50ad9d37c91c478946f9c33d15abfdf","from":["RAzSdYQhjCFdyhjrBz1AZQDVg3Hu8DrzYc"],"to":["bYp9ncp3V7FYsymipriVbdd3QL72hK9hio"],"total_amount":"1000","spent_by_me":"0","received_by_me":"0","my_balance_change":"0","block_height":0,"timestamp":0,"fee_details":{"amount":"0.00001"},"coin":"MYCOIN","internal_id":"9a4c0b3f85ed0bb24dc9575ce7c2fd6bc50ad9d37c91c478946f9c33d15abfdf"}}},{"timestamp":1588244038699,"event":{"type":"MakerPaymentWaitConfirmStarted"}},{"timestamp":1588244053712,"event":{"type":"MakerPaymentValidatedAndConfirmed"}},{"timestamp":1588244053745,"event":{"type":"TakerPaymentSent","data":{"tx_hex":"0400008085202f8901c6272ed6f0900c449a6a6f948d74f85688228a843a672661ddbf1c4d48d71871010000006b483045022100c37627385c66b7bdf466b4dd4e7095b7551e6f5ea35f9bcc6344eb629f2edcb202203280eaba64b4d72010500166fab62cf34a687a516b2fe83d4eceaf8572cb37a7012102859a80b83941e4e8ff2f511080e3ea5021db4ba95caec30eb37864e71ae73521ffffffff0218e476481700000017a91431ff75ed72cd135a7ce50d121e71efc37066b9f9873915cb40170000001976a914d24e799df360da3ca3158d63b89ffaff27722c1588ac55aeaa5e000000000000000000000000000000","tx_hash":"b4718ce94aa43f9073ab0b70c18ab4ea4b587338fb7110f20ff7d1bb452df08f","from":["RUTBzLtJNTn89Wkb6oZocbesKrjBDTRMrC"],"to":["RUTBzLtJNTn89Wkb6oZocbesKrjBDTRMrC","bHHdqM8XWDHee2oyzydwUJKEE2BjofEtZH"],"total_amount":"1998.71298873","spent_by_me":"1998.71298873","received_by_me":"998.71298873","my_balance_change":"-1000","block_height":0,"timestamp":0,"fee_details":{"amount":"0.00001"},"coin":"MYCOIN1","internal_id":"b4718ce94aa43f9073ab0b70c18ab4ea4b587338fb7110f20ff7d1bb452df08f"}}},{"timestamp":1588244078860,"event":{"type":"TakerPaymentSpent","data":{"transaction":{"tx_hex":"0400008085202f89018ff02d45bbd1f70ff21071fb3873584beab48ac1700bab73903fa44ae98c71b400000000d747304402204f0d641a3916e54d6788744c3229110a431eff18634c66fbd1741f9ca7dba99d02202315ee1d9317cc4d5d75d01f066f2c8a59876f790106d310144cdc03c25f985e0120dedf3c8dcfff9ee2787b4bf211f960fd044fdab7fa8e922ef613a0115848a498004c6b6304bcccaa5eb1752102859a80b83941e4e8ff2f511080e3ea5021db4ba95caec30eb37864e71ae73521ac6782012088a9149304bce3196f344b2a22dc99db406e95ab6f3107882102987d5f82205a55d789616f470ae9df48537f050ee050d501aa316651642a0a4dac68ffffffff0130e07648170000001976a91412c553e8469363f2d30268c475af1e9186cc90af88ac54a0aa5e000000000000000000000000000000","tx_hash":"e7aed7a77e47b44dc9d12166589bbade70faea10b64888f73ed4be04bcc9f9a9","from":["bHHdqM8XWDHee2oyzydwUJKEE2BjofEtZH"],"to":["RAzSdYQhjCFdyhjrBz1AZQDVg3Hu8DrzYc"],"total_amount":"999.99999","spent_by_me":"0","received_by_me":"0","my_balance_change":"0","block_height":29,"timestamp":1588244070,"fee_details":{"amount":"0.00001"},"coin":"MYCOIN1","internal_id":"e7aed7a77e47b44dc9d12166589bbade70faea10b64888f73ed4be04bcc9f9a9"},"secret":"dedf3c8dcfff9ee2787b4bf211f960fd044fdab7fa8e922ef613a0115848a498"}}},{"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"}}},{"timestamp":1588244078871,"event":{"type":"Finished"}}],"maker_amount":"999.99999","maker_coin":"MYCOIN","taker_amount":"999.99999","taker_coin":"MYCOIN1","gui":"nogui","mm_version":"UNKNOWN","success_events":["Started","Negotiated","TakerFeeSent","MakerPaymentReceived","MakerPaymentWaitConfirmStarted","MakerPaymentValidatedAndConfirmed","TakerPaymentSent","TakerPaymentSpent","MakerPaymentSpent","Finished"],"error_events":["StartFailed","NegotiateFailed","TakerFeeSendFailed","MakerPaymentValidateFailed","MakerPaymentWaitConfirmFailed","TakerPaymentTransactionFailed","TakerPaymentWaitConfirmFailed","TakerPaymentDataSendFailed","TakerPaymentWaitForSpendFailed","MakerPaymentSpendFailed","TakerPaymentWaitRefundStarted","TakerPaymentRefunded","TakerPaymentRefundFailed"]}"#; | ||
const UNFINISHED_TAKER_SWAP: &str = r#"{"type":"Taker","uuid":"5acb0e63-8b26-469e-81df-7dd9e4a9ad15","events":[{"timestamp":1588244036253,"event":{"type":"Started","data":{"taker_coin":"MYCOIN1","maker_coin":"MYCOIN","maker":"987d5f82205a55d789616f470ae9df48537f050ee050d501aa316651642a0a4d","my_persistent_pub":"02859a80b83941e4e8ff2f511080e3ea5021db4ba95caec30eb37864e71ae73521","lock_duration":7800,"maker_amount":"999.99999","taker_amount":"999.99999","maker_payment_confirmations":1,"maker_payment_requires_nota":false,"taker_payment_confirmations":1,"taker_payment_requires_nota":false,"taker_payment_lock":1588251836,"uuid":"5acb0e63-8b26-469e-81df-7dd9e4a9ad15","started_at":1588244036,"maker_payment_wait":1588247156,"maker_coin_start_block":12,"taker_coin_start_block":11}}},{"timestamp":1588244038239,"event":{"type":"Negotiated","data":{"maker_payment_locktime":1588259636,"maker_pubkey":"02987d5f82205a55d789616f470ae9df48537f050ee050d501aa316651642a0a4d","secret_hash":"9304bce3196f344b2a22dc99db406e95ab6f3107"}}},{"timestamp":1588244038271,"event":{"type":"TakerFeeSent","data":{"tx_hex":"0400008085202f8901bdde9bca02870787441f6068e4c2a869a3aac3d1d0925f6a6e27874343544d0a010000006a47304402206694a794693b55fbe8205cb1cbb992d92fa2a9a851763ad1bf1628c16deaf73e02203cfff465504bdd6c51e8fbd45dd2e1187142fdc29e82f36f577616d9d6097d7a012102859a80b83941e4e8ff2f511080e3ea5021db4ba95caec30eb37864e71ae73521ffffffff02dfceab07000000001976a914ca1e04745e8ca0c60d8c5881531d51bec470743f88ac39fd41892e0000001976a914d24e799df360da3ca3158d63b89ffaff27722c1588ac46aeaa5e000000000000000000000000000000","tx_hash":"7118d7484d1cbfdd6126673a848a228856f8748d946f6a9a440c90f0d62e27c6","from":["RUTBzLtJNTn89Wkb6oZocbesKrjBDTRMrC"],"to":["RThtXup6Zo7LZAi8kRWgjAyi1s4u6U9Cpf","RUTBzLtJNTn89Wkb6oZocbesKrjBDTRMrC"],"total_amount":"2000","spent_by_me":"2000","received_by_me":"1998.71298873","my_balance_change":"-1.28701127","block_height":0,"timestamp":0,"fee_details":{"amount":"0.00001"},"coin":"MYCOIN1","internal_id":"7118d7484d1cbfdd6126673a848a228856f8748d946f6a9a440c90f0d62e27c6"}}}],"maker_amount":"999.99999","maker_coin":"MYCOIN","taker_amount":"999.99999","taker_coin":"MYCOIN1","gui":"nogui","mm_version":"UNKNOWN","success_events":["Started","Negotiated","TakerFeeSent","MakerPaymentReceived","MakerPaymentWaitConfirmStarted","MakerPaymentValidatedAndConfirmed","TakerPaymentSent","TakerPaymentSpent","MakerPaymentSpent","MakerPaymentSpendConfirmed","Finished"],"error_events":["StartFailed","NegotiateFailed","TakerFeeSendFailed","MakerPaymentValidateFailed","MakerPaymentWaitConfirmFailed","TakerPaymentTransactionFailed","TakerPaymentWaitConfirmFailed","TakerPaymentDataSendFailed","TakerPaymentWaitForSpendFailed","MakerPaymentSpendFailed","TakerPaymentWaitRefundStarted","TakerPaymentRefunded","TakerPaymentRefundFailed"]}"#; | ||
const FINISHED_TAKER_SWAP: &str = r#"{"type":"Taker","uuid":"5acb0e63-8b26-469e-81df-7dd9e4a9ad15","events":[{"timestamp":1588244036253,"event":{"type":"Started","data":{"taker_coin":"MYCOIN1","maker_coin":"MYCOIN","maker":"987d5f82205a55d789616f470ae9df48537f050ee050d501aa316651642a0a4d","my_persistent_pub":"02859a80b83941e4e8ff2f511080e3ea5021db4ba95caec30eb37864e71ae73521","lock_duration":7800,"maker_amount":"999.99999","taker_amount":"999.99999","maker_payment_confirmations":1,"maker_payment_requires_nota":false,"taker_payment_confirmations":1,"taker_payment_requires_nota":false,"taker_payment_lock":1588251836,"uuid":"5acb0e63-8b26-469e-81df-7dd9e4a9ad15","started_at":1588244036,"maker_payment_wait":1588247156,"maker_coin_start_block":12,"taker_coin_start_block":11}}},{"timestamp":1588244038239,"event":{"type":"Negotiated","data":{"maker_payment_locktime":1588259636,"maker_pubkey":"02987d5f82205a55d789616f470ae9df48537f050ee050d501aa316651642a0a4d","secret_hash":"9304bce3196f344b2a22dc99db406e95ab6f3107"}}},{"timestamp":1588244038271,"event":{"type":"TakerFeeSent","data":{"tx_hex":"0400008085202f8901bdde9bca02870787441f6068e4c2a869a3aac3d1d0925f6a6e27874343544d0a010000006a47304402206694a794693b55fbe8205cb1cbb992d92fa2a9a851763ad1bf1628c16deaf73e02203cfff465504bdd6c51e8fbd45dd2e1187142fdc29e82f36f577616d9d6097d7a012102859a80b83941e4e8ff2f511080e3ea5021db4ba95caec30eb37864e71ae73521ffffffff02dfceab07000000001976a914ca1e04745e8ca0c60d8c5881531d51bec470743f88ac39fd41892e0000001976a914d24e799df360da3ca3158d63b89ffaff27722c1588ac46aeaa5e000000000000000000000000000000","tx_hash":"7118d7484d1cbfdd6126673a848a228856f8748d946f6a9a440c90f0d62e27c6","from":["RUTBzLtJNTn89Wkb6oZocbesKrjBDTRMrC"],"to":["RThtXup6Zo7LZAi8kRWgjAyi1s4u6U9Cpf","RUTBzLtJNTn89Wkb6oZocbesKrjBDTRMrC"],"total_amount":"2000","spent_by_me":"2000","received_by_me":"1998.71298873","my_balance_change":"-1.28701127","block_height":0,"timestamp":0,"fee_details":{"amount":"0.00001"},"coin":"MYCOIN1","internal_id":"7118d7484d1cbfdd6126673a848a228856f8748d946f6a9a440c90f0d62e27c6"}}},{"timestamp":1588244038698,"event":{"type":"MakerPaymentReceived","data":{"tx_hex":"0400008085202f890163cc0aceb3b432f84c1407991a0389b74b7842f030aa261203aa0b4b9a9a15fd010000006b483045022100f3239794b7b0e1c75aae084a65535270f154231ce86a4739976ff69eeff1ebd402206c0aeb28b7b4b2e77e98b3c3efd9a20d85c2cd0627acc3f45d577c0e88c34fb4012102987d5f82205a55d789616f470ae9df48537f050ee050d501aa316651642a0a4dffffffff0118e476481700000017a914dc4ed4686174503b85374bfb0aefe07a9fb37bcf8746aeaa5e000000000000000000000000000000","tx_hash":"9a4c0b3f85ed0bb24dc9575ce7c2fd6bc50ad9d37c91c478946f9c33d15abfdf","from":["RAzSdYQhjCFdyhjrBz1AZQDVg3Hu8DrzYc"],"to":["bYp9ncp3V7FYsymipriVbdd3QL72hK9hio"],"total_amount":"1000","spent_by_me":"0","received_by_me":"0","my_balance_change":"0","block_height":0,"timestamp":0,"fee_details":{"amount":"0.00001"},"coin":"MYCOIN","internal_id":"9a4c0b3f85ed0bb24dc9575ce7c2fd6bc50ad9d37c91c478946f9c33d15abfdf"}}},{"timestamp":1588244038699,"event":{"type":"MakerPaymentWaitConfirmStarted"}},{"timestamp":1588244053712,"event":{"type":"MakerPaymentValidatedAndConfirmed"}},{"timestamp":1588244053745,"event":{"type":"TakerPaymentSent","data":{"tx_hex":"0400008085202f8901c6272ed6f0900c449a6a6f948d74f85688228a843a672661ddbf1c4d48d71871010000006b483045022100c37627385c66b7bdf466b4dd4e7095b7551e6f5ea35f9bcc6344eb629f2edcb202203280eaba64b4d72010500166fab62cf34a687a516b2fe83d4eceaf8572cb37a7012102859a80b83941e4e8ff2f511080e3ea5021db4ba95caec30eb37864e71ae73521ffffffff0218e476481700000017a91431ff75ed72cd135a7ce50d121e71efc37066b9f9873915cb40170000001976a914d24e799df360da3ca3158d63b89ffaff27722c1588ac55aeaa5e000000000000000000000000000000","tx_hash":"b4718ce94aa43f9073ab0b70c18ab4ea4b587338fb7110f20ff7d1bb452df08f","from":["RUTBzLtJNTn89Wkb6oZocbesKrjBDTRMrC"],"to":["RUTBzLtJNTn89Wkb6oZocbesKrjBDTRMrC","bHHdqM8XWDHee2oyzydwUJKEE2BjofEtZH"],"total_amount":"1998.71298873","spent_by_me":"1998.71298873","received_by_me":"998.71298873","my_balance_change":"-1000","block_height":0,"timestamp":0,"fee_details":{"amount":"0.00001"},"coin":"MYCOIN1","internal_id":"b4718ce94aa43f9073ab0b70c18ab4ea4b587338fb7110f20ff7d1bb452df08f"}}},{"timestamp":1588244078860,"event":{"type":"TakerPaymentSpent","data":{"transaction":{"tx_hex":"0400008085202f89018ff02d45bbd1f70ff21071fb3873584beab48ac1700bab73903fa44ae98c71b400000000d747304402204f0d641a3916e54d6788744c3229110a431eff18634c66fbd1741f9ca7dba99d02202315ee1d9317cc4d5d75d01f066f2c8a59876f790106d310144cdc03c25f985e0120dedf3c8dcfff9ee2787b4bf211f960fd044fdab7fa8e922ef613a0115848a498004c6b6304bcccaa5eb1752102859a80b83941e4e8ff2f511080e3ea5021db4ba95caec30eb37864e71ae73521ac6782012088a9149304bce3196f344b2a22dc99db406e95ab6f3107882102987d5f82205a55d789616f470ae9df48537f050ee050d501aa316651642a0a4dac68ffffffff0130e07648170000001976a91412c553e8469363f2d30268c475af1e9186cc90af88ac54a0aa5e000000000000000000000000000000","tx_hash":"e7aed7a77e47b44dc9d12166589bbade70faea10b64888f73ed4be04bcc9f9a9","from":["bHHdqM8XWDHee2oyzydwUJKEE2BjofEtZH"],"to":["RAzSdYQhjCFdyhjrBz1AZQDVg3Hu8DrzYc"],"total_amount":"999.99999","spent_by_me":"0","received_by_me":"0","my_balance_change":"0","block_height":29,"timestamp":1588244070,"fee_details":{"amount":"0.00001"},"coin":"MYCOIN1","internal_id":"e7aed7a77e47b44dc9d12166589bbade70faea10b64888f73ed4be04bcc9f9a9"},"secret":"dedf3c8dcfff9ee2787b4bf211f960fd044fdab7fa8e922ef613a0115848a498"}}},{"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"}}},{"timestamp":1588244078871,"event":{"type":"Finished"}}],"maker_amount":"999.99999","maker_coin":"MYCOIN","taker_amount":"999.99999","taker_coin":"MYCOIN1","gui":"nogui","mm_version":"UNKNOWN","success_events":["Started","Negotiated","TakerFeeSent","MakerPaymentReceived","MakerPaymentWaitConfirmStarted","MakerPaymentValidatedAndConfirmed","TakerPaymentSent","TakerPaymentSpent","MakerPaymentSpent","MakerPaymentSpendConfirmed","Finished"],"error_events":["StartFailed","NegotiateFailed","TakerFeeSendFailed","MakerPaymentValidateFailed","MakerPaymentWaitConfirmFailed","TakerPaymentTransactionFailed","TakerPaymentWaitConfirmFailed","TakerPaymentDataSendFailed","TakerPaymentWaitForSpendFailed","MakerPaymentSpendFailed","TakerPaymentWaitRefundStarted","TakerPaymentRefunded","TakerPaymentRefundFailed"]}"#; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added "MakerPaymentSpendConfirmed" event ed91569
@@ -49,6 +49,7 @@ pub async fn get_command_based_on_maker_or_watcher_activity( | |||
Err(e) => ERR!("Error {} when trying to find taker payment spend", e), | |||
}, | |||
TakerSwapCommand::SpendMakerPayment => check_maker_payment_spend_and_add_event(ctx, swap, saved).await, | |||
TakerSwapCommand::ConfirmMakerPaymentSpend => Ok(command), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
Ok(TakerSwapCommand::Finish) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
c.c. @dimxy
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's better to fix also get_command() to ensure MakerPaymentSpentByWatcher event triggers ConfirmMakerPaymentSpend.
yep, if we want to return
TakerSwapCommand::ConfirmMakerPaymentSpend
incheck_maker_payment_spend_and_add_event
, then we also need to cover confirmation step inget_command
forMakerPaymentSpentByWatcher
Hmm it means that we also need to update fn is_recoverable
according to the same note as here #2199 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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) |
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We might need to handle it here too
TakerSwapEvent::MakerPaymentSpentByWatcher(_) => Some(TakerSwapCommand::Finish), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's used for utxo <-> utxo swap in production.
Got it, then I also update MakerPaymentSpentByWatcher cases
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We might need to handle it here too
TakerSwapEvent::MakerPaymentSpentByWatcher(_) => Some(TakerSwapCommand::Finish),
yep, its what dimxy was referencing about
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. Covered TakerSwapCommand::ConfirmMakerPaymentSpend
for restart and watchers here 0661ee2
And good to have a new swap watcher test for this to ensure the flow works okay
@dimxy new events matching in get_command
function already covered in watcher (restart) tests in swap_watchers_test.rs
, when we compare expected event list with actual using check_actual_events
function
fn check_actual_events(mm_alice: &MarketMakerIt, uuid: &str, expected_events: &[&'static str]) -> Value { |
for example I removed "MakerPaymentSpendConfirmed"
event from expected_events
list in test_taker_saves_the_swap_as_successful_after_restart_panic_at_maker_payment_spend
test
and got err https://github.com/laruh/atomicDEX-API/actions/runs/11091593379/job/30815533174#step:6:3409
thread 'docker_tests::swap_watcher_tests::test_taker_saves_the_swap_as_successful_after_restart_panic_at_maker_payment_spend' panicked at 'assertion failed: `(left == right)`
left: `["Started", "Negotiated", "TakerFeeSent", "TakerPaymentInstructionsReceived", "MakerPaymentReceived", "MakerPaymentWaitConfirmStarted", "MakerPaymentValidatedAndConfirmed", "TakerPaymentSent", "WatcherMessageSent", "TakerPaymentSpent", "MakerPaymentSpentByWatcher", "Finished"]`,
right: `["Started", "Negotiated", "TakerFeeSent", "TakerPaymentInstructionsReceived", "MakerPaymentReceived", "MakerPaymentWaitConfirmStarted", "MakerPaymentValidatedAndConfirmed", "TakerPaymentSent", "WatcherMessageSent", "TakerPaymentSpent", "MakerPaymentSpentByWatcher", "MakerPaymentSpendConfirmed", "Finished"]`', mm2src/mm2_main/tests/docker_tests/swap_watcher_tests.rs:269:5
This reverts commit 2c119be
4462a41
to
0073fe8
Compare
// we should wait for only one confirmation to make sure our spend transaction is not failed | ||
let confirm_maker_payment_spend_input = ConfirmPaymentInput { | ||
payment_tx: self.r().maker_payment_spend.clone().unwrap().tx_hex.0, | ||
confirmations: std::cmp::min(1, self.r().data.maker_payment_confirmations), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can maker_payment_confirmations be 0?
Should 'confirmations' be also 0 in this case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
@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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I will update we should wait for only one confirmation...
comment then, to make it clear that we also allow 0 confirmations
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added note f93fcde
… in is_recoverable function
cd75bd0
to
aed8d4c
Compare
d2fd10f
to
5ce137f
Compare
…nfirm_taker_payment_spend
related to #2175 (comment)