v0.8.5-dev to master#205
Conversation
WalkthroughThis change set updates dependencies and minimum toolchain versions, modifies error handling for proof verification, and adjusts method signatures to align with updated APIs. The Rust toolchain is bumped from 1.81 to 1.85, and the minimum required Rust version in the package is set to 1.85. The Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant BackendTask
participant dash-sdk
User->>BackendTask: Initiate proof verification or document registration
BackendTask->>dash-sdk: Call method (with new signature/args)
dash-sdk-->>BackendTask: Return result (with new error variant if error)
BackendTask->>BackendTask: Handle error (match on GroveDBError, log proof with updated error messages)
BackendTask-->>User: Return result or error message
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (4)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (11)
Cargo.toml(2 hunks)rust-toolchain.toml(1 hunks)src/backend_task/broadcast_state_transition.rs(1 hunks)src/backend_task/contested_names/query_dpns_contested_resources.rs(2 hunks)src/backend_task/contested_names/query_dpns_vote_contenders.rs(2 hunks)src/backend_task/contested_names/query_ending_times.rs(2 hunks)src/backend_task/identity/register_dpns_name.rs(2 hunks)src/backend_task/identity/register_identity.rs(2 hunks)src/backend_task/identity/top_up_identity.rs(2 hunks)src/config.rs(1 hunks)src/model/qualified_identity/qualified_identity_public_key.rs(2 hunks)
🔇 Additional comments (25)
rust-toolchain.toml (1)
3-3:Details
❓ Verification inconclusive
Toolchain version upgraded correctly.
The Rust toolchain version has been updated from 1.81 to 1.83, which aligns with the broader dependency updates in this PR.
There's a slight mismatch between this toolchain version (1.83) and the minimum Rust version in Cargo.toml (1.85). Is this intentional to provide backward compatibility while recommending a newer version?
🏁 Script executed:
#!/bin/bash # Check if README.md also mentions the Rust version as per the comment on line 2 grep -n "Rust" README.mdLength of output: 200
We need to confirm the declared Rust minimum in Cargo.toml and any hard‑coded version references in the README. Let's run:
🏁 Script executed:
#!/bin/bash # 1. Find the rust-version field in Cargo.toml grep -R "rust-version" -n Cargo.toml # 2. Locate any occurrences of "1.85" to see where the minimum is enforced grep -R "1.85" -n . # 3. Check README.md for any pinned Rust version (e.g., "1.8*") grep -n "1.8" README.mdLength of output: 141
Mismatch Between rust-toolchain and Cargo.toml Versions
The
rust-toolchain.tomlspecifies:channel = "1.83"but your
Cargo.tomldeclares:rust-version = "1.85"Should we align the toolchain channel to
1.85, or is this version gap intentional (e.g., to preserve backward compatibility)? Please confirm.src/backend_task/identity/register_identity.rs (2)
361-361: API alignment update looks good.Adding the explicit
Noneparameter to theput_to_platform_and_wait_for_responsecall correctly aligns with the updated SDK API requirements.
374-374: API alignment update looks good in error handling branch.The explicit
Noneparameter has been consistently added to this second instance ofput_to_platform_and_wait_for_responsein the error handling branch.Cargo.toml (2)
3-3: Package version increment is appropriate.Version bump from 0.8.4 to 0.8.5 aligns with the PR title and the scope of changes.
28-28:Details
❓ Verification inconclusive
Verify that all breaking changes from the dash-sdk update are handled.
The dash-sdk dependency has been updated to a newer commit hash, which likely introduces the API changes observed in other files.
It would be helpful to verify that all breaking changes from this dependency update have been properly addressed. Consider running the following to see the difference between the old and new versions:
🏁 Script executed:
#!/bin/bash # Get the previous commit hash from git history OLD_HASH=$(git show HEAD~1:Cargo.toml | grep "dash-sdk" | grep -o 'rev = "[^"]*"' | cut -d'"' -f2) NEW_HASH="db1fcb806f051bbb20b9393392e06b45ca91395c" echo "Old hash: $OLD_HASH" echo "New hash: $NEW_HASH" # Check if we can access the repository to compare changes if gh repo view dashpay/platform &>/dev/null; then echo "Changes between versions:" gh api repos/dashpay/platform/compare/$OLD_HASH...$NEW_HASH --jq '.commits[] | {message: .commit.message, author: .commit.author.name}' || echo "Unable to fetch comparison" fiLength of output: 9245
Review Breaking Changes from dash‑sdk Update
The comparison between v1.8.0 and db1fcb8… reveals several breaking changes and new features that likely affect our usage of the SDK:
- feat(platform)!: token base support (#2383)
- fix(dpp)!: wrapping overflow issue (#2430)
- feat(platform)!: improved token validation & config update transition (#2435)
- feat(platform)!: enhanced token configuration and validation (#2439)
- fix(sdk)!: bigint for uint64 values (#2443)
- feat(platform)!: token advanced distribution & updates (#2471)
- feat(platform)!: token payment info (#2517)
Please ensure that all affected API calls and data‑model assumptions in your code have been updated accordingly. In particular:
- Verify any token‑related method signatures and return types (e.g., new token queries, proofs, payment info).
- Check that state‑transition methods handle the bigint<->uint64 changes.
- Run
cargo buildand full test suite to catch any compilation or runtime errors.src/backend_task/identity/register_dpns_name.rs (2)
135-135: API alignment change for preorder document looks good.Changing from passing
self.dpns_contract.clone()toNonealigns with the updated SDK API requirements, maintaining consistency with other similar changes in the PR.
147-147: API alignment change for domain document looks good.Changing from passing
self.dpns_contract.clone()toNonealigns with the updated SDK API requirements, maintaining consistency with similar changes throughout the codebase.src/backend_task/broadcast_state_transition.rs (1)
16-16:Details
❓ Verification inconclusive
Behavior change: Method no longer waits for confirmation
The method has changed from
broadcast_and_waitto justbroadcast, which means it will now return success as soon as the state transition is broadcasted without waiting for confirmation that it was successfully processed. This could potentially introduce race conditions if subsequent code assumes the transition has been fully processed.
🏁 Script executed:
#!/bin/bash # Look for code that depends on the immediate completion of broadcast_state_transition rg -A 3 -B 3 "broadcast_state_transition.*\.await" --glob "*.rs" | grep -v "src/backend_task/broadcast_state_transition.rs"Length of output: 123
🏁 Script executed:
#!/bin/bash # Check for any remaining uses of the old broadcast_and_wait API rg -n -C2 "broadcast_and_wait" --glob "*.rs" # List all other broadcast(...) calls (excluding the file under review) for context rg -n -C2 "broadcast(" --glob "*.rs" | grep -v "src/backend_task/broadcast_state_transition.rs"Length of output: 637
🏁 Script executed:
#!/bin/bash # Find all calls to broadcast_state_transition outside its own file rg -n "broadcast_state_transition" --glob "*.rs" | grep -v "src/backend_task/broadcast_state_transition.rs" # Show top of broadcast_state_transition.rs for context sed -n '1,50p' src/backend_task/broadcast_state_transition.rs # Show the part of add_key_to_identity.rs that still uses broadcast_and_wait sed -n '60,80p' src/backend_task/identity/add_key_to_identity.rsLength of output: 1878
Verify non-blocking broadcast behavior
The
broadcast_state_transitionhelper now callsstate_transition.broadcast(sdk, None).awaitinstead of
broadcast_and_wait, so it only waits for submission, not final confirmation. This change can introduce race conditions if downstream logic assumes the transition has been fully processed.Please review and confirm whether:
- You intend for
broadcast_state_transition(in src/backend_task/broadcast_state_transition.rs) to be non‐blocking.- All callers (e.g., in src/backend_task/mod.rs) should adopt this “submit‐only” behavior.
- Any tasks needing confirmation (like the direct use of
broadcast_and_waitin src/backend_task/identity/add_key_to_identity.rs) should continue usingbroadcast_and_waitor be refactored to use the new helper with an explicit wait.If immediate confirmation is still required, revert to
broadcast_and_waitor introduce a follow‐up wait step.src/backend_task/identity/top_up_identity.rs (2)
250-250: Method signature adaptation for SDK updateThis added
Noneparameter aligns with the updatedtop_up_identitymethod signature in the dash-sdk. This helps maintain compatibility with the new API.
264-264: Method signature adaptation for SDK updateThe same
Noneparameter has been added to the retry path, ensuring consistent handling across both paths.src/config.rs (1)
127-127: Improved error handling with explicit parse failure messageChanged
AddressList::fromtoAddressList::from_strwith an explicit error message. This improves debugging by providing a clear panic message if DAPI address parsing fails.src/backend_task/contested_names/query_dpns_contested_resources.rs (3)
53-59: Error variant adaptation for SDK updateThe error pattern matching has been updated from
GroveDBProofVerificationErrortoGroveDBErrorto align with changes in the dash-sdk's error structure. This ensures compatibility with the updated SDK.
71-83: Removed code for handling path_queryThe commented-out code reflects the SDK's error structure change, where
path_queryis no longer available in the new error variant. The removal is appropriate since this field doesn't exist in the updated error structure.
89-89: Updated handling of verification path queryAn empty vector is now used for
verification_path_query_bytessince thepath_queryfield is no longer available in the updated error structure. This is a correct adaptation to the SDK changes.src/backend_task/contested_names/query_dpns_vote_contenders.rs (4)
64-69: Updated error pattern matching to align with SDK changesThe error pattern matching has been updated to match the new
dash_sdk::ProofVerifierError::GroveDBErrorstructure instead of the previousGroveDBProofVerificationErrorvariant. This change aligns with updates to the dash-sdk dependency.
84-95: Commented out code for path_query handlingThe commented-out code that previously handled encoding of
path_queryis no longer needed due to the SDK changes. Since the new error variant doesn't include apath_queryfield, this code became obsolete.Consider removing the commented code entirely in a future cleanup PR rather than keeping it commented out.
102-102: Using empty vector for verification_path_query_bytesThis change correctly adapts to the SDK changes by providing an empty vector where previously the encoded path_query was used.
110-110: Updated error message formattingThe error message now includes a more descriptive prefix, which improves error identification and debugging.
src/backend_task/contested_names/query_ending_times.rs (4)
65-70: Updated error pattern matching to align with SDK changesSimilar to the changes in the vote_contenders file, the error pattern matching has been updated to match the new
dash_sdk::ProofVerifierError::GroveDBErrorstructure instead of the previousGroveDBProofVerificationErrorvariant. This change maintains consistency across the codebase with the updated dash-sdk dependency.
85-96: Commented out code for path_query handlingThe commented-out code that previously handled encoding of
path_queryis no longer needed due to the SDK changes. Since the new error variant doesn't include apath_queryfield, this code became obsolete.Consider removing the commented code entirely in a future cleanup PR rather than keeping it commented out, which would improve code maintainability.
103-103: Using empty vector for verification_path_query_bytesThis change correctly adapts to the SDK changes by providing an empty vector where previously the encoded path_query was used.
111-111: Updated error message formattingThe error message now includes a more descriptive prefix, which improves error identification and debugging.
src/model/qualified_identity/qualified_identity_public_key.rs (3)
4-5: Updated imports for address derivation functionalityThe imports have been updated to include the necessary types for the refactored address derivation approach, including
Address,Network,PublicKey, and the appropriate accessor traits.
42-46: Refactored address derivation to be more explicitThe code now explicitly constructs a
PublicKeyfrom raw bytes and then derives a P2PKH address, making the process more direct and clear. The use ofexpectassumes the public key data will always be valid, which could be a potential failure point.Consider adding a fallback mechanism if the public key data is invalid, rather than using
expectwhich will panic the application. For example:-let pubkey = - PublicKey::from_slice(value.data().as_slice()).expect("Expected valid public key"); - -let address = Address::p2pkh(&pubkey, network); +let pubkey = match PublicKey::from_slice(value.data().as_slice()) { + Ok(key) => key, + Err(e) => { + tracing::error!("Invalid public key data: {}", e); + return Self { + identity_public_key: value, + in_wallet_at_derivation_path: None, + }; + } +}; + +let address = Address::p2pkh(&pubkey, network);
47-58: Updated wallet iteration logicThe iteration logic correctly uses the newly constructed address to check for matches in known wallet addresses. The logic flow remains the same, but now uses the directly derived address.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
rust-toolchain.toml(1 hunks)src/backend_task/identity/transfer.rs(1 hunks)src/context_provider.rs(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- rust-toolchain.toml
🔇 Additional comments (4)
src/backend_task/identity/transfer.rs (1)
32-32: Updated to match the new return type structureThe change correctly adapts to the modified return type of the
transfer_creditsmethod, which now appears to return a tuple or struct instead of a direct value. This change aligns with broader API standardization in the PR.src/context_provider.rs (3)
8-8: New import supports method signature updates.The added import
PlatformVersionfromdash_sdk::dpp::versionis necessary for the updatedget_data_contractmethod signature.
26-30: Improved code formatting enhances readability.The method signature has been reformatted with parameters on separate lines, improving readability without changing functionality.
31-32: Restructured assignment improves readability.The
cookie_pathassignment has been reformatted for better code organization, maintaining the same functionality.
| fn get_data_contract( | ||
| &self, | ||
| data_contract_id: &dash_sdk::platform::Identifier, | ||
| platform_version: &PlatformVersion, |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
New parameter added but unused in implementation.
The platform_version parameter has been added to align with updated SDK requirements but isn't currently used in the method body.
This is likely required for compatibility with the updated dash-sdk dependency. If you plan to use this parameter in future work, consider adding a TODO comment.
🏁 Script executed:
#!/bin/bash
# Check if other implementations of `get_data_contract` in the codebase use the platform_version parameter
rg -A 5 "fn get_data_contract.+platform_version.+\{" --type rustLength of output: 66
🏁 Script executed:
#!/bin/bash
# Locate the get_data_contract implementation and check for platform_version usage
rg -n "fn get_data_contract" -A 20 src/context_provider.rs
rg -n "platform_version" -n src/context_provider.rsLength of output: 1096
Document or remove unused platform_version parameter
The platform_version parameter in src/context_provider.rs:75 is currently not referenced in the method body. To avoid dead‑code warnings and clarify intent:
- If you’re reserving this parameter for future SDK compatibility, please add a
// TODOexplaining how and when it will be used. - Otherwise, rename it to
_platform_versionor remove it until it’s needed.
Location:
- src/context_provider.rs:75
Proposed diff:
- platform_version: &PlatformVersion,
+ // TODO: use `platform_version` for SDK compatibility once dash‑sdk ≥ X.Y is supported
+ platform_version: &PlatformVersion,
1 participant
Summary by CodeRabbit