-
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 1 commit
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 |
|---|---|---|
|
|
@@ -148,9 +148,15 @@ 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 { | ||
| // | ||
| // The same predicate also tells us whether an access list is strictly | ||
| // required for this settlement: the gas-limit workaround only matters | ||
| // when at least one trade sends ETH to a smart contract. For any other | ||
| // settlement the access list is purely a gas optimization, so a fetch | ||
| // failure should not abort submission. | ||
| let per_trade_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()); | ||
| return Result::<_, Error>::Ok((false, eth::AccessList::default())); | ||
| } | ||
| let tx = eth::Tx { | ||
| from: solution.solver().address(), | ||
|
|
@@ -159,17 +165,20 @@ impl Settlement { | |
| input: Default::default(), | ||
| access_list: Default::default(), | ||
| }; | ||
| Result::<_, Error>::Ok(simulator.access_list(&tx).await?) | ||
| Ok((true, simulator.access_list(&tx).await?)) | ||
| })) | ||
| .await?; | ||
| let partial_access_list = partial_access_lists | ||
| let access_list_required = per_trade_access_lists.iter().any(|(r, _)| *r); | ||
| let partial_access_list = per_trade_access_lists | ||
| .into_iter() | ||
| .map(|(_, list)| list) | ||
| .fold(eth::AccessList::default(), |acc, list| acc.merge(list)); | ||
|
squadgazzz marked this conversation as resolved.
Outdated
|
||
|
|
||
| // Simulate the settlement and get the access list and gas. | ||
| let (access_list, gas) = Self::simulate( | ||
| transaction.internalized.clone(), | ||
| &partial_access_list, | ||
| access_list_required, | ||
| eth, | ||
| simulator, | ||
| ) | ||
|
|
@@ -203,6 +212,7 @@ impl Settlement { | |
| Self::simulate( | ||
| transaction.uninternalized.clone(), | ||
| &partial_access_list, | ||
| access_list_required, | ||
| eth, | ||
| simulator, | ||
| ) | ||
|
|
@@ -224,15 +234,30 @@ impl Settlement { | |
| async fn simulate( | ||
| tx: eth::Tx, | ||
| partial_access_list: ð::AccessList, | ||
| access_list_required: bool, | ||
|
squadgazzz marked this conversation as resolved.
Outdated
|
||
| 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()); | ||
|
|
||
| // 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 an access | ||
| // list (no ETH -> contract transfer), treat the fetch as best-effort: | ||
| // a failing RPC would otherwise abort the whole settlement even though | ||
| // the on-chain tx would succeed without it. | ||
| let access_list = match simulator.access_list(&tx).await { | ||
| Ok(list) => list, | ||
| Err(err) if !access_list_required => { | ||
|
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 think here we need to do stricter error handling. If the error indicates a revert we should still abort. Only if we can't reason about the outcome of the simulation we should assume things are okay (e.g. deserialization error like in the post-mortem, transport error, something else? 🤷♂️).
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. Fixed |
||
| 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. | ||
|
|
||
|
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. The error handling should be split into a separate PR IMO.
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. Done #4358 |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,65 @@ | ||
| //! Axum extractors that emit a `warn` log when request deserialization | ||
| //! fails, then delegate to the stock extractor's rejection so the HTTP | ||
| //! response shape is unchanged. | ||
|
|
||
| use { | ||
| axum::{ | ||
| extract::{ | ||
| FromRequest, | ||
| FromRequestParts, | ||
| Request, | ||
| rejection::{JsonRejection, QueryRejection}, | ||
| }, | ||
| http::request::Parts, | ||
| }, | ||
| serde::de::DeserializeOwned, | ||
| }; | ||
|
|
||
| pub struct Json<T>(pub T); | ||
|
|
||
| impl<S, T> FromRequest<S> for Json<T> | ||
| where | ||
| S: Send + Sync, | ||
| T: DeserializeOwned, | ||
| { | ||
| type Rejection = JsonRejection; | ||
|
|
||
| async fn from_request(req: Request, state: &S) -> Result<Self, Self::Rejection> { | ||
| match axum::Json::<T>::from_request(req, state).await { | ||
| Ok(axum::Json(value)) => Ok(Self(value)), | ||
| Err(rejection) => { | ||
| tracing::warn!( | ||
| err = %rejection, | ||
| target = std::any::type_name::<T>(), | ||
| "failed to deserialize JSON request body", | ||
| ); | ||
| Err(rejection) | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| pub struct Query<T>(pub T); | ||
|
|
||
| impl<S, T> FromRequestParts<S> for Query<T> | ||
| where | ||
| S: Send + Sync, | ||
| T: DeserializeOwned, | ||
| { | ||
| type Rejection = QueryRejection; | ||
|
|
||
| async fn from_request_parts(parts: &mut Parts, state: &S) -> Result<Self, Self::Rejection> { | ||
| match axum::extract::Query::<T>::from_request_parts(parts, state).await { | ||
| Ok(axum::extract::Query(value)) => Ok(Self(value)), | ||
| Err(rejection) => { | ||
| tracing::warn!( | ||
| err = %rejection, | ||
| target = std::any::type_name::<T>(), | ||
| query = parts.uri.query().unwrap_or_default(), | ||
| "failed to deserialize query string", | ||
| ); | ||
| Err(rejection) | ||
| } | ||
| } | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -28,6 +28,7 @@ use { | |
| }; | ||
|
|
||
| mod error; | ||
| mod extract; | ||
| pub mod routes; | ||
|
|
||
| pub struct Api { | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.