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

Verify quote with pre interactions #3160

Merged
merged 9 commits into from
Dec 13, 2024

Conversation

MartinquaXD
Copy link
Contributor

@MartinquaXD MartinquaXD commented Dec 11, 2024

Description

Not too long ago #3081 changed the simulation logic to not do any set up steps on behalf of the user if the quote provided some pre-interactions. That was done to avoid reverts when a pre-interactions actually set up everything for the trade (e.g. get balances by unstaking).
However, this caused issues with verifying quotes with permit pre-interactions. Because the quote provided the permit pre-interaction the simulation logic also did not try to fake balances although it could have.

Changes

Now we again always call the code to setup approvals and such BUT we do it as the very last pre-interaction. That way this operation becomes a no-op if the user already sets up everything with their pre-interactions OR does the usual logic and reverts with helpful error messages if the pre-conditions can't be met.
Since this function may now be a no-op I renamed it to ensureSwapPreconditions().
Additionally this moves code that was previously run before the settlement inside the settlement which increases the measured gas cost. To work around that I also introduced a wrapper function in Solver.sol that simply calls the Trader function and discounts the gas costs. Note that we can't simply do the necessary steps in the Solver because we need to be inside the context of the Trader to be able to set approvals and such.

That way we can verify quotes which do no, partial or the entire swap setup in their pre-interactions while keeping good error messages and accurate gas estimates.
Kudos to @nlordell for the idea.

How to test

I added an e2e test that fails without the PR.

@MartinquaXD MartinquaXD requested a review from a team as a code owner December 11, 2024 11:59
@nlordell
Copy link
Contributor

However, this caused issues with verifying quotes with permit pre-interactions. Because the quote provided the permit pre-interaction the simulation logic also did not try to fake balances although it could have.

Does this mean that the user is signing a permit allowance without actually having balances? Feels weird to request a quote with a legitimate allowance signature if you don't intend to trade.

@MartinquaXD
Copy link
Contributor Author

Does this mean that the user is signing a permit allowance without actually having balances? Feels weird to request a quote with a legitimate allowance signature if you don't intend to trade.

Yeah, fair point. OTOH we started to support the use case where users can open orders for which they currently don't have the balance. Maybe that's an issue where @alfetopito could chime in but overall the proposed solution feels like the lesser of 2 evils.

Copy link
Contributor

@squadgazzz squadgazzz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I spent more time than anticipated to find potential pitfalls or other workarounds, but actually couldn't come up with anything. The mentioned tradeoffs make sense.

# Description
The naive solver is extremely basic and never wins any auctions anymore.
Also AFAICT it's no longer used in e2e tests either so by now it's just
dead weight.
Sparked by this
[thread](https://cowservices.slack.com/archives/C036JAGRQ04/p1733911881085109)

# Changes
Removes:
* original algorithm in legacy code
* wrapper logic in `solvers` crate
* config code
* test cases
* documentation

Also in many cases we had an enum with 2 variants (`baseline` and
`naive`). Since we now only have 1 variant left I collapsed all the
logic and got rid of a few modules which are no longer necessary.
@alfetopito
Copy link
Contributor

Does this mean that the user is signing a permit allowance without actually having balances? Feels weird to request a quote with a legitimate allowance signature if you don't intend to trade.

Yeah, fair point. OTOH we started to support the use case where users can open orders for which they currently don't have the balance. Maybe that's an issue where @alfetopito could chime in but overall the proposed solution feels like the lesser of 2 evils.

if you don't intend to trade.
It's not that a user does not intend to trade or don't have the balance (well, they don't, that's true).
The goal is to allow the balance to be funded from a pre-hook and used in the order.
Quick example: Claim an airdrop/exit a pool then use whatever is coming as a sell token.

You can already do that in the UI by turning on the Hooks tab, currently hidden behind a setting:

image

image

Copy link
Contributor

@squadgazzz squadgazzz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool, the updated version seems much better 🙌

crates/shared/src/price_estimation/trade_verifier.rs Outdated Show resolved Hide resolved
crates/contracts/solidity/Trader.sol Show resolved Hide resolved
@MartinquaXD MartinquaXD enabled auto-merge (squash) December 13, 2024 12:35
@MartinquaXD MartinquaXD disabled auto-merge December 13, 2024 12:39
@MartinquaXD MartinquaXD merged commit a520f40 into main Dec 13, 2024
11 checks passed
@MartinquaXD MartinquaXD deleted the verify-quote-with-pre-interactions branch December 13, 2024 12:52
@github-actions github-actions bot locked and limited conversation to collaborators Dec 13, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants