[SI][Vaults] feat: Dashboard UX updates #915
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Jeday
reviewed
Jan 9, 2025
tamtamchik
reviewed
Jan 9, 2025
test/0.8.25/vaults/dashboard/contracts/LidoLocator__HarnessForDashboard.sol
Outdated
Show resolved
Hide resolved
test/0.8.25/vaults/dashboard/contracts/LidoLocator__HarnessForDashboard.sol
Outdated
Show resolved
Hide resolved
…ashboard-ux-updates
|
Hardhat Unit Tests Coverage SummaryDiff against masterResults for commit: 067f38b Minimum allowed coverage is ♻️ This comment has been updated with latest results |
DiRaiks
changed the title
tamtamchik
approved these changes
Jan 13, 2025
…re into feat/vaults-dashboard-recovery
[SI][Vault] Dashboard recovery
folkyatina
requested changes
Jan 15, 2025
| PermitInput calldata _permit | ||
| ) | ||
| external | ||
| virtual | ||
| onlyRole(DEFAULT_ADMIN_ROLE) | ||
| trustlessPermit(address(STETH), msg.sender, address(this), _permit) | ||
| { | ||
| _burn(_tokens); | ||
| _burn(msg.sender, _amountOfShares); |
There was a problem hiding this comment.
Suggested change
| _burn(msg.sender, _amountOfShares); | |
| _burnFrom(msg.sender, _amountOfShares); |
Comment on lines
415
to
416
| uint256 _amount; | ||
| if (_token == address(0)) revert ZeroArgument("_token"); |
There was a problem hiding this comment.
Suggested change
| uint256 _amount; | |
| if (_token == address(0)) revert ZeroArgument("_token"); | |
| if (_token == address(0)) revert ZeroArgument("_token"); | |
| uint256 _amount; |
folkyatina
reviewed
Jan 21, 2025
| @@ -357,48 +374,44 @@ contract Dashboard is AccessControlEnumerable { | |||
| return; | |||
| } | |||
| } | |||
| revert("Permit failure"); | |||
| revert Erc20Error(token, "Permit failure"); | |||
folkyatina
approved these changes
Jan 21, 2025
TheDZhon
requested changes
Jan 22, 2025
| IWETH9 public immutable WETH; | ||
|
|
||
| /// @notice ETH address convention per EIP-7528 | ||
| address public constant ETH = address(0xEeeeeEeeeEeEeeEeEeEeeEEEeeeeEeeeeeeeEEeE); |
There was a problem hiding this comment.
Suggested change
| address public constant ETH = address(0xEeeeeEeeeEeEeeEeEeEeeEEEeeeeEeeeeeeeEEeE); | |
| address public constant ETH = 0xEeeeeEeeeEeEeeEeEeEeeEEEeeeeEeeeeeeeEEeE; |
| @@ -109,6 +111,10 @@ contract Dashboard is AccessControlEnumerable { | |||
| vaultHub = VaultHub(stakingVault.vaultHub()); | |||
| _grantRole(DEFAULT_ADMIN_ROLE, msg.sender); | |||
There was a problem hiding this comment.
let's add an admin argument to avoid using msg.sender inside the contract
There was a problem hiding this comment.
maybe a bit later will be done by @failingtwice
| @@ -109,6 +111,10 @@ contract Dashboard is AccessControlEnumerable { | |||
| vaultHub = VaultHub(stakingVault.vaultHub()); | |||
| _grantRole(DEFAULT_ADMIN_ROLE, msg.sender); | |||
|
|
|||
| // reduces gas cost for `burnWsteth` | |||
There was a problem hiding this comment.
Suggested change
| // reduces gas cost for `burnWsteth` | |
| // reduces gas cost for `mintWsteth` |
| // reduces gas cost for `burnWsteth` | ||
| // dashboard will hold STETH during this tx | ||
| STETH.approve(address(WSTETH), type(uint256).max); | ||
|
|
||
| emit Initialized(); |
There was a problem hiding this comment.
should we emit args of the init?
| @@ -180,11 +186,11 @@ contract Dashboard is AccessControlEnumerable { | |||
|
|
|||
| /** | |||
| * @notice Returns the maximum number of shares that can be minted with deposited ether. | |||
There was a problem hiding this comment.
Suggested change
| * @notice Returns the maximum number of shares that can be minted with deposited ether. | |
| * @notice Returns the maximum number of shares that can be minted with funded ether. |
| function recoverERC721(address _token, uint256 _tokenId) external onlyRole(DEFAULT_ADMIN_ROLE) { | ||
| if (_token == address(0)) revert ZeroArgument("_token"); | ||
|
|
||
| IERC721(_token).transferFrom(address(this), msg.sender, _tokenId); |
There was a problem hiding this comment.
Suggested change
| IERC721(_token).transferFrom(address(this), msg.sender, _tokenId); | |
| IERC721(_token).safeTransferFrom(address(this), msg.sender, _tokenId); |
There was a problem hiding this comment.
| @@ -540,4 +621,7 @@ contract Dashboard is AccessControlEnumerable { | |||
|
|
|||
| /// @notice Error when the contract is already initialized. | |||
| error AlreadyInitialized(); | |||
|
|
|||
| /// @notice Error interacting with an ERC20 token | |||
| error Erc20Error(address token, string reason); | |||
contracts/0.8.9/Burner.sol
Outdated
There was a problem hiding this comment.
let's move this one to 0.8.25 and take recovery lib from CSM
| await expect(result).to.emit(steth, "Transfer").withArgs(wsteth, dashboard, weiStethDown); // unwrap wsteth to steth | ||
| await expect(result).to.emit(wsteth, "Transfer").withArgs(dashboard, ZeroAddress, weiShare); // burn wsteth | ||
| // transfer shares to hub | ||
| await expect(result).to.emit(steth, "Transfer").withArgs(dashboard, hub, weiStethDownDown); |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
|
||
| WETH.transferFrom(msg.sender, address(this), _wethAmount); | ||
| WETH.withdraw(_wethAmount); | ||
| function fundByWeth(uint256 _amountWETH) external virtual onlyRole(DEFAULT_ADMIN_ROLE) { |
There was a problem hiding this comment.
Suggested change
| function fundByWeth(uint256 _amountWETH) external virtual onlyRole(DEFAULT_ADMIN_ROLE) { | |
| function fundWithWeth(uint256 _amountWETH) external virtual onlyRole(DEFAULT_ADMIN_ROLE) { |
or
Suggested change
| function fundByWeth(uint256 _amountWETH) external virtual onlyRole(DEFAULT_ADMIN_ROLE) { | |
| function fundWeth(uint256 _amountWETH) external virtual onlyRole(DEFAULT_ADMIN_ROLE) { |
TheDZhon
approved these changes
Jan 28, 2025
DiRaiks
commented
Jan 29, 2025
| function mintStETH( | ||
| address _recipient, | ||
| uint256 _amountStETH | ||
| ) external payable virtual onlyRole(DEFAULT_ADMIN_ROLE) fundAndProceed { |
There was a problem hiding this comment.
Do we still need virtual onlyRole(DEFAULT_ADMIN_ROLE) here?
DiRaiks
commented
Jan 29, 2025
| function burnSharesWithPermit( | ||
| uint256 _amountShares, | ||
| PermitInput calldata _permit | ||
| ) external virtual onlyRole(DEFAULT_ADMIN_ROLE) safePermit(address(STETH), msg.sender, address(this), _permit) { |
There was a problem hiding this comment.
Do we still need role?
| @@ -41,19 +41,31 @@ contract VaultHub__MockForDashboard { | |||
| emit Mock__VaultDisconnected(vault); | |||
| } | |||
|
|
|||
| function mintSharesBackedByVault(address /* vault */, address recipient, uint256 amount) external { | |||
| function mintSharesBackedByVault(address vault, address recipient, uint256 amount) external { | |||
There was a problem hiding this comment.
Suggested change
| function mintSharesBackedByVault(address vault, address recipient, uint256 amount) external { | |
| function mintSharesBackedByVault(address _vault, address _recipient, uint256 _shares) external { |
TheDZhon
approved these changes
Jan 29, 2025
| // SPDX-License-Identifier: GPL-3.0 | ||
| // SPDX-FileCopyrightText: 2024 Lido <[email protected]> | ||
|
|
||
| // See contracts/COMPILERS.md | ||
| pragma solidity 0.8.25; | ||
|
|
||
| import {Permissions} from "./Permissions.sol"; | ||
| import {OwnableUpgradeable} from "contracts/openzeppelin/5.2/upgradeable/access/OwnableUpgradeable.sol"; |
|
|
||
| // See contracts/COMPILERS.md | ||
| pragma solidity 0.8.25; | ||
|
|
||
| import {Permissions} from "./Permissions.sol"; | ||
| import {OwnableUpgradeable} from "contracts/openzeppelin/5.2/upgradeable/access/OwnableUpgradeable.sol"; | ||
| import {IERC20} from "@openzeppelin/contracts-v5.2/token/ERC20/IERC20.sol"; | ||
| import {IERC20Permit} from "@openzeppelin/contracts-v5.2/token/ERC20/extensions/IERC20Permit.sol"; | ||
| import {Clones} from "@openzeppelin/contracts-v5.2/proxy/Clones.sol"; |
| import {IERC20Permit} from "@openzeppelin/contracts-v5.2/token/ERC20/extensions/IERC20Permit.sol"; | ||
| import {ILido as IStETH} from "contracts/0.8.25/interfaces/ILido.sol"; | ||
| import {ILidoLocator} from "contracts/common/interfaces/ILidoLocator.sol"; | ||
| import {IStakingVault} from "contracts/0.8.25/vaults/interfaces/IStakingVault.sol"; |
There was a problem hiding this comment.
maybe namespace should be 'vaults.Permissions.X'
| * @param _permit data required for the stETH.permit() method to set the allowance | ||
| */ | ||
| function burnWithPermit( | ||
| uint256 _tokens, | ||
| function burnStethWithPermit( |
There was a problem hiding this comment.
Suggested change
| function burnStethWithPermit( | |
| function burnStETHWithPermit( |
| * @param _token Address of the token to recover or 0xeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee for ether | ||
| * @param _recipient Address of the recovery recipient | ||
| */ | ||
| function recoverERC20(address _token, address _recipient) external onlyRole(DEFAULT_ADMIN_ROLE) { |
There was a problem hiding this comment.
Suggested change
| function recoverERC20(address _token, address _recipient) external onlyRole(DEFAULT_ADMIN_ROLE) { | |
| function recoverERC20(address _token, uint256 _amount, address _recipient) external onlyRole(DEFAULT_ADMIN_ROLE) { |
There was a problem hiding this comment.
keccak256("Vault.Delegation.CuratorRole") ⇒ keccak256("vaults.Delegation.CuratorRole") and so on
There was a problem hiding this comment.
let's remove msg.sender from initialization
There was a problem hiding this comment.
vote -> multiConfirmationRole (everywhere)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
A short summary of the changes.
Context
General
Dashboard Contract
LidoLocatorfor retrievingstETHandwstETHcontract addresses.stETHtowstETH.getMintableSharestoprojectedMintableShares.mintWstETHto utilize the initial approval ofstETH.burnWstETHto align with changes in the_burnmethod.burnWithPermitto align with changes in the_burnmethod.burnWstETHWithPermitto align with changes in the_burnmethod.burn->burnSharesmint->mintSharesburn/fundmethods to accommodate changes in_burn/_fund:_fundnow takesvalueas an argument._burnnow accepts_senderand_amountOfSharesas arguments._sendertype, eithertransferSharesortransferSharesFromis executed.transferSharesis used if the operation originates from the Dashboard contract to avoid additionalstETHapprovals.Delegation Contract
fund/burnmethods to account for updates to_burn/_fundin the Dashboard contract.Testing
Permitfunctionality when the shares-to-stETHratio is not 1:1 (e.g.,1 share = 0.5 stETH,1 share = 2 stETH).LidoLocatorsetup in the constructor.Additional Notes
Permitmechanism withstETHduring rebalancing of the shares-to-stETHratio will still use thevalueofstETHtokens. As a result, the values passed to theburnWithPermitmethod and those signed in thePermitmay differ. To address this, theStETHPermitcontract would need updates, but this could introduce unnecessary overhead.Problem
What problem this PR solves, link relevant issue if it exists
Solution
Your proposed solution