refactor(sdk)!: separate dash core client error #2380
Conversation
WalkthroughThe pull request introduces several modifications across multiple files, primarily enhancing error handling mechanisms. Key changes include the introduction of a new error type, Changes
Possibly related PRs
Suggested labels
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 (
|
lklimek
changed the title
lklimek
force-pushed
the
refactor/dash-core-client-error
branch
from
87c4bcf to
4b6ecf4
Compare
|
@coderabbitai review all |
|
@lklimek, I'll proceed to review all the changes in the PR as requested. ✅ Actions performedFull review triggered. |
![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 (4)
packages/rs-sdk/src/core/dash_core_client.rs (3)
Line range hint
146-159: Update Callers to Handle New Error Type inlist_unspentThe
list_unspentmethod now returnsDashCoreError. Ensure that all callers of this method are updated to handleDashCoreErrorinstead of the previousContextProviderErrorto prevent compilation errors and ensure proper error handling.Run the following script to identify callers of
list_unspent:#!/bin/bash # Description: Find all usages of `list_unspent` in the codebase. rg 'list_unspent\(.*\)'Verify that each usage handles
DashCoreErrorappropriately.
Line range hint
160-168: Update Callers to Handle New Error Type inget_balanceThe
get_balancemethod's return type has changed toResult<Amount, DashCoreError>. Confirm that any code invoking this method is updated to manage the new error type, ensuring consistency in error handling.Search for usages of
get_balance:#!/bin/bash # Description: Find all usages of `get_balance` in the codebase. rg '\.get_balance\(\)'Ensure that error handling for
DashCoreErroris properly implemented in each case.
Line range hint
170-187: Handle Potential Errors inget_quorum_public_keyConversionThe method
get_quorum_public_keyperforms a conversion from a slice to aQuorumHash, which may fail. Ensure that the error handling for this conversion is comprehensive, and consider providing more context in the error message if necessary.Consider enhancing the error message:
- .map_err(|e| DashCoreError::InvalidQuorum(format!("invalid quorum hash: {}", e)))?; + .map_err(|e| DashCoreError::InvalidQuorum(format!("Invalid quorum hash provided: {}", e)))?;packages/rs-sdk/src/mock/provider.rs (1)
218-218: Consider enhancing the Cache implementationThe thread-safe cache implementation looks solid, but could benefit from the following improvements:
- Add validation for the capacity parameter
- Enhance error messages for lock poisoning
- Add usage examples in documentation
Consider applying these improvements:
pub struct Cache<K: Hash + Eq, V> { inner: std::sync::RwLock<lru::LruCache<K, Arc<V>>>, } impl<K: Hash + Eq, V> Cache<K, V> { + /// Creates a new cache with the specified capacity. + /// + /// # Examples + /// + /// ``` + /// use std::num::NonZeroUsize; + /// let cache = Cache::new(NonZeroUsize::new(100).unwrap()); + /// ``` pub fn new(capacity: NonZeroUsize) -> Self { Self { inner: std::sync::RwLock::new(lru::LruCache::new(capacity)), } } pub fn get(&self, k: &K) -> Option<Arc<V>> { - let mut guard = self.inner.write().expect("cache lock poisoned"); + let mut guard = self.inner.write() + .expect("Failed to acquire write lock on cache: lock is poisoned"); guard.get(k).map(Arc::clone) } pub fn put(&self, k: K, v: V) { - let mut guard = self.inner.write().expect("cache lock poisoned"); + let mut guard = self.inner.write() + .expect("Failed to acquire write lock on cache: lock is poisoned"); guard.put(k, Arc::new(v)); }Also applies to: 228-256
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (10)
packages/rs-dpp/src/bls/native_bls.rs(0 hunks)packages/rs-drive-proof-verifier/src/error.rs(1 hunks)packages/rs-sdk/src/core/dash_core_client.rs(8 hunks)packages/rs-sdk/src/core/error.rs(1 hunks)packages/rs-sdk/src/core/mod.rs(1 hunks)packages/rs-sdk/src/error.rs(3 hunks)packages/rs-sdk/src/mock/provider.rs(1 hunks)packages/rs-sdk/src/sync.rs(0 hunks)packages/rs-sdk/tests/fetch/config.rs(1 hunks)packages/rs-sdk/tests/fetch/evonode.rs(0 hunks)
💤 Files with no reviewable changes (3)
- packages/rs-sdk/tests/fetch/evonode.rs
- packages/rs-dpp/src/bls/native_bls.rs
- packages/rs-sdk/src/sync.rs
🔇 Additional comments (11)
packages/rs-sdk/src/core/error.rs (3)
16-27: Well-structured DashCoreError Enum Enhances Error Handling
The DashCoreError enum is well-defined, covering various error scenarios when communicating with Dash Core. Utilizing the thiserror crate provides clear and informative error messages, improving maintainability and debuggability.
29-37: Effective Conversion Implementation to ContextProviderError
The From<DashCoreError> implementation for ContextProviderError ensures seamless integration with existing error handling mechanisms. This promotes consistency across the codebase.
39-56: Correct Implementation of CanRetry Trait for Retry Logic
The CanRetry trait is implemented accurately for DashCoreError. The logic correctly identifies retryable errors based on specific RPC error codes and transport errors, enhancing the robustness of retry mechanisms.
packages/rs-sdk/src/core/mod.rs (1)
9-10: Expose DashCoreError for Improved Error Handling
Introducing the error module and publicly exporting DashCoreError enhances the SDK's ability to handle Dash Core errors effectively. This allows other modules to utilize the specific error types, promoting better error management.
packages/rs-sdk/src/error.rs (3)
49-49: Updated CoreClientError Variant Enhances Error Specificity
Changing the CoreClientError variant to use DashCoreError improves error specificity and aligns with the new error handling structure. This enhances the clarity and maintainability of error management in the SDK.
138-143: Enhanced Retry Logic Incorporates DashCoreError
The can_retry method now includes Error::CoreClientError(e) => e.can_retry(), which appropriately delegates retry logic based on the specifics of DashCoreError. This change improves the resilience of the SDK against transient errors.
130-134: Ensure Comprehensive Conversion from dashcore_rpc::Error
The From<dashcore_rpc::Error> implementation correctly converts to Error using the new DashCoreError type. Verify that all possible variants of dashcore_rpc::Error are appropriately mapped to maintain error handling consistency.
Run the following script to identify any unmapped dashcore_rpc::Error variants:
✅ Verification successful
Let me gather more information about the error types and their handling.
Let me check the DashCoreError implementation to ensure proper error mapping.
Let me try to find the correct location of the DashCoreError definition.
Let me check the DashCoreError implementation in the core error module.
Error Conversion Implementation is Comprehensive and Correct
The implementation of From<dashcore_rpc::Error> for Error is correct and comprehensive. The conversion path is:
dashcore_rpc::Error->DashCoreError::Rpc(in core/error.rs)DashCoreError->Error::CoreClientError(in error.rs)
All variants of dashcore_rpc::Error are properly handled through this chain, and the error handling is consistent with the codebase's usage patterns seen in rs-drive-abci.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all uses of `dashcore_rpc::Error` and check for missing conversions.
rg 'dashcore_rpc::Error::' --glob '!packages/rs-sdk/src/error.rs'
Length of output: 289
Script:
#!/bin/bash
# Check the implementation of Error type and its variants
cat packages/rs-sdk/src/error.rs
Length of output: 6465
Script:
#!/bin/bash
# Check the implementation of DashCoreError
cat packages/rs-sdk/src/core.rs
Length of output: 93
Script:
#!/bin/bash
# Find files containing DashCoreError
rg "DashCoreError" -l
Length of output: 378
Script:
#!/bin/bash
# Check the DashCoreError implementation
cat packages/rs-sdk/src/core/error.rs
Length of output: 1937
packages/rs-sdk/src/core/dash_core_client.rs (2)
190-205: Ensure Robust Handling in get_platform_activation_height
When retrieving the platform activation height, the code assumes the presence of the "mn_rr" key in softforks. If this key is missing, it triggers an ActivationForkError. Confirm that this assumption is valid in all expected environments, or consider handling the absence more gracefully.
Run the following script to check for possible values of softforks in different environments:
✅ Verification successful
Let me search for more context about this fork activation and error handling.
Let me search for more specific information about the mn_rr fork and error handling in the codebase.
Based on the search results, I can now provide a conclusive response about the mn_rr fork check in get_platform_activation_height. The code shows that this fork is a critical platform activation indicator used across multiple components, including Drive ABCI and proof verification.
Error Handling for Platform Activation Height is Appropriate
The current error handling in get_platform_activation_height is correct and necessary because:
- The
mn_rrfork is the official indicator of platform activation in Dash Core - The fork must be both present and active for platform functionality to work correctly
- The error handling is used by critical platform components to verify initialization conditions
The error cases are properly handled:
- Missing fork info triggers
ActivationForkError("no fork info for mn_rr") - Missing height triggers
ActivationForkError("unknown fork height")
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Simulate `get_blockchain_info` responses to verify the presence of `"mn_rr"`.
# Note: Since we cannot run the actual RPC command, review documentation or mock the responses.
echo "Check Dash Core documentation or environments to ensure 'mn_rr' is always present in 'softforks'"
Length of output: 214
Script:
#!/bin/bash
# Search for tests or mock responses related to blockchain info and mn_rr fork
rg -l "mn_rr" --type rust
# Look for related error handling patterns
ast-grep --pattern 'match $_.softforks { $$ }'
# Search for test files containing blockchain info mocks
rg -l "get_blockchain_info.*mock" --type rust
# Look for documentation about mn_rr fork
fd README -e md -X cat {}
Length of output: 65839
Script:
#!/bin/bash
# Look for mn_rr fork references and error handling
rg -A 5 -B 5 "mn_rr" --type rust
# Search for tests related to platform activation height
rg -l "get_platform_activation_height.*test" --type rust
# Look for blockchain info mock responses
rg -A 10 "get_blockchain_info.*mock" --type rust
Length of output: 12309
57-68: Confirm Correct Error Conversion in Retry Logic
In the retry! macro, the error e is converted into DashCoreError using into(). Ensure that all error types produced within the macro can be converted into DashCoreError without loss of information, and that the retry logic operates as intended.
Run the following script to verify that error conversions to DashCoreError are correctly implemented:
✅ Verification successful
Error Conversion in Retry Logic is Properly Implemented
The error conversion in the retry macro is correctly implemented. The DashCoreError enum has a #[from] attribute for dashcore_rpc::Error, which automatically implements the From trait, allowing seamless conversion of RPC errors. The enum also handles other specific error cases (InvalidQuorum, ActivationForkError) that might occur during retry operations. The error types used within the retry macro are properly convertible to DashCoreError.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all error types that need to implement `Into<DashCoreError>`.
rg 'Err\((.*)\)\s+=>\s+\{' -A 1 --no-filename | grep 'let err: DashCoreError = \1.into();'
Length of output: 121
Script:
#!/bin/bash
# Find all error conversions to DashCoreError in the retry macro
rg 'let err: DashCoreError = .+into\(\);' -B 2
# Find all error types used in the retry macro
ast-grep --pattern 'retry!($$$)'
# Find implementations of Into<DashCoreError>
rg 'impl Into<DashCoreError>'
# Find error type definitions that might be converted to DashCoreError
rg 'pub enum .+Error'
Length of output: 6770
Script:
#!/bin/bash
# Find the error types that are actually used within the retry macro
rg -B 5 'let err: DashCoreError = .+into\(\);' packages/rs-sdk/src/core/dash_core_client.rs
# Find implementations of From trait for DashCoreError
rg 'impl From<.+> for DashCoreError'
# Check the DashCoreError definition
rg -A 20 'pub enum DashCoreError' packages/rs-sdk/src/core/error.rs
Length of output: 1110
packages/rs-sdk/src/mock/provider.rs (1)
218-218: Verify error type conversion implementation
The change correctly propagates errors from the core client, but we should verify that there's proper conversion implemented between DashCoreError and ContextProviderError.
✅ Verification successful
Error conversion is properly implemented
The verification confirms that there's a complete From<DashCoreError> implementation for ContextProviderError in core/error.rs that handles all variants of DashCoreError appropriately:
DashCoreError::Rpc→ContextProviderError::DashCoreErrorDashCoreError::InvalidQuorum→ContextProviderError::InvalidQuorumDashCoreError::ActivationForkError→ContextProviderError::ActivationForkError
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for error conversion implementation between DashCoreError and ContextProviderError
# Search for From/Into implementation
ast-grep --pattern 'impl From<DashCoreError> for ContextProviderError'
# Search for error conversion in other ways
rg -l "DashCoreError.*ContextProviderError|ContextProviderError.*DashCoreError"
Length of output: 884
packages/rs-sdk/tests/fetch/config.rs (1)
13-13: LGTM!
Clean removal of unused import.
Issue being fixed or feature implemented
For #2328 , we need to retry on some errors returned by dash core.
What was done?
Built on top of #2328.
How Has This Been Tested?
GHA
Breaking Changes
Checklist:
For repository code-owners and collaborators only
Summary by CodeRabbit
Release Notes
New Features
DashCoreErrorvariant for improved error handling within the SDK.Bug Fixes
get_platform_activation_heightmethod to better propagate errors.Tests
Chores