DO NOT MERGE: testing #205
Conversation
Signed-off-by: Smuu <18609909+Smuu@users.noreply.github.com>
WalkthroughThis pull request introduces several modifications across multiple files in a Rust project. The changes include updating the build process with a new feature flag in 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}
Signed-off-by: Smuu <18609909+Smuu@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (7)
crates/cli/src/main.rs (3)
81-84: Consider adding error handling for invalid keystore type.While the current implementation has a fallback to
keychain, it might be better to explicitly handle invalid types.let keystore: Box<dyn KeyStore> = match config.keystore_type.as_str() { "file" => Box::new(FileStore::new(config.keystore_path.clone().into())), - "keychain" | _ => Box::new(KeyChain), + "keychain" => Box::new(KeyChain), + invalid_type => return Err(Error::new( + ErrorKind::InvalidInput, + format!("Invalid keystore type: {}", invalid_type), + )), };
86-96: Remove commented code.The commented-out code block should be removed as it's been replaced with a cleaner implementation.
97-99: Consider adding debug logging for key operations.While the error handling is good, adding debug logging for key operations would help with troubleshooting.
let signing_key = keystore.get_or_create_signing_key(SIGNING_KEY_ID).map_err(|e| { Error::new(ErrorKind::Other, format!("Failed to get or create signing key: {}", e)) })?; +debug!("Successfully obtained signing key");Also applies to: 120-120
crates/cli/src/cfg.rs (1)
179-196: Remove commented out functions.The commented-out helper functions
get_keystore_typeandget_keystore_pathshould be removed as they're not being used.Dockerfile (1)
28-34: Consider using a more minimal keyring solution.The addition of
gnome-keyringbrings in many dependencies. Consider using a more minimal solution likesecret-serviceif possible.- gnome-keyring \ + libsecret-1-0 \ci/docker-compose copy.yml (2)
1-1: Rename the file to follow standard naming convention.The file name
docker-compose copy.ymlappears to be a copy-paste artifact. Consider renaming it to something more descriptive likedocker-compose.devnet.ymlordocker-compose.mock.ymlto better reflect its purpose.
85-89: Remove commented curl examples and p2p address.These comments appear to be development notes and should be moved to documentation instead of being kept in the configuration file.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (6)
Cargo.toml(1 hunks)Dockerfile(1 hunks)ci/docker-compose copy.yml(1 hunks)crates/cli/Cargo.toml(1 hunks)crates/cli/src/cfg.rs(5 hunks)crates/cli/src/main.rs(6 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: unused dependencies
- GitHub Check: clippy
- GitHub Check: unit-test
- GitHub Check: integration-test
- GitHub Check: build-and-push-image
🔇 Additional comments (5)
crates/cli/src/main.rs (2)
6-6: LGTM! Dependencies look good.The new imports for
FileStore,SigningKeyEd, andOsRngalign with the keystore implementation changes.Also applies to: 10-11
24-24: LGTM! Good constant naming.The constant
SIGNING_KEY_IDfollows Rust naming conventions and provides a centralized location for the key identifier.crates/cli/src/cfg.rs (1)
48-59: LGTM! Well-documented configuration options.The new keystore configuration options are well-documented with clear descriptions and default values.
crates/cli/Cargo.toml (1)
36-37: LGTM! Dependencies align with implementation.The addition of
ed25519-consensusandranddependencies aligns with the keystore implementation changes.Cargo.toml (1)
76-76: Verify compatibility with mock prover functionality.The major version bump of
keystore-rsfrom 0.1.2 to 0.3.0 could introduce breaking changes. Since this PR is focused on testing the mock prover, please ensure this version is compatible with the mock prover implementation.
| pub keystore_type: String, | ||
| pub keystore_path: String, |
There was a problem hiding this comment.
💡 Codebase verification
Keystore path needs proper expansion and validation.
The current implementation uses a hardcoded "/.prism/keystore.json" path without proper expansion of the "" character. The previous implementation (now commented out) using dirs::home_dir() should be restored and enhanced with proper path validation.
crates/cli/src/cfg.rs: Add path expansion and validation before passing toFileStore::new()- Consider restoring and improving the commented out
get_keystore_path()implementation that usedhome_dir()
🔗 Analysis chain
Consider validating keystore path expansion.
The default keystore path uses ~ which needs to be expanded. Consider adding validation and path expansion.
Also applies to: 128-129
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if the keystore path is properly handled
rg -A 2 "keystore_path.*=.*~"
Length of output: 31
Script:
#!/bin/bash
# Find keystore_path initialization and usage
rg -g '!target' -A 3 "keystore_path"
# Look for path expansion utilities
rg -g '!target' "expand.*path|shellexpand|canonicalize"
# Check for shell-related path handling
ast-grep --pattern 'use std::path::{$$$}'
Length of output: 86584
Script:
#!/bin/bash
# Find FileStore implementation and path handling
rg -g '!target' "FileStore" -A 5
# Look for path-related imports and utilities
rg -g '!target' "use std::(path|fs)::"
# Check for shellexpand or similar crate usage in Cargo.toml
rg -g 'Cargo.toml' "shellexpand|home|dirs"
Length of output: 2190
Script:
#!/bin/bash
# Find dirs crate usage and path handling
rg -g '!target' "dirs::" -B 2 -A 2
# Look for path handling and validation
rg -g '!target' -B 2 -A 2 "keystore_path.*=|fn.*keystore"
# Check for path-related error handling
rg -g '!target' "Path|path" "fn.*validate|check.*path"
Length of output: 939
ci/docker-compose copy.yml
Outdated
| - --celestia-start-height=4180975 | ||
| - --operation-namespace-id=000000000000000000000000000000000000707269736d6f70657261 | ||
| - --snark-namespace-id=000000000000000000000000000000000000707269736d736f61726b | ||
| - --verifying-key= |
There was a problem hiding this comment.
Fix empty verifying key parameter.
The --verifying-key= parameter is empty, which could cause validation issues.
Also applies to: 71-71
ci/docker-compose copy.yml
Outdated
| services: | ||
| consensus-full: | ||
| image: ghcr.io/celestiaorg/celestia-app:v3.0.2 | ||
| volumes: | ||
| - ./data/consensus-full:/home/celestia | ||
| command: | ||
| - start | ||
| restart: always | ||
| da-bridge: | ||
| image: ghcr.io/celestiaorg/celestia-node:v0.20.4 | ||
| volumes: | ||
| - ./data/da-bridge:/home/celestia | ||
| environment: | ||
| - NODE_TYPE=bridge | ||
| - P2P_NETWORK=mocha-4 | ||
| command: | ||
| - celestia | ||
| - bridge | ||
| - start | ||
| - --core.ip=consensus-full-mocha-4.celestia-mocha.com # TODO: point to local consensus | ||
| - --p2p.network=mocha-4 | ||
| - --archival | ||
| - --rpc.skip-auth # TODO: remove | ||
| restart: always | ||
| da-light: | ||
| image: ghcr.io/celestiaorg/celestia-node:v0.20.4 | ||
| volumes: | ||
| - ./data/da-light:/home/celestia | ||
| environment: | ||
| - NODE_TYPE=light | ||
| - P2P_NETWORK=mocha-4 | ||
| command: | ||
| - celestia | ||
| - light | ||
| - start | ||
| - --core.ip=consensus-full-mocha-4.celestia-mocha.com # TODO: point to local consensus | ||
| - --p2p.network=mocha-4 | ||
| - --rpc.skip-auth # TODO: remove | ||
| restart: always | ||
| redis: | ||
| image: redis:latest | ||
| restart: always | ||
| volumes: | ||
| - ./data/redis:/data | ||
| command: ["redis-server", "--save", "60", "1", "--loglevel", "warning"] | ||
| prism-prover: | ||
| image: ghcr.io/deltadevsde/prism:pr-205 | ||
| volumes: | ||
| - ./data/prism-prover:/root/.prism | ||
| restart: always | ||
| entrypoint: ["sleep", "infinity"] | ||
| # command: | ||
| # - prover | ||
| # - --celestia-client=ws://da-bridge:26658 | ||
| # - --celestia-start-height=4180975 | ||
| # - --redis-client=redis://redis:6379 | ||
| # - --operation-namespace-id=000000000000000000000000000000000000707269736d6f70657261 | ||
| # - --snark-namespace-id=000000000000000000000000000000000000707269736d736f61726b | ||
| prism-full: | ||
| image: ghcr.io/deltadevsde/prism:pr-205 | ||
| volumes: | ||
| - ./data/prism-full:/root/.prism | ||
| restart: always | ||
| command: | ||
| - full-node | ||
| - --verifying-key | ||
| - --celestia-client=ws://da-bridge:26658 | ||
| - --celestia-start-height=4180975 | ||
| - --operation-namespace-id=000000000000000000000000000000000000707269736d6f70657261 | ||
| - --snark-namespace-id=000000000000000000000000000000000000707269736d736f61726b | ||
| - --verifying-key= | ||
| prism-light: | ||
| image: ghcr.io/deltadevsde/prism:pr-205 | ||
| volumes: | ||
| - ./data/prism-light:/root/.prism | ||
| restart: always | ||
| command: | ||
| - light-client | ||
| - --verifying-key | ||
| - --celestia-client=ws://da-bridge:26658 | ||
| - --celestia-start-height=4180975 | ||
| - --operation-namespace-id=000000000000000000000000000000000000707269736d6f70657261 | ||
| - --snark-namespace-id=000000000000000000000000000000000000707269736d736f61726b | ||
|
|
||
| # /dns/da-bridge/tcp/2121/p2p/12D3KooWDek5ZWNVpgKBXcBn7dajueYhLDm4SubTgwJj6MKKyudq | ||
|
|
||
|
|
||
| # curl --max-time 0.5 -s -X POST -H "Content-type: application/json" -H "Accept: application/json" -d '{"id":1,"jsonrpc":"2.0","method":"header.Subscribe","params":[]}' "http://localhost:26658" | ||
| # curl --max-time 0.5 -s -X POST -H "Content-type: application/json" -H "Accept: application/json" -H "Authorization: Bearer $AUTH_TOKEN" -d '{"id":1,"jsonrpc":"2.0","method":"header.Subscribe","params":[]}' "http://localhost:26658" |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add environment variable configuration.
Create a .env.example file to document all required environment variables and their expected values. This includes:
- REDIS_PASSWORD
- PRISM_VERSION
- CELESTIA_START_HEIGHT
- OPERATION_NAMESPACE_ID
- SNARK_NAMESPACE_ID
Would you like me to generate a .env.example file with all the required variables and documentation?
ci/docker-compose copy.yml
Outdated
| image: ghcr.io/deltadevsde/prism:pr-205 | ||
| volumes: | ||
| - ./data/prism-prover:/root/.prism | ||
| restart: always | ||
| entrypoint: ["sleep", "infinity"] | ||
| # command: | ||
| # - prover | ||
| # - --celestia-client=ws://da-bridge:26658 | ||
| # - --celestia-start-height=4180975 | ||
| # - --redis-client=redis://redis:6379 | ||
| # - --operation-namespace-id=000000000000000000000000000000000000707269736d6f70657261 | ||
| # - --snark-namespace-id=000000000000000000000000000000000000707269736d736f61726b | ||
| prism-full: |
There was a problem hiding this comment.
Fix prism-prover configuration.
The prism-prover service has several issues:
- Using PR-specific image tag (
pr-205) which isn't sustainable - The actual prover command is commented out
- Missing environment variables for configuration
prism-prover:
- image: ghcr.io/deltadevsde/prism:pr-205
+ image: ghcr.io/deltadevsde/prism:${PRISM_VERSION}
volumes:
- ./data/prism-prover:/root/.prism
restart: always
- entrypoint: ["sleep", "infinity"]
- # command:
- # - prover
- # - --celestia-client=ws://da-bridge:26658
- # - --celestia-start-height=4180975
- # - --redis-client=redis://redis:6379
- # - --operation-namespace-id=000000000000000000000000000000000000707269736d6f70657261
- # - --snark-namespace-id=000000000000000000000000000000000000707269736d736f61726b
+ command:
+ - prover
+ - --celestia-client=ws://da-bridge:26658
+ - --celestia-start-height=${CELESTIA_START_HEIGHT}
+ - --redis-client=redis://${REDIS_USER}:${REDIS_PASSWORD}@redis:6379
+ - --operation-namespace-id=${OPERATION_NAMESPACE_ID}
+ - --snark-namespace-id=${SNARK_NAMESPACE_ID}📝 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.
| image: ghcr.io/deltadevsde/prism:pr-205 | |
| volumes: | |
| - ./data/prism-prover:/root/.prism | |
| restart: always | |
| entrypoint: ["sleep", "infinity"] | |
| # command: | |
| # - prover | |
| # - --celestia-client=ws://da-bridge:26658 | |
| # - --celestia-start-height=4180975 | |
| # - --redis-client=redis://redis:6379 | |
| # - --operation-namespace-id=000000000000000000000000000000000000707269736d6f70657261 | |
| # - --snark-namespace-id=000000000000000000000000000000000000707269736d736f61726b | |
| prism-full: | |
| image: ghcr.io/deltadevsde/prism:${PRISM_VERSION} | |
| volumes: | |
| - ./data/prism-prover:/root/.prism | |
| restart: always | |
| command: | |
| - prover | |
| - --celestia-client=ws://da-bridge:26658 | |
| - --celestia-start-height=${CELESTIA_START_HEIGHT} | |
| - --redis-client=redis://${REDIS_USER}:${REDIS_PASSWORD}@redis:6379 | |
| - --operation-namespace-id=${OPERATION_NAMESPACE_ID} | |
| - --snark-namespace-id=${SNARK_NAMESPACE_ID} | |
| prism-full: |
| image: redis:latest | ||
| restart: always | ||
| volumes: | ||
| - ./data/redis:/data | ||
| command: ["redis-server", "--save", "60", "1", "--loglevel", "warning"] |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance Redis security configuration.
The Redis service is using the latest tag and lacks security configurations:
- Pin the Redis version instead of using
latest - Add
--requirepassfor authentication - Consider adding memory limits
redis:
- image: redis:latest
+ image: redis:7.2.3
restart: always
volumes:
- ./data/redis:/data
- command: ["redis-server", "--save", "60", "1", "--loglevel", "warning"]
+ command: ["redis-server", "--save", "60", "1", "--loglevel", "warning", "--requirepass", "${REDIS_PASSWORD}", "--maxmemory", "2gb", "--maxmemory-policy", "allkeys-lru"]Committable suggestion skipped: line range outside the PR's diff.
| - --core.ip=consensus-full-mocha-4.celestia-mocha.com # TODO: point to local consensus | ||
| - --p2p.network=mocha-4 | ||
| - --archival | ||
| - --rpc.skip-auth # TODO: remove |
There was a problem hiding this comment.
💡 Codebase verification
Security: Remove or properly secure RPC authentication flags
The security concern is valid. Found multiple instances where RPC authentication is potentially bypassed:
- Hardcoded
--rpc.skip-authflags with TODO comments inci/docker-compose copy.yml - Environment variable controlled
$SKIP_AUTHin node running scripts
These authentication bypasses should be removed or replaced with proper authentication mechanisms before deployment.
🔗 Analysis chain
Security: Remove TODO comments about skipping authentication.
The --rpc.skip-auth flags are marked as TODO for removal. This should be addressed before deployment as it could pose security risks.
Also applies to: 38-38
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if skip-auth is used in other configuration files
rg -l "skip-auth" .
Length of output: 89
Script:
#!/bin/bash
# Check the context of skip-auth usage in all files
echo "=== docker-compose copy.yml ==="
rg "skip-auth" "./ci/docker-compose copy.yml" -C 3
echo -e "\n=== run-lightnode.sh ==="
rg "skip-auth" "./ci/run-lightnode.sh" -C 3
echo -e "\n=== run-bridge.sh ==="
rg "skip-auth" "./ci/run-bridge.sh" -C 3
Length of output: 1285
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
crates/cli/src/main.rs (3)
79-87: Consider refactoring keystore initialization to avoid code duplicationThe code for initializing the
keystoreis duplicated in both theProverandFullNodecommand handlers. Extracting this logic into a helper function or method can improve maintainability and reduce code redundancy.
89-99: Remove commented-out code to enhance code clarityThe code from lines 89 to 99 is commented out. Removing this dead code can help keep the codebase clean and make it easier for others to read and maintain.
126-127: Review the necessity of logging the verifying keyLogging the verifying key with
info!("verifying key: {:?}", verifying_key);may expose unnecessary details. While verifying keys are public, consider whether logging them is needed, especially in production environments, to avoid cluttering logs with sensitive information.Cargo.toml (1)
84-85: Review exact version pinning ofgeneric-arrayThe dependency
generic-arrayis pinned to an exact version=1.1.1. Strict version pinning can prevent receiving important updates or patches. Consider using a caret requirement (^1.1.1) to allow for compatible updates.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (2)
Cargo.toml(1 hunks)crates/cli/src/main.rs(6 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: integration-test
- GitHub Check: unused dependencies
- GitHub Check: clippy
- GitHub Check: unit-test
- GitHub Check: build-and-push-image
| @@ -73,14 +73,16 @@ thiserror = "1.0.62" | |||
| anyhow = "1.0.44" | |||
|
|
|||
| # cli | |||
| keystore-rs = "0.1.2" | |||
| keystore-rs = { git = "https://github.com/deltadevsde/keystore.git", branch = "fix-filebackend" } | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Specify a commit hash for the keystore-rs Git dependency
Using a branch (branch = "fix-filebackend") for the keystore-rs dependency can lead to non-reproducible builds if the branch updates. To ensure consistent and reproducible builds, specify a commit hash instead.
Signed-off-by: Smuu <18609909+Smuu@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
ci/docker-compose copy.yml (2)
32-32:⚠️ Potential issueCritical: Remove RPC authentication bypass.
The
--rpc.skip-authflags are currently bypassing authentication, which is a security risk.Also applies to: 47-47
51-55: 🛠️ Refactor suggestionEnhance Redis security configuration.
The Redis configuration lacks security measures and uses the latest tag.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (5)
ci/docker-compose copy.yml(1 hunks)crates/cli/Cargo.toml(1 hunks)crates/cli/src/main.rs(8 hunks)crates/keys/Cargo.toml(1 hunks)crates/keys/src/verifying_keys.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/cli/Cargo.toml
🧰 Additional context used
🪛 Gitleaks (8.21.2)
ci/docker-compose copy.yml
63-63: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: unused dependencies
- GitHub Check: unit-test
- GitHub Check: integration-test
- GitHub Check: clippy
- GitHub Check: build-and-push-image
🔇 Additional comments (7)
crates/keys/src/verifying_keys.rs (1)
253-257: LGTM! Clean implementation of base64 encoding.The implementation is well-structured:
- ToBase64 trait implementation leverages the existing to_bytes() method
- Display trait modification maintains consistency by using the new to_base64() method
Also applies to: 261-261
crates/keys/Cargo.toml (1)
13-13: LGTM! Appropriate dependency addition.The base64 workspace dependency addition aligns with the new ToBase64 implementation.
ci/docker-compose copy.yml (2)
1-108: Create environment variable documentation.Create a
.env.examplefile to document all required environment variables.🧰 Tools
🪛 Gitleaks (8.21.2)
63-63: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
57-72: 🛠️ Refactor suggestionImprove prism-prover service configuration.
Several issues need to be addressed in the prism-prover service configuration:
- Using PR-specific image tag
- Hardcoded values that should be environment variables
Apply these changes:
prism-prover: - image: ghcr.io/deltadevsde/prism:pr-205 + image: ghcr.io/deltadevsde/prism:${PRISM_VERSION} environment: - - SYMMETRIC_KEY=3c4a987867c9fa3cf8f5309b236cde63100e9fcfbe8ac453a2eeddd0b3251a08 + - SYMMETRIC_KEY=${SYMMETRIC_KEY} command: - prover - --celestia-client=ws://da-bridge:26658 - - --celestia-start-height=4180975 + - --celestia-start-height=${CELESTIA_START_HEIGHT} - --keystore-type=file - - --redis-client=redis://redis:6379 + - --redis-client=redis://${REDIS_USER}:${REDIS_PASSWORD}@redis:6379 - - --operation-namespace-id=000000000000000000000000000000000000707269736d5350456f70 - - --snark-namespace-id=000000000000000000000000000000000000707269736d535045736e + - --operation-namespace-id=${OPERATION_NAMESPACE_ID} + - --snark-namespace-id=${SNARK_NAMESPACE_ID}Likely invalid or redundant comment.
🧰 Tools
🪛 Gitleaks (8.21.2)
63-63: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
crates/cli/src/main.rs (3)
80-88: LGTM! Well-structured keystore selection.The dynamic keystore selection with proper error handling is a good improvement.
49-53: LGTM! Improved verifying key handling.The verifying key handling now uses base64 consistently with proper error handling.
Also applies to: 183-187
90-95: LGTM! Enhanced error handling.The error messages are now more descriptive and helpful for debugging.
| restart: always | ||
| # entrypoint: ["sleep", "infinity"] | ||
| environment: | ||
| - SYMMETRIC_KEY=3c4a987867c9fa3cf8f5309b236cde63100e9fcfbe8ac453a2eeddd0b3251a08 |
There was a problem hiding this comment.
Critical: Remove hardcoded symmetric key.
The symmetric key should not be hardcoded in the configuration file as it poses a security risk.
Move the symmetric key to an environment variable:
environment:
- - SYMMETRIC_KEY=3c4a987867c9fa3cf8f5309b236cde63100e9fcfbe8ac453a2eeddd0b3251a08
+ - SYMMETRIC_KEY=${SYMMETRIC_KEY}📝 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.
| - SYMMETRIC_KEY=3c4a987867c9fa3cf8f5309b236cde63100e9fcfbe8ac453a2eeddd0b3251a08 | |
| - SYMMETRIC_KEY=${SYMMETRIC_KEY} |
🧰 Tools
🪛 Gitleaks (8.21.2)
63-63: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
Signed-off-by: Smuu <18609909+Smuu@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
crates/cli/src/cfg.rs (1)
119-120:⚠️ Potential issueKeystore path needs proper expansion and validation.
The keystore path uses "~" without proper expansion. The commented-out implementation using
dirs::home_dir()would be more robust.This is a duplicate of a previous review comment. The keystore path still needs proper expansion and validation.
Also applies to: 132-133
🧹 Nitpick comments (4)
ci/docker-compose copy.yml (1)
80-81: Restrict exposed ports to localhost.The webserver ports should be restricted to localhost if not needed externally.
ports: - - 41997:41997 + - 127.0.0.1:41997:41997Also applies to: 106-108
crates/node_types/prover/src/webserver.rs (2)
28-28: LGTM! Consider documenting the prefix field.The
prefixfield addition is a good way to support flexible URL routing.Consider adding documentation to explain the purpose and format of the prefix field:
+ /// URL prefix for all routes. For example, "/api/v1" would make routes accessible at "/api/v1/transaction" etc. pub prefix: String,
123-126: Consider using a URL path builder for route construction.The current string formatting approach for route construction could be improved using a proper URL path builder to handle edge cases (e.g., multiple slashes, proper escaping).
Consider using the
path-cleancrate or implementing a helper function:fn build_route(prefix: &str, path: &str) -> String { let parts: Vec<_> = [prefix, path] .iter() .flat_map(|s| s.split('/')) .filter(|s| !s.is_empty()) .collect(); format!("/{}", parts.join("/")) }crates/cli/src/cfg.rs (1)
48-58: Document keystore type options and path format.The keystore configuration looks good, but could benefit from more detailed documentation.
Consider enhancing the documentation:
/// The type of keystore to use. /// - /// Can be one of: `keychain`, `file`. + /// Can be one of: + /// - `keychain`: Use the system's secure keychain (recommended for production) + /// - `file`: Store keys in a JSON file (suitable for development) #[arg(long, default_value = "keychain")] keystore_type: String, /// The path to the keystore. /// - /// This is only used if the keystore type is `file`. + /// This is only used if the keystore type is `file`. + /// The path supports shell expansion (e.g., "~" for home directory). + /// Default: "~/.prism/keystore.json" #[arg(long, default_value = "~/.prism/keystore.json")] keystore_path: String,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
ci/docker-compose copy.yml(1 hunks)crates/cli/src/cfg.rs(7 hunks)crates/node_types/prover/src/webserver.rs(3 hunks)
🧰 Additional context used
🪛 Gitleaks (8.21.2)
ci/docker-compose copy.yml
63-63: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: unit-test
- GitHub Check: integration-test
- GitHub Check: clippy
- GitHub Check: unused dependencies
- GitHub Check: build-and-push-image
🔇 Additional comments (6)
ci/docker-compose copy.yml (4)
50-55: Enhance Redis security configuration.The Redis service lacks essential security configurations as highlighted in previous reviews.
32-32: Security: Remove or properly secure RPC authentication flags.The
--rpc.skip-authflags pose security risks as highlighted in previous reviews.Also applies to: 47-47
63-63: Critical: Remove hardcoded symmetric key.The symmetric key should be moved to an environment variable as highlighted in previous reviews.
🧰 Tools
🪛 Gitleaks (8.21.2)
63-63: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
29-29: Update TODO: Replace remote consensus with local configuration.Both da-bridge and da-light services are configured to use a remote consensus node. As per TODO comments, these should point to the local consensus service.
- - --core.ip=consensus-full-mocha-4.celestia-mocha.com # TODO: point to local consensus + - --core.ip=consensus-full # Use local service nameAlso applies to: 45-45
✅ Verification successful
Confirmed: Update consensus endpoint to use local service
The suggested change is correct and safe to implement. The local consensus service is properly configured, and other services in the stack already use Docker's service discovery pattern (e.g., redis://redis:6379). The change won't impact dependent services as they connect through da-bridge/da-light endpoints.
🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash # Check the docker-compose file content cat "ci/docker-compose copy.yml" # Look for similar files and configurations fd docker-compose --type f --exec cat {}Length of output: 10493
crates/cli/src/cfg.rs (2)
249-250: 🛠️ Refactor suggestionUse helper functions for keystore configuration.
The direct assignment of keystore values bypasses validation.
Use the helper functions in
apply_command_line_args:- keystore_type: args.keystore_type, - keystore_path: args.keystore_path, + keystore_type: get_keystore_type(&args)?, + keystore_path: get_keystore_path(&args)?,Likely invalid or redundant comment.
183-199: 🛠️ Refactor suggestionRestore and improve the commented keystore helper functions.
The commented-out functions provide better path handling using
home_dir(). They should be restored and improved.Restore and enhance the helper functions:
-// fn get_keystore_type(args: &CommandArgs) -> Result<String> { -// args.keystore_type -// .clone() -// .or_else(|| Some("keychain".to_string())) -// .ok_or_else(|| { -// GeneralError::MissingArgumentError("could not determine keystore type".to_string()).into() -// }) -// } - -// fn get_keystore_path(args: &CommandArgs) -> Result<String> { -// args.keystore_path -// .clone() -// .or_else(|| home_dir().map(|path| format!("{}/.prism/keystore.json", path.to_string_lossy()))) -// .ok_or_else(|| { -// GeneralError::MissingArgumentError("could not determine keystore path".to_string()).into() -// }) -// } +fn get_keystore_type(args: &CommandArgs) -> Result<String> { + let keystore_type = args.keystore_type.clone(); + match keystore_type.as_str() { + "keychain" | "file" => Ok(keystore_type), + _ => Err(GeneralError::InvalidArgumentError(format!( + "invalid keystore type: {}. Expected 'keychain' or 'file'", + keystore_type + )).into()) + } +} + +fn get_keystore_path(args: &CommandArgs) -> Result<String> { + let raw_path = args.keystore_path.clone(); + + // Expand ~ to home directory + let expanded_path = if raw_path.starts_with('~') { + let home = home_dir().ok_or_else(|| { + GeneralError::MissingArgumentError("could not determine home directory".to_string()) + })?; + raw_path.replacen('~', &home.to_string_lossy(), 1) + } else { + raw_path + }; + + // Ensure parent directory exists + if let Some(parent) = Path::new(&expanded_path).parent() { + fs::create_dir_all(parent).context("Failed to create keystore directory")?; + } + + Ok(expanded_path) +}Likely invalid or redundant comment.
| - ./data/redis:/data | ||
| command: ["redis-server", "--save", "60", "1", "--loglevel", "warning"] | ||
| prism-prover: | ||
| image: ghcr.io/deltadevsde/prism:pr-205 |
There was a problem hiding this comment.
Replace PR-specific image tags with versioned tags.
Using PR-specific image tags (pr-205) isn't sustainable for production deployment.
- image: ghcr.io/deltadevsde/prism:pr-205
+ image: ghcr.io/deltadevsde/prism:${PRISM_VERSION}Also applies to: 76-76, 95-95
| command: | ||
| - prover | ||
| - --celestia-client=ws://da-bridge:26658 | ||
| - --celestia-start-height=4180975 |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Move configuration values to environment variables.
Several configuration values should be externalized to environment variables:
- Celestia start height
- Namespace IDs
- Verifying keys
- Redis connection details
- --celestia-client=ws://da-bridge:26658
- - --celestia-start-height=4180975
+ - --celestia-start-height=${CELESTIA_START_HEIGHT}
- --keystore-type=file
- - --redis-client=redis://redis:6379
+ - --redis-client=redis://${REDIS_USER}:${REDIS_PASSWORD}@redis:6379
- - --operation-namespace-id=000000000000000000000000000000000000707269736d5350456f70
- - --snark-namespace-id=000000000000000000000000000000000000707269736d535045736e
+ - --operation-namespace-id=${OPERATION_NAMESPACE_ID}
+ - --snark-namespace-id=${SNARK_NAMESPACE_ID}Would you like me to generate a .env.example file with all the required variables and documentation?
Also applies to: 85-85, 102-102, 69-71, 89-90, 103-105
| docs-redirect: | ||
| image: nginx:alpine | ||
| volumes: | ||
| - ./data/docs-redirect/default.conf:/etc/nginx/conf.d/default.conf | ||
| ports: | ||
| - 80:80 | ||
| restart: always |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Pin nginx version and consider adding TLS.
The docs-redirect service has potential security concerns:
- Using latest alpine tag which could lead to unexpected updates
- Exposing port 80 without TLS
docs-redirect:
- image: nginx:alpine
+ image: nginx:1.25.3-alpine
volumes:
- ./data/docs-redirect/default.conf:/etc/nginx/conf.d/default.conf
ports:
- - 80:80
+ - 127.0.0.1:80:80 # Restrict to localhost if possible
restart: always📝 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.
| docs-redirect: | |
| image: nginx:alpine | |
| volumes: | |
| - ./data/docs-redirect/default.conf:/etc/nginx/conf.d/default.conf | |
| ports: | |
| - 80:80 | |
| restart: always | |
| docs-redirect: | |
| image: nginx:1.25.3-alpine | |
| volumes: | |
| - ./data/docs-redirect/default.conf:/etc/nginx/conf.d/default.conf | |
| ports: | |
| - 127.0.0.1:80:80 # Restrict to localhost if possible | |
| restart: always |
crates/cli/src/cfg.rs
Outdated
| #[arg(short, long, requires = "webserver_active", default_value = "41997")] | ||
| port: Option<u16>, | ||
|
|
||
| /// Prefix for the webserver to listen on | ||
| #[arg(long, requires = "webserver_active")] | ||
| prefix: Option<String>, |
There was a problem hiding this comment.
💡 Codebase verification
Document the reason for port 41997 or mark it as arbitrary
The port number 41997 lacks documentation explaining its selection. While it's consistently used across the codebase and falls within the safe dynamic port range, its specific choice appears arbitrary. Add a doc comment explaining the rationale for this port number, or explicitly note that it's an arbitrary choice to avoid confusion.
🔗 Analysis chain
Document the reason for the specific default port.
The default port change to 41997 seems arbitrary.
Please document why this specific port was chosen and verify it doesn't conflict with other services:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if the port number is documented or used elsewhere
rg -g '!target' "41997|port.*=.*41997"
Length of output: 333
Script:
#!/bin/bash
# Search for documentation about the port in markdown files and comments
echo "=== Searching markdown files ==="
rg -g "*.md" "41997|port" -A 2 -B 2
echo -e "\n=== Searching for doc comments near port configuration ==="
rg -g "*.rs" -B 5 "41997"
echo -e "\n=== Checking commit messages ==="
git log -n 5 --grep="41997\|port" --pretty=format:"%h %s"
Length of output: 18520
Signed-off-by: Smuu <18609909+Smuu@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
crates/cli/src/cfg.rs (1)
54-58:⚠️ Potential issueKeystore path needs proper expansion and validation.
The hardcoded "
/.prism/keystore.json" path needs proper expansion of the "" character. Consider:
- Using
dirs::home_dir()for path expansion- Adding path validation
- Ensuring the parent directory exists
- #[arg(long, default_value = "~/.prism/keystore.json")] - keystore_path: String, + #[arg(long)] + keystore_path: Option<String>,
🧹 Nitpick comments (1)
crates/cli/src/cfg.rs (1)
115-116: Add documentation and serde attributes to Config fields.The new keystore fields should be documented and have appropriate serde attributes for consistency with other fields.
+ /// The type of keystore to use for managing signing keys. + #[serde(default = "default_keystore_type")] pub keystore_type: String, + /// The path to the keystore file when using file-based keystore. + #[serde(default = "default_keystore_path")] pub keystore_path: String,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
crates/cli/src/cfg.rs(6 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: unit-test
- GitHub Check: integration-test
- GitHub Check: clippy
- GitHub Check: build-and-push-image
- GitHub Check: unused dependencies
🔇 Additional comments (2)
crates/cli/src/cfg.rs (2)
105-105: Document the reason for port 41997 or mark it as arbitrary.Add a doc comment explaining the rationale for choosing port 41997, or explicitly note that it's an arbitrary choice within the safe dynamic port range.
+ /// Port number for the webserver to listen on. + /// Port 41997 was chosen as an arbitrary port within the dynamic/private port range (49152-65535) + /// to avoid conflicts with common services. #[arg(short, long, requires = "webserver_active", default_value = "41997")]
244-245:⚠️ Potential issueAdd validation and path handling in config application.
The direct assignment of keystore values needs validation and proper path handling:
- Validate keystore type against allowed values
- Expand and normalize keystore path
- Ensure parent directories exist
- keystore_type: args.keystore_type, - keystore_path: args.keystore_path, + keystore_type: validate_keystore_type(args.keystore_type)?, + keystore_path: expand_and_validate_keystore_path(args.keystore_path)?,Likely invalid or redundant comment.
| // fn get_keystore_type(args: &CommandArgs) -> Result<String> { | ||
| // args.keystore_type | ||
| // .clone() | ||
| // .or_else(|| Some("keychain".to_string())) | ||
| // .ok_or_else(|| { | ||
| // GeneralError::MissingArgumentError("could not determine keystore type".to_string()).into() | ||
| // }) | ||
| // } | ||
|
|
||
| // fn get_keystore_path(args: &CommandArgs) -> Result<String> { | ||
| // args.keystore_path | ||
| // .clone() | ||
| // .or_else(|| home_dir().map(|path| format!("{}/.prism/keystore.json", path.to_string_lossy()))) | ||
| // .ok_or_else(|| { | ||
| // GeneralError::MissingArgumentError("could not determine keystore path".to_string()).into() | ||
| // }) | ||
| // } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Restore and improve the commented out keystore functions.
The commented out functions provide better path handling using home_dir(). Consider:
- Restoring these functions
- Enhancing them with proper error handling
- Adding path validation
| /// The type of keystore to use. | ||
| /// | ||
| /// Can be one of: `keychain`, `file`. | ||
| #[arg(long, default_value = "keychain")] | ||
| keystore_type: String, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance keystore type validation and documentation.
The keystore type should be validated against a fixed set of allowed values. Consider:
- Using an enum instead of String to enforce valid values at compile time
- Adding runtime validation if String must be used
- Documenting all supported values in the doc comment
- /// Can be one of: `keychain`, `file`.
+ /// The type of keystore to use for managing signing keys.
+ ///
+ /// Supported values:
+ /// - `keychain`: Use the system's secure keychain (recommended for production)
+ /// - `file`: Use a JSON file-based keystore (recommended for development)
#[arg(long, default_value = "keychain")]
- keystore_type: String,
+ keystore_type: KeystoreType,
+
+#[derive(Args, Deserialize, Clone, Debug)]
+pub enum KeystoreType {
+ #[serde(rename = "keychain")]
+ Keychain,
+ #[serde(rename = "file")]
+ File,
+}Committable suggestion skipped: line range outside the PR's diff.
This PR builds a container with the mock prover for the devnet
Summary by CodeRabbit
Release Notes
New Features
Dependencies
Infrastructure
Performance