-
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
[r2r] Swap watcher node basic functionality #1457
Conversation
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.
Looks great. thanks
Just couple questions..and suggestions
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 PR! First review iteration 🙂
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.
Great work!
First review iteration
fn watcher_validate_taker_payment( | ||
&self, | ||
_input: WatcherValidatePaymentInput, | ||
) -> Box<dyn Future<Item = (), Error = String> + Send>; |
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.
Could you please add corresponding WatcherValidatePayment
error?
Probably at the next sprint.
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.
Amazing work 🔥 My first review iteration 🙂
mm2src/coins/utxo/utxo_common.rs
Outdated
.get(0) | ||
.ok_or("Transaction doesn't have any outputs")?) |
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.
How about doing this validation as the first step of the function(no need create lock_time, n_time, etc. if the input is invalid anyway) and leave here as input.prev_transaction.outputs[0].value
?
) -> TransactionFut { | ||
let my_address = try_tx_fus!(coin.as_ref().derivation_method.iguana_or_err()).clone(); | ||
let mut prev_transaction: UtxoTx = try_tx_fus!(deserialize(maker_payment_tx).map_err(|e| ERRL!("{:?}", e))); | ||
prev_transaction.tx_hash_algo = coin.as_ref().tx_hash_algo; |
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.
You can drop mutability of prev_transaction
since it's done with mutability after this line.
@@ -779,4 +800,4 @@ pub fn withdraw_max_and_send_v1(mm: &MarketMakerIt, coin: &str, to: &str) -> Tra | |||
assert_eq!(rc.0, StatusCode::OK, "send_raw_transaction request failed {}", rc.1); | |||
|
|||
tx_details | |||
} | |||
} |
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.
Could you please add empty newline? Otherwise there will be problem for git calculating diffs of this file.
c403935
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.
Great work!
Could you answer just one minor question?
@sergeyboyko0791 It's waiting for your final approval. |
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.
Thank you for the fixes! I've suggested two quick changes, could you please consider them?
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.
Great, thank you for the quick fix!
@caglaryucekaya we've just pushed ETH tests fix, please merge it, and then we can merge this branch as well 🙂 |
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.
Re-approve
The first template for the swap watcher node implementation for UTXO. If the use_watchers config is set to true, the taker broadcasts a signed preimage of the maker payment to the network. If is_watcher config is set to true, the watcher node receives the message, waits for the maker to spend the taker payment, extracts the secret, adds it to the signed preimage of the maker payment and spends it. The remaining functionality will be added in the upcoming PRs.