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

Executed total fee token #2966

Open
wants to merge 15 commits into
base: main
Choose a base branch
from
Open

Executed total fee token #2966

wants to merge 15 commits into from

Conversation

sunce86
Copy link
Contributor

@sunce86 sunce86 commented Sep 10, 2024

Description

Implements (1) and (2) from #2963.
Also, renames surplus_fee to total_fee in database.

Changes

  • Added new field order_execution.total_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 sunce86 self-assigned this Sep 10, 2024
@sunce86 sunce86 requested a review from a team as a code owner September 10, 2024 14:05
Copy link

github-actions bot commented Sep 10, 2024

Reminder: Please update the DB Readme.


Caused by:

@sunce86 sunce86 changed the title Executed surplus fee token Executed total fee token Sep 11, 2024
crates/orderbook/openapi.yml Outdated Show resolved Hide resolved
crates/database/src/order_execution.rs Outdated Show resolved Hide resolved
crates/database/src/order_execution.rs Outdated Show resolved Hide resolved
database/README.md Outdated Show resolved Hide resolved
crates/orderbook/openapi.yml Outdated Show resolved Hide resolved
&total_fee,
&executed_protocol_fees,
Asset {
token: ByteArray(order_fee.total.token.0 .0),
Copy link
Contributor

@m-lord-renkse m-lord-renkse Sep 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe worth having From<TokenAddress> for ByteArray as it appears in more occurrences 🤔

Comment on lines +137 to +138
///
/// Denominated in SELL token
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need this clarification here since the function name is self-explanatory already?

@@ -0,0 +1,24 @@
ALTER TABLE order_execution
RENAME COLUMN surplus_fee TO total_fee;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

new column is still called total_fee instead of executed_fee in the migration and readme.md.

ADD COLUMN total_fee_token bytea NOT NULL DEFAULT '\x0000000000000000000000000000000000000000';

-- Now populate existing rows with the sell token taken from the 'orders' table, or if it doesn't exist, try from 'jit_orders'.
UPDATE order_execution oe
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you test that this query does not leave any surplus_token uninitialized?

Copy link

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.

@github-actions github-actions bot added the stale label Sep 27, 2024
@sunce86 sunce86 removed the stale label Sep 27, 2024
Copy link

github-actions bot commented Oct 5, 2024

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.

@github-actions github-actions bot added the stale label Oct 5, 2024
@sunce86 sunce86 removed the stale label Oct 5, 2024
Copy link

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.

@github-actions github-actions bot added the stale label Oct 13, 2024
@sunce86 sunce86 removed the stale label Oct 14, 2024
Copy link

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.

@sunce86
Copy link
Contributor Author

sunce86 commented Oct 31, 2024

Will be worked on in Q4.

@sunce86 sunce86 reopened this Oct 31, 2024
@github-actions github-actions bot removed the stale label Nov 1, 2024
Copy link

github-actions bot commented Nov 9, 2024

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.

@github-actions github-actions bot added the stale label Nov 9, 2024
@github-actions github-actions bot closed this Nov 16, 2024
@sunce86 sunce86 reopened this Nov 28, 2024
@github-actions github-actions bot removed the stale label Nov 29, 2024
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

Successfully merging this pull request may close these issues.

5 participants