Need generic type or similar change for VerificationOpts

[BlinkyStitt]

In my first pass at upgrading siwe to use ethers 2, I changed siwe to accept an Arc. My code worked with this but it changed the api. Prestwich then showed us how to have a contract that took a reference, so I removed the Arc. It compiled with that change, but I didn't test it with my app.

The problem now is that my app currently has the provider in an Arc. So I can pass either an Arc<Provider<Http>> or a &Provider<Http>, but VerificationOpts currently wants a Provider<Http>.

So something about VerificationOpts needs to change. I'm not sure the best option.

Maybe change it to take a reference? Maybe use the maybe-owned crate?

We can also use this as a chance to let it accept other connection types instead of just Http (I would like to use IPC for my app, but websockets are also common).

[prestwich]

suggestion: instead of having the opts own a provider, make a trait and capture the provider calls within the trait, and blanket implement the trait over M: Middleware?

trait SiweMiddlewareExt: Middleware {
    fn do_thing(&self, &VerificationOpts) { /* do the thing here */ }
}

impl SiweMiddlewareExt for M where M: Middleware {}

[chunningham]

something like this? I think that would be a very useful pattern, the one hangup after trying this out is managing the generics on VerificationOpts in async blocks in the tests, but I'm sure users will know what they want to go with when they're using it. I'll discuss this approach with our team

[prestwich]

Mmm I am not verify familiar with the codebase, but it's not really idiomatic to put a generic communication mechanism into an __Opts struct. it pushes the fallible async things deep down the stack. Instead, I'd keep all the local computation as a unit and move the provider to allow it to be a user implementation detail

It is idiomatic to put all the async methods onto the object that manages/causes the async computation. This also allows you to avoid cfg blocks in the middle of functions (they get moved to the trait as a whole)

Idiomatic version might look like this

struct Message { .. } // not generic
struct VerificationOpt { .. } // not generic

// all local computation in one method
impl Message { fn verify_local(&self, opts: &VerificationOpts ); }

#[cfg(feature = "ethers")]
#[async_trait] // optional dep for ethers feature
trait SiweMiddlewareExt: Middleware {
   async fn verifiy_eip1271(&self,     address: [u8; 20],
    message_hash: &[u8; 32],
    signature: &[u8],
  )  -> Result<_, _> { .. }

  #[cfg(feature = "ethers")]
  async fn verify_full(&self, message: &Message, opts: &VerificationOpts) -> Result<_, _> { 
        message.verify_local()?; // all non-communication checks
        self.verify_eip1271().await?;  // then communication checks
  }
}

impl SiweMiddlewareExt for M where M: Middleware {}

[chunningham]

This does improve the ergonomics of the opts compared to that other solution, I'll raise it. If you'd like pr authorship, we are also happy to review a pr.

[prestwich]

I probably won't be able to get to it for several days. if you have time, you should

This is of course, a breaking API change