(4.0) fix: return structured insufficient funds errors

[reez]

Description

We already expose a structured CreateTxError::InsufficientFunds containing needed and available amounts.

Currently are converting the errors into the generic string based CoinSelection variant though.

So this PR just maps the upstream amounts to the existing InsufficientFunds and adds regression coverage.

Notes to the reviewers

Documentation

• bdk_wallet

https://docs.rs/bdk_wallet/3.1.0/bdk_wallet/coin_selection/struct.InsufficientFunds.html

https://docs.rs/bdk_wallet/3.1.0/src/bdk_wallet/wallet/coin_selection.rs.html#123-132

https://docs.rs/bdk_wallet/3.1.0/bdk_wallet/error/enum.CreateTxError.html#variant.CoinSelection

• bitcoin
• uniffi
• Other:

Changelog

Checklists

All Submissions:

• I've signed all my commits
• I followed the contribution guidelines
• I ran cargo fmt and cargo clippy before committing
• I've added exactly one changelog:* label
• I've linked the relevant upstream docs or specs above

New Features:

• I've added tests for the new feature
• I've added docs for the new feature

Bugfixes:

• This pull request breaks the existing API
• I've added tests to reproduce the issue which are now passing
• I'm linking the issue being fixed by this PR

[Johnosezele]

ACK d80034a

[thunderbiscuit]

I'm trying to dig into this one and I think we might have had an extra variant that cannot be hit?

If you look at the CreateTxError in Rust, you find a CreateTxError::CoinSelection variant: https://docs.rs/bdk_wallet/3.1.0/bdk_wallet/error/enum.CreateTxError.html#variant.CoinSelection. This variant contains the InsufficientFunds struct.

Here instead we have InsufficientFunds as an error variant, and the CreateTx::CoinSelection variant can never really be returned anywhere as far as I see.

I think we can't return full structs inside our errors (wait can we with the new 0.31? need to check), and in my mind the CreateTxError::InsufficientFunds we are currently using is the most intuitive way to return this to the user (rather than putting the fields on a CreateTxError::CoinSelection variant), but just want to point it out, and suggest we remove the CreateTxError::CoinSelection variant as part of this PR.

[thunderbiscuit]

Ok following up on this:

1. Yes we can return fields with full complex structs in our errors! Not needed here IMO but it opens up the space of what we can do with the errors, and adds to my neverending list of things to take a look at.
2. According to Claude this is unfortunately a breaking change, because although not a compilation breaking change, behaviouraly if you previously expected to get this "insufficient funds" data through the CreateTxError::CoinSelection and your were matching on it, and now that variant never returns anymore and instead this info comes in from the CreateTxError::InsufficientFunds variant, you wouldn't know about it and not get the info you previously were able to get.
3. When we do add it as a 4.0 breaking change, I would remove the CreateTxError::CoinSelection variant at the same time.

[reez]

Agree. Adding to 4.0 tracking, and we can get this just how we want it for 4.0