Closed clangenb closed 2 years ago
I agree that clap is not elegant and that clap_nested is annoying to debug because of meaningless panic messages
structopt looks neat
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:
And the problems I see with the current structure:
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
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)
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.
the current "cut" defintion of stf and main client is not really applicable for customer customized solutions. This discussion is proabably related to issue #269 - if we decide to keep the main / stf client differentation.
Examples of custom client usages:
Long story short: Should we really keep the stf/ main differentation?
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?
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
I agree with most of your inputs.
I have only two remarks:
cli.rs
. And simply import and concatenate them in the client.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. ;)
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.
Substrate has changed to use clap v3 instead of structopt: https://github.com/paritytech/substrate/pull/10632. I didn't find a reasoning in the related issues, but either way: Does this affect this issue?
If clap v3 is better than structopts, it does :P
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
After Version update
I believe that the CLI can be greatly simplified if structopt is used for the following reasons:
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 https://github.com/scs/substrate-api-client/issues/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.