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.

Sign up for GitHub

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

Convert executed_fee_amount to surplus token #3224

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
+59 −13
Open

Convert executed_fee_amount to surplus token #3224

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
5bed473
Initial commit
sunce86 Jan 3, 2025
a036de0
safe math
sunce86 Jan 8, 2025
33ddf84
other place
sunce86 Jan 8, 2025
5694559
Merge branch 'main' into convert-executed-fee-amount
sunce86 Jan 8, 2025
e77a7d7
protect div by zero
sunce86 Jan 8, 2025
6af37c7
fix docs
sunce86 Jan 10, 2025
8475e97
Merge branch 'main' into convert-executed-fee-amount
sunce86 Jan 10, 2025
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
1 change: 1 addition & 0 deletions crates/model/src/order.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -671,6 +671,7 @@ pub struct OrderMetadata {
pub executed_sell_amount: BigUint,
#[serde_as(as = "HexOrDecimalU256")]
pub executed_sell_amount_before_fees: U256,
// The fee amount is expressed in the surplus token
#[serde_as(as = "HexOrDecimalU256")]
pub executed_fee_amount: U256,
#[serde_as(as = "HexOrDecimalU256")]
Expand Down
4 changes: 3 additions & 1 deletion crates/orderbook/openapi.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -902,7 +902,9 @@ components:
allOf:
- $ref: "#/components/schemas/BigUint"
executedFeeAmount:
description: The total amount of fees that have been executed for this order.
description: The total amount of signed order fees that have been executed
for this order. The value is expressed in surplus token (sell token for
buy orders, and in buy token for sell orders).
allOf:
- $ref: "#/components/schemas/BigUint"
invalidated:
Expand Down
33 changes: 27 additions & 6 deletions crates/orderbook/src/database/orders.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -615,12 +615,33 @@ fn full_order_into_model_order(order: FullOrder) -> Result<Order> {
// Executed fee amounts and sell amounts before fees are capped by
// order's fee and sell amounts, and thus can always fit in a `U256`
// - as it is limited by the order format.
executed_sell_amount_before_fees: big_decimal_to_u256(&(order.sum_sell - &order.sum_fee))
.context(
"executed sell amount before fees does not fit in a u256",
)?,
executed_fee_amount: big_decimal_to_u256(&order.sum_fee)
.context("executed fee amount is not a valid u256")?,
executed_sell_amount_before_fees: big_decimal_to_u256(&(&order.sum_sell - &order.sum_fee))
.context("executed sell amount before fees does not fit in a u256")?,
executed_fee_amount: if order.sum_fee.is_zero() {
0.into()
} else {
let fee_in_sell_token = big_decimal_to_u256(&order.sum_fee)
.context("executed fee amount is not a valid u256")?;
match order.kind {
DbOrderKind::Buy => fee_in_sell_token,
DbOrderKind::Sell => {
// convert to buy token using executed amounts which are a very good
// approximation of UCP prices conversion rate
Comment on lines +628 to +629
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this could be false for multiple reasons:

  1. I know of at least 1 integration that created their orders with buy_amount: 1 out of convenience since cow protocol's competition would still protect this order by giving it a good price
  2. applying a generous slippage to your price would introduce an error

I think in order to make this change correct we'd have to use the actual settlement's UCP to convert this price. For orders with partial fills even multiple prices of the respective settlements.
I believe this might not even be a huge change. Our big order SQL query already has to join all the relevant trades while computing sum_fee. If this logic would be moved into the SQL query we should get actually accurate prices while still not having to change how the underlying gets stored in the DB.

Is that correct / reasonable?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For conversion we use executedBuyAmount which is not equal to signed buy_amount you mentioned.

Reference from settlement contract:
Screenshot 2025-01-10 at 11 49 46

fee_in_sell_token
.checked_mul(
big_decimal_to_u256(&order.sum_buy)
.context("executed buy amount is not an unsigned integer")?,
)
.context("fee_in_sell_token overflow")?
.checked_div(
big_decimal_to_u256(&(&order.sum_sell - &order.sum_fee)).context(
"executed sell amount before fees does not fit in a u256",
)?,
)
.context("fee_in_sell_token division by zero")?
}
}
},
executed_surplus_fee: big_decimal_to_u256(&order.executed_fee)
.context("executed fee is not a valid u256")?,
executed_fee: big_decimal_to_u256(&order.executed_fee)
Expand Down
34 changes: 28 additions & 6 deletions crates/shared/src/db_order_conversions.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
use {
anyhow::{Context, Result},
app_data::AppDataHash,
bigdecimal::Zero,
database::{
onchain_broadcasted_orders::OnchainOrderPlacementError as DbOnchainOrderPlacementError,
orders::{
Expand Down Expand Up @@ -73,12 +74,33 @@ pub fn full_order_into_model_order(order: database::orders::FullOrder) -> Result
// Executed fee amounts and sell amounts before fees are capped by
// order's fee and sell amounts, and thus can always fit in a `U256`
// - as it is limited by the order format.
executed_sell_amount_before_fees: big_decimal_to_u256(&(order.sum_sell - &order.sum_fee))
.context(
"executed sell amount before fees does not fit in a u256",
)?,
executed_fee_amount: big_decimal_to_u256(&order.sum_fee)
.context("executed fee amount is not a valid u256")?,
executed_sell_amount_before_fees: big_decimal_to_u256(&(&order.sum_sell - &order.sum_fee))
.context("executed sell amount before fees does not fit in a u256")?,
executed_fee_amount: if order.sum_fee.is_zero() {
0.into()
} else {
let fee_in_sell_token = big_decimal_to_u256(&order.sum_fee)
.context("executed fee amount is not a valid u256")?;
match order.kind {
DbOrderKind::Buy => fee_in_sell_token,
DbOrderKind::Sell => {
// convert to buy token using executed amounts which are a very good
// approximation of UCP prices conversion rate
fee_in_sell_token
.checked_mul(
big_decimal_to_u256(&order.sum_buy)
.context("executed buy amount is not an unsigned integer")?,
)
.context("fee_in_sell_token overflow")?
.checked_div(
big_decimal_to_u256(&(&order.sum_sell - &order.sum_fee)).context(
"executed sell amount before fees does not fit in a u256",
)?,
)
.context("fee_in_sell_token division by zero")?
}
}
},
sunce86 marked this conversation as resolved.
Show resolved Hide resolved
executed_surplus_fee: big_decimal_to_u256(&order.executed_fee)
.context("executed fee is not a valid u256")?,
executed_fee: big_decimal_to_u256(&order.executed_fee)
Expand Down
Loading