Voyager verification support - #3287
Conversation
Adds support for verifying contracts after they are migrated, allowing for increased confidence in the deployed code. This includes fetching source files, interacting with a verification API, and displaying results to the user.
Enables users to verify deployed contracts using services like Voyager. Adds flags to specify the verification service and a custom API URL. Supports watching the verification progress until completion.
Adds support for verifying deployed contracts via external services. This change introduces contract verification functionality by adding necessary dependencies and implementing the verification logic. It allows users to verify their deployed contracts using services like Voyager. A separate function `create_verification_config` is created to handle constructing verification configurations based on user-provided service options. The `verify` and `verify_url` parameters from `MigrateArgs` are passed into this function to generate a service specific configuration.
Improves the display of contract verification results by adding color-coding to indicate success, failure, or warnings. It also ensures that verification failures do not halt the migration process and displays a warning message, allowing users to continue even if some contracts fail verification.
Moves the verification configuration logic into a dedicated `VerifyOptions` struct. This change promotes modularity and improves the organization of the code by encapsulating verification-related options and their associated logic within a separate module.
Refactors contract verification to use the manifest file for discovering contracts, models, and events. This change eliminates the need to manually collect declared classes and allows the verifier to automatically verify all deployed artifacts. It also introduces project root discovery by searching upwards for manifest files and includes all project source files for accurate verification.
Implements contract verification after deployment using a 3rd party API. Introduces a new module for interacting with the contract verification service, allowing automated verification of deployed contracts. Includes retry logic with exponential backoff and a circuit breaker to handle potential API issues. Adds new configuration options to control the verification process, such as timeouts and retry attempts. Improves source file collection logic to better support various project layouts and ensure all necessary files are included.
WalkthroughOhayo sensei! This PR integrates contract verification into the Dojo migration workflow by adding verification options to the migration command, embedding verification logic in the migration process, and introducing the new Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI (sozo)
participant Migration
participant ContractVerifier
participant VerificationClient
participant Verification API
User->>CLI (sozo): Run migrate command with --verify
CLI (sozo)->>Migration: Start migration with verification config
Migration->>Migration: Perform migration steps
alt Verification enabled
Migration->>ContractVerifier: verify_deployed_contracts()
ContractVerifier->>VerificationClient: verify_contract (for each contract)
VerificationClient->>Verification API: POST /verify
Verification API-->>VerificationClient: job_id
loop Polling
ContractVerifier->>VerificationClient: check_verification_status(job_id)
VerificationClient->>Verification API: GET /status/{job_id}
Verification API-->>VerificationClient: status
alt Success/Failure/Timeout
ContractVerifier->>Migration: Return verification result
end
end
end
Migration->>CLI (sozo): Return migration and verification results
CLI (sozo)->>User: Display migration and verification summary
Estimated code review effort5 (~2+ hours) Possibly related PRs
Suggested reviewers
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (7)
bin/sozo/src/commands/options/verify.rs (2)
28-30: Consider making service URLs configurable, sensei.The hardcoded service URLs could be made configurable through environment variables for better flexibility in different deployment environments.
- "voyager" => Url::parse("https://api.voyager.online/beta")?, - "voyager-sepolia" => Url::parse("https://sepolia-api.voyager.online/beta")?, - "voyager-dev" => Url::parse("https://dev-api.voyager.online/beta")?, + "voyager" => Url::parse( + &std::env::var("VOYAGER_API_URL").unwrap_or_else(|_| "https://api.voyager.online/beta".to_string()) + )?, + "voyager-sepolia" => Url::parse( + &std::env::var("VOYAGER_SEPOLIA_API_URL").unwrap_or_else(|_| "https://sepolia-api.voyager.online/beta".to_string()) + )?, + "voyager-dev" => Url::parse( + &std::env::var("VOYAGER_DEV_API_URL").unwrap_or_else(|_| "https://dev-api.voyager.online/beta".to_string()) + )?,
47-54: Consider making timeout values configurable, sensei.The hardcoded timeout values might not be suitable for all network conditions or project sizes. With max_attempts=30 and exponential backoff, the total wait time could be excessive.
Consider adding CLI options for these values or using environment variables:
Ok(Some(VerificationConfig { api_url, watch: self.verify_watch, include_tests: true, // Default to including tests for Dojo projects - timeout: 300, // 5 minutes default timeout - verification_timeout: 1800, // 30 minutes total for verification - max_attempts: 30, // Maximum retry attempts + timeout: std::env::var("VERIFY_TIMEOUT").ok() + .and_then(|v| v.parse().ok()).unwrap_or(300), + verification_timeout: std::env::var("VERIFY_TOTAL_TIMEOUT").ok() + .and_then(|v| v.parse().ok()).unwrap_or(1800), + max_attempts: std::env::var("VERIFY_MAX_ATTEMPTS").ok() + .and_then(|v| v.parse().ok()).unwrap_or(30), }))crates/sozo/voyager/src/verifier.rs (1)
34-50: Ohayo! The jitter implementation could be improved, sensei.While the current implementation works, using system time for randomness isn't ideal for cryptographic or distributed systems use cases.
Consider using a proper random number generator:
+use rand::Rng; + fn add_jitter(&self, duration: Duration) -> Duration { - // Use a simple linear congruential generator for jitter - // This avoids needing external random dependencies - let seed = SystemTime::now() - .duration_since(UNIX_EPOCH) - .unwrap_or(Duration::from_secs(0)) - .as_nanos() as u64; - - let jitter_ms = seed % 1000; // 0-999ms jitter + let mut rng = rand::thread_rng(); let base_ms = duration.as_millis() as u64; // Add ±25% jitter let jitter_range = base_ms / 4; // 25% of base duration - let actual_jitter = (jitter_ms % (jitter_range * 2)).saturating_sub(jitter_range); + let jitter = rng.gen_range(0..jitter_range * 2); + let actual_jitter = jitter.saturating_sub(jitter_range); Duration::from_millis(base_ms.saturating_add(actual_jitter)) }crates/sozo/voyager/src/client.rs (1)
26-34: Consider making circuit breaker parameters configurableOhayo sensei! The circuit breaker implementation uses hard-coded values for
failure_thresholdandrecovery_timeout. Consider making these configurable throughVerificationConfigto allow different environments to tune these parameters based on their needs.- fn new() -> Self { + fn new(failure_threshold: u32, recovery_timeout: Duration) -> Self { Self { failure_count: 0, last_failure_time: None, - failure_threshold: 5, // Open circuit after 5 consecutive failures - recovery_timeout: Duration::from_secs(60), // Wait 1 minute before retry + failure_threshold, + recovery_timeout, } }crates/sozo/ops/src/migrate/mod.rs (1)
136-149: Consider making verification failure behavior configurableSensei, while it's good that verification failures don't block migration, some users might want stricter behavior. Consider adding a configuration option to fail the migration on verification errors.
pub struct VerificationConfig { // ... existing fields ... + /// Whether to fail migration on verification errors + pub fail_on_error: bool, }crates/sozo/voyager/src/analyzer.rs (2)
24-50: Inconsistent error handling between similar methodsOhayo sensei! The
extract_dojo_versionreturnsOptionwhileextract_package_namereturnsResult, but both perform similar file operations. Consider making them consistent by having both returnResultfor better error propagation.- pub fn extract_dojo_version(&self) -> Option<String> { + pub fn extract_dojo_version(&self) -> Result<Option<String>> { let scarb_toml_path = self.project_root.join("Scarb.toml"); - let contents = fs::read_to_string(&scarb_toml_path).ok()?; - let parsed: toml::Value = toml::from_str(&contents).ok()?; + let contents = fs::read_to_string(&scarb_toml_path) + .map_err(|e| anyhow!("Failed to read Scarb.toml: {}", e))?; + let parsed: toml::Value = toml::from_str(&contents) + .map_err(|e| anyhow!("Failed to parse Scarb.toml: {}", e))?; // Look for dependencies.dojo.tag - parsed.get("dependencies")?.get("dojo")?.get("tag")?.as_str().map(|s| s.to_string()) + Ok(parsed.get("dependencies")?.get("dojo")?.get("tag")?.as_str().map(|s| s.to_string())) }
589-644: Add debug logging to track which file discovery strategy succeededSensei, the multiple fallback strategies are good, but it would be helpful to log which strategy successfully found the file for debugging purposes.
if self.file_contains_definition(&content, contract_name) { + debug!("Found contract definition in file: {}", file.name); return Ok(file.name.clone()); }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (14)
Cargo.toml(7 hunks)bin/sozo/src/commands/migrate.rs(4 hunks)bin/sozo/src/commands/options/mod.rs(1 hunks)bin/sozo/src/commands/options/verify.rs(1 hunks)crates/sozo/ops/Cargo.toml(1 hunks)crates/sozo/ops/src/migrate/mod.rs(6 hunks)crates/sozo/ops/src/migration_ui.rs(2 hunks)crates/sozo/ops/src/tests/migration.rs(2 hunks)crates/sozo/voyager/Cargo.toml(1 hunks)crates/sozo/voyager/src/analyzer.rs(1 hunks)crates/sozo/voyager/src/client.rs(1 hunks)crates/sozo/voyager/src/config.rs(1 hunks)crates/sozo/voyager/src/lib.rs(1 hunks)crates/sozo/voyager/src/verifier.rs(1 hunks)
🧠 Learnings (9)
📓 Common learnings
Learnt from: glihm
PR: dojoengine/dojo#2633
File: crates/dojo/world/src/contracts/abigen/world.rs:3809-0
Timestamp: 2024-11-05T04:30:12.852Z
Learning: Code in `crates/dojo/world/src/contracts/abigen/` is auto-generated and should be excluded from code reviews.
Learnt from: glihm
PR: dojoengine/dojo#2650
File: crates/dojo/utils/src/tx/mod.rs:201-272
Timestamp: 2024-11-07T14:43:23.530Z
Learning: In `crates/dojo/utils/src/tx/mod.rs`, fee token configurations are verified when the `fee_config` is built, making additional validations in the `send_with_cfg` methods redundant.
crates/sozo/ops/src/tests/migration.rs (1)
Learnt from: glihm
PR: #2650
File: bin/sozo/src/commands/migrate.rs:64-65
Timestamp: 2024-11-07T13:57:57.616Z
Learning: In the codebase, the use of into() and try_into() for converting TransactionOptions is intentional. Conversions that may fail use try_into(), while those that cannot fail use into(). This approach is appropriate and should be preserved.
Examples:
try_into()is used inbin/sozo/src/commands/migrate.rsandbin/sozo/src/commands/execute.rswhere conversions can fail.into()is used inbin/sozo/src/commands/register.rsandbin/sozo/src/commands/auth.rswhere conversions are infallible.
crates/sozo/voyager/src/lib.rs (2)
Learnt from: glihm
PR: #2633
File: crates/dojo/world/src/contracts/abigen/world.rs:3809-0
Timestamp: 2024-11-05T04:30:12.852Z
Learning: Code in crates/dojo/world/src/contracts/abigen/ is auto-generated and should be excluded from code reviews.
Learnt from: glihm
PR: #2650
File: crates/dojo/utils/src/tx/mod.rs:201-272
Timestamp: 2024-11-07T14:43:23.530Z
Learning: In crates/dojo/utils/src/tx/mod.rs, fee token configurations are verified when the fee_config is built, making additional validations in the send_with_cfg methods redundant.
bin/sozo/src/commands/options/verify.rs (1)
Learnt from: glihm
PR: #2650
File: bin/sozo/src/commands/migrate.rs:64-65
Timestamp: 2024-11-07T13:57:57.616Z
Learning: In the codebase, the use of into() and try_into() for converting TransactionOptions is intentional. Conversions that may fail use try_into(), while those that cannot fail use into(). This approach is appropriate and should be preserved.
Examples:
try_into()is used inbin/sozo/src/commands/migrate.rsandbin/sozo/src/commands/execute.rswhere conversions can fail.into()is used inbin/sozo/src/commands/register.rsandbin/sozo/src/commands/auth.rswhere conversions are infallible.
bin/sozo/src/commands/migrate.rs (1)
Learnt from: glihm
PR: #2650
File: bin/sozo/src/commands/migrate.rs:64-65
Timestamp: 2024-11-07T13:57:57.616Z
Learning: In the codebase, the use of into() and try_into() for converting TransactionOptions is intentional. Conversions that may fail use try_into(), while those that cannot fail use into(). This approach is appropriate and should be preserved.
Examples:
try_into()is used inbin/sozo/src/commands/migrate.rsandbin/sozo/src/commands/execute.rswhere conversions can fail.into()is used inbin/sozo/src/commands/register.rsandbin/sozo/src/commands/auth.rswhere conversions are infallible.
Cargo.toml (2)
Learnt from: glihm
PR: #2650
File: bin/sozo/src/commands/migrate.rs:64-65
Timestamp: 2024-11-07T13:57:57.616Z
Learning: In the codebase, the use of into() and try_into() for converting TransactionOptions is intentional. Conversions that may fail use try_into(), while those that cannot fail use into(). This approach is appropriate and should be preserved.
Examples:
try_into()is used inbin/sozo/src/commands/migrate.rsandbin/sozo/src/commands/execute.rswhere conversions can fail.into()is used inbin/sozo/src/commands/register.rsandbin/sozo/src/commands/auth.rswhere conversions are infallible.
Learnt from: glihm
PR: #2633
File: crates/dojo/world/src/contracts/abigen/world.rs:3809-0
Timestamp: 2024-11-05T04:30:12.852Z
Learning: Code in crates/dojo/world/src/contracts/abigen/ is auto-generated and should be excluded from code reviews.
crates/sozo/voyager/src/analyzer.rs (1)
Learnt from: glihm
PR: #2633
File: crates/dojo/world/src/contracts/abigen/world.rs:3809-0
Timestamp: 2024-11-05T04:30:12.852Z
Learning: Code in crates/dojo/world/src/contracts/abigen/ is auto-generated and should be excluded from code reviews.
crates/sozo/ops/src/migrate/mod.rs (1)
Learnt from: glihm
PR: #2650
File: crates/dojo/utils/src/tx/mod.rs:201-272
Timestamp: 2024-11-07T14:43:23.530Z
Learning: In crates/dojo/utils/src/tx/mod.rs, fee token configurations are verified when the fee_config is built, making additional validations in the send_with_cfg methods redundant.
crates/sozo/voyager/src/config.rs (1)
Learnt from: glihm
PR: #2650
File: crates/dojo/utils/src/tx/mod.rs:201-272
Timestamp: 2024-11-07T14:43:23.530Z
Learning: In crates/dojo/utils/src/tx/mod.rs, fee token configurations are verified when the fee_config is built, making additional validations in the send_with_cfg methods redundant.
🧰 Additional context used
🧠 Learnings (9)
📓 Common learnings
Learnt from: glihm
PR: dojoengine/dojo#2633
File: crates/dojo/world/src/contracts/abigen/world.rs:3809-0
Timestamp: 2024-11-05T04:30:12.852Z
Learning: Code in `crates/dojo/world/src/contracts/abigen/` is auto-generated and should be excluded from code reviews.
Learnt from: glihm
PR: dojoengine/dojo#2650
File: crates/dojo/utils/src/tx/mod.rs:201-272
Timestamp: 2024-11-07T14:43:23.530Z
Learning: In `crates/dojo/utils/src/tx/mod.rs`, fee token configurations are verified when the `fee_config` is built, making additional validations in the `send_with_cfg` methods redundant.
crates/sozo/ops/src/tests/migration.rs (1)
Learnt from: glihm
PR: #2650
File: bin/sozo/src/commands/migrate.rs:64-65
Timestamp: 2024-11-07T13:57:57.616Z
Learning: In the codebase, the use of into() and try_into() for converting TransactionOptions is intentional. Conversions that may fail use try_into(), while those that cannot fail use into(). This approach is appropriate and should be preserved.
Examples:
try_into()is used inbin/sozo/src/commands/migrate.rsandbin/sozo/src/commands/execute.rswhere conversions can fail.into()is used inbin/sozo/src/commands/register.rsandbin/sozo/src/commands/auth.rswhere conversions are infallible.
crates/sozo/voyager/src/lib.rs (2)
Learnt from: glihm
PR: #2633
File: crates/dojo/world/src/contracts/abigen/world.rs:3809-0
Timestamp: 2024-11-05T04:30:12.852Z
Learning: Code in crates/dojo/world/src/contracts/abigen/ is auto-generated and should be excluded from code reviews.
Learnt from: glihm
PR: #2650
File: crates/dojo/utils/src/tx/mod.rs:201-272
Timestamp: 2024-11-07T14:43:23.530Z
Learning: In crates/dojo/utils/src/tx/mod.rs, fee token configurations are verified when the fee_config is built, making additional validations in the send_with_cfg methods redundant.
bin/sozo/src/commands/options/verify.rs (1)
Learnt from: glihm
PR: #2650
File: bin/sozo/src/commands/migrate.rs:64-65
Timestamp: 2024-11-07T13:57:57.616Z
Learning: In the codebase, the use of into() and try_into() for converting TransactionOptions is intentional. Conversions that may fail use try_into(), while those that cannot fail use into(). This approach is appropriate and should be preserved.
Examples:
try_into()is used inbin/sozo/src/commands/migrate.rsandbin/sozo/src/commands/execute.rswhere conversions can fail.into()is used inbin/sozo/src/commands/register.rsandbin/sozo/src/commands/auth.rswhere conversions are infallible.
bin/sozo/src/commands/migrate.rs (1)
Learnt from: glihm
PR: #2650
File: bin/sozo/src/commands/migrate.rs:64-65
Timestamp: 2024-11-07T13:57:57.616Z
Learning: In the codebase, the use of into() and try_into() for converting TransactionOptions is intentional. Conversions that may fail use try_into(), while those that cannot fail use into(). This approach is appropriate and should be preserved.
Examples:
try_into()is used inbin/sozo/src/commands/migrate.rsandbin/sozo/src/commands/execute.rswhere conversions can fail.into()is used inbin/sozo/src/commands/register.rsandbin/sozo/src/commands/auth.rswhere conversions are infallible.
Cargo.toml (2)
Learnt from: glihm
PR: #2650
File: bin/sozo/src/commands/migrate.rs:64-65
Timestamp: 2024-11-07T13:57:57.616Z
Learning: In the codebase, the use of into() and try_into() for converting TransactionOptions is intentional. Conversions that may fail use try_into(), while those that cannot fail use into(). This approach is appropriate and should be preserved.
Examples:
try_into()is used inbin/sozo/src/commands/migrate.rsandbin/sozo/src/commands/execute.rswhere conversions can fail.into()is used inbin/sozo/src/commands/register.rsandbin/sozo/src/commands/auth.rswhere conversions are infallible.
Learnt from: glihm
PR: #2633
File: crates/dojo/world/src/contracts/abigen/world.rs:3809-0
Timestamp: 2024-11-05T04:30:12.852Z
Learning: Code in crates/dojo/world/src/contracts/abigen/ is auto-generated and should be excluded from code reviews.
crates/sozo/voyager/src/analyzer.rs (1)
Learnt from: glihm
PR: #2633
File: crates/dojo/world/src/contracts/abigen/world.rs:3809-0
Timestamp: 2024-11-05T04:30:12.852Z
Learning: Code in crates/dojo/world/src/contracts/abigen/ is auto-generated and should be excluded from code reviews.
crates/sozo/ops/src/migrate/mod.rs (1)
Learnt from: glihm
PR: #2650
File: crates/dojo/utils/src/tx/mod.rs:201-272
Timestamp: 2024-11-07T14:43:23.530Z
Learning: In crates/dojo/utils/src/tx/mod.rs, fee token configurations are verified when the fee_config is built, making additional validations in the send_with_cfg methods redundant.
crates/sozo/voyager/src/config.rs (1)
Learnt from: glihm
PR: #2650
File: crates/dojo/utils/src/tx/mod.rs:201-272
Timestamp: 2024-11-07T14:43:23.530Z
Learning: In crates/dojo/utils/src/tx/mod.rs, fee token configurations are verified when the fee_config is built, making additional validations in the send_with_cfg methods redundant.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: fmt
🔇 Additional comments (22)
bin/sozo/src/commands/options/mod.rs (1)
6-6: Ohayo sensei! Clean module addition for verification options.The new
verifymodule declaration properly exposes the verification CLI options, maintaining consistency with the existing module structure.crates/sozo/ops/src/migration_ui.rs (2)
5-5: Ohayo sensei! Good integration of verification UI trait.The import of
VerificationUiproperly enables the migration UI to support verification status updates.
98-102: Ohayo sensei! Clean trait implementation for verification UI.The
VerificationUitrait implementation correctly forwards to the existingupdate_text_boxedmethod, enabling seamless integration between the migration UI and verification workflow without code duplication.crates/sozo/ops/src/tests/migration.rs (2)
88-89: Ohayo sensei! Proper test update for extended migration result.The destructuring pattern correctly handles the expanded
MigrationResultstructure by ignoring the new verification-related fields while preserving access to the requiredmanifestandhas_changesfields.
99-100: Ohayo sensei! Consistent test pattern for migration results.The destructuring approach matches the pattern used in the other test function, maintaining consistency while accommodating the extended result structure.
crates/sozo/voyager/Cargo.toml (1)
1-17: Ohayo sensei! Well-structured manifest for the new voyager crate.The Cargo.toml properly uses workspace inheritance and includes appropriate dependencies for contract verification functionality. The feature selections (multipart for reqwest, derive for serde, time for tokio) align well with the verification use case.
crates/sozo/ops/Cargo.toml (4)
17-17: Ohayo sensei! Good addition of scarb-interop dependency.The
scarb-interopdependency addition supports project analysis functionality needed for the verification workflow.
19-19: Ohayo sensei! Proper integration of sozo-voyager dependency.The
sozo-voyagerdependency correctly enables the ops crate to use the new verification functionality.
25-25: Ohayo sensei! Appropriate tokio feature addition.Adding the "time" feature to tokio supports timeout functionality that's likely needed for verification operations.
40-41: Ohayo sensei! Smart cargo-machete configuration.The configuration properly ignores the optional
sozo-walnutdependency, preventing false positives from the unused dependency checker.bin/sozo/src/commands/options/verify.rs (1)
6-21: Ohayo! The struct definition looks good, sensei!The CLI options are well-structured with clear documentation and appropriate default values.
crates/sozo/voyager/src/lib.rs (1)
1-19: Ohayo! Excellent crate structure, sensei!The module organization is clean and the re-exports provide a convenient public API. The documentation clearly explains the crate's purpose.
bin/sozo/src/commands/migrate.rs (2)
23-23: Ohayo! Clean integration of verification options, sensei!The addition of VerifyOptions is properly integrated with the existing command structure.
Also applies to: 42-43, 56-56
76-101: Well-structured conditional migration construction, sensei!The code correctly creates Migration with or without verification based on the configuration.
crates/sozo/voyager/src/verifier.rs (2)
131-134: Good choice of backoff parameters, sensei!The exponential backoff with 1.5 multiplier and proper bounds (2s to 30s) provides a good balance between responsiveness and API load.
138-199: Excellent retry logic and error handling, sensei!The polling mechanism properly handles all verification states and includes backoff even for errors to avoid overwhelming the API.
Cargo.toml (1)
6-6: Ohayo! Workspace configuration updates look good, sensei!The new crates are properly integrated into the workspace structure and dependency declarations.
Also applies to: 18-19, 22-22, 81-84
crates/sozo/ops/src/migrate/mod.rs (1)
69-115: LGTM! Clean implementation of optional verificationOhayo! The dual constructor pattern nicely maintains backward compatibility while adding the new verification feature. The optional field approach is well-suited for this use case.
crates/sozo/voyager/src/analyzer.rs (1)
554-587: LGTM! Comprehensive file validationOhayo! The file validation logic is well-implemented with appropriate size limits and extension checks. The 20MB limit is reasonable for source files.
crates/sozo/voyager/src/config.rs (3)
26-37: LGTM! Reasonable default configuration valuesOhayo sensei! The default timeout values are well-chosen - 5 minutes for HTTP requests and 30 minutes total for verification should handle most cases without being excessive.
74-91: Well-designed status enum with proper unknown handlingThe numeric status code mapping with an
Unknownvariant for unrecognized values is a good defensive programming practice. This will prevent deserialization failures if the API adds new status codes.
186-222: LGTM! User-friendly result representationThe
VerificationResultenum provides clear, emoji-enhanced messages that make it easy for users to understand verification outcomes at a glance. Theis_success()helper method is a nice touch.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (6)
bin/sozo/src/commands/migrate.rs (1)
103-103: Great work implementing the enum-based matching, sensei!The verification results display correctly uses pattern matching on
VerificationResultenum variants instead of checking for emoji characters in strings, which makes the code more robust and maintainable.Also applies to: 106-141
crates/sozo/voyager/src/client.rs (4)
18-79: Ohayo! Excellent work on the error handling improvements, sensei!The HTTP client constructor now properly returns a
Resultinstead of usingexpect(), and the CircuitBreaker implementation provides robust failure handling with appropriate thresholds.
101-101: The license is still hardcoded, sensei.Consider making the license configurable through project metadata instead of defaulting to "MIT".
- .text("license", metadata.license.as_deref().unwrap_or("MIT").to_string()); + .text("license", metadata.license.as_deref().unwrap_or("MIT").to_string());Note: The metadata already has a license field that can be populated from the project configuration.
142-239: Great implementation of DTO-based parsing, sensei!The verification status checking now uses
VerificationJobDtofor deserialization with proper error handling and fallback logic, which is much more maintainable than manual JSON field extraction.
242-248: Ohayo! Perfect refactoring of JSON manipulation, sensei!The
remove_files_fieldmethod now properly parses JSON intoserde_json::Value, removes the field, and re-serializes, which is much more robust than manual string manipulation.crates/sozo/voyager/src/analyzer.rs (1)
519-574: Excellent implementation of depth limiting, sensei!The recursive file collection now includes proper depth tracking with a reasonable limit of 20 levels, which prevents potential stack overflow on deeply nested directories.
🧹 Nitpick comments (1)
crates/sozo/voyager/src/utils.rs (1)
31-66: Consider extracting the default version constant, sensei.The implementation handles errors well, but the hardcoded "2.8.0" appears multiple times. Consider extracting it to a constant for easier maintenance.
+const DEFAULT_VERSION: &str = "2.8.0"; + /// Get Cairo and Scarb versions from project configuration. pub fn get_project_versions() -> Result<(String, String), anyhow::Error> { use std::process::Command; // Get Cairo version from scarb metadata let cairo_version = if let Ok(output) = Command::new("scarb").args(["metadata", "--format-version", "1"]).output() { if output.status.success() { let metadata_str = String::from_utf8(output.stdout)?; if let Ok(metadata) = serde_json::from_str::<serde_json::Value>(&metadata_str) { - metadata["cairo_version"].as_str().unwrap_or("2.8.0").to_string() + metadata["cairo_version"].as_str().unwrap_or(DEFAULT_VERSION).to_string() } else { - "2.8.0".to_string() + DEFAULT_VERSION.to_string() } } else { - "2.8.0".to_string() + DEFAULT_VERSION.to_string() } } else { - "2.8.0".to_string() + DEFAULT_VERSION.to_string() }; // Get Scarb version let scarb_version = if let Ok(output) = Command::new("scarb").args(["--version"]).output() { if output.status.success() { let version_str = String::from_utf8(output.stdout)?; // Parse "scarb 2.8.0" format - version_str.split_whitespace().nth(1).unwrap_or("2.8.0").to_string() + version_str.split_whitespace().nth(1).unwrap_or(DEFAULT_VERSION).to_string() } else { - "2.8.0".to_string() + DEFAULT_VERSION.to_string() } } else { - "2.8.0".to_string() + DEFAULT_VERSION.to_string() }; Ok((cairo_version, scarb_version)) }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
bin/sozo/src/commands/migrate.rs(4 hunks)crates/sozo/ops/src/migrate/mod.rs(5 hunks)crates/sozo/voyager/src/analyzer.rs(1 hunks)crates/sozo/voyager/src/client.rs(1 hunks)crates/sozo/voyager/src/config.rs(1 hunks)crates/sozo/voyager/src/lib.rs(1 hunks)crates/sozo/voyager/src/utils.rs(1 hunks)crates/sozo/voyager/src/verifier.rs(1 hunks)
🧠 Learnings (4)
📓 Common learnings
Learnt from: glihm
PR: dojoengine/dojo#2633
File: crates/dojo/world/src/contracts/abigen/world.rs:3809-0
Timestamp: 2024-11-05T04:30:12.852Z
Learning: Code in `crates/dojo/world/src/contracts/abigen/` is auto-generated and should be excluded from code reviews.
Learnt from: glihm
PR: dojoengine/dojo#2650
File: crates/dojo/utils/src/tx/mod.rs:201-272
Timestamp: 2024-11-07T14:43:23.530Z
Learning: In `crates/dojo/utils/src/tx/mod.rs`, fee token configurations are verified when the `fee_config` is built, making additional validations in the `send_with_cfg` methods redundant.
bin/sozo/src/commands/migrate.rs (1)
Learnt from: glihm
PR: #2650
File: bin/sozo/src/commands/migrate.rs:64-65
Timestamp: 2024-11-07T13:57:57.616Z
Learning: In the codebase, the use of into() and try_into() for converting TransactionOptions is intentional. Conversions that may fail use try_into(), while those that cannot fail use into(). This approach is appropriate and should be preserved.
Examples:
try_into()is used inbin/sozo/src/commands/migrate.rsandbin/sozo/src/commands/execute.rswhere conversions can fail.into()is used inbin/sozo/src/commands/register.rsandbin/sozo/src/commands/auth.rswhere conversions are infallible.
crates/sozo/voyager/src/client.rs (2)
Learnt from: glihm
PR: #2650
File: bin/sozo/src/commands/migrate.rs:64-65
Timestamp: 2024-11-07T13:57:57.616Z
Learning: In the codebase, the use of into() and try_into() for converting TransactionOptions is intentional. Conversions that may fail use try_into(), while those that cannot fail use into(). This approach is appropriate and should be preserved.
Examples:
try_into()is used inbin/sozo/src/commands/migrate.rsandbin/sozo/src/commands/execute.rswhere conversions can fail.into()is used inbin/sozo/src/commands/register.rsandbin/sozo/src/commands/auth.rswhere conversions are infallible.
Learnt from: glihm
PR: #2650
File: crates/dojo/utils/src/tx/mod.rs:201-272
Timestamp: 2024-11-07T14:43:23.530Z
Learning: In crates/dojo/utils/src/tx/mod.rs, fee token configurations are verified when the fee_config is built, making additional validations in the send_with_cfg methods redundant.
crates/sozo/voyager/src/analyzer.rs (1)
Learnt from: glihm
PR: #2633
File: crates/dojo/world/src/contracts/abigen/world.rs:3809-0
Timestamp: 2024-11-05T04:30:12.852Z
Learning: Code in crates/dojo/world/src/contracts/abigen/ is auto-generated and should be excluded from code reviews.
🚧 Files skipped from review as they are similar to previous changes (4)
- crates/sozo/voyager/src/lib.rs
- crates/sozo/voyager/src/verifier.rs
- crates/sozo/ops/src/migrate/mod.rs
- crates/sozo/voyager/src/config.rs
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: glihm
PR: dojoengine/dojo#2633
File: crates/dojo/world/src/contracts/abigen/world.rs:3809-0
Timestamp: 2024-11-05T04:30:12.852Z
Learning: Code in `crates/dojo/world/src/contracts/abigen/` is auto-generated and should be excluded from code reviews.
Learnt from: glihm
PR: dojoengine/dojo#2650
File: crates/dojo/utils/src/tx/mod.rs:201-272
Timestamp: 2024-11-07T14:43:23.530Z
Learning: In `crates/dojo/utils/src/tx/mod.rs`, fee token configurations are verified when the `fee_config` is built, making additional validations in the `send_with_cfg` methods redundant.
bin/sozo/src/commands/migrate.rs (1)
Learnt from: glihm
PR: #2650
File: bin/sozo/src/commands/migrate.rs:64-65
Timestamp: 2024-11-07T13:57:57.616Z
Learning: In the codebase, the use of into() and try_into() for converting TransactionOptions is intentional. Conversions that may fail use try_into(), while those that cannot fail use into(). This approach is appropriate and should be preserved.
Examples:
try_into()is used inbin/sozo/src/commands/migrate.rsandbin/sozo/src/commands/execute.rswhere conversions can fail.into()is used inbin/sozo/src/commands/register.rsandbin/sozo/src/commands/auth.rswhere conversions are infallible.
crates/sozo/voyager/src/client.rs (2)
Learnt from: glihm
PR: #2650
File: bin/sozo/src/commands/migrate.rs:64-65
Timestamp: 2024-11-07T13:57:57.616Z
Learning: In the codebase, the use of into() and try_into() for converting TransactionOptions is intentional. Conversions that may fail use try_into(), while those that cannot fail use into(). This approach is appropriate and should be preserved.
Examples:
try_into()is used inbin/sozo/src/commands/migrate.rsandbin/sozo/src/commands/execute.rswhere conversions can fail.into()is used inbin/sozo/src/commands/register.rsandbin/sozo/src/commands/auth.rswhere conversions are infallible.
Learnt from: glihm
PR: #2650
File: crates/dojo/utils/src/tx/mod.rs:201-272
Timestamp: 2024-11-07T14:43:23.530Z
Learning: In crates/dojo/utils/src/tx/mod.rs, fee token configurations are verified when the fee_config is built, making additional validations in the send_with_cfg methods redundant.
crates/sozo/voyager/src/analyzer.rs (1)
Learnt from: glihm
PR: #2633
File: crates/dojo/world/src/contracts/abigen/world.rs:3809-0
Timestamp: 2024-11-05T04:30:12.852Z
Learning: Code in crates/dojo/world/src/contracts/abigen/ is auto-generated and should be excluded from code reviews.
🔇 Additional comments (5)
bin/sozo/src/commands/migrate.rs (1)
23-23: Ohayo! The verification integration looks solid, sensei!The implementation correctly adds verification support to the migration command with proper conditional handling based on whether verification is requested.
Also applies to: 42-44, 56-56, 76-101
crates/sozo/voyager/src/utils.rs (1)
6-28: Ohayo! The project root discovery logic looks good, sensei.The implementation correctly searches upward for project markers and handles edge cases properly with a fallback to the current directory.
crates/sozo/voyager/src/analyzer.rs (3)
25-66: Ohayo! Clean metadata extraction implementation, sensei.The methods properly handle optional values and provide good error messages when required fields are missing.
69-137: Solid artifact discovery logic, sensei!The implementation correctly handles different artifact types with appropriate naming conventions and robust error handling for class hash parsing.
620-760: Ohayo! Comprehensive contract file discovery implementation, sensei.The multi-step approach with pattern matching and fallback strategies ensures reliable contract file detection across different project structures.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
crates/sozo/voyager/src/client.rs (2)
101-101: Make license configurable instead of hard-codingOhayo! The license is still hard-coded as "MIT", but different projects might use different licenses. This was mentioned in a previous review.
- .text("license", metadata.license.as_deref().unwrap_or("MIT").to_string()); + .text("license", metadata.license.as_deref().unwrap_or(&config.default_license).to_string());Consider adding a
default_licensefield toVerificationConfigto make this configurable.
169-191: Use serde-derived structs for JSON parsingOhayo sensei! The manual JSON parsing approach with fallback logic is still present. As mentioned in a previous review, this could be improved by using proper serde deserialization.
The current approach of parsing to
VerificationJobDtoand falling back toremove_files_fieldon syntax errors is already close to the recommended pattern from the previous review. However, consider defining the DTO to explicitly exclude the problematicfilesfield:#[derive(Deserialize)] struct VerificationJobDto { #[serde(rename = "jobid")] job_id: String, status: u64, // ... other fields ... // Note: no `files` field - serde will ignore it by default }This would eliminate the need for the fallback logic entirely.
🧹 Nitpick comments (6)
crates/sozo/voyager/src/client.rs (1)
26-34: Consider making circuit breaker parameters configurableOhayo sensei! The circuit breaker implementation looks solid, but the hardcoded threshold and recovery timeout values could limit flexibility in different deployment scenarios.
Consider accepting these as parameters:
- pub(crate) fn new() -> Self { + pub(crate) fn new(failure_threshold: u32, recovery_timeout_secs: u64) -> Self { Self { failure_count: 0, last_failure_time: None, - failure_threshold: 5, // Open circuit after 5 consecutive failures - recovery_timeout: Duration::from_secs(60), // Wait 1 minute before retry + failure_threshold, + recovery_timeout: Duration::from_secs(recovery_timeout_secs), } }Then update the usage in VerificationClient:
- Ok(Self { client, config, circuit_breaker: std::sync::Mutex::new(CircuitBreaker::new()) }) + Ok(Self { + client, + config: config.clone(), + circuit_breaker: std::sync::Mutex::new(CircuitBreaker::new(5, 60)) + })crates/sozo/voyager/src/lib.rs (1)
122-124: Improve jitter test assertion precisionOhayo! The jitter test assertion could be more precise about the expected range.
- assert!(jittered.as_millis() <= base_duration.as_millis() + 3500); // 25% + 1s max jitter + // Jitter adds up to 25% of base + up to 999ms + let max_jitter = base_duration.as_millis() * 125 / 100 + 999; + assert!(jittered.as_millis() <= max_jitter);crates/sozo/voyager/src/verifier.rs (2)
35-59: Consider using a proper random number generator for jitterOhayo! The current jitter implementation using system time as seed could produce predictable patterns. Consider using a proper RNG for better distribution.
use rand::Rng; pub(crate) fn add_jitter(&self, duration: Duration) -> Duration { let mut rng = rand::thread_rng(); let base_ms = duration.as_millis() as u64; if base_ms == 0 { return Duration::from_millis(rng.gen_range(0..100)); } // Add ±25% jitter let jitter_range = base_ms / 4; let jitter = if jitter_range > 0 { rng.gen_range(0..jitter_range * 2) as i64 - jitter_range as i64 } else { rng.gen_range(0..100) as i64 }; Duration::from_millis((base_ms as i64 + jitter).max(0) as u64) }This would require adding
randto the crate dependencies.
77-104: Consider parallel verification for better performanceOhayo sensei! The current implementation verifies contracts sequentially, which could be slow when dealing with many contracts.
Consider implementing parallel verification with a configurable concurrency limit to improve performance while avoiding overwhelming the API:
use futures::stream::{self, StreamExt}; // In verify_deployed_contracts: let max_concurrent = self.config.max_concurrent_verifications.unwrap_or(3); let results = stream::iter(artifacts) .map(|artifact| async move { // verification logic here }) .buffer_unordered(max_concurrent) .collect::<Vec<_>>() .await;This would allow multiple contracts to be verified simultaneously while respecting API rate limits.
crates/sozo/voyager/src/analyzer.rs (2)
589-622: Comprehensive file validation with good security practicesOhayo sensei! The file validation implements good security measures with size limits and extension whitelisting.
Consider including the actual file size in the error message for better debugging:
return Err(anyhow!( - "File {} exceeds maximum size limit of {}MB", + "File {} exceeds maximum size limit of {}MB (actual size: {}MB)", file.path.display(), - MAX_FILE_SIZE / (1024 * 1024) + MAX_FILE_SIZE / (1024 * 1024), + metadata.len() / (1024 * 1024) ));
715-764: Comprehensive pattern matching for contract definitionsOhayo! The pattern matching covers a wide range of Cairo constructs including Dojo-specific annotations.
Consider using regex for more efficient pattern matching instead of multiple string contains checks:
use regex::Regex; lazy_static! { static ref DEFINITION_REGEX: Regex = Regex::new( r"(?x) (?:struct|trait|mod|impl|enum)\s+(?:m_|e_)?Position| \#\[derive\((?:Model|Event)\)\]\s*\n\s*struct\s+(?:m_|e_)?Position " ).unwrap(); }This would reduce the number of string searches and improve performance for large files.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (5)
crates/sozo/voyager/Cargo.toml(1 hunks)crates/sozo/voyager/src/analyzer.rs(1 hunks)crates/sozo/voyager/src/client.rs(1 hunks)crates/sozo/voyager/src/lib.rs(1 hunks)crates/sozo/voyager/src/verifier.rs(1 hunks)
🧠 Learnings (5)
📓 Common learnings
Learnt from: glihm
PR: dojoengine/dojo#2633
File: crates/dojo/world/src/contracts/abigen/world.rs:3809-0
Timestamp: 2024-11-05T04:30:12.852Z
Learning: Code in `crates/dojo/world/src/contracts/abigen/` is auto-generated and should be excluded from code reviews.
Learnt from: glihm
PR: dojoengine/dojo#2650
File: crates/dojo/utils/src/tx/mod.rs:201-272
Timestamp: 2024-11-07T14:43:23.530Z
Learning: In `crates/dojo/utils/src/tx/mod.rs`, fee token configurations are verified when the `fee_config` is built, making additional validations in the `send_with_cfg` methods redundant.
crates/sozo/voyager/src/lib.rs (2)
Learnt from: glihm
PR: #2633
File: crates/dojo/world/src/contracts/abigen/world.rs:3809-0
Timestamp: 2024-11-05T04:30:12.852Z
Learning: Code in crates/dojo/world/src/contracts/abigen/ is auto-generated and should be excluded from code reviews.
Learnt from: glihm
PR: #2650
File: crates/dojo/utils/src/tx/mod.rs:201-272
Timestamp: 2024-11-07T14:43:23.530Z
Learning: In crates/dojo/utils/src/tx/mod.rs, fee token configurations are verified when the fee_config is built, making additional validations in the send_with_cfg methods redundant.
crates/sozo/voyager/src/client.rs (2)
Learnt from: glihm
PR: #2650
File: bin/sozo/src/commands/migrate.rs:64-65
Timestamp: 2024-11-07T13:57:57.616Z
Learning: In the codebase, the use of into() and try_into() for converting TransactionOptions is intentional. Conversions that may fail use try_into(), while those that cannot fail use into(). This approach is appropriate and should be preserved.
Examples:
try_into()is used inbin/sozo/src/commands/migrate.rsandbin/sozo/src/commands/execute.rswhere conversions can fail.into()is used inbin/sozo/src/commands/register.rsandbin/sozo/src/commands/auth.rswhere conversions are infallible.
Learnt from: glihm
PR: #2650
File: crates/dojo/utils/src/tx/mod.rs:201-272
Timestamp: 2024-11-07T14:43:23.530Z
Learning: In crates/dojo/utils/src/tx/mod.rs, fee token configurations are verified when the fee_config is built, making additional validations in the send_with_cfg methods redundant.
crates/sozo/voyager/src/verifier.rs (1)
Learnt from: glihm
PR: #2650
File: crates/dojo/utils/src/tx/mod.rs:201-272
Timestamp: 2024-11-07T14:43:23.530Z
Learning: In crates/dojo/utils/src/tx/mod.rs, fee token configurations are verified when the fee_config is built, making additional validations in the send_with_cfg methods redundant.
crates/sozo/voyager/src/analyzer.rs (1)
Learnt from: glihm
PR: #2633
File: crates/dojo/world/src/contracts/abigen/world.rs:3809-0
Timestamp: 2024-11-05T04:30:12.852Z
Learning: Code in crates/dojo/world/src/contracts/abigen/ is auto-generated and should be excluded from code reviews.
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/sozo/voyager/Cargo.toml
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: glihm
PR: dojoengine/dojo#2633
File: crates/dojo/world/src/contracts/abigen/world.rs:3809-0
Timestamp: 2024-11-05T04:30:12.852Z
Learning: Code in `crates/dojo/world/src/contracts/abigen/` is auto-generated and should be excluded from code reviews.
Learnt from: glihm
PR: dojoengine/dojo#2650
File: crates/dojo/utils/src/tx/mod.rs:201-272
Timestamp: 2024-11-07T14:43:23.530Z
Learning: In `crates/dojo/utils/src/tx/mod.rs`, fee token configurations are verified when the `fee_config` is built, making additional validations in the `send_with_cfg` methods redundant.
crates/sozo/voyager/src/lib.rs (2)
Learnt from: glihm
PR: #2633
File: crates/dojo/world/src/contracts/abigen/world.rs:3809-0
Timestamp: 2024-11-05T04:30:12.852Z
Learning: Code in crates/dojo/world/src/contracts/abigen/ is auto-generated and should be excluded from code reviews.
Learnt from: glihm
PR: #2650
File: crates/dojo/utils/src/tx/mod.rs:201-272
Timestamp: 2024-11-07T14:43:23.530Z
Learning: In crates/dojo/utils/src/tx/mod.rs, fee token configurations are verified when the fee_config is built, making additional validations in the send_with_cfg methods redundant.
crates/sozo/voyager/src/client.rs (2)
Learnt from: glihm
PR: #2650
File: bin/sozo/src/commands/migrate.rs:64-65
Timestamp: 2024-11-07T13:57:57.616Z
Learning: In the codebase, the use of into() and try_into() for converting TransactionOptions is intentional. Conversions that may fail use try_into(), while those that cannot fail use into(). This approach is appropriate and should be preserved.
Examples:
try_into()is used inbin/sozo/src/commands/migrate.rsandbin/sozo/src/commands/execute.rswhere conversions can fail.into()is used inbin/sozo/src/commands/register.rsandbin/sozo/src/commands/auth.rswhere conversions are infallible.
Learnt from: glihm
PR: #2650
File: crates/dojo/utils/src/tx/mod.rs:201-272
Timestamp: 2024-11-07T14:43:23.530Z
Learning: In crates/dojo/utils/src/tx/mod.rs, fee token configurations are verified when the fee_config is built, making additional validations in the send_with_cfg methods redundant.
crates/sozo/voyager/src/verifier.rs (1)
Learnt from: glihm
PR: #2650
File: crates/dojo/utils/src/tx/mod.rs:201-272
Timestamp: 2024-11-07T14:43:23.530Z
Learning: In crates/dojo/utils/src/tx/mod.rs, fee token configurations are verified when the fee_config is built, making additional validations in the send_with_cfg methods redundant.
crates/sozo/voyager/src/analyzer.rs (1)
Learnt from: glihm
PR: #2633
File: crates/dojo/world/src/contracts/abigen/world.rs:3809-0
Timestamp: 2024-11-05T04:30:12.852Z
Learning: Code in crates/dojo/world/src/contracts/abigen/ is auto-generated and should be excluded from code reviews.
🔇 Additional comments (8)
crates/sozo/voyager/src/client.rs (2)
72-79: Previous HTTP client error handling issue has been resolved ✅The HTTP client creation now properly returns a
Resultand handles errors gracefully, addressing the previous review comment.
242-248: JSON field removal properly implemented ✅The
remove_files_fieldmethod now correctly usesserde_json::Valuefor parsing and manipulation, addressing the previous review comment about fragile string manipulation.crates/sozo/voyager/src/lib.rs (2)
150-278: Comprehensive analyzer test coverage ✅Ohayo sensei! The analyzer tests provide excellent coverage of the ProjectAnalyzer functionality, including:
- Metadata extraction (Dojo version, package name, license)
- Contract artifact discovery with proper prefixing
- File validation for size limits and allowed extensions
- Security checks for path traversal attempts
The test structure with
create_temp_project()helper is clean and maintainable.
280-336: Well-structured circuit breaker tests ✅The client tests effectively verify:
- Circuit breaker state transitions
- Failure threshold behavior
- Reset functionality on success
- Default configuration values
crates/sozo/voyager/src/verifier.rs (1)
136-217: Robust retry logic with exponential backoff ✅The
wait_for_verificationimplementation demonstrates excellent practices:
- Exponential backoff with configurable multiplier
- Jitter to prevent request clustering
- Proper handling of terminal states (success/failure)
- Graceful degradation on timeout
crates/sozo/voyager/src/analyzer.rs (3)
523-536: Depth limit properly implemented ✅Ohayo! The recursive file collection now includes proper depth limiting to prevent stack overflow, addressing the previous review comment.
192-210: Clean artifact name extraction logic ✅The
extract_contract_name_from_tagmethod properly handles:
- Package name prefix stripping
- Type-specific prefixing (m_ for models, e_ for events)
- Graceful fallback for unexpected tag formats
624-679: Robust contract file discovery with multiple strategies ✅The
find_contract_filemethod implements a comprehensive multi-step approach:
- Special handling for models/events (using lib.cairo)
- Content-based search for contract definitions
- Filename matching with variations
- Convention-based fallbacks
- Last resort fallback
This layered approach ensures contracts can be found even in non-standard project structures.
Improves contract verification by handling the scenario where a contract or class has already been verified, preventing errors and providing a more informative user experience. Introduces a `VerificationError` enum to handle specific verification-related errors, including the "AlreadyVerified" case. Modifies the `migrate` command to correctly display a success message when a contract is already verified.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
crates/sozo/voyager/src/client.rs (1)
125-125: License is still hardcoded as "MIT"Ohayo sensei! As mentioned in the previous review, the license should be configurable rather than defaulting to "MIT". Different projects use different licenses.
- .text("license", metadata.license.as_deref().unwrap_or("MIT").to_string()); + .text("license", metadata.license.as_deref().unwrap_or("").to_string());Consider requiring the license in
ProjectMetadataor making it explicit that no license was provided.
🧹 Nitpick comments (4)
crates/sozo/voyager/src/client.rs (2)
51-52: Consider making circuit breaker parameters configurableOhayo sensei! The circuit breaker parameters are hardcoded. Consider making them configurable through
VerificationConfigfor different deployment environments.- failure_threshold: 5, // Open circuit after 5 consecutive failures - recovery_timeout: Duration::from_secs(60), // Wait 1 minute before retry + failure_threshold: 5, // TODO: Make configurable via VerificationConfig + recovery_timeout: Duration::from_secs(60), // TODO: Make configurable via VerificationConfig
222-244: JSON parsing improved but still uses fallback workaroundOhayo! Good improvement using
VerificationJobDtofor deserialization. The fallback toremove_files_fieldappears necessary for handling malformed API responses, though ideally this should be fixed on the API side.Consider documenting why this workaround is needed and tracking when it can be removed once the API is fixed.
crates/sozo/voyager/src/config.rs (2)
29-29: Document beta API endpoint statusOhayo! The default API URL points to a beta endpoint. Consider adding a comment about the beta status and when this might change.
- api_url: Url::parse("https://api.voyager.online/beta").unwrap(), + // TODO: Update to stable API endpoint when available + api_url: Url::parse("https://api.voyager.online/beta").unwrap(),
75-92: Consider consolidating status code mappingOhayo! The status code mapping is duplicated between serde attributes and the From implementation. Consider using a single source of truth to avoid maintenance issues.
You could use a match on the enum discriminant or a constant mapping to avoid duplication:
impl VerifyJobStatus { pub fn from_code(code: u64) -> Self { match code { 0 => Self::Submitted, 1 => Self::Compiled, 2 => Self::CompileFailed, 3 => Self::Fail, 4 => Self::Success, 5 => Self::InProgress, _ => Self::Unknown, } } }Then use this in the From implementation.
Also applies to: 161-169
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
bin/sozo/src/commands/migrate.rs(4 hunks)crates/sozo/voyager/src/client.rs(1 hunks)crates/sozo/voyager/src/config.rs(1 hunks)crates/sozo/voyager/src/verifier.rs(1 hunks)
🧠 Learnings (3)
📓 Common learnings
Learnt from: glihm
PR: dojoengine/dojo#2633
File: crates/dojo/world/src/contracts/abigen/world.rs:3809-0
Timestamp: 2024-11-05T04:30:12.852Z
Learning: Code in `crates/dojo/world/src/contracts/abigen/` is auto-generated and should be excluded from code reviews.
Learnt from: glihm
PR: dojoengine/dojo#2650
File: crates/dojo/utils/src/tx/mod.rs:201-272
Timestamp: 2024-11-07T14:43:23.530Z
Learning: In `crates/dojo/utils/src/tx/mod.rs`, fee token configurations are verified when the `fee_config` is built, making additional validations in the `send_with_cfg` methods redundant.
crates/sozo/voyager/src/client.rs (2)
Learnt from: glihm
PR: #2650
File: bin/sozo/src/commands/migrate.rs:64-65
Timestamp: 2024-11-07T13:57:57.616Z
Learning: In the codebase, the use of into() and try_into() for converting TransactionOptions is intentional. Conversions that may fail use try_into(), while those that cannot fail use into(). This approach is appropriate and should be preserved.
Examples:
try_into()is used inbin/sozo/src/commands/migrate.rsandbin/sozo/src/commands/execute.rswhere conversions can fail.into()is used inbin/sozo/src/commands/register.rsandbin/sozo/src/commands/auth.rswhere conversions are infallible.
Learnt from: glihm
PR: #2650
File: crates/dojo/utils/src/tx/mod.rs:201-272
Timestamp: 2024-11-07T14:43:23.530Z
Learning: In crates/dojo/utils/src/tx/mod.rs, fee token configurations are verified when the fee_config is built, making additional validations in the send_with_cfg methods redundant.
crates/sozo/voyager/src/config.rs (1)
Learnt from: glihm
PR: #2650
File: crates/dojo/utils/src/tx/mod.rs:201-272
Timestamp: 2024-11-07T14:43:23.530Z
Learning: In crates/dojo/utils/src/tx/mod.rs, fee token configurations are verified when the fee_config is built, making additional validations in the send_with_cfg methods redundant.
🚧 Files skipped from review as they are similar to previous changes (2)
- bin/sozo/src/commands/migrate.rs
- crates/sozo/voyager/src/verifier.rs
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: glihm
PR: dojoengine/dojo#2633
File: crates/dojo/world/src/contracts/abigen/world.rs:3809-0
Timestamp: 2024-11-05T04:30:12.852Z
Learning: Code in `crates/dojo/world/src/contracts/abigen/` is auto-generated and should be excluded from code reviews.
Learnt from: glihm
PR: dojoengine/dojo#2650
File: crates/dojo/utils/src/tx/mod.rs:201-272
Timestamp: 2024-11-07T14:43:23.530Z
Learning: In `crates/dojo/utils/src/tx/mod.rs`, fee token configurations are verified when the `fee_config` is built, making additional validations in the `send_with_cfg` methods redundant.
crates/sozo/voyager/src/client.rs (2)
Learnt from: glihm
PR: #2650
File: bin/sozo/src/commands/migrate.rs:64-65
Timestamp: 2024-11-07T13:57:57.616Z
Learning: In the codebase, the use of into() and try_into() for converting TransactionOptions is intentional. Conversions that may fail use try_into(), while those that cannot fail use into(). This approach is appropriate and should be preserved.
Examples:
try_into()is used inbin/sozo/src/commands/migrate.rsandbin/sozo/src/commands/execute.rswhere conversions can fail.into()is used inbin/sozo/src/commands/register.rsandbin/sozo/src/commands/auth.rswhere conversions are infallible.
Learnt from: glihm
PR: #2650
File: crates/dojo/utils/src/tx/mod.rs:201-272
Timestamp: 2024-11-07T14:43:23.530Z
Learning: In crates/dojo/utils/src/tx/mod.rs, fee token configurations are verified when the fee_config is built, making additional validations in the send_with_cfg methods redundant.
crates/sozo/voyager/src/config.rs (1)
Learnt from: glihm
PR: #2650
File: crates/dojo/utils/src/tx/mod.rs:201-272
Timestamp: 2024-11-07T14:43:23.530Z
Learning: In crates/dojo/utils/src/tx/mod.rs, fee token configurations are verified when the fee_config is built, making additional validations in the send_with_cfg methods redundant.
🔇 Additional comments (4)
crates/sozo/voyager/src/client.rs (2)
92-99: Great error handling improvement!Ohayo! The constructor now properly returns a
Resultand handles HTTP client creation errors gracefully, addressing the previous review feedback perfectly.
294-301: Excellent refactor of JSON field removal!Ohayo sensei! The JSON manipulation now uses
serde_json::Valueas suggested in the previous review. This is much more robust than manual string manipulation.crates/sozo/voyager/src/config.rs (2)
100-185: Well-designed DTO pattern implementationOhayo sensei! Excellent separation between the API DTO and domain model. The field mapping with serde rename attributes and the From trait implementation make the code maintainable and clear.
256-282: Nice user-friendly result display implementationOhayo sensei! The
VerificationResultenum is well-designed with clear variants and user-friendly display messages. The emoji usage adds great visual feedback for CLI users.
|
Will close in favor of #3288. |
2 participants
Description
Adds contract verification to Sozo migrations using the Voyager API.
This change introduces contract verification functionality to the Sozo migration process, allowing users to automatically verify deployed contracts against the Voyager API. It refactors verification options and integrates a new sozo-voyager crate for handling verification logic.
Changes
sozo-voyagercrate for contract verification. This crate handles interaction with the Voyager API, project analysis, and file collection.VerifyOptionsstruct tobin/sozo/src/commands/options/verify.rsfor command-line arguments related to verification, including service selection and API URL configuration.bin/sozo/src/commands/migrate.rsto incorporate the verification process. A VerificationConfig is created based on the provided options, and the Migration struct is extended to support optional verification. Verification results are displayed after the migration.crates/sozo/ops/Cargo.tomlandcrates/sozo/ops/src/migrate/mod.rsto include and use thesozo-voyagercrate. The Migration struct andMigration::migratefunction are modified to optionally perform contract verification using thesozo-voyagercrate.Cargo.tomlandCargo.lockto include and update dependencies related to the newsozo-voyagercrate, includingreqwest,serde,toml, andurl.Impact
sozo-voyagercrate as a dependency, encapsulating verification logic.migratecommand, allowing users to configure verification settings.Migrationstruct andMigration::migratefunction to accommodate the new verification functionality.sozo-voyagermight introduce compatibility issues in the future if the crate's API changes.Tests
Added to documentation?
Checklist
scripts/rust_fmt.sh,scripts/cairo_fmt.sh)scripts/clippy.sh,scripts/docs.sh)Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes
Chores
Tests
Documentation