[Draft] Vaults UX suggestions #893
Conversation
contracts/0.8.25/vaults/VaultHub.sol
Outdated
| /// @notice Returns all vaults owned by a given address | ||
| /// @param _owner Address of the owner | ||
| /// @return An array of vaults owned by the given address | ||
| function vaultsByOwner(address _owner) external view returns (IHubVault[] memory) { |
There was a problem hiding this comment.
I'm not sure about the correctness of this method, but such a method will be very useful
There was a problem hiding this comment.
Maybe needs vaultsIndexesByOwner and then this method around it.
There was a problem hiding this comment.
The thing is that Vaults are created without an owner, and then the owner and other roles are added. And it seems to me that storing another array in memory that will be updated frequently may be a bad decision.
If this is not the case, then it makes sense to have an additional owner field in the VaultSocket structure.
There was a problem hiding this comment.
Maybe needs
vaultsIndexesByOwnerand then this method around it.
We can't maintain the correctness of such index, because the owner can be changed in the vault, independently.
contracts/0.8.25/vaults/VaultHub.sol
Outdated
| /// @notice Returns all vaults owned by a given address | ||
| /// @param _owner Address of the owner | ||
| /// @return An array of vaults owned by the given address | ||
| function vaultsByOwner(address _owner) external view returns (IHubVault[] memory) { |
There was a problem hiding this comment.
If the owner is the dashboard contract, how are you planning to use it?
| * @notice Funds the staking vault with wrapped ether. Approvals for the passed amounts should be done before. | ||
| * @param _wethAmount Amount of wrapped ether to fund the staking vault with | ||
| */ | ||
| function fundByWeth(uint256 _wethAmount) external virtual onlyRole(DEFAULT_ADMIN_ROLE) { |
| @@ -159,6 +233,15 @@ contract Dashboard is AccessControlEnumerable { | |||
| _withdraw(_recipient, _ether); | |||
| } | |||
|
|
|||
| /** | |||
| * @notice Withdraws stETH tokens from the staking vault to wrapped ether. Approvals for the passed amounts should be done before. | |||
There was a problem hiding this comment.
Sorry, didn't get what it should do. RN, ether is still on the dashboard contract. Should it be transferred to msg.sender or some other recepient?
There was a problem hiding this comment.
@folkyatina The idea is to send to the fundByWeth method the amount of WETH that needs to be "unwrap" to the ETH and then fund Vault
WETH withdraw method https://github.com/gnosis/canonical-weth/blob/master/contracts/WETH9.sol#L38
Now I understand that it seems that this code will not work correctly
Co-authored-by: Aleksei Potapkin <[email protected]>
Co-authored-by: Aleksei Potapkin <[email protected]>
… into feat/vaults-suggestions
| wstETH.permit(msg.sender, address(this), _wstETHPermit.value, _wstETHPermit.deadline, _wstETHPermit.v, _wstETHPermit.r, _wstETHPermit.s); | ||
|
|
||
| wstETH.transferFrom(msg.sender, address(this), _tokens); | ||
| stETH.approve(address(wstETH), _tokens); |
There was a problem hiding this comment.
You don't need approve for unwrap.
| * @param _wstETHPermit data required for the wstETH.permit() method to set the allowance | ||
| */ | ||
| function burnWstETHWithPermit(uint256 _tokens, PermitInput calldata _wstETHPermit) external virtual onlyRole(DEFAULT_ADMIN_ROLE) { | ||
| wstETH.permit(msg.sender, address(this), _wstETHPermit.value, _wstETHPermit.deadline, _wstETHPermit.v, _wstETHPermit.r, _wstETHPermit.s); |
There was a problem hiding this comment.
Needs check for allowance to prevent ddos. See updated WQ contract.
tamtamchik
added
solidity
A short summary of the changes.
Context
Problem
What problem this PR solves, link relevant issue if it exists
Solution
Your proposed solution