Fixes for new simulation check#4539
Conversation
|
Claude finished @MartinquaXD's task in 3m 19s —— View job PR Review
Reviewed against One issue worth changing (left as an inline comment):
|
There was a problem hiding this comment.
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.
| let inputs = self | ||
| .prepare_simulation(order, full_app_data, full_balance_check) | ||
| .await?; | ||
| analyze_simulation(order, inputs).await |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
| .and_then(|val| val.checked_div(order.data.sell_amount)) | ||
| .unwrap_or(U256::ONE), |
There was a problem hiding this comment.
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?
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
stETHorxStockstokens as those are relatively popular and are currently not supported by our balance override code.clamp(1 wei, sellToken.balanceOf(trader), full order amount).full_balance_checkinto 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.