feature: add properties for JSON REST responses #204
Conversation
WalkthroughThe pull request introduces structural changes to the Changes
Sequence DiagramsequenceDiagram
participant Client
participant WebServer
participant Prover
Client->>WebServer: Request Commitment
WebServer->>Prover: Retrieve Commitment
Prover-->>WebServer: Return Commitment
WebServer->>WebServer: Wrap in CommitmentResponse
WebServer-->>Client: Return CommitmentResponse
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
⏰ Context from checks skipped due to timeout of 90000ms (2)
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 (
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
crates/common/src/operation.rs (1)
32-37: Consider adding documentation for data size limits.The
SetDataoperation looks good, but it would be helpful to document any size limits for the data field, similar to how they will be enforced invalidate_basic./// Set arbitrary signed data to an existing account. Replaces all existing data. + /// Note: The data size must be less than the maximum allowed size (TBD). SetData { #[serde(with = "raw_or_b64")] data: Vec<u8>, data_signature: SignatureBundle, },crates/node_types/prover/src/webserver.rs (1)
79-82: Add OpenAPI documentation for the response struct.The
CommitmentResponsestruct should include OpenAPI documentation to describe its purpose and usage.#[derive(Serialize, Deserialize, ToSchema)] +/// Response containing the current commitment (tree root) of the IndexedMerkleTree. pub struct CommitmentResponse { + /// The commitment value representing the tree root. commitment: Digest, }crates/common/src/transaction_builder.rs (1)
400-410: Consider extracting common logic between add_ and set_ methods.**The implementation of
set_randomly_signed_datafollows the same pattern asadd_randomly_signed_data. Consider extracting the common logic into a private helper method to reduce code duplication.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
crates/common/src/account.rs(3 hunks)crates/common/src/operation.rs(3 hunks)crates/common/src/transaction_builder.rs(1 hunks)crates/node_types/prover/src/prover/mod.rs(1 hunks)crates/node_types/prover/src/webserver.rs(2 hunks)crates/tree/src/snarkable_tree.rs(1 hunks)crates/tree/src/tests.rs(3 hunks)
🧰 Additional context used
📓 Learnings (2)
crates/common/src/operation.rs (1)
Learnt from: distractedm1nd
PR: deltadevsde/prism#141
File: crates/common/src/operation.rs:311-313
Timestamp: 2024-11-12T11:47:59.930Z
Learning: When implementing `AddSignedData` in `crates/common/src/operation.rs`, ensure that signatures cover the entire operation (including metadata like account ID) to prevent replay attacks or unauthorized data modifications. Including a `value_signature: Option<(VerifyingKey, Signature)>` allows for externally signed data while ensuring proper validation.
crates/common/src/account.rs (1)
Learnt from: distractedm1nd
PR: deltadevsde/prism#141
File: crates/common/src/operation.rs:311-313
Timestamp: 2024-11-12T11:47:59.930Z
Learning: When implementing `AddSignedData` in `crates/common/src/operation.rs`, ensure that signatures cover the entire operation (including metadata like account ID) to prevent replay attacks or unauthorized data modifications. Including a `value_signature: Option<(VerifyingKey, Signature)>` allows for externally signed data while ensuring proper validation.
🪛 GitHub Check: unit-test
crates/tree/src/tests.rs
[failure] 202-202:
no field 1 on type SignedData
[failure] 203-203:
no field 1 on type SignedData
[failure] 224-224:
no field 1 on type SignedData
🪛 GitHub Check: clippy
crates/tree/src/tests.rs
[failure] 202-202:
no field 1 on type prism_common::account::SignedData
[failure] 203-203:
no field 1 on type prism_common::account::SignedData
[failure] 224-224:
no field 1 on type prism_common::account::SignedData
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: unused dependencies
- GitHub Check: integration-test
- GitHub Check: build-and-push-image
🔇 Additional comments (10)
crates/common/src/operation.rs (2)
78-78: LGTM!The
get_public_keymethod correctly handles both data operations consistently.
Line range hint
103-108: Define a proper maximum data size.The TODO comment indicates that a proper maximum data size needs to be determined. This is important for preventing potential DoS attacks through large data submissions.
Consider factors like:
- Network bandwidth limitations
- Storage capacity
- Processing overhead
- Typical use case requirements
crates/common/src/account.rs (3)
12-16: LGTM! Improved struct readability.The change from tuple struct to named fields makes the code more maintainable and self-documenting.
138-141: LGTM! Consistent signature validation.The signature validation for
SetDatafollows the same secure pattern asAddData, ensuring proper verification of externally signed data.
181-188: LGTM! Clear data replacement logic.The implementation correctly replaces all existing data with the new entry.
crates/node_types/prover/src/webserver.rs (1)
240-240: LGTM! Consistent response format.The endpoint now returns a properly structured response object.
crates/tree/src/snarkable_tree.rs (1)
92-95: LGTM! Consistent transaction processing.The
SetDataoperation is correctly integrated into the transaction processing flow, using the same update mechanism as other account modifications.crates/common/src/transaction_builder.rs (1)
423-435:⚠️ Potential issueReplace unwrap() with proper error handling.
The
expectcall on line 408 could panic if key creation fails. Consider propagating the error using theResulttype instead.Apply this diff to improve error handling:
- let value_signing_key = - SigningKey::new_with_algorithm(algorithm).expect("Failed to create value signing key"); + let value_signing_key = SigningKey::new_with_algorithm(algorithm) + .map_err(|e| anyhow::anyhow!("Failed to create value signing key: {}", e))?;Likely invalid or redundant comment.
crates/node_types/prover/src/prover/mod.rs (1)
494-497: LGTM!The addition of
SetDatato the match arm follows the existing pattern and maintains consistent validation logic with other operations.crates/tree/src/tests.rs (1)
162-245: LGTM! Comprehensive test coverage.The test function thoroughly covers:
- Data addition with different signing methods
- Data content verification
- SetData replacement behavior
- Invalid signature handling
🧰 Tools
🪛 GitHub Check: unit-test
[failure] 202-202:
no field1on typeSignedData
[failure] 203-203:
no field1on typeSignedData
[failure] 224-224:
no field1on typeSignedData🪛 GitHub Check: clippy
[failure] 202-202:
no field1on typeprism_common::account::SignedData
[failure] 203-203:
no field1on typeprism_common::account::SignedData
[failure] 224-224:
no field1on typeprism_common::account::SignedData
25a2a9c to
6a5f953
Compare
6a5f953 to
6aef596
Compare
Adds properties to SignedData and Commitment in JSON, which simplifies client-side REST API interaction.
Merge after #198
Summary by CodeRabbit
Release Notes
New Features
CommitmentResponsestruct for more structured API responsesRefactor
SignedDatastruct to use named fields for improved code clarityDocumentation
CommitmentResponsetypeTests