Include fees + File for checking final ETH slippage for single tx hash #31
Conversation
| return current_quote_sell_amount - self.sell_amount | ||
| raise ValueError(f"Order kind {self.kind} is invalid.") | ||
|
|
||
| def compute_surplus_fee(self) -> int: |
There was a problem hiding this comment.
Although surplus_fee is the term the backend uses, i feel it would be clearer if we just name it total_fee.
cc @fhenneke
There was a problem hiding this comment.
Using total_fee is fine as well. But I would object that the total fee is a clear concept in our context. But adding more documentation might resolve that.
surplus_fee is nice because it is clear that it is a quantity expressed in surplus. What the backend did with that by converting back to sell tokens is a bit unintuitive. The meaning in this code will probably clash with what backend does.
Maybe surplus_reduction or something similar might work.
There was a problem hiding this comment.
let's add at least some comment to clarify what this is computing
| sell_token_clearing_price: int | ||
| buy_token_clearing_price: int | ||
|
|
||
| def volume(self) -> int: |
There was a problem hiding this comment.
@fhenneke is the naming of the function accurate? In case there is a protocol fee, this is not really the volume but rather the remaining amount after the protocol fee has been subtracted
There was a problem hiding this comment.
That is the definition of volume I use. The same as for surplus. It is not the hypothetical "what would volume have been if there were no fee?" but rather "what is the volume?".
We could have another raw_volume function but that would not be used for anything. Or is there a use case?
There was a problem hiding this comment.
I guess one issue i have is that if we talk about, e.g., an order having 1% volume fee, this volume we are talking about here is not the one we should check in order to compute that fee.
There was a problem hiding this comment.
This is exactly the same issue as for surplus, is it not?
Because of that we always have to use modified factors in computing protocol fees in the autopilot, e.g. here.
There was a problem hiding this comment.
I would argue that surplus is mainly from a user point of view so defining surplus for a sell order as executed_buy_amount - limit_price makes sense to me. I.e., since we explicitly say that score is surplus plus protocol fees, referring to surplus as the portion the user receives seems fine.
But i haven't thought too much about it, tbh.
There was a problem hiding this comment.
To me it is perfectly consistent to use this very function for computing volume. This is the volume (in the surplus token) of the order. Dune uses that number for volume as well in some cases (if there is no price for the other token).
We need exactly this function to compute protocol fees. In principle it could be renamed. But I am in favor of keeping the name.
Using a different definition is not possible unless a volume factor is added as parameter
def volume_before_fees(self, volume_factor):
if self.kind == "sell":
return self.buy_amount / (1 - volume_factor)
if self.kind == "buy":
return self.sell_amount / (1 + volume_factor)So the volume would then depend on which fees are applied. It would not be a property of the trade itself. Might be a helpful function.
src/fees/compute_fees.py
Outdated
| # if __name__ == "__main__": | ||
| # tx_hash = HexBytes( | ||
| # "0xbd8cf4a21ad811cc3b9e49cff5e95563c3c2651b0ea41e0f8a7987818205c984" | ||
| # ) | ||
| # protocol_fees, network_fees = batch_fee_imbalances(tx_hash) | ||
| # print(protocol_fees) |
|
If you are confident this PR works, i would suggest merging it so that we start getting some fee calculations, as it is non-trivial to review and there are a lot of dependencies here and there. We can work towards cleaning it up at some other point, with the following in mind:
|
|
Attempted to fix the mypy errors but it seems not a one-line change so reverted and will wait for @shubhagarwal03 to take care of these |
Regarding the fee computation, I am pretty confident it works. It is using the code from the circuit breaker which has proven to be correct. The main change from the circuit breaker is the addition of network fees. Here, the implementation looks a lot cleaner than what we used before. If that part looks reasonable to you, the computations should be fine. The data fetching could potentially be an issue, depending on the node setup used. The circuit breaker has problems with that at the moment. But token imbalances are processed with some delay, right? Then there would be no problem. |
|
Ok sorry for the mess. Now my changes are properly reverted |
|
I am merging although not confident if this works, but we can address issues in a follow-up PR |
As the title suggests, this PR incorporates protocol and network fees which are also being written to a table in solver_slippage DB called "fees".
Still need to discuss how we want to calculate the net slippage, e.g. should it be written to its own table? If not, pull data from all other tables at the end of a period and calculate the slippage using price feeds?
Since this is unknown and we don't have a currently running daemon for latest data, I've temporarily added a file
test_single_hash.pyto output all data that is used throughout the steps of finding net slippage, i.e. raw token imbalances, protocol fees, network fees, raw slippage, and final slippage in ETH using price feed.I've found it useful for debugging/cross-checking.