fix(drive-abci): make sure identities in token config exist #2583
Conversation
|
Warning Rate limit exceeded@QuantumExplorer has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 4 minutes and 24 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
WalkthroughThis update introduces comprehensive validation for identities referenced in token configurations within data contract creation and update transitions. A new error type, Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant DriveAbci
participant PlatformDrive
participant StateValidator
Client->>DriveAbci: Submit DataContractCreate or Update Transition
DriveAbci->>StateValidator: validate_state_v0(transition)
StateValidator->>PlatformDrive: Fetch referenced identities (auth keys or balances)
alt Identity exists
PlatformDrive-->>StateValidator: Identity found
StateValidator-->>DriveAbci: Continue validation
else Identity missing
PlatformDrive-->>StateValidator: Identity not found
StateValidator-->>DriveAbci: Return IdentityInTokenConfigurationNotFoundError
end
DriveAbci-->>Client: Return validation result (success or error)
Suggested reviewers
Poem
✨ 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
packages/rs-dpp/src/data_contract/associated_token/token_configuration/accessors/v0/mod.rs (1)
8-8: Remove unused importBTreeMap.The static analysis warning indicates that
BTreeMapis imported but never used in this file. While it may be used in the implementation ofall_change_control_rules()in other files, it's not needed in this trait definition.-use std::collections::{BTreeMap, BTreeSet}; +use std::collections::BTreeSet;🧰 Tools
🪛 GitHub Check: Rust packages (wasm-dpp) / Linting
[warning] 8-8: unused import:
BTreeMap
warning: unused import:BTreeMap
--> packages/rs-dpp/src/data_contract/associated_token/token_configuration/accessors/v0/mod.rs:8:24
|
8 | use std::collections::{BTreeMap, BTreeSet};
| ^^^^^^^^
|
= note:#[warn(unused_imports)]on by default🪛 GitHub Check: Rust packages (dash-sdk) / Linting
[warning] 8-8: unused import:
BTreeMap
warning: unused import:BTreeMap
--> packages/rs-dpp/src/data_contract/associated_token/token_configuration/accessors/v0/mod.rs:8:24
|
8 | use std::collections::{BTreeMap, BTreeSet};
| ^^^^^^^^
|
= note:#[warn(unused_imports)]on by defaultpackages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/data_contract_create/mod.rs (1)
257-257: Optimization: Improved memory efficiency by using slice literals.Replaced vector creation with slice literals (
&[...]instead of&vec![...]) to avoid unnecessary allocations when passing data toprocess_raw_state_transitions.Also applies to: 320-320, 383-383, 480-480, 608-608, 701-701, 817-817, 924-925, 1018-1018, 1153-1153, 1256-1256, 1393-1393, 1536-1536, 1611-1611, 1647-1647, 1761-1761, 1866-1866, 2004-2004, 2221-2221, 2353-2353, 2483-2483, 2611-2611, 2740-2740, 2868-2868, 2996-2996
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
packages/rs-dpp/src/data_contract/associated_token/token_configuration/accessors/mod.rs(1 hunks)packages/rs-dpp/src/data_contract/associated_token/token_configuration/accessors/v0/mod.rs(2 hunks)packages/rs-dpp/src/data_contract/associated_token/token_configuration/v0/accessors.rs(1 hunks)packages/rs-dpp/src/errors/consensus/codes.rs(2 hunks)packages/rs-dpp/src/errors/consensus/state/identity/identity_for_change_control_rule_not_found.rs(1 hunks)packages/rs-dpp/src/errors/consensus/state/identity/mod.rs(1 hunks)packages/rs-dpp/src/errors/consensus/state/state_error.rs(2 hunks)packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/batch/tests/token/distribution/perpetual/block_based.rs(1 hunks)packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/data_contract_create/mod.rs(25 hunks)packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/data_contract_create/state/v0/mod.rs(3 hunks)packages/rs-drive/src/drive/identity/key/fetch/fetch_identity_keys/v0/mod.rs(2 hunks)packages/rs-drive/src/drive/identity/key/fetch/mod.rs(18 hunks)packages/wasm-dpp/src/errors/consensus/consensus_error.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.
🧬 Code Graph Analysis (2)
packages/rs-dpp/src/data_contract/associated_token/token_configuration/accessors/v0/mod.rs (2)
packages/rs-dpp/src/data_contract/associated_token/token_configuration/accessors/mod.rs (1)
all_change_control_rules(160-164)packages/rs-dpp/src/data_contract/associated_token/token_configuration/v0/accessors.rs (1)
all_change_control_rules(163-196)
packages/rs-dpp/src/data_contract/associated_token/token_configuration/accessors/mod.rs (2)
packages/rs-dpp/src/data_contract/associated_token/token_configuration/accessors/v0/mod.rs (1)
all_change_control_rules(75-75)packages/rs-dpp/src/data_contract/associated_token/token_configuration/v0/accessors.rs (1)
all_change_control_rules(163-196)
🪛 GitHub Check: Rust packages (wasm-dpp) / Linting
packages/rs-dpp/src/data_contract/associated_token/token_configuration/accessors/v0/mod.rs
[warning] 8-8: unused import: BTreeMap
warning: unused import: BTreeMap
--> packages/rs-dpp/src/data_contract/associated_token/token_configuration/accessors/v0/mod.rs:8:24
|
8 | use std::collections::{BTreeMap, BTreeSet};
| ^^^^^^^^
|
= note: #[warn(unused_imports)] on by default
🪛 GitHub Check: Rust packages (dash-sdk) / Linting
packages/rs-dpp/src/data_contract/associated_token/token_configuration/accessors/v0/mod.rs
[warning] 8-8: unused import: BTreeMap
warning: unused import: BTreeMap
--> packages/rs-dpp/src/data_contract/associated_token/token_configuration/accessors/v0/mod.rs:8:24
|
8 | use std::collections::{BTreeMap, BTreeSet};
| ^^^^^^^^
|
= note: #[warn(unused_imports)] on by default
packages/rs-drive/src/drive/identity/key/fetch/mod.rs
[failure] 26-26: unresolved import crate::drive::identity::identity_query_keys_security_level_tree_path_vec
error[E0432]: unresolved import crate::drive::identity::identity_query_keys_security_level_tree_path_vec
--> packages/rs-drive/src/drive/identity/key/fetch/mod.rs:26:5
|
26 | identity_query_keys_security_level_tree_path_vec, identity_transfer_keys_path_vec,
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
| |
| no identity_query_keys_security_level_tree_path_vec in drive::identity
| help: a similar name exists in the module: identity_query_keys_tree_path_vec
|
note: found an item that was configured out
--> packages/rs-drive/src/drive/identity/mod.rs:266:15
|
266 | pub(crate) fn identity_query_keys_security_level_tree_path_vec(
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
note: the item is gated behind the server feature
--> packages/rs-drive/src/drive/identity/mod.rs:265:7
|
265 | #[cfg(feature = "server")]
| ^^^^^^^^^^^^^^^^^^
[failure] 964-964: failed to resolve: use of undeclared type SecurityLevel
error[E0433]: failed to resolve: use of undeclared type SecurityLevel
--> packages/rs-drive/src/drive/identity/key/fetch/mod.rs:964:21
|
964 | SecurityLevel::MASTER,
| ^^^^^^^^^^^^^ use of undeclared type SecurityLevel
|
help: a type alias with a similar name exists
|
964 | SecurityLevelU8::MASTER,
| ~~~~~~~~~~~~~~~
help: consider importing this enum
|
2 + use dpp::identity::SecurityLevel;
|
⏰ Context from checks skipped due to timeout of 90000ms (12)
- GitHub Check: Rust packages (drive-abci) / Unused dependencies
- GitHub Check: Rust packages (drive-abci) / Linting
- GitHub Check: Rust packages (drive) / Unused dependencies
- GitHub Check: Rust packages (wasm-dpp) / Tests
- GitHub Check: Rust packages (wasm-dpp) / Formatting
- GitHub Check: Rust packages (dpp) / Check each feature
- GitHub Check: Rust packages (dpp) / Linting
- GitHub Check: Rust packages (dash-sdk) / 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 (22)
packages/rs-dpp/src/errors/consensus/state/identity/mod.rs (1)
20-22: LGTM: New error module for identity validationThis new error module adds support for the scenario described in the PR objectives - handling cases where identities referenced in token configuration change control rules don't exist.
packages/wasm-dpp/src/errors/consensus/consensus_error.rs (2)
80-80: LGTM: Imported new error type for WASM bindingsCorrectly imports the new error type to make it available for WASM error mapping.
412-414: LGTM: Added error conversion for the new error typeThe new match arm properly integrates the
IdentityForChangeControlRuleNotFoundErrorinto the consensus error system, following the established pattern of using thegeneric_consensus_error!macro.packages/rs-drive/src/drive/identity/key/fetch/fetch_identity_keys/v0/mod.rs (2)
2-3: LGTM: Added new key request type importImport now includes the
LatestAuthenticationMasterKeyenum variant, which is used to verify existence of identities referenced in token change control rules.
42-42: LGTM: Updated match arm to handle the new request typeThe updated match arm now handles the
LatestAuthenticationMasterKeyrequest type identically toAllKeys, reusing the same query logic and result handling. This is appropriate since both request types need to retrieve identity key information from the same path.packages/rs-dpp/src/data_contract/associated_token/token_configuration/accessors/mod.rs (1)
159-164: LGTM: Added accessor method for all change control rulesThis method provides a consolidated way to access all change control rules in a token configuration, which supports the PR objective of verifying that referenced identities exist. The implementation correctly delegates to the corresponding method in the V0 implementation, following the established pattern in this file.
packages/rs-dpp/src/data_contract/associated_token/token_configuration/accessors/v0/mod.rs (1)
74-75: LGTM! Good addition of consolidated accessor for change control rules.This method provides a clear way to access all change control rules in a token configuration, including those nested within distribution rules, making validation more straightforward.
packages/rs-dpp/src/errors/consensus/state/state_error.rs (2)
38-38: LGTM! Well-defined import for the new error type.Good addition of the import for the new identity validation error.
303-304: LGTM! Properly integrated new error variant.The new error variant correctly follows the established pattern in the enum, using the transparent error attribute to wrap the specific error type.
packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/batch/tests/token/distribution/perpetual/block_based.rs (1)
758-758: LGTM! Function name changed for consistency.Good improvement to match the naming convention used throughout the codebase, using lowercase 'max' instead of uppercase 'MAX'.
packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/data_contract_create/state/v0/mod.rs (3)
5-5: LGTM! Well-structured import additions.Appropriately added imports for the new validation logic, including
BTreeSetfor tracking validated identities, the new error type, and the identity key fetch functionality.Also applies to: 8-8, 14-14, 27-30
77-120: LGTM! Excellent implementation of identity validation for change control rules.This implementation effectively validates that all identities referenced in token configuration change control rules actually exist before allowing contract creation. Key strengths:
- Uses a
BTreeSetto track already validated identities, avoiding redundant lookups- Properly fetches the latest authentication master key for each identity
- Returns a clear, specific error when an identity doesn't exist
- Includes contract ID, token position, rule path, and identity ID in the error for easy debugging
The code is well-structured and seamlessly integrates with the existing validation flow.
81-81: LGTM! Good use of the new consolidated accessor.Using the new
all_change_control_rules()method here makes the code more maintainable by ensuring all rules are consistently validated, including those nested in distribution rules.packages/rs-dpp/src/data_contract/associated_token/token_configuration/v0/accessors.rs (1)
163-196: Adding accessor for comprehensive token control rule inspection.The implementation provides a consolidated view of all change control rules in the token configuration, which is essential for the PR's objective of validating referenced identities. This method effectively aggregates rules from both top-level fields and nested within distribution rules, each with a descriptive path string.
packages/rs-dpp/src/errors/consensus/codes.rs (2)
238-239: Added error codes for new validation checks.New error codes 40005 and 40006 have been added to the data contract errors range, properly integrating the new error types into the consensus error handling system.
315-317: Shifted error codes to maintain consistent numbering.Token error codes have been properly adjusted following the relocation of
PreProgrammedDistributionTimestampInPastErrorto the data contract errors range.packages/rs-drive/src/drive/identity/key/fetch/mod.rs (2)
86-88: Added new key request type for validating identity existence.The addition of
LatestAuthenticationMasterKeyprovides an efficient way to check if an identity exists, which is exactly what's needed for validating token configuration references.
687-691: Processing cost calculation for new request type.Correctly applies the single identity key fetch cost for the new request type, consistent with similar operations.
packages/rs-dpp/src/errors/consensus/state/identity/identity_for_change_control_rule_not_found.rs (1)
1-58: Well-structured error type for missing identity in change control rules.This new error type provides a clear and detailed error message that includes all relevant context (identity ID, rule path, token position, and contract ID). The implementation follows Rust best practices for error types with appropriate trait derivations and proper serialization support.
The error's design serves the PR objective by providing specific information about which identity is missing in which rule, allowing for better debugging and troubleshooting of contract creation issues.
packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/data_contract_create/mod.rs (3)
947-1050: New test: Verifies successful validation when token configuration references a valid identity.This test ensures that data contract creation succeeds when the token configuration references an identity ID that exists in the system. It verifies the positive case of the new identity existence validation.
1475-1475: Added import for StateError to support new identity validation tests.The import enables assertion testing against the new
IdentityForChangeControlRuleNotFoundErrorerror type.
1797-1903: New test: Verifies contract creation fails when referencing non-existent identity.This test is the critical part of the PR that verifies the system now properly validates identity references in token configurations. It confirms that:
- Creating a contract with a token configuration that references a non-existent identity fails
- The specific error
IdentityForChangeControlRuleNotFoundErroris returned- No token balance is created
The test directly addresses the issue in the PR objectives where contracts could previously be created with invalid identity references.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (7)
packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/data_contract_update/mod.rs (2)
673-673: Performance optimization with slice syntaxChanged
&vec![item.clone()]to the more efficient&[item.clone()]syntax, avoiding unnecessary vector allocation.Also applies to: 795-795, 923-923, 1053-1053, 1185-1185, 1619-1619, 1801-1801
2542-2542: Consistent platform_version parameter passingUpdated parameter passing to use values instead of references, maintaining consistency with the rest of the codebase.
Also applies to: 2557-2557, 2906-2906, 2910-2910, 2921-2921, 2938-2938, 2939-2939
packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/data_contract_update/state/v0/mod.rs (3)
186-195: Missing accounting for identity-key fetch operation
fetch_identity_keysperforms an I/O-heavy GroveDB read but noValidationOperation
is added toexecution_context, unlike the balance checks that add
RetrieveIdentityTokenBalance.If accounting accuracy is important for fee estimation, add a dedicated
ValidationOperation::RetrieveIdentityAuthenticationKey(or reuse an existing one):let maybe_key = platform.drive.fetch_identity_keys::<OptionalSingleIdentityPublicKeyOutcome>( /* … */ )?; +execution_context + .add_operation(ValidationOperation::RetrieveIdentityAuthenticationKey);This keeps operation tracking symmetrical with the balance reads.
224-240: EarlycontinueafterOption::Nonecheck could skipadd_operation
add_operation(…RetrieveIdentityTokenBalance)is invoked before theNonecheck.
If the balance isNone, the earlyreturnprevents subsequent operations from
being appended, which is fine. However, the already-added balance-operation will
remain in the context even though the transition is aborted.To avoid charging for a failed read that is never useful, push the
operation only aftermaybe_balance.is_some():- let maybe_balance = platform.drive.fetch_identity_balance(/*…*/)?; - execution_context.add_operation(ValidationOperation::RetrieveIdentityTokenBalance); - if maybe_balance.is_none() { … } + let maybe_balance = platform.drive.fetch_identity_balance(/*…*/)?; + if maybe_balance.is_none() { … } + execution_context.add_operation(ValidationOperation::RetrieveIdentityTokenBalance);Same applies to the pre-programmed and minting recipient blocks below.
487-489: Nit: avoid vague comment in tests
// We have tons of operations here so not sure if we want to assert all of themLeaving TODO-like comments can clutter tests. Either remove, or replace with a succinct explanation such as:
// Operation count is not asserted here as it’s implementation-specific.packages/rs-dpp/src/errors/consensus/state/identity/identity_for_token_configuration_not_found_error.rs (2)
11-16: Enum variant names could be more self-descriptive
DefaultMintingRecipientandPerpetualDistributionRecipientare clear, but
ChangeControlRule(String)hides the specific rule name in a generic tuple
variant. A struct-like variant would improve readability:- ChangeControlRule(String), + ChangeControlRule { rule_name: String },This avoids positional-argument confusion and provides an explicit field
name in pattern matches.
62-66: MissingFrom<ProtocolError>orcode()helperMost consensus errors expose a
code()method orFrom<…>conversions for
ergonomic handling. Providing the same for the new error will keep the API
consistent:impl IdentityInTokenConfigurationNotFoundError { pub const ERROR_CODE: u32 = 40006; pub fn code(&self) -> u32 { Self::ERROR_CODE } }Optionally add
impl From<ProtocolError> for IdentityInTokenConfigurationNotFoundError
if applicable.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
packages/rs-dpp/src/data_contract/associated_token/token_configuration/accessors/v0/mod.rs(1 hunks)packages/rs-dpp/src/errors/consensus/codes.rs(2 hunks)packages/rs-dpp/src/errors/consensus/state/identity/identity_for_token_configuration_not_found_error.rs(1 hunks)packages/rs-dpp/src/errors/consensus/state/identity/mod.rs(1 hunks)packages/rs-dpp/src/errors/consensus/state/state_error.rs(2 hunks)packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/data_contract_create/mod.rs(25 hunks)packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/data_contract_create/state/v0/mod.rs(3 hunks)packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/data_contract_update/mod.rs(14 hunks)packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/data_contract_update/state/v0/mod.rs(8 hunks)packages/rs-drive/src/drive/identity/key/fetch/fetch_identity_keys/v0/mod.rs(2 hunks)packages/wasm-dpp/src/errors/consensus/consensus_error.rs(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- packages/rs-dpp/src/data_contract/associated_token/token_configuration/accessors/v0/mod.rs
- packages/rs-dpp/src/errors/consensus/state/identity/mod.rs
- packages/rs-dpp/src/errors/consensus/state/state_error.rs
- packages/rs-drive/src/drive/identity/key/fetch/fetch_identity_keys/v0/mod.rs
- packages/wasm-dpp/src/errors/consensus/consensus_error.rs
- packages/rs-dpp/src/errors/consensus/codes.rs
- packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/data_contract_create/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 (19)
- GitHub Check: Rust packages (wasm-dpp) / Formatting
- GitHub Check: Rust packages (wasm-dpp) / Unused dependencies
- GitHub Check: Rust packages (wasm-dpp) / Tests
- GitHub Check: Rust packages (dash-sdk) / Formatting
- GitHub Check: Rust packages (dpp) / Check each feature
- GitHub Check: Rust packages (wasm-dpp) / Linting
- GitHub Check: Rust packages (dpp) / Tests
- GitHub Check: Rust packages (dash-sdk) / Tests
- GitHub Check: Rust packages (drive-abci) / Check each feature
- GitHub Check: Rust packages (drive-abci) / Tests
- GitHub Check: Rust packages (dash-sdk) / Linting
- GitHub Check: Rust packages (dash-sdk) / Check each feature
- GitHub Check: Rust packages (drive-abci) / Linting
- GitHub Check: Rust packages (drive) / Tests
- GitHub Check: Rust packages (drive) / Linting
- 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 Docker images (Dashmate helper, dashmate-helper, dashmate-helper) / Build Dashmate helper image
- GitHub Check: Build JS packages / Build JS
🔇 Additional comments (8)
packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/data_contract_create/state/v0/mod.rs (4)
81-130: Excellent identity validation implementation for change control rulesThe added validation effectively ensures that identities referenced in token configuration change control rules actually exist, addressing the core issue from the PR objectives. The use of a
BTreeSetto track already validated identities avoids redundant validations, improving performance.
132-164: Well-implemented validation for minting recipientsThis section appropriately verifies that default minting recipient identities exist by checking their token balance, returning detailed error information when validation fails.
166-198: Thorough validation for perpetual distribution recipientsThe implementation correctly checks if perpetual distribution recipient identities exist, maintaining the same validation pattern established throughout the code.
214-244: Complete validation coverage for pre-programmed distribution recipientsThis section extends the existing timestamp validation to also verify that all identity recipients in pre-programmed distributions exist, ensuring comprehensive validation of token configurations.
packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/data_contract_update/mod.rs (3)
1221-1229: Appropriate module imports for token validationAdded necessary imports for token distribution rules, perpetual distribution, and change control rules to support the new token validation tests.
1333-1537: Comprehensive test coverage for identity validation in tokensThese tests verify both the success case (token with existing identity) and failure case (token with non-existent identity), ensuring the new identity validation logic works correctly.
1826-2103: Thorough test coverage for token configuration edge casesAdded tests for important validation scenarios:
- Missing main group validation
- Invalid distribution functions
- Unsupported random distribution
- Prevention of overwriting existing token positions
These tests help ensure the robustness of the token configuration validation.
packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/data_contract_update/state/v0/mod.rs (1)
303-336: Use a uniform identity-existence probeFor change-control rules you probe existence via keys; for distribution & minting via balance.
An identity with zero balance but valid keys will pass CC-rules and fail distribution, producing inconsistent behaviour.Recommend standardising on a single authoritative check (e.g.
fetch_identity(keys)), or at least documenting the intentional difference to avoid future confusion.
...execution/validation/state_transition/state_transitions/data_contract_update/state/v0/mod.rs
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (10)
packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/data_contract_update/state/v0/mod.rs (4)
500-501: Minor comment improvement suggestionThe comment is somewhat vague. Consider being more specific about the nature of the operations being generated during validation.
- // We have tons of operations here so not sure if we want to assert all of them + // There are many validation operations accumulated at this point (identity lookups, token balance checks, etc.) + // Asserting each specific operation would be verbose and brittle to future changes
573-574: Minor comment improvement suggestionSame comment improvement as earlier in the file.
- // We have tons of operations here so not sure if we want to assert all of them + // There are many validation operations accumulated at this point (identity lookups, token balance checks, etc.) + // Asserting each specific operation would be verbose and brittle to future changes
658-659: Minor comment improvement suggestionSame comment improvement as earlier in the file.
- // We have tons of operations here so not sure if we want to assert all of them + // There are many validation operations accumulated at this point (identity lookups, token balance checks, etc.) + // Asserting each specific operation would be verbose and brittle to future changes
739-740: Minor comment improvement suggestionSame comment improvement as earlier in the file.
- // We have tons of operations here so not sure if we want to assert all of them + // There are many validation operations accumulated at this point (identity lookups, token balance checks, etc.) + // Asserting each specific operation would be verbose and brittle to future changespackages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/data_contract_create/state/v0/mod.rs (6)
451-452: Minor comment improvement suggestionThe comment is somewhat vague. Consider being more specific about the nature of the operations being generated during validation.
- // We have tons of operations here so not sure if we want to assert all of them + // There are many validation operations accumulated at this point (identity lookups, token balance checks, etc.) + // Asserting each specific operation would be verbose and brittle to future changes
544-545: Minor comment improvement suggestionSame comment improvement as earlier in the file.
- // We have tons of operations here so not sure if we want to assert all of them + // There are many validation operations accumulated at this point (identity lookups, token balance checks, etc.) + // Asserting each specific operation would be verbose and brittle to future changes
624-625: Minor comment improvement suggestionSame comment improvement as earlier in the file.
- // We have tons of operations here so not sure if we want to assert all of them + // There are many validation operations accumulated at this point (identity lookups, token balance checks, etc.) + // Asserting each specific operation would be verbose and brittle to future changes
687-688: Minor comment improvement suggestionSame comment improvement as earlier in the file.
- // We have tons of operations here so not sure if we want to assert all of them + // There are many validation operations accumulated at this point (identity lookups, token balance checks, etc.) + // Asserting each specific operation would be verbose and brittle to future changes
757-758: Minor comment improvement suggestionSame comment improvement as earlier in the file.
- // We have tons of operations here so not sure if we want to assert all of them + // There are many validation operations accumulated at this point (identity lookups, token balance checks, etc.) + // Asserting each specific operation would be verbose and brittle to future changes
805-806: Minor comment improvement suggestionSame comment improvement as earlier in the file.
- // We have tons of operations here so not sure if we want to assert all of them + // There are many validation operations accumulated at this point (identity lookups, token balance checks, etc.) + // Asserting each specific operation would be verbose and brittle to future changes
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/data_contract_create/state/v0/mod.rs(9 hunks)packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/data_contract_update/state/v0/mod.rs(8 hunks)packages/rs-drive/src/drive/identity/key/fetch/mod.rs(19 hunks)packages/rs-drive/src/drive/identity/mod.rs(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/rs-drive/src/drive/identity/mod.rs
- packages/rs-drive/src/drive/identity/key/fetch/mod.rs
⏰ Context from checks skipped due to timeout of 90000ms (14)
- GitHub Check: Rust packages (drive-abci) / Detect immutable structure changes
- GitHub Check: Rust packages (drive) / Linting
- GitHub Check: Rust packages (drive) / Unused dependencies
- GitHub Check: Rust packages (drive) / Tests
- GitHub Check: Rust packages (wasm-dpp) / Linting
- GitHub Check: Rust packages (wasm-dpp) / Tests
- GitHub Check: Rust packages (wasm-dpp) / Formatting
- GitHub Check: Rust packages (dpp) / Tests
- GitHub Check: Rust packages (dpp) / Check each feature
- GitHub Check: Rust packages (dpp) / Linting
- 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 Docker images (Dashmate helper, dashmate-helper, dashmate-helper) / Build Dashmate helper image
- GitHub Check: Build JS packages / Build JS
🔇 Additional comments (2)
packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/data_contract_update/state/v0/mod.rs (1)
169-351: Great implementation of identity validation with optimized cache reuse!This validation logic ensures that all identities referenced in token configurations actually exist in the platform before allowing data contract updates. The code properly implements the cache optimization we discussed previously, using
validated_identitiesacross all identity checks to avoid redundant database reads.The implementation covers all key identity contexts:
- Change control rules authorized action takers
- Perpetual distribution recipients
- Pre-programmed distribution recipients
- Default minting recipients
Each validation path returns appropriate detailed errors when an identity doesn't exist, making troubleshooting easier for users.
packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/data_contract_create/state/v0/mod.rs (1)
81-258: Well-implemented identity validation for contract creationThe implementation ensures all identities referenced in token configurations exist before allowing contract creation. This prevents contracts with invalid identity references from entering the system, which aligns perfectly with the PR objectives.
Key strengths:
- Uses
validated_identitiesto optimize and avoid redundant identity lookups- Provides specific error contexts based on where the identity was referenced
- Properly adds operation tracking for fee calculations
- Handles different identity roles appropriately (change control vs distribution recipients)
This implementation mirrors the approach used in the contract update validation, ensuring consistency across the codebase.
…:dashpay/platform into fix/makeSureIdentitiesInTokenConfigExist
Issue being fixed or feature implemented
Previously you could create a contract where the token config had authorized action taker identity id of identities that did not exist. For example you could write an identifier with only 0s.
What was done?
We not verify on data contract create and update that the identity exists.
How Has This Been Tested?
Added unit tests
Checklist:
For repository code-owners and collaborators only
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Refactor
Style