feat(python): unify option arguments in trezorctl

python/.changelog.d/2123.changed

@@ -0,0 +1 @@
+Unify boolean arguments/options in trezorlib commands to on/off

python/src/trezorlib/cli/__init__.py

@@ -150,3 +150,53 @@ def with_client(func: "Callable[Concatenate[TrezorClient, P], R]") -> "Callable[
     # the return type of @click.pass_obj is improperly specified and pyright doesn't
     # understand that it converts f(obj, *args, **kwargs) to f(*args, **kwargs)
     return trezorctl_command_with_client  # type: ignore [cannot be assigned to return type]
+
+
+class AliasedGroup(click.Group):
+    """Command group that handles aliases and Click 6.x compatibility.
+
+    Click 7.0 silently switched all underscore_commands to dash-commands.
+    This implementation of `click.Group` responds to underscore_commands by invoking
+    the respective dash-command.
+
+    Supply an `aliases` dict at construction time to provide an alternative list of
+    command names:
+
+    >>> @click.command(cls=AliasedGroup, aliases={"do_bar", do_foo})
+    >>> def cli():
+    >>>     ...
+
+    If these commands are not known at the construction time, they can be set later:
+
+    >>> @click.command(cls=AliasedGroup)
+    >>> def cli():
+    >>>     ...
+    >>>
+    >>> @cli.command()
+    >>> def do_foo():
+    >>>     ...
+    >>>
+    >>> cli.aliases={"do_bar", do_foo}
+    """
+
+    def __init__(
+        self,
+        aliases: Optional[Dict[str, click.Command]] = None,
+        *args: Any,
+        **kwargs: Any,
+    ) -> None:
+        super().__init__(*args, **kwargs)
+        self.aliases = aliases or {}
+
+    def get_command(self, ctx: click.Context, cmd_name: str) -> Optional[click.Command]:
+        cmd_name = cmd_name.replace("_", "-")
+        # try to look up the real name
+        cmd = super().get_command(ctx, cmd_name)
+        if cmd:
+            return cmd
+
+        # look for a backwards compatibility alias
+        if cmd_name in self.aliases:
+            return self.aliases[cmd_name]
+
+        return None

python/src/trezorlib/cli/device.py

@@ -39,8 +39,8 @@ BACKUP_TYPE = {
 }
 
 SD_PROTECT_OPERATIONS = {
-    "enable": messages.SdProtectOperationType.ENABLE,
-    "disable": messages.SdProtectOperationType.DISABLE,
+    "on": messages.SdProtectOperationType.ENABLE,
+    "off": messages.SdProtectOperationType.DISABLE,
     "refresh": messages.SdProtectOperationType.REFRESH,
 }
 
@@ -259,8 +259,8 @@ def sd_protect(
     device. The options are:
 
     \b
-    enable - Generate SD card secret and use it to protect the PIN and storage.
-    disable - Remove SD card secret protection.
+    on - Generate SD card secret and use it to protect the PIN and storage.
+    off - Remove SD card secret protection.
     refresh - Replace the current SD card secret with a new one.
     """
     if client.features.model == "1":

python/src/trezorlib/cli/settings.py

@@ -14,12 +14,12 @@
 # You should have received a copy of the License along with this library.
 # If not, see <https://www.gnu.org/licenses/lgpl-3.0.html>.
 
-from typing import TYPE_CHECKING, Optional
+from typing import TYPE_CHECKING, Optional, cast
 
 import click
 
 from .. import device, firmware, messages, toif
-from . import ChoiceType, with_client
+from . import AliasedGroup, ChoiceType, with_client
 
 if TYPE_CHECKING:
     from ..client import TrezorClient
@@ -89,30 +89,49 @@ def image_to_tt(filename: str) -> bytes:
     return toif_image.to_bytes()
 
 
+def _should_remove(enable: Optional[bool], remove: bool) -> bool:
+    """Helper to decide whether to remove something or not.
+
+    Needed for backwards compatibility purposes, so we can support
+    both positive (enable) and negative (remove) args.
+    """
+    if remove and enable:
+        raise click.ClickException("Argument and option contradict each other")
+
+    if remove or enable is False:
+        return True
+
+    return False
+
+
 @click.group(name="set")
 def cli() -> None:
     """Device settings."""
 
 
 @cli.command()
-@click.option("-r", "--remove", is_flag=True)
+@click.option("-r", "--remove", is_flag=True, hidden=True)
+@click.argument("enable", type=ChoiceType({"on": True, "off": False}), required=False)
 @with_client
-def pin(client: "TrezorClient", remove: bool) -> str:
+def pin(client: "TrezorClient", enable: Optional[bool], remove: bool) -> str:
     """Set, change or remove PIN."""
-    return device.change_pin(client, remove)
+    # Remove argument is there for backwards compatibility
+    return device.change_pin(client, remove=_should_remove(enable, remove))
 
 
 @cli.command()
-@click.option("-r", "--remove", is_flag=True)
+@click.option("-r", "--remove", is_flag=True, hidden=True)
+@click.argument("enable", type=ChoiceType({"on": True, "off": False}), required=False)
 @with_client
-def wipe_code(client: "TrezorClient", remove: bool) -> str:
+def wipe_code(client: "TrezorClient", enable: Optional[bool], remove: bool) -> str:
     """Set or remove the wipe code.
 
     The wipe code functions as a "self-destruct PIN". If the wipe code is ever
     entered into any PIN entry dialog, then all private data will be immediately
     removed and the device will be reset to factory defaults.
     """
-    return device.change_wipe_code(client, remove)
+    # Remove argument is there for backwards compatibility
+    return device.change_wipe_code(client, remove=_should_remove(enable, remove))
 
 
 @cli.command()
@@ -233,25 +252,39 @@ def experimental_features(client: "TrezorClient", enable: bool) -> str:
 #
 
 
-@cli.group()
-def passphrase() -> None:
+# Using special class AliasedGroup, so we can support multiple commands
+# to invoke the same function to keep backwards compatibility
+@cli.command(cls=AliasedGroup, name="passphrase")
+def passphrase_main() -> None:
     """Enable, disable or configure passphrase protection."""
     # this exists in order to support command aliases for "enable-passphrase"
     # and "disable-passphrase". Otherwise `passphrase` would just take an argument.
 
 
-@passphrase.command(name="enabled")
+# Cast for type-checking purposes
+passphrase = cast(AliasedGroup, passphrase_main)
+
+
+@passphrase.command(name="on")
 @click.option("-f/-F", "--force-on-device/--no-force-on-device", default=None)
 @with_client
-def passphrase_enable(client: "TrezorClient", force_on_device: Optional[bool]) -> str:
+def passphrase_on(client: "TrezorClient", force_on_device: Optional[bool]) -> str:
     """Enable passphrase."""
     return device.apply_settings(
         client, use_passphrase=True, passphrase_always_on_device=force_on_device
     )
 
 
-@passphrase.command(name="disabled")
+@passphrase.command(name="off")
 @with_client
-def passphrase_disable(client: "TrezorClient") -> str:
+def passphrase_off(client: "TrezorClient") -> str:
     """Disable passphrase."""
     return device.apply_settings(client, use_passphrase=False)
+
+
+# Registering the aliases for backwards compatibility
+# (these are not shown in --help docs)
+passphrase.aliases = {
+    "enabled": passphrase_on,
+    "disabled": passphrase_off,
+}

python/src/trezorlib/cli/trezorctl.py

@@ -29,6 +29,7 @@ from ..client import TrezorClient
 from ..transport import enumerate_devices
 from ..transport.udp import UdpTransport
 from . import (
+    AliasedGroup,
     TrezorConnection,
     binance,
     btc,
@@ -57,8 +58,8 @@ LOG = logging.getLogger(__name__)
 
 COMMAND_ALIASES = {
     "change-pin": settings.pin,
-    "enable-passphrase": settings.passphrase_enable,
-    "disable-passphrase": settings.passphrase_disable,
+    "enable-passphrase": settings.passphrase_on,
+    "disable-passphrase": settings.passphrase_off,
     "wipe-device": device.wipe,
     "reset-device": device.setup,
     "recovery-device": device.recover,
@@ -86,16 +87,9 @@ COMMAND_ALIASES = {
 }
 
 
-class TrezorctlGroup(click.Group):
+class TrezorctlGroup(AliasedGroup):
     """Command group that handles compatibility for trezorctl.
 
-    The purpose is twofold: convert underscores to dashes, and ensure old-style commands
-    still work with new-style groups.
-
-    Click 7.0 silently switched all underscore_commands to dash-commands.
-    This implementation of `click.Group` responds to underscore_commands by invoking
-    the respective dash-command.
-
     With trezorctl 0.11.5, we started to convert old-style long commands
     (such as "binance-sign-tx") to command groups ("binance") with subcommands
     ("sign-tx"). The `TrezorctlGroup` can perform subcommand lookup: if a command
@@ -104,16 +98,12 @@ class TrezorctlGroup(click.Group):
     """
 
     def get_command(self, ctx: click.Context, cmd_name: str) -> Optional[click.Command]:
-        cmd_name = cmd_name.replace("_", "-")
-        # try to look up the real name
         cmd = super().get_command(ctx, cmd_name)
         if cmd:
             return cmd
 
-        # look for a backwards compatibility alias
-        if cmd_name in COMMAND_ALIASES:
-            return COMMAND_ALIASES[cmd_name]
-
+        # the subsequent lookups rely on dash-separated command names
+        cmd_name = cmd_name.replace("_", "-")
         # look for subcommand in btc - "sign-tx" is now "btc sign-tx"
         cmd = btc.cli.get_command(ctx, cmd_name)
         if cmd:
@@ -138,7 +128,11 @@ def configure_logging(verbose: int) -> None:
         log.OMITTED_MESSAGES.add(messages.Features)
 
 
-@click.command(cls=TrezorctlGroup, context_settings={"max_content_width": 400})
+@click.command(
+    cls=TrezorctlGroup,
+    context_settings={"max_content_width": 400},
+    aliases=COMMAND_ALIASES,
+)
 @click.option(
     "-p",
     "--path",