-
Notifications
You must be signed in to change notification settings - Fork 15
Pectra upgrade #115
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Pectra upgrade #115
Conversation
@@ -10,5 +10,4 @@ struct ValidatorKeyData { | |||
bytes32 depositDataRoot; | |||
bytes[] blsEncryptedPrivKeyShares; | |||
bytes blsPubKeySet; | |||
bytes raveEvidence; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should rethink this one. If we remove it right now, and in the future we decide to add a new field to this struct, it might contain some bad data. This shouldn't be an issue, but it is still possible. I'd rather have it deprecated_raveEvidence
or something like that just to eliminate that chance, even though it is minimal.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
bytes[] blsEncryptedPrivKeyShares; | ||
bytes blsPubKeySet; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can remove these two, they are related to enclave
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Used deprecated_* as in prev comment
view | ||
returns (bytes[] memory, bytes memory, uint256, uint256) | ||
{ | ||
function getPayload(bytes32 moduleName) external view returns (bytes memory, uint256, uint256) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
99% sure that this was only used by the enclave, We can remove this function
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed
We can remove
|
It would be nice if we introduced 2 ways of doing execution layer withdrawals/exits Way 1: In this flow, if the validator is 0x02, the same function could be used to withdraw the rewards from beacon chain something like:
|
Removed |
Since we're not adding support to 0x02 validators in this PR, and when we do we have to change more stuff I'd say to implement those changes in a future PR when we're sure on how will 0x02 validators will affect our architecture. Also in my implementation, the PMM function can only be called from a new role We can sync on this if you want |
Codecov ReportAll modified and coverable lines are covered by tests ✅
... and 7 files with indirect coverage changes 🚀 New features to boost your workflow:
|
event ValidatorKeyRegistered( | ||
bytes pubKey, uint256 indexed pufferModuleIndex, bytes32 indexed moduleName, bool usingEnclave | ||
); | ||
event ValidatorKeyRegistered(bytes pubKey, uint256 indexed pufferModuleIndex, bytes32 indexed moduleName); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This changes the event signature, if we want to have it easy on the off-chain side(subgraph), we can keep the old event signature and always emit false
for usingEnclave. IMO, it is not a big deal to change it as well, but it is something we must be aware of.
overall, the PR looks good. Lets sync on this topic tomorrow. Would be nice to bundle a few things in one audit, and have 1 upgrade instead of more. |
This Pull Request was made for mainly 2 different things:
Currently this is a WIP. There are a couple of things left to do, basically: