Simulate EIP-1271 orders at creation#4366
Open
squadgazzz wants to merge 78 commits intonew-api-simulator-cratefrom
Open
Simulate EIP-1271 orders at creation#4366squadgazzz wants to merge 78 commits intonew-api-simulator-cratefrom
squadgazzz wants to merge 78 commits intonew-api-simulator-cratefrom
Conversation
Runs the OrderSimulator concurrently with the cheap isValidSignature check. In shadow mode (default), logs disagreements via metrics and structured logs but returns the cheap check's result. In enforce mode, (cheap Pass, sim Fail) is upgraded to ValidationError::SimulationFailed; other combinations stay unchanged. Infra errors never reject. Covers scope from plan Tasks 3, 4 and 5: shadow-mode quadrants, enforce-mode cases, infra/skip-flag/no-sim paths.
The Shadow/Enforce distinction is a mode variant, not a property of the capability — the same simulator infrastructure is active in both modes. Keeping "shadow" only where it names a mode variant, and renaming the trait, error, mode enum, metrics, constant, config fields, and module accordingly.
Doc comments and local variable names still described the capability as 'shadow' — rewritten to reference the mode variant only where it genuinely applies (config docs, test names that exercise Shadow mode, the Shadow enum variant).
The simulator, mode, and timeout are only meaningful together. Collapsing them into a single Option<Eip1271SimConfig> field lets the call site in run.rs return None cleanly when order_simulation isn't configured, instead of passing placeholder mode/timeout values that aren't read.
… to Simulator Matches the project's convention of -ing suffix for traits (OrderValidating/OrderValidator, SignatureValidating, BalanceFetching). The former Eip1271SimConfig bundle becomes the concrete Eip1271Simulator struct, and the trait it depends on becomes Eip1271Simulating.
The helper now accepts the full simulator bundle instead of three separate (simulator, mode, timeout) args. Introduces shadow_mode_sim / enforce_mode_sim helpers to keep test call sites tight.
The constant is only used by tests; keeping it at module scope (and public) implied an API contract with the configs crate that doesn't exist. configs::orderbook::default_eip1271_sim_timeout is authoritative at runtime.
The existing EIP-1271 check is an isValidSignature call; 'cheap' was editorializing on cost rather than describing what it does. Renaming to 'signature' for the metric axis, outcome enum, and supporting names. Also collapsing sim_only_total into total by adding a 'skipped' value to the signature axis — one counter with a signature × sim matrix covers every case the two used to cover.
…configs, logs The Eip1271Simulator struct keeps its -Simulator suffix (matching OrderValidator/-Validating), but everywhere sim was used as a modifier or qualifier (enum variants, error type, metric subsystem/labels, config fields, log messages, test names) it now reads as simulation.
Operators can now opt out of the simulation at order creation on a per-chain basis without giving up the /debug/simulation endpoint. The shared mode enum stays binary (Shadow/Enforce); Disabled translates to None for OrderValidator at the wiring layer. The OrderSimulator is still constructed for the debug endpoint. Also removed the redundant impls_trait compile-check test in eip1271_simulation.rs — the impl block above already enforces that at compile time.
Mock both the signature validator and the simulator with times(0) and submit an Eip712 (EOA) order. Catches a regression where the sim is accidentally wired to run for non-1271 orders.
The seven near-identical tests covering every (signature, simulation, mode) combination collapsed into one table-driven test with a single mock-driven helper. Failure messages include a label per row so any regression still pinpoints the failing cell. Also inlined the now-unused enforce_mode_simulator helper and replaced shadow_mode_simulator with a general simulator_with_mode.
- Move simulation_time histogram timer inside simulation_fut so it no longer captures the max of sig-check + simulation latency. - Warn-level (not info) for simulation failures in the eip1271_skip_creation_validation path, matching the normal path. - Drop Default derive on the shared Eip1271SimulationMode since the operational default lives in configs (Disabled) and no code reaches for it. Updated the doc to clarify the split. - New configs test asserting "shadow" deserializes to the Shadow variant.
…ototype-eip1271-on-new-api
squadgazzz
changed the title
# Conflicts: # crates/orderbook/src/run.rs
Wires the call sites that the merge from main left broken: - `SettlementSimulator::new` now takes `native_token` and `max_gas_limit`. - `Prices::Limit` is gone, set per-order via `Order::fill_at(_, PriceEncoding::LimitPrice)`. - `add_order` -> `add_orders`, `with_override` -> `with_overrides`. Also collapses the two `SettlementSimulator` instances in `orderbook/run.rs` into one shared by the EIP-1271 adapter and the orderbook. Side-effect: the orderbook's order simulator now gets the real FlashLoanRouter address instead of the zero default the second build site was passing.
# Conflicts: # crates/e2e/tests/e2e/malformed_requests.rs # crates/orderbook/src/run.rs
- Restore the single shared SettlementSimulator that 9338743 introduced. The merge brought back the old two-block layout where the eip1271 path used unwrap_or_default() for the flashloan router, silently falling back to the zero address on chains without a deployment. - Update simulator call sites to the new API in the base branch: add_orders -> with_orders, with_overrides([BuyTokensForBuffers]) -> provide_sufficient_buy_tokens(). Affects orderbook eip1271_simulation and the aave_replay test. - Restore the "Invalid kind enum value" comment on the http_validation test, which the merge replaced with text that did not match the body.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces EIP-1271 order simulation at creation time, allowing the orderbook to verify that orders with smart contract signatures will successfully settle. It adds configuration for simulation modes (Shadow, Enforce, Disabled) and timeouts, implements an OrderSimulatorAdapter using the existing SettlementSimulator, and integrates this logic into the OrderValidator. New E2E and replay tests are included to verify the simulation behavior and its impact on order acceptance. No critical issues were found, and I have no feedback to provide.
MartinquaXD
reviewed
May 6, 2026
Contributor
MartinquaXD
left a comment
MartinquaXD
left a comment
There was a problem hiding this comment.
I still have to formulate a more helpful feedback but this seems like an insane amount of code for optionally running some extra check and comparing results.
Other than that all of the wording is eip1271 specific when long term the new logic is supposed be used for all orders.
|
|
||
| /// Mode for the EIP-1271 order simulation. | ||
| #[serde(default)] | ||
| pub eip1271_simulation_mode: Eip1271SimulationMode, |
Contributor
There was a problem hiding this comment.
The new simulation logic is technically not tied to EIP 1271.
Contributor
Author
There was a problem hiding this comment.
Yeah, sorry about that, it probably was changed at some point and I forgot to update the naming and comments.
Comment on lines
+69
to
+72
| /// Defined here rather than in `crates/simulator` because `OrderValidator` | ||
| /// cannot depend on `orderbook`, where the concrete implementation lives. | ||
| /// To be removed once the `simulator` crate's API can be depended on | ||
| /// directly. |
Contributor
There was a problem hiding this comment.
should not be a doc comment.
Contributor
Author
There was a problem hiding this comment.
This is actually an outdated comment. Since you finalized the API, this was refactored in 888e0b1 (this PR)
jmg-duarte
reviewed
May 6, 2026
Comment on lines
+64
to
+73
| /// Mode for the EIP-1271 order simulation. | ||
| #[serde(default)] | ||
| pub eip1271_simulation_mode: Eip1271SimulationMode, | ||
|
|
||
| /// Per-call timeout for the EIP-1271 order simulation. | ||
| #[serde( | ||
| default = "default_eip1271_simulation_timeout", | ||
| with = "humantime_serde" | ||
| )] | ||
| pub eip1271_simulation_timeout: Duration, |
Contributor
There was a problem hiding this comment.
as is, is ok, but the prefix here hints me that it could probably be
eip1271_simulation: {mode, timeout}
Contributor
Author
There was a problem hiding this comment.
No need for that anymore, I renamed the configs.
| "version": "1.14.0" | ||
| }"#; | ||
|
|
||
| /// Returns `app_data` minified with keys sorted alphabetically. The output |
Contributor
There was a problem hiding this comment.
I'm curious how do you make sure that "with keys sorted alphabetically" is always the case
| let _timer = Eip1271SimulationMetrics::get() | ||
| .simulation_time | ||
| .start_timer(); | ||
| match tokio::time::timeout(timeout, sim.simulate(order, full_app_data)).await { |
Contributor
There was a problem hiding this comment.
nit: split the timeout out
Contributor
There was a problem hiding this comment.
let Ok(simulation_result) = tokio::time::timeout(timeout, sim.simulate(order, full_app_data)).await else {
return SimulationOutcome::Infra(anyhow!("eip1271 simulation timeout"))
}
match simulation_result { ... }
I find this easier to read because we're not mixing two failure modes together
- Rename EIP-1271-specific identifiers to order-simulation since the
underlying logic is not 1271-specific. Affects the trait
(Eip1271Simulating -> OrderSimulating), error type, mode enum,
bundle struct, metrics, config fields (eip1271_simulation_{mode,
timeout} -> order_simulation_{mode, timeout}), file name
(eip1271_simulation.rs -> order_simulation.rs), and the API error
code (Eip1271SimulationFailed -> OrderSimulationFailed). Per
MartinquaXD's review.
- Make Tenderly's simulate_and_share lazy in the order-simulation
adapter. Previously every successful simulation triggered a
Tenderly API call whose URL was discarded. Now the local eth_call
runs first and Tenderly is only consulted when there is a revert
to attach a diagnostic URL to the error. Adds SettlementSimulator::
tenderly() so the adapter can reach the configured Tenderly handle.
- Move the maintainer note about why the Eip1271Simulating trait
lives in `shared` rather than `simulator` from a doc comment to a
plain `//` comment. The trait's user-facing summary stays as
rustdoc.
- Inline the single-use build_preview_order_for_sim helper at its
one call site.
- Switch the OrderSimulationConfig parse-test fixtures from hex
"0x1000000" to decimal "16777216" to match the rest of the
gas-limit configuration in the codebase.
- Rephrase canonicalise_app_data's doc to credit serde_json::Value's
BTreeMap backing for the alphabetical ordering rather than the
input being pre-sorted.
- Rename the test no_simulator_configured_preserves_existing_behaviour
to no_simulator_configured_returns_invalid_eip1271_signature_on_invalid_signature
so the assertion is named after the actual behaviour, not "existing
behaviour" which rots.
- Replace Address::from([0xcf; 20]) with Address::repeat_byte(0xcf)
to match Address::repeat_byte(0xef) on the sibling line.
- Tighten the run_eip1271_checks rustdoc with doclinks and a clearer
true/false split for eip1271_skip_creation_validation.
- Add TestDefault for OrderSimulationConfig and use destructuring in
the e2e enforce-mode helper (replaces the .as_mut().expect() chain).
The trait used to live in `shared::order_validation` because `OrderValidator` could not depend on `orderbook` (where the concrete adapter lived). The `simulator` crate's API is stable now, so: - Add `simulator` to `shared`'s `[dependencies]` (and to `[dev-dependencies]` with the `test-util` feature so `MockOrderSimulating` is available to validator tests). - Move `OrderSimulating` trait, `OrderSimulationError` enum, and `OrderSimulatorAdapter` into a new `simulator::order_simulation` module. - Re-export the trait + error from `simulator` and import them in `shared::order_validation` instead of redefining. - Delete `crates/orderbook/src/order_simulation.rs` and its module declaration; `orderbook::run` now constructs the adapter directly from `simulator::order_simulation::OrderSimulatorAdapter`. - The mockall attribute on the trait switches from `cfg(test)` to `any(test, feature = "test-util")` so downstream crates can use `MockOrderSimulating`. The `OrderSimulator` bundle (simulator + mode + timeout) and `OrderSimulationMode` stay in `shared` since they describe the validator's behaviour, not the simulation itself.
`OrderSimulationConfig.order_simulation_mode` and `OrderSimulationConfig.order_simulation_timeout` repeated the `order_simulation_` prefix that the struct name already conveys. Drop the prefix: - `order_simulation_mode` -> `mode` - `order_simulation_timeout` -> `timeout` - `default_order_simulation_timeout` -> `default_simulation_timeout` TOML keys go from `order-simulation-mode` / `order-simulation-timeout` to `mode` / `timeout`. Updates the parse-test fixtures, the TestDefault impl, run.rs wiring, and the e2e enforce-mode helper.
MartinquaXD
reviewed
May 7, 2026
Contributor
MartinquaXD
left a comment
MartinquaXD
left a comment
There was a problem hiding this comment.
I think I see now why this is a lot more code than I anticipated. This PR seems to implement this change as it it's supposed to stay like this for a long time when in reality we only want to have a short test period where we run both in parallel, iron out any issues (logs should be sufficient for that), and finally switch over to doing only this new check going forward.
What are your thoughts on this?
Comment on lines
+52
to
+64
| /// Mode controlling whether the order creation simulation can reject orders. | ||
| /// The operational default lives in `configs::orderbook::OrderSimulationMode` | ||
| /// (currently `Disabled`). This enum only represents the on-path states | ||
| /// `OrderValidator` actually executes. | ||
| #[derive(Copy, Clone, Debug, Eq, PartialEq)] | ||
| pub enum OrderSimulationMode { | ||
| /// Log disagreements, emit metrics. Never reject. | ||
| Shadow, | ||
| /// If the signature check passes but the order simulation fails, reject | ||
| /// the order with `ValidationError::SimulationFailed`. Infra errors | ||
| /// still never reject (fail-open). | ||
| Enforce, | ||
| } |
Contributor
There was a problem hiding this comment.
I wonder if the logic of configuring shadow vs enforce makes sense.
For now we only need the shadow mode and once that works well it's supposed to become the default. So when we switch it over we don't need to distinguish anymore, right?
So in order to keep things simple I think we can drop the configuration part, always do the shadow mode, and later follow up with another PR implementing only the enforced mode.
Contributor
Author
There was a problem hiding this comment.
I was mostly worried about some corner cases that might come up rarely. But agree, we should probably simplify the config. Will do that.
Contributor
Author
There was a problem hiding this comment.
Updated the code.
Comment on lines
+548
to
+550
| /// Computes the `verification_gas_limit` for an order. Returns `0` for | ||
| /// non-EIP-1271 signatures, otherwise delegates to `run_eip1271_checks`. | ||
| async fn calculate_verification_gas_limit( |
Contributor
There was a problem hiding this comment.
Not directly addressable in this PR but food for thought. We have logic handling extra gas from signature checks and hooks to account for the fact that quotes are actually really unique if you make use of many features.
I wonder if instead of having all this additional accounting logic we should just generate random quote ids and assume if a user knows the quote ID they are the original receiver of the quote and that the gas cost matches.
Ultimately since we no longer have to agree on a fee upfront there is no risk in us losing money on pricing that incorrectly which I suspect was the reason this logic was introduced in the first place. 🤔
Replays a mainnet order that the driver dropped ~295 times on
2026-05-05 with Simulation(Revert("execution reverted")), pinned
to block 25_028_258. Asserts the simulator reproduces the on-chain
revert at the pre-hook step where the protocol adapter tries to
move the trader's aWBTC balance.
Drops the disabled / shadow / enforce mode enum and the per-outcome metric matrix. Order simulation now runs alongside the signature check purely for observability. Disagreements are logged. The signature check alone decides whether the order is accepted. Enforce mode (rejecting orders whose simulation reverts) will land in a follow-up PR. The negative e2e test that asserted HTTP 400 OrderSimulationFailed comes back with it.
The enum was a near-1:1 copy of the underlying Result with the timeout case folded in. Returning the Result directly drops the enum and the single-caller run_order_simulation_only helper.
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
Problem
Same as #4355: the orderbook accepts an EIP-1271 order if the signer contract's
isValidSignaturesays yes. That single check lets through Aave flashloan-style orders where the signature passes but the post-hook can never settle. We catch these later, after they're in an auction, wasting solver cycles.Fix
Successor to #4355, rebuilt on top of
new-api-simulator-crate(@MartinquaXD'sSettlementSimulatorrefactor). At order-creation time, run the signature check and a full-order simulation concurrently. The signature result alone decides whether the order is accepted (current behaviour). The simulation result is logged. Disagreements (signature pass + simulation revert, or vice versa) surface as warnings.This is shadow-mode only. Once we trust the sim against real traffic, an enforce-mode follow-up turns the simulation result into rejections.
Infra errors (RPC, timeout, Tenderly) are logged and ignored.
What changed vs #4355
SettlementSimulator::new_simulation_builder()instead of the legacyOrderSimulator.parameters_from_app_datahandles pre/post hooks, custom wrappers, and Aave flashloan configs uniformly from the user-signed app_data. The earlier prototype injected these manually.WrapperConfig::Flashloan(...)) routes through the deployedFlashLoanRouter, so the validation-time simulation reflects the real Aave flashloan flow end-to-end. The borrower-to-trader transfer concern raised in the Simulate EIP-1271 orders at creation #4355 review is handled by the user-signed pre-hook (deploy helper, fund from factory), whichparameters_from_app_dataincludes automatically.orderbook::runpasses theFlashLoanRouterdeployment address toSettlementSimulator::newinstead ofAddress::ZERO, which would otherwise silently no-op the wrapper call.Changes
shared::order_validation: newOrderSimulatingtrait,OrderSimulator { simulator, timeout }threaded intoOrderValidator. Validation runs signature + sim concurrently viatokio::join!with a per-call timeout. Disagreements are logged.orderbook::order_simulation:OrderSimulatorAdapterdrivingSettlementSimulator::new_simulation_builder()+parameters_from_app_data.orderbook::run: wire the deployedFlashLoanRouteraddress.How to test
Unit tests in
shared::order_validationcover the signature × simulation matrix in shadow mode, fail-open on infra errors, theeip1271_skip_creation_validationpath, and the no-simulator-configured path.Local-node e2e in
crates/e2e/tests/e2e/eip1271_creation_simulation.rs: a Safe-signed order with empty app_data is accepted, proving the simulation runs without disrupting the happy path. The negative-rejection test lands with the enforce-mode follow-up PR.Forked-mainnet replays in
crates/simulator/tests/aave_replay.rs(gated onMAINNET_RPC_URL, otherwise skipped). A full e2e against a forked node isn't viable:OrderValidator::partial_validatechecksvalid_toagainstSystemTime::now(), and any historical order'svalid_tois in the past on a fresh test run, so the orderbook rejects withInsufficientValidTobefore the simulation runs. These tests therefore bypass the validator and driveSettlementSimulatordirectly.flashloan.amountrewritten past Aave's WETH liquidity. Asserts the simulation reverts withexecution reverted. The eth_call goes toFlashLoanRouter, which forwards toAaveBorrower, which asks the Aave Pool for the loan. Aave is what reverts (insufficient liquidity), but the revert only reaches us because the router and borrower addresses are correctly wired. If the wrapper had no-op'd againstAddress::ZEROthe simulation would have succeeded silently.Simulation(Revert(_)). Asserts the simulator reproduces the EVM revert. Real-order counterpart to the synthetic flashloan-oversubscribed test.