Implement Interactive CLI #357

patrickkuo · 2022-02-04T03:18:19Z

A CLI implementation using rustyline as line processor and structopt as the command processor.
This allow the cli client to work in both shell and no-shell mode

2 new binaries
fastx -- for starting the network and handle genesis
wallet -- for managing accounts, objects, and interact with client apis

Progress:

all commands are working in both shell and no-shell mode.
Added fastx binary for genesis and starting the network
autocomplete working using rustyline
Move call use command arg instead of config file -- done
Fix license problem -- done
test move call and publish code path

WIP:

Todos:

[fastx CLI] Start authorities in docker container #398
[fastx CLI] Add customisation to genesis #400
[fastx CLI] Cargo task for installing fastx in user's PATH #399
[fastx CLI] Improve demo wallet's UX and add option to choose between pretty print or json output #403
Work with @arun-koshy to refactor any sharable code with rest server

sblackshear · 2022-02-04T20:38:30Z

fastpay/src/cli_commands.rs

+    about = "A Byzantine fault tolerant payments chain with low-latency finality and high throughput",
+    rename_all = "kebab-case"
+)]
+pub enum ClientCommands {


For this, I think it would be good to start from a very minimal set of commands that we all agree on the structure of + that the REST API can also support (e.g. the commands suggested here) instead of trying support everything that the old CLI did (since I think there is much that we'd like to get rid of or change).

@patrickkuo I've already ported the commands from the GDC doc here in my other PR and it works with completion, etc.
You can just copy it over as we discussed and use the skeleton provided.
https://github.com/MystenLabs/fastnft/blob/ee8def7664a021639f8fb162ac5e2e5c777f5f77/fastpay/src/cli.rs#L17

@sblackshear agree, all the commands will adhere to the doc when it's done

@oxade are you sure completion works? I have checked out yours but its not working..... (completion is auto completing commands when you press the tab key).

I have everything wired up with working queries and transfer, now working on refactoring the genesis and account creation, also working on better UX and unit tests

oxade · 2022-02-07T15:47:44Z

Great, thanks for working avidly on this.
Few things from my sample runs:

I sometimes get this even when a command succeeds. If they're innocuous, we should not show them to the same CLI. We can pipe the authority errors to a different view

2022-02-07T15:32:32.412017Z ERROR fastx_network::transport: Error while reading TCP stream: Connection reset by peer (os error 54)
2022-02-07T15:32:32.413751Z ERROR fastx_network::transport: Error while reading TCP stream: Connection reset by peer (os error 54)
2022-02-07T15:32:32.415160Z ERROR fastx_network::transport: Error while reading TCP stream: Connection reset by peer (os error 54)
2022-02-07T15:32:32.416572Z ERROR fastx_network::transport: Error while reading TCP stream: Connection reset by peer (os error 54)

Hitting enter with no command should do nothing as below. This is typically used when users want to space out their commands for visual ease

fastx>-$ 
fastx>-$ 
fastx>-$

In this CLI, hitting ret prints a multi-line help which is not really ideal

Not critical now but output formatting can be improved and made uniform, which also helps testing. See the example I had using Pretty Table

huitseeker

As a general statement/FYI: camino is a heck of a lot more ergonomic in argument positions than the str soup we have to deal with (fallibly!) here (and that's valid for the whole code base, so you may want to take this with a grain of scope).

One thing I'd also love to see in future iterations of this is a nod towards #370.

huitseeker · 2022-02-07T23:06:58Z

fastpay/Cargo.toml

+move-core-types = { git = "https://github.com/diem/move", rev = "62b5a5075378ae6a7102bbfc1fb33b57ba6125d2", features = ["address20"] }
+move-bytecode-verifier = { git = "https://github.com/diem/move", rev = "62b5a5075378ae6a7102bbfc1fb33b57ba6125d2" }
+
+shellfish = { version = "0.6.0", features = ["async"] }


I'm afraid that as cargo-deny indicates, this imports the BSL-licensed str-buf, error-code and clipboard-win, which IIUC revert to a copyleft license (GPL v2) after 4y. I don't think this is something we're ok with at this stage..
/cc @sblackshear

I am removing shellfish as we only use it as a line parser, which can be handled ourselves, however I am still using rustyline, which might contain some of these dependencies..... I will see if I can replace it...

Digging into this, BSL is actually the SPDX of the Boost software license, which is what those crates are using (I did eyeball each repo's LICENSE file).

The Boost Software License is fine (it's an MIT/BSD style w/o copyright redistribution).
You can add the SPDX identifier for the Boost software license here:
https://github.com/MystenLabs/fastnft/blob/main/deny.toml#L78-L88

The SPDX is defined here: https://spdx.org/licenses/

fastpay/src/config.rs

fastpay/src/fastx.rs

huitseeker · 2022-02-07T23:23:47Z

fastpay/src/fastx.rs

+    let mut config =
+        NetworkConfig::read_or_create(&network_conf_path).expect("Unable to read user accounts");
+
+    match options.command {


It's going to be a bit hard to maintain the functionality in the handler, could we export the scenario run in each command to a different file, or at least a different function?

huitseeker · 2022-02-07T23:24:06Z

fastpay/src/fastx.rs

+impl NetworkConfig {
+    pub fn read_or_create(path: &str) -> Result<Self, anyhow::Error> {
+        let path_buf = PathBuf::from(path);
+        Ok(if path_buf.exists() {


Same comment on logging with tracing.

huitseeker · 2022-02-07T23:25:15Z

fastpay/src/fastx.rs

+    config_path: String,
+}
+
+impl NetworkConfig {


Could we define a common interface trait Config which contains the common functionality and then just implement the specifics here?

I will make one and put it in utils.rs, will be useful for rest server as well
FYI @arun-koshy

huitseeker · 2022-02-07T23:27:50Z

fastpay/src/wallet.rs

+}
+
+#[tokio::main]
+async fn main() -> Result<(), Box<dyn std::error::Error>> {


That works, but returning anyhow::Error rather than a manual Box<dyn std::error::Error should be more ergonomic.

huitseeker · 2022-02-07T23:30:52Z

fastpay/src/wallet_commands.rs

+        println!("Owner: {:#?}", object.owner);
+        println!("Version: {:#?}", object.version().value());
+        println!("ID: {:#?}", object.id());
+        println!("Readonly: {:#?}", object.is_read_only());
+        println!(
+            "Type: {:#?}",
+            object
+                .data
+                .type_()
+                .map_or("Type Unwrap Failed".to_owned(), |type_| type_
+                    .module
+                    .as_ident_str()
+                    .to_string())
+        );
+        if deep {
+            println!("Full Info: {:#?}", object);
+        }


Is there a formatting routine missing in the client, that could be reused here?

patrickkuo · 2022-02-08T16:57:40Z

This PR is code complete

Although I am still working on unit test, readme.md and adding more code comments, I will mark this as ready to review before it grow too big....

Thanks @huitseeker for the early review 😂, I have addressed most of your comments and ready for round 2.

patrickkuo · 2022-02-08T18:18:04Z

thanks @huitseeker for figuring out BSL License is ok to include, I will revert the line processor back to rustyline as it's more stable and mature then reedline

huitseeker

So sorry for the license confusion I originated!

I can foresee myself debugging this after an error, and lacking some info about the setup and details of what happened.

How do I figure out the following from secondary output (e.g. logs, or at least stderr):

what are the processes that have been launched?
what their type is?
what their (network) location is?

The feedback may not be for this CLI tool: you may want to punt that it's the role of the spawn of e.g. a server to make its context clear (and please open issues for anything missing there). But note tracing spans can help here:

docs,
example,

huitseeker · 2022-02-08T18:06:38Z

fastpay/src/fastx.rs

+        make_server(&authority, &committee, &preload_objects, config.buffer_size).await;
+    }
+
+    let wallet_config = WalletConfig::create("./wallet.conf")?;


Does re-running this make the assumption we are in a clean directory?

yes, genesis assume it's writing to a clean directory, it does a check at line 109 to make sure there are no existing network configured, if there are no authorities configured but there are existing wallet config file, it will be overwritten.

huitseeker · 2022-02-08T18:16:54Z

fastpay/src/config.rs

+    pub committee_config: CommitteeConfig,
+}
+
+#[derive(Serialize, Deserialize)]


Nit:
I expect that outside contexts where we generate a dummy account (and want to write the generated key pair to disk as part of generation), we'll want to skip serializing the key in the future.

No need to address this as part of this PR, I'm just writing this so you don't introduce a #[derive(Serialize)] you could avoid.

huitseeker · 2022-02-08T18:24:31Z

fastpay/src/shell.rs

+            prompt_indicator: "fastx>-$ ".to_string(),
+        };
+
+        'shell: loop {


Oh wow, this is our own repl shell.

huitseeker · 2022-02-08T18:24:39Z

fastpay/src/shell.rs

+
+impl<P: Display, S: Send, H: AsyncHandler<S>> Shell<P, S, H> {
+    pub async fn run_async(&mut self) -> Result<(), anyhow::Error> {
+        let mut keybindings = default_emacs_keybindings();


Not emacs...

gdanezis

This is very cool, and a key part of the demo.

The more I look at this the more I want to build a Python library to call fastx client functions, since then we can use higher productivity libraries and tools to write tools like this in. Watching you iterate through strings (there must be a regex lib) broke my heart.

gdanezis · 2022-02-09T12:02:09Z

fastpay/src/fastx.rs

+
+#[derive(StructOpt)]
+#[structopt(
+    name = "FastX",


I guess this is really a "fastx local" tool right? Not the main server that people should run.

gdanezis · 2022-02-09T12:04:01Z

fastpay/src/fastx.rs

+    .await;
+
+    for object in pre_load_objects {
+        state.init_order_lock(object.to_object_reference()).await;


Do we ever use insert_object without the lock? Maybe we should integrate the lock update inside the insert. Different PR though.

gdanezis · 2022-02-09T12:04:40Z

fastpay/src/shell.rs

+            prompt_indicator: "fastx>-$ ".to_string(),
+        };
+
+        'shell: loop {


Oh wow, this is our own repl shell.

gdanezis · 2022-02-09T12:05:55Z

fastpay/src/shell.rs

+            };
+
+            // Runs the line
+            match Self::unescape(line.trim()) {


Is there not a standard regex library in Rust to make your life easier here?

I don't like this as well 😞, let me check if I can do it with regex or maybe other libs.

gdanezis · 2022-02-09T12:06:33Z

fastpay/src/shell.rs

+        let mut string = false;
+
+        // Go through each char in the string
+        for c in command.chars() {


You need regex here too to build the tokenizer

* use conf file instead of account/ committee files and arg values * removed create accounts command * added add-address command * removed benchmark from CLI commands * revert client.rs code * more refactoring * got structopt working with shellfish add sync client command added transfer command added objects command cli refactoring

* added fastx binary for genesis and start authorities

* new PortAllocator

* command completion using rustyline * new Config trait for easy loading/storing config files * address some PR comment

This reverts commit 371dce41

* allow BSL-1.0 license

address PR comments

oxade · 2022-02-09T14:20:56Z

Glad to see how far this has come. Nice!

* chore(deps): bump bindgen from 0.59.2 to 0.60.1 Bumps [bindgen](https://github.com/rust-lang/rust-bindgen) from 0.59.2 to 0.60.1. - [Release notes](https://github.com/rust-lang/rust-bindgen/releases) - [Changelog](https://github.com/rust-lang/rust-bindgen/blob/master/CHANGELOG.md) - [Commits](rust-lang/rust-bindgen@v0.59.2...v0.60.1) --- updated-dependencies: - dependency-name: bindgen dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <[email protected]> * update workspace-hack hakari Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Anastasios Kichidis <[email protected]>

patrickkuo mentioned this pull request Feb 4, 2022

RFC: CLI Part 1: Add interactive CLI stubs and mock output #354

Closed

sblackshear reviewed Feb 4, 2022

View reviewed changes

patrickkuo self-assigned this Feb 5, 2022

huitseeker mentioned this pull request Feb 5, 2022

[documentation] Document our binaries in a README #370

Closed

patrickkuo force-pushed the pat/interactive-cli-and-refactoring branch 3 times, most recently from 792715d to 0f40b18 Compare February 6, 2022 18:08

huitseeker reviewed Feb 7, 2022

View reviewed changes

patrickkuo force-pushed the pat/interactive-cli-and-refactoring branch 2 times, most recently from a993789 to f556f74 Compare February 8, 2022 15:46

patrickkuo marked this pull request as ready for review February 8, 2022 16:57

patrickkuo requested review from sblackshear, gdanezis, huitseeker, arun-koshy and oxade February 8, 2022 16:58

huitseeker reviewed Feb 8, 2022

View reviewed changes

huitseeker mentioned this pull request Feb 8, 2022

chore: start replacement FastNFT => Sui #395

Merged

patrickkuo force-pushed the pat/interactive-cli-and-refactoring branch from 277370a to f86c986 Compare February 9, 2022 11:47

gdanezis approved these changes Feb 9, 2022

View reviewed changes

patrickkuo added 7 commits February 9, 2022 13:19

get_owned_objects don't need to be async

7eeb070

fixup after rebase

00f973e

* port should be u16

84958f6

* added fastx binary for genesis and start authorities

* more verbose

74c610d

* remove portpicker

b32e67b

* new PortAllocator

* move call using args instead of config file

e29ef04

patrickkuo added 14 commits February 9, 2022 13:19

* minor improvements

91c1b24

* remove shellfish

e9f31f2

* command completion using rustyline * new Config trait for easy loading/storing config files * address some PR comment

fixup after rebase

ecd31c0

replace rustyline with reedline to avoid BSL license

24edafc

fixup after rebase

6ebf1a4

* command completion support multiple command flag

b51ebea

Revert "replace rustyline with reedline to avoid BSL license"

60775f3

This reverts commit 371dce41

* fixup after revert

03355f2

* allow BSL-1.0 license

* allow BSL-1.0 license

3c19b7e

* add back clear command

636d8de

* bugfix genesis not writing to wallet.conf

42be3d4

fixup after rebase

b30b3d0

use unescape crate to handle unescaping string

d39d48e

address PR comments

fixup after rebase

8c4eb5c

patrickkuo force-pushed the pat/interactive-cli-and-refactoring branch from 6e41a12 to 8c4eb5c Compare February 9, 2022 13:36

patrickkuo merged commit 232da92 into main Feb 9, 2022

patrickkuo deleted the pat/interactive-cli-and-refactoring branch February 9, 2022 14:12

This was referenced Feb 9, 2022

[CLI Refactoring] - Refactor CLI client, Benchmark, and CLI utility #341

Closed

[fastx cli] Implement Interactive CLI #353

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement Interactive CLI #357

Implement Interactive CLI #357

patrickkuo commented Feb 4, 2022 •

edited

Loading

sblackshear Feb 4, 2022

oxade Feb 5, 2022

patrickkuo Feb 5, 2022

patrickkuo Feb 5, 2022

oxade commented Feb 7, 2022

huitseeker left a comment

huitseeker Feb 7, 2022

patrickkuo Feb 8, 2022

huitseeker Feb 8, 2022

huitseeker Feb 7, 2022

huitseeker Feb 7, 2022

huitseeker Feb 7, 2022

patrickkuo Feb 8, 2022

huitseeker Feb 7, 2022

huitseeker Feb 7, 2022

patrickkuo commented Feb 8, 2022

patrickkuo commented Feb 8, 2022

huitseeker left a comment

huitseeker Feb 8, 2022

patrickkuo Feb 9, 2022

huitseeker Feb 8, 2022

huitseeker Feb 8, 2022

gdanezis Feb 9, 2022

huitseeker Feb 8, 2022

oxade Feb 8, 2022

gdanezis left a comment

gdanezis Feb 9, 2022

gdanezis Feb 9, 2022

gdanezis Feb 9, 2022

gdanezis Feb 9, 2022

patrickkuo Feb 9, 2022

gdanezis Feb 9, 2022

oxade commented Feb 9, 2022

Implement Interactive CLI #357

Implement Interactive CLI #357

Conversation

patrickkuo commented Feb 4, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oxade commented Feb 7, 2022

huitseeker left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patrickkuo commented Feb 8, 2022

patrickkuo commented Feb 8, 2022

huitseeker left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gdanezis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oxade commented Feb 9, 2022

patrickkuo commented Feb 4, 2022 •

edited

Loading