This doesn't sit well with me. I don't understand "how is PartiallySignedTransaction a Signer?". Seems to me that partially signed transaction should be an object/argument of signing and thus ... passed to signer?

I was not paying attention to the matter, so I might be just confused. I'm just skimming through what's going on in the rust-bitcoin project.

So the goal with the BIP-174 support is to make it hard for people to mess up the workflow?

Should it then be something more like session types? With PartiallySignedTransaction remembering its state either at runtime, or in the type itself PartiallySignedTransaction<State>?

And then the question is which parts of the role should be swappable and which ones are always the same.

verify() could be a method on a PartiallySignedTransaction if the verification is always the same. Does it make sense to have completely different verification for different implementations/users of rust-bitcoin?

Is it necessary to even have a trait Signer? Is there anything that actually needs to work on any, arbitrarily provided signer or are these always going to be concrete?

"PartiallySignedTransaction accessed from a Signer role perspective"

Why we need roles? Well, we have them in Bitcoin Core, and the purpose of this repo is to follow it. Also, it simplifies passing PSBT around, such that wallets using rust-bitcoin may have frunctions accepting as a parameter some specific role of PSBTs only (fn(psbt: impl Signer)).

This is the third iteration, in the previous two we had both of your suggestions: more strirct workflow and PSBT as a parameter to signer object (#456 #457). Here, we have only an ability to add existing signature (to avoid complexity required by async methods) and we will have another Sync/AsyncSigner structures for creating signatures with given full PSBT object, which will internally call add_signature method from this implementaiton.

Is there more than just one way to add an existing signature to PSBT? If not, there is no point of having a trait. A method on PSBT would do, no? The reason to have a trait is to allow extending something with alternative implementations.

The responsibility is role of someone doing something with PSBT, not a state or function of PSBT, no?

Sync and Async do not generalize well in Rust. You might have to do trait Signer and trait AsyncSigner etc.

Just a note: the same stuff we are discussing here applies not only to Signer trait, but other role-specific traits defined in this PR.

So, for most of them, they will be implemented not only for PartiallySignedTransaction, but also for Input and Output structs; so yes; we probably need them as traits. Also, I'd like to give wallets ability to limit the scope of what can be done with PSBT object passed as parameter, and by providing traits (even if there is only one structure implementing it) we can do that with generics.

Most probably if trait Signer is useful, it should be something like:

fn sign(tx: PSBT) -> PSBT;

or

fn sign(tx: PSBT) -> SomeStuffThatYouNeedToPassToCombiner;

The signer is responsible for coming up with the signature, and can't expect to be given it. A struct MyKeyIsOnTheDisk will produce signature from a locally stored secret key, and HSMSigner will ask HSM to do it or something. Any neccessary arguments would be provided in a constructor of the concrete type implementing trait Signer .

Code writing over generic Signer just cares that it has "some singer" that was somehow created by something, and now it can use it to sign stuff without knowing what it really is.

Pls see my comment above; this is not a generic Signer but a Signer role cross-section of PSBT (used as roles::Signer). There will be a Sync/AsyncSigner trait outside of the role scope that will be taking PSBT in a Signer role as an arbument.

What we can think of is renaming roles into adjectives, like Signer -> Signable and Combiner -> Combinable

Oh. These names would be much better.