Repository: gakonst/ethers-rs (rust)

Issue:
Recovering sender of a legacy (type 0) transaction fails
**Version**

development ethers version:
`ethers v0.17.0 (https://github.com/gakonst/ethers-rs?rev=afdab2a555c10d55a32c5991282b4353a4a81256#afdab2a5)`

**Description**

Seems like the code to recover the sender of a transaction doesn't work if it's a legacy transaction.

Let's use https://etherscan.io/tx/0x280cde7cdefe4b188750e76c888f13bd05ce9a4d7767730feefe8a0e50ca6fc4 as an example, we can access the raw RLP in Etherscan: https://etherscan.io/getRawTx?tx=0x280cde7cdefe4b188750e76c888f13bd05ce9a4d7767730feefe8a0e50ca6fc4

Here is a test to reproduce:

```
#[test]
fn test_rlp_decoding() {
    use ethers::types::*;

    let raw_tx = "0xf9015482078b8505d21dba0083022ef1947a250d5630b4cf539739df2c5dacb4c659f2488d880c46549a521b13d8b8e47ff36ab50000000000000000000000000000000000000000000066ab5a608bd00a23f2fe000000000000000000000000000000000000000000000000000000000000008000000000000000000000000048c04ed5691981c42154c6167398f95e8f38a7ff00000000000000000000000000000000000000000000000000000000632ceac70000000000000000000000000000000000000000000000000000000000000002000000000000000000000000c02aaa39b223fe8d0a0e5c4f27ead9083c756cc20000000000000000000000006c6ee5e31d828de241282b9606c8e98ea48526e225a0c9077369501641a92ef7399ff81c21639ed4fd8fc69cb793cfa1dbfab342e10aa0615facb2f1bcf3274a354cfe384a38d0cc008a11c2dd23a69111bc6930ba27a8";
    let mut decoded_tx = ethers::utils::rlp::decode::<Transaction>(
        &raw_tx.parse::<Bytes>().expect("unable to parse raw tx"),
    )
    .expect("unable to decode raw tx");
    decoded_tx.recover_from_mut().unwrap();
    decoded_tx.hash = decoded_tx.hash();
    dbg!(&decoded_tx);
    assert_eq!(
        decoded_tx.from,
        H160(
            b"\xa1\x2e\x14\x62\xd0\xce\xd5\x72\xf3\x96\xf5\x8b\x6e\x2d\x03\x89\x4c\xd7\xc8\xa4"
                .clone()
        )
    );
    assert_eq!(decoded_tx.value, U256::from(884487398604084184u64));
}
```

The test fails, but the `from` address that I check for is the sender of the transaction (Etherscan confirms), so it means that ethers is incorrectly recovering the sender.

Pull request:
fix: legacy signed rlp decoding
<!--
Thank you for your Pull Request. Please provide a description above and review
the requirements below.

Bug fixes and new features should include tests.

Contributors guide: https://github.com/gakonst/ethers-rs/blob/master/CONTRIBUTING.md

The contributors guide includes instructions for running rustfmt and building the
documentation.
-->

## Motivation
Closes #1732

fixes some issues around signed transaction rlp decoding.

* Use conditional stream size depending on chain_id
* fix `TypedTransaction::decoded_signed` to support legacy transactions: replace the wrong `is_data()` check and treat lists as legacy transactions

cc @Rjected 


imo the rlp decode_unsigned/decode_signed are a bit confusing. especially since the unsiged variant is the default, which would lead to wrong chain ids (37 instead 1 for mainnet). Not sure what to do about it.

need to make this very clear in the docs at least.
<!--
Explain the context and why you're making that change. What is the problem
you're trying to solve? In some cases there is not a problem and this can be
thought of as being the motivation for your change.
-->

## Solution

<!--
Summarize the solution and provide any necessary context needed to understand
the code change.
-->

## PR Checklist

- [x] Added Tests
- [ ] Added Documentation
- [ ] Updated the changelog


Patch:
diff --git a/Cargo.lock b/Cargo.lock
index 43b7f81d..6b7c6490 100644
--- a/Cargo.lock
+++ b/Cargo.lock
@@ -1238,7 +1238,6 @@ dependencies = [
  "ethers-contract-derive",
  "ethers-core",
  "ethers-derive-eip712",
- "ethers-middleware",
  "ethers-providers",
  "ethers-signers",
  "ethers-solc",
@@ -1429,7 +1428,7 @@ dependencies = [
  "coins-ledger",
  "elliptic-curve",
  "eth-keystore",
- "ethers-contract",
+ "ethers-contract-derive",
  "ethers-core",
  "ethers-derive-eip712",
  "futures-executor",
diff --git a/ethers-contract/Cargo.toml b/ethers-contract/Cargo.toml
index d6512ab6..db602458 100644
--- a/ethers-contract/Cargo.toml
+++ b/ethers-contract/Cargo.toml
@@ -26,7 +26,6 @@ futures-util = { version = "^0.3" }
 hex = { version = "0.4.3", default-features = false, features = ["std"] }
 
 [dev-dependencies]
-ethers-middleware = { version = "^0.17.0", path = "../ethers-middleware" }
 ethers-providers = { version = "^0.17.0", path = "../ethers-providers", default-features = false, features = [
     "ws"
 ] }
diff --git a/ethers-contract/src/contract.rs b/ethers-contract/src/contract.rs
index 17443e5a..11ec62a3 100644
--- a/ethers-contract/src/contract.rs
+++ b/ethers-contract/src/contract.rs
@@ -185,9 +185,9 @@ impl<M> Contract<M> {
         &self.base_contract.abi
     }
 
-    /// Returns a reference to the contract's client
-    pub fn client(&self) -> &M {
-        &self.client
+    /// Returns a pointer to the contract's client
+    pub fn client(&self) -> Arc<M> {
+        self.client.clone()
     }
 }
 
diff --git a/ethers-contract/src/factory.rs b/ethers-contract/src/factory.rs
index 7f51e8d3..bffb9045 100644
--- a/ethers-contract/src/factory.rs
+++ b/ethers-contract/src/factory.rs
@@ -147,8 +147,8 @@ impl<M: Middleware, C: From<Contract<M>>> ContractDeployer<M, C> {
         self.deployer.abi()
     }
 
-    /// Returns a reference to the deployer's client
-    pub fn client(&self) -> &M {
+    /// Returns a pointer to the deployer's client
+    pub fn client(&self) -> Arc<M> {
         self.deployer.client()
     }
 }
@@ -261,9 +261,9 @@ impl<M: Middleware> Deployer<M> {
         &self.abi
     }
 
-    /// Returns a reference to the deployer's client
-    pub fn client(&self) -> &M {
-        &self.client
+    /// Returns a pointer to the deployer's client
+    pub fn client(&self) -> Arc<M> {
+        self.client.clone()
     }
 }
 
diff --git a/ethers-core/src/types/chain.rs b/ethers-core/src/types/chain.rs
index cfe3745a..758eabbf 100644
--- a/ethers-core/src/types/chain.rs
+++ b/ethers-core/src/types/chain.rs
@@ -50,6 +50,7 @@ pub enum Chain {
     OptimismKovan = 69,
     Arbitrum = 42161,
     ArbitrumTestnet = 421611,
+    ArbitrumGoerli = 421613,
     Cronos = 25,
     CronosTestnet = 338,
     #[strum(serialize = "bsc")]
@@ -81,7 +82,7 @@ impl Chain {
     /// <https://polygonscan.com/chart/blocktime>
     pub fn average_blocktime_hint(&self) -> Option<Duration> {
         let ms = match self {
-            Chain::Arbitrum | Chain::ArbitrumTestnet => 1_300,
+            Chain::Arbitrum | Chain::ArbitrumTestnet | Chain::ArbitrumGoerli => 1_300,
             Chain::Mainnet | Chain::Optimism => 13_000,
             Chain::Polygon | Chain::PolygonMumbai => 2_100,
             Chain::Moonbeam | Chain::Moonriver => 12_500,
@@ -164,6 +165,10 @@ impl Chain {
             Chain::ArbitrumTestnet => {
                 ("https://api-testnet.arbiscan.io/api", "https://testnet.arbiscan.io")
             }
+            Chain::ArbitrumGoerli => (
+                "https://goerli-rollup-explorer.arbitrum.io/api",
+                "https://goerli-rollup-explorer.arbitrum.io",
+            ),
             Chain::Cronos => ("https://api.cronoscan.com/api", "https://cronoscan.com"),
             Chain::CronosTestnet => {
                 ("https://api-testnet.cronoscan.com/api", "https://testnet.cronoscan.com")
@@ -244,6 +249,7 @@ impl fmt::Display for Chain {
             Chain::BinanceSmartChainTestnet => "bsc-testnet",
             Chain::Arbitrum => "arbitrum",
             Chain::ArbitrumTestnet => "arbitrum-testnet",
+            Chain::ArbitrumGoerli => "arbitrum-goerli",
             Chain::Cronos => "cronos",
             Chain::CronosTestnet => "cronos-testnet",
             Chain::Poa => "poa",
@@ -313,6 +319,7 @@ impl TryFrom<u64> for Chain {
             97 => Chain::BinanceSmartChainTestnet,
             42161 => Chain::Arbitrum,
             421611 => Chain::ArbitrumTestnet,
+            421613 => Chain::ArbitrumGoerli,
             25 => Chain::Cronos,
             338 => Chain::CronosTestnet,
             99 => Chain::Poa,
@@ -372,6 +379,7 @@ impl FromStr for Chain {
             "bsc-testnet" => Chain::BinanceSmartChainTestnet,
             "arbitrum" => Chain::Arbitrum,
             "arbitrum-testnet" => Chain::ArbitrumTestnet,
+            "arbitrum-goerli" => Chain::ArbitrumGoerli,
             "cronos" => Chain::Cronos,
             "cronos-testnet" => Chain::CronosTestnet,
             "poa" => Chain::Poa,
@@ -403,6 +411,7 @@ impl Chain {
                 Chain::BinanceSmartChainTestnet |
                 Chain::Arbitrum |
                 Chain::ArbitrumTestnet |
+                Chain::ArbitrumGoerli |
                 Chain::Rsk |
                 Chain::Oasis |
                 Chain::Emerald |
diff --git a/ethers-core/src/types/transaction/eip2718.rs b/ethers-core/src/types/transaction/eip2718.rs
index ef4a5f8d..32d0d8f5 100644
--- a/ethers-core/src/types/transaction/eip2718.rs
+++ b/ethers-core/src/types/transaction/eip2718.rs
@@ -320,33 +320,31 @@ impl TypedTransaction {
 
     /// Decodes a signed TypedTransaction from a rlp encoded byte stream
     pub fn decode_signed(rlp: &rlp::Rlp) -> Result<(Self, Signature), TypedTransactionError> {
-        let tx_type: Option<U64> = match rlp.is_data() {
-            true => Ok(Some(rlp.data()?.into())),
-            false => Err(TypedTransactionError::MissingTransactionType),
-        }?;
+        let data = rlp.data()?;
+        let first = *data.first().ok_or(rlp::DecoderError::Custom("empty slice"))?;
+        if rlp.is_list() {
+            // Legacy (0x00)
+            // use the original rlp
+            let decoded_request = TransactionRequest::decode_signed_rlp(rlp)?;
+            return Ok((Self::Legacy(decoded_request.0), decoded_request.1))
+        }
 
         let rest = rlp::Rlp::new(
             rlp.as_raw().get(1..).ok_or(TypedTransactionError::MissingTransactionPayload)?,
         );
 
-        match tx_type {
-            Some(x) if x == U64::from(1u64) => {
-                // EIP-2930 (0x01)
-                let decoded_request = Eip2930TransactionRequest::decode_signed_rlp(&rest)?;
-                Ok((Self::Eip2930(decoded_request.0), decoded_request.1))
-            }
-            Some(x) if x == U64::from(2u64) => {
-                // EIP-1559 (0x02)
-                let decoded_request = Eip1559TransactionRequest::decode_signed_rlp(&rest)?;
-                Ok((Self::Eip1559(decoded_request.0), decoded_request.1))
-            }
-            _ => {
-                // Legacy (0x00)
-                // use the original rlp
-                let decoded_request = TransactionRequest::decode_signed_rlp(&rest)?;
-                Ok((Self::Legacy(decoded_request.0), decoded_request.1))
-            }
+        if first == 0x01 {
+            // EIP-2930 (0x01)
+            let decoded_request = Eip2930TransactionRequest::decode_signed_rlp(&rest)?;
+            return Ok((Self::Eip2930(decoded_request.0), decoded_request.1))
+        }
+        if first == 0x02 {
+            // EIP-1559 (0x02)
+            let decoded_request = Eip1559TransactionRequest::decode_signed_rlp(&rest)?;
+            return Ok((Self::Eip1559(decoded_request.0), decoded_request.1))
         }
+
+        Err(rlp::DecoderError::Custom("invalid tx type").into())
     }
 }
 
diff --git a/ethers-core/src/types/transaction/request.rs b/ethers-core/src/types/transaction/request.rs
index 914b11e5..ffd3aa9d 100644
--- a/ethers-core/src/types/transaction/request.rs
+++ b/ethers-core/src/types/transaction/request.rs
@@ -7,6 +7,7 @@ use crate::{
     utils::keccak256,
 };
 
+use crate::types::transaction::BASE_NUM_TX_FIELDS;
 use rlp::{Decodable, RlpStream};
 use serde::{Deserialize, Serialize};
 use thiserror::Error;
@@ -161,14 +162,16 @@ impl TransactionRequest {
     /// signing. Assumes the chainid exists.
     pub fn rlp(&self) -> Bytes {
         let mut rlp = RlpStream::new();
-        rlp.begin_list(NUM_TX_FIELDS);
-        self.rlp_base(&mut rlp);
-
-        // Only hash the 3 extra fields when preparing the
-        // data to sign if chain_id is present
-        rlp_opt(&mut rlp, &self.chain_id);
-        rlp.append(&0u8);
-        rlp.append(&0u8);
+        if let Some(chain_id) = self.chain_id {
+            rlp.begin_list(BASE_NUM_TX_FIELDS);
+            self.rlp_base(&mut rlp);
+            rlp.append(&chain_id);
+            rlp.append(&0u8);
+            rlp.append(&0u8);
+        } else {
+            rlp.begin_list(BASE_NUM_TX_FIELDS - 3);
+            self.rlp_base(&mut rlp);
+        }
         rlp.out().freeze().into()
     }
 
@@ -350,10 +353,9 @@ impl TransactionRequest {
 #[cfg(test)]
 #[cfg(not(feature = "celo"))]
 mod tests {
-    use crate::types::{Bytes, NameOrAddress, Signature};
+    use super::*;
+    use crate::types::{transaction::eip2718::TypedTransaction, Bytes, NameOrAddress, Signature};
     use rlp::{Decodable, Rlp};
-
-    use super::{Address, TransactionRequest, U256, U64};
     use std::str::FromStr;
 
     #[test]
@@ -558,4 +560,17 @@ mod tests {
             assert_eq!(from_tx, from_addr);
         }
     }
+
+    #[test]
+    fn test_recover_legacy_tx() {
+        let raw_tx = "f9015482078b8505d21dba0083022ef1947a250d5630b4cf539739df2c5dacb4c659f2488d880c46549a521b13d8b8e47ff36ab50000000000000000000000000000000000000000000066ab5a608bd00a23f2fe000000000000000000000000000000000000000000000000000000000000008000000000000000000000000048c04ed5691981c42154c6167398f95e8f38a7ff00000000000000000000000000000000000000000000000000000000632ceac70000000000000000000000000000000000000000000000000000000000000002000000000000000000000000c02aaa39b223fe8d0a0e5c4f27ead9083c756cc20000000000000000000000006c6ee5e31d828de241282b9606c8e98ea48526e225a0c9077369501641a92ef7399ff81c21639ed4fd8fc69cb793cfa1dbfab342e10aa0615facb2f1bcf3274a354cfe384a38d0cc008a11c2dd23a69111bc6930ba27a8";
+
+        let data = hex::decode(raw_tx).unwrap();
+        let rlp = Rlp::new(&data);
+        let (tx, sig) = TypedTransaction::decode_signed(&rlp).unwrap();
+        let recovered = sig.recover(tx.sighash()).unwrap();
+
+        let expected: Address = "0xa12e1462d0ced572f396f58b6e2d03894cd7c8a4".parse().unwrap();
+        assert_eq!(expected, recovered);
+    }
 }
diff --git a/ethers-etherscan/src/lib.rs b/ethers-etherscan/src/lib.rs
index 33fd37cf..dcab1fc0 100644
--- a/ethers-etherscan/src/lib.rs
+++ b/ethers-etherscan/src/lib.rs
@@ -93,6 +93,7 @@ impl Client {
             Chain::BinanceSmartChainTestnet |
             Chain::Arbitrum |
             Chain::ArbitrumTestnet |
+            Chain::ArbitrumGoerli |
             Chain::Cronos |
             Chain::CronosTestnet |
             Chain::Aurora |
diff --git a/ethers-signers/Cargo.toml b/ethers-signers/Cargo.toml
index 84ab59a9..66dffd69 100644
--- a/ethers-signers/Cargo.toml
+++ b/ethers-signers/Cargo.toml
@@ -44,10 +44,7 @@ eth-keystore = { version = "0.5.0" }
 home = { version = "0.5.3", optional = true }
 
 [dev-dependencies]
-ethers-contract = { version = "^0.17.0", path = "../ethers-contract", features = [
-    "eip712",
-    "abigen"
-] }
+ethers-contract-derive = { version = "^0.17.0", path = "../ethers-contract/ethers-contract-derive" }
 ethers-derive-eip712 = { version = "^0.17.0", path = "../ethers-core/ethers-derive-eip712" }
 serde_json = { version = "1.0.64" }
 tracing-subscriber = "0.3.15"
diff --git a/ethers-signers/src/ledger/app.rs b/ethers-signers/src/ledger/app.rs
index fc9e4fe2..5932f0b8 100644
--- a/ethers-signers/src/ledger/app.rs
+++ b/ethers-signers/src/ledger/app.rs
@@ -250,7 +250,7 @@ impl LedgerEthereum {
 mod tests {
     use super::*;
     use crate::Signer;
-    use ethers_contract::EthAbiType;
+    use ethers_contract_derive::EthAbiType;
     use ethers_core::types::{
         transaction::eip712::Eip712, Address, TransactionRequest, I256, U256,
     };
diff --git a/ethers-signers/src/trezor/app.rs b/ethers-signers/src/trezor/app.rs
index 5994c79a..a9e7b555 100644
--- a/ethers-signers/src/trezor/app.rs
+++ b/ethers-signers/src/trezor/app.rs
@@ -233,7 +233,7 @@ impl TrezorEthereum {
 mod tests {
     use super::*;
     use crate::Signer;
-    use ethers_contract::EthAbiType;
+    use ethers_contract_derive::EthAbiType;
     use ethers_core::types::{
         transaction::{
             eip2930::{AccessList, AccessListItem},


Test patch:
diff --git a/ethers-contract/tests/it/abigen.rs b/ethers-contract/tests/it/abigen.rs
index d09c2b96..dbcaa1df 100644
--- a/ethers-contract/tests/it/abigen.rs
+++ b/ethers-contract/tests/it/abigen.rs
@@ -7,9 +7,7 @@ use ethers_core::{
     types::{transaction::eip2718::TypedTransaction, Chain, Eip1559TransactionRequest, U256},
     utils::Anvil,
 };
-use ethers_middleware::SignerMiddleware;
 use ethers_providers::{MockProvider, Provider};
-use ethers_signers::{LocalWallet, Signer};
 use ethers_solc::Solc;
 use std::{
     convert::{TryFrom, TryInto},
@@ -617,11 +615,14 @@ fn can_handle_overloaded_events() {
 async fn can_send_struct_param() {
     abigen!(StructContract, "./tests/solidity-contracts/StructContract.json");
 
-    let server = Anvil::new().spawn();
-    let wallet: LocalWallet = server.keys()[0].clone().into();
-    let provider = Provider::try_from(server.endpoint()).unwrap();
-    let client =
-        Arc::new(SignerMiddleware::new(provider, wallet.with_chain_id(Chain::AnvilHardhat)));
+    // launch the network & connect to it
+    let anvil = Anvil::new().spawn();
+    let from = anvil.addresses()[0];
+    let provider = Provider::try_from(anvil.endpoint())
+        .unwrap()
+        .with_sender(from)
+        .interval(std::time::Duration::from_millis(10));
+    let client = Arc::new(provider);
 
     let contract = StructContract::deploy(client, ()).unwrap().legacy().send().await.unwrap();
 
diff --git a/ethers-contract/tests/it/contract.rs b/ethers-contract/tests/it/contract.rs
index 756210b2..5968b19d 100644
--- a/ethers-contract/tests/it/contract.rs
+++ b/ethers-contract/tests/it/contract.rs
@@ -13,7 +13,6 @@ mod eth_tests {
         utils::{keccak256, Anvil},
     };
     use ethers_derive_eip712::*;
-    use ethers_middleware::signer::SignerMiddleware;
     use ethers_providers::{Http, Middleware, PendingTransaction, Provider, StreamExt};
     use ethers_signers::{LocalWallet, Signer};
     use std::{convert::TryFrom, sync::Arc, time::Duration};
@@ -747,19 +746,17 @@ mod eth_tests {
         // get ABI and bytecode for the DeriveEip712Test contract
         let (abi, bytecode) = compile_contract("DeriveEip712Test", "DeriveEip712Test.sol");
 
-        // launch anvil
+        // launch the network & connect to it
         let anvil = Anvil::new().spawn();
+        let from = anvil.addresses()[0];
+        let provider = Provider::try_from(anvil.endpoint())
+            .unwrap()
+            .with_sender(from)
+            .interval(std::time::Duration::from_millis(10));
+        let client = Arc::new(provider);
 
         let wallet: LocalWallet = anvil.keys()[0].clone().into();
 
-        let provider = Provider::<Http>::try_from(anvil.endpoint())
-            .expect("failed to instantiate provider from anvil endpoint")
-            .interval(Duration::from_millis(10u64));
-
-        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());
 
         let contract = factory