Bugfix/withdrawal manager#219
Conversation
| EnumerableSet.Bytes32Set private activeWithdrawalRequests; | ||
| mapping(address => EnumerableSet.Bytes32Set) private userWithdrawalRequestsSet; | ||
|
|
||
| struct RebasingTokenConfig { |
There was a problem hiding this comment.
Hey since EL tracks rebasing already, can we use their functions underlyingToSharesView and sharesToUnderlyingView to do the rebasing calc?
With that method, we remove all the code overhead of rebasing including all the admin management. Also all if(rebasingToken) can be removed since we can just call the functions for any strategy and if it is not rebasing, it will simply return 1 * 10 ** decimals
There was a problem hiding this comment.
We would then need to update queuedAssetBalances -> queuedAssetElShares
This actually also helps with accurate pricing of the LAT because in totalAssets calc, we can utilize the actual rebased value of the token
| mapping(bytes32 => ILiquidTokenManager.Redemption) public redemptions; | ||
|
|
||
| uint256 public withdrawalDelay; | ||
| mapping(bytes32 => uint256[]) public withdrawalRequestShares; |
There was a problem hiding this comment.
I'm thinking do we need to keep this as a separate mapping? What if we update the Redemption struct to store requestedElShares & withdrawableElShares instead of requestedAmounts and withdrawableAmounts. Seems more gas efficient since we can work with all existing loops (and less code bloat)
Have you made this consideration already? If so, curious to know why separate mapping was preferred
| uint256 requestedAmount = request.requestedAmounts[i]; | ||
| uint256 maxSlippageAmount = (requestedAmount * config.maxSlippageBps) / BPS_DENOMINATOR; | ||
|
|
||
| if (rebasingAmount > requestedAmount + maxSlippageAmount) { |
There was a problem hiding this comment.
We would need to use withdrawableAmount here, the requestedAmount is never used other than in events. This is because requestedAmount is the pre-slashing amount of what the user wants, but it is never to be expected as the result
|
|
||
| totalWithdrawalValue = totalWithdrawalValue > withdrawalValue ? totalWithdrawalValue - withdrawalValue : 0; | ||
|
|
||
| liquidToken.debitQueuedAssetBalances(assets, actualAmounts, sharesDeposited); |
There was a problem hiding this comment.
The issue here is the the total debited can be less than the total that was credited in settleUserWithdrawals
Regardless of rebasing and slashing loss, the total credit of queuedAssetBalance must equal the total debit after fulfilment so that it nets out to 0
| } | ||
| } | ||
|
|
||
| function _calculateActualAmountsWithSlippageProtection( |
There was a problem hiding this comment.
If we accept the suggestion of queuedAssetBalances -> queuedAssetElShares then we can minimize this function to solve only for the 1-2 wei transfer loss
parsaaba
force-pushed
the
bugfix/withdrawal-manager
branch
from
4b1e082 to
943945e
Compare
2 participants
No description provided.