bug: The Staging 0x solver does not compute a fee for fok limit orders #1959
Assignees
Labels
bug
Something isn't working
E:3.1 Driver Colocation
See https://github.com/cowprotocol/pm/issues/14 for details
Comments
harisang
changed the title
fleupold
added
the
E:3.1 Driver Colocation
|
Looking into it. I see we did encode three prices (and computed a surplus fee on the solver side), but it looks like something may be off with the interaction amount encoding (we are sending the whole sell amount in and not 4994). Logs: https://production-6de61f.kb.eu-central-1.aws.cloud.es.io/app/r/s/7FXyg 
|
If I am not mistaken, I feel this is the intended behavior, as it is easier to then pocket the fee in the buy token, so as to not invoke the API once again. I.e., solvers are supposed to report a fee in the sell token, but then Gnosis solvers decide to incur negative slippage in the sell token and then positive slippage in the buy token, as the driver ultimately corrects the price to account for that fee. So my complaint was mainly for not propagating the surplus fee into the database; something is lost on the way. Anyways, seems you have already figured most things out so I don't want to add noise here. |
This was referenced Oct 13, 2023
fleupold
added a commit
that referenced
this issue
Oct 16, 2023
# Description The computation of surplus fees for partially fillable limit orders happens delayed in the co-located world. This is because at the time of solution ranking, the autopilot doesn't have access to the executed amounts for each order (something that #1949 will hopefully address). We therefore wait to populate the surplus fee until the settlement (and thus the different order specific clearing prices which incorporate the fee) has been observed on-chain. The logic that then computes the fee loads and updates the existing executions from the DB. However, it simply assumes there is no surplus fee computation if a "solver fee" is set: https://github.com/cowprotocol/services/blob/efa7c756ff9e58b8ae3258d5715f86e2ee04185f/crates/autopilot/src/decoded_settlement.rs#L336-L338 If we look at the code that is writing this value we see that currently this fee is always set to 0! Therefore we also assume that the surplus fee is zero and are not reporting it for limit orders (e.g. [this one](https://explorer.cow.fi/orders/0xf61a074c198e0b8556eb07784a137f664023822ab05f3ff368883f1ed7e7eb5dffab14b181409170378471b13ff2bff5be012c64654f6e8a?tab=overview)). The fix is to set a NULL instead of 0 solver fee when writing the original order execution. Alternatively, we could also fix the fee computation in `decode_settlement` to always add a potential solver fee and the discrepancy between uniform and order specific clearing prices (if any) as those are effectively fees. This would be necessary if we ever allowed both type of fees (fixed + surplus fees). I'm not sure if this is necessary though. # Changes - [x] Make solver fee optional on save order_executions - [x] Set it to None for orders where fee is set from surplus ## How to test - [x] e2e test ## Related Issues Fixes #1959
MartinquaXD
added a commit
that referenced
this issue
Oct 18, 2023
# Description Supersedes #1531 #1469 introduced simulations to the single order solvers so that we can produce accurate fees when solving for partially fillable limit orders. This PR ports some of that logic to our solvers binary for the DEX aggregator solvers in the co-located world. In particular, it introduces a new `Swapper` helper contract for setting up simulations on-chain for DEX aggregator swaps. We also only do the simulation for limit orders (to avoid additional roudtrips for market orders, which don't use this gas information anyway, so the heuristic value is more than good enough). # Changes - [x] Introduce new simulation helper contract. - [x] Execute simulations for limit orders and use the simulated gas amount for computing accurate solver fees. - [x] : Mock node request/responses for executing the simulation in the `partial_fill` tests. ## How to test Adjusted some existing tests to work. ## Related Issues Fixes #1959 --------- Co-authored-by: Martin Beckmann <[email protected]> Co-authored-by: Martin Beckmann <[email protected]>
fleupold
added a commit
that referenced
this issue
Oct 20, 2023
# Description The added e2e test adds ~10s in unnecessary total time to the test suite, because the onchain settlement updater runs on its own schedule (which is hardcoded at 10s). My first thought was to make this schedule configurable (and set it to 1s in the test), but it seems more correct for this to also be driven by new blocks incoming. This PR refactors the component to use `CurrentBlockStream` instead of its own polling. # Changes - Use CurrentBlockStream instead of hardcoded polling loop One subtlety is that the SettlementUpdater is not reading the tx_hash from the onchain settlement event, but from the DB instead. This means that the Settlement has to be processed first (and then the competition data may only be updated in the next run since it doesn't yet see the settlement when it runs). This is not an issue in practice (submission data will be written to the DB once the next block arrives). For the For the e2e test, we now add extra `evm_mine` in the `wait_for_condition_block` to simulate the chain advancing. I was thinking to either add a concept of dependencies into the different jobs that run when a new block arrive, or put the settlement updating code on the same codepath that processes the original settlement events, but both seem to be involved refactorings for fairly little benefit imo. ## How to test e2e tests now run faster ## Related Issues Improves test from #1959
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Problem
Here is a settlement from 0x on barn from a few hours ago, that executes two limit orders and sets zero fee to them.
https://etherscan.io/tx/0x21fa43eab9efae65320ebc170b31176edd03a9097b064dd240142b1dc56cf6d2
https://explorer.cow.fi/tx/0x21fa43eab9efae65320ebc170b31176edd03a9097b064dd240142b1dc56cf6d2?tab=orders
The two order ids are:
0xf61a074c198e0b8556eb07784a137f664023822ab05f3ff368883f1ed7e7eb5dffab14b181409170378471b13ff2bff5be012c64654f6e8a0x574b858644c51cc19614b18f936ccb0dcc1867f110f3299874f8065a1e742275ffab14b181409170378471b13ff2bff5be012c64654f94d7I checked the database and this is what I saw:
This log seems to suggest there was some fee computed by the solver, if I am not mistaken.
The text was updated successfully, but these errors were encountered: