Domain autopilot fee #2701
Domain autopilot fee #2701
Conversation
| let bought = self | ||
| .executed | ||
| .0 | ||
| .checked_mul(prices.sell) | ||
| .ok_or(error::Math::Overflow)? | ||
| .checked_ceil_div(&prices.buy) | ||
| .ok_or(error::Math::DivisionByZero)?; |
There was a problem hiding this comment.
Being able to convert amounts from one side of the trade to another side of the trade (e.g. some amount in buy tokens to that amount in sell tokens) seems to be quite a common operation here (I see it at least 3 times). Maybe we can expose this in a more easy to use way that can be shared?
There was a problem hiding this comment.
Tried but not happy with what I ended up with, particularly because types are not identical in these code repetitions.
| order::Side::Sell => factor / (1.0 - factor), | ||
| order::Side::Buy => factor / (1.0 + factor), |
There was a problem hiding this comment.
I know this isn't possible in the current code path, but should we be defensive here and return math errors in case we divide by 0 (we just had a panic in prod this week)
There was a problem hiding this comment.
it would result in f64::NaN and be handled without panic. https://users.rust-lang.org/t/how-should-i-handle-f64-division-by-zero-panic-elegantly/32485/2
Unless you are worried we might in the future add some more code right after this line which might have assumptions that f64 is not nan?
There was a problem hiding this comment.
In this case apply_factor will return 0 if I'm not mistake (from_f64_lossy for NaN yields 0), which is wrong (volume fee should be large in that case not 0) and going to be gnarly to debug. Why can't we simply return a Math::DivisionByZero error in this case (which intuitively is the correct thing to do here)?
| .amount | ||
| .checked_sub(&self.surplus_over_limit_price()?.amount) | ||
| .ok_or(error::Math::Negative)?; | ||
| // We don't have to convert the fee to the sell token, we can just use the fee |
There was a problem hiding this comment.
This comment only applies to the order::Side::Buy case, right? Because in the other case we compute the fee in buy token.
There was a problem hiding this comment.
Yes. But the comment stays above the whole line since if we want to drop this conversion, then we remove the whole statement.
Yeah, that would be the ultimate idea. Extracting surplus/fee/score calculation on the encoded format of the settlement can be definitely shared but requires non-trivial effort. Might be a good task for an external contributor to tackle though. |
Description
Part 5 of #2436
Adds protocol fee calculation.
Adds native fee calculation (total fee = protocol fee + network fee).
Adds score calculation.
Most of the code is taken from the
driverwith some refactoring since here we need to calculate the native fee (total fee) as a surplus difference.How to test
Updated unit test