Skip to content
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

Withdrawal address prompt #27

Merged

Conversation

valefar-on-discord
Copy link
Collaborator

@valefar-on-discord valefar-on-discord commented Apr 28, 2024

Adding a prompt for withdrawal address when executing key generation.

There are a few changes of note:

  • I have added a default param for the captive prompt. I simply don't see why not.
  • I am not confirming selection if the user provides an empty value for the withdrawal address but we can add it if necessary
  • closest_match in intl.py creates some pretty weird UX scenarios especially during language selection. As an example, if you provide 1234 as the option for language, it will chose 12. My assumption this is used to avoid non-standard character match issues.

#16

@valefar-on-discord valefar-on-discord changed the title [WIP] Withdrawal address prompt Withdrawal address prompt Apr 28, 2024
@valefar-on-discord
Copy link
Collaborator Author

Figured out the issue with the stalling test and I think this is good to go.
I'm not 100% sure on the prompt wording but I do think it is important to acknowledge it is an optional field but the user will at least be prompted.

@remyroy remyroy self-requested a review April 30, 2024 20:17
Copy link
Member

@remyroy remyroy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Everything seems good.

@@ -19,7 +19,7 @@
},
"arg_execution_address": {
"help": "The 20-byte (Eth1) execution address that will be used in withdrawal",
"prompt": "Please enter the 20-byte execution address for the new withdrawal credentials. Note that you CANNOT change it once you have set it on chain.",
"prompt": "Please enter the optional 20-byte execution address for the new withdrawal credentials. Note that you CANNOT change it once you have set it on chain.",
Copy link
Member

@remyroy remyroy Apr 30, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would add something like this at the end of this message:

Please enter the optional 20-byte execution address for the new withdrawal credentials. Note that you CANNOT change it once you have set it on chain. Use a wallet address that you will have full control over for the foreseeable future.

What do you think?

@remyroy remyroy merged commit 8a45ca8 into eth-educators:main Apr 30, 2024
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants