Skip to content

Fixes for new simulation check#4539

Open
MartinquaXD wants to merge 4 commits into
integrate-simulation-report-into-simulatorfrom
order-validation-simulation-fixes
Open

Fixes for new simulation check#4539
MartinquaXD wants to merge 4 commits into
integrate-simulation-report-into-simulatorfrom
order-validation-simulation-fixes

Conversation

@MartinquaXD

Copy link
Copy Markdown
Contributor

Description

Follow up to #4538. This PR finally makes use of the features introduces in the previous 2 PRs to address the limitations of the simulation approach. It also addresses 2 other smaller issues with the current code.

Changes

  1. We now use the state override errors and the simulation report to infer whether our state override errors caused the simulation to fail. If everything worked fine until the simulation hits the buy token transfer AND we previously failed to compute the necessary state overrides we now err on the side of allowing the order to be placed. The most relevant examples of this would be orders buying stETH or xStocks tokens as those are relatively popular and are currently not supported by our balance override code.
  2. Most of the simulation disagreements I saw were caused by the new simulation filling the order more than possible. The currently active logic tries to send only 1, 10, and 100 wei of the sell token into the settlement contract (for partially fillable orders). To allow the same partially funded orders to be placed via the new logic we now chose the fill amount as clamp(1 wei, sellToken.balanceOf(trader), full order amount).
  3. Lastly we now also pipe the full_balance_check into the simulation setup which the user can opt-in to when they only want fully funded orders to be placed.

Additionally this PR also starts logging the order simulation report to help us debug the remaining cases of disagreements faster.

How to test

Added a unit test for detecting and handling those edge cases of our simulation.

@MartinquaXD MartinquaXD requested a review from a team as a code owner June 19, 2026 13:32
@claude

claude Bot commented Jun 19, 2026

Copy link
Copy Markdown

Claude finished @MartinquaXD's task in 3m 19s —— View job


PR Review

  • Read the diff and changed files
  • Understand the simulation/state-override logic
  • Review for correctness, edge cases, style
  • Post inline comments for issues
  • Post review summary

Reviewed against origin/integrate-simulation-report-into-simulator. The state-override-aware revert classification (extract_critical_revert + the disallow_broken_hook / allow_failed_buy_token_transfer / allow_unfunded_presign_order helpers) reads cleanly and is well-tested, and piping full_balance_check through is correct. The zero-amount guard in order_validation.rs (line 782) runs before the simulator, so the clamp(U256::ONE, executable_amount) can't panic on a zero upper bound. 👍

One issue worth changing (left as an inline comment):

  • Buy-order unit mismatch in the partial-fill amount (order_creation_simulation.rs:103-108). sell_token_balance is in sell-token units, but for buy orders both the clamp bound and the resulting ExecutionAmount::Explicit are interpreted in buy-token units. This can reject a partially-fundable buy order outright (when the sell balance numerically exceeds buy_amount, the upper bound forces a full fill that the balance can't cover) or fill a meaningless dust amount. Suggested a per-kind conversion in the inline comment. This path also isn't covered by the new unit tests.
    · branch order-validation-simulation-fixes

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request enhances order creation simulation by introducing partial balance checks and filtering out non-critical reverts (such as failed buy token transfers when state overrides fail, or unfunded pre-sign orders) to prevent false positives. The reviewer identified two critical issues in the new logic: first, a unit mismatch in prepare_simulation where the sell token balance is clamped against the buy token amount for buy orders; second, an unhandled edge case in allow_failed_buy_token_transfer where a zero-address receiver is not correctly resolved to the order owner, causing transfer checks to fail.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment thread crates/shared/src/order_creation_simulation.rs Outdated
Comment thread crates/shared/src/order_creation_simulation.rs
Comment thread crates/shared/src/order_creation_simulation.rs Outdated
Comment on lines +80 to +83
let inputs = self
.prepare_simulation(order, full_app_data, full_balance_check)
.await?;
analyze_simulation(order, inputs).await

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This now uses debug_trace_call for every order instead of eth_call, right? Should we keep simulate() as the fast path and call simulation_report() only when it reverts?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was on the fence about this one. Probably best to assume that the majority of orders are well behaved so falling back to the new logic only for reverts is not that costly.

Comment on lines +110 to +111
.and_then(|val| val.checked_div(order.data.sell_amount))
.unwrap_or(U256::ONE),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unwrap_or(U256::ONE) only fires on None, which here means overflow in checked_mul or sell_amount == 0 in checked_div. A small numerator is still a valid Some(0), since integer division floors, so it slips past unwrap_orand we getExecutionAmount::Explicit(0)`.

e.g. Example: clamped sell 1, buy 100, sell 1000 gives 100 / 1000 = Some(0). executed_amount passes that through, so an underfunded partially-fillable buy order fills 0, the transfers are 0 and pass, and it clears the check without a real transfer. A .max(U256::ONE) on the final amount would keep the minimal fill. Does it make sense?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants