Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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?
fix(legacy-swap): taker failed spend maker payment marked as failed #2199
Changes from 23 commits
1461212
31292be
0ede740
9c6c892
b716806
5c47ef0
3117c5f
eca0bda
0e9a807
eb16235
1a1a775
3af96a5
1c3c935
7829b4b
075de97
cc7c963
69ed68e
b2446b9
2c119be
b9ba150
165ddfe
b50b2a3
6fa527a
0073fe8
aed8d4c
ed91569
0661ee2
5ce137f
f93fcde
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 ofTakerSwapCommand::Finish
herekomodo-defi-framework/mm2src/mm2_main/src/lp_swap/taker_restart.rs
Line 145 in 1c3c935
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.
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
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.
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.
It will be done here
komodo-defi-framework/mm2src/mm2_main/src/lp_swap/taker_restart.rs
Lines 135 to 145 in 1c3c935
Yes, I suppose.
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
komodo-defi-framework/mm2src/mm2_main/src/lp_swap/taker_swap.rs
Line 187 in 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.
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.
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@dimxy new events matching in
get_command
function already covered in watcher (restart) tests inswap_watchers_test.rs
, when we compare expected event list with actual usingcheck_actual_events
functionkomodo-defi-framework/mm2src/mm2_main/tests/docker_tests/swap_watcher_tests.rs
Line 264 in 0661ee2
for example I removed
"MakerPaymentSpendConfirmed"
event fromexpected_events
list intest_taker_saves_the_swap_as_successful_after_restart_panic_at_maker_payment_spend
testand got err https://github.com/laruh/atomicDEX-API/actions/runs/11091593379/job/30815533174#step:6:3409