chore: should we remove unused `SignerError` variants ?

[oleonardolima]

Describe the enhancement

As noticed in bitcoindevkit/bdk#1448 and bitcoindevkit/bdk#1424 there are some unused error variants on SignerError

Use case

Do we need such variants, e.g sighash::TaprootError, transaction::TxInputsIndexError, or should it be merged with InputIndexOutOfRange (for example) ?

Additional context

Issue meant for related discussion.

[ValuedMammal]

Do you want to make this into a super issue to figure out further improvements to the signer module as a whole?

[oleonardolima]

Do you want to make this into a super issue to figure out further improvements to the signer module as a whole?

That could work, I don't see a problem doing it that way.

You mean also considering Steve's comments?

[LLFourn]

Do you want to make this into a super issue to figure out further improvements to the signer module as a whole?

The end goal is to remove the Signer trait and to remove Signers from wallet in favor of a library that helps you sign PSBTs with whatever key material you have. I think we'll tackle this during transaction building which comes in v2.

[ValuedMammal]

Playing with the idea of removing signers from the Wallet structure, I naively imagined simply adding a signer parameter to the Wallet::sign method thinking the user could construct their own SignersContainer just before signing. I realized that the wallet signers are used in create_tx in order to extract_policy which itself is somewhat of a rabbit hole, although it's curious that extracting a policy seems to works fine even when the wallet has no signers.. so maybe there's a way to disentangle that logic. Anyway I see this issue being related to the policy vs plan discussion, which in turn implies a tx-builder redesign. Still brainstorming ways to divide the problem into chunks we can easily tackle.

Regarding the proposal to remove the TransactionSigner trait, I think that would also simplify the whole concept of SignersContainer. Maybe we keep a sign method on Wallet that takes in a keymap or similar and internally calls Psbt::sign.

[notmandatory]

Ok to remove this from 1.0.0-beta milestone? Based on above it sounds like this should likely be fixed in a 2.0 release when signer is removed from wallet.

[oleonardolima]

Ok to remove this from 1.0.0-beta milestone? Based on above it sounds like this should likely be fixed in a 2.0 release when signer is removed from wallet.

Yes, as Lloyd and Mammal mentioned above it'll probably rely on TxBuilder redesign, which is already intended for v2 (please correct me if I'm wrong). Also, do we already have an issue for that?

[ValuedMammal]

Ok to remove this from 1.0.0-beta milestone? Based on above it sounds like this should likely be fixed in a 2.0 release when signer is removed from wallet.

Yes, as Lloyd and Mammal mentioned above it'll probably rely on TxBuilder redesign, which is already intended for v2 (please correct me if I'm wrong). Also, do we already have an issue for that?

These are some tx-builder issues that I'm aware of #164 #151

[ValuedMammal]

These are some ideas I'm tracking for updating the signer module.

• Remove unused SignerError variants (this issue)
• Remove method Wallet::add_signer with the goal of taking TransactionSigner and SignersContainer out of the public API. But I want to verify whether we have users who rely on things like add_signer and SignerOrdering, etc.
• Change visibility of signer module (make it private).
• Implement GetKey for KeyMap rust-bitcoin/rust-miniscript#765
• Add method Wallet::sign_psbt that internally uses Psbt::sign