refactor(prism-bin): renaming to prism-cli, refactoring args/config, and adding full-node type #145
Conversation
WalkthroughThe pull request includes significant changes to the Changes
Possibly related PRs
Suggested reviewers
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: 5
🧹 Outside diff range and nitpick comments (14)
Cargo.toml (1)
Line range hint
18-92: Summary: Consistent renaming and workspace updatesThe changes in this file consistently implement the renaming from "prism-bin" to "prism-cli" across workspace configuration and dependencies. This aligns well with the PR objectives and improves the clarity of the project structure.
Key points:
- Default members updated (line 18)
- Workspace members updated (line 29)
- Workspace dependencies updated (line 92)
These changes should provide a more intuitive experience for users and developers working with the CLI components of the project.
Consider updating the project's documentation, especially the README.md file, to reflect these naming changes and provide updated instructions for building and using the CLI.
README.md (3)
48-56: LGTM: Clear building instructions addedThe new "Building" section is a valuable addition to the README, providing clear instructions for building the project. It aligns with the PR objective of renaming to "prism-cli" and improves the overall documentation.
Consider adding a brief explanation of what SP1 is and why it's used, as it might not be familiar to all users. For example:
This will compile the `prism-cli` binary and sp1 `ELF` that are used to run the prover, light-client, and full-node. + +SP1 (Succinct Proofs of Execution) is used for generating zero-knowledge proofs in our system.
68-81: LGTM: Comprehensive instructions for starting different node typesThe updated "Starting the prover" section provides clear instructions for running Prism in different modes (prover, light-client, full-node). This aligns well with the PR objectives of renaming to "prism-cli" and introducing the new full-node type.
Consider adding a brief explanation of when a user might choose each node type. For example:
3. as a full-node (acts as a service provider, processing all transactions and making the state available to the light-clients) + +Choose the prover mode if you're running a service provider, the light-client mode for verifying proofs, or the full-node mode for both processing transactions and serving light clients.
83-88: LGTM: Clear instructions for starting light-client and full-nodeThe instructions for starting the light-client or full-node are clear and align well with the PR objectives. The use of the
--verifying-keyparameter is correctly explained.Consider adding a brief explanation of the difference between light-client and full-node modes when starting with this command. For example:
prism-cli light-client|full-node --verifying-key <verifying-key> + +Use 'light-client' for a minimal setup that only verifies proofs, or 'full-node' for a setup that processes transactions and serves light clients.Also, consider updating the link to webserver.rs to use a relative path within the repository, making it easier to maintain:
-You can then interact with Prism via the interfaces defined in [webserver.rs](https://github.com/deltadevsde/prism/blob/crates/prover/src/webserver.rs). +You can then interact with Prism via the interfaces defined in [webserver.rs](crates/prover/src/webserver.rs).justfile (1)
Line range hint
1-158: Consider adding commands for new CLI functionalityWhile the existing commands are comprehensive, consider adding new commands to showcase and test the new CLI functionality introduced in this PR. For example:
- A command to run the new full-node type with a sample verifying key.
- A command to demonstrate the new argument structure with flags positioned after the node type.
These additions would help users and developers quickly test and understand the new features.
Here's a suggestion for new commands:
# Run a full-node with a sample verifying key full-node-test: @echo "Running full-node with sample verifying key..." cargo run --release -- full-node --verifying-key <sample_key_here> # Demonstrate new CLI argument structure cli-demo: @echo "Demonstrating new CLI argument structure..." cargo run --release -- <node_type> --flag1 value1 --flag2 value2Replace
<sample_key_here>,<node_type>, and the flags with appropriate values based on your implementation.crates/common/src/keys.rs (2)
112-117: LGTM! Consider using a constant for consistency.The implementation of
std::fmt::DisplayforVerifyingKeylooks good. It correctly encodes the key bytes using the base64 engine and writes the result to the formatter.For consistency with the
TryFrom<String>implementation, consider using a constant for the base64 engine:+ const BASE64_ENGINE: base64::engine::GeneralPurpose = base64::engine::general_purpose::STANDARD; impl std::fmt::Display for VerifyingKey { fn fmt(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result { - let encoded = engine.encode(self.as_bytes()); + let encoded = BASE64_ENGINE.encode(self.as_bytes()); write!(f, "{}", encoded) } }This change would make the code more maintainable and reduce the risk of inconsistencies if the base64 engine needs to be changed in the future.
112-117: Add a test for the new Display implementation.The new
Displayimplementation forVerifyingKeylooks good, but it's important to ensure its correctness with a unit test. This will also help maintain the implementation in the future.Consider adding the following test to the
testsmodule:#[test] fn test_verifying_key_display() { let ed25519_vk = SigningKey::Ed25519(Box::new(Ed25519SigningKey::new(OsRng))).verifying_key(); let encoded = format!("{}", ed25519_vk); let decoded_vk = VerifyingKey::try_from(encoded).unwrap(); assert_eq!(ed25519_vk, decoded_vk); let secp256k1_vk = SigningKey::Secp256k1(Secp256k1SigningKey::new(&mut OsRng)).verifying_key(); let encoded = format!("{}", secp256k1_vk); let decoded_vk = VerifyingKey::try_from(encoded).unwrap(); assert_eq!(secp256k1_vk, decoded_vk); }This test ensures that the
Displayimplementation produces a string that can be correctly parsed back into aVerifyingKey, for both Ed25519 and Secp256k1 key types.crates/da/src/celestia.rs (1)
71-75: Simplification ofoperation_namespacecreation looks good.The changes to
operation_namespacecreation are consistent with theCelestiaConfigstruct changes and simplify the code. The removal of the fallback tosnark_namespaceis appropriate given thatoperation_namespace_idis now a required field.Consider wrapping the entire
create_namespacecall in aResult::map_errto provide more context:let operation_namespace = create_namespace(&config.operation_namespace_id) .map_err(|e| anyhow!( "Failed to create operation namespace from '{}': {}", &config.operation_namespace_id, e ))?;This change would provide more detailed error information if namespace creation fails.
crates/cli/src/main.rs (1)
83-90: Improve error handling by using a custom error typeMapping all errors to
std::io::ErrorKind::Othermay limit the granularity of error information. Consider defining a custom error type or using crates likethiserrororanyhowfor more descriptive and structured error handling. This can enhance debugging and provide clearer context when errors occur.Also applies to: 133-140
crates/cli/src/cfg.rs (4)
21-41: Consider Documenting All Command-Line ArgumentsWhile some fields in
CommandArgshave documentation comments, not all fields are documented. Adding doc comments to all fields, such asredis_client,verifying_key, andconfig_path, would improve code readability and help users understand the purpose of each argument.To address this, consider adding documentation like:
/// Redis connection string #[arg(short = 'r', long)] redis_client: Option<String>, /// Path to the verifying key file #[arg(long)] verifying_key: Option<String>, /// Path to the configuration file #[arg(long)] config_path: Option<String>,
Line range hint
115-139: Improve Error Handling inload_configFunctionThe
load_configfunction currently warns if theverifying_keyis not provided but continues execution. Given that not providing a verifying key is "not recommended," consider elevating this warning to an error or providing a fallback mechanism to ensure the application's security and integrity.Consider modifying the code as follows to enforce the presence of a verifying key:
if final_config.verifying_key.is_none() { - warn!("prover's public key was not provided. this is not recommended and epoch signatures will not be verified."); + return Err(GeneralError::MissingArgumentError("Verifying key is required for secure operation.".to_string()).into()); }
Line range hint
148-188: Refactorapply_command_line_argsto Reduce RepetitionThe
apply_command_line_argsfunction contains repetitive patterns when applying command-line arguments over the configuration. This can be simplified to enhance maintainability and readability.Consider using utility functions or macros to abstract the repetitive
unwrap_orpatterns. For example:fn merge_option<T>(arg: Option<T>, config: T) -> T { arg.unwrap_or(config) }And then apply it:
enabled: merge_option(args.webserver.webserver_active, webserver_config.enabled),
Line range hint
190-216: Clarify the Use ofunreachable!()MacroIn the
initialize_da_layerfunction, the use ofunreachable!()after a loop that handles all cases might be confusing to readers.Consider refactoring to explicitly return an error when the maximum number of attempts is exceeded:
Err(DataAvailabilityError::NetworkError(format!( "failed to connect to celestia node after {} attempts", DA_RETRY_COUNT )).into())This makes the code's intent clearer and avoids relying on the
unreachable!()macro.crates/node_types/prover/src/prover/mod.rs (1)
43-43: Correct the documentation link forverifying_keyThe documentation comment for
verifying_keyincludes a link that may not render correctly:/// This is not necessarily the counterpart to signing_key, as fullnodes must use the [`verifying_key`] of the prover.Ensure that the link to
verifying_keyis correctly formatted so that it references the field properly in the generated documentation.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (12)
- Cargo.toml (3 hunks)
- README.md (1 hunks)
- crates/bin/src/main.rs (0 hunks)
- crates/cli/Cargo.toml (1 hunks)
- crates/cli/src/cfg.rs (8 hunks)
- crates/cli/src/main.rs (1 hunks)
- crates/common/src/keys.rs (1 hunks)
- crates/da/src/celestia.rs (2 hunks)
- crates/node_types/lightclient/src/lightclient.rs (1 hunks)
- crates/node_types/prover/src/prover/mod.rs (6 hunks)
- crates/tests/src/lib.rs (1 hunks)
- justfile (1 hunks)
💤 Files with no reviewable changes (1)
- crates/bin/src/main.rs
✅ Files skipped from review due to trivial changes (1)
- crates/node_types/lightclient/src/lightclient.rs
🧰 Additional context used
🔇 Additional comments (17)
crates/cli/Cargo.toml (1)
2-2: LGTM: Package name updated correctly.The package name has been successfully updated from "prism-bin" to "prism-cli", which aligns with the PR objectives of renaming the project for better clarity and user experience.
To ensure consistency across the project, please run the following script to check for any remaining references to the old package name:
If the script returns any results (excluding changelog or history files), those occurrences should be updated to "prism-cli" for consistency.
✅ Verification successful
To ensure no remaining references to "prism-bin" exist outside of
Cargo.toml, please run the following script:
Verified: No remaining references to "prism-bin" found outside of
Cargo.toml.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for any remaining references to "prism-bin" in the project # Test: Search for "prism-bin" in all files. Expect: No results or only in changelog/history files. rg --type-not toml "prism-bin"Length of output: 30
Script:
#!/bin/bash # Description: Search for exact references to "prism-bin" in all non-TOML files rg --type-not toml '\bprism-bin\b'Length of output: 36
Cargo.toml (3)
18-18: LGTM: Updated default-members to include CLI crateThe change from "crates/bin" to "crates/cli" in the default-members list aligns with the PR objective of renaming the project. This ensures that the CLI crate is included in the default build, which is the desired behavior.
29-29: LGTM: Updated members list to include CLI crateThe change from "crates/bin" to "crates/cli" in the members list is consistent with the update in default-members. This ensures that the CLI crate is properly recognized as a workspace member, which is correct.
92-92: LGTM: Updated workspace dependency to prism-cliThe change from "prism-bin" to "prism-cli" in the workspace dependencies is correct and aligns with the project renaming. The path is correctly updated to "crates/cli".
To ensure this change is consistently applied throughout the project, please run the following script:
This will help identify any places where the renaming might have been missed or where the new name should be used.
✅ Verification successful
Verified: All references successfully updated from prism-bin to prism-cli
The verification confirms that there are no remaining references to "prism-bin" and that "prism-cli" is correctly referenced in the relevant files. The renaming has been properly implemented across the project.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that all references to prism-bin have been updated to prism-cli # Test: Search for any remaining references to prism-bin echo "Searching for remaining references to prism-bin:" rg --type toml --type rust 'prism-bin' # Test: Confirm the presence of prism-cli in relevant files echo "Confirming the presence of prism-cli:" rg --type toml --type rust 'prism-cli'Length of output: 357
README.md (3)
40-46: LGTM: Simplified dependency installation processThe new "Install Dependencies" section effectively simplifies the installation process by using
justas a task runner. This change aligns well with the PR objective of cleaning up the configuration process and provides a more streamlined experience for users.
58-64: LGTM: Clear instructions for running local DA layerThe updated instructions for running a local Celestia network are clear and concise. The use of
just celestia-upmaintains consistency with the installation process and simplifies the setup for users.
Line range hint
1-88: Overall: Excellent improvements to README clarity and completenessThe changes to the README.md file significantly improve its clarity and completeness. The updates accurately reflect the renaming from "prism-bin" to "prism-cli", introduce instructions for the new full-node type, and provide a more streamlined installation and usage process. The document now offers a comprehensive guide for users to get started with Prism, aligning well with the PR objectives.
Key improvements:
- Simplified dependency installation using
just.- Clear building instructions added.
- Updated and consistent commands for running different node types.
- Introduction of the full-node concept alongside existing prover and light-client modes.
These changes will greatly enhance the user experience for newcomers to the Prism project.
justfile (1)
90-92: Update in coverage command reflects project renamingThe change from
--exclude prism-binto--exclude prism-cliin the coverage command aligns with the project's renaming from "prism-bin" to "prism-cli" as mentioned in the PR objectives.To ensure this change is consistent across the project, let's verify the renaming:
✅ Verification successful
Renaming from
prism-bintoprism-cliSuccessfully Verified
- No remaining references to
prism-binwere found.prism-cliis correctly defined inCargo.toml.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the renaming from prism-bin to prism-cli across the project # Test 1: Check for any remaining references to prism-bin echo "Checking for any remaining references to prism-bin:" rg "prism-bin" --type-not toml # Test 2: Verify the existence of prism-cli in Cargo.toml echo "Verifying the existence of prism-cli in Cargo.toml:" rg "prism-cli" -g "Cargo.toml"Length of output: 373
crates/tests/src/lib.rs (1)
79-81: LGTM! Verify Config struct definition in prism_prover crate.The update to the
prover_cfgconfiguration structure looks good. The change fromkey: signing_keytosigning_keyaligns with the expected structure of theConfigtype.To ensure consistency across the codebase, please run the following script to verify the
Configstruct definition in theprism_provercrate:crates/common/src/keys.rs (1)
112-117: Summary: Good addition, enhances VerifyingKey functionality.The new
Displayimplementation forVerifyingKeyis a valuable addition that complements the existingTryFrom<String>implementation. It provides a convenient way to convertVerifyingKeyinstances to base64-encoded strings, which is useful for display and serialization purposes.The implementation is correct, consistent with the existing code, and doesn't introduce any breaking changes. It enhances the overall functionality of the
VerifyingKeyenum without negatively impacting other parts of the codebase.To further improve this change:
- Consider using a constant for the base64 engine for consistency.
- Add a unit test to ensure the correctness of the new implementation.
Overall, this is a solid improvement to the
keys.rsmodule.crates/da/src/celestia.rs (2)
43-45: Verify impact of changing defaultstart_heightto 1.The default value for
start_heighthas been changed from 0 to 1. This change might affect the behavior of code that relies on the default value.Please review all code that uses
CelestiaConfigwith default values to ensure this change doesn't introduce unexpected behavior. You can use the following script to find relevant code:#!/bin/bash # Search for CelestiaConfig::default() usage in the codebase rg --type rust "CelestiaConfig::default\(\)" -C 5The change to
operation_namespace_id(removingSome) is consistent with the struct field type change and looks good.
36-36: Verify impact of makingoperation_namespace_idnon-optional.The
operation_namespace_idfield has been changed fromOption<String>toString, making it a required field. This change simplifies the usage of this field but may break existing code that relies on it being optional.Please ensure that all code using
CelestiaConfighas been updated to provide a value foroperation_namespace_id. You can use the following script to check for potential issues:crates/cli/src/main.rs (1)
19-19: Verify the correctness of the path ininclude_bytes!The
include_bytes!macro uses the relative path"../../../elf/riscv32im-succinct-zkvm-elf". Ensure that this path is correct and that the ELF file exists in the repository. Relative paths can cause issues in different build environments.Run the following script to verify the existence of the required ELF file:
✅ Verification successful
Path Verification Successful
The
include_bytes!macro correctly references the ELF file at../../../elf/riscv32im-succinct-zkvm-elf. The file exists in the repository, and the relative path is accurate.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the required ELF file exists in the repository. # Expected result: The file should be found within the `elf/` directory. fd 'riscv32im-succinct-zkvm-elf' elf/Length of output: 69
crates/cli/src/cfg.rs (4)
Line range hint
15-19: Appropriate Use of Subcommands with Associated ArgumentsIncluding
CommandArgsin each variant of theCommandsenum leverages Clap's subcommand functionality effectively, allowing each command to have its own set of arguments. This enhances modularity and aligns with best practices for command-line interface design.
43-48: RenamingCommandLineArgstoCliEnhances ClarityRenaming the
CommandLineArgsstruct toCliimproves code readability and conciseness. It aligns with common conventions and makes the purpose of the struct immediately clear.
Line range hint
50-68: Effective Use of Flattened Argument StructuresUsing
#[command(flatten)]to includeCelestiaArgsandWebserverArgswithinCommandArgspromotes modularity and reusability. This approach neatly organizes related arguments and helps maintain a clean command-line interface.Also applies to: 70-83
Line range hint
86-94: Ensure All Optional Configurations Are Appropriately InitializedThe
Configstruct contains several optional fields, such aswebserver,celestia_config, andredis_config. While defaults are provided, it's important to verify that these configurations are properly initialized to prevent potentialNonedereferencing at runtime.Run the following script to check for any usages where these optional configurations might be unwrapped without checking:
✅ Verification successful
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (11)
crates/cli/Cargo.toml (1)
Line range hint
9-13: Consider documenting feature flags.The package includes several feature flags (
default,groth16,test_utils,mock_prover) but their purpose isn't immediately clear. Consider adding documentation comments to explain each feature's functionality.Example documentation:
[features] +# Default features default = [] +# Enable Groth16 zero-knowledge proof system groth16 = [] +# Enable test utilities test_utils = [] +# Enable mock prover for testing mock_prover = []README.md (4)
40-46: Consider listing key dependencies for transparency.While using
just install-depssimplifies the installation process, it would be helpful to list the key dependencies that will be installed. This helps users understand the system requirements upfront and troubleshoot if any issues arise.
48-56: Consider adding build requirements and options.The build instructions are clear but could benefit from:
- Mentioning any build prerequisites (e.g., required Rust version)
- Documenting any available build flags or configurations
58-64: Add verification steps for Celestia network.Consider adding:
- Expected output or indicators of successful network startup
- How to verify the network is running correctly
- Common troubleshooting steps
70-73: Clarify differences between node types.Consider adding brief descriptions of:
- The specific responsibilities of each node type
- When to use each type
- Resource requirements for different modes
crates/cli/src/main.rs (1)
18-18: Add documentation for the PRISM_ELF constant.Consider adding documentation that explains the purpose and usage of this embedded ELF binary, including its format and any size constraints.
crates/cli/src/cfg.rs (4)
114-119: Add documentation for DALayerOption enum.Consider adding documentation comments to explain:
- The purpose of each variant
- When to use each option
- The implications of choosing one over the other
Line range hint
147-155: Consider more flexible config path resolution.The current config path resolution only checks two locations:
- Command-line provided path
- ~/.prism/config.toml
Consider adding support for:
- XDG base directory specification
- Environment variable for config path
- Working directory config file
Example implementation:
fn get_config_path(args: &CommandArgs) -> Result<String> { // Priority order: // 1. Command line argument // 2. Environment variable // 3. Working directory // 4. User config directory (XDG or ~/.prism) args.config_path .clone() .or_else(|| std::env::var("PRISM_CONFIG").ok()) .or_else(|| { std::path::Path::new("prism.toml") .exists() .then(|| "prism.toml".to_string()) }) .or_else(|| { dirs::config_dir().map(|path| { path.join("prism") .join("config.toml") .to_string_lossy() .into_owned() }) }) .or_else(|| { home_dir().map(|path| format!("{}/.prism/config.toml", path.to_string_lossy())) }) .ok_or_else(|| { GeneralError::MissingArgumentError("could not determine config path".to_string()).into() }) }
Line range hint
171-211: Simplify nested configuration application.The current implementation of
apply_command_line_argshas deep nesting and repetitive unwrap patterns. Consider refactoring to reduce complexity and improve readability.Example implementation:
fn apply_command_line_args(config: Config, args: CommandArgs) -> Config { let webserver_config = config.webserver.unwrap_or_default(); let redis_config = config.redis_config.unwrap_or_default(); let celestia_config = config.celestia_config.unwrap_or_default(); let webserver = WebServerConfig { enabled: args.webserver.webserver_active.unwrap_or(webserver_config.enabled), host: args.webserver.host.unwrap_or(webserver_config.host), port: args.webserver.port.unwrap_or(webserver_config.port), }; let redis = RedisConfig { connection_string: args.redis_client.unwrap_or(redis_config.connection_string), }; let celestia = CelestiaConfig { connection_string: args.celestia.celestia_client.unwrap_or(celestia_config.connection_string), start_height: args.celestia.celestia_start_height.unwrap_or(celestia_config.start_height), snark_namespace_id: args.celestia.snark_namespace_id.unwrap_or(celestia_config.snark_namespace_id), operation_namespace_id: args.celestia.operation_namespace_id.unwrap_or(celestia_config.operation_namespace_id), }; Config { webserver: Some(webserver), redis_config: Some(redis), celestia_config: Some(celestia), da_layer: config.da_layer, verifying_key: args.verifying_key.or(config.verifying_key), } }
Line range hint
215-242: Well-implemented DA layer initialization with retry logic.The retry logic for Celestia connection is robust and includes proper error handling. However, consider adding a comment explaining why the
unreachable!()is safe here.Add a comment before the unreachable macro:
// Unreachable: the loop will either return Ok(da) or return Err on the last iteration unreachable!()crates/node_types/prover/src/prover/mod.rs (1)
56-64: Consider removing unnecessary cloneThe
signing_key.clone()is unnecessary sincecreate_signing_key()already returns an owned value that could be moved directly.Apply this diff to optimize the code:
- let signing_key = create_signing_key(); - - Config { - prover: true, - batcher: true, - webserver: WebServerConfig::default(), - signing_key: signing_key.clone(), - verifying_key: signing_key.verification_key(), - start_height: 1, - } + let signing_key = create_signing_key(); + let verifying_key = signing_key.verification_key(); + + Config { + prover: true, + batcher: true, + webserver: WebServerConfig::default(), + signing_key, + verifying_key, + start_height: 1, + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (12)
Cargo.toml(3 hunks)README.md(1 hunks)crates/bin/src/main.rs(0 hunks)crates/cli/Cargo.toml(1 hunks)crates/cli/src/cfg.rs(8 hunks)crates/cli/src/main.rs(1 hunks)crates/common/src/keys.rs(1 hunks)crates/da/src/celestia.rs(2 hunks)crates/node_types/lightclient/src/lightclient.rs(1 hunks)crates/node_types/prover/src/prover/mod.rs(6 hunks)crates/tests/src/lib.rs(1 hunks)justfile(1 hunks)
💤 Files with no reviewable changes (1)
- crates/bin/src/main.rs
🚧 Files skipped from review as they are similar to previous changes (6)
- Cargo.toml
- crates/common/src/keys.rs
- crates/da/src/celestia.rs
- crates/node_types/lightclient/src/lightclient.rs
- crates/tests/src/lib.rs
- justfile
🔇 Additional comments (8)
crates/cli/Cargo.toml (1)
2-2: Package rename looks good, verifying workspace configuration.
The rename from prism-bin to prism-cli is a good change that better reflects the package's purpose and follows established naming conventions.
Let's verify the workspace configuration is updated correctly:
✅ Verification successful
Package rename is safe and workspace configuration is correct
The verification shows that:
- The workspace members in root Cargo.toml correctly reference
crates/cli - No references to the old package name
prism-binwere found in:- Other crates' dependencies
- Rust imports across the codebase
- Workspace dependencies
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify workspace configuration and dependencies are updated
# Test 1: Check if workspace members are updated in root Cargo.toml
echo "Checking workspace members configuration..."
rg -A 5 "members\s*=|default-members\s*=" Cargo.toml
# Test 2: Check for any remaining references to prism-bin
echo "Checking for old package references..."
rg -l "prism-bin" -g '!.git/*'
Length of output: 560
Script:
#!/bin/bash
# Let's check for any dependencies on the old package name in other crates' Cargo.toml files
# Search for dependencies on prism-bin in all Cargo.toml files
echo "Checking dependencies in Cargo.toml files..."
fd Cargo.toml --exec grep -l "prism-bin"
# Check if there are any imports using the old name in Rust files
echo "Checking imports in Rust files..."
rg -l "use.*prism_bin|use.*prism-bin" --type rust
# Check workspace.dependencies section in root Cargo.toml
echo "Checking workspace dependencies..."
rg -A 10 "\[workspace\.dependencies\]" Cargo.toml
Length of output: 797
crates/cli/src/main.rs (2)
73-73: Review default configuration usage.
The code uses unwrap_or_default() for webserver configuration. This could potentially expose unintended defaults if the configuration is missing. Consider:
- Explicitly documenting the default values
- Making these configurations required
- Using domain-specific defaults instead of type defaults
#!/bin/bash
# Search for other default usage patterns
rg "unwrap_or_default"Also applies to: 131-131
40-45: Standardize verifying key handling across commands.
The verifying key handling differs between commands:
- LightClient makes it optional
- FullNode requires it
- Prover derives it from signing key
Consider standardizing this behavior or documenting why different approaches are necessary for security reasons.
Also applies to: 114-126
crates/cli/src/cfg.rs (1)
Line range hint 1-25: Well-structured command hierarchy!
The command structure effectively implements the new node types mentioned in the PR objectives, with a clean and consistent approach using shared CommandArgs.
crates/node_types/prover/src/prover/mod.rs (4)
43-48: LGTM! Clear distinction between signing and verifying keys
The renaming of key to signing_key and addition of verifying_key with clear documentation improves code clarity and security by explicitly separating the concerns of signing and verification.
270-275: Skipping comment as it duplicates existing review
A previous review already correctly identified the issue with panic usage in signature verification.
306-317: Skipping comment as it duplicates existing review
A previous review already correctly identified the issue with panic usage in proof verification.
409-409: LGTM! Correct usage of signing key
The signature insertion correctly uses the renamed signing_key from the Config struct.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
README.md (2)
56-56: Add context about the sp1 ELF.Consider explaining what the sp1 ELF is and its role in the system, as this might not be clear to new users.
58-58: Expand the DA acronym.Consider expanding "DA" to "Data Availability" for clarity, as this term might not be familiar to all users.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
README.md(1 hunks)
🔇 Additional comments (3)
README.md (3)
40-46: LGTM! Clear and concise dependency installation instructions.
The use of just as a task runner with a single command simplifies the installation process.
70-73: LGTM! Clear explanation of node types.
The different node types and their purposes are well explained.
88-88: LGTM! Correct link format.
The webserver.rs link is properly formatted with the full GitHub URL.
…and adding full-node nodetype
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (11)
README.md (5)
40-46: List the dependencies that will be installed.Please add a brief list of the core dependencies that
just install-depswill install. This helps users understand what will be installed on their system before running the command.
56-56: Clarify technical terms and component relationships.Please explain:
- What an
ELFfile is and its role in the system- The relationship between the
prism-clibinary and the sp1ELF
58-64: Add context about the DA layer and prerequisites.Please provide:
- A brief explanation of what a DA (Data Availability) layer is and its role in Prism
- Any prerequisites needed for running a local Celestia network
70-73: Clarify the differences between node types.Please explain:
- The key differences between a light-client and a full-node
- When to use each type of node
- The resource requirements for each node type
88-88: Add interface usage examples.Please provide examples of common interactions with the webserver interfaces, such as:
- Basic curl commands or code snippets
- Expected request/response formats
- Common use cases
crates/cli/src/cfg.rs (3)
114-119: Add documentation for DALayerOptionConsider adding documentation comments to explain when each variant should be used, especially the InMemory option which might be intended for testing.
Line range hint
170-204: Simplify configuration merging logicThe nested configuration merging in
apply_command_line_argsis complex and could be simplified using the builder pattern or a dedicated merge trait.Consider implementing a trait for merging configurations:
trait ConfigMerge { fn merge(self, args: &CommandArgs) -> Self; } impl ConfigMerge for WebServerConfig { fn merge(mut self, args: &CommandArgs) -> Self { if let Some(active) = args.webserver.webserver_active { self.enabled = active; } // ... similar for other fields self } }
Line range hint
209-236: Improve control flow in DA layer initializationThe
unreachable!()macro could be eliminated by restructuring the retry loop:- for attempt in 1..=DA_RETRY_COUNT { + let mut attempt = 1; + loop { match CelestiaConnection::new(&celestia_conf, None).await { Ok(da) => return Ok(Arc::new(da) as Arc<dyn DataAvailabilityLayer + 'static>), Err(e) => { - if attempt == DA_RETRY_COUNT { + if attempt >= DA_RETRY_COUNT { return Err(DataAvailabilityError::NetworkError(format!( "failed to connect to celestia node after {} attempts: {}", DA_RETRY_COUNT, e )) .into()); } error!("Attempt {} to connect to celestia node failed: {}. Retrying in {} seconds...", attempt, e, DA_RETRY_INTERVAL.as_secs()); tokio::time::sleep(DA_RETRY_INTERVAL).await; + attempt += 1; } } } - unreachable!()crates/da/src/celestia.rs (2)
175-175: Consider enhancing logging for better observabilityWhile the namespace changes are correct, consider adding debug logs when namespace operations fail to help with troubleshooting deployment issues during the transition.
Add debug logs before the operations:
let maybe_blobs = + debug!("Fetching blobs from operation namespace at height {}", height); BlobClient::blob_get_all(&self.client, height, &[self.operation_namespace])+debug!("Creating blob in operation namespace for transaction"); Blob::new(self.operation_namespace, data)Also applies to: 219-219
Line range hint
89-97: Consider adding namespace validationThe
create_namespacefunction could benefit from additional validation to ensure namespace IDs follow Celestia's requirements (e.g., length, format).Consider adding validation before hex decoding:
fn create_namespace(namespace_hex: &str) -> Result<Namespace> { // Validate namespace format if !namespace_hex.len() == 20 || !namespace_hex.chars().all(|c| c.is_ascii_hexdigit()) { return Err(anyhow!("Invalid namespace format. Expected 20 hex characters")); } let decoded_hex = hex::decode(namespace_hex)?; Namespace::new_v0(&decoded_hex) }crates/node_types/prover/src/prover/mod.rs (1)
58-67: Consider separating default implementation for production use.The current default implementation derives the
verifying_keyfrom thesigning_key. While this works for testing, in production:
- The verifying key should typically come from a trusted source (e.g., the designated prover)
- Using self-derived keys could potentially bypass security measures
Consider creating separate implementations for testing and production environments.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (12)
Cargo.toml(3 hunks)README.md(1 hunks)crates/bin/src/main.rs(0 hunks)crates/cli/Cargo.toml(1 hunks)crates/cli/src/cfg.rs(7 hunks)crates/cli/src/main.rs(1 hunks)crates/common/src/keys.rs(1 hunks)crates/da/src/celestia.rs(4 hunks)crates/node_types/lightclient/src/lightclient.rs(1 hunks)crates/node_types/prover/src/prover/mod.rs(6 hunks)crates/tests/src/lib.rs(1 hunks)justfile(1 hunks)
💤 Files with no reviewable changes (1)
- crates/bin/src/main.rs
🚧 Files skipped from review as they are similar to previous changes (7)
- Cargo.toml
- crates/cli/Cargo.toml
- crates/cli/src/main.rs
- crates/common/src/keys.rs
- crates/node_types/lightclient/src/lightclient.rs
- crates/tests/src/lib.rs
- justfile
🔇 Additional comments (8)
crates/cli/src/cfg.rs (4)
21-25: LGTM: Well-structured command hierarchy
The command structure effectively implements the new node types mentioned in the PR objectives while maintaining consistency by reusing CommandArgs across all variants.
51-54: LGTM: Clean CLI structure
The CLI struct follows clap's best practices with proper derive macros and subcommand handling.
76-89: Revise webserver argument configuration
Referencing previous review: The combination of requires = "webserver_active" with default_value on the host field is contradictory. Consider removing the requires constraint since it has a default value.
36-37: Verify verifying_key requirement for FullNode
The PR objectives mention that full-node requires a verifying key (--verifying-key <VK>), but it's implemented as optional. This might need verification.
crates/da/src/celestia.rs (2)
53-53: LGTM: Consistent renaming throughout CelestiaConnection
The renaming from transaction_namespace to operation_namespace has been applied consistently, including error messages and initialization code.
Also applies to: 71-75, 82-82
36-36:
Breaking changes detected in CelestiaConfig
The changes introduce two potentially breaking modifications:
operation_namespace_idis now a required field (no longerOption<String>)- Default
start_heightchanged from 0 to 1
Let's verify the impact of these changes:
Also applies to: 43-43, 45-45
crates/node_types/prover/src/prover/mod.rs (2)
45-50: LGTM! Clear distinction between signing and verification keys.
The renaming of key to signing_key and addition of verifying_key improves clarity by explicitly distinguishing between keys used for signing and verification. The documentation accurately describes their purposes.
414-414: LGTM! Consistent use of signing_key.
The update correctly uses the renamed signing_key field from the Config struct.
View new readme for instructions.
-prism-bin -> prism-cli (this is also reth's naming convention and feels more natural on command line)
prism-cli full-node --verifying-key <VK>(<VK>is logged when running the prover)Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores