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

Feat: create/import keystore wallet (password-encrypted) + docstrings #164

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

philogicae
Copy link
Member

@philogicae philogicae commented Sep 9, 2024

  • Add keystore wallet compatibility: Encrypt the private key with a password and store it as a JSON file.
  • When private key file is missing, prompt user to either:
    1. Create a new private key
    2. Import an existing one
    3. Import it from a passphrase.
  • lru_cache for the last loaded private key (to avoid reentering password too often)

@github-actions github-actions bot added the RED This PR is complex and may require more time to review. label Sep 9, 2024
Copy link

github-actions bot commented Sep 9, 2024

"summary": "PR introduces new key management functions and refactors existing code for better security and usability.",
"details": [
"The PR introduces two new functions, create_key and save_key, which provide a more user-friendly way to manage private keys.",
"create_key allows the user to either import an existing private key or generate a new one.",
"save_key saves the private key to a file, supporting both unencrypted .key files and encrypted .json keystore files.",
"The existing get_fallback_private_key function has been refactored to use the new load_key function, which supports loading keys from both unencrypted and encrypted files.",
"The PR also includes changes to the configuration settings to support the new file formats for private keys.",
"The refactoring aims to improve the security and usability of the private key management in the SDK."
],
"highlight": [
"def create_key() -> bytes:",
"def save_key(private_key: bytes, path: Path):",
"def load_key(private_key_path: Path) -> bytes:",
"settings.PRIVATE_KEY_FILE = Path(settings.CONFIG_HOME, "private-keys", pk_file)"
]
}

Copy link
Member

@hoh hoh left a 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 ?

src/aleph/sdk/conf.py Outdated Show resolved Hide resolved
src/aleph/sdk/chains/common.py Outdated Show resolved Hide resolved
src/aleph/sdk/chains/common.py Outdated Show resolved Hide resolved
Returns:
bytes: The private key as bytes. 7e0c27fff7e434ec5aa47127e7bcdce81c4eba6f3ce980f425b60b1cd019a947
"""
if Prompt.ask("Import an existing wallet", choices=["y", "n"], default="n") == "y":
Copy link
Member

@hoh hoh Sep 11, 2024

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.

src/aleph/sdk/chains/common.py Outdated Show resolved Hide resolved
"""
address = None
path.parent.mkdir(exist_ok=True, parents=True)
if path.name.endswith(".key") or "pytest" in sys.modules:
Copy link
Member

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 ?

Comment on lines +209 to +213
password = Prompt.ask(
"Enter a password to encrypt your keystore", password=True
)
Copy link
Member

@hoh hoh Sep 11, 2024

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.

src/aleph/sdk/chains/common.py Outdated Show resolved Hide resolved

private_key = (
generate_key()
if path.name.endswith(".key") or "pytest" in sys.modules
Copy link
Member

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 ?

src/aleph/sdk/conf.py Show resolved Hide resolved
@hoh
Copy link
Member

hoh commented Sep 11, 2024

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 aleph-client.

@philogicae philogicae marked this pull request as draft September 11, 2024 08:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
RED This PR is complex and may require more time to review.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants