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

Add Eth1 address withdrawal support #195

Merged
merged 7 commits into from
Mar 30, 2021
Merged

Add Eth1 address withdrawal support #195

merged 7 commits into from
Mar 30, 2021

Conversation

hwwhww
Copy link
Contributor

@hwwhww hwwhww commented Mar 25, 2021

Fix #188, support ethereum/consensus-specs#2149

  • cli interface: add --eth1_withdrawal_address argument: If this field is set and valid, the given Eth1 address will be used to create the withdrawal credentials. Otherwise, it will generate withdrawal credentials with the mnemonic-derived withdrawal public key.
  • validate_deposit: add public key and withdrawal credentials validations.

@hwwhww hwwhww requested a review from CarlBeek March 25, 2021 15:26
def validate_eth1_withdrawal_address(cts: click.Context, param: Any, address: str) -> HexAddress:
if address is None:
return None
if not is_hex_address(address):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note: the prefix 0x is optional here.

Choose a reason for hiding this comment

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

hi

Choose a reason for hiding this comment

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

hello word

@hwwhww
Copy link
Contributor Author

hwwhww commented Mar 25, 2021

/cc @djrtwo
Considering your de-Eth1/Eth2 rebranding, any plan to rename ETH1_ADDRESS_WITHDRAWAL_PREFIX?

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 like that this is being added. 👍

One issue is that by adding this functionality to generate_keys it is automagically added to both the new-mnemonic and existing-mnemonic commands. The latter of which doesn't have tests and isn't mentioned in the README.

eth2deposit/credentials.py Show resolved Hide resolved
eth2deposit/utils/validation.py Show resolved Hide resolved
@hwwhww hwwhww changed the base branch from master to dev March 30, 2021 11:16
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.

LGTM! Let's get it merged.

@CarlBeek CarlBeek added the enhancement New feature or request label Mar 30, 2021
@hwwhww hwwhww merged commit 27abb5c into dev Mar 30, 2021
@hwwhww hwwhww deleted the eth1-address-withdrawal branch March 30, 2021 12:23
@CarlBeek CarlBeek mentioned this pull request Apr 2, 2021
1 task
Copy link

@G-11-P G-11-P left a comment

Choose a reason for hiding this comment

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

commit and merge

everhusk pushed a commit to earthwallet/earth-wallet-cli that referenced this pull request Aug 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support ETH1_ADDRESS_WITHDRAWAL_PREFIX withdrawal credentials (0x01)
4 participants