Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR upgrades CosmWasm from version 2.1 to 3.0, which introduces significant API changes and improvements. The upgrade includes updating all CosmWasm-related dependencies and adapting the codebase to use the new APIs, particularly around error handling and binary serialization functions.
- Dependency version upgrades from CosmWasm 2.1 to 3.0 across all packages
- Migration from deprecated functions to new CosmWasm 3.0 APIs (e.g.,
generic_err→msg,to_binary→to_json_binary) - Refactoring of error handling and verification result structures to improve type safety
Reviewed Changes
Copilot reviewed 33 out of 35 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
Cargo.toml |
Updates workspace dependencies to CosmWasm 3.0 and excludes specific contracts |
packages/cheqd/src/ibc.rs |
Replaces deprecated StdError::generic_err with StdError::msg |
packages/anoncreds/src/tests/types.rs |
Updates binary serialization functions to new CosmWasm 3.0 APIs |
contracts/sdjwt-verifier/src/verifier.rs |
Refactors verification handlers and result structures |
contracts/sdjwt-verifier/src/types.rs |
Changes VerifyResult structure from Result to separate success/error fields |
scripts/schemas.sh |
Updates schema generation command from cargo run to cargo schema |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| ) | ||
| } | ||
|
|
||
| /// Exec message handlers returns Response as data for verification |
There was a problem hiding this comment.
The comment is unclear and grammatically incorrect. It should be 'Exec message handler that returns Response with verification data' or 'Execute message handler that returns verification results as response data'.
| /// Exec message handlers returns Response as data for verification | |
| /// Execute message handler that returns verification results as response data |
| Ok(Response::new().set_data(to_json_binary(&res)?)) | ||
| } | ||
|
|
||
| /// Sudo message handlers returns error if verification fails |
There was a problem hiding this comment.
The comment is grammatically incorrect. It should be 'Sudo message handler that returns error if verification fails' or 'Sudo message handler - returns error if verification fails'.
| /// Sudo message handlers returns error if verification fails | |
| /// Sudo message handler that returns error if verification fails |
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 45 out of 52 changed files in this pull request and generated 3 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| let keys = route_req.issuer_pubkeys.as_ref().map(|jwks| { | ||
| jwks.iter() | ||
| .map(|(_, jwk)| serde_json::to_string(jwk).unwrap()) | ||
| jwks.values() | ||
| .map(|jwk| serde_json::to_string(jwk).unwrap()) | ||
| .collect() | ||
| }); |
There was a problem hiding this comment.
Using unwrap() on serde_json::to_string() can cause panics if serialization fails. Consider using proper error handling with ? operator or map_err() to convert to the appropriate error type.
| SdjwtVerifierError::SdjwtVerifierResultError( | ||
| SdjwtVerifierResultError::VerifiedClaimsToBinaryError(e.to_string()), | ||
| ) |
There was a problem hiding this comment.
The nested error wrapping SdjwtVerifierError::SdjwtVerifierResultError(SdjwtVerifierResultError::VerifiedClaimsToBinaryError(...)) creates an unnecessarily complex error chain. Consider using a direct error variant or flattening the error hierarchy.
| SdjwtVerifierError::SdjwtVerifierResultError( | |
| SdjwtVerifierResultError::VerifiedClaimsToBinaryError(e.to_string()), | |
| ) | |
| SdjwtVerifierError::VerifiedClaimsToBinaryError(e.to_string()) |
| assert!(err | ||
| .to_string() | ||
| .contains(&SdjwtVerifierError::UnauthorisedCaller.to_string())); |
There was a problem hiding this comment.
The error assertion pattern using to_string().contains() is fragile and may break if error message formatting changes. Consider using pattern matching or direct error comparison for more robust tests.
| assert!(err | |
| .to_string() | |
| .contains(&SdjwtVerifierError::UnauthorisedCaller.to_string())); | |
| // Robustly check that the error is the expected contract error | |
| let contract_err = err.root_cause().to_string(); | |
| assert!( | |
| contract_err.contains("UnauthorisedCaller"), | |
| "Expected UnauthorisedCaller error, got: {}", | |
| contract_err | |
| ); |
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 46 out of 52 changed files in this pull request and generated 3 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| @@ -35,5 +35,5 @@ josekit = "0.8.6" | |||
| sd-jwt-rs = "0.7.0" | |||
| jsonwebtoken = { version="9.3.0", features=["use_pem"]} | |||
| avida-test-utils = { path = "../../packages/avida_test_utils/", features = ["sdjwt"]} | |||
There was a problem hiding this comment.
This dependency references avida_test_utils which has been removed/renamed to avida_tests according to the changes in this PR. This will cause a build failure.
| avida-test-utils = { path = "../../packages/avida_test_utils/", features = ["sdjwt"]} | |
| avida_tests = { path = "../../packages/avida_tests/", features = ["sdjwt"]} |
| jwks.values() | ||
| .map(|jwk| serde_json::to_string(jwk).unwrap()) |
There was a problem hiding this comment.
Using unwrap() on serde_json::to_string() can cause panics. Consider using proper error handling instead of unwrapping, especially since this is in a query function that should return meaningful errors.
| serde_json::to_vec(&value) | ||
| .map_err(|e| SdjwtVerifierError::VerifyResultError(e.to_string()))?, |
There was a problem hiding this comment.
This serialization is redundant since the value is already serialized JSON. Consider using to_json_binary(&value) directly or storing the binary representation differently to avoid double serialization.
No description provided.