Simulate EIP-1271 orders at creation#4366
Open
squadgazzz wants to merge 67 commits intonew-api-simulator-cratefrom
Open
Simulate EIP-1271 orders at creation#4366squadgazzz wants to merge 67 commits intonew-api-simulator-cratefrom
squadgazzz wants to merge 67 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.
|
|
||
| #[test] | ||
| fn parses_simulation_mode_default() { | ||
| let toml = r#"gas-limit = "0x1000000""#; |
Contributor
There was a problem hiding this comment.
nit: Odd to use hex numbers here. All other instances of gas-limit configurations use decimal in the code base.
Contributor
There was a problem hiding this comment.
probably makes sense to rename to order_simulation as this handles every aspect of the order.
| .map_err(|err| Eip1271SimulationError::Infra(anyhow!(err).context("build")))?; | ||
|
|
||
| let report = inputs | ||
| .simulate_with_tenderly_report() |
Contributor
There was a problem hiding this comment.
I think running a full tenderly simulation for every placed order is too costly and slow.
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.
| } | ||
| } | ||
|
|
||
| fn build_preview_order_for_sim( |
Contributor
There was a problem hiding this comment.
This function doesn't seem to carry its weight.
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}
Comment on lines
+138
to
+143
| let mut orderbook_config = configs::orderbook::Configuration::test_default(); | ||
| orderbook_config | ||
| .order_simulation | ||
| .as_mut() | ||
| .expect("test_default enables order_simulation") | ||
| .eip1271_simulation_mode = Eip1271SimulationMode::Enforce; |
Contributor
There was a problem hiding this comment.
If OrderSimulationConfig: TestDefault you could use destructuring here
Configuration {
order_simulation: OrderSimulationConfig {
eip1271_simulation_mode: Eip1271SimulationMode::Enforce,
..TestDefault::test_default()
}
..TestDefault::test_default()
}
no mut, no expect
| "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
| } | ||
|
|
||
| #[tokio::test] | ||
| async fn no_simulator_configured_preserves_existing_behaviour() { |
Contributor
There was a problem hiding this comment.
"existing behaviour" is vague and the meaning can change overtime
| eip1271_skip_creation_validation, | ||
| Default::default(), | ||
| HooksTrampoline::Instance::new( | ||
| Address::from([0xcf; 20]), |
Contributor
There was a problem hiding this comment.
Suggested change
| Address::from([0xcf; 20]), | |
| Address::repeat_byte(0xcf), |
Comment on lines
+641
to
+648
| /// Two paths, depending on the `eip1271_skip_creation_validation` flag: | ||
| /// | ||
| /// - **Skipped**: the cheap `isValidSignature` check is bypassed by the | ||
| /// operator, and we return a `verification_gas_limit` of `0` (no gas was | ||
| /// spent on-chain verifying the signature). If the optional | ||
| /// `Eip1271Simulator` is configured, the full simulation still runs for | ||
| /// observability only and can never reject. | ||
| /// - **Not skipped**: delegates to `run_eip1271_with_signature_check`. |
Contributor
There was a problem hiding this comment.
Suggested change
| /// Two paths, depending on the `eip1271_skip_creation_validation` flag: | |
| /// | |
| /// - **Skipped**: the cheap `isValidSignature` check is bypassed by the | |
| /// operator, and we return a `verification_gas_limit` of `0` (no gas was | |
| /// spent on-chain verifying the signature). If the optional | |
| /// `Eip1271Simulator` is configured, the full simulation still runs for | |
| /// observability only and can never reject. | |
| /// - **Not skipped**: delegates to `run_eip1271_with_signature_check`. | |
| /// When the `eip1271_skip_creation_validation` flag is: | |
| /// | |
| /// - `true`: the cheap `isValidSignature` check is bypassed by the | |
| /// operator, and we return a `verification_gas_limit` of `0` (no gas was | |
| /// spent on-chain verifying the signature). If the optional | |
| /// `Eip1271Simulator` is configured, the full simulation still runs for | |
| /// observability only and can never reject. | |
| /// - `false`: delegates to `run_eip1271_with_signature_check`. |
Furthermore, turning the vars and functions into doclinks (or whatever is the name) would help doc navigation
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: orderbook accepts an EIP-1271 order if the signer contract's
isValidSignaturesays yes. That single check is enough to let 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.A symmetric class (Euler-style) is too strict on signatures but would actually settle fine. Those are rejected today.
Fix
This PR is the successor to #4355, rebuilt on top of
new-api-simulator-crate(@MartinquaXD'sSettlementSimulatorrefactor). At order-creation time, we run the signature check and a full-order simulation concurrently, then combine the results.Per chain, a mode config picks what to do with the combined result:
disabled(default): same as today. Signature check only.shadow: both run, disagreements land in metrics and logs, behaviour unchanged.enforce: if signature passes but simulation fails, reject with HTTP 400Eip1271SimulationFailed.Infra errors (RPC, timeout, Tenderly) never reject in any mode. Signature-fail cases return the same
InvalidEip1271Signatureas before, regardless of the simulation result.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 now 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.Rollout
Same as #4355. Land as
disabled. Flip toshadowin prod to see the matrix on real traffic. Flip toenforceonce we trust the sim.Changes
shared::order_validation: newEip1271Simulatingtrait,Eip1271Simulatorbundle (simulator + mode + timeout) threaded intoOrderValidator. Validation runs signature + sim concurrently viatokio::join!with a per-call timeout, emits aneip1271_simulation_total{signature, simulation}Prometheus matrix, upgrades enforce-mode disagreements toValidationError::SimulationFailed.orderbook::eip1271_simulation:OrderSimulatorAdapterdrivingSettlementSimulator::new_simulation_builder()+parameters_from_app_data.orderbook::run: wire the deployedFlashLoanRouteraddress.configs:Eip1271SimulationModeenum withdisabled,shadow,enforcevariants.How to test
Unit tests in
shared::order_validationcover the signature × simulation matrix, enforce-mode rejection, fail-open on infra errors, theeip1271_skip_creation_validationpath, and the no-simulator-configured path.configstests deserialize the three modes.Local-node e2e in
crates/e2e/tests/e2e/eip1271_creation_simulation.rs:app_data.protocol.wrapperspoints at a custom always-revert wrapper. WithEip1271SimulationMode::Enforce, the orderbook returns HTTP 400Eip1271SimulationFailed.Forked-mainnet replay in
crates/simulator/tests/aave_replay.rs(gated onMAINNET_RPC_URL, otherwise skipped).A full e2e test 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 ever 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.