fix(platform): ensure document types only target valid tokens for token payments #2631
Conversation
|
Caution Review failedThe pull request is closed. """ WalkthroughThis change updates the document type creation and schema parsing logic to include a new Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant DocumentType
participant TokenConfigMap
participant SchemaValidator
Caller->>DocumentType: create_document_types_from_document_schemas(..., &token_configurations, ...)
DocumentType->>DocumentType: create_document_types_from_document_schemas_v0/v1(..., &token_configurations, ...)
loop For each schema
DocumentType->>DocumentType: try_from_schema(..., &token_configurations, ...)
DocumentType->>SchemaValidator: Validate schema, check token positions via token_configurations
SchemaValidator-->>DocumentType: Return validation result or error
end
DocumentType-->>Caller: Return created DocumentTypes or error
Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
✨ 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: 0
🔭 Outside diff range comments (1)
packages/rs-dpp/src/data_contract/document_type/class_methods/try_from_schema/mod.rs (1)
59-68: 💡 Verification agent🧩 Analysis chain
Verify: Version 0 implementation missing token_configurations parameter
The
token_configurationsparameter is passed to version 1 implementation but not to version 0. This creates inconsistent behavior between versions.If version 0 should support token configurations, this is a bug. If not, consider adding a comment explaining why version 0 doesn't need this parameter.
🏁 Script executed:
#!/bin/bash # Check if DocumentTypeV0::try_from_schema accepts token_configurations parameter ast-grep --pattern 'impl DocumentTypeV0 { $$$ fn try_from_schema($$$) { $$$ } $$$ }'Length of output: 98
I’ll locate and compare the
try_from_schemasignatures for both v0 and v1 to confirm whethertoken_configurationsis expected by v0.
🏁 Script executed:
#!/bin/bash set -e echo "Files under try_from_schema/:" fd -e rs -d 1 packages/rs-dpp/src/data_contract/document_type/class_methods/try_from_schema echo -e "\nAll try_from_schema definitions with context:" rg -n 'fn try_from_schema' -C2 packages/rs-dpp/src/data_contract/document_type/class_methods/try_from_schemaLength of output: 785
Inspecting both version implementations to confirm their signatures:
🏁 Script executed:
#!/bin/bash set -e echo "Files in try_from_schema/:" find packages/rs-dpp/src/data_contract/document_type/class_methods/try_from_schema -maxdepth 1 -type f echo -e "\n--- v0 signature ---" rg -n "fn try_from_schema" -C3 packages/rs-dpp/src/data_contract/document_type/class_methods/try_from_schema/v0.rs echo -e "\n--- v1 signature ---" rg -n "fn try_from_schema" -C3 packages/rs-dpp/src/data_contract/document_type/class_methods/try_from_schema/v1.rsLength of output: 559
To locate where the v0/v1 implementations live and inspect their signatures:
🏁 Script executed:
#!/bin/bash set -e echo "Subdirectories under try_from_schema:" find packages/rs-dpp/src/data_contract/document_type/class_methods/try_from_schema -maxdepth 1 -type d echo -e "\n--- v0 implementation signature ---" if [ -d packages/rs-dpp/src/data_contract/document_type/class_methods/try_from_schema/v0 ]; then rg -n "fn try_from_schema" -C3 packages/rs-dpp/src/data_contract/document_type/class_methods/try_from_schema/v0 else echo "No v0 directory found" fi echo -e "\n--- v1 implementation signature ---" if [ -d packages/rs-dpp/src/data_contract/document_type/class_methods/try_from_schema/v1 ]; then rg -n "fn try_from_schema" -C3 packages/rs-dpp/src/data_contract/document_type/class_methods/try_from_schema/v1 else echo "No v1 directory found" fiLength of output: 2764
Gathering full
try_from_schemasignatures for both versions:
🏁 Script executed:
#!/bin/bash set -e echo "--- v0 signature ---" rg -n "pub(super) fn try_from_schema" -A15 packages/rs-dpp/src/data_contract/document_type/class_methods/try_from_schema/v0/mod.rs echo -e "\n--- v1 signature ---" rg -n "pub(super) fn try_from_schema" -A15 packages/rs-dpp/src/data_contract/document_type/class_methods/try_from_schema/v1/mod.rsLength of output: 192
Inspecting the function signatures directly from the source:
🏁 Script executed:
#!/bin/bash set -e echo "--- v0 signature (lines 55–90) ---" sed -n '55,90p' packages/rs-dpp/src/data_contract/document_type/class_methods/try_from_schema/v0/mod.rs echo -e "\n--- v1 signature (lines 70–110) ---" sed -n '70,110p' packages/rs-dpp/src/data_contract/document_type/class_methods/try_from_schema/v1/mod.rsLength of output: 3566
Version 0
try_from_schemamissingtoken_configurations
Version 1’stry_from_schematakes atoken_configurations: &BTreeMap<TokenContractPosition, TokenConfiguration>argument, but version 0’s signature does not—leading to inconsistent behavior between versions.Please choose one of the following fixes:
• If v0 should also accept token configurations, update its signature and dispatch call to include
token_configurations, and add#[allow(unused_variables)]to ignore it in v0’s implementation.
• If token support was introduced in v1 on purpose, add a comment in v0’s impl (or above the v0 arm inmod.rs) clarifying that tokens are unsupported in v0.Locations to update:
packages/rs-dpp/src/data_contract/document_type/class_methods/try_from_schema/v0/mod.rs(function signature)packages/rs-dpp/src/data_contract/document_type/class_methods/try_from_schema/mod.rs(v0 arm in the version dispatch)
🧹 Nitpick comments (1)
packages/rs-dpp/src/data_contract/document_type/class_methods/try_from_schema/v1/mod.rs (1)
704-704: Test updates correctly handle the new parameter.All test calls have been appropriately updated to pass
&BTreeMap::new()for thetoken_configurationsparameter. Using an empty map is correct for these tests since they don't involve token validation scenarios.Consider adding dedicated tests for token position validation scenarios:
#[test] fn should_validate_token_position_exists() { let platform_version = PlatformVersion::latest(); let mut token_configurations = BTreeMap::new(); token_configurations.insert(0, TokenConfiguration::default()); let schema = platform_value!({ "type": "object", "properties": { "name": { "type": "string", "position": 0 } }, "tokenCost": { "create": { "tokenPosition": 0, "amount": 100 } }, "additionalProperties": false }); let config = DataContractConfig::default_for_version(platform_version) .expect("should create a default config"); let result = DocumentTypeV1::try_from_schema( Identifier::new([1; 32]), "test_type", schema, None, &token_configurations, &config, true, &mut vec![], platform_version, ); assert!(result.is_ok()); } #[test] fn should_fail_with_invalid_token_position() { let platform_version = PlatformVersion::latest(); let token_configurations = BTreeMap::new(); // Empty map let schema = platform_value!({ "type": "object", "properties": { "name": { "type": "string", "position": 0 } }, "tokenCost": { "create": { "tokenPosition": 999, // Invalid position "amount": 100 } }, "additionalProperties": false }); let config = DataContractConfig::default_for_version(platform_version) .expect("should create a default config"); let result = DocumentTypeV1::try_from_schema( Identifier::new([1; 32]), "test_type", schema, None, &token_configurations, &config, true, &mut vec![], platform_version, ); assert_matches!( result, Err(ProtocolError::ConsensusError(boxed)) => { assert_matches!( boxed.as_ref(), ConsensusError::BasicError( BasicError::InvalidTokenPositionError(_) ) ) } ); }Also applies to: 736-736, 779-779, 848-848
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
packages/rs-dpp/src/data_contract/document_type/class_methods/create_document_types_from_document_schemas/mod.rs(3 hunks)packages/rs-dpp/src/data_contract/document_type/class_methods/create_document_types_from_document_schemas/v0/mod.rs(3 hunks)packages/rs-dpp/src/data_contract/document_type/class_methods/create_document_types_from_document_schemas/v1/mod.rs(2 hunks)packages/rs-dpp/src/data_contract/document_type/class_methods/try_from_schema/mod.rs(3 hunks)packages/rs-dpp/src/data_contract/document_type/class_methods/try_from_schema/v1/mod.rs(8 hunks)packages/rs-dpp/src/data_contract/serialized_version/mod.rs(1 hunks)packages/rs-dpp/src/data_contract/v0/methods/schema.rs(2 hunks)packages/rs-dpp/src/data_contract/v0/serialization/mod.rs(3 hunks)packages/rs-dpp/src/data_contract/v1/methods/schema.rs(3 hunks)packages/rs-dpp/src/data_contract/v1/serialization/mod.rs(3 hunks)packages/rs-dpp/src/errors/consensus/basic/token/invalid_token_position_error.rs(2 hunks)packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/batch/action_validation/token/token_base_transition_action/structure_v0/mod.rs(1 hunks)packages/strategy-tests/src/operations.rs(2 hunks)
🧰 Additional context used
🧠 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 (19)
- GitHub Check: Rust packages (drive-abci) / Tests
- GitHub Check: Rust packages (drive) / Tests
- GitHub Check: Rust packages (drive) / Unused dependencies
- GitHub Check: Rust packages (drive) / Linting
- GitHub Check: Rust packages (wasm-dpp) / Linting
- GitHub Check: Rust packages (wasm-dpp) / Formatting
- GitHub Check: Rust packages (wasm-dpp) / Tests
- GitHub Check: Rust packages (dpp) / Check each feature
- GitHub Check: Rust packages (dpp) / Tests
- GitHub Check: Rust packages (dpp) / Unused dependencies
- GitHub Check: Rust packages (dpp) / Linting
- GitHub Check: Rust packages (dash-sdk) / Tests
- GitHub Check: Rust packages (dash-sdk) / Linting
- GitHub Check: Rust packages (dash-sdk) / Check each feature
- 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 Docker images (Drive, drive, drive-abci) / Build Drive image
- GitHub Check: Build JS packages / Build JS
- GitHub Check: Rust crates security audit
🔇 Additional comments (26)
packages/rs-dpp/src/data_contract/serialized_version/mod.rs (1)
9-9: LGTM: Clean import organization.The reorganization of the
EMPTY_KEYWORDSimport improves code organization and aligns with the broader changes involvingBTreeMapusage for token configurations.packages/rs-dpp/src/data_contract/v0/serialization/mod.rs (2)
8-8: LGTM: Appropriate import addition.The
BTreeMapimport is correctly added to support the new token configuration parameter in document type creation methods.
98-98: LGTM: Correct handling of token configurations for v0 contracts.Passing an empty
BTreeMapfor token configurations is appropriate for v0 contracts since they don't support tokens. This maintains backward compatibility while integrating with the new token-aware document type creation API.Also applies to: 139-139
packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/batch/action_validation/token/token_base_transition_action/structure_v0/mod.rs (1)
35-35: LGTM: Cleaner token key retrieval.The use of
.last().copied()is more concise and safer than explicit dereferencing withexpect. This change aligns well with the broader improvements to token position handling and the optional nature ofmax_token_positionin error scenarios.packages/strategy-tests/src/operations.rs (2)
4-4: LGTM: Necessary import for token access.The import of
DataContractV1Gettersis required to access thetokens()method used in the document type deserialization process.
434-434: LGTM: Proper token configuration integration in tests.Adding
contract.tokens()as a parameter toDocumentType::try_from_schemacorrectly integrates token configuration validation into the test deserialization process. This ensures that token-related validations are properly exercised during document type reconstruction in strategy tests.packages/rs-dpp/src/data_contract/v1/serialization/mod.rs (3)
7-7: LGTM!The BTreeMap import is correctly added to support the new token configuration parameter.
97-97: Correct implementation for v0 compatibility.Passing an empty BTreeMap for v0 data contracts is appropriate since version 0 doesn't support tokens. This maintains backward compatibility while integrating with the new token validation system.
156-156: Proper token configuration integration for v1.Correctly passes the actual tokens from the v1 data contract for validation. The logic appropriately uses the tokens for document type creation when they're available.
packages/rs-dpp/src/data_contract/v1/methods/schema.rs (2)
25-25: Correct token configuration integration.Properly passes the tokens from the data contract to enable token validation during document type creation from schemas.
49-49: Consistent token configuration passing.Using
self.tokens()method call is appropriate for single document schema operations and maintains consistency with the token validation system.packages/rs-dpp/src/data_contract/v0/methods/schema.rs (2)
25-25: Appropriate v0 compatibility handling.Using an empty BTreeMap for v0 data contracts is correct since version 0 doesn't support tokens. This maintains backward compatibility with the existing v0 behavior.
49-49: Consistent v0 token handling.Correctly passes an empty token configuration map for single document schema operations in v0, maintaining consistency with the v0 design that doesn't include token support.
packages/rs-dpp/src/errors/consensus/basic/token/invalid_token_position_error.rs (4)
13-19: Well-designed conditional error message formatting.The error message formatting correctly handles both cases - when a maximum token position is set and when it's not. The conditional logic using
if let Some(max)is clear and provides appropriate messages for both scenarios.
22-22: Appropriate field type change.Making
max_token_positionoptional provides the necessary flexibility for cases where no maximum token position limit is defined, which aligns with the enhanced token validation system.
27-35: Constructor correctly updated for optional field.The constructor properly accepts an optional
max_token_positionparameter and initializes the struct consistently. This maintains the API's usability while supporting the new optional behavior.
37-39: Getter properly returns optional type.The getter method correctly returns
Option<TokenContractPosition>to match the updated field type, maintaining consistency throughout the API.packages/rs-dpp/src/data_contract/document_type/class_methods/try_from_schema/mod.rs (1)
8-8: LGTM: Token configuration integration looks goodThe addition of token configuration support to the schema parsing is well implemented. The parameter is properly typed and threaded through the API.
Also applies to: 46-46, 75-75
packages/rs-dpp/src/data_contract/document_type/class_methods/create_document_types_from_document_schemas/v0/mod.rs (2)
12-12: LGTM: Clean integration of token configurationsThe implementation correctly:
- Moves from
DocumentTypeV0toDocumentType- Adds the
token_configurationsparameter to the method signature- Passes it through to the
try_from_schemacallThis maintains consistency with the API evolution across versions.
Also applies to: 17-17, 43-43
88-88: Good test update for backward compatibilityThe test correctly passes an empty
BTreeMapfor the newtoken_configurationsparameter, maintaining backward compatibility while testing the new signature.packages/rs-dpp/src/data_contract/document_type/class_methods/create_document_types_from_document_schemas/mod.rs (2)
6-6: LGTM: Consistent API evolutionThe addition of
token_configurationsparameter to the main entry point method is well-designed and maintains consistency across the document type creation API.Also applies to: 42-42
56-77: Good refactoring: Consolidated version handlingThe change from
DocumentTypeV0::andDocumentTypeV1::toDocumentType::for both versions simplifies the API and consolidates version handling. Thetoken_configurationsparameter is correctly passed to both version implementations.packages/rs-dpp/src/data_contract/document_type/class_methods/create_document_types_from_document_schemas/v1/mod.rs (1)
12-12: LGTM: Consistent v1 implementationThe changes mirror the v0 implementation perfectly:
- Moves from
DocumentTypeV0toDocumentType- Adds the
token_configurationsparameter- Passes it through to
try_from_schemaThis maintains consistency across all version implementations.
Also applies to: 18-18, 45-45
packages/rs-dpp/src/data_contract/document_type/class_methods/try_from_schema/v1/mod.rs (3)
35-35: LGTM: Appropriate imports added for token validation.The new imports for
InvalidTokenPositionError,TokenConfiguration, andTokenContractPositionare necessary for implementing the token position validation functionality.Also applies to: 60-60
582-594: Well-implemented token position validation logic.The validation correctly:
- Only runs under
full_validationandvalidationfeature flag- Checks if the token position exists in the provided configurations
- Returns an appropriate error with context (last valid position and invalid position)
- Uses
last_key_value()to get the maximum valid position for error reportingThe error construction provides helpful context for debugging invalid token positions.
82-82:✅ Verification successful
Breaking change: Method signature updated with token configurations parameter.
The addition of the
token_configurationsparameter is a breaking change but aligns with the PR objectives to ensure document types only target valid tokens. The parameter type&BTreeMap<TokenContractPosition, TokenConfiguration>is appropriate for efficient lookups during validation.Verify that all callers of this method have been updated to provide the token configurations parameter:
🏁 Script executed:
#!/bin/bash # Description: Find all calls to DocumentTypeV1::try_from_schema to ensure they've been updated # Expected: All calls should include the new token_configurations parameter rg -A 10 "DocumentTypeV1::try_from_schema" --type rustLength of output: 6570
Confirmed: All
try_from_schemainvocations updated
I’ve verified that every call toDocumentTypeV1::try_from_schemanow includes the newtoken_configurations: &BTreeMap<TokenContractPosition, TokenConfiguration>argument. No further changes are required.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/wasm-dpp/src/errors/consensus/consensus_error.rs (1)
64-64: Consider breaking up the long import line for better readability.While the import follows the existing pattern, the line is quite long. Consider splitting it into multiple lines for improved maintainability.
-use dpp::consensus::basic::data_contract::{ContestedUniqueIndexOnMutableDocumentTypeError, ContestedUniqueIndexWithUniqueIndexError, DataContractTokenConfigurationUpdateError, DecimalsOverLimitError, DuplicateKeywordsError, GroupExceedsMaxMembersError, GroupMemberHasPowerOfZeroError, GroupMemberHasPowerOverLimitError, GroupNonUnilateralMemberPowerHasLessThanRequiredPowerError, GroupPositionDoesNotExistError, GroupRequiredPowerIsInvalidError, GroupTotalPowerLessThanRequiredError, InvalidDescriptionLengthError, InvalidDocumentTypeRequiredSecurityLevelError, InvalidKeywordCharacterError, InvalidKeywordLengthError, InvalidTokenBaseSupplyError, InvalidTokenDistributionFunctionDivideByZeroError, InvalidTokenDistributionFunctionIncoherenceError, InvalidTokenDistributionFunctionInvalidParameterError, InvalidTokenDistributionFunctionInvalidParameterTupleError, InvalidTokenLanguageCodeError, InvalidTokenNameCharacterError, InvalidTokenNameLengthError, MainGroupIsNotDefinedError, NewTokensDestinationIdentityOptionRequiredError, NonContiguousContractGroupPositionsError, NonContiguousContractTokenPositionsError, RedundantDocumentPaidForByTokenWithContractId, TokenPaymentByBurningOnlyAllowedOnInternalTokenError, TooManyKeywordsError, UnknownDocumentActionTokenEffectError, UnknownDocumentCreationRestrictionModeError, UnknownGasFeesPaidByError, UnknownSecurityLevelError, UnknownStorageKeyRequirementsError, UnknownTradeModeError, UnknownTransferableTypeError}; +use dpp::consensus::basic::data_contract::{ + ContestedUniqueIndexOnMutableDocumentTypeError, ContestedUniqueIndexWithUniqueIndexError, + DataContractTokenConfigurationUpdateError, DecimalsOverLimitError, DuplicateKeywordsError, + GroupExceedsMaxMembersError, GroupMemberHasPowerOfZeroError, GroupMemberHasPowerOverLimitError, + GroupNonUnilateralMemberPowerHasLessThanRequiredPowerError, GroupPositionDoesNotExistError, + GroupRequiredPowerIsInvalidError, GroupTotalPowerLessThanRequiredError, + InvalidDescriptionLengthError, InvalidDocumentTypeRequiredSecurityLevelError, + InvalidKeywordCharacterError, InvalidKeywordLengthError, InvalidTokenBaseSupplyError, + InvalidTokenDistributionFunctionDivideByZeroError, InvalidTokenDistributionFunctionIncoherenceError, + InvalidTokenDistributionFunctionInvalidParameterError, InvalidTokenDistributionFunctionInvalidParameterTupleError, + InvalidTokenLanguageCodeError, InvalidTokenNameCharacterError, InvalidTokenNameLengthError, + MainGroupIsNotDefinedError, NewTokensDestinationIdentityOptionRequiredError, + NonContiguousContractGroupPositionsError, NonContiguousContractTokenPositionsError, + RedundantDocumentPaidForByTokenWithContractId, TokenPaymentByBurningOnlyAllowedOnInternalTokenError, + TooManyKeywordsError, UnknownDocumentActionTokenEffectError, UnknownDocumentCreationRestrictionModeError, + UnknownGasFeesPaidByError, UnknownSecurityLevelError, UnknownStorageKeyRequirementsError, + UnknownTradeModeError, UnknownTransferableTypeError +};packages/rs-dpp/src/errors/consensus/basic/data_contract/redundant_document_paid_for_by_token_with_contract_id.rs (1)
9-16: Consider including contract_id in error message for better debugging.The error struct is well-implemented with appropriate derives and a clear message. Consider enhancing the error message to include the contract_id value for improved debugging experience.
-#[error("Document paid for by a token has a contractId {contract_id} set, which is redundant because it is targeting the current contract")] +#[error("Document paid for by a token has a contractId {contract_id} set, which is redundant because it is targeting the current contract")]The current placeholder
{contract_id}should automatically display the field value when the error is formatted.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
packages/rs-dpp/src/data_contract/document_type/class_methods/try_from_schema/v1/mod.rs(10 hunks)packages/rs-dpp/src/errors/consensus/basic/basic_error.rs(2 hunks)packages/rs-dpp/src/errors/consensus/basic/data_contract/mod.rs(2 hunks)packages/rs-dpp/src/errors/consensus/basic/data_contract/redundant_document_paid_for_by_token_with_contract_id.rs(1 hunks)packages/rs-dpp/src/errors/consensus/codes.rs(1 hunks)packages/rs-sdk/tests/fetch/common.rs(2 hunks)packages/wasm-dpp/src/errors/consensus/consensus_error.rs(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/rs-dpp/src/errors/consensus/basic/data_contract/mod.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/rs-dpp/src/data_contract/document_type/class_methods/try_from_schema/v1/mod.rs
🧰 Additional context used
🧠 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 (20)
- GitHub Check: Rust packages (dash-sdk) / Linting
- GitHub Check: Rust packages (dash-sdk) / Check each feature
- GitHub Check: Rust packages (dash-sdk) / Tests
- GitHub Check: Rust packages (dash-sdk) / Unused dependencies
- GitHub Check: Rust packages (drive-abci) / Linting
- GitHub Check: Rust packages (drive-abci) / Tests
- GitHub Check: Rust packages (drive-abci) / Check each feature
- GitHub Check: Rust packages (drive-abci) / Unused dependencies
- GitHub Check: Rust packages (drive) / Linting
- GitHub Check: Rust packages (drive) / Unused dependencies
- GitHub Check: Rust packages (drive) / Tests
- GitHub Check: Rust packages (drive) / Formatting
- GitHub Check: Rust packages (wasm-dpp) / Tests
- GitHub Check: Rust packages (wasm-dpp) / Formatting
- GitHub Check: Rust packages (wasm-dpp) / Linting
- GitHub Check: Rust packages (wasm-dpp) / Unused dependencies
- 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 (9)
packages/rs-dpp/src/errors/consensus/codes.rs (1)
119-119: LGTM! Error code assignment follows established patterns.The new error code
10272is correctly placed in the DataContract Errors range (10200-10349) and follows the sequential numbering pattern.packages/wasm-dpp/src/errors/consensus/consensus_error.rs (1)
827-829: LGTM! Error conversion follows established pattern.The error conversion logic correctly uses the
generic_consensus_error!macro consistent with other error types.packages/rs-sdk/tests/fetch/common.rs (2)
6-6: LGTM! Clean import addition.The
BTreeMapimport is appropriately added to support the newtoken_configurationsparameter.
43-43: LGTM! Appropriate handling of new parameter in test code.Passing an empty
BTreeMapfortoken_configurationsis the correct approach for test utilities where specific token configurations aren't needed.packages/rs-dpp/src/errors/consensus/basic/basic_error.rs (2)
26-26: LGTM! Import addition follows established pattern.The
RedundantDocumentPaidForByTokenWithContractIdimport is correctly added to the existing import statement.
568-569: LGTM! Error variant addition follows established pattern.The new error variant correctly uses the
#[error(transparent)]attribute and follows the same pattern as other error variants. Placing it at the end of the enum maintains serialization compatibility.packages/rs-dpp/src/errors/consensus/basic/data_contract/redundant_document_paid_for_by_token_with_contract_id.rs (3)
1-7: LGTM! Well-organized imports.The import statements are well-structured and include all necessary dependencies for implementing a consensus error type with proper serialization support.
18-26: LGTM! Clean and idiomatic implementation.The constructor and getter methods follow Rust conventions perfectly. The implementation is clean, minimal, and provides good encapsulation.
28-34: LGTM! Proper integration with consensus error hierarchy.The From trait implementation correctly wraps this error in the BasicError enum and integrates seamlessly with the consensus error framework.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/rs-dpp/src/errors/consensus/state/token/invalid_token_position_error.rs (1)
12-19: Consider simplifying the error message formatting.While the conditional formatting works correctly, the inline conditional expression in the error macro is quite complex and may impact readability.
Consider moving the message formatting to the
Displayimplementation or using a simpler approach:-#[error( - "Invalid token position: {invalid_token_position}. {max_token_message}", - max_token_message = if let Some(max) = self.max_token_position { - format!("The maximum allowed token position is {}", max) - } else { - "No maximum token position limit is set.".to_string() - } -)] +#[error("Invalid token position: {invalid_token_position}")]And implement a custom
Displaytrait if more complex formatting is needed.packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/data_contract_create/mod.rs (1)
2446-2557: Well-structured test for non-existent external token contract validation.This test properly validates that referencing a non-existent external token contract results in a
DataContractNotFoundError. The test setup is comprehensive and follows the established pattern in the file.A minor suggestion for improved clarity:
{ let document_type = data_contract .document_types_mut() .get_mut("card") .expect("expected a document type with name card"); + // Set up token cost with non-existent contract ID to test validation document_type.set_document_creation_token_cost(Some(DocumentActionTokenCost { contract_id: Some(Identifier::new([0; 32])),
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
packages/rs-dpp/src/data_contract/document_type/accessors/mod.rs(4 hunks)packages/rs-dpp/src/data_contract/document_type/accessors/v1/mod.rs(2 hunks)packages/rs-dpp/src/data_contract/document_type/token_costs/accessors.rs(1 hunks)packages/rs-dpp/src/data_contract/document_type/token_costs/mod.rs(1 hunks)packages/rs-dpp/src/data_contract/document_type/token_costs/v0/mod.rs(1 hunks)packages/rs-dpp/src/data_contract/document_type/v1/accessors.rs(2 hunks)packages/rs-dpp/src/errors/consensus/codes.rs(2 hunks)packages/rs-dpp/src/errors/consensus/state/data_contract/data_contract_not_found_error.rs(1 hunks)packages/rs-dpp/src/errors/consensus/state/data_contract/mod.rs(1 hunks)packages/rs-dpp/src/errors/consensus/state/state_error.rs(3 hunks)packages/rs-dpp/src/errors/consensus/state/token/invalid_token_position_error.rs(1 hunks)packages/rs-dpp/src/errors/consensus/state/token/mod.rs(2 hunks)packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/data_contract_create/mod.rs(1 hunks)packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/data_contract_create/state/v0/mod.rs(2 hunks)packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/data_contract_update/state/v0/mod.rs(3 hunks)packages/wasm-dpp/src/errors/consensus/consensus_error.rs(4 hunks)
✅ Files skipped from review due to trivial changes (2)
- packages/rs-dpp/src/errors/consensus/state/data_contract/mod.rs
- packages/rs-dpp/src/errors/consensus/state/token/mod.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/wasm-dpp/src/errors/consensus/consensus_error.rs
- packages/rs-dpp/src/errors/consensus/codes.rs
🧰 Additional context used
🧠 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.
🧬 Code Graph Analysis (6)
packages/rs-dpp/src/data_contract/document_type/token_costs/mod.rs (2)
packages/rs-dpp/src/data_contract/document_type/token_costs/accessors.rs (6)
document_creation_token_cost_ref(24-24)document_replacement_token_cost_ref(27-27)document_deletion_token_cost_ref(30-30)document_transfer_token_cost_ref(33-33)document_price_update_token_cost_ref(36-36)document_purchase_token_cost_ref(39-39)packages/rs-dpp/src/data_contract/document_type/token_costs/v0/mod.rs (6)
document_creation_token_cost_ref(54-56)document_replacement_token_cost_ref(58-60)document_deletion_token_cost_ref(62-64)document_transfer_token_cost_ref(66-68)document_price_update_token_cost_ref(70-72)document_purchase_token_cost_ref(74-76)
packages/rs-dpp/src/data_contract/document_type/accessors/mod.rs (2)
packages/rs-dpp/src/data_contract/document_type/accessors/v1/mod.rs (2)
all_document_token_costs(51-51)all_external_token_costs_contract_tokens(55-57)packages/rs-dpp/src/data_contract/document_type/v1/accessors.rs (2)
all_document_token_costs(158-181)all_external_token_costs_contract_tokens(183-197)
packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/data_contract_create/state/v0/mod.rs (2)
packages/rs-dpp/src/errors/consensus/state/data_contract/data_contract_not_found_error.rs (1)
new(19-21)packages/rs-dpp/src/errors/consensus/state/token/invalid_token_position_error.rs (1)
new(27-35)
packages/rs-dpp/src/data_contract/document_type/accessors/v1/mod.rs (2)
packages/rs-dpp/src/data_contract/document_type/v1/accessors.rs (2)
all_document_token_costs(158-181)all_external_token_costs_contract_tokens(183-197)packages/rs-dpp/src/data_contract/document_type/accessors/mod.rs (6)
all_document_token_costs(630-635)all_document_token_costs(690-695)all_document_token_costs(750-755)all_external_token_costs_contract_tokens(637-644)all_external_token_costs_contract_tokens(697-704)all_external_token_costs_contract_tokens(757-764)
packages/rs-dpp/src/errors/consensus/state/data_contract/data_contract_not_found_error.rs (2)
packages/rs-dpp/src/errors/consensus/state/token/invalid_token_position_error.rs (1)
from(47-49)packages/rs-dpp/src/errors/consensus/state/state_error.rs (1)
from(327-329)
packages/rs-dpp/src/data_contract/document_type/v1/accessors.rs (2)
packages/rs-dpp/src/data_contract/document_type/accessors/v1/mod.rs (2)
all_document_token_costs(51-51)all_external_token_costs_contract_tokens(55-57)packages/rs-dpp/src/data_contract/document_type/accessors/mod.rs (6)
all_document_token_costs(630-635)all_document_token_costs(690-695)all_document_token_costs(750-755)all_external_token_costs_contract_tokens(637-644)all_external_token_costs_contract_tokens(697-704)all_external_token_costs_contract_tokens(757-764)
⏰ Context from checks skipped due to timeout of 90000ms (19)
- GitHub Check: Rust packages (wasm-dpp) / Tests
- GitHub Check: Rust packages (wasm-dpp) / Unused dependencies
- GitHub Check: Rust packages (wasm-dpp) / Formatting
- GitHub Check: Rust packages (dash-sdk) / Linting
- GitHub Check: Rust packages (wasm-dpp) / Linting
- GitHub Check: Rust packages (dash-sdk) / Check each feature
- GitHub Check: Rust packages (dash-sdk) / Tests
- GitHub Check: Rust packages (dpp) / Tests
- GitHub Check: Rust packages (dpp) / Linting
- GitHub Check: Rust packages (dpp) / Check each feature
- GitHub Check: Rust packages (drive-abci) / Linting
- GitHub Check: Rust packages (drive-abci) / Tests
- GitHub Check: Rust packages (drive-abci) / Check each feature
- GitHub Check: Rust packages (drive) / Linting
- GitHub Check: Rust packages (drive) / Tests
- GitHub Check: Build Docker images (Dashmate helper, dashmate-helper, dashmate-helper) / Build Dashmate helper image
- 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 JS packages / Build JS
🔇 Additional comments (17)
packages/rs-dpp/src/errors/consensus/state/state_error.rs (1)
27-27: LGTM! Error enum updates follow established patterns.The new error variants
DataContractNotFoundErrorandInvalidTokenPositionStateErrorare correctly added to theStateErrorenum following the existing pattern with proper imports and#[error(transparent)]annotations.Also applies to: 47-47, 319-323
packages/rs-dpp/src/data_contract/document_type/accessors/v1/mod.rs (2)
1-4: LGTM! Imports are appropriate for the new trait methods.The added imports (
TokenContractPosition,Identifier,BTreeMap,BTreeSet) are necessary for the return types of the new trait methods.
50-57: LGTM! Well-documented trait methods for internal validation.The new trait methods
all_document_token_costsandall_external_token_costs_contract_tokensare properly documented and clearly indicate their purpose for internal validation. The return types are appropriate for their intended use.packages/rs-dpp/src/data_contract/document_type/v1/accessors.rs (3)
16-16: LGTM! Import added for required type.The
TokenContractPositionimport is necessary for the new method implementations.
158-181: LGTM! Comprehensive collection of all document token costs.The implementation correctly collects all possible document token costs from the various operation types (creation, replacement, deletion, transfer, price update, purchase) and returns them as a vector of references.
183-197: LGTM! Efficient mapping of external token costs.The implementation correctly filters for external token costs (those with a
contract_id) and builds a map from contract identifiers to sets of token contract positions. The logic efficiently leverages theall_document_token_costsmethod and uses appropriate data structures.packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/data_contract_update/state/v0/mod.rs (1)
9-9: LGTM! Imports correctly added for new validation features.The imports for
DataContractNotFoundError,InvalidTokenPositionStateError, andDocumentTypeV1Gettersare necessary for the new external token cost validation logic.Also applies to: 15-15, 24-24
packages/rs-dpp/src/data_contract/document_type/token_costs/v0/mod.rs (1)
54-76: Good addition of reference-returning methods for performance optimization.The new methods provide efficient read-only access to token costs without cloning. The implementation correctly uses
.as_ref()to convertOption<T>toOption<&T>.packages/rs-dpp/src/data_contract/document_type/token_costs/accessors.rs (1)
23-39: Well-designed trait extension with reference accessors.The trait methods are properly documented and follow a consistent naming convention with the
_refsuffix. This complements the owned-value accessors and provides flexibility for callers.packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/data_contract_create/state/v0/mod.rs (2)
8-25: Appropriate imports for external token validation.The new imports for error types and accessor traits support the external token cost validation logic.
319-385: Well-implemented validation for external token costs.The validation logic correctly:
- Fetches external contracts and adds fees to the execution context
- Validates contract existence before checking token positions
- Uses proper error types (
DataContractNotFoundErrorandInvalidTokenPositionStateError)- Maintains consistency with the existing validation patterns
The use of
last_key_value()at line 360 to get the maximum token position is appropriate sinceBTreeMapmaintains sorted order.packages/rs-dpp/src/data_contract/document_type/token_costs/mod.rs (1)
56-90: LGTM! Consistent implementation of reference-returning accessors.The new reference-returning getter methods follow the established pattern perfectly:
- Consistent naming convention with
_refsuffix- Proper delegation to inner
TokenCostsV0implementation- Maintains the same structure as existing owned-value getters
This provides efficient reference access to token costs without unnecessary cloning.
packages/rs-dpp/src/errors/consensus/state/data_contract/data_contract_not_found_error.rs (1)
1-32: LGTM! Well-structured error type following established patterns.The
DataContractNotFoundErrorimplementation is excellent:
- Proper derive attributes for error handling, serialization, and debugging
- Clear error message including the missing data contract ID
- Consistent constructor and getter pattern
- Proper conversion to
ConsensusErrorviaStateErrorwrappingThis follows the same patterns as other error types in the codebase and will integrate well with the consensus error handling framework.
packages/rs-dpp/src/errors/consensus/state/token/invalid_token_position_error.rs (1)
26-50: LGTM! Standard error type implementation.The constructor, getters, and
ConsensusErrorconversion follow established patterns perfectly:
- Clear constructor with descriptive parameter names
- Simple getter methods for both fields
- Proper conversion via
StateErrorwrappingThe implementation is consistent with other error types in the consensus framework.
packages/rs-dpp/src/data_contract/document_type/accessors/mod.rs (2)
630-644: LGTM! Well-designed aggregate accessor methods.The new
all_document_token_costsandall_external_token_costs_contract_tokensmethods are excellent additions:
- Version compatibility: V0 returns empty collections, V1 delegates appropriately
- Clear naming: Method names clearly indicate they return aggregate data
- Proper return types: Using references and appropriate collection types
- Consistent pattern: Follows the same structure as other V1Getters methods
These methods provide valuable aggregate access to token cost information.
690-704: LGTM! Consistent implementation across all document type variants.The implementations for
DocumentTypeRefandDocumentTypeMutRefcorrectly mirror the mainDocumentTypeimplementation:
- Identical patterns: All three implementations follow the same V0/V1 branching logic
- Type consistency: Each returns the same types and handles empty cases properly
- Complete coverage: All document type variants now support these aggregate accessors
This ensures consistent behavior regardless of which document type variant is used.
Also applies to: 750-764
packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/data_contract_create/mod.rs (1)
2559-2684: Excellent test coverage for invalid token position validation.This test effectively validates that referencing an invalid token position (position 4) within an existing external token contract triggers an
InvalidTokenPositionStateError. The test properly creates a valid token contract first, then references an invalid position, which is the correct approach for this scenario.The test structure is solid and the error assertion is appropriate:
assert_matches!( processing_result.execution_results().as_slice(), [StateTransitionExecutionResult::PaidConsensusError( ConsensusError::StateError(StateError::InvalidTokenPositionStateError(_)), _ )] );
...execution/validation/state_transition/state_transitions/data_contract_update/state/v0/mod.rs
Outdated
Show resolved
Hide resolved
QuantumExplorer
changed the title
Issue being fixed or feature implemented
Document types were allowing invalid tokens for token payments, which could lead to errors during validation.
What was done?
Updated the logic in the document type methods to ensure that only valid tokens are targeted for token payments. This involved modifying the
create_document_types_from_document_schemasmethods to include token configuration checks and updating related error handling for invalid token positions.How Has This Been Tested?
Changes have been validated through existing unit tests, which were updated to cover new scenarios involving token configurations.
Breaking Changes
None
Checklist
For repository code-owners and collaborators only
Summary by CodeRabbit
New Features
contractIdusage in documents paid by tokens, enhancing error reporting.Bug Fixes
Refactor
Chores