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.
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
generate_bls_to_execution_change
#313Add
generate_bls_to_execution_change
#313Changes from 10 commits
d3fd1da
7404b69
d68a1c7
6a4ba11
47bf1aa
63fdc5e
c5651ea
2b5c218
1f8bab5
f5e2803
245ee7b
3b7e144
676279e
75e8ea0
0be70aa
962ce8c
1912868
c9b4e01
33f847b
7df39ba
27af99f
9b86724
ca5062a
d83c312
2c3fb22
db3e628
2cd1d99
aae55a6
f79bbac
9aed027
ee052fa
69f778d
ed092fc
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 you try to run the
bls-to-execution-change
subcommand with--help
it will actually skip the help menu and drop right into interactive mode:This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
Sorry, something went wrong.
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 find the wording of this description a little confusing. Perhaps something like this?
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.
It is indeed too complicated to explain the ERC-2334 in one sentence. 😭
I feel "From your mnemonic" might sound like the word index of a mnemonic. How about "The index position for the keys to start generating withdrawal credentials in ERC-2334 format"?
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.
can we have a prompt or something to make sure the user is doing this offline? to get the signatures in an airgapped location or something.
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.
yes, please kind sir allow for the bulk bonus feature mentioned here https://notes.ethereum.org/YGWpjmGHQceeq1gijiZuJg
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 validator_start_index throws an uncaught exception if the user misenters, you can't tell from the response that this was incorrect and user probably might not know what the right answer is.
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.
Hey @james-prysm could you elaborate on this case more? I tried to input random English and the cli did detect my wrong input and asked for re-input. What was the testing input you used?
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 broke it :)
https://gist.github.com/infosecual/d7de7005bf54732a863dce21d503e445
using
"1,2,3,4,5,6,7,8,9,10"
as the input to:
Please enter a list of the old BLS withdrawal credentials of your validator(s). Split multiple items with whitespaces or commas.:
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.
@hwwhww I totally missed this, put in the wrong validator_start_index with the corresponding validator indexs and it broke.
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.
use case is
using a mnemonic of 1800 keys, 0~1799 index
input for validator_start_index: 1
validator_index: 0
withdrawal_credentials: 0x004652a99d8c9278708a98dab2c09b45cf09ea732037a17583ae2513a329fd8a
execution_address:
i get the following error
File "staking_deposit/utils/validation.py", line 246, in validate_bls_withdrawal_credentials_matching staking_deposit.exceptions.ValidationError: The given withdrawal credentials is matching the old BLS withdrawal credentials that mnemonic generated. [76365] Failed to execute script 'deposit' due to unhandled exception!
if i used validator_start_index:0 it works
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.
can both non hex and hex be handled? or if the error for non hex lets the user know,I believe some users will get the withdrawal pub key from the intial deposit.json which doesn't have 0x
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.
bls_withdrawal_credentials_list
can be with0x
prefix or no0x
.eth1_withdrawal_address
requires the checksum form and0x
prefix.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.
@hwwhww Does this mean if the user pastes an all lowercase address it will be blocked?
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.
@wackerow
in a very edge case, an all-number address is a valid checksum address 😄 but I understand what you mean, yes, the input addresses have to be checksummed and capital letters matter.