feat(platform)!: distribute prefunded specialized balances after vote #2422
Conversation
WalkthroughThis pull request introduces significant enhancements to the Dash Platform protocol, focusing on protocol version upgrades, voting mechanisms, and specialized balance management. The changes span multiple packages, with key modifications in platform events, credit pools, and versioning. The primary objectives include improving the transition to protocol version 8, refining vote poll cleanup processes, and adding more robust handling of specialized balances. Changes
Possibly related PRs
Suggested reviewers
Poem
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[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-drive/src/drive/prefunded_specialized_balances/deduct_from_prefunded_specialized_balance_operations/mod.rs (1)
Line range hint
61-65: Update known versions in error message.The error message only lists version 0 as known, but the implementation supports both versions 0 and 1.
known_versions: vec![0], + known_versions: vec![0, 1],
🧹 Nitpick comments (16)
packages/rs-drive/src/drive/prefunded_specialized_balances/empty_prefunded_specialized_balance_operations/mod.rs (2)
16-16: Correct the typo in the documentation comment.There's a typo in the documentation comment on line 16. The word "it's" should be "its" to indicate possession.
Apply this diff to fix the typo:
- /// The operation Deducts from a prefunded specialized balance it's entire amount + /// The operation Deducts from a prefunded specialized balance its entire amount
16-29: Expand the documentation to include all parameters.The documentation currently only describes
transactionandplatform_versionin the arguments section. For better clarity and maintainability, please document all parameters of the function:
specialized_balance_iderror_if_does_not_existestimated_costs_only_with_layer_infoConsider updating the documentation as follows:
/// # Arguments /// - /// * `transaction` - A `TransactionArg` object representing the transaction to be used for adding to the system credits. - /// * `platform_version` - A `PlatformVersion` object specifying the version of Platform. + /// * `specialized_balance_id` - An `Identifier` for the specialized balance to be emptied. + /// * `error_if_does_not_exist` - A `bool` indicating whether to error if the balance does not exist. + /// * `estimated_costs_only_with_layer_info` - A mutable reference to an optional `HashMap` for estimated costs with layer information. + /// * `transaction` - A `TransactionArg` representing the transaction to be used. + /// * `platform_version` - A reference to the `PlatformVersion`. /// /// # Returnspackages/rs-drive/src/drive/credit_pools/epochs/credit_distribution_pools/add_epoch_processing_credits_for_distribution_operation/v0/mod.rs (2)
39-42: Clarify the error message for unexpected element type.The error message
"epochs processing fee must be an item"could be more specific. Since the expected element type isElement::SumItem, consider updating the error message to reflect this for better clarity.Apply this diff to improve the error message:
return Err(Error::Drive(DriveError::UnexpectedElementType( - "epochs processing fee must be an item", + "epochs processing fee must be a SumItem", )))
46-48: Enhance the overflow error message for clarity.The error message
"adding over i64::Max to processing fee pool"can be rephrased to be more informative. Consider updating it to clearly state that the amount exceeds the maximum allowed value for ani64.Apply this diff to improve the error message:
return Err(Error::Protocol(ProtocolError::Overflow( - "adding over i64::Max to processing fee pool", + "amount exceeds i64::MAX; cannot add to processing fee pool", )));packages/rs-drive/src/drive/prefunded_specialized_balances/deduct_from_prefunded_specialized_balance_operations/v1/mod.rs (1)
69-70: Avoid castingi64::MAXtou64to prevent potential issuesCasting
i64::MAX(which is2^63 - 1) tou64might not cause an immediate overflow, but it can introduce risks in future calculations or if the code is modified. It's safer to define a constant that represents the maximum possible balance inu64and use it consistently.Consider defining a
MAX_SPECIALIZED_BALANCEconstant of typeu64and using it here and elsewhere in the codebase for clarity and safety.packages/rs-drive/src/drive/prefunded_specialized_balances/add_prefunded_specialized_balance_operations/v1/mod.rs (1)
69-73: Inconsistent error message regarding maximum creditsThe error message in line 71 mentions exceeding
i64::MAX, while the condition checks againstMAX_CREDITS. This could be confusing ifMAX_CREDITSdiffers fromi64::MAX.Consider updating the error message to reference
MAX_CREDITSdirectly for clarity:- "trying to set prefunded specialized balance to over max credits amount (i64::MAX)", + "trying to set prefunded specialized balance over the maximum allowed credits (MAX_CREDITS)",Ensure
MAX_CREDITSis formatted appropriately in the message.packages/rs-drive-abci/src/execution/platform_events/protocol_upgrade/perform_events_on_first_block_of_protocol_change/v0/mod.rs (2)
24-24: Remove unused import:prefunded_specialized_balances_for_voting_pathThe import
prefunded_specialized_balances_for_voting_pathat line 24 is unused and can be safely removed to clean up the code.Apply this diff to remove the unused import:
use drive::drive::prefunded_specialized_balances::{ - prefunded_specialized_balances_for_voting_path, prefunded_specialized_balances_for_voting_path_vec, };🧰 Tools
🪛 GitHub Check: Rust packages (drive-abci) / Linting
[warning] 24-24: unused import:
prefunded_specialized_balances_for_voting_path
warning: unused import:prefunded_specialized_balances_for_voting_path
--> packages/rs-drive-abci/src/execution/platform_events/protocol_upgrade/perform_events_on_first_block_of_protocol_change/v0/mod.rs:24:5
|
24 | prefunded_specialized_balances_for_voting_path,
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
= note:#[warn(unused_imports)]on by default
108-108: Consider reducing the size of theErrortype to optimizeResultClippy warns that the
Errvariant returned fromtransition_to_version_8is very large, which may affect performance. Consider boxing large elements within theErrortype to reduce the size of theResult.🧰 Tools
🪛 GitHub Check: Rust packages (drive-abci) / Linting
[warning] 108-108: the
Err-variant returned from this function is very large
warning: theErr-variant returned from this function is very large
--> packages/rs-drive-abci/src/execution/platform_events/protocol_upgrade/perform_events_on_first_block_of_protocol_change/v0/mod.rs:108:10
|
108 | ) -> Result<(), Error> {
| ^^^^^^^^^^^^^^^^^
|
::: packages/rs-drive-abci/src/error/mod.rs:28:5
|
28 | Abci(#[from] AbciError),
| ----------------------- the largest variant contains at least 208 bytes
|
= help: try reducing the size oferror::Error, for example by boxing large elements or replacing it withBox<error::Error>
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#result_large_errpackages/rs-drive-abci/tests/strategy_tests/voting_tests.rs (1)
Line range hint
1326-2836: Refactor duplicated test code for maintainabilitySeveral test functions, such as
run_chain_with_voting_after_won_by_identity_no_specialized_funds_distributionandrun_chain_with_voting_after_won_by_identity_with_specialized_funds_distribution, contain duplicated setup and execution logic. Consider extracting common code into helper functions or fixtures to reduce duplication and improve maintainability.packages/rs-drive/src/drive/credit_pools/epochs/credit_distribution_pools/get_epoch_processing_credits_for_distribution/mod.rs (1)
29-29: Update parameter name in documentation.The parameter has been renamed from
epoch_treetoepoch, but the documentation still refers to the old name.Update the documentation to match the new parameter name:
- /// * `epoch_tree` - A reference to the Epoch. + /// * `epoch` - A reference to the Epoch.Also applies to: 41-41
packages/rs-drive/src/drive/credit_pools/epochs/credit_distribution_pools/add_epoch_processing_credits_for_distribution_operation/mod.rs (1)
16-24: Enhance documentation to clarify purpose and impact.The documentation should explain:
- The purpose of adding processing fees to the epoch
- The impact on credit distribution
- Any constraints or prerequisites
/// # Arguments /// -/// * `epoch_tree` - A reference to the Epoch. +/// * `epoch` - A reference to the Epoch to which processing fees will be added for distribution. /// * `amount` - The amount to add. /// * `transaction` - A TransactionArg instance. /// * `platform_version` - A PlatformVersion instance representing the version of the drive. +/// +/// This operation is part of the credit distribution mechanism where processing fees +/// are collected and later distributed according to the platform's rules.packages/rs-drive/src/drive/prefunded_specialized_balances/empty_prefunded_specialized_balance/v0/mod.rs (1)
20-21: Fix return type in documentation.The documentation states
Result<(), Error>but the actual return type isResult<Credits, Error>./// # Returns /// -/// * `Result<(), Error>` - If successful, returns `Ok(())`. If an error occurs during the operation, returns an `Error`. +/// * `Result<Credits, Error>` - If successful, returns the emptied credits amount. If an error occurs during the operation, returns an `Error`.packages/rs-drive/src/drive/prefunded_specialized_balances/empty_prefunded_specialized_balance/mod.rs (1)
13-27: Complete the documentation.The documentation is missing descriptions for
specialized_balance_idanderror_if_does_not_existparameters./// # Arguments /// +/// * `specialized_balance_id` - The identifier of the prefunded specialized balance to empty. +/// * `error_if_does_not_exist` - If true, returns an error when the balance doesn't exist. /// * `transaction` - A `TransactionArg` object representing the transaction to be used for adding to the system credits. /// * `platform_version` - A `PlatformVersion` object specifying the version of Platform.packages/rs-drive/src/drive/prefunded_specialized_balances/mod.rs (1)
27-27: Document the significance of the constant value.The value 128 for
PREFUNDED_BALANCES_FOR_VOTINGseems arbitrary. Consider adding documentation explaining why this specific value was chosen.-/// The key for prefunded balances for voting +/// The key for prefunded balances for voting. +/// Uses value 128 to ensure no collision with other potential balance types +/// in the future, as this occupies the upper half of the u8 range.packages/rs-drive/src/drive/prefunded_specialized_balances/empty_prefunded_specialized_balance_operations/v0/mod.rs (2)
40-47: Consider adding a constant for the query target value.The magic number
8in the query target could be more maintainable as a named constant.+const SPECIALIZED_BALANCE_QUERY_TARGET: u32 = 8; DirectQueryType::StatelessDirectQuery { in_tree_using_sums: true, - query_target: QueryTargetValue(8), + query_target: QueryTargetValue(SPECIALIZED_BALANCE_QUERY_TARGET), }
60-74: Consider consolidating the error handling logic.The nested if-else blocks with multiple return paths could be simplified for better readability.
-if estimated_costs_only_with_layer_info.is_none() { - return if error_if_does_not_exist { - Err(Error::Drive( - DriveError::PrefundedSpecializedBalanceDoesNotExist(format!( - "trying to deduct from a prefunded specialized balance {} that does not exist", - specialized_balance_id - )), - )) - } else { - Ok((0, drive_operations)) - }; -} else { - 0 -} +match (estimated_costs_only_with_layer_info.is_none(), error_if_does_not_exist) { + (true, true) => Err(Error::Drive( + DriveError::PrefundedSpecializedBalanceDoesNotExist(format!( + "trying to deduct from a prefunded specialized balance {} that does not exist", + specialized_balance_id + )), + )), + (true, false) => Ok((0, drive_operations)), + (false, _) => Ok((0, drive_operations)) +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (38)
packages/rs-drive-abci/src/execution/platform_events/protocol_upgrade/perform_events_on_first_block_of_protocol_change/v0/mod.rs(3 hunks)packages/rs-drive-abci/src/execution/platform_events/voting/check_for_ended_vote_polls/v0/mod.rs(1 hunks)packages/rs-drive-abci/src/execution/platform_events/voting/clean_up_after_contested_resources_vote_polls_end/mod.rs(4 hunks)packages/rs-drive-abci/src/execution/platform_events/voting/clean_up_after_contested_resources_vote_polls_end/v0/mod.rs(7 hunks)packages/rs-drive-abci/src/execution/platform_events/voting/clean_up_after_contested_resources_vote_polls_end/v1/mod.rs(1 hunks)packages/rs-drive-abci/src/execution/platform_events/voting/clean_up_after_vote_polls_end/mod.rs(3 hunks)packages/rs-drive-abci/src/execution/platform_events/voting/clean_up_after_vote_polls_end/v0/mod.rs(3 hunks)packages/rs-drive-abci/src/query/prefunded_specialized_balances/balance/v0/mod.rs(1 hunks)packages/rs-drive-abci/tests/strategy_tests/upgrade_fork_tests.rs(2 hunks)packages/rs-drive-abci/tests/strategy_tests/voting_tests.rs(5 hunks)packages/rs-drive/src/drive/credit_pools/epochs/credit_distribution_pools/add_epoch_processing_credits_for_distribution_operation/mod.rs(1 hunks)packages/rs-drive/src/drive/credit_pools/epochs/credit_distribution_pools/add_epoch_processing_credits_for_distribution_operation/v0/mod.rs(1 hunks)packages/rs-drive/src/drive/credit_pools/epochs/credit_distribution_pools/get_epoch_processing_credits_for_distribution/mod.rs(2 hunks)packages/rs-drive/src/drive/credit_pools/epochs/credit_distribution_pools/get_epoch_processing_credits_for_distribution/v0/mod.rs(1 hunks)packages/rs-drive/src/drive/credit_pools/epochs/credit_distribution_pools/mod.rs(1 hunks)packages/rs-drive/src/drive/mod.rs(1 hunks)packages/rs-drive/src/drive/prefunded_specialized_balances/add_prefunded_specialized_balance_operations/mod.rs(2 hunks)packages/rs-drive/src/drive/prefunded_specialized_balances/add_prefunded_specialized_balance_operations/v1/mod.rs(1 hunks)packages/rs-drive/src/drive/prefunded_specialized_balances/deduct_from_prefunded_specialized_balance_operations/mod.rs(2 hunks)packages/rs-drive/src/drive/prefunded_specialized_balances/deduct_from_prefunded_specialized_balance_operations/v1/mod.rs(1 hunks)packages/rs-drive/src/drive/prefunded_specialized_balances/empty_prefunded_specialized_balance/mod.rs(1 hunks)packages/rs-drive/src/drive/prefunded_specialized_balances/empty_prefunded_specialized_balance/v0/mod.rs(1 hunks)packages/rs-drive/src/drive/prefunded_specialized_balances/empty_prefunded_specialized_balance_operations/mod.rs(1 hunks)packages/rs-drive/src/drive/prefunded_specialized_balances/empty_prefunded_specialized_balance_operations/v0/mod.rs(1 hunks)packages/rs-drive/src/drive/prefunded_specialized_balances/mod.rs(2 hunks)packages/rs-drive/src/fees/mod.rs(1 hunks)packages/rs-drive/src/lib.rs(1 hunks)packages/rs-drive/src/util/grove_operations/grove_get_raw_optional/v0/mod.rs(1 hunks)packages/rs-platform-value/src/inner_value.rs(1 hunks)packages/rs-platform-version/src/version/drive_abci_versions/drive_abci_method_versions/v5.rs(1 hunks)packages/rs-platform-version/src/version/drive_versions/drive_credit_pool_method_versions/mod.rs(1 hunks)packages/rs-platform-version/src/version/drive_versions/drive_credit_pool_method_versions/v1.rs(1 hunks)packages/rs-platform-version/src/version/drive_versions/mod.rs(2 hunks)packages/rs-platform-version/src/version/drive_versions/v1.rs(1 hunks)packages/rs-platform-version/src/version/drive_versions/v2.rs(1 hunks)packages/rs-platform-version/src/version/drive_versions/v3.rs(1 hunks)packages/rs-platform-version/src/version/mocks/v2_test.rs(1 hunks)packages/rs-platform-version/src/version/v8.rs(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/rs-drive/src/fees/mod.rs
🧰 Additional context used
🪛 GitHub Check: Rust packages (drive-abci) / Linting
packages/rs-drive-abci/src/execution/platform_events/protocol_upgrade/perform_events_on_first_block_of_protocol_change/v0/mod.rs
[warning] 24-24: unused import: prefunded_specialized_balances_for_voting_path
warning: unused import: prefunded_specialized_balances_for_voting_path
--> packages/rs-drive-abci/src/execution/platform_events/protocol_upgrade/perform_events_on_first_block_of_protocol_change/v0/mod.rs:24:5
|
24 | prefunded_specialized_balances_for_voting_path,
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
= note: #[warn(unused_imports)] on by default
[warning] 108-108: the Err-variant returned from this function is very large
warning: the Err-variant returned from this function is very large
--> packages/rs-drive-abci/src/execution/platform_events/protocol_upgrade/perform_events_on_first_block_of_protocol_change/v0/mod.rs:108:10
|
108 | ) -> Result<(), Error> {
| ^^^^^^^^^^^^^^^^^
|
::: packages/rs-drive-abci/src/error/mod.rs:28:5
|
28 | Abci(#[from] AbciError),
| ----------------------- the largest variant contains at least 208 bytes
|
= help: try reducing the size of error::Error, for example by boxing large elements or replacing it with Box<error::Error>
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#result_large_err
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Test Suite (Test Suite, test:suite, 0, 0) / Run Test Suite
- GitHub Check: Test Suite (Test Suite in browser (1), test:browsers, 0, 2) / Run Test Suite in browser (1)
- GitHub Check: JS packages (dash) / Tests
- GitHub Check: Dashmate E2E tests (Local network, test/e2e/localNetwork.spec.js, true) / Run Local network tests
🔇 Additional comments (43)
packages/rs-drive-abci/src/execution/platform_events/voting/clean_up_after_contested_resources_vote_polls_end/v1/mod.rs (1)
21-86: Code changes are well-implemented and handle credits correctly.The implementation appropriately cleans up after contested resources vote polls end. It correctly:
- Retrieves operations for cleanup.
- Iterates over specialized balance IDs and empties the prefunded specialized balances.
- Accumulates total credits and handles potential overflows using
checked_add.- Adds the credits to epoch processing for distribution if applicable.
- Applies batch operations only when there are operations to execute.
packages/rs-drive/src/drive/prefunded_specialized_balances/deduct_from_prefunded_specialized_balance_operations/v1/mod.rs (3)
60-71: Potential issue with balance estimation when balance does not existIn the case where the specialized balance does not exist, and
estimated_costs_only_with_layer_infoisSome, the code assignsi64::MAX as u64toprevious_credits_in_specialized_balance(lines 69-70). Usingi64::MAX as u64might lead to incorrect cost estimations or overflow in subsequent calculations when estimating costs.Consider using a more appropriate placeholder value that accurately reflects the absence of a balance without risking overflow. This will ensure that cost estimations remain accurate and safe.
75-81: Correct handling of insufficient balance during deductionThe use of
checked_subensures that underflow does not occur when deducting the amount from the previous balance. If the subtraction would result in a negative value, an appropriate error is returned.This is a proper use of safe arithmetic operations to prevent runtime panics due to underflow.
62-67: Clear error message for non-existent specialized balanceThe error returned when attempting to deduct from a non-existent specialized balance includes the identifier, providing valuable context for debugging.
Including specific identifiers in error messages enhances traceability and facilitates quicker issue resolution.
packages/rs-drive/src/drive/prefunded_specialized_balances/add_prefunded_specialized_balance_operations/v1/mod.rs (2)
65-67: Proper handling of potential overflow when adding balancesThe use of
checked_addsafeguards against arithmetic overflow when adding theamountto theprevious_credits_in_specialized_balance. An error is returned if an overflow would occur.This ensures the integrity of balance calculations and prevents critical state corruption.
69-73: Avoid potential off-by-one error when checking maximum creditsThe condition
if new_total >= MAX_CREDITS(line 69) might allownew_totalto be exactlyMAX_CREDITS, which could be undesirable ifMAX_CREDITSis intended to be the absolute maximum allowed value.Confirm whether
new_totalshould be strictly less thanMAX_CREDITS. If so, adjust the condition:- if new_total >= MAX_CREDITS { + if new_total > MAX_CREDITS {This ensures that
new_totaldoes not exceed the maximum allowed credits.packages/rs-platform-version/src/version/drive_versions/v3.rs (2)
88-97: Verify consistency of method versioning for specialized balance operationsIn the
prefunded_specialized_balancessection, the versions foradd_prefunded_specialized_balance_operationsanddeduct_from_prefunded_specialized_balanceare set to1, indicating updates in version 3. However,deduct_from_prefunded_specialized_balance_operationsremains at version0.Please confirm if
deduct_from_prefunded_specialized_balance_operationsshould also be updated to version1to reflect the changes made in the corresponding modulededuct_from_prefunded_specialized_balance_operations/v1/mod.rs.
70-73: Ensure method versioning aligns with implementation changesThe methods
commit_transactionandapply_partial_batch_low_level_drive_operations(lines 72-73) share the same version0. If there have been changes specific to version 3, consider updating their version numbers to reflect these changes.Aligning method versions with their implementations helps maintain clarity in version control and ensures that the correct method versions are used during protocol upgrades.
packages/rs-drive-abci/src/execution/platform_events/voting/clean_up_after_contested_resources_vote_polls_end/v0/mod.rs (2)
32-49: Good modularization of cleanup operationsThe separation of operation collection into the
clean_up_after_contested_resources_vote_polls_end_operations_v0method improves modularity and readability. The updated implementation inclean_up_after_contested_resources_vote_polls_end_v0correctly applies the operations when they are not empty.
Line range hint
52-163: Implementation of operations collection is correctThe new method
clean_up_after_contested_resources_vote_polls_end_operations_v0properly collects all necessary low-level drive operations for cleaning up after ended contested resource vote polls. The logic for handling voters and identities is correctly implemented.packages/rs-drive/src/drive/credit_pools/epochs/credit_distribution_pools/mod.rs (1)
6-6: LGTM! Verify module integration with credit distribution system.The new module
add_epoch_processing_credits_for_distribution_operationaligns with the PR's objective of handling specialized balance distribution.Let's verify the module's integration:
✅ Verification successful
Module is properly integrated within the credit distribution system ✅
The module is correctly structured with:
- Proper module declaration in mod.rs
- Versioned implementation (v0)
- Operation handling functionality in place
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify module integration and usage # Check for proper imports and usage of the new module rg -p "use.*add_epoch_processing_credits_for_distribution_operation" packages/Length of output: 80
Script:
#!/bin/bash # Check mod.rs content and module exports echo "=== Checking mod.rs content ===" cat packages/rs-drive/src/drive/credit_pools/epochs/credit_distribution_pools/mod.rs echo -e "\n=== Checking for potential uses of the module functionality ===" # Search for any struct or function names that might be from this module rg -p "add_epoch_processing_credits.*distribution" packages/rs-drive/src/Length of output: 1624
packages/rs-drive/src/lib.rs (1)
44-46: LGTM! Module visibility change is appropriate.Making the fees module public is necessary for handling specialized balance distribution functionality.
packages/rs-drive-abci/src/execution/platform_events/voting/clean_up_after_vote_polls_end/mod.rs (2)
5-5: LGTM! Import added for new parameter type.The BlockInfo import supports the new parameter addition.
21-21: Verify BlockInfo usage in cleanup process.The addition of BlockInfo parameter enhances the cleanup process with block context, which is necessary for proper specialized balance distribution after vote polls end.
Let's verify the BlockInfo usage:
Also applies to: 34-34
✅ Verification successful
BlockInfo parameter is correctly implemented and necessary
The code shows proper implementation of BlockInfo parameter with version handling (v0 and v1), where v1 specifically uses BlockInfo for enhanced cleanup process. The change maintains backward compatibility while adding necessary block context support.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify BlockInfo parameter usage in cleanup process # Check how BlockInfo is used in v0 implementation rg -p "block_info.*clean_up_after_vote_polls_end_v0" packages/Length of output: 64
Script:
#!/bin/bash # Find the implementation of clean_up_after_vote_polls_end ast-grep --pattern 'pub async fn clean_up_after_vote_polls_end($_) { $$$ }' # Also search for any BlockInfo usage in the same file rg -p "BlockInfo" "packages/rs-drive-abci/src/execution/platform_events/voting/clean_up_after_vote_polls_end/mod.rs"Length of output: 270
Script:
#!/bin/bash # Get the complete implementation with context rg -p -B 5 -A 20 "pub async fn clean_up_after_vote_polls_end" "packages/rs-drive-abci/src/execution/platform_events/voting/clean_up_after_vote_polls_end/mod.rs" # Look for balance-related operations rg -p "balance|amount|distribution" "packages/rs-drive-abci/src/execution/platform_events/voting/clean_up_after_vote_polls_end/mod.rs"Length of output: 295
Script:
#!/bin/bash # Get the entire file content to understand structure cat "packages/rs-drive-abci/src/execution/platform_events/voting/clean_up_after_vote_polls_end/mod.rs" # Look for any impl blocks or trait implementations ast-grep --pattern 'impl $_ { $$$ }'Length of output: 66165
Script:
#!/bin/bash # Check v0 implementation cat "packages/rs-drive-abci/src/execution/platform_events/voting/clean_up_after_vote_polls_end/v0/mod.rs"Length of output: 2493
Script:
#!/bin/bash # Check the implementation of clean_up_after_contested_resources_vote_polls_end rg -p "clean_up_after_contested_resources_vote_polls_end" "packages/rs-drive-abci/src/execution/platform_events/voting/" -A 5Length of output: 3600
packages/rs-drive/src/drive/credit_pools/epochs/credit_distribution_pools/add_epoch_processing_credits_for_distribution_operation/mod.rs (1)
29-55: Implementation looks good!The version handling and error management are well-implemented. The method correctly delegates to version-specific implementation while maintaining backward compatibility.
packages/rs-drive/src/drive/prefunded_specialized_balances/empty_prefunded_specialized_balance/v0/mod.rs (1)
26-51: Implementation looks good!The method correctly:
- Uses batch operations for atomicity
- Tracks and applies costs
- Handles errors appropriately
packages/rs-drive/src/drive/prefunded_specialized_balances/empty_prefunded_specialized_balance/mod.rs (1)
27-52: Implementation looks good!The version handling and error management are well-implemented, following the established pattern for versioned methods.
packages/rs-platform-version/src/version/drive_versions/drive_credit_pool_method_versions/v1.rs (1)
28-28: Version initialization looks good!The addition of
add_epoch_processing_credits_for_distribution_operationwith version 0 follows the established pattern and is correctly placed in the structure.packages/rs-drive-abci/src/execution/platform_events/voting/clean_up_after_contested_resources_vote_polls_end/mod.rs (2)
24-24: LGTM: Clean version upgrade with backward compatibilityThe addition of
block_infoparameter and v1 handling is well-structured, maintaining backward compatibility while introducing new functionality.Also applies to: 46-52
55-55: Verify version array completenessThe known versions array should be kept in sync with all implemented versions.
✅ Verification successful
Known versions array is complete and accurate ✓
The array
vec![0, 1]correctly matches all implemented versions (v0, v1) in the codebase.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify all version implementations exist echo "Checking for version implementations..." fd -t d "v[0-9]+" "packages/rs-drive-abci/src/execution/platform_events/voting/clean_up_after_contested_resources_vote_polls_end"Length of output: 446
packages/rs-platform-version/src/version/drive_versions/drive_credit_pool_method_versions/mod.rs (1)
31-31: LGTM: New feature version field for epoch processing creditsThe addition of
add_epoch_processing_credits_for_distribution_operationaligns with the PR's objective of managing specialized balance distribution.packages/rs-drive-abci/src/execution/platform_events/voting/clean_up_after_vote_polls_end/v0/mod.rs (1)
22-22: LGTM: Consistent parameter propagationThe BlockInfo parameter is correctly propagated through the v0 implementation to maintain consistency with the updated interface.
Also applies to: 49-49
packages/rs-drive/src/drive/prefunded_specialized_balances/add_prefunded_specialized_balance_operations/mod.rs (2)
54-60: LGTM: Clean version upgrade implementationThe v1 implementation is properly integrated with appropriate parameter passing.
1-2: Verify v1 module implementationEnsure the v1 module is properly implemented with all required functionality.
packages/rs-drive/src/drive/prefunded_specialized_balances/deduct_from_prefunded_specialized_balance_operations/mod.rs (1)
54-60: LGTM! Version 1 support added correctly.The implementation properly extends the version handling to support version 1 of the deduct operation.
packages/rs-drive/src/drive/prefunded_specialized_balances/mod.rs (2)
10-12: LGTM! New modules for empty balance operations.The addition of empty balance operation modules aligns with the PR objective of handling specialized balances after vote completion.
Line range hint
31-50: LGTM! Visibility changes for path functions.The increased visibility of these functions is appropriate as they need to be accessed during vote cleanup operations.
packages/rs-drive/src/drive/credit_pools/epochs/credit_distribution_pools/get_epoch_processing_credits_for_distribution/v0/mod.rs (1)
18-25: LGTM! Parameter rename improves clarity.The parameter rename from
epoch_treetoepochbetter reflects its type and usage, improving code readability.packages/rs-drive/src/util/grove_operations/grove_get_raw_optional/v0/mod.rs (1)
16-16: LGTM! Appropriate visibility restriction.The visibility change from
pub(crate)topub(super)follows Rust's principle of least privilege, restricting access to only the necessary scope.packages/rs-drive/src/drive/prefunded_specialized_balances/empty_prefunded_specialized_balance_operations/v0/mod.rs (1)
23-32: LGTM! Well-documented method signature.The method signature is clear and includes all necessary parameters for emptying a prefunded specialized balance.
packages/rs-platform-version/src/version/v8.rs (1)
34-35: LGTM! Well-documented version upgrade.The upgrade to DRIVE_VERSION_V3 is properly documented with the reason for the change.
packages/rs-drive-abci/src/query/prefunded_specialized_balances/balance/v0/mod.rs (1)
53-53: LGTM! Improved error message clarity.The error message now correctly reflects that a specialized balance was not found, rather than a generic "No Identity found" message.
packages/rs-platform-version/src/version/drive_versions/v1.rs (1)
96-96: LGTM! Consistent version definition.The addition of empty_prefunded_specialized_balance with version 0 maintains consistency with other method versions in the struct.
packages/rs-platform-version/src/version/drive_versions/v2.rs (1)
96-96: LGTM! Verify integration with protocol version 8.The addition of
empty_prefunded_specialized_balancewith version 0 aligns with the PR objective of distributing prefunded specialized balances after vote concludes.Run the following script to verify the feature version integration:
✅ Verification successful
Integration with protocol version 8 verified successfully
The
empty_prefunded_specialized_balancefeature is properly integrated with protocol version 8 through DRIVE_VERSION_V3 and is used during protocol version transition. The implementation aligns with the PR objective of distributing prefunded specialized balances after vote concludes.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the integration of empty_prefunded_specialized_balance with protocol version 8 # Search for protocol version 8 references and empty_prefunded_specialized_balance usage rg -A 5 "protocol.*version.*8|empty_prefunded_specialized_balance"Length of output: 73510
packages/rs-platform-version/src/version/drive_abci_versions/drive_abci_method_versions/v5.rs (1)
94-94: LGTM! Version bump for vote cleanup.The version increment of
clean_up_after_contested_resources_vote_poll_endfrom 0 to 1 indicates enhanced cleanup logic, likely incorporating the distribution of prefunded specialized balances.Run the following script to verify the implementation of version 1:
✅ Verification successful
Verified: Implementation matches the version bump rationale
The version 1 implementation indeed adds specialized balance cleanup functionality, emptying prefunded balances and distributing recovered credits to epoch processing, exactly as suggested in the review comment.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the implementation of clean_up_after_contested_resources_vote_poll_end_v1 # Search for the v1 implementation and its usage ast-grep --pattern 'fn clean_up_after_contested_resources_vote_poll_end_v1($$$)'Length of output: 82
Script:
#!/bin/bash # Search for cleanup implementations and contested resources rg -i "clean_up.*contested.*resources" -A 5 rg "fn clean_up.*vote" -A 5 rg "contested.*resources.*cleanup" -A 5 # Find files related to vote cleanup implementations fd "cleanup|clean_up" --type f # Look in the specific version directory fd . "packages/rs-platform-version/src/version"Length of output: 34151
Script:
#!/bin/bash # Look at the v1 implementation details rg -A 20 "clean_up_after_contested_resources_vote_polls_end_v1" packages/rs-drive-abci/src/execution/platform_events/voting/clean_up_after_contested_resources_vote_polls_end/v1/mod.rs # Compare with v0 implementation rg -A 20 "clean_up_after_contested_resources_vote_polls_end_v0" packages/rs-drive-abci/src/execution/platform_events/voting/clean_up_after_contested_resources_vote_polls_end/v0/mod.rsLength of output: 2017
Script:
#!/bin/bash # Get more context from v1 implementation rg -B 5 -A 30 "clean out the specialized balances" packages/rs-drive-abci/src/execution/platform_events/voting/clean_up_after_contested_resources_vote_polls_end/v1/mod.rs # Look for specialized balance related code rg "specialized.*balance" packages/rs-drive-abci/src/execution/platform_events/voting/clean_up_after_contested_resources_vote_polls_end/v1/Length of output: 3229
packages/rs-platform-version/src/version/drive_versions/mod.rs (2)
24-24: LGTM! New version module added.The addition of
v3module prepares for future protocol version changes.
89-89: LGTM! New feature version field added.The addition of
empty_prefunded_specialized_balancetoDrivePrefundedSpecializedMethodVersionsaligns with the feature addition in v2.rs.packages/rs-drive/src/drive/mod.rs (1)
45-48: LGTM! Module visibility change with documentation.The change from
pub(crate)topubfor theprefunded_specialized_balancesmodule appropriately exposes the functionality needed for balance distribution after votes. The added documentation improves code clarity.packages/rs-drive-abci/tests/strategy_tests/upgrade_fork_tests.rs (1)
786-786: Document the performance optimization trade-off.The sum tree verification has been disabled in two test functions for performance reasons. While this speeds up the tests, it's important to document:
- What verification we're skipping
- Why it's safe to skip in these test cases
- Whether this should be conditionally enabled in CI
Consider adding a comment explaining the trade-offs or making it configurable:
- verify_sum_trees: false, //faster without this + verify_sum_trees: cfg!(feature = "slow-tests"), // Disabled for faster local development. Enable with --features slow-testsAlso applies to: 1372-1372
packages/rs-platform-version/src/version/mocks/v2_test.rs (1)
130-130: LGTM! Consider adding test coverage for the new version.The addition of
empty_prefunded_specialized_balancewith version 0 aligns with the PR's objective of managing prefunded specialized balances after vote conclusion.Let's verify the test coverage for this new version:
✅ Verification successful
Test coverage for empty_prefunded_specialized_balance is verified and sufficient
The feature is well-tested through:
- Mock tests in platform-version (v2_test.rs, v3_test.rs)
- Integration with voting cleanup and protocol upgrade systems
- Consistent version handling across platform versions
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for test cases covering the new empty_prefunded_specialized_balance functionality rg -l "empty_prefunded_specialized_balance.*test"Length of output: 51
Script:
#!/bin/bash # Search for any usage of empty_prefunded_specialized_balance with context rg -A 5 "empty_prefunded_specialized_balance" # Find test files that might be using this mock fd "test\.rs$" packages/rs-platform-version/src/Length of output: 14202
packages/rs-drive-abci/src/execution/platform_events/voting/check_for_ended_vote_polls/v0/mod.rs (2)
Line range hint
313-317: LGTM! Clean integration of block_info parameter.The addition of
block_infoparameter toclean_up_after_vote_polls_endenhances the context available during cleanup operations, which is essential for proper handling of prefunded specialized balances.
Line range hint
71-78: Verify the testnet cleanup condition.The special case handling for testnet cleanup at epoch 1434->1435 seems to be hardcoded. Consider making this configurable or documenting why this specific epoch transition is important.
Let's check for related configurations or documentation:
packages/rs-platform-value/src/inner_value.rs (1)
364-364: LGTM! Improved lifetime specification.The addition of the
'alifetime annotation to the return type makes the relationship between the input reference and output slice explicit, which is a good practice for preventing potential lifetime issues.
.../platform_events/protocol_upgrade/perform_events_on_first_block_of_protocol_change/v0/mod.rs
Outdated
Show resolved
Hide resolved
...src/drive/prefunded_specialized_balances/add_prefunded_specialized_balance_operations/mod.rs
Outdated
Show resolved
Hide resolved
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
.../platform_events/protocol_upgrade/perform_events_on_first_block_of_protocol_change/v0/mod.rs
Outdated
Show resolved
Hide resolved
.../platform_events/protocol_upgrade/perform_events_on_first_block_of_protocol_change/v0/mod.rs
Outdated
Show resolved
Hide resolved
.../platform_events/protocol_upgrade/perform_events_on_first_block_of_protocol_change/v0/mod.rs
Show resolved
Hide resolved
.../platform_events/protocol_upgrade/perform_events_on_first_block_of_protocol_change/v0/mod.rs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
packages/rs-drive-abci/src/execution/platform_events/protocol_upgrade/perform_events_on_first_block_of_protocol_change/v0/mod.rs (3)
116-116: Enhance function documentationThe comment could be more descriptive about why specialized balances need to be emptied and what happens to them after the transition.
161-161: Consider defining the query limit as a constantThe magic number 30000 should be defined as a named constant at the module level for better maintainability and documentation.
+const MAX_VOTE_QUERY_LIMIT: u32 = 30000; // Maximum number of votes to query in a single request // ... in the query -limit: Some(30000), //Just a high number that shouldn't break the system +limit: Some(MAX_VOTE_QUERY_LIMIT),
117-239: Consider breaking down the function for better maintainabilityThe
transition_to_version_8function is quite long and handles multiple responsibilities. Consider extracting the following operations into separate functions:
- Retrieving specialized balances
- Processing active votes
- Handling non-active balances
Example refactor:
fn get_all_specialized_balances(&self, transaction: &Transaction, platform_version: &PlatformVersion) -> Result<HashSet<Identifier>, Error> { // Lines 124-143 } fn get_active_specialized_balances(&self, transaction: &Transaction, platform_version: &PlatformVersion) -> Result<HashSet<Identifier>, Error> { // Lines 145-189 } fn process_non_active_balances( &self, non_active_balances: impl Iterator<Item = &Identifier>, block_info: &BlockInfo, transaction: &Transaction, platform_version: &PlatformVersion, ) -> Result<(), Error> { // Lines 195-236 }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/rs-drive-abci/src/execution/platform_events/protocol_upgrade/perform_events_on_first_block_of_protocol_change/v0/mod.rs(3 hunks)
🧰 Additional context used
📓 Learnings (1)
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.
⏰ 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) / Formatting
- GitHub Check: Rust packages (dpp) / Unused dependencies
- GitHub Check: Rust packages (dpp) / Tests
- GitHub Check: Rust packages (dpp) / Linting
- GitHub Check: Rust packages (dpp) / Check each feature
- GitHub Check: Rust packages (dash-sdk) / Unused dependencies
- GitHub Check: Rust packages (dash-sdk) / Tests
- GitHub Check: Rust packages (dash-sdk) / Linting
- GitHub Check: Rust packages (dash-sdk) / Check each feature
- GitHub Check: Rust packages (dapi-grpc) / Unused dependencies
- GitHub Check: Rust packages (dapi-grpc) / Tests
- GitHub Check: Rust packages (dapi-grpc) / Check each feature
- GitHub Check: Rust packages (dapi-grpc) / Linting
- 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 Docker images (Dashmate helper, dashmate-helper, dashmate-helper) / Build Dashmate helper image
- GitHub Check: Build JS packages / Build JS
🔇 Additional comments (3)
packages/rs-drive-abci/src/execution/platform_events/protocol_upgrade/perform_events_on_first_block_of_protocol_change/v0/mod.rs (3)
1-30: LGTM! Well-organized importsThe new imports are properly organized and necessary for implementing the version 8 transition functionality.
74-86: LGTM! Appropriate error handling for version 8 transitionThe error handling approach is well-documented and intentionally designed to allow the protocol upgrade to proceed even if non-critical errors occur during the transition.
199-214: LGTM! Robust credit overflow handlingThe implementation properly handles credit overflow scenarios with appropriate error messages.
shumkov
deleted the
feat/distributePrefundedSpecializedBalancesAfterVote
branch
Issue being fixed or feature implemented
The prefunded specialized balances were not being distributed as they should be. This PR rectifies this situation.
What was done?
When a vote ends it's prefunded specialized balance is sent to processing pool for later distribution.
When we hit protocol version 8, we will distribute all expired specialized balances.
How Has This Been Tested?
Many tests were written to make sure this works.
Breaking Changes
Requires protocol version 8.
Checklist:
For repository code-owners and collaborators only
Summary by CodeRabbit
Release Notes
New Features
Improvements
Bug Fixes
Performance