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

Make --execution_address and --eth1_withdrawal_address compatible alias #339

Merged
merged 1 commit into from Mar 14, 2023

Conversation

hwwhww
Copy link
Contributor

@hwwhww hwwhww commented Mar 13, 2023

Issue

In deposit commands, the option is called --eth1_withdrawal_address. But in BTEC, we use --execution_address because (1) it's the parameter of the spec and (2) generally removes "eth1" terminology.

How did I fix it

  • Make JITOption.param_decls accept a list of parameter declarations like click.Option.
  • Make --execution_address and --eth1_withdrawal_address compatible alias

Warning

In the case that user somehow types in BOTH --eth1_withdrawal_address and --execution_address, click by default will override the previous with the latter. I tried to raise exception by implementing click.Option.type_cast_value API, but my changes were not compatible with our other parameters. It should be fix, but I think this PR is ready to be included in the release now.

@hwwhww hwwhww requested a review from CarlBeek March 13, 2023 19:34
@hwwhww hwwhww mentioned this pull request Mar 13, 2023
2 tasks
Copy link
Collaborator

@CarlBeek CarlBeek left a comment

Choose a reason for hiding this comment

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

I agree that there isn't a major issue with supplying both --eth1_withdrawal_address and --execution_address.

@CarlBeek CarlBeek merged commit 0175701 into dev Mar 14, 2023
1 check passed
everhusk pushed a commit to earthwallet/earth-wallet-cli that referenced this pull request Aug 3, 2023
Make `--execution_address` and `--eth1_withdrawal_address` compatible alias
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.

None yet

2 participants