forked from jl777/SuperNET
-
Notifications
You must be signed in to change notification settings - Fork 94
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
Open
laruh
wants to merge
27
commits into
dev
Choose a base branch
from
taker-failed-swaps-marked-successful
base: dev
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
27 commits
Select commit
Hold shift + click to select a range
1461212
WIP: new fn confirm_maker_payment_spend, Taker command and events
laruh 31292be
Merge remote-tracking branch 'origin/dev' into taker-failed-swaps-mar…
laruh 0ede740
update events in recreate_taker_swap_taker_expected.json
laruh 9c6c892
update events in recreate_taker_swap_taker_payment_wait_confirm_faile…
laruh b716806
update events in for_tests.rs consts, taker_swap.rs mod taker_swap_te…
laruh 5c47ef0
add "MakerPaymentSpendConfirmed" to TAKER_ACTUAL_EVENTS_TAKER_SPENDS_…
laruh 3117c5f
Merge remote-tracking branch 'origin/dev' into taker-failed-swaps-mar…
laruh eca0bda
provide "MakerPaymentSpendConfirmed" success event in more test jsons
laruh 0e9a807
log events
laruh eb16235
TakerSwapEvent::MakerPaymentSpentByWatcher(_) => Some(TakerSwapComman…
laruh 1a1a775
in taker_restart.rs provide TakerSwapCommand::ConfirmMakerPaymentSpen…
laruh 3af96a5
move TakerSwapEvent::MakerPaymentSpendConfirmFailed in MakerSavedEvent
laruh 1c3c935
use TakerSwapCommand::SpendMakerPayment => check_maker_payment_spend_…
laruh 7829b4b
return TakerSwapEvent::MakerPaymentSpentByWatcher(_) => Some(TakerSwa…
laruh 075de97
add "MakerPaymentSpendConfirmed" to TAKER_USING_WATCHERS_SUCCESS_EVENTS
laruh cc7c963
add "MakerPaymentSpendConfirmed" to TAKER_USING_WATCHERS_SUCCESS_EVEN…
laruh 69ed68e
remove logs
laruh b2446b9
self.r().data.maker_payment_requires_nota.unwrap_or(false)
laruh 2c119be
provide number of confirmations in wait_for_confirmations
laruh b9ba150
Merge remote-tracking branch 'origin/dev' into taker-failed-swaps-mar…
laruh 165ddfe
provide requires_nota:false in confirm_maker_payment_spend
laruh b50b2a3
review: make doc com of wait_for_confirmations clearer
laruh 6fa527a
Merge remote-tracking branch 'origin/dev' into taker-failed-swaps-mar…
laruh 0073fe8
Revert "provide number of confirmations in wait_for_confirmations"
laruh aed8d4c
review: for old saved swaps check "TakerSwapEvent::MakerPaymentSpent"…
laruh ed91569
review: add "MakerPaymentSpendConfirmed" event in const FINISHED_TAKE…
laruh 0661ee2
review: cover ConfirmMakerPaymentSpend for MakerPaymentSpentByWatcher
laruh File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Large diffs are not rendered by default.
Oops, something went wrong.
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Large diffs are not rendered by default.
Oops, something went wrong.
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
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.
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.
@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 legacyget_command()
functionThere 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