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

Check if limit price is respected #88

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

m-lord-renkse
Copy link
Collaborator

Enforce the limit price required in the settlement contract: https://github.com/cowprotocol/contracts/blob/main/src/contracts/GPv2Settlement.sol#L368-L371

@m-lord-renkse m-lord-renkse force-pushed the check-limit-price-respected branch from b46292d to c1098e6 Compare November 22, 2024 13:49
@m-lord-renkse m-lord-renkse force-pushed the check-limit-price-respected branch from c1098e6 to 2a60553 Compare November 22, 2024 13:51
@m-lord-renkse m-lord-renkse force-pushed the check-limit-price-respected branch from 2a60553 to 8fdd0af Compare November 22, 2024 13:56
@fleupold
Copy link
Contributor

I'm quite surprised this PR is needed (checking the limit price sounds like a very basic thing the solver code base should already be doing).

Can you please add more context & explanation on why this is needed? From quickly searching the code I found at least these two places, which already check the limit price, so I'd like to be able to read from the PR description why those aren't sufficient:

  1. let valid = swap.satisfies(order);
  2. if order.sell.amount.checked_mul(buy)? < order.buy.amount.checked_mul(sell)? {

@squadgazzz
Copy link
Contributor

I would really appreciate more context for this PR. It is not clear why this change is required due to the absence of tests. Does the driver do the same validation?

@m-lord-renkse
Copy link
Collaborator Author

@fleupold @squadgazzz we are getting simulations failing here: https://github.com/cowprotocol/contracts/blob/main/src/contracts/GPv2Settlement.sol#L368-L371 for these solvers. Which seems to indicate we aren't doing this check properly.

  1. if order.sell.amount.checked_mul(buy)? < order.buy.amount.checked_mul(sell)? {

I somehow didn't see this. Maybe this math is not 100 % precise and we have some cases which triggers this error in the settlement contract.

@m-lord-renkse m-lord-renkse marked this pull request as draft November 22, 2024 16:17
@m-lord-renkse
Copy link
Collaborator Author

m-lord-renkse commented Nov 22, 2024

Does the driver do the same validation?

Exactly the same.

@squadgazzz
Copy link
Contributor

Ok, so if I got it correctly, this is required to reduce the driver's error rate and, instead, just use warnings on the solver's side, right? But does it make sense to keep everything as it is so we can still have signals that there is something wrong with solutions?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants