test(platform): additional token distribution tests and fixes #2550
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
## Walkthrough
This set of changes refactors and extends the token distribution logic for perpetual distributions. The `DistributionFunction` enum variants, especially `StepDecreasingAmount`, are updated with more descriptive field names and new parameters. Encoding, decoding, evaluation, and validation logic are revised to use these new names and parameters, with enhanced boundary checks, overflow handling, and parameter validation. Constants for parameter bounds are added, and documentation is improved throughout. Logging initialization is made idempotent, and related tests and display formatting are updated to match the new structures and logic.
## Changes
| File(s) | Change Summary |
|---------|---------------|
| `.../distribution_function/encode.rs`, `.../distribution_function/mod.rs` | Refactored `DistributionFunction` variants with new/renamed fields, updated encoding/decoding logic, added constants for parameter bounds, and improved documentation and display formatting. |
| `.../distribution_function/evaluate.rs` | Revised evaluation logic for all distribution function variants, focusing on integer arithmetic, overflow handling, clamping, and parameter renaming. Updated tests for new behavior and parameter names. |
| `.../distribution_function/validation.rs` | Enhanced validation logic with new parameter checks, error handling, and test coverage. Method signature updated to accept `PlatformVersion`. |
| `.../reward_distribution_type/mod.rs` | Changed max cycle logic to use a constant for fixed amount distributions, updated parameter names, and unified calculation path. |
| `.../invalid_token_distribution_function_incoherence_error.rs` | Minor change to error message formatting by removing a trailing period. |
| `.../src/logging/logger.rs` | Added static `OnceLock` to prevent multiple logging initializations, added `as_subscriber` method, and made `try_install` idempotent. |
| `.../src/logging/mod.rs` | Updated logging test to use explicit dispatcher setup instead of global install. |
| `.../batch/tests/token/distribution/perpetual/time_based.rs` | Updated tests to use new constant name `MAX_LINEAR_SLOPE_A_PARAM` in linear distribution function configuration. |
| `.../Cargo.toml` | Added `serial_test` as a development dependency. |
| `.../data_contract_create/basic_structure/v0/mod.rs` | Added validation of perpetual token distributions during data contract creation. |
| `.../data_contract_create/mod.rs` | Added new tests verifying valid and invalid perpetual distribution configurations during data contract creation. |
| `.../data_contract_update/basic_structure/v0/mod.rs` | Added validation of perpetual token distributions during data contract update. |
## Sequence Diagram(s)
```mermaid
sequenceDiagram
participant User
participant DistributionFunction
participant Validator
participant Evaluator
User->>DistributionFunction: Define variant with parameters
DistributionFunction->>Validator: validate(start_moment, platform_version)
Validator-->>DistributionFunction: Validation result
User->>DistributionFunction: encode()/decode()
User->>Evaluator: evaluate(x)
Evaluator-->>User: Distribution amount (with clamping, overflow checks)Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Learnt from: QuantumExplorer |
QuantumExplorer
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (4)
packages/rs-dpp/src/data_contract/associated_token/token_perpetual_distribution/distribution_function/evaluate.rs (1)
401-407: Copy‑paste typo in error messageInside the
Logarithmicbranch we return
"InvertedLogarithmic: evaluation overflow".- "InvertedLogarithmic: evaluation overflow", + "Logarithmic: evaluation overflow",packages/rs-dpp/src/data_contract/associated_token/token_perpetual_distribution/distribution_function/mod.rs (1)
12-19: Comment maths does not match the constant
MAX_DISTRIBUTION_CYCLES_PARAMis set to32_767(2^15‑1) but the comment
says2^(63 - 48) - 1. That evaluates to2^15‑1, yet the indirection
makes the reader stop and do the math. Consider stating it directly:-/// This is applied to linear distribution. -/// For all other distributions we use a versioned max cycles contained in the platform version. -/// That other version is much lower because the calculations for other distributions are more -/// complex. +/// Applies to linear distributions only. Other distribution types use a +/// version‑specific, lower limit because their calculations are more complex.packages/rs-dpp/src/data_contract/associated_token/token_perpetual_distribution/distribution_function/encode.rs (1)
20-40: Keepencode/decodeorder in a single source of truth
Encode,Decode, andBorrowDecodenow duplicate the exact same field
ordering logic for every variant. Any future change risks introducing a
subtle mismatch.Consider factoring the common field‑ordering into a helper macro or
#[derive(Encode, Decode)]on a lightweight struct for each variant. This
removes the manual duplication, reduces review surface, and prevents
serialization bugs.packages/rs-dpp/src/data_contract/associated_token/token_perpetual_distribution/distribution_function/validation.rs (1)
86-92: Misleading field name in validation errorThe error message still refers to the generic placeholder
n, but the field being validated was renamed todistribution_start_amount. This will confuse developers and API users when the error is surfaced.-InvalidTokenDistributionFunctionInvalidParameterError::new( - "n".to_string(), +InvalidTokenDistributionFunctionInvalidParameterError::new( + "distribution_start_amount".to_string(),
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (10)
packages/rs-dpp/src/data_contract/associated_token/token_perpetual_distribution/distribution_function/encode.rs(11 hunks)packages/rs-dpp/src/data_contract/associated_token/token_perpetual_distribution/distribution_function/evaluate.rs(27 hunks)packages/rs-dpp/src/data_contract/associated_token/token_perpetual_distribution/distribution_function/mod.rs(14 hunks)packages/rs-dpp/src/data_contract/associated_token/token_perpetual_distribution/distribution_function/validation.rs(88 hunks)packages/rs-dpp/src/data_contract/associated_token/token_perpetual_distribution/reward_distribution_type/mod.rs(3 hunks)packages/rs-dpp/src/errors/consensus/basic/data_contract/invalid_token_distribution_function_incoherence_error.rs(1 hunks)packages/rs-drive-abci/Cargo.toml(1 hunks)packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/batch/tests/token/distribution/perpetual/time_based.rs(3 hunks)packages/rs-drive-abci/src/logging/logger.rs(3 hunks)packages/rs-drive-abci/src/logging/mod.rs(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: QuantumExplorer
PR: dashpay/platform#2257
File: packages/rs-drive-abci/src/mimic/test_quorum.rs:159-164
Timestamp: 2024-11-20T16:16:01.830Z
Learning: QuantumExplorer prefers not to receive auto-generated messages asking to post on social media.
⏰ Context from checks skipped due to timeout of 90000ms (18)
- GitHub Check: Rust packages (dash-sdk) / Check each feature
- GitHub Check: Rust packages (dash-sdk) / Detect immutable structure changes
- GitHub Check: Rust packages (dash-sdk) / Unused dependencies
- GitHub Check: Rust packages (dpp) / Check each feature
- GitHub Check: Rust packages (drive) / Unused dependencies
- GitHub Check: Rust packages (drive) / Linting
- GitHub Check: Rust packages (drive-abci) / Formatting
- GitHub Check: Rust packages (wasm-dpp) / Tests
- GitHub Check: Rust packages (drive-abci) / Check each feature
- GitHub Check: Rust packages (drive-abci) / Tests
- GitHub Check: Rust packages (dpp) / Linting
- GitHub Check: Rust packages (dpp) / Tests
- GitHub Check: Rust packages (wasm-dpp) / Linting
- GitHub Check: Rust packages (wasm-dpp) / Formatting
- 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 (14)
packages/rs-drive-abci/src/logging/mod.rs (1)
109-111: Improved test reliability with explicit dispatcher setupThis change improves the testing approach by explicitly creating and setting a tracing dispatcher rather than installing it globally. This approach aligns with the new idempotent logging initialization in
logger.rs, preventing potential conflicts between tests that manipulate the global logging state.packages/rs-drive-abci/src/logging/logger.rs (3)
56-60: LGTM: Good addition of idempotent logging initializationThe addition of
OnceLockis a clean way to track the logging initialization state without using mutable statics or other less safe approaches.
173-178: Well-designed API extension for subscriber creationThis new
as_subscriber()method nicely separates the creation of the subscriber from its installation, providing more flexibility for clients (like tests) that need more control over dispatcher lifetime.
192-207: Improved error handling with idempotent initializationThe modification to
try_install()makes it properly idempotent by checking theLOGGING_INSTALLEDstatus before attempting initialization. This is a significant improvement that prevents errors when the method is called multiple times, while maintaining the expected behavior.packages/rs-drive-abci/Cargo.toml (1)
109-109: Added new serial_test dependency for test organization.The addition of the
serial_testdependency will help ensure tests that could interfere with each other run in sequence rather than in parallel, supporting more robust testing of token distribution functionality.packages/rs-dpp/src/errors/consensus/basic/data_contract/invalid_token_distribution_function_incoherence_error.rs (1)
13-13: Improved error message format.Removed the trailing period in the error message format for better consistency and readability when rendering error messages with dynamic content.
packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/batch/tests/token/distribution/perpetual/time_based.rs (3)
14-14: Updated import to use renamed constant.Import now references the more descriptive
MAX_LINEAR_SLOPE_A_PARAMconstant which aligns with the broader parameter naming refactoring in the distribution function module.
1105-1106: Updated constant references for linear distribution.Updated to use the renamed
MAX_LINEAR_SLOPE_A_PARAMconstant and improved the code formatting with aligned comments for better readability.
1290-1291: Updated constant references in second test case.Consistently updated the constant usage to
MAX_LINEAR_SLOPE_A_PARAMwith aligned comments, maintaining consistency throughout the test file.packages/rs-dpp/src/data_contract/associated_token/token_perpetual_distribution/reward_distribution_type/mod.rs (4)
4-4: Updated import to include MAX_DISTRIBUTION_CYCLES_PARAM.Import now includes the
MAX_DISTRIBUTION_CYCLES_PARAMconstant which is used to standardize cycle limits for fixed amount distributions.
157-165: Improved cycle limit handling with clearer parameter names.The implementation now:
- Renames the parameter to the more descriptive
max_non_fixed_amount_cycles- Uses a dedicated constant
MAX_DISTRIBUTION_CYCLES_PARAMfor fixed amount distributions- Provides a clearer distinction between handling of fixed vs. non-fixed amount distributions
This approach centralizes the cycle limit definition and makes the code more maintainable.
175-175: Updated to use the new max_cycles variable.Modified the BlockBasedMoment calculation to use the newly defined
max_cyclesvariable, which now properly handles fixed vs. non-fixed amount distributions.
182-182: Updated to use the new max_cycles variable.Modified the TimeBasedMoment calculation to use the newly defined
max_cyclesvariable, ensuring consistent handling across all moment types.packages/rs-dpp/src/data_contract/associated_token/token_perpetual_distribution/distribution_function/evaluate.rs (1)
44-46: Edge‑case whenmax - min == u64::MAX
range = max.saturating_sub(*min).saturating_add(1);
Ifmin = 0andmax = u64::MAX,rangesaturates tou64::MAX, but the
correct range size is2^64.z % rangewill then never yieldmax.
Given the 48‑bit cap this is unlikely, yet consider guarding with validation
or dropping the+1and accepting half‑open[min,max)semantics.
...ata_contract/associated_token/token_perpetual_distribution/distribution_function/evaluate.rs
Show resolved
Hide resolved
...ata_contract/associated_token/token_perpetual_distribution/distribution_function/evaluate.rs
Show resolved
Hide resolved
...a_contract/associated_token/token_perpetual_distribution/distribution_function/validation.rs
Outdated
Show resolved
Hide resolved
...a_contract/associated_token/token_perpetual_distribution/distribution_function/validation.rs
Outdated
Show resolved
Hide resolved
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Issue being fixed or feature implemented
A series of tests and fixes for token distribution.
Breaking Changes
None
Checklist:
For repository code-owners and collaborators only
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Chores