feat(platform): allow new tokens on contract update and refactor contract struct validations #2542
Conversation
WalkthroughThis update introduces versioned basic structure validation for data contract create and update state transitions, including new validation modules and traits for both transition types. The validation logic for data contract tokens and groups is refactored and restructured, with some checks moved between advanced and basic structure validations. The changes add support for version 6 of Drive ABCI validation, updating the platform version configuration accordingly. Test cases are expanded and adapted to cover new validation scenarios, particularly for tokens. The control flow for state transition validation is adjusted to ensure that contract creation and update transitions now perform their own basic structure validation. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant StateTransition
participant BasicValidator
participant AdvancedValidator
Client->>StateTransition: Submit DataContractCreate/UpdateTransition
StateTransition->>BasicValidator: validate_basic_structure (dispatch by version)
BasicValidator->>StateTransition: Result (success/errors)
StateTransition->>AdvancedValidator: validate_advanced_structure (dispatch by version)
AdvancedValidator->>StateTransition: Result (success/errors)
StateTransition->>Client: Validation result
Suggested labels
Suggested reviewers
Poem
Tip ⚡💬 Agentic Chat (Pro Plan, General Availability)
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (5)
🪧 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: 0
🧹 Nitpick comments (1)
packages/rs-dpp/src/data_contract/methods/validate_update/v0/mod.rs (1)
246-316: Validating newly added tokens
- Checks for contiguous positions, overflow in
base_supply, valid localizations, existence of required groups, and distribution timestamps.- The single distribution timestamp check is a potential risk if multiple distributions exist, since only the first is validated. Consider verifying all distribution timestamps or the earliest one to prevent partial rejections.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/rs-dpp/src/data_contract/methods/validate_update/mod.rs(3 hunks)packages/rs-dpp/src/data_contract/methods/validate_update/v0/mod.rs(20 hunks)packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/data_contract_update/mod.rs(5 hunks)packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/data_contract_update/state/v0/mod.rs(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (4)
packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/data_contract_update/state/v0/mod.rs (1)
packages/rs-drive/src/state_transition_action/batch/batched_transition/document_transition/document_create_transition_action/v0/mod.rs (1)
block_info(53-53)
packages/rs-dpp/src/data_contract/methods/validate_update/mod.rs (1)
packages/rs-drive/src/state_transition_action/batch/batched_transition/document_transition/document_create_transition_action/v0/mod.rs (1)
block_info(53-53)
packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/data_contract_update/mod.rs (6)
packages/rs-drive-abci/src/platform_types/platform_state/mod.rs (1)
v0(116-120)packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/mod.rs (3)
None(937-937)None(947-947)None(1298-1298)packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/data_contract_create/mod.rs (2)
None(929-929)None(1558-1558)packages/rs-dpp/src/data_contract/associated_token/token_configuration/v0/mod.rs (1)
default_most_restrictive(166-289)packages/rs-dpp/src/state_transition/state_transitions/contract/data_contract_update_transition/v0/v0_methods.rs (1)
new_from_data_contract(18-49)packages/rs-drive-abci/src/platform_types/state_transitions_processing_result/mod.rs (1)
execution_results(109-111)
packages/rs-dpp/src/data_contract/methods/validate_update/v0/mod.rs (3)
packages/rs-drive/src/state_transition_action/batch/batched_transition/document_transition/document_create_transition_action/v0/mod.rs (1)
block_info(53-53)packages/wasm-dpp/src/data_contract/data_contract.rs (1)
token_configuration(435-443)packages/rs-dpp/src/data_contract/associated_token/token_configuration/v0/mod.rs (1)
default_most_restrictive(166-289)
⏰ Context from checks skipped due to timeout of 90000ms (15)
- GitHub Check: Rust packages (dash-sdk) / Formatting
- GitHub Check: Rust packages (drive) / Unused dependencies
- GitHub Check: Rust packages (dpp) / Tests
- GitHub Check: Rust packages (drive-abci) / Unused dependencies
- GitHub Check: Rust packages (drive) / Linting
- GitHub Check: Rust packages (dpp) / Check each feature
- GitHub Check: Rust packages (drive-abci) / Check each feature
- GitHub Check: Rust packages (drive-abci) / Linting
- GitHub Check: Rust packages (dpp) / Linting
- GitHub Check: Rust packages (wasm-dpp) / Linting
- GitHub Check: Rust packages (wasm-dpp) / Tests
- GitHub Check: Build Docker images (Dashmate helper, dashmate-helper, dashmate-helper) / Build Dashmate helper image
- GitHub Check: Build Docker images (DAPI, dapi, dapi) / Build DAPI image
- GitHub Check: Build Docker images (Drive, drive, drive-abci) / Build Drive image
- GitHub Check: Build JS packages / Build JS
🔇 Additional comments (23)
packages/rs-dpp/src/data_contract/methods/validate_update/mod.rs (3)
1-1: New import forBlockInfo
This import cleanly introduces block-related context.
14-14: Includingblock_infoparameter
Addingblock_infoto thevalidate_updatemethod signature allows the validator to access block-specific data (e.g., timestamps), which supports richer validation logic, especially for token distribution checks.
23-23: Forwardingblock_infotovalidate_update_v0
Propagating the new parameter ensures consistent usage of block data in all versioned validation methods.packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/data_contract_update/state/v0/mod.rs (1)
134-136: Passingblock_infotovalidate_update
This change ensures the old contract is validated against the new contract with awareness of the block context (e.g., for time-based checks). The approach looks correct.packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/data_contract_update/mod.rs (12)
1183-1185: Introducing token configuration accessors
These new imports forTokenConfigurationV0GettersandTokenConfigurationV0Setterscorrectly support reading and modifying token settings.
1194-1195: Test name and structure
This test function name,test_data_contract_update_can_add_new_token, is aligned with the new feature goal. Good clarity on what it validates.
1206-1222: Preparing original contract with no tokens
Lines 1206-1222 handle fetching and applying the base contract before adding the new token. This ensures the test starts with the correct baseline state.
1224-1293: Adding a well-formed token and verifying success
This block builds a valid token configuration (including base supply, valid localizations) and checks that the state transition is successfully applied. The thorough approach is excellent.
1296-1306: Adding test for contiguous token positions
These lines set up a scenario to ensure that skipping token positions triggers an error. The logic is in line with the new contiguous-position requirement.
1308-1328: Applying the original contract with token at position 0
Similar to other tests, this ensures the test environment is precisely known before checking errors around the new token’s position.
1330-1338: Attempt to add token at position 2
This negative test ensures that a gap in token positions triggers aNonContiguousContractTokenPositionsError. The coverage is good.
1352-1369: Serializing the state transition and checking the result
The test properly verifies that the correct consensus error is raised. The code’s clarity and coverage are strong.
1370-1386: Validating large base supply
Ensuringbase_supplydoes not exceedi64::MAXis prudent. This test properly demonstrates a boundary check for numeric overflow.
1394-1418: Reapplying the contract environment
Reusing the pattern of applying the original contract and then updating it ensures consistent test coverage. The steps are well-ordered and easy to follow.
1420-1474: Rejecting tokens with base supply overi64::MAX
Line 1425 specifically sets a base supply that is one overi64::MAX, verifying the intended error path. This is a robust boundary check.
1476-1563: Testing invalid localization
This block checks that a token with empty localizations is rejected, aligning with the new constraints. The code is well-structured, ensuring it’s easy to maintain or expand.packages/rs-dpp/src/data_contract/methods/validate_update/v0/mod.rs (7)
1-3: Imports for timestamp and block checks
These imports extend validation logic, enabling detection of past distribution timestamps.
7-8: Inclusion of new token errors
InvalidTokenBaseSupplyErrorandNonContiguousContractTokenPositionsErrorreflect a more granular validation approach for token additions.
14-16: Expanding token-accessor usage
Fetching token distribution rules and pre-programmed distribution is essential for the revised validation logic.
19-19: AddingTokenContractPosition
Storing the token position type clarifies the contract’s approach to enumerating tokens—improves readability and consistency.
30-30: Introducingblock_infoinvalidate_update
This parameter is essential for time-sensitive checks. The approach to pass it through is clean and consistent with Rust’s design.
779-847: Timestamp in the past error
This negative test ensures that if a distribution date precedes the current block’s timestamp, an error is returned. The logic is solid, but again only checks the first distribution.Would you like a script to search for possible multi-timestamp checks that might be incomplete or untested, to confirm no additional logic is needed?
849-959: Testing token position and base supply boundaries
These sections cover a range of potential errors (non-contiguous positions and supply overflows)—robust coverage.
pauldelucia
changed the title
pauldelucia
changed the title
...validation/state_transition/state_transitions/data_contract_create/basic_structure/v0/mod.rs
Show resolved
Hide resolved
packages/rs-drive-abci/src/execution/validation/state_transition/processor/v0/mod.rs
Outdated
Show resolved
Hide resolved
...abci/src/execution/validation/state_transition/state_transitions/data_contract_create/mod.rs
Show resolved
Hide resolved
...ges/rs-platform-version/src/version/drive_abci_versions/drive_abci_validation_versions/v1.rs
Outdated
Show resolved
Hide resolved
...ges/rs-platform-version/src/version/drive_abci_versions/drive_abci_validation_versions/v1.rs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/data_contract_create/advanced_structure/v0/mod.rs (2)
23-37: Comment clarifies version validation placementThe comment explains that this validation was moved to basic structure validation in protocol version 9, which helps understand why this check exists in V0 but might be handled differently in newer versions.
However, ensure this is still needed in V0 advanced validation since it's now also in basic validation. If it's for backward compatibility, consider clarifying that in the comment.
- // Moved this to basic structure validation in protocol version 9 + // Maintained for backward compatibility, but moved to basic structure validation in protocol version 9
92-92: Test updated to match new method signatureThe test has been correctly updated to match the new method signature, using execution context instead of platform version.
Consider adding a specific test for version validation to ensure complete coverage of this validation path.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/data_contract_create/advanced_structure/v0/mod.rs(4 hunks)
🧰 Additional context used
🧠 Learnings (1)
packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/data_contract_create/advanced_structure/v0/mod.rs (1)
Learnt from: QuantumExplorer
PR: dashpay/platform#2227
File: packages/rs-drive-abci/src/platform_types/platform_state/mod.rs:141-141
Timestamp: 2024-10-08T13:28:03.529Z
Learning: When converting `PlatformStateV0` to `PlatformStateForSavingV1` in `packages/rs-drive-abci/src/platform_types/platform_state/mod.rs`, only version `0` needs to be handled in the match on `platform_state_for_saving_structure_default` because the changes are retroactive.
⏰ Context from checks skipped due to timeout of 90000ms (17)
- GitHub Check: Rust packages (drive) / Formatting
- GitHub Check: Rust packages (drive) / Tests
- GitHub Check: Rust packages (dpp) / Unused dependencies
- GitHub Check: Rust packages (wasm-dpp) / Linting
- GitHub Check: Rust packages (wasm-dpp) / Formatting
- GitHub Check: Rust packages (dpp) / Linting
- GitHub Check: Rust packages (dpp) / Check each feature
- GitHub Check: Rust packages (dpp) / Formatting
- GitHub Check: Rust packages (dpp) / Tests
- GitHub Check: Rust packages (dash-sdk) / Check each feature
- GitHub Check: Rust packages (dash-sdk) / Linting
- GitHub Check: Rust packages (dash-sdk) / Tests
- GitHub Check: Rust packages (dash-sdk) / Unused dependencies
- GitHub Check: Build Docker images (Drive, drive, drive-abci) / Build Drive image
- GitHub Check: Build Docker images (DAPI, dapi, dapi) / Build DAPI image
- GitHub Check: Build Docker images (Dashmate helper, dashmate-helper, dashmate-helper) / Build Dashmate helper image
- GitHub Check: Build JS packages / Build JS
🔇 Additional comments (3)
packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/data_contract_create/advanced_structure/v0/mod.rs (3)
2-5: These imports support the refactored validation approachThe new imports properly support the shift from direct platform version validation to using execution context and delegating to the V1 validation implementation.
13-13: API change improves validation context handlingChanging the method signature from accepting
platform_versionto acceptingexecution_contextis a good design decision as it provides more context for validation and aligns with modern Rust patterns for contextual validation.
39-39: Delegation to V1 validation completes the validation chainThe implementation follows a good validation chain pattern, where V0 performs minimal checks before delegating to the V1 implementation. This maintains backward compatibility while allowing the validation logic to evolve.
QuantumExplorer
changed the title
QuantumExplorer
changed the title
3 participants
Issue being fixed or feature implemented
Allow new tokens on contract update, add advanced structure validation for contract update, basic structure validation for contract update and create.
What was done?
Remove the check for new tokens and replace with validations. Refactor contract validations.
How Has This Been Tested?
Added unit tests
Breaking Changes
Checklist:
For repository code-owners and collaborators only
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores