Separate auth command into more useful subparsers #84
Conversation
njgheorghita
force-pushed
the
simple-auth-setup
branch
3 times, most recently
from
10d356a to
6445b95
Compare
njgheorghita
force-pushed
the
simple-auth-setup
branch
from
6445b95 to
a5bfc50
Compare
pipermerriam
left a comment
pipermerriam
left a comment
There was a problem hiding this comment.
I'd like to give it another pass when ready.
|
|
||
|
|
||
| def link_keyfile(keyfile_path: Path) -> None: | ||
| if valid_keyfile_exists(): |
There was a problem hiding this comment.
I find it odd that this branch of the logic completely ignores the function argument keyfile_path. Haven't gotten to the place where this function is used yet so maybe this is benign...
|
|
||
| def unlink_keyfile() -> None: | ||
| if not valid_keyfile_exists(): | ||
| cli_logger.info("Unable to unlink keyfile: empty keyfile found.") |
There was a problem hiding this comment.
The phrase: "empty keyfile found" might be misleading. Should this say something like "no keyfile found" and maybe include the search path so that they know where it was supposed to be?
| else: | ||
| keyfile_path = get_keyfile_path() | ||
| address = get_authorized_address() | ||
| keyfile_path.write_text("") |
There was a problem hiding this comment.
I find this behavior surprising. I would expect it to remove the file...
| f"Keyfile detected. Please use `ethpm auth unlink` to delete this " | ||
| "keyfile before initializing a new one." | ||
| ) | ||
| return |
There was a problem hiding this comment.
I think expected behavior would be for the process to return a non-zero return code so that if this is run in some sort of script that this logic branch produces a detectable failure.
| ) | ||
| if not agreement: | ||
| cli_logger.info("Aborting keyfile initialization.") | ||
| return |
There was a problem hiding this comment.
Same here, this should probably result in the CLI exiting with a non-zero return code..
| cli_logger.info("Aborting keyfile initialization.") | ||
| return | ||
|
|
||
| private_key = to_bytes(text=input("Please enter your 32-length private key: ")) |
There was a problem hiding this comment.
I don't think this is ok. I know you are probably expecting people to copy/paste which might be fine if it could be enforced, but this will also result in people typing the key in, or pasting it with an accidental newline at the end or typing it in its hexidecimal representation, or any number of insane things we can't think of.... I think that if someone wants to bring in a raw private key we should require them to do the heavy lifting of creating the keyfile themselves so that we're not responsible for mistakes by encouraging this pattern... thoughts?
cc @holiman
| ethpm_xdg_root = get_xdg_ethpmcli_root() | ||
| ethpm_cli_keyfile_path = ethpm_xdg_root / KEYFILE_PATH | ||
| with atomic_replace(ethpm_cli_keyfile_path) as file: | ||
| file.write(json.dumps(keyfile_json)) |
There was a problem hiding this comment.
nitpick you can change this to json.dump(keyfile_json, file) which is maybe just slightly more idiomatic.
| tmp_keyfile = Path(tempfile.NamedTemporaryFile().name) | ||
| tmp_keyfile.write_text(keyfile_path.read_text()) | ||
| tmp_keyfile.replace(ethpm_cli_keyfile_path) | ||
| with atomic_replace(ethpm_cli_keyfile_path) as file: |
There was a problem hiding this comment.
orthogonal
I'm thinking we should move this utility into eth-utils as atomic_replace is a pretty useful idiom.
| """ | ||
| keyfile = get_keyfile_data() | ||
| return keyfile["address"] | ||
| return add_0x_prefix(keyfile["address"]) |
There was a problem hiding this comment.
thoughts on making this a checksum address? It's getting logged in a bunch of places which means it will end up in people's clipboards. Since people will need to fund this address, they're likely to copy/pasta it to send funds...
|
|
||
|
|
||
| def auth_init_action(args: argparse.Namespace) -> None: | ||
| Config(args) |
There was a problem hiding this comment.
This is a code smell to me since I'm assuming you are relying on some sort of side-effect action that happens by instantiating this object? If so I would advocate making this API more explicit so that it is clear what is happening here and why. At minimum a code comment, but I think that the side effect nature of object instantiation isn't ideal and should potentially be moved into something like a classmethod which handles the side-effect stuff and keep the validation checks in the __init__...
2 participants
What was wrong?
ethpm authrequired users to link an already generated encrypted keyfile. Now, the cli will handle that for you.A couple new commands introduced...
ethpm auth- displays currently authenticated addrethpm auth link --keyfile-path ./path/to/keyfile- will link an existing keyfileethpm auth unlink- will unwrite whatever keyfile is stored in the xdg direthpm auth init- will display a warning about private key handling, then prompt user for private key / password and store encrypted keyfile json in xdg directoryCute Animal Picture