Skip to content
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

Fix bug when introducing prices for liquidity orders #848

Closed
harisang opened this issue Nov 24, 2022 · 4 comments · Fixed by #878
Closed

Fix bug when introducing prices for liquidity orders #848

harisang opened this issue Nov 24, 2022 · 4 comments · Fixed by #878
Assignees
Labels
bug Something isn't working

Comments

@harisang
Copy link
Contributor

In order for the smart contract to execute an order, it requires prices for both the sell and the buy token. This applies to liquidity orders as well.

It was observed that due to this part of the code (thanks to @MartinquaXD for identifying the line with the bug)
https://github.com/cowprotocol/services/blob/main/crates/solver/src/settlement/settlement_encoder.rs#L392-L398

the driver sometimes introduces a price for the sell token of a liquidity order that is equal to the buy amount, even if there is already a price computed for the buy token. This leads to a very wrong clearing price vector.

@harisang harisang added the bug Something isn't working label Nov 24, 2022
@MartinquaXD
Copy link
Contributor

A bit more context can be found in the slack thread. It also contains logs with concrete examples for the bug.

@MartinquaXD
Copy link
Contributor

MartinquaXD commented Nov 24, 2022

Linking #849 as it appears to have the same underlying cause (making liquidity order prices part of SettlementEncoder::clearin_prices). If we manage to handle liquidity order prices in the SettlementEncoder without adding them to SettlementEncoder::clearing_prices both these issues should be resolved. Of course all those prices still have to end up in the final EncodedSettlement generated by SettlementEncoder::finish().

@harisang
Copy link
Contributor Author

Added the external solver label since at least one solver has been suffering from this bug.

@harisang
Copy link
Contributor Author

Adding here also that the correct solution should be to recompute from scratch new prices specific to each liquidity order, for both the sell and the buy token. This is because it has been observed that solvers that might be compressing their prices a bit to save on gas have had issues with the execution of liquidity orders, as rounding problems show up that can distort the amounts traded by the liquidity order.

One extreme example is the following, where this was no fault of the solver.
https://etherscan.io/tx/0xcfe7577b60e475d99ba8bc1436fc6f701164dac52e977ef746d8d510f3cf8cea

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants