Skip to content
This repository was archived by the owner on Oct 6, 2025. It is now read-only.

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

Merged
CarlBeek merged 1 commit intodevfrom
execution-address-alias
Mar 14, 2023
Merged

Make --execution_address and --eth1_withdrawal_address compatible alias#339
CarlBeek merged 1 commit intodevfrom
execution-address-alias

Conversation

@hwwhww
Copy link
Copy Markdown
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 force-pushed the execution-address-alias branch from 229eb07 to 4064f12 Compare March 13, 2023 20:11
@hwwhww hwwhww mentioned this pull request Mar 13, 2023
2 tasks
Copy link
Copy Markdown
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
sangheraio 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 subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants