Wrapping `Box<dyn Signer>` to make it Send + Sync?

[ebrightfield]

I'm having trouble working with anything other than a Keypair in the client constructor. Things like Box<dyn Signer> or RemoteWallet are not Send + Sync, so workarounds are needed.

My hope was to wrap in an Arc<Mutex> but no dice. What's the proper way to handle this?

The answer to this question has broader application than just this library, so I detailed more in this stack exchange question: https://solana.stackexchange.com/questions/3320/working-in-rust-with-signers-across-threads-t-signer-send-sync

[VegetarianOrc]

Good news! I think that we can actually remove the Send + Sync requirements from the type. I'm not sure why I slammed them on there, probably was just being overzealous. I can take a look at removing them sometime tomorrow!

That being said, your example from stack exchange is super close. You'd have to mark the trait object used as inner as Send + Sync as well. This compiles on my machine:

#[derive(Clone)]
pub struct ThreadsafeSigner {
    pub inner: Arc<RwLock<Box<dyn Signer + Send + Sync>>>,
}

impl Signer for ThreadsafeSigner {
    fn try_pubkey(&self) -> Result<Pubkey, SignerError> {
        Ok(self.inner.read().unwrap().pubkey())
    }

    fn try_sign_message(&self, message: &[u8]) -> Result<Signature, SignerError> {
        self.inner.read().unwrap().try_sign_message(message)
    }

    fn is_interactive(&self) -> bool {
        self.inner.read().unwrap().is_interactive()
    }
}

    let keypair: Arc<RwLock<Box<dyn Signer + Send + Sync>>> = Arc::new(RwLock::new(Box::new(
        read_keypair_file(KEYPAIR_PATH).expect("failed to load keypair at path"),
    )));

    let signer = ThreadsafeSigner { inner: keypair };
    let shdw_drive_client = ShadowDriveClient::new(signer, "https://ssc-dao.genesysgo.net");

I can write that up on stack exchange tomorrow as well if it works for you so LMK!

[ebrightfield]

Unfortunately, inner as Send + Sync only works if my inner is Send + Sync, which I could just use directly instead of a wrapper class if that were the case. But I'm trying to use e.g. a RemoteWallet, which unlike the Keypair is not Send + Sync.

But, either way, great news that I don't even need to bother with the Send + Sync! I'll just wait until you push the change.

I'm extremely grateful for this library's existence, by the way, thank you!

[VegetarianOrc]

@ebrightfield I'm glad you like the project! I appreciate you taking the time to file issues to make it better!

The main branch now has the Send + Sync requirement removed. Version 0.4.0 will be available on crates.io shortly!

[ebrightfield]

Thanks you can mark this issue closed if you want!

On a similar note, however, is it not more idiomatic to use a Box<dyn Signer> rather than a T: Signer in the client constructor? I'm not 100% sure, but this seems to be how I've usually seen it done all over the Solana code. And for my part, I had to create a wrapper class that holds my Box<dyn Signer> that I get back from --keypair on the command-line, because apparently Box<dyn Signer> doesn't count as satisfying the generic T: Signer.

It's a small difference and slightly off-topic from here, so sorry to raise it here. Thoughts, though?

[VegetarianOrc]

I'm honestly not 100% sure why Box<dyn Signer> doesn't satisfy T: Signer. I poked at it a little bit but didn't come to any conclusion.

My general approach, taken from the advice in Rust for Rustaceans, is to try to avoid choosing dynamic dispatch (trait objects like Box<dyn Signer> require a VTable lookup for method calls) for the users of a library. In application code I tend to use the trait objects for simplicity, but using the generic types in libraries is intended to allow users to user either concrete types or trait objects as appropriate for their use.

That being said, if we can't get to the bottom of why Box<dyn Signer> doesn't satisfy T: Signer, then I think it may make sense to update the SDK to use the trait object.

[ebrightfield]

That being said, if we can't get to the bottom of why Box doesn't satisfy T: Signer, then I think it may make sense to update the SDK to use the trait object.

Agreed 100%. Also don't know why Box<dyn Signer> doesn't satisfy Signer, I could've sworn I even saw an impl for it, but I guess I'm mis-remembering because looking at it now, there is none. https://docs.rs/solana-sdk/latest/solana_sdk/signer/trait.Signer.html

There is a From to turn a T: Signer into a Box<dyn Signer> though haha!

[ebrightfield]

Sorry to resurrect a dead ticket here, but I thought you might find this interesting.

I made a concrete signer type that parses out all the same* as the Solana CLI (usb://, prompt://, stdin://, file://, etc), but works for T: Signer situations instead of Box<dyn Signer>.

https://github.com/Jungle-Finance/jungle-fi-cli-utils/blob/master/extra-signers/src/concrete_signer.rs

Only caveat is that I changed the Presigner input syntax because I wasn't in love with how they did theirs, but that's a pretty rare edge case anyway. Also, it's totally decoupled from Clap, no ArgMatches have to go in!