Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support calculating fees for atomic multi-TCO2 redemptions #35

Open
0xmichalis opened this issue Jan 15, 2024 · 8 comments
Open

Support calculating fees for atomic multi-TCO2 redemptions #35

0xmichalis opened this issue Jan 15, 2024 · 8 comments

Comments

@0xmichalis
Copy link
Member

0xmichalis commented Jan 15, 2024

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 #28

@kosecki123
Copy link
Contributor

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
Copy link
Member Author

0xmichalis commented Jan 17, 2024

Copying from #36 (review)

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 #35. Otherwise we'll have to add yet another function calculateMultiRedemptionFees() to the interface later like you already drafted in #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
Copy link
Member

aspiers commented Jan 17, 2024

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
Copy link
Member Author

0xmichalis commented Jan 24, 2024

@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
Copy link
Contributor

@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
Copy link
Member Author

@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:

  • redeemOutMany or redeemExactOutputMany (similar to the Uni v4 nomenclature). This function would return as outputs (TCO2s) the exact amounts provided to the function. Currently the fee module should be integrated with this function.
  • redeemInMany or redeemExactInputMany (similar to the Uni v4 nomenclature). This function uses a fixed input (POOL token) to redeem a floating output. It is how we charge fees today for BCT/NCT. The fee module would need to be extended to add support for this, although it's tricky.

@kosecki123
Copy link
Contributor

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

@0xmichalis
Copy link
Member Author

0xmichalis commented Jan 25, 2024

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 #39 for this. PTAL

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants