Make driver access list fetch optional by squadgazzz · Pull Request #4353 · cowprotocol/services

squadgazzz · 2026-04-21T11:00:10Z

Description

The driver currently aborts the whole settlement whenever simulator.access_list fails. The access list is only functionally required for trades that send ETH to a smart-contract receiver (the EVM hard gas limit bypass described at settlement.rs line 140). For every other settlement it is just a gas optimization, so a failed RPC should not cost the solver the auction.

Changes

Access list is fetched best-effort when the partial access list is empty (no ETH->contract trade). On a non-revert failure (transport, deserialization, etc.) the driver now logs a warn and falls back to the empty partial list instead of aborting the settlement.
Revert errors are always propagated, even with an empty partial list, since they signal a real problem with the settlement.

How to test

Existing tests.

gemini-code-assist

Code Review

This pull request introduces enhanced logging for request deserialization and makes access list estimation best-effort for certain settlements. Specifically, it adds custom Axum extractors for JSON and Query types that log warnings on failure. It also updates the settlement simulation logic to treat access list fetch failures as non-fatal warnings when no ETH-to-contract transfers are involved, preventing unnecessary transaction aborts. I have no feedback to provide.

MartinquaXD

Sorry, forgot to submit the review yesterday. :(

MartinquaXD · 2026-04-21T13:15:17Z

+        // 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 => {


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? 🤷‍♂️).

MartinquaXD · 2026-04-21T13:16:57Z

The error handling should be split into a separate PR IMO.

A revert from simulator.access_list signals a real problem with the settlement even when no trade strictly requires the access-list workaround, so it must still propagate. The empty-partial-list fallback now fires only for simulator::Error::Other (transport, deserialization, etc.) where we cannot reason about the simulation outcome.

Revert the LoggingJson / LoggingQuery extractors and the /solve body parse warning. They land in a follow-up PR so this branch only covers the access-list-fetch optionality change.

MartinquaXD · 2026-04-22T13:39:06Z

+        // settlement.
+        let access_list = match simulator.access_list(&tx).await {
+            Ok(list) => list,
+            Err(simulator::Error::Other(err)) if partial_access_list.is_empty() => {


Can you please double check that matching on Error::Other is sufficient / correct here?
What I'm worried about is that the Other variant still contains a bunch of revert related errors internally.

Based on this

services/crates/simulator/src/lib.rs

Lines 152 to 169 in 3dd0844

move |err| {

let err: SimulatorError = err.into();

let tx = match &err {

SimulatorError::Tenderly(tenderly::Error::Http(_)) => None,

SimulatorError::Tenderly(tenderly::Error::Revert(_)) => Some(tx.clone()),

SimulatorError::Tenderly(tenderly::Error::Other(_)) => None,

SimulatorError::Ethereum(_) => Some(tx.clone()),

SimulatorError::GasExceeded(..) => Some(tx.clone()),

};

match tx {

Some(tx) => Error::Revert(RevertError {

err,

tx: tx.clone(),

block,

}),

None => Error::Other(err),

}

}

, the Error::Other is used only for http and Tenderly's "other" errors. Everything else is mapped to Error::Revert.

MartinquaXD · 2026-04-22T13:40:25Z

+/// smart-contract receiver. Returns an empty access list when the trade does
+/// not trigger the gas-limit workaround (non-ETH buy or EOA receiver).


I wonder if a new type makes sense here: RequiredAccessList.
This function would then return Result<Option<RequiredAccessList>> to indicate on a type level whether we can continue after non-revert errors.

Makes sense. Done in 5fa6473

Encodes "trade needs the ETH->contract gas-limit workaround" at the type level. partial_access_list_for returns Option<RequiredAccessList> (None = not required), the aggregation reduces only the required entries, and the simulate() fallback keys off is_none() instead of is_empty() — unambiguous regardless of whether a required entry set happens to be empty.

jmg-duarte

Codewise LGTM but there's space to make the docs clearer

jmg-duarte · 2026-04-24T09:08:31Z

+/// 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(


"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?

jmg-duarte · 2026-04-24T09:10:50Z

+                    "access list estimation failed; continuing without it (no ETH->contract \
+                     trades)"


Like the other comment, I also don't understand what "ETH->contract" is supposed to mean, is it a transfer?

Make driver access list fetch optional and warn on deser errors

5b02c74

squadgazzz marked this pull request as ready for review April 21, 2026 11:31

squadgazzz requested a review from a team as a code owner April 21, 2026 11:31

gemini-code-assist Bot reviewed Apr 21, 2026

View reviewed changes

jmg-duarte reviewed Apr 21, 2026

View reviewed changes

Address review feedback

4d965e2

squadgazzz requested a review from jmg-duarte April 22, 2026 07:48

MartinquaXD reviewed Apr 22, 2026

View reviewed changes

squadgazzz added 2 commits April 22, 2026 11:36

Split request deserialization logging into separate PR

788c229

Revert the LoggingJson / LoggingQuery extractors and the /solve body parse warning. They land in a follow-up PR so this branch only covers the access-list-fetch optionality change.

squadgazzz mentioned this pull request Apr 22, 2026

Log driver API request deserialization errors #4358

Open

squadgazzz changed the title ~~Make driver access list fetch optional and warn on deser errors~~ Make driver access list fetch optional Apr 22, 2026

Merge branch 'main' into feat/driver-optional-access-list-and-deser-warn

1120878

squadgazzz requested a review from MartinquaXD April 22, 2026 13:39

MartinquaXD reviewed Apr 22, 2026

View reviewed changes

squadgazzz requested a review from MartinquaXD April 22, 2026 15:16

jmg-duarte reviewed Apr 24, 2026

View reviewed changes

	move \|err\| {
	let err: SimulatorError = err.into();
	let tx = match &err {
	SimulatorError::Tenderly(tenderly::Error::Http(_)) => None,
	SimulatorError::Tenderly(tenderly::Error::Revert(_)) => Some(tx.clone()),
	SimulatorError::Tenderly(tenderly::Error::Other(_)) => None,
	SimulatorError::Ethereum(_) => Some(tx.clone()),
	SimulatorError::GasExceeded(..) => Some(tx.clone()),
	};
	match tx {
	Some(tx) => Error::Revert(RevertError {
	err,
	tx: tx.clone(),
	block,
	}),
	None => Error::Other(err),
	}
	}

		/// smart-contract receiver. Returns an empty access list when the trade does
		/// not trigger the gas-limit workaround (non-ETH buy or EOA receiver).

		"access list estimation failed; continuing without it (no ETH->contract \
		trades)"

Conversation

squadgazzz commented Apr 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Changes

How to test

Uh oh!

gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

MartinquaXD left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jmg-duarte left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

squadgazzz commented Apr 21, 2026 •

edited

Loading