-
Notifications
You must be signed in to change notification settings - Fork 75
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
Include Full Fee Amount For Limit Orders #792
Conversation
/// the order on chain. This is the fee in full without any subsidies. The | ||
/// fee is denoted in the sell token. | ||
pub full_fee_amount: U256, |
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.
Types are awesome. The fact that this is in the sell token should be reflected by the type IMO. That'd be really nice. But obviously outside the scope of this PR.
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 fact that this is in the sell token should be reflected by the type IMO.
That would be the holy grail! We used to have a graph-based price fining algorithm for the first version of this project (Gnosis Protocol it was called) - and mixing up amounts for different tokens was one of the primary causes of logic bugs.
8dce72d
to
11ae6af
Compare
Fixes #770
This PR makes sure limit orders also set their
full_fee_amount
. Specifically, CoW Protocol orders have 2 different fee fields:fee_amount
: This is the actual fee amount that we charge the user. In order to make trading more attractive on CoW Swap, we have a few "knobs" for configuring subsidies on orders used to compute this amount. For limit orders, this is the analogous tosurplus_fee
which is the actual fee we change the user (but instead of it being a pro-rated fee, it is taken from the order's surplus).full_fee_amount
: This is the actual expected "cost" of executing the order. We store this for a couple of reasons:These fields should be accurate for limit orders now 🎉.
Test Plan
Unit test to check that this field is being set. It gets read in the same way that it gets read for regular orders during auction building.