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

Jump to bottom

Remove user fee #2586

Merged
merged 7 commits into from
Apr 9, 2024
Merged

Remove user fee #2586

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
3792fc7
Remove user fee
sunce86 Apr 1, 2024
66e3e78
fix tests
sunce86 Apr 1, 2024
9dbb2ac
Update solvers api
sunce86 Apr 1, 2024
bf6e9b9
comments
sunce86 Apr 4, 2024
bbeb2cd
small fix
sunce86 Apr 9, 2024
6aff741
Merge branch 'main' into remove-user-fee-locally
sunce86 Apr 9, 2024
6d10d60
fix tests
sunce86 Apr 9, 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
1 change: 0 additions & 1 deletion crates/autopilot/src/boundary/order.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@ pub fn to_domain(
buy_token: order.data.buy_token,
sell_amount: order.data.sell_amount,
buy_amount: order.data.buy_amount,
user_fee: order.data.fee_amount,
protocol_fees,
valid_to: order.data.valid_to,
side: order.data.kind.into(),
Expand Down
1 change: 0 additions & 1 deletion crates/autopilot/src/domain/auction/order.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@ pub struct Order {
pub buy_token: H160,
pub sell_amount: U256,
pub buy_amount: U256,
pub user_fee: U256,
pub protocol_fees: Vec<fee::Policy>,
pub side: Side,
pub class: Class,
Expand Down
4 changes: 0 additions & 4 deletions crates/autopilot/src/infra/persistence/dto/order.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,8 +21,6 @@ pub struct Order {
pub sell_amount: U256,
#[serde_as(as = "HexOrDecimalU256")]
pub buy_amount: U256,
#[serde_as(as = "HexOrDecimalU256")]
pub user_fee: U256,
pub protocol_fees: Vec<FeePolicy>,
pub valid_to: u32,
pub kind: boundary::OrderKind,
Expand All @@ -49,7 +47,6 @@ pub fn from_domain(order: domain::Order) -> Order {
buy_token: order.buy_token,
sell_amount: order.sell_amount,
buy_amount: order.buy_amount,
user_fee: order.user_fee,
protocol_fees: order.protocol_fees.into_iter().map(Into::into).collect(),
valid_to: order.valid_to,
kind: order.side.into(),
Expand Down Expand Up @@ -78,7 +75,6 @@ pub fn to_domain(order: Order) -> domain::Order {
buy_token: order.buy_token,
sell_amount: order.sell_amount,
buy_amount: order.buy_amount,
user_fee: order.user_fee,
protocol_fees: order.protocol_fees.into_iter().map(Into::into).collect(),
valid_to: order.valid_to,
side: order.kind.into(),
Expand Down
6 changes: 0 additions & 6 deletions crates/driver/openapi.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -197,12 +197,6 @@ components:
buyAmount:
description: Amount to be bought
$ref: "#/components/schemas/TokenAmount"
userFee:
description: |
The fee that the user signed to pay for getting their order included.

This fee may be different from solver fee as it includes subsidies.
$ref: "#/components/schemas/TokenAmount"
validTo:
description: The time until which the order is valid.
type: integer
Expand Down
7 changes: 5 additions & 2 deletions crates/driver/src/boundary/settlement.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -262,7 +262,10 @@ fn to_boundary_order(order: &competition::Order) -> Order {
buy_token: order.buy.token.into(),
sell_amount: order.sell.amount.into(),
buy_amount: order.buy.amount.into(),
fee_amount: order.user_fee.into(),
// The fee amount is guaranteed to be 0 and it no longer exists in the domain, but for
// the proper encoding of the order where the `model::OrderData` struct is used, we must
// set it to 0.
fee_amount: 0.into(),
MartinquaXD marked this conversation as resolved.
Show resolved Hide resolved
receiver: order.receiver.map(Into::into),
valid_to: order.valid_to.into(),
app_data: AppDataHash(order.app_data.into()),
Expand All @@ -283,7 +286,7 @@ fn to_boundary_order(order: &competition::Order) -> Order {
},
metadata: OrderMetadata {
full_fee_amount: Default::default(),
solver_fee: order.user_fee.into(),
solver_fee: 0.into(),
class: match order.kind {
competition::order::Kind::Market => OrderClass::Market,
competition::order::Kind::Liquidity => OrderClass::Liquidity,
Expand Down
15 changes: 1 addition & 14 deletions crates/driver/src/domain/competition/auction.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -238,20 +238,7 @@ impl AuctionProcessor {
}
};

let available_sell_amount = {
let available = order.available(weth);
available.sell.amount.0.checked_add(available.user_fee.0)
};
let max_sell = match available_sell_amount {
Some(amount) => order::SellAmount(amount),
None => {
observe::order_excluded_from_auction(
order,
observe::OrderExcludedFromAuctionReason::CouldNotCalculateMaxSell,
);
return false;
}
};
let max_sell = order::SellAmount(order.available(weth).sell.amount.0);

let allocated_balance = match order.partial {
order::Partial::Yes { .. } => max_sell.min(*remaining_balance),
Expand Down
11 changes: 3 additions & 8 deletions crates/driver/src/domain/competition/order/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,6 @@ pub struct Order {
/// The maximum amount this order is allowed to sell when completely filled.
pub sell: eth::Asset,
pub side: Side,
pub user_fee: SellAmount,
pub kind: Kind,
pub app_data: AppData,
pub partial: Partial,
Expand Down Expand Up @@ -118,8 +117,6 @@ pub struct Available {
/// The available minimum buy amount for an order that gets passed to a
/// solver engine.
pub buy: eth::Asset,
/// The available fee amount.
pub user_fee: SellAmount,
}

impl Order {
Expand Down Expand Up @@ -172,7 +169,6 @@ impl Order {
token: self.buy.token.wrap(weth),
amount: self.buy.amount,
},
user_fee: self.user_fee,
};

let available = match self.partial {
Expand All @@ -181,9 +177,9 @@ impl Order {
};
let target = self.target();

for amount in [&mut amounts.sell.amount.0, &mut amounts.user_fee.0] {
*amount = util::math::mul_ratio(*amount, available.0, target.0).unwrap_or_default();
}
amounts.sell.amount = util::math::mul_ratio(amounts.sell.amount.0, available.0, target.0)
.unwrap_or_default()
.into();

amounts.buy.amount =
util::math::mul_ratio_ceil(amounts.buy.amount.0, available.0, target.0)
Expand Down Expand Up @@ -427,7 +423,6 @@ mod tests {
Some(executed) if executed.token == buy(0).token => Side::Buy,
_ => panic!(),
},
user_fee: Default::default(),
kind: Kind::Limit,
app_data: Default::default(),
partial: available
Expand Down
7 changes: 6 additions & 1 deletion crates/driver/src/domain/competition/solution/trade.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,12 @@ impl Fulfillment {
/// considering their signed order and the uniform clearing prices
pub fn fee(&self) -> order::SellAmount {
match self.fee {
Fee::Static => self.order.user_fee,
Fee::Static => {
// Orders with static fees are no longer used, except for quoting purposes, when
// the static fee is set to 0. This is expected to be resolved with https://github.com/cowprotocol/services/issues/2543
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there anything blocking #2543 ? If not could we simply implement this now to combine the cleanups (I'm not sure there will be time scheduled for the follow up cleanup)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Started working on it here: #2590
Do you think #2543 should block merging this PR?

// Once resolved, this code will be simplified as part of https://github.com/cowprotocol/services/issues/2507
order::SellAmount(0.into())
}
Fee::Dynamic(fee) => fee,
}
}
Expand Down
1 change: 0 additions & 1 deletion crates/driver/src/domain/quote.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,7 +120,6 @@ impl Order {
buy: self.buy(),
sell: self.sell(),
side: self.side,
user_fee: Default::default(),
kind: competition::order::Kind::Market,
app_data: Default::default(),
partial: competition::order::Partial::No,
Expand Down
3 changes: 0 additions & 3 deletions crates/driver/src/infra/api/routes/solve/dto/auction.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,6 @@ impl Auction {
Kind::Sell => competition::order::Side::Sell,
Kind::Buy => competition::order::Side::Buy,
},
user_fee: order.user_fee.into(),
kind: match order.class {
Class::Market => competition::order::Kind::Market,
Class::Limit => competition::order::Kind::Limit,
Expand Down Expand Up @@ -229,8 +228,6 @@ struct Order {
sell_amount: eth::U256,
#[serde_as(as = "serialize::U256")]
buy_amount: eth::U256,
#[serde_as(as = "serialize::U256")]
user_fee: eth::U256,
protocol_fees: Vec<FeePolicy>,
valid_to: u32,
kind: Kind,
Expand Down
1 change: 0 additions & 1 deletion crates/driver/src/infra/observe/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -372,7 +372,6 @@ pub fn deadline(deadline: &Deadline, timeouts: &Timeouts) {
#[derive(Debug)]
pub enum OrderExcludedFromAuctionReason {
CouldNotFetchBalance,
CouldNotCalculateMaxSell,
InsufficientBalance,
OrderWithZeroAmountRemaining,
}
Expand Down
3 changes: 0 additions & 3 deletions crates/driver/src/infra/solver/dto/auction.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,7 +108,6 @@ impl Auction {
buy_token: available.buy.token.into(),
sell_amount: available.sell.amount.into(),
buy_amount: available.buy.amount.into(),
fee_amount: available.user_fee.into(),
kind: match order.side {
competition::order::Side::Buy => Kind::Buy,
competition::order::Side::Sell => Kind::Sell,
Expand Down Expand Up @@ -284,8 +283,6 @@ struct Order {
sell_amount: eth::U256,
#[serde_as(as = "serialize::U256")]
buy_amount: eth::U256,
#[serde_as(as = "serialize::U256")]
fee_amount: eth::U256,
kind: Kind,
partially_fillable: bool,
class: Class,
Expand Down
36 changes: 0 additions & 36 deletions crates/driver/src/tests/cases/fees.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,25 +7,6 @@ use crate::{
},
};

#[tokio::test]
#[ignore]
async fn rejects_unwarranted_solver_fee() {
let test = tests::setup()
.name("Solver fee on market order".to_string())
.pool(ab_pool())
.order(
// A solver reporting a fee on a swap order
ab_order()
.user_fee(1000.into())
.solver_fee(Some(500.into())),
)
.solution(ab_solution())
.done()
.await;

test.solve().await.status(hyper::StatusCode::BAD_REQUEST);
}

#[tokio::test]
#[ignore]
async fn solver_fee() {
Expand All @@ -46,20 +27,3 @@ async fn solver_fee() {
test.solve().await.ok().orders(&[order]);
}
}

#[tokio::test]
#[ignore]
async fn user_fee() {
for side in [order::Side::Buy, order::Side::Sell] {
let order = ab_order().side(side).user_fee(1000.into());
let test = tests::setup()
.name(format!("User Fee: {side:?}"))
.pool(ab_pool())
.order(order.clone())
.solution(ab_solution())
.done()
.await;

test.solve().await.ok().orders(&[order]);
}
}
4 changes: 2 additions & 2 deletions crates/driver/src/tests/setup/blockchain.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -148,7 +148,7 @@ impl QuotedOrder {
sell_amount: self.sell_amount(),
buy_amount: self.buy_amount(),
valid_to: u32::try_from(time::now().timestamp()).unwrap() + self.order.valid_for.0,
user_fee: self.order.user_fee,
user_fee: 0.into(),
side: self.order.side,
secret_key: blockchain.trader_secret_key,
domain_separator: blockchain.domain_separator,
Expand Down Expand Up @@ -632,7 +632,7 @@ impl Blockchain {
.unwrap()
.mint(
self.trader_address,
"1e-7".ether().into_wei() * execution.sell + order.user_fee,
"1e-7".ether().into_wei() * execution.sell,
)
.from(trader_account.clone())
.send(),
Expand Down
1 change: 0 additions & 1 deletion crates/driver/src/tests/setup/driver.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,6 @@ pub fn solve_req(test: &Test) -> serde_json::Value {
"buyToken": hex_address(test.blockchain.get_token(quote.order.buy_token)),
"sellAmount": quote.sell_amount().to_string(),
"buyAmount": quote.buy_amount().to_string(),
"userFee": quote.order.user_fee.to_string(),
"protocolFees": match quote.order.kind {
order::Kind::Market => json!([]),
order::Kind::Liquidity => json!([]),
Expand Down
18 changes: 1 addition & 17 deletions crates/driver/src/tests/setup/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -122,7 +122,6 @@ pub struct Order {
pub valid_for: util::Timestamp,
pub kind: order::Kind,

pub user_fee: eth::U256,
// Currently used for limit orders to represent the surplus_fee calculated by the solver.
pub solver_fee: Option<eth::U256>,

Expand Down Expand Up @@ -167,13 +166,6 @@ impl Order {
}
}

pub fn user_fee(self, amount: eth::U256) -> Self {
Self {
user_fee: amount,
..self
}
}

/// Ensure that this order generates no surplus, and therefore most likely
/// has a negative score.
pub fn no_surplus(self) -> Self {
Expand Down Expand Up @@ -290,7 +282,6 @@ impl Default for Order {
partial: Default::default(),
valid_for: 100.into(),
kind: order::Kind::Market,
user_fee: Default::default(),
solver_fee: Default::default(),
name: Default::default(),
surplus_factor: DEFAULT_SURPLUS_FACTOR.ether().into_wei(),
Expand Down Expand Up @@ -1041,10 +1032,6 @@ impl<'a> Solve<'a> {
blockchain: self.blockchain,
}
}

pub fn status(self, code: hyper::StatusCode) {
assert_eq!(self.status, code);
}
}

impl<'a> SolveOk<'a> {
Expand Down Expand Up @@ -1115,10 +1102,7 @@ impl<'a> SolveOk<'a> {

let (expected_sell, expected_buy) = match &expected.expected_amounts {
Some(executed_amounts) => (executed_amounts.sell, executed_amounts.buy),
None => (
fulfillment.quoted_order.sell + fulfillment.quoted_order.order.user_fee,
fulfillment.quoted_order.buy,
),
None => (fulfillment.quoted_order.sell, fulfillment.quoted_order.buy),
};
assert!(u256(trade.get("sellAmount").unwrap()) == expected_sell);
assert!(u256(trade.get("buyAmount").unwrap()) == expected_buy);
Expand Down
1 change: 0 additions & 1 deletion crates/driver/src/tests/setup/solver.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,6 @@ impl Solver {
"buyToken": hex_address(config.blockchain.get_token(buy_token)),
"sellAmount": sell_amount,
"buyAmount": buy_amount,
"feeAmount": quote.order.user_fee.to_string(),
"kind": match quote.order.side {
order::Side::Sell => "sell",
order::Side::Buy => "buy",
Expand Down
1 change: 0 additions & 1 deletion crates/e2e/tests/e2e/partially_fillable_balance.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,6 @@ async fn test(web3: Web3) {
let order = auction.orders.into_iter().next().unwrap();
assert!(order.partially_fillable);
assert!(matches!(order.class, OrderClass::Limit));
assert_eq!(order.user_fee, 0.into());

tracing::info!("Waiting for trade.");
let trade_happened =
Expand Down
1 change: 0 additions & 1 deletion crates/e2e/tests/e2e/partially_fillable_pool.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,6 @@ async fn test(web3: Web3) {
let order = auction.orders.into_iter().next().unwrap();
assert!(order.partially_fillable);
assert!(matches!(order.class, OrderClass::Limit));
assert_eq!(order.user_fee, 0.into());

tracing::info!("Waiting for trade.");
let trade_happened =
Expand Down
4 changes: 0 additions & 4 deletions crates/orderbook/openapi.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -914,10 +914,6 @@ components:
description: see `OrderParameters::buyAmount`
allOf:
- $ref: "#/components/schemas/TokenAmount"
userFee:
description: see `OrderParameters::feeAmount`
allOf:
- $ref: "#/components/schemas/TokenAmount"
validTo:
description: see `OrderParameters::validTo`
type: integer
Expand Down
2 changes: 0 additions & 2 deletions crates/orderbook/src/dto/order.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,8 +22,6 @@ pub struct Order {
pub sell_amount: U256,
#[serde_as(as = "HexOrDecimalU256")]
pub buy_amount: U256,
#[serde_as(as = "HexOrDecimalU256")]
pub user_fee: U256,
pub protocol_fees: Vec<FeePolicy>,
pub valid_to: u32,
pub kind: OrderKind,
Expand Down
2 changes: 0 additions & 2 deletions crates/solvers-dto/src/auction.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,8 +34,6 @@ pub struct Order {
pub sell_amount: U256,
#[serde_as(as = "HexOrDecimalU256")]
pub buy_amount: U256,
#[serde_as(as = "HexOrDecimalU256")]
pub fee_amount: U256,
Comment on lines -37 to -38
Copy link
Contributor

Choose a reason for hiding this comment

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

How confident are we that this breaking change is not going to shoot us in the foot? I think it would be ok to leave this value on the API level but simply always set it to 0 in code (and document that it's deprecated and always 0 in the API spec). For our internal models (domain, etc), we would want to remove this field however.

The frontend team would like to re-vamp our API and I think it would be ok to defer breaking changes thereto.

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 mentioned field is part of the solvers API. Not sure what frontend has with it? If you got confused and referred to the model data struct than yes, we still want to keep fee_amount there.
Solvers API should be kept clean, this moment is good enough to make this change as any other IMO (actually, would have been better if we made this change before solvers moved to this API 😄 buy hey...)

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, wrong file (I wanted to comment on the one above: crates/orderbook/src/dto/order.rs), there the field is user_fee. Just want to make sure only our frontend depends on this field and can live without it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a similar change we had few weeks ago with solver_fee: #2272 No major problems arose except shadow competition was down and alerter being down AFAIR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This time I would expect only shadow to be down because alerter is not deserializing user_fee.

pub kind: Kind,
pub partially_fillable: bool,
pub class: Class,
Expand Down
Loading
Loading