Introduce Component for Balance Overrides #3125
Draft
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
This PR is the first in a stack to add a system for overriding balances so that more quotes can be verified. One issue with the current system, is that it requires that the account creating the quote has the balance available to the account (or available to the account after a pre-hook is executed) in order for the quote to be properly verified. This isn't always possible for all flows (and notably flows at Safe, where transactions to prepare the balance happens at the same time as the `setPreSignature` transaction executes and after the quote). The overall solution I would propose (hopefully a pragmatic one that isn't TOO hacky) would be enable special handling for the most commonly traded tokens, by configuring for each token how the storage slot is computed for the token balance. This way, you could maintain a file that contains a `token => computation_strategy` map for the most popular tokens allowing trades to be verified even for quotes from users without the balance available. If this strategy is accepted, in a follow up PRs I would: 1. Add the component to the trade verifier and use it to fund the trader when simulating quotes 2. Pipe configuration to the trade verifier and balance overrides component
|
This pull request has been marked as stale because it has been inactive a while. Please update this pull request or it will be automatically closed. |
|
@MartinquaXD is on vacation and will be back mid next week to take a look |
nlordell
added a commit
to nlordell/cowprotocol-services
that referenced
this pull request
Nov 22, 2024
This PR is a follow up to cowprotocol#3125 and uses the component introduced in the aforementioned PR for setting up a simulated token balance using state overrides in order for quote verification to work even when the trader does not have sufficient balance. The way it works is by configuring known mapping slots for the `mapping(address => uint256) balances` in ERC20 token contract implementations and using this to compute the slot for overriding the **solver's** token balance for the simulation. The solver can then use this balance to top up the trader's account if needed (in the case were we allow mocking preconditions in the simulation - currently this is determined by whether or not the quote has hooks). We intentionally do not override the trader's balance in order to not interfere with hook execution in verified quotes. Note that the type of the state override changed slightly. This is because it was wrong to begin with. Node implementations I tested with (Geth and Anvil) expect both the slot and the value for state overrides to be exactly 32-bytes long (so `H256`). I guess this feature of the state override in the `ethrpc` crate was not used in the past and therefore no one noticed 🤷. ### Test Plan Added an E2E test that uses the new token balance override feature in order to produce a verified quote for a trader with no balances. Note that commenting out the API arguments causes the test to fail as expected.
|
Note that I finished the follow up PR with a working E2E test 🎉. I created the PR to my fork (since I don't know how to create PR stacks when forking a repo). |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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
This PR is the first in a stack to add a system for overriding balances so that more quotes can be verified.
One limitation with the current quote verification system, is that it requires that the
fromaccount in the quote has the sell token balance available (or available after a pre-hook is executed) in order for the quote to be properly verified. This isn't always possible for all flows (and notably flows at Safe, where transactions to prepare the balance happens at the same time as thesetPreSignaturetransaction executes, so after the quote).The overall solution I would propose (hopefully a pragmatic one that isn't considered too hacky) would be enable special handling for the most commonly traded tokens, by configuring for each token how the storage slot is computed for the token balance. This way, you could maintain a file that contains a
token => computation_strategymap for the most popular tokens allowing trades to be verified even for quotes from users without the balance available.This PR is the first piece for this overall solution, which introduces a component for computing storage slots needed for overriding balances for
eth_callsimulations. If this strategy is accepted, in a follow up PRs I would:Solversimulation entrypoint, which would top up theTraderbalance if needed; this way the missing balance can be reported as part of the simulation and logged).Changes
BalanceOverridingcomponentHow to test
Added unit tests verifying logic.