-
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?
Conversation
919126f to
5415644
Compare
# Conflicts: # crates/orderbook/src/arguments.rs
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.
Pull Request Overview
This PR introduces timestamp-based configuration for volume fee policies to ensure synchronization between the orderbook and autopilot services. The changes enable both services to start applying volume fees at the same configured time, preventing users from receiving incorrect quotes.
- Added optional
effective_from_timestampto volume fee configuration in orderbook - Introduced "upcoming" fee policies configuration in autopilot with effective timestamp
- Both services now check timestamps before applying volume fees
Reviewed Changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| crates/orderbook/src/run.rs | Updated to pass the new volume_fee_config structure instead of simple fee factor |
| crates/orderbook/src/quoter.rs | Added timestamp checking logic to only apply volume fees when effective timestamp is reached, updated tests |
| crates/orderbook/src/arguments.rs | Refactored volume fee configuration into a VolumeFeeConfig struct with optional effective_from_timestamp field |
| crates/e2e/tests/e2e/quoting.rs | Updated test to use new command-line arguments with future timestamp to verify fee is not applied |
| crates/e2e/tests/e2e/protocol_fee.rs | Added new test for upcoming future fee policies and updated existing test to use new configuration structure |
| crates/e2e/tests/e2e/limit_orders.rs | Migrated from to_string() to into_args() method for protocol fee configuration |
| crates/e2e/src/setup/fee.rs | Restructured ProtocolFeesConfig to support upcoming fee policies and converted Display implementation to into_args() method |
| crates/autopilot/src/run.rs | Updated to pass the full fee policies config structure instead of individual parameters |
| crates/autopilot/src/domain/fee/mod.rs | Added logic to select between current and upcoming fee policies based on order creation timestamp |
| crates/autopilot/src/arguments.rs | Introduced FeePoliciesConfig and UpcomingFeePolicies structures to support timestamp-based fee policy switching |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
# Conflicts: # crates/e2e/tests/e2e/quoting.rs # crates/orderbook/src/arguments.rs # crates/orderbook/src/run.rs
m-sz
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.
Thanks for extensively covering the change by well documented E2E tests!
|
|
||
| let order = services.get_order(&uid).await.unwrap(); | ||
| let fee_in_buy_token = quote.fee_amount * quote.buy_amount / quote.sell_amount; | ||
| assert!(order.metadata.executed_fee >= fee_in_buy_token + quote.sell_amount / 10); |
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.
The 10 comes from effective Volume Fee Policy set to 0.1 right?
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.
This test is just a copy of the previous buy order, with a single tiny configuration difference.
| pub struct UpcomingProtocolFees { | ||
| fee_policies: Vec<ProtocolFee>, | ||
| effective_from_timestamp: DateTime<Utc>, | ||
| } |
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.
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 :)
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.
Sorry, I didn't get your comment. Could you rephrase?
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.
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 upcoming_fee_policies which are essentially the same, but with an activation timestamp.
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.
Do you mean after the configured upcoming fee policy? |
Yes, my bad. |
m-sz
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
| pub struct UpcomingProtocolFees { | ||
| fee_policies: Vec<ProtocolFee>, | ||
| effective_from_timestamp: DateTime<Utc>, | ||
| } |
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.
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 upcoming_fee_policies which are essentially the same, but with an activation timestamp.
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.
fleupold
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.
Overall the PR looks good (thanks for adding a test!). I think the semantics we want though is that even orders that are already in the orderbook should start carrying the new fee policies.
which should be useful once switched to a long-lasting config.
I would hope we remove this config once it's permanent.
| #[clap(long, env, default_value = "0.01")] | ||
| pub fee_policy_max_partner_fee: FeeFactor, | ||
| #[clap(flatten)] | ||
| pub fee_policies_config: FeePoliciesConfig, |
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!
| ) -> domain::Order { | ||
| let protocol_fees = self | ||
| .fee_policies | ||
| // Use new fee policies if the order creation date is after their effective |
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 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.
| } | ||
|
|
||
| #[test] | ||
| fn test_ignore_volume_fees_before_effective_date() { |
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.
Should it also test the other case so we know the timestamp flow works?
Description
#3900 introduced a way to show the volume fees to users in advance via quotes. Autopilot should start applying the same policies simultaneously to avoid situations where the user receives an incorrect quote. This PR introduces timestamp-based configs in both orderbook and autopilot crates that control when the volume fee should start applying.
For the orderbook, it is pretty straightforward, which adds an optional timestamp param to the existing volume fee factor config, and each time the service tries to apply volume fees, it checks for the current time. If the timestamp config is None, it means volume fees are applied unconditionally, which should be useful once switched to a long-lasting config.
The autopilot config is a bit more sophisticated. It introduces a separate "upcoming" fee policies config with the effective "from" timestamp. So, if another order's creation timestamp is after the configured upcoming fee policy timestamp, the service starts using this fee policy. This is useful because the volume fee factor affects price improvement and surplus fee policy caps, so each time the volume fee factor is updated, other fee policy configs need to be adjusted accordingly, so we need to switch to the new fee policies set altogether. The config is also optional and can be easily switched to permanent.
The major disadvantage of this approach is that orderbook and autopilot use configs from different sources. A more correct approach would be to use a shared config via a DB or similar, but this would require many more changes, and we should probably avoid any mistakes by making deeper reviews.
How to test
New e2e tests.