-
Notifications
You must be signed in to change notification settings - Fork 89
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
Define protocol fee params and forward to driver #2098
Merged
Merged
Changes from 10 commits
Commits
Show all changes
25 commits
Select commit
Hold shift + click to select a range
8f96168
Define protocol fee params
sunce86 2ce7191
add comment
sunce86 53d450a
Driver side
sunce86 a999ef1
add protocolFee to tests
sunce86 448d867
float
sunce86 4cd02a2
cr fixes
sunce86 2dd0134
self cr
sunce86 6f26b5c
Merge branch 'main' into protocol-fee-params
sunce86 9c573c6
test fee serde
sunce86 f783d68
Align the fee params with the db change
sunce86 1e79009
Fix arguments
sunce86 5ef6d5d
bool args fix
sunce86 88e0152
fix print
sunce86 b6ff257
fix comments
sunce86 a65d537
fix comments 2
sunce86 1164bf2
Merge branch 'main' into protocol-fee-params
sunce86 484ca6b
cr fixes
sunce86 900aa34
cr fixes 2
sunce86 723a4bf
add default value
sunce86 7557415
fix comment
sunce86 b655bec
rename
sunce86 f3974c2
remove deserialize
sunce86 ac5ddac
camelCase
sunce86 cb3e54f
Merge branch 'main' into protocol-fee-params
sunce86 87743ec
fix tests
sunce86 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,22 @@ | ||
#[derive(Clone, Debug)] | ||
pub enum FeePolicy { | ||
/// Applies to limit orders only. | ||
/// This fee should be taken if the solver provided good enough solution | ||
/// that even after the surplus fee is taken, there is still more | ||
/// surplus left above whatever the user expects [order limit price | ||
/// vs best quote]. | ||
QuoteDeviation { | ||
/// Percentage of the order's `available surplus` should be taken as a | ||
/// protocol fee. | ||
/// | ||
/// `Available surplus` is the difference between the executed_price | ||
/// (adjusted by surplus_fee) and the closer of the two: order | ||
/// limit_price or best_quote. For out-of-market limit orders, | ||
/// order limit price is closer to the executed price. For | ||
/// in-market limit orders, best quote is closer to the executed | ||
/// price. | ||
factor: f64, | ||
/// Cap protocol fee with a percentage of the order's volume. | ||
volume_cap_factor: f64, | ||
}, | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -115,6 +115,19 @@ impl Auction { | |
data: order.signature.into(), | ||
signer: order.owner.into(), | ||
}, | ||
fee_policies: order | ||
.fee_policies | ||
.into_iter() | ||
.map(|policy| match policy { | ||
FeePolicy::QuoteDeviation { | ||
factor, | ||
volume_cap_factor, | ||
} => competition::order::FeePolicy::QuoteDeviation { | ||
factor, | ||
volume_cap_factor, | ||
}, | ||
}) | ||
.collect(), | ||
}) | ||
.collect(), | ||
self.tokens.into_iter().map(|token| { | ||
|
@@ -234,6 +247,7 @@ struct Order { | |
signing_scheme: SigningScheme, | ||
#[serde_as(as = "serialize::Hex")] | ||
signature: Vec<u8>, | ||
fee_policies: Vec<FeePolicy>, | ||
} | ||
|
||
#[derive(Debug, Deserialize)] | ||
|
@@ -287,3 +301,9 @@ enum Class { | |
Limit, | ||
Liquidity, | ||
} | ||
|
||
#[derive(Debug, Deserialize)] | ||
#[serde(rename_all = "camelCase", deny_unknown_fields)] | ||
enum FeePolicy { | ||
QuoteDeviation { factor: f64, volume_cap_factor: f64 }, | ||
} | ||
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. Shouldn't the dto also have documentation (so that it can be generated for the swagger docs)? |
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 would it semantically mean to define multiple entries with the same policy but different parameters? That the protocol has multiple fees for the same order type? This sounds confusing to me.
I don't foresee us using vastly different parameters for in market vs out of market orders (if so we can define them later as two different fields). I therefore think we should just define a struct with
On a per order basis, I think we should then just signal the concrete fee for this order, which can be one of three (maybe 4) in my opinion, potentially chained in case integrators want to charge additional fees.
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 enables us to take a quote deviation fee, and after that another fee as percent of volume (meaning we can chain multiple different fees for a single order). IIUC, this is something we have to support for integrators.
One could argue that we should summarize all of them to a single value and send that single value to the
driver
, but can't do that since not all types of fees are known upfront (volume percent and absolute fee are, quote deviation not).Then, since db migrations are painful, I decided to model the db table for fee polices in a way that supports all foreseeable types of policies and not just for price improvement type. That design was created to be memory efficient and decent performance wise (I could have created a separate table for each fee type but then the fetching would be slower).
Why
Because I expect to add a third type - TWAP orders.
I hope my reasonings are clearer now. Note that this PR is not yet ready for merging, I just need to get the approval for the last commit that my new logic is ok.
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.
Also, I hoped that I could support these two types of fees in the first iteration, since it will be trivial with this design:
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.
My vision for integrators is that they specify their fee policy as part of the app data and it doesn't become part of the arguments we start the core protocol with.
The core protocol will experiment with different fees (price improvements, volume based, etc) but I foresee its fee config to be relatively concise and not a chain of many different fee compositions. Also, the protocol can only charge fees on order types that are a first class citizen in the core protocol (limit orders).
TWAP orders are not part of the core CoW Protocol. They are a set of orders that semantically belong together and are orchestrated and placed from a watch tower, but as far as CoW protocol is concerned users are simply placing limit orders in awkwardly consistent periods.
So my goal here is to split what the protocol boots up with as a fee policy from the different types of protocol fees we can attach to each order that is sent to the solvers.
E.g. the protocol may define that we want to take 0.1% volume fee on all orders. This is what the autopilot should be started with.
If an integrator (e.g. CoW Swap's TWAP feature or Metamask) wants to charge 0.85% volume fee on top, then they can create an order specifying this in the app data and what we end up sending to the driver is two fees (0.1% for the protocol, 0.85% for the integrator), both of which will be charged from solvers if they settle this order at the end. Potentially the protocol will demand a cut (cf. Apple charging 30% of all app store purchases) from the integrator fees.
To sum up, I think we should have a
FeePolicy
struct on the autopilot arguments and the aprotocol_fee: Vec<Fee>
on the order itself.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.
Ok, not using configuration type for autopilot domain makes sense, will split it.
Do you think we should keep the possibility that 2 different policies (for example quote deviation and volume based) can be sent for a single order to the driver, at the same time?
If you don't like having a third enum variant for TWAP orders, how would you model a situation where we set quote deviation policy for limit orders and volume based policy for TWAP orders using:
We talked about not supporting this on slack, if so, I can just add another
whether to include twap orders
flag.