Modify `FYToken.sol` to be ERC-5095 compliant
This PR is for the ERC-5095 compliant implementation of FYToken.sol. Closes #451
This is a very rough draft of the implementation.
Things to be most likely added/modified:
- Tests (could include the migration of the FYToken tests to Foundry also in this PR)
- We could make an interface or abstract contract for ERC-5095 like the others in
yield-utils-v2 - Logic in
withdrawandredeemwill likely need fixing (The previous logic ofredeemlooks more similar to howwithdrawis implemented in ERC-5095) - Moreover logic between
IFYToken.sol'sredeemand the ERC-5095'sredeemneeds addressing such that the function signatures do not conflict. I'm thinking we could just change the signature inIFYToken.solto be the same as ERC-5095?
Thoughts on these?
@Sabnock01 great first draft!
please take a look at our implementation and try to model it on that: https://github.com/yieldprotocol/ERC5095/pull/1/files esp read some of the comments about reverting etc. u can use the modifier if u want, or inline it, i dont have a strong preference personally.
although, in that version we made redeem/withdraw public fns and the previews were external+internal....but looking at this one, I almost feel like it should be the opposite. or else both external+internal.... lets get everything else working and we can make the call on teh public vs external+internal later
needs tests as you noted. lets get some unit tests in on the next pass
fyi -- i didn't dig deeper or review carefully the amount_.wmul(_accrual()) math -- i just assumed you ported it over correctly
We could make an interface or abstract contract for ERC-5095 like the others in yield-utils-v2
@Sabnock01 what do u mean by this? i'm interested
Moreover logic between IFYToken.sol's redeem and the ERC-5095's redeem needs addressing such that the function signatures do not conflict. I'm thinking we could just change the signature in IFYToken.sol to be the same as ERC-5095?
If we go with my proposed solution (overload redeem()) then we don't need to change this AND we stay backwards compatible...(we would need to add a second redeem() to the interface though)
We could make an interface or abstract contract for ERC-5095 like the others in yield-utils-v2
@Sabnock01 what do u mean by this? i'm interested
Pretty much the exact file of yours you linked to of ERC5095. Similar to ERC20Permit, inherited by FYToken.sol.
But wouldn't that be better fit in yield-utils-v2 similar to ERC20.sol or ERC20Permit.sol?
What we need to put in yield-utils-v2 is an ERC20FlashMintable that inherits from ERC20. Then FYToken would inherit from ERC20Permit, ERC20FlashMintable and IERC5095. The inheritance path might be fun.
I don't think that the 5095 logic belongs in yield-utils-v2, because that repo holds generic and reusable contracts, and for a generic 5095 implementation we already have the reference implementation.
The logic looks solid to me, a few style fixes now, please:
We are mixing the naming conventions from the old fyToken.sol and EIP5095, let's move to the 5095 naming style completely:
to -> receiver
from -> holder
amount -> principalAmount or underlyingAmount
any others?
Also a few functions miss natspec, please add it.