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

Fix volume based cap calculation #2465

Merged
merged 6 commits into from
Mar 6, 2024
Merged

Fix volume based cap calculation #2465

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
72428a5
Fix volume based cap calculation
sunce86 Mar 4, 2024
a586fc8
refactor protocol fee to surplus token
sunce86 Mar 5, 2024
56a55ed
Reuse sell/buy amounts for volume based fee
sunce86 Mar 5, 2024
e6ee605
protocol_fee_in_sell_token
sunce86 Mar 5, 2024
dbd9eca
separate module for errors
sunce86 Mar 6, 2024
e96eef4
Merge branch 'main' into fix-volume-fee-calc
sunce86 Mar 6, 2024
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
19 changes: 11 additions & 8 deletions crates/driver/src/domain/competition/solution/fee.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -156,14 +156,17 @@ impl Fulfillment {
Side::Sell => executed,
};
// Sell slightly more `sell_token` to capture the `surplus_fee`
let executed_sell_amount_with_fee = executed_sell_amount
.checked_add(
// surplus_fee is always expressed in sell token
self.surplus_fee()
.map(|fee| fee.0)
.ok_or(trade::Error::ProtocolFeeOnStaticOrder)?,
)
.ok_or(trade::Error::Overflow)?;
let executed_sell_amount_with_fee = match self.order().side {
Side::Buy => executed_sell_amount
.checked_add(
// surplus_fee is always expressed in sell token
self.surplus_fee()
.map(|fee| fee.0)
.ok_or(trade::Error::ProtocolFeeOnStaticOrder)?,
)
.ok_or(trade::Error::Overflow)?,
Side::Sell => executed_sell_amount,
Copy link
Contributor

@fleupold fleupold Mar 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Honestly, it doesn't make a lot of sense to me to in some case return the executed sell amount without fee and assign it to a variable that is called executed_sell_amount_with_fee, Why exactly is this needed? Can we capture this information for future maintainers of this code to understand by adding a comment?

Also does this mean that we support volume based fees for sell orders which have non-solver determined fees?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The reason is that the autopilot (or solver reward script to be more precise) is currently calculating volume based fees in surplus token, so that it can be compared to fee from surplus (which is always) in surplus token. I can try tomorrow to refactor this part of the code to use surplus token, I believe it will make more sense then.

Copy link
Contributor Author

@sunce86 sunce86 Mar 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for raising this. I refactored the protocol fee to be calculated in surplus token, which is natural and already discussed with solver team internally as something we want to refactor.

Now, when applying protocol fee to Fulfillment, the conversion of the fee from surplus token to sell token is done at the very end and only because our Fulfillment object requires it.
Also, I think now it's much easier to understand why we add surplus fee to BUY orders only, and it's because we want to consider the full amounts (from user point of view) when calculating volume based fee.

};
apply_factor(executed_sell_amount_with_fee, factor)
}
}
Expand Down
4 changes: 2 additions & 2 deletions crates/driver/src/tests/cases/protocol_fees.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -144,7 +144,7 @@ async fn surplus_protocol_fee_sell_order_capped() {
quote_buy_amount: 40000000000000000000u128.into(),
executed: 40000000000000000000u128.into(),
executed_sell_amount: 50000000000000000000u128.into(),
executed_buy_amount: 35000000000000000000u128.into(),
executed_buy_amount: 36000000000000000000u128.into(),
};

protocol_fee_test_case(test_case).await;
Expand Down Expand Up @@ -182,7 +182,7 @@ async fn volume_protocol_fee_sell_order() {
quote_buy_amount: 40000000000000000000u128.into(),
executed: 40000000000000000000u128.into(),
executed_sell_amount: 50000000000000000000u128.into(),
executed_buy_amount: 15000000000000000000u128.into(),
executed_buy_amount: 20000000000000000000u128.into(),
};

protocol_fee_test_case(test_case).await;
Expand Down
37 changes: 21 additions & 16 deletions crates/e2e/tests/e2e/protocol_fee.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,46 +88,51 @@ async fn surplus_fee_sell_order_capped_test(web3: Web3) {
max_volume_factor: 0.1,
};
// Without protocol fee:
// Expected executed_surplus_fee is 167058994203399
// Expected execution is 10000000000000000000 GNO for
// 9871415430342266811 DAI, with executed_surplus_fee = 167058994203399 GNO
//
// With protocol fee:
// Expected executed_surplus_fee is 167058994203399 +
// 0.1*10000000000000000000 = 1000167058994203400
// 0.1*(10000000000000000000 - 167058994203399) = 1000150353094783059
//
// Final execution is 10000000000000000000 GNO for 8884257395945205588 DAI, with
// executed_surplus_fee = 1000167058994203400 GNO
// Final execution is 10000000000000000000 GNO for 8884273887308040129 DAI, with
// executed_surplus_fee = 1000150353094783059 GNO
//
// Settlement contract balance after execution = 1000167058994203400 GNO =
// 1000167058994203400 GNO * 8884257395945205588 / (10000000000000000000 -
// 1000167058994203400) = 987322948025407485 DAI
// Settlement contract balance after execution = 1000150353094783059 GNO =
// 1000150353094783059 GNO * 8884273887308040129 / (10000000000000000000 -
// 1000150353094783059) = 987306456662572858 DAI
execute_test(
web3.clone(),
fee_policy,
OrderKind::Sell,
1000167058994203400u128.into(),
987322948025407485u128.into(),
1000150353094783059u128.into(),
987306456662572858u128.into(),
)
.await;
}

async fn volume_fee_sell_order_test(web3: Web3) {
let fee_policy = FeePolicyKind::Volume { factor: 0.1 };
// Without protocol fee:
// Expected executed_surplus_fee is 167058994203399
// Expected execution is 10000000000000000000 GNO for
// 9871415430342266811 DAI, with executed_surplus_fee = 167058994203399 GNO
//
// With protocol fee:
// Expected executed_surplus_fee is 167058994203399 +
// 0.1*10000000000000000000 = 1000167058994203400
// 0.1*(10000000000000000000 - 167058994203399) = 1000150353094783059
//
// Final execution is 10000000000000000000 GNO for 8884273887308040129 DAI, with
// executed_surplus_fee = 1000150353094783059 GNO
//
// Settlement contract balance after execution = 1000167058994203400 GNO =
// 1000167058994203400 GNO * 8884257395945205588 / (10000000000000000000 -
// 1000167058994203400) = 987322948025407485 DAI
// Settlement contract balance after execution = 1000150353094783059 GNO =
// 1000150353094783059 GNO * 8884273887308040129 / (10000000000000000000 -
// 1000150353094783059) = 987306456662572858 DAI
execute_test(
web3.clone(),
fee_policy,
OrderKind::Sell,
1000167058994203400u128.into(),
987322948025407485u128.into(),
1000150353094783059u128.into(),
987306456662572858u128.into(),
)
.await;
}
Expand Down
Loading