-
Notifications
You must be signed in to change notification settings - Fork 21
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
[CLI] New CLI tool for node operators #268
base: dev
Are you sure you want to change the base?
Conversation
After discussion with @hoh regarding the garbage collector, we decided to do garbage collection differently from what is proposed in #250. The idea is that the GC will be invoked manually for the time being. To make this easier for node operators, we decided to introduce a new CLI tool that will implement useful commands for node operators. This PR is the initial work for this, before the introduction of the GC. To do / To discuss:
|
f08a6c0
to
50744dd
Compare
50744dd
to
fdf844f
Compare
Problem: web wallets do not allow signing raw messages. Instead, they require binary payloads in a specific format. Solution: support Micheline-style signatures, i.e. signatures supported by wallets like Beacon. Users can now use Micheline or raw signatures by specifying the `signature.signingType` field to "micheline" or "raw". By default, "raw" is assumed. Co-authored-by: Mike Hukiewitz <[email protected]>
We now provide a CLI tool that integrates all the operations commonly performed by node operators. Currently, this CLI allows to: * generate private keys for the node, replacing a functionality that was implemented in the CCN main app directly. * run migrations, replacing the config updater script.
fdf844f
to
50798e3
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.
A few comments, looks good overall 👍
class CliConfig: | ||
config_file_path: Path | ||
key_dir: Path | ||
verbose: bool |
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.
No default ?
from typing import cast | ||
from aleph.ccn_cli.services.keys import generate_keypair, save_keys | ||
|
||
keys_ns = typer.Typer() |
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 naming does not seem clear to me. The habit in Typer is app = typer.Typer()
, so I would base the naming on that.
keys_ns = typer.Typer() | |
app = typer.Typer() |
The keys will be created in the key directory. You can modify the destination | ||
by using the --key-dir option. | ||
""" | ||
cli_config = cast(CliConfig, ctx.obj) |
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 casting is not obvious. Would it be worth adding a comment to explain it ?
by using the --key-dir option. | ||
""" | ||
cli_config = cast(CliConfig, ctx.obj) | ||
print(cli_config) |
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 print
?
|
||
|
||
@keys_ns.command() | ||
def generate(ctx: typer.Context): |
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.
Replace generate
with the simpler create
term ?
def generate(ctx: typer.Context): | |
def create(ctx: typer.Context): |
except Exception as e: | ||
typer.echo(f"{command} failed: {e}.", err=True) | ||
if cli_config.verbose: | ||
typer.echo(format_exc()) |
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.
Do we really want to only display the exception in verbose mode ? I am wondering if it would not make it easier that any user can copy paste the traceback of an error without running the command again 🤔
return config | ||
|
||
|
||
def import_module_from_path(path: str) -> ModuleType: |
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.
def import_module_from_path(path: str) -> ModuleType: | |
def import_module_from_path(path: Path) -> ModuleType: |
import typer | ||
|
||
from .cli_config import CliConfig | ||
from .commands.keys import keys_ns |
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.
If using the app
naming convention in the module, this could become:
from .commands.keys import keys_ns | |
import .commands.keys.app as keys_app |
else:
from .commands.keys import keys_ns | |
import . import commands.keys | |
... | |
app.add_typer(commands.keys.app, name="keys", help="Operations on private keys.") |
return key_pair | ||
|
||
|
||
def save_keys(key_pair: KeyPair, key_dir: str) -> None: |
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.
Use pathlib.Path
?
def generate_keypair() -> KeyPair: | ||
""" | ||
Generates a new key pair for the node. | ||
""" | ||
key_pair = create_new_key_pair() | ||
return key_pair |
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 proxy function ?
def generate_keypair() -> KeyPair: | |
""" | |
Generates a new key pair for the node. | |
""" | |
key_pair = create_new_key_pair() | |
return key_pair | |
generate_keypair = create_new_key_pair |
d9f920c
to
3d016e0
Compare
48b3f90
to
11bf24a
Compare
We now provide a CLI tool that integrates all the operations
commonly performed by node operators. Currently, this CLI
allows to:
that was implemented in the CCN main app directly.