-
Notifications
You must be signed in to change notification settings - Fork 2
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(cli): setup skeleton for quartz tool #105
Conversation
.finish() | ||
.init(); | ||
|
||
let request = Request::try_from(args.command)?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The idea is to parse the input args and convert them into Request
s which are correct-by-construction types that the tool can handle (so ideally all validation happens during this conversion). Each Request
defines an associated Handler
(i.e. logic) and Response
. All handlers are free to log to the terminal and these logs are sent to stderror
. Handlers must use Response
s to output to stdout
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might be worth converting your GH issue comment into a comment in the code 🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cli/README.md
Outdated
start Configure Gramine, sign, and start the enclave binary | ||
deploy Deploy the WASM binary to the blockchain and call instantiate | ||
run Run the enclave handler, and expose a public query server for users |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These 3 commands would most likely be confusing for any newcomer to Quartz. I'd recommend having enclave
and contract
subcommands like so:
quartz enclave start
quartz contract deploy
Also, please remind me why we have a start
and run
command for the enclave? That would most likely be confusing for users.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just removed run
(f894c15). 👍
Regarding enclave
/contract
commands, I think it would be better instead to rename start
to start-enclave
and deploy
to deploy-contract
, because we only have one subcommand per command currently. WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we plan on only having a single command for each going forward? If so then your proposal makes sense to me!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point! I am not sure TBH. 😅 IMHO it's easier to start with single commands for now because I cannot think of concrete examples for multiple sub-commands. Actually we could have separate build
commands (e.g. quartz {enclave,contract} build
) instead of quartz build
in the future. 🤔 I guess what you proposed initially makes sense. Will update. 🙏
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
.finish() | ||
.init(); | ||
|
||
let request = Request::try_from(args.command)?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might be worth converting your GH issue comment into a comment in the code 🙂
… into hu55a1n1/61-quartz-cli-skeleton
Co-authored-by: Thane Thomson <[email protected]>
Just pushed a commit to print the reponse (1448713) which I had forgotten before. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks good to me, I reviewed the docs on clap to understand how it's working a bit deeper, but didn't spend super long. In general it looks good for a skeleton setup, I am not approving because I don't understand clap well enough to say all the decisions are the best.
I think you said Thane was going to review it anyways, so I would rely on his approval. However if you want me to look deeper into understanding the packages used I can spend time doing that, and then I can approve it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just one question, but happy to approve here! The skeleton LGTM 👍
|
||
fn handle(self, _verbosity: Verbosity) -> Result<Self::Response, Self::Error> { | ||
trace!("initializing directory structure..."); | ||
todo!() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At what point will this be filled out, if not in this PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was hoping once we had the skeleton in place, we (the team) could work on the commands in parallel. (we could also copy the work that @dangush is doing on the cycles-protocol repo, e.g. https://github.com/informalsystems/cycles-protocol/pull/14)
I'd be happy to implement quartz init
myself in a separate PR. 🙏
Partially addresses: #61
This PR adds a skeleton for the CLI tool (i.e. structure for basic error handling, logging, arg parsing, validation, handling, etc.). The idea is to incorporate and consolidate rust rewrites of our scripts (e.g. https://github.com/informalsystems/cycles-protocol/pull/14) into this tool.