refactor: split cli and client into workspace #407

tomyrd · 2024-06-28T19:51:19Z

This PR restructures the repo to separate the client and cli in separate crates sharing a workspace.

Cargo.toml

miden-cli/Cargo.toml

bobbinth · 2024-06-28T20:08:57Z

btw, how is this PR different from #400?

igamigo · 2024-06-28T20:15:10Z

btw, how is this PR different from #400?

It's not, that PR was originally @mFragaBA's attempt which was behind in the commit history. Eventually it was decided that it was better to start from a fresh branch off of next which resulted in this other PR. I closed that first one.

mFragaBA

Awesome work! Left tiny fixes and with that I think we're ok to merge

bin/miden-cli/Cargo.toml

crates/miden-client/Cargo.toml

Makefile

mFragaBA · 2024-06-28T22:21:51Z

We debugged offline for a while on why the integration tests were failing (assert_cmd wasn't finding the CLI binary). We noticed the CLI binary was not being built. As a fast fix we noticed that you can just run make build before the tests and that'd be enough. Now we're seeing if adding the CLI as a dev-dependency also forces the build on the CLI and it gets found.

UPDATE: including it as a dependency didn't work so we'll go with building before running the test

bobbinth

Looks good! Thank you. I left a few comments inline.

Also, we should update the docs to make sure they are up to date.

bin/miden-cli/src/cli/account.rs

bin/miden-cli/Cargo.toml

bin/miden-cli/src/cli/mod.rs

bobbinth · 2024-06-29T22:36:04Z

crates/miden-client/src/client/mod.rs

Similar to above comments, I'm not sure we need the client submodule any more. Contents of this file should probably go into lib.rs and all other files brought up one level.

bin/miden-cli/Cargo.toml

bobbinth

I left a few more comments inline. Also, a couple of additional comments:

We should rename crates/miden-client into crates/rust-client.
we should create README.md files for each crate (i.e., bin/miden-cli/README.md and crates/rust-client/README.md).
Looking at files in bin/miden-cli/src now, it seems like we've made the structure "too flat". Maybe we should have commands sub-module to contain one file per command (e.g., bin/miden-cli/src/commands/account.rs).

bin/miden-cli/Cargo.toml

bobbinth · 2024-07-01T05:46:37Z

crates/miden-client/src/errors.rs

+pub use miden_objects::{
    accounts::AccountId, crypto::merkle::MmrError, notes::NoteId, AccountError, AssetError,
    AssetVaultError, Digest, NoteError, TransactionScriptError, Word,
 };
-use miden_tx::{
+pub use miden_tx::{
    utils::{DeserializationError, HexParseError},
    DataStoreError, TransactionExecutorError, TransactionProverError,
 };


We don't need to re-export things like AccountId, NoteId, Word, and Digest from here.

Also, Some error are re-exports from other places too - e.g., DataStoreError, TransactionExecutorError, and DeserializationError exported from sub-modules defined at the root. We should have a consistent methodology and either export all errors from here - or all errors from corresponding sub-modules.

I would leave re-exports from other crates in lib.rs. Also, in #374 I move around errors a bit maybe I can rebase once we merge this PR and deal with inconsistencies then

tests/Cargo.toml

bin/miden-cli/src/account.rs

bin/miden-cli/Cargo.toml

igamigo · 2024-07-01T18:08:51Z

Makefile

 	cargo nextest run --release --test=integration --features $(FEATURES_INTEGRATION_TESTING) --no-default-features

 .PHONY: integration-test-full
-integration-test-full: ## Run the integration test binary with ignored tests included
+integration-test-full: build ## Run the integration test binary with ignored tests included


Let's create an issue to move the CLI integration tests to the CLI crate

Done, created issue #411

README.md

igamigo · 2024-07-01T18:11:02Z

README.md

-```
+The workspace is organized as follows:
+- The `bin` folder contains crates that are meant to be compiled into binaries (like the CLI).
+- The `crates` folder contains the library crates that are meant to be used as dependencies (like the rust client library).


nit:

Suggested change

- The `crates` folder contains the library crates that are meant to be used as dependencies (like the rust client library).

- The `crates` folder contains the library crates that are meant to be used as dependencies (like the Rust client library).

bobbinth · 2024-07-01T18:41:52Z

tests/Cargo.toml

+figment = { version = "0.10", features = ["toml", "env"] }
+miden-client = { path = "../crates/rust-client", features = ["testing"] }
+miden-objects = { package = "miden-objects", git = "https://github.com/0xPolygonMiden/miden-base.git", branch = "next", default-features = false, features = ["serde"] }
+assert_cmd = { version = "2.0" }


nit: let's make sure these are sorted in alphabetical order.

bobbinth

Looks good! Thank you. I left a few more comments inline.

Also, I think e should still get rid of the client sub-module in miden-client.

bobbinth · 2024-07-01T18:44:30Z

bin/miden-cli/README.md

+Before you can use the Miden client, you'll need to make sure you have both
+[Rust](https://www.rust-lang.org/tools/install) and sqlite3 installed. Miden
+client requires rust version **1.78** or higher.


nit: we usually don't manually wrap lines in markdown files.

bobbinth · 2024-07-01T18:45:56Z

bin/miden-cli/README.md

+You can either build from source with:
+
+```bash
+cargo build --release
+```
+
+Once the binary is built, you can find it on `./target/release/miden-client`.


We should mention building with testing feature enabled.

Also, is the target directory correct here?

Fixed, the target directory is ok but the binary name isn't

bobbinth · 2024-07-01T18:47:09Z

bin/miden-cli/README.md

+```bash
+cargo install miden-client
+```


This should be miden-cli, right?

Yes, this is the old binary name

bobbinth · 2024-07-01T18:49:13Z

bin/miden-cli/README.md

Let's add a "License" section to this file (as we have in the main file).

bobbinth · 2024-07-01T18:50:17Z

bin/miden-cli/README.md

@@ -0,0 +1,27 @@
+# Miden Client CLI
+
+This binary is a wrapper around the library exposing its functionality via a simple command-line interface (CLI).


I would make this description a bit more informative (i.e., that this is for interacting with the Miden rollup). This fill will be the main file on crates.io for the miden-cli crate.

bobbinth · 2024-07-01T18:52:51Z

crates/rust-client/Cargo.toml

+miden-lib = { package = "miden-lib", git = "https://github.com/0xPolygonMiden/miden-base.git", branch = "next", default-features = false, features = ["testing"] }
+miden-objects = { package = "miden-objects", git = "https://github.com/0xPolygonMiden/miden-base.git", branch = "next", default-features = false, features = ["serde", "testing"] }
+uuid = { version = "1.6", features = ["serde", "v4"] }
+miden-client = { path = ".", features = ["testing", "concurrent", "sqlite", "tonic", "tokio"]}


nit: let's alphabetize this list.

bobbinth · 2024-07-01T18:53:53Z

crates/rust-client/README.md

Let's add the "License" section to this file.

bobbinth · 2024-07-01T18:55:55Z

crates/rust-client/README.md

+- `concurrent`: used to enable concurrency during execution and proof generation.
+- `testing`: useful feature that lowers PoW difficulty when enabled, meant to be used during development and not on production.
+- `sqlite`: includes `SqliteStore`, a SQLite implementation of the `Store` trait that can be used as a component of `Client`.
+- `async`: enables async traits. Disabled by default.
+- `tonic`: includes `TonicRpcClient`, a Tonic client to communicate with Miden node, that can be used as a component of `Client`.
+- `executable`: builds the CLI, based on `SqliteStore` and `TonicRpcClient`.


We don't have executable feature any more, right?

Nice catch, we removed it and I forgot to delete it from the list

bobbinth · 2024-07-01T18:57:42Z

crates/rust-client/README.md

+By default, the library is `no_std` compatible.
+
+### Features


I would try to do something more similar to how we describe features in other places (e.g., here.

For example, I would name the section "Crate features", would describe how to use --no-default-features flag etc.

bobbinth · 2024-07-01T18:59:23Z

crates/rust-client/src/lib.rs

@@ -47,16 +53,16 @@ pub mod crypto {
            },
            rand::{FeltRng, RpoRandomCoin},
        },
-        Digest,
+        Digest, ONE, ZERO,


I would probably move ONE and ZERO exports to the root level (i.e., to line 60 below).

Co-authored-by: igamigo <ignacio.amigo@lambdaclass.com>

igamigo · 2024-07-02T13:57:08Z

bin/miden-cli/Cargo.toml

+path = "src/main.rs"
+
+[features]
+default = ["dep:clap", "dep:comfy-table", "dep:figment", "dep:tokio", "dep:toml"]


I think we should be able to leave default as empty, and remove optional dependencies.

mFragaBA

Is it possible that tonic files are not being generated? I tried deleting the crates/rust-client/src/client/rpc/tonic_client directory and building and the files didn't get generated. Perhaps I'm missing something

we'll probably need to update the getting-started documentation in miden base.

mFragaBA · 2024-07-02T13:56:03Z

tests/Cargo.toml

+edition.workspace = true
+
+[features]
+integration = ["miden-client/integration"]


I'm. wondering if an integration feature is necessary in the client still. We don't have to change it in this PR, but I'd try removing it and checking if integration tests are not being ran during make test and the appropiate integration tests are being ran during make integration-test

If we remove it we'll also need to update the readme

I think we should definitely be able to remove it on #411

Added a comment referencing this change in the issue

igamigo

LGTM!

mFragaBA

LGTM!

bobbinth

I know this PR has been merged already, but I left a few comments below. Would be good to address them in a follow-up PR.

bobbinth · 2024-07-02T20:56:23Z

bin/miden-cli/README.md

+These actions can also be executed when inside the repository via the Makefile with `make build` or `make install`.
+
+## License
+This project is [MIT licensed](./LICENSE).


I think this link is broken. Probably should refer to the parent directory.

bobbinth · 2024-07-02T20:57:06Z

bin/miden-cli/README.md

@@ -0,0 +1,30 @@
+# Miden Client CLI
+
+This binary allows the user to interact with the Miden rollup via a simple command-line interface (CLI). It's a wrapper around the Miden client library exposing its functionality in order to create accounts, create and consume notes, all executed and proved using the Miden VM.


I would make the "Miden client" reference in the second sentence a link to the miden-client crate on crates.io.

do we need to release it first as miden-client?

I think we can put in the link now assuming it will be published at the time miden-cli crate is published.

bobbinth · 2024-07-02T20:57:44Z

crates/rust-client/README.md

+The `sqlite` and `tonic` features provide implementations for these traits using [Rusqlite](https://github.com/rusqlite/rusqlite) and [Tonic](https://github.com/hyperium/tonic) respectively.
+
+## License
+This project is [MIT licensed](./LICENSE).


I think this link is broken. Probably should refer to the parent directory.

bobbinth · 2024-07-02T21:01:21Z

crates/rust-client/src/lib.rs

+    pub use miden_objects::assets::{Asset, AssetVault, FungibleAsset, TokenSymbol};
+}
+
+mod store_authenticator;


nit: this line should probably be moved up to the top section of the file.

bobbinth · 2024-07-02T21:01:54Z

crates/rust-client/src/lib.rs

+pub mod accounts;
+pub mod config;
+pub mod errors;
+pub(crate) mod note_screener;
+pub mod notes;
+pub mod rpc;
+pub mod store;
+pub mod sync;
+pub mod transactions;


I'm not sure any of these modules need to be pub any more.

We moved the re-exports to each of the relevant submodules so we should make them (the modules) public for them to be accesible. I'll try moving the re-exports back to lib.rs and renaming the modules so that they dont cause conflicts.

bobbinth · 2024-07-02T21:04:45Z

crates/rust-client/src/note_screener.rs

Should this and notes.rs file go into src/notes directory?

bobbinth · 2024-07-02T21:06:03Z

crates/rust-client/src/sync.rs

I would probably move this file into its own directory (i.e., src/sync/mod.rs) and split it up into several files.

* refactor: split into workspace * Update CHANGELOG * refactor: separate directories for binaries and crates * feat: add workspace dependencies * refactor: enable integration tests as separate crate * fix: workspace features * fix: address suggestions * fix: makefile descriptions * fix: add build step before tests * fix: Integration tests * fix: Go back to next for integration tests * Change docs make target * fix: address suggestions for miden-cli Cargo.toml * fix: remove unnecessary exports in miden-client * fix: remove unnecessary dependency from tests * fix: remove patch versions from all dependencies * refactor: rename `miden-client` directory to `rust-client` * refactor: create `commands` sub-module for miden-cli * refactor: distribute sections of workspace README to the crate/bin * fix: add general workspace information and links to READMEs * fix: cli name in README Co-authored-by: igamigo <ignacio.amigo@lambdaclass.com> * fix: alphabetize dependencies in tests crate * fix: improve miden-cli README * fix: add more information about the rust-client features * refactor: remove `client` sub-module * fix: tonic generated file builder * fix: remove optional dependencies in cli --------- Co-authored-by: Ignacio Amigo <ignacio.amigo@lambdaclass.com>

tomyrd force-pushed the tomyrd-workspace-refactor branch from ca7cef7 to 27c4a59 Compare June 28, 2024 19:54

bobbinth reviewed Jun 28, 2024

View reviewed changes

Cargo.toml Outdated Show resolved Hide resolved

bobbinth reviewed Jun 28, 2024

View reviewed changes

miden-cli/Cargo.toml Outdated Show resolved Hide resolved

igamigo mentioned this pull request Jun 28, 2024

[WIP] refactor: workspace refactor #400

Closed

tomyrd force-pushed the tomyrd-workspace-refactor branch from 27c4a59 to 40e4b6a Compare June 28, 2024 20:20

tomyrd added 5 commits June 28, 2024 18:06

refactor: split into workspace

6f5d782

Update CHANGELOG

83421cc

refactor: separate directories for binaries and crates

6b37542

feat: add workspace dependencies

4e07a0c

refactor: enable integration tests as separate crate

8b5a8ec

tomyrd force-pushed the tomyrd-workspace-refactor branch from b673fcb to 8b5a8ec Compare June 28, 2024 21:06

tomyrd marked this pull request as ready for review June 28, 2024 21:10

fix: workspace features

49ad991

mFragaBA requested changes Jun 28, 2024

View reviewed changes

bin/miden-cli/Cargo.toml Outdated Show resolved Hide resolved

crates/miden-client/Cargo.toml Outdated Show resolved Hide resolved

Makefile Show resolved Hide resolved

tomyrd added 2 commits June 28, 2024 18:35

fix: address suggestions

19fb973

fix: makefile descriptions

c9a763c

fix: add build step before tests

6349fd6

bobbinth reviewed Jun 29, 2024

View reviewed changes

bobbinth mentioned this pull request Jun 29, 2024

feat: add re exports from base used in public api #394

Merged

2 tasks

igamigo added 2 commits June 30, 2024 11:09

Rebase

f715555

fix: Integration tests

23c8ede

igamigo force-pushed the tomyrd-workspace-refactor branch from ced1195 to 23c8ede Compare July 1, 2024 01:56

igamigo added 2 commits June 30, 2024 22:59

fix: Go back to next for integration tests

1131acf

Change docs make target

6aaa2f9

bobbinth reviewed Jul 1, 2024

View reviewed changes

tomyrd added 2 commits July 1, 2024 11:08

fix: address suggestions for miden-cli Cargo.toml

256128b

fix: remove unnecessary exports in miden-client

a6d4a63

bobbinth reviewed Jul 1, 2024

View reviewed changes

bin/miden-cli/Cargo.toml Outdated Show resolved Hide resolved

tomyrd added 5 commits July 1, 2024 11:49

fix: remove patch versions from all dependencies

9183699

refactor: rename miden-client directory to rust-client

72a1619

refactor: create commands sub-module for miden-cli

a9661b9

refactor: distribute sections of workspace README to the crate/bin

dacf7a6

fix: add general workspace information and links to READMEs

49b2c6e

igamigo reviewed Jul 1, 2024

View reviewed changes

README.md Outdated Show resolved Hide resolved

igamigo reviewed Jul 1, 2024

View reviewed changes

bobbinth reviewed Jul 1, 2024

View reviewed changes

fix: cli name in README

44b268b

Co-authored-by: igamigo <ignacio.amigo@lambdaclass.com>

tomyrd mentioned this pull request Jul 1, 2024

Move CLI integration tests inside the miden-cli crate #411

Closed

tomyrd added 4 commits July 1, 2024 17:10

fix: alphabetize dependencies in tests crate

6a44628

fix: improve miden-cli README

76e35e7

fix: add more information about the rust-client features

e844b0d

refactor: remove client sub-module

b8520ae

igamigo reviewed Jul 2, 2024

View reviewed changes

mFragaBA requested changes Jul 2, 2024

View reviewed changes

tomyrd added 2 commits July 2, 2024 11:31

fix: tonic generated file builder

afd268a

fix: remove optional dependencies in cli

c73d780

igamigo approved these changes Jul 2, 2024

View reviewed changes

mFragaBA approved these changes Jul 2, 2024

View reviewed changes

mFragaBA merged commit 146a076 into next Jul 2, 2024
7 checks passed

mFragaBA deleted the tomyrd-workspace-refactor branch July 2, 2024 14:58

mFragaBA mentioned this pull request Jul 2, 2024

Add WebStore to miden-client #402

Merged

bobbinth reviewed Jul 2, 2024

View reviewed changes

tomyrd mentioned this pull request Jul 2, 2024

Workspace rework follow-ups #414

Closed

	- The `crates` folder contains the library crates that are meant to be used as dependencies (like the rust client library).
	- The `crates` folder contains the library crates that are meant to be used as dependencies (like the Rust client library).

		@@ -0,0 +1,27 @@
		# Miden Client CLI

		This binary is a wrapper around the library exposing its functionality via a simple command-line interface (CLI).

		@@ -0,0 +1,30 @@
		# Miden Client CLI

		This binary allows the user to interact with the Miden rollup via a simple command-line interface (CLI). It's a wrapper around the Miden client library exposing its functionality in order to create accounts, create and consume notes, all executed and proved using the Miden VM.

refactor: split cli and client into workspace #407

refactor: split cli and client into workspace #407

Conversation

tomyrd commented Jun 28, 2024 • edited Loading

bobbinth commented Jun 28, 2024

igamigo commented Jun 28, 2024

mFragaBA left a comment

Choose a reason for hiding this comment

mFragaBA commented Jun 28, 2024 • edited Loading

bobbinth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bobbinth 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

igamigo Jul 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bobbinth 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

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

igamigo Jul 2, 2024 • edited Loading

Choose a reason for hiding this comment

mFragaBA 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

igamigo left a comment

Choose a reason for hiding this comment

mFragaBA left a comment

Choose a reason for hiding this comment

bobbinth 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

Choose a reason for hiding this comment

tomyrd commented Jun 28, 2024 •

edited

Loading

mFragaBA commented Jun 28, 2024 •

edited

Loading

igamigo Jul 1, 2024 •

edited

Loading

igamigo Jul 2, 2024 •

edited

Loading