(fast3) Gate placement of fast-path orders on config - #4858
Conversation
|
Claude finished @MartinquaXD's task in 1m 42s —— View job Review completeI reviewed the fast-path config gating changes. The mechanism (config gate → reject when unset, otherwise compute
The rest (config plumbing, error mapping in |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| app_data | ||
| .inner | ||
| .protocol | ||
| .valid_from | ||
| .or_else(|| Some(time::now_in_epoch_seconds() + exclusivity.as_secs() as u32)) |
There was a problem hiding this comment.
The code diverges from the documented behavior. The config doc says the exclusivity is applied "unless the user set a later one", and the field is named min_fast_path_exclusivity — both imply a floor. But .or_else(...) only fills in a value when the user provided none: if a user sets a validFrom earlier than now + exclusivity (or in the past), it is preserved verbatim, shrinking or eliminating the exclusivity window and bypassing the intended minimum.
If the floor is intended, clamp to the later of the two:
| app_data | |
| .inner | |
| .protocol | |
| .valid_from | |
| .or_else(|| Some(time::now_in_epoch_seconds() + exclusivity.as_secs() as u32)) | |
| let min_valid_from = | |
| time::now_in_epoch_seconds() + exclusivity.as_secs() as u32; | |
| Some( | |
| app_data | |
| .inner | |
| .protocol | |
| .valid_from | |
| .map_or(min_valid_from, |user| user.max(min_valid_from)), | |
| ) |
The existing tests wouldn't catch this — the user-validFrom test only exercises a value later than the floor (now + 100 vs now + 30). Worth adding a case with an earlier validFrom to pin down the intended behavior either way.
There was a problem hiding this comment.
Seems like a good catch, otherwise we should call it fast_path_default_exclusivity.
| let valid_from = if app_data.inner.protocol.enable_fast_path { | ||
| let Some(exclusivity) = self.min_fast_path_exclusivity else { | ||
| return Err(ValidationError::FastPathDisabled); | ||
| }; |
There was a problem hiding this comment.
Minor: enable_fast_path is known from app-data very early (the previous code rejected it during app-data validation). Now the FastPathDisabled rejection runs at the very end of validate_and_construct_order, i.e. after quote fetching, balance checks and signature validation. For an order that can never succeed because the feature is off, that's wasted work and a slower error response. Consider gating earlier (where the old enable_fast_path rejection lived) and only computing the effective valid_from here.
MartinquaXD
changed the title
Adds a `min_fast_path_exclusivity` orderbook config option and an `OrderValidator::with_min_fast_path_exclusivity` chainable setter. Orders whose app data has `metadata.enableFastPath = true` now: * fail validation with a new `FastPathDisabled` variant (HTTP 400) if the config knob is unset, * otherwise get `valid_from = now + exclusivity`, holding them out of the regular auction until the fast-path solver has had its exclusivity window. Also drops the leftover "'enableFastPath' is not yet supported" stubs from the quoter and order validator, since the flag is now handled. Signed-off-by: MartinquaXD <martin@cow.fi>
d8d7768 to
f5af2e9
Compare
| /// later one). Unset disables the fast path: orders requesting it are | ||
| /// rejected. | ||
| #[serde(with = "humantime_serde", default)] | ||
| pub min_fast_path_exclusivity: Option<Duration>, |
There was a problem hiding this comment.
Why the "minimum"? Can it be held out for longer? I assume it's because the solver can also settle it in the first block after the deadline. While technically correct, I wouldn't carry this detail in the name and just call it fast_path_default_exclusivity.
| config.order_validation.max_gas_per_order, | ||
| config.order_validation.same_tokens_policy, | ||
| ) | ||
| .with_min_fast_path_exclusivity(config.order_validation.min_fast_path_exclusivity), |
There was a problem hiding this comment.
Any reason we are using the builder pattern here now, if all other parameters are constructor args? Feels inconsistent.
| app_data | ||
| .inner | ||
| .protocol | ||
| .valid_from | ||
| .or_else(|| Some(time::now_in_epoch_seconds() + exclusivity.as_secs() as u32)) |
There was a problem hiding this comment.
Seems like a good catch, otherwise we should call it fast_path_default_exclusivity.
jmg-duarte
left a comment
jmg-duarte
left a comment
There was a problem hiding this comment.
Felix raised the points I'd raise too 😅
fleupold
left a comment
fleupold
left a comment
There was a problem hiding this comment.
Not sure if this is the right PR, but we should probably make sure the following edge cases are handled correctly:
- If a fast path order isn't able to find its quote, it should be rejected. I believe currently, we may fetch a new quote with
fastPath: falseand then try to hold solvers that didn't know accountable to that - Same with orders that changed the limit price compared to its quote. Currently I believe they just get re-classified as out of market, but in this case the winning solver will not be willing to settle them.
- PreSign and ERC1271 orders should not be able to use fast path since they are inserted before they are "ready" (so the fast path cannot succed)
3 participants
Description
So far all orders that have
enableFastPath: truein the appdata were rejected because the feature was disabled entirely.Changes
This PR adds an optional config parameter
min-fast-path-exclusivitywhich acts as a feature gate.Only if this value is configured will fast path orders be allowed.
How to test
extended / updated existing unit tests