feat(sdk)!: retry broadcast operations #2337
Conversation
WalkthroughThe changes in this pull request focus on enhancing the Changes
Suggested labels
Suggested reviewers
Warning Rate limit exceeded@lklimek has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 5 minutes and 22 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 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: 1
🧹 Outside diff range and nitpick comments (6)
packages/rs-dapi-client/src/lib.rs (2)
Line range hint
82-87: LGTM! Consider updating documentation for the deprecationThe deprecation of
is_node_failurein favor ofcan_retryimproves code clarity. The implementation correctly maintains backward compatibility while guiding users toward the preferred method.Consider adding a note in the crate's documentation or changelog about this deprecation to help users migrate their code.
Line range hint
1-87: Well-structured architectural approachThe integration of retry functionality through the
WrapWithExecutionResulttrait and the existingCanRetryinterface provides a clean and modular approach to implementing broadcast operation retries. The architecture maintains good separation of concerns while extending the system's capabilities.Consider documenting the retry strategy (e.g., maximum attempts, backoff policy) in the module-level documentation to guide implementers of these traits.
packages/rs-sdk/src/platform/transition/broadcast.rs (4)
103-111: Improve error message clarity in timeout handlingWhen a timeout occurs in
broadcast_and_wait, the error message includes the debug representation of the errore, which may not provide meaningful information. Simplify the error message to clearly indicate that the operation has timed out.You could modify the error construction as:
Error::TimeoutReached( timeout, - format!("Timeout waiting for state transition result: {:?}", e), + "Timeout waiting for state transition result".to_string(), )
29-29: Remove redundant or outdated commentThe comment on line 29 appears to be unnecessary or left over from previous development. Removing it will help maintain code cleanliness.
117-117: Remove unnecessary commented-out codeThe commented line
//Result<StateTransitionProofResult, Error>on line 117 seems redundant. If it's not serving a purpose, consider deleting it to keep the codebase tidy.
54-54: Consider renamingtime_out_mstotimeout_msfor consistencyFor consistency and readability, consider renaming the parameter
time_out_mstotimeout_ms, aligning with thetimeoutvariable used in the method.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
packages/rs-dapi-client/src/executor.rs(2 hunks)packages/rs-dapi-client/src/lib.rs(1 hunks)packages/rs-sdk/src/platform/transition/broadcast.rs(2 hunks)
🔇 Additional comments (5)
packages/rs-dapi-client/src/lib.rs (1)
25-25: Verify the implementation of WrapWithExecutionResult trait
The new trait export aligns with the PR's objective of implementing retry functionality for broadcast operations. Let's verify its implementation and usage.
✅ Verification successful
Let me gather more information about the implementation and usage of this trait.
Let me check one more thing to see how this trait is actually used in the implementation.
Export of WrapWithExecutionResult trait is properly implemented and used
The trait is well-defined in the executor module with proper documentation and is being used appropriately in the broadcast operations module for state transitions. The export aligns with the PR's objective of implementing retry functionality.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check the implementation of WrapWithExecutionResult and its usage in broadcast operations
# Find the trait implementation
ast-grep --pattern 'trait WrapWithExecutionResult {
$$$
}'
# Find usage in broadcast operations
rg -l "WrapWithExecutionResult" "packages/rs-sdk/src/platform/transition/broadcast.rs"
Length of output: 200
Script:
#!/bin/bash
# Get the trait implementation details
rg -A 10 "trait WrapWithExecutionResult"
# Check how it's used in broadcast.rs
rg -A 5 "WrapWithExecutionResult" "packages/rs-sdk/src/platform/transition/broadcast.rs"
Length of output: 1396
Script:
#!/bin/bash
# Look for implementations of WrapWithExecutionResult
ast-grep --pattern 'impl WrapWithExecutionResult<$_,$_,$_> for $_ {
$$$
}'
# Get more context from broadcast.rs to see how it's used
rg -B 5 -A 15 "use rs_dapi_client::WrapWithExecutionResult" "packages/rs-sdk/src/platform/transition/broadcast.rs"
Length of output: 1052
packages/rs-sdk/src/platform/transition/broadcast.rs (2)
27-48: Implementation of retry mechanism in broadcast method looks good
The retry mechanism in the broadcast method is correctly implemented using the retry function. The factory closure is well-defined, and errors are appropriately handled and mapped.
54-115: Implementation of retry and timeout in broadcast_and_wait method is correct
The broadcast_and_wait method effectively utilizes the retry function and handles optional timeouts using tokio::time::timeout. Error handling and mapping are properly managed throughout the method.
packages/rs-dapi-client/src/executor.rs (2)
127-131: Implementation of From<ExecutionResponse<R>> for ExecutionResult<R, E> enhances usability
The addition of this From implementation allows for seamless conversion from ExecutionResponse<R> to ExecutionResult<R, E>, improving code readability and simplifying result handling.
133-137: Implementation of From<ExecutionError<E>> for ExecutionResult<R, E> ensures consistent error handling
By implementing From for ExecutionError<E>, you facilitate straightforward conversion to ExecutionResult<R, E>, maintaining consistency in error propagation and handling throughout the codebase.
|
@coderabbitai review all |
|
@lklimek: Understood, I will review all the changes again. ✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (17)
packages/rs-dpp/src/state_transition/proof_result.rs (1)
Line range hint
8-16: Consider adding documentation for the enum and its variants.Adding documentation comments would help clarify:
- The purpose of each variant
- Expected conversion behaviors from
TryInto- Format of the
Displayimplementation+/// Represents the result of verifying different types of state transitions. +/// Implements Display for logging and TryInto for conversion handling. #[derive(Debug, strum::Display, derive_more::TryInto)] pub enum StateTransitionProofResult { + /// Successfully verified data contract VerifiedDataContract(DataContract), + /// Successfully verified complete identity VerifiedIdentity(Identity), // ... add docs for other variantspackages/rs-sdk/src/platform/transition/put_settings.rs (2)
12-13: Enhance documentation for wait_timeout fieldWhile the documentation is present, it would be helpful to include:
- The default value behavior
- Expected range of values
- What happens when timeout is reached
Consider expanding the documentation like this:
- /// The time to wait for the response of a state transition after it has been broadcast + /// The time to wait for the response of a state transition after it has been broadcast. + /// Defaults to None. When set, specifies the maximum duration to wait for the response. + /// If the timeout is reached, the operation will return with a timeout error.
Line range hint
8-14: Consider implementing a builder pattern for PutSettingsGiven that
PutSettingshas multiple optional fields and might grow in the future, consider implementing a builder pattern. This would make it easier to construct instances with different combinations of settings and improve code readability at call sites.Example usage would look like:
let settings = PutSettingsBuilder::default() .with_wait_timeout(Duration::from_secs(30)) .with_user_fee_increase(fee_increase) .build();packages/rs-sdk/src/platform/transition/transfer.rs (1)
56-56: LGTM! Consider updating the function documentation.The change to pass
settingstobroadcast_and_waitaligns well with the PR's objective to implement retry functionality for broadcast operations. The implementation maintains proper error handling and type safety.Consider updating the function documentation to mention the role of
settingsin controlling broadcast behavior, particularly regarding retry settings and wait timeout.packages/rs-dapi-client/src/request_settings.rs (1)
22-23: Enhance timeout documentation clarityThe added clarification about timeout being a soft limit and the total execution time is valuable. However, consider these improvements:
- Fix the double space in the formula
- Add a brief explanation of why it's considered a "soft limit"
- Include an example to illustrate the behavior
/// Timeout for single request (soft limit). - /// Note that the total maximum time of execution can exceed `(timeout + connect_timeout) * retries`. + /// Note that the total maximum time of execution can exceed `(timeout + connect_timeout) * retries` + /// as it accounts for internal processing time between retries. + /// + /// Example: With timeout=1s, connect_timeout=1s, and retries=3, the actual execution + /// might take longer than 6 seconds due to retry processing overhead.packages/rs-sdk/src/error.rs (3)
61-63: LGTM! Consider adding more detailed documentation.The new
AlreadyExistserror variant is well-structured and consistent with the existing error patterns.Consider adding a doc comment to provide more context about when this error occurs:
+ /// Returned when an attempt is made to create an object that already exists in the system #[error("Object already exists: {0}")] AlreadyExists(String),
Line range hint
86-102: Consider improving error handling robustness.The error handling logic is good, but could be enhanced in a few ways:
- Consider adding debug logging for consensus error deserialization failures:
.map(|consensus_error| { Self::Protocol(ProtocolError::ConsensusError(Box::new(consensus_error))) }) - .unwrap_or_else(Self::Protocol); + .unwrap_or_else(|e| { + tracing::debug!("Failed to deserialize consensus error: {}", e); + Self::Protocol(e) + });
- Consider caching the status message conversion:
- return Self::AlreadyExists(status.message().to_string()); + let message = status.message().to_string(); + return Self::AlreadyExists(message);Also applies to: 105-105
Line range hint
1-105: Consider documenting retry behavior in error variants.Since this PR implements retry mechanisms for broadcast operations, it would be valuable to document which error conditions are retryable. While the
CanRetrytrait implementation exists, consider adding doc comments to error variants indicating their retry behavior.packages/rs-sdk/src/core/transaction.rs (2)
Line range hint
82-82: Consider implementing retry mechanism as part of this PRThere's a TODO comment about implementing retries for stream message handling. Since this PR's objective is to implement retry operations for broadcast operations, this would be a good opportunity to address this TODO.
Would you like help implementing a retry mechanism for the stream message handling?
Line range hint
82-279: Consider breaking down the complex stream processing logicThe
wait_for_asset_lock_proof_for_transactionmethod is quite complex with multiple nested conditions. Consider breaking it down into smaller, more focused functions:
- Message processing logic
- Chain lock verification
- Platform height verification
This would improve:
- Code readability
- Testability
- Maintainability
packages/rs-sdk/src/platform/transition/put_contract.rs (1)
79-79: Simplify error context when broadcasting.While using
?propagates errors, consider adding context to any potential errors from the broadcast operation for better debugging. This can be done using themap_errmethod.For example:
- transition.broadcast(sdk, settings).await?; + transition.broadcast(sdk, settings).await.map_err(|e| Error::BroadcastError(e.to_string()))?;packages/rs-sdk/src/platform/transition/put_document.rs (4)
96-96: Consider removing the unused parameter_data_contractThe parameter
_data_contractin thewait_for_responsemethod is unused. Removing it can simplify the function signature unless it's needed for future compatibility or trait compliance.
122-122: Consider removing the unused parameter_data_contractSimilarly, in the
put_to_platform_and_wait_for_responsemethod, the_data_contractparameter is unused. If it's not required, consider removing it to clean up the interface.
137-150: Refactor duplicated error handling logicThe match block handling
StateTransitionProofResult::VerifiedDocumentsis similar to the one in thewait_for_responsemethod. Consider refactoring this logic into a helper function to reduce code duplication and improve maintainability.Here's a possible refactor:
fn extract_verified_document( mut documents: HashMap<DocumentId, Option<Document>>, document_id: &DocumentId, ) -> Result<Document, Error> { documents .remove(document_id) .ok_or_else(|| { Error::InvalidProvedResponse("did not prove the sent document".to_string()) })? .ok_or_else(|| { Error::InvalidProvedResponse("expected there to actually be a document".to_string()) }) }Then, in both methods, you can use:
match result { StateTransitionProofResult::VerifiedDocuments(mut documents) => { let document = extract_verified_document(documents, self.id_ref())?; Ok(document) } _ => Err(Error::DapiClientError("proved a non document".to_string())), }
137-150: Simplify nestedOptionhandlingYou can simplify the extraction of the document using pattern matching to make the code more readable.
Apply this diff to simplify the match block:
match result { StateTransitionProofResult::VerifiedDocuments(mut documents) => { - let document = documents - .remove(self.id_ref()) - .ok_or(Error::InvalidProvedResponse( - "did not prove the sent document".to_string(), - ))? - .ok_or(Error::InvalidProvedResponse( - "expected there to actually be a document".to_string(), - ))?; + match documents.remove(self.id_ref()) { + Some(Some(document)) => Ok(document), + Some(None) => Err(Error::InvalidProvedResponse( + "expected there to actually be a document".to_string(), + )), + None => Err(Error::InvalidProvedResponse( + "did not prove the sent document".to_string(), + )), + } } _ => Err(Error::DapiClientError("proved a non document".to_string())), }packages/rs-sdk/src/platform/transition/broadcast.rs (2)
34-37: Refactor duplicated logic for obtaining retry settingsThe code for extracting
retry_settingsfromsettingsis duplicated in both thebroadcastandwait_for_responsemethods. Consider extracting this logic into a helper function to adhere to the DRY (Don't Repeat Yourself) principle and enhance maintainability.Also applies to: 65-68
105-114: Include detailed error information when conversion failsIn the
T::try_from(result)call, the error returned byTryFromis being ignored. Capturing and including the error details can provide more context for debugging. Consider modifying the error handling to include the error message from the conversion attempt.Apply this diff to capture and include the error details:
T::try_from(result) - .map_err(|_| { + .map_err(|e| { Error::InvalidProvedResponse(format!( - "invalid proved response: cannot convert from {} to {}", + "invalid proved response: cannot convert from {} to {}: {}", variant_name, std::any::type_name::<T>(), + e )) }) .wrap(&response)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (14)
packages/rs-dapi-client/src/request_settings.rs(1 hunks)packages/rs-dpp/Cargo.toml(2 hunks)packages/rs-dpp/src/state_transition/proof_result.rs(1 hunks)packages/rs-sdk/src/core/transaction.rs(1 hunks)packages/rs-sdk/src/error.rs(4 hunks)packages/rs-sdk/src/platform/transition.rs(0 hunks)packages/rs-sdk/src/platform/transition/broadcast.rs(1 hunks)packages/rs-sdk/src/platform/transition/context.rs(0 hunks)packages/rs-sdk/src/platform/transition/purchase_document.rs(2 hunks)packages/rs-sdk/src/platform/transition/put_contract.rs(3 hunks)packages/rs-sdk/src/platform/transition/put_document.rs(5 hunks)packages/rs-sdk/src/platform/transition/put_settings.rs(2 hunks)packages/rs-sdk/src/platform/transition/transfer.rs(1 hunks)packages/rs-sdk/src/platform/transition/withdraw_from_identity.rs(1 hunks)
💤 Files with no reviewable changes (2)
- packages/rs-sdk/src/platform/transition.rs
- packages/rs-sdk/src/platform/transition/context.rs
🔇 Additional comments (14)
packages/rs-dpp/src/state_transition/proof_result.rs (1)
8-8: LGTM! Verify conversion implementations.
The addition of Display and TryInto traits is appropriate for supporting retry mechanisms. Please ensure that all possible type conversions using TryInto are properly tested to handle error cases.
packages/rs-sdk/src/platform/transition/put_settings.rs (2)
1-1: LGTM: Import is correctly placed and necessary
The Duration import is appropriately added for the new wait_timeout field.
16-19: Verify field dropping in From implementation
The implementation correctly extracts request_settings, but other fields (identity_nonce_stale_time_s, user_fee_increase, wait_timeout) are dropped during conversion. Please verify this is the intended behavior.
packages/rs-sdk/src/platform/transition/withdraw_from_identity.rs (1)
65-65: LGTM! Verify settings propagation for retry functionality.
The change to pass settings to broadcast_and_wait aligns with the PR's objective of implementing retry functionality for broadcast operations. This is consistent with similar changes in other files.
packages/rs-dapi-client/src/request_settings.rs (1)
Line range hint 5-8: Verify default values for broadcast retry scenarios
The current default values (10s timeout, 5 retries) appear reasonable, but please verify they are appropriate for the broadcast retry use case. Consider:
- Are broadcast operations typically longer than regular operations?
- Is the default retry count sufficient for broadcast reliability?
- Should broadcast operations have different default timeouts?
packages/rs-dpp/Cargo.toml (2)
32-32: Verify cryptographic operations compatibility.
The addition of the "eddsa" feature to dashcore is appropriate. Please verify that this aligns with any existing EdDSA operations in the codebase, particularly for state transition signing.
59-59: LGTM! Addition of try_into supports error handling.
The addition of the "try_into" feature to derive_more is well-aligned with implementing retry functionality, enabling proper error type conversions for broadcast operations.
packages/rs-sdk/src/core/transaction.rs (2)
60-60: Improved error handling using type conversion
The change from explicit string conversion to using Into trait is a good improvement as it:
- Leverages Rust's type system for proper error propagation
- Preserves error context instead of converting to string
- Follows Rust's error handling best practices
Line range hint 186-187: Consider implementing better execution error handling
The TODO comment indicates a need for better execution error handling. This aligns with the error handling improvements being made in this PR.
Would you like assistance in implementing a more robust error handling strategy for execution errors?
packages/rs-sdk/src/platform/transition/put_contract.rs (1)
16-16: Use of BroadcastStateTransition trait is appropriate.
The import of BroadcastStateTransition is necessary for invoking the broadcast method on the state transition.
packages/rs-sdk/src/platform/transition/purchase_document.rs (3)
19-19: LGTM!
The import of BroadcastStateTransition is correct and necessary for using the broadcast method.
96-96: Correct use of broadcast method on state transition
The broadcast call on the transition object is correctly implemented and awaits the result, ensuring the state transition is broadcasted properly.
105-105: Parameter _data_contract is appropriately marked as unused
By prefixing data_contract with an underscore, you correctly indicate that this parameter is currently unused, which suppresses compiler warnings.
packages/rs-sdk/src/platform/transition/broadcast.rs (1)
119-133: Verify proper timeout error handling
When a wait_timeout is specified, the error handling in the timeout case may not provide sufficient details about the underlying issue. Ensure that the timeout error is adequately captured and that any nested errors are properly logged or propagated to aid in debugging.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (15)
packages/rs-dpp/src/state_transition/proof_result.rs (1)
Line range hint
8-16: Consider adding documentation for the new traits.Adding documentation comments that explain the string representation format (Display trait) and the supported type conversions (TryInto trait) would help users understand how to properly use these new capabilities.
+/// Represents the result of a state transition proof verification. +/// +/// # Display Implementation +/// The Display implementation provides a human-readable representation of the verification result. +/// +/// # Type Conversion +/// This enum can be converted into other types using TryInto. The conversion will succeed +/// when the variant matches the expected type. #[derive(Debug, strum::Display, derive_more::TryInto)] pub enum StateTransitionProofResult {packages/rs-sdk/src/platform/transition/put_settings.rs (1)
12-13: Consider enhancing the documentationWhile the documentation is clear, it would be helpful to include:
- The default behavior when timeout is None
- The unit of measurement (milliseconds/seconds)
- Any minimum/maximum constraints
packages/rs-dapi-client/src/lib.rs (1)
Line range hint
77-86: Consider updating deprecation notice with migration guideThe deprecation notice for
is_node_failureprovides the alternative method to use (can_retry), but it could be more helpful to include a brief explanation of whycan_retryis preferred and how to migrate existing code.-#[deprecated = "Use !can_retry() instead"] +#[deprecated = "Use !can_retry() instead. The can_retry() method provides more accurate retry semantics for broadcast operations"]packages/rs-sdk/src/platform/transition/withdraw_from_identity.rs (1)
Line range hint
1-74: Consider enhancing error handling and documentationWhile the implementation is functionally correct, consider the following improvements:
- Add documentation about retry behavior and settings impact
- Create more specific error variants for withdrawal-related failures
- Consider splitting the implementation into smaller, focused functions for better maintainability
Example documentation enhancement:
/// Function to withdraw credits from an identity. Returns the final identity balance. /// If signing_withdrawal_key_to_use is not set, we will try to use one in the signer that is /// available for withdrawal /// /// # Arguments /// * `settings` - Optional settings for configuring broadcast behavior including retry attempts /// and timeouts. If None, default settings will be used. /// /// # Errors /// Returns an error if: /// * The withdrawal transaction fails to broadcast after retry attempts /// * The identity balance cannot be verified after withdrawal /// * The state transition verification failspackages/rs-dapi-client/src/request_settings.rs (2)
22-23: Fix documentation formatting and approve the clarification.The documentation improvement is valuable as it clarifies the timeout behavior. However, there's a minor formatting issue in the note.
- /// Note that the total maximum time of execution can exceed `(timeout + connect_timeout) * retries`. + /// Note that the total maximum time of execution can exceed `(timeout + connect_timeout) * retries`.
22-23: Consider adding retry-specific documentation.Since this PR implements retry functionality for broadcast operations, consider adding documentation about how the timeout interacts with retry attempts, especially for broadcast operations. This would help developers better understand the timing implications when retries occur.
packages/rs-sdk/src/platform/transition/purchase_document.rs (2)
96-98: LGTM! Simplified broadcasting approach.The direct broadcast call with settings passthrough improves code clarity by removing unnecessary request object creation. The comment accurately documents the empty response behavior.
18-19: Well-structured error handling and state transition management.The architectural changes demonstrate good practices:
- Clear separation of concerns between broadcasting and response handling
- Strong error typing and propagation
- Efficient async/await usage
- Consistent implementation patterns across the codebase
This aligns well with the PR's objective of improving broadcast operations reliability.
packages/rs-sdk/src/platform/transition/put_document.rs (2)
96-98: Address TODO comment and enhance verificationThe code contains a TODO comment regarding verification. Consider implementing proper verification checks for the response to ensure data integrity.
Would you like help implementing the verification logic? I can suggest an approach that includes:
- Document integrity verification
- Response validation against expected state
- Error handling for verification failures
136-150: Consider refactoring duplicate error handling logicThe error handling logic is duplicated between
wait_for_responseandput_to_platform_and_wait_for_responsemethods. Consider extracting this into a shared helper function.fn extract_document_from_proof_result( documents: HashMap<DocumentId, Option<Document>>, document_id: &DocumentId, ) -> Result<Document, Error> { documents .remove(document_id) .ok_or(Error::InvalidProvedResponse( "did not prove the sent document".to_string(), ))? .ok_or(Error::InvalidProvedResponse( "expected there to actually be a document".to_string(), )) }packages/rs-sdk/src/error.rs (2)
99-102: Improve comment clarity for error code handlingThe implementation is correct, but the comment could be more precise.
- // Otherwise we parse the error code and act accordingly + // Check for specific error codes and handle accordingly
105-105: Consider adding more context to generic error conversionThe generic error conversion could benefit from including more context about the error source.
- Self::DapiClientError(value.to_string()) + Self::DapiClientError(format!("DAPI client error: {}", value))packages/rs-sdk/src/core/transaction.rs (1)
Line range hint
82-277: Consider refactoring stream processing logicThe
wait_for_asset_lock_proof_for_transactionmethod is quite complex with multiple responsibilities:
- Stream message processing
- Multiple response type handling
- Chain height verification
- Error handling
Consider breaking this down into smaller, focused functions for better maintainability and testability.
Suggested structure:
impl Sdk { async fn process_instant_send_lock_messages(...) -> Result<Option<AssetLockProof>, Error> { ... } async fn process_merkle_block(...) -> Result<Option<AssetLockProof>, Error> { ... } async fn wait_for_chain_lock(...) -> Result<u32, Error> { ... } async fn wait_for_platform_height(...) -> Result<(), Error> { ... } }packages/rs-sdk/src/platform/transition/put_contract.rs (1)
90-90: Consider passingsettingstowait_for_responseCurrently,
state_transition.wait_for_response(sdk, None).await?is called withNonefor the settings. If there are timeout configurations or other parameters inPutSettingsthat should apply during the wait, consider passingsettingsinstead ofNoneto maintain consistent behavior.packages/rs-sdk/src/platform/transition/broadcast.rs (1)
18-28: Consider adding documentation comments to new methodsAdding Rust documentation comments (
///) to the new or modified methods will improve code readability and maintainability for future developers.Also applies to: 33-142
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (16)
packages/rs-dapi-client/src/executor.rs(2 hunks)packages/rs-dapi-client/src/lib.rs(1 hunks)packages/rs-dapi-client/src/request_settings.rs(1 hunks)packages/rs-dpp/Cargo.toml(2 hunks)packages/rs-dpp/src/state_transition/proof_result.rs(1 hunks)packages/rs-sdk/src/core/transaction.rs(1 hunks)packages/rs-sdk/src/error.rs(4 hunks)packages/rs-sdk/src/platform/transition.rs(0 hunks)packages/rs-sdk/src/platform/transition/broadcast.rs(1 hunks)packages/rs-sdk/src/platform/transition/context.rs(0 hunks)packages/rs-sdk/src/platform/transition/purchase_document.rs(2 hunks)packages/rs-sdk/src/platform/transition/put_contract.rs(3 hunks)packages/rs-sdk/src/platform/transition/put_document.rs(5 hunks)packages/rs-sdk/src/platform/transition/put_settings.rs(2 hunks)packages/rs-sdk/src/platform/transition/transfer.rs(1 hunks)packages/rs-sdk/src/platform/transition/withdraw_from_identity.rs(1 hunks)
💤 Files with no reviewable changes (2)
- packages/rs-sdk/src/platform/transition.rs
- packages/rs-sdk/src/platform/transition/context.rs
🔇 Additional comments (23)
packages/rs-dpp/src/state_transition/proof_result.rs (1)
8-8: LGTM! The derive macro additions are well-chosen.
The addition of Display and TryInto traits provides essential functionality for error handling and type conversion in the retry mechanism. These changes are non-breaking and align well with the PR's objective.
packages/rs-sdk/src/platform/transition/put_settings.rs (3)
1-1: LGTM: Clean import addition
The Duration import is appropriately placed and necessary for the new timeout functionality.
Line range hint 8-14: Review the use of Copy trait
The struct derives Copy, which might become problematic as more fields are added. Consider if Clone alone would be sufficient, as Copy is typically reserved for small, stack-only types.
16-19: LGTM: Clean From trait implementation
The conversion from PutSettings to RequestSettings is straightforward and maintains type safety.
packages/rs-sdk/src/platform/transition/transfer.rs (1)
56-56: LGTM! Verify timeout configuration.
The propagation of settings to broadcast_and_wait enhances the configurability of the broadcast operation, which aligns well with the retry functionality objective.
Please ensure that appropriate timeout values are configured in the PutSettings to prevent operations from hanging indefinitely during retries.
packages/rs-dapi-client/src/lib.rs (2)
25-25: LGTM: New trait export aligns with PR objectives
The addition of WrapWithExecutionResult to the public exports is consistent with implementing retry functionality for broadcast operations. This trait will allow for proper error wrapping and handling during retry attempts.
Line range hint 31-43: Verify documentation completeness
The example in the DapiRequest trait documentation shows basic usage but doesn't demonstrate the new retry functionality. Consider adding an example that shows how to use the new WrapWithExecutionResult trait with retry logic.
packages/rs-sdk/src/platform/transition/withdraw_from_identity.rs (1)
65-65: Verify retry settings behavior for withdrawal operations
The addition of settings parameter to broadcast_and_wait aligns with the PR's objective to implement retry functionality for broadcast operations. However, ensure that:
- The retry behavior is appropriate for withdrawal operations, as failed withdrawals could have financial implications
- The settings passed through the withdrawal function properly configure retry attempts and timeouts
- Error handling remains robust across retry attempts
packages/rs-sdk/src/platform/transition/purchase_document.rs (1)
105-107: 🛠️ Refactor suggestion
Consider removing the unused data_contract parameter.
The _data_contract parameter is no longer used in the implementation. Since this is a public trait method, removing it would be a breaking change. Consider:
- Documenting why the parameter is retained if there's a specific reason
- Planning for its removal in the next major version
- Adding a deprecation notice if appropriate
packages/rs-sdk/src/platform/transition/put_document.rs (2)
1-3: LGTM: Import changes align with new functionality
The addition of broadcast and PutSettings imports properly supports the enhanced broadcasting functionality with retry settings.
88-89: LGTM: Improved broadcasting with retry support
The simplified broadcasting approach properly integrates retry settings while maintaining error handling. Please verify that the settings propagation works as expected with various retry configurations.
packages/rs-sdk/src/error.rs (1)
60-63: LGTM! Well-structured error variant addition
The new AlreadyExists error variant follows Rust error handling best practices with a clear error message format.
packages/rs-dpp/Cargo.toml (1)
32-32: LGTM! Verify feature flag dependencies.
The additions of "eddsa" to dashcore and "try_into" to derive_more are appropriate for supporting the retry mechanism implementation. These changes enable necessary cryptographic operations and type conversions required for robust error handling.
Please verify that:
- The "eddsa" feature is properly gated behind relevant feature flags (e.g., "ed25519-dalek")
- Any new code using TryInto trait derivation is covered by appropriate feature flags
Also applies to: 59-59
packages/rs-sdk/src/core/transaction.rs (3)
60-60: Improved error handling approach
The change from explicit string conversion to using Into trait is a better approach as it:
- Preserves the original error context
- Allows for more detailed error information to propagate
- Follows Rust's error handling best practices
Line range hint 82-85: Consider implementing retry logic for stream messages
The TODO comment indicates missing retry logic for stream messages. Given this is a network operation, implementing proper retry logic with backoff would improve reliability.
Would you like help implementing a retry mechanism with exponential backoff?
Line range hint 186-189: Review temporary implementation for headers stream
The comment indicates this is a temporary implementation until headers stream runs in the background. This could lead to reliability issues and should be addressed.
Consider:
- Documenting the limitations of the current approach
- Creating a tracking issue for implementing the proper headers stream solution
- Adding metrics/logging to monitor potential issues with the current implementation
packages/rs-sdk/src/platform/transition/put_contract.rs (2)
15-16: Import statement added appropriately
The addition of use super::broadcast::BroadcastStateTransition; correctly brings the BroadcastStateTransition trait into scope, enabling the use of the broadcast method on the state transition object.
79-79: Ensure proper handling of potential broadcast errors
The use of transition.broadcast(sdk, settings).await? propagates any errors that occur during the broadcast operation. Please verify that any calling code appropriately handles these errors to prevent unintended side effects.
packages/rs-sdk/src/platform/transition/broadcast.rs (3)
18-28: Method signatures updated to enhance configurability
The methods in the BroadcastStateTransition trait are correctly updated to accept an optional PutSettings, allowing for configurable broadcast operations.
86-93: Verify that context_provider is properly initialized
The code checks if sdk.context_provider() returns None and handles it by returning an error. Ensure that the context_provider is initialized before calling wait_for_response to prevent runtime errors.
60-134: Implementation of wait_for_response and broadcast_and_wait is correct
The methods are implemented with proper error handling, retry logic, and timeout management, which enhances the robustness of the broadcast operations.
packages/rs-dapi-client/src/executor.rs (2)
127-138: Implementing From Traits for ExecutionResult
The additions of the From implementations for ExecutionResult<R, E> from ExecutionResponse<R> and ExecutionError<E> enhance the conversion mechanisms between these types, promoting better interoperability and cleaner code.
161-212: Previous Refactoring Suggestion Still Applicable
The earlier suggestion to simplify the WrapWithExecutionResult trait and its implementation to reduce complexity remains valid. Streamlining the trait can improve readability and maintainability.
lklimek
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (8)
packages/rs-sdk/src/error.rs (1)
Line range hint
1-158: Consider addressing TODOs in future iterations.The error handling architecture is solid, but there are opportunities for future improvements:
- Decomposing
DapiClientErrorinto more specific errors- Replacing generic errors with domain-specific ones
These improvements would enhance error handling granularity and maintainability.
packages/rs-dapi-client/src/executor.rs (1)
161-162: Consider more descriptive names for the trait and type parametersThe trait and its type parameters could be more self-documenting:
- 'Wrap' could be 'WrapWithContext' or 'ExecutionContextWrapper'
- Type parameters could be more descriptive:
- R -> ResultType
- RE -> ErrorType
- W -> Context
-pub trait Wrap<R, RE, W>: Sized { +pub trait WrapWithContext<ResultType, ErrorType, Context>: Sized {packages/rs-dapi-client/src/transport/grpc.rs (4)
135-166: Enhance macro documentation with additional detailsWhile the documentation is comprehensive, consider adding:
- Safety considerations and requirements
- Error handling expectations
- Trait bounds required for the generic types (
$request,$response,$client)Add these sections to the documentation:
/// # Parameters /// /// - `$request:ty`: The request type for which the `TransportRequest` trait will be implemented. /// - `$response:ty`: The response type returned by the transport request. /// - `$client:ty`: The client type used to execute the transport request (eg. generated by `tonic` crate). /// - `$settings:expr`: The settings to be used for the transport request; these settings will override client's /// default settings, but can still be overriden by arguments to /// the [`DapiRequestExecutor::execute`](crate::DapiRequestExecutor::execute) method. /// - `$($method:tt)+`: The method of `$client` to be called to execute the request. +/// +/// # Safety and Requirements +/// +/// - The request type must implement `IntoRequest` +/// - The response type must implement `From<tonic::Response>` +/// - The client type must provide the specified method +/// +/// # Error Handling +/// +/// The generated implementation will: +/// - Convert gRPC errors into `TransportError::Grpc` +/// - Preserve timeout settings from the request configuration
Line range hint
167-198: Consider enhancing the macro implementationThe implementation could be improved in several ways:
- The timeout handling could check for negative durations
- Error handling could be more granular to distinguish between different types of gRPC errors
- The boxed future's lifetime could be more explicit
Consider this enhanced implementation:
macro_rules! impl_transport_request_grpc { ($request:ty, $response:ty, $client:ty, $settings:expr, $($method:tt)+) => { impl TransportRequest for $request { type Client = $client; type Response = $response; const SETTINGS_OVERRIDES: RequestSettings = $settings; fn method_name(&self) -> &'static str { stringify!($($method)+) } fn execute_transport<'c>( self, client: &'c mut Self::Client, settings: &AppliedRequestSettings, ) -> BoxFuture<'c, Result<Self::Response, TransportError>> { let mut grpc_request = self.into_request(); - if !settings.timeout.is_zero() { + // Ensure timeout is positive and non-zero + if settings.timeout.as_nanos() > 0 { grpc_request.set_timeout(settings.timeout); } client .$($method)+(grpc_request) .map_err(|e| match e.code() { + tonic::Code::DeadlineExceeded => TransportError::Timeout(e), + tonic::Code::Unavailable => TransportError::ConnectionFailed(e), + _ => TransportError::Grpc(e), + }) .map_ok(|response| response.into_inner()) .boxed() } } }; }
Line range hint
200-450: Improve organization and documentation of request implementationsConsider:
- Grouping related requests together with descriptive comments
- Documenting the rationale for non-default settings
- Consistent timeout handling for similar request types
For example:
// Group related requests with descriptive comments +// Identity-related requests impl_transport_request_grpc!( platform_proto::GetIdentityRequest, platform_proto::GetIdentityResponse, PlatformGrpcClient, RequestSettings::default(), get_identity ); +// Document rationale for non-default settings +/// Uses a longer timeout due to the asynchronous nature of state transition processing impl_transport_request_grpc!( platform_proto::WaitForStateTransitionResultRequest, platform_proto::WaitForStateTransitionResultResponse, PlatformGrpcClient, RequestSettings { timeout: Some(Duration::from_secs(120)), ..RequestSettings::default() }, wait_for_state_transition_result ); +// Consider consistent timeouts for all streaming operations impl_transport_request_grpc!( core_proto::TransactionsWithProofsRequest, Streaming<core_proto::TransactionsWithProofsResponse>, CoreGrpcClient, RequestSettings { timeout: Some(STREAMING_TIMEOUT), ..RequestSettings::default() }, subscribe_to_transactions_with_proofs );
Consider adding tests for retry behavior and macro implementations
Based on the code analysis:
- The retry behavior is implemented but lacks dedicated test coverage
- The
impl_transport_request_grpcmacro, while functional, would benefit from test cases- The
CanRetrytrait implementation fortonic::Statusshould be tested with different status codesSuggested locations for test additions:
packages/rs-dapi-client/src/transport/grpc.rs: Add test module with macro and retry behavior testspackages/rs-dapi-client/src/executor.rs: Expand tests for retry execution flow🔗 Analysis chain
Line range hint
1-450: Consider adding tests and documenting retry behaviorThe implementation looks solid, but consider:
- Adding unit tests for the macro with different request types and settings
- Documenting how the retry behavior (from
CanRetrytrait) interacts with different request types and timeoutsLet's verify the retry behavior implementation:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for retry-related implementations and tests echo "Checking retry behavior implementation..." rg -l "impl.*CanRetry" . rg -l "fn.*can_retry" . echo "Checking for related tests..." rg -l "mod.*tests.*CanRetry" . rg -l "#\[test\].*retry" .Length of output: 791
Script:
#!/bin/bash # Let's check the retry behavior implementation and tests in more detail echo "Checking retry behavior implementation in transport/grpc.rs..." rg -A 10 "impl CanRetry" ./packages/rs-dapi-client/src/transport/grpc.rs echo -e "\nChecking retry behavior tests..." rg -A 10 "#\[test\].*retry" ./packages/rs-dapi-client/src/transport/grpc.rs echo -e "\nChecking for test modules..." rg "mod\s+tests" ./packages/rs-dapi-client/src/transport/grpc.rs echo -e "\nChecking macro tests..." rg -A 5 "impl_transport_request_grpc.*test" ./packages/rs-dapi-client/src/transport/grpc.rsLength of output: 933
Script:
#!/bin/bash # Let's check the CanRetry trait definition and its usage in tests across the codebase echo "Checking CanRetry trait definition..." rg -A 5 "trait CanRetry" ./packages/rs-dapi-client/src/transport.rs echo -e "\nChecking for test modules in transport.rs..." rg -B 5 -A 15 "mod\s+tests" ./packages/rs-dapi-client/src/transport.rs echo -e "\nChecking for test modules in executor.rs (which uses retry)..." rg -B 5 -A 15 "mod\s+tests" ./packages/rs-dapi-client/src/executor.rs echo -e "\nChecking retry-related code in executor..." rg -A 10 "retry" ./packages/rs-dapi-client/src/executor.rsLength of output: 1011
packages/rs-sdk/tests/fetch/contested_resource.rs (2)
239-239: Consider creating an issue to track the disabled test caseThe test case is commented out due to bug PLAN-653. To ensure better tracking and visibility, it would be helpful to have this documented in the issue tracker.
Would you like me to help create a GitHub issue to track this disabled test case and its dependency on PLAN-653?
279-280: Consider adding test cases for valid limit valuesWhile the current test cases cover important boundary conditions (0 and u16::MAX), it would be beneficial to include test cases for valid limit values to ensure complete coverage of the limit validation logic.
Consider adding test cases like:
#[test_case::test_case(|q| q.limit = Some(1), Ok("ContestedResources([".into()); "limit 1 returns successfully")] #[test_case::test_case(|q| q.limit = Some(100), Ok("ContestedResources([".into()); "limit 100 returns successfully")]
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (9)
packages/rs-dapi-client/src/executor.rs(2 hunks)packages/rs-dapi-client/src/lib.rs(1 hunks)packages/rs-dapi-client/src/request_settings.rs(1 hunks)packages/rs-dapi-client/src/transport/grpc.rs(1 hunks)packages/rs-sdk/src/error.rs(4 hunks)packages/rs-sdk/src/platform/transition/broadcast.rs(1 hunks)packages/rs-sdk/src/platform/transition/put_settings.rs(2 hunks)packages/rs-sdk/src/platform/transition/transfer.rs(2 hunks)packages/rs-sdk/tests/fetch/contested_resource.rs(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- packages/rs-dapi-client/src/lib.rs
- packages/rs-dapi-client/src/request_settings.rs
- packages/rs-sdk/src/platform/transition/broadcast.rs
- packages/rs-sdk/src/platform/transition/put_settings.rs
- packages/rs-sdk/src/platform/transition/transfer.rs
🧰 Additional context used
📓 Learnings (1)
packages/rs-dapi-client/src/executor.rs (1)
Learnt from: lklimek
PR: dashpay/platform#2337
File: packages/rs-dapi-client/src/executor.rs:161-212
Timestamp: 2024-11-20T14:14:54.721Z
Learning: In `packages/rs-dapi-client/src/executor.rs`, the `WrapWithExecutionResult` trait supports on-the-fly type conversions using the `From` trait, which is useful when using the `?` operator to return errors.
🔇 Additional comments (6)
packages/rs-sdk/src/error.rs (2)
60-63: LGTM! Well-structured error variant addition.
The new AlreadyExists error variant follows Rust error handling best practices with proper documentation and meaningful payload.
Line range hint 86-108: LGTM! Improved error handling with better debugging support.
The changes enhance error handling by:
- Adding debug logging for consensus error deserialization failures
- Properly handling the
AlreadyExistsstatus code - Maintaining a clear and logical error conversion flow
packages/rs-dapi-client/src/executor.rs (3)
127-137: LGTM! Well-designed From implementations
The new From trait implementations for ExecutionResult provide clean and type-safe conversions, complementing the existing IntoInner and InnerInto traits. This enhances the ergonomics of working with execution results.
161-199: LGTM! Well-designed trait for context preservation
The Wrap trait is well-documented and provides a robust mechanism for:
- Preserving execution context (retries and address) across operation chains
- Supporting type conversions through From bounds
- Maintaining error context in a type-safe way
201-220: LGTM! Solid implementation with proper type safety
The implementation correctly:
- Uses From bounds for flexible type conversions
- Preserves execution context in both success and error paths
- Maintains type safety throughout the transformation
packages/rs-sdk/tests/fetch/contested_resource.rs (1)
235-237: LGTM: Error message format updates are consistent and descriptive
The updated error messages follow the new format using "status: InvalidArgument" prefix and provide clear context about the validation failures.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
packages/rs-sdk/tests/fetch/contested_resource_vote_state.rs (1)
283-285: Consider enhancing error validationWhile the error message format changes look good, consider enhancing these test cases by:
- Validating additional error details beyond just the message
- Adding assertions for error codes or specific error types
- Ensuring error messages are properly localized if internationalization is supported
Example enhancement:
-#[test_case(|q| q.vote_poll.document_type_name = "nx doctype".to_string(), Err(r#"status: InvalidArgument, message: "document type nx doctype not found"#); "non existing document type returns InvalidArgument")] +#[test_case(|q| q.vote_poll.document_type_name = "nx doctype".to_string(), Err(r#"status: InvalidArgument, message: "document type nx doctype not found", code: 3"#); "non existing document type returns InvalidArgument with code")]
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
packages/rs-sdk/tests/fetch/contested_resource_vote_state.rs(2 hunks)
🔇 Additional comments (1)
packages/rs-sdk/tests/fetch/contested_resource_vote_state.rs (1)
110-110: LGTM: Error message format standardization
The updated error message format aligns with the standardization effort to prepend "status:" instead of "code:" in error messages.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
packages/rs-dapi-client/src/dapi_client.rs (1)
230-234: Consider enhancing the warning log message with retry information.The warning log provides good visibility into address banning, but could be more informative by including retry-related context since this is part of the retry mechanism.
Consider this enhanced log message:
-tracing::warn!( - ?address, - ?error, - "received server error, banning address" -); +tracing::warn!( + ?address, + ?error, + retries = retries_counter.load(std::sync::atomic::Ordering::Acquire), + "received server error, banning address after failed attempt" +);This change would:
- Add retry count context to help track retry progression
- Make it clearer that this is happening during the retry process
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
packages/rs-dapi-client/src/dapi_client.rs(1 hunks)
🔇 Additional comments (2)
packages/rs-dapi-client/src/dapi_client.rs (2)
Line range hint 1-350: Implementation looks robust and well-structured.
The retry mechanism implementation demonstrates several strong points:
- Configurable retry settings through
RequestSettings - Proper error propagation with detailed context
- Comprehensive address management with ban/unban logic
- Good observability through structured logging
230-234: Verify log level consistency across retry-related messages.
The code uses warn level for both address banning and retry notifications. Let's verify this is consistent across the codebase.
✅ Verification successful
Log levels are consistent with their severity and purpose
The log levels are appropriately used across the codebase:
tracefor detailed operation tracking (requests/responses)warnfor recoverable issues (retries, address banning)errorfor non-recoverable request failures
The original concern about log level consistency is unfounded as the warn level is correctly used for both address banning and retry scenarios, which are recoverable issues that require attention but don't halt operation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for all retry-related log messages to verify level consistency
rg --type rust 'tracing::(warn|error|info|debug|trace).*retry' ./packages/rs-dapi-client/src/
Length of output: 95
Script:
#!/bin/bash
# Search for all log messages in the file to analyze logging patterns
rg --type rust 'tracing::(warn|error|info|debug|trace)!' ./packages/rs-dapi-client/src/dapi_client.rs -B 2 -A 2
Length of output: 1237
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
packages/rs-dapi-client/src/executor.rs (1)
174-174: Fix incorrect trait name in documentation exampleThe example code references
Wrapbut should referenceWrapToExecutionResult.-use rs_dapi_client::{ExecutionResponse, ExecutionResult, Wrap}; +use rs_dapi_client::{ExecutionResponse, ExecutionResult, WrapToExecutionResult};
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
packages/rs-dapi-client/src/executor.rs(2 hunks)packages/rs-dapi-client/src/lib.rs(1 hunks)packages/rs-sdk/src/platform/transition/broadcast.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/rs-dapi-client/src/lib.rs
- packages/rs-sdk/src/platform/transition/broadcast.rs
🧰 Additional context used
📓 Learnings (1)
packages/rs-dapi-client/src/executor.rs (1)
Learnt from: lklimek
PR: dashpay/platform#2337
File: packages/rs-dapi-client/src/executor.rs:161-212
Timestamp: 2024-11-20T14:14:54.721Z
Learning: In `packages/rs-dapi-client/src/executor.rs`, the `WrapWithExecutionResult` trait supports on-the-fly type conversions using the `From` trait, which is useful when using the `?` operator to return errors.
🔇 Additional comments (2)
packages/rs-dapi-client/src/executor.rs (2)
127-137: LGTM: Clean and idiomatic From implementations
The From trait implementations for ExecutionResult<R, E> are well-structured and follow Rust conventions, providing clean type conversions from both ExecutionResponse and ExecutionError.
201-220: LGTM: Well-implemented context preservation with type conversion support
The implementation effectively:
- Preserves execution context (retries and address)
- Supports type conversions through
Fromtrait bounds - Handles both success and error cases appropriately
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
packages/rs-dapi-client/src/executor.rs (2)
161-199: Consider enhancing trait documentationThe trait documentation is comprehensive, but it could be improved by explicitly mentioning its type conversion capabilities using the
Fromtrait, which is particularly useful when using the?operator.Add a note about type conversion support to the documentation:
/// Convert Result<T,TE> to ExecutionResult<R,E>, taking context from ExecutionResponse. +/// +/// This trait supports automatic type conversions through the `From` trait, making it particularly +/// useful when using the `?` operator for error propagation. pub trait WrapToExecutionResult<R, RE, W>: Sized {
201-220: Consider adding a success case testThe implementation looks good, but the documentation example only demonstrates the error case. Consider adding a success case test to ensure complete coverage.
Add this test case to the documentation:
// Success case let response = ExecutionResponse { inner: 42i8, retries: 123, address: "http://127.0.0.1".parse().expect("create mock address"), }; let result: ExecutionResult<i32, String> = Ok(100i32).wrap_to_execution_result(&response); if let ExecutionResult::Ok(success) = result { assert_eq!(success.inner, 100); assert_eq!(success.retries, 123); } else { panic!("Expected success"); }packages/dapi-grpc/build.rs (1)
Line range hint
1-302: Consider architectural improvements for maintainabilityWhile the current implementation is functional, consider the following improvements:
- Move the
VERSIONED_REQUESTSandVERSIONED_RESPONSESarrays to a separate configuration file for easier maintenance- Extract the version number (
#[grpc_versions(0)]) to a constant or configuration value- Add compile-time verification for feature flag combinations
Example structure:
// config/proto_messages.rs pub const VERSIONED_REQUESTS: [&str; 30] = [...]; pub const VERSIONED_RESPONSES: [&str; 29] = [...]; pub const CURRENT_GRPC_VERSION: u32 = 0; // build.rs use config::proto_messages::{VERSIONED_REQUESTS, VERSIONED_RESPONSES, CURRENT_GRPC_VERSION};
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
packages/dapi-grpc/build.rs(1 hunks)packages/rs-dapi-client/src/executor.rs(2 hunks)
🧰 Additional context used
📓 Learnings (1)
packages/rs-dapi-client/src/executor.rs (1)
Learnt from: lklimek
PR: dashpay/platform#2337
File: packages/rs-dapi-client/src/executor.rs:161-212
Timestamp: 2024-11-20T14:14:54.721Z
Learning: In `packages/rs-dapi-client/src/executor.rs`, the `WrapWithExecutionResult` trait supports on-the-fly type conversions using the `From` trait, which is useful when using the `?` operator to return errors.
🔇 Additional comments (2)
packages/rs-dapi-client/src/executor.rs (1)
127-137: LGTM: Clean From trait implementations
The new From trait implementations for ExecutionResult provide convenient type conversions that align well with Rust's standard patterns and complement the existing conversion traits.
packages/dapi-grpc/build.rs (1)
292-292: Verify the tonic-build API change impact
The change from .compile to .compile_protos is a significant API change that could affect the generated code structure.
Let's verify the impact of this change:
Additionally, please ensure that:
- The error handling behavior remains consistent with the previous implementation
- The generated code maintains backward compatibility
- The change is documented in the changelog if it introduces breaking changes
✅ Verification successful
Let me gather more information to verify this change properly.
The previous search didn't yield much information about the generated code structure because the paths might be different. Let's examine the build configuration and recent changes to make a more informed decision about the API change impact.
Based on the examination of the code, I can now provide a final response:
API change from .compile to .compile_protos is safe and intentional
The change is consistent with the tonic-build v0.12 API, as seen in the dependencies. The method is used correctly within the generate function of MappingConfig, maintaining all necessary configuration including:
- Proper proto file paths and includes
- Feature-based client/server generation
- Extensive attribute configurations for serialization
- Correct error handling through Result type
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any breaking changes in the generated code structure
# Check if the tonic-build version has been updated
rg -l "tonic-build.*=" .
# Look for any potential compilation issues in the generated code
fd -e rs . src/core src/platform 2>/dev/null | while read -r file; do
echo "=== Checking $file ==="
rg "(?:compile|tonic)" "$file" || true
done
Length of output: 108
Script:
#!/bin/bash
# Check the tonic-build version and dependency specification
cat ./packages/dapi-grpc/Cargo.toml
# Check the build.rs file for context around the API change
cat ./packages/dapi-grpc/build.rs
# Look for any related changes or discussions in recent commits
git log -n 5 --oneline ./packages/dapi-grpc/build.rs
Length of output: 12605
Issue being fixed or feature implemented
Modification operations, like create identity etc., don't support retries even if DAPI returns retryable error.
What was done?
How Has This Been Tested?
Breaking Changes
Option<PutSettings>Option<PutSettings>Checklist:
For repository code-owners and collaborators only
Summary by CodeRabbit
Release Notes
New Features
PutSettingsfor better handling of state transition response waits.BroadcastStateTransitionwith configurable retry settings and new methods for handling responses.transfer_creditsandwithdrawmethods to clarify their behavior.StateTransitionProofResultwith additional capabilities for formatted output and conversion.DapiClientto improve logging around banned addresses.AlreadyExistsfor more informative error reporting.Bug Fixes
Documentation
CanRetrytrait, encouraging best practices in error handling.timeoutfield inRequestSettings.