refactor(sdk): contested resource as struct type #2225
Conversation
WalkthroughThe pull request includes updates across multiple files, primarily focusing on the Changes
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (15)
💤 Files with no reviewable changes (7)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used🪛 Biome
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 (
|
QuantumExplorer
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: 0
🧹 Outside diff range and nitpick comments (5)
packages/rs-sdk/Cargo.toml (1)
Line range hint
76-86: Informative comments added for testing features.The added comments for
offline-testingandnetwork-testingfeatures provide clear explanations about their behavior and precedence. This improves code maintainability and user understanding.Consider adding a brief explanation of what each testing mode does. For example:
# Run integration tests using test vectors from `tests/vectors/` instead of connecting to live Dash Platform. +# This mode uses pre-recorded responses for testing, ensuring consistent and reproducible results. # # This feature is enabled by default to allow testing without connecting to the Dash Platform as # part of CI/CD workflows. # # If both `offline-testing` and `network-testing` are enabled, "offline-testing" will take precedence. offline-testing = ["mocks"] # Run integration tests using a live Dash Platform network. +# This mode connects to an actual Dash Platform instance, allowing for real-world interaction testing. # # Requires configuration of Dash Platform connectivity. # See [README.md] for more details. # # If both `offline-testing` and `network-testing` are enabled, "offline-testing" will take precedence. network-testing = ["mocks"]packages/rs-sdk/tests/fetch/contested_resource.rs (3)
108-108: LGTM! Consider adding a type annotation for clarity.The change from
ContestedResource::Value(start_value)toContestedResource(start_value)simplifies the code and reflects the updated structure ofContestedResource. This change is correct and aligns with the new implementation.For improved readability, consider adding a type annotation to
start_value:- let ContestedResource(start_value) = start.clone(); + let ContestedResource(start_value): ContestedResource = start.clone();This would make the type of
start_valueexplicit and help future readers understand the code more easily.
220-222: LGTM! Consider consistent formatting for improved readability.The change from
ContestedResource::Value(last)toContestedResource(last)correctly reflects the updated structure ofContestedResource. This modification is consistent with the changes made in thecontested_resources_start_at_valuefunction.For improved readability and consistency with the rest of the codebase, consider adjusting the formatting slightly:
- let ContestedResource(last) = - rss.0.into_iter().last().expect("last contested resource"); + let ContestedResource(last) = rss.0 + .into_iter() + .last() + .expect("last contested resource");This change maintains the single-line destructuring while improving the overall readability of the code.
PLAN-656 References Found Without Corresponding Issue Tracker Entry
The codebase contains multiple references to PLAN-656 as TODO comments and disabled tests. However, no corresponding issue was found in the GitHub issue tracker.
- Action Items:
- Verify if PLAN-656 exists in an internal issue tracking system.
- If it exists internally, ensure that it's documented in the public issue tracker for better visibility.
- If PLAN-656 has been resolved, update the code comments and re-enable the affected tests.
- If PLAN-656 is no longer relevant, consider removing the related comments and tests to maintain code cleanliness.
🔗 Analysis chain
Line range hint
1-465: Overall changes look good. Consider addressing PLAN-656.The modifications to this file consistently reflect the updated structure of
ContestedResource, changing from an enum to a struct. These changes are minimal and focused, maintaining the existing test behavior while adapting to the new type structure.The presence of "PLAN_656" in the
contested_resources_limit_PLAN_656function name suggests an ongoing issue or plan. It would be beneficial to verify the status of this issue and update the function name or add a comment if it has been resolved.To check the status of PLAN-656, you can run the following command:
If the issue has been resolved, consider updating the function name or adding a comment to explain its historical context.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for any mentions of PLAN-656 in the codebase or issue tracker rg -i "PLAN-656|PLAN 656" --type rust gh issue list --search "PLAN-656 in:title,body"Length of output: 602
packages/rs-drive-proof-verifier/src/proof.rs (1)
1382-1382: Simplify themapfunction by removing the closureYou can make the code more concise by passing the
ContestedResourceconstructor directly to themapfunction instead of using a closure.Apply this diff to simplify the code:
- items.into_iter().map(|v| ContestedResource(v)).collect(); + items.into_iter().map(ContestedResource).collect();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (6)
- packages/rs-dapi-client/Cargo.toml (2 hunks)
- packages/rs-drive-proof-verifier/src/proof.rs (1 hunks)
- packages/rs-drive-proof-verifier/src/types.rs (4 hunks)
- packages/rs-sdk/Cargo.toml (1 hunks)
- packages/rs-sdk/src/lib.rs (1 hunks)
- packages/rs-sdk/tests/fetch/contested_resource.rs (2 hunks)
🧰 Additional context used
🔇 Additional comments (10)
packages/rs-dapi-client/Cargo.toml (2)
8-8: LGTM: New featuretokio-sleepaddedThe addition of the
tokio-sleepfeature is a good improvement. It allows users to opt-in for tokio-based sleep functionality, which is likely used for backoff or retry mechanisms. This change enhances the flexibility of the package.
23-23: Verify thebackondependency configurationThe modification of the
backondependency might lead to unexpected behavior:
- Removing
default-features = falsemeans the package will now use the default features ofbackon, which may include unnecessary dependencies.- The explicit
features = ["tokio-sleep"]has been removed, which seems to contradict the addition of thetokio-sleepfeature we just reviewed.To ensure consistency and avoid potential issues, consider the following options:
- If
tokio-sleepis intended to be an optional feature:backon = { version = "1.2", default-features = false, optional = true }Then update the
tokio-sleepfeature to:tokio-sleep = ["dep:backon", "backon/tokio-sleep"]
- If
tokio-sleepshould always be available:backon = { version = "1.2", default-features = false, features = ["tokio-sleep"] }Please verify the intended behavior and adjust the configuration accordingly.
To check the impact of this change, you can run:
This will show the dependency tree and help verify if
backonis correctly included with thetokio-sleepfeature.packages/rs-sdk/src/lib.rs (1)
79-79: Approved: New export enhances SDK functionality.The addition of
pub use drive_proof_verifier::types as query_types;is a good enhancement to the SDK's interface. It makes additional types available for complex query operations, which can improve the SDK's usability.Consider adding a brief documentation comment above this line to explain the purpose and contents of the
query_typesmodule. This will help users understand what types are available and how they can be used.Example:
/// Re-exports types from the drive_proof_verifier module for query operations. pub use drive_proof_verifier::types as query_types;To ensure this change doesn't introduce any breaking changes, please run the following script:
packages/rs-sdk/Cargo.toml (3)
Line range hint
3-3: Version update looks good.The package version has been updated to 1.4.0-dev.7, which follows semantic versioning principles and indicates a development version.
Line range hint
1-114: Overall, the changes to Cargo.toml are well-structured and beneficial.The version update, new feature addition, and improved documentation for testing features all contribute to better project management and developer understanding. The suggestions provided are minor and aimed at further enhancing clarity.
60-60: Newtokio-sleepfeature looks good.The addition of the
tokio-sleepfeature, which depends onrs-dapi-client/tokio-sleep, is clear and consistent with the project structure.To ensure this feature is properly utilized, please run the following script to check for its usage:
packages/rs-drive-proof-verifier/src/types.rs (4)
244-247: SimplifiedInto<Value>implementationThe
Into<Value>implementation forContestedResourcehas been updated to directly return the innerValue. This change is consistent with the new struct definition and simplifies the conversion process, making it more efficient.
257-257: Updated serialization and deserialization forContestedResourceThe
platform_encodeandplatform_versioned_decodeimplementations forContestedResourcehave been updated to work with the new struct definition. They now directly encode/decode the innerValue, which is consistent with the changes made to the type.These modifications ensure that serialization and deserialization work correctly with the new
ContestedResourcetype.Also applies to: 267-267
227-228: SimplifiedContestedResourcetypeThe change from an enum to a struct with a single
Valuefield simplifies theContestedResourcetype. This makes it more straightforward to use and understand.However, consider the following points:
- The public field allows direct access to the
Value, which might break encapsulation. Consider making the field private and providing a getter method if direct access is necessary.- This change might affect other parts of the codebase that expect
ContestedResourceto be an enum. Ensure that all usages of this type have been updated accordingly.To verify the impact of this change, run the following script:
Line range hint
227-267: Summary ofContestedResourcechangesThe modifications to
ContestedResourceand its related implementations have simplified the type and made it more straightforward to use. The changes include:
- Converting
ContestedResourcefrom an enum to a struct with a singleValuefield.- Updating the
Into<Value>implementation to directly return the innerValue.- Modifying the
platform_encodeandplatform_versioned_decodeimplementations to work with the new struct definition.These changes should improve code readability and potentially performance. However, it's crucial to ensure that these modifications don't negatively impact other parts of the codebase that may rely on the previous enum structure of
ContestedResource.Consider the following steps to ensure a smooth transition:
- Review all usages of
ContestedResourcethroughout the codebase to update any code that might have been relying on the enum structure.- Update any documentation or comments that describe the
ContestedResourcetype to reflect its new structure.- If there are any public APIs that expose
ContestedResource, consider versioning the API or providing a migration guide for users of your library.To help identify potential areas that might need updates, run the following script:
This will help you identify areas of the code and documentation that might need to be updated in light of these changes.
Issue being fixed or feature implemented
changed
to
pub struct ContestedResource(pub Value)What was done?
Also some minor fixes to tokio sleeper feature.
How Has This Been Tested?
Breaking Changes
Breaking but since this is not used yet in production we can let it slide.
Checklist:
For repository code-owners and collaborators only
Summary by CodeRabbit
Release Notes
New Features
tokio-sleepfeature in SDK and DAPI client for enhanced asynchronous operations.query_typesin the SDK to facilitate complex queries.Improvements
ContestedResourceby changing it from an enum to a struct.Tests
ContestedResourcestructure.Chores