search

galacticcouncil / HydraDX-simulations

Apache License 2.0
5 stars 10 forks source link

Bug fix fees #103

Closed jepidoptera closed 1 year ago

mrq1911 commented 1 year ago

Task linked: CU-8677ck9e7 fix recalculating fees bug

jepidoptera commented 1 year ago

I am not convinced this will be correct.

Can we add a test to demonstrate that when e.g. LRNA (or some other token like DOT) is sold to the pool for USD, the protocol fee for USD and the asset fee for USD both update? The protocol fee for USD is not used when USD is bought from the pool, but it still must be updated.

I was actually not aware that we have moved away from computing all fees at the beginning of each block, this would be my preference for our implementation currently. It is not correct to only update the fees that are used in a trade, and I think it's confusing to have to update fees that are not used in a trade. I think we should just update all fees once per block, in the Omnipool's update() function, right after updating the oracles.

EDIT: Actually I think we have to update all the fees every block, I don't think we can just update when a trade in the asset in question occurs? Fees are supposed to decay over time if there is no trade in the asset, is that happening? Would be another good thing to test.

So long as we're sticking to dynamic fees, updating in the update function is probably the better way to do it. I had changed it this way because using the update function broke at least one test, test_slip_fees. But if we are using dynamic fees instead of slip fees, that won't matter. I could put it back in the update function and just delete that test.

poliwop commented 1 year ago

I think this is fine, we do not need our Development branch version currently to support slip-based fees (maybe comment out the test instead of deleting?)