fix(platform)!: properly use withdrawal system data contract. #2675
+4,117
−87
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
""" WalkthroughThis set of changes introduces enhanced error reporting, logging, and configuration flexibility across the withdrawal and data contract modules. It adds new helper methods for fetching withdrawal documents, updates test expectations, refines serialization and deserialization logic, and centralizes system contract configuration logic. Several tests and comments are updated for clarity and accuracy. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant SystemDataContract
participant PlatformVersion
participant DataContractFactory
Caller->>SystemDataContract: configuration_in_platform_version(platform_version)
SystemDataContract->>PlatformVersion: (reads version info)
SystemDataContract-->>Caller: DataContractConfig
Caller->>DataContractFactory: create(..., DataContractConfig, ...)
DataContractFactory-->>Caller: DataContract
sequenceDiagram
participant Drive
participant Transaction
participant PlatformVersion
participant WithdrawalContractCache
Drive->>WithdrawalContractCache: load withdrawals contract
Drive->>Drive: query all withdrawal documents
Drive-->>Drive: group documents by status, sort by updatedAt
Drive-->>Caller: BTreeMap<status, Vec<Document>>
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (19)
✨ Finishing Touches
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:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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: 1
♻️ Duplicate comments (2)
packages/rs-drive/tests/query_tests.rs (2)
6551-6552: Duplicate literal root hashSame vector re-appears here. Re-use the shared constant/helper proposed above.
6652-6653: Yet another copy of the same hashThird occurrence – extract to one place to keep the tests DRY and less error-prone.
🧹 Nitpick comments (4)
packages/rs-drive/tests/query_tests.rs (1)
6471-6472: Hard-coded root hash updated – please centralise itThe new 32-byte vector is now the third literal copy in this module. Consider extracting a
const EXPECTED_WITHDRAWAL_HASH: [u8; 32] = *b"...";(or decode from a hex string) and reference it in each assertion to avoid triple maintenance next time the hash shifts.packages/rs-dpp/src/data_contract/serialized_version/v1/mod.rs (1)
1-2: Defaulting toDataContractConfigV1may break backward-compatibilityChanging the serde
defaultprovider from V0 to V1 means that deserializing legacy contracts without aconfigfield will now get V1 defaults, not V0.
If V1 introduces semantics that older nodes don’t understand (or if hashes are included in merkle proofs) this becomes a consensus risk.Please verify:
- Whether platform version gating already ensures only v9+ nodes see these objects.
- That V1 default values are binary-compatible with the previous V0 defaults, or migrations are handled elsewhere.
If unsure, keep the provider on V0 and upgrade through an explicit contract bump.
Also applies to: 28-31
packages/rs-dpp/src/system_data_contracts.rs (1)
19-42: Consider grouping contracts by configuration type for better readabilityThe implementation correctly handles different configurations for system contracts. Consider reorganizing the match arms to group by configuration type for improved readability:
impl ConfigurationForSystemContract for SystemDataContract { fn configuration_in_platform_version( &self, platform_version: &PlatformVersion, ) -> Result<DataContractConfig, ProtocolError> { + let mut config = DataContractConfig::default_for_version(platform_version)?; + + let sized_integer_types_enabled = match self { + SystemDataContract::TokenHistory | SystemDataContract::KeywordSearch => true, + _ => false, + }; + + config.set_sized_integer_types_enabled(sized_integer_types_enabled); + Ok(config) - match self { - SystemDataContract::Withdrawals - | SystemDataContract::MasternodeRewards - | SystemDataContract::FeatureFlags - | SystemDataContract::DPNS - | SystemDataContract::Dashpay - | SystemDataContract::WalletUtils => { - let mut config = DataContractConfig::default_for_version(platform_version)?; - config.set_sized_integer_types_enabled(false); - Ok(config) - } - SystemDataContract::TokenHistory | SystemDataContract::KeywordSearch => { - let mut config = DataContractConfig::default_for_version(platform_version)?; - config.set_sized_integer_types_enabled(true); - Ok(config) - } - } } }packages/rs-drive/src/drive/identity/withdrawals/document/fetch_oldest_withdrawal_documents_by_status/v0/mod.rs (1)
406-548: Consider extracting document parsing logic to reduce duplicationThe test effectively verifies the grouped fetch functionality. However, the document parsing logic (lines 443-502) is duplicated from the previous test. Consider extracting this into a helper function to improve maintainability.
Create a helper function to reduce duplication:
fn parse_and_setup_withdrawal_documents( drive: &Drive, data_contract: &DataContract, document_type: &DocumentType, transaction: TransactionArg, platform_version: &PlatformVersion, ) -> Result<(usize, usize), Error> { // Move the common parsing logic here // Return (total_count, queued_count) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
packages/rs-dpp/src/data_contract/config/v0/mod.rs(1 hunks)packages/rs-dpp/src/data_contract/document_type/property/mod.rs(1 hunks)packages/rs-dpp/src/data_contract/mod.rs(0 hunks)packages/rs-dpp/src/data_contract/serialized_version/v1/mod.rs(2 hunks)packages/rs-dpp/src/document/serialization_traits/platform_serialization_conversion/mod.rs(1 hunks)packages/rs-dpp/src/system_data_contracts.rs(2 hunks)packages/rs-drive-abci/src/execution/platform_events/withdrawals/pool_withdrawals_into_transactions_queue/v1/mod.rs(4 hunks)packages/rs-drive-abci/src/query/document_query/v0/mod.rs(8 hunks)packages/rs-drive/src/drive/document/query/query_documents/v0/mod.rs(1 hunks)packages/rs-drive/src/drive/identity/withdrawals/document/fetch_oldest_withdrawal_documents_by_status/mod.rs(2 hunks)packages/rs-drive/src/drive/identity/withdrawals/document/fetch_oldest_withdrawal_documents_by_status/v0/mod.rs(9 hunks)packages/rs-drive/src/error/mod.rs(1 hunks)packages/rs-drive/tests/deterministic_root_hash.rs(1 hunks)packages/rs-drive/tests/query_tests.rs(3 hunks)packages/rs-drive/tests/supporting_files/contract/withdrawals/withdrawals-contract.json(1 hunks)packages/rs-platform-version/src/version/drive_versions/v4.rs(2 hunks)packages/rs-platform-version/src/version/v9.rs(3 hunks)
💤 Files with no reviewable changes (1)
- packages/rs-dpp/src/data_contract/mod.rs
🧰 Additional context used
📓 Path-based instructions (2)
`packages/rs-drive/**`: Drive is the platform's decentralized storage component, implementing a replicated state machine for storing and retrieving application data.
packages/rs-drive/**: Drive is the platform's decentralized storage component, implementing a replicated state machine for storing and retrieving application data.
packages/rs-drive/tests/supporting_files/contract/withdrawals/withdrawals-contract.jsonpackages/rs-drive/tests/deterministic_root_hash.rspackages/rs-drive/src/error/mod.rspackages/rs-drive/src/drive/document/query/query_documents/v0/mod.rspackages/rs-drive/src/drive/identity/withdrawals/document/fetch_oldest_withdrawal_documents_by_status/mod.rspackages/rs-drive/src/drive/identity/withdrawals/document/fetch_oldest_withdrawal_documents_by_status/v0/mod.rspackages/rs-drive/tests/query_tests.rs
`packages/rs-dpp/**`: This directory contains the Rust implementation of the Dash Platform Protocol (DPP), which defines data structures and validation rules.
packages/rs-dpp/**: This directory contains the Rust implementation of the Dash Platform Protocol (DPP), which defines data structures and validation rules.
packages/rs-dpp/src/data_contract/serialized_version/v1/mod.rspackages/rs-dpp/src/data_contract/document_type/property/mod.rspackages/rs-dpp/src/data_contract/config/v0/mod.rspackages/rs-dpp/src/document/serialization_traits/platform_serialization_conversion/mod.rspackages/rs-dpp/src/system_data_contracts.rs
🧠 Learnings (1)
📓 Common learnings
Learnt from: QuantumExplorer
PR: dashpay/platform#2257
File: packages/rs-drive-abci/src/mimic/test_quorum.rs:159-164
Timestamp: 2024-11-20T16:16:01.830Z
Learning: QuantumExplorer prefers not to receive auto-generated messages asking to post on social media.
⏰ Context from checks skipped due to timeout of 90000ms (16)
- GitHub Check: Rust packages (dpp) / Check each feature
- GitHub Check: Rust packages (dpp) / Tests
- GitHub Check: Rust packages (dpp) / Linting
- GitHub Check: Rust packages (wasm-dpp) / Unused dependencies
- GitHub Check: Rust packages (wasm-dpp) / Formatting
- GitHub Check: Rust packages (wasm-dpp) / Tests
- GitHub Check: Rust packages (dash-sdk) / Tests
- GitHub Check: Rust packages (wasm-dpp) / Linting
- GitHub Check: Rust packages (dash-sdk) / Linting
- GitHub Check: Rust packages (dash-sdk) / Check each feature
- GitHub Check: Rust packages (dash-sdk) / Formatting
- GitHub Check: Rust packages (dash-sdk) / Unused dependencies
- GitHub Check: Build Docker images (Dashmate helper, dashmate-helper, dashmate-helper) / Build Dashmate helper image
- GitHub Check: Build JS packages / Build JS
- GitHub Check: Build Docker images (Drive, drive, drive-abci) / Build Drive image
- GitHub Check: Build Docker images (DAPI, dapi, dapi) / Build DAPI image
🔇 Additional comments (25)
packages/rs-drive/src/error/mod.rs (1)
70-72: Enhanced error reporting with contextual informationThis addition improves debugging capabilities by allowing protocol errors to be wrapped with additional context strings. The implementation correctly formats the error message to include both the protocol error and the contextual information.
packages/rs-drive/tests/supporting_files/contract/withdrawals/withdrawals-contract.json (1)
2-2: Contract format version updated to align with platform versioningThe format version upgrade from "0" to "1" aligns with the broader platform versioning updates and data contract serialization changes. This change is consistent with the PR objective to properly use the withdrawal system data contract.
packages/rs-platform-version/src/version/drive_versions/v4.rs (2)
52-52: Documentation update for contract version changeThe comment addition helps track that the contract field was modified in this version, which is good practice for version documentation.
105-105: Documentation update for grove version fieldThe comment helps document the grove_version field, improving code maintainability.
packages/rs-drive/tests/deterministic_root_hash.rs (1)
305-305: Updated test expectation for newer protocol versionsThe hash expectation update reflects changes in data contract serialization and platform versioning for protocol versions above 8. This correctly maintains backward compatibility while accommodating the new hash calculation results.
packages/rs-dpp/src/data_contract/document_type/property/mod.rs (1)
534-537: ```shell
#!/bin/bashShow the imports and constants near the top of the file to check for existing limits
sed -n '1,200p' packages/rs-dpp/src/data_contract/document_type/property/mod.rs
Explanation: Inspecting existing constants and imports to ensure we don't duplicate or conflict with other limits before adding MAX_VARINT_LEN. </details> <details> <summary>packages/rs-dpp/src/data_contract/config/v0/mod.rs (1)</summary> `103-116`: **Default impl refactor looks good** Swapping `std::default::Default` for `Default` is idiomatic and keeps behaviour unchanged. </details> <details> <summary>packages/rs-platform-version/src/version/v9.rs (1)</summary> `8-9`: **Ensure all version constants stay in lock-step** Importing `DPP_METHOD_VERSIONS_V2` and wiring it into `PLATFORM_V9` is correct, but note that: * `STATE_TRANSITION_METHOD_VERSIONS_V1` remains unchanged – confirm V2 methods don’t require a bump here. * Doc comment was updated to reflect token support; great for clarity. No action required if the above matrix was validated in CI. Also applies to: 32-32, 56-56 </details> <details> <summary>packages/rs-drive-abci/src/query/document_query/v0/mod.rs (5)</summary> `775-778`: **LGTM: Consistent value type normalization.** The change from various unsigned integer types to `Value::I64` for document properties aligns with the broader standardization effort mentioned in the AI summary. This ensures compatibility with the enhanced document processing improvements. --- `939-942`: **LGTM: Test data consistency maintained.** The systematic conversion to `Value::I64` for withdrawal document properties maintains test consistency while supporting the unified document handling approach. --- `1103-1106`: **LGTM: Proper value type unification.** The change to `Value::I64` for document properties continues the consistent pattern throughout the test suite. --- `1259-1262`: **LGTM: Final test case alignment.** The conversion to `Value::I64` completes the systematic update across all test cases. --- `1304-1308`: **LGTM: Query clause consistency.** The update of query clause values to use `Value::I64` ensures compatibility with the document property changes and maintains test coherence. Also applies to: 1430-1433, 1475-1479, 1489-1490, 1497-1497 </details> <details> <summary>packages/rs-drive/src/drive/document/query/query_documents/v0/mod.rs (2)</summary> `95-105`: **LGTM: Enhanced error context for debugging.** The new error handling wraps deserialization failures with valuable debugging context including hex-encoded document bytes and contract information. This will significantly improve troubleshooting when document deserialization fails. --- `106-106`: **LGTM: Proper error type propagation.** The change from `ProtocolError` to `Error` in the collect result type is necessary to handle the new `Error::ProtocolWithInfoString` variant. </details> <details> <summary>packages/rs-dpp/src/document/serialization_traits/platform_serialization_conversion/mod.rs (1)</summary> `146-200`: **LGTM: Comprehensive withdrawal deserialization test.** This test provides excellent coverage for withdrawal document deserialization with: - Detailed comments explaining each byte segment of the serialized format - Real-world withdrawal document data - Proper use of latest platform version and contract loading The extensive documentation of the serialization format (header, metadata, user properties) is particularly valuable for understanding the withdrawal document structure. </details> <details> <summary>packages/rs-drive-abci/src/execution/platform_events/withdrawals/pool_withdrawals_into_transactions_queue/v1/mod.rs (3)</summary> `45-99`: **Enhanced observability with comprehensive withdrawal status logging.** The addition of detailed debug logging when no queued withdrawals are found provides valuable insights into system state. The code properly: - Fetches and categorizes withdrawals by all status types - Logs meaningful counts for each status - Uses appropriate debug level for operational information --- `107-112`: **LGTM: Appropriate trace logging for detailed diagnostics.** The trace-level logging for withdrawal limit calculations provides detailed diagnostic information without cluttering regular debug output. --- `154-158`: **LGTM: Clear logging for empty document processing.** The debug log clearly indicates when no documents are available for processing after applying withdrawal limits. </details> <details> <summary>packages/rs-drive/src/drive/identity/withdrawals/document/fetch_oldest_withdrawal_documents_by_status/mod.rs (1)</summary> `7-7`: **LGTM: Well-designed helper method with proper versioning.** The new `fetch_oldest_withdrawal_documents` method provides a useful complement to the existing status-filtered method by returning all withdrawal documents grouped by status. The implementation follows the established versioning pattern and has a clear, descriptive name that indicates it fetches documents without status filtering. The `BTreeMap<u8, Vec<Document>>` return type is appropriate for grouping documents by status values. Also applies to: 42-63 </details> <details> <summary>packages/rs-dpp/src/system_data_contracts.rs (3)</summary> `7-18`: **Well-designed trait for system contract configuration** The new `ConfigurationForSystemContract` trait provides a clean abstraction for retrieving contract-specific configurations based on platform version. The imports and trait definition follow Rust best practices. --- `62-67`: **Correct integration of configuration mechanism** The modification properly uses the new trait method to provide contract-specific configuration instead of the previous hardcoded `None` value. --- `157-170`: **Good test coverage for configuration versioning** The comments clearly explain the configuration version differences, and the byte length assertions effectively verify that the configuration changes impact serialization as expected. </details> <details> <summary>packages/rs-drive/src/drive/identity/withdrawals/document/fetch_oldest_withdrawal_documents_by_status/v0/mod.rs (2)</summary> `23-26`: **Efficient use of cached system contracts** Good optimization to use the cached withdrawals contract instead of loading it each time. This improves performance by avoiding redundant contract loading. --- `270-404`: **Excellent test coverage with real testnet data** Great approach to test with actual testnet withdrawal documents. The test thoroughly verifies: - Document limit enforcement - Sorting by updatedAt timestamp - Status filtering correctness </details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
...rc/drive/identity/withdrawals/document/fetch_oldest_withdrawal_documents_by_status/v0/mod.rs
Outdated
Show resolved
Hide resolved
QuantumExplorer
changed the title
pauldelucia
approved these changes
Jun 24, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Issue being fixed or feature implemented
Two big issues here: the first is that system contracts were changing their configuration on load, going from config v0 to config v1. This would cause them to start using the typed integers and all hell would break loose. Now instead we continue to use non typed integers in pre 2.0 system contracts.
The second issue is that for some methods we were getting the withdrawal contract from disk (or cache) and for some methods we were getting it from the system contract. Now we only get it from the system contract.
How Has This Been Tested?
Added tests.
Breaking Changes
Yes
Checklist:
For repository code-owners and collaborators only
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Tests
Documentation