-
Notifications
You must be signed in to change notification settings - Fork 152
Protocol fee switch-over timestamp #3907
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
base: orderbook/volume-fees
Are you sure you want to change the base?
Changes from all commits
7be3dee
c44359e
e519dff
5415644
6f8a75b
b08afdf
096ce99
ff799ff
1015c1b
828b7dd
a6cbac6
fd56126
d6c19b6
3a93506
20ef2c6
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -14,6 +14,7 @@ use { | |
| }, | ||
| alloy::primitives::{Address, U256}, | ||
| app_data::Validator, | ||
| chrono::{DateTime, Utc}, | ||
| derive_more::Into, | ||
| ethrpc::alloy::conversions::{IntoAlloy, IntoLegacy}, | ||
| primitive_types::H160, | ||
|
|
@@ -53,25 +54,47 @@ impl From<arguments::FeePolicy> for ProtocolFee { | |
| } | ||
| } | ||
|
|
||
| pub struct UpcomingProtocolFees { | ||
| fee_policies: Vec<ProtocolFee>, | ||
| effective_from_timestamp: DateTime<Utc>, | ||
| } | ||
|
Comment on lines
+57
to
+60
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we handle the upcoming/general protocol fees uniformly, by having the "always present" fees just start at UNIX epoch? This would simplify the approach, although will add timestamp checking to all fees in general. Let's discuss :)
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sorry, I didn't get your comment. Could you rephrase?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The ProtocolFees struct: pub struct ProtocolFees {
fee_policies: Vec<ProtocolFee>,
max_partner_fee: FeeFactor,
upcoming_fee_policies: Option<UpcomingProtocolFees>,
}contains fee_policies that are always applied unless superseded by I was suggesting to unify these two, and instead support the generally applicable and late activated protocol fees in the same way. This would require the fee_policies to be a Vec of UpcomingProtocolFees. But thinking about it the second time, I think it would unnecessarily complicate the solution as currently we support only 1 instance of upcoming protocol fees, whereas the generalization would have to support an arbitrary number of upcoming protocol fees. Then the evaluation would require making sure We apply the correct protocol fee based on the current time. I am happy to approve the PR as-is. |
||
|
|
||
| impl From<arguments::UpcomingFeePolicies> for Option<UpcomingProtocolFees> { | ||
| fn from(value: arguments::UpcomingFeePolicies) -> Self { | ||
| value | ||
| // both config fields must be non-empty | ||
| .effective_from_timestamp | ||
| .filter(|_| !value.fee_policies.is_empty()) | ||
| .map(|effective_from_timestamp| UpcomingProtocolFees { | ||
| fee_policies: value | ||
| .fee_policies | ||
| .into_iter() | ||
| .map(ProtocolFee::from) | ||
| .collect::<Vec<_>>(), | ||
| effective_from_timestamp, | ||
| }) | ||
| } | ||
| } | ||
|
|
||
| pub type ProtocolFeeExemptAddresses = HashSet<H160>; | ||
|
|
||
| pub struct ProtocolFees { | ||
| fee_policies: Vec<ProtocolFee>, | ||
| max_partner_fee: FeeFactor, | ||
| upcoming_fee_policies: Option<UpcomingProtocolFees>, | ||
| } | ||
|
|
||
| impl ProtocolFees { | ||
| pub fn new( | ||
| fee_policies: &[arguments::FeePolicy], | ||
| fee_policy_max_partner_fee: FeeFactor, | ||
| ) -> Self { | ||
| pub fn new(config: &arguments::FeePoliciesConfig) -> Self { | ||
| Self { | ||
| fee_policies: fee_policies | ||
| fee_policies: config | ||
| .fee_policies | ||
| .iter() | ||
| .cloned() | ||
| .map(ProtocolFee::from) | ||
| .collect(), | ||
| max_partner_fee: fee_policy_max_partner_fee, | ||
| max_partner_fee: config.fee_policy_max_partner_fee, | ||
| upcoming_fee_policies: config.upcoming_fee_policies.clone().into(), | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -230,13 +253,22 @@ impl ProtocolFees { | |
| quote: domain::Quote, | ||
| partner_fees: Vec<Policy>, | ||
| ) -> domain::Order { | ||
| let protocol_fees = self | ||
| .fee_policies | ||
| // Use new fee policies if the order creation date is after their effective | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think this is the desired logic. As of the target date, all orders that are settled should be settled according to the new fee policies (as these are what decides the rewards that solvers will receive for these trades0. In particular, a limit order that has been placed in the past should also carry the new fee policy going forward. I'm not sure what the best way to achieve this is, but don't we build the fee policies from scratch on every auction? In this case I'd apply the logic like in the orderbook - as of a certain timestamp we start adding them unconditionally on when the order was placed. |
||
| // timestamp. | ||
| let fee_policies = self | ||
| .upcoming_fee_policies | ||
| .as_ref() | ||
| .filter(|upcoming| order.metadata.creation_date >= upcoming.effective_from_timestamp) | ||
| .map(|upcoming| &upcoming.fee_policies) | ||
| .unwrap_or(&self.fee_policies); | ||
|
|
||
| let protocol_fees = fee_policies | ||
| .iter() | ||
| .filter_map(|fee_policy| Self::protocol_fee_into_policy(&order, "e, fee_policy)) | ||
| .flat_map(|policy| Self::variant_fee_apply(&order, "e, policy)) | ||
| .chain(partner_fees) | ||
| .collect::<Vec<_>>(); | ||
|
|
||
| boundary::order::to_domain(order, protocol_fees, Some(quote)) | ||
| } | ||
|
|
||
|
|
||
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.
nice refactoring!