From 68fba606c23197959c27b2c9cfe2ef56e95ca98b Mon Sep 17 00:00:00 2001 From: Nicolas Gotchac Date: Fri, 19 Aug 2022 23:33:12 +0200 Subject: [PATCH] Don't auto-generate an AccessList when sending/filling a tx (#1619) Add an optionnal `block` parameter to the gas estimation method --- ethers-contract/src/call.rs | 2 +- ethers-middleware/src/signer.rs | 8 +++- ethers-providers/src/lib.rs | 24 +++++------ ethers-providers/src/provider.rs | 72 ++++++-------------------------- 4 files changed, 31 insertions(+), 75 deletions(-) diff --git a/ethers-contract/src/call.rs b/ethers-contract/src/call.rs index 8dcc4fba..1f753414 100644 --- a/ethers-contract/src/call.rs +++ b/ethers-contract/src/call.rs @@ -150,7 +150,7 @@ where /// Returns the estimated gas cost for the underlying transaction to be executed pub async fn estimate_gas(&self) -> Result> { - self.client.estimate_gas(&self.tx).await.map_err(ContractError::MiddlewareError) + self.client.estimate_gas(&self.tx, self.block).await.map_err(ContractError::MiddlewareError) } /// Queries the blockchain via an `eth_call` for the provided transaction. diff --git a/ethers-middleware/src/signer.rs b/ethers-middleware/src/signer.rs index e38ed7e1..b911a660 100644 --- a/ethers-middleware/src/signer.rs +++ b/ethers-middleware/src/signer.rs @@ -303,9 +303,13 @@ where self.signer.sign_message(data.into()).await.map_err(SignerMiddlewareError::SignerError) } - async fn estimate_gas(&self, tx: &TypedTransaction) -> Result { + async fn estimate_gas( + &self, + tx: &TypedTransaction, + block: Option, + ) -> Result { let tx = self.set_tx_from_if_none(tx); - self.inner.estimate_gas(&tx).await.map_err(SignerMiddlewareError::MiddlewareError) + self.inner.estimate_gas(&tx, block).await.map_err(SignerMiddlewareError::MiddlewareError) } async fn create_access_list( diff --git a/ethers-providers/src/lib.rs b/ethers-providers/src/lib.rs index 3c1403ed..f5f58fa3 100644 --- a/ethers-providers/src/lib.rs +++ b/ethers-providers/src/lib.rs @@ -106,7 +106,7 @@ pub enum SyncingStatus { /// /// ```rust /// use ethers_providers::{Middleware, FromErr}; -/// use ethers_core::types::{U64, TransactionRequest, U256, transaction::eip2718::TypedTransaction}; +/// use ethers_core::types::{U64, TransactionRequest, U256, transaction::eip2718::TypedTransaction, BlockId}; /// use thiserror::Error; /// use async_trait::async_trait; /// @@ -147,9 +147,9 @@ pub enum SyncingStatus { /// /// /// Overrides the default `estimate_gas` method to log that it was called, /// /// before forwarding the call to the next layer. -/// async fn estimate_gas(&self, tx: &TypedTransaction) -> Result { +/// async fn estimate_gas(&self, tx: &TypedTransaction, block: Option) -> Result { /// println!("Estimating gas..."); -/// self.inner().estimate_gas(tx).await.map_err(FromErr::from) +/// self.inner().estimate_gas(tx, block).await.map_err(FromErr::from) /// } /// } /// ``` @@ -182,15 +182,11 @@ pub trait Middleware: Sync + Send + Debug { /// 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 - /// 7. Set the chain_id with the provider's, if not already set + /// 3. Estimate gas usage + /// 4. Poll and set legacy or 1559 gas prices + /// 5. Set the chain_id with the provider's, if not already set /// /// 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 @@ -321,8 +317,12 @@ pub trait Middleware: Sync + Send + Debug { self.inner().get_transaction_count(from, block).await.map_err(FromErr::from) } - async fn estimate_gas(&self, tx: &TypedTransaction) -> Result { - self.inner().estimate_gas(tx).await.map_err(FromErr::from) + async fn estimate_gas( + &self, + tx: &TypedTransaction, + block: Option, + ) -> Result { + self.inner().estimate_gas(tx, block).await.map_err(FromErr::from) } async fn call( diff --git a/ethers-providers/src/provider.rs b/ethers-providers/src/provider.rs index d4d0a0eb..ed57facd 100644 --- a/ethers-providers/src/provider.rs +++ b/ethers-providers/src/provider.rs @@ -352,34 +352,10 @@ impl Middleware for Provider

{ } } - // 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?; + let gas_estimate = self.estimate_gas(tx, block).await?; tx.set_gas(gas_estimate); } @@ -611,8 +587,14 @@ impl Middleware for Provider

{ /// required (as a U256) to send it This is free, but only an estimate. Providing too little /// gas will result in a transaction being rejected (while still consuming all provided /// gas). - async fn estimate_gas(&self, tx: &TypedTransaction) -> Result { - self.request("eth_estimateGas", [tx]).await + async fn estimate_gas( + &self, + tx: &TypedTransaction, + block: Option, + ) -> Result { + let tx = utils::serialize(tx); + let block = utils::serialize(&block.unwrap_or_else(|| BlockNumber::Latest.into())); + self.request("eth_estimateGas", [tx, block]).await } async fn create_access_list( @@ -2064,42 +2046,14 @@ mod tests { assert_eq!(tx.gas_price(), Some(max_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; + // --- fills a 1559 transaction, leaving the existing gas limit unchanged, + // without generating an access-list let mut tx = Eip1559TransactionRequest::new() .gas(gas) .max_fee_per_gas(max_fee) .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(max_fee) - .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()); @@ -2107,14 +2061,12 @@ mod tests { 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 + // --- fills a 1559 transaction, using estimated gas let mut tx = Eip1559TransactionRequest::new() .max_fee_per_gas(max_fee) .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();