From a1accbf6aca3bb4a395485eadb9bdc7f98706829 Mon Sep 17 00:00:00 2001 From: wolflo <33909953+wolflo@users.noreply.github.com> Date: Tue, 1 Mar 2022 05:13:06 -0700 Subject: [PATCH] fix(providers): Propogate gas limit with access list (#901) * fix(providers): Propogate gas price with access list * Update CHANGELOG.md * Fix clippy lint * Clarify fill_transaction comments * Fill tx gas price before gas limit Updates Provider::fill_transaction() to fill the gas price of a transaction before filling the gas limit. There are cases where the gas used by a transaction may be dependent on the gas price. For example, the following contract bytecode branches based on the result of the GASPRICE opcode: GASPRICE PUSH1 0xff GT PUSH1 {label} JUMPI * Cleanup * Propogate eth_estimateGas failure --- CHANGELOG.md | 2 + ethers-providers/src/provider.rs | 199 +++++++++++++++++++++++++++---- 2 files changed, 175 insertions(+), 26 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 16db8093..bbb59e47 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -42,6 +42,8 @@ - Add Arbitrum mainnet and testnet to the list of known chains - Add ENS avatar and TXT records resolution [#889](https://github.com/gakonst/ethers-rs/pull/889) +- Do not override gas limits provided by an outer middleware when including an EIP-2930 access list + [#901](https://github.com/gakonst/ethers-rs/pull/901) - Add a getter to `ProjectCompileOutput` that returns a mapping of compiler versions to a vector of name + contract struct tuples [#908](https://github.com/gakonst/ethers-rs/pull/908) diff --git a/ethers-providers/src/provider.rs b/ethers-providers/src/provider.rs index d2ddf209..c5192ced 100644 --- a/ethers-providers/src/provider.rs +++ b/ethers-providers/src/provider.rs @@ -275,8 +275,7 @@ impl Middleware for Provider

{ } } - // TODO: Can we poll the futures below at the same time? - // Access List + Name resolution and then Gas price + Gas + // TODO: Join the name resolution and gas price future // set the ENS name if let Some(NameOrAddress::Name(ref ens_name)) = tx.to() { @@ -284,29 +283,7 @@ impl Middleware for Provider

{ 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); - } - + // fill gas price match tx { TypedTransaction::Eip2930(_) | TypedTransaction::Legacy(_) => { let gas_price = maybe(tx.gas_price(), self.get_gas_price()).await?; @@ -322,6 +299,37 @@ 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?; + tx.set_gas(gas_estimate); + } + Ok(()) } @@ -1462,7 +1470,9 @@ mod tests { use super::*; use crate::Http; use ethers_core::{ - types::{TransactionRequest, H256}, + types::{ + transaction::eip2930::AccessList, Eip1559TransactionRequest, TransactionRequest, H256, + }, utils::Geth, }; use futures_util::StreamExt; @@ -1692,4 +1702,141 @@ mod tests { .unwrap(); dbg!(traces); } + + #[tokio::test] + async fn test_fill_transaction_1559() { + let (mut provider, mock) = Provider::mocked(); + provider.from = Some("0x6fC21092DA55B392b045eD78F4732bff3C580e2c".parse().unwrap()); + + let gas = U256::from(21000_usize); + let basefee = U256::from(25_usize); + let prio_fee = U256::from(25_usize); + let access_list: AccessList = vec![Default::default()].into(); + + // --- leaves a filled 1559 transaction unchanged, making no requests + let from: Address = "0x0000000000000000000000000000000000000001".parse().unwrap(); + let to: Address = "0x0000000000000000000000000000000000000002".parse().unwrap(); + let mut tx = Eip1559TransactionRequest::new() + .from(from) + .to(to) + .gas(gas) + .max_fee_per_gas(basefee) + .max_priority_fee_per_gas(prio_fee) + .access_list(access_list.clone()) + .into(); + provider.fill_transaction(&mut tx, None).await.unwrap(); + + assert_eq!(tx.from(), Some(&from)); + assert_eq!(tx.to(), Some(&to.into())); + assert_eq!(tx.gas(), Some(&gas)); + assert_eq!(tx.gas_price(), Some(basefee + prio_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; + let mut tx = Eip1559TransactionRequest::new() + .gas(gas) + .max_fee_per_gas(basefee) + .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(basefee) + .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 + let mut tx = Eip1559TransactionRequest::new() + .max_fee_per_gas(basefee) + .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(); + + 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())); + + // --- propogates estimate_gas() error + let mut tx = Eip1559TransactionRequest::new() + .max_fee_per_gas(basefee) + .max_priority_fee_per_gas(prio_fee) + .into(); + + // bad mock value causes error response for eth_estimateGas + mock.push(b'b').unwrap(); + + let res = provider.fill_transaction(&mut tx, None).await; + + assert!(matches!(res, Err(ProviderError::JsonRpcClientError(_)))); + } + + #[tokio::test] + async fn test_fill_transaction_legacy() { + let (mut provider, mock) = Provider::mocked(); + provider.from = Some("0x6fC21092DA55B392b045eD78F4732bff3C580e2c".parse().unwrap()); + + let gas = U256::from(21000_usize); + let gas_price = U256::from(50_usize); + + // --- leaves a filled legacy transaction unchanged, making no requests + let from: Address = "0x0000000000000000000000000000000000000001".parse().unwrap(); + let to: Address = "0x0000000000000000000000000000000000000002".parse().unwrap(); + let mut tx = + TransactionRequest::new().from(from).to(to).gas(gas).gas_price(gas_price).into(); + provider.fill_transaction(&mut tx, None).await.unwrap(); + + assert_eq!(tx.from(), Some(&from)); + assert_eq!(tx.to(), Some(&to.into())); + assert_eq!(tx.gas(), Some(&gas)); + assert_eq!(tx.gas_price(), Some(gas_price)); + assert!(tx.access_list().is_none()); + + // --- fills an empty legacy transaction + let mut tx = TransactionRequest::new().into(); + mock.push(gas).unwrap(); + mock.push(gas_price).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.gas_price(), Some(gas_price)); + assert!(tx.access_list().is_none()); + } }