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

log fees in FillLog #212

Merged
merged 3 commits into from
Jan 11, 2024
Merged

log fees in FillLog #212

merged 3 commits into from
Jan 11, 2024

Conversation

Lou-Kamades
Copy link
Contributor

No description provided.

@@ -255,7 +255,7 @@ pub struct FillEvent {

pub price: i64,
pub peg_limit: i64,
pub quantity: i64, // number of quote lots
pub quantity: i64, // number of base lots
Copy link
Contributor Author

@Lou-Kamades Lou-Kamades Jan 10, 2024

Choose a reason for hiding this comment

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

this is match_base_lots per

let fill = FillEvent::new(
side,
maker_out,
best_opposing.node.owner_slot,
now_ts,
event_heap.header.seq_num,
best_opposing.node.owner,
best_opposing.node.client_order_id,
best_opposing.node.timestamp,
*owner,
order.client_order_id,
best_opposing_price,
best_opposing.node.peg_limit,
match_base_lots,
);

@@ -187,6 +187,13 @@ impl OpenOrdersAccount {
}
}

// calculate taker fee for logging, ignoring self trades
let taker_fees = if quote_native > 0 && fill.maker != fill.taker {
Copy link
Contributor

Choose a reason for hiding this comment

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

Please note that this amount will not correctly reflect the fees paid by the taker as it can be (slightly) greater than the actual fees paid: taker fees are computed from the total taken amount, not from the sum of the fill amounts, that is market.taker_fee_ceil(total_base_taken_wo_self_trade) <= \sum_i market.taker_fee_ceil(fill_amount_wo_self_trade_i)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't fully understand this. Wouldn't the difference between incrementally calculating the taker fee vs. calculating using the total taken amount just be attributable to the rounding in market.taker_fees_ceil()?

I'd expect that total_quote_lots_taken and then total_quote_taken_native_wo_self would be equal to the sum over all fill events of the quote_native calculated in the fill event?

let quote_native = (fill.quantity * fill.price * market.quote_lot_size) as u64;

If so, I think the rounding error is acceptable. Otherwise I'm afraid this change would be much more invasive since the taker fee can only be calculated after multiple fill events are emitted.

Copy link
Contributor

Choose a reason for hiding this comment

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

exactly, the differences will be only attributable to the rounding in market.taker_fees_ceil(). just wanted to make it clear because I wasn't sure how much precision you would need for the events; maybe i'd suggests to add the suffix _ceil to taker_fess or something similar to make it completely explicit in the code.

On the other hand, total_quote_lots_taken will be equal to the sum over all fill events quote_native, so you could approximate the self-trade amount with the difference between the actual fees paid & the hypothetical fee that would be paid if no self-trade amount was discounted.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the explanation!

@skrrb skrrb merged commit 77f1080 into openbook-dex:master Jan 11, 2024
5 checks passed
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.

2 participants