feat: add ZK (sp1) development tools #3
Conversation
WalkthroughThis update introduces a new "ZK Development" guide and associated Nix flake template for zero-knowledge (ZK) development environments. The documentation is expanded to include references to the new guide and template. Two new Nix packages, Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Nix
participant Flake
participant Packages
User->>Nix: nix flake init -t github:zero-nix/zero.nix#zk-dev
Nix->>Flake: Load zk-development flake template
Flake->>Packages: Provide sp1, sp1-rust, cargo, rustc
Packages-->>Flake: Packages available in devshell
Flake-->>User: Development shell ready with ZK tooling
Poem
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (8)
packages/sp1-rust.nix (1)
25-62: Missing meta sectionThe derivation is missing a
metasection, which is a good practice for Nix packages. This section typically includes information like description, homepage, license, maintainers, etc. This helps users understand what the package is for, where to find more information, and the license terms.Consider adding a meta section like this:
stdenv.mkDerivation rec { name = "sp1-rust"; version = "1.82.0"; # ... existing code ... + meta = with lib; { + description = "Rust toolchain for SP1 zero-knowledge development"; + homepage = "https://github.com/succinctlabs/rust"; + license = licenses.mit; # Adjust based on actual license + platforms = [ "x86_64-linux" "aarch64-linux" "x86_64-darwin" "aarch64-darwin" ]; + maintainers = with maintainers; [ /* add yourself here */ ]; + }; }docs/guides/zk-development.md (3)
3-4: Grammar correction needed for "setup"The word "setup" is being used as a verb, but it should be "set up" (two words) when used as a verb.
-Zero.nix provides the `sp1` and `sp1-rust` packages to make it -easy to setup a nix development shell with sp1 tooling. +Zero.nix provides the `sp1` and `sp1-rust` packages to make it +easy to set up a nix development shell with sp1 tooling.🧰 Tools
🪛 LanguageTool
[grammar] ~4-~4: The word “setup” is a noun. The verb is spelled with a white space.
Context: ...sp1-rustpackages to make it easy to setup a nix development shell with sp1 toolin...(NOUN_VERB_CONFUSION)
6-6: Add comma for improved readabilityA comma should be added after "To use them" to improve readability and grammar.
-To use them create a flake.nix with the following contents: +To use them, create a flake.nix with the following contents:🧰 Tools
🪛 LanguageTool
[uncategorized] ~6-~6: A comma might be missing here.
Context: ...lopment shell with sp1 tooling. To use them create a flake.nix with the following c...(AI_EN_LECTOR_MISSING_PUNCTUATION_COMMA)
8-14: Code block formatting issueThere's an unnecessary blank line between the command and the closing code fence.
```bash nix flake init -t github:timewave-computer/zero.nix#zk-dev -</blockquote></details> <details> <summary>packages/sp1.nix (1)</summary><blockquote> `12-12`: **Consider adding version update mechanism** The package is pinned to "unstable-2025-03-06", which will require manual updates. Consider adding a comment explaining the update process or automating it with a script. </blockquote></details> <details> <summary>packages/default.nix (1)</summary><blockquote> `10-12`: **Consider creating a new section for ZK tools** As you add more ZK-related tools in the future, consider grouping them under a dedicated attribute set for better organization. ```diff sp1-rust = pkgs.callPackage ./sp1-rust.nix {}; sp1 = pkgs.callPackage ./sp1.nix { inherit (config.packages) sp1-rust; };You could organize them like this:
# Future organization suggestion zk-tools = { sp1-rust = pkgs.callPackage ./sp1-rust.nix {}; sp1 = pkgs.callPackage ./sp1.nix { inherit (config.packages.zk-tools) sp1-rust; }; # Future ZK tools would go here };templates/zk-development/flake.nix (2)
20-21: Unused inputzero-nixThe
zero-nixinput is defined but doesn't appear to be used directly in this file. If it's required by the sp1 or sp1-rust packages, consider documenting this dependency with a comment.Consider adding a comment explaining its purpose:
zero-nix.url = "github:timewave-computer/zero.nix"; + # Used by sp1 and sp1-rust packages };
40-47: Consider adding environment variables for SP1 developmentThe devshell setup includes the necessary packages but might benefit from environment variables specific to SP1 development.
You could enhance the development experience by adding environment variables:
devshells.workshop = {pkgs, ...}: { commands = [ {package = pkgs.cargo;} {package = pkgs.rustc;} {package = inputs'.packages.sp1-rust;} {package = inputs'.packages.sp1;} ]; + env = [ + { + name = "SP1_PATH"; + value = "${inputs'.packages.sp1}/bin"; + } + # Add other SP1-specific environment variables as needed + ]; };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
docs/SUMMARY.md(1 hunks)docs/guides/index.md(1 hunks)docs/guides/zk-development.md(1 hunks)flake.nix(1 hunks)packages/default.nix(1 hunks)packages/sp1-rust.nix(1 hunks)packages/sp1.nix(1 hunks)templates/default.nix(1 hunks)templates/zk-development/flake.nix(1 hunks)
🧰 Additional context used
🪛 markdownlint-cli2 (0.17.2)
docs/SUMMARY.md
7-7: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
🪛 LanguageTool
docs/guides/zk-development.md
[grammar] ~4-~4: The word “setup” is a noun. The verb is spelled with a white space.
Context: ... sp1-rust packages to make it easy to setup a nix development shell with sp1 toolin...
(NOUN_VERB_CONFUSION)
[uncategorized] ~6-~6: A comma might be missing here.
Context: ...lopment shell with sp1 tooling. To use them create a flake.nix with the following c...
(AI_EN_LECTOR_MISSING_PUNCTUATION_COMMA)
🔇 Additional comments (12)
packages/sp1-rust.nix (1)
7-8:❌ Incorrect review comment
Unconventional use of ellipsis in function arguments
The use of
...in function arguments is unusual in Nix. This idiom is typically used in JavaScript and other languages but might cause issues in Nix. Consider using standard Nix idioms for handling variable arguments likelib.makeExtensibleor explicitly listing all required dependencies.
🏁 Script executed:
#!/bin/bash # Check if this pattern is used elsewhere in the codebase rg -l "\.\.\." --type nix | grep -v "sp1-rust.nix"Length of output: 652
Ellipsis syntax in Nix attrsets is a valid varargs idiom
The use of...in function argument patterns (e.g.{ foo, bar, … }:) is part of the Nix language for capturing or permitting extra attributes. It’s a standard, built-in feature and is used consistently across this repo (see flake.nix, default.nix, modules, etc.). No change required.Likely an incorrect or invalid review comment.
docs/guides/index.md (1)
4-4: LGTM: Guide entry added correctlyThe new ZK Development guide entry has been properly added to the list, following the existing pattern of the other guide entries.
docs/SUMMARY.md (1)
7-7: LGTM: Consistent indentation with existing entriesThe new ZK Development entry is correctly added to the summary with the same indentation level as the other guide entries. The static analysis tool flagged this line for having 4-space indentation instead of 2, but this is consistent with all the other entries in this list, so it follows the project's established convention.
🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
7-7: Unordered list indentation
Expected: 2; Actual: 4(MD007, ul-indent)
flake.nix (1)
25-25: LGTM: Templates import added correctlyThe addition of
./templates/default.nixto the imports array is correctly implemented. This change integrates the new ZK development templates into the flake.docs/guides/zk-development.md (1)
1-18: Documentation looks good overallThe ZK development guide provides clear instructions on how to use the sp1 tooling with Zero.nix, including both manual creation and template initialization options.
🧰 Tools
🪛 LanguageTool
[grammar] ~4-~4: The word “setup” is a noun. The verb is spelled with a white space.
Context: ...sp1-rustpackages to make it easy to setup a nix development shell with sp1 toolin...(NOUN_VERB_CONFUSION)
[uncategorized] ~6-~6: A comma might be missing here.
Context: ...lopment shell with sp1 tooling. To use them create a flake.nix with the following c...(AI_EN_LECTOR_MISSING_PUNCTUATION_COMMA)
packages/sp1.nix (2)
1-30: Package definition looks well-structuredThe sp1 package is properly defined using rustPlatform.buildRustPackage with appropriate dependencies and source specification. Good practice to include the specific commit hash and disable tests when necessary.
19-20:✅ Verification successful
Verify sp1-cli package availability
Ensure that the sp1-cli package is available in the repository at the specified commit. This is especially important since you're targeting a specific package within the repository.
🏁 Script executed:
#!/bin/bash # Check if the sp1-cli package exists in the Cargo.toml files in the repository curl -s https://raw.githubusercontent.com/succinctlabs/sp1/9f202bf603b3cab5b7c9db0e8cf5524a3428fbee/Cargo.toml | grep -A 20 "\[workspace\]" | grep "members" curl -s https://raw.githubusercontent.com/succinctlabs/sp1/9f202bf603b3cab5b7c9db0e8cf5524a3428fbee/crates/sp1-cli/Cargo.toml | grep -E "name|version"Length of output: 321
🏁 Script executed:
#!/usr/bin/env bash set -euxo pipefail # Fetch and display the workspace section of the root Cargo.toml curl -s https://raw.githubusercontent.com/succinctlabs/sp1/9f202bf603b3cab5b7c9db0e8cf5524a3428fbee/Cargo.toml \ | sed -n '/\[workspace\]/,/^ *\]/p' # Verify existence of the sp1-cli crate and print its name & version CRATE_URL="https://raw.githubusercontent.com/succinctlabs/sp1/9f202bf603b3cab5b7c9db0e8cf5524a3428fbee/crates/sp1-cli/Cargo.toml" if curl -s --fail "$CRATE_URL" > /dev/null; then echo "✔ Found crates/sp1-cli/Cargo.toml, details:" curl -s "$CRATE_URL" | grep -E '^(name|version)' else echo "❌ crates/sp1-cli/Cargo.toml not found at specified commit" fiLength of output: 1100
🏁 Script executed:
#!/usr/bin/env bash set -euxo pipefail # Check the crate name and version in crates/cli CLI_CRATE_URL="https://raw.githubusercontent.com/succinctlabs/sp1/9f202bf603b3cab5b7c9db0e8cf5524a3428fbee/crates/cli/Cargo.toml" if curl -s --fail "$CLI_CRATE_URL" > /dev/null; then echo "✔ Found crates/cli/Cargo.toml, details:" curl -s "$CLI_CRATE_URL" | grep -E '^(name|version)' else echo "❌ crates/cli/Cargo.toml not found at specified commit" fiLength of output: 570
sp1-cli package verification passed
The
sp1-clicrate is defined undercrates/cliin the workspace and itsCargo.tomldeclares the correct package name:
- Root
Cargo.tomlworkspace members include"crates/cli".crates/cli/Cargo.tomlcontains:name = "sp1-cli" version = { workspace = true }No changes needed.
templates/default.nix (1)
1-22: Template structure looks goodThe template is well-structured with a clear description, package list, and documentation link. Good practice to use a common welcome message variable for reusability.
packages/default.nix (1)
9-12: Package integration is clean and well-organizedThe sp1-rust and sp1 packages are properly integrated into the existing package set. Good practice to explicitly inherit the dependency between them.
templates/zk-development/flake.nix (3)
1-50: Overall flake structure looks well-organizedThe flake is well-structured with clear sections for description, configuration, inputs, and outputs. It provides a standardized development environment for ZK development using SP1.
4-10: Cachix configuration ensures reproducible buildsGood setup of the Cachix caches which will help speed up builds and ensure reproducibility across different systems. The trusted public keys are properly configured.
13-21: Good dependency pinning practiceProper pinning of nixpkgs to a specific version (nixos-24.11) and other dependencies to GitHub repositories. This ensures consistency across different environments.
| hash = ""; | ||
| }; |
There was a problem hiding this comment.
Empty hash for aarch64-linux platform
The hash value for aarch64-linux platform is empty. Without a hash, Nix cannot verify the integrity of the downloaded file, which is a security risk and may also cause build issues. Please provide a valid SHA256 hash for this platform.
#!/bin/bash
# Fetch the tarball and calculate its SHA256 hash
curl -sL "https://github.com/succinctlabs/rust/releases/download/succinct-1.82.0/rust-toolchain-aarch64-unknown-linux-gnu.tar.gz" | sha256sum| - sp1 (cargo prove) | ||
| - rust-sp1 |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Inconsistent package naming in welcome text
In the welcome text, you refer to "rust-sp1", but the actual package name in packages/default.nix is "sp1-rust". This inconsistency might confuse users.
- - sp1 (cargo prove)
- - rust-sp1
+ - sp1 (cargo prove)
+ - sp1-rust| {package = inputs'.packages.sp1-rust;} | ||
| {package = inputs'.packages.sp1;} |
There was a problem hiding this comment.
Undefined variable inputs' in workshop devshell
There's a reference to inputs' which doesn't appear to be defined. In flake-parts, the per-system inputs should be passed as a parameter to the function, but it's missing from the function arguments.
Fix this by updating the perSystem function to include inputs' in its parameters:
perSystem = {
pkgs,
inputs,
+ inputs',
...
}: {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| {package = inputs'.packages.sp1-rust;} | |
| {package = inputs'.packages.sp1;} | |
| perSystem = { | |
| pkgs, | |
| inputs, | |
| inputs', | |
| ... | |
| }: { | |
| # … rest of function body … | |
| } |
Summary by CodeRabbit