From ef9b3988ce2f0b4f4fc100513cb9c15ab6568885 Mon Sep 17 00:00:00 2001 From: Georgios Konstantopoulos Date: Wed, 20 Oct 2021 23:18:58 +0300 Subject: [PATCH] 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 --- Cargo.toml | 2 +- ethers-core/src/types/transaction/eip2718.rs | 36 +++++- ethers-providers/src/lib.rs | 112 ++++++++----------- 3 files changed, 81 insertions(+), 69 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index 9a1525ee..c93e3c2f 100644 --- 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 index c70ab99e..6be9dae4 100644 --- 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 { + 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>(&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 index 5d2b9dd6..417dac6e 100644 --- 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, ) -> Result<(), Self::Error> { - let mut tx_clone = tx.clone(); + 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); + } - // 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); + TypedTransaction::Eip2930(_) | TypedTransaction::Legacy(_) => { + let gas_price = maybe(tx.gas_price(), self.get_gas_price()).await?; + tx.set_gas_price(gas_price); } - 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(); - } - - 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); - + 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(()) }