fix: ensure gas estimation includes the access list in 1559/2930 txs (#…

…523) * fix: ensure gas estimation includes the access list in 1559/2930 txs * feat: add gas_price to typed tx * feat: add access list to typed tx * chore(providers): cleanup fill_transaction * chore: fmt * chore: enable 712 on ethers-contract at top level crate * fix: do not error if eth_createAccessList is not available * feat: only use access list if needed
gakonst · Oct 20, 2021 · ef9b398 · ef9b398
1 parent 9db36e5
commit ef9b398
Show file tree

Hide file tree

Showing 3 changed files with 79 additions and 67 deletions.
diff --git a/Cargo.toml b/Cargo.toml
@@ -60,7 +60,7 @@ legacy = [
 # individual features per sub-crate
 ## core
 setup = ["ethers-core/setup"]
-eip712 = ["ethers-core/eip712"]
+eip712 = ["ethers-contract/eip712", "ethers-core/eip712"]
 ## providers
 ws = ["ethers-providers/ws"]
 ipc = ["ethers-providers/ipc"]

diff --git a/ethers-core/src/types/transaction/eip2718.rs b/ethers-core/src/types/transaction/eip2718.rs
@@ -1,4 +1,7 @@
-use super::{eip1559::Eip1559TransactionRequest, eip2930::Eip2930TransactionRequest};
+use super::{
+    eip1559::Eip1559TransactionRequest,
+    eip2930::{AccessList, Eip2930TransactionRequest},
+};
 use crate::{
     types::{Address, Bytes, NameOrAddress, Signature, TransactionRequest, H256, U256, U64},
     utils::keccak256,
@@ -119,6 +122,21 @@ impl TypedTransaction {
         };
     }
 
+    pub fn gas_price(&self) -> Option<U256> {
+        match self {
+            Legacy(inner) => inner.gas_price,
+            Eip2930(inner) => inner.tx.gas_price,
+            Eip1559(inner) => {
+                match (inner.max_fee_per_gas, inner.max_priority_fee_per_gas) {
+                    (Some(basefee), Some(prio_fee)) => Some(basefee + prio_fee),
+                    // this also covers the None, None case
+                    (None, prio_fee) => prio_fee,
+                    (basefee, None) => basefee,
+                }
+            }
+        }
+    }
+
     pub fn set_gas_price<T: Into<U256>>(&mut self, gas_price: T) {
         let gas_price = gas_price.into();
         match self {
@@ -139,6 +157,22 @@ impl TypedTransaction {
         }
     }
 
+    pub fn access_list(&self) -> Option<&AccessList> {
+        match self {
+            Legacy(_) => None,
+            Eip2930(inner) => Some(&inner.access_list),
+            Eip1559(inner) => Some(&inner.access_list),
+        }
+    }
+
+    pub fn set_access_list(&mut self, access_list: AccessList) {
+        match self {
+            Legacy(_) => {}
+            Eip2930(inner) => inner.access_list = access_list,
+            Eip1559(inner) => inner.access_list = access_list,
+        };
+    }
+
     pub fn set_data(&mut self, data: Bytes) {
         match self {
             Legacy(inner) => inner.data = Some(data),

diff --git a/ethers-providers/src/lib.rs b/ethers-providers/src/lib.rs
@@ -215,80 +215,58 @@ pub trait Middleware: Sync + Send + Debug {
         tx: &mut TypedTransaction,
         block: Option<BlockId>,
     ) -> Result<(), Self::Error> {
-        let mut tx_clone = tx.clone();
-
-        // TODO: Maybe deduplicate the code in a nice way
-        match tx {
-            TypedTransaction::Legacy(ref mut inner) => {
-                if let Some(NameOrAddress::Name(ref ens_name)) = inner.to {
-                    let addr = self.resolve_name(ens_name).await?;
-                    inner.to = Some(addr.into());
-                    tx_clone.set_to(addr);
-                };
-
-                if inner.from.is_none() {
-                    inner.from = self.default_sender();
-                }
-
-                let (gas_price, gas) = futures_util::try_join!(
-                    maybe(inner.gas_price, self.get_gas_price()),
-                    maybe(inner.gas, self.estimate_gas(&tx_clone)),
-                )?;
-                inner.gas = Some(gas);
-                inner.gas_price = Some(gas_price);
+        if let Some(default_sender) = self.default_sender() {
+            if tx.from().is_none() {
+                tx.set_from(default_sender);
             }
-            TypedTransaction::Eip2930(inner) => {
-                if let Ok(lst) = self.create_access_list(&tx_clone, block).await {
-                    inner.access_list = lst.access_list;
-                }
-
-                if let Some(NameOrAddress::Name(ref ens_name)) = inner.tx.to {
-                    let addr = self.resolve_name(ens_name).await?;
-                    inner.tx.to = Some(addr.into());
-                    tx_clone.set_to(addr);
-                };
-
-                if inner.tx.from.is_none() {
-                    inner.tx.from = self.default_sender();
+        }
+
+        // TODO: Can we poll the futures below at the same time?
+        // Access List + Name resolution and then Gas price + Gas
+
+        // 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;
+                    }
                 }
-
-                let (gas_price, gas) = futures_util::try_join!(
-                    maybe(inner.tx.gas_price, self.get_gas_price()),
-                    maybe(inner.tx.gas, self.estimate_gas(&tx_clone)),
-                )?;
-                inner.tx.gas = Some(gas);
-                inner.tx.gas_price = Some(gas_price);
             }
-            TypedTransaction::Eip1559(inner) => {
-                if let Ok(lst) = self.create_access_list(&tx_clone, block).await {
-                    inner.access_list = lst.access_list;
-                }
-
-                if let Some(NameOrAddress::Name(ref ens_name)) = inner.to {
-                    let addr = self.resolve_name(ens_name).await?;
-                    inner.to = Some(addr.into());
-                    tx_clone.set_to(addr);
-                };
+        }
 
-                if inner.from.is_none() {
-                    inner.from = self.default_sender();
-                }
-
-                let gas = maybe(inner.gas, self.estimate_gas(&tx_clone)).await?;
-                inner.gas = Some(gas);
+        if !al_used {
+            tx.set_gas(gas);
+        }
 
+        match tx {
+            TypedTransaction::Eip2930(_) | TypedTransaction::Legacy(_) => {
+                let gas_price = maybe(tx.gas_price(), self.get_gas_price()).await?;
+                tx.set_gas_price(gas_price);
+            }
+            TypedTransaction::Eip1559(ref mut inner) => {
                 if inner.max_fee_per_gas.is_none() || inner.max_priority_fee_per_gas.is_none() {
                     let (max_fee_per_gas, max_priority_fee_per_gas) =
                         self.estimate_eip1559_fees(None).await?;
-                    if inner.max_fee_per_gas.is_none() {
-                        inner.max_fee_per_gas = Some(max_fee_per_gas);
-                    }
-                    if inner.max_priority_fee_per_gas.is_none() {
-                        inner.max_priority_fee_per_gas = Some(max_priority_fee_per_gas);
-                    }
-                }
+                    inner.max_fee_per_gas = Some(max_fee_per_gas);
+                    inner.max_priority_fee_per_gas = Some(max_priority_fee_per_gas);
+                };
             }
-        };
+        }
 
         Ok(())
     }