refactor: move fill_transaction impl to provider rather than default (#697)

* refactor: move fill_transaction impl to provider rather than default

* chore: changelog and docstring for fill_transaction

CHANGELOG.md

@@ -18,6 +18,8 @@
   [#668](https://github.com/gakonst/ethers-rs/pull/668)
 - Fix `fill_transaction` to set nonces in transactions, if the sender is known
   and no nonce is specified
+- Move `fill_transaction` implementation to the provider, to allow middleware
+  to properly override its behavior.
 
 ## ethers-contract-abigen
 

ethers-providers/src/lib.rs

@@ -159,66 +159,32 @@ pub trait Middleware: Sync + Send + Debug {
         self.inner().client_version().await.map_err(FromErr::from)
     }
 
-    /// Helper for filling a transaction
+    /// Fill necessary details of a transaction for dispatch
+    ///
+    /// This function is defined on providers to behave as follows:
+    /// 1. populate the `from` field with the default sender
+    /// 2. resolve any ENS names in the tx `to` field
+    /// 3. Estimate gas usage _without_ access lists
+    /// 4. Estimate gas usage _with_ access lists
+    /// 5. Enable access lists IFF they are cheaper
+    /// 6. Poll and set legacy or 1559 gas prices
+    ///
+    /// It does NOT set the nonce by default.
+    /// It MAY override the gas amount set by the user, if access lists are
+    /// cheaper.
+    ///
+    /// Middleware are encouraged to override any values _before_ delegating
+    /// to the inner implementation AND/OR modify the values provided by the
+    /// default implementation _after_ delegating.
+    ///
+    /// E.g. a middleware wanting to double gas prices should consider doing so
+    /// _after_ delegating and allowing the default implementation to poll gas.
     async fn fill_transaction(
         &self,
         tx: &mut TypedTransaction,
         block: Option<BlockId>,
     ) -> Result<(), Self::Error> {
-        if let Some(default_sender) = self.default_sender() {
+        self.inner().fill_transaction(tx, block).await.map_err(FromErr::from)
-            if tx.from().is_none() {
-                tx.set_from(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;
-                    }
-                }
-            }
-        }
-
-        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?;
-                    inner.max_fee_per_gas = Some(max_fee_per_gas);
-                    inner.max_priority_fee_per_gas = Some(max_priority_fee_per_gas);
-                };
-            }
-        }
-
-        Ok(())
     }
 
     async fn get_block_number(&self) -> Result<U64, Self::Error> {

ethers-providers/src/provider.rs

@@ -1,5 +1,5 @@
 use crate::{
-    ens,
+    ens, maybe,
     pubsub::{PubsubClient, SubscriptionStream},
     stream::{FilterWatcher, DEFAULT_POLL_INTERVAL},
     FromErr, Http as HttpProvider, JsonRpcClient, JsonRpcClientWrapper, MockProvider,
@@ -260,6 +260,67 @@ impl<P: JsonRpcClient> Middleware for Provider<P> {
         self.request("web3_clientVersion", ()).await
     }
 
+    async fn fill_transaction(
+        &self,
+        tx: &mut TypedTransaction,
+        block: Option<BlockId>,
+    ) -> Result<(), Self::Error> {
+        if let Some(default_sender) = self.default_sender() {
+            if tx.from().is_none() {
+                tx.set_from(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;
+                    }
+                }
+            }
+        }
+
+        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?;
+                    inner.max_fee_per_gas = Some(max_fee_per_gas);
+                    inner.max_priority_fee_per_gas = Some(max_priority_fee_per_gas);
+                };
+            }
+        }
+
+        Ok(())
+    }
+
     /// Gets the latest block number via the `eth_BlockNumber` API
     async fn get_block_number(&self) -> Result<U64, ProviderError> {
         self.request("eth_blockNumber", ()).await