-
Notifications
You must be signed in to change notification settings - Fork 10
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
Add ability to generate exit message #4
Add ability to generate exit message #4
Conversation
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.
Everything is generally good. I still have some real world tests I would like to complete on Holesky. Let me know what you think of these comments.
Validating keystore file on input Adding NON_PRATER_CHAIN_KEYS to reduce code duplication
Removing validator_indices int cast to be properly validated
Ran through all the items and added quite a few changes:
If I missed anything, let me know and thanks for such a thorough review |
What's the issue with having
should result in the same as
but right now only the second one works. |
When selecting a chain, the previous verification logic for the parameter was The problem here is the verification logic for the captive callback was different: The resulting UX is the prater chain would not be an option for the initial chain prompt but if you trigger the captive callback logic, it would show as an option because it wasn't being filtered. I'm not sure why they went this route to begin with but at this point we are probably safe to add an issue to completely remove goerli, prater, and zhejiang as they are all deprecated and then this bit of logic and be replaced with |
I think removing all dead or deprecated chains is the way forward. I don't think there is a lot of value in keeping them around. I've moved that concern into #20 . |
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.
I think there are 1 minor thing remaining before I can approve this:
- Replacing the pre-baked hex data in
tests/test_utils/test_validation.py
I'm still waiting on my validators to be activated on Holeksy to test these changes on a real network. According to beaconcha.in, I still have around 6 days to wait.
We should add the |
I thought I did that. Did I mess something up there? |
Sorry, I missed it with being on the wrong branch. Everything is good here. |
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.
Everything is good in the review. What's left are some real world tests that I should be able to perform in the next few days.
My validators were just activated on Holesky and I tried to submit the exit messages but I have to wait another 2 days until I can start the exit process. |
I've successfully exited 2 validators, one with a keystore and one with a mnemonic. |
I tried to resolve the pending merge conflict but I think I broke the tests. It feels like they are stuck waiting for some input on stdin. |
There was a problem with the git merge that combined two changes in the existing mnemonic test where it provided the argument for the execution address and input. |
Port from ethereum#355
Adds two commands:
exit-transaction-keystore and exit-transaction-mnemonic
exit-transaction-keystore
Allows the user to specify a single keystore file to generate an exit message if the provided password is correct.
The success of this command will result in a single
signed_exit_transaction-***.json
file inexit_transactions
exit-transaction-mnemonic
Allows the user to specify a mnemonic to generate 1-n exit messages depending on how many validator indices are provided. The success of this command will result in n
signed_exit_transaction-***.json
files inexit_transactions
for each validator index provided.Testing
Build as instructed
Create a new mnemonic and keystore(s) on mainnet with
./deposit.sh new-mnemonic
For each keystore, create an entry in this base
offline-preparation.json
file:With the saved
offline-preparation.json
above in the same location as ethdo, create an exit message with ethdo using the following command:./ethdo validator exit --offline --mnemonic="<GENERATED_MNEMONIC>" --validator=<VALIDATOR_INDEX_IN_PREF_FILE>
Create an exit message with the CLI using both
./deposit.sh exit-transaction-mnemonic --epoch=274294
for each keystore
./deposit.sh exit-transaction-keystore --epoch=274294
All outputs should match