-
Notifications
You must be signed in to change notification settings - Fork 84
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
Comments
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 |
Agreed, just wasn't sure if it's the right time for renaming. But why not, will adjust the field.
I was almost finished with the implementation when you posted the comment. Will finish it since already started, frontend will be happy about it. |
This issue has been marked as stale because it has been inactive a while. Please update this issue or it will be automatically closed. |
# 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.
# 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.
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:
executed_surplus_fee_token
to order_execution database table and populate historic entries with sell token of the order.executed_surplus_fee_token
over API wheneverexecuted_surplus_fee
is exposed.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 ifexecuted_surplus_fee
is in sell token (for historic entries) or in surplus token (for new entries). There is also an alternative to try to convertexecuted_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:
The text was updated successfully, but these errors were encountered: