Relax trade verification checks for pre-interactions #3081
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
squadgazzz
reviewed
Oct 24, 2024
Comment on lines
+73
to
+77
| // Warm the storage for sending ETH to smart contract addresses. | ||
| // We allow this call to revert becaues it was either unnecessary in the first place | ||
| // or failing to send `ETH` to the `receiver` will cause a revert in the settlement | ||
| // contract. | ||
| receiver.call{value: 0}(""); |
There was a problem hiding this comment.
Was it moved here just as a part of refactoring, or is it essential for the changes to work correctly?
There was a problem hiding this comment.
Having to warm the storage for sending ETH to a smart contract wallet is a very annoying implementation detail the user shouldn't have to worry about. This part should always happen and should not lead to reverts for any reasonable smart contract receiver.
And since the prepareSwap() step is now optional depending on the existence of pre-interactions I moved it out of there. Also the context of this doesn't matter (i.e. whether the trader or the solver sends it).
squadgazzz
approved these changes
Oct 24, 2024
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Currently the trade verification does a few checks and actions on behalf of the user that requested the quote to make the verification possible.
For example if a user has ETH and no WETH but requests a quote for WETH we'd try wrap the necessary WETH before doing the fake settlement in the simulation.
We also do a check before the settlement to verify that the user has the required sell token balance. That way we can return a more helpful error message than when the settlement contract reverts due to the missing funds.
Both of these things could fail if the user provides pre-hooks that will only set up the necessary trade pre-conditions for the
fromaddress.Changes
Adjusted the simulation code such that we can control whether some reasonable pre-conditions will be set up as part of the simulation or not.
If a user provides pre-conditions we now assume that they are necessary to set things up and we don't do our set up to not interfere with that.
How to test
Added an e2e test that transfers the needed sell tokens from a safe to the trader address in a pre-hook. This test fails without the change introduced in this PR.
Edit: The test works locally but appears to cause issues in CI for whatever reason. :/
Edit2: It's actually an existing test that fails exactly because of the reason described 👇
While working on the test I learned about a niche edge case that currently can't be verified: If you try to get a quote for a
fromaddress that is a contract that will only be deployed in the pre-interaction.The reason is that before we do the simulation we check whether the
fromaddress is a contract. If it is a contract we use some state override magic to make our fake user during the simulation behave like the contract that was deployed at that address. But we can't know that thefromaddress will contain contract code if the contract will only be deployed in the pre-interaction.