feat(sdk): consensus error 2 #2275
Conversation
WalkthroughThe pull request introduces modifications to error handling in the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
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 and nitpick comments (1)
packages/rs-dapi-client/src/transport.rs (1)
51-51: LGTM! Addition of 'static bound enables robust error handling.The addition of the 'static bound is a good architectural decision as it:
- Enables downcasting capabilities through the Any trait
- Ensures errors can be safely stored and handled across async boundaries
- Aligns with Rust's standard error handling patterns
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- packages/rs-dapi-client/src/transport.rs (1 hunks)
- packages/rs-sdk/src/error.rs (2 hunks)
🔇 Additional comments (3)
packages/rs-dapi-client/src/transport.rs (1)
51-51: Verify existing TransportClient implementations.Since adding 'static bound is a breaking change, we should verify all existing implementations of TransportClient to ensure their Error types comply with the new constraint.
✅ Verification successful
Let me gather more specific information about the TransportClient implementations to verify their Error types.
The first script will help us find all implementations of TransportClient and their Error type definitions. The second script will show us the trait definition to understand the constraints. The third script will help us understand any custom Error types defined in the transport module. This will allow us to verify if the 'static bound addition is a breaking change.
Let me try a different approach to find the implementations and error types.
This should help us find:
- The TransportClient trait definition and its requirements
- Any implementations of TransportClient
- Files containing Error type definitions that might be related to TransportClient implementations
Let me verify if the Error type being used (dapi_grpc::tonic::Status) already implements the required traits.
This will help us verify if the Error type used in the implementations (tonic::Status) already satisfies the 'static bound and other trait requirements.
Let me check the Mockable trait implementation and verify if tonic::Status is already 'static.
This will help us verify if the Error type (tonic::Status) already implements Mockable and if it's inherently 'static, which would mean the change isn't breaking.
Based on the gathered information, I can now provide a final response:
Adding 'static bound is not a breaking change in this case
The existing implementations of TransportClient use
dapi_grpc::tonic::Statusas their Error type, which:
- Already implements
CanRetry(verified in transport/grpc.rs)- Already implements
Mockable(verified in dapi-grpc/src/mock.rs)- Is inherently 'static as it's a standard error type from tonic
The change is safe because:
- Both existing implementations (PlatformGrpcClient and CoreGrpcClient) use tonic::Status which already satisfies all bounds including 'static
- No other implementations were found that might be affected by this change
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for all TransportClient implementations to verify their Error types ast-grep --pattern 'impl $_ for $type { type Error = $err; $$$ }' # Also search for potential custom Error type definitions rg -l "pub.*enum.*Error|pub.*struct.*Error"Length of output: 31089
Script:
#!/bin/bash # Search for TransportClient implementations and their Error type definitions ast-grep --pattern 'impl $_ for $type { type Error = $err; $$$ }' # Search for TransportClient trait definition ast-grep --pattern 'trait TransportClient { $$$ }' # Search for specific Error type definitions in the transport module rg -A 5 "pub.*enum.*Error|pub.*struct.*Error" packages/rs-dapi-client/src/transport.rsLength of output: 215
Script:
#!/bin/bash # Search for the TransportClient trait and its implementations rg -A 10 "trait TransportClient" packages/rs-dapi-client/src/ # Search for impl blocks that might be implementing TransportClient rg -A 10 "impl.*TransportClient" packages/rs-dapi-client/src/ # Look for Error type definitions in files that might implement TransportClient fd . packages/rs-dapi-client/src/ -t f -x rg -l "type Error.*=.*;"Length of output: 3219
Script:
#!/bin/bash # Check if tonic::Status implements the required traits rg "impl.*CanRetry.*for.*Status" packages/rs-dapi-client/src/ rg "impl.*Mockable.*for.*Status" packages/rs-dapi-client/src/ # Look for trait implementations in the crate where these traits are defined fd . -t f -x rg -l "impl.*for.*Status"Length of output: 872
Script:
#!/bin/bash # Check for Mockable implementation for Status rg -A 5 "impl.*Mockable.*for.*Status" packages/dapi-grpc/src/mock.rs # Check if Status is already marked as 'static in its definition rg "pub.*struct.*Status" packages/dapi-grpc/src/ # Check if the Mockable trait is implemented elsewhere for Status rg "impl.*Mockable.*for.*Status" --type rustLength of output: 496
packages/rs-sdk/src/error.rs (2)
2-9: LGTM: New imports are properly utilizedThe added imports support the enhanced error handling capabilities, particularly for consensus errors.
80-97: Verify the consensus error handling integrationThe implementation correctly extracts and deserializes consensus errors from gRPC metadata. Let's verify the integration with the gRPC server.
| return ConsensusError::deserialize_from_bytes( | ||
| consensus_error_value.as_encoded_bytes(), | ||
| ) | ||
| .map(|consensus_error| { | ||
| Error::Protocol(ProtocolError::ConsensusError(Box::new(consensus_error))) | ||
| }) | ||
| .unwrap_or_else(Error::Protocol); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider enhancing error handling granularity
The current implementation silently falls back to a generic Protocol error if deserialization fails. Consider providing more specific error information in failure cases.
- return ConsensusError::deserialize_from_bytes(
- consensus_error_value.as_encoded_bytes(),
- )
- .map(|consensus_error| {
- Error::Protocol(ProtocolError::ConsensusError(Box::new(consensus_error)))
- })
- .unwrap_or_else(Error::Protocol);
+ return match ConsensusError::deserialize_from_bytes(
+ consensus_error_value.as_encoded_bytes(),
+ ) {
+ Ok(consensus_error) => {
+ Error::Protocol(ProtocolError::ConsensusError(Box::new(consensus_error)))
+ }
+ Err(e) => Error::Protocol(ProtocolError::Generic(format!(
+ "Failed to deserialize consensus error: {}",
+ e
+ ))),
+ };Committable suggestion was skipped due to low confidence.
| if let DapiClientError::Transport(ref t, _) = &value { | ||
| if let Some(status) = (t as &dyn Any).downcast_ref::<dapi_grpc::tonic::Status>() { | ||
| if let Some(consensus_error_value) = | ||
| status.metadata().get_bin("drive-error-data-bin") |
There was a problem hiding this comment.
Currently cbor is inside. We should do it without cbor
|
Close in favor of #2274 |
Issue being fixed or feature implemented
What was done?
How Has This Been Tested?
Breaking Changes
Checklist:
For repository code-owners and collaborators only
Summary by CodeRabbit
New Features
Bug Fixes