Refactor CLI to use clap v3

[clangenb]

I believe that the CLI can be greatly simplified if structopt is used for the following reasons:

• It removes a lot of boilerplate code
• neatly supports adding external subcommands.
• Simplify code with flattening: Calls to functions like get_chain_api(matches) could simply be replaced by: let api = Api::new(ApiOpts.url, ApiOpts.port) (Note: substrate_api_client does not allow yet to pass url and port separately, see Pass adress and port independently to constructor. scs/substrate-api-client#109)

I personally think changing this is going to ease future bugfixing, as the code is much more readable, so it is goint to be worth the effort.

[brenzi]

I agree that clap is not elegant and that clap_nested is annoying to debug because of meaningless panic messages

structopt looks neat

[haerdib]

If we refactor the client, I suggest to rethink the current structure as a whole. Summarizing what we currently have and what I've understood is their functiontionality:

• main client: handles the general client commands, mostly node interactions, not encrypted
• stf client: handles the customer specific client parts, using trusted operations which are encrypted with enclave shielding key (hence trusted)

And the problems I see with the current structure:

1. alot of duplicate code because the main client depends on the stf to handle TrustedOperations, resulting in the stf client not being able to import main client functions :

error: cyclic package dependency: package `substratee-stf v0.8.0 (/home/bhaerdi/ternoa/sgx-keyvault/stf)` depends on itself. Cycle:
package `substratee-stf v0.8.0 (/home/bhaerdi/ternoa/sgx-keyvault/stf)`
    ... which is depended on by `ternoa-client v0.8.0 (/home/bhaerdi/ternoa/sgx-keyvault/client)`
    ... which is depended on by `substratee-stf v0.8.0 (/home/bhaerdi/ternoa/sgx-keyvault/stf)`
Makefile:168: recipe for target 'bin/app' failed

2. the interface resp. the difference of main and stf client is not obvious for developers. The commands are defined in the stf, but the functionality is implemented in the main client (https://github.com/integritee-network/worker/blob/master/client/src/main.rs#L442)

3. both client files are huge. Can we somehow split the code? Currently there are two solution suggestions / implementation stragies I've seen: 1) have all cmd implemented in the main, but the functionality somewhere differently, 2) for each cmd one file, including the argument defintions and readings. I think both approaches have their positive and negative sides.. and both are better than the current one file implementation.

4. the current "cut" defintion of stf and main client is not really applicable for customer customized solutions. This discussion is proabably related to issue STF Design Enhancement Proposal #269 - if we decide to keep the main / stf client differentation.

Examples of custom client usages:

• PolkaDEX: Client needs to talk directly with the worker. Therefore, some cli cmds are implemented on the trusted stf side: https://github.com/Polkadex-Substrate/polkadexTEE-worker/tree/develop/stf/src/commands. But they are not actually trusted because they do not get encrypted (due to time optimizations). So it was necessary to adapt the main client to not encrypt the direct calls anymore: https://github.com/Polkadex-Substrate/polkadexTEE-worker/blob/develop/client/src/main.rs#L449
• Ternoa: Does not use the originally intended trusted/untrusted differentation, but rather direct / indirect (see the issue for more information) with extended functionalities. Amongst others the additional "nft" subcommand to talk with nft pallet of their node.

[haerdib]

Long story short: Should we really keep the stf/ main differentation?

[haerdib]

Update to Ternoa: After a discussion with brenzi regarding the stf / main client split, we've decided to put all client commands (including the direct worker calls using TrustedOperations) in the client folder. All clap commands will be defined in the main.rs file, where as the functionality will be outsourced to different files, which should make it easy to swap clap with structopt.

This decision was based on the argument that ternoa (just like polkadex) does not use the stf.

@mullefel you're currently working on the architecture refactoring. What are your thoughts on this issue?

[murerfel]

In my refactoring designs, I also extracted the CLI components from the STF crate to the client crate, just as @haerdib suggests. In addition, I'd also pull out the primitives definitions from https://github.com/integritee-network/worker/blob/master/stf/src/lib.rs into a separate crate, to leave the STF crate with only the STF specific stuff, that is located in https://github.com/integritee-network/worker/blob/master/stf/src/sgx.rs

Inside the client, we should make as much code re-usable for the CLI as possible. I've done some of that refactoring in the PolkaDex project (where it's still part of the STF crate).
https://github.com/integritee-network/worker/blob/master/client/src/main.rs is also way too large, and needs to be split up into multiple modules and source files. Where possible, I would suggest to separate CLI parsing from the actual business logic (using abstractions by traits)

Edit: Design proposal for the STF crate and relation to the client

[clangenb]

I agree with most of your inputs.

I have only two remarks:

1. I would not declare the cli in the client main file. We should keep it as empty as possible. I think it is better to define the commands in a separate cli.rs. And simply import and concatenate them in the client.
2. We should analyze the stuff defined in the client closely. It's likely that it is of more generic use than the client itself and does conceptually not belong to it. I have started a primitives directory that contains different crates, where I try to be as fine-grained as possible to define generically usable crates. In this branch, I started this pattern: https://github.com/integritee-network/worker/tree/broadcast-blocks.

I generally came to the philosophy lately that I would like to define as much as possible in other crates whereas the main.rs and lib.rs only assemble the code. This forces a modular pattern and rustc even helps you more because it does not allow cyclic deps. ;)

[murerfel]

I totally agree to keep the main.rs and lib.rs files as slim as possible - and organize the code into smaller modules. If we find there is CLI code to be shared across client and worker, we can do that in a separate, shared crate, otherwise I would just keep it as module in the client.

[haerdib]

Substrate has changed to use clap v3 instead of structopt: paritytech/substrate#10632. I didn't find a reasoning in the related issues, but either way: Does this affect this issue?

[clangenb]

If clap v3 is better than structopts, it does :P

[gaudenzkessler]

Help output before change:

gkessler@devsgx02:~/integritee/worker/bin$ ./integritee-cli --help
integritee-cli 0.8.0
Integritee AG <hello@integritee.network>
interact with integritee-node and workers

USAGE:
    integritee-cli [OPTIONS] [SUBCOMMAND]

FLAGS:
    -h, --help       Prints help information
    -V, --version    Prints version information

OPTIONS:
    -p, --node-port <STRING>              node port [default: 9944]
    -u, --node-url <STRING>               node url [default: ws://127.0.0.1]
    -P, --trusted-worker-port <STRING>    worker direct invocation port [default: 2000]
    -U, --worker-url <STRING>             worker url [default: wss://127.0.0.1]

SUBCOMMANDS:
    balance               query on-chain balance for AccountId
    faucet                send some bootstrapping funds to supplied account(s)
    help                  Prints this message or the help of the given subcommand(s)
    list-accounts         lists all accounts in keystore for the integritee chain
    list-workers          query enclave registry and list all workers
    listen                listen to on-chain events
    new-account           generates a new account for the integritee chain
    print-metadata        query node metadata and print it as json to stdout
    print-sgx-metadata    query sgx-runtime metadata and print it as json to stdout
    shield-funds          Transfer funds from an on-chain account to an incognito account
    transfer              transfer funds from one on-chain account to another
    trusted               trusted calls to worker enclave

stf subcommands depend on the stf crate this has been built against

[gaudenzkessler]

After Version update