-
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 2 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, | ||
|
|
@@ -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,22 @@ 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 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 partial_access_list.is_empty() => { | ||
| tracing::warn!( | ||
| ?err, | ||
| "access list estimation failed; continuing without it (no ETH->contract \ | ||
| trades)" | ||
| ); | ||
| 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 +368,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 { | ||
|
|
||
|
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 LoggingJson<T>(pub T); | ||
|
|
||
| impl<S, T> FromRequest<S> for LoggingJson<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 LoggingQuery<T>(pub T); | ||
|
|
||
| impl<S, T> FromRequestParts<S> for LoggingQuery<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 { | ||
|
|
||
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?