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
This commit is contained in:
wolflo 2022-03-01 05:13:06 -07:00 committed by GitHub
parent a158e12ed0
commit a1accbf6ac
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
2 changed files with 175 additions and 26 deletions

View File

@ -42,6 +42,8 @@
- Add Arbitrum mainnet and testnet to the list of known chains - Add Arbitrum mainnet and testnet to the list of known chains
- Add ENS avatar and TXT records resolution - Add ENS avatar and TXT records resolution
[#889](https://github.com/gakonst/ethers-rs/pull/889) [#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 - Add a getter to `ProjectCompileOutput` that returns a mapping of compiler
versions to a vector of name + contract struct tuples versions to a vector of name + contract struct tuples
[#908](https://github.com/gakonst/ethers-rs/pull/908) [#908](https://github.com/gakonst/ethers-rs/pull/908)

View File

@ -275,8 +275,7 @@ impl<P: JsonRpcClient> Middleware for Provider<P> {
} }
} }
// TODO: Can we poll the futures below at the same time? // TODO: Join the name resolution and gas price future
// Access List + Name resolution and then Gas price + Gas
// set the ENS name // set the ENS name
if let Some(NameOrAddress::Name(ref ens_name)) = tx.to() { if let Some(NameOrAddress::Name(ref ens_name)) = tx.to() {
@ -284,29 +283,7 @@ impl<P: JsonRpcClient> Middleware for Provider<P> {
tx.set_to(addr); tx.set_to(addr);
} }
// estimate the gas without the access list // fill gas price
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 { match tx {
TypedTransaction::Eip2930(_) | TypedTransaction::Legacy(_) => { TypedTransaction::Eip2930(_) | TypedTransaction::Legacy(_) => {
let gas_price = maybe(tx.gas_price(), self.get_gas_price()).await?; let gas_price = maybe(tx.gas_price(), self.get_gas_price()).await?;
@ -322,6 +299,37 @@ 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?;
tx.set_gas(gas_estimate);
}
Ok(()) Ok(())
} }
@ -1462,7 +1470,9 @@ mod tests {
use super::*; use super::*;
use crate::Http; use crate::Http;
use ethers_core::{ use ethers_core::{
types::{TransactionRequest, H256}, types::{
transaction::eip2930::AccessList, Eip1559TransactionRequest, TransactionRequest, H256,
},
utils::Geth, utils::Geth,
}; };
use futures_util::StreamExt; use futures_util::StreamExt;
@ -1692,4 +1702,141 @@ mod tests {
.unwrap(); .unwrap();
dbg!(traces); 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());
}
} }