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

bug: Wrong executed amounts in competition data #2150

Closed
fhenneke opened this issue Dec 9, 2023 · 3 comments · Fixed by #2151
Closed

bug: Wrong executed amounts in competition data #2150

fhenneke opened this issue Dec 9, 2023 · 3 comments · Fixed by #2151
Assignees
Labels
bug Something isn't working E:3.1 Driver Colocation See https://github.com/cowprotocol/pm/issues/14 for details

Comments

@fhenneke
Copy link

fhenneke commented Dec 9, 2023

Problem

It seems that the data on the buy and sell amounts of orders added in #1975 is not accurate.
This is only implemented on staging if I understand correctly.

For example, the transaction with hash 0x2548006a1bf02717d4e0a20bd6f0db7e8762d146161135f2d22f857eb726740d gives executed amounts
image
but the sell amount on etherscan
image
and our order book endpoint
image
are different.

Impact

Having the correct data in the competition endpoint is required for EBBO tests.

Expected behaviour

I expect that the "sellAmount" and "buyAmount" in the competition endpoint to be the amounts send and received by a user in that settlement.

@fhenneke fhenneke added the bug Something isn't working label Dec 9, 2023
@fleupold fleupold added the E:3.1 Driver Colocation See https://github.com/cowprotocol/pm/issues/14 for details label Dec 9, 2023
@fleupold
Copy link
Contributor

fleupold commented Dec 9, 2023

It looks like the amount shown there is the sellAmount + solver fee (which doesn't matter here because it's a "market" order where the fee is specified by the order itself: log

This seems to be because the driver assumes the fee is correctly solver specified if it's present on the response of the solver engine:

competition::solution::trade::Fulfillment::new(
order,
fulfillment.executed_amount.into(),
match fulfillment.fee {
Some(fee) => competition::solution::trade::Fee::Dynamic(
competition::order::SellAmount(fee),
),
None => competition::solution::trade::Fee::Static,
},

We can either check there is the order has a solver defined fee, or fix it in the solver engines to not set this value for order types that don't expect it. The more defensive thing imo would be to change it in the driver.

@fleupold
Copy link
Contributor

Actually I think my assessment from yesterday was incorrect, we do already reject settlements from solvers that set a fee on orders they aren't supposed to. The discrepancy between the amount you see and executedSellAmount in the API is the difference between the subsidized and unsubsidized fee amount.

The executed amount is adding the "solver fee" (this name is used with many different semantics throughout the codebase, but here it's supposed to mean the fee that the solver should use when evaluating the objective value which factors our the 0.002 ETH subsidy we give on market orders), but it should be adding the "user fee" instead.

#2151 should address the issue.

I assume it's ok to report the fully executed buy and sell amounts (after all fees) for the EBBO test.

@fleupold fleupold assigned fleupold and unassigned sunce86 Dec 11, 2023
@fhenneke
Copy link
Author

I assume it's ok to report the fully executed buy and sell amounts (after all fees) for the EBBO test.

Yes, this is exactly what we need: the amount users send us (i.e. before deducting any fees in the sell token) and the amount users receive (i.e. after deducting any fees in the buy token).

fleupold added a commit that referenced this issue Dec 11, 2023
# Description
Market orders currently return incorrect executed amounts (cf. #2150)
because we report the fee amount solvers are supposed to use for scoring
(which add the 0.002 ETH gas subsidy we currently apply on user orders
to compensate the gas overhead in the settlement contract).
However, this fee amount is only used for scoring (to avoid solutions
getting rejected because of negative scores) and is not what the user
actually pays.

# Changes

- [ ] Rename current "solver_fee()" method on `Trade` to `scoring_fee()`
as it captures more accurately what this specific value should be used
for (I started renaming all occurrences of solver_fee which imply this
semantic, but this diff is becoming huge.
- [ ] Introduce a new `fee` method which returns the fee from the user
perspective (namely the subsidised fee in the case of market orders)
- [ ] Use 👆 when computing executed fee amount
- [ ] Added integration tests for the fee amount behavior

## How to test
Newly added tests

## Related Issues

Fixes #2150
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working E:3.1 Driver Colocation See https://github.com/cowprotocol/pm/issues/14 for details
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants