[bvotteler]

Description

Fixes #380.

Propagate an error rather than panic when derive_sp returns an error inside create_sp_tx.

This is done by:

• adding implementation for trait: From<SpSendError> for BDKCliError
• replacing derive_sp([..]).expect([..]); with derive_sp([..])?;
• adding a test for the scenario described in create_sp_tx panics on derivation failure instead of returning an error #380

Expand: output before & after

Output before:

$ target/debug/bdk-cli wallet --wallet wsh_wallet create_sp_tx \
  --to-sp sprt1qqtnxgt7kn0fpr7fl0u0ndjj35f499y8t9hgmpkp8n2rmkr2gpjzyxqe3qp5n5apt0c5fas9xwy7002l0zqsuhrv7y9clz8yvhnmvut83lypguwer:20000
thread 'main' (3008870) panicked at src/handlers/offline.rs:508:76:
will fix later: MissingInputsForSharedSecretDerivation
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

(For output with backtrace, see issue description for #380)

Output after:

$ target/debug/bdk-cli wallet --wallet wsh_wallet create_sp_tx \
  --to-sp sprt1qqtnxgt7kn0fpr7fl0u0ndjj35f499y8t9hgmpkp8n2rmkr2gpjzyxqe3qp5n5apt0c5fas9xwy7002l0zqsuhrv7y9clz8yvhnmvut83lypguwer:20000
Error: Cannot create silent payment transaction: No available inputs for shared secret derivation

Notes to the reviewers

• I wasn't readily able to come up with a test setup to hit the other branches (SinglePubKey::XOnly and DescriptorSecretKey::XPrv), either.
  The propagation method will do the same, either way. Therefore, I didn't try too hard to fabricate those scenarios. Happy to address that if preferred.
• I added tests to tests/integration/online.rs despite the handler being in src/handlers/offline.rs, simply because that is where the currently existing silent payment tests live.
  Happy to move the test to tests/integrtion/offline if that is preferable.

Checklists

All Submissions:

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

Bugfixes:

• I've added tests to reproduce the issue which are now passing
• I'm linking the issue being fixed by this PR

[codecov]

Codecov Report

❌ Patch coverage is 66.66667% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 59.66%. Comparing base (5558555) to head (995999e).
⚠️ Report is 1 commits behind head on master.

Files with missing lines	Patch %	Lines
src/handlers/offline.rs	66.66%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #388      +/-   ##
==========================================
- Coverage   60.00%   59.66%   -0.35%     
==========================================
  Files          25       25              
  Lines        4736     4552     -184     
==========================================
- Hits         2842     2716     -126     
+ Misses       1894     1836      -58     

Flag	Coverage Δ
rust	59.66% <66.66%> (-0.35%)	⬇️

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.

[tvpeter]

Thank you @bvotteler for working on this fix.

I have left some comments

[tvpeter]

Similar occurrence here, so check if you can fix it as well

[bvotteler]

I can fix it (on it), but to add a test I think I would need a refactor that method to be able to set up a test.
Let me know if that would be useful in this PR, or to be added in a stacked PR. Or even better: If you can come up with a descriptor or different test setup that would make me hit that line naturally.

[bvotteler]

Fixed the occurrence in fa33cba .

Just need to know if I should (or even need to) refactor the method to be able to test as part of this PR or split the refactor off to a separate one.

[tvpeter]

Also, please fix the PR title, it does not follow the Conventional Commit style.