Let anchor-client use any Signer instead of only Keypair

[dnut]

The rust anchor client can only work with a Keypair as the payer, which means the private key must be loaded into any client. This is a deal breaker for a lot of use cases - for example, any use of a hardware wallet. So I only find this library useful for building a proof-of-concept, and I'm forced to fork the library with these changes if I want to build a tool that can be used on mainnet.

The change in this PR allows clients to use any Signer.

EDIT: See discussion below, I changed it to use Rc<dyn Signer> instead of references to retain backwards compatibility. The Client constructors break compatibility however so we can have a clean api.

Original Comment:

It is implemented using &'a dyn Signer (same as the vec of signers used in RequestBuilder). This means an 'a lifetime parameter was added to Client, Program, and RequestBuilder, which are all public structs, so backwards compatibility is affected for some usages of these types. I view this as a necessary evil because the current behavior is unacceptable, and I see no way to fix the issue while retaining full backwards compatibility.

[fanatid]

Why dynamic dispatch instead of static?

[dnut]

Mainly because it retains more backwards compatibility by keeping the struct's parameters simpler. In this implementation I just added a lifetime parameter, but for static dispatch, I would have to add a lifetime parameter and a type parameter. Lifetime parameters can be omitted in some situations where type parameters cannot be omitted.

[fanatid]

Type parameter also can be omitted if we set the default type. Not sure that I understand why we need lifetime in addition to type parameter 🤔

struct Config<S: Signer = Keypair> {
    cluster: Cluster,
    payer: S,
    options: Option<CommitmentConfig>,
}

[dnut]

default type

That's cool. I wasn't aware of that feature in rust.

Not sure that I understand why we need lifetime in addition to type parameter

The payer needs to be stored somewhere. Client, Program, and RequestBuilder each need access to it, and none of them nest each other. Since Client instantiates Program, and Program instantiates RequestBuilder, either Client needs to own it and the others store a reference, or a broader context owns it and all of them store references to it. Alternatively, the payer could be cloned or copied but that's not ideal because it imposes another type constraint.

[dnut]

Another issue is that the solana libraries return dyn Signer. You can't pass that into something that expects a static type. For example, see solana_clap_utils::keypair::DefaultSigner::signer_from_path. This is how the solana cli loads a wallet. I found it very useful to use the same code with my own forked RequestBuilder so my multisig cli is compatible with anything the solana cli is compatible with and can be configured the same way.

[dnut]

Actually, we don't need to add lifetime parameters if we use smart pointers instead of references. And if we store the payer as Rc<dyn Signer> in all the structs, then we also have compatibility with the official Solana libraries.

The complication is with Client::new and Client::new_with_options. They won't be compatible with Solana libraries unless they can accept a reference or smart pointer. But if we change those function signatures, that means any usages of those functions will have to change to wrap the value in the smart pointer. It's a trivial change, but a break in backwards compatibility nonetheless.

The solution that fully retains backwards compatibility while also supporting the Solana libraries is to add new constructors to Client that accept a smart pointer while only deprecating the existing functions. The cost is that it clutters the API.

What do we value more, backwards compatibility or a clean API?

I updated the PR to use Rc<dyn Signer> with additional constructors for Client.

[armaniferrante]

What do we value more, backwards compatibility or a clean API?

Clean API. Happy to make a breaking change.

[dnut]

Alright. I took out the extra constructors I just added, and changed the original ones to be more generic.

[armaniferrante]

Can you add a CHANGELOG Breaking entry (I'm unable to push to this branch)?

[dnut]

I added a note for the breaking change in the unreleased version 0.19.0. Is this sufficient or should I add instructions about how usages need to change?

[dnut]

The repo has this new requirement that didn't exist when I first created the PR:

The base branch requires all commits to be signed.

The easiest way seems to be squashing all the commits into a single commit and signing that one, then force pushing. So that's what I did just now. But it's the exact same change otherwise.

[armaniferrante]

Need to move this into the unrelased section.

[armaniferrante]

Need to merge master for the build to pass. I tried but I don't have write access to the fork.