-
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
Open
squadgazzz
wants to merge
6
commits into
main
Choose a base branch
from
feat/driver-optional-access-list-and-deser-warn
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+77
−25
Open
Changes from all commits
Commits
Show all changes
6 commits
Select commit
Hold shift + click to select a range
5b02c74
Make driver access list fetch optional and warn on deser errors
squadgazzz 4d965e2
Address review feedback
squadgazzz fefcbde
Only suppress non-revert errors in access list fallback
squadgazzz 788c229
Split request deserialization logging into separate PR
squadgazzz 1120878
Merge branch 'main' into feat/driver-optional-access-list-and-deser-warn
squadgazzz 5fa6473
Introduce RequiredAccessList newtype
squadgazzz File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| 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,28 +153,26 @@ 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)); | ||
| // | ||
| // `Some(..)` means at least one trade strictly requires an access list; | ||
| // `None` means it is purely a gas optimization for this settlement, so a | ||
| // non-revert fetch failure below can be tolerated. | ||
| let partial_access_list: Option<RequiredAccessList> = try_join_all( | ||
| solution | ||
| .user_trades() | ||
| .map(|trade| partial_access_list_for(trade, &solution, eth, simulator)), | ||
| ) | ||
| .await? | ||
| .into_iter() | ||
| .flatten() | ||
| .map(|required| required.0) | ||
| .reduce(|acc, list| acc.merge(list)) | ||
| .map(RequiredAccessList); | ||
|
|
||
| // Simulate the settlement and get the access list and gas. | ||
| let (access_list, gas) = Self::simulate( | ||
| transaction.internalized.clone(), | ||
| &partial_access_list, | ||
| partial_access_list.as_ref(), | ||
| eth, | ||
| simulator, | ||
| ) | ||
|
|
@@ -202,7 +205,7 @@ impl Settlement { | |
| // that the settlement simulates even when internalizations are disabled. | ||
| Self::simulate( | ||
| transaction.uninternalized.clone(), | ||
| &partial_access_list, | ||
| partial_access_list.as_ref(), | ||
| eth, | ||
| simulator, | ||
| ) | ||
|
|
@@ -223,16 +226,36 @@ impl Settlement { | |
| #[instrument(name = "simulate_settlement", skip_all)] | ||
| async fn simulate( | ||
| tx: eth::Tx, | ||
| partial_access_list: ð::AccessList, | ||
| partial_access_list: Option<&RequiredAccessList>, | ||
| eth: &Ethereum, | ||
| simulator: &Simulator, | ||
| ) -> Result<(eth::AccessList, eth::Gas), Error> { | ||
| // Add the partial access list to the settlement tx. | ||
| let tx = tx.set_access_list(partial_access_list.to_owned()); | ||
| let tx = tx.set_access_list( | ||
| partial_access_list | ||
| .map(|required| required.0.clone()) | ||
| .unwrap_or_default(), | ||
| ); | ||
|
|
||
| // 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. When no trade strictly requires the | ||
| // access-list workaround, 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_none() => { | ||
| tracing::warn!( | ||
| ?err, | ||
| "access list estimation failed; continuing without it (no ETH->contract \ | ||
| trades)" | ||
| ); | ||
| eth::AccessList::default() | ||
| } | ||
| 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 +376,35 @@ impl Settlement { | |
| } | ||
| } | ||
|
|
||
| /// Access list entries covering a trade that strictly requires the | ||
| /// ETH→contract gas-limit workaround. Distinct from a bare `eth::AccessList` | ||
| /// so the settlement-level fallback can tell "no trade required one" (`None`) | ||
| /// apart from "trade required one but the list turned out empty" (`Some`). | ||
| #[derive(Debug, Clone)] | ||
| struct RequiredAccessList(eth::AccessList); | ||
|
|
||
| /// Return the partial access list for a single trade. `None` means the trade | ||
| /// does not trigger the gas-limit workaround (non-ETH buy or EOA receiver), | ||
| /// so its absence must not be treated as "required but empty". | ||
| 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<Option<RequiredAccessList>, Error> { | ||
| if !trade.order().buys_eth() || !trade.order().pays_to_contract(eth).await? { | ||
| return Ok(None); | ||
| } | ||
| let tx = eth::Tx { | ||
| from: solution.solver().address(), | ||
| to: trade.order().receiver(), | ||
| value: 1.into(), | ||
| input: Default::default(), | ||
| access_list: Default::default(), | ||
| }; | ||
| Ok(Some(RequiredAccessList(simulator.access_list(&tx).await?))) | ||
| } | ||
|
|
||
| /// Should the interactions be internalized? | ||
| #[derive(Debug, Clone, Copy, PartialEq, Eq)] | ||
| pub enum Internalization { | ||
|
|
||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Like the other comment, I also don't understand what "ETH->contract" is supposed to mean, is it a transfer?