-
Notifications
You must be signed in to change notification settings - Fork 5
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
Feat: create/import keystore wallet (password-encrypted) + docstrings #164
base: main
Are you sure you want to change the base?
Conversation
"summary": "PR introduces new key management functions and refactors existing code for better security and usability.",
|
d5aba50
to
30bf7a4
Compare
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.
Interesting feature to add.
I added some comments regarding the structure of the code, as a clearer separation of concert would make the code easier to test and reuse by not prompting the user within "deep" functions.
I am confused regarding the terms of password
, passphrase
and mnemonic
in these changes.
Still regarding the use of passphrases and passwords, can thos be of length 64 or start with 0x
?
These new features have no test coverage yet, would it be better to mark the Pull Request as Draft while waiting for those to be implemented ?
Returns: | ||
bytes: The private key as bytes. 7e0c27fff7e434ec5aa47127e7bcdce81c4eba6f3ce980f425b60b1cd019a947 | ||
""" | ||
if Prompt.ask("Import an existing wallet", choices=["y", "n"], default="n") == "y": |
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.
Users should be prompted in the CLI, not in the SDK.
""" | ||
address = None | ||
path.parent.mkdir(exist_ok=True, parents=True) | ||
if path.name.endswith(".key") or "pytest" in sys.modules: |
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.
Why or "pytest" in sys.modules
?
password = Prompt.ask( | ||
"Enter a password to encrypt your keystore", password=True | ||
) |
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.
Users should be prompted in the CLI, not in the SDK.
|
||
private_key = ( | ||
generate_key() | ||
if path.name.endswith(".key") or "pytest" in sys.modules |
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.
Why this mention of pytest
here ?
I will update my comments about the prompts, I forgot that this branch is regarding the SDK and user interactions should go in the CLI |
30bf7a4
to
3c62dda
Compare
keystore wallet compatibility
: Encrypt the private key with a password and store it as a JSON file.missing
, prompt user to either:new
private keyexisting
onefrom a passphrase
.lru_cache
for the last loaded private key (to avoid reentering password too often)