-
Notifications
You must be signed in to change notification settings - Fork 154
Use flashloan hints to decide when to skip checks #4016
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
MartinquaXD
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.
Rather than continuing to skip the checks on flashloan orders we can now use the parsed data to "fake" the execution of the flashloan in the simulation using state overrides like the driver does it.
Although ideally we'd move all that logic into a separate crate to unify it.
| return false; | ||
| }; | ||
|
|
||
| match app_data::parse(full_app_data.as_bytes()) { |
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 has the potential to add quite a bit of latency since we are parsing appdatas for thousands of orders during every auction. The driver gets around this with a caching solution. We should consider making that logic reusable.
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 currently only runs for Eip1271 orders, there aren't that many of these. It's a potential footgun should someone remove that condition in the future, so a cache should be there anyway. 👌
I thought the endgame was to not do any filtering here and let the driver do it. Do you think about it differently now? What do we do with this PR? I think this PR is a net benefit, but it's admittedly a small improvement, so one might consider it not worth merging. Using simulations and/or refactoring all that into a separate crate is something we should plan, though, it's not a quick in and out adventure like this one. |
d18184c to
a30cbbc
Compare
Isn't the whole PR fundamentally at odds with that idea? Either we don't want filtering then it doesn't matter if it's a flashloan order or not. Or we want filtering but then we might as well do it the best we can. Maybe it makes sense to spell out exactly what problem this PR is trying to solve. Is there someone complaining that we currently don't filter out any EIP 1271 orders? |
This PR is attempting to bring a small improvement to a situation in which we are right now. One does not simply turn off all filtering at once as we learned the last time we tried. It seems to me that as long as we accept the premise that orders should be filtered in the autopilot it's better to do the filtering more selectively using the flash loan hints than just letting all 1271s through.
I think small, iterative improvements are worth doing. I cooked this one up quickly because it was on my list since we introduced this whole 1271 exception. It should have been there from the get-go, but it was added as a "urgent" hotfix. Nobody is complaining about the current state of affairs, but I think it's likely that "doing the best we can" means pushing that work to Q5 and having no improvement, however small, at all. |
I'm confused. We are currently not doing any checks for the 1271 orders. So unless someone is complaining about the missing filtering I don't understand why we would move away from the "no filtering" approach in the autopilot. |
The idea is that it's not doing these checks, but it should. This PR then makes it skip only when it really needs to, meaning it performs more checks and lets fewer orders get to the driver. It's operating in the current paradigm, not in the one where we nuke all autopilot filtering. |
But why should it? I would understand it if there are some specific errors coming from the fact that we removed all 1271 filtering but if we already have this disabled completely (which is what we want long term) and seemingly nobody is complaining. So unless I'm missing something making it so that we can re-enable signature checks for some orders this is strictly a step back from what we want to achieve without any upsides (i.e. no reduction in people complaining). |
Ah, I finally get you. You are saying we never actually needed this filtering in the first place, so there is no need to make it better. That makes sense to me. |
Description
Currently we let all 1271 orders go through without balance and signature checks when
disable_1271_order_{sig,balance}_filterflags are on (which they now are in prod). This change remove these flags and only skips these checks for 1271 orders that have a flashloan hint, which means we won't skipping fewer checks. To replace the two flags this PR addsdisable-flashloan-hint-filter-bypassflag that control both signature and balance checks.Changes
How to test
Unit tests.