-
Notifications
You must be signed in to change notification settings - Fork 3
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
Conversation
3a8aaf7
to
2199e61
Compare
There was a problem hiding this 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.
2199e61
to
9b2dec8
Compare
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
9b2dec8
to
149ebf0
Compare
There was a problem hiding this 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).
Consolidate the fee module interface functions
into a single function per action as originally
developed.
This consolidation should:
fee module since there is only a single function
per action to use
modules in the future are determining fee recipients
based on the TCO2 that is deposited to or redeemed
from the pool
there is only a single CALL to the fee module
instead of two per action
This change reverts neutral-protocol@99f19a4