Alternative validator registration logic #39
Conversation
WalidOfNow
force-pushed
the
feat/alt-validator-registration
branch
from
7683ad4 to
37f7a26
Compare
| // SPDX-License-Identifier: GPL-3.0 | ||
| pragma solidity >=0.8.0 <0.9.0; | ||
|
|
||
| import { MerkleUtils } from "./MerkleUtils.sol"; |
There was a problem hiding this comment.
Why not use "@openzeppelin/contracts/utils/cryptography/MerkleProof.sol"?
There was a problem hiding this comment.
in general, MerkleProof you have less control over the structure of provided proof.
1- MerkleUtils has a merkleize() function to construct a Merkle tree from an array of chunks. which is used for constructing the validator tree hash. It's important that we pass the validator information so we can check them in the sc and contruct the validator tree hash
2- proof verification takes an additional paramter index which helps with partial proofs.
3- not a big reason but we would need to pass a hashing function that uses sha256 for MerkleProof since the default one is keccak256
I am experimenting with the proving logic in general and trying to find a good balance between readability and control of the proofs.
| * @param newDelay The new registration delay in seconds. | ||
| * @dev Restricted to the DAO | ||
| */ | ||
| function setRegistrationDelay(uint64 newDelay) external; |
There was a problem hiding this comment.
IMO interface doesn't need to contain functions that are not meant to to be called by public. It adds unnecessary noise and LOC. I vote fore removing functions like this from the Interface and just keeping them + natspec in the contract
There was a problem hiding this comment.
This function is callable by DAO. So if our DAO becomes a contract in the future, we would need the interface for it.
There was a problem hiding this comment.
True, but DAO contracts generally do not import interfaces. They deal with target & calldata. e.g. https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/governance/Governor.sol
| * @inheritdoc IUniFiAVSManager | ||
| */ | ||
| function avsDirectory() external view returns (address) { | ||
| return address(AVS_DIRECTORY); |
There was a problem hiding this comment.
Inconsistent getter naming, the previous function is called getRestakeableStrategies and this one just avsDirectory
There was a problem hiding this comment.
this interface is required by EigenLayer. Hence why the naming convention is different.
l1-contracts/src/UniFiAVSManager.sol
Outdated
| } | ||
|
|
||
| $.slashedOperators[operator].push( | ||
| InvalidValidator({ slashingBeneficiary: msg.sender, blsPubKeyHash: blsPubKeyHash }) |
There was a problem hiding this comment.
slashingBeneficiary naming is bad, how can somebody benefit from being slashed? :D maybe use slashedOperator?
There was a problem hiding this comment.
slashingBeneficiary is the person calling the slashing function. which is the person getting reward.
| error BeaconBlockProofForProposerIndex(); | ||
|
|
||
| function verifyValidatorExistence(InclusionProof memory inclusionProof) internal returns (bool) { | ||
| (, bytes32 beaconBlockRoot) = getRootFromTimestamp(inclusionProof.timestamp); |
There was a problem hiding this comment.
This can be moved down in this function after the 2 proof checks to save on some gas in case of bad proof
There was a problem hiding this comment.
good call! although this would revert the outer call so the whole transaction should be reverting. But I think it's good to add it in case we use this in another place.
| function setDeregistrationDelay(uint64 newDelay) external restricted { | ||
| _setDeregistrationDelay(newDelay); | ||
| } | ||
|
|
There was a problem hiding this comment.
If an operator calls setOperatorCommitment multiple times before updateOperatorCommitment, earlier pending commitments are overwritten without any checks, so is this expected?
| } | ||
|
|
||
| ValidatorRegistrationData storage validatorRegistrationData = $.validatorRegistrations[storedBlsPubKeyHash]; | ||
| if (proof.timestamp < validatorRegistrationData.registeredAt || proof.timestamp > validator.registeredUntil) { |
There was a problem hiding this comment.
proof timestamp validation doesn't account for block.timestamp vs block.number inconsistencies, so may allow proofs from future blocks to be valid. can add checks for those also
No description provided.