Do not count "in-market" orders for the order limit #2433

[m-lord-renkse]

Description

Only check for order limits when the orders are class "Limit". I kept the legacy code/behavior, in order not to make a regression breaking change.

Changes

• Only do the order limit check if the order is class "Limit" and it is not "in-market"

How to test

1. Unit test
2. e2e test

Fixes #2433

[MartinquaXD]

There is already a SQL query that counts limit orders. Instead of fetching all this data and counting in the rust code you could have the in-market check already in SQL to only return a single number.

[m-lord-renkse]

@MartinquaXD how could we do that? we need to precisely operate the f64 so we can consider the limit order as "out of market" 🤔 do you mean by doing the operation directly in SQL?

see https://github.com/cowprotocol/services/pull/2456/files#diff-d22c8612ee861d55e838fdc5487a1c139da73b795295670acdcccea91f805011R386

[MartinquaXD]

Basically reimplement the logic of is_order_outside_market_price() inside count_limit_orders_by_owner(). Hopefully there is a way in PSQL to convert the types appropriately for the computation.

[m-lord-renkse]

@MartinquaXD In my opinion, doing arithmetical operations in PSQL query involving U256 and f64 is really not a good idea (unless it is plain sum, and even so), we are going to lose precision, and we are going to push CPU overhead to the database. What's more, I don't think we will save time, since database CPU is less powerful than the backend CPU, and the rust code will be definitely faster.

I understand your concern of potential long queries in case the user placed many orders, for that please, consider adding the internal class (as I proposed here #2456 (comment)), which will tackle down all the issues. We could also do the queries by pagination in case of long queries.

[MartinquaXD]

not a good idea (unless it is plain sum

Interesting. Why do you think so?

we are going to lose precision

This is also the case in the rust code since.fee()calls U256::from_f64_lossy().

I don't think we will save time, since database CPU is less powerful than the backend CPU, and the rust code will be definitely faster.

Computations might be faster in rust but I very much doubt that returning all that data, parsing it, allocating the structs (1 Vec per BigDecimal) and doing the computation in rust will be faster than doing a computation inside the db and returning a single number.

This is not a hill I'm willing to die on (although I disagree) if you feel strongly about keeping the computation out of the DB I'd at least consider fetching the data using a stream instead of call .fetch_all() to limit the memory consumption when a user has lots of orders.

[fleupold]

I guess we would need to benchmark this in order to get a definitive answer. It's true that for accounts with a lot of orders this may become a bottleneck. In theory we could also decide the "in-marketness" of an order when it's inserted into the db (and then simply count based on a flag)?

I'd be curious how PSQL math is less accurate than rust math?

[fleupold]

Looks good in general to me. Not too sure how much work it would be to decide the in-marketness at insertion time (we can run a DB migration to update historic orders).

[fleupold]

I guess we would need to benchmark this in order to get a definitive answer. It's true that for accounts with a lot of orders this may become a bottleneck. In theory we could also decide the "in-marketness" of an order when it's inserted into the db (and then simply count based on a flag)?

I'd be curious how PSQL math is less accurate than rust math?

[m-lord-renkse]

I'd be curious how PSQL math is less accurate than rust math?

@fleupold @MartinquaXD implementation of the same standard IEEE 754 does not guarantee exactly same behavior. For example, the rounding is the same, but when the rounding is done, may differ. The standard doesn't define many corner cases which makes each implementation slightly different. Also the compilation and optimizations impacts it a lot, actually same code of Rust can vary f64 operations depending on the architecture and on the optimization selected to compile the code.

Interesting articles (I actually didn't know many things of those articles, it is quite interesting):

• https://copyprogramming.com/howto/floating-point-accuracy-with-different-languages
• https://grouper.ieee.org/groups/msc/ANSI_IEEE-Std-754-2019/background/addendum.html

That's why I do not like splitting arithmetic business logic between PSQL and Rust. Well, that's also why I don't like f64 at all, in detriment of https://docs.rs/crate/rust_decimal/latest (I may expand more about this topic in Slack). We are losing precision in the fees because of the f64 operations in Rust (but I don't know how important it is, or the impact it really has).

[MartinquaXD]

Approving since we discussed via DMs that the next task will be to refactor how we store order classes (while getting rid of liquidity orders) which will make the whole discussion obsolete.

[m-lord-renkse]

@fleupold @MartinquaXD the issue to fully fix it and eliminate the order class, as agreed: #2469