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

Tutorial feedback #314

Closed
bobbinth opened this issue May 2, 2024 · 15 comments
Closed

Tutorial feedback #314

bobbinth opened this issue May 2, 2024 · 15 comments
Assignees

Comments

@bobbinth
Copy link
Contributor

bobbinth commented May 2, 2024

A few thoughts after going through the latest tutorials (these are mostly CLI-related comments, many of which could probably be separate issues):

  1. We should print out better confirmation messages after most commands. For example, miden-client account new basic-mutable command should print out some info about the created account (e.g., ID - but maybe something else as well). Other examples:
    a. sync command should print out more info about the sync process (e.g., number of new input notes detected).
    b. tx new command should print out some progress info (e.g., executing transaction, proving transaction, transaction submitted to node etc.).
  2. I do think we should shorten the binary name - possibility to just miden. We already have an issue for this (#109).
  3. It feels like having a top-level import command may be a good idea. So, for example miden-client import note rather than miden-client note-inputs -i.
    a. Similar to point 1, would be good to get some extra info after the note was imported. For example, in addition to "Successfully imported ..." message, might be good to print something like "To view note details execute miden-client input-notes -s 0x123..".
  4. For note details command input-notes -s, would be good to print out more detailed note info (and probably not in a single table row format). A few thoughts
    a. For well-known note scripts (e.g., P2ID), maybe good to print out the script name next to script hash.
    b. For note assets (we should rename "vault" to "assets") we should print out the actual contents of the vault in addition to the hash.
    c. Similar for note inputs, we should print out the actual input values in addition to the inputs hash.
    d. When available, we should print out note metadata (e.g., sender, tag etc.).
  5. Would be good to have default behavior for top level commands. For example, miden-client account should list accounts (same behavior as miden-client account -l).
    a. Also, how do we decide on using options vs. commands? For example why miden-client account list and not miden-client account --list (the latter would be more similar to how git works for example - e.g., git branch --list).
  6. Similar to point 4 above, would be good to have a better printout for account show command. Like we can print out full account details and then we don't need sub-commands to show different parts of the account (also, if we don't have these sub commands, we can probably do account --show instead of account show).
  7. For input notes (e.g., for miden-client input-notes -l command), would be good to show note status to indicate whether the note has been consumed or not. For example, instead of "Commit Height" we could show "Status" with options pending, committed (height), consumed (account_id).
  8. For transactions (e.g., for miden-client tx -l), Status field does not seem to update correctly (or maybe at all - it always says "pending").
  9. For info command, we should print out more information (right now we print out just block height). A few things come to mind:
    a. Client version.
    b. Address of the node we are connected to.
    c. Maybe size of the database file.
    d. Maybe some other info (e.g., total number of accounts, number of pending input notes).
  10. We should have a way to view output notes. We could have output-notes command, or maybe we just have a single notes command and then have options for showing input notes, output notes, or both.
@gobsponge
Copy link
Contributor

image

We should move this^ part to a separate section or towards the end of the tutorial; there are more chances that the reader clears the state at this step and then not be able to do the next tutorial where they should not have reset the state of their local client.

@igamigo
Copy link
Collaborator

igamigo commented May 2, 2024

Agreed on most of these. Some were discussed already or are planned (#304 for point 10).
Point 8 sounds like a bug, it should show committed after syncing (it's thoroughly tested on integration tests, but maybe the CLI is showing something incorrectly).
For point 3, I believe we do show number of new input notes detected. However, it's under an INFO trace log exclusively IIRC.

@mFragaBA
Copy link
Contributor

mFragaBA commented May 3, 2024

Here's a list of tasks to keep track of

Command changes (taking care of this on #322)

  • rename CLI to miden (Change the name of the CLI #109)
  • add command to export output notes (there is already a feat: export notes from output notes table (converted to input notes) #318 for that)
    • consider having a single notes command instead of separating them by input-notes and output-notes
  • add default behavior for top level commands
    • For example, miden-client account should list accounts (same behavior as miden-client account -l).
  • standardize options vs commands usage
    • For example why miden-client account list and not miden-client account --list (the latter would be more similar to how git works for example - e.g., git branch --list)
  • remove account show subcommands and always show everything instead
  • move import subcommands (account and note) into a top level command

Better Feedback messages (taking care of this on #323)

  • After a note is imported, in addition to "Successfully imported ..." message, might be good to print something like "To view note details execute miden-client input-notes -s 0x123..".
  • miden-client account new basic-mutable command should print out some info about the created account (e.g., ID - but maybe something else as well)
  • sync command should print out more info about the sync process (e.g., number of new input notes detected)
  • tx new command should print out some progress info (e.g., executing transaction, proving transaction, transaction submitted to node etc.).

Extra CLI output (#326)

  • for info command, print client version, address of the node, size of the db file, tbd(number of accounts, number of pending input notes)
  • show input note status (pending, committed (height), consumed(account_id_that_consumed_the_note)) on input-notes list
  • print more note info for input-notes show
    • if possible not in a single table row format
    • For well-known note scripts (e.g., P2ID), maybe good to print out the script name next to script hash.
    • For note assets (we should rename "vault" to "assets") we should print out the actual contents of the vault in addition to the hash.
    • For note inputs, we should print out the actual input values in addition to the inputs hash.
    • If available, we should print out note metadata (e.g., sender, tag etc.).

Documentation (0xPolygonMiden/miden-node#351 and 0xPolygonMiden/miden-base#683)

  • move the clear state section of the get started tutorials to the end of both offchain and onchain tutorials
  • In the tutorial, there is a minor inconsistency in the number of tokens received from the faucet in one of the figures. Should be 333 instead of 100.

Fixes

  • For transactions (e.g., for miden-client tx -l), Status field does not seem to update correctly (or maybe at all - it always says "pending").

@igamigo igamigo moved this to In Progress in User's testnet May 6, 2024
@mFragaBA
Copy link
Contributor

mFragaBA commented May 6, 2024

@bobbinth quick question regarding this:

Also, how do we decide on using options vs. commands? For example why miden-client account list and not miden-client account --list (the latter would be more similar to how git works for example - e.g., git branch --list).

The current way of using commands/options seems rather consistent.

  • commands are used to select an action to perform
  • options modify how the action is performed

Perhaps there's a specific way users are getting confused by the way we handle commands/options, which would clarify a bit more if we need to use it.

Edit: maybe the tx new and account new do not follow the philosophy mentioned above. We could re-consider those (keeping the account type as a subcommand is a bit more simpler to manage on the code side though)

@mFragaBA
Copy link
Contributor

mFragaBA commented May 6, 2024

also, a side question regarding

consider having a single notes command instead of separating them by input-notes and output-notes

What would happen to the commands currently under the input-note subcommand? One way I can think of handling those is:

  • For list we fetch both input and output notes, and we show in a column whether it an input, output or both. Or we first show all the input notes and then all the output notes. Or we could filter based on an option (--input/--output/--all).
  • For show we do the same as above.
  • for export we only fetch output notes (already done in feat: export notes from output notes table (converted to input notes) #318)
  • for list consumable we only list input notes

@bobbinth
Copy link
Contributor Author

bobbinth commented May 7, 2024

The current way of using commands/options seems rather consistent.

  • commands are used to select an action to perform
  • options modify how the action is performed

Perhaps there's a specific way users are getting confused by the way we handle commands/options, which would clarify a bit more if we need to use it.

This may be more of a bias that I have because of using git (e.g., I was expecting miden-client account --list to work), and the issue may be more that we are using nested commands rather than relatively flat ones. For example, maybe instead of miden-client account new <OPTIONS> it should be something like miden-client new-account<OPTIONS> - but I think that's a bigger discussion.

For list we fetch both input and output notes, and we show in a column whether it an input, output or both. Or we first show all the input notes and then all the output notes. Or we could filter based on an option (--input/--output/--all).

This may be difficult: input and output notes may have different data available to be displayed.

One option may be to show minimal information about each note (e.g., ID and status) and maybe show them in two columns (input notes on the left and output notes on the right).

@Al-Kindi-0
Copy link
Collaborator

In the tutorial, there is a minor inconsistency in the number of tokens received from the faucet in one of the figures. Should be 333 instead of 100.

@mFragaBA
Copy link
Contributor

mFragaBA commented May 7, 2024

In the tutorial, there is a minor inconsistency in the number of tokens received from the faucet in one of the figures. Should be 333 instead of 100.

added this to the list above

@mFragaBA
Copy link
Contributor

mFragaBA commented May 7, 2024

This may be difficult: input and output notes may have different data available to be displayed.

One option may be to show minimal information about each note (e.g., ID and status) and maybe show them in two columns (input notes on the left and output notes on the right).

I implemented this in #318 if you want to check it out. Basically I added a Exportable? column to the ones we showed. Then I fetch both input and output notes and try to match them by ID. for each one I fill the table info by checking the combinations of having both input and output note or either one and try to fill as much info as possible. While it seems to work, it does make the CLi code a bit more complex for the list and show commands. The Client stays the same though

@bobbinth
Copy link
Contributor Author

bobbinth commented May 7, 2024

I implemented this in #318 if you want to check it out. Basically I added a Exportable? column to the ones we showed.

If not too difficult, could you post a screenshot of how this looks like?

@tomyrd
Copy link
Collaborator

tomyrd commented May 8, 2024

I implemented this in #318 if you want to check it out. Basically I added a Exportable? column to the ones we showed.

If not too difficult, could you post a screenshot of how this looks like?

Here:
image

@tomyrd tomyrd linked a pull request May 8, 2024 that will close this issue
@tomyrd
Copy link
Collaborator

tomyrd commented May 8, 2024

For transactions (e.g., for miden-client tx -l), Status field does not seem to update correctly (or maybe at all - it always says "pending").

I can't seem to replicat this error. The transaction just takes some time to process in the node so you have to wait a little before the sync updates the tx list to "Commited".

@bobbinth
Copy link
Contributor Author

bobbinth commented May 9, 2024

I can't seem to replicat this error. The transaction just takes some time to process in the node so you have to wait a little before the sync updates the tx list to "Commited".

OK - let's consider this closed for now. If it comes up again in the future, I'll open a separate issue.

Here:
image

I would still probably go with two separate lists - maybe something that looks like so:

image

Basically, two independent lists side-by-side.

This is probably a relatively big change - so, maybe makes sense to break it out into a stand-alone issue.

igamigo added a commit to 0xPolygonMiden/miden-node that referenced this issue May 9, 2024
Addresses changes following 0xPolygonMiden/miden-client#314, which changed the name of the client into `miden`
@igamigo
Copy link
Collaborator

igamigo commented May 9, 2024

All points were addressed. PRs are referenced here.

The remaining items have their own issues now to discuss and implement separately:

Once the remaining PRs are merged we can close this and continue each work item in its own issue.

igamigo added a commit to 0xPolygonMiden/miden-node that referenced this issue May 9, 2024
Addresses changes following 0xPolygonMiden/miden-client#314, which changed the name of the client into `miden`
@mFragaBA mFragaBA moved this from In Progress to In Review in User's testnet May 15, 2024
@mFragaBA
Copy link
Contributor

closing as all issues have been solved for v0.3

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

No branches or pull requests

6 participants