refactor(prover): Using some std libs instead of thirdparty lib #1573
Conversation
WalkthroughThe changes involve modifications to the Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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: 2
🧹 Outside diff range and nitpick comments (2)
prover/src/version.rs (1)
17-17: Consider removing unnecessary clone()The
clone()call creates an unnecessary copy of the string. SinceOnceLock::get_or_initreturns a reference, we can return it directly.- VERSION.get_or_init(init_version).clone() + VERSION.get_or_init(init_version).to_string()prover/src/zk_circuits_handler/common.rs (1)
14-17: Consider simplifying lifetime parameterThe function signature uses an explicit lifetime 'a which might be unnecessary since
OnceLockprovides static references.-pub fn get_params_map_instance<'a, F>(load_params_func: F) -> &'a BTreeMap<u32, ParamsKZG<Bn256>> +pub fn get_params_map_instance<F>(load_params_func: F) -> &'static BTreeMap<u32, ParamsKZG<Bn256>>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
prover/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (6)
prover/Cargo.toml(0 hunks)prover/src/config.rs(3 hunks)prover/src/version.rs(2 hunks)prover/src/zk_circuits_handler/common.rs(1 hunks)prover/src/zk_circuits_handler/darwin.rs(1 hunks)prover/src/zk_circuits_handler/darwin_v2.rs(1 hunks)
💤 Files with no reviewable changes (1)
- prover/Cargo.toml
🔇 Additional comments (8)
prover/src/version.rs (1)
1-1: LGTM! Good transition to standard library
The change from OnceCell to OnceLock improves thread safety while reducing third-party dependencies. The initialization remains correct and the unsafe block is properly eliminated.
Also applies to: 4-4
prover/src/zk_circuits_handler/common.rs (2)
3-6: LGTM! Clean import organization
Good organization of imports, properly bringing in Arc and OnceLock from std::sync.
8-8: LGTM! Proper thread-safe static initialization
Good transition from Rc to Arc for thread-safe reference counting, combined with OnceLock for safe initialization.
prover/src/config.rs (1)
54-54: LGTM! Thread-safe static initialization
Good transition to OnceLock for thread-safe initialization of the assets directories.
prover/src/zk_circuits_handler/darwin.rs (2)
19-19: LGTM! Good use of standard library for thread-safe lazy initialization.
The change from once_cell::Lazy to std::sync::LazyLock aligns well with the PR objective of using standard library features instead of third-party dependencies. LazyLock provides the same thread-safe initialization guarantees.
Also applies to: 22-22
Line range hint 271-287: LGTM! Consistent use of LazyLock across test configuration.
The change to LazyLock has been consistently applied across all static configuration variables in the test module, maintaining the same initialization patterns and functionality.
prover/src/zk_circuits_handler/darwin_v2.rs (2)
19-19: LGTM! Consistent implementation with darwin.rs.
The change to LazyLock mirrors the implementation in darwin.rs, maintaining consistency across both handler implementations while benefiting from standard library features.
Also applies to: 22-22
Line range hint 402-418: LGTM! Consistent test configuration with darwin.rs.
The change to LazyLock has been consistently applied across all static configuration variables, maintaining parity with the implementation in darwin.rs. The only difference is the use of DARWIN_V2_TEST_DIR environment variable for the work directory, which is appropriate for this V2 implementation.
|
|
||
| SCROLL_PROVER_ASSETS_DIRS.get_or_init(|| dirs.into_iter().map(|s| s.to_string()).collect()); | ||
| log::info!( | ||
| "init SCROLL_PROVER_ASSETS_DIRS: {:?}", | ||
| SCROLL_PROVER_ASSETS_DIRS | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider improving error handling in initialization
The initialization could benefit from better error handling and validation.
pub fn init() -> Result<()> {
let value = std::env::var(SCROLL_PROVER_ASSETS_DIR_ENV_NAME)?;
let dirs: Vec<&str> = value.split(',').collect();
if dirs.len() != 2 {
bail!("env variable SCROLL_PROVER_ASSETS_DIR value must be 2 parts seperated by comma.")
}
- SCROLL_PROVER_ASSETS_DIRS.get_or_init(|| dirs.into_iter().map(|s| s.to_string()).collect());
+ if SCROLL_PROVER_ASSETS_DIRS.set(dirs.into_iter().map(|s| s.to_string()).collect()).is_err() {
+ bail!("SCROLL_PROVER_ASSETS_DIRS was already initialized");
+ }
log::info!(
"init SCROLL_PROVER_ASSETS_DIRS: {:?}",
SCROLL_PROVER_ASSETS_DIRS
);
Ok(())Committable suggestion skipped: line range outside the PR's diff.
|
Do you have time to review this PR? @georgehao |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
prover/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (6)
prover/Cargo.toml(0 hunks)prover/src/config.rs(3 hunks)prover/src/version.rs(2 hunks)prover/src/zk_circuits_handler/common.rs(1 hunks)prover/src/zk_circuits_handler/darwin.rs(1 hunks)prover/src/zk_circuits_handler/darwin_v2.rs(1 hunks)
💤 Files with no reviewable changes (1)
- prover/Cargo.toml
🚧 Files skipped from review as they are similar to previous changes (4)
- prover/src/zk_circuits_handler/darwin_v2.rs
- prover/src/version.rs
- prover/src/zk_circuits_handler/common.rs
- prover/src/zk_circuits_handler/darwin.rs
🔇 Additional comments (3)
prover/src/config.rs (3)
4-4: Good use of std::sync::OnceLock for thread-safety!
The transition from a mutable static to OnceLock is a great improvement that aligns with Rust's safety principles and the PR's objective of utilizing standard library features.
Also applies to: 54-54
66-71: Consider improving error handling in initialization
The initialization could benefit from better error handling and validation.
76-87: Refactor repeated code in enable methods
The enable_first and enable_second methods contain significant code duplication and multiple unwrap calls.
Also applies to: 91-102
| debug_assert!( | ||
| SCROLL_PROVER_ASSETS_DIRS.get().is_some() | ||
| && SCROLL_PROVER_ASSETS_DIRS.get().unwrap().len() >= 2 | ||
| ); | ||
| log::info!( | ||
| "set env {SCROLL_PROVER_ASSETS_DIR_ENV_NAME} to {}", | ||
| &SCROLL_PROVER_ASSETS_DIRS.get().unwrap()[0] | ||
| ); | ||
| std::env::set_var( | ||
| SCROLL_PROVER_ASSETS_DIR_ENV_NAME, | ||
| &SCROLL_PROVER_ASSETS_DIRS.get().unwrap()[0], | ||
| ); |
There was a problem hiding this comment.
💡 Codebase verification
Based on the search results, I can now generate the final response:
Environment variable usage is consistent but has multiple setters
The SCROLL_PROVER_ASSETS_DIR environment variable is used consistently across the codebase, but there are multiple locations where it's being set:
prover/src/config.rs: Sets the variable twice with different values from a split arraycommon/libzkp/impl/src/verifier.rs: Sets usinglow_conf.assets_pathcommon/libzkp/impl/src/verifier/darwin.rs: Sets usingassets_dircommon/libzkp/impl/src/verifier/darwin_v2.rs: Sets usingassets_dir
The variable is properly validated in config.rs to ensure it contains at least 2 comma-separated paths, but the other setters might not follow this requirement. Consider centralizing the environment variable management to avoid potential conflicts.
🔗 Analysis chain
Verify environment variable usage across the codebase
Let's ensure that the SCROLL_PROVER_ASSETS_DIR environment variable is consistently used and that there are no conflicting usages.
Also applies to: 91-102
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for consistent usage of SCROLL_PROVER_ASSETS_DIR environment variable
# Search for all occurrences of the environment variable
echo "Searching for environment variable usage..."
rg -A 2 "SCROLL_PROVER_ASSETS_DIR"
# Search for potential environment variable setters
echo "Checking for environment variable setters..."
ast-grep --pattern 'std::env::set_var($name, $value)'
Length of output: 3884
Purpose or design rationale of this PR
Using some std libs instead of third-party lib, and removing unnecessary deps.
This PR also removes some unnecessary unsafe blocks.
PR title
Your PR title must follow conventional commits (as we are doing squash merge for each PR), so it must start with one of the following types:
Deployment tag versioning
Has
tagincommon/version.gobeen updated or have you addedbump-versionlabel to this PR?Breaking change label
Does this PR have the
breaking-changelabel?Summary by CodeRabbit
New Features
Bug Fixes
Refactor