Partially filled orders support in simulation#4306
Partially filled orders support in simulation#4306m-sz wants to merge 4 commits intocustom-order-simulationfrom
Conversation
|
Reminder: Please consider backward compatibility when modifying the API specification.
Caused by: |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces an executed_amount parameter to the order simulation API, allowing users to override the on-chain fill state during debugging. The changes include updates to the OpenAPI specification, API handlers, and the core simulation logic to calculate remaining amounts based on this override. A high-severity issue was identified in the OpenAPI definition where the new parameter was incorrectly nested under an existing one, violating the specification.
m-sz
force-pushed
the
custom-order-simulation
branch
from
031704f to
b96c0ea
Compare
m-sz
force-pushed
the
partial-fill-order-simulation
branch
from
89ff12b to
71f5dc5
Compare
| let remaining = | ||
| remaining_amounts::Remaining::from_order(&remaining_order).with_context(|| { | ||
| format!( | ||
| "could not compute remaining amounts for order {}", | ||
| order.metadata.uid | ||
| ) | ||
| })?; | ||
| let remaining_sell = remaining | ||
| .remaining(order.data.sell_amount) | ||
| .context("overflow computing remaining sell amount")?; | ||
| let remaining_buy = remaining | ||
| .remaining(order.data.buy_amount) | ||
| .context("overflow computing remaining buy amount")?; |
There was a problem hiding this comment.
Would these errors lead to 500? Should it be a bad request error instead?
There was a problem hiding this comment.
I don't believe it should be a bad request since it comes from our internal services state. Let's keep it as is.
| description: > | ||
| Block number to simulate the order at. If not specified, the | ||
| simulation uses the latest block. | ||
| - in: query |
There was a problem hiding this comment.
(Spent some time with claude, brushing up on openAPI spec)
The CI is failing because required: false (L819 below) on the blockNumber property inside the SimulationRequest schema isn't valid OpenAPI 3.0.
In Schema Objects, required is only valid as an array on the parent object.
That one should probably fix it 👀
There was a problem hiding this comment.
Since you went through all that, you could use the suggest feature in the GitHub to provide a more concrete change
| description: > | ||
| Block number to simulate the order at. If not specified, the | ||
| simulation uses the latest block. | ||
| - in: query |
There was a problem hiding this comment.
Since you went through all that, you could use the suggest feature in the GitHub to provide a more concrete change
| /// Override for how much of the order has already been filled, expressed | ||
| /// in the order's fill token (sell token for sell orders, buy token for | ||
| /// buy orders). When absent, the current on-chain fill state from the | ||
| /// order metadata is used. | ||
| #[serde_as(as = "Option<HexOrDecimalU256>")] | ||
| pub executed_amount: Option<U256>, |
There was a problem hiding this comment.
I don't like this, are we adding all parameters in the query from here on? Feels like a slippery slope to me
| let executed_amount = executed_amount.unwrap_or_else(|| match order.data.kind { | ||
| OrderKind::Buy => big_uint_to_u256(&order.metadata.executed_buy_amount) | ||
| .unwrap_or(order.data.buy_amount), | ||
| OrderKind::Sell => order.metadata.executed_sell_amount_before_fees, | ||
| }); | ||
| let remaining_order = remaining_amounts::Order { | ||
| kind: order.data.kind, | ||
| buy_amount: order.data.buy_amount, | ||
| sell_amount: order.data.sell_amount, | ||
| fee_amount: order.data.fee_amount, | ||
| executed_amount, | ||
| partially_fillable: order.data.partially_fillable, | ||
| }; | ||
| let remaining = remaining_amounts::Remaining::from_order(&remaining_order) | ||
| .with_context(|| { | ||
| format!( | ||
| "could not compute remaining amounts for order {}", | ||
| order.metadata.uid | ||
| ) | ||
| }) | ||
| .map_err(Error::Other)?; | ||
| let remaining_sell = remaining | ||
| .remaining(order.data.sell_amount) | ||
| .context("overflow computing remaining sell amount") | ||
| .map_err(Error::Other)?; | ||
| let remaining_buy = remaining | ||
| .remaining(order.data.buy_amount) | ||
| .context("overflow computing remaining buy amount") | ||
| .map_err(Error::Other)?; |
There was a problem hiding this comment.
IMO this should be extracted into a separate function
| &self, | ||
| order: &Order, | ||
| wrappers: Vec<WrapperCall>, | ||
| executed_amount: Option<U256>, |
There was a problem hiding this comment.
I'm not sure if we should move this up and split the function in two: with and without executed amount — which would eventually be "with and without overrides"
I'd say that for now it can stay but I'm afraid it will gather more parameters if we're not careful
| let uid = services.create_order(&order).await.unwrap(); | ||
|
|
||
| // Transfer 1 WETH away so the trader now holds only 1 WETH. | ||
| let burn = Address::from([0x42u8; 20]); |
There was a problem hiding this comment.
| let burn = Address::from([0x42u8; 20]); | |
| let burn = Address::repeat_byte(0x42); |
| assert_eq!(response.status(), StatusCode::OK); | ||
| let result = response.json::<OrderSimulationResult>().await.unwrap(); | ||
| assert!( | ||
| result.error.is_some(), |
There was a problem hiding this comment.
Checking something about the error would be helpful to ensure this test breaks if some other error happens
| sell_amount_including_fee: BigDecimal::from(1_000_000_000_000_000_000u64), | ||
| buy_amount: BigDecimal::from(500_000_000_000_000_000u64), |
There was a problem hiding this comment.
Even if it's a bit round-about, I'd say you should use the 1u64.eth() (whatever is the right value here) into a BigDecimal to improve readability
5 participants
Description
The order simulation endpoint does not take into account partially filled orders and always simulates the full amount.
Changes
Adds an optional query parameter to the order simulation that overrides the executed amount to be used for sim. If not provided, the current executed amount is taken from the order's metadata.
How to test
E2E test covering the executed amount query parameter.