Initial version #28
Initial version #28
Conversation
|
Thanks for putting this together. Got some thoughts we may want to discuss separately.
|
| uint256 _totalSupply = totalSupply; | ||
|
|
||
| // Calculate a fee - zero if user is the last to withdraw | ||
| uint256 _fee = (_totalSupply == 0 || _totalSupply - _shares == 0) ? 0 : assets.mulDivDown(fees.withdrawFeePercentage, DENOMINATOR); |
There was a problem hiding this comment.
- Can
_totalSupply - _sharesunderflow? I supposed because it is uint it cannot be negative though. - Why should the fee be zero for the last user to withdraw? Why does the fee charging change sometimes? Can this be exploited somehow?
| if (!(_harvestBounty >= _minBounty)) revert InsufficientAmountOut(); | ||
|
|
||
| IERC20(WETH).safeTransfer(_receiver, _harvestBounty); | ||
| } |
There was a problem hiding this comment.
Nitpick, do we have some formatter or linter for Solidity? That indention looks off here.
There was a problem hiding this comment.
I'll do proper formatting.
| /// @dev Adds emitting of YbTokenTransfer event to the original function | ||
| function transfer(address to, uint256 amount) public override returns (bool) { | ||
| balanceOf[msg.sender] -= amount; | ||
|
|
||
| // Cannot overflow because the sum of all user | ||
| // balances can't exceed the max uint256 value. | ||
| unchecked { | ||
| balanceOf[to] += amount; | ||
| } | ||
|
|
||
| emit Transfer(msg.sender, to, amount); | ||
| emit YbTokenTransfer(msg.sender, to, amount, convertToAssets(amount)); | ||
|
|
||
| return true; | ||
| } |
There was a problem hiding this comment.
What is the best practice here? Shouldn't there be some method that we inject and the rest of transfer remains the standard implementation? The current code looks like everything is copied and one line added for our use case. Not sure this is how we should be doing it.
| for(uint256 i = 0; i < length;) { | ||
| address assetVault = assetVaultList[i]; | ||
| totalAUM += IAssetVault(_assetVault).getEthBalance(); | ||
| unchecked{ ++i; } |
There was a problem hiding this comment.
Imma asking all the noob questions here. Why should i be incremented within the unchecked block? Why don't we do it without?
There was a problem hiding this comment.
It disables the built-in overflow check (which is not required here because the length would not exceed 2^256) and so saves gas.
| address assetVault = assetVaultList[i]; | ||
| totalAUM += IAssetVault(_assetVault).getEthBalance(); |
There was a problem hiding this comment.
That looks wrong somehow. We have assetVault and _assetVault like one comes from a different scope.
There was a problem hiding this comment.
That a mistake. Will fix.
| /// @notice The address of WSTETH token. | ||
| address internal constant WSTETH = 0x7f39C581F595B53c5cb19bD0b3f8dA6c935E2Ca0; | ||
| /// @notice The address of STETH token. | ||
| address internal constant STETH = 0xae7ab96520DE3A18E5e111B5EaAb095312D7fE84; |
There was a problem hiding this comment.
How do we add or remove LSDs when these are hard coded here?
There was a problem hiding this comment.
LSD vaults are added/removed on fortETH contract. The commented one is stETH LSD vault implementation.
|
I have a problem with the design. As I see it, the flow should be something like that;
user has 3 choices now:
|
|
Design reworked.
|
|
Sounds good. Will take a look at the code tmr hopefully. In the meantime, can you elaborate on the strategy design you have in mind? How should that look like? |
|
@johnnyonline Strategy design:
|
Core design ideas:
There are two ways we may proceed after deposit (I implemented both, but we need to choice one we want to keep):
(a) deposited assets are immediately distribute between LSDs vaults (gas expensive);
(b) deposited assets are buffered in the contract until distributeBufferedAssets() is called for some bounty (less expensive);
(a) previously deposited assets remain untouched and new deposits are distributed according to updated weights
(b) all asset in LSDs vaults converted to WETH and returned to fortETH contract, after they are redistributed back according to new weights (via calling redistributeAssets()).
Problems to consider: withdrawal (stETH -> ETH) is not currently available via Lido contract (https://www.coindesk.com/business/2023/04/06/lido-stakers-can-expect-ether-withdrawals-no-sooner-than-early-may/), therefore we could either wait when it become available or swap it through stETH/ETH pool (less effective due to slippage).