From 1d14f9d26ee3f94867bbc35c9c091076832f346d Mon Sep 17 00:00:00 2001 From: Dan Cline Date: Sun, 10 Apr 2022 13:55:30 -0400 Subject: [PATCH] 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 | 12 ++-- CHANGELOG.md | 3 + ethers-contract/tests/contract.rs | 8 +-- ethers-middleware/src/signer.rs | 87 +++++++++++++++++++++++++-- ethers-middleware/tests/gas_oracle.rs | 4 +- ethers-middleware/tests/signer.rs | 8 +-- 6 files changed, 103 insertions(+), 19 deletions(-) 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-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/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 { 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> { + 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::() .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(); 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::::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::() .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);