refactor(sdk)!: unify state transition processing #2338
Conversation
WalkthroughThe changes in this pull request introduce a new public module called Changes
Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
lklimek
force-pushed
the
refactor/sdk-put-dry
branch
from
b035068 to
ebf61e0
Compare
|
@coderabbitai review all |
|
@lklimek, I will review all the changes in the PR. ✅ Actions performedFull review triggered. |
lklimek
changed the title
![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 (10)
packages/rs-sdk/src/platform/transition/transfer.rs (1)
61-65: Consider enhancing error handling and validationThe new implementation is more concise and idiomatic, but could benefit from these improvements:
- The error message could be more specific about the failure condition
- Consider adding validation for the balance value
- A more specific error type might be appropriate instead of
DapiClientErrorHere's a suggested improvement:
- let identity: PartialIdentity = state_transition.broadcast_and_wait(sdk, settings).await?; - - identity.balance.ok_or(Error::DapiClientError( - "expected an identity balance after transfer".to_string(), - )) + let identity: PartialIdentity = state_transition.broadcast_and_wait(sdk, settings).await?; + + identity.balance + .filter(|&balance| balance > 0) // Optional: Validate balance if needed + .ok_or_else(|| Error::DapiClientError( + format!( + "Identity transfer failed: balance not available for identity {}", + identity.id() + ) + ))packages/rs-sdk/src/platform/transition/put_contract.rs (1)
20-20: Consider enhancing trait documentationWhile the trait changes look good, consider improving the documentation to:
- Explain what settings can be configured
- Document the impact of different settings on the behavior
- Provide examples of common settings usage
Example documentation improvement:
/// A trait for putting a contract to platform +/// +/// # Settings +/// The optional `settings` parameter allows customizing the contract deployment: +/// - Connection behavior +/// - Timeout configurations +/// - Other platform-specific settings +/// +/// # Example +/// ```rust +/// let settings = PutSettings::default().with_timeout(Duration::from_secs(30)); +/// contract.put_to_platform(sdk, key, signer, Some(settings)).await?; +/// ```Also applies to: 37-37
packages/rs-sdk/src/platform/transition/put_document.rs (1)
2-2: Good architectural improvement!The introduction of the
Waitabletrait and removal of the redundantwait_for_responsemethod fromPutDocumentfollows DRY principles by centralizing the response handling logic.packages/rs-sdk/src/platform/transition/purchase_document.rs (1)
41-41: Add documentation for the settings parameterWhile the code change is good, consider adding documentation for the
settingsparameter to explain its purpose and possible values.async fn purchase_document_and_wait_for_response( &self, price: Credits, sdk: &Sdk, document_type: DocumentType, purchaser_id: Identifier, identity_public_key: IdentityPublicKey, signer: &S, - settings: Option<PutSettings>, + /// Optional settings to configure the purchase behavior. + /// If None, default settings will be used. + settings: Option<PutSettings>, ) -> Result<Document, Error>;packages/rs-sdk/src/sdk.rs (1)
849-849: Consider enhancing the documentation.While the implementation is correct, consider adding documentation that explicitly mentions the default values that will be used if this method is not called.
packages/rs-sdk/src/platform/transition/top_up_identity.rs (1)
42-44: Enhance error message for missingidentity.balanceWhen
identity.balanceisNone, the error message returned is generic. Consider providing a more descriptive error message to aid in debugging, possibly including the identity ID or additional context.Apply the following diff to improve the error message:
identity.balance.ok_or(Error::DapiClientError( - "expected an identity balance".to_string(), + format!("Failed to retrieve balance for identity {}", self.id), ))packages/rs-sdk/src/platform/transition/put_identity.rs (2)
17-17: Address the TODO regarding consumingselfThe TODO comment suggests discussing whether
put_to_platformshould consumeselfsince the identity may no longer be valid after the operation (e.g., identity ID is changed). Consider whether changing the method to take ownership ofselfis appropriate.Would you like assistance in refactoring the method to consume
self, or should we open a GitHub issue to track this discussion?
55-55: Consider enhancing error context when broadcastingThe use of
?propagates any errors fromstate_transition.broadcast(sdk, settings).await. While this is acceptable, consider adding context to the error to facilitate easier debugging.Apply this diff to enhance error context:
- state_transition.broadcast(sdk, settings).await?; + state_transition.broadcast(sdk, settings).await.map_err(|e| { + Error::BroadcastError(format!("Failed to broadcast state transition: {}", e)) + })?;packages/rs-sdk/src/platform/transition/waitable.rs (2)
18-18: Typographical error in documentation commentThere is a typo in the comment on line 18. The word "conveniance" should be "convenience".
96-96: Address the TODO regarding error handlingThe TODO comment on line 96 indicates a need to refactor
sdk Errorto retrieve gRPC error codes and identify conflicts. Would you like assistance in refactoring theErrorhandling to facilitate this improvement?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (11)
packages/rs-sdk/src/platform/transition.rs(1 hunks)packages/rs-sdk/src/platform/transition/purchase_document.rs(4 hunks)packages/rs-sdk/src/platform/transition/put_contract.rs(3 hunks)packages/rs-sdk/src/platform/transition/put_document.rs(4 hunks)packages/rs-sdk/src/platform/transition/put_identity.rs(3 hunks)packages/rs-sdk/src/platform/transition/top_up_identity.rs(4 hunks)packages/rs-sdk/src/platform/transition/transfer.rs(2 hunks)packages/rs-sdk/src/platform/transition/transfer_document.rs(4 hunks)packages/rs-sdk/src/platform/transition/update_price_of_document.rs(3 hunks)packages/rs-sdk/src/platform/transition/waitable.rs(1 hunks)packages/rs-sdk/src/sdk.rs(6 hunks)
🔇 Additional comments (40)
packages/rs-sdk/src/platform/transition.rs (1)
16-16: LGTM! The module addition follows good practices.
The new waitable module is appropriately placed and marked as public, following the established module organization pattern.
Let's verify the integration of the new waitable module with other modules:
✅ Verification successful
The waitable module is well integrated and properly used across the codebase
The verification shows:
- The
Waitabletrait is implemented for key types:DataContract,Document, andIdentity - The trait is actively used in 6 transition-related modules:
- put_identity.rs
- update_price_of_document.rs
- put_contract.rs
- put_document.rs
- purchase_document.rs
- transfer_document.rs
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the usage of the Waitable trait across modules
# Expected: Find implementations and usage of the Waitable trait
# Check for Waitable trait implementations
ast-grep --pattern 'impl Waitable for $_'
# Check for Waitable trait usage in other modules
rg -l "use.*waitable::Waitable" packages/rs-sdk/src/platform/
Length of output: 8714
packages/rs-sdk/src/platform/transition/transfer.rs (2)
8-8: LGTM: Import addition is well-organized
The PartialIdentity import is correctly grouped with related identity types.
61-61: Verify Waitable trait integration
The code uses broadcast_and_wait, but let's verify it properly implements the new Waitable trait system mentioned in the PR summary.
✅ Verification successful
Implementation aligns with Waitable trait system
The code correctly uses broadcast_and_wait which is part of the Waitable trait system. The verification shows:
broadcast_and_waitis defined in the trait interface atpackages/rs-sdk/src/platform/transition/broadcast.rs- The method is consistently used across state transitions (top-up, transfer, withdraw)
- The implementation follows the same pattern as other state transitions in the codebase
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for Waitable trait implementation for IdentityCreditTransferTransition
ast-grep --pattern 'impl Waitable for IdentityCreditTransferTransition'
# Check if broadcast_and_wait is defined in the Waitable trait
ast-grep --pattern 'trait Waitable {
$$$
broadcast_and_wait($$$)
$$$
}'
Length of output: 153
Script:
#!/bin/bash
# Let's try a broader search for Waitable trait definition
ast-grep --pattern 'trait Waitable'
# Search for any broadcast_and_wait method implementations
ast-grep --pattern 'fn broadcast_and_wait'
# Search for state transition related traits and implementations
rg "trait.*Transition|impl.*Transition" -A 5
Length of output: 103973
Script:
#!/bin/bash
# Let's search for broadcast_and_wait method and Waitable trait in the SDK
rg "broadcast_and_wait|trait Waitable" -A 5 packages/rs-sdk/src/
# Search for implementation of broadcast_and_wait
ast-grep --pattern 'fn broadcast_and_wait'
# Search for any usage of broadcast_and_wait
rg "broadcast_and_wait" packages/rs-sdk/src/
Length of output: 4272
packages/rs-sdk/src/platform/transition/put_contract.rs (2)
16-16: LGTM: Import of Waitable trait
The import is correctly placed and follows the module structure.
84-84: Implementation correctly uses new Waitable trait
The changes properly propagate settings and utilize the new Waitable trait's functionality.
Let's verify the Waitable trait implementation:
Also applies to: 87-87, 90-90
✅ Verification successful
Let me try a different search strategy to locate the trait and its implementations.
Implementation correctly uses Waitable trait
The verification confirms that:
- The
Waitabletrait is properly defined inwaitable.rswith the requiredwait_for_responsemethod - The trait is correctly implemented for
DataContract,Document, andIdentitytypes - The implementation in
put_contract.rscorrectly uses the trait's functionality throughSelf::wait_for_response - The settings parameter is properly propagated through the trait's methods
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the Waitable trait implementation and its usage
# Expected: Find the trait definition and all its implementations
# Find the Waitable trait definition
echo "=== Waitable trait definition ==="
ast-grep --pattern 'trait Waitable {
$$$
}'
# Find implementations of Waitable
echo "=== Waitable implementations ==="
ast-grep --pattern 'impl$$ Waitable for $$ {
$$$
}'
Length of output: 255
Script:
#!/bin/bash
# Search for Waitable using ripgrep for broader context
echo "=== Searching for Waitable trait and implementations ==="
rg "Waitable" -A 5 -B 2
# Also search for wait_for_response as it was mentioned in the review
echo "=== Searching for wait_for_response usage ==="
rg "wait_for_response" -A 2 -B 2
Length of output: 24602
packages/rs-sdk/src/platform/transition/put_document.rs (3)
98-102: Clean implementation of the new waiting mechanism
The changes effectively:
- Propagate settings through the call chain
- Utilize the new
Waitabletrait for response handling - Result in more maintainable code
Line range hint 1-102: Verify version bump and documentation updates
This refactor changes public API signatures which could be breaking changes. Please verify:
- Has the SDK version been bumped appropriately?
- Have the documentation and changelog been updated?
37-37: Verify the safety of removing the data_contract parameter
While adding optional settings improves flexibility, please verify that the removal of the data_contract parameter doesn't break existing functionality.
Also applies to: 89-89
packages/rs-sdk/src/platform/transition/purchase_document.rs (3)
1-2: LGTM: Import changes are appropriate
The addition of the Waitable trait import is correctly placed and necessary for the new functionality.
110-110: LGTM: Clean implementation of the Waitable trait
The implementation correctly uses the new Waitable trait for response handling, which helps reduce code duplication.
Line range hint 96-106: Verify settings propagation
The settings parameter is correctly propagated through the call chain. Let's verify that all callers of this method have been updated accordingly.
✅ Verification successful
Let me search for more context about the usage of this method.
Let me search for the actual implementations and usages of this method in the codebase.
Let me try one more search to find any direct usage of this method.
Based on the search results, I can now generate the final response:
Settings parameter propagation is correctly implemented
The purchase_document_and_wait_for_response method is only defined as a trait method and implemented once in the codebase. The settings parameter is properly propagated through the call chain to purchase_document. There are no direct callers of this method that need to be updated, as it appears to be an internal implementation detail.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for calls to purchase_document_and_wait_for_response
rg -A 3 "purchase_document_and_wait_for_response" --type rust
Length of output: 776
Script:
#!/bin/bash
# Search for the implementation and usage of purchase_document_and_wait_for_response
ast-grep --pattern 'async fn purchase_document_and_wait_for_response($$$)'
# Also search for any calls to this method
rg "purchase_document_and_wait_for_response\(" --type rust -A 10
Length of output: 3139
Script:
#!/bin/bash
# Search for trait implementations and method calls
rg "impl.*PurchaseDocument" --type rust -A 20
rg "\.purchase_document_and_wait_for_response\(" --type rust -A 5
Length of output: 2062
Script:
#!/bin/bash
# Search for any method calls with 'purchase_document' in the name
rg "\.purchase_document.*\(" --type rust -B 2 -A 5
# Also search for any struct that might implement this trait
rg "struct.*Document" --type rust -A 5
Length of output: 100369
packages/rs-sdk/src/sdk.rs (5)
53-61: LGTM! Well-structured default settings.
The default request settings are reasonable and well-documented, with clear instructions for customization.
718-718: LGTM! Good API design making settings optional.
The change to make settings optional aligns well with the new default settings pattern.
768-768: LGTM! Consistent with the new optional settings pattern.
The default initialization to None properly enables fallback to DEFAULT_REQUEST_SETTINGS.
965-969: LGTM! Robust initialization with default settings.
The implementation correctly handles optional settings and properly initializes the client in a thread-safe manner.
Also applies to: 976-976
1039-1039: LGTM! Consistent mock implementation.
The mock SDK initialization maintains consistency with the main SDK implementation regarding settings handling.
packages/rs-sdk/src/platform/transition/top_up_identity.rs (3)
30-30: Ensure method signature in implementation matches trait definition
The top_up_identity method in the impl TopUpIdentity for Identity now includes the settings parameter. Confirm that this signature matches the updated trait definition and that the parameter is utilized correctly within the method.
40-40: Verify correct handling of settings in broadcast_and_wait
The call to broadcast_and_wait includes the settings parameter. Ensure that this method properly handles both Some and None values for settings and that it behaves as expected in different scenarios.
Run the following script to review the implementation of broadcast_and_wait:
✅ Verification successful
Let me gather more information about how settings is handled in the related methods.
Let me search for the wait_for_response implementation with a different pattern.
Based on the code analysis, I can now provide the final response:
settings parameter is properly handled in broadcast_and_wait
The implementation correctly handles both Some and None cases for the settings parameter:
- In
broadcast_and_wait, thesettingsare properly forwarded to bothbroadcastandwait_for_responsemethods - Both methods handle the
Option<PutSettings>by:- Using the provided settings to override default settings when
Some(s)is passed - Falling back to
sdk.dapi_client_settingswhenNoneis passed
- Using the provided settings to override default settings when
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Retrieve the definition of `broadcast_and_wait` to verify handling of the `settings` parameter.
# Search for the definition of `broadcast_and_wait` in the codebase
rg -A 10 -B 2 'fn broadcast_and_wait' --type rust
Length of output: 2081
Script:
#!/bin/bash
# Search for the implementation of broadcast and wait_for_response methods
rg -A 15 'fn broadcast.*settings: Option<PutSettings>' --type rust
rg -A 15 'fn wait_for_response.*settings: Option<PutSettings>' --type rust
Length of output: 3060
Script:
#!/bin/bash
# Search for the wait_for_response implementation
rg -A 15 'async fn wait_for_response' --type rust
Length of output: 7801
18-18: Verify all implementations of TopUpIdentity include the new settings parameter
Adding the settings parameter to the top_up_identity method in the TopUpIdentity trait may affect existing implementations. Ensure that all implementations of this trait have been updated to include the settings parameter to prevent potential compilation errors.
Run the following script to identify any implementations that may not include the updated parameter:
✅ Verification successful
Let me verify the trait definition and implementation to ensure we have a complete picture.
Let me check if there are any other implementations of the TopUpIdentity trait in the codebase.
All implementations of TopUpIdentity include the new settings parameter
The verification shows that there is only one implementation of TopUpIdentity trait in the codebase, which is for the Identity struct. This implementation already includes the settings: Option<PutSettings> parameter in its top_up_identity method signature, and it's being used correctly in the implementation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all implementations of `TopUpIdentity` and check for the `settings` parameter in the `top_up_identity` method.
# Search for implementations of `TopUpIdentity` without the `settings` parameter
rg -A 5 -B 2 'impl .*TopUpIdentity for' --type rust | rg -B 2 -A 5 'fn top_up_identity\(' | grep -v 'settings'
Length of output: 742
Script:
#!/bin/bash
# Get the trait definition and implementation
ast-grep --pattern 'trait TopUpIdentity {
$$$
}'
ast-grep --pattern 'impl TopUpIdentity for Identity {
$$$
}'
Length of output: 2435
Script:
#!/bin/bash
# Find all implementations of TopUpIdentity in the codebase
rg "impl.*TopUpIdentity" --type rust
Length of output: 131
packages/rs-sdk/src/platform/transition/put_identity.rs (8)
4-6: Added necessary imports for new functionality
The imports of BroadcastStateTransition, PutSettings, and Waitable are appropriate for the new trait extensions and method implementations.
10-10: Importing StateTransition is appropriate
Importing StateTransition is necessary for the updated method return types.
13-13: Applied #[async_trait::async_trait] attribute
Adding the #[async_trait::async_trait] attribute allows the trait to contain async functions, which is necessary for the asynchronous methods defined.
14-14: Extending Waitable trait in PutIdentity
The PutIdentity trait now extends Waitable, allowing it to use the wait_for_response method for consistent response handling.
45-46: Updated implementation of put_to_platform
The implementation of put_to_platform now includes settings: Option<PutSettings> and returns Result<StateTransition, Error>. Ensure that the method correctly returns the StateTransition object and that error handling is appropriate.
24-25: Updated method signature in put_to_platform
The method put_to_platform now includes settings: Option<PutSettings> and returns Result<StateTransition, Error>. Ensure that this change is propagated to all implementations and that any code calling this method is updated accordingly.
Run the following script to identify affected implementations and method calls:
✅ Verification successful
Let me gather more context about the implementations and calls to understand the impact better.
Method signature change is properly implemented
The verification shows that:
- The trait definition and its implementation in
Identityboth include thesettings: Option<PutSettings>parameter and returnResult<StateTransition, Error> - All calls to
put_to_platformin the codebase (put_document.rs,put_contract.rs, andput_identity.rs) are passing thesettingsparameter correctly
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find implementations of `put_to_platform` and calls to it that may need updating.
# Find trait implementations that define `put_to_platform`
rg -A5 'impl<.*> PutIdentity<.*> for' | rg 'fn put_to_platform'
# Find calls to `put_to_platform`
rg '(\.|::)put_to_platform\('
Length of output: 484
Script:
#!/bin/bash
# Check the implementation and surrounding context
rg -B5 -A10 'fn put_to_platform' packages/rs-sdk/src/platform/transition/put_identity.rs
# Check the calling contexts
rg -B5 -A5 '\.put_to_platform\(' packages/rs-sdk/src/platform/transition/put_document.rs
rg -B5 -A5 '\.put_to_platform\(' packages/rs-sdk/src/platform/transition/put_contract.rs
rg -B5 -A5 '\.put_to_platform\(' packages/rs-sdk/src/platform/transition/put_identity.rs
Length of output: 3340
77-77: Ensure wait_for_response handles errors appropriately
The call to Self::wait_for_response(sdk, state_transition, settings).await relies on the Waitable trait implementation. Verify that wait_for_response handles potential errors and edge cases as expected.
Run the following script to review the implementation of wait_for_response:
✅ Verification successful
Let me gather more information about the error handling in the state transition implementation.
Let me try a different approach to find the error handling implementation.
Based on the code inspection, I can now provide a final response:
wait_for_response has appropriate error handling mechanisms
The implementation shows proper error handling with:
- Result types for all error propagation
- Specific error handling for
AlreadyExistscases - Protocol error handling for invalid state transitions
- Proper error wrapping and propagation in the broadcast implementation
- Execution error handling with retry settings
- Tracing for debugging purposes
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Retrieve the implementation of `wait_for_response` for inspection.
# Find the `wait_for_response` method implementation
rg -A15 'impl Waitable for' | rg -A15 'fn wait_for_response'
Length of output: 4000
Script:
#!/bin/bash
# Search for StateTransition wait_for_response implementation
rg -A 20 "impl StateTransition" | rg -A 20 "wait_for_response"
Length of output: 62
Script:
#!/bin/bash
# Search for wait_for_response method implementations and error handling patterns
rg -A 25 "async fn wait_for_response"
Length of output: 11496
34-35: Updated method signature in put_to_platform_and_wait_for_response
The method put_to_platform_and_wait_for_response now includes settings: Option<PutSettings> and returns Result<Self, Error>. Verify that this change is consistent with the desired functionality and that all dependent code is updated accordingly.
Run the following script to find usages of this method:
✅ Verification successful
Method signature change is consistent with the codebase
The put_to_platform_and_wait_for_response method signature with settings: Option<PutSettings> parameter and Result<Self, Error> return type is correctly defined in the trait PutIdentity and consistently implemented across all related types (Identity, Document, Vote, and DataContract). The change is part of a uniform pattern where all platform transition operations support optional settings and proper error handling.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find calls to `put_to_platform_and_wait_for_response` that may need updating.
# Find calls to `put_to_platform_and_wait_for_response`
rg '(\.|::)put_to_platform_and_wait_for_response\('
Length of output: 53
Script:
#!/bin/bash
# Let's try a different approach to find the trait and its implementations
# First, let's find the trait definition
ast-grep --pattern 'trait $_ {
$$$
fn put_to_platform_and_wait_for_response($$$) -> Result<$_, $_>;
$$$
}'
# Also search for implementations
ast-grep --pattern 'impl $_ for $_ {
$$$
fn put_to_platform_and_wait_for_response($$$) -> Result<$_, $_> {
$$$
}
$$$
}'
# Additionally, let's search for any file containing this method name
rg -l "put_to_platform_and_wait_for_response"
Length of output: 510
Script:
#!/bin/bash
# Let's examine the trait definition and implementation in these specific files
# Check put_identity.rs for the trait definition and implementation
cat packages/rs-sdk/src/platform/transition/put_identity.rs
# Check other files to see if they implement the same trait
cat packages/rs-sdk/src/platform/transition/put_document.rs
cat packages/rs-sdk/src/platform/transition/vote.rs
cat packages/rs-sdk/src/platform/transition/put_contract.rs
Length of output: 15802
packages/rs-sdk/src/platform/transition/update_price_of_document.rs (6)
3-4: Added necessary imports for new functionality
The imports BroadcastStateTransition and Waitable are required for the new trait implementations and functionality.
18-18: Trait UpdatePriceOfDocument now extends Waitable
By extending the Waitable trait, all implementations of UpdatePriceOfDocument must implement the methods defined in Waitable. Ensure that this is intentional and that all necessary implementations are updated accordingly.
39-39: Added optional settings parameter to method signature
The method update_price_of_document_and_wait_for_response now includes an optional settings parameter to allow for custom configurations during execution. Verify that all calls to this method are updated to pass the appropriate settings as needed.
81-81: Refactored to use broadcast method for state transition
The state transition is now broadcasted using transition.broadcast(sdk, Some(settings)).await?;. This refactoring simplifies the process and leverages the new broadcast method. Ensure that the broadcast method handles all required functionality and error handling as expected.
92-92: Included optional settings parameter in method
The settings parameter has been added to update_price_of_document_and_wait_for_response, aligning it with the updated method signatures elsewhere. This enhances flexibility in configuration.
98-98:
Pass settings parameter consistently in method calls
In the call to update_price_of_document, settings is passed as None, despite settings being available as a parameter. This may lead to inconsistent behavior if configurations are expected.
Apply this diff to ensure consistent use of settings:
- .update_price_of_document(price, sdk, document_type, identity_public_key, signer, None)
+ .update_price_of_document(price, sdk, document_type, identity_public_key, signer, settings.clone())This change ensures that any custom configurations provided in settings are applied throughout the method calls.
Likely invalid or redundant comment.
packages/rs-sdk/src/platform/transition/transfer_document.rs (7)
1-1: Adding import of Waitable trait.
The addition of use super::waitable::Waitable; correctly imports the Waitable trait needed for extending the TransferDocument trait.
14-14: Adding necessary imports from rs_dapi_client.
The imports DapiRequest and IntoInner from rs_dapi_client are correctly added for request execution and result handling.
18-18: Extending TransferDocument trait with Waitable.
The TransferDocument trait now extends Waitable, which centralizes the response handling logic and promotes code reuse.
39-39: Adding settings parameter to method signature.
The addition of settings: Option<PutSettings> to transfer_document_to_identity_and_wait_for_response allows for customizable request settings, enhancing flexibility.
Ensure that all calls to transfer_document_to_identity_and_wait_for_response have been updated to include the new settings parameter to prevent breaking changes.
99-99: Implementing settings parameter in method definition.
The method transfer_document_to_identity_and_wait_for_response now correctly accepts settings: Option<PutSettings>, matching the trait definition and enabling optional request configurations.
108-108: Passing settings to transfer_document_to_identity.
The settings parameter is appropriately passed to transfer_document_to_identity, ensuring consistency and proper propagation of settings.
112-112: Utilizing wait_for_response method from Waitable trait.
The method now calls Self::wait_for_response(sdk, state_transition, settings).await, leveraging the Waitable trait for consistent response handling.
Review the error handling and ensure that any specific error contexts or handling present in the previous implementation are preserved with the new centralized wait_for_response method.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
packages/rs-sdk/src/platform/transition/waitable.rs (4)
15-18: Typographical error and improve documentation clarityThere is a typo in the documentation comment, and it can be rephrased for better clarity.
Apply this diff to correct the typo and enhance the documentation:
-/// Waitable trait provides a wait to wait for a response of a state transition after it has been broadcast and -/// receive altered objects. -/// -/// This is simple conveniance trait wrapping the [`BroadcastStateTransition::wait_for_response`] method. +/// The `Waitable` trait allows waiting for a response after a state transition has been broadcast, +/// enabling the retrieval of altered objects. +/// +/// This is a simple convenience trait wrapping the [`BroadcastStateTransition::wait_for_response`] method.
38-83: Consider handling multiple documents or clarify the single document expectationCurrently, the implementation expects exactly one document ID in the
DocumentsBatchstate transition.
Option 1: If supporting multiple documents is anticipated, consider modifying the implementation to handle multiple documents. This would involve iterating over all document IDs and processing them accordingly.
Option 2: If only single document handling is intended, update the documentation and error messages to reflect this constraint explicitly.
Also, ensure that the error messages provide clear and actionable information. For example, in the error at lines 48-58, you might include the expected and actual number of documents.
69-80: Handle potential absence of documents more gracefullyIn the retrieval of the document from the
documentsmap, the code returns an error if the document is not found or isNone.Consider providing more detailed error messages to aid in debugging. For example, include the
doc_idin the error message to identify which document was missing.Apply this diff to enhance the error handling:
.ok_or(Error::InvalidProvedResponse( - "did not prove the sent document".to_string(), + format!("Did not prove the sent document with ID {}", doc_id), ))? .ok_or(Error::InvalidProvedResponse( - "expected there to actually be a document".to_string(), + format!("Expected a document for ID {}, but got None", doc_id), ))?;
85-119: Address the TODO regarding gRPC error codesThere's a TODO comment indicating the need to refactor
sdk::Errorto retrieve gRPC error codes and identify conflicts.I can assist in outlining a plan to refactor
sdk::Errorto enable retrieval and handling of gRPC error codes, which will improve error identification and conflict resolution. Would you like me to help with this task?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
packages/rs-sdk/src/platform/transition/waitable.rs(1 hunks)
🔇 Additional comments (4)
packages/rs-sdk/src/platform/transition/waitable.rs (4)
1-14: LGTM!
The imports and dependencies are correctly specified, and the necessary modules are included.
20-26: LGTM!
The Waitable trait is appropriately defined with a clear asynchronous method wait_for_response.
28-36: LGTM!
The implementation of Waitable for DataContract correctly delegates the wait_for_response method.
85-119: LGTM!
The implementation of Waitable for Identity appropriately handles the response, including fetching the existing identity if it already exists.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
packages/rs-sdk/src/platform/transition/vote.rs (2)
129-129: Consider implementing retry logic before merging.The code has a TODO comment about implementing retry logic. Since this PR unifies state transition processing, it would be ideal to implement the retry mechanism now.
Would you like me to help implement a retry mechanism using the new unified state transition processing?
Line range hint
102-112: Improve error handling reliability.The comment "todo make this more reliable" and the error message suggest that the duplicate vote handling needs improvement. Consider:
- Adding specific error types for duplicate votes instead of string matching
- Implementing proper validation before attempting to fetch an existing vote
Here's a suggested improvement:
- return if e.to_string().contains("already exists") { - 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(), - )) - } else { - Err(e.into()) - } + match e { + DapiError::DuplicateVote => { + let vote = Vote::fetch(sdk, VoteQuery::new(voter_pro_tx_hash, vote_poll_id)).await?; + vote.ok_or(Error::InconsistentState("Vote reported as duplicate but not found".into())) + } + _ => Err(e.into()) + }packages/rs-sdk/src/platform/transition/transfer.rs (1)
63-67: Consider adding balance validation before transferWhile the error handling for missing balance is good, consider validating the balance before initiating the transfer.
async fn transfer_credits<S: Signer + Send>( &self, sdk: &Sdk, to_identity_id: Identifier, amount: u64, signing_transfer_key_to_use: Option<&IdentityPublicKey>, signer: S, settings: Option<PutSettings>, ) -> Result<u64, Error> { + // Validate current balance before transfer + let current_balance = self.balance().ok_or(Error::DapiClientError( + "insufficient balance for transfer".to_string(), + ))?; + if current_balance < amount { + return Err(Error::DapiClientError( + format!("insufficient balance: has {}, needs {}", current_balance, amount) + )); + } let new_identity_nonce = sdk.get_identity_nonce(self.id(), true, settings).await?; // ... rest of the implementationpackages/rs-sdk/src/platform/transition/put_document.rs (1)
Line range hint
89-102: Consider adding retry configuration in settingsSince this is a network operation, consider adding retry configuration in the settings parameter for better reliability.
pub struct PutSettings { pub user_fee_increase: Option<UserFeeIncrease>, + pub max_retries: Option<u32>, + pub retry_delay: Option<Duration>, } impl Default for PutSettings { fn default() -> Self { Self { user_fee_increase: None, + max_retries: Some(3), + retry_delay: Some(Duration::from_secs(1)), } } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
packages/rs-sdk/src/platform/transition/purchase_document.rs(4 hunks)packages/rs-sdk/src/platform/transition/put_document.rs(4 hunks)packages/rs-sdk/src/platform/transition/top_up_identity.rs(3 hunks)packages/rs-sdk/src/platform/transition/transfer.rs(2 hunks)packages/rs-sdk/src/platform/transition/vote.rs(2 hunks)packages/rs-sdk/src/platform/transition/waitable.rs(1 hunks)packages/rs-sdk/src/sdk.rs(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/rs-sdk/src/platform/transition/waitable.rs
🔇 Additional comments (13)
packages/rs-sdk/src/platform/transition/purchase_document.rs (4)
1-2: LGTM: Import changes align with the refactoring
The new imports for BroadcastStateTransition and Waitable are correctly added and necessary for the unified state transition processing.
18-18: LGTM: Correctly added Waitable supertrait
The addition of Waitable as a supertrait to PurchaseDocument properly addresses the previous review comment and aligns with the unified state transition processing approach.
110-110: LGTM: Properly utilizing Waitable trait
The implementation correctly uses the Waitable trait's wait_for_response method, which reduces code duplication and improves maintainability.
41-41: Verify the safety of removing data_contract parameter
While adding the optional settings parameter improves flexibility, please verify that the removal of the data_contract parameter doesn't break any existing functionality.
Also applies to: 96-96, 106-106
✅ Verification successful
Removal of data_contract parameter is safe
The data_contract parameter removal is safe because:
- The contract ID is obtained directly from the
document_typeparameter viadocument_type.data_contract_id() - The
DocumentTypetype already contains all necessary contract information - The change eliminates redundancy as the contract information was available through two parameters
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining direct uses of data_contract in document purchase operations
rg -A 5 "purchase_document.*data_contract" --type rust
# Search for any potential breaking changes in test files
rg -A 5 "purchase_document.*test" --type rust
Length of output: 104
Script:
#!/bin/bash
# Search for the previous implementation of purchase_document function
ast-grep --pattern 'fn purchase_document($$$)' --lang rust
# Search for any tests related to purchase_document
fd -e rs -x grep -l "purchase_document" {}
# Search for any references to data_contract in the same file
rg "data_contract" "packages/rs-sdk/src/platform/transition/purchase_document.rs"
Length of output: 469
Script:
#!/bin/bash
# Let's check the content of both files to understand the changes better
cat packages/rs-sdk/src/platform/transition/purchase_document.rs
# Check the transition.rs file for any usage patterns
cat packages/rs-sdk/src/platform/transition.rs
Length of output: 4260
packages/rs-sdk/src/platform/transition/vote.rs (1)
17-21: LGTM! Note breaking change.
The addition of the Waitable trait bound to PutVote is well-structured and aligns with the refactoring goals. This is a breaking change that requires implementers to support the Waitable trait.
Let's verify other potential implementers of PutVote that might be affected:
✅ Verification successful
Breaking change impact is minimal and contained
The addition of the Waitable trait bound to PutVote affects only one implementation in the codebase - the Vote struct in the same file. Since this is an internal implementation detail within the same module, and no external implementations of PutVote were found across the codebase, the breaking change impact is well-contained.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for PutVote implementations that might need updates
ast-grep --pattern 'impl$_? PutVote<$_?> for $_?'
Length of output: 51
Script:
#!/bin/bash
# Let's try a broader search to find PutVote implementations using ripgrep
rg "impl.*PutVote" -A 5
Length of output: 501
Script:
#!/bin/bash
# Also search for struct/type definitions that might implement PutVote
rg "struct.*:.*PutVote"
Length of output: 25
Script:
#!/bin/bash
# Let's also check where PutVote is used as a type bound
rg "where.*PutVote"
Length of output: 21
Script:
#!/bin/bash
# Search for files that mention PutVote to understand usage
rg "PutVote" --type rust
Length of output: 199
packages/rs-sdk/src/platform/transition/top_up_identity.rs (2)
12-20: LGTM: Clean trait definition with proper Waitable integration
The trait definition properly extends Waitable and includes the new settings parameter, aligning with the PR's objective to unify state transition processing.
Line range hint 31-45: Verify error handling for broadcast_and_wait
The implementation is more concise with broadcast_and_wait, but we should ensure proper error propagation.
✅ Verification successful
Error handling for broadcast_and_wait is properly implemented
The implementation shows proper error handling through the Rust Result type and error propagation:
broadcast_and_waitproperly propagates errors from bothbroadcastandwait_for_responseusing the?operator- The underlying
wait_for_responseimplementation:- Handles settings configuration errors
- Propagates DAPI client errors
- Properly converts state transition proof results to the expected type
- The top-level implementation in
top_up_identity.rscorrectly:- Propagates errors from the state transition broadcast
- Handles the case where balance is missing with a specific error message
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for error handling patterns in broadcast_and_wait implementations
ast-grep --pattern 'fn broadcast_and_wait($$$) {
$$$
Result<$_, Error>
$$$
}'
Length of output: 85
Script:
#!/bin/bash
# Let's try a different approach to find broadcast_and_wait implementation and its error handling
rg -A 10 "broadcast_and_wait" --type rust
Length of output: 4232
Script:
#!/bin/bash
# Let's check the error handling in wait_for_response since broadcast_and_wait delegates to it
rg -A 10 "wait_for_response" --type rust
Length of output: 24758
packages/rs-sdk/src/platform/transition/transfer.rs (1)
Line range hint 15-33: LGTM: Well-documented trait with clear return value specification
The trait documentation clearly explains the function's purpose, parameters, and return value. The Waitable extension is properly integrated.
packages/rs-sdk/src/platform/transition/put_document.rs (1)
Line range hint 16-38: Verify impact of removing data_contract parameter
The removal of the data_contract parameter from put_to_platform_and_wait_for_response might affect type safety. Please ensure all callers can still properly validate document types.
packages/rs-sdk/src/sdk.rs (4)
53-61: LGTM: Default request settings look reasonable
The default settings with 3 retries and no timeouts provide a good balance between reliability and flexibility. Users can still override these defaults using SdkBuilder::with_settings.
718-718: LGTM: Optional settings field enables default values pattern
The change to make settings optional is a good design choice, allowing for clear distinction between user-provided and default settings.
Also applies to: 768-768
965-969: Verify settings merge strategy
The current implementation uses override_by to combine default and user settings, but there's a previous comment from shumkov suggesting that settings should be merged on top of defaults rather than replaced. Consider discussing the merge strategy in a separate PR as suggested.
1043-1043: LGTM: Mock implementation maintains consistency
The addition of dapi_client_settings to mock SDK initialization ensures consistent behavior between mock and real implementations.
Issue being fixed or feature implemented
Some code in state transition broadcast logic is very similar. This makes maintenance a bit more difficult, and things like retry logic harder to implement.
What was done?
Waitabletrait to wait for response and receive it from the server. Moved related code from direct implementations toimpl Waitable.BroadcastStateTransitionimpl forStateTransitioninsteadHow Has This Been Tested?
GHA
Breaking Changes
use Waitableto callwait_for_response()Checklist:
For repository code-owners and collaborators only
Summary by CodeRabbit
Release Notes
New Features
Waitabletrait for improved response handling in state transitions.settingsparameters to various methods for enhanced configuration flexibility.Bug Fixes
Refactor
PurchaseDocument,PutContract,PutDocument,PutIdentity,TopUpIdentity,Transfer,TransferDocument, andUpdatePriceOfDocumenttraits by removing redundant methods and consolidating logic.Chores