fix(providers): Propogate gas limit with access list (#901)

* fix(providers): Propogate gas price with access list * Update CHANGELOG.md * Fix clippy lint * Clarify fill_transaction comments * Fill tx gas price before gas limit Updates Provider::fill_transaction() to fill the gas price of a transaction before filling the gas limit. There are cases where the gas used by a transaction may be dependent on the gas price. For example, the following contract bytecode branches based on the result of the GASPRICE opcode: GASPRICE PUSH1 0xff GT PUSH1 {label} JUMPI * Cleanup * Propogate eth_estimateGas failure
gakonst · Mar 1, 2022 · a1accbf · a1accbf
1 parent a158e12
commit a1accbf
Show file tree

Hide file tree

Showing 2 changed files with 175 additions and 26 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -42,6 +42,8 @@
 - Add Arbitrum mainnet and testnet to the list of known chains
 - Add ENS avatar and TXT records resolution
   [#889](https://github.com/gakonst/ethers-rs/pull/889)
+- Do not override gas limits provided by an outer middleware when including an EIP-2930 access list
+  [#901](https://github.com/gakonst/ethers-rs/pull/901)
 - Add a getter to `ProjectCompileOutput` that returns a mapping of compiler
   versions to a vector of name + contract struct tuples
   [#908](https://github.com/gakonst/ethers-rs/pull/908)

diff --git a/ethers-providers/src/provider.rs b/ethers-providers/src/provider.rs
@@ -275,38 +275,15 @@ impl<P: JsonRpcClient> Middleware for Provider<P> {
             }
         }
 
-        // TODO: Can we poll the futures below at the same time?
-        // Access List + Name resolution and then Gas price + Gas
+        // TODO: Join the name resolution and gas price future
 
         // set the ENS name
         if let Some(NameOrAddress::Name(ref ens_name)) = tx.to() {
             let addr = self.resolve_name(ens_name).await?;
             tx.set_to(addr);
         }
 
-        // estimate the gas without the access list
-        let gas = maybe(tx.gas().cloned(), self.estimate_gas(tx)).await?;
-        let mut al_used = false;
-
-        // set the access lists
-        if let Some(access_list) = tx.access_list() {
-            if access_list.0.is_empty() {
-                if let Ok(al_with_gas) = self.create_access_list(tx, block).await {
-                    // only set the access list if the used gas is less than the
-                    // normally estimated gas
-                    if al_with_gas.gas_used < gas {
-                        tx.set_access_list(al_with_gas.access_list);
-                        tx.set_gas(al_with_gas.gas_used);
-                        al_used = true;
-                    }
-                }
-            }
-        }
-
-        if !al_used {
-            tx.set_gas(gas);
-        }
-
+        // fill gas price
         match tx {
             TypedTransaction::Eip2930(_) | TypedTransaction::Legacy(_) => {
                 let gas_price = maybe(tx.gas_price(), self.get_gas_price()).await?;
@@ -322,6 +299,37 @@ impl<P: JsonRpcClient> Middleware for Provider<P> {
             }
         }
 
+        // If the tx has an access list but it is empty, it is an Eip1559 or Eip2930 tx,
+        // and we attempt to populate the acccess list. This may require `eth_estimateGas`,
+        // in which case we save the result in maybe_gas_res for later
+        let mut maybe_gas = None;
+        if let Some(starting_al) = tx.access_list() {
+            if starting_al.0.is_empty() {
+                let (gas_res, al_res) = futures_util::join!(
+                    maybe(tx.gas().cloned(), self.estimate_gas(tx)),
+                    self.create_access_list(tx, block)
+                );
+                let mut gas = gas_res?;
+
+                if let Ok(al_with_gas) = al_res {
+                    // Set access list if it saves gas over the estimated (or previously set) value
+                    if al_with_gas.gas_used < gas {
+                        // Update the gas estimate with the lower amount
+                        gas = al_with_gas.gas_used;
+                        tx.set_access_list(al_with_gas.access_list);
+                    }
+                }
+                maybe_gas = Some(gas);
+            }
+        }
+
+        // Set gas to estimated value only if it was not set by the caller,
+        // even if the access list has been populated and saves gas
+        if tx.gas().is_none() {
+            let gas_estimate = maybe(maybe_gas, self.estimate_gas(tx)).await?;
+            tx.set_gas(gas_estimate);
+        }
+
         Ok(())
     }
 
@@ -1462,7 +1470,9 @@ mod tests {
     use super::*;
     use crate::Http;
     use ethers_core::{
-        types::{TransactionRequest, H256},
+        types::{
+            transaction::eip2930::AccessList, Eip1559TransactionRequest, TransactionRequest, H256,
+        },
         utils::Geth,
     };
     use futures_util::StreamExt;
@@ -1692,4 +1702,141 @@ mod tests {
             .unwrap();
         dbg!(traces);
     }
+
+    #[tokio::test]
+    async fn test_fill_transaction_1559() {
+        let (mut provider, mock) = Provider::mocked();
+        provider.from = Some("0x6fC21092DA55B392b045eD78F4732bff3C580e2c".parse().unwrap());
+
+        let gas = U256::from(21000_usize);
+        let basefee = U256::from(25_usize);
+        let prio_fee = U256::from(25_usize);
+        let access_list: AccessList = vec![Default::default()].into();
+
+        // --- leaves a filled 1559 transaction unchanged, making no requests
+        let from: Address = "0x0000000000000000000000000000000000000001".parse().unwrap();
+        let to: Address = "0x0000000000000000000000000000000000000002".parse().unwrap();
+        let mut tx = Eip1559TransactionRequest::new()
+            .from(from)
+            .to(to)
+            .gas(gas)
+            .max_fee_per_gas(basefee)
+            .max_priority_fee_per_gas(prio_fee)
+            .access_list(access_list.clone())
+            .into();
+        provider.fill_transaction(&mut tx, None).await.unwrap();
+
+        assert_eq!(tx.from(), Some(&from));
+        assert_eq!(tx.to(), Some(&to.into()));
+        assert_eq!(tx.gas(), Some(&gas));
+        assert_eq!(tx.gas_price(), Some(basefee + prio_fee));
+        assert_eq!(tx.access_list(), Some(&access_list));
+
+        // --- fills a 1559 transaction, leaving the existing gas limit unchanged, but including
+        // access list if cheaper
+        let gas_with_al = gas - 1;
+        let mut tx = Eip1559TransactionRequest::new()
+            .gas(gas)
+            .max_fee_per_gas(basefee)
+            .max_priority_fee_per_gas(prio_fee)
+            .into();
+
+        mock.push(AccessListWithGasUsed {
+            access_list: access_list.clone(),
+            gas_used: gas_with_al,
+        })
+        .unwrap();
+
+        provider.fill_transaction(&mut tx, None).await.unwrap();
+
+        assert_eq!(tx.from(), provider.from.as_ref());
+        assert!(tx.to().is_none());
+        assert_eq!(tx.gas(), Some(&gas));
+        assert_eq!(tx.access_list(), Some(&access_list));
+
+        // --- fills a 1559 transaction, ignoring access list if more expensive
+        let gas_with_al = gas + 1;
+        let mut tx = Eip1559TransactionRequest::new()
+            .max_fee_per_gas(basefee)
+            .max_priority_fee_per_gas(prio_fee)
+            .into();
+
+        mock.push(AccessListWithGasUsed {
+            access_list: access_list.clone(),
+            gas_used: gas_with_al,
+        })
+        .unwrap();
+        mock.push(gas).unwrap();
+
+        provider.fill_transaction(&mut tx, None).await.unwrap();
+
+        assert_eq!(tx.from(), provider.from.as_ref());
+        assert!(tx.to().is_none());
+        assert_eq!(tx.gas(), Some(&gas));
+        assert_eq!(tx.access_list(), Some(&Default::default()));
+
+        // --- fills a 1559 transaction, using estimated gas if create_access_list() errors
+        let mut tx = Eip1559TransactionRequest::new()
+            .max_fee_per_gas(basefee)
+            .max_priority_fee_per_gas(prio_fee)
+            .into();
+
+        // bad mock value causes error response for eth_createAccessList
+        mock.push(b'b').unwrap();
+        mock.push(gas).unwrap();
+
+        provider.fill_transaction(&mut tx, None).await.unwrap();
+
+        assert_eq!(tx.from(), provider.from.as_ref());
+        assert!(tx.to().is_none());
+        assert_eq!(tx.gas(), Some(&gas));
+        assert_eq!(tx.access_list(), Some(&Default::default()));
+
+        // --- propogates estimate_gas() error
+        let mut tx = Eip1559TransactionRequest::new()
+            .max_fee_per_gas(basefee)
+            .max_priority_fee_per_gas(prio_fee)
+            .into();
+
+        // bad mock value causes error response for eth_estimateGas
+        mock.push(b'b').unwrap();
+
+        let res = provider.fill_transaction(&mut tx, None).await;
+
+        assert!(matches!(res, Err(ProviderError::JsonRpcClientError(_))));
+    }
+
+    #[tokio::test]
+    async fn test_fill_transaction_legacy() {
+        let (mut provider, mock) = Provider::mocked();
+        provider.from = Some("0x6fC21092DA55B392b045eD78F4732bff3C580e2c".parse().unwrap());
+
+        let gas = U256::from(21000_usize);
+        let gas_price = U256::from(50_usize);
+
+        // --- leaves a filled legacy transaction unchanged, making no requests
+        let from: Address = "0x0000000000000000000000000000000000000001".parse().unwrap();
+        let to: Address = "0x0000000000000000000000000000000000000002".parse().unwrap();
+        let mut tx =
+            TransactionRequest::new().from(from).to(to).gas(gas).gas_price(gas_price).into();
+        provider.fill_transaction(&mut tx, None).await.unwrap();
+
+        assert_eq!(tx.from(), Some(&from));
+        assert_eq!(tx.to(), Some(&to.into()));
+        assert_eq!(tx.gas(), Some(&gas));
+        assert_eq!(tx.gas_price(), Some(gas_price));
+        assert!(tx.access_list().is_none());
+
+        // --- fills an empty legacy transaction
+        let mut tx = TransactionRequest::new().into();
+        mock.push(gas).unwrap();
+        mock.push(gas_price).unwrap();
+        provider.fill_transaction(&mut tx, None).await.unwrap();
+
+        assert_eq!(tx.from(), provider.from.as_ref());
+        assert!(tx.to().is_none());
+        assert_eq!(tx.gas(), Some(&gas));
+        assert_eq!(tx.gas_price(), Some(gas_price));
+        assert!(tx.access_list().is_none());
+    }
 }