Protocol fee adjustment in driver #2213
Conversation
sunce86
added
the
E:4.2 Protocol Fee Implementation
There was a problem hiding this comment.
I don't feel comfortable adding this large chunk of core logic right into the existing fulfilment constructor.
Can we create a fee specific domain module (domain/competition/solution/fee.rs) and add an extension impl Fulfilment there which exposes an apply_fee(self, ...) -> Self that gets triggered from the solution (either upon constructing or before being encoded)?
This module can then encapsulate all fee related logic.
As for testing, I think one nice side effect of the examples you worked on with Olga and Anxo is that they can serve as ideal e2e tests, or at least driver integration tests, which is how we want to test core functionality going forward (instead of unit tests).
Tried this already but disliked the fact that the |
|
Refactored, requesting review again. |
There was a problem hiding this comment.
Thanks, I like this approach much more. I still think we can factor the code inside fee.rs to make it easier to read and follow but it's mainly style preference.
I don't think the error handling is correct yet, and overflow handling for additions may be missing.
There was a problem hiding this comment.
Code looks good to me (just two small issues with static orders and multiple fee policies).
How do you want to go about adding tests? Are we going to keep the unit tests? In any case, I'd like to see a driver or integration test with the two examples we have in notion to make sure this works without coupling the test to the concrete code factoring.
|
|
||
| fn protocol_fee(&self, prices: ClearingPrices) -> Result<eth::U256, Error> { | ||
| let mut protocol_fee = eth::U256::zero(); | ||
| for fee_policy in self.order().fee_policies.iter() { |
There was a problem hiding this comment.
Probably not an issue for now, but I wonder if the current logic would work for multiple fee policies (e.g. two 50% price improvements). I would think those would be applied sequentially (ie. we first get a fulfillment from applying the first fee - with less surplus! - and then only take 50% of the remaining surplus rather than 100%).
My suggestion would be to expose another apply method on a local impl extension of the FeePolicy enum, that takes a fulfilment and prices and returns a new fulfilment with its policy applied.
But we can also cross that bridge when we get to it, in this case we should probably document and guard with an error that multiple fee policies aren't yet properly implemented.
There was a problem hiding this comment.
I need to think more if we should allow for multiple policies of the same kind. Does it make sense to have two price improvement policies on the same order, where the calculated fee amounts depend on the ordering of policies.
Would you expect two policies, one volume based, one price improvement, to also be sequentially applied?
I'm not going to keep the unit tests (they just show that the calculations are correct and match with the current surplus calculation we have, for example). They will exist in the PR history so we can reproduce them again in the future if needed. I planned on adding e2e test when everything is done. Do you think it should be part of this PR? Considering that we send no policies at the moment, merging this PR is safe. |
There was a problem hiding this comment.
🙈 I don't think the current test setup lends itself nicely for this new feature and also needs some refactoring. My goal is that we can put the tests we have in Notion in code (and not come up with completely new examples that way fewer people have thought through and agreed upon)
|
E2E tests are extracted into a separate PR: |
| let surplus = limit_sell_amount | ||
| .checked_sub(executed_sell_amount_with_fee) | ||
| .unwrap_or(eth::U256::zero()); |
There was a problem hiding this comment.
If this is used for the surplus based fee we need to reference the quoted price somehow.
Otherwise we'll only compare against the signed limit price which already has the slippage baked in. This is not what we want, right?
There was a problem hiding this comment.
The current implementation only works for out of market limit orders (where the quote is worse than the limit price). It's true that the naming is a bit misleading and the real fee policy we are implementing here is is SurplusFee not PriceImprovement
There was a problem hiding this comment.
I see the PriceImprovement as a general term - improvement over SOME price, whether it's a limit price or some other artificial price (artificial because driver works with limit orders technically and doesn't care whether it has slippage baked in and in which amount).
If it were called MarketPriceImprovement then it would be misleading IMO. Having a general term is better since we will implement the improvement over quotes 99.99% so renaming it would lead to renaming the autopilot/driver interface which is not too problematic right now, but it could be in the future.
OTOH, I might be biased since I spent a lot of time thinking in those terms, so if you both prefer to have SurplusFee I will update it.
There was a problem hiding this comment.
I've captured the potential renaming #2266
I believe this is ok since chances are higher that we will implement this so it's easier to close the issue than to revert the changes.
There was a problem hiding this comment.
It is true that it is quite inconsistent that we implement a "price improvement fee" but cannot actually take it from the price improvement (since we don't know the quotes).
I'm not sure if it's worthwhile renaming the FeePolicy type to Surplus for now which would be correct, since technically your slippage tolerance is part of the surplus.
Functionality wise it doesn't matter since it's only planned to apply to limit orders for now, but I wonder if we start creating a cognitive mess by introducing this inconsistency (which may or may not be cleaned up one day). In any case, there may be merit in having a general "surplus" fee policy on top of price improvement and volume.
| let surplus = limit_sell_amount | ||
| .checked_sub(executed_sell_amount_with_fee) | ||
| .unwrap_or(eth::U256::zero()); |
There was a problem hiding this comment.
The current implementation only works for out of market limit orders (where the quote is worse than the limit price). It's true that the naming is a bit misleading and the real fee policy we are implementing here is is SurplusFee not PriceImprovement
There was a problem hiding this comment.
Only nits remaining for me.
Approving since both sides of the renaming discussion make sense to me and I don't have strong feelings about it.
Just wanted to point it out in case this was not intentional since I remembered the plan differently. But if this is just an intermediate PR until we have all the data needed for actually taking fees over price improvement compared to quoted amounts I'm fine with it.
# Description Merges to #2213, not main. Adds e2e tests for protocol fee calculation. - Sell order - Sell order, but protocol fee capped by max volume - Buy order - Buy order, but protocol fee capped by max volume Asserts that if `executed_surplus_fee = X` without protocol fees, then `executed_surplus_fee = X + protocol_fee` with protocol fees.
Description
Replacement PR for #2097
As @fleupold suggested, protocol fee adjustments are done fully in
driver, in classFulfillment. It goes like this:(note that this is basically the same as increasing
surplus_fee)SettlementEncoderfunctionality, meaning custom price adjustments will be reused for protocol fee also.How to test
Added unit tests for
(but now removed to keep the domain clean)