-
Notifications
You must be signed in to change notification settings - Fork 162
Make driver access list fetch optional #4353
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 4 commits
5b02c74
4d965e2
fefcbde
788c229
1120878
5fa6473
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,10 @@ | ||
| use { | ||
| super::{Error, Solution, encoding, trade::ClearingPrices}, | ||
| super::{ | ||
| Error, | ||
| Solution, | ||
| encoding, | ||
| trade::{self, ClearingPrices}, | ||
| }, | ||
| crate::{ | ||
| domain::{ | ||
| self, | ||
|
|
@@ -15,7 +20,7 @@ use { | |
| alloy::primitives::U256, | ||
| eth_domain_types as eth, | ||
| futures::future::try_join_all, | ||
| simulator::Simulator, | ||
| simulator::{self, Simulator}, | ||
| std::collections::{BTreeSet, HashMap, HashSet}, | ||
| tracing::instrument, | ||
| }; | ||
|
|
@@ -148,23 +153,19 @@ impl Settlement { | |
| // The solution is to do access list estimation in two steps: first, simulate | ||
| // moving 1 wei into every smart contract to get a partial access list, and then | ||
| // use that partial access list to calculate the final access list. | ||
| let partial_access_lists = try_join_all(solution.user_trades().map(|trade| async { | ||
| if !trade.order().buys_eth() || !trade.order().pays_to_contract(eth).await? { | ||
| return Ok(Default::default()); | ||
| } | ||
| let tx = eth::Tx { | ||
| from: solution.solver().address(), | ||
| to: trade.order().receiver(), | ||
| value: 1.into(), | ||
| input: Default::default(), | ||
| access_list: Default::default(), | ||
| }; | ||
| Result::<_, Error>::Ok(simulator.access_list(&tx).await?) | ||
| })) | ||
| .await?; | ||
| let partial_access_list = partial_access_lists | ||
| .into_iter() | ||
| .fold(eth::AccessList::default(), |acc, list| acc.merge(list)); | ||
| // | ||
| // A non-empty partial access list also signals that at least one trade | ||
| // strictly requires an access list. Conversely, an empty partial list | ||
| // means the access list is only a gas optimization for this settlement, | ||
| // so a fetch failure below can be tolerated. | ||
| let partial_access_list = try_join_all( | ||
| solution | ||
| .user_trades() | ||
| .map(|trade| partial_access_list_for(trade, &solution, eth, simulator)), | ||
| ) | ||
| .await? | ||
| .into_iter() | ||
| .fold(eth::AccessList::default(), |acc, list| acc.merge(list)); | ||
|
|
||
| // Simulate the settlement and get the access list and gas. | ||
| let (access_list, gas) = Self::simulate( | ||
|
|
@@ -231,8 +232,25 @@ impl Settlement { | |
| let tx = tx.set_access_list(partial_access_list.to_owned()); | ||
|
|
||
| // Simulate the full access list, passing the partial access | ||
| // list into the simulation. | ||
| let access_list = simulator.access_list(&tx).await?; | ||
| // list into the simulation. If the partial list is empty, no trade | ||
| // strictly requires the access-list workaround, so a non-revert | ||
| // failure (transport, deserialization, ...) shouldn't abort the | ||
| // settlement: we can't reason about the outcome, but the on-chain | ||
| // tx would still succeed without the access list. Revert errors are | ||
| // propagated either way since they signal a real problem with the | ||
| // settlement. | ||
| let access_list = match simulator.access_list(&tx).await { | ||
| Ok(list) => list, | ||
| Err(simulator::Error::Other(err)) if partial_access_list.is_empty() => { | ||
| tracing::warn!( | ||
| ?err, | ||
| "access list estimation failed; continuing without it (no ETH->contract \ | ||
| trades)" | ||
|
Comment on lines
+252
to
+253
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Like the other comment, I also don't understand what "ETH->contract" is supposed to mean, is it a transfer? |
||
| ); | ||
| partial_access_list.clone() | ||
| } | ||
| Err(err) => return Err(err.into()), | ||
| }; | ||
| let tx = tx.set_access_list(access_list.clone()); | ||
|
|
||
| // Simulate the settlement using the full access list and get the gas used. | ||
|
|
@@ -353,6 +371,28 @@ impl Settlement { | |
| } | ||
| } | ||
|
|
||
| /// Return the partial access list entries needed to forward 1 wei of ETH to a | ||
| /// smart-contract receiver. Returns an empty access list when the trade does | ||
| /// not trigger the gas-limit workaround (non-ETH buy or EOA receiver). | ||
|
squadgazzz marked this conversation as resolved.
Outdated
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I wonder if a new type makes sense here:
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Makes sense. Done in 5fa6473 |
||
| async fn partial_access_list_for( | ||
|
Comment on lines
+379
to
+389
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. "gas-limit workaround" is a bit abstract here, could you cover what it exactly is? My understanding it that without the access list the TX would not pass below the gas limit? |
||
| trade: &trade::Fulfillment, | ||
| solution: &Solution, | ||
| eth: &Ethereum, | ||
| simulator: &Simulator, | ||
| ) -> Result<eth::AccessList, Error> { | ||
| if !trade.order().buys_eth() || !trade.order().pays_to_contract(eth).await? { | ||
| return Ok(eth::AccessList::default()); | ||
| } | ||
| let tx = eth::Tx { | ||
| from: solution.solver().address(), | ||
| to: trade.order().receiver(), | ||
| value: 1.into(), | ||
| input: Default::default(), | ||
| access_list: Default::default(), | ||
| }; | ||
| Ok(simulator.access_list(&tx).await?) | ||
| } | ||
|
|
||
| /// Should the interactions be internalized? | ||
| #[derive(Debug, Clone, Copy, PartialEq, Eq)] | ||
| pub enum Internalization { | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please double check that matching on
Error::Otheris sufficient / correct here?What I'm worried about is that the
Othervariant still contains a bunch of revert related errors internally.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Based on this
services/crates/simulator/src/lib.rs
Lines 152 to 169 in 3dd0844
, the
Error::Otheris used only for http and Tenderly's "other" errors. Everything else is mapped toError::Revert.