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

[mini-RFC] GNO Commands #460

Closed
zivkovicmilos opened this issue Jan 16, 2023 · 9 comments
Closed

[mini-RFC] GNO Commands #460

zivkovicmilos opened this issue Jan 16, 2023 · 9 comments
Assignees
Labels
📖 documentation Improvements or additions to documentation

Comments

@zivkovicmilos
Copy link
Member

As suggested by @moul, I am making this internal mini RFC visible on the GitHub repo for the general public, so we can get as much input as possible.

The original Notion document is available here, and it contains valuable comments not visible in this markdown.

cc @piux2 @anarcher @giansalex et al.

GNO Commands

Created: January 4, 2023 4:04 PM
Tags: Gno, Tech

This document aims to outline the current command structure in the gno repository, and give suggestions on how it can be improved, which areas are lacking clarity user-wise and how some redundancies can be streamlined.

Available Commands

The current command structure can be essentially divided into two groups:

  • commands intended to be used actively by regular users
  • commands intended to be used by gno developers

User commands are the following:

  • gnokey - used primarily for key manipulation, but also for general interaction with gnoland
  • gnoland - runs the blockchain node
  • gnotxport - used for importing and exporting transactions from the local blockchain node storage
  • website - serves the gno website, along with user-defined content
  • logos - intended to be used as a browser

Developer commands are the following:

  • gnoscan - dumps imports from the specified file’s AST
  • genproto - helper for generating .proto implementations
  • gnofaucet - serves a faucet for native network currency
  • gnodev - handy tool for developing gno packages / contracts

General Suggestions

Before diving deep into each command and their respective subcommands, it is worth noting some suggestions that apply to the majority (or all) commands mentioned.

Improve the CLI code workflow

The current command code structure is custom-made, without relying on third-party tools such as Cobra for management. This respects one of the central points of the gno project philosophy – no excessive dependencies, but also increases code complexity, somewhat compromises readability and makes it harder to add or tweak commands down the line.

Cobra usually gets a bad rep because it used to come bundled with the whole command orchestration circus (CLI generation tooling, docs generation tooling, viper etc.). That hasn’t been the case for a long time, it has a much smaller footprint now. Additionally, the gno project seemingly used Cobra in the past, but eventually deprecated the usage.

Nevertheless, for the use-case of the gno project, a much stabler package needs to be sought, one that is not updated as often and that contains the bare minimum features for the command structure to be easy to manage.

Such a package is ff/cli, that has a similar core feature set to Cobra, but without the unnecessary baggage.

What problems would using such a tool specifically solve?

Command line tools in general solve one central problem – standardization. The gno project has its fair share of non-standard command behavior, such as:

  • missing or non-defined default flag values
  • no help output for some commands (ex. genproto, gnoscan)
  • repetitive custom help generation logic (true for all top-level commands), that sometimes doesn’t follow the same output format
  • commands sometimes utilizing relative and not absolute paths (ex. running gnoland inside the build directory)
  • flags that apply to some commands, but are not used for others (ex. BaseOptions are not used for most commands that don’t require direct user input and node interaction)
  • setting mutually exclusive / required flags with ease

Utilizing tools like ff/cli can help standardize usage with minimal changes to the current command functionality, while improving UX.

💡 Note Manfred

Another advantage of switching to ff/cli → better consistency across our tools and other tools; better consistency in terms of code style

I suggest we go with ff/cli, but wrap it from pkgs/commands

Separate repos for complementary functionality

There are several tools bundled in the official gno repo that should be extracted out into separate repositories on the gnolang GitHub organization, as their functionality is only complementary and not essential to modules contained within gno.

The modules that should be moved to their separate respective repositories are:

  • gnoland/website - the static site serving does not need to be bundled with the rest of the codebase, and can benefit from being moved to a separate repository
  • misc/logos - seems to be self-contained, and thus can be moved out
  • cmd/gnotxport - this standalone command tool interacts with the node using web socket connections, and thus does not require core functionality from the rest of the repo to function. Alternatively, it can be incorporated into a separate command as a subcommand (ex. gnoland)
💡 Note Manfred

I’m maybe biased but I love monorepos.

Some reasons:

  • weaken dependencies, allows having inconsistent state i.e., the current master of two repos are not compatible.
  • makes development workflows more complex.
  • Since we’ve only a few deps, it’s convenient to say that you just git clone, go mod download, and tada, you’re ready to work offline.

I suggest we make the same virtual split, not with repos, but with folders and better documentation for now.

Probably it will make more sense to split the day we consider that Gnolang and Gnoland are mature enough and should be split.

Support configuration files

Currently, no command in the gno repository takes in configuration files.

For now, the only command that would benefit from such parameter simplification is the gnoland command, and these improvements are explored in a later section.

Configuration files are usually used to for containing mutable / immutable runtime parameters. The GNO project should consider adding support for configuration file formats (for now, only in the gnoland command) such as:

  • JSON
  • YAML
  • TOML
💡 Note Manfred:

if we go this way, we need to take care of silenced unexpected behaviors.
I think we can define some things like:

  • env var always works but also always has a prefix, i.e. --home is available at $GNOKEY_HOME
  • when/if we support multiple configuration methods, the order is important, i.e. CLI args > config files > env vars > defaults.

Completely drop unused helpers

Commands that are exclusively used as helper code snippets should be moved to script files, and not offered as special runtime binaries. An example of this type of command is goscan, that should be removed from binary generation (and the repository as a command package), as it is a simple helper script.

Additionally, this command does not utilize anything from the rest of the repository and is self-sufficient.

💡 Note Manfred:

Agree, I like having a misc/ or things like x/ for experiments

Specific Command Suggestions

gnotxport

Apart from the initial general suggestion of moving out this functionality into a separate repository, this command can benefit from a performance boost.

Currently, the gnotxport command simply iterates over transactions for importing and exporting, which can be suboptimal. A better approach to this would be to utilize batching of transactions, and compression of these batches (using gzip, for example). Transactions, instead of being exported out one by one to disk, would be batched together, and those batches could be compressed to further save disk space. The import process would be the inverse – decompression and just batch imports.

This way, the functionality of gnotxport is clear – it is used for backing up a local node’s transactions.

This command can also be renamed to gnobackup as it better reflects the functionality it offers.

💡 Note Manfred:

I also had concerns with this tool, for me the highest priority is not about gzip or transactions, but making an automated way to resume, i.e., by using sqlite as backend or automatic file discovery.
Then, we could also add a --watch flag or something allowing us to keep the process in the foreground with polling or another method.

gnobackup sounds better for me, maybe gnosync is even more accurate?

gnoland

It was previously mentioned that the functionality of reading configuration files is not present in the current command structure. The command that could benefit from this the most is the gnoland command.

Specifically, the gnoland command has flags that are used only on initial start, but are not relevant during runtime, that can be extracted out into a genesis.json:

  • chainid - the ID of the chain
  • genesis-balances-file - the initial balance sheet for accounts
  • genesis-txs-file - the initial transactions to be executed

The proposed genesis.json can contain startup logic for the node, that doesn’t need to be specified each time during runtime.

The gnoland command also has 2 flags that should be phased out:

  • skip-failing-genesis-txs - skips transactions that fail from the genesis-txs-file. This functionality should be phased out, as transactions contained in the genesis-txs-file should never fail in the first place
  • skip-start - stops node execution before it has a chance to ramp up (creates directories and sets the stage for the node, but doesn’t allow it to start). Any sort of generation logic can be moved to other commands entirely, and existing commands shouldn’t be modified to have a kill switch for execution, as it complicates the command flow
💡 Note Manfred:

I think genesis-txs.txt file should only contain transaction, because you may want to compose a chain with multiple genesis files and use custom chainid.
But if we go with better config file support, we can imagine shipping genesis.json + params.json together instead of giving a list of params.

About the params that are only useful during init time; let’s prefix them with init-.

gnokey

The gnokey command is discussed last, as it contains the majority of suggestions.

The original concept of the gnokey command was to facilitate private key management for the user, and have helper functionality for interacting with the underlying key-base, like manually signing and verifying transactions.

However, the gnokey command has since grown to be bulky and should be split up into multiple commands / subcommands based on the functionality it provides.

The full list of gnokey subcommands is:

  • add - adds a new private key to the key-base
  • delete - removes a key from the key-base
  • generate - generates a new key, and adds it to the key-base
  • export - exports a private key’s encrypted armor
  • import - imports a private key’s encrypted armor. Pending PR review
  • list - outputs a list of all keys present in the key-base
  • sign - signs a transaction (in a file)
  • verify - verifies a signature of a transaction (from a file)
  • broadcast - broadcasts a signed document
  • query - makes an ABCI query
  • maketx - composes a transaction document to be signed
    • addpkg - uploads a new package
    • call - calls a public function
    • send - sends native currency to an address

There are several subcommands that stand out here, as mainly just requiring private key access to work, but don’t specifically add / remove anything from the key-base, which should be the main functionality of the gnokey command.

Namely, these subcommands are:

  • sign and verify - signing and verifying transactions (files) can be extracted out into a separate command like gnosign, that offers these two subcommands, and takes in a reference to the key-base
  • broadcast and query - these subcommands feel like they belong in the applicative section of gno, possibly with the gnoland command
  • maketx - the maketx command and its subcommands should be moved out to a separate command, much like sign and verify, for example to a command like gnotx

General suggestions for gnokey

  • remove the dry-run flag, as it doesn’t seem to be used as much, and it clutters the code flow
  • the list subcommand should have a better, stylized (formatted) output for present keys. A good package for this sort of output would be columnize, as it has a small and stable footprint
  • the broadcast subcommand takes in a file as a parameter, but it isn’t mentioned anywhere (even in the help usage output), this should be explained better
  • query offers two flags that are still unsupported: height and prove. These flags should be removed for now as they have no function
  • SignBroadcastOptions are verified with each command and subcommand, instead of being verified at the top call level
  • the maketx send command has a flag named send, that is really the amount to be sent, so this flag should be renamed (for ex. amount) to reflect that (and the help description updated)
  • the maketx send command can also be used to broadcast the transaction, instead of simply creating it. This functionality breaks the principle of maketx being used solely for transaction creations, not broadcasts
💡 Note by Ilker:

This is more like a general note;

I think we should maintain the abstraction between the commands and features inside these commands, meaning that commands should have no/less business logic related to the underlying feature but only have logic related to command itself like command args,flags,description or listing data, moving, combining data between different business logics. All the business logic related to a feature could be a separate, reusable Go package.

This abstraction makes things easier when we start building more clients other than the CLI commands e.g. while building the Gno IDE, we could benefit from some of these reusable packages/features.

@zivkovicmilos zivkovicmilos added 📖 documentation Improvements or additions to documentation help wanted labels Jan 16, 2023
@zivkovicmilos zivkovicmilos self-assigned this Jan 16, 2023
@piux2
Copy link
Contributor

piux2 commented Jan 16, 2023

Good stuff, @moul @zivkovicmilos. Thank you for sharing!

The GNO commands provide essential functions for GNO IDE as a product.

Therefore, I break it down into functionalities. What we have implemented and are yet to implement fit here nicely. The feature list could be a starting point to map the features and commands.

  1. Package develop
  • precompile & build
  • testing
  • debugging
  • benchmarking (load, tx gas, storage rent)
  1. Package Deployment
  • contract signing
  • deployment
  1. Interaction with realm packages
  • making calls to on-chain realm API with signed messages
  1. Key management
  • key CRUD
  • recover from mnemonic
  • multi-sig
  1. Launch local testing work
  • launch a local chain
  • import & export states

@moul
Copy link
Member

moul commented Jan 16, 2023

Thank you.

As discussed during today's call. The next step is to validate that our current favorite choice is compatible with our requirements.

Here is a basic breakdown:

  • confirm that ff + ffcli + fftoml can be wrapped in pkg/commands so we can keep only one import path when using commands.
  • confirm that we can support advanced flags, like string arrays.
  • confirm that the toml support is flexible and efficient.

If it can help, I made one package that extended ffcli with an async flag builder -> https://github.com/moul/climan/blob/main/climan.go.

@zivkovicmilos
Copy link
Member Author

zivkovicmilos commented Jan 17, 2023

@moul

Just looked into your climan package, I really love the FlagSetBuilder idea. It's something that just makes sense to define when you're creating the Command structure.

As I can see in the README and code, the differences between the packages are minimal. I wanted to mention that the climan package may probably be outdated, but then I remembered ff/cli is rarely being updated (commit log shows that climan is ~4 commits behind), which is the reason why are favoring this package in the first place 🙂

I'm fine with using either, as the functionality climan has can easily be ported into a wrapper for ff/cli (apart from ContinueOnError, not sure at this point how we would set that if relying on ff/cli)

@zivkovicmilos
Copy link
Member Author

@moul

I've tried playing around with ff/cli and ff/toml, and it seems relatively simple to have support for TOML config files, as well as array values of any arbitrary type.

I've created a small example gist that illustrates both of these functionalities:
https://gist.github.com/zivkovicmilos/6ad3193f0a48ee802f25f1d02baef7ed

In the example, you can set the arr flag variable either from the command line, or a configuration .toml file (that you specify using a CLI flag)

@moul
Copy link
Member

moul commented Jan 18, 2023

Looks great; you can start the PoC with real command creating the pkg/commands wrapper and updating one or two cmd/... packages.

Please make sure to:

  • configure flags with structs instead of inline variables.
  • include in your PoC an example showing two or three depth levels of commands (subcommands).

@zivkovicmilos
Copy link
Member Author

zivkovicmilos commented Jan 31, 2023

@moul

I've opened up a small draft PR that introduces a wrapper for ffcli, and applies this logic to the following commands:

  • gnotxport (because it has 2 subcommands, that share a common root flag)
  • gnoland (only has top-level flags)

Let me know what you think, so I can continue applying this logic to other commands as well. I think it vastly cuts down on the code needed to instrument a command

@zivkovicmilos
Copy link
Member Author

@moul
I've marked the ffcli PR as ready for review - it took a bit longer than I expected because we had a lot of requirements already in place for commands (specifically how we utilize input / output):

#497

I've also stumbled upon a caveat of ffcli while working on this, and it relates to the fact that you cannot mix places for flags and arguments (flags always go first, remaining arguments go last) - this is the related issue on ffcli, and it's due to the way flags are being parsed after the command has been invoked. I've updated all of our docs / workflows / tests to reflect this change

@moul
Copy link
Member

moul commented Apr 11, 2023

Hi @zivkovicmilos, I recommend closing your initial RFC and continuing the discussion on #731 since it's related to this issue. The new proposal refers back to this one.

@zivkovicmilos
Copy link
Member Author

Hi @zivkovicmilos, I recommend closing your initial RFC and continuing the discussion on #731 since it's related to this issue. The new proposal refers back to this one.

Closing now - let's continue the chat on the new issue, as this one is outdated as is

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
📖 documentation Improvements or additions to documentation
Projects
Development

No branches or pull requests

3 participants