ToucanProtocol / dynamic-fee-pools

Fee calculator contracts for Toucan Protocol pools
2 stars 3 forks source link

Support calculating fees for atomic multi-TCO2 redemptions #35

Open 0xmichalis opened 10 months ago

0xmichalis commented 10 months ago

Currently if a client tries to estimate the fees to be charged during a multi-TCO2 redemption they will only be able to calculate the sum of the fees charged if any of the underlying TCO2 redemptions were done in isolation. This sounds incorrect though since the user should be charged based on the resulting composition of the pool after the multi-redemption and not based on the sum of all redemptions done in isolation?

Previous attempt at this was https://github.com/toucanprotocol/dynamic-fee-pools/pull/28

kosecki123 commented 10 months ago

This is interesting remark. Because each redemption / deposit is changing the composition the fee for next redeption / deposit for n+1 asset will change as well. We will look at this more thoroughly.

0xmichalis commented 10 months ago

Copying from https://github.com/toucanprotocol/dynamic-fee-pools/pull/36#pullrequestreview-1827410767

Only question I have is whether we should change the signature of calculateRedemptionFees() to (address pool, address[] tco2s, uint256[] redemptionAmount) in anticipation of adding support for redeemMany() later as per https://github.com/toucanprotocol/dynamic-fee-pools/issues/35. Otherwise we'll have to add yet another function calculateMultiRedemptionFees() to the interface later like you already drafted in https://github.com/toucanprotocol/dynamic-fee-pools/pull/28, and then have to either maintain both long term, or migrate away from the one in this PR. And that seems a bit ugly because we probably don't need both, unless there is an argument that it makes things like getting fee quotes directly via polygonscan are easier when you don't need to use arrays. But even then, a simple wrapper function could be included in the fee module without including it in the interface.

If we did this, it wouldn't matter that we don't yet have fee calculation working correctly for multiple TCO2s in one go; for now we can make this contract simply revert if tco2s.length != 1 (and CHAR should do the same in redeemMany() anyway).

aspiers commented 10 months ago

As mentioned in https://github.com/ToucanProtocol/tokenizer/pull/3210#discussion_r1456078288 I wonder if it makes sense to do this change ASAP, since it avoids another change to Pool.sol in the near future.

0xmichalis commented 9 months ago

@aspiers I just realized today as part of looking into the interface refactoring that one question we'd need to answer is how to calculate the TCO2 amounts to redeem for the individual redemptions. Today we calculate the fee to be charged based on the provided amount, then deduct the fee from the amount and return amount-fee to the user (see here).

If we supported the alternative option where the provided amounts are exactly what the user will be receiving, then it'd be simpler because we would only need to require them to pay sum(amounts)+fee pool tokens. I guess an option here would be to calculate the overall fee then deduct it proportionally from the amounts to be redeemed. I suspect this will be a bit involved though and may result in precision loss.

kosecki123 commented 9 months ago

@0xmichalis @aspiers understand you approach, we also tried to implement this such that for given amount of pool token we return the amount of tco2.

Given the assumption that fee is denominated in pool token and calculated based on a change of the ratio for chosen tco2, I'm not sure if there is a analytical way to solve this.

Let's try to walk this through, let assume that we have 10 POOL tokens and we want to know how much of selected tco2 we get. Part of the 10 POOL tokens will be a fee and rest will be used for redemption, given we don't know yet the fee, not sure we can calc this.

The proposed solution was done by implementing it with the assumption that we start with TCO2 you wish to get and we return the quote in POOL.

To workaround this one would need to find the optimal solution by providing the TCO2 number until you get the closest POOL amount.

0xmichalis commented 9 months ago

@kosecki123 @aspiers we should probably create a new issue to track this discussion as it's different from the multi-TCO2 support. Currently the fee module supports "I have X TCO2, how much POOL do I get?" and we have incorrectly integrated this in the pool (I have a WIP PR internally to change this). I am extending the pool code to support both flows:

kosecki123 commented 9 months ago

Forking discussion on POOL -> TCO2 to separate issue #38

0xmichalis commented 9 months ago

As mentioned in ToucanProtocol/tokenizer#3210 (comment) I wonder if it makes sense to do this change ASAP, since it avoids another change to Pool.sol in the near future.

Opened https://github.com/toucanprotocol/dynamic-fee-pools/pull/39 for this. PTAL