Skip to content
This repository has been archived by the owner on Oct 19, 2024. It is now read-only.

Don't auto-generate an AccessList when sending/filling a tx #1619

Merged
merged 1 commit into from
Aug 19, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion ethers-contract/src/call.rs
Original file line number Diff line number Diff line change
Expand Up @@ -150,7 +150,7 @@ where

/// Returns the estimated gas cost for the underlying transaction to be executed
pub async fn estimate_gas(&self) -> Result<U256, ContractError<M>> {
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.
Expand Down
8 changes: 6 additions & 2 deletions ethers-middleware/src/signer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -303,9 +303,13 @@ where
self.signer.sign_message(data.into()).await.map_err(SignerMiddlewareError::SignerError)
}

async fn estimate_gas(&self, tx: &TypedTransaction) -> Result<U256, Self::Error> {
async fn estimate_gas(
&self,
tx: &TypedTransaction,
block: Option<BlockId>,
) -> Result<U256, Self::Error> {
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(
Expand Down
24 changes: 12 additions & 12 deletions ethers-providers/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
///
Expand Down Expand Up @@ -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<U256, Self::Error> {
/// async fn estimate_gas(&self, tx: &TypedTransaction, block: Option<BlockId>) -> Result<U256, Self::Error> {
/// println!("Estimating gas...");
/// self.inner().estimate_gas(tx).await.map_err(FromErr::from)
/// self.inner().estimate_gas(tx, block).await.map_err(FromErr::from)
/// }
/// }
/// ```
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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<U256, Self::Error> {
self.inner().estimate_gas(tx).await.map_err(FromErr::from)
async fn estimate_gas(
&self,
tx: &TypedTransaction,
block: Option<BlockId>,
) -> Result<U256, Self::Error> {
self.inner().estimate_gas(tx, block).await.map_err(FromErr::from)
}

async fn call(
Expand Down
72 changes: 12 additions & 60 deletions ethers-providers/src/provider.rs
Original file line number Diff line number Diff line change
Expand Up @@ -352,34 +352,10 @@ 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?;
let gas_estimate = self.estimate_gas(tx, block).await?;
tx.set_gas(gas_estimate);
}

Expand Down Expand Up @@ -611,8 +587,14 @@ impl<P: JsonRpcClient> Middleware for Provider<P> {
/// 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<U256, ProviderError> {
self.request("eth_estimateGas", [tx]).await
async fn estimate_gas(
&self,
tx: &TypedTransaction,
block: Option<BlockId>,
) -> Result<U256, ProviderError> {
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(
Expand Down Expand Up @@ -2064,57 +2046,27 @@ 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());
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
// --- 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();
Expand Down