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

chore: Executed surplus fee in surplus token #2963

Open
sunce86 opened this issue Sep 9, 2024 · 3 comments
Open

chore: Executed surplus fee in surplus token #2963

sunce86 opened this issue Sep 9, 2024 · 3 comments

Comments

@sunce86
Copy link
Contributor

sunce86 commented Sep 9, 2024

Background

Both solver team and frontend team would like to have this field expressed over surplus token, instead of sell token. This would help a lot with showing complete breakdown of fees (network fee, protocol fee, total fee) as protocol fees are already calculated in surplus tokens.

Details

Steps:

  1. Add executed_surplus_fee_token to order_execution database table and populate historic entries with sell token of the order.
  2. Expose the field executed_surplus_fee_token over API whenever executed_surplus_fee is exposed.
  3. Switch to saving surplus fee in surplus token instead of sell token

So, bottom line, there will be a timestamp after which all fees will be in surplus token. For historic entries, executed surplus fee in sell token will remain.
Both solver and frontend team need to cope with this. I assume frontend will have to have a special IF to show things differently depending on if executed_surplus_fee is in sell token (for historic entries) or in surplus token (for new entries). There is also an alternative to try to convert executed_surplus_fee to surplus token for historic entries on the frontend side, where traded sell/buy amounts could be decent approximates (at least in most cases).

In the meantime, we decided to do a manual migration for historic entries, so that ALL fees will be expressed over SURPLUS token, so no special handling will be needed to support backward compatibility.

Acceptance criteria

Complete breakdown of taken fees is exposed on get_trades and get_order endpoints:

  1. network fee in surplus token
  2. protocol fees per fee policy, in surplus token
  3. total fee in surplus token (or maybe omitted since it can be calculated by summarizing (1) and (2))
@fleupold
Copy link
Contributor

Not sure about the naming (maybe we can finally drop the "surplus" from the field, given that there is no non surplus fee anymore and "surplus fee token" = surplus token is 🤯)

Otherwise I think the plan is sound since the surplus token can either be buy or sell token (and so the frontend in any case needs to handle both cases). It's just that for historic orders it will displayed in sell token regardless of order kind.

I'm just not sure in terms of priority if this is something we should do this quarter. cc @MartinquaXD

@sunce86 sunce86 mentioned this issue Sep 10, 2024
3 tasks
@sunce86
Copy link
Contributor Author

sunce86 commented Sep 11, 2024

Not sure about the naming

Agreed, just wasn't sure if it's the right time for renaming. But why not, will adjust the field.

I'm just not sure in terms of priority if this is something we should do this quarter.

I was almost finished with the implementation when you posted the comment. Will finish it since already started, frontend will be happy about it.

Copy link

This issue has been marked as stale because it has been inactive a while. Please update this issue or it will be automatically closed.

@github-actions github-actions bot added the stale label Nov 11, 2024
@sunce86 sunce86 reopened this Nov 29, 2024
@github-actions github-actions bot removed the stale label Nov 30, 2024
sunce86 added a commit that referenced this issue Dec 24, 2024
# Description
Implements (1) and (2) from
#2963.
Also, renames `surplus_fee` to `executed_fee` in database.

# Changes
<!-- List of detailed changes (how the change is accomplished) -->

- [ ] Added new field `order_execution.executed_fee_token` to database
- [ ] Populated this field with order sell token, otherwise default
value is 0x0
- [ ] Token is exposed over API the same way as `executed_fee`.

Note: all places where sell token needs to be substituted with surplus
token are marked with `TODO surplus token` and this will be tackled as
part of (3) from #2963.

## How to test
Existing e2e tests. Observed values during the tests.
sunce86 added a commit that referenced this issue Dec 25, 2024
# Description
Implements (1) and (2) from
#2963.
Also, renames `surplus_fee` to `executed_fee` in database.

# Changes
<!-- List of detailed changes (how the change is accomplished) -->

- [ ] Added new field `order_execution.executed_fee_token` to database
- [ ] Populated this field with order sell token, otherwise default
value is 0x0
- [ ] Token is exposed over API the same way as `executed_fee`.

Note: all places where sell token needs to be substituted with surplus
token are marked with `TODO surplus token` and this will be tackled as
part of (3) from #2963.

## How to test
Existing e2e tests. Observed values during the tests.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants