Conversation
|
hieronx
left a comment
There was a problem hiding this comment.
Really like the overall setup of the contracts!
| /** | ||
| * @dev Performs a transfer in of shares. By default, it takes the shares from the owner. | ||
| * Used by {requestRedeem}. | ||
| * | ||
| * IMPORTANT: If overriding to burn shares immediately (instead of holding them in the vault), | ||
| * you must ALSO override {_fulfillRedeem} to use a snapshotted exchange rate, since the | ||
| * shares will no longer exist in `totalSupply()` at fulfillment time. Simply overriding | ||
| * {_completeSharesIn} to do nothing is not sufficient. | ||
| */ | ||
| function _lockSharesIn(uint256 shares, address owner) internal virtual { | ||
| _update(owner, address(this), shares); | ||
| } | ||
|
|
||
| /// @dev Performs a fulfillment of a redeem request. By default, it burns the shares. Used by {_fulfillRedeem}. | ||
| function _completeSharesIn(uint256 shares, address /* controller */) internal virtual { | ||
| _burn(address(this), shares); | ||
| } |
There was a problem hiding this comment.
This is neat, nice design @ernestognw !
| /// @inheritdoc IERC20Vault | ||
| function asset() public view virtual returns (address); | ||
|
|
There was a problem hiding this comment.
My suggestion would be to just add the share() function here.
It will instantly make all ERC-4626 vaults also ERC-7575 compatible.
| /// @inheritdoc IERC20Vault | |
| function asset() public view virtual returns (address); | |
| /// @inheritdoc IERC20Vault | |
| function asset() public view virtual returns (address); | |
| /// @inheritdoc IERC20Vault | |
| function share() public view virtual returns (address) { | |
| return address(this); | |
| } |
There was a problem hiding this comment.
Yes, this is a good idea. I'm still debating whether it makes sense because the function might suggest a developer that if they override it with another token address, then all the share transfer logic will be externalized, when in reality, they'd need to override other functions.
I'd prefer adding the share() function once we start with 7575
| * Users should only approve operators they fully trust with both their assets and shares. | ||
| * ==== | ||
| */ | ||
| abstract contract ERC7540Operator is ERC165, ERC20Vault, IERC7540Operator { |
There was a problem hiding this comment.
I wonder if you could have a abstract contract OperatorControl which implements setOperator, isOperator, and supportsInterface, which is then used by both ERC7540 as well as ERC6909. The inspiration for this feature set came from ERC6909 and it's almost a one to one match.
There was a problem hiding this comment.
I didn't mean adding full ERC-7741. Rather just moving setOperator, isOperator and supportsInterface to an abstract contract that is reused by ERC7540 and ERC6909 implementations.
| uint256 pendingShares = pendingRedeemRequest(requestId, controller); | ||
| require(shares <= pendingShares, ERC7540RedeemInsufficientPendingShares(shares, pendingShares)); | ||
|
|
||
| uint256 assets = convertToAssets(shares); |
There was a problem hiding this comment.
By always using convertToAssets here, I think you limit the ability to add fees specific to one side, i.e. charging an additional fee on redeem fulfillment that does not apply to deposit fulfillment.
Would it make sense to introduce separate _redeemPrice and _depositPrice internal methods, which by default just call convertToAssets?
There was a problem hiding this comment.
Yes, it does make sense! I didn't consider additional fees on redeem or deposit fulfillment where a common step to override with extra logic. The _redeemPrice and _depositPrice functions give developers a clean function to hook on.
| _completeSharesIn(shares, controller); | ||
| _setClaimableRedeem(controller, claimableAssets + assets, claimableShares + shares); | ||
| _setPendingRedeem(controller, pendingShares - shares); | ||
| return assets; |
There was a problem hiding this comment.
In the Centrifuge implementation we added DepositClaimable and RedeemClaimable events (https://github.com/centrifuge/protocol/blob/882b984111150b664e7a1982f80e0975ecc1975a/src/vaults/interfaces/IAsyncVault.sol#L41). These aren't standardized because not all implementations can emit these on claimable transition, but I think they are very valuable for offchain indexers, so worth considering to add.
There was a problem hiding this comment.
I was hesitant on adding an event here because _completeSharesIn (in redeem) and _mint (in deposit) would emit an event themselves and it felt redundant. I guess the DepositClaimable and RedeemClaimable events allow indexing the operator and request id. Is that its main purpose?
To be clear, I like the idea of using both events as in Centrifuge's implementation, but I'm curious on your thoughts about emitting more than 1 event in this function.
There was a problem hiding this comment.
You mean Transfer(from=..., to=address(0)) in _completeSharesIn (in redeem)?
I personally think that's quite opaque. There could be other reasons for burning shares in the implementing contract. So would add a specific event for the fulfillment/claimable state of shares.
| /// @inheritdoc IERC4626 | ||
| function previewWithdraw(uint256 /* assets */) public view virtual returns (uint256) { | ||
| revert ERC7540PreviewNotAvailable(); | ||
| } | ||
|
|
||
| /// @inheritdoc IERC4626 | ||
| function previewRedeem(uint256 /* shares */) public view virtual returns (uint256) { | ||
| revert ERC7540PreviewNotAvailable(); | ||
| } |
There was a problem hiding this comment.
Perhaps these should go in ERC7540Redeem instead of here
| function previewDeposit(uint256 /* assets */) public view virtual returns (uint256) { | ||
| revert ERC7540PreviewNotAvailable(); | ||
| } | ||
|
|
||
| /// @inheritdoc IERC4626 | ||
| function previewMint(uint256 /* shares */) public view virtual returns (uint256) { | ||
| revert ERC7540PreviewNotAvailable(); | ||
| } |
There was a problem hiding this comment.
And these in ERC7540Deposit
contracts/interfaces/IERC4626.sol
Outdated
| * - MUST return as close to and no fewer than the exact amount of assets that would be deposited in a mint call | ||
| * * MUST return as close to and no fewer than the exact amount of assets that would be deposited in a mint call |
There was a problem hiding this comment.
The - bullets don't render in the docs, but * do. I just updated accordingly but it's not strictly needed for ERC-7540
There was a problem hiding this comment.
The - generally renders just fine in our system?
There was a problem hiding this comment.
True. After we migrated to the new engine it works. This was true for AsciiDoc
Fixes #4761
PR Checklist
npx changeset add)