[x] When I open src folder I see a lot of files there, it's good practice to only have main there and the other files in sub-folders. Similar to what you did with src/sample_circuits/
[x] Is src/sample_circuits/example4.rs an actual Circuit? If so, I suggest renaming it
[x] Maybe for the src/sample_circuits/ folder you could go one step further and have sub-folders for the circuits? I see that a lot of them is some sort of derivation of the Fibonacci Circuit?
Testing
[x] All the tests are inside the circuit themselves. I would recommend creating a separate test folder for the tests. Then if you have a separate folder for each circuit then inside you would have a few files with the logic of the circuit and a file for the tests. Or you could have a test folder with different test files for each circuit. This is a nice doc for testing, sometime I peak at it myself: https://doc.rust-lang.org/book/ch11-03-test-organization.html
Code
[x] Run the following to let Rust format the code for you:
cargo fmt --
[x] Run the following to get clippy sugestions from Rust
[ ] I would also add comments to private functions and structs. These can be either the regular comments // or the documentation comments ///
[x] I saw some variables that start with a double underscore, like __enabled_selectors. In Rust, variables that start with underscore are only used to indicate that such variable is unused. Therefore, I would recommend removing underscores from any variable that's being used.
[x] I see that some of the benchmark code is not being used. If you only plan to use such code for benchmarking, I recommend introducing features for the project and/or using profiles. For example, if you introduce a feature to the project called benches you could have a code like the following where benchmark mod would only compile if you enable feature benches.
#[cfg(feature = "benches")]
pub mod benchmark;
[ ] Substitute all println! with either log::info!, log::warn! or log::err!. This crate in particular.
[x] If you want to add a very nice touch to the codebase, I would suggest introducing custom errors. Where everywhere you expect an error, instead of panicking or just logging something to stderr, you could return such custom Error using error propagation. I can help with this one to get you started :) . This might be a good doc too: https://doc.rust-lang.org/book/ch09-02-recoverable-errors-with-result.html
[x] Also on the error wagon, I recommend getting rid of all unwrap() calls and handle them appropriately (potentially propagating them back with question mark ?). An easy way to do this would be to make all those methods return Result<()>. You can get that from the anyhow crate: https://crates.io/crates/anyhow
By the way, it's totally fine to have unwrap()'s on test code as test code is supposed to be clear and explicit
[x] Everywhere you have something string literal that you want to convert to String use to_owned() instead of to_string(). to_string() might make extra memory allocations which make to_owned() more performant. So something that looks like "bla".to_string() should become "bla".to_owned.
[x] I see a few comments that says //*** what is this? */, I ask the same question :) Remove that or add a TODO.
I liked integration_tests.rs, the tests there are short, very explicit, predictable and easy to follow.
I didn't look into halo2 repo as I assume you only brought that in to modify some struct's and functions' access scope.
Organizational
srcfolder I see a lot of files there, it's good practice to only have main there and the other files in sub-folders. Similar to what you did withsrc/sample_circuits/src/sample_circuits/example4.rsan actual Circuit? If so, I suggest renaming itsrc/sample_circuits/folder you could go one step further and have sub-folders for the circuits? I see that a lot of them is some sort of derivation of the Fibonacci Circuit?Testing
Code
[x] Run the following to let Rust format the code for you:
[x] Run the following to get clippy sugestions from Rust
You might need to install clippy with:
[x] Remove all comments that's just hanging there, the famous debug comments.
[x] Add documentation comments to all public functions and structs (including tests). These are the comments that start with
///. If you want to get fancy on the comments I recommend checking this out: https://doc.rust-lang.org/book/ch14-02-publishing-to-crates-io.html#making-useful-documentation-comments[ ] I would also add comments to private functions and structs. These can be either the regular comments
//or the documentation comments///[x] I saw some variables that start with a double underscore, like
__enabled_selectors. In Rust, variables that start with underscore are only used to indicate that such variable is unused. Therefore, I would recommend removing underscores from any variable that's being used.[x] I see that some of the benchmark code is not being used. If you only plan to use such code for benchmarking, I recommend introducing features for the project and/or using profiles. For example, if you introduce a feature to the project called
benchesyou could have a code like the following wherebenchmarkmod would only compile if you enable featurebenches.[ ] Substitute all
println!with eitherlog::info!,log::warn!orlog::err!. This crate in particular.[x] If you want to add a very nice touch to the codebase, I would suggest introducing custom errors. Where everywhere you expect an error, instead of panicking or just logging something to stderr, you could return such custom Error using error propagation. I can help with this one to get you started :) . This might be a good doc too: https://doc.rust-lang.org/book/ch09-02-recoverable-errors-with-result.html
[x] Also on the error wagon, I recommend getting rid of all
unwrap()calls and handle them appropriately (potentially propagating them back with question mark?). An easy way to do this would be to make all those methods returnResult<()>. You can get that from theanyhowcrate: https://crates.io/crates/anyhowunwrap()'s on test code as test code is supposed to be clear and explicit[x] This is what I call magic numbers: https://github.com/quantstamp/research-zk-fm/blob/main/korrekt/src/analyzer_io.rs#L38-L65 inside the match statement for
1and2create const variables for them and use that instead. There's some other places that I see that too like line 116 of that same file.[x] Everywhere you have something string literal that you want to convert to String use
to_owned()instead ofto_string().to_string()might make extra memory allocations which maketo_owned()more performant. So something that looks like"bla".to_string()should become"bla".to_owned.[x] I see a few comments that says
//*** what is this? */, I ask the same question :) Remove that or add a TODO.I liked
integration_tests.rs, the tests there are short, very explicit, predictable and easy to follow.I didn't look into halo2 repo as I assume you only brought that in to modify some struct's and functions' access scope.