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

[LILA-6145] Consolidate interface functions #36

Merged
merged 1 commit into from
Jan 17, 2024
Merged

Conversation

0xmichalis
Copy link
Member

@0xmichalis 0xmichalis commented Jan 15, 2024

Consolidate the fee module interface functions
into a single function per action as originally
developed.

This consolidation should:

  • make it easier for pools to integrate with the
    fee module since there is only a single function
    per action to use
  • make the interface future-proof in case fee
    modules in the future are determining fee recipients
    based on the TCO2 that is deposited to or redeemed
    from the pool
  • make the fee calculation more performant since
    there is only a single CALL to the fee module
    instead of two per action

This change reverts neutral-protocol@99f19a4

@0xmichalis 0xmichalis force-pushed the interface-update branch 3 times, most recently from 3a8aaf7 to 2199e61 Compare January 15, 2024 18:35
@0xmichalis 0xmichalis marked this pull request as ready for review January 15, 2024 18:39
@0xmichalis 0xmichalis changed the title Consolidate interface functions [LILA-6145] Consolidate interface functions Jan 15, 2024
@0xmichalis 0xmichalis marked this pull request as draft January 16, 2024 12:15
src/FeeCalculator.sol Outdated Show resolved Hide resolved
Copy link
Member

@aspiers aspiers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, bearing in mind my comments. There seems to be quite a lot of duplicated code in the tests but maybe there is no good way around that.

@0xmichalis 0xmichalis marked this pull request as ready for review January 17, 2024 10:14
@0xmichalis 0xmichalis requested a review from aspiers January 17, 2024 10:14
Consolidate the fee module interface functions
into a single function per action as originally
developed.

This consolidation should:
* make it easier for pools to integrate with the
fee module since there is only a single function
per action to use
* make the interface future-proof in case fee
modules in the future are determining fee recipients
based on the TCO2 that is deposited to or redeemed
from the pool
* make the fee calculation more performant since
there is only a single CALL to the fee module
instead of two per action

This change reverts 99f19a4
Copy link
Member

@aspiers aspiers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM (I didn't review the tests though).

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).

@0xmichalis
Copy link
Member Author

@aspiers not sure this question needs to be answered in this PR, let's move this discussion in #35.

@0xmichalis 0xmichalis merged commit 0a44deb into main Jan 17, 2024
1 check passed
@0xmichalis 0xmichalis deleted the interface-update branch January 17, 2024 14:35
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

Successfully merging this pull request may close these issues.

2 participants