EIP-4626 Withdrawals with a crank #46
Conversation
venables
commented
Oct 7, 2022
•
venables
commented
- Allow lenders to request to redeem (ask for # of shares to convert), or withdraw (ask for an amount of $$)
- Adds support for all EIP-4626 withdraw/redeem methods, as well as the equivalent for withdrawRequest/redeemRequest
- Adds a "light" crank to allocate balances to the right people. This actually takes up about the same amount of gas as the "heavy" crank, but allows for 4626 support.
- Cleaned up the method logic and naming
- Moved most logic to the PoolLib
- Charge a fee when making a withdraw / redeem request
contracts/Pool.sol
Outdated
| @@ -114,6 +125,13 @@ contract Pool is IPool, ERC20 { | |||
| _serviceConfiguration = IServiceConfiguration(serviceConfiguration); | |||
| _firstLossVault = new FirstLossVault(address(this), liquidityAsset); | |||
| _setPoolLifeCycleState(IPoolLifeCycleState.Initialized); | |||
|
|
|||
| // Allow the contract to move infinite amount of vault liquidity assets | |||
| bool approved = _liquidityAsset.approve( | |||
There was a problem hiding this comment.
We need this because we use transferFrom for moving the funds during actual withdrawal right? What's the advantage over using safeTransfer there instead and omitting this?
There was a problem hiding this comment.
I believe we would still need the approve here regardless if using safeTrasnferFrom or not, but I will double check.
contracts/Pool.sol
Outdated
| * is set to 10_000 (100%) | ||
| */ | ||
| function withdrawGate() public view returns (uint256) { | ||
| if (_poolLifeCycleState == IPoolLifeCycleState.Closed) { |
There was a problem hiding this comment.
Nice. Do you think we also need to check ... || now < poolCloseTime?
There was a problem hiding this comment.
Great call -- I've update this to use the method lifeCycleState() which will consider the pool endDate as well. We may need to consider this same change for the modifiers as well.
| function crank() external returns (uint256 redeemableShares) { | ||
| // Calculate the amount available for withdrawal | ||
| // TODO: Find the real available liquidity | ||
| uint256 availableLiquidity = totalSupply(); |
There was a problem hiding this comment.
Would this just be USDC.balanceOf(this), i.e. liquidity reserve?
Edit: to avoid overdrafting the liquidity reserve, I wonder if we instead start with the liquidity reserve, calculate the withdrawal-gate portion, and then convert that to a # of shares?
contracts/Pool.sol
Outdated
| @@ -507,6 +791,9 @@ contract Pool is IPool, ERC20 { | |||
| maxDeposit(receiver), | |||
| _mint | |||
| ); | |||
|
|
|||
| // Approve the contract for infinite tokens. | |||
| approve(address(this), type(uint128).max); | |||
There was a problem hiding this comment.
Is there somewhere else we could put this? Does it need to be called repeatedly?
There was a problem hiding this comment.
Ah yes, now that it's in the constructor, I've removed this.
| onlyLender | ||
| returns (uint256 shares) | ||
| { | ||
| shares = convertToShares(assets); |
There was a problem hiding this comment.
Just so I understand...if you request in assets, we convert that to shares (at an exchange rate at request time), and then store the shares in the various withdrawal states?
There was a problem hiding this comment.
As of now, yes, that's how it works. We store all in requests in shares. We could pretty easily store both separately but it seemed to add a bit of complexity. I don't feel strongly overall, thoughts?
There was a problem hiding this comment.
Cool, just confirming I understood! I think we roll with this for now, and keep that in mind as we / if we tweak?
|
|
||
| _globalWithdrawState = PoolLib.updateWithdrawStateForWithdraw( | ||
| globalState, | ||
| convertToAssets(redeemableShares), |
There was a problem hiding this comment.
Do we need to exclude these earmarked shares / assets from our exchange rate calculations, e.g. convertToShares? In future work, should the assets be moved elsewhere or locked so that they'll remain available for withdrawal?
There was a problem hiding this comment.
Yeah we will have to exclude these -- treating them as essentially withdrawn. I've filed a ticket here that I can use to follow up https://circlepay.atlassian.net/browse/VAL-54
