ERC-7540: Asynchronous ERC-4626 Tokenized Vaults #5457
Conversation
|
DeeJayElly
force-pushed
the
feat/ERC-7540
branch
from
5dd9d86 to
9c8b97d
Compare
…ingDepositRequest/claimableDepositRequest
| function requestDeposit(uint256 assets, address controller, address owner) external returns (uint256 requestId); | ||
|
|
||
| /** | ||
| * @dev Initiates a redeem request. | ||
| * @param shares The amount of shares to redeem. | ||
| * @param controller The address of the controller managing the request. | ||
| * @param owner The owner of the shares. | ||
| * @return requestId The unique identifier for this redeem request. | ||
| */ | ||
| function requestRedeem(uint256 shares, address controller, address owner) external returns (uint256 requestId); | ||
|
|
||
| /** | ||
| * @dev Gets the pending deposit request amount for a given controller and requestId. | ||
| * @param requestId The unique identifier for the request. | ||
| * @param controller The address of the controller. | ||
| * @return assets The amount of assets in the pending state. | ||
| */ | ||
| function pendingDepositRequest(uint256 requestId, address controller) external view returns (uint256 assets); | ||
|
|
||
| /** | ||
| * @dev Gets the claimable deposit request amount for a given controller and requestId. | ||
| * @param requestId The unique identifier for the request. | ||
| * @param controller The address of the controller. | ||
| * @return assets The amount of assets in the claimable state. | ||
| */ | ||
| function claimableDepositRequest(uint256 requestId, address controller) external view returns (uint256 assets); | ||
|
|
||
| /** | ||
| * @dev Gets the pending redeem request amount for a given controller and requestId. | ||
| * @param requestId The unique identifier for the request. | ||
| * @param controller The address of the controller. | ||
| * @return shares The amount of shares in the pending state. | ||
| */ | ||
| function pendingRedeemRequest(uint256 requestId, address controller) external view returns (uint256 shares); | ||
|
|
||
| /** | ||
| * @dev Gets the claimable redeem request amount for a given controller and requestId. | ||
| * @param requestId The unique identifier for the request. | ||
| * @param controller The address of the controller. | ||
| * @return shares The amount of shares in the claimable state. | ||
| */ | ||
| function claimableRedeemRequest(uint256 requestId, address controller) external view returns (uint256 shares); |
There was a problem hiding this comment.
Ordering could be more consistent.
Either:
- requests (deposit + redeem)
- pending (deposit + redeem) request
- claimable (deposit + redeem)
Or
- deposit (request + pending request + claimable)
- redeem (request + pending request + claimable)
| /** | ||
| * @dev Abstract implementation of the ERC-7540 standard, extending ERC-4626. | ||
| */ | ||
| abstract contract ERC7540 is ERC4626, IERC7540, ReentrancyGuard { |
There was a problem hiding this comment.
Is reentrancy guard really necessary? None of the contract in the codebase use it. We always make sure that behaviour is safe without it.
| } | ||
|
|
||
| // Mappings to track pending and claimable requests | ||
| mapping(address => mapping(uint256 => Request)) internal _pendingDepositRequests; |
There was a problem hiding this comment.
All state variables should be private
| address controller, | ||
| address owner | ||
| ) external override nonReentrant returns (uint256 requestId) { | ||
| require(assets > 0, "ERC7540: assets must be greater than zero"); |
There was a problem hiding this comment.
It would be better if asset == 0 did not revert, but instead was a no-op.
(see how ERC-20 transfers of amount 0 are supported)
| address owner | ||
| ) external override nonReentrant returns (uint256 requestId) { | ||
| require(assets > 0, "ERC7540: assets must be greater than zero"); | ||
| require(owner == msg.sender || isOperator(owner, msg.sender), "ERC7540: unauthorized"); |
There was a problem hiding this comment.
Two things here:
- we avoid using
msg.senderand instead use_msgSender()provided byContext. This is necessary for supporting ERC-2771meta-tx. - We avoid using revert strings. Instead, we only use custom errors that can give more context. Please see ERC-6093
| * @dev Internal function to generate a unique request ID. | ||
| */ | ||
| function _generateRequestId(address controller, uint256 input) internal view returns (uint256) { | ||
| return uint256(keccak256(abi.encodePacked(block.timestamp, block.number, msg.sender, controller, input))); |
There was a problem hiding this comment.
Using both block.timestamp and block.number is really a duplication of the same thing. We we ever expect one to change but not the other?
Note: on L2, block.number is more reliable.
Regardless, this function helps with uniqueness, but doesn't provide any real guarantee. While it is more expensive, maybe using a counter would be the better option.
| IERC20(asset()).safeTransferFrom(owner, address(this), assets); | ||
|
|
||
| _pendingDepositRequests[controller][requestId].amount += assets; |
There was a problem hiding this comment.
It feels like we should inverse the two lines and remove nonReentrant. WDYT?
| function requestRedeem( | ||
| uint256 shares, | ||
| address controller, | ||
| address owner | ||
| ) external override nonReentrant returns (uint256 requestId) { | ||
| require(shares > 0, "ERC7540: shares must be greater than zero"); | ||
| require(owner == msg.sender || isOperator(owner, msg.sender), "ERC7540: unauthorized"); | ||
|
|
||
| requestId = _generateRequestId(controller, shares); | ||
|
|
||
| _burn(owner, shares); | ||
|
|
||
| _pendingRedeemRequests[controller][requestId].amount += shares; | ||
|
|
||
| emit RedeemRequest(controller, owner, requestId, msg.sender, shares); | ||
| } |
There was a problem hiding this comment.
There is no external call in this function, why do we need nonReentrant ?
| import {ERC4626} from "../../token/ERC20/extensions/ERC4626.sol"; | ||
|
|
||
| contract ERC7540Mock is ERC7540 { | ||
| constructor(address asset) ERC20("ERC4626Mock", "E4626M") ERC7540(IERC20(asset)) {} |
There was a problem hiding this comment.
for some reason, it seems this is breakin, the transpiler
| constructor(address asset) ERC20("ERC4626Mock", "E4626M") ERC7540(IERC20(asset)) {} | |
| constructor(IERC20 asset) ERC20("ERC4626Mock", "E4626M") ERC7540(asset) {} |
Fixes #????
PR Checklist
npx changeset add)