0xPolygonMiden / miden-client

Client library that facilitates interaction with the Miden rollup
MIT License
25 stars 22 forks source link

Refactor commands in favor of git-like usage #333

Closed igamigo closed 1 month ago

igamigo commented 1 month ago

What should be done?

Standardize options vs commands usage in the Clap structures. For example why miden account list and not miden account --list (the latter would be more similar to how git works for example - e.g., git branch --list).

How should it be done?

Refactor structures to reflect this on all top-level commands.

When is this task done?

When we have a flatter command structure that works like the examples above.

Additional context

314

tomyrd commented 1 month ago

Right now the structure of subcommands is:

- account
    - list
    - show
    - new
        - basic-immutable
        - basic-mutable
        - fungible-faucet
        - non-fungible-faucet
    - default
        - set
        - show
        - unset
- import
    - account
    - note
- export
    - note
- init
- notes
    - list
    - show
    - list-consumable
- sync
- info
- tags
    - list
    - add
    - remove
- transaction
    - list
    - new
        - p2id
        - mint
        - p2idr
        - consume-notes

If we want to flatten the structure to only have a single level, one possible option is this (with all second levels being flags):

- account
    - list
    - show
- new-account
    - basic-immutable
    - basic-mutable
    - fungible-faucet
    - non-fungible-faucet
- default-account
    - set
    - show
    - unset
- import
    - account
    - note
- export
    - note
- init
- notes
    - list
    - show
    - list-consumable
- sync
- info
- tags
    - list
    - add
    - remove
- transaction
    - list
- new-transaction
    - p2id
    - mint
    - p2idr
    - consume-notes
bobbinth commented 1 month ago

I like the flatter approach better. A few comments:

Default account

I don't think we need default-account command. When we list accounts, we should probably mark the default account somehow differently. Setting the default account could be done as:

miden account --default <account_id>

I'm not sure we need a command for unsetting the default account, but if we do want to have it, it could be something like:

miden account --default none

Account command

To clarify account command options --list and --show would be flags rather than sub-commands. if no flags are specified miden account would work the same was as miden account --list.

Import/export command

As mentioned in some other discussions, I think import should infer the type of the object it is importing from the file. So, we'd just have miden import (without the need for --note or --account) flags.

Similar comment about export command. The user would be expected to execute it as miden export <note_id>, and if the note_id parameter is invalid, we'd just show an error.

New account command

I'm wondering if we should split this command into two:

miden new-wallet <wallet options>
miden new-faucet <faucet options>

Transaction command

I think we should not have a full form of the command. miden tx (or maybe miden txn) should be the default way we refer to this command.

New transaction command

I'm also wondering if we should split this into several commands:

miden send     # for creating P2ID and P2IDR notes
miden consume  # for consuming input notes for the specified account ID
miden exec     # for executing arbitrary transaction request files
miden mint     # for minting assets; though maybe this should be combined with `exec`.

Notes command

I would not separate --list from --list-consumable: consumable could be just a parameter for --list. For example:

miden note --list
miden note --list all
miden note --list consumable

I'm also thinking that we could add --consume flag which would work as follows:

miden note --consume <list of note IDs>

Maybe there is a way to replace miden consume I mentioned in the previous section with this command.

tomyrd commented 1 month ago

I made a proof of concept PR implementing the first two points plus a version of the new-account command (it's not the final version). I added screenshots with its usage to show how it would behave.

I tried to use clap as much as possible but there are a few validations which now we would have to do by hand as the clap api lacks some functionality for flags/options as opposed to subcommands. Let me know what you think.

tomyrd commented 1 month ago

Import/export command As mentioned in some other discussions, I think import should infer the type of the object it is importing from the file. So, we'd just have miden import (without the need for --note or --account) flags. Similar comment about export command. The user would be expected to execute it as miden export , and if the note_id parameter is invalid, we'd just show an error.

For the import type inference, should I use the extension? Or should I just try to deserialize the file for both objects and use the one that executes correctly?

bobbinth commented 1 month ago

For the import type inference, should I use the extension? Or should I just try to deserialize the file for both objects and use the one that executes correctly?

I'd say for now we just try to decode as both and see what succeeds. In the future, we could do something a bit more sophisticated.