-
Notifications
You must be signed in to change notification settings - Fork 12
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
Deploy new registries and release packages #49
Conversation
a011648
to
118af99
Compare
ping @pipermerriam for 👀 |
private_key = eth_keyfile.extract_key_from_keyfile( | ||
str(keyfile_path), to_bytes(text=password) | ||
) | ||
except ValueError: |
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.
A nice upstream change would be for the library to raise something less generic than a ValueError
on authentication failure.
owner_address = Account.from_key(private_key).address | ||
signing_middleware = construct_sign_and_send_raw_middleware(private_key) | ||
w3.middleware_onion.add(signing_middleware) | ||
w3.eth.defaultAccount = to_checksum_address(owner_address) |
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 might be a good place to issue a DEBUG
level logging statement indicating that in-flight signing has been enabled for whatever account address.
ethpm_cli/config.py
Outdated
return Web3(load_provider_from_uri(infura_url, headers)) | ||
w3 = Web3(load_provider_from_uri(infura_url, headers)) | ||
|
||
if private_key: |
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.
nitpick
I'm in favor of more precise checks which in this case would be if private_key is not None
ethpm_cli/registry.py
Outdated
registry_uri = URI(f"erc1319://{registry_address}:{chain_id}") | ||
add_registry(registry_uri, alias, config) | ||
activate_registry(registry_uri, config) | ||
explorer_uri = f"http://explorer.ethpm.com/browse/{chain_name}/{registry_address}" |
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 don't think it's right for this function to return the explorer URI. I'd advocate for having it return the address and to do the URI construction elsewhere.
f"Deploying a new registry to {chain_name}, this may take a minute..." | ||
) | ||
# todo: handle tx timeout error gracefully | ||
registry_address = config.w3.pm.deploy_and_set_registry() |
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.
Not needed for this PR but the UX for this would be significantly improved if it gave continuous feedback as the various things happened.
Deploying new registry to ....
Broadcast transaction 0x1234... deploying registry contract....
Waiting for tx to be mined....
Transaction included in block #81234234
Doing other thing....
...
118af99
to
abf4a47
Compare
dc230b2
to
c89c10b
Compare
c89c10b
to
20f270a
Compare
What was wrong?
Added cli commands...
Deploys a new ERC1319 registry to specified blockchain
Release specified package on the currently active registry
Cute Animal Picture