Check banned users on order arrival - #4696
Conversation
|
Did this also have a measurable latency reduction when building auctions? Fetching banned users happens concurrently with other things so I might have 0 net impact if the other things we do concurrently always take longer than the ban check. |
This is part of the "update solvable orders" phase, sadly, it didn't have a measurable impact |
Keep in mind that real world improvements should be the measurement of success - especially for PRs introducing non-trivial changes. In this case I believe there is an argument to be made to add this component now because we already have a few additions to that in mind but without those additions I don't think this PR should be merged. |
0741b43 to
8619593
Compare
| tokio::spawn(async move { | ||
| while let Some(arrival) = arrivals.next().await { | ||
| let owner = match arrival { | ||
| Ok(order) => order.owner(), |
There was a problem hiding this comment.
This currently only looks at the order's owner but not the receiver. For the other 2 use cases (balance cache warming, order fast path) we both need to look up the order. So probably best to already fetch the order from the DB in the new order_notify.
There was a problem hiding this comment.
Pushing back against this while we don't actually have the other components in here.
I checked mainnet and in the last hour (at the time of writing), 1.4k orders went in with the owner being the same as the receiver, while only 68 went in with the receiver being different.
I also checked base, 401 owner=receiver, 17 owner!=receiver
| /// Wakes the run loop so the arriving order makes it into the next auction as | ||
| /// soon as possible. |
There was a problem hiding this comment.
The comment is not correct. This waker is supposed to help the runloop to only start when a new block or order appears.
| boundary::{self, SolvableOrders}, | ||
| domain::{self, auction::Price}, | ||
| infra::{self, banned}, | ||
| infra::{self}, |
There was a problem hiding this comment.
unnecessary change growing the diff
2 participants
Description
We're doing all the user ban checks when performing auction cutting, however, we can check them eagerly when the autopilot gets notified of new orders; this works because we can extract the owner from the order UID. As such we can check on order arrival leading to the cache being pre-warmed when the auction cutting happens.
Changes
How to test
Tested in staging to ensure nothing broke and in prod:
The overall latency was reduced as one can see based on less bucket variance, there are still some spikes as receivers are still searched later on, since the order UID doesn't carry info on them.
Heatmap panel: