feat: http client and request builders #235
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Skipped Deployment
|
WalkthroughThis pull request expands the workspace by adding two new crates, Changes
Sequence Diagram(s)sequenceDiagram
participant Client as PrismHttpClient
participant Prism as PrismApi Trait
participant Server
Client->>Prism: get_account(id) / get_commitment()
Prism->>Server: HTTP Request (fetch/post)
Server-->>Prism: JSON Response
Prism-->>Client: Returns Response
sequenceDiagram
participant Builder as RequestBuilder
participant UTX as UnsignedTransaction
participant Signer as SigningKey
participant TX as Transaction
Builder->>UTX: Create unsigned transaction
UTX->>Signer: Request signature
Signer-->>UTX: Provides signature
UTX->>TX: Produce signed transaction
TX-->>Builder: Return signed transaction
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}
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
crates/api/src/api.rs (1)
126-163: 🛠️ Refactor suggestionPrevent potential infinite loops in
wait_with_interval.
Relying on a loop that never exits unlessnoncechanges could lead to hangs in scenarios where finalization is delayed indefinitely. Consider adding a timeout parameter or maximum retry count.
🧹 Nitpick comments (16)
crates/common/src/transaction.rs (2)
12-37: Well-defined approach for separating unsigned and signed transactions.
Encapsulating the transaction signing logic inUnsignedTransactionhelps avoid partial or repeated signatures inTransaction. Ensure you include unit tests that verify a transaction can only be signed once and that the fields (idandnonce) are valid.
80-105: Consider adding more granular variants for transaction errors.
While these errors cover typical issues, situations like a distinct "InvalidSignature" variant may provide more clarity than bundling everything underSigningFailed.crates/client/src/http_client.rs (3)
1-9: Initialize client with optional configuration.
Storingreqwest::Clientis fine. Consider parameterizing the construction of the client, allowing custom timeouts or headers if needed.
11-57: Robust request helpers with potential improvements.
Thefetch,post, andpost_no_responsemethods handle JSON well. However, you might want to log or handle status codes before attempting to deserialize JSON, for improved debugging.
115-125: Refine conversion fromTransactionErrorto HTTP client error.
Mapping allTransactionErrorvariants toPrismHttpClientError::Requestcan obscure the root cause. Consider creating distinct error variants or at least logging the original details.crates/api/src/api.rs (1)
13-124: Trait-driven design is coherent and flexible.
ThePrismApitrait effectively delineates account and transaction operations. You might want to document the expected client-side usages for each method to guide implementers.crates/api/src/builder.rs (4)
1-37: Consider adding doc comments for theRequestBuilderstruct and its methods.
These builders are a key entry point for constructing account-related transactions, so providing doc comments describing usage, parameters, and return types would improve maintainability and clarity.
77-112: Ensure consistent usage of custom errors.
You correctly returnTransactionError::MissingKeywhen no verifying key is provided, but other invalid states (e.g., emptyidorservice_id) might benefit from similarly explicit errors.
295-321: Add potential fallback or error handling for the signing step.
Usingsigning_key.sign(...)can fail under certain key or algorithm conditions. Consider returning a meaningful error instead of unwrapping in deeper calls (e.g., inUnsignedTransaction::sign).
323-343: Provide side effects or confirmation after sending the transaction.
Oncesendcompletes, you create aPendingTransaction. If there are follow-up steps (e.g., logging, user notification, or storing a reference locally), consider integrating them here or exposing a callback.crates/common/src/transaction_builder.rs (3)
260-267: Avoidunwrap()for signing.
If something goes wrong (e.g., invalid key usage),unwrap()will panic. Returning aResultmight be safer.
297-304: Enforce consistent error handling inrevoke_key.
Like inadd_key, consider gracefully propagating signing errors without panicking.
549-556: Ensure safe concurrency when setting data.
If multiple clients set data concurrently for the same accountid, reusingaccount.nonce()can lead to conflicts; consider concurrency strategies.crates/client/src/lib.rs (1)
1-3: Ensurehttp_clientis tested independently.
The new module is essential for client-server communication. Consider adding integration tests to verify requests and responses.Do you want help writing a basic test scaffold that hits a mock server?
crates/api/src/lib.rs (1)
31-39: Consider adding validation constraints to schema example.The schema example is helpful but could be enhanced by adding validation constraints for the hex strings to ensure they are exactly 64 characters long (32 bytes).
#[schema(example = r#"{ - "leaf": "1234567890abcdef1234567890abcdef1234567890abcdef1234567890abcdef", + "leaf": { + "type": "string", + "pattern": "^[a-f0-9]{64}$", + "example": "1234567890abcdef1234567890abcdef1234567890abcdef1234567890abcdef" + }, "siblings": [ - "abcdef1234567890abcdef1234567890abcdef1234567890abcdef1234567890", - "9876543210fedcba9876543210fedcba9876543210fedcba9876543210fedcba" + { + "type": "string", + "pattern": "^[a-f0-9]{64}$", + "example": "abcdef1234567890abcdef1234567890abcdef1234567890abcdef1234567890" + } ] }"#)]crates/node_types/prover/src/webserver.rs (1)
131-141: Consider using anyhow::Result for consistent error handling.The error handling could be simplified by using anyhow's Result type and the ? operator.
- let get_account_result = session.get_account(&request.id).await; - let Ok(account_response) = get_account_result else { - return ( - StatusCode::INTERNAL_SERVER_ERROR, - format!( - "Failed to retrieve account or non-membership-proof: {}", - get_account_result.unwrap_err() - ), - ) - .into_response(); - }; + let account_response = session + .get_account(&request.id) + .await + .map_err(|e| ( + StatusCode::INTERNAL_SERVER_ERROR, + format!("Failed to retrieve account or non-membership-proof: {}", e), + ))?;
📜 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 (16)
Cargo.toml(4 hunks)crates/api/Cargo.toml(1 hunks)crates/api/src/api.rs(1 hunks)crates/api/src/builder.rs(1 hunks)crates/api/src/lib.rs(1 hunks)crates/client/Cargo.toml(1 hunks)crates/client/src/http_client.rs(1 hunks)crates/client/src/lib.rs(1 hunks)crates/common/src/account.rs(2 hunks)crates/common/src/transaction.rs(4 hunks)crates/common/src/transaction_builder.rs(7 hunks)crates/node_types/prover/Cargo.toml(1 hunks)crates/node_types/prover/src/prover/mod.rs(6 hunks)crates/node_types/prover/src/webserver.rs(3 hunks)crates/tree/Cargo.toml(1 hunks)crates/tree/src/proofs.rs(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: clippy
- GitHub Check: unit-test
- GitHub Check: unused dependencies
- GitHub Check: integration-test
- GitHub Check: build-and-push-image
🔇 Additional comments (28)
crates/common/src/transaction.rs (2)
1-2: No issues found with imported traits.
These imports are standard and align with the newly added display logic.
56-69: Validate coverage for signature verification.
Theverify_signaturemethod is straightforward and relies onto_unsigned_txto reconstruct the message. Consider adding or confirming unit tests to ensure tampering withid,operation, ornonceis consistently caught.crates/client/src/http_client.rs (1)
59-74: Implementation ofPrismApiis concise.
These methods are straightforward wrappers around the HTTP helpers and appear to fulfill the trait contract as expected.crates/api/src/api.rs (3)
1-2: Duration import is straightforward.
No issues found.
3-7: Consistent usage ofprism_commontransaction and accounts.
These imports are aligned with the domain logic.
11-12: Builder and response imports look correct.
Ensure the builder usage remains consistent across new operations.crates/api/src/builder.rs (3)
39-112: Validate non-emptyidandservice_id.
In theCreateAccountRequestBuilder, there is no explicit check thatidandservice_idare non-empty. This could lead to invalid account creation if either field is accidentally left blank.Would you like to ensure that empty strings trigger an error?
114-171: Consider adding parallel validation here as inCreateAccountRequestBuilder.
TheRegisterServiceRequestBuilderalso setsidand a verifying key, but there's no explicit check for empty or invalidid. Consistency in validation helps prevent subtle bugs.
173-293: Revisit disallowing a zero nonce invalidate_id_and_nonce.
Currently, the builder fails ifnonce == 0. In some contexts, zero might be a permissible initial nonce. Please confirm this conforms to the intended logical flow.crates/common/src/transaction_builder.rs (4)
7-7: Good addition ofUnsignedTransactionusage.
This helps separate transaction creation from signing, improving clarity and modularity.
129-136: Confirm intentional use of a zero nonce.
Here,nonceis set to0for the newly created service. This may conflict if the serviceidalready exists and expects a nonzero nonce.
199-206: Watch for reuse of account ID with zero nonce.
As with service registrations, consider verifying the account doesn't already exist when usingnonce = 0.
439-446: Double-check thatnonce = account.nonce()is appropriate for data addition.
If the account’s nonce is already incremented elsewhere, overwriting data might result in concurrency or replay issues.crates/client/src/lib.rs (1)
3-14: Re-export choices look solid.
Providing these re-exports under a single client API is convenient for end users. No issues spotted here.crates/api/src/lib.rs (1)
48-55: LGTM! The empty constructor is well implemented.The empty constructor provides a convenient way to create an empty proof with no leaf and empty siblings.
crates/node_types/prover/src/webserver.rs (1)
157-160: LGTM! The commitment endpoint response handling is clean and concise.The match expression provides clear error handling and uses the new CommitmentResponse type effectively.
crates/common/src/account.rs (1)
100-100: LGTM! Signature verification is now more encapsulated.The change to use
tx.verify_signature()?improves encapsulation by moving the signature verification logic into the Transaction struct.crates/tree/src/proofs.rs (1)
222-237: LGTM! The hashed method implementation is clean and efficient.The implementation correctly maps the leaf and sibling hashes from SparseMerkleProof to HashedMerkleProof, maintaining the tree structure.
crates/node_types/prover/src/prover/mod.rs (3)
3-3: LGTM! Import of PrismApi and related types.The import of
PrismApiand related types from the newprism-apicrate aligns with the PR objectives of API redesign.
463-466: LGTM! Method renames improve clarity.The renaming of
get_commitmenttoget_commitment_from_treeandget_accounttoget_account_from_treebetter describes their functionality and source of data.Also applies to: 468-473
512-537: LGTM! Clean PrismApi trait implementation.The implementation of the
PrismApitrait for theProverstruct is well-structured:
- Proper error type definition
- Clear mapping between internal methods and API methods
- Consistent error handling
crates/api/Cargo.toml (1)
1-25: LGTM! Well-structured Cargo.toml for the new API crate.Dependencies are appropriate for an API crate and workspace configuration is correctly set up.
crates/client/Cargo.toml (1)
1-28: LGTM! Well-structured Cargo.toml for the new client crate.Dependencies are appropriate for an HTTP client implementation and workspace configuration is correctly set up.
crates/tree/Cargo.toml (1)
15-15: LGTM! Dependency changes align with API redesign.The addition of
prism-apiand removal ofutoipaaligns with centralizing API definitions in the newprism-apicrate.crates/node_types/prover/Cargo.toml (1)
26-26: LGTM!The addition of the
prism-apidependency is correctly specified using workspace inheritance, aligning with the PR objectives of API redesign.Cargo.toml (3)
19-19: LGTM!The new crates
apiandclientare correctly added to bothdefault-membersandmemberssections, aligning with the PR objectives of API redesign.Also applies to: 21-21, 35-35, 37-37
69-72: LGTM!The HTTP client dependencies are well-chosen:
reqwestis a popular, stable HTTP client libraryurlis the de-facto URL parsing library
134-134: LGTM!The workspace dependencies for the new crates are correctly specified using relative paths.
Also applies to: 136-136
There was a problem hiding this comment.
Struggling to find improvements to this PR. It's great.
The only thing I could ask to refine (which we should do, but maybe in next PR), would be adding rustdoc and eventually creating (http) client tests. Would rather have this merged immediately so it can be used by teams
So I would say we open issues for:
- Updating tutorial in docs + Adding rustdocs, including example usage (high priority, should be done next week)
- Creating integration tests with the http client (can probably be punted back a bit, medium priority)
jns-ps
force-pushed
the
reuse-types-for-client
branch
from
d1ff6ad to
eb1f2ab
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
crates/client/src/http_client.rs (1)
111-113:⚠️ Potential issueHandle cases where
err.status()could beNone.Relying on
expectcan lead to panics if the error doesn't contain a status.- PrismHttpClientError::Status( - err.status().expect("Status error should contain status").into(), - ) + match err.status() { + Some(status) => PrismHttpClientError::Status(status.as_u16()), + None => PrismHttpClientError::Unknown, + }
🧹 Nitpick comments (14)
crates/api/src/api.rs (2)
28-30: Rename or clarify the builder method name.
build_requestis a bit vague. Together with the chainable pattern, it might be clearer to rename it torequest_builder,new_request, or something that implies returning a builder instance.
32-32: Enhance post-transaction feedback.
post_transactionreturns aResult<(), Self::Error>, which may be too minimal. Consider returning a more informative response (e.g., transaction hash or status) to track or debug the transaction.crates/common/src/test_transaction_builder.rs (3)
129-136: Handle signing errors gracefully instead of unwrapping.
Throughout these lines,unwrap()is used immediately after signing unsigned transactions. This will panic on error, even though the library supports fallible operations. For a more robust test suite, consider handling errors with?or returning aResult.-let transaction = unsigned_tx.sign(&signing_key).unwrap(); +let transaction = match unsigned_tx.sign(&signing_key) { + Ok(tx) => tx, + Err(e) => { + // Optionally log or handle the error as needed + panic!("Failed to sign transaction: {:?}", e); + } +};Also applies to: 199-206, 260-267, 297-304, 439-446, 549-556
60-60: Confirm whether the name “TestTransactionBuilder” is sufficiently descriptive.
Previously known asTransactionBuilder, it has been renamed for testing usage. Ensure team members understand its scope is purely test-oriented and it won’t be used in production. If usage in production builds is unintended, consider adding doc comments indicating so.Also applies to: 83-83
426-452: DRY up repeated transaction-building patterns.
Many methods construct anUnsignedTransaction, compute a nonce, sign, and then wrap intoUncommittedTransaction. This pattern is repeated across add, revoke, set, etc. Consider refactoring into a helper function to reduce duplication.crates/api/src/types.rs (2)
1-26: Add clarifications or references for cryptographic fields.
For example,commitment: DigestinCommitmentResponseandid: StringinAccountRequestmight benefit from referencing how these fields are generated or used in the cryptographic flow. This can help integrators or new maintainers.
45-51: Provide partial equality or debugging support forHashedMerkleProofusage.
Currently, there is a straightforward approach (impl Debug, Serialize, Deserialize, ToSchema). If you plan to compare proofs in tests or logs, you might also derive or implement relevant traits. PartialEq is already there, so considerEqif feasible.crates/common/src/transaction.rs (2)
25-36: Consider adding error context to TransactionError.The error handling could be more informative by including the specific cause of encoding or signing failures.
- let bytes = self.encode_to_bytes().map_err(|_| TransactionError::EncodingFailed)?; + let bytes = self.encode_to_bytes().map_err(|e| TransactionError::EncodingFailed(e.to_string()))?;
80-105: Enhance error handling with additional context.The TransactionError enum could be more informative by including error details in its variants.
pub enum TransactionError { InvalidOp(String), InvalidNonce(u64), MissingKey, - EncodingFailed, - SigningFailed, + EncodingFailed(String), + SigningFailed(String), } impl Display for TransactionError { fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result { match self { TransactionError::InvalidOp(msg) => write!(f, "Invalid operation: {}", msg), TransactionError::InvalidNonce(nonce) => write!(f, "Invalid nonce: {}", nonce), TransactionError::MissingKey => write!(f, "Public Key for account is missing"), - TransactionError::EncodingFailed => write!(f, "Encoding transaction failed"), - TransactionError::SigningFailed => write!(f, "Signing transaction failed"), + TransactionError::EncodingFailed(msg) => write!(f, "Encoding transaction failed: {}", msg), + TransactionError::SigningFailed(msg) => write!(f, "Signing transaction failed: {}", msg), } } }crates/client/src/http_client.rs (1)
23-30: Add timeout configuration for HTTP requests.The fetch method should include a configurable timeout to prevent hanging on slow responses.
+ pub fn with_timeout(mut self, timeout: std::time::Duration) -> Self { + self.client = self.client.timeout(timeout); + self + } pub async fn fetch<T>(&self, path: &str) -> Result<T, PrismHttpClientError> where T: DeserializeOwned, { let url = self.join_url(path)?; - let response = self.client.get(&url).send().await?; + let response = self.client + .get(&url) + .send() + .await + .map_err(|e| match e.is_timeout() { + true => PrismHttpClientError::Timeout, + false => e.into(), + })?; response.json::<T>().await.map_err(Into::<PrismHttpClientError>::into) }crates/node_types/prover/src/webserver.rs (2)
146-146: Consider adding response metadata.The account response could include metadata like request timestamp or server version.
- (StatusCode::OK, Json(account_response)).into_response() + let response = AccountResponseWrapper { + data: account_response, + metadata: ResponseMetadata { + timestamp: chrono::Utc::now(), + version: env!("CARGO_PKG_VERSION"), + }, + }; + (StatusCode::OK, Json(response)).into_response()
161-161: Add rate limiting for commitment endpoint.The get_commitment endpoint should include rate limiting to prevent abuse.
Consider implementing rate limiting using a middleware like
tower::limit::RateLimit.crates/node_types/prover/src/prover/mod.rs (1)
516-554: Consider adding error context in PrismApi implementation.The implementation could benefit from more descriptive error contexts to help with debugging.
Apply this diff to improve error handling:
async fn get_account(&self, id: &str) -> Result<AccountResponse, Self::Error> { - let acc_response = match self.get_account_from_tree(id).await? { + let acc_response = match self.get_account_from_tree(id) + .await + .with_context(|| format!("Failed to get account from tree: {}", id))? { Found(account, inclusion_proof) => { let hashed_inclusion_proof = inclusion_proof.hashed(); AccountResponse {Cargo.toml (1)
134-136: Integration of Prism Crate Dependencies.
Addingprism-api = { path = "crates/api" }andprism-client = { path = "crates/client" }in the[workspace.dependencies]section correctly integrates the new crates into the project. Verify that inter-crate imports and usage across the codebase align with these new dependency paths.
📜 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 (24)
Cargo.toml(4 hunks)crates/api/Cargo.toml(1 hunks)crates/api/src/api.rs(1 hunks)crates/api/src/builder.rs(1 hunks)crates/api/src/lib.rs(1 hunks)crates/api/src/types.rs(1 hunks)crates/client/Cargo.toml(1 hunks)crates/client/src/http_client.rs(1 hunks)crates/client/src/lib.rs(1 hunks)crates/common/src/account.rs(2 hunks)crates/common/src/lib.rs(1 hunks)crates/common/src/test_transaction_builder.rs(11 hunks)crates/common/src/transaction.rs(4 hunks)crates/keys/src/signatures.rs(1 hunks)crates/keys/src/verifying_keys.rs(3 hunks)crates/node_types/prover/Cargo.toml(2 hunks)crates/node_types/prover/src/prover/mod.rs(6 hunks)crates/node_types/prover/src/prover/tests.rs(5 hunks)crates/node_types/prover/src/webserver.rs(3 hunks)crates/tests/src/lib.rs(2 hunks)crates/tree/Cargo.toml(0 hunks)crates/tree/src/proofs.rs(1 hunks)crates/tree/src/tests.rs(12 hunks)crates/zk/sp1-script/src/main.rs(5 hunks)
💤 Files with no reviewable changes (1)
- crates/tree/Cargo.toml
✅ Files skipped from review due to trivial changes (1)
- crates/common/src/lib.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- crates/api/src/lib.rs
- crates/client/src/lib.rs
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: clippy
- GitHub Check: unit-test
- GitHub Check: unused dependencies
- GitHub Check: build-and-push-image
- GitHub Check: integration-test
🔇 Additional comments (33)
crates/api/src/api.rs (1)
24-25: Consider documenting potential error conditions.
Bothget_accountandget_commitmentonly expose aResulttype without clarifying (in doc comments) under what conditions an error might occur (e.g., network errors, missing records, etc.). Documenting these error scenarios can improve maintainability and user understanding.Also applies to: 26-27
crates/common/src/test_transaction_builder.rs (1)
37-44: Confirm concurrency assumptions for account key management.
Removing account keys from the builder’s internal map is done in-place. If tests run in parallel, this could lead to data races unless each test obtains its own instance ofTestTransactionBuilder. Confirm your concurrency model before using this code in multi-threaded tests.crates/common/src/transaction.rs (2)
12-21: LGTM! Well-structured UnsignedTransaction implementation.The struct is well-documented and includes all necessary fields for transaction handling.
57-60: LGTM! Robust signature verification.The signature verification is properly implemented using the unsigned transaction data.
crates/keys/src/signatures.rs (1)
16-22: LGTM! Clean removal of Placeholder variant.The Signature enum is now more focused with only the necessary variants.
crates/common/src/account.rs (1)
100-100: LGTM! Good encapsulation of signature verification.The change simplifies the code by moving the signature verification logic into the
Transactionstruct, following the Single Responsibility Principle.crates/tests/src/lib.rs (2)
7-7: LGTM! Good separation of test utilities.The change improves code organization by using a dedicated test transaction builder.
98-98: LGTM! Consistent use of test transaction builder.The change maintains consistency with the import changes while preserving test functionality.
crates/tree/src/proofs.rs (2)
221-236: LGTM! Clear and focused implementation.The new implementation provides a cleaner way to construct
HashedMerkleProofby directly extracting the required hashes.
238-242: LGTM! Simplified struct definition.The struct has been streamlined to contain only the essential fields needed for Merkle proof verification.
crates/node_types/prover/src/prover/tests.rs (2)
2-2: LGTM! Consistent test utility usage.The change maintains consistency with other test files by using the dedicated test transaction builder.
20-20: LGTM! Consistent builder instantiations.All transaction builder instantiations have been updated to use
TestTransactionBuilder, maintaining consistency while preserving test functionality.Also applies to: 39-39, 54-54, 91-91
crates/api/src/builder.rs (3)
11-37: LGTM! Well-structured builder pattern implementation.The
RequestBuilderprovides a clean and intuitive API for creating accounts, registering services, and modifying accounts.
39-112: LGTM! Robust account creation implementation with proper validation.The
CreateAccountRequestBuilderincludes:
- Proper validation of required fields
- Secure challenge signing
- Basic validation of operations
295-321: LGTM! Clean separation of signing and sending concerns.The separation of
SigningTransactionRequestBuilderandSendingTransactionRequestBuilderfollows the single responsibility principle well.Also applies to: 323-343
crates/keys/src/verifying_keys.rs (1)
120-120: LGTM! Improved error messages with signature type information.The error messages now include the specific signature type (ed25519, secp256k1, secp256r1) being verified, which will help with debugging.
Also applies to: 130-130, 140-140
crates/tree/src/tests.rs (1)
4-4: LGTM! Consistent use of TestTransactionBuilder across tests.The change from
TransactionBuildertoTestTransactionBuilderimproves test isolation and makes it clear that these are test-specific transactions.Also applies to: 14-14, 45-45, 65-65, 97-97, 122-122, 146-146, 164-164, 249-249, 288-288, 325-325, 342-342
crates/node_types/prover/src/prover/mod.rs (1)
467-478: LGTM! Improved encapsulation with private helper methods.The renaming of methods to
get_commitment_from_treeandget_account_from_treewith private visibility improves encapsulation.crates/zk/sp1-script/src/main.rs (7)
93-93: LGTM!The function signature has been correctly updated to use
TestTransactionBuilder.
100-100: LGTM!The function signature has been correctly updated to use
TestTransactionBuilder.
107-107: LGTM!The function signature has been correctly updated to use
TestTransactionBuilder.
114-114: LGTM!The function signature has been correctly updated to use
TestTransactionBuilder.
122-122: LGTM!The function parameter has been correctly updated to use
TestTransactionBuilder.
257-257: LGTM!The variable declaration has been correctly updated to use
TestTransactionBuilder.
16-16:✅ Verification successful
Verify test-specific usage of TestTransactionBuilder.
The script is using a test-specific builder. Ensure this is intentional and not meant for production use.
🏁 Script executed:
#!/bin/bash # Description: Check if this file is in a test directory or has test-specific usage. # Test: Search for test-related markers. Expect: Test-related markers or test directory path. rg -l "test|mock|bench" crates/zk/sp1-script/src/main.rsLength of output: 91
Test-specific usage verified.
The grep result confirms that "crates/zk/sp1-script/src/main.rs" contains test-related markers, which validates the intentional use of the test-specific builder (TestTransactionBuilder). No changes are required here.crates/api/Cargo.toml (1)
1-27: LGTM! Package structure aligns with discussed design.The package configuration and dependencies are well-organized. The decision to keep this as a separate crate from
commonwas previously discussed and agreed upon for separation of concerns.crates/client/Cargo.toml (1)
1-31: LGTM! Well-structured package configuration.The package configuration and dependencies are well-organized, with clear categorization of dependencies. The inclusion of HTTP client libraries (
reqwest,url) aligns with the client implementation purpose.crates/node_types/prover/Cargo.toml (2)
16-16: LGTM! Required dependency for async traits.The addition of
async-traitis necessary for implementing async traits in the prover.
27-27: LGTM! Integration with new API crate.The addition of
prism-apialigns with the API redesign, enabling the prover to implement the new API trait.Cargo.toml (4)
19-21: New Crates Added to Workspace Default-members.
The addition of"crates/api"and"crates/client"in thedefault-memberssection ensures that these new crates will be included in workspace builds. Please verify that these crates are correctly configured with their respectiveCargo.tomlfiles and adhere to workspace conventions.
35-37: Updated Workspace Members Array.
Themembersarray now explicitly includes"crates/api"and"crates/client". This update guarantees that the new crates are part of the project’s member list. Ensure that any paths or project-specific configuration settings in the new crates match the overall repository standards.
69-71: Addition of HTTP Client Dependencies.
The inclusion ofreqwest = { version = "0.12", features = ["json"] }andurl = { version = "2.5" }under the[workspace.dependencies]section is important for enabling HTTP requests and URL parsing in the redesigned client API. Confirm that these versions are compatible with other dependencies and that no dependency conflicts arise.
78-78: Async-trait Version Bump.
Upgradingasync-traitfrom"0.1.68"to"0.1.86"can provide improvements and bug fixes for asynchronous traits. Please ensure that the updated version has been tested with all async functionalities across the project, paying special attention to compatibility with the new API implementations.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
Cargo.toml(6 hunks)justfile(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: unused dependencies
- GitHub Check: unit-test
- GitHub Check: clippy
- GitHub Check: build-and-push-image
- GitHub Check: integration-test
🔇 Additional comments (6)
justfile (1)
91-91:❓ Verification inconclusive
Verify the output directory exists.
The build command now outputs to
elf/directory in the repository root. Ensure this directory exists or is created before building.Run this script to check and create the output directory:
🏁 Script executed:
#!/bin/bash # Description: Check if the output directory exists and create it if needed. # Check if elf/ directory exists at repository root if [ ! -d "elf" ]; then echo "Creating elf/ directory..." mkdir -p elf fiLength of output: 20
Ensure the ELF Output Directory Is Set Up Correctly
The build command now directs the output to the
elf/directory at the repository root:cd crates/zk/sp1 && cargo prove build --output-directory ../../../elf/ --elf-name riscv32im-succinct-zkvm-elfA shell script was provided to check and create the directory if it doesn't exist. Note that the script produced no output—this likely means the directory already exists, but it can also make it difficult to distinguish between an already-present directory and an execution issue.
- Action: Manually verify that the
elf/directory exists in the repository root after running the build command (or in your CI environment).- Suggestion: Consider enhancing the script to always log a confirmation message (e.g., “elf/ directory exists” or “elf/ directory created”) to improve clarity during execution.
Cargo.toml (5)
5-7: LGTM!The author list is properly formatted.
19-32: LGTM!The workspace configuration is properly updated to include the new
apiandclientcrates, maintaining consistency betweendefault-membersandmemberssections.Also applies to: 35-50
105-108: LGTM!The secp256k1 features are properly configured for the required functionality.
134-136: LGTM!The new prism crates are properly added to the workspace dependencies.
69-71:❓ Verification inconclusive
Verify reqwest version compatibility.
The reqwest version 0.12 is very recent. Ensure it's stable and compatible with your deployment environment.
🌐 Web query:
What is the latest stable version of reqwest? Are there any known issues with version 0.12?💡 Result:
The latest stable version of reqwest as of February 2025 is v0.12.12 based on changelog entries[1][4][7]. Key updates in the 0.12.x series include HTTP/3 support (under
reqwest_unstable), rustls TLS improvements, and WASM compatibility fixes[1][4][9].Known issues in v0.12.x:
HTTP/2 timeout errors: Some users experience timeouts when using HTTP/2[9].
- Workaround: Force HTTP/1.1 via
ClientBuilder::http1_only()or adjust ALPN settings[9].rustls TLS certificate validation: Issues with self-signed certificates and truncated peer certificates were reported[2][4].
- Partially addressed in v0.12.6 with
danger_accept_invalid_hostnamesoption[4].Proxy handling: Earlier 0.12.x versions had proxy priority conflicts between environment variables[4]:
// Fixed in v0.12.6: HTTP_PROXY now takes precedence over ALL_PROXY ClientBuilder::new().proxy(Proxy::custom(...))Breaking API changes:
- Conversion from
http::Requestnow usesTryFrominstead of infallibleFrom[5][4].- Removed several
Error::is_*inspector methods in v0.10+[4].Recent patches (v0.12.7+) addressed decompression errors with HTTP/2 empty data frames and improved WASM compilation[4]. Users encountering issues should verify TLS configuration and consider constraining to HTTP/1 if experiencing connection problems[2][9].
Citations:
- 1: https://github.com/seanmonstar/reqwest/blob/master/CHANGELOG.md
- 2: Failing to connect with rustls-tls in reqwest 0.12 seanmonstar/reqwest#2191
- 3: https://users.rust-lang.org/t/using-async-std-was-reqwest/32735
- 4: https://docs.rs/crate/reqwest/latest/source/CHANGELOG.md
- 5: https://users.rust-lang.org/t/a-proxy-with-axum-0-7-and-reqwest-0-12-based-on-http-1/112489
- 6: https://cloud.google.com/release-notes
- 7: https://crates.io/crates/reqwest
- 8: https://docs.rs/crate/reqwest/latest/source/src/lib.rs
- 9: A time out error after upgrading from 0.11.x to 0.12.x and using http2 seanmonstar/reqwest#2283
- 10: https://docs.rs/reqwest/
Action Required: Confirm reqwest v0.12 Compatibility with Deployment Environment
The dependency in your Cargo.toml (lines 69–71) specifies:
reqwest = { version = "0.12", features = ["json"] }This aligns with the latest stable release (v0.12.12 as of February 2025). However, please note the following known considerations for the 0.12.x series:
- HTTP/2 Timeouts: Some users report timeout errors when using HTTP/2. If you experience these, consider forcing HTTP/1.1 using
ClientBuilder::http1_only()or review your ALPN settings.- TLS Certificate Validation: There have been issues with self-signed or truncated certificates. If applicable, check your TLS configurations; the
danger_accept_invalid_hostnamesoption was introduced as a partial remedy.- Proxy Handling and API Changes: Earlier versions had proxy priority issues and API adjustments (e.g., conversion changes from infallible
Fromto fallibleTryFrom). Ensure that these do not impact your usage.Please verify that your deployment environment can handle these edge cases or adjust your configurations accordingly.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
.github/workflows/docker.yml (1)
53-57: Consistent Dependency Installation in Docker WorkflowThe newly added "Install dependencies" step correctly installs the required packages (
fontconfigandlibfontconfig1-dev) before the Docker image build step. This helps ensure that all necessary system libraries are present. However, the commands in the run block have extra indentation compared to typical style in GitHub Actions workflows. Consider reducing the leading whitespace for improved readability and consistency with other steps..github/workflows/ci.yml (1)
117-122: Ensure System Dependencies Are Prepared for ClippyThe addition of the "Install dependencies" step in the clippy job is a prudent update. By running an update and installing
fontconfigandlibfontconfig1-devprior to setting up the Rust toolchain with Clippy, you mitigate potential issues arising from missing system libraries required by some dependencies (such as those brought by the new crates).If these installation steps are common across multiple jobs, you might consider refactoring them into a reusable workflow to reduce duplication.
📜 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 (4)
.github/workflows/ci.yml(1 hunks).github/workflows/docker.yml(1 hunks)Cargo.toml(7 hunks)crates/keys/Cargo.toml(0 hunks)
💤 Files with no reviewable changes (1)
- crates/keys/Cargo.toml
🚧 Files skipped from review as they are similar to previous changes (1)
- Cargo.toml
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: integration-test
- GitHub Check: clippy
- GitHub Check: unused dependencies
- GitHub Check: build-and-push-image
- GitHub Check: unit-test
jns-ps
changed the title
Opened issues #237 , #238 , #239 , #240 and #241 for future improvements. |
distractedm1nd
force-pushed
the
reuse-types-for-client
branch
from
cfb6e24 to
d4c1701
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
crates/api/src/builder.rs (1)
280-292:⚠️ Potential issueImprove nonce validation in validate_id_and_nonce.
The current implementation only checks if nonce is zero, which might not be sufficient for preventing replay attacks.
🧹 Nitpick comments (5)
crates/api/src/types.rs (1)
28-43: Consider adding validation for the Merkle proof siblings.The
HashedMerkleProofstruct could benefit from validation of the siblings vector to ensure it maintains a valid Merkle proof structure.impl HashedMerkleProof { + /// Validates that the proof structure is consistent with a binary Merkle tree + pub fn validate(&self) -> bool { + // For a binary Merkle tree, the number of siblings should be log2(n) where n is the number of leaves + self.siblings.len() <= 32 // Assuming max tree depth of 32 for reasonable size + } + pub fn empty() -> Self { Self { leaf: None, siblings: vec![], } } }crates/common/src/transaction.rs (1)
25-36: Consider adding nonce validation in the sign method.The
signmethod could validate the nonce before creating a signed transaction to prevent invalid transactions from being created.pub fn sign(self, sk: &SigningKey) -> Result<Transaction, TransactionError> { + if self.nonce == 0 { + return Err(TransactionError::InvalidNonce(self.nonce)); + } let bytes = self.encode_to_bytes().map_err(|_| TransactionError::EncodingFailed)?; let signature = sk.sign(&bytes);crates/node_types/prover/src/webserver.rs (1)
146-146: Consider adding error details in the success response.While the success case returns a 200 status code, it might be helpful to include additional context or metadata in the response for better observability.
- (StatusCode::OK, Json(account_response)).into_response() + ( + StatusCode::OK, + Json(json!({ + "status": "success", + "data": account_response, + "metadata": { + "timestamp": chrono::Utc::now(), + "version": env!("CARGO_PKG_VERSION"), + } + })) + ).into_response()crates/tree/src/proofs.rs (1)
240-244: Consider adding documentation for HashedMerkleProof.The struct lacks documentation explaining its purpose and usage.
+/// Represents a hashed version of a Merkle proof, containing the leaf hash and sibling hashes. +/// +/// This struct is used to provide a simplified view of a Merkle proof where only the hashed values +/// are needed, rather than the full proof structure. #[derive(Debug, Clone)] pub struct HashedMerkleProof { + /// The hash of the leaf node, if it exists pub leaf: Option<Digest>, + /// The hashes of the sibling nodes in the proof path pub siblings: Vec<Digest>, }crates/common/src/test_transaction_builder.rs (1)
129-136: Consider extracting common transaction creation logic.The pattern of creating and signing an UnsignedTransaction is repeated across multiple methods.
+ fn create_signed_transaction( + &self, + id: String, + operation: Operation, + nonce: u64, + signing_key: &SigningKey, + ) -> Transaction { + let unsigned_tx = UnsignedTransaction { + id, + operation, + nonce, + }; + unsigned_tx.sign(signing_key).unwrap() + }Also applies to: 199-206, 260-267, 297-304, 439-446, 549-556
📜 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 (27)
Cargo.toml(7 hunks)crates/api/Cargo.toml(1 hunks)crates/api/src/api.rs(1 hunks)crates/api/src/builder.rs(1 hunks)crates/api/src/lib.rs(1 hunks)crates/api/src/types.rs(1 hunks)crates/client/Cargo.toml(1 hunks)crates/client/src/http_client.rs(1 hunks)crates/client/src/lib.rs(1 hunks)crates/common/src/account.rs(2 hunks)crates/common/src/lib.rs(1 hunks)crates/common/src/test_transaction_builder.rs(11 hunks)crates/common/src/transaction.rs(4 hunks)crates/keys/Cargo.toml(0 hunks)crates/keys/src/signatures.rs(1 hunks)crates/keys/src/verifying_keys.rs(3 hunks)crates/node_types/prover/Cargo.toml(2 hunks)crates/node_types/prover/src/prover/mod.rs(6 hunks)crates/node_types/prover/src/prover/tests.rs(5 hunks)crates/node_types/prover/src/webserver.rs(3 hunks)crates/tests/src/lib.rs(2 hunks)crates/tree/Cargo.toml(0 hunks)crates/tree/src/proofs.rs(1 hunks)crates/tree/src/tests.rs(12 hunks)crates/zk/sp1-script/Cargo.toml(0 hunks)crates/zk/sp1-script/src/main.rs(5 hunks)justfile(1 hunks)
💤 Files with no reviewable changes (3)
- crates/tree/Cargo.toml
- crates/keys/Cargo.toml
- crates/zk/sp1-script/Cargo.toml
🚧 Files skipped from review as they are similar to previous changes (13)
- crates/api/src/lib.rs
- crates/common/src/lib.rs
- justfile
- crates/tests/src/lib.rs
- crates/node_types/prover/Cargo.toml
- crates/common/src/account.rs
- crates/client/Cargo.toml
- crates/keys/src/verifying_keys.rs
- crates/client/src/lib.rs
- crates/api/Cargo.toml
- crates/tree/src/tests.rs
- crates/node_types/prover/src/prover/tests.rs
- crates/api/src/api.rs
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: clippy
- GitHub Check: unused dependencies
- GitHub Check: integration-test
- GitHub Check: unit-test
- GitHub Check: build-and-push-image
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (19)
crates/api/src/types.rs (1)
5-19: LGTM! Well-structured account request/response types.The types are well-documented and implement appropriate traits for serialization and API schema generation.
crates/common/src/transaction.rs (1)
57-60: LGTM! Robust signature verification.The
verify_signaturemethod correctly verifies the transaction signature by reconstructing the original message.crates/keys/src/signatures.rs (1)
17-24: LGTM! Simplified signature enum.The removal of the Placeholder variant makes the signature handling more robust and explicit.
crates/client/src/http_client.rs (2)
111-113: Handle cases whereerr.status()could beNone.Relying on
expect("Status error should contain status")can lead to unexpected panics if the underlying error doesn't provide a status.- PrismHttpClientError::Status( - err.status().expect("Status error should contain status").into(), - ) + match err.status() { + Some(status) => PrismHttpClientError::Status(status.as_u16()), + None => PrismHttpClientError::Unknown, + }
126-130: Improve error mapping for TransactionError.The current implementation maps all transaction errors to a generic request error, losing valuable context.
impl From<TransactionError> for PrismHttpClientError { - fn from(_: TransactionError) -> Self { - PrismHttpClientError::Request + fn from(err: TransactionError) -> Self { + match err { + TransactionError::InvalidOp(msg) => PrismHttpClientError::InvalidRequest(msg), + TransactionError::InvalidNonce(nonce) => PrismHttpClientError::InvalidRequest(format!("Invalid nonce: {}", nonce)), + _ => PrismHttpClientError::Request, + } } }crates/node_types/prover/src/webserver.rs (2)
4-7: LGTM! Good modularization of API types.The imports from
prism_apimodule show good separation of concerns, moving common API types to a centralized location.
161-161: Consider adding error details in the success response.Similar to the account endpoint, consider adding metadata to the commitment response.
- Ok(commitment_response) => (StatusCode::OK, Json(commitment_response)).into_response(), + Ok(commitment_response) => ( + StatusCode::OK, + Json(json!({ + "status": "success", + "data": commitment_response, + "metadata": { + "timestamp": chrono::Utc::now(), + "version": env!("CARGO_PKG_VERSION"), + } + })) + ).into_response(),crates/tree/src/proofs.rs (1)
223-238: LGTM! Clean implementation of the hashed method.The implementation efficiently extracts leaf hash and sibling hashes from the SparseMerkleProof.
crates/api/src/builder.rs (1)
11-37: LGTM! Well-structured request builder implementation.The RequestBuilder provides a clean interface for creating different types of requests.
crates/common/src/test_transaction_builder.rs (1)
60-67: LGTM! Good renaming to clarify test-specific code.The rename to TestTransactionBuilder better reflects its purpose as a test utility.
crates/node_types/prover/src/prover/mod.rs (4)
2-2: LGTM!The imports are correctly added to support the new
PrismApitrait implementation.Also applies to: 4-7
467-470: LGTM!The method has been correctly renamed and made private, as it's now an internal implementation detail used by the public trait method.
472-477: LGTM!The method has been correctly renamed and made private, with an appropriate return type for the internal implementation.
516-554: LGTM!The
PrismApitrait implementation is well-structured and correctly maps the API methods to internal functionality:
get_accountproperly transforms the internal account response into the API response typeget_commitmentappropriately wraps the internal commitmentpost_transactioncorrectly delegates to the existing validation logiccrates/zk/sp1-script/src/main.rs (1)
16-16:❓ Verification inconclusive
Verify test coverage with the new builder.
The change from
TransactionBuildertoTestTransactionBuilderis consistent across all functions. However, ensure that the test builder provides equivalent functionality and test coverage.Run this script to verify test coverage:
Also applies to: 93-93, 100-100, 107-107, 114-114, 122-122, 159-159, 324-324
🏁 Script executed:
#!/bin/bash # Description: Check test coverage for TestTransactionBuilder # Search for test functions using TestTransactionBuilder echo "Checking test coverage..." rg "fn.*test.*TestTransactionBuilder" -A 5Length of output: 105
Action Required: Manual Test Coverage Verification for TestTransactionBuilder
The static search for test functions using
TestTransactionBuilderdid not yield any results. Please manually verify that test cases covering the functionality of the new builder exist and offer equivalent coverage to those forTransactionBuilder. Specifically, ensure that:
- The new
TestTransactionBuilderis correctly integrated in your tests.- All functions previously covered by tests using
TransactionBuilderhave corresponding tests usingTestTransactionBuilder.The affected locations include the changes at:
crates/zk/sp1-script/src/main.rson lines: 16, 93, 100, 107, 114, 122, 159, 324.Cargo.toml (4)
19-20: LGTM!The new
apiandclientcrates are correctly added to bothdefault-membersandmemberssections.Also applies to: 21-21, 36-37, 38-38
71-74: LGTM!The new dependencies
reqwestandurlare appropriate for implementing HTTP client functionality.
81-81: Verify SP1 version compatibility.The SP1 dependencies have been updated to version 4.1.0, but some patch dependencies still reference version 4.0.0.
Run this script to check for any mismatched SP1 versions:
#!/bin/bash # Description: Check for mismatched SP1 versions in dependencies and patches. # Search for SP1 version references echo "Checking SP1 versions in dependencies and patches..." rg "sp1-[a-z]+" -A 1 rg "patch-.*sp1-[0-9]" -A 1Also applies to: 101-103
139-139: LGTM!The new workspace dependencies
prism-apiandprism-clientare correctly added with appropriate paths.Also applies to: 141-141
The plan is to have a unified way of creating requests towards prism. Currently, this way is via the PrismApi trait. Manually creating transactions is not needed anymore.
Because we are using tokio in prism-api, it was transitively included in prism-tree. This led to compile errors for tokio on sp1.
Before, ecdsa was added to specify "der" feature, which was not re-exported in p256 crate. By now, however, "der" is transitively activated by "serde", which we use already. Therefore, ecdsa can be removed as a 1st class dependency.
Summary by CodeRabbit