-
Notifications
You must be signed in to change notification settings - Fork 151
Add volume fee overrides #3976
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
Add volume fee overrides #3976
Conversation
|
|
||
| let source = | ||
| BackgroundInitLiquiditySource::new("fake", init, Duration::from_millis(10), None); | ||
| BackgroundInitLiquiditySource::new("fake_delayed_init", init, Duration::from_millis(10), None); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not related to the ret of the PR, just fixes flakiness of this test when run in parallel with other uni tests.
Co-authored-by: Martin Magnus <martin.beckmann@protonmail.com>
| }, | ||
| } | ||
|
|
||
| #[derive(Debug, Clone, Copy, PartialEq, Into)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A potentially controversial move: we had a "domain" FeeFactor and a "dto/arguments" FeeFactor that were identical with identical implementation of TryFrom and FromStr. I deleted the "domain" one and just keeping the "dto/record" type. IMHO it was too much Java for no value added.
squadgazzz
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
crates/autopilot/src/arguments.rs
Outdated
| pub volume_fee_bucket_overrides: Vec<TokenBucketFeeOverride>, | ||
|
|
||
| /// Enable volume fees for trades where sell token equals buy token. | ||
| /// By default, volume fees are NOT applied to same-token trades. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By default, volume fees are NOT applied to same-token trades.
Isn't this currently a required parameter so there is no default?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It has an implicit default. Default for bool is ArgAction::SetTrue source
/// When encountered, act as if `"true"` was encountered on the command-line
///
/// If no [`default_value`][super::Arg::default_value] is set, it will be `false`.
Also proven by the e2e tests which don't set the flag but autopilot & oderbook start just fine.
crates/autopilot/src/arguments.rs
Outdated
| /// stablecoin-to-stablecoin trades or specific token pairs (2-token | ||
| /// buckets). Multiple buckets can be separated by commas. | ||
| #[clap(long, env, value_delimiter = ',')] | ||
| pub volume_fee_bucket_overrides: Vec<TokenBucketFeeOverride>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These arguments are currently duplicated in the autopilot and orderbook. You can add them to the shared arguments to keep them in sync.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That way these args can't be where they belong with their friends in VolumeFeeConfig in the orderbook, but it's still better than having it duplicated. Done.
| boundary::OrderClass::Market => None, | ||
| boundary::OrderClass::Liquidity => None, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not related to this PR itself but is it actually correct that we don't apply the fee to market and liquidity orders? I was under the impression that we moved the decision which fee applies to which order into the config parameters. Any logic that hardcodes the behavior based on order types seems suspicious now.
Should this be an unreachable!() instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let me investigate this as a follow up, but let's not do unreachable: it might not be hit in staging but then we could see panics in prod. 🥴
crates/shared/src/fee.rs
Outdated
| /// Determines the applicable volume fee factor for a token pair, considering | ||
| /// same-token trade configuration, token bucket overrides and default fee | ||
| /// factor. | ||
| pub fn get_applicable_volume_fee_factor( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the advantage to having this a free function. Currently the autopilot and orderbook both store the necessary parameters differently and pass them into this function. Why not have 1 shared component defined in shared that contains all the necessary data, make this a member function, and instantiate 1 of those structs in the orderbook and autopilot?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with Martin here, 'carrying around' the necessary arguments is cumbersome and a single component responsible for the fees will be even better than a free standing function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It took a some refactoring and I had to compromise on the volume fee param which is not really accessible early enough without tearing the relevant segment of the autopilot apart, but this is now done.
|
The fees situation currently is intertwined as we have the // crates/shared/src/fee.rs
pub struct VolumeFeePolicy {
bucket_overrides: Vec<TokenBucketFeeOverride>,
default_factor: Option<FeeFactor>,
enable_sell_equals_buy_volume_fee: bool,
}
// crates/orderbook/src/arguments.rs
/// Volume-based protocol fee factor to be applied to quotes.
#[derive(clap::Parser, Debug, Clone)]
pub struct VolumeFeeConfig {
/// This is a decimal value (e.g., 0.0002 for 0.02% or 2 basis points).
/// The fee is applied to the surplus token (buy token for sell orders,
/// sell token for buy orders).
#[clap(...)]
pub factor: Option<FeeFactor>,
/// The timestamp from which the volume fee becomes effective.
#[clap(...)]
pub effective_from_timestamp: Option<DateTime<Utc>>,
}
// crates/autopilot/src/domain/fee/mod.rs
pub struct ProtocolFee {
policy: policy::Policy,
order_class: OrderClass,
}
pub struct ProtocolFees {
fee_policies: Vec<ProtocolFee>,
max_partner_fee: FeeFactor,
upcoming_fee_policies: Option<UpcomingProtocolFees>,
volume_fee_policy: VolumeFeePolicy,
}
pub struct UpcomingProtocolFees {
fee_policies: Vec<ProtocolFee>,
effective_from_timestamp: DateTime<Utc>,
}
// crates/autopilot/src/domain/fee/policy.rs
pub enum Policy {
Surplus(Surplus),
PriceImprovement(PriceImprovement),
Volume(Volume),
}
pub struct Surplus {
factor: FeeFactor,
max_volume_factor: FeeFactor,
}
pub struct PriceImprovement {
factor: FeeFactor,
max_volume_factor: FeeFactor,
}
pub struct Volume {
factor: FeeFactor,
}In autopilot, the Volume fee Policy will always use the VolumeFeePolicy to determine token pair volume fee while always passing override factor impl Volume {
pub fn apply(
&self,
order: &boundary::Order,
volume_fee_policy: &VolumeFeePolicy,
) -> Option<domain::fee::Policy> {
match order.metadata.class {
boundary::OrderClass::Market => None,
boundary::OrderClass::Liquidity => None,
boundary::OrderClass::Limit => {
// Use shared function to determine applicable volume fee factor
let factor = volume_fee_policy.get_applicable_volume_fee_factor(
order.data.buy_token,
order.data.sell_token,
Some(self.factor),
)?;
Some(domain::fee::Policy::Volume { factor })
}
}
}
}Whereas the orderbook, when constructing impl QuoteHandler {
pub fn new(
order_validator: Arc<dyn OrderValidating>,
quoter: Arc<dyn OrderQuoting>,
app_data: Arc<app_data::Registry>,
volume_fee: Option<VolumeFeeConfig>,
volume_fee_bucket_overrides: Vec<TokenBucketFeeOverride>,
enable_sell_equals_buy_volume_fee: bool,
) -> Self {
let volume_fee_policy = VolumeFeePolicy::new(
volume_fee_bucket_overrides,
volume_fee.as_ref().and_then(|config| config.factor),
enable_sell_equals_buy_volume_fee,
);
...The VolumeFeeConfig can determine an effective-from timestamp. Same as UpcomingProtocolFees in autopilot etc. It is not for this PR to fix the state of things, but I wanted to point it out, as I lean on the side that the way we specify, calculate and apply fees could be more uniform. |
Agreed, I think it would be ideal to have unified fee configs as much as possible, but it would be a much larger refactor. |
Description
Implements volume fee bucket overrides, allowing custom volume fees for groups of tokens. This enables us to have different fees for a chosen set of tokens, e.g. stable coins.
Changes
--volume-fee-bucket-overridesCLI argument"factor:token1,token2,..."(e.g.,"0:0xA0b86...,0x6B175...,0xdAC17...")How to test