feat(sdk): sdk-level retry logic for fetch and fetch_many
#2266
Conversation
lklimek
force-pushed
the
feat/sdk-retry
branch
from
071d7b2
to
60d220c
Compare
lklimek
changed the base branch from
feat/sdk/retry
to
feat/sdk/execution_info
WalkthroughThe pull request includes several modifications to the Changes
Possibly related PRs
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 review |
✅ Actions performedReview 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: 19
🧹 Outside diff range and nitpick comments (16)
packages/rs-sdk/src/platform/transition/broadcast.rs (2)
29-31: Address TODO comment regarding execution error handlingGiven that this PR's objective is to implement SDK-level retry logic, this TODO comment about better execution error handling should be addressed. This is particularly important for handling higher-level errors like stale node detection (issue #2254).
Would you like assistance with implementing a more robust error handling strategy here? I can help design a solution that:
- Implements proper retry logic for specific error types
- Handles stale node detection
- Provides better error context
Line range hint
38-41: Implement the unused timeout parameterThe
_time_out_msparameter is currently unused. Consider implementing it to provide timeout control for the state transition wait operation.Consider using tokio's timeout:
use tokio::time::{timeout, Duration}; let duration = Duration::from_millis(time_out_ms.unwrap_or(30000)); let result = timeout(duration, async { let request = self.wait_for_state_transition_result_request()?; request .execute(sdk, RequestSettings::default()) .await .into_inner() }).await??;packages/rs-dapi-client/src/request_settings.rs (2)
24-25: Documentation is clear but could be even more helpful.The documentation clearly explains the retry behavior. However, consider enhancing it further with additional details:
Consider expanding the documentation:
- /// Number of retries until returning the last error. `0` means no retries, that is only one request will be sent. - /// 1 means one request and one retry in case of error, etc. + /// Number of retries until returning the last error. `0` means no retries, that is only one request will be sent. + /// 1 means one request and one retry in case of error, etc. + /// Defaults to 5 retries if not specified. + /// + /// Retries are triggered for recoverable errors such as network timeouts and stale node + /// detections. See issue #2254 for more details about error handling.
Line range hint
7-7: Consider adding const assertions for DEFAULT_RETRIES.To prevent potential misconfiguration and ensure reasonable retry limits, consider adding const assertions.
Add const assertions after the constants:
const DEFAULT_RETRIES: usize = 5; +const _: () = assert!(DEFAULT_RETRIES <= 10, "Default retries should not be excessive"); +const _: () = assert!(DEFAULT_RETRIES > 0, "Default retries should allow at least one retry");packages/rs-sdk/src/platform/transition/top_up_identity.rs (2)
50-50: Address or update TODO commentsThese TODO comments about needing better error handling should be addressed as part of this PR since it's specifically focused on improving error handling with retry logic.
Either:
- Remove the TODOs once retry logic is implemented, or
- Update them to be more specific about what additional improvements are needed beyond retry logic
Also applies to: 57-57
Line range hint
1-85: Consider a more comprehensive retry strategyWhile the error handling has been simplified, the current implementation doesn't fully address the PR's objective of implementing SDK-level retry logic for higher-level errors. Consider:
Creating a dedicated retry module that defines:
- Which errors are retryable (e.g., stale node errors)
- Retry policies (attempts, backoff, jitter)
- Error transformation logic
Implementing this retry logic at a higher level, possibly as a wrapper around the DapiRequest trait, to avoid duplicating retry logic across different request types.
This would provide a more maintainable and consistent approach to handling retries across the SDK.
packages/rs-sdk/Cargo.toml (1)
9-9: Consider pinning the exact version ofbackon.While the current version specification
"1.2"is acceptable, consider pinning to an exact version (e.g.,"1.2.0") to ensure reproducible builds. Thebackoncrate is crucial for retry logic, and minor version updates could introduce subtle behavioral changes.packages/rs-sdk/src/platform/transition/put_identity.rs (2)
59-61: TODO comment needs more context.While the error handling change using
into_inner()is good, the TODO comment should be more specific about what improvements are needed for error handling. Consider documenting:
- What specific execution errors need better handling
- What the current limitations are
- What the desired behavior should be
85-87: TODO comment needs clarification and the error handling could be more robust.The comment "make this more reliable" is too vague. Consider:
- Documenting specific reliability concerns
- Defining what "reliable" means in this context
- Outlining potential edge cases that need handling
Also applies to: 91-91
packages/rs-sdk/src/platform/transition/transfer_document.rs (1)
118-118: Document error scenarios and retry behavior for state transition verification.Given the critical nature of state transition verification:
- The TODO comment should specify which execution errors need better handling
- Document the retry strategy for different error scenarios (e.g., network issues vs. invalid proofs)
- Consider adding logging for retry attempts
Consider implementing different retry strategies based on error types:
- Transient errors (network issues): Exponential backoff
- Validation errors (invalid proofs): Fail fast without retries
packages/rs-sdk/src/platform/transition/vote.rs (1)
Line range hint
115-130: Replace string-based error checking with proper error typesThe current implementation relies on string matching (
contains("already exists")) to detect duplicate votes. This approach is fragile and could break if error messages change.Consider:
- Defining proper error types in
rs_dapi_clientfor common cases- Using pattern matching on error types instead of string content
Example approach:
match response_result { Ok(_) => {} Err(DapiClientError::AlreadyExists(_)) => { let vote = Vote::fetch(sdk, VoteQuery::new(voter_pro_tx_hash, vote_poll_id)).await?; vote.ok_or(Error::DapiClientError( "vote was proved to not exist but was said to exist".to_string(), )) } Err(e) => Err(e.into()) }packages/rs-sdk/src/core/transaction.rs (1)
Line range hint
15-186: Consider implementing a comprehensive retry strategy.Given that this PR aims to implement SDK-level retry logic, consider creating a centralized retry strategy that can be reused across different operations. This could include:
- A configurable retry policy with different strategies for different types of operations
- Proper error categorization to determine which errors are retryable
- Circuit breaker pattern for failing fast when upstream services are down
- Metrics collection for retry attempts and failures
This would provide a more robust and maintainable solution for handling transient failures across the SDK.
packages/rs-sdk/src/sdk.rs (1)
122-124: Add documentation for the new field.Consider adding documentation for the
dapi_client_settingsfield to explain its purpose and usage within the SDK's retry logic implementation.+ /// Global settings for DAPI client requests, including retry configuration. + /// These settings are used as defaults for all requests made through the SDK. pub(crate) dapi_client_settings: RequestSettings,packages/rs-dapi-client/src/executor.rs (1)
42-42: Typo in documentation comment: "loosing" should be "losing"There's a typo in the comment on line 42: "Convert inner error type without loosing retries and address." It should be "without losing retries and address."
packages/rs-sdk/src/platform/fetch_unproved.rs (1)
83-83: Consider minimizing unnecessary cloning of the requestThe request is cloned at lines 83 and 91:
...If
Cloneoperations on the request are resource-intensive due to large internal data structures, consider refactoring to avoid unnecessary cloning. Using references or adjusting the ownership model might improve performance without compromising functionality.Also applies to: 91-91
packages/rs-sdk/src/platform/fetch.rs (1)
160-201: Recommend adding unit tests for the retry mechanismConsider adding unit tests to verify that the retry logic functions as expected. Testing should include scenarios for retryable errors, non-retryable errors, maximum retry attempts, and successful execution after retries.
Would you like assistance in generating unit tests for this functionality?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (20)
- packages/rs-dapi-client/Cargo.toml (1 hunks)
- packages/rs-dapi-client/src/executor.rs (4 hunks)
- packages/rs-dapi-client/src/request_settings.rs (1 hunks)
- packages/rs-sdk/Cargo.toml (1 hunks)
- packages/rs-sdk/src/core/transaction.rs (2 hunks)
- packages/rs-sdk/src/platform/fetch.rs (4 hunks)
- packages/rs-sdk/src/platform/fetch_many.rs (4 hunks)
- packages/rs-sdk/src/platform/fetch_unproved.rs (2 hunks)
- packages/rs-sdk/src/platform/transition/broadcast.rs (3 hunks)
- packages/rs-sdk/src/platform/transition/purchase_document.rs (3 hunks)
- packages/rs-sdk/src/platform/transition/put_contract.rs (3 hunks)
- packages/rs-sdk/src/platform/transition/put_document.rs (3 hunks)
- packages/rs-sdk/src/platform/transition/put_identity.rs (4 hunks)
- packages/rs-sdk/src/platform/transition/top_up_identity.rs (2 hunks)
- packages/rs-sdk/src/platform/transition/transfer_document.rs (3 hunks)
- packages/rs-sdk/src/platform/transition/update_price_of_document.rs (3 hunks)
- packages/rs-sdk/src/platform/transition/vote.rs (4 hunks)
- packages/rs-sdk/src/sdk.rs (4 hunks)
- packages/rs-sdk/src/sync.rs (3 hunks)
- packages/rs-sdk/tests/fetch/epoch.rs (2 hunks)
🔇 Additional comments (37)
packages/rs-dapi-client/Cargo.toml (1)
22-22: LGTM! Adding tokio-sleep feature to enable async retry capabilities.The addition of the
tokio-sleepfeature tobackonis appropriate for implementing SDK-level retry logic, as it provides the necessary async retry capabilities while maintaining compatibility with the existing tokio runtime.Let's verify the usage of backon's retry functionality:
✅ Verification successful
The search results show that the backon dependency is being properly used in the rs-dapi-client codebase for implementing retry logic. The code shows:
- The backon crate is imported and used with its ConstantBuilder for retry configuration
- The retry settings are configured with:
- Max retries from applied_settings
- A constant delay of 10ms between retries
- The tokio-sleep feature is required for the async retry functionality
I can now generate the final response:
✓ The tokio-sleep feature addition is correctly implemented and used
The backon dependency with tokio-sleep feature is properly integrated into the retry logic implementation, providing the necessary async retry capabilities with constant backoff strategy.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that backon's retry functionality is being used in the codebase # Expected: Find retry implementations using backon # Search for retry-related imports and usage rg "use backon::(BackoffBuilder|Retryable|retry)" -A 5 # Search for specific retry implementations ast-grep --pattern 'BackoffBuilder::default().$$$'Length of output: 109
Script:
#!/bin/bash # Let's try a broader search for backon usage rg "backon" -l # Search for any retry-related terms to catch different import patterns rg "BackoffBuilder|Retryable|retry" -A 5 # Check the src directory specifically for retry implementations fd -e rs . packages/rs-dapi-client/src -x cat {}Length of output: 208541
Script:
#!/bin/bash # Let's search for backon usage in the rs-dapi-client codebase rg "use backon" packages/rs-dapi-client/src -A 5 # Search for retry implementation details ast-grep --pattern 'retry($_, $_, $_).await' # Search for retry configuration rg "ConstantBuilder::default\(\)" -A 5 packages/rs-dapi-client/srcLength of output: 1143
packages/rs-sdk/src/platform/transition/broadcast.rs (1)
10-10: LGTM: Import of IntoInner traitThe addition of the
IntoInnertrait import aligns with the PR's objective to improve error handling at the SDK level.packages/rs-sdk/src/platform/transition/top_up_identity.rs (2)
14-14: LGTM: Import change aligns with new error handling approachThe addition of
IntoInnertrait import is appropriate for the new error handling implementation.
57-58:⚠️ Potential issueEnsure consistent retry behavior across requests
This request should implement the same retry mechanism as the first request to maintain consistency in error handling behavior.
Let's verify if there are other similar patterns in the codebase that would need the same retry logic:
packages/rs-sdk/Cargo.toml (2)
9-9: LGTM on the retry logic implementation approach.The addition of
backonis a good choice for implementing SDK-level retry logic:
- It's a mature and well-maintained crate for async retry operations
- The
tokio-sleepfeature ensures proper async backoff behavior- This aligns well with the PR objective of handling higher-level errors
9-9: Verify feature coordination with rs-dapi-client.The addition of
backonwithtokio-sleepfeature and removal of the SDK'stokio-sleepfeature suggests a refactor of retry logic. Let's verify this coordination.✅ Verification successful
No coordination issues with rs-dapi-client's retry logic
The search results show that rs-dapi-client implements its own retry mechanism using a
ConstantBuilderand doesn't rely on tokio-sleep features directly. The addition ofbackonwithtokio-sleepfeature in rs-sdk is independent and doesn't affect rs-dapi-client's retry implementation, which uses its own retry policy and error handling throughcan_retry()trait implementations.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the coordination of tokio-sleep feature between packages # Test 1: Check if rs-dapi-client's Cargo.toml has tokio-sleep in default features echo "Checking rs-dapi-client's default features..." rg -A 5 'default.*=.*\[' packages/rs-dapi-client/Cargo.toml # Test 2: Check if there are any remaining references to tokio-sleep feature echo "Checking for remaining tokio-sleep feature references..." rg 'tokio-sleep.*=.*\[.*rs-dapi-client' -g 'Cargo.toml'Length of output: 464
Script:
#!/bin/bash # Let's check for any usage of backon and tokio-sleep features across the codebase # Check for backon usage in rs-sdk echo "Checking backon usage in rs-sdk..." rg "use backon" packages/rs-sdk/ -A 3 # Check for tokio-sleep feature usage echo "Checking tokio-sleep feature usage..." rg "tokio-sleep" -g "*.rs" # Check if rs-dapi-client has any retry logic that might be affected echo "Checking retry logic in rs-dapi-client..." rg "retry|backoff" packages/rs-dapi-client/src/ -A 3Length of output: 5843
packages/rs-sdk/src/platform/transition/put_identity.rs (2)
17-17: LGTM! Import change aligns with SDK-level retry logic implementation.The addition of
IntoInnertrait is consistent with the PR's objective to improve error handling at the SDK level.
Line range hint
91-102: Verify potential race condition in identity verification.The current implementation might be susceptible to race conditions when handling
AlreadyExistserrors. Consider:
- The time gap between receiving
AlreadyExistsand fetching the identity- The identity might be deleted between these operations
packages/rs-sdk/src/platform/transition/put_contract.rs (2)
21-21: LGTM: Import addition aligns with error handling improvements.The addition of
IntoInnertrait import supports the simplified error handling approach implemented in this PR.
90-90: Verify error handling completeness for higher-level errors.While simplifying error handling with
into_inner()is good, please ensure this change doesn't lose important error context, especially for higher-level errors like stale node detection mentioned in issue #2254.Consider documenting the error types that can be returned and how they're handled, particularly for:
- Transport-level errors (handled by rs-dapi-client)
- Higher-level errors (stale node, etc.)
Would you like assistance in documenting the error handling strategy or implementing comprehensive error type mapping?
packages/rs-sdk/tests/fetch/epoch.rs (2)
15-15: LGTM: Import addition is consistent with the new error handling approach.The addition of
IntoInnerto the imports aligns with the simplified error handling implementation.
29-29: Consider improving error handling and messaging.While using
expectin test code is common, there are a few potential improvements to consider:
- The error message could be more descriptive, e.g., "failed to get identity response"
- As noted in the TODO comment, a more robust error handling approach might be beneficial, especially if this pattern is used in other tests
Let's check if this pattern is used elsewhere in the tests:
packages/rs-sdk/src/platform/transition/update_price_of_document.rs (1)
21-21: LGTM: Import addition aligns with SDK-level retry logic implementation.The addition of
IntoInnertrait import is consistent with the PR's objective to implement SDK-level retry logic and matches similar changes across other files.packages/rs-sdk/src/platform/transition/transfer_document.rs (3)
21-21: LGTM: Import addition supports SDK-level retry logic.The addition of
IntoInnertrait aligns with the PR's objective of implementing retry logic at the SDK level.
Line range hint
100-118: Consider adding tests for retry scenarios and security implications.The retry logic implementation touches critical blockchain operations. Please ensure:
- Add tests covering different retry scenarios:
- Network timeouts
- Invalid state transitions
- Stale node responses
- Document security considerations:
- Maximum retry attempts
- Timeout configurations
- Impact on transaction finality
Let's check for existing retry-related tests:
#!/bin/bash # Search for retry-related tests rg -i "test.*retry|retry.*test" packages/rs-sdk/src/
100-100: Please enhance the TODO comment and document retry behavior.The TODO comment about better error handling needs more context. Consider:
- Documenting specific scenarios that need better handling
- Adding details about the current retry behavior with
into_inner()- Explaining what improvements are planned
Let's check for similar TODOs in related files:
packages/rs-sdk/src/platform/transition/put_document.rs (3)
20-20: LGTM: Import addition supports improved error handling.The addition of
IntoInnertrait import is consistent with the improved error handling implementation below.
Line range hint
98-118: Address TODOs and implement comprehensive retry strategy.The current changes simplify error handling, but there are still TODO comments indicating areas needing improvement. Consider:
- Implementing a comprehensive retry strategy for both execution points
- Documenting the retry behavior (attempts, backoff strategy, etc.)
- Addressing the TODO comments about better error handling
This would fully align with the PR's objective of implementing SDK-level retry logic for handling higher-level errors.
Let's check for other TODO comments related to error handling:
#!/bin/bash # Search for TODO comments related to error handling rg -i "todo.*error" packages/rs-sdk/
117-117: 🛠️ Refactor suggestionEnsure consistent retry behavior here.
Similar to the previous change, this is another point where SDK-level retry logic should be implemented. Ensure that the retry strategy is consistent across both request execution points.
Let's check for other similar patterns in the codebase that might need the same retry logic:
Consider implementing the same retry pattern here:
retry.retry(|| { request .execute(sdk, RequestSettings::default()) .await .into_inner() }).await?packages/rs-sdk/src/platform/transition/purchase_document.rs (3)
22-22: LGTM: Import addition aligns with error handling improvements.The addition of
IntoInnertrait import supports the SDK-level retry logic implementation.
Line range hint
1-180: Consider comprehensive retry strategy implementation.While the changes introduce basic error handling improvements, the implementation doesn't fully address the PR's objective of implementing SDK-level retry logic. Consider:
- Defining a consistent retry strategy at the SDK level (possibly in a separate module)
- Identifying all operations that need retry logic (network calls, state transitions, etc.)
- Implementing proper error typing to distinguish between retryable and non-retryable errors
- Adding configuration options for retry policies (attempts, backoff, etc.)
This would provide a more robust solution for handling various types of failures, including the stale node detection issue (#2254).
Let's check the current retry implementations in the codebase:
#!/bin/bash # Search for existing retry implementations rg "retry|backoff|ExponentialBuilder" --type rust # Search for retry-related configurations rg "RetryConfig|retry_policy|max_attempts" --type rust
104-106: TODO comment needs elaboration.The TODO comment "We need better way to handle execution errors" is vague. Since this PR implements SDK-level retry logic, please specify:
- What specific execution errors need better handling?
- How should these errors be handled differently?
- Are there specific retry strategies needed for different error types?
Let's check for similar TODO patterns in the codebase:
packages/rs-sdk/src/platform/transition/vote.rs (2)
20-20: LGTM: Import of IntoInner traitThe addition of
IntoInnertrait import aligns with the new error handling approach used throughout the file.
115-115: Consolidate duplicate TODO comments about execution handlingThere are multiple identical TODO comments about needing a better way to handle execution response and errors. Since this PR implements SDK-level retry logic, these TODOs should be addressed consistently across the codebase.
Let's analyze the scope of this issue:
#!/bin/bash # Find all similar TODO comments about execution handling in the SDK rg -B 2 -A 2 "TODO.*better way to handle execution" packages/rs-sdk/Also applies to: 137-137
packages/rs-sdk/src/core/transaction.rs (1)
15-15: LGTM: Import addition aligns with error handling improvements.The addition of
IntoInnerto the imports is consistent with the PR's objective to improve error handling at the SDK level.packages/rs-sdk/src/sdk.rs (2)
140-140: LGTM!The Clone implementation correctly includes the new
dapi_client_settingsfield.
965-965: LGTM! Verify settings initialization.The
dapi_client_settingsfield is correctly initialized from the builder's settings in both mock and non-mock paths.Also applies to: 1028-1028
✅ Verification successful
Based on the search results, I can see that
RequestSettingsis properly used across the codebase. The settings initialization in both mock and non-mock paths is consistent with the codebase's patterns, whereRequestSettingsis used to configure request behavior like timeouts, retries, etc. The settings are correctly passed from the builder to the SDK instance.LGTM! Settings initialization verified.
The
dapi_client_settingsfield is properly initialized from the builder's settings in both paths, following the standard pattern used throughout the codebase for handling request settings.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that RequestSettings is properly used across the codebase # Test: Search for RequestSettings usage to ensure consistency rg -A 5 'RequestSettings' # Test: Search for retry-related configurations ast-grep --pattern 'settings: RequestSettings'Length of output: 73332
packages/rs-sdk/src/platform/fetch_unproved.rs (5)
4-4: ImportingretryfunctionThe addition of
retryfromcrate::syncis appropriate for implementing the retry logic in thefetch_unproved_with_settingsmethod.
12-12: Importing types for enhanced error handlingThe import of
ExecutionError,ExecutionResponse, andIntoInnerfromrs_dapi_clientis necessary for the updated error handling and response processing mechanisms.
75-107: Implementation of retry logic infetch_unproved_with_settingsThe introduction of the retry mechanism using a closure and the
retryfunction enhances the robustness of the data fetching process. The use ofExecutionResponseandExecutionErrorprovides better context for error handling and ensures that retries are managed effectively.
96-100: Consistent error mapping with additional contextThe mapping of errors into
ExecutionErrorincludes theaddressandretries, which is beneficial for debugging. Ensure that all relevant error information is preserved to aid in troubleshooting.
109-110: Proper application of settings and retry mechanismOverriding the SDK's default DAPI client settings with the provided
settingsensures that the retry logic respects custom configurations. Invokingretry(settings, closure).await.into_inner()correctly applies the retry mechanism and unwraps the final result.packages/rs-sdk/src/platform/fetch_many.rs (1)
44-47: Imports Updated AppropriatelyThe necessary imports from
rs_dapi_clienthave been added, includingExecutionError,ExecutionResponse,IntoInner, andRequestSettings, which are essential for the new retry logic and error handling.packages/rs-sdk/src/platform/fetch.rs (4)
124-126: Refactorfetch_with_metadatato delegate tofetch_with_metadata_and_proofThe method
fetch_with_metadatanow delegates its functionality tofetch_with_metadata_and_proof, which streamlines the code and promotes reuse, reducing potential duplication.
175-182: Ensure comprehensive error context when parsing proofsWhen mapping errors in
sdk.parse_proof_with_metadata_and_proof, the construction ofExecutionErrorincludesinner,address, andretries. Verify that all relevant error information is captured to aid in debugging and error resolution.
189-193: Validate the construction ofExecutionResponseThe
ExecutionResponseis constructed withinner,address, andretries. Ensure that this encapsulation accurately represents the execution outcome and that theinnervalue contains the expected data for downstream processing.
197-201: Integrate retry logic effectively with the fetch operationThe implementation appropriately integrates the
retrymechanism by applying it to the asynchronous closurefut. This ensures that transient errors are retried according to the specifiedRequestSettings, enhancing the robustness of the fetch operation.
shumkov
changed the title
fetch and fetch_many
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (6)
packages/rs-dapi-client/src/executor.rs (2)
23-36: LGTM! Consider adding examples to trait documentation.The traits are well-defined with clear documentation about their behavior, particularly the warning about lossy operations in
IntoInner.Consider adding usage examples to the trait documentation to illustrate the difference between lossy and lossless conversions:
/// Example: /// ``` /// let error = ExecutionError { inner: "error", retries: 1, address: Some(addr) }; /// let inner: &str = error.into_inner(); // loses retries and address /// ```
143-163: LGTM! Consider documenting error handling behavior.The implementations correctly handle both success and error cases while maintaining type safety.
Consider adding documentation to clarify how errors are handled during conversion:
/// Converts the ExecutionResult into a basic Result, dropping execution context. /// /// # Example /// ``` /// let result: ExecutionResult<String, Error> = ...; /// let basic_result: Result<String, Error> = result.into_inner(); /// ```packages/rs-dapi-client/src/mock.rs (1)
294-317: LGTM with suggestion: Consider strengthening the test-only warning.The
FromInnertrait and its implementation forExecutionResponseare well-designed and documented. However, since this is intended for test use only, consider making this warning more prominent.Apply this change to strengthen the warning:
/// Create full wrapping object from inner type, using defaults for /// fields that cannot be derived from the inner type. +#[cfg_attr(doc, doc = "# Warning")] +/// +/// **This trait is intended for test use only!** Using this trait in production code +/// may lead to incorrect behavior as it uses default values for important fields. pub trait FromInner<R> where Self: Default,packages/rs-sdk/src/sync.rs (3)
167-169: Consider making the delay configurableThe backoff delay is hardcoded to 10ms with a comment suggesting it's not needed. If the delay isn't needed, consider removing it or making it configurable through the
RequestSettings.
194-194: Fix inconsistent spacingThere are inconsistent spaces around operators in this line.
- let result= ::backon::Retryable::retry(closure,backoff_strategy) + let result = ::backon::Retryable::retry(closure, backoff_strategy)
200-200: Fix typo in commentThe word "preceeding" should be "preceding".
- // requests sent in all preceeding attempts; user expects `settings.retries +1` + // requests sent in all preceding attempts; user expects `settings.retries + 1`
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (19)
- packages/rs-dapi-client/src/executor.rs (4 hunks)
- packages/rs-dapi-client/src/mock.rs (3 hunks)
- packages/rs-dapi-client/src/request_settings.rs (1 hunks)
- packages/rs-sdk/src/core/transaction.rs (2 hunks)
- packages/rs-sdk/src/error.rs (2 hunks)
- packages/rs-sdk/src/platform/fetch.rs (4 hunks)
- packages/rs-sdk/src/platform/fetch_many.rs (4 hunks)
- packages/rs-sdk/src/platform/fetch_unproved.rs (2 hunks)
- packages/rs-sdk/src/platform/transition/broadcast.rs (3 hunks)
- packages/rs-sdk/src/platform/transition/purchase_document.rs (3 hunks)
- packages/rs-sdk/src/platform/transition/put_contract.rs (3 hunks)
- packages/rs-sdk/src/platform/transition/put_document.rs (3 hunks)
- packages/rs-sdk/src/platform/transition/put_identity.rs (4 hunks)
- packages/rs-sdk/src/platform/transition/top_up_identity.rs (2 hunks)
- packages/rs-sdk/src/platform/transition/transfer_document.rs (3 hunks)
- packages/rs-sdk/src/platform/transition/update_price_of_document.rs (3 hunks)
- packages/rs-sdk/src/platform/transition/vote.rs (4 hunks)
- packages/rs-sdk/src/sync.rs (3 hunks)
- packages/rs-sdk/tests/fetch/epoch.rs (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (12)
- packages/rs-dapi-client/src/request_settings.rs
- packages/rs-sdk/src/core/transaction.rs
- packages/rs-sdk/src/platform/fetch.rs
- packages/rs-sdk/src/platform/transition/broadcast.rs
- packages/rs-sdk/src/platform/transition/purchase_document.rs
- packages/rs-sdk/src/platform/transition/put_contract.rs
- packages/rs-sdk/src/platform/transition/put_document.rs
- packages/rs-sdk/src/platform/transition/put_identity.rs
- packages/rs-sdk/src/platform/transition/top_up_identity.rs
- packages/rs-sdk/src/platform/transition/transfer_document.rs
- packages/rs-sdk/src/platform/transition/vote.rs
- packages/rs-sdk/tests/fetch/epoch.rs
🔇 Additional comments (15)
packages/rs-sdk/src/error.rs (2)
8-8: LGTM: Import aligns with SDK retry logic implementation.The addition of
ExecutionErrorto the existing rs_dapi_client imports is appropriate and consistent with the PR's objective of implementing SDK-level retry logic.
Line range hint
100-104: Verify retry behavior for execution errors.The current
CanRetryimplementation might need to be updated to handle retryable cases fromExecutionError. Let's verify the retry behavior in the codebase.✅ Verification successful
Let me analyze the retry behavior implementations found in the codebase.
The results show that
ExecutionErroralready has a properCanRetryimplementation inpackages/rs-dapi-client/src/executor.rsthat delegates to its inner error type:impl<E: CanRetry> CanRetry for ExecutionError<E> { fn can_retry(&self) -> bool { self.inner.can_retry() } }This means that
ExecutionErrorwill correctly handle retries based on its wrapped error type. The current implementation inError::can_retry()that handlesStaleNodeandTimeoutReachedis appropriate since:
ExecutionErrordelegates retry behavior to its inner error- Other error types in the codebase (like
DapiClientErrorand gRPCStatus) have their own retry implementations- No evidence suggests that
ExecutionErrorintroduces new retry cases that need handling inError::can_retry()No changes needed for retry behavior.
The retry mechanism is properly implemented through type delegation, ensuring consistent retry behavior across the error hierarchy.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check retry behavior implementation across the codebase # Check for retry-related implementations echo "Checking retry implementations:" rg -A 5 "impl.*CanRetry" # Check for execution error handling patterns echo "Checking execution error handling:" rg -A 5 "ExecutionError.*can_retry"Length of output: 2189
packages/rs-dapi-client/src/executor.rs (2)
50-62: LGTM! Clean implementation of lossless error conversion.The implementation correctly preserves all context (retries and address) while converting the inner error type.
102-124: LGTM! Consistent implementation pattern.The implementations of
IntoInnerandInnerIntoforExecutionResponsefollow the same well-structured pattern asExecutionError, providing both lossy and lossless conversion options.packages/rs-sdk/src/platform/transition/update_price_of_document.rs (2)
21-21: LGTM: Import addition aligns with error handling improvements.The addition of
IntoInnertrait import supports the simplified error handling approach used throughout the file.
101-101: 🛠️ Refactor suggestionConsider implementing retry logic for broadcast operations.
While the simplified error handling with
into_inner()is acceptable, consider implementing retry logic for broadcast operations to align with the PR's objective of SDK-level retry handling. This would help handle transient failures during document price updates.Example implementation:
- .into_inner()?; + .with_retry(|| { + backoff::ExponentialBackoffBuilder::new() + .with_max_elapsed_time(Some(std::time::Duration::from_secs(30))) + .build() + }) + .into_inner()?;Likely invalid or redundant comment.
packages/rs-dapi-client/src/mock.rs (2)
16-17: LGTM: Import changes align with PR objectives.The added imports support the implementation of SDK-level retry logic and are used appropriately throughout the file.
77-82: LGTM: Improved type constraints in load method.The signature change improves flexibility while maintaining type safety by moving constraints to the where clause. This is a good practice as it makes the method signature cleaner and more maintainable.
packages/rs-sdk/src/sync.rs (3)
7-15: LGTM! Imports are well-organized and appropriate.The new imports provide the necessary components for implementing the retry logic, including atomic reference counting and error handling.
156-229: LGTM! Retry implementation is robust and well-documented.The implementation correctly:
- Handles the total number of retries across all layers
- Uses atomic operations for thread safety
- Provides detailed logging for debugging
- Properly manages mutable state using Arc and Mutex
348-369: LGTM! Test implementation is comprehensive.The test implementation:
- Uses test_matrix to cover multiple retry scenarios
- Properly tracks request counts using atomic operations
- Verifies the expected number of requests
- Includes multiple iterations for stability testing
packages/rs-sdk/src/platform/fetch_unproved.rs (2)
75-111: LGTM!The implementation of the retry logic in the
fetch_unproved_with_settingsmethod enhances error handling and robustness. The use of a closure with theretryfunction is appropriate and integrates well with the existing codebase.
75-111: Consider adding tests for the retry mechanismTo ensure the retry logic functions as intended under various failure scenarios, please consider adding unit or integration tests. This will help validate the behavior and reliability of the retry mechanism.
packages/rs-sdk/src/platform/fetch_many.rs (2)
148-184: Well-structured implementation of retry logic infetch_manyThe addition of the retry mechanism within the
fetch_manymethod enhances the robustness of the SDK by handling transient errors effectively. The use of closures to encapsulate the request logic is efficient and aligns with asynchronous best practices.
257-289: Effective retry logic infetch_manyforDocumentThe implementation of the retry functionality for fetching documents is clear and maintains consistency with the overall design. Error handling is appropriately managed, ensuring that retries are conducted smoothly without unhandled exceptions.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (8)
packages/rs-dapi-client/src/executor.rs (2)
22-35: LGTM! Consider adding examples to trait documentation.The traits are well-designed and documented. The distinction between lossy
IntoInnerand context-preservingInnerIntois clear.Consider adding usage examples to the trait documentation to demonstrate the difference between these conversions:
/// Example: /// ``` /// let error = ExecutionError { /// inner: "error", /// retries: 1, /// address: Some(address), /// }; /// /// // Lossy conversion - loses retries and address /// let inner: &str = error.into_inner(); /// /// // Context-preserving conversion /// let converted: ExecutionError<String> = error.inner_into(); /// assert_eq!(converted.retries, 1); /// ```
128-148: LGTM! Consider adding Debug bounds for better error messages.The Result conversion implementations are correct and properly handle both success and error cases.
Consider adding
Debugbounds to improve error messages in case of panics during conversion:-impl<F, FE, T, TE> InnerInto<ExecutionResult<T, TE>> for ExecutionResult<F, FE> +impl<F: Debug, FE: Debug, T, TE> InnerInto<ExecutionResult<T, TE>> for ExecutionResult<F, FE>packages/rs-sdk/src/platform/fetch_unproved.rs (1)
75-107: LGTM: Well-structured implementation with proper error handling.The closure-based implementation effectively encapsulates the request execution logic while maintaining proper error context and metadata. The error handling is comprehensive, preserving both the address and retry count information.
However, consider documenting the rationale for cloning the request, as it occurs twice (lines 83 and 91). If the request type is large, this could have performance implications.
Consider adding a comment explaining why the request needs to be cloned, or if possible, explore ways to avoid the clone:
- request.clone() + // Clone needed because the request is used multiple times across await points + request.clone()packages/rs-sdk/src/sync.rs (4)
200-201: Fix typo and clarify commentThe comment has a typo ("preceeding") and could be clearer.
- // requests sent in all preceeding attempts; user expects `settings.retries +1` + // requests sent in all preceding attempts; user expects total of `settings.retries + 1` requests
194-194: Fix inconsistent spacingThere are extra spaces around the
=operator.- let result= ::backon::Retryable::retry(closure,backoff_strategy) + let result = ::backon::Retryable::retry(closure, backoff_strategy)
213-213: Enhance error logging messageThe error message could be more informative by including the total attempts made.
- tracing::error!(retry = all_requests_sent, max_retries, error=?e, "no more retries left, giving up"); + tracing::error!(retry = all_requests_sent, max_retries, error=?e, "exceeded maximum retry attempts ({}/{}), giving up", all_requests_sent, max_retries + 1);
352-353: Remove unnecessary loopThe loop with a single iteration (
for _ in 0..1) serves no purpose and can be removed.- for _ in 0..1 { - let counter = Arc::new(AtomicUsize::new(0)); - } + let counter = Arc::new(AtomicUsize::new(0));packages/rs-sdk/src/platform/fetch_many.rs (1)
148-182: Consider enhancing error logging for better observability.While the implementation correctly handles errors and includes basic tracing, consider adding more detailed error logging to help with debugging production issues.
Add error context to the trace log:
tracing::trace!( request = ?request, response = ?response, ?address, retries, object_type, + error = ?response.as_ref().err(), "fetched object from platform" );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
- packages/rs-dapi-client/src/executor.rs (3 hunks)
- packages/rs-sdk/src/platform/fetch.rs (4 hunks)
- packages/rs-sdk/src/platform/fetch_many.rs (4 hunks)
- packages/rs-sdk/src/platform/fetch_unproved.rs (2 hunks)
- packages/rs-sdk/src/platform/transition/transfer_document.rs (3 hunks)
- packages/rs-sdk/src/sync.rs (3 hunks)
- packages/rs-sdk/tests/fetch/epoch.rs (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/rs-sdk/src/platform/transition/transfer_document.rs
- packages/rs-sdk/tests/fetch/epoch.rs
🔇 Additional comments (9)
packages/rs-dapi-client/src/executor.rs (2)
49-69: LGTM! Error conversion implementations are correct.The implementations properly handle both context-preserving and lossy conversions for errors, with appropriate trait bounds.
90-121: LGTM! Response implementations with proper test isolation.The implementations correctly handle conversions and the default mock address is appropriately isolated behind the "mocks" feature flag.
packages/rs-sdk/src/platform/fetch_unproved.rs (2)
4-4: LGTM: Import changes align with new retry functionality.The new imports properly support the retry mechanism implementation and error handling enhancements.
Also applies to: 12-12
109-110: LGTM: Clean retry implementation.The retry mechanism is properly integrated with the SDK settings, allowing for custom overrides while maintaining the default behavior.
packages/rs-sdk/src/platform/fetch.rs (3)
12-12: LGTM: Import changes align with retry logic implementation.The new imports support the SDK-level retry mechanism and enhanced error handling capabilities.
Also applies to: 22-22
124-126: LGTM: Clean delegation to new proof-aware method.The implementation maintains backward compatibility while following the single responsibility principle.
194-198: Verify retry behavior with different error scenarios.The implementation uses the
retryfunction with settings fromdapi_client_settings. Let's verify the retry behavior.✅ Verification successful
Let me gather more information about the retry implementation to verify the error handling scenarios.
Let me gather more information about the retry behavior and error handling.
Based on the gathered information, I can now provide a final response about the retry behavior verification.
Retry implementation is properly handling error scenarios
The retry mechanism is well-implemented with the following key aspects:
- Uses
backoncrate for retry strategy with configurable settings- Implements proper error handling through the
CanRetrytrait- Retries are performed for retriable errors like:
- Stale node errors (height mismatch)
- Timeout errors
- gRPC status codes: DataLoss, Cancelled, Unknown
- Non-retriable errors like "NoAvailableAddresses" fail fast
- Settings are properly propagated through the retry chain
- Default retry settings can be overridden per request
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check retry implementation and error handling # Test 1: Search for retry configuration echo "Checking retry configuration..." rg -A 5 "retry.*settings" --type rust # Test 2: Search for retriable error definitions echo "Checking retriable error definitions..." rg -A 5 "is_retriable|should_retry" --type rust # Test 3: Search for retry tests echo "Checking retry tests..." rg -A 5 "test.*retry" --type rustLength of output: 4705
Script:
#!/bin/bash # Let's examine the retry implementation and error handling in more detail # Check the retry function implementation echo "Checking retry function implementation..." ast-grep --pattern 'pub async fn retry<F, Fut, T, E>($$$) $$$' # Check error handling in the retry mechanism echo "Checking error handling in retry..." rg -B 2 -A 10 "Error::" --type rust packages/rs-sdk/src/sync.rs # Look for retry settings and configuration echo "Checking retry settings..." rg -B 2 -A 5 "RequestSettings" --type rustLength of output: 96173
Script:
#!/bin/bash # Let's examine the retry implementation and error handling in more detail # Check the retry implementation in sync.rs echo "Checking retry implementation in sync.rs..." rg -B 5 -A 15 "pub async fn retry" packages/rs-sdk/src/sync.rs # Check for error handling and retry conditions echo "Checking error handling and retry conditions..." rg -B 2 -A 10 "CanRetry" --type rust # Look for retry tests and their assertions echo "Checking retry tests and assertions..." rg -B 2 -A 10 "test.*retry.*assert" --type rustLength of output: 14073
packages/rs-sdk/src/sync.rs (2)
7-15: LGTM! The imports are well-organized and appropriate.The new imports provide the necessary components for implementing the retry logic, including atomic reference counting, error handling, and retry-specific traits.
204-215: Verify the retry count comparison logicThe comparison
all_requests_sent <= max_retriesmight allow one extra retry than intended. Consider the following:
max_retriesis set to N- When
all_requests_sentequals N, another retry will be allowed- This results in N+1 total requests, which exceeds the intended limit
Consider using strict less than:
- if all_requests_sent <=max_retries { // we account for for initial request + if all_requests_sent < max_retries + 1 { // we account for initial request✅ Verification successful
Let me search for more context around this retry logic to better understand the implementation.
Let me check how the retry count is initialized and used in the broader context.
The retry count comparison logic is correct
After analyzing the complete implementation, the current comparison
all_requests_sent <= max_retriesis actually correct because:
max_retriesis initialized fromsettings.retrieswhich represents the number of retries (not total requests)- The code correctly accounts for both initial request and retries:
- Initial request counts as 1
- Each retry attempt counts as 1
- The test cases in the file verify this behavior, specifically testing various retry counts (1,2,3,5,7,8,10,11,23,49)
- The suggested change to
all_requests_sent < max_retries + 1would be equivalent but less readableThe current implementation correctly ensures that total requests don't exceed initial request + configured retries.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for similar retry implementations in the codebase rg -A 5 'max_retries.*all_requests'Length of output: 494
Script:
#!/bin/bash # Get more context around the retry logic implementation rg -B 10 -A 10 'all_requests_sent.*max_retries' packages/rs-sdk/src/sync.rsLength of output: 1445
Script:
#!/bin/bash # Get the full context of the sync.rs file to understand retry initialization cat packages/rs-sdk/src/sync.rsLength of output: 13779
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (5)
packages/rs-sdk/src/sync.rs (3)
200-201: Fix typos in commentsThere are typos in the comments:
- "preceeding" should be "preceding"
- Extra spaces and missing period
- // requests sent in all preceeding attempts; user expects `settings.retries +1` + // requests sent in all preceding attempts; user expects `settings.retries + 1`.
213-214: Enhance error logging for retry exhaustionConsider including the original error details in the log message when retries are exhausted to aid in debugging.
- tracing::error!(retry = all_requests_sent, max_retries, error=?e, "no more retries left, giving up"); + tracing::error!( + retry = all_requests_sent, + max_retries, + error = ?e, + original_error = %e.inner, + "no more retries left, giving up" + );
352-353: Remove redundant loop in testThe test contains a loop that only runs once, making it redundant.
- for _ in 0..1 { let counter = Arc::new(AtomicUsize::new(0)); - }packages/rs-sdk/src/platform/fetch_many.rs (2)
160-161: Enhance trace logging with query details.The current trace logging could be more helpful for debugging by including query-specific information.
- tracing::trace!(request = ?request, response = ?response, ?address, retries, object_type, "fetched object from platform"); + tracing::trace!( + request = ?request, + response = ?response, + ?address, + retries, + object_type, + query_type = std::any::type_name::<Q>(), + "fetched object from platform" + );
272-276: Simplify error mapping using method chaining.The error mapping block can be simplified for better readability.
- .map_err(|e| ExecutionError { - inner: e, - retries, - address: Some(address.clone()), - })? + .map_err(|inner| ExecutionError { inner, retries, address: Some(address.clone()) })?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- packages/rs-sdk/src/platform/fetch_many.rs (4 hunks)
- packages/rs-sdk/src/sync.rs (3 hunks)
🔇 Additional comments (4)
packages/rs-sdk/src/sync.rs (2)
7-15: LGTM! Imports are well-organized and necessary.The new imports support the retry mechanism implementation with appropriate synchronization primitives and error handling.
349-375: LGTM! Comprehensive test coverageThe test matrix covers a good range of scenarios including edge cases. The implementation properly verifies the retry behavior with deterministic outcomes.
packages/rs-sdk/src/platform/fetch_many.rs (2)
279-283: 🛠️ Refactor suggestionConsider moving unwrap_or_default earlier for better readability.
The
unwrap_or_default()call at the end of the chain could be moved earlier to make the flow clearer.Ok(ExecutionResponse { - inner: documents, + inner: documents.unwrap_or_default(), retries, address, }) }) .await - .into_inner() + .map(|response| response.inner)Likely invalid or redundant comment.
149-157: 🛠️ Refactor suggestionConsider optimizing request execution to avoid SDK cloning.
The closure captures the
sdkreference which is then cloned during request execution. Consider passing the SDK as a parameter to avoid capturing and cloning.- let closure = |settings: RequestSettings| async move { + let closure = |settings: RequestSettings, sdk: &Sdk| async move { let ExecutionResponse { address, retries, inner: response, } = request - .clone() - .execute(sdk, settings) + .execute(sdk, settings) .await .map_err(|e| e.inner_into())?;Likely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
packages/rs-dapi-client/src/lib.rs (1)
Line range hint
82-97: Consider enhancingCanRetrytrait for stale node scenarios.Given that this PR aims to handle higher-level errors like "stale node detected" (issue #2254), consider enhancing the
CanRetrytrait to provide more specific retry behavior classification. This could help distinguish between different types of retryable errors (transport vs. stale node).Consider adding a method like:
fn retry_reason(&self) -> RetryReason { if self.can_retry() { RetryReason::determine_from_error(self) } else { RetryReason::NonRetryable } }This would provide better context for retry decisions and logging while maintaining backward compatibility.
packages/rs-sdk/src/platform/fetch_unproved.rs (2)
75-107: LGTM: Well-structured retry implementation with proper error handling.The closure-based implementation effectively manages the retry logic while preserving error context and metadata. The error handling is particularly well done, maintaining address and retry count information throughout the process.
Consider adding documentation to explain the retry behavior, particularly:
- What types of errors trigger retries
- Maximum retry attempts
- Backoff strategy
async fn fetch_unproved_with_settings<Q: Query<<Self as FetchUnproved>::Request>>( sdk: &Sdk, query: Q, settings: RequestSettings, ) -> Result<(Option<Self>, ResponseMetadata), Error> where Self: FromUnproved< <Self as FetchUnproved>::Request, Request = <Self as FetchUnproved>::Request, Response = <<Self as FetchUnproved>::Request as TransportRequest>::Response, >, { + // This implementation includes retry logic for handling transient failures. + // Retries are performed according to the RequestSettings configuration, + // with each attempt preserving the context (address, retry count) for debugging.
76-86: Consider adding debug logging for retry attempts.Adding debug logging would help with troubleshooting and monitoring the retry behavior in production.
let closure = move |local_settings: RequestSettings| async move { + tracing::debug!( + "Executing request with settings: {:?}, attempt: {}", + local_settings, + local_settings.retry_attempts.unwrap_or(0) + ); // Execute the request using the Sdk instance let ExecutionResponse {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
- packages/rs-dapi-client/src/lib.rs (1 hunks)
- packages/rs-dapi-client/src/mock.rs (1 hunks)
- packages/rs-sdk/src/core/transaction.rs (3 hunks)
- packages/rs-sdk/src/platform/fetch_unproved.rs (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/rs-dapi-client/src/mock.rs
- packages/rs-sdk/src/core/transaction.rs
🔇 Additional comments (2)
packages/rs-dapi-client/src/lib.rs (1)
24-26: LGTM! The new trait exports align with the PR objectives.The addition of
InnerIntoandIntoInnertraits to the public API is consistent with implementing SDK-level retry logic. These traits will enable better error handling and response processing while preserving retry context.Let's verify the trait implementations in the executor module:
✅ Verification successful
Trait implementations are properly implemented and align with SDK-level retry logic
The verification shows that both
InnerIntoandIntoInnertraits are well-defined and have comprehensive implementations:
IntoInner<T>for unwrapping inner types while dropping contextInnerInto<T>for converting inner types while preserving retry context- Complete implementations for
ExecutionError,ExecutionResponse, andExecutionResulttypes- Generic implementations that maintain type safety through proper trait bounds
The trait exports in the library's public API are justified and properly backed by their implementations.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the trait implementations for InnerInto and IntoInner # Expected: Find implementations for ExecutionError, ExecutionResponse, and ExecutionResult # Search for trait implementations ast-grep --pattern 'impl$_ InnerInto<$_> for $_' ast-grep --pattern 'impl$_ IntoInner<$_> for $_'Length of output: 101
Script:
#!/bin/bash # Let's try a different approach to find trait implementations # First, locate the executor module file fd "executor\.rs" --type f # Then search for trait definitions and implementations rg -A 5 "trait (InnerInto|IntoInner)" rg -A 5 "impl.*?(InnerInto|IntoInner)"Length of output: 3744
packages/rs-sdk/src/platform/fetch_unproved.rs (1)
4-4: LGTM: Import changes align with retry implementation.The added imports support the new retry logic and enhanced error handling capabilities.
Also applies to: 12-12
Issue being fixed or feature implemented
Some requests can fail and require retry. For transport-level errors, rs-dapi-client implements retry logic. For higher-level errors (like, stale node detected #2254 ) we can also implement a retry mechanism on Sdk level.
What was done?
Implemented retry logic for
fetchandfetch_manyin SDK, on top of #2258Other methods will be covered in further PRs
How Has This Been Tested?
Manual testing on local devnet.
Breaking Changes
None
Checklist:
For repository code-owners and collaborators only
Summary by CodeRabbit
Release Notes
New Features
tokio-sleepfeature to enhance the DAPI client functionality.retryfunction for managing asynchronous operations with retry logic.Improvements
IntoInnertrait, improving clarity and maintainability.retriesfield inRequestSettingsfor better user understanding.dapi_client_settingsto theSdkstruct for improved configuration capabilities.Bug Fixes
Chores