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

Remove executed_solver_fee #2178

Merged
merged 7 commits into from
Dec 19, 2023
Merged

Remove executed_solver_fee #2178

merged 7 commits into from
Dec 19, 2023

Conversation

sunce86
Copy link
Contributor

@sunce86 sunce86 commented Dec 16, 2023

Description

executed_solver_fee was only used to store solver_fee of market orders during execution, so that it could be easily fetched by OnSettlementEventUpdater.
Now is removed and executed_solver_fee for market orders is fetched directly from the order.

I hope this change finally opens the door for other cleanups:

  • move saving the order executions completely, from the autopilot runloop to the OnSettlementEventUpdater.
  • dropping database column solver_fee (this is now safe to do since the column is not used for anything)

@sunce86 sunce86 requested a review from a team as a code owner December 16, 2023 01:51
Copy link
Contributor

@fleupold fleupold left a comment

Choose a reason for hiding this comment

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

Looks good to me, but why do we still need to store order_executions at all from the autopilot?

What information do we not have in the decoded settlement from OnSettlementEventUpdater that needs to be persisted?

crates/autopilot/src/decoded_settlement.rs Show resolved Hide resolved
crates/autopilot/src/decoded_settlement.rs Outdated Show resolved Hide resolved
crates/autopilot/src/decoded_settlement.rs Outdated Show resolved Hide resolved
crates/autopilot/src/database/orders.rs Show resolved Hide resolved
@sunce86
Copy link
Contributor Author

sunce86 commented Dec 18, 2023

Looks good to me, but why do we still need to store order_executions at all from the autopilot?

Would you prefer for it to be done "in flight" by the consumers of this table?
We at least need it to be calculated for limit orders (executed_surplus_fee so it could be shown on frontend).

Copy link
Contributor

@fleupold fleupold left a comment

Choose a reason for hiding this comment

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

Look good!

crates/autopilot/src/decoded_settlement.rs Show resolved Hide resolved
crates/autopilot/src/database/competition.rs Outdated Show resolved Hide resolved
crates/autopilot/src/decoded_settlement.rs Outdated Show resolved Hide resolved
crates/autopilot/src/database/orders.rs Outdated Show resolved Hide resolved
let uniform_sell_price = self.clearing_prices.get(sell_index).cloned()?;
let uniform_buy_price = self.clearing_prices.get(buy_index).cloned()?;

let sell_index = trade.sell_token_index.as_u64() as usize;
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: buy_token and buy_token_index are only needed in the ExecutedFee::Surplus case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Together with the comment 👇 , I tried to find more readable code but failed. Note that most of the orders will go through the ExecutedFee::Surplus code path once swap with zero fees feature is enabled. Feel free to reorganize the code in another way in a separate PR, since I failed to make any improvements (or even in this one).

let adjusted_sell_price = self.clearing_prices.get(sell_index).cloned()?;
let adjusted_buy_price = self.clearing_prices.get(buy_index).cloned()?;

// get uniform prices
let sell_index = self.tokens.iter().position(|token| token == 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.

I found it pretty confusing that (sell|buy)_index was used once to refer to the adjusted price and once for the UCP.
It would help to either prefix them maybe ucp_sell_index or limit the scope where you change the meaning like:

                // get uniform prices
                let uniform_sell_price = {
                    let sell_index = self.tokens.iter().position(|token| token == sell_token)?;
                    self.clearing_prices.get(sell_index).cloned()?
                };
                let uniform_buy_price = {
                    let buy_index = self.tokens.iter().position(|token| token == buy_token)?;
                    self.clearing_prices.get(buy_index).cloned()?
                };

@sunce86 sunce86 enabled auto-merge (squash) December 19, 2023 12:08
@sunce86 sunce86 merged commit 808a85e into main Dec 19, 2023
8 checks passed
@sunce86 sunce86 deleted the replace-executed-solver-fee branch December 19, 2023 12:11
@github-actions github-actions bot locked and limited conversation to collaborators Dec 19, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants