o1-labs / zkapp-cli

CLI to create a zkApp (zero-knowledge app) for Mina Protocol
https://docs.minaprotocol.com/zkapps/how-to-write-a-zkapp
Apache License 2.0
114 stars 43 forks source link

Extend information printed during deployment procedure & fix for Faucet URL. #631

Closed shimkiv closed 4 months ago

shimkiv commented 4 months ago

Closes #630


image
shimkiv commented 4 months ago

Maybe Smart contract should be part of the zkApp section (like Alias and Account for Fee payer)?

shimkiv commented 4 months ago

Maybe Smart contract should be part of the zkApp section (like Alias and Account for Fee payer)?

Like this:

image
MartinMinkov commented 4 months ago

PR Review

โฑ๏ธ Estimated effort to review [1-5] 3, because the PR involves changes in multiple files with modifications to both logic and string formatting. The changes are not overly complex but require a careful review to ensure that the modifications in string outputs and conditions do not introduce any logical errors or regressions.
๐Ÿงช Relevant tests No
๐Ÿ” Possible issues URL Formatting Issue: The modification in `src/lib/config.js` changes how the URL is constructed. The use of `&?` might be incorrect and should be `&`. This could lead to malformed URLs.
๐Ÿ”’ Security concerns No
Code feedback:
relevant filesrc/lib/config.js
suggestion       Consider correcting the URL parameter separator from `&?` to `&` to avoid potential issues with URL parsing. This change ensures that the URL is correctly formatted and functional. [important]
relevant line(explorerName ? `&?explorer=${explorerName}` : '') +

relevant filesrc/lib/deploy.js
suggestion       It's recommended to handle the scenario where `transactionJson` or `transaction` might be undefined or null before using them. This can prevent runtime errors in cases where the transaction fails to initialize properly. [important]
relevant linelet transactionJson = transaction.json;

relevant filesrc/lib/deploy.js
suggestion       Consider adding error handling for the `process.exit(1);` call to provide a cleaner exit strategy, such as logging the error or cleaning up resources before exiting. This improves the robustness of the deployment script. [medium]
relevant lineprocess.exit(1);

relevant filesrc/lib/deploy.js
suggestion       To enhance readability and maintainability, consider refactoring the large block of changes into smaller, more focused functions. This can help in isolating functionality and making the code easier to understand and test. [medium]
relevant linechalk.bold('Transaction fee'), chalk.reset(`${Number(fee) / 1e9} Mina`)],


โœจ Review tool usage guide:
**Overview:** The `review` tool scans the PR code changes, and generates a PR review which includes several types of feedbacks, such as possible PR issues, security threats and relevant test in the PR. More feedbacks can be [added](https://pr-agent-docs.codium.ai/tools/review/#general-configurations) by configuring the tool. The tool can be triggered [automatically](https://pr-agent-docs.codium.ai/usage-guide/automations_and_usage/#github-app-automatic-tools-when-a-new-pr-is-opened) every time a new PR is opened, or can be invoked manually by commenting on any PR. - When commenting, to edit [configurations](https://github.com/Codium-ai/pr-agent/blob/main/pr_agent/settings/configuration.toml#L23) related to the review tool (`pr_reviewer` section), use the following template: ``` /review --pr_reviewer.some_config1=... --pr_reviewer.some_config2=... ``` - With a [configuration file](https://pr-agent-docs.codium.ai/usage-guide/configuration_options/), use the following template: ``` [pr_reviewer] some_config1=... some_config2=... ``` See the review [usage page](https://pr-agent-docs.codium.ai/tools/review/) for a comprehensive guide on using this tool.
shimkiv commented 4 months ago

PR Review

Thanks @MartinMinkov and CodiumAI PR-Agent :D

MartinMinkov commented 4 months ago

PR Review Thanks @MartinMinkov and CodiumAI PR-Agent :D

I wanted to try out this tool :laughing: Not sure if it's that helpful, thanks for your patience @shimkiv !

ymekuria commented 4 months ago

I wanted to try out this tool ๐Ÿ˜† Not sure if it's that helpful, thanks for your patience @shimkiv !

This is cool @Martin!

ymekuria commented 4 months ago

Maybe Smart contract should be part of the zkApp section (like Alias and Account for Fee payer)?

Like this:

image

I like this @shimkiv .

shimkiv commented 4 months ago

Are we ok to merge this for now and continue improvements as part of https://github.com/o1-labs/zkapp-cli/issues/633? cc @ymekuria @MartinMinkov

ymekuria commented 4 months ago

Are we ok to merge this for now and continue improvements as part of #633? cc @ymekuria @MartinMinkov

@shimkiv This is good to merge as far as I'm concerned.