[Vaults] Immutably fix the node operator in the basic StakingVault
#897
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
folkyatina
reviewed
Dec 13, 2024
|
|
||
| $.locked = SafeCast.toUint128(_locked); |
There was a problem hiding this comment.
Maybe, it's not worth to trust oracle here so much?
There was a problem hiding this comment.
I think we will definitely need a sanity checker here but for now we can leave the safecast.
| * Key master can: | ||
| * - deposit validators to the beacon chain | ||
| */ | ||
| bytes32 public constant KEY_MASTER_ROLE = keccak256("Vault.Delegation.KeyMasterRole"); |
There was a problem hiding this comment.
It may be preliminary to get rid of this role.
There was a problem hiding this comment.
Why? Keymaster does not have any duties in the delegation contract, since they use the base vault for depositing validator keys.
There was a problem hiding this comment.
It still can be a useful to have a role that can only deposit, but have no access to fees or role setup.
There was a problem hiding this comment.
Looks like we have 3 different role hierarchy: independent operator in vault, operatir in delegation and default_admin in delegation.
But looks like it should be only two of them:
default_admin and operator(that is the same address that is in the basic vault)
| * - Total value = report.valuation + (current inOutDelta - report.inOutDelta) | ||
| * - Vault is "healthy" if total value >= locked amount | ||
| * - Unlocked ETH = max(0, total value - locked amount) | ||
| * 2. Valuation & Balance |
There was a problem hiding this comment.
Balance here is read as eth balance. Maybe, "Valuation and Lock
tamtamchik
reviewed
Dec 16, 2024
| @@ -4,7 +4,7 @@ | |||
| // See contracts/COMPILERS.md | |||
| pragma solidity 0.8.25; | |||
|
|
|||
| import {MemUtils} from "../../common/lib/MemUtils.sol"; | |||
| import {MemUtils} from "contracts/common/lib/MemUtils.sol"; | |||
There was a problem hiding this comment.
I am not sure what the process is here, do we need to import our contracts always under "contracts/" namespace or use a relative path?
There was a problem hiding this comment.
I think imports relative to root are much more readable. You can clearly see where the file is imported from
| @@ -26,7 +26,7 @@ interface IDepositContract { | |||
| * | |||
| * This contract will be refactored to support custom deposit amounts for MAX_EB. | |||
| */ | |||
| contract VaultBeaconChainDepositor { | |||
| contract BeaconChainDepositLogistics { | |||
There was a problem hiding this comment.
Not sure if this makes the naming better, tbh
Comment on lines
125
to
128
| /** | ||
| * @notice Returns the current version of the contract | ||
| * @return uint64 contract version number | ||
| */ |
There was a problem hiding this comment.
Suggested change
| /** | |
| * @notice Returns the current version of the contract | |
| * @return uint64 contract version number | |
| */ | |
| /// @return uint64 contract version number |
| .connect(vaultOwner) | ||
| .createVault({ managementFee: 0n, performanceFee: 0n, manager, operator }, "0x"); | ||
| const vaultCreationReceipt = await vaultCreationTx.wait(); | ||
| if (!vaultCreationReceipt) throw new Error("Vault creation receipt not found"); |
There was a problem hiding this comment.
Is this a possible scenario in the test?
There was a problem hiding this comment.
I think it depends on the network. It is always defined in Hardhat but maybe null in some other network
| address _owner = owner(); | ||
| uint256 codeSize; | ||
|
|
||
| assembly { |
There was a problem hiding this comment.
Suggested change
| assembly { | |
| // Checking if owner is a contract by getting its code size | |
| assembly { |
|
|
||
| try IReportReceiver(owner()).onReport(_valuation, _inOutDelta, _locked) {} catch (bytes memory reason) { | ||
| emit OnReportFailed(address(this), reason); | ||
| if (codeSize > 0) { |
There was a problem hiding this comment.
Suggested change
| if (codeSize > 0) { | |
| // If owner is a contract (has code), try to notify it about the report | |
| if (codeSize > 0) { |
# Conflicts: # package.json # test/0.8.25/vaults/delegation-voting.test.ts # test/0.8.25/vaults/delegation.test.ts # yarn.lock
|
Hardhat Unit Tests Coverage SummaryDiff against masterResults for commit: f7086b5 Minimum allowed coverage is ♻️ This comment has been updated with latest results |
…/lidofinance/core into feat-immutable-operator-in-vault
…o feat-immutable-operator-in-vault
…/lidofinance/core into feat-immutable-operator-in-vault
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.
This PR fixes the operator address in the basic vault. This means that the vault owner cannot change the node operator address in
Delegationand, thus, we don't need the LIDO_DAO role inDelegationto approve the change.