test(platform): distribution log tests #2548
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThis set of changes significantly refactors and extends the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant DistributionFunction
participant Validator
participant Evaluator
participant Encoder
User->>DistributionFunction: Define new distribution function (with new/renamed fields)
DistributionFunction->>Validator: Validate parameters (including new fields)
Validator-->>DistributionFunction: Validation result
DistributionFunction->>Encoder: Encode/Decode distribution function
Encoder-->>DistributionFunction: Encoded/Decoded data
DistributionFunction->>Evaluator: Evaluate distribution at given cycle
Evaluator-->>DistributionFunction: Computed token amount
Possibly related PRs
Suggested reviewers
Poem
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:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
QuantumExplorer
changed the base branch from
v2.0-dev
to
v2.0-dev-distributions
![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
🔭 Outside diff range comments (1)
packages/rs-dpp/src/data_contract/associated_token/token_perpetual_distribution/distribution_function/evaluate.rs (1)
389-435:⚠️ Potential issueContinuation of mislabeled errors in Logarithmic branch.
Multiple lines here still reference
"InvertedLogarithmic"in theLogarithmicmatch arm. Correcting these strings is recommended to prevent confusion and false error identities.
🧹 Nitpick comments (4)
packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/batch/tests/token/distribution/perpetual/block_based.rs (1)
25-25: Wildcard import from the parent module.Importing with
use super::*;is convenient for tests but can lead to name collisions if expanded further. Fine for test modules; just ensure the scope remains clear.packages/rs-dpp/src/data_contract/associated_token/token_perpetual_distribution/distribution_function/evaluate.rs (1)
257-257: Parameter rename note.Using
b: c,in the match arm might be slightly confusing to future readers since we've renamed this parameter tobacross the codebase. Consider removing the alias if no longer necessary.packages/rs-dpp/src/data_contract/associated_token/token_perpetual_distribution/distribution_function/validation.rs (2)
465-475: Check valid range fora.Here,
*a == 0 || *a > MAX_EXP_A_PARAMtriggers an error. Ensuring a nonzero and reasonably smallais crucial to keep exponential curves stable. Note the error message referencesMAX_LOG_A_PARAMin the constructor—might be a simple mismatch. Consider verifying that the correct constant is used in the error hints.
2476-2495: Negative or largeavalues forInvertedLogarithmic.These tests confirm that extremely large positive or negative
atriggers errors. The error string references zero as disallowed, which is correct, but consider clarifying the entire range in the message to match the new constants.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
packages/rs-dpp/src/data_contract/associated_token/token_perpetual_distribution/distribution_function/encode.rs(8 hunks)packages/rs-dpp/src/data_contract/associated_token/token_perpetual_distribution/distribution_function/evaluate.rs(19 hunks)packages/rs-dpp/src/data_contract/associated_token/token_perpetual_distribution/distribution_function/mod.rs(13 hunks)packages/rs-dpp/src/data_contract/associated_token/token_perpetual_distribution/distribution_function/validation.rs(38 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(0 hunks)packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/batch/tests/token/distribution/perpetual/block_based.rs(10 hunks)packages/rs-platform-version/src/version/v9.rs(0 hunks)
💤 Files with no reviewable changes (2)
- packages/rs-drive-abci/Cargo.toml
- packages/rs-platform-version/src/version/v9.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 (drive) / Tests
- GitHub Check: Rust packages (drive) / Linting
- GitHub Check: Rust packages (drive) / Unused dependencies
- GitHub Check: Rust packages (drive) / Formatting
- GitHub Check: Rust packages (dash-sdk) / Unused dependencies
- GitHub Check: Rust packages (dash-sdk) / Formatting
- GitHub Check: Rust packages (dash-sdk) / Linting
- GitHub Check: Rust packages (dash-sdk) / Check each feature
- GitHub Check: Rust packages (dash-sdk) / Tests
- GitHub Check: Rust packages (drive-abci) / Tests
- GitHub Check: Rust packages (drive-abci) / Check each feature
- GitHub Check: Rust packages (drive-abci) / Linting
- GitHub Check: Rust packages (wasm-dpp) / Tests
- GitHub Check: Rust packages (dpp) / Tests
- GitHub Check: Rust packages (dpp) / Check each feature
- 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 (59)
packages/rs-dpp/src/errors/consensus/basic/data_contract/invalid_token_distribution_function_incoherence_error.rs (1)
13-13: Minor consistency fix in error message format.Removing the trailing period is consistent with other error messages and does not introduce any functional impact. The change appears correct and aligns with typical error message formatting.
packages/rs-dpp/src/data_contract/associated_token/token_perpetual_distribution/distribution_function/encode.rs (6)
25-28: Renamed and newly added fields inStepDecreasingAmountmatch updated logic.These lines properly reference
start_decreasing_offset,max_interval_count,distribution_start_amount, andtrailing_distribution_interval_amount. The naming is consistent with the new distribution parameters described in the summary. Looks good.
36-36: Encoding additional fieldmax_interval_count.This new field’s encoder call is straightforward and matches the declared type (
Option<u16>). No issues found.
38-38: Encodingtrailing_distribution_interval_amount.Adding the encoding line for this field is aligned with the new logic for trailing distribution intervals.
67-67: Parameter renaming inPolynomialandExponentialvariants.
- Line 67 & 78: Using
start_momentconsistently is clear.- Line 90: Renaming
btoc(or vice versa, as used internally) aligns with the updated design for exponential functions.
No functional or consistency concerns observed.Also applies to: 78-78, 90-90
174-176: Decode logic for newly added fields.
- Lines 174–176: Decoding and assigning
max_interval_countandtrailing_distribution_interval_amountis correct.- Line 248: Parallel rename for
b: cin Exponential decoding ensures consistency with encoding changes.Also applies to: 248-248
324-326: BorrowDecode logic mirrors Decode changes.These lines ensure the borrow-based decoder also handles the new/renamed fields. Using the same consistent approach is good for maintainability.
Also applies to: 333-335, 398-399
packages/rs-dpp/src/data_contract/associated_token/token_perpetual_distribution/reward_distribution_type/mod.rs (3)
4-4: ImportingMAX_DISTRIBUTION_CYCLES_PARAM.Including this constant is necessary to unify the logic for distribution cycle limits within this module.
157-165: Refined cycle capping logic formax_cycle_moment().Using:
let max_cycles = if matches!(self.function(), DistributionFunction::FixedAmount { .. }) { MAX_DISTRIBUTION_CYCLES_PARAM } else { max_non_fixed_amount_cycles as u64 };correctly differentiates unlimited cycles for fixed-amount distributions from user-provided cycle counts for others. The cast from
u32tou64is safe. No issues noted.
175-175: Usage ofsaturating_multo prevent overflow.Applying
.saturating_mul(max_cycles)ensures safer arithmetic. This approach is beneficial for avoiding panics on large multiplications.Also applies to: 178-178
packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/batch/tests/token/distribution/perpetual/block_based.rs (14)
11-14: IntroducingINITIAL_BALANCEconstant.Defining this hardcoded initial balance once improves clarity and avoids scattered magic numbers. Good approach.
88-88: Switched from vector to slice for transitions.Using
&[claim_serialized_transition.clone()]is a minor clarity improvement over&vec![...]. This is efficient and more idiomatic.Also applies to: 91-91
159-159: Consistent transition array usage and passing.All occurrences of passing
[claim_serialized_transition.clone()]are aligned with the updated approach, ensuring uniform usage. No concerns.Also applies to: 229-229, 340-340, 462-462
357-357: Assertion withPaidConsensusError.Verifying the
PaidConsensusError(...)variant is matched ensures the correct error is expected. This reflects a solid test for a failing claim scenario.
503-667: Newfixed_amounttest module.Comprehensive tests cover standard, overflow, and edge cases (e.g., zero amounts, huge amounts). They thoroughly exercise the distribution logic. This significantly boosts confidence in correctness.
668-771: Newrandomtest module.These tests verify min/max distributions, overflow boundaries, and entropy-based coverage. They appear comprehensive for random-based distribution scenarios.
772-869: Additional random distribution entropy testing.The entropy-check approach is a novel method to ensure distribution variety. This is a robust approach to verifying random logic behavior.
870-1516:step_decreasingtest suite.Extensive coverage for various step-based scenarios, offset handling, saturating multiplications, and trail intervals. Good thorough coverage.
1517-1572:stepwisedistribution test module.Testing multi-step intervals ensures correct boundary transitions between steps. The final checks appear correct.
1573-1717:lineardistribution test module.Validation for edge cases (e.g., divide by zero, negative parameters, min/max constraints) is well done. The coverage is substantial.
1718-2056:polynomialdistribution test module.Handles complicated polynomial logic (power exponents, negative values, zero divisions). Thorough coverage helps catch corner cases.
2057-2216:logarithmicdistribution test module.Checks large boundary values (e.g.,
u64::MAX, high parameters) and ensures no internal errors. Nicely addresses potential overflow or invalid logs.
2217-2394:inverted_logarithmicdistribution test module.Demonstrates diminishing or increasing returns through negative
avalues, verifying correctness in extreme log-based scenarios.
2396-2997:test_suitemodule for unified test logic.This engine-like structure systematically covers multiple distribution scenarios and is well-designed for reusability. It simplifies test administration across variants.
packages/rs-dpp/src/data_contract/associated_token/token_perpetual_distribution/distribution_function/evaluate.rs (14)
2-4: Add import for new default cycles constant.Importing
DEFAULT_STEP_DECREASING_AMOUNT_MAX_CYCLES_BEFORE_TRAILING_DISTRIBUTIONhere makes sense to provide a clear fallback for step-based distributions. No issues found with these lines.
44-44: Good use of saturating arithmetic.Using
saturating_subandsaturating_addhelps prevent underflow/overflow when computing the random distribution range.
56-59: New StepDecreasingAmount fields introduced.These fields (
start_decreasing_offset,max_interval_count,distribution_start_amount,trailing_distribution_interval_amount) expand configuration flexibility. Looks consistent with the rest of the struct usage.
68-69: Offset default logic.Using
unwrap_or(contract_registration_step)ensures a safe default start if no offset is provided. Implementation is straightforward.
70-71: Return start amount if before offset.Returning
distribution_start_amountwhenx <= s_valmatches the documented behavior of no decrease prior to offset.
74-75: Step count usage.Integer division
(x - s_val) / step_count as u64to compute steps passed is correct for a discrete step approach. No concerns here.
76-77: Default max interval count.Falling back to
DEFAULT_STEP_DECREASING_AMOUNT_MAX_CYCLES_BEFORE_TRAILING_DISTRIBUTION(128) if not provided is reasonable and documented.
79-81: Switch to trailing amount after max intervals.Exiting early with
trailing_distribution_interval_amountifsteps_passed > max_intervalsmatches the newly introduced logic for final intervals. Implementation is clear.
83-90: Iterative reduction logic.Multiplying by
reduction_numeratorand dividing bydenominatorin a loop correctly applies repeated step-based decreases. Use ofsaturating_subfor the numerator is a solid approach for avoiding negative underflow.
92-92: Local result variable introduction.Storing current
numeratorintoresultis fine for clarity before applying further checks.
94-98: Minimum value clamping.Conditionally clamping
resulttomin_valueensures we never go below a specified floor. Implementation is correct.
100-101: Final return.Returning
Ok(result)concludes the step decreasing evaluation properly.
349-359: Argument computation shortcuts.Breaking down the logarithmic argument by special cases (
m == 1,n == 1) optimizes performance. Implementation is straightforward.
364-364: Overflow check for log result.Ensuring
log_valis finite and withinu64::MAXis essential for safe casting. Good practice.packages/rs-dpp/src/data_contract/associated_token/token_perpetual_distribution/distribution_function/mod.rs (8)
14-19: Clarify max cycles documentation.These doc lines explain the maximum cycle parameter usage. Straightforward and consistent with the new distribution logic.
21-21: Default step decreasing cycles constant.Introducing
DEFAULT_STEP_DECREASING_AMOUNT_MAX_CYCLES_BEFORE_TRAILING_DISTRIBUTION(128) clarifies the fallback behavior for step-based distribution.
25-28: Parameter bounds for logarithmic/exponential.Defining
MIN_LOG_A_PARAM,MAX_LOG_A_PARAM, andMAX_EXP_A_PARAMensures a consistent validation range for these function variants.
101-102: Enhanced step decreasing docs.Documenting the formula and behavior for
x <= smore explicitly (returningn) removes ambiguity in this variant.
105-112: Detailed documentation for new fields.Expanded comments for
start_decreasing_offset,max_interval_count,distribution_start_amount, etc., improve maintainability and clarity.
257-257: Renaming exponential parameter tob.Replacing the old
cname withbmakes the parameter naming consistent across polynomial, logarithmic, and exponential variants.
349-359: Logarithmic function example expansions.These updated lines show a more detailed doc snippet. The illustration of partial fractions clarifies usage. Looks good.
485-524: InvertedLogarithmic example doc.This block explains an inverted log formula with a visually guided example. The step-by-step breakdown is helpful for readers.
packages/rs-dpp/src/data_contract/associated_token/token_perpetual_distribution/distribution_function/validation.rs (13)
8-9: Add new constants for validation checks.Importing
MAX_LOG_A_PARAM,MIN_LOG_A_PARAM,MAX_EXP_A_PARAM, etc., centralizes parameter constraint logic in the validation code.
21-21: Range check for distribution amount.Ensure that
nis nonzero and does not exceedMAX_DISTRIBUTION_PARAM. This prevents invalid fixed amounts. Good boundary enforcement.
71-73:distribution_start_amountconstraints.Requiring it to be within 1..=MAX_DISTRIBUTION_PARAM avoids zero or excessively large start amounts. This check aligns with the step decreasing logic.
85-95: Trailing distribution validation.Ensuring
trailing_distribution_interval_amountstays belowMAX_DISTRIBUTION_PARAMand verifyingmax_interval_countrange (2..=1024) fosters safe step-based distribution usage.
114-124: Denominator and ratio checks.Zero denominators or numerator ≥ denominator for the step decrease are disallowed. These constraints safeguard arithmetic integrity and ensure a strictly decreasing factor.
154-165: Offsetswithin bounds.Capping
start_decreasing_offsetatMAX_DISTRIBUTION_PARAMaligns with the bounding logic used everywhere else. Consistency is key.
433-443: Exponential validation form > 0.Requiring a
max_valueifmis positive ensures unbounded exponential growth won't inadvertently overflow. This prevents runaway distribution amounts.
517-527: Validation of exponential parameterb.Confirming
b <= MAX_DISTRIBUTION_PARAMavoids out-of-range offsets. Good strategic check.
613-633: Logarithmic function constraints onmanda.
m == 0ormtoo large is disallowed. CheckingaagainstMIN_LOG_A_PARAMandMAX_LOG_A_PARAMensures safe scaling for the log function.
655-666: Bound check forb.Disallowing
baboveMAX_DISTRIBUTION_PARAMkeeps final log results within valid emission ranges. No concerns here.
733-744: Ensuring (x - s + o) > 0.This block forbids invalid log arguments that are zero or negative. This forcibly prevents run-time overflow or undefined results in the log function.
782-793:InvertedLogarithmicrange checks fora.This code rejects zero or out-of-bounds
a, ensuring the function's slope remains meaningful. The validation message references an allowed range of -32766..32767. Implementation looks consistent with newly introduced constants.
795-805: Upper bound for offsetb.Similar to other variants, verifying that
bis at mostMAX_DISTRIBUTION_PARAMavoids initial offsets that could produce immediate overflow.
| let intermediate = if *a == 1 { | ||
| log_val | ||
| } else if *a == -1 { | ||
| -log_val | ||
| } else { | ||
| (*a as f64) * log_val | ||
| }; | ||
|
|
||
| let value = if d == &1 { | ||
| if !intermediate.is_finite() || intermediate > (i64::MAX as f64) { | ||
| if let Some(max_value) = max_value { | ||
| if intermediate.is_sign_positive() { | ||
| *max_value as i64 | ||
| } else { | ||
| return Err(ProtocolError::Overflow( | ||
| "InvertedLogarithmic: evaluation overflow intermediate bigger than i64::max", | ||
| )); | ||
| } |
There was a problem hiding this comment.
Additional mismatch in error messages.
Throughout this block, references to "InvertedLogarithmic" appear inside Logarithmic logic. This likely results from copy-paste. Updating them to "Logarithmic" will improve clarity and correctness.
| if !log_val.is_finite() || log_val > (u64::MAX as f64) { | ||
| return Err(ProtocolError::Overflow( | ||
| "Logarithmic function evaluation overflow or negative", | ||
| "InvertedLogarithmic: evaluation overflow", |
There was a problem hiding this comment.
Likely incorrect error label.
This line uses "InvertedLogarithmic: evaluation overflow" within the Logarithmic variant. The error message label seems mismatched and could confuse users.
- "InvertedLogarithmic: evaluation overflow",
+ "Logarithmic: evaluation overflow",📝 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.
| "InvertedLogarithmic: evaluation overflow", | |
| "Logarithmic: evaluation overflow", |
1 participant
Issue being fixed or feature implemented
Added log distribution tests and made minor fixes and performance improvements.
Breaking Changes
None
Checklist:
For repository code-owners and collaborators only
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores