Ensure a consistent chain ID between a signer and provider in SignerMiddleware (#1095)

* feat(middleware): fetch chainid from middleware

Require SignerMiddleware to fetch the inner middleware's to set for the
signer. SignerMiddleware now requires an instantiated middleware with
an accessible get_chainid method.

* ci: update ganache

newer ganache version is needed to specify the ganache provider chain id
in tests

* set SignerMiddleware constructor return to result

* create new method for pulling chainid

* add consistent chainid CHANGELOG entry

* remove awaits

* switch test_derive_eip712 to use consistent signer

 * remove gas estimation equality assert in deploy_and_call_contract -
   updated version of ganache no longer returns the same value for gas
   estimation and gas_used

* revert with_signer to non-async non-result

* cargo fmt

* expand SignerMiddleware::new comment

* remove doc indent

.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"

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

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"),

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();
 

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);

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);