feat(platform): token distribution part two #2450
Conversation
WalkthroughThe pull request adds comprehensive support for token pre‐programmed distributions across multiple language clients and the underlying platform. New RPC methods and message types have been integrated into Java, NodeJS, Objective‑C, Python, and Web clients. Additionally, extensive modifications have been made to the Rust modules (rs-dpp, Drive, and versioning components) to support new distribution keys, encoding/decoding, cost estimation, querying, and verification. Several helper functions, enums, and structures have been introduced or updated to ensure proper handling of token distribution operations throughout the service, state transitions, and storage layers. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant gRPC Server
participant ServiceLogic
Client->>gRPC Server: getTokenPreProgrammedDistributions(request)
gRPC Server->>ServiceLogic: Process pre-programmed distribution request
ServiceLogic-->>gRPC Server: Distribution response (or proof)
gRPC Server->>Client: Return response
Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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 review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (40)
packages/rs-dpp/src/data_contract/associated_token/token_perpetual_distribution/mod.rs (1)
10-12: Consider adding module-level documentation.The new modules provide a clear separation of concerns for token distribution functionality. Consider adding module-level documentation (//! comments) to describe the purpose and contents of each module.
packages/rs-drive/src/drive/tokens/estimated_costs/for_token_pre_programmed_distribution/mod.rs (2)
51-63: Validate handling of empty or largetimesiterators.
Iftimesis empty, the method currently performs no operations, which may or may not be intended. For large iterators, ensure this approach is performant enough, as each timestamp might add an estimated layer entry.
65-70: Confirm that all future versions will follow a similar pattern.
The version handling logic is straightforward but ensure that any new version uses the same naming convention (v1,v2, etc.) to keep code consistent and easy to follow.packages/rs-drive/src/drive/tokens/distribution/add_pre_programmed_distribution/mod.rs (1)
14-21: Add clarity on subtree creation.
The docstring states that necessary subtrees are created, but the actual creation logic is delegated toadd_pre_programmed_distributions_v0. Consider documenting or referencing that logic explicitly for maintainability.packages/rs-drive/src/drive/tokens/distribution/prove/mod.rs (2)
31-31: Fix doc mismatch about the returned typeThe doc comment says "A
Resultcontaining a nestedBTreeMapon success or anErroron failure," but the function actually returns aResult<Vec<u8>, Error>. Please align the documentation with the code:-/// A `Result` containing a nested `BTreeMap` on success or an `Error` on failure. +/// A `Result` containing proof bytes (`Vec<u8>`) on success or an `Error` on failure.
69-69: Fix doc mismatch about the returned typeSimilarly, the doc comment refers to a nested
BTreeMap, while the method still returnsResult<Vec<u8>, Error>. Please update the doc block here as well:-/// A `Result` containing a nested `BTreeMap` on success or an `Error` on failure. +/// A `Result` containing proof bytes (`Vec<u8>`) on success or an `Error` on failure.packages/rs-drive/src/drive/tokens/distribution/add_perpetual_distribution/v0/mod.rs (1)
40-45: Remove or clarify commented-out codeThe commented-out code within this
ifstatement may cause confusion. Consider removing it if it's no longer needed or uncommenting it if it’s intended to be used:if estimated_costs_only_with_layer_info.is_some() { - // Drive::add_estimation_costs_for_perpetual_distribution( - // estimated_costs_only_with_layer_info, - // &platform_version.drive, - // )?; }packages/rs-drive-abci/src/query/token_queries/token_pre_programmed_distributions/v0/mod.rs (3)
37-50: Clarify default vs max query limit.
The code checks iflimit_value as u16exceedsconfig.default_query_limit, yet it reports an error message referencingconfig.max_query_limit. This may cause confusion ifdefault_query_limitandmax_query_limitdiffer. Consider ensuring that the error accurately reflects the specific limit being exceeded or aligning both values if they are intended to match.- .ok_or(drive::error::Error::Query(QuerySyntaxError::InvalidLimit( - format!("limit greater than max limit {}", config.max_query_limit), - )))?; + .ok_or(drive::error::Error::Query(QuerySyntaxError::InvalidLimit( + format!("limit greater than allowed limit {}", config.default_query_limit), + )))?;
81-100: Extract proof handling into a dedicated helper method.
The “prove” logic is nicely separated by an if-else, but we can further improve readability by extracting the proof sequence into a dedicated helper. This would keep the main function concise and more maintainable.
101-136: Check for large distribution lists.
When fetching token distributions, if the returned list is unexpectedly large, performance might degrade. Consider adding guard checks for the total distributions count or evaluating a streaming approach to prevent potential memory overhead.packages/rs-dpp/src/data_contract/associated_token/token_perpetual_distribution/distribution_function/encode.rs (2)
6-12: Validate infinities.
NotNan<f64>currently protects only against NaN. If your logic disallows infinity or negative infinity, consider adding checks (e.g., returning an error if the decoded float is infinite) to prevent unexpected extremes.
144-208: Reduce repetition in decode/borrow_decode implementations.
TheDecodeandBorrowDecodeimplementations largely duplicate logic. Consider unifying them via a shared helper or macro to simplify maintenance and reduce risk of inconsistencies.packages/rs-drive/src/drive/tokens/distribution/add_pre_programmed_distribution/v0/mod.rs (3)
103-104: Avoid overshadowing variable names.
Infor (time, distribution) in distribution.distributions(), the loop variabledistributionshadows the parameter nameddistribution. Renaming the inner variable to something likeentriesordist_entryclarifies the logic.-for (time, distribution) in distribution.distributions() { +for (time, dist_entry) in distribution.distributions() { ... }
82-100: Validate existence checks earlier.
The code returns an error if the pre-programmed distribution already exists. If feasible, validate existence in a preceding step (e.g., during a prior check or guard) to fail fast before performing partial insert operations.
134-183: Assess potential integer overflow.
You correctly guard against amounts exceedingi64::MAX. Also consider logging or monitoring high values that are not rejected but remain dangerously close to this limit to avoid silent near-overflow issues.packages/rs-drive/src/drive/tokens/paths.rs (1)
30-31: Correct the doc comment mismatch forTOKEN_TIMED_DISTRIBUTIONS_KEY.The doc comment says "Key for the perpetual distributions," but the constant clearly represents timed distributions. Please fix to reflect the intended meaning and avoid confusion.
packages/rs-drive-abci/src/execution/platform_events/protocol_upgrade/perform_events_on_first_block_of_protocol_change/v0/mod.rs (1)
405-470: Ensure consistency in subtree creation and coverage in tests.All nested distribution subtrees are initialized as empty trees. Confirm that each sub-distribution scenario (timed, perpetual, pre-programmed) either needs sum tracking or can remain a plain tree. Also consider adding or expanding test scenarios to validate each subtree's creation and usage paths.
packages/rs-drive/src/drive/tokens/distribution/mod.rs (1)
1-11: Add or expand documentation for newly introduced modules.The new modules
add_perpetual_distribution,add_pre_programmed_distribution,fetch, andproveare referenced conditionally via feature flags. For clarity, consider adding short doc comments explaining their responsibilities and expected usage patterns.packages/rs-dpp/src/data_contract/associated_token/token_pre_programmed_distribution/methods/v0/mod.rs (1)
6-24: Consider enhancing method documentation.The trait implementation is well-structured and uses appropriate data structures. However, the method documentation could be more detailed.
Consider adding more detailed documentation for each method:
/// Trait for managing pre-programmed token distributions. pub trait TokenPreProgrammedDistributionV0Methods { - /// Gets the scheduled token distributions. + /// Gets the scheduled token distributions. + /// Returns a BTreeMap where: + /// - Keys are timestamps in milliseconds + /// - Values are maps of recipient identifiers to token amounts fn distributions(&self) -> &BTreeMap<TimestampMillis, BTreeMap<Identifier, TokenAmount>>; - /// Sets the scheduled token distributions. + /// Sets the scheduled token distributions. + /// Replaces the entire distribution schedule with the provided map. fn set_distributions( &mut self, distributions: BTreeMap<TimestampMillis, BTreeMap<Identifier, TokenAmount>>, ); - /// Adds a new token distribution for a recipient at a specific time. + /// Adds a new token distribution for a recipient at a specific time. + /// If a distribution already exists for the given time and recipient, + /// it will be overwritten with the new amount. fn add_distribution( &mut self, time: TimestampMillis, recipient: Identifier, amount: TokenAmount, ); }packages/rs-dpp/src/data_contract/associated_token/token_perpetual_distribution/distribution_recipient.rs (1)
12-14: Enhance documentation for EvonodesByParticipation.The documentation for
EvonodesByParticipationcould be more detailed about the participation criteria and epoch-based distribution mechanism.Consider expanding the documentation:
- /// Distribute tokens by participation - /// This distribution can only happen when choosing epoch based distribution + /// Distribute tokens to Evonodes based on their participation metrics + /// This distribution can only happen when choosing epoch based distribution. + /// The distribution amount for each Evonode is calculated proportionally + /// to their participation score within the epoch. EvonodesByParticipation,packages/rs-dpp/src/data_contract/associated_token/token_perpetual_distribution/methods/v0/mod.rs (1)
5-7: Enhance trait documentation.Consider adding more detailed documentation explaining:
- The purpose and lifecycle of token perpetual distributions
- The relationship between distribution type and recipient
- Example usage scenarios
packages/rs-drive/src/drive/tokens/mod.rs (1)
47-48: Enhance distribution module documentation.The current documentation "Distribution module" is too brief. Consider adding details about:
- The purpose of token distributions
- Types of distributions supported (perpetual, pre-programmed)
- Key functionalities provided by the module
packages/rs-dpp/src/data_contract/associated_token/token_perpetual_distribution/v0/methods.rs (1)
15-17: Add similar overflow protection for time and epoch calculations.The time and epoch calculations could also benefit from overflow protection using checked arithmetic operations.
Also applies to: 20-22
packages/rs-drive/src/drive/tokens/estimated_costs/for_root_token_ms_interval_distribution/mod.rs (1)
12-19: Add documentation for the method parameters and return value.The method implementation looks good, but could benefit from detailed documentation explaining:
- Purpose of the
timesiterator- Structure and usage of the
estimated_costs_only_with_layer_infomap- Expected error conditions
Add rustdoc comments:
+/// Adds cost estimation entries for root token MS interval distribution +/// +/// # Arguments +/// * `times` - Iterator of timestamp milliseconds for intervals +/// * `estimated_costs_only_with_layer_info` - Map to store estimated layer information +/// * `drive_version` - Version information for compatibility check +/// +/// # Returns +/// `Ok(())` on success, or `Error` if version is not supportedpackages/rs-dpp/src/data_contract/associated_token/token_perpetual_distribution/methods/mod.rs (1)
10-36: LGTM! Well-structured accessor implementation.The implementation:
- Properly handles version-specific access
- Provides both getters and setters
- Uses appropriate pattern matching
Consider adding documentation for the trait methods to improve API clarity.
packages/rs-drive/src/drive/tokens/distribution/add_perpetual_distribution/mod.rs (1)
15-28: Enhance method documentation with parameter details.While the basic documentation is present, it would be helpful to add:
- Parameter descriptions
- Expected error conditions
- Example usage
Add detailed rustdoc:
/// Adds a TokenPerpetualDistribution to the state tree. +/// +/// # Arguments +/// * `token_id` - Unique identifier of the token +/// * `owner_id` - Identity of the token owner +/// * `distribution` - Distribution configuration to be added +/// * `block_info` - Current block information +/// * `estimated_costs_only_with_layer_info` - Optional cost estimation map +/// * `batch_operations` - Vector to collect state modifications +/// * `transaction` - Transaction context +/// * `platform_version` - Platform version information +/// +/// # Returns +/// `Ok(())` on success, or `Error` if version is not supportedpackages/rs-dpp/src/data_contract/associated_token/token_perpetual_distribution/reward_distribution_type/mod.rs (1)
8-16: LGTM! Well-structured enum design.The enum effectively models different distribution types with appropriate parameters. Good use of type safety with specific interval types.
Consider adding examples in the doc comments for each variant to illustrate typical usage:
#[derive(Serialize, Deserialize, Decode, Encode, Debug, Clone, PartialEq, Eq, PartialOrd)] pub enum RewardDistributionType { /// An amount of tokens is emitted every n blocks + /// + /// # Example + /// ``` + /// use crate::RewardDistributionType; + /// let distribution = RewardDistributionType::BlockBasedDistribution( + /// 100, // every 100 blocks + /// 1000.into(), // 1000 tokens + /// DistributionFunction::FixedAmount, + /// ); + /// ``` BlockBasedDistribution(BlockHeightInterval, TokenAmount, DistributionFunction),packages/rs-drive/src/drive/tokens/distribution/prove/pre_programmed_distributions/mod.rs (1)
32-40: Consider adding input validation.The method could benefit from validating the
limitparameter to ensure it's within reasonable bounds.pub(super) fn prove_token_pre_programmed_distributions_operations_v0( &self, token_id: [u8; 32], start_at: Option<QueryPreProgrammedDistributionStartAt>, limit: Option<u16>, drive_operations: &mut Vec<LowLevelDriveOperation>, transaction: TransactionArg, platform_version: &PlatformVersion, ) -> Result<Vec<u8>, Error> { + if let Some(limit) = limit { + if limit == 0 { + return Err(Error::InvalidArgument("limit cannot be zero".into())); + } + } let path_query = pre_programmed_distributions_query(token_id, start_at, limit);packages/rs-drive/src/drive/tokens/estimated_costs/for_root_token_ms_interval_distribution/v0/mod.rs (1)
39-39: Document the magic numbers used in estimations.The code uses magic numbers for layer counts and sizes:
- Line 39:
EstimatedLevel(10, false)- Why 10 layers?- Line 58:
DEFAULT_HASH_SIZE_U32 * 2 + 2- What's the reasoning behind this calculation?Consider adding comments explaining these values or extracting them as named constants.
Also applies to: 58-58
packages/rs-drive/src/drive/tokens/estimated_costs/for_token_pre_programmed_distribution/v0/mod.rs (2)
49-49: Consider making the estimation level configurable.The hardcoded value of 10 for
estimated_layer_countmight not be optimal for all scenarios. Consider making this configurable or calculating it based on actual usage patterns.
75-75: Consider making the estimation level configurable.The hardcoded value of 3 for
estimated_layer_countmight not be optimal for all scenarios. Consider making this configurable or calculating it based on actual usage patterns.packages/rs-drive/src/drive/tokens/distribution/fetch/mod.rs (1)
38-38: Consider documenting thestart_atparameter.The
start_atparameter's purpose and usage are not documented in the function's documentation. Consider adding details about how this parameter affects the query behavior.packages/rs-drive/src/drive/initialization/v1/mod.rs (1)
91-131: Consider adding a tree structure diagram in comments.The initialization creates a complex tree structure for token distributions. Consider adding an ASCII diagram in the comments to visualize the tree hierarchy and relationships between different distribution types.
Example structure:
tokens/ ├── distributions/ │ ├── timed/ │ │ ├── ms/ │ │ ├── block/ │ │ └── epoch/ │ ├── perpetual/ │ └── pre_programmed/packages/rs-drive/src/drive/contract/insert/insert_contract/v1/mod.rs (1)
218-247: LGTM! Consider extracting distribution handling into a separate method.The implementation correctly processes both perpetual and pre-programmed distributions. However, to improve readability and maintainability, consider extracting the distribution handling logic into a separate method.
fn insert_contract_operations_v1( &self, contract_element: Element, contract: &DataContract, block_info: &BlockInfo, estimated_costs_only_with_layer_info: &mut Option< HashMap<KeyInfoPath, EstimatedLayerInformation>, >, transaction: TransactionArg, platform_version: &PlatformVersion, ) -> Result<Vec<LowLevelDriveOperation>, Error> { // ... existing code ... for (token_pos, token_config) in contract.tokens() { // ... existing token setup code ... - if let Some(perpetual_distribution) = - token_config.distribution_rules().perpetual_distribution() - { - self.add_perpetual_distribution( - token_id.to_buffer(), - contract.owner_id().to_buffer(), - perpetual_distribution, - block_info, - estimated_costs_only_with_layer_info, - &mut batch_operations, - transaction, - platform_version, - )?; - } - - if let Some(pre_programmed_distribution) = token_config - .distribution_rules() - .pre_programmed_distribution() - { - self.add_pre_programmed_distributions( - token_id.to_buffer(), - contract.owner_id().to_buffer(), - pre_programmed_distribution, - block_info, - estimated_costs_only_with_layer_info, - &mut batch_operations, - transaction, - platform_version, - )?; - } + self.handle_token_distributions( + token_config, + token_id, + contract.owner_id(), + block_info, + estimated_costs_only_with_layer_info, + &mut batch_operations, + transaction, + platform_version, + )?; // ... remaining code ... } } +impl Drive { + fn handle_token_distributions( + &self, + token_config: &TokenConfiguration, + token_id: &TokenId, + owner_id: &IdentityId, + block_info: &BlockInfo, + estimated_costs_only_with_layer_info: &mut Option<HashMap<KeyInfoPath, EstimatedLayerInformation>>, + batch_operations: &mut Vec<LowLevelDriveOperation>, + transaction: TransactionArg, + platform_version: &PlatformVersion, + ) -> Result<(), Error> { + if let Some(perpetual_distribution) = token_config.distribution_rules().perpetual_distribution() { + self.add_perpetual_distribution( + token_id.to_buffer(), + owner_id.to_buffer(), + perpetual_distribution, + block_info, + estimated_costs_only_with_layer_info, + batch_operations, + transaction, + platform_version, + )?; + } + + if let Some(pre_programmed_distribution) = token_config.distribution_rules().pre_programmed_distribution() { + self.add_pre_programmed_distributions( + token_id.to_buffer(), + owner_id.to_buffer(), + pre_programmed_distribution, + block_info, + estimated_costs_only_with_layer_info, + batch_operations, + transaction, + platform_version, + )?; + } + + Ok(()) + } +}packages/dapi-grpc/clients/platform/v0/python/platform_pb2_grpc.py (1)
464-469: Add docstring to describe the method's purpose.The method is missing a descriptive docstring that explains its purpose and behavior.
Apply this diff to add documentation:
def getTokenPreProgrammedDistributions(self, request, context): - """Missing associated documentation comment in .proto file.""" + """Retrieves pre-programmed token distributions for a given token. + + Args: + request: GetTokenPreProgrammedDistributionsRequest containing the token ID + context: gRPC service context + + Returns: + GetTokenPreProgrammedDistributionsResponse containing the distributions + + Raises: + NotImplementedError: Method needs to be implemented by service + """packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/data_contract_create/mod.rs (3)
694-862: Consider adding edge cases to pre-programmed distribution tests.While the test comprehensively covers the happy path, consider adding test cases for:
- Invalid timestamp values (e.g., past dates, duplicate timestamps)
- Out-of-order timestamps
- Empty distribution maps
- Overlapping distributions
813-821: Consider extracting token balance verification to a helper function.The token balance verification logic is duplicated across multiple test cases. Consider extracting it to a helper function to improve maintainability and reduce code duplication:
fn assert_no_token_balance( platform: &TestPlatform, token_id: [u8; 32], identity_id: [u8; 32], platform_version: &PlatformVersion, ) { let token_balance = platform .drive .fetch_identity_token_balance( token_id, identity_id.to_buffer(), None, platform_version, ) .expect("expected to fetch token balance"); assert_eq!(token_balance, None); }Also applies to: 952-960, 1064-1073, 1180-1188, 1316-1324, 1445-1453, 1704-1712, 1833-1841, 1964-1972
868-961: Enhance error messages in test assertions.The test cases for error conditions could benefit from more descriptive error messages in the assertions. Consider adding detailed messages that explain the expected failure condition:
assert_matches!( processing_result.execution_results().as_slice(), [StateTransitionExecutionResult::PaidConsensusError( ConsensusError::BasicError(BasicError::InvalidTokenBaseSupplyError(_)), _ )], "Expected InvalidTokenBaseSupplyError when base supply exceeds maximum" );Also applies to: 964-1074, 1077-1189
packages/dapi-grpc/clients/platform/v0/objective-c/Platform.pbobjc.m (1)
15069-15145: Consider adding documentation for pagination parameters.While the implementation is correct, adding documentation about the pagination behavior (limit, startAtInfo) would help API consumers understand the query parameters better.
Would you like me to generate documentation comments explaining the pagination parameters and their usage?
packages/dapi-grpc/protos/platform/v0/platform.proto (1)
1421-1436: New Request Message Implementation
The newly addedGetTokenPreProgrammedDistributionsRequestmessage is implemented consistently with other request messages (using a nested V0 message and oneof versioning). The fields are clearly defined with appropriate types and field numbers.
- The
token_id,start_at_info(which encapsulates start-time and start-recipient details), andlimitfields are set up properly.- Nitpick: For enhanced naming consistency with the response (which uses
recipient_id), consider renaming thestart_recipientfield withinStartAtInfotostart_recipient_idif it represents an identifier. This can help avoid any ambiguity between a recipient's identity and other potential data.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (73)
packages/dapi-grpc/clients/platform/v0/java/org/dash/platform/dapi/v0/PlatformGrpc.java(9 hunks)packages/dapi-grpc/clients/platform/v0/nodejs/platform_protoc.js(3 hunks)packages/dapi-grpc/clients/platform/v0/objective-c/Platform.pbobjc.h(2 hunks)packages/dapi-grpc/clients/platform/v0/objective-c/Platform.pbobjc.m(2 hunks)packages/dapi-grpc/clients/platform/v0/objective-c/Platform.pbrpc.h(3 hunks)packages/dapi-grpc/clients/platform/v0/objective-c/Platform.pbrpc.m(1 hunks)packages/dapi-grpc/clients/platform/v0/python/platform_pb2_grpc.py(4 hunks)packages/dapi-grpc/clients/platform/v0/web/platform_pb.d.ts(1 hunks)packages/dapi-grpc/clients/platform/v0/web/platform_pb.js(3 hunks)packages/dapi-grpc/clients/platform/v0/web/platform_pb_service.d.ts(3 hunks)packages/dapi-grpc/clients/platform/v0/web/platform_pb_service.js(2 hunks)packages/dapi-grpc/protos/platform/v0/platform.proto(2 hunks)packages/rs-dpp/src/data_contract/associated_token/mod.rs(1 hunks)packages/rs-dpp/src/data_contract/associated_token/token_distribution_key.rs(1 hunks)packages/rs-dpp/src/data_contract/associated_token/token_perpetual_distribution/distribution_function/encode.rs(3 hunks)packages/rs-dpp/src/data_contract/associated_token/token_perpetual_distribution/distribution_function/mod.rs(2 hunks)packages/rs-dpp/src/data_contract/associated_token/token_perpetual_distribution/distribution_recipient.rs(1 hunks)packages/rs-dpp/src/data_contract/associated_token/token_perpetual_distribution/methods/mod.rs(1 hunks)packages/rs-dpp/src/data_contract/associated_token/token_perpetual_distribution/methods/v0/mod.rs(1 hunks)packages/rs-dpp/src/data_contract/associated_token/token_perpetual_distribution/mod.rs(1 hunks)packages/rs-dpp/src/data_contract/associated_token/token_perpetual_distribution/reward_distribution_type/mod.rs(1 hunks)packages/rs-dpp/src/data_contract/associated_token/token_perpetual_distribution/v0/methods.rs(1 hunks)packages/rs-dpp/src/data_contract/associated_token/token_perpetual_distribution/v0/mod.rs(1 hunks)packages/rs-dpp/src/data_contract/associated_token/token_pre_programmed_distribution/methods/mod.rs(1 hunks)packages/rs-dpp/src/data_contract/associated_token/token_pre_programmed_distribution/methods/v0/mod.rs(1 hunks)packages/rs-dpp/src/data_contract/associated_token/token_pre_programmed_distribution/mod.rs(1 hunks)packages/rs-dpp/src/state_transition/state_transitions/document/batch_transition/batched_transition/token_freeze_transition/v0/mod.rs(0 hunks)packages/rs-dpp/src/state_transition/state_transitions/document/batch_transition/batched_transition/token_unfreeze_transition/v0/mod.rs(0 hunks)packages/rs-dpp/src/state_transition/state_transitions/document/batch_transition/methods/mod.rs(0 hunks)packages/rs-dpp/src/state_transition/state_transitions/document/batch_transition/methods/v1/mod.rs(1 hunks)packages/rs-dpp/src/state_transition/state_transitions/document/batch_transition/v1/v0_methods.rs(0 hunks)packages/rs-dpp/src/tokens/token_event.rs(2 hunks)packages/rs-drive-abci/src/execution/platform_events/protocol_upgrade/perform_events_on_first_block_of_protocol_change/v0/mod.rs(4 hunks)packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/data_contract_create/mod.rs(3 hunks)packages/rs-drive-abci/src/query/service.rs(2 hunks)packages/rs-drive-abci/src/query/token_queries/mod.rs(1 hunks)packages/rs-drive-abci/src/query/token_queries/token_pre_programmed_distributions/mod.rs(1 hunks)packages/rs-drive-abci/src/query/token_queries/token_pre_programmed_distributions/v0/mod.rs(1 hunks)packages/rs-drive/src/drive/contract/insert/insert_contract/v1/mod.rs(1 hunks)packages/rs-drive/src/drive/initialization/v1/mod.rs(2 hunks)packages/rs-drive/src/drive/tokens/distribution/add_perpetual_distribution/mod.rs(1 hunks)packages/rs-drive/src/drive/tokens/distribution/add_perpetual_distribution/v0/mod.rs(1 hunks)packages/rs-drive/src/drive/tokens/distribution/add_pre_programmed_distribution/mod.rs(1 hunks)packages/rs-drive/src/drive/tokens/distribution/add_pre_programmed_distribution/v0/mod.rs(1 hunks)packages/rs-drive/src/drive/tokens/distribution/fetch/mod.rs(1 hunks)packages/rs-drive/src/drive/tokens/distribution/fetch/pre_programmed_distributions/mod.rs(1 hunks)packages/rs-drive/src/drive/tokens/distribution/mod.rs(1 hunks)packages/rs-drive/src/drive/tokens/distribution/prove/mod.rs(1 hunks)packages/rs-drive/src/drive/tokens/distribution/prove/pre_programmed_distributions/mod.rs(1 hunks)packages/rs-drive/src/drive/tokens/distribution/queries.rs(1 hunks)packages/rs-drive/src/drive/tokens/estimated_costs/for_root_token_ms_interval_distribution/mod.rs(1 hunks)packages/rs-drive/src/drive/tokens/estimated_costs/for_root_token_ms_interval_distribution/v0/mod.rs(1 hunks)packages/rs-drive/src/drive/tokens/estimated_costs/for_token_pre_programmed_distribution/mod.rs(1 hunks)packages/rs-drive/src/drive/tokens/estimated_costs/for_token_pre_programmed_distribution/v0/mod.rs(1 hunks)packages/rs-drive/src/drive/tokens/estimated_costs/mod.rs(1 hunks)packages/rs-drive/src/drive/tokens/mod.rs(1 hunks)packages/rs-drive/src/drive/tokens/paths.rs(3 hunks)packages/rs-drive/src/state_transition_action/batch/batched_transition/token_transition/token_transfer_transition_action/mod.rs(1 hunks)packages/rs-drive/src/state_transition_action/batch/batched_transition/token_transition/token_transfer_transition_action/v0/mod.rs(1 hunks)packages/rs-drive/src/util/grove_operations/batch_insert_empty_sum_tree/v0/mod.rs(1 hunks)packages/rs-drive/src/util/grove_operations/batch_insert_empty_tree_if_not_exists/v0/mod.rs(1 hunks)packages/rs-drive/src/verify/tokens/mod.rs(1 hunks)packages/rs-drive/src/verify/tokens/verify_token_pre_programmed_distributions/mod.rs(1 hunks)packages/rs-drive/src/verify/tokens/verify_token_pre_programmed_distributions/v0/mod.rs(1 hunks)packages/rs-platform-version/src/version/drive_abci_versions/drive_abci_query_versions/mod.rs(1 hunks)packages/rs-platform-version/src/version/drive_abci_versions/drive_abci_query_versions/v1.rs(1 hunks)packages/rs-platform-version/src/version/drive_versions/drive_identity_method_versions/mod.rs(1 hunks)packages/rs-platform-version/src/version/drive_versions/drive_identity_method_versions/v1.rs(1 hunks)packages/rs-platform-version/src/version/drive_versions/drive_token_method_versions/mod.rs(3 hunks)packages/rs-platform-version/src/version/drive_versions/drive_token_method_versions/v1.rs(4 hunks)packages/rs-platform-version/src/version/drive_versions/drive_verify_method_versions/mod.rs(1 hunks)packages/rs-platform-version/src/version/drive_versions/drive_verify_method_versions/v1.rs(1 hunks)packages/rs-platform-version/src/version/mocks/v2_test.rs(1 hunks)
💤 Files with no reviewable changes (4)
- packages/rs-dpp/src/state_transition/state_transitions/document/batch_transition/batched_transition/token_freeze_transition/v0/mod.rs
- packages/rs-dpp/src/state_transition/state_transitions/document/batch_transition/methods/mod.rs
- packages/rs-dpp/src/state_transition/state_transitions/document/batch_transition/batched_transition/token_unfreeze_transition/v0/mod.rs
- packages/rs-dpp/src/state_transition/state_transitions/document/batch_transition/v1/v0_methods.rs
✅ Files skipped from review due to trivial changes (4)
- packages/rs-drive-abci/src/query/token_queries/mod.rs
- packages/rs-drive/src/verify/tokens/mod.rs
- packages/rs-dpp/src/data_contract/associated_token/mod.rs
- packages/rs-dpp/src/data_contract/associated_token/token_pre_programmed_distribution/mod.rs
🧰 Additional context used
📓 Learnings (3)
packages/rs-drive/src/drive/tokens/estimated_costs/for_token_pre_programmed_distribution/mod.rs (1)
Learnt from: QuantumExplorer
PR: dashpay/platform#2431
File: packages/rs-drive/src/drive/tokens/system/fetch_token_total_aggregated_identity_balances/v0/mod.rs:42-50
Timestamp: 2025-01-19T07:36:09.383Z
Learning: The `add_estimation_costs_for_token_balances` method in Drive is a properly implemented estimation logic method used for token balance operations, and the warning about incomplete estimation logic was incorrect.
packages/rs-drive-abci/src/execution/platform_events/protocol_upgrade/perform_events_on_first_block_of_protocol_change/v0/mod.rs (1)
Learnt from: shumkov
PR: dashpay/platform#2422
File: packages/rs-drive-abci/src/execution/platform_events/protocol_upgrade/perform_events_on_first_block_of_protocol_change/v0/mod.rs:152-163
Timestamp: 2025-01-15T08:09:59.365Z
Learning: In the `transition_to_version_8` function, errors from `grove_get_path_query` when retrieving active contested resource votes are intentionally logged and ignored (returning `Ok(())`) to allow the protocol upgrade to proceed despite query failures.
packages/rs-drive/src/drive/tokens/estimated_costs/for_token_pre_programmed_distribution/v0/mod.rs (1)
Learnt from: QuantumExplorer
PR: dashpay/platform#2431
File: packages/rs-drive/src/drive/tokens/system/fetch_token_total_aggregated_identity_balances/v0/mod.rs:42-50
Timestamp: 2025-01-19T07:36:09.383Z
Learning: The `add_estimation_costs_for_token_balances` method in Drive is a properly implemented estimation logic method used for token balance operations, and the warning about incomplete estimation logic was incorrect.
🔇 Additional comments (89)
packages/rs-dpp/src/data_contract/associated_token/token_perpetual_distribution/mod.rs (2)
2-2: LGTM! Import changes align with new functionality.The addition of
ProtocolErrorand platform serialization traits supports the enhanced token distribution functionality.Also applies to: 5-5
15-30: Verify the unversioned serialization decision.The enum is marked as unversioned for platform serialization. Please confirm this is intentional, as versioning is typically important for maintaining backward compatibility in distributed systems.
✅ Verification successful
Unversioned serialization is consistently applied across the codebase.
The grep output confirms that the use of
#[platform_serialize(unversioned)]is not unique toTokenPerpetualDistributionbut is part of a design choice used in many similar enums and structs (e.g., in identity, asset lock, extended epochs, errors, etc.). This consistency indicates that the decision to mark these types as unversioned is intentional and aligned with the overall serialization strategy.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if other similar enums in the codebase are also unversioned # to ensure consistency in serialization approach. echo "Searching for other platform_serialize attributes..." rg --type rust "platform_serialize.*unversioned" -B 2 -A 2 echo "Searching for TokenPerpetualDistribution usage..." rg --type rust "TokenPerpetualDistribution" -B 2 -A 2Length of output: 65884
packages/rs-drive/src/state_transition_action/batch/batched_transition/token_transition/token_transfer_transition_action/mod.rs (1)
5-5: LGTM!The removal of unused imports
RecipientKeyIndexandSenderKeyIndexis appropriate as they are not referenced in the code.packages/rs-dpp/src/state_transition/state_transitions/document/batch_transition/methods/v1/mod.rs (1)
12-12: LGTM!The removal of unused imports is appropriate as they are not referenced in the trait definitions.
packages/rs-dpp/src/tokens/token_event.rs (1)
8-9: Verify the encapsulation of key index types.The removal of
RecipientKeyIndexandSenderKeyIndeximports suggests that these types are now encapsulated withinSharedEncryptedNote. Please ensure that:
- The
SharedEncryptedNotetype properly encapsulates these key index types- The key index values extracted from
token_event_shared_encrypted_noteare compatible with the property types in theBTreeMapRun the following script to verify the type definitions:
Also applies to: 24-24
✅ Verification successful
Encapsulation of Key Index Types Verified
- The generated platform protobuf definitions for
SharedEncryptedNote(as seen in the Objective-C and TypeScript client files) include bothsenderKeyIndexandrecipientKeyIndexfields.- These fields are typed as unsigned integers (uint32), which is consistent with their use in Rust (e.g., when mapping to a
BTreeMap).- There is no evidence that the removal of the standalone
RecipientKeyIndexandSenderKeyIndextypes has adversely affected type compatibility.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the encapsulation of key index types in SharedEncryptedNote # Test: Search for SharedEncryptedNote definition ast-grep --pattern 'struct SharedEncryptedNote { $$$ }' # Test: Search for key index type usage in SharedEncryptedNote rg -A 5 'SharedEncryptedNote'Length of output: 74392
packages/dapi-grpc/clients/platform/v0/nodejs/platform_protoc.js (3)
380-390: New exports for token distribution messages
These newly introduced export symbols appear consistent with the existing structure and naming conventions. No issues found.
54816-56514: Auto-generated response classes appear consistent
These definitions and nested message fields closely follow typical JspB patterns. Everything looks in order, with no obvious concerns regarding correctness or security.
5126-5294: Validate the 'limit' field usage
Although this code is auto-generated, thelimitparameter could lead to unexpectedly large or zero-length queries. Please verify that consuming parts of the codebase handle out-of-range or zero values appropriately.Below is a script to locate references of
GetTokenPreProgrammedDistributionsRequestV0in the repository, which may reveal any limit-handling logic:packages/dapi-grpc/clients/platform/v0/web/platform_pb.js (3)
380-390: No issues found.
This block simply exports the newly defined proto symbols for the token distribution messages.
5126-5294: All good.
These additions are auto-generated classes and references forGetTokenPreProgrammedDistributionsRequest. If changes are necessary, please update the corresponding.protofile rather than modifying compiled output.
54816-56514: Minor grammatical note and general approval.
The docstrings contain a small grammatical slip ("When of these fields' value is set"), but as this code is auto-generated, any text fixes should be applied in the original.protodefinitions. Functionality and structure appear correct.packages/rs-dpp/src/data_contract/associated_token/token_perpetual_distribution/v0/mod.rs (3)
1-1: No issues with introducing themod methods;statement.
14-15: Field introduction looks good.
Definingdistribution_recipientalongsidedistribution_typeis a clear and maintainable approach for representing token distribution details.
22-24: Formatted string usage is appropriate.
The customDisplayimplementation is simple and sufficient. If advanced formatting is needed in the future, consider structuring the output more formally (e.g., JSON-like).packages/rs-drive-abci/src/query/token_queries/token_pre_programmed_distributions/mod.rs (4)
1-13: Imports and module structure look well-organized.
The new dependencies and module references appear consistent with the rest of the codebase. No issues found.
22-29: Good handling of the optionalversionfield.
Using theelseclause to return a decoding error when the version is missing is a clear and explicit approach to error handling.
39-49: Version boundary checks are well implemented.
Ensuring the feature version is within the supported range avoids potential runtime errors. This is a good pattern for future expansions.
51-64: Suggest adding dedicated test coverage.
While the match on request versions looks correct, consider adding unit tests or integration tests to verify behavior for valid and invalid inputs, including boundary cases for the version field.packages/rs-drive/src/verify/tokens/verify_token_pre_programmed_distributions/mod.rs (3)
1-2: New module file recognized.
No issues with declaringmod v0;or adding high-level imports.
17-46: Comprehensive documentation.
The doc comments concisely explain the method’s purpose, parameters, and errors. This clarity helps maintainers and users alike.
49-70: Version-based dispatch is consistent with the rest of the codebase.
Providing explicit error handling for unsupported versions is good for maintainability. Future expansions can extend the match with additional version branches.packages/rs-drive/src/drive/tokens/distribution/add_pre_programmed_distribution/mod.rs (1)
40-40: Double-check overflow checks.
The comment mentions preventing overflow for token amounts by ensuring they don’t exceedi64::MAX. Verify that the actual code inv0logic consistently enforces this constraint at insertion time.packages/rs-drive/src/drive/tokens/distribution/fetch/pre_programmed_distributions/mod.rs (1)
65-73: Validate 8-byte assumption for distribution timestamps.
Popping the last path component and expecting 8 bytes in big-endian is correct given the documentation, but consider explicit test cases that confirm the system behaves as expected when the size is incorrect (e.g., corruption scenarios).packages/rs-drive/src/verify/tokens/verify_token_pre_programmed_distributions/v0/mod.rs (1)
16-116: LGTM overallNo issues stand out in this method. The approach to grouping the proved key-value pairs by timestamp and verifying negative token amounts is comprehensive.
packages/rs-dpp/src/data_contract/associated_token/token_perpetual_distribution/distribution_function/encode.rs (1)
21-34: Confirm variant ordering consistency.
When new variants are introduced (e.g.,FixedAmount,StepDecreasingAmount), ensure that all related decoding/borrow-decoding logic matches the updated variant indices to avoid misalignment during encoding/decoding.packages/rs-drive/src/drive/tokens/paths.rs (1)
457-476: Validate boundary cases for interval calculations.The logic for calculating the next interval (by doing
block_info.height - block_info.height % interval + interval, etc.) appears correct. However, consider edge cases around very large block heights or epoch indexes to ensure no off-by-one or overflow conditions arise.packages/rs-drive-abci/src/execution/platform_events/protocol_upgrade/perform_events_on_first_block_of_protocol_change/v0/mod.rs (3)
27-31: Imports look consistent.The references to new token paths and distribution constants appear correct and fulfill the usage in
transition_to_version_9.
103-195: Check for potential key ID overflow and add logging for empty key sets.
- If
last_key_idis already near the maximum allowable key ID, adding 1 could overflow. Consider introducing safeguards or bounds checks.- You silently skip masternodes with no existing keys. A small log message might be helpful to track such cases for debugging.
396-399: Confirm storage requirements for distribution data.You are inserting an
Element::empty_tree()for[TOKEN_DISTRIBUTIONS_KEY]. If you expect aggregated calculations on tokens under this subtree, verify that a big sum tree (e.g.,Element::empty_big_sum_tree()) is not more appropriate.packages/rs-drive/src/drive/tokens/estimated_costs/mod.rs (1)
12-17: LGTM! Well-organized module structure.The new modules for token distribution cost estimation are well-organized and follow consistent naming and documentation patterns.
packages/rs-dpp/src/data_contract/associated_token/token_distribution_key.rs (1)
8-31: LGTM! Well-structured key composition.The enum and struct definitions are well-designed with appropriate trait derivations. However, let's verify the usage of unversioned serialization.
Let's verify if there are any other structs in the codebase that use versioned serialization for similar token-related keys:
✅ Verification successful
Let's gather additional details before finalizing the review. The previous command didn’t produce any output, so I'll run an enhanced search to check for instances of
platform_serializewith bothversionedandunversionedparameters in the repository.
Verification: Consistent Use of Unversioned Serialization in Token-Related Keys
After reviewing the repository:
- The
TokenDistributionKeystruct inpackages/rs-dpp/src/data_contract/associated_token/token_distribution_key.rsemploys#[platform_serialize(unversioned)].- Similar token-related structs across the codebase also consistently use
#[platform_serialize(unversioned)], with no instances of#[platform_serialize(versioned)]in comparable contexts.The serialization strategy is consistent and as expected.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for token-related structs with platform_serialize attribute ast-grep --pattern 'struct $name { $$$ } #[platform_serialize($_)]'Length of output: 72
Script:
#!/bin/bash echo "Searching for instances using #[platform_serialize(versioned)]..." rg '#\[platform_serialize\(versioned\)' -n . echo "Searching for instances using #[platform_serialize(unversioned)]..." rg '#\[platform_serialize\(unversioned\)' -n .Length of output: 31943
packages/rs-dpp/src/data_contract/associated_token/token_perpetual_distribution/distribution_recipient.rs (1)
6-15: Verify the necessity of PartialOrd trait.The enum derives
PartialOrd, but it's not clear why ordering would be needed for distribution recipient types. Consider removing it unless there's a specific use case.Let's search for usage of ordering operations on TokenDistributionRecipient:
✅ Verification successful
PartialOrd appears to be unnecessary for TokenDistributionRecipient.
Our search did not reveal any ordering comparisons or operations (such as<,>, orpartial_cmp) involving this enum. If there's no intended sorting or comparison logic elsewhere that depends on it, consider removing the PartialOrd derivation.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for comparisons of TokenDistributionRecipient rg "TokenDistributionRecipient.*(<|>|<=|>=|cmp|partial_cmp)" -A 3Length of output: 1523
packages/rs-dpp/src/data_contract/associated_token/token_perpetual_distribution/methods/v0/mod.rs (1)
23-24: LGTM! Clear documentation of interval type flexibility.The comment effectively explains the use of u64 for different interval types (time, block, epoch).
packages/rs-dpp/src/data_contract/associated_token/token_pre_programmed_distribution/methods/mod.rs (2)
26-40: Add validation in add_distribution method.Consider adding validation for:
- Non-negative token amounts
- Future timestamps
- Duplicate recipient checks within the same timestamp
This will help prevent invalid distributions from being added.
10-15: LGTM! Efficient implementation of distributions getter.The use of BTreeMap provides ordered access to distributions, and pattern matching ensures proper version handling.
packages/rs-drive/src/drive/tokens/estimated_costs/for_root_token_ms_interval_distribution/mod.rs (1)
20-38: LGTM! Well-structured version handling.The implementation correctly:
- Matches against the version
- Delegates to version-specific implementation
- Returns appropriate error for unknown versions
packages/rs-platform-version/src/version/drive_versions/drive_token_method_versions/v1.rs (1)
46-49: LGTM! Clear and consistent version configuration.The version configuration:
- Properly initializes new distribution methods
- Maintains consistency with existing version numbering
- Uses descriptive field names
packages/rs-dpp/src/data_contract/associated_token/token_perpetual_distribution/methods/mod.rs (1)
38-44: LGTM! Clean method implementation.The
next_intervalimplementation:
- Correctly delegates to version-specific logic
- Maintains consistent pattern matching style
packages/rs-drive/src/drive/tokens/distribution/add_perpetual_distribution/mod.rs (1)
29-52: LGTM! Well-structured version handling and error management.The implementation:
- Properly handles version dispatch
- Provides clear error information
- Follows consistent implementation pattern
packages/rs-drive/src/util/grove_operations/batch_insert_empty_sum_tree/v0/mod.rs (1)
11-11: LGTM! Good encapsulation practice.The visibility change from
pub(crate)topub(super)appropriately restricts access to only the parent module, improving encapsulation.packages/rs-dpp/src/data_contract/associated_token/token_perpetual_distribution/reward_distribution_type/mod.rs (1)
18-44: LGTM! Clear and informative Display implementation.The Display implementation provides human-readable output that clearly shows all relevant distribution parameters.
packages/rs-drive/src/drive/tokens/distribution/prove/pre_programmed_distributions/mod.rs (1)
11-31: LGTM! Comprehensive documentation.The documentation clearly explains the method's purpose, parameters, and return value. Good explanation of the nested mapping structure.
packages/rs-platform-version/src/version/drive_versions/drive_token_method_versions/mod.rs (2)
11-18: LGTM! Well-structured versioning additions.The new
DriveTokenDistributionMethodVersionsstruct and its integration into existing structures follows the established versioning pattern consistently.
32-32: LGTM! Consistent feature versioning.The addition of
pre_programmed_distributionsto both fetch and prove versions maintains symmetry in the versioning system.Also applies to: 45-45
packages/rs-drive/src/drive/tokens/estimated_costs/for_root_token_ms_interval_distribution/v0/mod.rs (1)
18-23: LGTM! Well-structured method signature.The method is well-defined with appropriate generic constraints and clear parameter types. The use of
ExactSizeIteratoris a good choice as it allows pre-allocation optimizations.packages/rs-platform-version/src/version/drive_versions/drive_verify_method_versions/v1.rs (1)
48-48: LGTM! Consistent version initialization.The new field follows the established pattern of initializing feature versions to 0.
packages/rs-drive/src/drive/tokens/distribution/queries.rs (1)
7-24: LGTM! Comprehensive documentation.The struct documentation clearly explains the purpose and usage of each field, with good examples of how the
start_at_recipientfield affects query behavior.packages/rs-platform-version/src/version/drive_abci_versions/drive_abci_query_versions/mod.rs (1)
34-34: LGTM! Consistent feature versioning.The new field follows the established pattern of using
FeatureVersionBoundsfor versioning token-related queries.packages/rs-drive/src/drive/tokens/estimated_costs/for_token_pre_programmed_distribution/v0/mod.rs (1)
27-33: LGTM! Well-structured generic implementation.The function signature is well-designed with generic type parameter
Ibounded by appropriate traits (IntoIteratorandExactSizeIterator), allowing for flexible usage with different timestamp collections.packages/rs-platform-version/src/version/drive_versions/drive_verify_method_versions/mod.rs (1)
66-66: LGTM! Consistent field addition.The new field follows the established pattern and naming convention of the struct, using the appropriate type for version tracking.
packages/rs-drive/src/drive/tokens/distribution/fetch/mod.rs (1)
35-51: LGTM! Clean and well-structured implementation.The public function provides a clean interface while properly delegating to the internal implementation.
packages/rs-drive/src/drive/initialization/v1/mod.rs (1)
65-89: LGTM! Good optimization of path reuse.The code efficiently reuses the
tokens_root_pathvariable instead of repeatedly callingtokens_root_path_vec().packages/rs-drive/src/state_transition_action/batch/batched_transition/token_transition/token_transfer_transition_action/v0/mod.rs (1)
6-8: Verify the impact of removingSenderKeyIndeximport.The removal of
SenderKeyIndexfrom the imports suggests a change in how token transfers are handled. Please ensure that this removal is intentional and doesn't affect any functionality.Run the following script to verify the usage of
SenderKeyIndex:✅ Verification successful
Removal of
SenderKeyIndexin the rs-drive module is safe.
The search shows thatSenderKeyIndexremains used in the rs-dpp modules handling token transfers, while it is not referenced in the rs-drive file. The removal appears intentional and should not affect the functionality of the rs-drive module.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify if SenderKeyIndex is used in the codebase. # Test: Search for any remaining usage of SenderKeyIndex in the file. ast-grep --pattern $'SenderKeyIndex'Length of output: 775
packages/rs-platform-version/src/version/drive_abci_versions/drive_abci_query_versions/v1.rs (1)
107-111: LGTM! The new field is properly initialized.The addition of
token_pre_programmed_distributionswith appropriate version bounds aligns with the broader token distribution enhancements.packages/rs-platform-version/src/version/drive_versions/drive_identity_method_versions/v1.rs (1)
134-135: LGTM! The new cost estimation fields are properly initialized.The addition of
for_token_pre_programmed_distributionandfor_root_token_ms_interval_distributionwith version 0 aligns with the broader token distribution enhancements.packages/rs-dpp/src/data_contract/associated_token/token_perpetual_distribution/distribution_function/mod.rs (3)
10-21: LGTM! TheFixedAmountvariant is well-designed and documented.The implementation provides a simple and effective way to emit a fixed amount of tokens per period. The documentation clearly explains the formula, use case, and provides a practical example.
23-39: LGTM! TheStepDecreasingAmountvariant is well-designed for Bitcoin/Dash-like emission models.The implementation effectively models decreasing token emissions with:
- Clear documentation of formula and use cases
- Practical examples referencing Bitcoin and Dash Core models
- Safe floating-point arithmetic using
NotNan<f64>
179-194: LGTM! The display formatting is clear and informative.The
fmt::Displayimplementation for both new variants:
- Provides clear, human-readable output
- Includes all relevant parameters
- Uses appropriate precision for floating-point values
packages/rs-platform-version/src/version/drive_versions/drive_identity_method_versions/mod.rs (1)
73-74: LGTM! The new cost estimation fields align with token distribution feature.The addition of
for_token_pre_programmed_distributionandfor_root_token_ms_interval_distributionfields to theDriveIdentityCostEstimationMethodVersionsstruct is well-structured and follows the existing pattern.packages/rs-drive/src/util/grove_operations/batch_insert_empty_tree_if_not_exists/v0/mod.rs (1)
19-19: LGTM! Improved encapsulation with tighter visibility.The change from
pub(crate)topub(super)appropriately restricts the method's visibility to the parent module, following Rust's best practices for encapsulation.packages/rs-platform-version/src/version/mocks/v2_test.rs (1)
245-249: LGTM! Test version configuration aligns with token distribution feature.The addition of
token_pre_programmed_distributionsto the test platform version configuration is consistent with other feature version bounds and properly initialized.packages/rs-drive-abci/src/query/service.rs (1)
40-41: LGTM! The implementation follows established patterns.The new
get_token_pre_programmed_distributionsmethod is well-integrated into the service, following the same patterns as other query methods for consistency and proper error handling.Also applies to: 680-690
packages/dapi-grpc/clients/platform/v0/web/platform_pb_service.d.ts (1)
340-347: LGTM! Type definitions are well-structured.The new type definition and static property for token pre-programmed distributions follow TypeScript best practices and maintain consistency with existing patterns.
Also applies to: 433-433
packages/dapi-grpc/clients/platform/v0/objective-c/Platform.pbrpc.h (1)
90-91: LGTM! Objective-C implementation is robust and consistent.The new class declarations and method implementations properly handle memory management with nullable annotations and maintain consistency across both modern and legacy protocols.
Also applies to: 295-297, 636-641
packages/dapi-grpc/clients/platform/v0/web/platform_pb_service.js (1)
346-353: LGTM! JavaScript client implementation is well-structured.The new method implementation follows established patterns for unary RPC calls with proper error handling and cleanup. The code maintains consistency with other client methods.
Also applies to: 1554-1583
packages/dapi-grpc/clients/platform/v0/objective-c/Platform.pbrpc.m (1)
878-896: LGTM! Well-structured implementation of the new gRPC method.The implementation follows the established pattern and includes all necessary components for the gRPC client method.
packages/dapi-grpc/clients/platform/v0/python/platform_pb2_grpc.py (2)
202-206: LGTM! Well-structured stub implementation.The gRPC stub implementation follows the established pattern and correctly configures the unary-unary channel.
1357-1373: LGTM! Well-structured experimental API implementation.The static method implementation follows the established pattern and includes all necessary parameters for the gRPC call.
packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/data_contract_create/mod.rs (1)
148-149: LGTM! New imports support token distribution functionality.The added imports for
TokenAmount,TimestampMillis,Identifier, andBTreeMapare appropriate for implementing token distribution features.Also applies to: 163-164, 171-171
packages/dapi-grpc/clients/platform/v0/java/org/dash/platform/dapi/v0/PlatformGrpc.java (8)
1165-1194: LGTM! Method descriptor declaration follows gRPC best practices.The method descriptor declaration for
getTokenPreProgrammedDistributionsis properly implemented with:
- Correct volatile field declaration
- Thread-safe initialization using double-checked locking
- Proper request/response type definitions
- Appropriate method type (UNARY)
1673-1678: LGTM! Service implementation is correctly defined.The abstract method implementation in
PlatformImplBasefollows the standard gRPC pattern for unary methods.
2347-2353: LGTM! Async stub implementation is properly defined.The async stub implementation correctly uses
asyncUnaryCallfor non-blocking operations.
2684-2689: LGTM! Blocking stub implementation is properly defined.The blocking stub implementation correctly uses
blockingUnaryCallfor synchronous operations.
3052-3058: LGTM! Future stub implementation is properly defined.The future stub implementation correctly uses
futureUnaryCallfor returningListenableFuture.
3138-3138: LGTM! Method ID and subsequent constants are properly incremented.The method ID constant is correctly defined and subsequent method IDs are properly incremented.
Also applies to: 3139-3143
3310-3313: LGTM! Method handler case is properly implemented.The method handler case in the switch statement correctly delegates to the service implementation.
3432-3432: LGTM! Service descriptor builder is properly updated.The service descriptor builder correctly includes the new method.
packages/dapi-grpc/clients/platform/v0/objective-c/Platform.pbobjc.h (3)
1-2: Important: This is an auto-generated file.The warning at the top indicates this file should not be manually edited as it's generated by the protocol buffer compiler.
197-202: New token distribution classes added.The following classes have been added to support token pre-programmed distributions:
- Request/response message types
- Support for pagination via StartAtInfo
- Support for both timed and regular distributions
- Proof verification capabilities
5861-6031: Token distribution implementation looks good.The implementation includes:
- Proper request/response message structures
- Pagination support
- Proof verification
- Handling of both timed and regular distributions
packages/dapi-grpc/clients/platform/v0/web/platform_pb.d.ts (1)
7042-7283: LGTM! Well-structured message types for token pre-programmed distributions.The implementation provides a robust TypeScript interface for token distribution functionality with:
- Proper versioning support with V0 message types
- Well-structured request parameters including tokenId, pagination and filtering options
- Comprehensive response types for distribution entries with timestamps and amounts
- Strong type safety through TypeScript definitions
packages/dapi-grpc/clients/platform/v0/objective-c/Platform.pbobjc.m (7)
268-275: LGTM! Class declarations are complete and well-structured.The declarations follow proper protocol buffer naming conventions and include all necessary components for token distribution functionality.
15012-15068: LGTM! Thread-safe implementation with proper memory management.The implementation correctly handles:
- Thread-safe descriptor initialization
- Proper storage structure for fields
- Version one-of case management
15214-15270: LGTM! Consistent implementation pattern.The response implementation maintains consistency with the request implementation, including proper thread safety and version handling.
15271-15344: LGTM! Complete response structure with proper type safety.The implementation correctly handles:
- Proof verification data
- Response metadata
- Result one-of case management
15408-15463: LGTM! Proper array management with timestamp support.The implementation correctly handles:
- Distribution array management
- Timestamp field
- Memory management for repeated fields
15465-15509: LGTM! Proper implementation of repeated message fields.The implementation correctly handles the array of timed distributions with proper memory management and type safety.
15147-15212: Consider adding timestamp validation.The StartAtInfo implementation could benefit from validation to ensure
startTimeMsis within a reasonable range.packages/dapi-grpc/protos/platform/v0/platform.proto (1)
1438-1463: New Response Message Implementation
The newGetTokenPreProgrammedDistributionsResponsemessage is well structured:
- The nested messages (
TokenDistributionEntry,TokenTimedDistributionEntry, andTokenDistributions) clearly break down the response data.- The oneof
resultcorrectly provides either a full set of token distributions or a cryptographic proof, following the established pattern elsewhere in the file.- The inclusion of
ResponseMetadatafurther ensures that blockchain state context accompanies the response.
Overall, the design is consistent with existing patterns in the protocol definitions.
| pub(crate) fn add_estimation_costs_for_token_pre_programmed_distribution<'a, I>( | ||
| token_id: [u8; 32], | ||
| times: I, | ||
| estimated_costs_only_with_layer_info: &mut HashMap<KeyInfoPath, EstimatedLayerInformation>, | ||
| drive_version: &DriveVersion, | ||
| ) -> Result<(), Error> | ||
| where | ||
| I: IntoIterator<Item = &'a TimestampMillis> + ExactSizeIterator, | ||
| { |
There was a problem hiding this comment.
💡 Codebase verification
Insufficient Test Coverage for Token Pre-programmed Distribution Estimation Function
The search results only reveal production code references to add_estimation_costs_for_token_pre_programmed_distribution (and its v0 variant) with no clear dedicated unit or integration tests specifically targeting its behavior. This suggests that explicit test coverage for the estimation pathway is lacking. It is recommended to add tests to verify that this function computes estimation costs as expected, helping safeguard it against future inadvertent changes.
- Verify if any existing tests indirectly cover this behavior, and if not, add dedicated unit tests.
- Consider integration tests if the function’s context within the drive token distribution workflow warrants end-to-end validation.
🔗 Analysis chain
Consider verifying test coverage for the new estimation function.
While the logic looks sound, adding unit tests (and possibly integration tests) would help ensure that future changes don't break this estimation pathway.
You can run a search to confirm the presence of test files referencing this method:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for test references to the added method in the codebase.
rg -A 5 "add_estimation_costs_for_token_pre_programmed_distribution" --threads 1
Length of output: 3501
| /// - `previous_batch_operations`: If provided, contains previously executed batch operations | ||
| /// that should be taken into account. |
There was a problem hiding this comment.
💡 Codebase verification
Discrepancy Found in Documentation Parameter
It appears that the documentation for previous_batch_operations in
packages/rs-drive/src/drive/tokens/distribution/add_pre_programmed_distribution/mod.rs
does not match the method signature since the parameter is not actually included.
Please update the docstring to accurately reflect the parameters in use.
- Remove or modify the doc comment for
previous_batch_operationsif it is not needed. - Verify the corresponding function signature and any related implementation if
previous_batch_operationsshould be added instead.
🔗 Analysis chain
Confirm usage of previous_batch_operations.
The documentation mentions a parameter to handle previously executed batch operations, but it's not included in the method signature. Ensure that the docstring accurately matches the parameters and usage.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check references to `previous_batch_operations` to confirm usage or remove from docstring if unused.
rg -A 2 "previous_batch_operations"
Length of output: 46387
| let sum_item = element.as_sum_item_value()?; | ||
| if sum_item < 0 { | ||
| return Err(Error::Drive(DriveError::CorruptedDriveState( | ||
| "negative token amount in pre-programmed distribution".to_string(), | ||
| ))); | ||
| } | ||
| let token_amount: TokenAmount = sum_item as TokenAmount; |
There was a problem hiding this comment.
Handle potential overflow for large sums.
Even though negative amounts are disallowed, consider what happens if the sum item is larger than i64::MAX. GroveDB might pass a value that’s too big for a 64-bit signed integer, resulting in unexpected behavior.
Potential fix (conceptual example):
-if sum_item < 0 {
- return Err(Error::Drive(DriveError::CorruptedDriveState(
- "negative token amount in pre-programmed distribution".to_string(),
- )));
-}
+if sum_item < 0 || sum_item > i64::MAX as i128 {
+ return Err(Error::Drive(DriveError::CorruptedDriveState(
+ "invalid or overly large token amount in pre-programmed distribution".to_string(),
+ )));
+}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let sum_item = element.as_sum_item_value()?; | |
| if sum_item < 0 { | |
| return Err(Error::Drive(DriveError::CorruptedDriveState( | |
| "negative token amount in pre-programmed distribution".to_string(), | |
| ))); | |
| } | |
| let token_amount: TokenAmount = sum_item as TokenAmount; | |
| let sum_item = element.as_sum_item_value()?; | |
| if sum_item < 0 || sum_item > i64::MAX as i128 { | |
| return Err(Error::Drive(DriveError::CorruptedDriveState( | |
| "invalid or overly large token amount in pre-programmed distribution".to_string(), | |
| ))); | |
| } | |
| let token_amount: TokenAmount = sum_item as TokenAmount; |
| pub(super) fn add_pre_programmed_distributions_v0( | ||
| &self, | ||
| token_id: [u8; 32], | ||
| owner_id: [u8; 32], | ||
| distribution: &TokenPreProgrammedDistribution, | ||
| block_info: &BlockInfo, | ||
| estimated_costs_only_with_layer_info: &mut Option< | ||
| HashMap<KeyInfoPath, EstimatedLayerInformation>, | ||
| >, | ||
| batch_operations: &mut Vec<LowLevelDriveOperation>, | ||
| transaction: TransactionArg, | ||
| platform_version: &PlatformVersion, | ||
| ) -> Result<(), Error> { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor for clarity.
The method add_pre_programmed_distributions_v0 is quite large and handles multiple steps (cost estimation, tree insertion, references, etc.). Splitting these into focused helper functions could improve readability and maintainability.
| RewardDistributionType::BlockBasedDistribution(interval, _, _) => { | ||
| block_info.height - block_info.height % interval + interval | ||
| } |
There was a problem hiding this comment.
💡 Codebase verification
Overflow risk in BlockBasedDistribution – Add checked arithmetic
The block-based branch in the next_interval method currently computes the next interval using:
block_info.height - block_info.height % interval + intervalWithout overflow protection, if block_info.height is near the maximum value for its type (likely u64), the addition could overflow. Even though our current test configuration doesn’t use extremely large interval values, it’s a good safety measure to use checked arithmetic. Consider refactoring as suggested:
block_info.height
.checked_sub(block_info.height % interval)
.and_then(|h| h.checked_add(interval))
.expect("Block interval calculation overflow")- Location:
packages/rs-dpp/src/data_contract/associated_token/token_perpetual_distribution/v0/methods.rsin theRewardDistributionType::BlockBasedDistributionbranch ofnext_interval.
🔗 Analysis chain
Add overflow protection for block-based interval calculation.
The current implementation might overflow for very large intervals. Consider adding checks:
Consider this safer implementation:
- block_info.height - block_info.height % interval + interval
+ block_info.height.checked_sub(block_info.height % interval)
+ .and_then(|h| h.checked_add(interval))
+ .expect("Block interval calculation overflow")I'll gather additional context from the repository to better understand how the block-based interval calculation is used and whether overflow checks are necessary. In particular, I'll inspect the definition of the RewardDistributionType enum, the implementation of the next_interval method, and the type definition for block_info.height.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for large interval values in tests or configurations
rg -A 5 "BlockBasedDistribution\(" | grep -E "[0-9]{10,}"
Length of output: 59
Script:
#!/bin/bash
echo "Searching for RewardDistributionType enum definition..."
rg -A 20 "enum RewardDistributionType" --color=never
echo "---------------------------------------------------------"
echo "Searching for next_interval method definition..."
rg -A 20 "fn next_interval" --color=never
echo "---------------------------------------------------------"
echo "Searching for block_info.height occurrences..."
rg -A 5 "block_info.height" --color=never packages/rs-dpp/src/data_contract/associated_token/token_perpetual_distribution/v0/methods.rs
Length of output: 8258
| start_at_recipient, | ||
| }) => { | ||
| let mut query = Query::new_single_query_item(QueryItem::RangeFrom( | ||
| start_at_time.to_be_bytes().to_vec().., |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use safe vector operations instead of unsafe slicing.
The code uses unsafe vector slicing with .. operator in multiple places:
start_at_time.to_be_bytes().to_vec()..
start_at_recipient.to_vec()..Consider using into_iter() or other safe alternatives to avoid potential runtime panics.
Also applies to: 63-63, 67-67
| #pragma mark - GetTokenPreProgrammedDistributionsResponse_GetTokenPreProgrammedDistributionsResponseV0_TokenDistributionEntry | ||
|
|
||
| @implementation GetTokenPreProgrammedDistributionsResponse_GetTokenPreProgrammedDistributionsResponseV0_TokenDistributionEntry | ||
|
|
||
| @dynamic recipientId; | ||
| @dynamic amount; | ||
|
|
||
| typedef struct GetTokenPreProgrammedDistributionsResponse_GetTokenPreProgrammedDistributionsResponseV0_TokenDistributionEntry__storage_ { | ||
| uint32_t _has_storage_[1]; | ||
| NSData *recipientId; | ||
| uint64_t amount; | ||
| } GetTokenPreProgrammedDistributionsResponse_GetTokenPreProgrammedDistributionsResponseV0_TokenDistributionEntry__storage_; | ||
|
|
||
| // This method is threadsafe because it is initially called | ||
| // in +initialize for each subclass. | ||
| + (GPBDescriptor *)descriptor { | ||
| static GPBDescriptor *descriptor = nil; | ||
| if (!descriptor) { | ||
| static GPBMessageFieldDescription fields[] = { | ||
| { | ||
| .name = "recipientId", | ||
| .dataTypeSpecific.clazz = Nil, | ||
| .number = GetTokenPreProgrammedDistributionsResponse_GetTokenPreProgrammedDistributionsResponseV0_TokenDistributionEntry_FieldNumber_RecipientId, | ||
| .hasIndex = 0, | ||
| .offset = (uint32_t)offsetof(GetTokenPreProgrammedDistributionsResponse_GetTokenPreProgrammedDistributionsResponseV0_TokenDistributionEntry__storage_, recipientId), | ||
| .flags = (GPBFieldFlags)(GPBFieldOptional | GPBFieldClearHasIvarOnZero), | ||
| .dataType = GPBDataTypeBytes, | ||
| }, | ||
| { | ||
| .name = "amount", | ||
| .dataTypeSpecific.clazz = Nil, | ||
| .number = GetTokenPreProgrammedDistributionsResponse_GetTokenPreProgrammedDistributionsResponseV0_TokenDistributionEntry_FieldNumber_Amount, | ||
| .hasIndex = 1, | ||
| .offset = (uint32_t)offsetof(GetTokenPreProgrammedDistributionsResponse_GetTokenPreProgrammedDistributionsResponseV0_TokenDistributionEntry__storage_, amount), | ||
| .flags = (GPBFieldFlags)(GPBFieldOptional | GPBFieldClearHasIvarOnZero), | ||
| .dataType = GPBDataTypeUInt64, | ||
| }, | ||
| }; | ||
| GPBDescriptor *localDescriptor = | ||
| [GPBDescriptor allocDescriptorForClass:[GetTokenPreProgrammedDistributionsResponse_GetTokenPreProgrammedDistributionsResponseV0_TokenDistributionEntry class] | ||
| rootClass:[PlatformRoot class] | ||
| file:PlatformRoot_FileDescriptor() | ||
| fields:fields | ||
| fieldCount:(uint32_t)(sizeof(fields) / sizeof(GPBMessageFieldDescription)) | ||
| storageSize:sizeof(GetTokenPreProgrammedDistributionsResponse_GetTokenPreProgrammedDistributionsResponseV0_TokenDistributionEntry__storage_) | ||
| flags:(GPBDescriptorInitializationFlags)(GPBDescriptorInitializationFlag_UsesClassRefs | GPBDescriptorInitializationFlag_Proto3OptionalKnown)]; | ||
| [localDescriptor setupContainingMessageClass:GPBObjCClass(GetTokenPreProgrammedDistributionsResponse_GetTokenPreProgrammedDistributionsResponseV0)]; | ||
| #if defined(DEBUG) && DEBUG | ||
| NSAssert(descriptor == nil, @"Startup recursed!"); | ||
| #endif // DEBUG | ||
| descriptor = localDescriptor; | ||
| } | ||
| return descriptor; | ||
| } | ||
|
|
||
| @end |
There was a problem hiding this comment.
💡 Codebase verification
Amount validation is not currently implemented.
- The Objective-C implementation for
TokenDistributionEntrydoes not include any logic to validate theamountfield. - A search for validation-related keywords (e.g.,
validateAmount) in the repository yielded no relevant results.
🔗 Analysis chain
Consider adding amount validation.
The TokenDistributionEntry implementation could benefit from validation to ensure the amount is within acceptable ranges.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for amount validation in the codebase
rg -A 5 "amount.*valid|validateAmount"
Length of output: 871
Issue being fixed or feature implemented
What was done?
How Has This Been Tested?
Breaking Changes
Checklist:
For repository code-owners and collaborators only
Summary by CodeRabbit
These improvements expand the API’s capability to manage and verify token distributions while offering a more reliable and efficient integration experience.