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

Fix taker fee lots vs native #251

Merged
merged 1 commit into from
Apr 21, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 21 additions & 13 deletions programs/openbook-v2/src/state/orderbook/book.rs
Original file line number Diff line number Diff line change
Expand Up @@ -267,16 +267,16 @@ impl<'a> Orderbook<'a> {

// Record the taker trade in the account already, even though it will only be
// realized when the fill event gets executed
let mut taker_fees = 0_u64;
let mut taker_fees_native = 0_u64;
Copy link
Contributor

Choose a reason for hiding this comment

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

why not calculate taker_fees_lots here as well?

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 think it makes sense to compute the fees in native units to have high resolution on them. Otherwise sub-lot fees would need to be rounded: but can't round up (otherwise openbook would possibly charge 1 lot taker fees on a 1 lot order), and rounding down would unnecessarily reduce collected fees.

We could compute lots and native taker_fees independently, but imo the correctness is clearer to see when the low-resolution value is derived from the high-resolution one.

if total_quote_lots_taken > 0 || total_base_lots_taken > 0 {
let total_quote_taken_native_wo_self =
((total_quote_lots_taken - decremented_quote_lots) * market.quote_lot_size) as u64;

if total_quote_taken_native_wo_self > 0 {
taker_fees = market.taker_fees_ceil(total_quote_taken_native_wo_self);
taker_fees_native = market.taker_fees_ceil(total_quote_taken_native_wo_self);

// Only account taker fees now. Maker fees accounted once processing the event
referrer_amount = taker_fees - maker_rebates_acc;
referrer_amount = taker_fees_native - maker_rebates_acc;
market.fees_accrued += referrer_amount as u128;
};

Expand All @@ -286,7 +286,7 @@ impl<'a> Orderbook<'a> {
side,
total_base_taken_native,
total_quote_taken_native,
taker_fees,
taker_fees_native,
referrer_amount,
);
} else {
Expand All @@ -295,12 +295,12 @@ impl<'a> Orderbook<'a> {

let (total_quantity_paid, total_quantity_received) = match side {
Side::Bid => (
total_quote_taken_native + taker_fees,
total_quote_taken_native + taker_fees_native,
total_base_taken_native,
),
Side::Ask => (
total_base_taken_native,
total_quote_taken_native - taker_fees,
total_quote_taken_native - taker_fees_native,
),
};

Expand All @@ -309,13 +309,21 @@ impl<'a> Orderbook<'a> {
taker: *owner,
total_quantity_paid,
total_quantity_received,
fees: taker_fees,
fees: taker_fees_native,
});
}

// The native taker fees in lots, rounded up.
//
// Imagine quote_lot_size = 10. A new bid comes in with max_quote lots = 10. It matches against
// other orders for 5 quote lots total. The taker_fees_native is 15, taker_fees_lots is 2. That
// means only up the 3 quote lots may be placed on the book.
let taker_fees_lots =
(taker_fees_native as i64 + market.quote_lot_size - 1) / market.quote_lot_size;

// Update remaining based on quote_lots taken. If nothing taken, same as the beginning
remaining_quote_lots =
order.max_quote_lots_including_fees - total_quote_lots_taken - (taker_fees as i64);
order.max_quote_lots_including_fees - total_quote_lots_taken - taker_fees_lots;

// Apply changes to matched asks (handles invalidate on delete!)
for (handle, new_quantity) in matched_order_changes {
Expand Down Expand Up @@ -367,7 +375,7 @@ impl<'a> Orderbook<'a> {
return err!(OpenBookError::WouldExecutePartially);
}

let mut maker_fees = 0;
let mut maker_fees_native = 0;
let mut posted_base_native = 0;
let mut posted_quote_native = 0;

Expand All @@ -386,12 +394,12 @@ impl<'a> Orderbook<'a> {

// Subtract maker fees in bid.
if side == Side::Bid {
maker_fees = market
maker_fees_native = market
.maker_fees_ceil(posted_quote_native)
.try_into()
.unwrap();

open_orders.position.locked_maker_fees += maker_fees;
open_orders.position.locked_maker_fees += maker_fees_native;
}

let bookside = self.bookside_mut(side);
Expand Down Expand Up @@ -477,8 +485,8 @@ impl<'a> Orderbook<'a> {
total_base_taken_native,
total_quote_taken_native,
referrer_amount,
taker_fees,
maker_fees,
taker_fees: taker_fees_native,
maker_fees: maker_fees_native,
})
}

Expand Down
101 changes: 101 additions & 0 deletions programs/openbook-v2/tests/cases/test_place_order_remaining.rs
Original file line number Diff line number Diff line change
Expand Up @@ -276,3 +276,104 @@ async fn test_cancel_order_yourself() -> Result<(), TransportError> {

Ok(())
}

#[tokio::test]
async fn test_place_order_taker_fees() -> Result<(), TransportError> {
let TestInitialize {
context,
collect_fee_admin,
owner,
owner_token_0,
owner_token_1,
market,
market_base_vault,
market_quote_vault,
price_lots,
tokens,
account_1,
account_2,
bids,
..
} = TestContext::new_with_market(TestNewMarketInitialize {
taker_fee: 11000, // 1.1%
maker_fee: 0,
quote_lot_size: 1000,
base_lot_size: 1,
..Default::default()
})
.await?;
let solana = &context.solana.clone();

// Set the initial oracle price
set_stub_oracle_price(solana, &tokens[1], collect_fee_admin, 1000.0).await;

send_tx(
solana,
PlaceOrderInstruction {
open_orders_account: account_1,
open_orders_admin: None,
market,
signer: owner,
user_token_account: owner_token_0,
market_vault: market_base_vault,
side: Side::Ask,
price_lots,
max_base_lots: 500,
max_quote_lots_including_fees: 500,
client_order_id: 0,
expiry_timestamp: 0,
order_type: PlaceOrderType::Limit,
self_trade_behavior: SelfTradeBehavior::default(),
remainings: vec![],
},
)
.await
.unwrap();

// Now place place a bid that fills the ask fully and has some remainder go to the book
let before_quote_balance = solana.token_account_balance(owner_token_1).await;
send_tx(
solana,
PlaceOrderInstruction {
open_orders_account: account_2,
open_orders_admin: None,
market,
signer: owner,
user_token_account: owner_token_1,
market_vault: market_quote_vault,
side: Side::Bid,
price_lots,
max_base_lots: 9999999, // unlimited
max_quote_lots_including_fees: 1000,
client_order_id: 0,
expiry_timestamp: 0,
order_type: PlaceOrderType::Limit,
self_trade_behavior: SelfTradeBehavior::default(),
remainings: vec![],
},
)
.await
.unwrap();
let after_quote_balance = solana.token_account_balance(owner_token_1).await;

// What should have happened is:
// - match against the ask, paying 500 quote lots for 500 base lots
// - taker fee native is 1.1% * 500 * 1000 = 5500 native
// - which is 5.5 quote lots, so only 500 - 6 = 494 quote lots can be placed on the book

let open_orders_account_2 = solana.get_account::<OpenOrdersAccount>(account_2).await;
assert_eq!(open_orders_account_2.position.bids_quote_lots, 494);
assert_eq!(open_orders_account_2.position.base_free_native, 500);

assert_eq!(
before_quote_balance - after_quote_balance,
// cost of buying 500 base lots
500 * 1000
// taker fee
+ 5500
// order on the book
+ 494 * 1000
);

Ok(())
}
Loading