Ensure a consistent chain ID between a signer and provider in SignerMiddleware

[Rjected]

Motivation

Frequently users run into errors returned by a Provider that are caused by chain ID inconsistencies between the user's Signer and Provider. For example, if the user tries to sign/send a transaction using a SignerMiddleware instantiated with a Signer and Provider that have different chain IDs, they might get this error from the provider:

Error: (code: -32700, message: Invalid signature v value, data: None)

Since users often use a SignerMiddleware to interact with both a provider and signer, it would be useful to ensure that the Signer uses the same chain ID as the Provider, to prevent this type of user error. Other approaches were discussed in a different issue.

Solution

Update the SignerMiddleware constructor and with_signer method to set the passed Signer's chain ID to the ID retrieved from the provider.
This changes the following SignerMiddleware methods:

Old:

    pub fn new(inner: M, signer: S) -> Self {
        // constructor...
    }

    pub fn with_signer(&self, signer: S) -> Self {
        // implementation...
    }

New:

    pub async fn new(inner: M, signer: S) -> Result<Self, SignerMiddlewareError<M, S>> {
        // constructor...
        // pulls the chain id from the inner middleware
    }

    pub async fn with_signer(&self, signer: S) -> Result<Self, SignerMiddlewareError<M, S>> {
        // implementation...
    }

We also have removed the #[must_use] attribute from with_signer, since it doesn't do anything for async functions.

Importantly, if a chain_id cannot be retrieved from the provider (due to a provider error or some other reason), we bubble up the error.

Previously, the implementation details of the provider were not relevant to the creation of a SignerMiddleware.
Now, the SignerMiddleware must be constructed with a provider with an implemented and working get_chainid method, or new/with_signer will return an error rather than a new SignerMiddleware.

Additionally, this breaks current uses of SignerMiddleware, since the Result must be handled, and each method is now async, since we're now interacting with a provider.

Tests are added which set a custom chain ID for Ganache and make sure the signer properly pulls the custom chain ID. To set a chain ID on Ganache, it had to be updated. There may be some differences in behavior between the two versions, so we should watch for test failures after re-enabling the relevant section of CI.

PR Checklist

• Added Tests
• Added Documentation
• Updated the changelog

[Rjected]

Seems like the updated ganache behaves differently and caused the run to fail, the test probably needs to be updated:

failures:

---- eth_tests::deploy_and_call_contract stdout ----
thread 'eth_tests::deploy_and_call_contract' panicked at 'assertion failed: `(left == right)`
  left: `56568`,
 right: `38856`', ethers-contract/tests/contract.rs:72:9
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace


failures:
    eth_tests::deploy_and_call_contract

test result: FAILED. 34 passed; 1 failed; 1 ignored; 0 measured; 0 filtered out; finished in 14.91s

error: test failed, to rerun pass '-p ethers-contract --test contract'

[mattsse]

cc @gakonst

[mattsse]

this is definitely an improvement but this breaks, and will be redundant if the user manually sets the chain id without requesting it from the inner middleware.

I'd rather like to make this a separate asynchronous function that does this. basically with_signer

[mattsse]

here for example, in this scenario, we're now querying the chain id twice

[gakonst]

I think moving to a new method instead of a breaking change makes sense here. Not sure what the best name for the function is, something that declares that it auto detects chain id ideally

[Rjected]

Just switched to a new method new_with_provider_chain:

/// Creates a new client from the provider and signer.
/// Sets the address of this middleware to the address of the signer.
/// Sets the chain id of the signer to the chain id of the inner [`Middleware`] passed in,
/// using the [`Signer`]'s implementation of with_chain_id.
///
/// [`Middleware`] ethers_providers::Middleware
/// [`Signer`] ethers_signers::Signer
pub async fn new_with_provider_chain(
    inner: M,
    signer: S,
) -> Result<Self, SignerMiddlewareError<M, S>> {
    let address = signer.address();
    let chain_id =
        inner.get_chainid().await.map_err(|e| SignerMiddlewareError::MiddlewareError(e))?;
    let signer = signer.with_chain_id(chain_id.as_u64());
    Ok(SignerMiddleware { inner, signer, address })
}

Edited the comment on new to point to this method & explain what might happen if the chainid is not consistent.
Also needed to remove an assert in deploy_and_call_contract since the gas used & estimation are no longer equal with the updated ganache.

[gakonst]

Need to remove the formatting change in the doc comments, otherwise lgtm

[Rjected]

Removed indents and rebased

[gakonst]

gg thx!