Remove Custom Price Order Sell Price from UCP #878
Conversation
There was a problem hiding this comment.
This patch looks correct to me. After thinking about it a bit more I believe initially only 1 token price was stored directly in the custom price trades as a gas optimization but I believe in this case it's better to have a sane implementation than saving tiny amounts of gas. Managing the Settlement and its encoding is already difficult enough. 👍
Yes. THe issue is we found that for solvers that would compress prices (reduce number of bits to represent prices for gas savings) we would get huge errors on what we wanted the price to actually be. Agree that this is the correct fix. In theory, we could do something complicated like bitshift prices so that there are no leading 0's when multiplied with the highest amounts for maximum precision, but as you said, settlement encoding is already complicated enough. |
| let sell_token = order.data.sell_token; | ||
| let sell_price = order.data.buy_amount; | ||
| self.tokens.push(sell_token); | ||
| self.clearing_prices.insert(sell_token, sell_price); |
There was a problem hiding this comment.
For context the assumption is that this line added the buy_amount of a foreign liquidity order contained in a solution to the clearing price vector of the Settlement if that order's sell_token was not already part of the clearing prices. Because this price is not denominated in ETH and actually only makes sense in relation to the custom price associated to that order it can make the Settlement fail the price deviation check.
|
I've updated the unit tests - moving to |
| Ok(()) | ||
| }) | ||
| .collect::<Result<Vec<()>>>()?; | ||
|
|
There was a problem hiding this comment.
I think there is no need to scale custom price orders anymore, since they are now completely decoupled from UCP.
| let adjusted_sell_price = buy_amount; | ||
| let adjusted_buy_price = sell_amount; |
There was a problem hiding this comment.
This always confuses me 🤣 - but I'm 99% sure its correct (you can never be 100% when it comes to prices and exchange rates...)
It fixes it because this bug was caused by foreign liquidity orders adding Anyhow I will skip fixing #849 with this PR until I have a proof for it. |
|
Great, thanks for taking care of this! As far as I understand, the prices determining the execution of a liquidity order are no longer stored in the UCP and, and if this means that these prices are not checked for max price deviation, then we are good to go with #849 as well. |
Fixes #848 and potentially #849
Custom price orders (liquidity orders and limit orders) now store both
sell_priceandbuy_priceoutside of the clearing price vector (as suggested by @MartinquaXD, thanks for clarifying the issue).