-
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
Changes from 29 commits
294e25a
c4b6d7a
ccc91dc
c285bb6
54838f1
2c72555
74e7e63
3239d1a
cba3518
b0107a4
46875c0
260b1b8
dfb50cb
7be3dee
c44359e
e519dff
5415644
ea0c5f7
6f8a75b
b08afdf
096ce99
ff799ff
1015c1b
828b7dd
a6cbac6
fd56126
b5502d5
cb199dc
d6c19b6
a123a79
3a93506
20ef2c6
ca13733
45eb434
18a35c6
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.
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.
We don't need to support more than 1 instance of upcoming fees. The idea is the following:
|
||
|
|
||
| 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 | ||
|
||
| // 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!