Walnut: Enable multi-class verification via single API request #2456
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2456 +/- ##
==========================================
+ Coverage 68.45% 68.47% +0.01%
==========================================
Files 368 368
Lines 48176 48162 -14
==========================================
Hits 32979 32979
+ Misses 15197 15183 -14 ☔ View full report in Codecov by Sentry. |
WalkthroughOhayo, sensei! The recent changes involve updates to the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant VerificationService
participant PayloadHandler
User->>VerificationService: Request verification
VerificationService->>PayloadHandler: Handle payload
PayloadHandler->>VerificationService: Return processed data
VerificationService-->>User: Send verification result
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
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: 3
Outside diff range and nitpick comments (1)
crates/sozo/walnut/src/verification.rs (1)
Line range hint
113-125: Add unit tests forverify_classesfunctionOhayo, sensei! The new
verify_classesfunction is key to the multi-class verification feature. Adding unit tests will help ensure it correctly handles various scenarios, including success and error cases.Tools
GitHub Check: codecov/patch
[warning] 113-113: crates/sozo/walnut/src/verification.rs#L113
Added line #L113 was not covered by tests
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
Files selected for processing (2)
- crates/sozo/walnut/Cargo.toml (0 hunks)
- crates/sozo/walnut/src/verification.rs (2 hunks)
Files not reviewed due to no reviewable changes (1)
- crates/sozo/walnut/Cargo.toml
Additional context used
GitHub Check: codecov/patch
crates/sozo/walnut/src/verification.rs
[warning] 63-64: crates/sozo/walnut/src/verification.rs#L63-L64
Added lines #L63 - L64 were not covered by tests
[warning] 71-72: crates/sozo/walnut/src/verification.rs#L71-L72
Added lines #L71 - L72 were not covered by tests
[warning] 78-79: crates/sozo/walnut/src/verification.rs#L78-L79
Added lines #L78 - L79 were not covered by tests
[warning] 82-83: crates/sozo/walnut/src/verification.rs#L82-L83
Added lines #L82 - L83 were not covered by tests
[warning] 85-88: crates/sozo/walnut/src/verification.rs#L85-L88
Added lines #L85 - L88 were not covered by tests
[warning] 113-113: crates/sozo/walnut/src/verification.rs#L113
Added line #L113 was not covered by tests
Additional comments not posted (3)
crates/sozo/walnut/src/verification.rs (3)
71-72: Ensure correct pairing of class names and hashesOhayo, sensei! When pushing data into
class_namesandclass_hashes, make sure each class name corresponds to the correct class hash. This will prevent any mismatches during the verification process.Tools
GitHub Check: codecov/patch
[warning] 71-72: crates/sozo/walnut/src/verification.rs#L71-L72
Added lines #L71 - L72 were not covered by tests
78-79: Consistency in model class dataOhayo, sensei! Similar to the contracts, verify that model
class_namesandclass_hashesare accurately paired when added to their respective vectors.Tools
GitHub Check: codecov/patch
[warning] 78-79: crates/sozo/walnut/src/verification.rs#L78-L79
Added lines #L78 - L79 were not covered by tests
82-83: Initialization ofVerificationPayloadis appropriateOhayo, sensei! Constructing the
VerificationPayloadwith vectors forclass_namesandclass_hashesaligns with the updated verification approach.Tools
GitHub Check: codecov/patch
[warning] 82-83: crates/sozo/walnut/src/verification.rs#L82-L83
Added lines #L82 - L83 were not covered by tests
| let mut class_names = Vec::new(); | ||
| let mut class_hashes = Vec::new(); |
There was a problem hiding this comment.
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
| /// 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>, |
There was a problem hiding this comment.
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.
| // 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())), |
There was a problem hiding this comment.
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.
| // 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
kariy
changed the title
|
@barabanovro pls rebase on main |
|
@kariy Done! |
Description
This PR optimizes the Walnut verification process, enabling verification through a single API request.
Tests
Added to documentation?
Checklist
scripts/prettier.sh,scripts/rust_fmt.sh,scripts/cairo_fmt.sh)scripts/clippy.sh,scripts/docs.sh)Summary by CodeRabbit