Separate insufficient balance by balance and allowance #3017
Conversation
|
|
||
| /// Removes orders that can't possibly be settled because the allowance is not | ||
| /// enough | ||
| fn orders_with_allowance(mut orders: Vec<Order>, balances: &Balances) -> Vec<Order> { |
There was a problem hiding this comment.
These functions are on the hot path and are now iterating over the majority of orders twice and doing a hashmap lookup for each one when it could happen in one sweep.
This also results in an almost exactly duplicated function.
Did you already check how you could avoid that?
I believe you could pass in the OrderFilterCounter and use checkpoint_by_invalid_orders() inside by collecting missing balances uids and missing allowance uids into 2 separate vectors.
There was a problem hiding this comment.
@MartinquaXD Iterating over orders to match a field should be relatively fast, and without much overhead. I agree it is sub-optimal. I will give it a thought and make it with one iteration. But being that the case, we could also try to make all the checks within one iteration though 🤔 I will draft a proposal. I'll think about it.
There was a problem hiding this comment.
My point is, that we already iterate multiple times over all the orders, and it is more of a general issue rather than an issue of this PR (which is not helping it, since it is adding one iteration more...). But I 100 % agree with you, and this is a good opportunity to come up with something to mitigate it.
There was a problem hiding this comment.
@MartinquaXD I wrote a draft proposal (tests are failing on purpose, I would need to clean it up a bit). If you like it/agree, after cleaning it up, rework comments and merged. I can do another PR to apply this to the rest of the filtering, avoiding doing many iterations over the orders, and scratching some time that hopefully one day will come in handy.
m-lord-renkse
force-pushed
the
2886/filter-order-by-allowance
branch
from
218468c to
a540000
Compare
| return true; | ||
| // The order of checks impacts the order of what we want to filter first / with | ||
| // more priority | ||
| if invalid_balance(order, balances) { |
There was a problem hiding this comment.
We can add here everything which is pre-"price fetching", even the checks which require async functions (like signature check). Eventually this can be an async iterator in which we wait concurrently for the results. Instead of doing each check individually by each check iterating over the orders, we could iterate once and do all the check per order.
By doing it like this, we don't only avoid doing all the iterations for each check, but also all the iterations we do for the counter.checkpoint().
Ultimately we can have another check for the post-"price fetched" checks.
| } | ||
|
|
||
| /// Returns true if the allowance is invalid for the given order | ||
| fn invalid_allowance(order: &Order, balances: &Balances) -> bool { |
There was a problem hiding this comment.
we could have all these functions in a different file for better grouping
| }; | ||
| balance >= needed_balance | ||
| if invalid_allowance(order, balances) { | ||
| filtered_orders.push(("insufficient_allowance", order.metadata.uid)); |
There was a problem hiding this comment.
we can delegate this action to the functions (invalid_allowance in this case), for a cleaner function body here.
| @@ -837,6 +874,39 @@ impl OrderFilterCounter { | |||
| filtered_orders.into_keys().collect() | |||
| } | |||
|
|
|||
| /// Creates a new checkpoint based on the found multiply invalid orders. | |||
| fn checkpoint_by_multiple_invalid_orders( | |||
There was a problem hiding this comment.
With this function, we just need to iterate over the invalid orders once.
| let orders = orders_with_balance(orders, &balances); | ||
| let removed = counter.checkpoint("insufficient_balance", &orders); | ||
| invalid_order_uids.extend(removed); | ||
| let filtered_orders = filter_orders_with_balance(&mut orders, &balances); |
There was a problem hiding this comment.
changed the function signature to filter by taking a mutable reference, this way it can returned the filtered order. I am open to make it as before and then return both: filtered orders and orders.
|
Seems like we'll remove the balance filtering from the |
@MartinquaXD thank you for the detailed explanation. Closing this PR. I can revive part of the code if we plan to move it to the driver in the future. |
Description
At the moment we don't differentiate between missing balances and missing allowances and always report
insufficient_balance.Changes
Balancewhich contains thebalanceandallowanceHow to test
Related Issues
Fixes #2886