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.

Sign up for GitHub

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

Jump to bottom

feat(cli/tauri): Emit events on Monero transaction confirmation update and redeem transaction publication #57

Merged
merged 11 commits into from
Sep 18, 2024
Merged

feat(cli/tauri): Emit events on Monero transaction confirmation update and redeem transaction publication #57

merged 11 commits into from
Sep 18, 2024

Conversation

Einliterflasche
Copy link

@Einliterflasche Einliterflasche commented Aug 30, 2024 •
edited by binarybaron
Loading

We now,

  • emit a Tauri event when the Monero lock transaction receives a new confirmation
  • emit a Tauri event with a list of transaction hashes once we have published the Monero redeem transaction

This PR closes #12.

@Einliterflasche Einliterflasche self-assigned this Aug 30, 2024
@Einliterflasche Einliterflasche linked an issue Aug 30, 2024 that may be closed by this pull request
Send updates of confirmations for Bitcoin transactions from Host to Guest #12
Closed
@Einliterflasche
Copy link
Author

In this commit I duplicated the wait_for_confirmations and watch_for_transfer functions to create variants that accept a listener function which get's called everytime wait_for_confirmations sees a new confirmation. I then used these functions to pass a listener that emits a tauri event. I wasn't able to get a swap started using the gui so I haven't tested whether this works yet, though it should (famous last words lol)

@binarybaron
Copy link

Probably better to modify the exiting functions instead

@Einliterflasche
Copy link
Author

True, at least for watch_for_confirmations, that is only used in that function AFAICT.

@Einliterflasche
Copy link
Author

I removed the old functions and passed an empty listener instead, though the result is a little ugly. I will look into ways to improve the code quality.

@binarybaron
Copy link

I'd say it's better to make the listener callback an Option.

@binarybaron binarybaron changed the title feat(cli/tauri): emit tauri events on XmrLock and BtcLock confirmations feat(cli/tauri): Emit Tauri events on Monero lock transaction confirmation update Sep 4, 2024
@binarybaron binarybaron force-pushed the cli/emit-lock-confirmations branch from 28d0c37 to d75d074 Compare September 4, 2024 18:22
@Einliterflasche Einliterflasche force-pushed the cli/emit-lock-confirmations branch from d75d074 to 73e52c4 Compare September 5, 2024 11:33
@Einliterflasche
Copy link
Author

I changed to Option and re-added watch_for_transfer that just passes the arguments and None to watch_for_transfer_with. Also changed to Box<dyn Fn> again since type inference doesn't work when passing None to a Option<impl Fn> argument.

@binarybaron binarybaron force-pushed the cli/emit-lock-confirmations branch from 73e52c4 to 249d745 Compare September 5, 2024 12:56
@binarybaron binarybaron force-pushed the master branch 2 times, most recently from 8639aba to 2a33923 Compare September 6, 2024 21:29
@Einliterflasche Einliterflasche force-pushed the cli/emit-lock-confirmations branch from 249d745 to 73e52c4 Compare September 7, 2024 17:35
binarybaron
binarybaron reviewed Sep 7, 2024
View reviewed changes
swap/src/protocol/alice/swap.rs Outdated
@@ -137,7 +137,10 @@ where
} => match state3.expired_timelocks(bitcoin_wallet).await? {
ExpiredTimelocks::None { .. } => {
monero_wallet
.watch_for_transfer(state3.lock_xmr_watch_request(transfer_proof.clone(), 1))
.watch_for_transfer_with(
Copy link

Choose a reason for hiding this comment

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

Use the watch_for_transfer method here

swap/src/monero/wallet.rs Outdated
@@ -477,7 +502,7 @@ mod tests {
}),
]));

wait_for_confirmations(
wait_for_confirmations_with(
Copy link

Choose a reason for hiding this comment

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

use the dedicated function here

binarybaron
binarybaron reviewed Sep 7, 2024
View reviewed changes
swap/src/protocol/alice/state.rs Outdated
@@ -518,7 +518,7 @@ impl State3 {
// Ensure that the XMR to be refunded are spendable by awaiting 10 confirmations
// on the lock transaction
monero_wallet
.watch_for_transfer(self.lock_xmr_watch_request(transfer_proof, 10))
.watch_for_transfer_with(self.lock_xmr_watch_request(transfer_proof, 10), None)
Copy link

Choose a reason for hiding this comment

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

Use the dedicated function here

binarybaron
binarybaron reviewed Sep 11, 2024
View reviewed changes
swap/src/protocol/bob/swap.rs Outdated
if let ExpiredTimelocks::None { .. } = state.expired_timelock(bitcoin_wallet).await? {
let watch_request = state.lock_xmr_watch_request(lock_transfer_proof);
// Check if the cancel timelock has expired => we cancel the swap
let ExpiredTimelocks::None { .. } = state.expired_timelock(bitcoin_wallet).await?
Copy link

Choose a reason for hiding this comment

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

Imo this syntax is worse than what was used before. It's not entirely clear when this codeblock will be executed at first sight. You need to know which other enum variants exist

binarybaron
binarybaron reviewed Sep 11, 2024
View reviewed changes
swap/src/protocol/bob/swap.rs Outdated
@@ -158,52 +158,56 @@ async fn next_state(

let tx_lock_status = bitcoin_wallet.subscribe_to(state3.tx_lock.clone()).await;

if let ExpiredTimelocks::None { .. } = state3.expired_timelock(bitcoin_wallet).await? {
tracing::info!("Waiting for Alice to lock Monero");
let ExpiredTimelocks::None { .. } = state3.expired_timelock(bitcoin_wallet).await?
Copy link

Choose a reason for hiding this comment

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

Imo this syntax is worse than what was used before. It's not entirely clear when this codeblock will be executed at first sight. You need to know which other enum variants exist

Copy link
Author

Choose a reason for hiding this comment

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

changed to a simple if statement after adding a cancel_timelock_expired method on ExpiredTimelocks

binarybaron
binarybaron reviewed Sep 11, 2024
View reviewed changes
Copy link

@binarybaron binarybaron left a comment

Choose a reason for hiding this comment

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

comments

@binarybaron
Copy link

I have tested this and it works.

@binarybaron binarybaron force-pushed the cli/emit-lock-confirmations branch from ab5c776 to 9764144 Compare September 17, 2024 16:25
@binarybaron binarybaron force-pushed the cli/emit-lock-confirmations branch from 9764144 to 8aac029 Compare September 17, 2024 16:40
@binarybaron
Copy link

Sorry for force pushing. Feel free to push your local branch. I can apply my changes again.

@binarybaron binarybaron mentioned this pull request Sep 18, 2024
Merged
@binarybaron
Copy link

Two clippy warnings need to be fixed otherwise this LGTM!

@binarybaron binarybaron changed the title feat(cli/tauri): Emit Tauri events on Monero lock transaction confirmation update feat(cli/tauri): Emit events on Monero transaction confirmation update and redeem transaction publication Sep 18, 2024
@binarybaron binarybaron merged commit 9d1151c into master Sep 18, 2024
9 of 29 checks passed
@binarybaron
Copy link

LGTM! I also really like the new if ... .cancel_timelock_expired() { return ... } syntax. It greatly improves readibility of the state machine logic.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@binarybaron binarybaron binarybaron left review comments

Assignees

@Einliterflasche Einliterflasche

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Send updates of confirmations for Bitcoin transactions from Host to Guest
2 participants
@Einliterflasche @binarybaron