-
Notifications
You must be signed in to change notification settings - Fork 151
Make metrics based bad order detection order specific #4021
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
github unfortunately marks this whole file as new when it actually was actually just moved and modified slightly. The new parts are:
- added
last_seen_at - added
spawn_gc_task(and the associated unit test)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The refactoring/renaming should probably be separated from the logic changes to avoid this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This diff is much better https://www.diffchecker.com/iMNVcpf7/
jmg-duarte
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
| fn current_unix_timestamp() -> u64 { | ||
| SystemTime::now() | ||
| .duration_since(UNIX_EPOCH) | ||
| .unwrap() | ||
| .as_secs() | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We already have
services/crates/model/src/time.rs
Lines 8 to 15 in 06ae435
| pub fn now_in_epoch_seconds() -> u32 { | |
| SystemTime::now() | |
| .duration_since(SystemTime::UNIX_EPOCH) | |
| .expect("now earlier than epoch") | |
| .as_secs() | |
| .try_into() | |
| .expect("epoch seconds larger than max u32") | |
| } |
| pub struct BadOrderDetection { | ||
| /// Tokens that are explicitly allow- or deny-listed. | ||
| pub tokens_supported: HashMap<eth::TokenAddress, bad_tokens::Quality>, | ||
| pub tokens_supported: HashMap<eth::TokenAddress, bad_orders::Quality>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is unfortunate that bad_orders now relate to tokens.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we really need to move the simulation to bad_orders? It still works with tokens, doesn't it?
Description
Currently the bad token detection assumes that we are perfectly able to detect "broken" orders and only orders that trade specific tokens that a particular solver is not able to handle cause problems. However this assumption does not work well with the increasing complexity of new order types that can suddenly start failing for any number of reasons.
The most prominent recent example were flashloan orders where the EIP 1271 signature verified correctly but transferring the tokens into the settlement contract failed because the user's Aave debt position was not healthy enough.
Our current logic caused a lot of collateral damage because such orders could cause many reasonable tokens to be flagged as unsupported although the tokens themselves were perfectly fine and only that particular order was problematic.
Changes
To address this this PR change the metrics based detection mechanism to only flag on an order by order basis instead flagging all orders trading specific tokens. The change itself is relatively simple (collect metrics keyed by
Uidinstead of token`) but came with a few related changes:bad_token_detectionis now incorrect in most (but not all!) cases so many things were renamedHow to test
Uid