Update Wallet to 2.4.0 and other deps

[tvpeter]

Description

This PR updates the Wallet API to v2.4.0 and other dependencies. It also adds WalletEvent to both sync and full_scan for all backends.

Fixes#243

Changelog notice

• Update bdk_wallet to v2.4.0
• Add WalletEvents to all backends Esplora, Electrum, Rpc and CBF.
• Add print_wallet_events function to print wallet events to the terminal during sync
• Update bdk_bitcoind_rpc to v0.22.0
• Update bdk_electrum to v0.24.0
• Update bdk_kyoto to v0.15.4
• Update bdk_esplora to v0.22.2

Checklists

All Submissions:

• I've signed all my commits
• I followed the contribution guidelines
• I ran cargo fmt and cargo clippy before committing

[tvpeter]

@notmandatory, I’m wondering if we should update our signing approach since the current method is deprecated. If we choose to proceed with the update, we will need to add miniscript as a separate dependency, as the version exported from the Wallet (v2.3.0) is 12.3.1. We require at least version 13.0.0. Or should we postpone this update until a later version?

[notmandatory]

@notmandatory, I’m wondering if we should update our signing approach since the current method is deprecated. If we choose to proceed with the update, we will need to add miniscript as a separate dependency, as the version exported from the Wallet (v2.3.0) is 12.3.1. We require at least version 13.0.0. Or should we postpone this update until a later version?

Good question. I've created bitcoindevkit/bdk_wallet#405 to revert the deprecating of the signer mod. The new PSBT signer probably won't be available until bdk_wallet 3.1.

[vadim-anfv]

Related #243.

[vadim-anfv]

Blocked by #252.

[tvpeter]

Blocked by #252.

Hi @va-an, could you please verify that you tagged the correct PR? Thank you!

[vadim-anfv]

Blocked by #252.

Hi @va-an, could you please verify that you tagged the correct PR? Thank you!

All correct. I've also reproduced error from PR's CI build in my local run just pre-push:

error[E0061]: this method takes 1 argument but 2 arguments were supplied
--> src/payjoin/mod.rs:605:30
|
605 | ... .check_payment(
| ^^^^^^^^^^^^^
...
618 | / ... |outpoint| {
619 | | ... let utxo = self.wallet.get_utxo(outpoint);
620 | | ... match utxo {
621 | | ... Some(_) => Ok(false),
... |
624 | | ... }
| |___________________________- unexpected argument #2 of type `{closure@src/payjoin/mod.rs:618:33: 618:43}`
|
note: method defined here
--> /home/vaan/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/payjoin-1.0.0-rc.2/src/core/receive/v2/mod.rs:1262:12
|
1262 | pub fn check_payment(
| ^^^^^^^^^^^^^
help: remove the extra argument
|
617 - },
618 - |outpoint| {
619 - let utxo = self.wallet.get_utxo(outpoint);
620 - match utxo {
621 - Some(_) => Ok(false),
622 - None => Ok(true),
623 - }
|

I've applied the fix from #252 to my local code to verify that it helps.

[vadim-anfv]

@tvpeter#252 is merged now, so this should be fine now.
Could you rebase on master? Other PRs will likely need to rebase on top of this once it's merged.

[codecov]

Codecov Report

❌ Patch coverage is 0% with 92 lines in your changes missing coverage. Please review.
✅ Project coverage is 11.36%. Comparing base (50d248c) to head (eea5ba9).
⚠️ Report is 8 commits behind head on master.

Files with missing lines	Patch %	Lines
src/handlers.rs	0.00%	60 Missing ⚠️
src/utils.rs	0.00%	32 Missing ⚠️

Additional details and impacted files

@@ Coverage Diff @@## master #248 +/- ##
==========================================
+ Coverage 10.21% 11.36% +1.15% 
==========================================
Files 8 8 Lines 2713 2807 +94 ==========================================
+ Hits 277 319 +42 - Misses 2436 2488 +52 

Flag	Coverage Δ
rust	11.36% <0.00%> (+1.15%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[vadim-anfv]

Thanks for the rebase, @tvpeter!

Unfortunately, CI is now failing because bdk_wallet 2.3.0 introduced a deprecation that causes warnings-as-errors to fail. The revert has already been merged but 2.4.0 hasn't been released yet.

Pinning back to 2.1.0 won't work either - this branch already uses apply_update_events which was introduced in 2.3.0, so downgrading breaks the build:

error[E0599]: no method named `apply_update_events` found for mutable reference `&mut Wallet`

So it looks like we're stuck waiting for 2.4.0 to be released. Until then, CI will fail on this PR (and on any PR that rebases after merging this one).
We could just keep this in mind during reviews and merges - the CI failure is a known issue, not a code problem.

[vadim-anfv]

utACK 4a41057

[vadim-anfv]

We could use tap for convenience:

Suggested change

	let wallet_events = wallet
	.apply_update_events(update)
	.map_err(|e| Error::Generic(e.to_string()))?;
	print_wallet_events(&wallet_events);
	Ok(())
	wallet
	.apply_update_events(update)
	.map_err(|e| Error::Generic(e.to_string()))?
	.tap(print_wallet_events);
	Ok(())

[tvpeter]

While this will make it cleaner, I think it is not worth the cost of pulling in a whole library. I'm always for using a minimal number of external libraries.

[vadim-anfv]

We already pull in tap as a dependency in #225:

bdk-cli/Cargo.toml

Line 30 in 0a771ca

tap = "1.0.1"

So this change doesn't add any extra cost.

That said, if you'd prefer, we can drop the dependency.

[tvpeter]

Maybe we can drop in another PR. Generally prefer to avoid deps cost.

[tvpeter]

Unfortunately, CI is now failing because bdk_wallet 2.3.0 introduced a deprecation that causes warnings-as-errors to fail.

Yes, I updated the warnings to be treated as errors in this commit 8d20cf2 because many Clippy warnings were being neglected and had to be merged into master. It's not coming from bdk_wallet

[vadim-anfv]

Yes, I updated the warnings to be treated as errors in this commit 8d20cf2 because many Clippy warnings were being neglected and had to be merged into master. It's not coming from bdk_wallet

The deny(warnings) change is great, glad you added it - lax warnings tend to pile up fast. The issue here is a different thing though: bdk_wallet 2.3.0 itself emits a deprecation warning on its own API (apply_update), and that warning hits the deny(warnings) flag. It'll be resolved once 2.4.0 is released (bitcoindevkit/bdk_wallet#405).

[notmandatory]

Unfortunately, CI is now failing because bdk_wallet 2.3.0 introduced a deprecation that causes warnings-as-errors to fail. The revert has already been merged but 2.4.0 hasn't been released yet.

The Wallet 2.4.0 lib was just published and should fix this issue

[vadim-anfv]

ACK 5def41f

Also noted that the PR description mentions "Update bdk_electrum to v0.24.0", but I don't see it in the changes - Cargo.toml (and Cargo.lock) still have bdk_electrum 0.23.2.

[vadim-anfv]

We already pull in tap as a dependency in #225:

bdk-cli/Cargo.toml

Line 30 in 0a771ca

tap = "1.0.1"

So this change doesn't add any extra cost.

That said, if you'd prefer, we can drop the dependency.

[tvpeter]

ACK 5def41f

Also noted that the PR description mentions "Update bdk_electrum to v0.24.0", but I don't see it in the changes - Cargo.toml (and Cargo.lock) still have bdk_electrum 0.23.2.

Seen the reason now. I amended the last commit and did not push. Thanks for catching that.

[tvpeter]

Description

This PR updates the Wallet API to v2.4.0 and other dependencies. It also adds WalletEvent to both sync and full_scan for all backends.

Fixes#243

Changelog notice

• Update bdk_wallet to v2.4.0
• Add WalletEvents to all backends Esplora, Electrum, Rpc and CBF.
• Add print_wallet_events function to print wallet events to the terminal during sync
• Update bdk_bitcoind_rpc to v0.22.0
• Update bdk_electrum to v0.24.0
• Update bdk_kyoto to v0.15.4
• Update bdk_esplora to v0.22.2

Checklists

All Submissions:

• I've signed all my commits
• I followed the contribution guidelines
• I ran cargo fmt and cargo clippy before committing

[tvpeter]

@notmandatory, I’m wondering if we should update our signing approach since the current method is deprecated. If we choose to proceed with the update, we will need to add miniscript as a separate dependency, as the version exported from the Wallet (v2.3.0) is 12.3.1. We require at least version 13.0.0. Or should we postpone this update until a later version?

[notmandatory]

@notmandatory, I’m wondering if we should update our signing approach since the current method is deprecated. If we choose to proceed with the update, we will need to add miniscript as a separate dependency, as the version exported from the Wallet (v2.3.0) is 12.3.1. We require at least version 13.0.0. Or should we postpone this update until a later version?

Good question. I've created bitcoindevkit/bdk_wallet#405 to revert the deprecating of the signer mod. The new PSBT signer probably won't be available until bdk_wallet 3.1.

[vadim-anfv]

Related #243.

[vadim-anfv]

Blocked by #252.

[tvpeter]

Blocked by #252.

Hi @va-an, could you please verify that you tagged the correct PR? Thank you!

[vadim-anfv]

Blocked by #252.

Hi @va-an, could you please verify that you tagged the correct PR? Thank you!

All correct. I've also reproduced error from PR's CI build in my local run just pre-push:

error[E0061]: this method takes 1 argument but 2 arguments were supplied
--> src/payjoin/mod.rs:605:30
|
605 | ... .check_payment(
| ^^^^^^^^^^^^^
...
618 | / ... |outpoint| {
619 | | ... let utxo = self.wallet.get_utxo(outpoint);
620 | | ... match utxo {
621 | | ... Some(_) => Ok(false),
... |
624 | | ... }
| |___________________________- unexpected argument #2 of type `{closure@src/payjoin/mod.rs:618:33: 618:43}`
|
note: method defined here
--> /home/vaan/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/payjoin-1.0.0-rc.2/src/core/receive/v2/mod.rs:1262:12
|
1262 | pub fn check_payment(
| ^^^^^^^^^^^^^
help: remove the extra argument
|
617 - },
618 - |outpoint| {
619 - let utxo = self.wallet.get_utxo(outpoint);
620 - match utxo {
621 - Some(_) => Ok(false),
622 - None => Ok(true),
623 - }
|

I've applied the fix from #252 to my local code to verify that it helps.

[vadim-anfv]

@tvpeter#252 is merged now, so this should be fine now.
Could you rebase on master? Other PRs will likely need to rebase on top of this once it's merged.

[codecov]

Codecov Report

❌ Patch coverage is 0% with 92 lines in your changes missing coverage. Please review.
✅ Project coverage is 11.36%. Comparing base (50d248c) to head (eea5ba9).
⚠️ Report is 8 commits behind head on master.

Files with missing lines	Patch %	Lines
src/handlers.rs	0.00%	60 Missing ⚠️
src/utils.rs	0.00%	32 Missing ⚠️

Additional details and impacted files

@@ Coverage Diff @@## master #248 +/- ##
==========================================
+ Coverage 10.21% 11.36% +1.15% 
==========================================
Files 8 8 Lines 2713 2807 +94 ==========================================
+ Hits 277 319 +42 - Misses 2436 2488 +52 

Flag	Coverage Δ
rust	11.36% <0.00%> (+1.15%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[vadim-anfv]

Thanks for the rebase, @tvpeter!

Unfortunately, CI is now failing because bdk_wallet 2.3.0 introduced a deprecation that causes warnings-as-errors to fail. The revert has already been merged but 2.4.0 hasn't been released yet.

Pinning back to 2.1.0 won't work either - this branch already uses apply_update_events which was introduced in 2.3.0, so downgrading breaks the build:

error[E0599]: no method named `apply_update_events` found for mutable reference `&mut Wallet`

So it looks like we're stuck waiting for 2.4.0 to be released. Until then, CI will fail on this PR (and on any PR that rebases after merging this one).
We could just keep this in mind during reviews and merges - the CI failure is a known issue, not a code problem.

[vadim-anfv]

utACK 4a41057

[vadim-anfv]

We could use tap for convenience:

Suggested change

	let wallet_events = wallet
	.apply_update_events(update)
	.map_err(|e| Error::Generic(e.to_string()))?;
	print_wallet_events(&wallet_events);
	Ok(())
	wallet
	.apply_update_events(update)
	.map_err(|e| Error::Generic(e.to_string()))?
	.tap(print_wallet_events);
	Ok(())

[tvpeter]

While this will make it cleaner, I think it is not worth the cost of pulling in a whole library. I'm always for using a minimal number of external libraries.

[vadim-anfv]

We already pull in tap as a dependency in #225:

bdk-cli/Cargo.toml

Line 30 in 0a771ca

tap = "1.0.1"

So this change doesn't add any extra cost.

That said, if you'd prefer, we can drop the dependency.

[tvpeter]

Maybe we can drop in another PR. Generally prefer to avoid deps cost.

[tvpeter]

Unfortunately, CI is now failing because bdk_wallet 2.3.0 introduced a deprecation that causes warnings-as-errors to fail.

Yes, I updated the warnings to be treated as errors in this commit 8d20cf2 because many Clippy warnings were being neglected and had to be merged into master. It's not coming from bdk_wallet

[vadim-anfv]

Yes, I updated the warnings to be treated as errors in this commit 8d20cf2 because many Clippy warnings were being neglected and had to be merged into master. It's not coming from bdk_wallet

The deny(warnings) change is great, glad you added it - lax warnings tend to pile up fast. The issue here is a different thing though: bdk_wallet 2.3.0 itself emits a deprecation warning on its own API (apply_update), and that warning hits the deny(warnings) flag. It'll be resolved once 2.4.0 is released (bitcoindevkit/bdk_wallet#405).

[notmandatory]

Unfortunately, CI is now failing because bdk_wallet 2.3.0 introduced a deprecation that causes warnings-as-errors to fail. The revert has already been merged but 2.4.0 hasn't been released yet.

The Wallet 2.4.0 lib was just published and should fix this issue

[vadim-anfv]

ACK 5def41f

Also noted that the PR description mentions "Update bdk_electrum to v0.24.0", but I don't see it in the changes - Cargo.toml (and Cargo.lock) still have bdk_electrum 0.23.2.

[vadim-anfv]

We already pull in tap as a dependency in #225:

bdk-cli/Cargo.toml

Line 30 in 0a771ca

tap = "1.0.1"

So this change doesn't add any extra cost.

That said, if you'd prefer, we can drop the dependency.

[tvpeter]

ACK 5def41f

Also noted that the PR description mentions "Update bdk_electrum to v0.24.0", but I don't see it in the changes - Cargo.toml (and Cargo.lock) still have bdk_electrum 0.23.2.

Seen the reason now. I amended the last commit and did not push. Thanks for catching that.

[tvpeter]

Description

This PR updates the Wallet API to v2.4.0 and other dependencies. It also adds WalletEvent to both sync and full_scan for all backends.

Fixes#243

Changelog notice

• Update bdk_wallet to v2.4.0
• Add WalletEvents to all backends Esplora, Electrum, Rpc and CBF.
• Add print_wallet_events function to print wallet events to the terminal during sync
• Update bdk_bitcoind_rpc to v0.22.0
• Update bdk_electrum to v0.24.0
• Update bdk_kyoto to v0.15.4
• Update bdk_esplora to v0.22.2

Checklists

All Submissions:

• I've signed all my commits
• I followed the contribution guidelines
• I ran cargo fmt and cargo clippy before committing

[tvpeter]

@notmandatory, I’m wondering if we should update our signing approach since the current method is deprecated. If we choose to proceed with the update, we will need to add miniscript as a separate dependency, as the version exported from the Wallet (v2.3.0) is 12.3.1. We require at least version 13.0.0. Or should we postpone this update until a later version?

[notmandatory]

@notmandatory, I’m wondering if we should update our signing approach since the current method is deprecated. If we choose to proceed with the update, we will need to add miniscript as a separate dependency, as the version exported from the Wallet (v2.3.0) is 12.3.1. We require at least version 13.0.0. Or should we postpone this update until a later version?

Good question. I've created bitcoindevkit/bdk_wallet#405 to revert the deprecating of the signer mod. The new PSBT signer probably won't be available until bdk_wallet 3.1.

[vadim-anfv]

Related #243.

[vadim-anfv]

Blocked by #252.

[tvpeter]

Blocked by #252.

Hi @va-an, could you please verify that you tagged the correct PR? Thank you!

[vadim-anfv]

Blocked by #252.

Hi @va-an, could you please verify that you tagged the correct PR? Thank you!

All correct. I've also reproduced error from PR's CI build in my local run just pre-push:

error[E0061]: this method takes 1 argument but 2 arguments were supplied
--> src/payjoin/mod.rs:605:30
|
605 | ... .check_payment(
| ^^^^^^^^^^^^^
...
618 | / ... |outpoint| {
619 | | ... let utxo = self.wallet.get_utxo(outpoint);
620 | | ... match utxo {
621 | | ... Some(_) => Ok(false),
... |
624 | | ... }
| |___________________________- unexpected argument #2 of type `{closure@src/payjoin/mod.rs:618:33: 618:43}`
|
note: method defined here
--> /home/vaan/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/payjoin-1.0.0-rc.2/src/core/receive/v2/mod.rs:1262:12
|
1262 | pub fn check_payment(
| ^^^^^^^^^^^^^
help: remove the extra argument
|
617 - },
618 - |outpoint| {
619 - let utxo = self.wallet.get_utxo(outpoint);
620 - match utxo {
621 - Some(_) => Ok(false),
622 - None => Ok(true),
623 - }
|

I've applied the fix from #252 to my local code to verify that it helps.

[vadim-anfv]

@tvpeter#252 is merged now, so this should be fine now.
Could you rebase on master? Other PRs will likely need to rebase on top of this once it's merged.

[codecov]

Codecov Report

❌ Patch coverage is 0% with 92 lines in your changes missing coverage. Please review.
✅ Project coverage is 11.36%. Comparing base (50d248c) to head (eea5ba9).
⚠️ Report is 8 commits behind head on master.

Files with missing lines	Patch %	Lines
src/handlers.rs	0.00%	60 Missing ⚠️
src/utils.rs	0.00%	32 Missing ⚠️

Additional details and impacted files

@@ Coverage Diff @@## master #248 +/- ##
==========================================
+ Coverage 10.21% 11.36% +1.15% 
==========================================
Files 8 8 Lines 2713 2807 +94 ==========================================
+ Hits 277 319 +42 - Misses 2436 2488 +52 

Flag	Coverage Δ
rust	11.36% <0.00%> (+1.15%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[vadim-anfv]

Thanks for the rebase, @tvpeter!

Unfortunately, CI is now failing because bdk_wallet 2.3.0 introduced a deprecation that causes warnings-as-errors to fail. The revert has already been merged but 2.4.0 hasn't been released yet.

Pinning back to 2.1.0 won't work either - this branch already uses apply_update_events which was introduced in 2.3.0, so downgrading breaks the build:

error[E0599]: no method named `apply_update_events` found for mutable reference `&mut Wallet`

So it looks like we're stuck waiting for 2.4.0 to be released. Until then, CI will fail on this PR (and on any PR that rebases after merging this one).
We could just keep this in mind during reviews and merges - the CI failure is a known issue, not a code problem.

[vadim-anfv]

utACK 4a41057

[vadim-anfv]

We could use tap for convenience:

Suggested change

	let wallet_events = wallet
	.apply_update_events(update)
	.map_err(|e| Error::Generic(e.to_string()))?;
	print_wallet_events(&wallet_events);
	Ok(())
	wallet
	.apply_update_events(update)
	.map_err(|e| Error::Generic(e.to_string()))?
	.tap(print_wallet_events);
	Ok(())

[tvpeter]

While this will make it cleaner, I think it is not worth the cost of pulling in a whole library. I'm always for using a minimal number of external libraries.

[vadim-anfv]

We already pull in tap as a dependency in #225:

bdk-cli/Cargo.toml

Line 30 in 0a771ca

tap = "1.0.1"

So this change doesn't add any extra cost.

That said, if you'd prefer, we can drop the dependency.

[tvpeter]

Maybe we can drop in another PR. Generally prefer to avoid deps cost.

[tvpeter]

Unfortunately, CI is now failing because bdk_wallet 2.3.0 introduced a deprecation that causes warnings-as-errors to fail.

Yes, I updated the warnings to be treated as errors in this commit 8d20cf2 because many Clippy warnings were being neglected and had to be merged into master. It's not coming from bdk_wallet

[vadim-anfv]

Yes, I updated the warnings to be treated as errors in this commit 8d20cf2 because many Clippy warnings were being neglected and had to be merged into master. It's not coming from bdk_wallet

The deny(warnings) change is great, glad you added it - lax warnings tend to pile up fast. The issue here is a different thing though: bdk_wallet 2.3.0 itself emits a deprecation warning on its own API (apply_update), and that warning hits the deny(warnings) flag. It'll be resolved once 2.4.0 is released (bitcoindevkit/bdk_wallet#405).

[notmandatory]

Unfortunately, CI is now failing because bdk_wallet 2.3.0 introduced a deprecation that causes warnings-as-errors to fail. The revert has already been merged but 2.4.0 hasn't been released yet.

The Wallet 2.4.0 lib was just published and should fix this issue

[vadim-anfv]

ACK 5def41f

Also noted that the PR description mentions "Update bdk_electrum to v0.24.0", but I don't see it in the changes - Cargo.toml (and Cargo.lock) still have bdk_electrum 0.23.2.

[vadim-anfv]

We already pull in tap as a dependency in #225:

bdk-cli/Cargo.toml

Line 30 in 0a771ca

tap = "1.0.1"

So this change doesn't add any extra cost.

That said, if you'd prefer, we can drop the dependency.

[tvpeter]

ACK 5def41f

Also noted that the PR description mentions "Update bdk_electrum to v0.24.0", but I don't see it in the changes - Cargo.toml (and Cargo.lock) still have bdk_electrum 0.23.2.

Seen the reason now. I amended the last commit and did not push. Thanks for catching that.

[tvpeter]

Description

This PR updates the Wallet API to v2.4.0 and other dependencies. It also adds WalletEvent to both sync and full_scan for all backends.

Fixes#243

Changelog notice

• Update bdk_wallet to v2.4.0
• Add WalletEvents to all backends Esplora, Electrum, Rpc and CBF.
• Add print_wallet_events function to print wallet events to the terminal during sync
• Update bdk_bitcoind_rpc to v0.22.0
• Update bdk_electrum to v0.24.0
• Update bdk_kyoto to v0.15.4
• Update bdk_esplora to v0.22.2

Checklists

All Submissions:

• I've signed all my commits
• I followed the contribution guidelines
• I ran cargo fmt and cargo clippy before committing

[tvpeter]

@notmandatory, I’m wondering if we should update our signing approach since the current method is deprecated. If we choose to proceed with the update, we will need to add miniscript as a separate dependency, as the version exported from the Wallet (v2.3.0) is 12.3.1. We require at least version 13.0.0. Or should we postpone this update until a later version?

[notmandatory]

@notmandatory, I’m wondering if we should update our signing approach since the current method is deprecated. If we choose to proceed with the update, we will need to add miniscript as a separate dependency, as the version exported from the Wallet (v2.3.0) is 12.3.1. We require at least version 13.0.0. Or should we postpone this update until a later version?

Good question. I've created bitcoindevkit/bdk_wallet#405 to revert the deprecating of the signer mod. The new PSBT signer probably won't be available until bdk_wallet 3.1.

[vadim-anfv]

Related #243.

[vadim-anfv]

Blocked by #252.

[tvpeter]

Blocked by #252.

Hi @va-an, could you please verify that you tagged the correct PR? Thank you!

[vadim-anfv]

Blocked by #252.

Hi @va-an, could you please verify that you tagged the correct PR? Thank you!

All correct. I've also reproduced error from PR's CI build in my local run just pre-push:

error[E0061]: this method takes 1 argument but 2 arguments were supplied
--> src/payjoin/mod.rs:605:30
|
605 | ... .check_payment(
| ^^^^^^^^^^^^^
...
618 | / ... |outpoint| {
619 | | ... let utxo = self.wallet.get_utxo(outpoint);
620 | | ... match utxo {
621 | | ... Some(_) => Ok(false),
... |
624 | | ... }
| |___________________________- unexpected argument #2 of type `{closure@src/payjoin/mod.rs:618:33: 618:43}`
|
note: method defined here
--> /home/vaan/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/payjoin-1.0.0-rc.2/src/core/receive/v2/mod.rs:1262:12
|
1262 | pub fn check_payment(
| ^^^^^^^^^^^^^
help: remove the extra argument
|
617 - },
618 - |outpoint| {
619 - let utxo = self.wallet.get_utxo(outpoint);
620 - match utxo {
621 - Some(_) => Ok(false),
622 - None => Ok(true),
623 - }
|

I've applied the fix from #252 to my local code to verify that it helps.

[vadim-anfv]

@tvpeter#252 is merged now, so this should be fine now.
Could you rebase on master? Other PRs will likely need to rebase on top of this once it's merged.

[codecov]

Codecov Report

❌ Patch coverage is 0% with 92 lines in your changes missing coverage. Please review.
✅ Project coverage is 11.36%. Comparing base (50d248c) to head (eea5ba9).
⚠️ Report is 8 commits behind head on master.

Files with missing lines	Patch %	Lines
src/handlers.rs	0.00%	60 Missing ⚠️
src/utils.rs	0.00%	32 Missing ⚠️

Additional details and impacted files

@@ Coverage Diff @@## master #248 +/- ##
==========================================
+ Coverage 10.21% 11.36% +1.15% 
==========================================
Files 8 8 Lines 2713 2807 +94 ==========================================
+ Hits 277 319 +42 - Misses 2436 2488 +52 

Flag	Coverage Δ
rust	11.36% <0.00%> (+1.15%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[vadim-anfv]

Thanks for the rebase, @tvpeter!

Unfortunately, CI is now failing because bdk_wallet 2.3.0 introduced a deprecation that causes warnings-as-errors to fail. The revert has already been merged but 2.4.0 hasn't been released yet.

Pinning back to 2.1.0 won't work either - this branch already uses apply_update_events which was introduced in 2.3.0, so downgrading breaks the build:

error[E0599]: no method named `apply_update_events` found for mutable reference `&mut Wallet`

So it looks like we're stuck waiting for 2.4.0 to be released. Until then, CI will fail on this PR (and on any PR that rebases after merging this one).
We could just keep this in mind during reviews and merges - the CI failure is a known issue, not a code problem.

[vadim-anfv]

utACK 4a41057

[vadim-anfv]

We could use tap for convenience:

Suggested change

	let wallet_events = wallet
	.apply_update_events(update)
	.map_err(|e| Error::Generic(e.to_string()))?;
	print_wallet_events(&wallet_events);
	Ok(())
	wallet
	.apply_update_events(update)
	.map_err(|e| Error::Generic(e.to_string()))?
	.tap(print_wallet_events);
	Ok(())

[tvpeter]

While this will make it cleaner, I think it is not worth the cost of pulling in a whole library. I'm always for using a minimal number of external libraries.

[vadim-anfv]

We already pull in tap as a dependency in #225:

bdk-cli/Cargo.toml

Line 30 in 0a771ca

tap = "1.0.1"

So this change doesn't add any extra cost.

That said, if you'd prefer, we can drop the dependency.

[tvpeter]

Maybe we can drop in another PR. Generally prefer to avoid deps cost.

[tvpeter]

Unfortunately, CI is now failing because bdk_wallet 2.3.0 introduced a deprecation that causes warnings-as-errors to fail.

Yes, I updated the warnings to be treated as errors in this commit 8d20cf2 because many Clippy warnings were being neglected and had to be merged into master. It's not coming from bdk_wallet

[vadim-anfv]

Yes, I updated the warnings to be treated as errors in this commit 8d20cf2 because many Clippy warnings were being neglected and had to be merged into master. It's not coming from bdk_wallet

The deny(warnings) change is great, glad you added it - lax warnings tend to pile up fast. The issue here is a different thing though: bdk_wallet 2.3.0 itself emits a deprecation warning on its own API (apply_update), and that warning hits the deny(warnings) flag. It'll be resolved once 2.4.0 is released (bitcoindevkit/bdk_wallet#405).

[notmandatory]

Unfortunately, CI is now failing because bdk_wallet 2.3.0 introduced a deprecation that causes warnings-as-errors to fail. The revert has already been merged but 2.4.0 hasn't been released yet.

The Wallet 2.4.0 lib was just published and should fix this issue

[vadim-anfv]

ACK 5def41f

Also noted that the PR description mentions "Update bdk_electrum to v0.24.0", but I don't see it in the changes - Cargo.toml (and Cargo.lock) still have bdk_electrum 0.23.2.

[vadim-anfv]

We already pull in tap as a dependency in #225:

bdk-cli/Cargo.toml

Line 30 in 0a771ca

tap = "1.0.1"

So this change doesn't add any extra cost.

That said, if you'd prefer, we can drop the dependency.

[tvpeter]

ACK 5def41f

Also noted that the PR description mentions "Update bdk_electrum to v0.24.0", but I don't see it in the changes - Cargo.toml (and Cargo.lock) still have bdk_electrum 0.23.2.

Seen the reason now. I amended the last commit and did not push. Thanks for catching that.

[tvpeter]

Description

This PR updates the Wallet API to v2.4.0 and other dependencies. It also adds WalletEvent to both sync and full_scan for all backends.

Fixes#243

Changelog notice

• Update bdk_wallet to v2.4.0
• Add WalletEvents to all backends Esplora, Electrum, Rpc and CBF.
• Add print_wallet_events function to print wallet events to the terminal during sync
• Update bdk_bitcoind_rpc to v0.22.0
• Update bdk_electrum to v0.24.0
• Update bdk_kyoto to v0.15.4
• Update bdk_esplora to v0.22.2

Checklists

All Submissions:

• I've signed all my commits
• I followed the contribution guidelines
• I ran cargo fmt and cargo clippy before committing

[tvpeter]

@notmandatory, I’m wondering if we should update our signing approach since the current method is deprecated. If we choose to proceed with the update, we will need to add miniscript as a separate dependency, as the version exported from the Wallet (v2.3.0) is 12.3.1. We require at least version 13.0.0. Or should we postpone this update until a later version?

[notmandatory]

@notmandatory, I’m wondering if we should update our signing approach since the current method is deprecated. If we choose to proceed with the update, we will need to add miniscript as a separate dependency, as the version exported from the Wallet (v2.3.0) is 12.3.1. We require at least version 13.0.0. Or should we postpone this update until a later version?

Good question. I've created bitcoindevkit/bdk_wallet#405 to revert the deprecating of the signer mod. The new PSBT signer probably won't be available until bdk_wallet 3.1.

[vadim-anfv]

Related #243.

[vadim-anfv]

Blocked by #252.

[tvpeter]

Blocked by #252.

Hi @va-an, could you please verify that you tagged the correct PR? Thank you!

[vadim-anfv]

Blocked by #252.

Hi @va-an, could you please verify that you tagged the correct PR? Thank you!

All correct. I've also reproduced error from PR's CI build in my local run just pre-push:

error[E0061]: this method takes 1 argument but 2 arguments were supplied
--> src/payjoin/mod.rs:605:30
|
605 | ... .check_payment(
| ^^^^^^^^^^^^^
...
618 | / ... |outpoint| {
619 | | ... let utxo = self.wallet.get_utxo(outpoint);
620 | | ... match utxo {
621 | | ... Some(_) => Ok(false),
... |
624 | | ... }
| |___________________________- unexpected argument #2 of type `{closure@src/payjoin/mod.rs:618:33: 618:43}`
|
note: method defined here
--> /home/vaan/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/payjoin-1.0.0-rc.2/src/core/receive/v2/mod.rs:1262:12
|
1262 | pub fn check_payment(
| ^^^^^^^^^^^^^
help: remove the extra argument
|
617 - },
618 - |outpoint| {
619 - let utxo = self.wallet.get_utxo(outpoint);
620 - match utxo {
621 - Some(_) => Ok(false),
622 - None => Ok(true),
623 - }
|

I've applied the fix from #252 to my local code to verify that it helps.

[vadim-anfv]

@tvpeter#252 is merged now, so this should be fine now.
Could you rebase on master? Other PRs will likely need to rebase on top of this once it's merged.

[codecov]

Codecov Report

❌ Patch coverage is 0% with 92 lines in your changes missing coverage. Please review.
✅ Project coverage is 11.36%. Comparing base (50d248c) to head (eea5ba9).
⚠️ Report is 8 commits behind head on master.

Files with missing lines	Patch %	Lines
src/handlers.rs	0.00%	60 Missing ⚠️
src/utils.rs	0.00%	32 Missing ⚠️

Additional details and impacted files

@@ Coverage Diff @@## master #248 +/- ##
==========================================
+ Coverage 10.21% 11.36% +1.15% 
==========================================
Files 8 8 Lines 2713 2807 +94 ==========================================
+ Hits 277 319 +42 - Misses 2436 2488 +52 

Flag	Coverage Δ
rust	11.36% <0.00%> (+1.15%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[vadim-anfv]

Thanks for the rebase, @tvpeter!

Unfortunately, CI is now failing because bdk_wallet 2.3.0 introduced a deprecation that causes warnings-as-errors to fail. The revert has already been merged but 2.4.0 hasn't been released yet.

Pinning back to 2.1.0 won't work either - this branch already uses apply_update_events which was introduced in 2.3.0, so downgrading breaks the build:

error[E0599]: no method named `apply_update_events` found for mutable reference `&mut Wallet`

So it looks like we're stuck waiting for 2.4.0 to be released. Until then, CI will fail on this PR (and on any PR that rebases after merging this one).
We could just keep this in mind during reviews and merges - the CI failure is a known issue, not a code problem.

[vadim-anfv]

utACK 4a41057

[vadim-anfv]

We could use tap for convenience:

Suggested change

	let wallet_events = wallet
	.apply_update_events(update)
	.map_err(|e| Error::Generic(e.to_string()))?;
	print_wallet_events(&wallet_events);
	Ok(())
	wallet
	.apply_update_events(update)
	.map_err(|e| Error::Generic(e.to_string()))?
	.tap(print_wallet_events);
	Ok(())

[tvpeter]

While this will make it cleaner, I think it is not worth the cost of pulling in a whole library. I'm always for using a minimal number of external libraries.

[vadim-anfv]

We already pull in tap as a dependency in #225:

bdk-cli/Cargo.toml

Line 30 in 0a771ca

tap = "1.0.1"

So this change doesn't add any extra cost.

That said, if you'd prefer, we can drop the dependency.

[tvpeter]

Maybe we can drop in another PR. Generally prefer to avoid deps cost.

[tvpeter]

Unfortunately, CI is now failing because bdk_wallet 2.3.0 introduced a deprecation that causes warnings-as-errors to fail.

Yes, I updated the warnings to be treated as errors in this commit 8d20cf2 because many Clippy warnings were being neglected and had to be merged into master. It's not coming from bdk_wallet

[vadim-anfv]

Yes, I updated the warnings to be treated as errors in this commit 8d20cf2 because many Clippy warnings were being neglected and had to be merged into master. It's not coming from bdk_wallet

The deny(warnings) change is great, glad you added it - lax warnings tend to pile up fast. The issue here is a different thing though: bdk_wallet 2.3.0 itself emits a deprecation warning on its own API (apply_update), and that warning hits the deny(warnings) flag. It'll be resolved once 2.4.0 is released (bitcoindevkit/bdk_wallet#405).

[notmandatory]

Unfortunately, CI is now failing because bdk_wallet 2.3.0 introduced a deprecation that causes warnings-as-errors to fail. The revert has already been merged but 2.4.0 hasn't been released yet.

The Wallet 2.4.0 lib was just published and should fix this issue

[vadim-anfv]

ACK 5def41f

Also noted that the PR description mentions "Update bdk_electrum to v0.24.0", but I don't see it in the changes - Cargo.toml (and Cargo.lock) still have bdk_electrum 0.23.2.

[vadim-anfv]

We already pull in tap as a dependency in #225:

bdk-cli/Cargo.toml

Line 30 in 0a771ca

tap = "1.0.1"

So this change doesn't add any extra cost.

That said, if you'd prefer, we can drop the dependency.

[tvpeter]

ACK 5def41f

Also noted that the PR description mentions "Update bdk_electrum to v0.24.0", but I don't see it in the changes - Cargo.toml (and Cargo.lock) still have bdk_electrum 0.23.2.

Seen the reason now. I amended the last commit and did not push. Thanks for catching that.

[tvpeter]

Description

This PR updates the Wallet API to v2.4.0 and other dependencies. It also adds WalletEvent to both sync and full_scan for all backends.

Fixes#243

Changelog notice

• Update bdk_wallet to v2.4.0
• Add WalletEvents to all backends Esplora, Electrum, Rpc and CBF.
• Add print_wallet_events function to print wallet events to the terminal during sync
• Update bdk_bitcoind_rpc to v0.22.0
• Update bdk_electrum to v0.24.0
• Update bdk_kyoto to v0.15.4
• Update bdk_esplora to v0.22.2

Checklists

All Submissions:

• I've signed all my commits
• I followed the contribution guidelines
• I ran cargo fmt and cargo clippy before committing

[tvpeter]

@notmandatory, I’m wondering if we should update our signing approach since the current method is deprecated. If we choose to proceed with the update, we will need to add miniscript as a separate dependency, as the version exported from the Wallet (v2.3.0) is 12.3.1. We require at least version 13.0.0. Or should we postpone this update until a later version?

[notmandatory]

@notmandatory, I’m wondering if we should update our signing approach since the current method is deprecated. If we choose to proceed with the update, we will need to add miniscript as a separate dependency, as the version exported from the Wallet (v2.3.0) is 12.3.1. We require at least version 13.0.0. Or should we postpone this update until a later version?

Good question. I've created bitcoindevkit/bdk_wallet#405 to revert the deprecating of the signer mod. The new PSBT signer probably won't be available until bdk_wallet 3.1.

[vadim-anfv]

Related #243.

[vadim-anfv]

Blocked by #252.

[tvpeter]

Blocked by #252.

Hi @va-an, could you please verify that you tagged the correct PR? Thank you!

[vadim-anfv]

Blocked by #252.

Hi @va-an, could you please verify that you tagged the correct PR? Thank you!

All correct. I've also reproduced error from PR's CI build in my local run just pre-push:

error[E0061]: this method takes 1 argument but 2 arguments were supplied
--> src/payjoin/mod.rs:605:30
|
605 | ... .check_payment(
| ^^^^^^^^^^^^^
...
618 | / ... |outpoint| {
619 | | ... let utxo = self.wallet.get_utxo(outpoint);
620 | | ... match utxo {
621 | | ... Some(_) => Ok(false),
... |
624 | | ... }
| |___________________________- unexpected argument #2 of type `{closure@src/payjoin/mod.rs:618:33: 618:43}`
|
note: method defined here
--> /home/vaan/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/payjoin-1.0.0-rc.2/src/core/receive/v2/mod.rs:1262:12
|
1262 | pub fn check_payment(
| ^^^^^^^^^^^^^
help: remove the extra argument
|
617 - },
618 - |outpoint| {
619 - let utxo = self.wallet.get_utxo(outpoint);
620 - match utxo {
621 - Some(_) => Ok(false),
622 - None => Ok(true),
623 - }
|

I've applied the fix from #252 to my local code to verify that it helps.

[vadim-anfv]

@tvpeter#252 is merged now, so this should be fine now.
Could you rebase on master? Other PRs will likely need to rebase on top of this once it's merged.

[codecov]

Codecov Report

❌ Patch coverage is 0% with 92 lines in your changes missing coverage. Please review.
✅ Project coverage is 11.36%. Comparing base (50d248c) to head (eea5ba9).
⚠️ Report is 8 commits behind head on master.

Files with missing lines	Patch %	Lines
src/handlers.rs	0.00%	60 Missing ⚠️
src/utils.rs	0.00%	32 Missing ⚠️

Additional details and impacted files

@@ Coverage Diff @@## master #248 +/- ##
==========================================
+ Coverage 10.21% 11.36% +1.15% 
==========================================
Files 8 8 Lines 2713 2807 +94 ==========================================
+ Hits 277 319 +42 - Misses 2436 2488 +52 

Flag	Coverage Δ
rust	11.36% <0.00%> (+1.15%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[vadim-anfv]

Thanks for the rebase, @tvpeter!

Unfortunately, CI is now failing because bdk_wallet 2.3.0 introduced a deprecation that causes warnings-as-errors to fail. The revert has already been merged but 2.4.0 hasn't been released yet.

Pinning back to 2.1.0 won't work either - this branch already uses apply_update_events which was introduced in 2.3.0, so downgrading breaks the build:

error[E0599]: no method named `apply_update_events` found for mutable reference `&mut Wallet`

So it looks like we're stuck waiting for 2.4.0 to be released. Until then, CI will fail on this PR (and on any PR that rebases after merging this one).
We could just keep this in mind during reviews and merges - the CI failure is a known issue, not a code problem.

[vadim-anfv]

utACK 4a41057

[vadim-anfv]

We could use tap for convenience:

Suggested change

	let wallet_events = wallet
	.apply_update_events(update)
	.map_err(|e| Error::Generic(e.to_string()))?;
	print_wallet_events(&wallet_events);
	Ok(())
	wallet
	.apply_update_events(update)
	.map_err(|e| Error::Generic(e.to_string()))?
	.tap(print_wallet_events);
	Ok(())

[tvpeter]

While this will make it cleaner, I think it is not worth the cost of pulling in a whole library. I'm always for using a minimal number of external libraries.

[vadim-anfv]

We already pull in tap as a dependency in #225:

bdk-cli/Cargo.toml

Line 30 in 0a771ca

tap = "1.0.1"

So this change doesn't add any extra cost.

That said, if you'd prefer, we can drop the dependency.

[tvpeter]

Maybe we can drop in another PR. Generally prefer to avoid deps cost.

[tvpeter]

Unfortunately, CI is now failing because bdk_wallet 2.3.0 introduced a deprecation that causes warnings-as-errors to fail.

Yes, I updated the warnings to be treated as errors in this commit 8d20cf2 because many Clippy warnings were being neglected and had to be merged into master. It's not coming from bdk_wallet

[vadim-anfv]

Yes, I updated the warnings to be treated as errors in this commit 8d20cf2 because many Clippy warnings were being neglected and had to be merged into master. It's not coming from bdk_wallet

The deny(warnings) change is great, glad you added it - lax warnings tend to pile up fast. The issue here is a different thing though: bdk_wallet 2.3.0 itself emits a deprecation warning on its own API (apply_update), and that warning hits the deny(warnings) flag. It'll be resolved once 2.4.0 is released (bitcoindevkit/bdk_wallet#405).

[notmandatory]

Unfortunately, CI is now failing because bdk_wallet 2.3.0 introduced a deprecation that causes warnings-as-errors to fail. The revert has already been merged but 2.4.0 hasn't been released yet.

The Wallet 2.4.0 lib was just published and should fix this issue

[vadim-anfv]

ACK 5def41f

Also noted that the PR description mentions "Update bdk_electrum to v0.24.0", but I don't see it in the changes - Cargo.toml (and Cargo.lock) still have bdk_electrum 0.23.2.

[vadim-anfv]

We already pull in tap as a dependency in #225:

bdk-cli/Cargo.toml

Line 30 in 0a771ca

tap = "1.0.1"

So this change doesn't add any extra cost.

That said, if you'd prefer, we can drop the dependency.

[tvpeter]

ACK 5def41f

Also noted that the PR description mentions "Update bdk_electrum to v0.24.0", but I don't see it in the changes - Cargo.toml (and Cargo.lock) still have bdk_electrum 0.23.2.

Seen the reason now. I amended the last commit and did not push. Thanks for catching that.

[tvpeter]

Description

This PR updates the Wallet API to v2.4.0 and other dependencies. It also adds WalletEvent to both sync and full_scan for all backends.

Fixes#243

Changelog notice

• Update bdk_wallet to v2.4.0
• Add WalletEvents to all backends Esplora, Electrum, Rpc and CBF.
• Add print_wallet_events function to print wallet events to the terminal during sync
• Update bdk_bitcoind_rpc to v0.22.0
• Update bdk_electrum to v0.24.0
• Update bdk_kyoto to v0.15.4
• Update bdk_esplora to v0.22.2

Checklists

All Submissions:

• I've signed all my commits
• I followed the contribution guidelines
• I ran cargo fmt and cargo clippy before committing

[tvpeter]

@notmandatory, I’m wondering if we should update our signing approach since the current method is deprecated. If we choose to proceed with the update, we will need to add miniscript as a separate dependency, as the version exported from the Wallet (v2.3.0) is 12.3.1. We require at least version 13.0.0. Or should we postpone this update until a later version?

[notmandatory]

@notmandatory, I’m wondering if we should update our signing approach since the current method is deprecated. If we choose to proceed with the update, we will need to add miniscript as a separate dependency, as the version exported from the Wallet (v2.3.0) is 12.3.1. We require at least version 13.0.0. Or should we postpone this update until a later version?

Good question. I've created bitcoindevkit/bdk_wallet#405 to revert the deprecating of the signer mod. The new PSBT signer probably won't be available until bdk_wallet 3.1.

[vadim-anfv]

Related #243.

[vadim-anfv]

Blocked by #252.

[tvpeter]

Blocked by #252.

Hi @va-an, could you please verify that you tagged the correct PR? Thank you!

[vadim-anfv]

Blocked by #252.

Hi @va-an, could you please verify that you tagged the correct PR? Thank you!

All correct. I've also reproduced error from PR's CI build in my local run just pre-push:

error[E0061]: this method takes 1 argument but 2 arguments were supplied
--> src/payjoin/mod.rs:605:30
|
605 | ... .check_payment(
| ^^^^^^^^^^^^^
...
618 | / ... |outpoint| {
619 | | ... let utxo = self.wallet.get_utxo(outpoint);
620 | | ... match utxo {
621 | | ... Some(_) => Ok(false),
... |
624 | | ... }
| |___________________________- unexpected argument #2 of type `{closure@src/payjoin/mod.rs:618:33: 618:43}`
|
note: method defined here
--> /home/vaan/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/payjoin-1.0.0-rc.2/src/core/receive/v2/mod.rs:1262:12
|
1262 | pub fn check_payment(
| ^^^^^^^^^^^^^
help: remove the extra argument
|
617 - },
618 - |outpoint| {
619 - let utxo = self.wallet.get_utxo(outpoint);
620 - match utxo {
621 - Some(_) => Ok(false),
622 - None => Ok(true),
623 - }
|

I've applied the fix from #252 to my local code to verify that it helps.

[vadim-anfv]

@tvpeter#252 is merged now, so this should be fine now.
Could you rebase on master? Other PRs will likely need to rebase on top of this once it's merged.

[codecov]

Codecov Report

❌ Patch coverage is 0% with 92 lines in your changes missing coverage. Please review.
✅ Project coverage is 11.36%. Comparing base (50d248c) to head (eea5ba9).
⚠️ Report is 8 commits behind head on master.

Files with missing lines	Patch %	Lines
src/handlers.rs	0.00%	60 Missing ⚠️
src/utils.rs	0.00%	32 Missing ⚠️

Additional details and impacted files

@@ Coverage Diff @@## master #248 +/- ##
==========================================
+ Coverage 10.21% 11.36% +1.15% 
==========================================
Files 8 8 Lines 2713 2807 +94 ==========================================
+ Hits 277 319 +42 - Misses 2436 2488 +52 

Flag	Coverage Δ
rust	11.36% <0.00%> (+1.15%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[vadim-anfv]

Thanks for the rebase, @tvpeter!

Unfortunately, CI is now failing because bdk_wallet 2.3.0 introduced a deprecation that causes warnings-as-errors to fail. The revert has already been merged but 2.4.0 hasn't been released yet.

Pinning back to 2.1.0 won't work either - this branch already uses apply_update_events which was introduced in 2.3.0, so downgrading breaks the build:

error[E0599]: no method named `apply_update_events` found for mutable reference `&mut Wallet`

So it looks like we're stuck waiting for 2.4.0 to be released. Until then, CI will fail on this PR (and on any PR that rebases after merging this one).
We could just keep this in mind during reviews and merges - the CI failure is a known issue, not a code problem.

[vadim-anfv]

utACK 4a41057

[vadim-anfv]

We could use tap for convenience:

Suggested change

	let wallet_events = wallet
	.apply_update_events(update)
	.map_err(|e| Error::Generic(e.to_string()))?;
	print_wallet_events(&wallet_events);
	Ok(())
	wallet
	.apply_update_events(update)
	.map_err(|e| Error::Generic(e.to_string()))?
	.tap(print_wallet_events);
	Ok(())

[tvpeter]

While this will make it cleaner, I think it is not worth the cost of pulling in a whole library. I'm always for using a minimal number of external libraries.

[vadim-anfv]

We already pull in tap as a dependency in #225:

bdk-cli/Cargo.toml

Line 30 in 0a771ca

tap = "1.0.1"

So this change doesn't add any extra cost.

That said, if you'd prefer, we can drop the dependency.

[tvpeter]

Maybe we can drop in another PR. Generally prefer to avoid deps cost.

[tvpeter]

Unfortunately, CI is now failing because bdk_wallet 2.3.0 introduced a deprecation that causes warnings-as-errors to fail.

Yes, I updated the warnings to be treated as errors in this commit 8d20cf2 because many Clippy warnings were being neglected and had to be merged into master. It's not coming from bdk_wallet

[vadim-anfv]

Yes, I updated the warnings to be treated as errors in this commit 8d20cf2 because many Clippy warnings were being neglected and had to be merged into master. It's not coming from bdk_wallet

The deny(warnings) change is great, glad you added it - lax warnings tend to pile up fast. The issue here is a different thing though: bdk_wallet 2.3.0 itself emits a deprecation warning on its own API (apply_update), and that warning hits the deny(warnings) flag. It'll be resolved once 2.4.0 is released (bitcoindevkit/bdk_wallet#405).

[notmandatory]

Unfortunately, CI is now failing because bdk_wallet 2.3.0 introduced a deprecation that causes warnings-as-errors to fail. The revert has already been merged but 2.4.0 hasn't been released yet.

The Wallet 2.4.0 lib was just published and should fix this issue

[vadim-anfv]

ACK 5def41f

Also noted that the PR description mentions "Update bdk_electrum to v0.24.0", but I don't see it in the changes - Cargo.toml (and Cargo.lock) still have bdk_electrum 0.23.2.

[vadim-anfv]

We already pull in tap as a dependency in #225:

bdk-cli/Cargo.toml

Line 30 in 0a771ca

tap = "1.0.1"

So this change doesn't add any extra cost.

That said, if you'd prefer, we can drop the dependency.

[tvpeter]

ACK 5def41f

Also noted that the PR description mentions "Update bdk_electrum to v0.24.0", but I don't see it in the changes - Cargo.toml (and Cargo.lock) still have bdk_electrum 0.23.2.

Seen the reason now. I amended the last commit and did not push. Thanks for catching that.

[tvpeter]

Description

This PR updates the Wallet API to v2.4.0 and other dependencies. It also adds WalletEvent to both sync and full_scan for all backends.

Fixes#243

Changelog notice

• Update bdk_wallet to v2.4.0
• Add WalletEvents to all backends Esplora, Electrum, Rpc and CBF.
• Add print_wallet_events function to print wallet events to the terminal during sync
• Update bdk_bitcoind_rpc to v0.22.0
• Update bdk_electrum to v0.24.0
• Update bdk_kyoto to v0.15.4
• Update bdk_esplora to v0.22.2

Checklists

All Submissions:

• I've signed all my commits
• I followed the contribution guidelines
• I ran cargo fmt and cargo clippy before committing

[tvpeter]

@notmandatory, I’m wondering if we should update our signing approach since the current method is deprecated. If we choose to proceed with the update, we will need to add miniscript as a separate dependency, as the version exported from the Wallet (v2.3.0) is 12.3.1. We require at least version 13.0.0. Or should we postpone this update until a later version?

[notmandatory]

@notmandatory, I’m wondering if we should update our signing approach since the current method is deprecated. If we choose to proceed with the update, we will need to add miniscript as a separate dependency, as the version exported from the Wallet (v2.3.0) is 12.3.1. We require at least version 13.0.0. Or should we postpone this update until a later version?

Good question. I've created bitcoindevkit/bdk_wallet#405 to revert the deprecating of the signer mod. The new PSBT signer probably won't be available until bdk_wallet 3.1.

[vadim-anfv]

Related #243.

[vadim-anfv]

Blocked by #252.

[tvpeter]

Blocked by #252.

Hi @va-an, could you please verify that you tagged the correct PR? Thank you!

[vadim-anfv]

Blocked by #252.

Hi @va-an, could you please verify that you tagged the correct PR? Thank you!

All correct. I've also reproduced error from PR's CI build in my local run just pre-push:

error[E0061]: this method takes 1 argument but 2 arguments were supplied
--> src/payjoin/mod.rs:605:30
|
605 | ... .check_payment(
| ^^^^^^^^^^^^^
...
618 | / ... |outpoint| {
619 | | ... let utxo = self.wallet.get_utxo(outpoint);
620 | | ... match utxo {
621 | | ... Some(_) => Ok(false),
... |
624 | | ... }
| |___________________________- unexpected argument #2 of type `{closure@src/payjoin/mod.rs:618:33: 618:43}`
|
note: method defined here
--> /home/vaan/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/payjoin-1.0.0-rc.2/src/core/receive/v2/mod.rs:1262:12
|
1262 | pub fn check_payment(
| ^^^^^^^^^^^^^
help: remove the extra argument
|
617 - },
618 - |outpoint| {
619 - let utxo = self.wallet.get_utxo(outpoint);
620 - match utxo {
621 - Some(_) => Ok(false),
622 - None => Ok(true),
623 - }
|

I've applied the fix from #252 to my local code to verify that it helps.

[vadim-anfv]

@tvpeter#252 is merged now, so this should be fine now.
Could you rebase on master? Other PRs will likely need to rebase on top of this once it's merged.

[codecov]

Codecov Report

❌ Patch coverage is 0% with 92 lines in your changes missing coverage. Please review.
✅ Project coverage is 11.36%. Comparing base (50d248c) to head (eea5ba9).
⚠️ Report is 8 commits behind head on master.

Files with missing lines	Patch %	Lines
src/handlers.rs	0.00%	60 Missing ⚠️
src/utils.rs	0.00%	32 Missing ⚠️

Additional details and impacted files

@@ Coverage Diff @@## master #248 +/- ##
==========================================
+ Coverage 10.21% 11.36% +1.15% 
==========================================
Files 8 8 Lines 2713 2807 +94 ==========================================
+ Hits 277 319 +42 - Misses 2436 2488 +52 

Flag	Coverage Δ
rust	11.36% <0.00%> (+1.15%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[vadim-anfv]

Thanks for the rebase, @tvpeter!

Unfortunately, CI is now failing because bdk_wallet 2.3.0 introduced a deprecation that causes warnings-as-errors to fail. The revert has already been merged but 2.4.0 hasn't been released yet.

Pinning back to 2.1.0 won't work either - this branch already uses apply_update_events which was introduced in 2.3.0, so downgrading breaks the build:

error[E0599]: no method named `apply_update_events` found for mutable reference `&mut Wallet`

So it looks like we're stuck waiting for 2.4.0 to be released. Until then, CI will fail on this PR (and on any PR that rebases after merging this one).
We could just keep this in mind during reviews and merges - the CI failure is a known issue, not a code problem.

[vadim-anfv]

utACK 4a41057

[vadim-anfv]

We could use tap for convenience:

Suggested change

	let wallet_events = wallet
	.apply_update_events(update)
	.map_err(|e| Error::Generic(e.to_string()))?;
	print_wallet_events(&wallet_events);
	Ok(())
	wallet
	.apply_update_events(update)
	.map_err(|e| Error::Generic(e.to_string()))?
	.tap(print_wallet_events);
	Ok(())

[tvpeter]

While this will make it cleaner, I think it is not worth the cost of pulling in a whole library. I'm always for using a minimal number of external libraries.

[vadim-anfv]

We already pull in tap as a dependency in #225:

bdk-cli/Cargo.toml

Line 30 in 0a771ca

tap = "1.0.1"

So this change doesn't add any extra cost.

That said, if you'd prefer, we can drop the dependency.

[tvpeter]

Maybe we can drop in another PR. Generally prefer to avoid deps cost.

[tvpeter]

Unfortunately, CI is now failing because bdk_wallet 2.3.0 introduced a deprecation that causes warnings-as-errors to fail.

Yes, I updated the warnings to be treated as errors in this commit 8d20cf2 because many Clippy warnings were being neglected and had to be merged into master. It's not coming from bdk_wallet

[vadim-anfv]

Yes, I updated the warnings to be treated as errors in this commit 8d20cf2 because many Clippy warnings were being neglected and had to be merged into master. It's not coming from bdk_wallet

The deny(warnings) change is great, glad you added it - lax warnings tend to pile up fast. The issue here is a different thing though: bdk_wallet 2.3.0 itself emits a deprecation warning on its own API (apply_update), and that warning hits the deny(warnings) flag. It'll be resolved once 2.4.0 is released (bitcoindevkit/bdk_wallet#405).

[notmandatory]

Unfortunately, CI is now failing because bdk_wallet 2.3.0 introduced a deprecation that causes warnings-as-errors to fail. The revert has already been merged but 2.4.0 hasn't been released yet.

The Wallet 2.4.0 lib was just published and should fix this issue

[vadim-anfv]

ACK 5def41f

Also noted that the PR description mentions "Update bdk_electrum to v0.24.0", but I don't see it in the changes - Cargo.toml (and Cargo.lock) still have bdk_electrum 0.23.2.

[vadim-anfv]

We already pull in tap as a dependency in #225:

bdk-cli/Cargo.toml

Line 30 in 0a771ca

tap = "1.0.1"

So this change doesn't add any extra cost.

That said, if you'd prefer, we can drop the dependency.

[tvpeter]

ACK 5def41f

Also noted that the PR description mentions "Update bdk_electrum to v0.24.0", but I don't see it in the changes - Cargo.toml (and Cargo.lock) still have bdk_electrum 0.23.2.

Seen the reason now. I amended the last commit and did not push. Thanks for catching that.