Repository: gakonst/ethers-rs
Pull request #1095 (2022-03-30 00:35:16)

Ensure a consistent chain ID between a signer and provider in SignerMiddleware
## 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](https://github.com/gakonst/ethers-rs/pull/879#issuecomment-1032994226).

## 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**:
```rust
    pub fn new(inner: M, signer: S) -> Self {
        // constructor...
    }

    pub fn with_signer(&self, signer: S) -> Self {
        // implementation...
    }
```
**New**:
```rust
    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](https://github.com/rust-lang/rust/issues/78149).

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](https://github.com/gakonst/ethers-rs/blob/3a653472cfb484bab7ae831ad6e3664d24b4e916/.github/workflows/ci.yml#L179).

## PR Checklist

- [x] Added Tests
- [x] Added Documentation
- [x] Updated the changelog


Rjected: Seems like the updated ganache behaves differently and [caused the run to fail](https://github.com/gakonst/ethers-rs/runs/5746590085?check_suite_focus=true), 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'
```
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`:
```rust
/// 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`](https://github.com/gakonst/ethers-rs/pull/1095/files#diff-f7ed5ca95548046c8f53e9acec04d1412a8e5db4cece5f3500be57c7a8067359L72) since the gas used & estimation are no longer equal with the updated ganache.
Rjected: Removed indents and rebased

Patch:
diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml
index ae2f4731..2ceb2b44 100644
--- a/.github/workflows/ci.yml
+++ b/.github/workflows/ci.yml
@@ -21,12 +21,12 @@ jobs:
     steps:
       - name: Checkout sources
         uses: actions/checkout@v2
-      - name: Install ganache-cli
+      - name: Set up node
         uses: actions/setup-node@v1
         with:
           node-version: 10
       - name: Install ganache
-        run: npm install -g ganache-cli
+        run: npm install -g ganache
       - name: Install Solc
         run: |
             mkdir -p "$HOME/bin"
@@ -66,13 +66,13 @@ jobs:
     steps:
       - name: Checkout sources
         uses: actions/checkout@v2
-      - name: Install ganache-cli
+      - name: Set up node
         uses: actions/setup-node@v1
         with:
           node-version: 10
       # TODO: can we combine these shared steps in github actions?
       - name: Install ganache
-        run: npm install -g ganache-cli
+        run: npm install -g ganache
       - name: Install Solc
         run: |
             mkdir -p "$HOME/bin"
@@ -183,12 +183,12 @@ jobs:
   #   steps:
   #     - name: Checkout sources
   #       uses: actions/checkout@v2
-  #     - name: Install ganache-cli
+  #     - name: Set up node
   #       uses: actions/setup-node@v1
   #       with:
   #         node-version: 10
   #     - name: Install ganache
-  #       run: npm install -g ganache-cli
+  #       run: npm install -g ganache
   #     - name: Install Solc
   #       run: |
   #           mkdir -p "$HOME/bin"
diff --git a/CHANGELOG.md b/CHANGELOG.md
index 10c47343..45ed440c 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -234,6 +234,9 @@
 
 ### Unreleased
 
+- Ensure a consistent chain ID between a Signer and Provider in SignerMiddleware
+  [#1095](https://gakonst/ethers-rs/pull/1095)
+
 ### 0.6.0
 
 - add the missing constructor for `Timelag` middleware via
diff --git a/ethers-middleware/src/signer.rs b/ethers-middleware/src/signer.rs
index c4b72a2e..746d2979 100644
--- a/ethers-middleware/src/signer.rs
+++ b/ethers-middleware/src/signer.rs
@@ -59,7 +59,7 @@ use thiserror::Error;
 /// # }
 /// ```
 ///
-/// [`Provider`]: ethers_providers::Provider
+/// [`Signer`]: ethers_signers::Signer
 pub struct SignerMiddleware<M, S> {
     pub(crate) inner: M,
     pub(crate) signer: S,
@@ -107,12 +107,23 @@ where
     S: Signer,
 {
     /// Creates a new client from the provider and signer.
+    /// Sets the address of this middleware to the address of the signer.
+    /// The chain_id of the signer will not be set to the chain id of the provider. If the signer
+    /// passed here is initialized with a different chain id, then the client may throw errors, or
+    /// methods like `sign_transaction` may error.
+    /// To automatically set the signer's chain id, see `new_with_provider_chain`.
+    ///
+    /// [`Middleware`] ethers_providers::Middleware
+    /// [`Signer`] ethers_signers::Signer
     pub fn new(inner: M, signer: S) -> Self {
         let address = signer.address();
         SignerMiddleware { inner, signer, address }
     }
 
-    /// Signs and returns the RLP encoding of the signed transaction
+    /// Signs and returns the RLP encoding of the signed transaction.
+    /// If the transaction does not have a chain id set, it sets it to the signer's chain id.
+    /// Returns an error if the transaction's existing chain id does not match the signer's chain
+    /// id.
     async fn sign_transaction(
         &self,
         tx: TypedTransaction,
@@ -148,6 +159,7 @@ where
         &self.signer
     }
 
+    /// Builds a SignerMiddleware with the given Signer.
     #[must_use]
     pub fn with_signer(&self, signer: S) -> Self
     where
@@ -160,6 +172,24 @@ where
         this
     }
 
+    /// 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 })
+    }
+
     fn set_tx_from_if_none(&self, tx: &TypedTransaction) -> TypedTransaction {
         let mut tx = tx.clone();
         if tx.from().is_none() {
@@ -329,7 +359,12 @@ mod tests {
         .into();
         let chain_id = 1u64;
 
-        let provider = Provider::try_from("http://localhost:8545").unwrap();
+        // Signer middlewares now rely on a working provider which it can query the chain id from,
+        // so we make sure ganache is started with the chain id that the expected tx was signed
+        // with
+        let ganache =
+            Ganache::new().args(vec!["--chain.chainId".to_string(), chain_id.to_string()]).spawn();
+        let provider = Provider::try_from(ganache.endpoint()).unwrap();
         let key = "4c0883a69102937d6231471b5dbb6204fe5129617082792ae468d01a3f362318"
             .parse::<LocalWallet>()
             .unwrap()
@@ -348,6 +383,50 @@ mod tests {
         assert_eq!(tx, expected_rlp);
     }
 
+    #[tokio::test]
+    async fn ganache_consistent_chainid() {
+        let ganache = Ganache::new().spawn();
+        let provider = Provider::try_from(ganache.endpoint()).unwrap();
+        let chain_id = provider.get_chainid().await.unwrap();
+        assert_eq!(chain_id, U256::from(1337));
+
+        // Intentionally do not set the chain id here so we ensure that the signer pulls the
+        // provider's chain id.
+        let key = LocalWallet::new(&mut rand::thread_rng());
+
+        // combine the provider and wallet and test that the chain id is the same for both the
+        // signer returned by the middleware and through the middleware itself.
+        let client = SignerMiddleware::new_with_provider_chain(provider, key).await.unwrap();
+        let middleware_chainid = client.get_chainid().await.unwrap();
+        assert_eq!(chain_id, middleware_chainid);
+
+        let signer = client.signer();
+        let signer_chainid = signer.chain_id();
+        assert_eq!(chain_id.as_u64(), signer_chainid);
+    }
+
+    #[tokio::test]
+    async fn ganache_consistent_chainid_not_default() {
+        let ganache = Ganache::new().args(vec!["--chain.chainId", "13371337"]).spawn();
+        let provider = Provider::try_from(ganache.endpoint()).unwrap();
+        let chain_id = provider.get_chainid().await.unwrap();
+        assert_eq!(chain_id, U256::from(13371337));
+
+        // Intentionally do not set the chain id here so we ensure that the signer pulls the
+        // provider's chain id.
+        let key = LocalWallet::new(&mut rand::thread_rng());
+
+        // combine the provider and wallet and test that the chain id is the same for both the
+        // signer returned by the middleware and through the middleware itself.
+        let client = SignerMiddleware::new_with_provider_chain(provider, key).await.unwrap();
+        let middleware_chainid = client.get_chainid().await.unwrap();
+        assert_eq!(chain_id, middleware_chainid);
+
+        let signer = client.signer();
+        let signer_chainid = signer.chain_id();
+        assert_eq!(chain_id.as_u64(), signer_chainid);
+    }
+
     #[tokio::test]
     async fn handles_tx_from_field() {
         let ganache = Ganache::new().spawn();
@@ -361,7 +440,7 @@ mod tests {
             )
             .await
             .unwrap();
-        let client = SignerMiddleware::new(provider, key);
+        let client = SignerMiddleware::new_with_provider_chain(provider, key).await.unwrap();
 
         let request = TransactionRequest::new();
 


Test patch:
diff --git a/ethers-contract/tests/contract.rs b/ethers-contract/tests/contract.rs
index 46e955db..6f78dc80 100644
--- a/ethers-contract/tests/contract.rs
+++ b/ethers-contract/tests/contract.rs
@@ -69,7 +69,6 @@ mod eth_tests {
         assert_eq!(last_sender.clone().call().await.unwrap(), addr2);
         assert_eq!(get_value.clone().call().await.unwrap(), "hi");
         assert_eq!(tx.input, calldata);
-        assert_eq!(tx_receipt.gas_used.unwrap(), gas_estimate);
 
         // we can also call contract methods at other addresses with the `at` call
         // (useful when interacting with multiple ERC20s for example)
@@ -579,7 +578,8 @@ mod eth_tests {
             .expect("failed to instantiate provider from ganache endpoint")
             .interval(Duration::from_millis(10u64));
 
-        let client = SignerMiddleware::new(provider, wallet.clone());
+        let client =
+            SignerMiddleware::new_with_provider_chain(provider, wallet.clone()).await.unwrap();
         let client = Arc::new(client);
 
         let factory = ContractFactory::new(abi.clone(), bytecode.clone(), client.clone());
@@ -597,8 +597,8 @@ mod eth_tests {
         let contract = DeriveEip712Test::new(addr, client.clone());
 
         let foo_bar = FooBar {
-            foo: I256::from(10),
-            bar: U256::from(20),
+            foo: I256::from(10u64),
+            bar: U256::from(20u64),
             fizz: b"fizz".into(),
             buzz: keccak256("buzz"),
             far: String::from("space"),
diff --git a/ethers-middleware/tests/gas_oracle.rs b/ethers-middleware/tests/gas_oracle.rs
index 49560319..c58f9e28 100644
--- a/ethers-middleware/tests/gas_oracle.rs
+++ b/ethers-middleware/tests/gas_oracle.rs
@@ -38,8 +38,10 @@ async fn using_gas_oracle() {
     // connect to the network
     let provider = Provider::<Http>::try_from(ganache.endpoint()).unwrap();
 
+    // this is set because ganache now sets 875000000 as the first block's base fee
+    let base_fee = 875000000;
     // assign a gas oracle to use
-    let gas_oracle = FakeGasOracle { gas_price: 1337.into() };
+    let gas_oracle = FakeGasOracle { gas_price: (base_fee + 1337).into() };
     let expected_gas_price = gas_oracle.fetch().await.unwrap();
 
     let provider = GasOracleMiddleware::new(provider, gas_oracle);
diff --git a/ethers-middleware/tests/signer.rs b/ethers-middleware/tests/signer.rs
index e2cca684..3b805158 100644
--- a/ethers-middleware/tests/signer.rs
+++ b/ethers-middleware/tests/signer.rs
@@ -35,8 +35,7 @@ async fn send_eth() {
         .unwrap()
         .interval(Duration::from_millis(10u64));
     let chain_id = provider.get_chainid().await.unwrap().as_u64();
-    let wallet = wallet.with_chain_id(chain_id);
-    let provider = SignerMiddleware::new(provider, wallet);
+    let provider = SignerMiddleware::new_with_provider_chain(provider, wallet).await.unwrap();
 
     // craft the transaction
     let tx = TransactionRequest::new().to(wallet2.address()).value(10000).chain_id(chain_id);
@@ -168,7 +167,8 @@ async fn send_transaction_handles_tx_from_field() {
 
     // connect to the network
     let provider = Provider::try_from(ganache.endpoint()).unwrap();
-    let provider = SignerMiddleware::new(provider, signer.clone());
+    let provider =
+        SignerMiddleware::new_with_provider_chain(provider, signer.clone()).await.unwrap();
 
     // sending a TransactionRequest with a from field of None should result
     // in a transaction from the signer address
@@ -231,7 +231,7 @@ async fn deploy_and_call_contract() {
         .parse::<LocalWallet>()
         .unwrap()
         .with_chain_id(chain_id);
-    let client = SignerMiddleware::new(provider, wallet);
+    let client = SignerMiddleware::new_with_provider_chain(provider, wallet).await.unwrap();
     let client = Arc::new(client);
 
     let factory = ContractFactory::new(abi, bytecode, client);