Skip to content
This repository has been archived by the owner on Dec 17, 2024. It is now read-only.

Cli tool initial version #39

Merged
merged 35 commits into from
Jan 21, 2024
Merged

Cli tool initial version #39

merged 35 commits into from
Jan 21, 2024

Conversation

musitdev
Copy link
Contributor

@musitdev musitdev commented Jan 17, 2024

Add a cli tool:

Deploy prover/verifier
Exec program workflow.

@musitdev musitdev changed the base branch from main to proto January 17, 2024 13:52
@musitdev musitdev requested a review from tuommaki January 17, 2024 13:53
Copy link
Contributor

@tuommaki tuommaki left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Few spelling fix suggestions and opening discussion on couple style related matters we should probably align on in general.

Except for command argument structure, the design & code otherwise LGTM 👍

Cargo.toml Outdated Show resolved Hide resolved
crates/cli/Cargo.toml Outdated Show resolved Hide resolved
@@ -0,0 +1,28 @@
[package]
name = "gevulot-cli"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've been thinking about the name. cli as such feels a bit wrong when the gevulot itself already provides bunch of functionality that the node operator needs, but at the same time I don't have good suggestions. client is not really the right one, as there's the JSON-RPC client library already. devctl or something along that side could maybe work, but that also feels a bit off.

I'd suggest that we go with the gevulot-cli but try to think about better name for this, to best communicate its functionality & purpose.

Also: We might want to extract this into its own repository at some point, WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the name by exp I know I'm not very good, so I take what it's done in other BC. But you can define any name, I don't really have any good idea.
I agree, it's better if the tool have its own repo because update and release rhythm and their consequences are not the same for the node and the client.
More generally, I agree with the Rust idiomatic's way to define crate: define small crate. The drawback is that it add complexity when you have a lot of crate not published in crates.io because you've to manage all the specific commit for all crates you use. It can become tedious when all crates move fast before the first release.

crates/cli/README.md Outdated Show resolved Hide resolved
crates/cli/README.md Outdated Show resolved Hide resolved
crates/cli/src/main.rs Outdated Show resolved Hide resolved
crates/cli/src/main.rs Outdated Show resolved Hide resolved
crates/cli/src/main.rs Outdated Show resolved Hide resolved
crates/cli/src/main.rs Outdated Show resolved Hide resolved
crates/cli/src/server.rs Show resolved Hide resolved
musitdev and others added 8 commits January 18, 2024 09:50
Co-authored-by: Tuomas Mäkinen <[email protected]>
Co-authored-by: Tuomas Mäkinen <[email protected]>
Co-authored-by: Tuomas Mäkinen <[email protected]>
Co-authored-by: Tuomas Mäkinen <[email protected]>
Co-authored-by: Tuomas Mäkinen <[email protected]>
Co-authored-by: Tuomas Mäkinen <[email protected]>
Co-authored-by: Tuomas Mäkinen <[email protected]>
Co-authored-by: Tuomas Mäkinen <[email protected]>
@musitdev
Copy link
Contributor Author

All check pass, I can merge?

@tuommaki
Copy link
Contributor

All check pass, I can merge?

I think there are still pending typos open in the diff side or is my GitHub view somehow not showing the latest? Most notable there are couple gelulot vs. gevulot points. I think at least those would be good to fix before merge.

@musitdev
Copy link
Contributor Author

Yes, they were hidden by github. I didn't see. I thought that solved issue were hidden and not opened. I look at them.

musitdev and others added 11 commits January 19, 2024 14:51
Co-authored-by: Tuomas Mäkinen <[email protected]>
Co-authored-by: Tuomas Mäkinen <[email protected]>
Co-authored-by: Tuomas Mäkinen <[email protected]>
Co-authored-by: Tuomas Mäkinen <[email protected]>
Co-authored-by: Tuomas Mäkinen <[email protected]>
Co-authored-by: Tuomas Mäkinen <[email protected]>
Co-authored-by: Tuomas Mäkinen <[email protected]>
Co-authored-by: Tuomas Mäkinen <[email protected]>
Co-authored-by: Tuomas Mäkinen <[email protected]>
Co-authored-by: Tuomas Mäkinen <[email protected]>
Co-authored-by: Tuomas Mäkinen <[email protected]>
@tuommaki tuommaki merged commit 906fae2 into proto Jan 21, 2024
4 checks passed
@tuommaki tuommaki deleted the cli-tool-initial-version branch January 21, 2024 14:26
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants