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

trezorctl: unify option arguments #1130

Closed
matejcik opened this issue Jul 24, 2020 · 6 comments · Fixed by #2123
Closed

trezorctl: unify option arguments #1130

matejcik opened this issue Jul 24, 2020 · 6 comments · Fixed by #2123
Assignees
Labels
low hanging fruit Simple, quick task. trezorlib Python library and the command line trezorctl tool.

Comments

@matejcik
Copy link
Contributor

due to the development history, similar subcommands in trezorctl have wildly different parameter conventions:

  • To enable pin, use trezorctl set pin. To disable pin, use trezorctl set pin -r
  • To enable passphrase, use trezorctl set passphrase enabled. To disable, trezorctl set passphrase disabled
  • To enable SD-protect, use trezorctl device sd-protect enable. To disable, use trezorctl device sd-protect disable
  • some commands accept a single required option, other accept a single argument
  • etc.

we should come up with a set of guidelines and make sure all commands conform to them.

@prusnak prusnak added this to the backlog milestone Jul 24, 2020
@matejcik matejcik added the trezorlib Python library and the command line trezorctl tool. label Jul 24, 2020
@tsusanka tsusanka removed the S4 Low label Feb 19, 2021
@tsusanka tsusanka removed this from the backlog milestone Oct 6, 2021
@matejcik matejcik added the low hanging fruit Simple, quick task. label Oct 7, 2021
@grdddj grdddj linked a pull request Feb 11, 2022 that will close this issue
@grdddj
Copy link
Contributor

grdddj commented Mar 24, 2022

Commands for QA to try:

trezorctl set pin
trezorctl set wipe-code
trezorctl set passphrase
trezorctl device sd-protect

Check that all these commands take an arguments on and off as a way to specify the positive and negative effects (trezorctl set pin on vs trezorctl set pin off).

In the case of trezorctl set passphrase, also the enabled/disabled argument should work (to maintain backward compatibility) and be identical to on/off.

In the case of trezorctl set pin and trezorctl set wipe-code, the "legacy" -r flag is still supported as a way to "remove" - the same as off argument. These two cannot conflict with each other, therefore the usage trezorctl set pin on -r should return an error.

@wendys-cats
Copy link

QA NOK - commands trezorctl set passphrase is not valid, nor is trezorctl device sd-protect

Info
trezorctl version 0.13.1 (via poetry from monorepo)
device used: TT 2.5.0
OS: Nix

trezorctl device sd-protect
image

trezorctl set passphrase
image

List of tried commands:
set pin OK
set pin off OK
set pin on OK
set pin -r OK
set wipe-code OK
set wipe code on OK
set wipe code -r OK
set wipe code off OK
set passphrase NOK
set passphrase on OK
set passphrase off OK
set passphrase enabled OK
set passphrase disabled OK
device sd-protect NOK
device sd-protect on OK
device sd-protect off OK
device sd-protection refresh OK

@grdddj
Copy link
Contributor

grdddj commented Apr 6, 2022

commands trezorctl set passphrase is not valid, nor is trezorctl device sd-protect

@wendys-cats With @matejcik I think we agreed that we will not be supporting default arguments in these commands, so without on/off, they should really be invalid. If they were really valid before, we can quite easily enable it for backward-compatibility purposes.

@matejcik
Copy link
Contributor Author

matejcik commented Apr 6, 2022

Correct. I don't think these commands worked without argument before, so this result is OK from my side.

@grdddj
Copy link
Contributor

grdddj commented Apr 6, 2022

don't think these commands worked without argument before

Right, I tried it on master before the change was merged, and none of those worked before:

(.venv)  /home/jmusil/trezor-firmware  ➦ e9364ea0a  trezorctl set passphrase                             
Usage: trezorctl set passphrase [OPTIONS] COMMAND [ARGS]...

  Enable, disable or configure passphrase protection.

Options:
  --help  Show this message and exit.

Commands:
  disabled  Disable passphrase.
  enabled   Enable passphrase.
(.venv)  /home/jmusil/trezor-firmware  ➦ e9364ea0a  trezorctl device sd-protect
Usage: trezorctl device sd-protect [OPTIONS] {enable|disable|refresh}
Try 'trezorctl device sd-protect --help' for help.

Error: Missing argument '{enable|disable|refresh}'. Choose from:
        enable,
        disable,
        refresh

@wendys-cats
Copy link

Great, changing to QA OK then :)

@hynek-jina hynek-jina moved this to ✅ Approved in Firmware Apr 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
low hanging fruit Simple, quick task. trezorlib Python library and the command line trezorctl tool.
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

5 participants