-
Notifications
You must be signed in to change notification settings - Fork 15
Remove Secure Signer from Provision Node flows #136
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: protocol-redesign
Are you sure you want to change the base?
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests.
🚀 New features to boost your workflow:
|
| bytes memory withdrawalCredentials = getWithdrawalCredentials($.validators[moduleName][index].module); | ||
|
|
||
| bytes32 depositDataRoot = | ||
| LibBeaconchainContract.getDepositDataRoot(validatorPubKey, validatorSignature, withdrawalCredentials); |
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.
if we are removing GUARDIAN_MODULE.validateProvisionNode() and such other validations on-chain, we can validate basics BLS signature by creating a library BLSSignatureVerifier and have basic logics to verify such as validateBLSSignatureFormat verifyDepositDataRoot. wdyt?
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.
Hmm which kind of validations will it have? Right now we only check the BLS pubkey lenght. What checks would we do on the signature? The DepositDataRoot is built in the LibBeaconchainContract, so no need to verify that part
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.
basic format validations such as BLS public key format - check that pubkey is not all zeros. withdrawal credential length. we can think if we need that. and moving all these checks to that library
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.
Ok, in the next PR I'm moving verification logic to a library so PufferProtocol is not so clogged with verification logic. I'll look into additional checks to add there. If there's nothing else regarding this PR we can merge it and will do this in the remove GuardianModule PR
| // Transfer 32 ETH to this contract from the Puffer Vault | ||
| PUFFER_VAULT.transferETH(address(this), 32 ether); | ||
|
|
||
| emit SuccessfullyProvisioned(validatorPubKey, index, moduleName); | ||
|
|
||
| // Increase lockedETH on Puffer Oracle | ||
| PUFFER_ORACLE.provisionNode(); | ||
|
|
||
| // Deposit into the Beacon Chain Deposit Contract directly | ||
| BEACON_DEPOSIT_CONTRACT.deposit{ value: 32 ether }( | ||
| validatorPubKey, withdrawalCredentials, validatorSignature, depositDataRoot | ||
| ); |
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.
wait. why this flow? this looks no-restaking flow but with incorrect withdrawal credentials.
for restaking flow the ETH should move to module and then call callStake.
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.
As discussed, it's not needed to do the flow from the EigenPod, we can deposit directly to the beacon deposit contract. The withdrawal credentials point to the EigenPod, so it is restaking
Adapted skipProvisioning and provisionNode flows to remove secure signer