Skip to content
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

[r2r] UTXO swap watcher further improvements #1552

Merged
merged 166 commits into from
Dec 12, 2022
Merged

[r2r] UTXO swap watcher further improvements #1552

merged 166 commits into from
Dec 12, 2022

Conversation

caglaryucekaya
Copy link

@caglaryucekaya caglaryucekaya commented Nov 21, 2022

Further improvements, validations, bugfixes and test cases for the UTXO swap watchers

@caglaryucekaya caglaryucekaya changed the title [wip] UTXO swap watcher further improvements [r2r] UTXO swap watcher further improvements Dec 3, 2022
Copy link

@sergeyboyko0791 sergeyboyko0791 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the fixes! I have only one note :)

mm2src/mm2_main/src/lp_swap/swap_watcher.rs Outdated Show resolved Hide resolved
Copy link
Member

@artemii235 artemii235 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One last note.

Comment on lines 220 to 223
"wait_for_maker_payment_spend_coefficient": wait_for_maker_payment_spend_coefficient,
"refund_start_time_coefficient": refund_start_time_coefficient,
"search_interval": search_interval,
"wait_for_taker_payment_duration": wait_for_taker_payment_duration
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In #1552 (comment) it was requested to create watcher_conf object and move all these fields there 🙂 Then, we can use SwapWatcherConf struct that will have #[serde(default = "fn_name")] for the missing fields case (I would also rename some fields to make them shorter a bit):

fn one_and_half() -> f64 { 1.5 }

fn three_hundred() -> f64 { 300. }

fn sixty() -> f64 { 60. }

#[derive(Deserialize)]
struct SwapWatcherConf {
    #[serde(default = "one_and_half")]
    wait_for_maker_payment_spend_factor: f64,
    #[serde(default = "one_and_half")]
    refund_start_time_factor: f64,
    #[serde(default = "three_hundred")]
    search_interval: f64,
    #[serde(default = "sixty")]
    wait_for_taker_payment_duration: f64,
}

Copy link
Collaborator

@shamardy shamardy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only one question :)

mm2src/mm2_main/src/lp_swap/swap_watcher.rs Outdated Show resolved Hide resolved
Copy link
Member

@laruh laruh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great progress!
Just have one question.

mm2src/mm2_main/src/lp_swap/swap_watcher.rs Show resolved Hide resolved
@caglaryucekaya caglaryucekaya changed the title [r2r] UTXO swap watcher further improvements [wip] UTXO swap watcher further improvements Dec 5, 2022
@caglaryucekaya caglaryucekaya changed the title [wip] UTXO swap watcher further improvements [r2r] UTXO swap watcher further improvements Dec 5, 2022
@caglaryucekaya
Copy link
Author

Before this is merged, there's one more point I want to ask. In this PR I added a new parameter to the wait_for_htlc_tx_spend method, check_every, to reduce the frequency of output spend checks for watchers. Normally it was once in ten seconds everywhere, except for ethereum in which it was once in five seconds:

https://github.com/KomodoPlatform/atomicDEX-API/blob/a6046fd79af444b7a1f83eb7d388bd8f8c85bcb8/mm2src/coins/eth.rs#L1497-L1506

Now I made it all once in ten seconds with a constant:

https://github.com/KomodoPlatform/atomicDEX-API/blob/dec9c70563aaa84cae8ce8d119643ed6fe0b8067/mm2src/mm2_main/src/lp_swap/taker_swap.rs#L41

Is this important?

shamardy
shamardy previously approved these changes Dec 6, 2022
Copy link
Collaborator

@shamardy shamardy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@artemii235
Copy link
Member

Normally it was once in ten seconds everywhere, except for ethereum in which it was once in five seconds:
Is this important?

@caglaryucekaya That's ok 🙂

Copy link
Member

@artemii235 artemii235 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One more note 🙂

}
}

#[derive(Serialize, Deserialize, Default)]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With derived Default, we get the following:

    let conf = WatcherConf::default();
    println!("{:?}", conf);

WatcherConf { wait_taker_payment: 0.0, wait_maker_payment_spend_factor: 0.0, refund_start_factor: 0.0, search_interval: 0.0 }

I suppose it's not what we expect to get here:
https://github.com/KomodoPlatform/atomicDEX-API/blob/dec9c70563aaa84cae8ce8d119643ed6fe0b8067/mm2src/mm2_main/src/lp_swap/swap_watcher.rs#L580

Could you please implement Default manually instead, using appropriate values?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this is resolved

@caglaryucekaya caglaryucekaya changed the title [r2r] UTXO swap watcher further improvements [wip] UTXO swap watcher further improvements Dec 8, 2022
@caglaryucekaya caglaryucekaya changed the title [wip] UTXO swap watcher further improvements [r2r] UTXO swap watcher further improvements Dec 8, 2022
Copy link

@sergeyboyko0791 sergeyboyko0791 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-approve

@artemii235 artemii235 merged commit 2e72027 into dev Dec 12, 2022
@artemii235 artemii235 deleted the watchtower branch December 12, 2022 04:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants