Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Walnut: Enable multi-class verification via single API request #2456

Merged
merged 3 commits into from
Sep 23, 2024
Merged

Walnut: Enable multi-class verification via single API request #2456

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
6159fed
Multi-class verification via single API request
barabanovro Sep 20, 2024
a8e1048
Remove futures dependency
barabanovro Sep 20, 2024
476dfa0
Merge remote-tracking branch 'upstream/main' into feat/multi-class-ve…
barabanovro Sep 23, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion Cargo.lock
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 0 additions & 1 deletion crates/sozo/walnut/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@ version.workspace = true
anyhow.workspace = true
console.workspace = true
dojo-world = { workspace = true, features = [ "contracts", "metadata", "migration" ] }
futures.workspace = true
reqwest.workspace = true
scarb.workspace = true
scarb-ui.workspace = true
Expand Down
55 changes: 18 additions & 37 deletions crates/sozo/walnut/src/verification.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@
use console::{pad_str, Alignment, Style, StyledObject};
use dojo_world::metadata::get_default_namespace_from_ws;
use dojo_world::migration::strategy::MigrationStrategy;
use futures::future::join_all;
use reqwest::StatusCode;
use scarb::core::Workspace;
use serde::Serialize;
Expand Down Expand Up @@ -61,50 +60,32 @@
// Collect source code
let source_code = collect_source_code(root_dir)?;

// Prepare verification payloads
let mut verification_tasks = Vec::new();
let mut class_tags = Vec::new();
let mut class_names = Vec::new();
let mut class_hashes = Vec::new();

Check warning on line 64 in crates/sozo/walnut/src/verification.rs

View check run for this annotation

Codecov / codecov/patch

crates/sozo/walnut/src/verification.rs#L63-L64 

Added lines #L63 - L64 were not covered by tests
Comment on lines +63 to +64
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add unit tests for the initialization of class vectors

Ohayo, sensei! The new vectors class_names and class_hashes are essential for the multi-class verification process. Please consider adding tests to ensure they are initialized and populated correctly.

Tools
GitHub Check: codecov/patch

[warning] 63-64: crates/sozo/walnut/src/verification.rs#L63-L64
Added lines #L63 - L64 were not covered by tests


for contract_migration in &migration_strategy.contracts {
let class_name = get_class_name_from_artifact_path(
&contract_migration.artifact_path,
&default_namespace,
)?;
let verification_payload = VerificationPayload {
class_name: class_name.clone(),
class_hash: contract_migration.diff.local_class_hash.to_hex_string(),
rpc_url: rpc_url.clone(),
source_code: source_code.clone(),
};
class_tags.push(contract_migration.diff.tag.clone());
verification_tasks.push(verify_class(verification_payload, &api_url, &api_key));
class_names.push(class_name);
class_hashes.push(contract_migration.diff.local_class_hash.to_hex_string());

Check warning on line 72 in crates/sozo/walnut/src/verification.rs

View check run for this annotation

Codecov / codecov/patch

crates/sozo/walnut/src/verification.rs#L71-L72 

Added lines #L71 - L72 were not covered by tests
}

for class_migration in &migration_strategy.models {
let class_name =
get_class_name_from_artifact_path(&class_migration.artifact_path, &default_namespace)?;
let verification_payload = VerificationPayload {
class_name: class_name.clone(),
class_hash: class_migration.diff.local_class_hash.to_hex_string(),
rpc_url: rpc_url.clone(),
source_code: source_code.clone(),
};
class_tags.push(class_migration.diff.tag.clone());
verification_tasks.push(verify_class(verification_payload, &api_url, &api_key));
class_names.push(class_name);
class_hashes.push(class_migration.diff.local_class_hash.to_hex_string());

Check warning on line 79 in crates/sozo/walnut/src/verification.rs

View check run for this annotation

Codecov / codecov/patch

crates/sozo/walnut/src/verification.rs#L78-L79 

Added lines #L78 - L79 were not covered by tests
}

// Run all verification tasks
let results = join_all(verification_tasks).await;
let verification_payload =
VerificationPayload { class_names, class_hashes, rpc_url, source_code };

Check warning on line 83 in crates/sozo/walnut/src/verification.rs

View check run for this annotation

Codecov / codecov/patch

crates/sozo/walnut/src/verification.rs#L82-L83 

Added lines #L82 - L83 were not covered by tests

for (i, result) in results.into_iter().enumerate() {
match result {
Ok(message) => {
ui.print(subtitle(format!("{}: {}", class_tags[i], message)));
}
Err(e) => {
ui.print(subtitle(format!("{}: {}", class_tags[i], e)));
}
}
// Send verification request
match verify_classes(verification_payload, &api_url, &api_key).await {
Ok(message) => ui.print(subtitle(message)),
Err(e) => ui.print(subtitle(e.to_string())),

Check warning on line 88 in crates/sozo/walnut/src/verification.rs

View check run for this annotation

Codecov / codecov/patch

crates/sozo/walnut/src/verification.rs#L85-L88 

Added lines #L85 - L88 were not covered by tests
Comment on lines +85 to +88
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Propagate errors from the verification request

Ohayo, sensei! Currently, if verify_classes returns an error, it's printed but not propagated. Consider returning the error to allow upstream handling and maintain proper error flow.

Apply this diff to propagate the error:

-match verify_classes(verification_payload, &api_url, &api_key).await {
-    Ok(message) => ui.print(subtitle(message)),
-    Err(e) => ui.print(subtitle(e.to_string())),
-}
+let message = verify_classes(verification_payload, &api_url, &api_key).await?;
+ui.print(subtitle(message));
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.

Suggested change
// Send verification request
match verify_classes(verification_payload, &api_url, &api_key).await {
Ok(message) => ui.print(subtitle(message)),
Err(e) => ui.print(subtitle(e.to_string())),
// Send verification request
let message = verify_classes(verification_payload, &api_url, &api_key).await?;
ui.print(subtitle(message));
Tools
GitHub Check: codecov/patch

[warning] 85-88: crates/sozo/walnut/src/verification.rs#L85-L88
Added lines #L85 - L88 were not covered by tests

}

Ok(())
Expand All @@ -118,18 +99,18 @@

#[derive(Debug, Serialize)]
struct VerificationPayload {
/// The name of the class we want to verify together with the selector.
pub class_name: String,
/// The hash of the Sierra class.
pub class_hash: String,
/// The RPC URL of the network where this class is declared (can only be a hosted network).
/// The names of the classes we want to verify together with the selector.
pub class_names: Vec<String>,
/// The hashes of the Sierra classes.
pub class_hashes: Vec<String>,
Comment on lines +102 to +105
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Update documentation for VerificationPayload structure

Ohayo, sensei! The VerificationPayload now uses vectors for class_names and class_hashes. Please update any associated documentation or comments to reflect these changes for clarity.

/// The RPC URL of the network where these classes are declared (can only be a hosted network).
pub rpc_url: String,
/// JSON that contains a map where the key is the path to the file and the value is the content
/// of the file. It should contain all files required to build the Dojo project with Sozo.
pub source_code: Value,
}

async fn verify_class(
async fn verify_classes(

Check warning on line 113 in crates/sozo/walnut/src/verification.rs

View check run for this annotation

Codecov / codecov/patch

crates/sozo/walnut/src/verification.rs#L113 

Added line #L113 was not covered by tests
payload: VerificationPayload,
api_url: &str,
api_key: &str,
Expand Down
Loading