Compute partner fees #52
Conversation
| @@ -21,17 +21,39 @@ | |||
| class Trade: | |||
| """Class for""" | |||
|
|
|||
| order_uid: HexBytes | |||
There was a problem hiding this comment.
This code now deviates sufficiently from the code in the circuit breaker to require actual tests.
| sell_token_clearing_price: int | ||
| buy_token_clearing_price: int | ||
| fee_policies: list["FeePolicy"] | ||
| def __init__( |
There was a problem hiding this comment.
a __post_init__ method could be used instead to set the remaining fields which require computation.
Co-authored-by: Felix Henneke <[email protected]>
src/fees/compute_fees.py
Outdated
| i = 0 | ||
| self.protocol_fee = 0 | ||
| self.partner_fee = 0 | ||
| if self.fee_policies: |
There was a problem hiding this comment.
Is this line required? If not, it should be removed. If yes, a small comment might help understand why it is needed.
There was a problem hiding this comment.
There is line 98 that needs to execute after the for-loop finishes, but it only makes sense if there was at least one fee policy.
There was a problem hiding this comment.
Oh, i see. But to me it is simpler to write
partner_fee = 0
for i, fee_policy in enumerate(reversed(self.fee_policies)):
raw_trade = fee_policy.reverse_protocol_fee(raw_trade)
## we assume that partner fee is the last to be applied
if i == 0 and self.partner_fee_recipient is not NULL_ADDRESS:
partner_fee = raw_trade.surplus() - self.surplus()
total_protocol_fee = raw_trade.surplus() - self.surplus()The other two initializations are not required.
That part of the code might even benefit from a function
def partner_fee(self):
if not self.fee_policies or self.partner_fee_recipient is NULL_ADDRESS:
return 0
# we assume that partner fee is the last to be applied
fee_policy = self.fee_policies[-1]
raw_trade = deepcopy(self)
raw_trade = fee_policy.reverse_protocol_fee(raw_trade)
return raw_trade.surplus() - self.surplus()and then just
partner_fee = self.partner_fee()
total_protocol_fee = self.total_protocol_fee()The small loss in efficiency should not matter compared to the increase in readability. (Though you might disagree on the readability, then feel free to ignore).
There was a problem hiding this comment.
Actually you are right and my concern was not really justified. I simplified the code and followed your first suggestion.
| for trade in settlement_data.trades: | ||
| # protocol fees | ||
| protocol_fee_amount = trade.protocol_fee() | ||
| protocol_fee_amount = trade.total_protocol_fee - trade.partner_fee |
There was a problem hiding this comment.
Note that this also breaks the convention (maybe we should!) with what we upload to Dune.
There was a problem hiding this comment.
Agreed but somehow it makes sense to have a table where when we look for a protocol fee, this is indeed the protocol fee and not something that potentially includes other fees there as well. So i would like to keep this here to be as explicit as possible in the db, I.e., have:
- network fee in sell token
- protocol fee in surplus token
- partner fee, if any, in surplus token
And there are no subtractions or anything like that. Protocol fee goes to the DAO, partner fee goes to the partner, and network fee to solvers, and that's it.
There was a problem hiding this comment.
I agree that the convention on Dune is a bit misleading. Maybe we should switch it there as well. I am not familiar with the legal accounting requirements, though. Partner fees might be revenue with an associated cost.
Regarding partner fees going to partners: unfortunately that is not so easy as we take a cut. But generally I agree with this distinction. I expect the protocol fee mess to become even more ugly in the future, so having a reasonable intermediate representation sound most sustainable.
In terms of consistency with backend: the planned changes might include protocol fees per fee policy. So it is even one level more fine grained in case we go crazy with stacking protocol fees (which I expect to happen).
| def compute_all_fees_of_batch( | ||
| tx_hash: HexBytes, | ||
| ) -> tuple[ | ||
| dict[str, tuple[str, int]], | ||
| dict[str, tuple[str, int, str]], | ||
| dict[str, tuple[str, int]], | ||
| ]: | ||
| orderbook_api = OrderbookFetcher() | ||
| settlement_data = orderbook_api.get_all_data(tx_hash) |
There was a problem hiding this comment.
Combining the functions compute_fee_imbalances and batch_fee_imbalances entangles data fetching and fee computations. This makes it more difficult to to test the new convention for protocol fees.
There was a problem hiding this comment.
All major issues have been addressed, I think.
The one thing which still is worrying to me is that there are no tests and the code has diverged quite a bit from the circuit breaker code.
I would say that the minimum would be to have an end-to-end tests for a single hash where we know the correct values. Those values are then just hard coded in the test. This requires to check single example (like was done multiple times already I suspect). It would also detect unexpected changes, should we change some of the fee code.
|
New dependencies detected. Learn more about Socket for GitHub ↗︎
|
|
Added a test which might be useful. Merging for now as i would like to test this PR through the weekend. |
This PR aims to distinguish between partner and protocol fees, instead of labeling everything as protocol fees.