feat(platform)!: withdrawal limits #2182
Conversation
WalkthroughThe changes encompass various updates across multiple files, primarily focusing on upgrading the Changes
Possibly related PRs
Suggested reviewers
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: 39
🧹 Outside diff range and nitpick comments (53)
packages/rs-drive/src/drive/identity/withdrawals/mod.rs (1)
4-4: New module aligns with PR objectives, but visibility and placement need attention.The addition of the
calculate_current_withdrawal_limitmodule aligns well with the PR objective of implementing withdrawal limits. However, there are a couple of points to consider:
The module is not declared as
pub. Is this intentional? If the calculation logic needs to be accessed from outside this module, consider making it public.The placement of the new module declaration between
documentandpathsseems arbitrary. Consider grouping related modules together or following a consistent ordering (e.g., alphabetical) for better readability.Consider applying the following changes:
/// Functions related to withdrawal documents pub mod document; -mod calculate_current_withdrawal_limit; /// Functions and constants related to GroveDB paths pub mod paths; /// Functions related to withdrawal transactions pub mod transaction; + +/// Functions for calculating the current withdrawal limit +mod calculate_current_withdrawal_limit;This change improves readability by grouping public modules together and adding a doc comment for the new module.
packages/rs-platform-version/src/version/limits.rs (2)
8-9: New withdrawal limit fields look good, but consider adding documentation.The addition of
max_withdrawal_amountanddaily_withdrawal_limitfields aligns well with the PR objective of implementing withdrawal limits. The types chosen (u64andOption<u64>) are appropriate for currency amounts and provide flexibility.Consider adding documentation comments to these new fields to explain their purpose, units (e.g., smallest currency unit), and any specific behaviors (e.g., what happens when limits are reached). This would improve the struct's self-documentation. For example:
/// Maximum amount allowed for a single withdrawal transaction, in the smallest currency unit. pub max_withdrawal_amount: u64, /// Optional daily withdrawal limit, in the smallest currency unit. /// If Some, represents the maximum total amount that can be withdrawn in a 24-hour period. /// If None, no daily limit is enforced. pub daily_withdrawal_limit: Option<u64>,
Line range hint
1-9: Consider long-term improvements toSystemLimitsstructureWhile the current changes are appropriate, here are some suggestions for potential long-term improvements to the
SystemLimitsstruct:
Group related limits: Consider grouping related limits into sub-structs. For example, all withdrawal-related limits could be in a
WithdrawalLimitsstruct.Consistent types: Evaluate if using
u64for all numeric fields would simplify the API without significant drawbacks.Naming consistency: Align the naming convention across similar concepts. For instance, consider renaming
max_withdrawal_amounttowithdrawal_amount_limitfor consistency.Use of newtypes: For added type safety, consider using newtype patterns for different types of limits.
Here's an example of how these changes might look:
use std::num::NonZeroU64; #[derive(Clone, Debug, Default)] pub struct SystemLimits { pub contract_limits: ContractLimits, pub withdrawal_limits: WithdrawalLimits, // ... other limit groups } #[derive(Clone, Debug, Default)] pub struct ContractLimits { pub max_serialized_size: NonZeroU64, pub max_field_value_size: NonZeroU64, pub max_state_transition_size: NonZeroU64, } #[derive(Clone, Debug, Default)] pub struct WithdrawalLimits { pub max_amount: NonZeroU64, pub daily_limit: Option<NonZeroU64>, pub transactions_per_block: NonZeroU64, }This structure provides better organization, type safety, and consistency. However, implement such changes carefully and consider their impact on the entire system.
packages/rs-dpp/src/withdrawal/daily_withdrawal_limit/mod.rs (2)
6-6: LGTM: Good modular structure. Consider adding documentation.The
v0module declaration suggests a well-organized approach for version-specific implementations. This structure will facilitate easier maintenance and potential future versioning.Consider adding a brief documentation comment explaining the purpose of the
v0module and how it fits into the overall structure of the daily withdrawal limit feature.
1-18: Overall implementation aligns well with PR objectives.The implementation of the daily withdrawal limit feature is well-structured and aligns with the PR objectives. Key points:
- Modular approach allows for version-specific implementations.
- Error handling is in place for unknown versions.
- The code is extensible for future versions.
To further improve the implementation:
- Add documentation comments to explain the purpose of the module and main function.
- Consider handling edge cases like negative versions.
- Ensure comprehensive unit tests are in place (not visible in this file).
As the feature evolves, consider implementing a trait for withdrawal limit calculations. This would allow for more flexible version handling and easier testing of individual implementations.
packages/rs-drive/src/drive/identity/withdrawals/transaction/queue/add_enqueue_untied_withdrawal_transaction_operations/v0/mod.rs (2)
8-8: LGTM: Constant for day duration is correctly defined.The
DAY_IN_MSconstant is correctly defined and used. However, for improved readability, consider adding a comment explaining the calculation:-pub const DAY_IN_MS: TimestampMillis = 86_400_000; +// 24 hours * 60 minutes * 60 seconds * 1000 milliseconds +pub const DAY_IN_MS: TimestampMillis = 24 * 60 * 60 * 1000;
14-14: LGTM: Method signature updated with new parameter.The addition of the
total_sum: Creditsparameter is consistent with the PR objective of implementing withdrawal limits.Consider adding a doc comment to explain the purpose of this new parameter:
/// @param total_sum The total amount of credits to be withdrawnpackages/rs-dpp/src/withdrawal/daily_withdrawal_limit/v0/mod.rs (1)
23-31: Implementation matches documentation, consider overflow handlingThe function implementation correctly follows the rules described in the documentation. However, there's an opportunity to improve the robustness of the function by handling potential overflow situations for very large credit values.
Consider using
checked_divto handle potential overflow when calculating 10% of large credit values:pub fn daily_withdrawal_limit_v0(total_credits_in_platform: Credits) -> Credits { if total_credits_in_platform >= 1000 { - total_credits_in_platform / 10 + total_credits_in_platform.checked_div(10).unwrap_or(Credits::MAX) } else if total_credits_in_platform >= 100 { 100 } else { total_credits_in_platform } }This change ensures that if an overflow occurs during division, the function will return the maximum possible
Creditsvalue instead of potentially panicking.packages/rs-drive/src/util/grove_operations/grove_insert_if_not_exists_return_existing_element/v0/mod.rs (1)
12-29: LGTM! Consider adding more detailed documentation.The implementation of
grove_insert_if_not_exists_return_existing_element_v0looks correct and aligns with its intended functionality. The method properly handles the insertion operation and manages the optionaldrive_operations.Consider expanding the documentation to include:
- A brief explanation of what the method does.
- Descriptions of the parameters.
- An explanation of the return value (what does the
boolrepresent?).Example:
/// Inserts an element into groveDB at the specified path if the key does not already exist. /// /// # Parameters /// /// * `path` - The subtree path where the element should be inserted. /// * `key` - The key for the element. /// * `element` - The element to be inserted. /// * `transaction` - The transaction argument for the operation. /// * `drive_operations` - Optional vector to store the low-level drive operations. /// * `drive_version` - The drive version to be used. /// /// # Returns /// /// Returns `Ok(true)` if the element was successfully inserted, or `Ok(false)` if the key already existed. /// Returns `Err` if an error occurred during the operation. pub(crate) fn grove_insert_if_not_exists_return_existing_element_v0<B: AsRef<[u8]>>( // ... (rest of the method signature) ) -> Result<bool, Error> { // ... (method body) }packages/rs-drive-abci/src/execution/platform_events/protocol_upgrade/perform_events_on_first_block_of_protocol_change/v0/mod.rs (2)
15-21: Consider removing unused parameter and improving documentation.
The
_epoch_infoparameter is prefixed with an underscore, indicating it's currently unused. Consider removing it if it's not needed for future implementations.While there's a brief comment describing the method's purpose, it would be beneficial to add more detailed documentation, especially regarding:
- The significance of the protocol version check (v4)
- The purpose and impact of inserting an empty sum tree
- Any potential side effects or important considerations when calling this method
Consider adding a more comprehensive doc comment, for example:
/// Performs necessary events on the first block of a protocol change. /// /// This method is called during an epoch change to handle protocol upgrades. /// It specifically checks for an upgrade to protocol version 4 or higher, /// and initializes the withdrawal transaction sum amount tree if necessary. /// /// # Arguments /// * `transaction` - The current transaction context /// * `previous_protocol_version` - The protocol version before the upgrade /// * `platform_version` - The current platform version /// /// # Returns /// A Result indicating success or an Error if the operation fails.
1-36: Summary: Implementation aligns with PR objectives, but requires attention to details.The new method
perform_events_on_first_block_of_protocol_change_v0introduces functionality for handling protocol upgrades and initializing withdrawal-related data structures, which aligns with the PR objective of implementing withdrawal limits. However, there are several areas that require attention:
- Documentation: Enhance the method documentation to provide more context about its purpose, usage, and potential impacts.
- Unused Parameter: Consider removing the unused
_epoch_infoparameter or justify its presence for future use.- Error Handling: Improve error handling to provide more context in case of failures.
- Version Check Logic: Verify that the protocol version check (
< 4to>= 4) is the intended upgrade path, considering future protocol versions.- Testing: Ensure comprehensive testing of this new functionality, including edge cases and potential reorgs.
These improvements will enhance the code's maintainability, robustness, and alignment with best practices.
Consider the following architectural points:
- Versioning Strategy: Ensure there's a clear strategy for handling future protocol upgrades. The
v0suffix in the method name suggests this might be part of a versioning approach, but it should be consistently applied and documented.- Upgrade Process: Document the overall upgrade process, including how this method fits into the larger picture of protocol changes.
- Data Migration: If this empty sum tree insertion is part of a data migration process for the new protocol version, consider creating a separate migration module or function to handle such operations, improving separation of concerns.
packages/rs-dpp/src/errors/consensus/basic/identity/invalid_identity_credit_withdrawal_transition_amount_error.rs (2)
23-23: LGTM: Newmax_amountfield added.The addition of the
max_amountfield is consistent with the new maximum withdrawal limit functionality. Consider adding a doc comment to explain the purpose of this field.Consider adding a doc comment:
/// The maximum allowed amount for credit withdrawal pub max_amount: u64,
43-45: LGTM: Getter method added formax_amount.The new
max_amountgetter method is consistent with the existing getter methods and provides access to the new field.Consider adding a doc comment for consistency with Rust best practices:
/// Returns the maximum allowed amount for credit withdrawal pub fn max_amount(&self) -> u64 { self.max_amount }packages/rs-drive/src/drive/identity/withdrawals/transaction/queue/add_enqueue_untied_withdrawal_transaction_operations/mod.rs (2)
14-19: Approve method signature change and request documentation updateThe addition of the
total_sum: Creditsparameter to the method signature is a good enhancement. It allows the method to process the total sum of credits associated with the withdrawal transactions.Consider updating the method documentation to reflect this new parameter and its purpose.
Line range hint
1-45: Summary of changes and alignment with PR objectivesThe changes in this file implement part of the "withdrawal limits" feature mentioned in the PR objectives. The addition of the
total_sumparameter allows for processing the total sum of credits associated with withdrawal transactions, which is likely related to implementing withdrawal limits.However, there are a few points to consider:
- The PR description lacks specific details about the implementation. It would be helpful to have more information about how these changes contribute to the withdrawal limits feature.
- The PR checklist items are unchecked, including self-review and updating tests. Please ensure these tasks are completed.
- Consider adding or updating tests to cover the new
total_sumparameter functionality.To fully implement and validate the withdrawal limits feature:
- Ensure that the
add_enqueue_untied_withdrawal_transaction_operations_v0method correctly utilizes thetotal_sumparameter for enforcing withdrawal limits.- Add appropriate error handling if the total sum exceeds the allowed withdrawal limit.
- Update or add unit tests to cover various scenarios, including cases where the withdrawal limit is reached or exceeded.
- Update the documentation to clearly explain how the withdrawal limits are enforced and what happens when they are exceeded.
🧰 Tools
🪛 GitHub Check: Rust packages (drive-abci) / Linting
[warning] 5-5: unused import:
dpp::block::block_info::BlockInfo
warning: unused import:dpp::block::block_info::BlockInfo
--> packages/rs-drive/src/drive/identity/withdrawals/transaction/queue/add_enqueue_untied_withdrawal_transaction_operations/mod.rs:5:5
|
5 | use dpp::block::block_info::BlockInfo;
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
= note:#[warn(unused_imports)]on by default🪛 GitHub Check: Rust packages (drive) / Linting
[warning] 5-5: unused import:
dpp::block::block_info::BlockInfo
warning: unused import:dpp::block::block_info::BlockInfo
--> packages/rs-drive/src/drive/identity/withdrawals/transaction/queue/add_enqueue_untied_withdrawal_transaction_operations/mod.rs:5:5
|
5 | use dpp::block::block_info::BlockInfo;
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
= note:#[warn(unused_imports)]on by defaultpackages/rs-drive/src/drive/identity/withdrawals/calculate_current_withdrawal_limit/mod.rs (3)
8-8: Consider adding a brief comment for thev0module.The
v0module declaration looks good. To improve code readability, consider adding a brief comment explaining the purpose of this module, e.g., "Module containing version 0 specific implementation".
10-52: LGTM: Well-implemented function with comprehensive documentation.The
calculate_current_withdrawal_limitfunction is well-implemented and aligns with the PR objective. The documentation is thorough and follows Rust conventions. A few suggestions for improvement:
- Consider adding a
TODOcomment to remind about implementing future versions.- The error message could be more specific about the method's full path.
Consider applying these minor improvements:
pub fn calculate_current_withdrawal_limit( &self, transaction: TransactionArg, platform_version: &PlatformVersion, ) -> Result<Credits, Error> { match platform_version .drive .methods .identity .withdrawals .calculate_current_withdrawal_limit { 0 => self.calculate_current_withdrawal_limit_v0(transaction, platform_version), + // TODO: Implement support for future versions version => Err(Error::Drive(DriveError::UnknownVersionMismatch { - method: "calculate_current_withdrawal_limit".to_string(), + method: "Drive::calculate_current_withdrawal_limit".to_string(), known_versions: vec![0], received: version, })), } }
1-53: Overall: Well-implemented feature with good design for future extensibility.The implementation of the
calculate_current_withdrawal_limitfunction aligns well with the PR objective of implementing withdrawal limits. The code is well-structured, properly documented, and follows Rust best practices. The version-aware design allows for easy extension to support future platform versions.However, there are a few points to consider:
- The PR description lacks details about the specific problem being solved and the exact nature of the changes. It would be beneficial to update the PR description with this information.
- The
v0module's implementation is not visible in this file. Ensure that the version-specific logic inv0/mod.rsis also reviewed thoroughly.- Consider adding unit tests for this new functionality to ensure its correctness and prevent future regressions.
To improve the overall quality of this PR:
- Update the PR description with more details about the problem and solution.
- Add unit tests for the
calculate_current_withdrawal_limitfunction.- Ensure that the
v0module implementation is also included in this PR for review.packages/rs-drive-abci/src/execution/platform_events/withdrawals/build_untied_withdrawal_transactions_from_documents/mod.rs (2)
Line range hint
22-36: Update function documentation to reflect new return typeThe function's documentation needs to be updated to reflect the change in the return type. It currently states that the function returns
Result<Vec<WithdrawalTransactionIndexAndBytes>, Error>, but it now returnsResult<(Vec<WithdrawalTransactionIndexAndBytes>, Credits), Error>.Please update the documentation to include information about the
Creditsvalue in the return type and its significance.
Line range hint
1-58: Summary of changes and request for more contextThe changes in this file appear to be setting up the groundwork for implementing withdrawal limits. The addition of the
Creditsimport and its inclusion in the function return type suggest that credits will play a role in enforcing these limits. However, the actual implementation details are not visible in this file.To better understand and review these changes:
- Please provide an overview of how the withdrawal limits are being implemented across the system.
- Explain how the
Creditsvalue is being used to enforce these limits.- If there are related changes in other files, please include them in the review or provide references to them.
This additional context will help ensure that the changes are correctly implemented and align with the overall architecture of the system.
packages/rs-drive/src/util/grove_operations/grove_insert_if_not_exists_return_existing_element/mod.rs (2)
37-51: Consider future-proofing the version-specific implementation.The current implementation only handles version 0, which might limit future extensibility. Consider implementing a more flexible version handling mechanism that allows for easy addition of new versions without modifying this core logic.
Here's a suggestion for a more extensible approach:
match drive_version.grove_methods.basic.grove_insert_if_not_exists { 0 => self.grove_insert_if_not_exists_return_existing_element_v0( path, key, element, transaction, drive_operations, drive_version, ), 1 => self.grove_insert_if_not_exists_return_existing_element_v1( path, key, element, transaction, drive_operations, drive_version, ), version => Err(Error::Drive(DriveError::UnknownVersionMismatch { method: "grove_insert_if_not_exists_return_existing_element".to_string(), known_versions: vec![0, 1], received: version, })), }This structure allows for easy addition of new versions (like v1) in the future.
35-35: Consider renamingdrive_versionparameter for clarity.The parameter
drive_versionmight be confused with a version of the entire drive system. Since it's specifically used for selecting the correct function version, consider renaming it tomethod_versionoroperation_versionfor clarity.Apply this diff to rename the parameter:
- drive_version: &DriveVersion, + method_version: &DriveVersion,Remember to update all occurrences of
drive_versiontomethod_versionwithin the method body as well.packages/rs-drive/src/util/grove_operations/grove_get_sum_tree_total_value/v0/mod.rs (3)
Line range hint
31-33: Improve error messages for better debugging.The error messages in the
DriveError::CorruptedCodeExecutionandDriveError::CorruptedBalancePathcould be more descriptive to aid in debugging.Consider providing more context in the error messages. For example:
- "can not query a non tree" + "Attempted to query a non-tree element in grove_get_sum_tree_total_value_v0" - "balance path does not refer to a sum tree" + "Expected a sum tree at the balance path in grove_get_sum_tree_total_value_v0, but found a different element type"Also applies to: 55-57
Line range hint
49-50: Consider using the?operator for better readability.The use of
map_errcould be replaced with the?operator for better readability and consistency with Rust's error handling idioms.Consider refactoring the error handling as follows:
- let element = value.map_err(Error::GroveDB)?; + let element = value?;This change assumes that
Error::GroveDBis the default error type for the?operator in this context. If it's not, you may need to usevalue.map_err(Error::GroveDB)?or define aFromimplementation for the error type.
Line range hint
1-61: Summary of review findings
- The visibility change from
pub(crate)topub(super)is the most significant modification. Ensure this aligns with the intended module organization.- The StatelessDirectQuery logic always returns 0, which may be incorrect.
- Error messages could be more descriptive to aid debugging.
- Minor refactoring suggestions for error handling have been provided.
Please address these points and consider adding tests to verify the correctness of the
grove_get_sum_tree_total_value_v0function, especially for the StatelessDirectQuery case.Additionally, given that this function is part of a versioned module (v0), consider documenting any changes in behavior or API if this represents a new version of an existing function.
packages/rs-dpp/src/identity/core_script.rs (1)
54-62: LGTM, but documentation is needed.The implementation of
new_p2shis correct for creating a standard P2SH script. However, please add documentation to explain the purpose and usage of this method.Consider adding a doc comment like this:
/// Creates a new Pay-to-Script-Hash (P2SH) script. /// /// # Arguments /// /// * `script_hash` - A 20-byte array representing the script hash. /// /// # Returns /// /// A new `CoreScript` instance containing the P2SH script. pub fn new_p2sh(script_hash: [u8; 20]) -> Self { // ... (existing implementation) }packages/rs-platform-version/src/version/dpp_versions.rs (1)
274-274: Add documentation for the newdaily_withdrawal_limitfield.The addition of the
daily_withdrawal_limitfield is consistent with the PR objectives. However, to improve code maintainability and clarity, consider adding a doc comment explaining the purpose and usage of this new field. This will help other developers understand its role in the system.Consider adding a doc comment above the new field, like this:
/// Represents the version of the daily withdrawal limit feature. /// This allows for versioning and gradual rollout of withdrawal limit functionality. pub daily_withdrawal_limit: FeatureVersion,packages/rs-platform-version/src/version/drive_abci_versions.rs (1)
341-341: Approved: New field added for protocol change events. Consider documentation and ordering.The addition of
perform_events_on_first_block_of_protocol_changeas anOptionalFeatureVersionis a good enhancement to handle specific events during protocol changes. However, there are a few suggestions to improve this change:
- Add documentation for the new field to explain its purpose and usage.
- Consider reordering the fields to maintain a logical grouping. For example, you might want to place this new field right after
upgrade_protocol_version_on_epoch_changefor better readability.Here's a suggested revision with added documentation and reordered fields:
#[derive(Clone, Debug, Default)] pub struct DriveAbciProtocolUpgradeMethodVersions { pub check_for_desired_protocol_upgrade: FeatureVersion, pub upgrade_protocol_version_on_epoch_change: FeatureVersion, /// Determines whether to perform specific events on the first block of a protocol change pub perform_events_on_first_block_of_protocol_change: OptionalFeatureVersion, pub protocol_version_upgrade_percentage_needed: u64, }packages/rs-platform-version/src/version/drive_versions.rs (1)
458-458: LGTM: New field addition is consistent and descriptive.The new field
batch_insert_sum_item_or_add_to_if_already_existsin theDriveGroveBatchMethodVersionsstruct is a good addition. It follows the existing naming convention and clearly describes the functionality it represents.While the name is descriptive, it's quite long. If you find that it becomes cumbersome in the future, consider shortening it while maintaining clarity.
packages/rs-platform-version/src/version/v2.rs (7)
479-481: LGTM. Consider adding documentation for the new field.The addition of
calculate_current_withdrawal_limittoDriveIdentityWithdrawalMethodVersionsis appropriate. This new field likely represents a method for calculating the current withdrawal limit, which aligns with the PR objective of implementing withdrawal limits.Consider adding a comment explaining the purpose of this field and how it relates to the withdrawal limit feature.
550-551: LGTM. Consider adding documentation for the new method.The addition of
grove_insert_if_not_exists_return_existing_elementtoDriveGroveBasicMethodVersionsis a good extension of the existing functionality. This new method likely provides more flexibility in handling insertion operations.Consider adding a comment explaining the purpose of this method and how it differs from the existing
grove_insert_if_not_existsmethod.
572-573: LGTM. Consider adding documentation for the new method.The addition of
batch_insert_sum_item_or_add_to_if_already_existstoDriveGroveBatchMethodVersionsis a valuable addition for batch operations involving sum items. This method likely provides an efficient way to update or insert sum values in a single operation.Consider adding a comment explaining the purpose of this method and how it handles the sum item insertion or addition.
578-579: LGTM. Consider adding documentation for the new method.The addition of
batch_delete_items_in_path_querytoDriveGroveBatchMethodVersionsis a useful feature for batch deletion operations. This method likely provides an efficient way to delete multiple items that match a specific path query.Consider adding a comment explaining the purpose of this method and how it handles the batch deletion based on path queries.
642-643: LGTM. Consider adding documentation for the new optional method.The addition of
perform_events_on_first_block_of_protocol_changetoDriveAbciProtocolUpgradeMethodVersionsis a good preparation for handling protocol changes. TheNoneinitialization suggests that this feature is not yet implemented or is conditionally available.Consider adding a comment explaining the purpose of this optional method and under what conditions it would be set to
Some(value)instead ofNone.
1290-1291: LGTM. Consider adding documentation for the new withdrawal limit fields.The addition of
max_withdrawal_amountanddaily_withdrawal_limittoSystemLimitsaligns well with the PR objective of implementing withdrawal limits. Themax_withdrawal_amountsets a clear upper bound for individual withdrawals, while thedaily_withdrawal_limitbeingNoneallows for future implementation of daily limits.Consider adding comments explaining:
- The rationale behind the chosen
max_withdrawal_amountvalue.- The implications of
daily_withdrawal_limitbeingNoneand under what circumstances it might be set toSome(value)in the future.
Line range hint
479-1291: Overall, the changes provide a solid foundation for implementing withdrawal limits.The additions to this file align well with the PR objective of implementing withdrawal limits. Key changes include:
- New methods for calculating and managing withdrawal limits.
- Additional Grove methods for efficient data operations.
- A new optional method for handling protocol changes.
- System limits for maximum withdrawal amounts and daily withdrawal limits.
These changes provide the necessary infrastructure to implement and manage withdrawal limits effectively. However, to improve maintainability and clarity, consider adding documentation for each new field and method, explaining their purpose and usage.
As you continue to develop this feature:
- Ensure that the
calculate_current_withdrawal_limitmethod is implemented to work correctly with themax_withdrawal_amountanddaily_withdrawal_limitfields.- Consider implementing a mechanism to update the
daily_withdrawal_limitfromNoneto a specific value when needed.- Develop comprehensive tests for these new methods and limits to ensure they function as expected in various scenarios.
packages/rs-drive/src/util/grove_operations/batch_delete_items_in_path_query/mod.rs (3)
12-12: Remove unused importSubtreePathThe import
grovedb_path::SubtreePathon line 12 is not used in the code and can be safely removed to clean up imports.🧰 Tools
🪛 GitHub Check: Rust packages (drive-abci) / Linting
[warning] 12-12: unused import:
grovedb_path::SubtreePath
warning: unused import:grovedb_path::SubtreePath
--> packages/rs-drive/src/util/grove_operations/batch_delete_items_in_path_query/mod.rs:12:5
|
12 | use grovedb_path::SubtreePath;
| ^^^^^^^^^^^^^^^^^^^^^^^^^🪛 GitHub Check: Rust packages (drive) / Linting
[warning] 12-12: unused import:
grovedb_path::SubtreePath
warning: unused import:grovedb_path::SubtreePath
--> packages/rs-drive/src/util/grove_operations/batch_delete_items_in_path_query/mod.rs:12:5
|
12 | use grovedb_path::SubtreePath;
| ^^^^^^^^^^^^^^^^^^^^^^^^^
51-51: Avoid hardcoding method names in error messagesTo prevent discrepancies if the method name changes, consider using a constant or the
stringify!macro to represent the method name instead of a hardcoded string.
19-21: Clarify parameter description forerror_if_intermediate_path_tree_not_presentThe current description can be rephrased for clarity. Consider:
- "Specifies whether the function should return an error (
true) or do nothing (false) if an intermediate path tree is not present."packages/rs-drive/src/drive/identity/withdrawals/calculate_current_withdrawal_limit/v0/mod.rs (1)
49-85: Unuseddrive_operationsvariableThe
drive_operationsvector is initialized and passed as a mutable reference to several methods, but it's not utilized afterward in this function. Ifdrive_operationsis intended to collect operations for later execution, consider handling or returning it appropriately. If it's not necessary in this context, you might remove it to simplify the code.packages/rs-drive-abci/src/execution/engine/run_block_proposal/mod.rs (2)
94-107: Ensure unit tests cover the protocol version change logicThe new logic for handling protocol version updates introduces important execution paths that should be thoroughly tested. It's crucial to ensure that unit tests cover:
- The scenario when
old_protocol_version != next_protocol_version- Successful execution of
perform_events_on_first_block_of_protocol_change- Proper update of
block_platform_statewith the new protocol version- Error handling if
perform_events_on_first_block_of_protocol_changereturns an errorWould you like assistance in creating unit tests for these scenarios, or should we open a new GitHub issue to track this task?
96-107: Consider adding logging for protocol version updatesAdding a log statement when the protocol version changes can aid in monitoring and debugging. It provides visibility into protocol upgrades during block processing.
You might add a log statement after updating the
block_platform_state:block_platform_state .set_current_protocol_version_in_consensus(next_protocol_version); + log::info!( + "Protocol version updated from {} to {}", + old_protocol_version, + next_protocol_version + ); // This is for events like adding stuff to the root tree, or making structural changes/fixes self.perform_events_on_first_block_of_protocol_change(Ensure that this complies with the project's logging framework and policies.
packages/rs-drive/src/util/grove_operations/batch_insert_if_not_exists_return_existing_element/v0/mod.rs (1)
31-140: Consider adding unit tests for the new functionTo ensure the correctness of
batch_insert_if_not_exists_return_existing_element_v0and prevent future regressions, consider adding unit tests covering various scenarios.Would you like me to help draft some unit tests or open a GitHub issue to track this task?
packages/rs-drive/src/util/batch/drive_op_batch/withdrawals.rs (2)
106-129: Add unit tests forReserveWithdrawalAmountoperationThe
ReserveWithdrawalAmountoperation introduces new logic that should be covered by unit tests to ensure correctness and prevent regressions.Do you want me to assist in creating unit tests for this operation or open a GitHub issue to track this task?
130-153: Add unit tests forFreeUpReservedWithdrawalAmountsForTimeoperationThe
FreeUpReservedWithdrawalAmountsForTimeoperation adds new functionality that should be tested to verify correct behavior and prevent future regressions.Do you want me to help generate unit tests for this operation or open a GitHub issue to track this task?
packages/rs-drive-abci/src/execution/platform_events/withdrawals/build_untied_withdrawal_transactions_from_documents/v0/mod.rs (2)
Line range hint
65-68: Include the underlying error when consensus encoding failsIf
consensus_encodefails, the current error message does not include the underlying error, which may hinder debugging. Consider modifying the error handling to include the original error information.You can modify the
map_errclosure to include the error:-withdrawal_transaction - .consensus_encode(&mut transaction_buffer) - .map_err(|_| { - Error::Execution(ExecutionError::CorruptedCodeExecution( - "Can't consensus encode a withdrawal transaction", - )) - })?; +withdrawal_transaction + .consensus_encode(&mut transaction_buffer) + .map_err(|e| { + Error::Execution(ExecutionError::ConsensusEncodingError(format!( + "Failed to consensus encode withdrawal transaction: {}", + e + ))) + })?;
Line range hint
187-218: Add test cases to verify overflow handlingThe function includes overflow checks when summing the withdrawal amounts. Consider adding test cases that simulate an overflow condition to ensure that the error handling works as expected.
packages/rs-drive/src/state_transition_action/identity/identity_credit_withdrawal/v0/transformer.rs (1)
144-153: Consider returning a more specific error for unsupported key typesWhen an unsupported key type is encountered, the code returns a
NoTransferKeyForCoreWithdrawalAvailableError. This might be misleading since a transfer key exists but its key type is unsupported for withdrawals. Consider returning a more specific error, such asUnsupportedKeyTypeForWithdrawalError, to more accurately convey the issue.packages/rs-drive-abci/src/execution/platform_events/withdrawals/pool_withdrawals_into_transactions_queue/v0/mod.rs (2)
Line range hint
134-142: Improve error handling when fetching document typeThe current error handling may obscure the underlying issue. Including the original error provides more context for debugging.
Apply this diff to include the original error:
- .map_err(|_| { + .map_err(|e| { Error::Execution(ExecutionError::CorruptedCodeExecution( - "Can't fetch withdrawal data contract", + format!("Can't fetch withdrawal data contract: {}", e), )) })
4-5: Confirm necessity of added importsEnsure that the newly added imports
DocumentV0GettersandBTreeValueMapHelperare necessary. Unused imports can be removed to keep the codebase clean.packages/rs-drive/src/util/grove_operations/batch_insert_sum_item_or_add_to_if_already_exists/v0/mod.rs (1)
177-218: Ensure consistent error messages for unsupported operationsIn the
PathKeyElementSize(lines 177-214) andPathKeyUnknownElementSize(lines 215-218) variants, the error messages for unsupported operations are slightly different:
- Lines 204-207 return an error with the message:
"document sizes for stateful insert in batch operations not supported".- Lines 215-217 return an error with the message:
"document sizes in batch operations not supported".To improve clarity and maintain consistency across the codebase, consider unifying these error messages.
packages/rs-platform-version/src/version/v1.rs (2)
479-479: Ensure Consistent Placement ofcalculate_current_withdrawal_limitThe
calculate_current_withdrawal_limit: 0,method version is added underDriveIdentityWithdrawalMethodVersionsat line 479. Please verify that this method is appropriately placed within thewithdrawalsmodule to maintain logical grouping and consistency with other withdrawal-related methods.
578-578: Inconsistent Naming ConventionThe method
batch_delete_items_in_path_queryat line 578 may not align with the existing naming conventions in the codebase. To maintain consistency, consider renaming it tobatch_delete_items_by_path_queryorbatch_delete_by_query_path.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (50)
- packages/rs-dpp/src/errors/consensus/basic/identity/invalid_identity_credit_withdrawal_transition_amount_error.rs (3 hunks)
- packages/rs-dpp/src/identity/core_script.rs (1 hunks)
- packages/rs-dpp/src/withdrawal/daily_withdrawal_limit/mod.rs (1 hunks)
- packages/rs-dpp/src/withdrawal/daily_withdrawal_limit/v0/mod.rs (1 hunks)
- packages/rs-dpp/src/withdrawal/mod.rs (2 hunks)
- packages/rs-drive-abci/src/execution/engine/initialization/init_chain/v0/mod.rs (1 hunks)
- packages/rs-drive-abci/src/execution/engine/run_block_proposal/mod.rs (1 hunks)
- packages/rs-drive-abci/src/execution/platform_events/protocol_upgrade/mod.rs (1 hunks)
- packages/rs-drive-abci/src/execution/platform_events/protocol_upgrade/perform_events_on_first_block_of_protocol_change/mod.rs (1 hunks)
- packages/rs-drive-abci/src/execution/platform_events/protocol_upgrade/perform_events_on_first_block_of_protocol_change/v0/mod.rs (1 hunks)
- packages/rs-drive-abci/src/execution/platform_events/withdrawals/append_signatures_and_broadcast_withdrawal_transactions/v0/mod.rs (1 hunks)
- packages/rs-drive-abci/src/execution/platform_events/withdrawals/build_untied_withdrawal_transactions_from_documents/mod.rs (2 hunks)
- packages/rs-drive-abci/src/execution/platform_events/withdrawals/build_untied_withdrawal_transactions_from_documents/v0/mod.rs (5 hunks)
- packages/rs-drive-abci/src/execution/platform_events/withdrawals/pool_withdrawals_into_transactions_queue/v0/mod.rs (6 hunks)
- packages/rs-drive-abci/src/execution/platform_events/withdrawals/pool_withdrawals_into_transactions_queue/v1/mod.rs (6 hunks)
- packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/identity_credit_withdrawal/mod.rs (1 hunks)
- packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/identity_credit_withdrawal/structure/v1/mod.rs (1 hunks)
- packages/rs-drive-abci/src/platform_types/platform_state/mod.rs (0 hunks)
- packages/rs-drive/src/drive/identity/withdrawals/calculate_current_withdrawal_limit/mod.rs (1 hunks)
- packages/rs-drive/src/drive/identity/withdrawals/calculate_current_withdrawal_limit/v0/mod.rs (1 hunks)
- packages/rs-drive/src/drive/identity/withdrawals/mod.rs (1 hunks)
- packages/rs-drive/src/drive/identity/withdrawals/paths.rs (3 hunks)
- packages/rs-drive/src/drive/identity/withdrawals/transaction/queue/add_enqueue_untied_withdrawal_transaction_operations/mod.rs (3 hunks)
- packages/rs-drive/src/drive/identity/withdrawals/transaction/queue/add_enqueue_untied_withdrawal_transaction_operations/v0/mod.rs (2 hunks)
- packages/rs-drive/src/drive/identity/withdrawals/transaction/queue/mod.rs (2 hunks)
- packages/rs-drive/src/drive/initialization/v0/mod.rs (1 hunks)
- packages/rs-drive/src/state_transition_action/identity/identity_credit_withdrawal/v0/transformer.rs (2 hunks)
- packages/rs-drive/src/util/batch/drive_op_batch/withdrawals.rs (5 hunks)
- packages/rs-drive/src/util/grove_operations/batch_delete_items_in_path_query/mod.rs (1 hunks)
- packages/rs-drive/src/util/grove_operations/batch_delete_items_in_path_query/v0/mod.rs (1 hunks)
- packages/rs-drive/src/util/grove_operations/batch_insert_if_not_exists/mod.rs (1 hunks)
- packages/rs-drive/src/util/grove_operations/batch_insert_if_not_exists/v0/mod.rs (1 hunks)
- packages/rs-drive/src/util/grove_operations/batch_insert_if_not_exists_return_existing_element/mod.rs (1 hunks)
- packages/rs-drive/src/util/grove_operations/batch_insert_if_not_exists_return_existing_element/v0/mod.rs (1 hunks)
- packages/rs-drive/src/util/grove_operations/batch_insert_sum_item_or_add_to_if_already_exists/mod.rs (1 hunks)
- packages/rs-drive/src/util/grove_operations/batch_insert_sum_item_or_add_to_if_already_exists/v0/mod.rs (1 hunks)
- packages/rs-drive/src/util/grove_operations/grove_get_sum_tree_total_value/v0/mod.rs (1 hunks)
- packages/rs-drive/src/util/grove_operations/grove_insert_if_not_exists_return_existing_element/mod.rs (1 hunks)
- packages/rs-drive/src/util/grove_operations/grove_insert_if_not_exists_return_existing_element/v0/mod.rs (1 hunks)
- packages/rs-drive/src/util/grove_operations/mod.rs (1 hunks)
- packages/rs-platform-version/src/version/dpp_versions.rs (1 hunks)
- packages/rs-platform-version/src/version/drive_abci_versions.rs (1 hunks)
- packages/rs-platform-version/src/version/drive_versions.rs (3 hunks)
- packages/rs-platform-version/src/version/limits.rs (1 hunks)
- packages/rs-platform-version/src/version/mocks/v2_test.rs (6 hunks)
- packages/rs-platform-version/src/version/mocks/v3_test.rs (6 hunks)
- packages/rs-platform-version/src/version/v1.rs (6 hunks)
- packages/rs-platform-version/src/version/v2.rs (6 hunks)
- packages/rs-platform-version/src/version/v3.rs (6 hunks)
- packages/rs-platform-version/src/version/v4.rs (6 hunks)
💤 Files not reviewed due to no reviewable changes (1)
- packages/rs-drive-abci/src/platform_types/platform_state/mod.rs
✅ Files skipped from review due to trivial changes (1)
- packages/rs-drive-abci/src/execution/platform_events/withdrawals/append_signatures_and_broadcast_withdrawal_transactions/v0/mod.rs
🧰 Additional context used
🪛 GitHub Check: Rust packages (wasm-dpp) / Linting
packages/rs-dpp/src/withdrawal/daily_withdrawal_limit/v0/mod.rs
[warning] 2-2: unused import:
crate::ProtocolError
warning: unused import:crate::ProtocolError
--> packages/rs-dpp/src/withdrawal/daily_withdrawal_limit/v0/mod.rs:2:5
|
2 | use crate::ProtocolError;
| ^^^^^^^^^^^^^^^^^^^^
|
= note:#[warn(unused_imports)]on by default
🪛 GitHub Check: Rust packages (drive-abci) / Linting
packages/rs-dpp/src/withdrawal/daily_withdrawal_limit/v0/mod.rs
[warning] 2-2: unused import:
crate::ProtocolError
warning: unused import:crate::ProtocolError
--> packages/rs-dpp/src/withdrawal/daily_withdrawal_limit/v0/mod.rs:2:5
|
2 | use crate::ProtocolError;
| ^^^^^^^^^^^^^^^^^^^^
|
= note:#[warn(unused_imports)]on by defaultpackages/rs-drive/src/drive/identity/withdrawals/transaction/queue/add_enqueue_untied_withdrawal_transaction_operations/mod.rs
[warning] 5-5: unused import:
dpp::block::block_info::BlockInfo
warning: unused import:dpp::block::block_info::BlockInfo
--> packages/rs-drive/src/drive/identity/withdrawals/transaction/queue/add_enqueue_untied_withdrawal_transaction_operations/mod.rs:5:5
|
5 | use dpp::block::block_info::BlockInfo;
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
= note:#[warn(unused_imports)]on by defaultpackages/rs-drive/src/util/batch/drive_op_batch/withdrawals.rs
[warning] 5-5: unused import:
get_withdrawal_transactions_sum_tree_path
warning: unused import:get_withdrawal_transactions_sum_tree_path
--> packages/rs-drive/src/util/batch/drive_op_batch/withdrawals.rs:5:49
|
5 | get_withdrawal_transactions_queue_path_vec, get_withdrawal_transactions_sum_tree_path,
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
[warning] 19-19: unused import:
WithdrawalTransactionIndexAndBytesAndAmounts
warning: unused import:WithdrawalTransactionIndexAndBytesAndAmounts
--> packages/rs-drive/src/util/batch/drive_op_batch/withdrawals.rs:19:5
|
19 | WithdrawalTransactionIndexAndBytesAndAmounts,
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^packages/rs-drive/src/util/grove_operations/batch_delete_items_in_path_query/mod.rs
[warning] 12-12: unused import:
grovedb_path::SubtreePath
warning: unused import:grovedb_path::SubtreePath
--> packages/rs-drive/src/util/grove_operations/batch_delete_items_in_path_query/mod.rs:12:5
|
12 | use grovedb_path::SubtreePath;
| ^^^^^^^^^^^^^^^^^^^^^^^^^packages/rs-drive/src/util/grove_operations/batch_delete_items_in_path_query/v0/mod.rs
[warning] 9-9: unused imports:
QueryResultElementandQueryResultElements
warning: unused imports:QueryResultElementandQueryResultElements
--> packages/rs-drive/src/util/grove_operations/batch_delete_items_in_path_query/v0/mod.rs:9:34
|
9 | use grovedb::query_result_type::{QueryResultElement, QueryResultElements};
| ^^^^^^^^^^^^^^^^^^ ^^^^^^^^^^^^^^^^^^^
🪛 GitHub Check: Rust packages (drive) / Linting
packages/rs-dpp/src/withdrawal/daily_withdrawal_limit/v0/mod.rs
[warning] 2-2: unused import:
crate::ProtocolError
warning: unused import:crate::ProtocolError
--> packages/rs-dpp/src/withdrawal/daily_withdrawal_limit/v0/mod.rs:2:5
|
2 | use crate::ProtocolError;
| ^^^^^^^^^^^^^^^^^^^^
|
= note:#[warn(unused_imports)]on by defaultpackages/rs-drive/src/drive/identity/withdrawals/transaction/queue/add_enqueue_untied_withdrawal_transaction_operations/mod.rs
[warning] 5-5: unused import:
dpp::block::block_info::BlockInfo
warning: unused import:dpp::block::block_info::BlockInfo
--> packages/rs-drive/src/drive/identity/withdrawals/transaction/queue/add_enqueue_untied_withdrawal_transaction_operations/mod.rs:5:5
|
5 | use dpp::block::block_info::BlockInfo;
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
= note:#[warn(unused_imports)]on by defaultpackages/rs-drive/src/util/batch/drive_op_batch/withdrawals.rs
[warning] 5-5: unused import:
get_withdrawal_transactions_sum_tree_path
warning: unused import:get_withdrawal_transactions_sum_tree_path
--> packages/rs-drive/src/util/batch/drive_op_batch/withdrawals.rs:5:49
|
5 | get_withdrawal_transactions_queue_path_vec, get_withdrawal_transactions_sum_tree_path,
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
[warning] 19-19: unused import:
WithdrawalTransactionIndexAndBytesAndAmounts
warning: unused import:WithdrawalTransactionIndexAndBytesAndAmounts
--> packages/rs-drive/src/util/batch/drive_op_batch/withdrawals.rs:19:5
|
19 | WithdrawalTransactionIndexAndBytesAndAmounts,
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^packages/rs-drive/src/util/grove_operations/batch_delete_items_in_path_query/mod.rs
[warning] 12-12: unused import:
grovedb_path::SubtreePath
warning: unused import:grovedb_path::SubtreePath
--> packages/rs-drive/src/util/grove_operations/batch_delete_items_in_path_query/mod.rs:12:5
|
12 | use grovedb_path::SubtreePath;
| ^^^^^^^^^^^^^^^^^^^^^^^^^packages/rs-drive/src/util/grove_operations/batch_delete_items_in_path_query/v0/mod.rs
[warning] 9-9: unused imports:
QueryResultElementandQueryResultElements
warning: unused imports:QueryResultElementandQueryResultElements
--> packages/rs-drive/src/util/grove_operations/batch_delete_items_in_path_query/v0/mod.rs:9:34
|
9 | use grovedb::query_result_type::{QueryResultElement, QueryResultElements};
| ^^^^^^^^^^^^^^^^^^ ^^^^^^^^^^^^^^^^^^^
🪛 GitHub Check: Rust packages (dpp) / Linting
packages/rs-dpp/src/withdrawal/daily_withdrawal_limit/v0/mod.rs
[warning] 2-2: unused import:
crate::ProtocolError
warning: unused import:crate::ProtocolError
--> packages/rs-dpp/src/withdrawal/daily_withdrawal_limit/v0/mod.rs:2:5
|
2 | use crate::ProtocolError;
| ^^^^^^^^^^^^^^^^^^^^
|
= note:#[warn(unused_imports)]on by default
🪛 GitHub Check: Rust packages (dash-sdk) / Linting
packages/rs-dpp/src/withdrawal/daily_withdrawal_limit/v0/mod.rs
[warning] 2-2: unused import:
crate::ProtocolError
warning: unused import:crate::ProtocolError
--> packages/rs-dpp/src/withdrawal/daily_withdrawal_limit/v0/mod.rs:2:5
|
2 | use crate::ProtocolError;
| ^^^^^^^^^^^^^^^^^^^^
|
= note:#[warn(unused_imports)]on by default
🔇 Additional comments (89)
packages/rs-drive-abci/src/execution/platform_events/protocol_upgrade/mod.rs (1)
1-3: LGTM! New module added successfully.The addition of the
perform_events_on_first_block_of_protocol_changemodule looks good. It appears to enhance the protocol upgrade functionality by handling events that occur on the first block of a protocol change.However, I noticed a discrepancy between the PR objectives and the actual changes:
The PR description mentions implementing withdrawal limits, but this change doesn't directly relate to that. Could you please update the PR description to clarify how this new module connects to the implementation of withdrawal limits? This will help reviewers better understand the context and purpose of these changes.
To verify the contents of the new module, please run the following script:
This will help us understand the implementation details of the new module and how it relates to withdrawal limits.
packages/rs-drive/src/drive/identity/withdrawals/mod.rs (1)
Line range hint
1-8: More context needed for comprehensive review of withdrawal limits feature.While the addition of the
calculate_current_withdrawal_limitmodule is a step towards implementing withdrawal limits, the changes in this file alone don't provide a complete picture of the feature. Consider the following points:
- Are there other files or changes not included in this review that are part of the withdrawal limits feature?
- The PR description lacks details about the implementation and testing procedures. It would be helpful to have more information about:
- The specific problem being solved
- The overall approach to implementing withdrawal limits
- Any new data structures or database changes
- How the new module integrates with existing code
- Testing strategy for the new feature
To get a better understanding of the feature implementation, let's check for other related changes:
This will help us identify if there are other relevant files that should be part of this review.
packages/rs-platform-version/src/version/limits.rs (1)
8-9: Request for additional context and implementation detailsWhile the addition of these fields is a good start for implementing withdrawal limits, there are several important aspects that need clarification:
- Implementation: How and where are these new limits enforced in the codebase?
- Testing: What test cases have been added to verify the correct application of these limits?
- Backwards Compatibility: If this struct is used in serialization/deserialization, how is backwards compatibility maintained?
- Limit Relationships:
- How do
max_withdrawal_amountanddaily_withdrawal_limitinteract?- What's the relationship between these new fields and the existing
withdrawal_transactions_per_block_limit?- Error Handling: How are limit violations handled in the system?
To help address some of these questions, could you provide more context about the usage of
SystemLimits? Here's a script to search for its usage:Additionally, please update the PR description with more details about the implementation, testing strategy, and any potential breaking changes.
packages/rs-dpp/src/withdrawal/daily_withdrawal_limit/mod.rs (2)
1-4: LGTM: Imports are appropriate and relevant.The imports bring in the necessary types and functions for implementing the daily withdrawal limit feature.
8-18: LGTM: Well-structured implementation with good error handling.The
daily_withdrawal_limitfunction is well-implemented with appropriate parameter types and return value. The use of pattern matching allows for easy extension to support future versions, and the error handling for unknown versions is good.Consider the following minor improvements:
- Add a brief documentation comment explaining the function's purpose and parameters.
- Consider handling negative versions explicitly, although this might be an edge case.
Example documentation:
/// Calculates the daily withdrawal limit based on the total credits in the platform and the platform version. /// /// # Arguments /// * `total_credits_in_platform` - The total credits available in the platform /// * `platform_version` - A reference to the current platform version /// /// # Returns /// A `Result` containing either the calculated `Credits` or a `ProtocolError`To ensure that this is the only place where the daily withdrawal limit is calculated, run the following script:
✅ Verification successful
Verification Successful: The
daily_withdrawal_limitfunction is uniquely implemented inmod.rs, with appropriate version-specific delegations.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that daily_withdrawal_limit is not implemented elsewhere # Test: Search for other implementations of daily_withdrawal_limit rg --type rust -g '!**/daily_withdrawal_limit/mod.rs' 'fn\s+daily_withdrawal_limit'Length of output: 231
packages/rs-dpp/src/withdrawal/mod.rs (3)
1-1: New module for daily withdrawal limit looks good, but more details needed.The addition of the
daily_withdrawal_limitmodule aligns well with the PR objective of implementing withdrawal limits. However, the implementation details are not visible in this file.Could you provide more information about the implementation of the
daily_withdrawal_limitmodule? Let's verify its contents:#!/bin/bash # Description: Check the contents of the daily_withdrawal_limit module # Test: Display the contents of the daily_withdrawal_limit.rs file cat packages/rs-dpp/src/withdrawal/daily_withdrawal_limit.rs # If the file doesn't exist, search for it if [ $? -ne 0 ]; then echo "File not found. Searching for daily_withdrawal_limit.rs:" fd -t f daily_withdrawal_limit.rs fi
25-25: New type alias looks good, but consider using a more precise type for amounts.The addition of
WithdrawalTransactionIndexAndBytesAndAmountsis a good step towards implementing withdrawal limits. However, I have a few questions and suggestions:
Why was
u64chosen for the amount field? This type might not be suitable for representing fractional amounts or very large values. Consider using a decimal type or a custom type that can handle a wider range of values and precision.How will this new type be used in the context of enforcing withdrawal limits? It would be helpful to see some usage examples or additional context.
Have you considered creating a struct instead of a tuple type? This could improve readability and make the code more self-documenting.
Let's check for any usage of this new type:
Line range hint
1-25: Overall changes look promising, but more information is needed.The changes in this file lay the groundwork for implementing withdrawal limits, which aligns with the PR objectives. However, to provide a comprehensive review, we need more information:
Could you provide an overview of how the
daily_withdrawal_limitmodule and the newWithdrawalTransactionIndexAndBytesAndAmountstype work together to enforce withdrawal limits?Are there other files changed in this PR that implement the actual limit checking logic?
What testing strategy have you employed to ensure the correct functioning of the withdrawal limits?
Have you considered any potential breaking changes this might introduce to existing systems?
Please update the PR description with this information, including details about the problem being solved, the complete set of changes implemented, and the testing procedures used.
Let's check for other related changes in the PR:
packages/rs-drive/src/drive/identity/withdrawals/transaction/queue/add_enqueue_untied_withdrawal_transaction_operations/v0/mod.rs (3)
4-5: LGTM: New imports are correctly added and utilized.The new imports for
CreditsandTimestampMillisare necessary for the changes made in this file and are properly used in the code.
Line range hint
1-32: Request for additional information in PR descriptionThe code changes look good overall and align with the PR objective of implementing withdrawal limits. However, the PR description lacks crucial information that would help reviewers better understand and validate the changes:
- Please provide more details about the specific problem being solved. What was the motivation for implementing these withdrawal limits?
- Describe any testing procedures you've implemented to ensure the correctness of these changes.
- Are there any potential breaking changes introduced by this PR? If so, please document them.
- Have you performed a self-review of your code? If so, please check the corresponding item in the PR template checklist.
Providing this additional information will greatly assist in the review process and ensure that the changes are thoroughly understood and validated.
23-28: LGTM: New ReserveWithdrawalAmount operation added.The new operation for reserving the withdrawal amount is correctly implemented and aligns with the PR objective.
However, consider adding a validation check for
total_sumto ensure it's greater than zero and matches the sum of individual withdrawal transactions. This can help prevent potential issues with incorrect input:assert!(total_sum > Credits::ZERO, "Total sum must be greater than zero"); let calculated_sum: Credits = withdrawal_transactions.iter().map(|tx| tx.amount).sum(); assert_eq!(total_sum, calculated_sum, "Total sum must match the sum of individual withdrawal transactions");To verify the usage of
Creditsand theamountfield in withdrawal transactions, please run the following script:packages/rs-dpp/src/withdrawal/daily_withdrawal_limit/v0/mod.rs (1)
4-22: Well-documented functionThe function documentation is comprehensive and follows Rust documentation best practices. It clearly explains the purpose, rules, parameters, and return value of the
daily_withdrawal_limit_v0function.packages/rs-drive/src/util/grove_operations/grove_insert_if_not_exists_return_existing_element/v0/mod.rs (1)
28-28: Verify the usage ofpush_drive_operation_result_optional.The
push_drive_operation_result_optionalfunction is used to handle the result of the grove operation. Let's verify its implementation to ensure it correctly processes thecost_contextand updatesdrive_operationsas expected.Run the following script to check the implementation of
push_drive_operation_result_optional:packages/rs-drive-abci/src/execution/platform_events/protocol_upgrade/perform_events_on_first_block_of_protocol_change/v0/mod.rs (1)
1-10: LGTM: Imports are appropriate and relevant.The imports cover all necessary dependencies for the implemented functionality, including error handling, platform types, version management, and withdrawal-related operations.
packages/rs-dpp/src/errors/consensus/basic/identity/invalid_identity_credit_withdrawal_transition_amount_error.rs (3)
13-13: LGTM: Error message updated to reflect new maximum limit.The error message has been appropriately updated to include information about the new maximum withdrawal limit. This change enhances the clarity of the error message and aligns with the new functionality.
27-32: LGTM: Constructor updated to includemax_amount.The
newmethod has been correctly updated to include themax_amountparameter and initialize the new field. This change is consistent with the struct modification.
Line range hint
13-45: Verify the impact of these changes on the codebase.The changes to
InvalidIdentityCreditWithdrawalTransitionAmountErrorseem complete and consistent. However, we should verify that all parts of the codebase that use this error have been updated to accommodate the newmax_amountfield.Run the following script to check for usages of this error type:
Please review the output to ensure all usages have been updated to include the
max_amountparameter.✅ Verification successful
Verification Complete: All usages of
InvalidIdentityCreditWithdrawalTransitionAmountErrorhave been appropriately updated to include themax_amountparameter. No issues found.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find usages of InvalidIdentityCreditWithdrawalTransitionAmountError # Search for the error type usage echo "Usages of InvalidIdentityCreditWithdrawalTransitionAmountError:" rg --type rust "InvalidIdentityCreditWithdrawalTransitionAmountError" -C 3 # Search for the error creation echo "\nCreation of InvalidIdentityCreditWithdrawalTransitionAmountError:" rg --type rust "InvalidIdentityCreditWithdrawalTransitionAmountError::new" -C 3Length of output: 12289
packages/rs-drive/src/drive/identity/withdrawals/transaction/queue/add_enqueue_untied_withdrawal_transaction_operations/mod.rs (1)
31-34: Approve implementation change and suggest internal method verificationThe integration of the
total_sumparameter into the internal method call is correct and minimal.Please ensure that the
add_enqueue_untied_withdrawal_transaction_operations_v0method correctly utilizes thetotal_sumparameter. You can verify this by running the following command:✅ Verification successful
Verified integration of
total_sumparameterThe
add_enqueue_untied_withdrawal_transaction_operations_v0method correctly utilizes thetotal_sumparameter in reserving the withdrawal amount.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check the implementation of add_enqueue_untied_withdrawal_transaction_operations_v0 # Test: Search for the method implementation ast-grep --lang rust --pattern $'fn add_enqueue_untied_withdrawal_transaction_operations_v0( $_, $_: Vec<WithdrawalTransactionIndexAndBytes>, $_: Credits, $_: &mut Vec<DriveOperation>, ) { $$$ }'Length of output: 206
Script:
#!/bin/bash # Description: Search for the implementation of add_enqueue_untied_withdrawal_transaction_operations_v0 rg 'fn add_enqueue_untied_withdrawal_transaction_operations_v0'Length of output: 270
Script:
#!/bin/bash # Description: Retrieve the full implementation of add_enqueue_untied_withdrawal_transaction_operations_v0 rg 'fn add_enqueue_untied_withdrawal_transaction_operations_v0' -A 20Length of output: 3609
packages/rs-drive/src/util/grove_operations/batch_insert_if_not_exists/mod.rs (1)
26-26: Clarification needed on return value change and PR objectivesThe change in the return value description appears to be more accurate given the method name
batch_insert_if_not_exists. However, this modification raises several concerns:
The new description ("Returns true if we were able to insert") is significantly different from the old one ("Returns true if the path key already exists without references"). This change could have implications for existing code that relies on the previous behavior.
The change doesn't seem directly related to implementing withdrawal limits as mentioned in the PR objectives. Could you please provide more context on how this change relates to the overall goal of the PR?
Given the significance of this change, it would be helpful to have more information:
- Why was this change necessary?
- Are there any corresponding changes in the method implementation?
- Have all usages of this method been updated to account for the new return value meaning?
To help verify the impact of this change, please run the following script:
This will help us understand how the method is used throughout the codebase and identify any potential areas that might need updating due to this change.
packages/rs-drive/src/drive/identity/withdrawals/calculate_current_withdrawal_limit/mod.rs (1)
1-6: LGTM: Imports are well-organized and relevant.The imports are logically organized and include all necessary components for the implementation. No unused imports are present.
packages/rs-drive-abci/src/execution/platform_events/withdrawals/build_untied_withdrawal_transactions_from_documents/mod.rs (4)
7-7: LGTM: Addition of Credits importThe import of
Creditsfromdpp::feeis appropriate given the changes in the function signature. This import will allow the use of theCreditstype in the function return value.
9-12: LGTM: Improved import structure for withdrawal typesThe change to a multi-line import for withdrawal-related types improves code readability. The addition of
WithdrawalTransactionIndexAndBytesAndAmountsto the imports suggests it might be used in the implementation, which aligns with the changes related to withdrawal limits.
42-42: Function signature updated to include CreditsThe change in the function signature to return a tuple that includes
Creditsaligns with the PR objective of implementing withdrawal limits. This modification allows the function to return information about the credits associated with the withdrawal transactions, which could be used to enforce limits.
42-42: Verify implementation of withdrawal limitsWhile the function signature change suggests progress towards implementing withdrawal limits, the actual implementation details are not visible in this file.
Could you provide more information on how the withdrawal limits are being enforced using the
Creditsvalue? Additionally, please run the following script to check for related changes in other files:packages/rs-drive/src/util/grove_operations/grove_get_sum_tree_total_value/v0/mod.rs (2)
Line range hint
24-39: Verify the correctness of StatelessDirectQuery logic.The StatelessDirectQuery branch always returns
Ok(0)after pushing the calculated cost todrive_operations. This seems suspicious and might not be the intended behavior.Please confirm if returning 0 for all StatelessDirectQuery cases is correct. If not, consider implementing the proper logic to return the actual sum tree total value.
17-17: Visibility change may impact module organization.The visibility of
grove_get_sum_tree_total_value_v0has been changed frompub(crate)topub(super). This restricts the method's accessibility to only the parent module, which could affect how it's used in other parts of the crate.Please ensure that this change aligns with the intended module organization and doesn't break any existing code that might have been using this method from other modules within the crate.
To verify the impact of this change, run the following script:
✅ Verification successful
Visibility change does not impact existing usages within the parent module.
The change from
pub(crate)topub(super)restrictsgrove_get_sum_tree_total_value_v0to the parent modulegrove_get_sum_tree_total_value/v0. The only usage found is withingrove_get_sum_tree_total_value/mod.rs, which is permitted underpub(super)visibility. No issues detected.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any usages of grove_get_sum_tree_total_value_v0 outside its parent module # Find the parent module parent_module=$(dirname "$(fd -p -t f 'grove_get_sum_tree_total_value/v0/mod.rs')") # Search for usages of the function outside the parent module rg --type rust -g '!*/grove_get_sum_tree_total_value/v0/mod.rs' -g "!$parent_module/*" 'grove_get_sum_tree_total_value_v0' # If there are any matches, they might be affected by the visibility changeLength of output: 570
packages/rs-drive/src/drive/identity/withdrawals/transaction/queue/mod.rs (3)
43-43: Please clarify the purpose of the new argument.An additional argument
100has been added to theadd_enqueue_untied_withdrawal_transaction_operationsfunction call. It's not clear what this value represents or how it affects the test.Could you please provide more context on this change? Consider adding a comment to explain the purpose of this argument.
To help understand the change, let's check the function definition:
#!/bin/bash # Description: Find the definition of add_enqueue_untied_withdrawal_transaction_operations # Test: Search for the function definition ast-grep --lang rust --pattern 'fn add_enqueue_untied_withdrawal_transaction_operations($$$)'
Line range hint
1-124: Please provide more context on how these changes implement withdrawal limits.The changes in this file are minimal but potentially impactful. While the new import is clear, the modification to the test function lacks explanation. Given that the PR objectives mention implementing withdrawal limits, it would be helpful to understand how these changes contribute to that goal.
Could you please elaborate on how these modifications relate to the implementation of withdrawal limits? This context would greatly assist in reviewing the changes effectively.
To gather more information about the withdrawal limits implementation, let's search for related changes:
#!/bin/bash # Description: Search for withdrawal limit related changes # Test: Search for withdrawal limit related terms rg --type rust "withdrawal.?limit"
14-17: LGTM: New import added correctly.The new import
WithdrawalTransactionIndexAndBytesAndAmountsis consistent with the existing imports and follows the established naming convention.To ensure the new import is being used correctly, let's verify its usage in the codebase:
packages/rs-drive/src/util/grove_operations/batch_insert_if_not_exists/v0/mod.rs (1)
18-18: Approve documentation correction with cautionThe updated comment accurately reflects the function's behavior, which returns
trueif an insertion occurred. This change improves the documentation's accuracy.However, this significant change in the interpretation of the return value may impact existing code that relies on the previous behavior.
To ensure consistency and prevent potential issues:
- Update the function name to clearly indicate its behavior, e.g.,
batch_insert_if_not_exists_and_return_insertion_status_v0.- Add a deprecation notice for the old behavior if applicable.
- Update all call sites to handle the new return value correctly.
Please run the following script to identify potential affected areas:
Review the results to ensure all usages are updated accordingly.
packages/rs-drive-abci/src/execution/engine/initialization/init_chain/v0/mod.rs (2)
16-16: Verify the removal ofValidatorSetUpdateimport.The
ValidatorSetUpdateimport has been removed. This change doesn't seem directly related to the PR objectives of implementing withdrawal limits.Let's verify if
ValidatorSetUpdateis still used in the file:#!/bin/bash # Description: Check if ValidatorSetUpdate is used in the file # Test: Search for ValidatorSetUpdate usage rg --type rust "ValidatorSetUpdate" "packages/rs-drive-abci/src/execution/engine/initialization/init_chain/v0/mod.rs"
16-16: Potential implicit usage of ValidatorSetUpdateWhile the
ValidatorSetUpdateimport has been removed and the type is not directly used in this file, thevalidator_set_updatefield is still present in theResponseInitChainstruct on line 153. This suggests that the type might be implicitly used or defined elsewhere.Let's verify if
ValidatorSetUpdateis defined or used in other files:✅ Verification successful
ValidatorSetUpdate Import Removal Verified
The
ValidatorSetUpdateimport has been successfully removed from the file, and there are no usages ofValidatorSetUpdatewithin the codebase. This change does not affect the functionality or integrity of the project.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if ValidatorSetUpdate is defined or used in other files # Test: Search for ValidatorSetUpdate definition or usage in the entire project rg --type rust "ValidatorSetUpdate"Length of output: 115
packages/rs-dpp/src/identity/core_script.rs (1)
54-62: Verify the relevance of this change to the PR objectives.The PR objectives mention implementing withdrawal limits, but this new method for creating P2SH scripts doesn't seem directly related to that goal. Could you please clarify how this change contributes to the implementation of withdrawal limits?
To help understand the context, let's search for any related changes:
✅ Verification successful
The
new_p2shmethod is relevant to the implementation of withdrawal limits as it facilitates the creation of scripts used in withdrawal transactions.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for withdrawal limit related changes rg --type rust -i "withdrawal.*(limit|restriction)"Length of output: 10395
Script:
#!/bin/bash # Search for usages of the `new_p2sh` method in the Rust codebase rg --type rust 'new_p2sh'Length of output: 298
packages/rs-platform-version/src/version/dpp_versions.rs (1)
274-274: Approved with suggestions for improvement.The addition of the
daily_withdrawal_limitfield to theDPPMethodVersionsstruct is approved. This change is consistent with the existing code structure and provides a good foundation for versioning the withdrawal limits feature.To summarize the review:
- The implementation is correct and follows established patterns.
- Consider adding documentation to the new field for improved clarity.
- Ensure that the complete implementation of the withdrawal limits feature is present in other parts of the codebase.
- Verify that appropriate tests are in place for this new feature.
Once these points are addressed, the change will be fully ready for integration.
packages/rs-platform-version/src/version/drive_abci_versions.rs (1)
341-341: Verify the implementation and usage of the new field across the codebase.The addition of
perform_events_on_first_block_of_protocol_changelikely introduces new functionality. To ensure its proper integration:
- Verify that the logic for handling these events on the first block of a protocol change is implemented correctly.
- Check if all places where
DriveAbciProtocolUpgradeMethodVersionsis instantiated or used are updated to account for this new field.- Ensure that appropriate tests are added or updated to cover this new feature.
To help with this verification, you can run the following script:
This script will help identify areas of the codebase that might need updates or additional testing due to this change.
packages/rs-drive/src/util/grove_operations/mod.rs (6)
111-112: LGTM: New module for batch deletion based on path query.The new module
batch_delete_items_in_path_queryis well-named and its purpose is clearly described in the comment. Making it public allows for external use, which is appropriate for this type of utility function.
114-115: LGTM: New module for conditional insertion with existing element return.The new module
batch_insert_if_not_exists_return_existing_elementis well-named and its purpose is clearly described in the comment. The public visibility is appropriate for this type of utility function.
117-118: LGTM: New module for sum item insertion or addition.The new module
batch_insert_sum_item_or_add_to_if_already_existsis well-named and its purpose is clearly described in the comment. The public visibility is appropriate for this type of utility function, especially for sum operations.
123-123: LGTM: Improved clarity in module description.The updated comment for
grove_get_proved_path_query_with_conditionalnow clearly indicates that the module supports an optional conditional query. This minor change enhances the understanding of the module's functionality.
126-127: LGTM: New GroveDB-specific module for conditional insertion with existing element return.The new module
grove_insert_if_not_exists_return_existing_elementis well-named and its purpose is clearly described in the comment. The public visibility is appropriate for this type of utility function.Note that this module appears to be a GroveDB-specific version of the batch operation
batch_insert_if_not_exists_return_existing_elementadded earlier. This separation allows for optimized implementations specific to GroveDB.
111-127: Overall changes enhance grove operations, but connection to withdrawal limits is unclear.The new modules and updated comment significantly expand the capabilities of grove operations, particularly in areas of conditional insertions, sum operations, and path queries. These additions are well-structured and clearly documented.
However, the direct connection between these changes and the stated PR objective of implementing withdrawal limits is not immediately apparent from these module declarations alone. It would be helpful to have more context on how these new grove operations contribute to the withdrawal limits feature.
To better understand the connection to withdrawal limits, we can search for usage of these new modules in other changed files:
✅ Verification successful
Verified connection between grove operations and withdrawal limits.
The newly added grove operation modules are utilized within the
drive_op_batch/withdrawals.rsfile, confirming their role in implementing withdrawal limits. This integration aligns the changes with the PR objectives.
packages/rs-drive/src/util/batch/drive_op_batch/withdrawals.rsuses:
batch_insert_sum_item_or_add_to_if_already_existsbatch_delete_items_in_path_query🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for usage of new modules in other changed files echo "Searching for usage of new modules:" rg --type rust "batch_delete_items_in_path_query|batch_insert_if_not_exists_return_existing_element|batch_insert_sum_item_or_add_to_if_already_exists|grove_insert_if_not_exists_return_existing_element" $(git diff --name-only)Length of output: 6556
packages/rs-drive/src/drive/initialization/v0/mod.rs (1)
173-173: Verify the implementation and update related components.The addition of the
platform_versionparameter toadd_initial_withdrawal_state_structure_operationsis approved. This change suggests that the withdrawal state structure might now depend on the platform version, which could allow for version-specific behavior.To ensure the change is properly implemented and doesn't introduce any regressions:
- Please verify that the
add_initial_withdrawal_state_structure_operationsmethod correctly uses theplatform_versionparameter.- Update any related documentation to reflect this change in the method signature.
- Modify existing tests or add new ones to cover different platform versions and their impact on the withdrawal state structure.
These checks will help ensure that the change is consistently implemented and properly tested across the codebase.
packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/identity_credit_withdrawal/mod.rs (2)
74-74: Update test cases and clarify PR objectivesWhile the addition of version 1 for basic structure validation is a good improvement, there are a few points to consider:
The test cases in this file don't seem to cover the new version 1 validation. Consider adding tests for this new version to ensure its correct implementation.
The PR objectives mention implementing withdrawal limits, but this change doesn't directly address that. Could you clarify how this change relates to the implementation of withdrawal limits? Is this a prerequisite for that feature?
To help with updating the tests, here's a script to find the existing test cases:
#!/bin/bash # Search for test cases related to identity credit withdrawal rg --type rust "#\[test\]" -A 5 -g "*identity_credit_withdrawal*.rs"This will help identify where new test cases for version 1 validation should be added.
74-74: LGTM: New version added for basic structure validation.The addition of version 1 for basic structure validation is a good step towards supporting new features or changes in the identity credit withdrawal transition process.
However, please ensure the following:
- Update the known versions in the error message to include version 1:
- known_versions: vec![0], + known_versions: vec![0, 1],
- Verify that the
validate_basic_structure_v1method is properly implemented. Can you confirm its implementation or provide its location?packages/rs-platform-version/src/version/drive_versions.rs (3)
434-434: LGTM: New field addition is consistent and well-named.The new field
grove_insert_if_not_exists_return_existing_elementin theDriveGroveBasicMethodVersionsstruct is a good addition. It follows the existing naming convention and seems to represent a useful new feature for version tracking.
568-568: LGTM: New field addition aligns with PR objectives.The new field
calculate_current_withdrawal_limitin theDriveIdentityWithdrawalMethodVersionsstruct is an excellent addition. It's concise, descriptive, and follows the existing naming convention.This field directly supports the PR's objective of implementing withdrawal limits, which is a crucial feature for the platform.
Line range hint
1-768: Overall, the changes in this file are well-implemented and support the PR objectives.The three new fields added to various structs in this file are consistent with the existing code structure and naming conventions. They provide version tracking for new features, including the calculation of withdrawal limits, which directly supports the PR's main objective.
However, there are a few points to consider:
- The PR description lacks details about the specific changes made. It would be helpful to update the PR description with more information about these new fields and their purposes.
- The checklist items in the PR template are unchecked. Please ensure to complete the self-review, add comments for complex code areas if necessary, update tests, and document any breaking changes before merging this PR.
To ensure that these new fields are properly utilized, let's check for their usage in the codebase:
This script will help verify that the new fields are being used appropriately in the codebase.
✅ Verification successful
✅ Verification Successful: New Fields are Utilized Appropriately
The new fields
grove_insert_if_not_exists_return_existing_element,batch_insert_sum_item_or_add_to_if_already_exists, andcalculate_current_withdrawal_limitare consistently used across multiple versions and modules in the codebase. Their integration appears correct and supports the intended functionality of the PR.Please ensure that the PR description is updated with these findings and that all checklist items are addressed before merging.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for usage of new fields in the codebase echo "Checking usage of new fields:" rg "grove_insert_if_not_exists_return_existing_element" --type rust rg "batch_insert_sum_item_or_add_to_if_already_exists" --type rust rg "calculate_current_withdrawal_limit" --type rustLength of output: 6217
packages/rs-platform-version/src/version/mocks/v3_test.rs (5)
480-480: LGTM. Consider adding documentation for the new field.The addition of
calculate_current_withdrawal_limitaligns with the PR objective of implementing withdrawal limits. This new field suggests the introduction of a method to calculate the current withdrawal limit for identities.Consider adding a brief comment explaining the purpose and usage of this new field, especially since it's a version 0 implementation.
551-551: LGTM. Please clarify the relation to withdrawal limits.The addition of
grove_insert_if_not_exists_return_existing_elementintroduces a new method for inserting elements with a check for existence. While this seems like a useful addition, its relation to the PR's main objective of implementing withdrawal limits is not immediately clear.Could you please explain how this new method supports the implementation of withdrawal limits? This will help in understanding the overall design and ensure that all changes are relevant to the PR's objective.
573-573: LGTM. Please provide more context on its usage for withdrawal limits.The addition of
batch_insert_sum_item_or_add_to_if_already_existsintroduces a new method for batch operations involving sum items. This could potentially be used for tracking withdrawal limits or balances efficiently.Could you elaborate on how this method will be used in the context of implementing withdrawal limits? Understanding its specific use case will help ensure it's properly implemented and tested.
579-579: LGTM. Please clarify the purpose in relation to withdrawal limits.The addition of
batch_delete_items_in_path_queryintroduces a new method for batch deletion operations. While this seems like a useful addition to the batch operations capabilities, its specific role in implementing withdrawal limits is not immediately apparent.Could you explain how this method contributes to the implementation of withdrawal limits? Understanding its purpose in this context will help ensure it's correctly implemented and used.
1291-1292: LGTM. Please provide rationale for the withdrawal limit values.The addition of
max_withdrawal_amountanddaily_withdrawal_limitdirectly addresses the PR objective of implementing withdrawal limits.However, there are a few points that need clarification:
- The
max_withdrawal_amountis set to a very large value (50 trillion). Is this intentional, or should it be a more restrictive limit?- The
daily_withdrawal_limitis set toNone. Does this mean there's no daily limit, or is this a placeholder for a value to be determined later?Please provide the rationale behind these choices to ensure they align with the intended withdrawal limit implementation.
packages/rs-drive/src/util/grove_operations/batch_delete_items_in_path_query/mod.rs (1)
28-56: Functionbatch_delete_items_in_path_queryimplementation looks goodThe implementation correctly handles versioning by matching against the
drive_versionand delegating to the appropriate version-specific function. This ensures extensibility and maintainability for future versions.packages/rs-drive/src/util/grove_operations/batch_insert_if_not_exists_return_existing_element/mod.rs (1)
30-52: Implementation correctly follows versioning patternsThe method
batch_insert_if_not_exists_return_existing_elementappropriately dispatches to the version-specific implementation based on the provideddrive_version. Error handling for unknown versions is properly managed, ensuring that unsupported versions are clearly identified.packages/rs-drive/src/util/grove_operations/batch_insert_sum_item_or_add_to_if_already_exists/mod.rs (1)
42-48: Verify the implementation of version 0 function.Ensure that the function
batch_insert_sum_item_or_add_to_if_already_exists_v0is properly implemented in modulev0and behaves as expected.Run the following script to confirm the existence of the version 0 implementation:
packages/rs-drive/src/drive/identity/withdrawals/paths.rs (5)
11-12: Definition of New Constant:WITHDRAWAL_TRANSACTIONS_SUM_AMOUNT_TREE_KEYThe addition of the constant
WITHDRAWAL_TRANSACTIONS_SUM_AMOUNT_TREE_KEYwith the value[2]is appropriate and does not conflict with existing keys. This constant is necessary for managing the sum of withdrawals.
31-36: Conditional Insertion Based on Protocol VersionThe conditional addition of the empty sum tree when
platform_version.protocol_version >= 4is appropriate. This ensures backward compatibility while introducing new functionality for newer protocol versions.
66-72: Addition of Helper Function:get_withdrawal_transactions_sum_tree_path_vecThe new helper function
get_withdrawal_transactions_sum_tree_path_veccorrectly provides the path to the withdrawal transactions sum tree as aVec<Vec<u8>>. This addition is consistent with existing helper functions and enhances code readability and maintainability.
74-80: Addition of Helper Function:get_withdrawal_transactions_sum_tree_pathThe helper function
get_withdrawal_transactions_sum_tree_pathprovides the path as a slice of byte slices, complementing the vector-based version. This maintains consistency with the codebase's pattern for path helpers.
16-19:⚠️ Potential issuePotential Breaking Change: Updated Function Signature
The function
add_initial_withdrawal_state_structure_operationsnow includes an additional parameterplatform_version: &PlatformVersion. This change modifies the function signature and may affect all places where this function is called. Please ensure that all invocations of this function have been updated to include the new parameter. Assess the potential impact on external code or public APIs that depend on this function.To verify that all calls to this function have been updated, you can run the following script:
✅ Verification successful
Verified: All invocations of
add_initial_withdrawal_state_structure_operationshave been updated with the newplatform_versionparameter.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find all usages of `add_initial_withdrawal_state_structure_operations` to check for proper parameter updates. # Test: Search for all calls to the function. Expect: All calls include the new `platform_version` argument. rg --type rust -A 2 'add_initial_withdrawal_state_structure_operations\('Length of output: 686
packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/identity_credit_withdrawal/structure/v1/mod.rs (5)
16-16: Addition ofPlatformVersionImportThe import of
PlatformVersionis necessary for accessing version-specific system limits in the validation logic.
26-29: Updated Implementation Method SignatureThe implementation of
validate_basic_structure_v1forIdentityCreditWithdrawalTransitioncorrectly includes theplatform_versionparameter, aligning with the updated trait definition.
40-40: Enhanced Error Reporting with Dynamic LimitsPassing
platform_version.system_limits.max_withdrawal_amountto theInvalidIdentityCreditWithdrawalTransitionAmountErrorconstructor provides users with accurate, version-specific information when an error occurs, improving the clarity of error messages.
32-35: Validation Against Platform-Specific Withdrawal LimitsThe addition of the check:
if amount < MIN_WITHDRAWAL_AMOUNT || amount > platform_version.system_limits.max_withdrawal_amount {ensures that the withdrawal amount adheres to both the minimum and the dynamically defined maximum limits per platform version. This enhances the robustness of the validation by accommodating different limits across platform versions.
[approve]
To confirm that
platform_version.system_limits.max_withdrawal_amountis correctly set and used throughout the codebase, you can run the following script:
20-20: Updated Trait Method SignatureThe method signature of
validate_basic_structure_v1in the trait now includes theplatform_versionparameter. This ensures that the validation can access platform-specific limits, which enhances flexibility and adaptability across different platform versions.To ensure consistency, verify that all implementations of the
IdentityCreditWithdrawalStateTransitionStructureValidationV1trait have been updated to include the newplatform_versionparameter.packages/rs-drive/src/drive/identity/withdrawals/calculate_current_withdrawal_limit/v0/mod.rs (2)
15-40: Comprehensive function documentationThe function
calculate_current_withdrawal_limit_v0is well-documented with clear explanations of its purpose, parameters, return values, and potential errors. This greatly enhances code maintainability and readability.
84-84: Appropriate use ofsaturating_subUsing
saturating_subto calculate the remaining withdrawal limit ensures that the result does not go below zero, which is appropriate to prevent negative withdrawal limits.packages/rs-drive/src/util/grove_operations/batch_delete_items_in_path_query/v0/mod.rs (1)
29-95: Function implementation looks goodThe
batch_delete_items_in_path_query_v0function is well-implemented, and the logic appears correct.packages/rs-drive-abci/src/execution/platform_events/withdrawals/pool_withdrawals_into_transactions_queue/v0/mod.rs (2)
60-62: Validatecalculate_current_withdrawal_limitimplementationEnsure that
calculate_current_withdrawal_limitcorrectly calculates the withdrawal limit and handles edge cases, such as maximum limits or zero limits.Run the following script to inspect the implementation:
#!/bin/bash # Description: Review the implementation of calculate_current_withdrawal_limit # Expected: The function should handle edge cases appropriately. rg 'fn calculate_current_withdrawal_limit' -A 20 \ packages/rs-drive-abci/src/drive/withdrawals/mod.rs
94-100: Ensure consistency between calculated and returned total amountsVerify that
total_amountreturned frombuild_untied_withdrawal_transactions_from_documentsmatchestotal_withdrawal_amountcalculated earlier to maintain consistency.Run the following script to compare the calculated total amounts:
packages/rs-platform-version/src/version/v1.rs (1)
642-642: Settingperform_events_on_first_block_of_protocol_changetoNoneAt line 642, the field
perform_events_on_first_block_of_protocol_changeis set toNone. Ensure that this change does not adversely affect the protocol upgrade process, especially if certain events are expected during the first block of a protocol change.Run the following script to check for usages and implications:
✅ Verification successful
✅ Verified: Setting
perform_events_on_first_block_of_protocol_changetoNonealigns with existing implementations and does not adversely affect the protocol upgrade process.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find all references to `perform_events_on_first_block_of_protocol_change` and review their context. rg --type rust -C 5 'perform_events_on_first_block_of_protocol_change'Length of output: 17701
packages/rs-platform-version/src/version/mocks/v2_test.rs (6)
1288-1289: Verify the values ofmax_withdrawal_amountanddaily_withdrawal_limitinSystemLimitsThe
max_withdrawal_amountis set to50_000_000_000_000, anddaily_withdrawal_limitis set toNoneinSystemLimits. Please ensure that these values are correct and reflect the intended withdrawal limits.You may want to double-check these values against the system requirements or specifications.
573-573: Verify implementation ofbatch_insert_sum_item_or_add_to_if_already_existsEnsure that the new method
batch_insert_sum_item_or_add_to_if_already_exists: 0is properly implemented inDriveGroveBatchMethodVersionsand integrated with batch operations.Check the implementation with:
#!/bin/bash # Description: Find the implementation of `batch_insert_sum_item_or_add_to_if_already_exists`. # Test: Look for the function definition. Expect: At least one match. rg --type rust 'fn\s+batch_insert_sum_item_or_add_to_if_already_exists' src/
551-551: Verify implementation ofgrove_insert_if_not_exists_return_existing_elementThe method version
grove_insert_if_not_exists_return_existing_element: 0has been added toDriveGroveBasicMethodVersions. Please confirm that this method is implemented and properly tested.Use the following script to locate the method:
#!/bin/bash # Description: Search for the implementation of `grove_insert_if_not_exists_return_existing_element`. # Test: Look for the function definition. Expect: At least one match. rg --type rust 'fn\s+grove_insert_if_not_exists_return_existing_element' src/
579-579: Verify implementation ofbatch_delete_items_in_path_queryConfirm that the method
batch_delete_items_in_path_query: 0is implemented appropriately and handles batch deletions correctly.Locate the method with:
#!/bin/bash # Description: Search for the implementation of `batch_delete_items_in_path_query`. # Test: Look for the function definition. Expect: At least one match. rg --type rust 'fn\s+batch_delete_items_in_path_query' src/
1272-1272: Ensure correct initialization ofdaily_withdrawal_limitinDPPMethodVersionsThe new field
daily_withdrawal_limit: 0is added toDPPMethodVersions. Confirm that this field is correctly integrated and used in the calculations related to withdrawal limits.Use this script to check for usage:
#!/bin/bash # Description: Search for usages of `daily_withdrawal_limit` in `DPPMethodVersions`. # Test: Find all references. Expect: Matches in methods and logic. rg --type rust 'daily_withdrawal_limit' src/
480-480: Verify implementation ofcalculate_current_withdrawal_limitThe new method version
calculate_current_withdrawal_limit: 0has been added toDriveIdentityWithdrawalMethodVersions. Please ensure that this method is properly implemented and that its logic aligns with the withdrawal limits feature.You can use the following script to check for the implementation:
✅ Verification successful
Verified
calculate_current_withdrawal_limitImplementationThe
calculate_current_withdrawal_limitmethod has been correctly implemented for versionv0inDriveIdentityWithdrawalMethodVersions. Thev0implementation aligns with the withdrawal limits feature as intended.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for the implementation of `calculate_current_withdrawal_limit`. # Test: Look for the function definition. Expect: At least one match. rg --type rust 'fn\s+calculate_current_withdrawal_limit' src/Length of output: 183
Script:
#!/bin/bash # Description: Search for the implementation of `calculate_current_withdrawal_limit`. # Test: Look for the function definition. Expect: At least one match. rg --type rust 'fn\s+calculate_current_withdrawal_limit' src/Length of output: 108
Script:
#!/bin/bash # Description: Search for the implementation of `calculate_current_withdrawal_limit` within the correct directory. # Test: Look for the function definition. Expect: At least one match. rg --type rust 'fn\s+calculate_current_withdrawal_limit' packages/rs-platform-version/src/Length of output: 92
Script:
#!/bin/bash # Description: Broad search for any occurrences of `calculate_current_withdrawal_limit` in the codebase. rg 'calculate_current_withdrawal_limit'Length of output: 2261
Script:
#!/bin/bash # Description: Search for all implementations of `calculate_current_withdrawal_limit` across different versions. # Find all function implementations with version suffixes rg --type rust 'fn\s+calculate_current_withdrawal_limit_v\d+' packages/rs-drive/src/drive/identity/withdrawals/Length of output: 264
packages/rs-platform-version/src/version/v4.rs (5)
580-580: Approve the addition ofbatch_delete_items_in_path_queryThe addition of
batch_delete_items_in_path_query: 0inDriveGroveBatchMethodVersionsis appropriate and aligns with existing method versions.
1275-1275: Approve the addition ofdaily_withdrawal_limitinDPPMethodVersionsThe addition of
daily_withdrawal_limit: 0inDPPMethodVersionsis consistent with the existing structure.
644-644: Verify the version number forperform_events_on_first_block_of_protocol_changeThe field
perform_events_on_first_block_of_protocol_changeis set toSome(4)inDriveAbciProtocolUpgradeMethodVersions. Please confirm that version4is correct and that related methods are updated accordingly.Run the following script to verify usage consistency:
#!/bin/bash # Description: Verify the usage of 'perform_events_on_first_block_of_protocol_change' with version 4 # Test: Search for occurrences with version 4. rg --type rust 'perform_events_on_first_block_of_protocol_change.*Some\(4\)'
1292-1293:⚠️ Potential issueVerify the correctness of withdrawal amount values
The fields
max_withdrawal_amountanddaily_withdrawal_limitare set to50_000_000_000_000, with comments indicating//500 Dash. Please ensure that the values accurately represent500 Dashbased on the unit definitions used.To confirm the unit conversion:
Assumption: If
1 Dash = 100,000,000 duffs, then500 Dash = 50,000,000,000 duffs.Current Value:
50,000,000,000,000suggests500,000 Dashif1 Dash = 100,000,000 duffs.Run the following script to verify unit definitions and occurrences:
#!/bin/bash # Description: Verify the unit conversions and consistency of withdrawal limits # Test: Check for unit definitions in the codebase. rg --type rust 'const\s+DUFFS_PER_DASH\s*=\s*\d+' # Test: Search for other occurrences of withdrawal limit values. rg --type rust 'max_withdrawal_amount|daily_withdrawal_limit' # Test: Review documentation for unit conversion rates. rg --files-with-matches 'unit conversion' | xargs grep -H 'Dash.*duff'If the unit conversion confirms that
50,000,000,000,000represents500 Dash, please update the comments to clarify the conversion to prevent confusion.
481-481: Confirm the addition ofcalculate_current_withdrawal_limitThe field
calculate_current_withdrawal_limitis added with version0inDriveIdentityWithdrawalMethodVersions. Please ensure that this method is implemented and that its usage is consistent across the codebase.Run the following script to verify the implementation and references:
✅ Verification successful
Verification Successful:
calculate_current_withdrawal_limitis Implemented and Consistently UsedThe
calculate_current_withdrawal_limitmethod is properly implemented and referenced across the relevant parts of the codebase.
Implementation found in:
packages/rs-drive/src/drive/identity/withdrawals/calculate_current_withdrawal_limit/mod.rspackages/rs-drive/src/drive/identity/withdrawals/calculate_current_withdrawal_limit/v0/mod.rsReferences found in multiple version files and test modules, ensuring consistent usage.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the implementation and usage of 'calculate_current_withdrawal_limit' # Test: Search for the method implementation. rg --type rust 'fn\s+calculate_current_withdrawal_limit' # Test: Search for references to the method. rg --type rust 'calculate_current_withdrawal_limit'Length of output: 3860
Script:
#!/bin/bash # Description: Verify the implementation and usage of 'calculate_current_withdrawal_limit' # Test: Search for the method implementation. rg --type rust 'fn\s+calculate_current_withdrawal_limit' # Test: Search for references to the method. rg --type rust 'calculate_current_withdrawal_limit'Length of output: 2621
packages/rs-platform-version/src/version/v3.rs (7)
486-486: Addition ofcalculate_current_withdrawal_limitmethod for withdrawalsThe addition of
calculate_current_withdrawal_limit: 0under theidentity.withdrawalsmethods aligns with the implementation of withdrawal limits and integrates appropriately within the platform versioning system.
557-557: Addition ofgrove_insert_if_not_exists_return_existing_elementmethodThe new method
grove_insert_if_not_exists_return_existing_element: 0extends the GroveDB basic methods and is consistent with existing naming conventions and structure.
579-579: Addition ofbatch_insert_sum_item_or_add_to_if_already_existsmethodThe method
batch_insert_sum_item_or_add_to_if_already_exists: 0enhances batch operation capabilities within GroveDB. This addition is appropriate and follows the existing method patterns.
585-585: Addition ofbatch_delete_items_in_path_querymethodThe addition of
batch_delete_items_in_path_query: 0to the batch methods expands the batch deletion capabilities, which is a valuable enhancement to the platform's functionality.
649-649: Verify the initialization ofperform_events_on_first_block_of_protocol_changeThe field
perform_events_on_first_block_of_protocol_changeis set toNone. Please ensure that this field is correctly typed as anOptionto acceptNonevalues. This will prevent potential issues with type mismatches during runtime.
1280-1280: Confirm default value fordaily_withdrawal_limitin DPP methodsThe
daily_withdrawal_limitis set to0inDPPMethodVersions. Please verify whether0is the intended default value, indicating no withdrawals allowed, or if it should be set to a specific limit orNoneto represent unlimited withdrawals.
1297-1298: Ensure correct typing fordaily_withdrawal_limitinsystem_limitsIn
system_limits,max_withdrawal_amountis set to50_000_000_000_000, whiledaily_withdrawal_limitis set toNone. Please verify thatdaily_withdrawal_limitis correctly typed as anOption<u64>to acceptNonevalues. This ensures consistency and prevents potential type mismatches.
| pub fn daily_withdrawal_limit_v0(total_credits_in_platform: Credits) -> Credits { | ||
| if total_credits_in_platform >= 1000 { | ||
| total_credits_in_platform / 10 | ||
| } else if total_credits_in_platform >= 100 { | ||
| 100 | ||
| } else { | ||
| total_credits_in_platform | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance robustness and add unit tests
While the basic functionality is implemented correctly, there are a few areas where the function could be improved:
- Input validation: Consider handling invalid input, such as negative credit values.
- Unit tests: Add unit tests to verify the function's behavior for different input scenarios.
- Error handling: Instead of using unwrap, consider returning a Result type to handle potential errors gracefully.
Here's a suggested improvement that addresses these points:
use crate::fee::Credits;
#[derive(Debug, thiserror::Error)]
pub enum WithdrawalLimitError {
#[error("Invalid credit amount: {0}")]
InvalidCreditAmount(i64),
#[error("Overflow error")]
Overflow,
}
pub fn daily_withdrawal_limit_v0(total_credits_in_platform: Credits) -> Result<Credits, WithdrawalLimitError> {
if total_credits_in_platform < 0 {
return Err(WithdrawalLimitError::InvalidCreditAmount(total_credits_in_platform));
}
Ok(if total_credits_in_platform >= 1000 {
total_credits_in_platform.checked_div(10).ok_or(WithdrawalLimitError::Overflow)?
} else if total_credits_in_platform >= 100 {
100
} else {
total_credits_in_platform
})
}
#[cfg(test)]
mod tests {
use super::*;
#[test]
fn test_daily_withdrawal_limit() {
assert_eq!(daily_withdrawal_limit_v0(2000).unwrap(), 200);
assert_eq!(daily_withdrawal_limit_v0(500).unwrap(), 100);
assert_eq!(daily_withdrawal_limit_v0(50).unwrap(), 50);
}
#[test]
fn test_invalid_input() {
assert!(matches!(daily_withdrawal_limit_v0(-100), Err(WithdrawalLimitError::InvalidCreditAmount(-100))));
}
#[test]
fn test_overflow() {
assert!(matches!(daily_withdrawal_limit_v0(Credits::MAX), Err(WithdrawalLimitError::Overflow)));
}
}This implementation adds input validation, error handling, and unit tests to increase the robustness and reliability of the function.
Would you like me to create a separate GitHub issue to track the implementation of these improvements?
There was a problem hiding this comment.
Credits is a u64. I don't think a division by 10 could ever overflow.
There was a problem hiding this comment.
@QuantumExplorer, you're correct. Since Credits is a u64, division by 10 won't cause an overflow. Thank you for the clarification.
✏️ Learnings added
Learnt from: QuantumExplorer
PR: dashpay/platform#2182
File: packages/rs-dpp/src/withdrawal/daily_withdrawal_limit/v0/mod.rs:23-31
Timestamp: 2024-09-28T20:34:49.001Z
Learning: `Credits` is a `u64`, and division by 10 won't cause an overflow.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
packages/rs-dpp/src/withdrawal/daily_withdrawal_limit/v0/mod.rs
Outdated
Show resolved
Hide resolved
| // use crate::drive::Drive; | ||
| // use crate::error::Error; | ||
| // use crate::fees::op::LowLevelDriveOperation; | ||
| // use crate::util::grove_operations::push_drive_operation_result_optional; | ||
| // use grovedb::{Element, TransactionArg}; | ||
| // use grovedb_path::SubtreePath; | ||
| // use platform_version::version::drive_versions::DriveVersion; | ||
| // | ||
| // impl Drive { | ||
| // /// Pushes the `OperationCost` of inserting an element in groveDB where the path key does not yet exist | ||
| // /// to `drive_operations`. | ||
| // pub(crate) fn grove_insert_if_not_exists_return_existing_element_v0<B: AsRef<[u8]>>( | ||
| // &self, | ||
| // path: SubtreePath<'_, B>, | ||
| // key: &[u8], | ||
| // element: Element, | ||
| // transaction: TransactionArg, | ||
| // drive_operations: Option<&mut Vec<LowLevelDriveOperation>>, | ||
| // drive_version: &DriveVersion, | ||
| // ) -> Result<bool, Error> { | ||
| // let cost_context = self.grove.insert_if_not_exists_return_existing_element( | ||
| // path, | ||
| // key, | ||
| // element, | ||
| // transaction, | ||
| // &drive_version.grove_version, | ||
| // ); | ||
| // push_drive_operation_result_optional(cost_context, drive_operations) | ||
| // } | ||
| // } |
There was a problem hiding this comment.
Remove comment markers from the entire file.
The entire file content is currently commented out, which would prevent the implementation from being used. This appears to be unintentional and should be corrected.
Apply this diff to uncomment the entire file:
-// use crate::drive::Drive;
-// use crate::error::Error;
-// use crate::fees::op::LowLevelDriveOperation;
-// use crate::util::grove_operations::push_drive_operation_result_optional;
-// use grovedb::{Element, TransactionArg};
-// use grovedb_path::SubtreePath;
-// use platform_version::version::drive_versions::DriveVersion;
-//
-// impl Drive {
-// /// Pushes the `OperationCost` of inserting an element in groveDB where the path key does not yet exist
-// /// to `drive_operations`.
-// pub(crate) fn grove_insert_if_not_exists_return_existing_element_v0<B: AsRef<[u8]>>(
-// &self,
-// path: SubtreePath<'_, B>,
-// key: &[u8],
-// element: Element,
-// transaction: TransactionArg,
-// drive_operations: Option<&mut Vec<LowLevelDriveOperation>>,
-// drive_version: &DriveVersion,
-// ) -> Result<bool, Error> {
-// let cost_context = self.grove.insert_if_not_exists_return_existing_element(
-// path,
-// key,
-// element,
-// transaction,
-// &drive_version.grove_version,
-// );
-// push_drive_operation_result_optional(cost_context, drive_operations)
-// }
-// }
+use crate::drive::Drive;
+use crate::error::Error;
+use crate::fees::op::LowLevelDriveOperation;
+use crate::util::grove_operations::push_drive_operation_result_optional;
+use grovedb::{Element, TransactionArg};
+use grovedb_path::SubtreePath;
+use platform_version::version::drive_versions::DriveVersion;
+
+impl Drive {
+ /// Pushes the `OperationCost` of inserting an element in groveDB where the path key does not yet exist
+ /// to `drive_operations`.
+ pub(crate) fn grove_insert_if_not_exists_return_existing_element_v0<B: AsRef<[u8]>>(
+ &self,
+ path: SubtreePath<'_, B>,
+ key: &[u8],
+ element: Element,
+ transaction: TransactionArg,
+ drive_operations: Option<&mut Vec<LowLevelDriveOperation>>,
+ drive_version: &DriveVersion,
+ ) -> Result<bool, Error> {
+ let cost_context = self.grove.insert_if_not_exists_return_existing_element(
+ path,
+ key,
+ element,
+ transaction,
+ &drive_version.grove_version,
+ );
+ push_drive_operation_result_optional(cost_context, drive_operations)
+ }
+}📝 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.
| // use crate::drive::Drive; | |
| // use crate::error::Error; | |
| // use crate::fees::op::LowLevelDriveOperation; | |
| // use crate::util::grove_operations::push_drive_operation_result_optional; | |
| // use grovedb::{Element, TransactionArg}; | |
| // use grovedb_path::SubtreePath; | |
| // use platform_version::version::drive_versions::DriveVersion; | |
| // | |
| // impl Drive { | |
| // /// Pushes the `OperationCost` of inserting an element in groveDB where the path key does not yet exist | |
| // /// to `drive_operations`. | |
| // pub(crate) fn grove_insert_if_not_exists_return_existing_element_v0<B: AsRef<[u8]>>( | |
| // &self, | |
| // path: SubtreePath<'_, B>, | |
| // key: &[u8], | |
| // element: Element, | |
| // transaction: TransactionArg, | |
| // drive_operations: Option<&mut Vec<LowLevelDriveOperation>>, | |
| // drive_version: &DriveVersion, | |
| // ) -> Result<bool, Error> { | |
| // let cost_context = self.grove.insert_if_not_exists_return_existing_element( | |
| // path, | |
| // key, | |
| // element, | |
| // transaction, | |
| // &drive_version.grove_version, | |
| // ); | |
| // push_drive_operation_result_optional(cost_context, drive_operations) | |
| // } | |
| // } | |
| use crate::drive::Drive; | |
| use crate::error::Error; | |
| use crate::fees::op::LowLevelDriveOperation; | |
| use crate::util::grove_operations::push_drive_operation_result_optional; | |
| use grovedb::{Element, TransactionArg}; | |
| use grovedb_path::SubtreePath; | |
| use platform_version::version::drive_versions::DriveVersion; | |
| impl Drive { | |
| /// Pushes the `OperationCost` of inserting an element in groveDB where the path key does not yet exist | |
| /// to `drive_operations`. | |
| pub(crate) fn grove_insert_if_not_exists_return_existing_element_v0<B: AsRef<[u8]>>( | |
| &self, | |
| path: SubtreePath<'_, B>, | |
| key: &[u8], | |
| element: Element, | |
| transaction: TransactionArg, | |
| drive_operations: Option<&mut Vec<LowLevelDriveOperation>>, | |
| drive_version: &DriveVersion, | |
| ) -> Result<bool, Error> { | |
| let cost_context = self.grove.insert_if_not_exists_return_existing_element( | |
| path, | |
| key, | |
| element, | |
| transaction, | |
| &drive_version.grove_version, | |
| ); | |
| push_drive_operation_result_optional(cost_context, drive_operations) | |
| } | |
| } |
| if previous_protocol_version < 4 && platform_version.protocol_version >= 4 { | ||
| let path = get_withdrawal_root_path(); | ||
| self.drive.grove_insert_if_not_exists( | ||
| (&path).into(), | ||
| &WITHDRAWAL_TRANSACTIONS_SUM_AMOUNT_TREE_KEY, | ||
| Element::empty_sum_tree(), | ||
| Some(transaction), | ||
| None, | ||
| &platform_version.drive, | ||
| )?; | ||
| } | ||
|
|
||
| Ok(()) |
There was a problem hiding this comment.
💡 Codebase verification
Consider Enhancing Error Handling While Maintaining Consistent Version Checks and Sum Tree Initializations.
The version check previous_protocol_version < 4 && platform_version.protocol_version >= 4 and the insertion of Element::empty_sum_tree() are consistently implemented across multiple modules, ensuring standardized behavior during protocol upgrades. However, the current error propagation using the ? operator could be improved by adding more contextual information. Implementing more specific error handling, such as using .map_err() or wrapping calls in match statements, would provide clearer insights during debugging and enhance maintainability.
🔗 Analysis chain
Consider enhancing error handling and verifying version check logic.
-
Error Handling: The current implementation relies on the
?operator for error propagation. Consider adding more specific error handling to provide context about where an error occurred. This could involve wrapping thegrove_insert_if_not_existscall in amatchstatement or using.map_err()to add context to any errors. -
Version Check: The condition
previous_protocol_version < 4 && platform_version.protocol_version >= 4assumes a specific upgrade path. Verify if this is the intended logic, especially considering future protocol versions. -
Empty Sum Tree: The insertion of an empty sum tree suggests initialization of a new data structure. Ensure that this is the correct approach for all scenarios, including potential reorgs or edge cases.
To verify the usage and impact of this method:
This script will help identify other areas of the codebase that might be affected by or related to this change, ensuring consistency and completeness of the implementation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for other occurrences of protocol version checks and sum tree insertions
# Test 1: Look for other protocol version checks
echo "Checking for protocol version checks:"
rg --type rust "protocol_version.*[<>=].*4"
# Test 2: Look for other sum tree insertions
echo "Checking for sum tree insertions:"
rg --type rust "Element::empty_sum_tree"
# Test 3: Check for withdrawal-related operations
echo "Checking for withdrawal-related operations:"
rg --type rust "withdrawal"
Length of output: 234831
| use dpp::block::block_info::BlockInfo; | ||
| use dpp::fee::Credits; |
There was a problem hiding this comment.
Remove unused import BlockInfo
The BlockInfo import on line 5 is not used in this file and has been flagged by the static analysis tool as unused.
Please remove the unused import:
-use dpp::block::block_info::BlockInfo;📝 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.
| use dpp::block::block_info::BlockInfo; | |
| use dpp::fee::Credits; | |
| use dpp::fee::Credits; |
🧰 Tools
🪛 GitHub Check: Rust packages (drive-abci) / Linting
[warning] 5-5: unused import:
dpp::block::block_info::BlockInfo
warning: unused import:dpp::block::block_info::BlockInfo
--> packages/rs-drive/src/drive/identity/withdrawals/transaction/queue/add_enqueue_untied_withdrawal_transaction_operations/mod.rs:5:5
|
5 | use dpp::block::block_info::BlockInfo;
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
= note:#[warn(unused_imports)]on by default
🪛 GitHub Check: Rust packages (drive) / Linting
[warning] 5-5: unused import:
dpp::block::block_info::BlockInfo
warning: unused import:dpp::block::block_info::BlockInfo
--> packages/rs-drive/src/drive/identity/withdrawals/transaction/queue/add_enqueue_untied_withdrawal_transaction_operations/mod.rs:5:5
|
5 | use dpp::block::block_info::BlockInfo;
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
= note:#[warn(unused_imports)]on by default
| }, | ||
| methods: DPPMethodVersions { | ||
| epoch_core_reward_credits_for_distribution: 0, | ||
| daily_withdrawal_limit: 0, |
There was a problem hiding this comment.
Inconsistent Definitions of daily_withdrawal_limit
The daily_withdrawal_limit: 0, is added in DPPMethodVersions at line 1273, while a different value (None) is set in system_limits at line 1291. This inconsistency could lead to confusion or errors in the withdrawal limit calculations. Please ensure that daily_withdrawal_limit is defined consistently across the codebase.
| max_withdrawal_amount: 50_000_000_000_000, | ||
| daily_withdrawal_limit: None, |
There was a problem hiding this comment.
Verify Withdrawal Limits Configuration
At lines 1290-1291, max_withdrawal_amount is set to 50_000_000_000_000, and daily_withdrawal_limit is set to None in system_limits. Confirm that these values align with the desired withdrawal policies and that having daily_withdrawal_limit as None does not inadvertently allow unlimited daily withdrawals.
| protocol_upgrade: DriveAbciProtocolUpgradeMethodVersions { | ||
| check_for_desired_protocol_upgrade: 0, | ||
| upgrade_protocol_version_on_epoch_change: 0, | ||
| perform_events_on_first_block_of_protocol_change: None, |
There was a problem hiding this comment.
Check the assignment of perform_events_on_first_block_of_protocol_change
The field perform_events_on_first_block_of_protocol_change is set to None in DriveAbciProtocolUpgradeMethodVersions. Please verify if this is intentional or if it should have a default version number.
| grove_insert_empty_tree: 0, | ||
| grove_insert_empty_sum_tree: 0, | ||
| grove_insert_if_not_exists: 0, | ||
| grove_insert_if_not_exists_return_existing_element: 0, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider simplifying the field name grove_insert_if_not_exists_return_existing_element
The field name grove_insert_if_not_exists_return_existing_element is quite long, which may affect readability and maintainability.
Consider renaming it to a more concise name, such as grove_upsert_return_existing, to improve clarity.
Apply this diff to rename the field:
- grove_insert_if_not_exists_return_existing_element: 0,
+ grove_upsert_return_existing: 0,📝 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.
| grove_insert_if_not_exists_return_existing_element: 0, | |
| grove_upsert_return_existing: 0, |
| batch_insert_empty_tree: 0, | ||
| batch_insert_empty_tree_if_not_exists: 0, | ||
| batch_insert_empty_tree_if_not_exists_check_existing_operations: 0, | ||
| batch_insert_sum_item_or_add_to_if_already_exists: 0, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Simplify the field name batch_insert_sum_item_or_add_to_if_already_exists
The field name batch_insert_sum_item_or_add_to_if_already_exists is excessively long, which can hinder readability.
Consider renaming it to batch_upsert_sum_item for brevity.
Apply this diff to rename the field:
- batch_insert_sum_item_or_add_to_if_already_exists: 0,
+ batch_upsert_sum_item: 0,📝 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.
| batch_insert_sum_item_or_add_to_if_already_exists: 0, | |
| batch_upsert_sum_item: 0, |
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Outside diff range and nitpick comments (16)
packages/masternode-reward-shares-contract/Cargo.toml (1)
Line range hint
1-14: Consider reviewing other dependenciesWhile updating
thiserroris a good practice, it's worth considering a review of other dependencies as well. Regular updates can help maintain security and take advantage of performance improvements.Consider running
cargo outdatedto check for any other dependencies that might benefit from updates. Pay special attention to theplatform-versionandplatform-valuedependencies, as they are local path dependencies and might need to be kept in sync with the rest of your project.packages/rs-json-schema-compatibility-validator/Cargo.toml (1)
8-12: Consider reviewing other dependencies for potential updates.While updating
thiserroris good, it's worth reviewing other dependencies to ensure they're all up to date. Here are some suggestions:
- Check if there are newer versions available for
json-patch,serde_json, andonce_cell.- If newer versions are available, consider updating them as well, especially if they offer important bug fixes or performance improvements.
- Ensure that any updates maintain compatibility with your current Rust version and other dependencies.
You can use the following command to check for outdated dependencies:
cargo outdatedThis will show you which dependencies have newer versions available. Remember to test thoroughly after any updates to ensure compatibility and stability.
packages/rs-platform-value/Cargo.toml (1)
Line range hint
1-35: Consider updating other dependencies.While updating
thiserroris good, it's worth checking if other dependencies also have updates available. This helps keep the project up-to-date and potentially benefits from bug fixes and performance improvements.You can use the following script to check for outdated dependencies:
#!/bin/bash # Description: Check for outdated dependencies in packages/rs-platform-value/Cargo.toml # Test: List outdated dependencies cargo outdated --manifest-path packages/rs-platform-value/Cargo.tomlpackages/rs-dpp/src/withdrawal/daily_withdrawal_limit/v0/mod.rs (2)
22-30: LGTM: Correct implementation with a minor suggestionThe
daily_withdrawal_limit_v0function is implemented correctly according to the specified rules. The code is concise and easy to understand.However, consider adding a check for potential overflow when dividing
total_credits_in_platformby 10. Although it's unlikely to occur withCreditsbeing au64, it's a good practice to handle edge cases.You could use the
checked_divmethod to handle potential overflow:pub fn daily_withdrawal_limit_v0(total_credits_in_platform: Credits) -> Credits { if total_credits_in_platform >= 1000 { total_credits_in_platform.checked_div(10).unwrap_or(Credits::MAX) } else if total_credits_in_platform >= 100 { 100 } else { total_credits_in_platform } }This change ensures that in the unlikely event of an overflow, the function will return the maximum possible
Creditsvalue instead of panicking.
36-41: LGTM with suggestions: Comprehensive test functionThe
test_daily_withdrawal_limitfunction covers the main scenarios described in the function documentation. The assertions correctly verify the expected output for each case.To improve test coverage, consider adding the following test cases:
- Edge case: Test with exactly 1000 credits (boundary between first and second rule).
- Edge case: Test with exactly 100 credits (boundary between second and third rule).
- Edge case: Test with 0 credits.
- Edge case: Test with the maximum possible value for
Credits(u64::MAX).Here's an example of how you could expand the test function:
#[test] fn test_daily_withdrawal_limit() { assert_eq!(daily_withdrawal_limit_v0(2000), 200); assert_eq!(daily_withdrawal_limit_v0(1000), 100); // Edge case assert_eq!(daily_withdrawal_limit_v0(500), 100); assert_eq!(daily_withdrawal_limit_v0(100), 100); // Edge case assert_eq!(daily_withdrawal_limit_v0(50), 50); assert_eq!(daily_withdrawal_limit_v0(0), 0); // Edge case assert_eq!(daily_withdrawal_limit_v0(u64::MAX), u64::MAX / 10); // Edge case }These additional test cases will ensure that the function behaves correctly at the boundaries between different rules and for extreme values.
packages/withdrawals-contract/src/lib.rs (1)
35-43: Approve changes to WithdrawalStatus enum with a minor suggestion.The additions to the
WithdrawalStatusenum are well-structured and provide a more granular representation of the withdrawal process. The use of#[repr(u8)]and derived traits is appropriate, ensuring consistent memory layout and enabling various operations.The explicit integer values and descriptive comments for each variant enhance clarity and maintainability. These changes will likely improve the tracking and management of withdrawals in the system.
For consistency, consider using all uppercase for the enum variant names. Apply this diff:
pub enum WithdrawalStatus { /// The documents are in the state and waiting to be processed. - QUEUED = 0, + QUEUED = 0, /// Pooled happens when we are waiting for signing. - POOLED = 1, + POOLED = 1, /// We have broadcasted the transaction to core. - BROADCASTED = 2, + BROADCASTED = 2, /// The transaction is now complete. - COMPLETE = 3, + COMPLETED = 3, /// We broadcasted the transaction but core never saw it or rejected it. - EXPIRED = 4, + EXPIRED = 4, }This change ensures all variant names are verbs in the past tense, improving consistency and readability.
packages/rs-drive/Cargo.toml (1)
80-80: LGTM! Consider using a version range for flexibility.The addition of
assert_matchesas a dev-dependency is appropriate and will enhance the expressiveness of test assertions. The crate is well-suited for testing purposes.Consider using a version range instead of a fixed version to allow for minor updates and patches:
-assert_matches = "1.5.0" +assert_matches = "1.5"This change would allow for automatic updates to patch versions (e.g., 1.5.1, 1.5.2) while still maintaining compatibility with the 1.5.x series.
packages/rs-drive-abci/src/execution/platform_events/withdrawals/dequeue_and_build_unsigned_withdrawal_transactions/v0/mod.rs (1)
Line range hint
1-204: Overall improvement in error handling, but further verification needed.The change in this file improves the specificity of error handling for document revision overflow. However, to ensure consistency across the codebase, it's recommended to:
- Review other occurrences of
increment_revision()calls to ensure they handle overflow errors similarly.- Update any error handling code that may be expecting the previous
CorruptedCodeExecutionerror type for this scenario.- Consider adding a comment explaining why overflow is a potential issue in this context, which could help future maintainers understand the rationale behind this specific error handling.
packages/strategy-tests/src/operations.rs (3)
498-499: LGTM. Consider adding documentation forAmountRange.The introduction of
AmountRangeas aRange<Credits>is a good approach for implementing withdrawal limits. It allows for flexible specification of minimum and maximum values.Consider adding a brief documentation comment explaining the purpose and usage of
AmountRange. For example:/// Represents a range of allowed amounts for operations involving credits. /// Used for specifying limits such as withdrawal ranges. pub type AmountRange = Range<Credits>;
503-505: LGTM. Update related documentation if it exists.The modification of
IdentityTopUpandIdentityWithdrawalvariants to includeAmountRangedirectly implements the withdrawal limits feature. This change provides a consistent way to set limits for both top-up and withdrawal operations.If there's any existing documentation for the
OperationTypeenum or these specific variants, make sure to update it to reflect the newAmountRangeparameter and its implications for operation limits.
609-611: LGTM. Consider adding unit tests for new deserialization logic.The
versioned_deserializemethod has been correctly updated to handle the deserialization of the newAmountRangeparameter forIdentityTopUpandIdentityWithdrawaloperations. The implementation is consistent with other variants and matches the serialization logic.Consider adding unit tests specifically for the deserialization of
IdentityTopUpandIdentityWithdrawaloperations with the newAmountRangeparameter. This will ensure the correctness of the deserialization process for these modified variants.Also applies to: 615-616
packages/rs-drive-abci/tests/strategy_tests/withdrawal_tests.rs (1)
213-228: LGTM! Consider parameterizing the expected locked amount.The addition of this assertion enhances the test by verifying the total locked amount in the withdrawal transactions sum tree. This is a valuable check that ensures the correct calculation and validation of the locked amount.
Consider parameterizing the expected locked amount (1,000,000) to make the test more flexible and easier to maintain. This could be done by calculating the expected amount based on the test data or by defining it as a constant at the beginning of the test function. For example:
let expected_locked_amount = 1_000_000; // Define this based on your test data // ... (rest of the test code) assert_eq!(locked_amount, expected_locked_amount);This change would make it easier to update the test if the expected amount changes in the future.
packages/strategy-tests/src/transitions.rs (3)
Line range hint
487-494: Update documentation to include theamount_rangeparameterThe function
create_identity_withdrawal_transitionnow accepts anamount_rangeparameter of typeAmountRange, but the documentation comment does not reflect this change. Please update the documentation to include the new parameter and explain its purpose.Apply this diff to update the documentation comment:
/// # Parameters /// - `identity`: A mutable reference to the identity making the withdrawal. +/// - `amount_range`: The range within which the withdrawal amount should be randomly generated. /// - `identity_nonce_counter`: A mutable reference to a BTreeMap that tracks the nonce for /// each identity, ensuring unique transaction identifiers. /// - `signer`: A mutable reference to the signer used to create the cryptographic signature for /// the state transition. +/// - `rng`: A mutable reference to a random number generator, used for generating the random +/// withdrawal amounts. /// /// # Returns /// - `StateTransition`: The constructed state transition representing the identity's credit
Line range hint
637-642: Update documentation to include theamount_rangeparameterThe function
create_identity_withdrawal_transition_with_output_addressnow accepts anamount_rangeparameter. Please update the documentation to include this parameter and describe its role in withdrawal amount generation.Apply this diff to update the documentation comment:
/// # Parameters /// - `identity`: A mutable reference to the identity making the withdrawal. +/// - `amount_range`: The range within which the withdrawal amount should be randomly generated. /// - `identity_nonce_counter`: A mutable reference to a BTreeMap that tracks the nonce for /// each identity, ensuring unique transaction identifiers. /// - `signer`: A mutable reference to the signer used to create the cryptographic signature for /// the state transition. +/// - `rng`: A mutable reference to a random number generator, used for generating the random +/// withdrawal amounts and output scripts. /// /// # Returns /// - `StateTransition`: The constructed and signed state transition representing the identity's
649-653: Add a comment to explain the random selection of output scriptsThe code randomly selects between
CoreScript::random_p2pkh(rng)andCoreScript::random_p2sh(rng)for theoutput_script. Including a comment to explain this logic can enhance code readability.Apply this diff to add an explanatory comment:
output_script: if rng.gen_bool(0.5) { + // Randomly select a Pay-to-Public-Key-Hash (P2PKH) script Some(CoreScript::random_p2pkh(rng)) } else { + // Randomly select a Pay-to-Script-Hash (P2SH) script Some(CoreScript::random_p2sh(rng)) },packages/strategy-tests/src/lib.rs (1)
1170-1173: Implement amount range handling inIdentityTopUpoperationThe
_amount_rangeparameter is introduced in theOperationType::IdentityTopUp(_amount_range)match arm but is currently not used within the implementation. There is a TODO comment indicating that amount ranges need to be implemented.Would you like assistance in implementing the amount range handling for the
IdentityTopUpoperation? I can help by providing code suggestions or opening a GitHub issue to track this task.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (31)
- packages/dashpay-contract/Cargo.toml (1 hunks)
- packages/data-contracts/Cargo.toml (1 hunks)
- packages/dpns-contract/Cargo.toml (1 hunks)
- packages/feature-flags-contract/Cargo.toml (1 hunks)
- packages/masternode-reward-shares-contract/Cargo.toml (1 hunks)
- packages/rs-dapi-client/Cargo.toml (1 hunks)
- packages/rs-dpp/src/identity/core_script.rs (1 hunks)
- packages/rs-dpp/src/withdrawal/daily_withdrawal_limit/v0/mod.rs (1 hunks)
- packages/rs-dpp/src/withdrawal/mod.rs (2 hunks)
- packages/rs-drive-abci/Cargo.toml (1 hunks)
- packages/rs-drive-abci/src/execution/platform_events/protocol_upgrade/perform_events_on_first_block_of_protocol_change/mod.rs (1 hunks)
- packages/rs-drive-abci/src/execution/platform_events/protocol_upgrade/perform_events_on_first_block_of_protocol_change/v0/mod.rs (1 hunks)
- packages/rs-drive-abci/src/execution/platform_events/withdrawals/build_untied_withdrawal_transactions_from_documents/mod.rs (2 hunks)
- packages/rs-drive-abci/src/execution/platform_events/withdrawals/build_untied_withdrawal_transactions_from_documents/v0/mod.rs (5 hunks)
- packages/rs-drive-abci/src/execution/platform_events/withdrawals/dequeue_and_build_unsigned_withdrawal_transactions/v0/mod.rs (1 hunks)
- packages/rs-drive-abci/tests/strategy_tests/withdrawal_tests.rs (2 hunks)
- packages/rs-drive/Cargo.toml (1 hunks)
- packages/rs-drive/src/drive/identity/withdrawals/transaction/queue/add_enqueue_untied_withdrawal_transaction_operations/mod.rs (3 hunks)
- packages/rs-drive/src/drive/identity/withdrawals/transaction/queue/mod.rs (1 hunks)
- packages/rs-drive/src/util/batch/drive_op_batch/withdrawals.rs (5 hunks)
- packages/rs-drive/src/util/grove_operations/batch_delete_items_in_path_query/v0/mod.rs (1 hunks)
- packages/rs-drive/src/util/grove_operations/batch_insert_if_not_exists_return_existing_element/mod.rs (1 hunks)
- packages/rs-drive/src/util/grove_operations/batch_insert_if_not_exists_return_existing_element/v0/mod.rs (1 hunks)
- packages/rs-json-schema-compatibility-validator/Cargo.toml (1 hunks)
- packages/rs-platform-value/Cargo.toml (1 hunks)
- packages/rs-sdk/Cargo.toml (2 hunks)
- packages/strategy-tests/src/lib.rs (2 hunks)
- packages/strategy-tests/src/operations.rs (5 hunks)
- packages/strategy-tests/src/transitions.rs (6 hunks)
- packages/withdrawals-contract/Cargo.toml (1 hunks)
- packages/withdrawals-contract/src/lib.rs (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (11)
- packages/rs-dpp/src/identity/core_script.rs
- packages/rs-dpp/src/withdrawal/mod.rs
- packages/rs-drive-abci/src/execution/platform_events/protocol_upgrade/perform_events_on_first_block_of_protocol_change/mod.rs
- packages/rs-drive-abci/src/execution/platform_events/withdrawals/build_untied_withdrawal_transactions_from_documents/mod.rs
- packages/rs-drive-abci/src/execution/platform_events/withdrawals/build_untied_withdrawal_transactions_from_documents/v0/mod.rs
- packages/rs-drive/src/drive/identity/withdrawals/transaction/queue/add_enqueue_untied_withdrawal_transaction_operations/mod.rs
- packages/rs-drive/src/drive/identity/withdrawals/transaction/queue/mod.rs
- packages/rs-drive/src/util/batch/drive_op_batch/withdrawals.rs
- packages/rs-drive/src/util/grove_operations/batch_delete_items_in_path_query/v0/mod.rs
- packages/rs-drive/src/util/grove_operations/batch_insert_if_not_exists_return_existing_element/mod.rs
- packages/rs-drive/src/util/grove_operations/batch_insert_if_not_exists_return_existing_element/v0/mod.rs
🧰 Additional context used
📓 Learnings (1)
packages/rs-dpp/src/withdrawal/daily_withdrawal_limit/v0/mod.rs (1)
Learnt from: QuantumExplorer PR: dashpay/platform#2182 File: packages/rs-dpp/src/withdrawal/daily_withdrawal_limit/v0/mod.rs:23-31 Timestamp: 2024-09-28T20:34:49.182Z Learning: `Credits` is a `u64`, and division by 10 won't cause an overflow.
🔇 Additional comments (25)
packages/dashpay-contract/Cargo.toml (1)
11-11: Dependency update looks good, but consider a few points.The update of
thiserrorfrom version 1.0.58 to 1.0.64 is a minor version bump, which is generally safe. However, please consider the following:
- Ensure that this update doesn't introduce any breaking changes that could affect the project.
- It might be worth checking if other dependencies can be updated as well for consistency.
- Verify that this change has been tested with the rest of the codebase to ensure compatibility.
To verify the impact of this change, you can run the following script:
This script will help identify any potential breaking changes in the
thiserrorchangelog and show wherethiserroris used in the project.✅ Verification successful
Dependency update verified. No breaking changes found.
The update of
thiserrorto version 1.0.64 does not introduce any breaking changes and is compatible with the project's usage.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any breaking changes in the thiserror changelog # Test: Search for breaking changes in the thiserror changelog curl -s https://raw.githubusercontent.com/dtolnay/thiserror/master/CHANGELOG.md | grep -i "breaking changes" -A 5 # Test: Check if thiserror is used in the project rg --type rust "use.*thiserror"Length of output: 22208
packages/feature-flags-contract/Cargo.toml (1)
10-10: Dependency version update looks good, but verify compatibility.The
thiserrordependency has been updated from version 1.0.58 to 1.0.64. This is a minor version update, which typically includes bug fixes and small improvements without breaking changes. However, it's important to ensure that this update doesn't introduce any unexpected behavior.To verify the impact of this change, please run the following script:
This script will fetch the changelog for
thiserrorand extract the relevant changes between versions 1.0.58 and 1.0.64. Review the output to ensure there are no breaking changes or significant updates that could affect your project.packages/masternode-reward-shares-contract/Cargo.toml (1)
10-10: Dependency update:thiserrorversion bumpThe
thiserrordependency has been updated from version 1.0.58 to 1.0.64. This is a minor version bump, which typically includes bug fixes and small improvements.To ensure this update doesn't introduce any breaking changes or conflicts, please run the following verification steps:
If these tests pass without any issues, the update can be considered safe. However, it's recommended to thoroughly test the entire project to ensure no unexpected behavior arises from this dependency update.
packages/rs-json-schema-compatibility-validator/Cargo.toml (1)
11-11: Dependency version update looks good, but consider additional checks.The update of
thiserrorfrom version 1.0.58 to 1.0.64 is a minor version bump, which typically indicates backward-compatible changes. This update is likely to be safe, but there are a few points to consider:
- Ensure that the new version is compatible with your Rust version and other dependencies.
- Check the changelog of
thiserrorfor any notable changes or improvements that might benefit your project.- Consider running your test suite to verify that the update doesn't introduce any unexpected behavior.
To ensure the update doesn't introduce any compatibility issues, you can run the following commands:
This script will attempt to build the project and run the test suite, which can help catch any potential issues introduced by the dependency update.
packages/withdrawals-contract/Cargo.toml (1)
10-10: Dependency update:thiserrorversion bumpThe
thiserrordependency has been updated from version 1.0.58 to 1.0.64. This is a minor version bump, which typically includes bug fixes and small improvements.To ensure this update doesn't introduce any breaking changes or conflicts, please run the following verification script:
packages/data-contracts/Cargo.toml (1)
10-10: Dependency update looks good, but verify compatibility.The update of
thiserrorfrom version 1.0.58 to 1.0.64 is a minor version change, which should be backward-compatible. However, it's important to ensure that this update doesn't introduce any unexpected behavior.To verify the impact of this change, please run the following script:
This script will help identify any potential issues introduced by the dependency update. If any breaking changes or deprecations are found, or if the build or tests fail, please review and address them before merging this PR.
packages/rs-platform-value/Cargo.toml (2)
Line range hint
1-35: PR description needs more details.The PR objectives mention implementing withdrawal limits, but this Cargo.toml update doesn't seem directly related to that feature. Please update the PR description to explain how this dependency update contributes to the withdrawal limits feature or if it's an unrelated maintenance task.
To check if there are other files in this PR that might be more relevant to the withdrawal limits feature, you can run:
#!/bin/bash # Description: List changed files in the PR # Test: List changed files gh pr view 2182 --json files --jq '.files[].path'
13-13: Dependency update:thiserrorversion bump.The
thiserrordependency has been updated from version 1.0.58 to 1.0.64. This is a minor version bump, which typically includes bug fixes and small improvements.To ensure compatibility and check for any breaking changes, please run the following script:
packages/rs-dpp/src/withdrawal/daily_withdrawal_limit/v0/mod.rs (3)
1-1: LGTM: Appropriate import statementThe import of
Creditsfrom thecrate::feemodule is correct and necessary for the function implementation.
3-20: Well-documented functionThe documentation for
daily_withdrawal_limit_v0is comprehensive and clear. It effectively explains the function's purpose, behavior, parameters, and return value. The rules for calculating the withdrawal limit are well-detailed.
32-35: LGTM: Proper test module setupThe test module is correctly declared with the
#[cfg(test)]attribute, ensuring that tests are only compiled when running tests. Theuse super::*;statement appropriately brings the tested function into scope.packages/rs-drive-abci/Cargo.toml (1)
26-26: Approve dependency update with verification recommendation.The update of the
thiserrordependency from version 1.0.58 to 1.0.64 is a good practice for maintaining project health and potentially improving error handling. However, please consider the following:
- Verify that this update doesn't introduce any unexpected changes in the project's error handling behavior.
- Update the PR description to mention this dependency update, as it's currently not aligned with the PR title "feat: withdrawal limits".
To ensure the update doesn't introduce any breaking changes, please run the project's test suite and verify that all tests pass. Additionally, you may want to check for any deprecation warnings or new features in the
thiserrorchangelog between versions 1.0.58 and 1.0.64.packages/rs-drive-abci/src/execution/platform_events/protocol_upgrade/perform_events_on_first_block_of_protocol_change/v0/mod.rs (2)
1-10: Import statements look good.The import statements are relevant to the functionality being implemented in this file. They cover error handling, platform types, version information, and necessary database-related modules.
1-74: Overall assessment: Good implementation with minor improvements needed.The file successfully implements protocol upgrade events for the Platform, focusing on the transition to version 4. The structure and logic are sound, with clear function names and helpful documentation.
Key points:
- The version check logic in
perform_events_on_first_block_of_protocol_change_v0is correct.- The
transition_to_version_4function properly initializes the required data structures.- The use of
grove_insert_if_not_existsensures idempotency in the upgrade process.Suggested improvements:
- Enhance error handling in both functions to provide more context when errors occur.
- Adjust return types to properly reflect the use of the
?operator and to provide more information to callers.These changes will improve the robustness and maintainability of the code. Once these minor issues are addressed, the implementation will be solid and ready for use in managing protocol upgrades.
packages/rs-sdk/Cargo.toml (6)
18-18: LGTM: Dependency update forthiserrorThe update from version 1.0.58 to 1.0.64 for the
thiserrorcrate is a minor version bump. This update is likely to include bug fixes and performance improvements while maintaining backwards compatibility.
20-20: LGTM: Dependency update fortokio-utilThe update from version 0.7.10 to 0.7.12 for the
tokio-utilcrate is a patch version bump. This update is likely to include only bug fixes and should be fully backwards compatible.
41-41: LGTM: Dev-dependency update fortokioThe update of the
tokiodev-dependency from version 1.36.0 to 1.40.0 is consistent with the maintokiodependency update. This ensures that the development environment matches the production environment, which is a good practice for maintaining consistency across testing and runtime behavior.
21-21: LGTM: Dependency update forasync-traitThe update from version 0.1.79 to 0.1.83 for the
async-traitcrate is a minor version bump within the 0.1.x series. This update is likely to include improvements and bug fixes while maintaining backwards compatibility.It's recommended to review the changelog for any notable changes or new features that could benefit the project. You can use the following command to view the changelog:
#!/bin/bash # Fetch the changelog for async-trait between versions 0.1.79 and 0.1.83 gh repo view dtolnay/async-trait --json url --jq .url | xargs -I {} gh api {}/contents/CHANGELOG.md --jq .content | base64 -d | sed -n '/## \[0.1.83\]/,/## \[0.1.79\]/p'
18-21: Summary: Routine dependency updatesThis change set includes minor version updates for several dependencies:
thiserror: 1.0.58 -> 1.0.64tokio: 1.36.0 -> 1.40.0 (both main and dev dependency)tokio-util: 0.7.10 -> 0.7.12async-trait: 0.1.79 -> 0.1.83These updates are likely to include bug fixes, performance improvements, and potentially new features while maintaining backwards compatibility. It's a good practice to keep dependencies up-to-date for security and stability reasons.
To ensure these updates don't introduce any unexpected changes, it's recommended to:
- Review the changelogs of the updated dependencies (as suggested in previous comments).
- Run the project's test suite to verify that all functionality remains intact.
- Perform a quick smoke test of the main features of the application.
You can use the following command to run the test suite:
#!/bin/bash # Run the test suite cargo test --all-featuresAlso applies to: 41-41
19-19: LGTM: Dependency update fortokioThe update from version 1.36.0 to 1.40.0 for the
tokiocrate is a minor version bump. This update is likely to include performance improvements and bug fixes while maintaining backwards compatibility. The features used remain unchanged.It's recommended to review the changelog for any notable changes or deprecations. You can use the following command to view the changelog:
packages/rs-drive-abci/src/execution/platform_events/withdrawals/dequeue_and_build_unsigned_withdrawal_transactions/v0/mod.rs (1)
153-154: Improved error handling for document revision overflow.The change from
CorruptedCodeExecutiontoOverflowerror type is a good improvement. It more accurately describes the nature of the potential issue and provides a clearer error message.To ensure this change doesn't introduce any inconsistencies, please run the following script to check for any other occurrences of
CorruptedCodeExecutionerrors related to document revision:This will help verify that all similar cases are handled consistently across the codebase.
✅ Verification successful
Verification of
CorruptedCodeExecutionremoval confirmed.No other occurrences of
CorruptedCodeExecutionerrors related to document revision were found. Allincrement_revision()calls are handled appropriately with proper error handling.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for other occurrences of CorruptedCodeExecution errors related to document revision # Test: Search for CorruptedCodeExecution errors related to document revision rg --type rust 'Error::Execution\(ExecutionError::CorruptedCodeExecution\(.*document revision.*\)\)' # Test: Search for other increment_revision calls to ensure consistent error handling rg --type rust 'increment_revision\(\)'Length of output: 3356
packages/strategy-tests/src/operations.rs (2)
546-548: LGTM. Serialization correctly handles newAmountRangeparameter.The
serialize_consume_to_bytes_with_platform_versionmethod has been updated to correctly handle the newAmountRangeparameter forIdentityTopUpandIdentityWithdrawaloperations. The implementation is consistent with the handling of other variants in the enum.Also applies to: 552-553
Line range hint
1-636: Overall, the changes effectively implement withdrawal limits.The modifications to this file successfully introduce the concept of withdrawal limits through the
AmountRangetype and its integration into theIdentityTopUpandIdentityWithdrawaloperations. The changes are consistent across the various structures and methods involved in serialization and deserialization.Key points:
- Introduction of
AmountRangetype alias.- Updates to
OperationTypeandOperationTypeInSerializationFormatenums.- Corresponding updates to serialization and deserialization methods.
These changes align well with the PR objective of implementing withdrawal limits. The use of
Range<Credits>allows for flexible specification of minimum and maximum values for both top-up and withdrawal operations.To further improve the implementation:
- Add documentation for the new
AmountRangetype and update existing documentation if necessary.- Ensure that serialization/deserialization tests are updated or added to cover the new
AmountRangeparameter.- Consider adding specific unit tests for the deserialization of modified operations.
packages/strategy-tests/src/transitions.rs (2)
504-507: Ensure correct propagation ofamount_rangeparameterWhen calling
create_identity_withdrawal_transition_sent_to_identity_transfer_key, theamount_rangeparameter is correctly passed. Verify that this parameter aligns with the intended range for withdrawal amounts.
512-514: Ensure correct propagation ofamount_rangeparameterWhen calling
create_identity_withdrawal_transition_with_output_address, theamount_rangeparameter is correctly passed. Verify that this parameter aligns with the intended range for withdrawal amounts.
| /// The documents are in the state and waiting to be processed. | ||
| QUEUED = 0, | ||
| /// Pooled happens when we are waiting for signing. | ||
| POOLED = 1, | ||
| /// We have broadcasted the transaction to core. | ||
| BROADCASTED = 2, | ||
| /// The transaction is now complete. | ||
| COMPLETE = 3, | ||
| /// We broadcasted the transaction but core never saw it or rejected it. |
There was a problem hiding this comment.
💡 Codebase verification
Unhandled WithdrawalStatus Variants Detected
The verification reveals that while the WithdrawalStatus enum is extensively used across the codebase, there are no match expressions handling its variants. Specifically, the newly added EXPIRED variant does not appear to be addressed in any pattern matching constructs. This oversight can lead to unhandled cases, potentially causing runtime issues or unexpected behaviors.
Action Items:
- Update Match Statements: Ensure that all existing match expressions involving
WithdrawalStatusinclude cases for the newEXPIREDvariant. - Add Comprehensive Handling: Review and modify any conditional logic or pattern matching to accommodate all possible enum variants.
- Testing: Implement tests to verify that each
WithdrawalStatusvariant is correctly handled in all relevant parts of the application.
🔗 Analysis chain
Verify usage of WithdrawalStatus enum across the codebase.
The addition of new variants to the WithdrawalStatus enum may have implications on other parts of the codebase. To ensure the changes are properly integrated:
- Check for any switch statements or pattern matching constructs that use
WithdrawalStatus. These may need to be updated to handle the new variants. - Verify that any serialization or deserialization logic involving
WithdrawalStatusis updated if necessary. - Update any documentation or API references that describe the possible withdrawal statuses.
Run the following script to identify potential areas that may need updates:
Please review the output and update the relevant parts of the codebase accordingly.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find usages of WithdrawalStatus in the codebase
# Search for WithdrawalStatus usage
echo "Searching for WithdrawalStatus usage:"
rg --type rust -A 5 "WithdrawalStatus"
# Search for potential match expressions on WithdrawalStatus
echo "\nSearching for potential match expressions:"
ast-grep --lang rust --pattern $'match $_ {
WithdrawalStatus::$_ => $$$,
$$$
}'
Length of output: 42036
| pub(super) fn perform_events_on_first_block_of_protocol_change_v0( | ||
| &self, | ||
| transaction: &Transaction, | ||
| previous_protocol_version: ProtocolVersion, | ||
| platform_version: &PlatformVersion, | ||
| ) -> Result<(), Error> { | ||
| if previous_protocol_version < 4 && platform_version.protocol_version >= 4 { | ||
| self.transition_to_version_4(transaction, platform_version); | ||
| } | ||
|
|
||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider improving error handling and clarifying the return value.
The function logic looks good, but there are two points to consider:
-
Error Handling: The call to
self.transition_to_version_4doesn't handle potential errors. Consider propagating any errors that might occur during the transition. -
Return Value: The function always returns
Ok(()), even if no transition occurred. It might be helpful to return a boolean indicating whether a transition was performed.
Here's a suggested improvement:
pub(super) fn perform_events_on_first_block_of_protocol_change_v0(
&self,
transaction: &Transaction,
previous_protocol_version: ProtocolVersion,
platform_version: &PlatformVersion,
) -> Result<bool, Error> {
if previous_protocol_version < 4 && platform_version.protocol_version >= 4 {
self.transition_to_version_4(transaction, platform_version)?;
Ok(true)
} else {
Ok(false)
}
}This change would provide more information to the caller about whether a transition occurred and propagate any errors from the transition process.
| fn transition_to_version_4( | ||
| &self, | ||
| transaction: &Transaction, | ||
| platform_version: &PlatformVersion, | ||
| ) { | ||
| let path = get_withdrawal_root_path(); | ||
| self.drive.grove_insert_if_not_exists( | ||
| (&path).into(), | ||
| &WITHDRAWAL_TRANSACTIONS_SUM_AMOUNT_TREE_KEY, | ||
| Element::empty_sum_tree(), | ||
| Some(transaction), | ||
| None, | ||
| &platform_version.drive, | ||
| )?; | ||
| } |
There was a problem hiding this comment.
Fix return type and consider improving error handling.
The function logic is correct, but there are two issues to address:
-
Return Type: The function uses the
?operator but doesn't have a return type ofResult<(), Error>. This will cause a compilation error. -
Error Handling: While the
?operator propagates errors, it doesn't add context. Consider using.map_err()to add more specific error information.
Here's a suggested improvement:
fn transition_to_version_4(
&self,
transaction: &Transaction,
platform_version: &PlatformVersion,
) -> Result<(), Error> {
let path = get_withdrawal_root_path();
self.drive.grove_insert_if_not_exists(
(&path).into(),
&WITHDRAWAL_TRANSACTIONS_SUM_AMOUNT_TREE_KEY,
Element::empty_sum_tree(),
Some(transaction),
None,
&platform_version.drive,
)
.map_err(|e| Error::DriveError(format!("Failed to initialize withdrawal sum tree: {}", e)))
}This change fixes the return type and adds more context to any errors that might occur during the insertion process.
| IdentityTopUp(AmountRange), | ||
| IdentityUpdate(IdentityUpdateOp), | ||
| IdentityWithdrawal, | ||
| IdentityWithdrawal(AmountRange), |
There was a problem hiding this comment.
💡 Codebase verification
Missing serialization/deserialization tests for IdentityTopUp and IdentityWithdrawal.
The changes to the IdentityTopUp and IdentityWithdrawal variants in OperationTypeInSerializationFormat introduce AmountRange. However, no corresponding serialization or deserialization tests were found. It's crucial to add these tests to ensure that these operations are correctly handled during the serialization and deserialization processes.
🔗 Analysis chain
LGTM. Verify serialization/deserialization tests.
The modification of IdentityTopUp and IdentityWithdrawal variants in OperationTypeInSerializationFormat to include AmountRange ensures consistency with the main OperationType enum. This is crucial for maintaining correct operation representation during serialization and deserialization.
Please ensure that the serialization and deserialization tests for OperationType are updated to cover the new AmountRange parameter for IdentityTopUp and IdentityWithdrawal operations. Run the following script to check for existing tests:
Since no serialization/deserialization tests for IdentityTopUp and IdentityWithdrawal were found, let's broaden our search to ensure comprehensive coverage.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for serialization/deserialization tests related to IdentityTopUp and IdentityWithdrawal
rg -i "test.*identity(topup|withdrawal).*serializ" --type rust
Length of output: 64
Script:
#!/bin/bash
# Search for any tests related to OperationTypeInSerializationFormat
rg -i "test.*operationtypeinserializationformat.*serializ" --type rust
Length of output: 72
Script:
#!/bin/bash
# Search for tests related to serialization or deserialization in general
rg -i "test.*(serialize|deserialize).*" --type rust
Length of output: 2852
Script:
#!/bin/bash
# Search for tests specifically covering IdentityTopUp and IdentityWithdrawal operations
rg -i "test.*(IdentityTopUp|IdentityWithdrawal).*" --type rust
Length of output: 64
| let mut withdrawal: StateTransition = IdentityCreditWithdrawalTransitionV1 { | ||
| identity_id: identity.id(), | ||
| amount: 1000000, // 1 duff | ||
| amount: rng.gen_range(amount_range), |
There was a problem hiding this comment.
Validate amount_range before generating withdrawal amounts
Ensure that amount_range provided to rng.gen_range(amount_range) results in valid withdrawal amounts. Consider adding validation to prevent zero or negative values, which could cause unexpected behavior.
| let mut withdrawal: StateTransition = IdentityCreditWithdrawalTransitionV1 { | ||
| identity_id: identity.id(), | ||
| amount: 1000000, // 1 duff | ||
| amount: rng.gen_range(amount_range), |
There was a problem hiding this comment.
Validate amount_range before generating withdrawal amounts
In the assignment amount: rng.gen_range(amount_range), it's important to ensure that amount_range is valid. Adding validation checks can prevent potential issues with invalid withdrawal amounts.
| amount_range: AmountRange, | ||
| identity_nonce_counter: &mut BTreeMap<Identifier, u64>, | ||
| signer: &mut SimpleSigner, | ||
| rng: &mut StdRng, | ||
| ) -> StateTransition { | ||
| let nonce = identity_nonce_counter.entry(identity.id()).or_default(); |
There was a problem hiding this comment.
Update documentation to include the amount_range parameter
The function create_identity_withdrawal_transition_sent_to_identity_transfer_key now accepts an amount_range parameter. The documentation should be updated to reflect this addition and explain how the amount is generated.
Apply this diff to update the documentation comment:
/// # Parameters
/// - `identity`: A mutable reference to the identity making the withdrawal.
+/// - `amount_range`: The range within which the withdrawal amount should be randomly generated.
/// - `identity_nonce_counter`: A mutable reference to a BTreeMap that tracks the nonce for
/// each identity, ensuring unique transaction identifiers.
+/// - `signer`: A mutable reference to the signer used to create the cryptographic signature for
+/// the state transition.
+/// - `rng`: A mutable reference to a random number generator, used for generating the random
+/// withdrawal amounts.
///
/// # Returns
/// - `StateTransition`: The constructed and signed state transition representing the identity's📝 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.
| amount_range: AmountRange, | |
| identity_nonce_counter: &mut BTreeMap<Identifier, u64>, | |
| signer: &mut SimpleSigner, | |
| rng: &mut StdRng, | |
| ) -> StateTransition { | |
| let nonce = identity_nonce_counter.entry(identity.id()).or_default(); | |
| /// # Parameters | |
| /// - `identity`: A mutable reference to the identity making the withdrawal. | |
| /// - `amount_range`: The range within which the withdrawal amount should be randomly generated. | |
| /// - `identity_nonce_counter`: A mutable reference to a BTreeMap that tracks the nonce for | |
| /// each identity, ensuring unique transaction identifiers. | |
| /// - `signer`: A mutable reference to the signer used to create the cryptographic signature for | |
| /// the state transition. | |
| /// - `rng`: A mutable reference to a random number generator, used for generating the random | |
| /// withdrawal amounts. | |
| /// | |
| /// # Returns | |
| /// - `StateTransition`: The constructed and signed state transition representing the identity's | |
| amount_range: AmountRange, | |
| identity_nonce_counter: &mut BTreeMap<Identifier, u64>, | |
| signer: &mut SimpleSigner, | |
| rng: &mut StdRng, | |
| ) -> StateTransition { | |
| let nonce = identity_nonce_counter.entry(identity.id()).or_default(); |
| OperationType::IdentityWithdrawal(amount_range) | ||
| if !current_identities.is_empty() => | ||
| { | ||
| for i in 0..count { | ||
| let index = (i as usize) % current_identities.len(); | ||
| let random_identity = &mut current_identities[index]; | ||
| let state_transition = | ||
| crate::transitions::create_identity_withdrawal_transition( | ||
| random_identity, | ||
| amount_range.clone(), |
There was a problem hiding this comment.
Ensure consistent handling of amount_range in IdentityWithdrawal and IdentityTopUp operations
In the OperationType::IdentityWithdrawal(amount_range) match arm, the amount_range parameter is passed to create_identity_withdrawal_transition, suggesting that withdrawal amounts are handled using the provided range. However, in the IdentityTopUp operation, the amount_range parameter is not utilized.
For consistency and to fully implement withdrawal limits as outlined in the PR objectives, consider implementing the amount range handling in both operations.
There was a problem hiding this comment.
Actionable comments posted: 12
🧹 Outside diff range and nitpick comments (17)
packages/rs-drive/src/util/batch/drive_op_batch/withdrawals.rs (2)
39-45: LGTM: New ReserveWithdrawalAmount variant added.The new
ReserveWithdrawalAmountvariant has been successfully added to theWithdrawalOperationTypeenum, implementing the functionality for reserving withdrawal amounts with an expiration time.Consider adding a brief explanation of the
expiration_afterfield in the documentation comment. For example:/// expiration date (in milliseconds from the current block time) expiration_after: TimestampMillis,This would clarify that the expiration is relative to the current block time, not an absolute timestamp.
Line range hint
55-121: LGTM: Implementation for ReserveWithdrawalAmount added.The changes successfully implement the handling of the new
ReserveWithdrawalAmountvariant in theinto_low_level_drive_operationsmethod. The addition of theblock_infoparameter and the calculation of the expiration date are correct and necessary for the new functionality.Consider adding a brief comment explaining the purpose of the
expiration_datecalculation:// Calculate the absolute expiration timestamp by adding the relative expiration time to the current block time let expiration_date = block_info.time_ms + expiration_after;This would improve code readability and make the intention clearer for future maintainers.
packages/rs-drive-abci/src/execution/platform_events/withdrawals/update_broadcasted_withdrawal_statuses/v0/mod.rs (1)
110-115: LGTM! Consider caching the expiration blocks value.The change from a hardcoded constant to a dynamic value from
platform_version.drive_abci.withdrawal_constants.core_expiration_blocksimproves flexibility and maintainability. This allows for easier configuration changes without code modifications.To optimize performance, consider caching the
core_expiration_blocksvalue at the beginning of the function:let core_expiration_blocks = platform_version .drive_abci .withdrawal_constants .core_expiration_blocks; // ... later in the function ... if block_height_difference > core_expiration_blocks { // ... }This would avoid repeated access to the nested structure in case of multiple withdrawals being processed.
packages/rs-drive-abci/src/execution/engine/run_block_proposal/v0/mod.rs (1)
329-339: LGTM! Consider adding more detailed documentation.The addition of
clean_up_expired_locks_of_withdrawal_amountsis a good implementation for managing withdrawal limits. It's correctly placed after pooling withdrawals and uses appropriate error handling.Consider adding more detailed documentation for this method, including:
- The specific conditions for a lock to be considered "expired".
- The impact of this cleanup on the overall withdrawal limit system.
- Any potential edge cases or important considerations for future maintenance.
This additional documentation will help maintain the code and understand its purpose in the long term.
packages/strategy-tests/src/transitions.rs (5)
80-97: LGTM: New function for dynamic range asset lock proofThe new function
instant_asset_lock_proof_fixture_with_dynamic_rangeis a good addition, allowing for dynamic amount ranges in asset lock proofs. This is consistent with the goal of implementing withdrawal limits.Consider adding documentation for this new function, similar to the existing
instant_asset_lock_proof_fixturefunction. This will help maintain consistency in code documentation and improve readability. Here's a suggested documentation block:/// Constructs an `AssetLockProof` representing an instant asset lock proof with a dynamic amount range. /// /// This function is similar to `instant_asset_lock_proof_fixture`, but allows for a dynamic amount /// range to be specified. /// /// # Parameters /// - `one_time_private_key`: A unique private key (`PrivateKey`) utilized for generating the underlying locking transaction. /// - `amount_range`: The range of amounts (`AmountRange`) from which the locked amount will be randomly selected. /// - `rng`: A mutable reference to a random number generator. /// /// # Returns /// - `AssetLockProof`: An asset lock proof derived from the instant asset lock mechanism with a dynamically determined amount.
Line range hint
572-602: LGTM: Updated withdrawal transition function with dynamic amount rangeThe
create_identity_withdrawal_transitionfunction has been correctly updated to include and use theamount_rangeparameter. This change is consistent with the goal of implementing dynamic withdrawal amounts.Consider updating the function documentation to reflect the new
amount_rangeparameter. Here's a suggested addition to the existing documentation:/// # Parameters /// ... /// - `amount_range`: The range of amounts (`AmountRange`) from which the withdrawal amount will be randomly selected. /// ...This will help maintain up-to-date and accurate documentation for the function.
639-642: LGTM: Updated function signature for dynamic amount rangeThe
create_identity_withdrawal_transition_sent_to_identity_transfer_keyfunction has been correctly updated to includeamount_rangeandrngparameters, which is consistent with the goal of implementing dynamic withdrawal amounts.Please update the function documentation to reflect the new parameters. Here's a suggested addition to the existing documentation:
/// # Parameters /// ... /// - `amount_range`: The range of amounts (`AmountRange`) from which the withdrawal amount will be randomly selected. /// - `rng`: A mutable reference to a random number generator, used for generating the random withdrawal amount. /// ...This will ensure that the documentation accurately reflects the function's current signature and behavior.
Line range hint
722-738: LGTM: Updated withdrawal transition function with dynamic amount and random output scriptThe
create_identity_withdrawal_transition_with_output_addressfunction has been correctly updated to include theamount_rangeparameter and implement dynamic withdrawal amounts. The addition of randomly choosing between P2PKH and P2SH output scripts adds desirable variability.Please update the function documentation to reflect the new
amount_rangeparameter and the randomized output script selection. Here's a suggested addition to the existing documentation:/// # Parameters /// ... /// - `amount_range`: The range of amounts (`AmountRange`) from which the withdrawal amount will be randomly selected. /// ... /// /// The function now randomly selects between a Pay-to-Public-Key-Hash (P2PKH) and /// a Pay-to-Script-Hash (P2SH) output script for added variability.This will ensure that the documentation accurately reflects the function's current behavior.
Line range hint
1023-1040: LGTM: Updated state transition creation with dynamic amount rangeThe
create_state_transitions_for_identitiesfunction has been correctly updated to include theamount_rangeparameter and use the newinstant_asset_lock_proof_fixture_with_dynamic_rangefunction. These changes are consistent with the goal of implementing dynamic amount ranges throughout the system.Please update the function documentation to reflect the new
amount_rangeparameter. Here's a suggested addition to the existing documentation:/// # Parameters /// ... /// - `amount_range`: A reference to the range of amounts (`&AmountRange`) from which the asset lock amount will be randomly selected for each identity. /// ...This will ensure that the documentation accurately reflects the function's current signature and behavior.
packages/rs-drive-abci/tests/strategy_tests/execution.rs (1)
Line range hint
1-1212: General code review observations
The file is quite long (1212 lines) and contains multiple complex functions. Consider breaking it down into smaller, more manageable modules.
There are numerous
expectcalls throughout the code, which could potentially panic. Consider using proper error handling withResulttypes for better error propagation.The
run_chain_for_strategyfunction is particularly long and complex. Consider refactoring it into smaller, more focused functions for better readability and maintainability.There are several TODO comments in the code. It would be beneficial to address these or create issues to track them.
The code uses a mix of
BTreeMapandHashMap. Ensure that the choice of map type is consistent and appropriate for each use case.Consider refactoring the large functions, particularly
run_chain_for_strategy, into smaller, more focused functions. This will improve readability and make the code easier to maintain and test.Given the complexity of this test file, consider splitting it into multiple files or modules, each focusing on a specific aspect of the strategy tests. This will make the codebase more modular and easier to navigate.
packages/rs-drive-abci/tests/strategy_tests/voting_tests.rs (2)
84-84: LGTM: Currency conversion applied in function call.The use of
dash_to_duffs!(1)..=dash_to_duffs!(1)is correct and consistent with the new import. However, since it creates a range with a single value, you might consider simplifying it.Consider simplifying the range to just
dash_to_duffs!(1)if a single value is intended, or clarify the reason for using a range if it's necessary for the test setup.
993-993: LGTM: Consistent use of currency conversion across test cases.The use of
dash_to_duffs!(1)..=dash_to_duffs!(1)is consistent with all previous instances.Given that this pattern is repeated across multiple test cases, consider refactoring to use a constant or a helper function. This would improve maintainability and reduce duplication. For example:
const TEST_DASH_AMOUNT: RangeInclusive<u64> = dash_to_duffs!(1)..=dash_to_duffs!(1); // Then use it in all test cases: create_state_transitions_for_identities( vec![identity1, identity2], &TEST_DASH_AMOUNT, &simple_signer, &mut rng, platform_version, )This refactoring would make it easier to update the test amount across all test cases if needed in the future.
packages/rs-drive-abci/src/execution/platform_events/withdrawals/cleanup_expired_locks_of_withdrawal_amounts/v0/mod.rs (2)
28-28: Improve code comment for clarityThe comment
// No clean upcan be more descriptive to enhance readability. Consider updating it to// Cleanup limit is zero; skipping cleanupfor better clarity.
16-68: Add documentation comments to the new methodTo improve maintainability and understandability, consider adding Rust documentation comments (
///) to thecleanup_expired_locks_of_withdrawal_amounts_v0method. Documenting the purpose, parameters, and return value will help other developers understand its functionality.packages/rs-drive-abci/tests/strategy_tests/strategy.rs (1)
Line range hint
1123-1135: Prevent potential panic when selecting random identitiesWhen using
choose_multiple(rng, count as usize), ifcount as usizeexceedscurrent_identities.len(), the function will panic. To avoid this, ensure thatcountdoes not exceed the number of available identities.Apply this diff to safeguard against panic:
+ let count = std::cmp::min(count as u16, current_identities.len() as u16); let indices: Vec<usize> = - (0..current_identities.len()).choose_multiple(rng, count as usize); + (0..current_identities.len()).choose_multiple(rng, count as usize);Alternatively:
let indices: Vec<usize> = - (0..current_identities.len()).choose_multiple(rng, count as usize); + (0..current_identities.len()).choose_multiple(rng, std::cmp::min(count as usize, current_identities.len()));packages/rs-drive-abci/tests/strategy_tests/withdrawal_tests.rs (2)
Line range hint
42-720: Refactor duplicated setup code to improve maintainabilityThe test functions
run_chain_withdraw_from_identitiesandrun_chain_withdrawal_expiredcontain a significant amount of duplicated code, particularly in the initialization ofstart_strategy,config, and the setup ofplatformandshared_core_state. Refactoring this common setup into helper functions or modules can enhance code maintainability, reduce duplication, and make the tests easier to read and modify.Also applies to: 722-1226
1229-1230: Simplify the test function name for readabilityThe test function
run_chain_withdraw_from_identities_too_many_withdrawals_within_a_day_hitting_limithas a very long name, which can make the code harder to read and navigate. Consider simplifying the function name or splitting the test into smaller, focused tests with shorter names to improve readability and maintainability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (26)
- packages/rs-dpp/src/util/units.rs (1 hunks)
- packages/rs-drive-abci/src/execution/engine/run_block_proposal/mod.rs (1 hunks)
- packages/rs-drive-abci/src/execution/engine/run_block_proposal/v0/mod.rs (1 hunks)
- packages/rs-drive-abci/src/execution/platform_events/protocol_upgrade/perform_events_on_first_block_of_protocol_change/mod.rs (1 hunks)
- packages/rs-drive-abci/src/execution/platform_events/protocol_upgrade/perform_events_on_first_block_of_protocol_change/v0/mod.rs (1 hunks)
- packages/rs-drive-abci/src/execution/platform_events/withdrawals/cleanup_expired_locks_of_withdrawal_amounts/mod.rs (1 hunks)
- packages/rs-drive-abci/src/execution/platform_events/withdrawals/cleanup_expired_locks_of_withdrawal_amounts/v0/mod.rs (1 hunks)
- packages/rs-drive-abci/src/execution/platform_events/withdrawals/mod.rs (1 hunks)
- packages/rs-drive-abci/src/execution/platform_events/withdrawals/update_broadcasted_withdrawal_statuses/v0/mod.rs (1 hunks)
- packages/rs-drive-abci/tests/strategy_tests/execution.rs (1 hunks)
- packages/rs-drive-abci/tests/strategy_tests/main.rs (12 hunks)
- packages/rs-drive-abci/tests/strategy_tests/strategy.rs (13 hunks)
- packages/rs-drive-abci/tests/strategy_tests/voting_tests.rs (6 hunks)
- packages/rs-drive-abci/tests/strategy_tests/withdrawal_tests.rs (21 hunks)
- packages/rs-drive/src/drive/identity/withdrawals/transaction/queue/add_enqueue_untied_withdrawal_transaction_operations/v0/mod.rs (2 hunks)
- packages/rs-drive/src/util/batch/drive_op_batch/withdrawals.rs (5 hunks)
- packages/rs-platform-version/src/version/drive_abci_versions.rs (3 hunks)
- packages/rs-platform-version/src/version/mocks/v2_test.rs (9 hunks)
- packages/rs-platform-version/src/version/mocks/v3_test.rs (9 hunks)
- packages/rs-platform-version/src/version/v1.rs (9 hunks)
- packages/rs-platform-version/src/version/v2.rs (9 hunks)
- packages/rs-platform-version/src/version/v3.rs (9 hunks)
- packages/rs-platform-version/src/version/v4.rs (9 hunks)
- packages/strategy-tests/src/lib.rs (5 hunks)
- packages/strategy-tests/src/operations.rs (6 hunks)
- packages/strategy-tests/src/transitions.rs (10 hunks)
🚧 Files skipped from review as they are similar to previous changes (11)
- packages/rs-drive-abci/src/execution/engine/run_block_proposal/mod.rs
- packages/rs-drive-abci/src/execution/platform_events/protocol_upgrade/perform_events_on_first_block_of_protocol_change/mod.rs
- packages/rs-drive-abci/src/execution/platform_events/protocol_upgrade/perform_events_on_first_block_of_protocol_change/v0/mod.rs
- packages/rs-drive/src/drive/identity/withdrawals/transaction/queue/add_enqueue_untied_withdrawal_transaction_operations/v0/mod.rs
- packages/rs-platform-version/src/version/mocks/v2_test.rs
- packages/rs-platform-version/src/version/v1.rs
- packages/rs-platform-version/src/version/v2.rs
- packages/rs-platform-version/src/version/v3.rs
- packages/rs-platform-version/src/version/v4.rs
- packages/strategy-tests/src/lib.rs
- packages/strategy-tests/src/operations.rs
🧰 Additional context used
🪛 Gitleaks
packages/strategy-tests/src/transitions.rs
189-189: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🔇 Additional comments (41)
packages/rs-drive/src/util/batch/drive_op_batch/withdrawals.rs (3)
5-6: LGTM: Unused import removed and necessary constant added.The changes address the previous review comment by removing the unused import
get_withdrawal_transactions_sum_tree_path. The addition ofWITHDRAWAL_TRANSACTIONS_NEXT_INDEX_KEYis likely related to the new functionality for withdrawal limits.
8-8: LGTM: New import added for BatchDeleteApplyType.The addition of
BatchDeleteApplyTypeimport is likely necessary for the new functionality or modifications in the existing code.
14-15: LGTM: New imports added for Credits, SignedCredits, and TimestampMillis.These new imports are likely necessary for the implementation of the
ReserveWithdrawalAmountfunctionality mentioned in the AI summary.packages/rs-drive-abci/src/execution/platform_events/withdrawals/update_broadcasted_withdrawal_statuses/v0/mod.rs (2)
Line range hint
1-321: Overall assessment: Approved with suggestions for improvementThe changes in this file enhance the flexibility of the withdrawal expiration mechanism by using a dynamic threshold. This is a positive improvement that allows for easier configuration without code changes.
Key points:
- The main logic change is sound and improves the system's adaptability.
- A minor optimization could be made by caching the expiration threshold value.
- The test coverage should be expanded to explicitly verify the new dynamic threshold behavior.
These improvements will ensure that the new functionality is both efficient and thoroughly tested.
Line range hint
210-321: Enhance test coverage for the new dynamic expiration threshold.While the existing test case
test_statuses_are_updatedchecks the basic functionality, it doesn't explicitly verify the new dynamic expiration threshold logic.Consider adding a new test case or modifying the existing one to cover the following scenarios:
- A withdrawal that should expire based on the dynamic threshold.
- A withdrawal that shouldn't expire when the dynamic threshold is changed.
This will ensure that the new flexibility in setting the expiration threshold is working as expected.
To assist with this, you can use the following script to locate the test file and relevant test cases:
packages/strategy-tests/src/transitions.rs (3)
Line range hint
36-40: LGTM: New imports are consistent with changesThe added imports are necessary for the new functionality related to dynamic amount ranges and withdrawal transactions. This change is consistent with the modifications throughout the file.
648-648: Implementation of dynamic withdrawal amountThe withdrawal amount is now correctly generated using
rng.gen_range(amount_range), which implements the desired dynamic withdrawal amount functionality.As mentioned in a previous review comment, it's important to validate the
amount_rangebefore using it to generate the withdrawal amount. Consider adding a check to ensure the range is valid and non-empty to prevent potential panics or unexpected behavior.
Line range hint
1-1080: Overall assessment: Well-implemented dynamic amount ranges with some suggestions for improvementThe changes in this file consistently implement dynamic amount ranges across various functions related to identity state transitions and asset lock proofs. This is in line with the goal of implementing withdrawal limits within the platform.
Key points:
- Dynamic amount ranges are correctly implemented and propagated through relevant functions.
- New functions for creating asset lock proofs and transactions with dynamic amounts are well-structured.
- The addition of randomized output script selection in withdrawal transitions adds desirable variability.
Suggestions for improvement:
- Update function documentation throughout the file to reflect new parameters and behaviors, especially for
amount_rangeandrngadditions.- Implement input validation for
amount_rangeto prevent potential issues with invalid or empty ranges.- Consider adding more comprehensive error handling for cases where invalid amount ranges might be provided.
These changes significantly enhance the flexibility of the system in handling various withdrawal scenarios. With the suggested improvements, the code will be more robust and better documented.
🧰 Tools
🪛 Gitleaks
189-189: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
packages/rs-drive-abci/tests/strategy_tests/execution.rs (1)
1212-1212: New parametersigneradded tocontinue_chain_for_strategyfunctionThe function signature has been updated to include a new parameter
signer. This change aligns with the AI-generated summary and expands the function's interface.To ensure this change is consistently applied throughout the codebase, let's check for any other occurrences of this function:
✅ Verification successful
Change Verified:
signerParameter Addition Limited to Test FileThe
signerparameter was added exclusively to thecontinue_chain_for_strategyfunction withinpackages/rs-drive-abci/tests/strategy_tests/execution.rs. No other occurrences of this function were found in the codebase, ensuring that the change is isolated to the test scope.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
rg "fn continue_chain_for_strategy" --type rustLength of output: 149
packages/rs-platform-version/src/version/mocks/v3_test.rs (7)
35-35: LGTM: New import added for withdrawal constants.The addition of
DriveAbciWithdrawalConstantsimport is appropriate and aligns with the new withdrawal-related constants introduced in this update.
573-573: New Grove batch methods added for optimized operations.Two new methods have been added to
DriveGroveBatchMethodVersions:
batch_insert_sum_item_or_add_to_if_already_existsbatch_delete_items_in_path_queryThese methods could potentially improve performance for certain batch operations.
Please ensure that these new methods are implemented correctly and their usage provides the expected performance improvements. You can verify this by running:
#!/bin/bash # Search for the implementations of the new methods rg --type rust "fn batch_insert_sum_item_or_add_to_if_already_exists" rg --type rust "fn batch_delete_items_in_path_query" # Search for usage of these new methods across the codebase rg --type rust "batch_insert_sum_item_or_add_to_if_already_exists" rg --type rust "batch_delete_items_in_path_query" # Look for any performance tests or benchmarks related to these methods rg --type rust "test.*batch_insert_sum_item_or_add_to_if_already_exists" rg --type rust "test.*batch_delete_items_in_path_query" rg --type rust "bench.*batch_insert_sum_item_or_add_to_if_already_exists" rg --type rust "bench.*batch_delete_items_in_path_query"Also applies to: 579-579
551-551: New Grove method added for optimized insert-or-get operation.The addition of
grove_insert_if_not_exists_return_existing_elementwith version 0 introduces a new method that could optimize certain operations by combining insert and get functionalities.Please ensure that this new method is implemented correctly and its usage is optimized across the codebase. You can verify this by running:
#!/bin/bash # Search for the implementation of the new method rg --type rust "fn grove_insert_if_not_exists_return_existing_element" # Search for usage of this new method across the codebase rg --type rust "grove_insert_if_not_exists_return_existing_element"
678-678: New method added for cleaning up expired withdrawal locks.The addition of
cleanup_expired_locks_of_withdrawal_amountswith version 0 introduces a new method for maintaining the integrity of the withdrawal system. This aligns with the PR objective of implementing withdrawal limits and ensures that expired locks are properly managed.Please ensure that this new method is implemented correctly and thoroughly tested. You can verify this by running:
#!/bin/bash # Search for the implementation of the new method rg --type rust "fn cleanup_expired_locks_of_withdrawal_amounts" # Search for tests related to this new method rg --type rust "test.*cleanup_expired_locks_of_withdrawal_amounts" # Look for any usage of this method in the withdrawal process rg --type rust "cleanup_expired_locks_of_withdrawal_amounts"
1296-1297:⚠️ Potential issueNew withdrawal limits added, but some concerns need addressing.
Two new fields have been added to
SystemLimits:
max_withdrawal_amount: 50_000_000_000_000daily_withdrawal_limit: NoneWhile the addition of these fields aligns with the PR objective of implementing withdrawal limits, there are some concerns:
- The
max_withdrawal_amountis set to a very large value (50 trillion units). Is this intentional? If so, what's the reasoning behind this specific limit?- The
daily_withdrawal_limitis set toNone. Does this mean there's no daily limit enforced? If so, is this intentional, and what are the security implications?Please address the following:
- Confirm if the
max_withdrawal_amountis set correctly and provide reasoning for the chosen value.- Clarify the intention behind setting
daily_withdrawal_limittoNoneand explain how this aligns with the goal of implementing withdrawal limits.- If these are placeholder values, please update them to more appropriate limits or add TODOs for future implementation.
To ensure these limits are properly implemented and used, please run:
#!/bin/bash # Search for usage of max_withdrawal_amount rg --type rust "max_withdrawal_amount" # Search for usage of daily_withdrawal_limit rg --type rust "daily_withdrawal_limit" # Look for any logic implementing these limits rg --type rust "fn.*withdrawal.*limit"
643-643: New method added for handling protocol change events.The addition of
perform_events_on_first_block_of_protocol_changewith a value ofNonesuggests a new feature for handling events during protocol upgrades. However, its current status is unclear.Please clarify the current status and implementation plans for this method. Is it intended to be optional, or is it not yet implemented? You can check for any existing implementations or TODOs by running:
#!/bin/bash # Search for any implementations or TODOs related to this method rg --type rust "fn perform_events_on_first_block_of_protocol_change" rg --type rust "TODO.*perform_events_on_first_block_of_protocol_change"
480-480: New method added for calculating withdrawal limit.The addition of
calculate_current_withdrawal_limitwith version 0 suggests a new feature for calculating withdrawal limits. This aligns with the PR objective of implementing withdrawal limits.Please ensure that the corresponding method is implemented and properly tested. You can verify this by running:
packages/rs-drive-abci/tests/strategy_tests/voting_tests.rs (3)
27-27: LGTM: New import added for currency conversion.The addition of
use dpp::dash_to_duffs;is consistent with the expected changes and will likely be used for currency conversion in the test cases.
368-368: LGTM: Consistent use of currency conversion.The use of
dash_to_duffs!(1)..=dash_to_duffs!(1)is consistent with the previous instance.As mentioned in the previous comment, consider simplifying this range if a single value is intended.
640-640: LGTM: Consistent application of currency conversion.The use of
dash_to_duffs!(1)..=dash_to_duffs!(1)is consistent with the previous instances.As mentioned in the previous comments, consider simplifying this range if a single value is intended.
packages/rs-drive-abci/tests/strategy_tests/main.rs (6)
88-88: New import added for dash conversion functions.The addition of
use dpp::{dash_to_credits, dash_to_duffs};imports new utility functions for converting Dash to credits and duffs. This is a good practice as it centralizes the conversion logic and makes the code more maintainable.
2329-2330: Updated start balance range for identity inserts.The
start_balance_rangehas been modified to use thedash_to_duffs!macro:start_balance_range: dash_to_duffs!(1)..=dash_to_duffs!(1),This change improves readability and consistency in handling Dash amounts. It ensures that the balance is always set to exactly 1 Dash (in duffs).
2463-2464: Consistent use of dash_to_duffs! macro for start balance range.Similar to the previous change, this modification uses the
dash_to_duffs!macro to set the start balance range:start_balance_range: dash_to_duffs!(1)..=dash_to_duffs!(1),This change maintains consistency across the codebase in handling Dash amounts.
2587-2588: Consistent application of dash_to_duffs! macro in create_state_transitions_for_identities function.The
dash_to_duffs!macro is used here as well:&(dash_to_duffs!(1)..=dash_to_duffs!(1)),This change ensures that the balance range passed to the
create_state_transitions_for_identitiesfunction is consistent with other parts of the code.
3242-3243: Consistent use of dash_to_duffs! macro across multiple test functions.The
dash_to_duffs!macro is consistently applied in multiple test functions to set thestart_balance_range:start_balance_range: dash_to_duffs!(1)..=dash_to_duffs!(1),This change improves consistency and readability throughout the test suite. It also ensures that all tests are using the same balance conversion method, which is crucial for maintaining consistent test behavior.
Also applies to: 3429-3430, 3592-3593, 3918-3919, 3988-3989
Line range hint
1-4092: Summary of changes: Consistent use of dash_to_duffs! macro for balance conversions.The changes in this file primarily focus on using the
dash_to_duffs!macro consistently for setting start balance ranges across multiple test functions. This modification:
- Improves code readability and maintainability.
- Ensures consistency in Dash to duffs conversions throughout the test suite.
- Centralizes the conversion logic, making it easier to update if needed in the future.
These changes are positive improvements to the codebase, enhancing its overall quality and consistency.
packages/rs-drive-abci/src/execution/platform_events/withdrawals/cleanup_expired_locks_of_withdrawal_amounts/v0/mod.rs (2)
36-38: Verify the key format used inPathQueryEnsure that
block_info.time_ms.to_be_bytes().to_vec()correctly matches the key format expected by the database at thesum_path. If there's a mismatch in the key encoding (e.g., endianness or byte representation), the query may not retrieve the intended records for cleanup.
52-52: Confirmis_known_to_be_subtree_with_sumflags are accurateIn
BatchDeleteApplyType::StatefulBatchDelete, the flags are set asSome((false, false)), indicating that the items are not subtrees and do not have sums. Please verify that this accurately reflects the data structure being operated on to prevent unexpected behavior during deletion.packages/rs-drive-abci/src/execution/platform_events/withdrawals/cleanup_expired_locks_of_withdrawal_amounts/mod.rs (1)
1-39: Code implementation and documentation look goodThe imports, module declaration, and the function
clean_up_expired_locks_of_withdrawal_amountsare correctly implemented with comprehensive documentation. This ensures clarity and maintainability of the code.packages/rs-platform-version/src/version/drive_abci_versions.rs (2)
8-8: Looks good: Addition ofwithdrawal_constantsfieldThe addition of
withdrawal_constantstoDriveAbciVersionis appropriate and follows the existing structure.
349-349: Looks good: Addition ofperform_events_on_first_block_of_protocol_changefieldThe addition of
perform_events_on_first_block_of_protocol_changeof typeOptionalFeatureVersiontoDriveAbciProtocolUpgradeMethodVersionsis appropriate and consistent with existing patterns.packages/rs-drive-abci/tests/strategy_tests/strategy.rs (11)
17-18: Imports updated appropriatelyThe addition of
AmountRangeandIdentityUpdateOpto the import list is appropriate and necessary for handling dynamic amount ranges and identity update operations.
44-44: Importing necessary functions for dynamic amount rangesIncluding
instant_asset_lock_proof_fixture_with_dynamic_rangeis essential for creating asset lock proofs with dynamic ranges.
47-47: AddedRangeInclusivefor range handlingThe import of
RangeInclusiveis required for managing ranges of amounts and balances.
1498-1498: Correctly assigningbalance_rangeThe
balance_rangeis appropriately assigned fromself.strategy.identity_inserts.start_balance_range.
1532-1532: Passingbalance_rangeto identity transitionsIncluding
balance_rangein the function call ensures that identities are created with balances within the specified range.
1545-1551: Usingbalance_rangein state transitionsThe
create_state_transitions_for_identitiesfunction now acceptsbalance_range, which allows for dynamic assignment of identity balances.
1560-1560: Acceptingamount_rangeincreate_identity_top_up_transitionThe function signature correctly includes
amount_rangeto handle dynamic top-up amounts.
1570-1574: Creating asset lock proof with dynamic amount rangeUsing
instant_asset_lock_proof_fixture_with_dynamic_rangeallows the asset lock proof to handle a range of amounts, enhancing flexibility.
1655-1655: AddingsignertoChainExecutionOutcomeIncluding
signer: SimpleSignerin the struct ensures that signing capabilities are available where needed, facilitating operations that require signatures.
1687-1687: Function acceptsbalance_rangeparameterThe addition of
balance_rangeto the parameters ofcreate_signed_instant_asset_lock_proofs_for_identitiesenables the creation of asset lock proofs with specified balance ranges.
1712-1716: Generating asset lock proofs with dynamic rangesPassing
balance_rangetoinstant_asset_lock_proof_fixture_with_dynamic_rangeallows for the generation of asset lock proofs that accommodate various balance amounts.
| @@ -1,5 +1,6 @@ | |||
| pub(in crate::execution) mod append_signatures_and_broadcast_withdrawal_transactions; | |||
| pub(in crate::execution) mod build_untied_withdrawal_transactions_from_documents; | |||
| pub(in crate::execution) mod cleanup_expired_locks_of_withdrawal_amounts; | |||
There was a problem hiding this comment.
💡 Codebase verification
Issue Detected: Missing Submodule File for cleanup_expired_locks_of_withdrawal_amounts.
The cleanup_expired_locks_of_withdrawal_amounts submodule is declared in mod.rs, but the corresponding file cleanup_expired_locks_of_withdrawal_amounts.rs does not exist in the repository. This will cause compilation errors and disrupt the module structure.
- Action Required: Please add the
cleanup_expired_locks_of_withdrawal_amounts.rsfile in the appropriate directory to ensure the submodule is properly implemented and the codebase remains consistent.
🔗 Analysis chain
LGTM! New submodule added for cleaning up expired withdrawal locks.
The addition of the cleanup_expired_locks_of_withdrawal_amounts submodule is consistent with the existing code structure and naming conventions. It appears to enhance the withdrawal process by managing expired locks on withdrawal amounts.
To ensure the proper implementation of this new feature, please run the following verification script:
This script will help verify the existence of the new submodule file, check for public functions within it, and ensure that there is test coverage for the new functionality.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the implementation of the new cleanup_expired_locks_of_withdrawal_amounts submodule
# Test 1: Check if the new submodule file exists
echo "Checking if the new submodule file exists:"
fd --type f "cleanup_expired_locks_of_withdrawal_amounts.rs" packages/rs-drive-abci/src/execution/platform_events/withdrawals
# Test 2: Search for public functions in the new submodule
echo "Searching for public functions in the new submodule:"
rg --type rust "pub fn" packages/rs-drive-abci/src/execution/platform_events/withdrawals/cleanup_expired_locks_of_withdrawal_amounts.rs
# Test 3: Check for test coverage
echo "Checking for test coverage:"
rg --type rust "#\[cfg\(test\)\]" packages/rs-drive-abci/src/execution/platform_events/withdrawals/cleanup_expired_locks_of_withdrawal_amounts.rs
Length of output: 990
Script:
#!/bin/bash
# Description: Comprehensive search for the cleanup_expired_locks_of_withdrawal_amounts submodule file
echo "Searching for any files related to cleanup_expired_locks_of_withdrawal_amounts in the entire repository:"
fd "cleanup_expired_locks_of_withdrawal_amounts" --type f
Length of output: 276
| #[macro_export] | ||
| macro_rules! dash_to_duffs { | ||
| // The macro takes a string literal representing the Dash amount. | ||
| ($dash:expr) => {{ | ||
| let dash_str = stringify!($dash); | ||
|
|
||
| // Parsing the input string to separate the whole and fractional parts. | ||
| let parts: Vec<&str> = dash_str.split('.').collect(); | ||
| let mut credits: u128 = 0; | ||
|
|
||
| // Process the whole number part if it exists. | ||
| if let Some(whole) = parts.get(0) { | ||
| if let Ok(whole_number) = whole.parse::<u128>() { | ||
| credits += whole_number * 100_000_000; // Whole Dash amount to credits | ||
| } | ||
| } | ||
|
|
||
| // Process the fractional part if it exists. | ||
| if let Some(fraction) = parts.get(1) { | ||
| let fraction_length = fraction.len(); | ||
| let fraction_number = fraction.parse::<u128>().unwrap_or(0); | ||
| // Calculate the multiplier based on the number of digits in the fraction. | ||
| let multiplier = 10u128.pow(8 - fraction_length as u32); | ||
| credits += fraction_number * multiplier; // Fractional Dash to credits | ||
| } | ||
|
|
||
| credits as u64 | ||
| }}; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider refactoring to reduce code duplication
The dash_to_duffs macro is very similar to the existing dash_to_credits macro. Consider refactoring these macros to share common logic, which would improve maintainability and reduce the risk of inconsistencies.
You could create a helper macro that takes the multiplication factors as parameters:
macro_rules! dash_to_units {
($dash:expr, $whole_multiplier:expr, $fraction_digits:expr) => {{
// Common logic here
}};
}
macro_rules! dash_to_credits {
($dash:expr) => {
dash_to_units!($dash, 100_000_000_000, 11)
};
}
macro_rules! dash_to_duffs {
($dash:expr) => {
dash_to_units!($dash, 100_000_000, 8)
};
}This approach would centralize the conversion logic and make it easier to maintain and extend in the future.
Add input validation for negative numbers and invalid characters
The current implementation doesn't validate the input for negative numbers or invalid characters. This could lead to unexpected behavior or panics.
Consider adding input validation at the beginning of the macro:
let dash_str = stringify!($dash);
if dash_str.starts_with('-') {
panic!("Negative Dash amounts are not supported");
}
if !dash_str.chars().all(|c| c.is_digit(10) || c == '.') {
panic!("Invalid characters in Dash amount");
}This will ensure that only valid, positive Dash amounts are processed.
| credits += fraction_number * multiplier; // Fractional Dash to credits | ||
| } | ||
|
|
||
| credits as u64 |
There was a problem hiding this comment.
Potential overflow when casting to u64
The final conversion credits as u64 may cause an overflow for large Dash amounts. Consider adding a check to ensure the value fits within u64 before casting.
You could add a runtime check:
if credits > u64::MAX as u128 {
panic!("Dash amount too large to convert to Duffs");
}
credits as u64Or, for a compile-time check (which would be preferable), you could use the TryInto trait:
use std::convert::TryInto;
credits.try_into().expect("Dash amount too large to convert to Duffs")There was a problem hiding this comment.
No need as this is not used in production code.
There was a problem hiding this comment.
@QuantumExplorer, understood. Thanks for the clarification.
(•.•)?
✏️ Learnings added
Learnt from: QuantumExplorer
PR: dashpay/platform#2182
File: packages/rs-dpp/src/util/units.rs:57-57
Timestamp: 2024-09-29T09:35:10.905Z
Learning: The macro `dash_to_duffs` in `units.rs` is not used in production code.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
There was a problem hiding this comment.
@QuantumExplorer Please add test flag then if it's not using in production code
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
| pub fn instant_asset_lock_proof_transaction_fixture_with_dynamic_amount( | ||
| one_time_private_key: PrivateKey, | ||
| amount_range: &AmountRange, | ||
| rng: &mut StdRng, | ||
| ) -> Transaction { | ||
| let secp = Secp256k1::new(); | ||
|
|
||
| let private_key_hex = "cSBnVM4xvxarwGQuAfQFwqDg9k5tErHUHzgWsEfD4zdwUasvqRVY"; | ||
| let private_key = PrivateKey::from_str(private_key_hex).unwrap(); | ||
| let public_key = private_key.public_key(&secp); | ||
| let public_key_hash = public_key.pubkey_hash(); | ||
| //let from_address = Address::p2pkh(&public_key, Network::Testnet); | ||
| let one_time_public_key = one_time_private_key.public_key(&secp); | ||
|
|
||
| // We are going to fund 1 Dash and | ||
| // assume that input has 100005000 | ||
| // 5000 will be returned back | ||
|
|
||
| let input_txid = | ||
| Txid::from_str("a477af6b2667c29670467e4e0728b685ee07b240235771862318e29ddbe58458").unwrap(); | ||
|
|
||
| let input_outpoint = OutPoint::new(input_txid, 0); | ||
|
|
||
| let input = TxIn { | ||
| previous_output: input_outpoint, | ||
| script_sig: ScriptBuf::new_p2pkh(&public_key_hash), | ||
| sequence: 0, | ||
| witness: Default::default(), | ||
| }; | ||
|
|
||
| let one_time_key_hash = one_time_public_key.pubkey_hash(); | ||
|
|
||
| let value_amount = if amount_range.start() == amount_range.end() { | ||
| *amount_range.start() //avoid using rng if possible | ||
| } else { | ||
| rng.gen_range(amount_range.clone()) | ||
| }; | ||
|
|
||
| let funding_output = TxOut { | ||
| value: value_amount, | ||
| script_pubkey: ScriptBuf::new_p2pkh(&one_time_key_hash), | ||
| }; | ||
|
|
||
| let burn_output = TxOut { | ||
| value: value_amount, | ||
| script_pubkey: ScriptBuf::new_op_return(&[]), | ||
| }; | ||
|
|
||
| let change_output = TxOut { | ||
| value: 5000, | ||
| script_pubkey: ScriptBuf::new_p2pkh(&public_key_hash), | ||
| }; | ||
|
|
||
| let payload = TransactionPayload::AssetLockPayloadType(AssetLockPayload { | ||
| version: 0, | ||
| credit_outputs: vec![funding_output], | ||
| }); | ||
|
|
||
| Transaction { | ||
| version: 0, | ||
| lock_time: 0, | ||
| input: vec![input], | ||
| output: vec![burn_output, change_output], | ||
| special_transaction_payload: Some(payload), | ||
| } | ||
| } |
There was a problem hiding this comment.
LGTM with a suggestion: New function for dynamic amount transaction fixture
The new function instant_asset_lock_proof_transaction_fixture_with_dynamic_amount is well-implemented and consistent with the goal of adding dynamic amounts for asset lock proofs. It effectively generates a random amount within the given range.
There's a potential issue with input validation. The function assumes that the amount_range is valid and non-empty. Consider adding a check to ensure the range is valid before using it. Here's a suggested improvement:
let value_amount = if amount_range.is_empty() {
return Err(ProtocolError::Generic("Invalid empty amount range".to_string()));
} else if amount_range.start() == amount_range.end() {
*amount_range.start() // avoid using rng if possible
} else {
rng.gen_range(amount_range.clone())
};This change will prevent potential panics or unexpected behavior if an invalid range is provided.
🧰 Tools
🪛 Gitleaks
189-189: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
There was a problem hiding this comment.
No need, as this is not production code.
There was a problem hiding this comment.
@QuantumExplorer, understood. Thanks for the clarification.
(_/)
(._.)
('')('')
✏️ Learnings added
Learnt from: QuantumExplorer
PR: dashpay/platform#2182
File: packages/strategy-tests/src/transitions.rs:182-247
Timestamp: 2024-09-29T09:34:16.767Z
Learning: In test code, input validation in functions like `instant_asset_lock_proof_transaction_fixture_with_dynamic_amount` is not required.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| pub(super) fn cleanup_expired_locks_of_withdrawal_amounts_v0( | ||
| &self, | ||
| block_info: &BlockInfo, | ||
| transaction: &Transaction, | ||
| platform_version: &PlatformVersion, | ||
| ) -> Result<(), Error> { | ||
| if platform_version | ||
| .drive_abci | ||
| .withdrawal_constants | ||
| .cleanup_expired_locks_of_withdrawal_amounts_limit | ||
| == 0 | ||
| { | ||
| // No clean up | ||
| return Ok(()); | ||
| } | ||
| let mut batch_operations = vec![]; | ||
|
|
||
| let sum_path = get_withdrawal_transactions_sum_tree_path_vec(); | ||
|
|
||
| let mut path_query = PathQuery::new_single_query_item( | ||
| sum_path, | ||
| QueryItem::RangeTo(..block_info.time_ms.to_be_bytes().to_vec()), | ||
| ); | ||
|
|
||
| path_query.query.limit = Some( | ||
| platform_version | ||
| .drive_abci | ||
| .withdrawal_constants | ||
| .cleanup_expired_locks_of_withdrawal_amounts_limit, | ||
| ); | ||
|
|
||
| self.drive.batch_delete_items_in_path_query( | ||
| &path_query, | ||
| true, | ||
| // we know that we are not deleting a subtree | ||
| BatchDeleteApplyType::StatefulBatchDelete { | ||
| is_known_to_be_subtree_with_sum: Some((false, false)), | ||
| }, | ||
| Some(transaction), | ||
| &mut batch_operations, | ||
| &platform_version.drive, | ||
| )?; | ||
|
|
||
| self.drive.apply_batch_low_level_drive_operations( | ||
| None, | ||
| Some(transaction), | ||
| batch_operations, | ||
| &mut vec![], | ||
| &platform_version.drive, | ||
| )?; | ||
|
|
||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
Add unit tests for cleanup_expired_locks_of_withdrawal_amounts_v0
The new method introduces critical functionality for cleaning up expired withdrawal locks. To ensure its reliability and prevent regressions, please add unit tests covering scenarios such as:
- Cleanup limit is zero (no cleanup happens).
- Cleanup limit is set, and there are expired locks to delete.
- Cleanup limit is set, but there are no expired locks to delete.
- Validation of correct items being deleted.
| pub pool_withdrawals_into_transactions_queue: FeatureVersion, | ||
| pub update_broadcasted_withdrawal_statuses: FeatureVersion, | ||
| pub append_signatures_and_broadcast_withdrawal_transactions: FeatureVersion, | ||
| pub cleanup_expired_locks_of_withdrawal_amounts: FeatureVersion, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider simplifying the field name cleanup_expired_locks_of_withdrawal_amounts
Similar to the previous suggestion, the field name cleanup_expired_locks_of_withdrawal_amounts is quite long. For improved readability, consider renaming it to cleanup_expired_withdrawal_locks.
Apply this diff to rename the field:
#[derive(Clone, Debug, Default)]
pub struct DriveAbciIdentityCreditWithdrawalMethodVersions {
pub build_untied_withdrawal_transactions_from_documents: FeatureVersion,
pub dequeue_and_build_unsigned_withdrawal_transactions: FeatureVersion,
pub fetch_transactions_block_inclusion_status: FeatureVersion,
pub pool_withdrawals_into_transactions_queue: FeatureVersion,
pub update_broadcasted_withdrawal_statuses: FeatureVersion,
pub append_signatures_and_broadcast_withdrawal_transactions: FeatureVersion,
- pub cleanup_expired_locks_of_withdrawal_amounts: FeatureVersion,
+ pub cleanup_expired_withdrawal_locks: FeatureVersion,
}Updating the field name will enhance code clarity and align with naming conventions.
📝 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.
| pub cleanup_expired_locks_of_withdrawal_amounts: FeatureVersion, | |
| pub cleanup_expired_withdrawal_locks: FeatureVersion, |
| #[derive(Clone, Debug, Default)] | ||
| pub struct DriveAbciWithdrawalConstants { | ||
| pub core_expiration_blocks: u32, | ||
| pub cleanup_expired_locks_of_withdrawal_amounts_limit: u16, | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider simplifying the field name cleanup_expired_locks_of_withdrawal_amounts_limit
The field name cleanup_expired_locks_of_withdrawal_amounts_limit is quite long, which might affect readability. Consider shortening it for clarity, such as expired_locks_cleanup_limit.
Apply this diff to rename the field:
#[derive(Clone, Debug, Default)]
pub struct DriveAbciWithdrawalConstants {
pub core_expiration_blocks: u32,
- pub cleanup_expired_locks_of_withdrawal_amounts_limit: u16,
+ pub expired_locks_cleanup_limit: u16,
}This change will enhance readability and maintain consistency.
📝 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.
| #[derive(Clone, Debug, Default)] | |
| pub struct DriveAbciWithdrawalConstants { | |
| pub core_expiration_blocks: u32, | |
| pub cleanup_expired_locks_of_withdrawal_amounts_limit: u16, | |
| } | |
| #[derive(Clone, Debug, Default)] | |
| pub struct DriveAbciWithdrawalConstants { | |
| pub core_expiration_blocks: u32, | |
| pub expired_locks_cleanup_limit: u16, | |
| } |
There was a problem hiding this comment.
Verbosity here is better. Rust doc would be even better.
There was a problem hiding this comment.
@shumkov, understood. Keeping the verbose field name makes sense here. Adding Rust doc comments would indeed improve clarity.
(\•ᴥ•)/?
✏️ Learnings added
Learnt from: shumkov
PR: dashpay/platform#2182
File: packages/rs-platform-version/src/version/drive_abci_versions.rs:116-121
Timestamp: 2024-09-29T13:13:54.054Z
Learning: In the `DriveAbciWithdrawalConstants` struct, prefer verbose field names even if they are lengthy.
Learnt from: shumkov
PR: dashpay/platform#2182
File: packages/rs-platform-version/src/version/drive_abci_versions.rs:116-121
Timestamp: 2024-09-29T13:13:54.054Z
Learning: Adding Rust doc comments to structs and fields is recommended to enhance clarity and documentation.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| OperationType::IdentityWithdrawal(amount) if !current_identities.is_empty() => { | ||
| let indices: Vec<usize> = | ||
| (0..current_identities.len()).choose_multiple(rng, count as usize); | ||
| for index in indices { | ||
| let random_identity = current_identities.get_mut(index).unwrap(); | ||
| let state_transition = | ||
| strategy_tests::transitions::create_identity_withdrawal_transition( | ||
| random_identity, | ||
| amount.clone(), |
There was a problem hiding this comment.
Ensure count does not exceed available identities
Similar to the previous comment, in the IdentityWithdrawal operation, prevent potential panics by ensuring count as usize is not greater than current_identities.len() when calling choose_multiple.
Apply this diff to fix the issue:
+ let count = std::cmp::min(count as u16, current_identities.len() as u16);
let indices: Vec<usize> =
- (0..current_identities.len()).choose_multiple(rng, count as usize);
+ (0..current_identities.len()).choose_multiple(rng, count as usize);Or adjust within choose_multiple:
let indices: Vec<usize> =
- (0..current_identities.len()).choose_multiple(rng, count as usize);
+ (0..current_identities.len()).choose_multiple(rng, std::cmp::min(count as usize, current_identities.len()));📝 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.
| OperationType::IdentityWithdrawal(amount) if !current_identities.is_empty() => { | |
| let indices: Vec<usize> = | |
| (0..current_identities.len()).choose_multiple(rng, count as usize); | |
| for index in indices { | |
| let random_identity = current_identities.get_mut(index).unwrap(); | |
| let state_transition = | |
| strategy_tests::transitions::create_identity_withdrawal_transition( | |
| random_identity, | |
| amount.clone(), | |
| OperationType::IdentityWithdrawal(amount) if !current_identities.is_empty() => { | |
| let count = std::cmp::min(count as u16, current_identities.len() as u16); | |
| let indices: Vec<usize> = | |
| (0..current_identities.len()).choose_multiple(rng, count as usize); | |
| for index in indices { | |
| let random_identity = current_identities.get_mut(index).unwrap(); | |
| let state_transition = | |
| strategy_tests::transitions::create_identity_withdrawal_transition( | |
| random_identity, | |
| amount.clone(), |
| // Withdrawal documents with pooled status should exist. | ||
| let withdrawal_documents_pooled = outcome | ||
| .abci_app | ||
| .platform | ||
| .drive | ||
| .fetch_oldest_withdrawal_documents_by_status( | ||
| withdrawals_contract::WithdrawalStatus::POOLED.into(), | ||
| DEFAULT_QUERY_LIMIT, | ||
| None, | ||
| platform_version, | ||
| ) | ||
| .unwrap(); | ||
| assert!(withdrawal_documents_pooled.is_empty()); | ||
|
|
There was a problem hiding this comment.
Inconsistency between comment and assertion regarding pooled withdrawal documents
The comment states that "Withdrawal documents with pooled status should exist," but the subsequent assertion checks that withdrawal_documents_pooled is empty. This inconsistency may indicate an error in the test logic or that the comment needs to be updated. Please verify whether pooled withdrawal documents are expected at this point and adjust the code or comment accordingly.
There was a problem hiding this comment.
@QuantumExplorer good point!
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
| let withdrawal_documents_pooled = outcome | ||
| .abci_app | ||
| .platform | ||
| .drive | ||
| .fetch_oldest_withdrawal_documents_by_status( | ||
| withdrawals_contract::WithdrawalStatus::POOLED.into(), | ||
| DEFAULT_QUERY_LIMIT, | ||
| None, | ||
| platform_version, | ||
| ) | ||
| .unwrap(); | ||
| assert!(withdrawal_documents_pooled.is_empty()); |
There was a problem hiding this comment.
Inconsistency between comment and assertion regarding pooled withdrawal documents
The comment states that "Withdrawal documents with pooled status should exist," but the assertion checks that withdrawal_documents_pooled is empty. This suggests a possible mismatch between the expected and actual state. Please verify whether pooled withdrawal documents should exist at this stage and update the comment or assertion as necessary.
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Outside diff range and nitpick comments (14)
packages/rs-drive/src/drive/identity/withdrawals/document/find_withdrawal_documents_by_status_and_transaction_indices/v0/mod.rs (1)
Potential Issue Found in Withdrawal Document Querying
- The function
find_withdrawal_documents_by_status_and_transaction_indices_v0is correctly utilized within the withdrawal limits feature and appropriately filters withdrawals based on their status.- However, there is a TODO comment indicating that the function currently queries only up to 100 documents. This limitation may lead to missing withdrawals if the number exceeds this threshold, potentially violating withdrawal limits.
🔗 Analysis chain
Line range hint
21-95: Verify alignment with PR objectivesThe function
find_withdrawal_documents_by_status_and_transaction_indices_v0appears to be implementing part of the withdrawal limits feature mentioned in the PR objectives. It queries withdrawal documents based on their status and transaction indices.To ensure this implementation aligns with the PR objectives:
- Confirm that this function is being used as part of the withdrawal limits feature.
- Verify that the
statusparameter is being used correctly to filter withdrawals based on their current state in the withdrawal process.- Check if there are any missing edge cases or additional requirements for withdrawal limits that are not covered by this function.
Run the following script to gather more context about the usage of this function and related withdrawal limit implementations:
This will help us understand how this function fits into the broader implementation of withdrawal limits and identify any areas that may need further attention.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for usages of the find_withdrawal_documents_by_status_and_transaction_indices_v0 function and other withdrawal limit related code. echo "Searching for usages of find_withdrawal_documents_by_status_and_transaction_indices_v0:" rg "find_withdrawal_documents_by_status_and_transaction_indices_v0" --type rust echo "\nSearching for other withdrawal limit related functions:" rg "withdrawal.*limit" --type rust echo "\nSearching for TODO comments related to withdrawals:" rg "TODO.*withdrawal" --type rustLength of output: 13812
packages/strategy-tests/src/lib.rs (3)
Line range hint
158-167: LGTM! Consider expanding the default range for more diversity.The addition of
start_balance_rangetoIdentityInsertInfois a good improvement, allowing for more flexible control over the starting balance of newly inserted identities.Consider expanding the default range to provide more diversity in starting balances. For example:
- start_balance_range: dash_to_duffs!(1)..=dash_to_duffs!(1), + start_balance_range: dash_to_duffs!(1)..=dash_to_duffs!(10),This change would allow for a wider range of starting balances in the default case, potentially creating more diverse testing scenarios.
Line range hint
1174-1194: Good start on implementing variable top-up amounts. Consider next steps.The addition of
_amount_rangeand the todo comment show progress towards implementing variable top-up amounts. The use of a cyclic iterator is a good approach to ensure the required number of transitions can be created.To fully implement the variable top-up amounts feature:
- Remove the underscore from
_amount_rangeto use it in the function.- Modify the
create_identity_top_up_transitionfunction to accept and use theamount_range.- Generate a random amount within the range for each transition.
Here's a sketch of how you might implement this:
let amount = rng.gen_range(amount_range.clone()); match crate::transitions::create_identity_top_up_transition( random_identity, amount, asset_lock_proofs, platform_version, ) { // ... rest of the code }Don't forget to update the
create_identity_top_up_transitionfunction signature and implementation to accept the newamountparameter.
Line range hint
1271-1286: LGTM! Consider applying similar pattern to IdentityTopUp for consistency.The implementation of variable withdrawal amounts using
amount_rangeis well done. Theamount_rangeis correctly passed to thecreate_identity_withdrawal_transitionfunction, which suggests it's being properly utilized.For consistency across the codebase, consider applying a similar pattern to the
IdentityTopUpoperation. This would involve:
- Renaming
_amount_rangetoamount_rangein theIdentityTopUpmatch arm.- Passing
amount_range.clone()to thecreate_identity_top_up_transitionfunction, similar to how it's done in theIdentityWithdrawaloperation.This change would make the implementation of both operations more uniform and easier to maintain.
packages/rs-drive-abci/tests/strategy_tests/withdrawal_tests.rs (10)
42-46: Consider adding a comment explaining the use of TEST_PLATFORM_V3The use of
TEST_PLATFORM_V3is not immediately clear. It would be helpful to add a comment explaining why this specific version is used and how it differs from other versions, especially in relation to withdrawal functionality.
Line range hint
87-97: Consider extracting configuration setup into a separate functionThe configuration setup for
PlatformConfigis quite lengthy and could be made more readable by extracting it into a separate function. This would also make it easier to reuse in other tests if needed.
Line range hint
285-297: Consider using destructuring for better readabilityThe
ChainExecutionOutcomestruct is destructured with many fields. Consider using the..syntax to ignore unused fields, making the code more concise and easier to read.Apply this change:
- let ( - ChainExecutionOutcome { - abci_app, - proposers, - validator_quorums: quorums, - current_validator_quorum_hash: current_quorum_hash, - current_proposer_versions, - end_time_ms, - identity_nonce_counter, - identity_contract_nonce_counter, - instant_lock_quorums, - identities, - .. - }, - last_block_pooled_withdrawals_amount, - ) = { + let ( + ChainExecutionOutcome { + abci_app, + proposers, + validator_quorums: quorums, + current_validator_quorum_hash: current_quorum_hash, + current_proposer_versions, + end_time_ms, + identity_nonce_counter, + identity_contract_nonce_counter, + instant_lock_quorums, + identities, + .. + }, + last_block_pooled_withdrawals_amount, + ) = {
Line range hint
374-387: Consider using destructuring for better readabilitySimilar to the previous comment, consider using the
..syntax to ignore unused fields when destructuringChainExecutionOutcome.Apply this change:
- let ChainExecutionOutcome { - abci_app, - proposers, - validator_quorums: quorums, - current_validator_quorum_hash: current_quorum_hash, - current_proposer_versions, - end_time_ms, - withdrawals: last_block_withdrawals, - identity_nonce_counter, - identity_contract_nonce_counter, - instant_lock_quorums, - identities, - .. - } = { + let ChainExecutionOutcome { + abci_app, + proposers, + validator_quorums: quorums, + current_validator_quorum_hash: current_quorum_hash, + current_proposer_versions, + end_time_ms, + withdrawals: last_block_withdrawals, + identity_nonce_counter, + identity_contract_nonce_counter, + instant_lock_quorums, + identities, + .. + } = {
724-726: Consider adding a comment explaining the use of TEST_PLATFORM_V3Similar to the previous function, it would be helpful to add a comment explaining why
TEST_PLATFORM_V3is used and how it differs from other versions, especially in relation to withdrawal functionality.
1230-1232: Consider adding a comment explaining the use of TEST_PLATFORM_V3Similar to the previous functions, it would be helpful to add a comment explaining why
TEST_PLATFORM_V3is used and how it differs from other versions, especially in relation to withdrawal functionality.
1633-1641: Consider adding assertions for the withdrawal amountsThe comment explains the expected behavior for each withdrawal block, but there are no assertions to verify these expectations. Consider adding assertions to ensure the behavior matches the comment.
1949-1951: Consider adding a comment explaining the use of TEST_PLATFORM_V3Similar to the previous functions, it would be helpful to add a comment explaining why
TEST_PLATFORM_V3is used and how it differs from other versions, especially in relation to withdrawal functionality.
2365-2366: Add an assertion to verify the locked amountThe comment explains that 0.2 is 0.0025 amount * 80 withdrawals, but there's no assertion to verify this calculation. Consider adding an assertion to ensure the locked amount matches the expected value.
Add this assertion:
assert_eq!(locked_amount, dash_to_credits!(0.0025 * 80.0) as i64);
Line range hint
1-2503: Overall suggestions for improving test readability and maintainability
Consider creating helper functions for common setup tasks, such as creating identities and performing withdrawals. This would reduce code duplication and make the tests more concise.
Use constants for magic numbers and repeated values throughout the tests. This would improve maintainability and make it easier to update values if needed.
Add more descriptive comments explaining the purpose of each test and the expected outcomes. This would make it easier for other developers to understand and maintain the tests.
Consider using a test setup function to initialize common objects and configurations, reducing repetition across test functions.
Implement parameterized tests for scenarios that are similar but with different input values. This could help reduce code duplication and make it easier to add new test cases.
Consider adding error messages to assertions to provide more context when tests fail.
Use more descriptive variable names, especially for intermediate results and temporary variables.
These improvements would enhance the overall quality and maintainability of the test suite.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (19)
- packages/rs-drive-abci/src/execution/platform_events/withdrawals/dequeue_and_build_unsigned_withdrawal_transactions/v0/mod.rs (2 hunks)
- packages/rs-drive-abci/src/execution/platform_events/withdrawals/pool_withdrawals_into_transactions_queue/v0/mod.rs (7 hunks)
- packages/rs-drive-abci/src/execution/platform_events/withdrawals/pool_withdrawals_into_transactions_queue/v1/mod.rs (7 hunks)
- packages/rs-drive-abci/tests/strategy_tests/main.rs (14 hunks)
- packages/rs-drive-abci/tests/strategy_tests/withdrawal_tests.rs (20 hunks)
- packages/rs-drive-proof-verifier/src/types.rs (0 hunks)
- packages/rs-drive/src/drive/identity/withdrawals/document/find_withdrawal_documents_by_status_and_transaction_indices/mod.rs (2 hunks)
- packages/rs-drive/src/drive/identity/withdrawals/document/find_withdrawal_documents_by_status_and_transaction_indices/v0/mod.rs (1 hunks)
- packages/rs-drive/src/util/grove_operations/batch_delete_items_in_path_query/mod.rs (1 hunks)
- packages/rs-drive/src/util/grove_operations/batch_delete_items_in_path_query/v0/mod.rs (1 hunks)
- packages/rs-drive/src/util/grove_operations/batch_insert_sum_item_or_add_to_if_already_exists/mod.rs (1 hunks)
- packages/rs-platform-version/src/version/drive_versions.rs (3 hunks)
- packages/rs-platform-version/src/version/mocks/v2_test.rs (10 hunks)
- packages/rs-platform-version/src/version/mocks/v3_test.rs (10 hunks)
- packages/rs-platform-version/src/version/v1.rs (10 hunks)
- packages/rs-platform-version/src/version/v2.rs (10 hunks)
- packages/rs-platform-version/src/version/v3.rs (10 hunks)
- packages/rs-platform-version/src/version/v4.rs (10 hunks)
- packages/strategy-tests/src/lib.rs (7 hunks)
💤 Files with no reviewable changes (1)
- packages/rs-drive-proof-verifier/src/types.rs
🚧 Files skipped from review as they are similar to previous changes (13)
- packages/rs-drive-abci/src/execution/platform_events/withdrawals/dequeue_and_build_unsigned_withdrawal_transactions/v0/mod.rs
- packages/rs-drive-abci/src/execution/platform_events/withdrawals/pool_withdrawals_into_transactions_queue/v0/mod.rs
- packages/rs-drive-abci/src/execution/platform_events/withdrawals/pool_withdrawals_into_transactions_queue/v1/mod.rs
- packages/rs-drive-abci/tests/strategy_tests/main.rs
- packages/rs-drive/src/util/grove_operations/batch_delete_items_in_path_query/mod.rs
- packages/rs-drive/src/util/grove_operations/batch_delete_items_in_path_query/v0/mod.rs
- packages/rs-drive/src/util/grove_operations/batch_insert_sum_item_or_add_to_if_already_exists/mod.rs
- packages/rs-platform-version/src/version/drive_versions.rs
- packages/rs-platform-version/src/version/mocks/v2_test.rs
- packages/rs-platform-version/src/version/mocks/v3_test.rs
- packages/rs-platform-version/src/version/v1.rs
- packages/rs-platform-version/src/version/v2.rs
- packages/rs-platform-version/src/version/v3.rs
🔇 Additional comments (8)
packages/rs-drive/src/drive/identity/withdrawals/document/find_withdrawal_documents_by_status_and_transaction_indices/mod.rs (2)
Line range hint
1-45: Summary of changes and request for additional informationThe changes in this file primarily involve renaming the method
find_up_to_100_withdrawal_documents_by_status_and_transaction_indicestofind_withdrawal_documents_by_status_and_transaction_indices. This change appears to be well-implemented and consistent throughout the file.However, there are a few points that require attention:
The removal of "up_to_100" from the method name suggests a change in functionality. Please ensure that this change is reflected in the method's implementation and any related documentation.
The PR objectives mention implementing withdrawal limits, but the changes in this file don't directly address that. Could you provide more information about how the withdrawal limits are being implemented? Are there other files involved in this implementation that we should review?
The versioning strategy for this method shows only version 0 as a known version. Please confirm if this is intentional or if additional versions should be considered.
To get a better understanding of the overall changes related to withdrawal limits, please run the following script:
#!/bin/bash # Description: Search for changes related to withdrawal limits # Test: Search for files with changes related to withdrawal limits git diff --name-only HEAD~1 | xargs rg --type rust "withdrawal.*limit" # Test: Search for new methods or functions related to withdrawal limits git diff HEAD~1 | rg "fn.*withdrawal.*limit"This will help identify other relevant changes related to the implementation of withdrawal limits across the codebase.
38-38: Approve error message update and review versioning strategy.The error message has been correctly updated to reflect the new method name. This change maintains consistency with the earlier method name modification.
However, I noticed that the
known_versionsarray still only includes version 0. Please verify if this is intentional or if additional versions should be added. To help with this, you can run the following script:This will help identify if there are any other version implementations or TODO comments related to versioning for this method.
✅ Verification successful
Versioning strategy verified successfully.
No additional versions or TODOs related to
find_withdrawal_documents_by_status_and_transaction_indiceswere found.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for other version implementations of the method # Test: Search for other version implementations. Expect: Only v0 implementation. rg --type rust "fn find_withdrawal_documents_by_status_and_transaction_indices_v\d+" # Test: Check for any TODO comments related to versioning. Expect: No results if versioning is complete. rg --type rust "TODO.*version.*find_withdrawal_documents_by_status_and_transaction_indices"Length of output: 3121
Script:
#!/bin/bash # Description: Check for any TODO comments related to versioning for the specific method. rg --type rust 'TODO.*version.*find_withdrawal_documents_by_status_and_transaction_indices'Length of output: 93
packages/rs-drive-abci/tests/strategy_tests/withdrawal_tests.rs (2)
194-207: Inconsistency between comment and assertion regarding pooled withdrawal documentsThe comment states that "Withdrawal documents with pooled status should exist," but the subsequent assertion checks that
withdrawal_documents_pooledis empty. This inconsistency may indicate an error in the test logic or that the comment needs to be updated.
876-887: Inconsistency between comment and assertion regarding pooled withdrawal documentsThe comment states that "Withdrawal documents with pooled status should exist," but the assertion checks that
withdrawal_documents_pooledis empty. This suggests a possible mismatch between the expected and actual state.packages/rs-platform-version/src/version/v4.rs (4)
644-644: Confirm version setting forperform_events_on_first_block_of_protocol_changeSetting
perform_events_on_first_block_of_protocol_changetoSome(4)indicates that events should be performed starting with protocol version 4. Ensure that this aligns with the intended protocol upgrade logic and that related handling is correctly implemented.
1297-1298: Verify consistency of withdrawal limitsThe
max_withdrawal_amountanddaily_withdrawal_limitare both set to50_000_000_000_000, representing 500 Dash.Please confirm that these values align with business requirements and are consistently enforced across the platform.
Run the following script to check usage of withdrawal limit constants:
#!/bin/bash # Description: Verify usage of `max_withdrawal_amount` and `daily_withdrawal_limit`. # Test: Search for references to these constants to ensure consistent application. rg --type rust 'max_withdrawal_amount|daily_withdrawal_limit' -A 2 -B 2
481-481: Verify consistent use ofcalculate_current_withdrawal_limitacross the codebaseThe addition of
calculate_current_withdrawal_limitseems appropriate. Please ensure that all references to this method are correctly implemented and consistently used throughout the codebase.Run the following script to check for consistent usage of
calculate_current_withdrawal_limit:✅ Verification successful
calculate_current_withdrawal_limitis consistently implemented and referenced throughout the codebase. No inconsistencies found.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all usages of `calculate_current_withdrawal_limit` are consistent. # Test: Search for the method usage. Expect: Consistent references and implementations. rg --type rust 'calculate_current_withdrawal_limit' -A 2 -B 2Length of output: 8396
580-580: Ensure correct implementation ofbatch_delete_items_in_path_queryThe addition of
batch_delete_items_in_path_query: 0appears correct. Please verify that this method is properly implemented and follows existing conventions for batch operations.Run the following script to find implementations of
batch_delete_items_in_path_query:✅ Verification successful
Batch Delete Items in Path Query is Properly Implemented
The
batch_delete_items_in_path_queryfield is consistently defined across platform versions (v1 to v4) and has corresponding method implementations and test cases in the codebase. This ensures adherence to existing conventions for batch operations.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Ensure that `batch_delete_items_in_path_query` is implemented correctly. # Test: Search for the method definition and usage. rg --type rust 'fn batch_delete_items_in_path_query' -A 5 -B 2Length of output: 12930
Script:
#!/bin/bash # Description: Search for the field or variable `batch_delete_items_in_path_query` in Rust files. rg --type rust 'batch_delete_items_in_path_query' --context 2Length of output: 13425
| .withdrawals | ||
| .document | ||
| .find_up_to_100_withdrawal_documents_by_status_and_transaction_indices | ||
| .find_withdrawal_documents_by_status_and_transaction_indices |
There was a problem hiding this comment.
💡 Codebase verification
Update module documentation to remove old method name references.
The old method name find_up_to_100_withdrawal_documents_by_status_and_transaction_indices is still referenced in packages/rs-drive/src/drive/identity/withdrawals/document/mod.rs. Please update this reference to ensure consistency and prevent potential confusion or bugs.
packages/rs-drive/src/drive/identity/withdrawals/document/mod.rs
🔗 Analysis chain
Approve method name change and verify its impact.
The method name change from find_up_to_100_withdrawal_documents_by_status_and_transaction_indices to find_withdrawal_documents_by_status_and_transaction_indices is appropriate. It reflects the removal of the hardcoded limit of 100 documents, allowing for more flexibility with the limit parameter.
To ensure consistency across the codebase, please run the following script to check for any remaining occurrences of the old method name:
This will help identify any places where the method name might need to be updated.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for any remaining occurrences of the old method name
# Test: Search for the old method name. Expect: No results.
rg --type rust "find_up_to_100_withdrawal_documents_by_status_and_transaction_indices"
# Test: Search for the new method name. Expect: Results showing the updated usage.
rg --type rust "find_withdrawal_documents_by_status_and_transaction_indices"
Length of output: 2892
There was a problem hiding this comment.
@QuantumExplorer, thank you for addressing the issue. The codebase is now consistent with the updated method names.
(\ ^_^ /)
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
| impl Drive { | ||
| // TODO(withdrawals): Currently it queries only up to 100 documents. | ||
| // It works while we don't have pooling | ||
| // This should be a pathquery directly instead of a drive query for efficiency |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider implementing the suggested optimization
The added comment suggests using a path query instead of a drive query for improved efficiency. This is a valid optimization suggestion, especially considering the existing TODO comment about the current implementation querying only up to 100 documents.
To address both comments:
- Implement a path query to replace the current drive query.
- Remove the limit of 100 documents or implement a pagination mechanism to handle larger result sets.
Here's a proposed plan of action:
- Research the implementation of path queries in this context.
- Create a new function that uses a path query to fetch the documents.
- Benchmark the new implementation against the current one to verify the performance improvement.
- If the new implementation proves more efficient, replace the current function with the optimized version.
- Update or remove the TODO comment based on the new implementation.
Would you like me to assist in creating a GitHub issue to track this optimization task?
There was a problem hiding this comment.
@QuantumExplorer, understood.
(=ＴェＴ=)
| let number_of_blocks_before_expiration: u32 = 48; | ||
| chain_locked_height += number_of_blocks_before_expiration; | ||
|
|
||
| core_state.chain_lock.block_height = chain_locked_height; | ||
|
|
||
| last_block_withdrawals.iter().for_each(|tx| { | ||
| let index = asset_unlock_index(tx); | ||
|
|
||
| core_state.asset_unlock_statuses.insert( | ||
| index, | ||
| AssetUnlockStatusResult { | ||
| index, | ||
| status: AssetUnlockStatus::Unknown, | ||
| }, | ||
| ); | ||
| }); | ||
| core_state.asset_unlock_statuses.insert( | ||
| index, | ||
| AssetUnlockStatusResult { | ||
| index, | ||
| status: AssetUnlockStatus::Unknown, | ||
| }, | ||
| ); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider extracting the core state update logic into a separate function
The logic for updating the core state before proceeding to the next block is quite complex. Consider extracting this into a separate function for better readability and potential reuse in other tests.
| assert_eq!( | ||
| total_credits_balance.total_identity_balances, | ||
| 409997575280380 | ||
| ); // Around 4100 Dash. | ||
|
|
||
| assert_eq!( | ||
| total_credits_balance.total_in_trees().unwrap(), | ||
| 410000000000000 | ||
| ); // Around 4100 Dash. |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using a constant for the expected balance values
The assertion for total credits balance uses magic numbers. Consider defining these as constants at the top of the test file for better maintainability and readability.
Apply this change:
const EXPECTED_TOTAL_IDENTITY_BALANCES: u64 = 409997575280380;
const EXPECTED_TOTAL_IN_TREES: u64 = 410000000000000;
// Then in the test:
assert_eq!(
total_credits_balance.total_identity_balances,
EXPECTED_TOTAL_IDENTITY_BALANCES
);
assert_eq!(
total_credits_balance.total_in_trees().unwrap(),
EXPECTED_TOTAL_IN_TREES
);There was a problem hiding this comment.
@QuantumExplorer, understood.
| assert_eq!( | ||
| total_credits_balance.total_identity_balances, | ||
| 409997575280380 | ||
| ); // Around 4100 Dash. | ||
|
|
||
| assert_eq!( | ||
| total_credits_balance.total_in_trees().unwrap(), | ||
| 410000000000000 | ||
| ); // Around 4100 Dash. |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using constants for expected balance values
Similar to a previous comment, the assertion for total credits balance uses magic numbers. Consider defining these as constants at the top of the test file for better maintainability and readability.
There was a problem hiding this comment.
@QuantumExplorer, understood.
(_/)
( ;_;)
| }, | ||
| methods: DPPMethodVersions { | ||
| epoch_core_reward_credits_for_distribution: 0, | ||
| daily_withdrawal_limit: 0, |
There was a problem hiding this comment.
Potential inconsistency in daily_withdrawal_limit initialization
In DPPMethodVersions, daily_withdrawal_limit is set to 0, while elsewhere it's set to Some(50_000_000_000_000).
Ensure that the default value of 0 here reflects the intended behavior and does not conflict with the value specified in system_limits.
There was a problem hiding this comment.
@QuantumExplorer, glad to hear that the issue has been fixed!
(◕‿◕)♪
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
| document: DriveIdentityWithdrawalDocumentMethodVersions { | ||
| fetch_oldest_withdrawal_documents_by_status: 0, | ||
| find_up_to_100_withdrawal_documents_by_status_and_transaction_indices: 0, | ||
| find_withdrawal_documents_by_status_and_transaction_indices: 0, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider simplifying the field name find_withdrawal_documents_by_status_and_transaction_indices
The field name find_withdrawal_documents_by_status_and_transaction_indices is quite long, which may affect readability and maintainability.
Consider renaming it to a more concise name, such as find_withdrawal_documents_by_status_and_indices, to improve clarity.
Apply this diff to rename the field:
- find_withdrawal_documents_by_status_and_transaction_indices: 0,
+ find_withdrawal_documents_by_status_and_indices: 0,📝 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.
| find_withdrawal_documents_by_status_and_transaction_indices: 0, | |
| find_withdrawal_documents_by_status_and_indices: 0, |
| withdrawal_constants: DriveAbciWithdrawalConstants { | ||
| core_expiration_blocks: 24, | ||
| cleanup_expired_locks_of_withdrawal_amounts_limit: 64, | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Simplify field names in DriveAbciWithdrawalConstants
The field cleanup_expired_locks_of_withdrawal_amounts_limit is lengthy, which could affect readability.
Consider renaming it to cleanup_expired_withdrawal_locks_limit for brevity and clarity.
Apply this diff to rename the field:
withdrawal_constants: DriveAbciWithdrawalConstants {
core_expiration_blocks: 24,
- cleanup_expired_locks_of_withdrawal_amounts_limit: 64,
+ cleanup_expired_withdrawal_locks_limit: 64,
},📝 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.
| withdrawal_constants: DriveAbciWithdrawalConstants { | |
| core_expiration_blocks: 24, | |
| cleanup_expired_locks_of_withdrawal_amounts_limit: 64, | |
| }, | |
| withdrawal_constants: DriveAbciWithdrawalConstants { | |
| core_expiration_blocks: 24, | |
| cleanup_expired_withdrawal_locks_limit: 64, | |
| }, |
| pool_withdrawals_into_transactions_queue: 0, | ||
| update_broadcasted_withdrawal_statuses: 0, | ||
| append_signatures_and_broadcast_withdrawal_transactions: 0, | ||
| cleanup_expired_locks_of_withdrawal_amounts: 0, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider simplifying the field name cleanup_expired_locks_of_withdrawal_amounts
The field name cleanup_expired_locks_of_withdrawal_amounts is quite long and may impact readability.
Consider renaming it to a more concise name, such as cleanup_expired_withdrawal_locks, to improve clarity.
Apply this diff to rename the field:
- cleanup_expired_locks_of_withdrawal_amounts: 0,
+ cleanup_expired_withdrawal_locks: 0,📝 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.
| cleanup_expired_locks_of_withdrawal_amounts: 0, | |
| cleanup_expired_withdrawal_locks: 0, |
| credits += fraction_number * multiplier; // Fractional Dash to credits | ||
| } | ||
|
|
||
| credits as u64 |
There was a problem hiding this comment.
@QuantumExplorer Please add test flag then if it's not using in production code
| /// | ||
| /// * `Credits`: The calculated daily withdrawal limit based on the available credits. | ||
| /// | ||
| pub fn daily_withdrawal_limit_v0(total_credits_in_platform: Credits) -> Credits { |
There was a problem hiding this comment.
I mean it's credits
| StateError::NoTransferKeyForCoreWithdrawalAvailableError( | ||
| NoTransferKeyForCoreWithdrawalAvailableError::new( | ||
| identity_credit_withdrawal.identity_id, | ||
| match key.key_type() { |
There was a problem hiding this comment.
I think is_core_address_key_type is not used anymore. Let's remove it.
| PathKeyRefElement((path, key, element)) => { | ||
| // Check if the element already exists | ||
| let existing_element = self.grove_get_raw_optional( | ||
| path.as_slice().into(), | ||
| key, | ||
| apply_type.to_direct_query_type(), | ||
| transaction, | ||
| drive_operations, | ||
| drive_version, | ||
| )?; | ||
| if let Some(existing_element) = existing_element { | ||
| return Ok(Some(existing_element)); | ||
| } | ||
|
|
||
| // Element does not exist, proceed with insertion | ||
| drive_operations.push(LowLevelDriveOperation::insert_for_known_path_key_element( | ||
| path, | ||
| key.to_vec(), | ||
| element, | ||
| )); | ||
| Ok(None) | ||
| } |
There was a problem hiding this comment.
@coderabbitai please suggest the refactoring (actual code)
| PathKeyRefElement((path, key, element)) => { | ||
| if let Element::SumItem(new_value, _) = element { | ||
| // Check if the sum item already exists | ||
| let existing_element = self.grove_get_raw_optional( | ||
| path.as_slice().into(), | ||
| key, | ||
| apply_type.to_direct_query_type(), | ||
| transaction, | ||
| drive_operations, | ||
| drive_version, | ||
| )?; | ||
|
|
||
| if let Some(Element::SumItem(existing_value, _)) = existing_element { | ||
| // Add to the existing sum item | ||
| let updated_value = existing_value | ||
| .checked_add(new_value) | ||
| .ok_or(ProtocolError::Overflow("overflow when adding to sum item"))?; | ||
| drive_operations.push( | ||
| LowLevelDriveOperation::insert_for_known_path_key_element( | ||
| path, | ||
| key.to_vec(), | ||
| Element::new_sum_item(updated_value), | ||
| ), | ||
| ); | ||
| } else if existing_element.is_some() { | ||
| return Err(Error::Drive(DriveError::CorruptedElementType( | ||
| "expected sum item element type", | ||
| ))); | ||
| } else { | ||
| // Insert as a new sum item | ||
| drive_operations.push( | ||
| LowLevelDriveOperation::insert_for_known_path_key_element( | ||
| path, | ||
| key.to_vec(), | ||
| Element::new_sum_item(new_value), | ||
| ), | ||
| ); | ||
| } | ||
| } else { | ||
| return Err(Error::Drive(DriveError::CorruptedCodeExecution( | ||
| "expected sum item element type", | ||
| ))); | ||
| } | ||
| Ok(()) | ||
| } | ||
| PathKeyElement((path, key, element)) => { | ||
| if let Element::SumItem(new_value, _) = element { | ||
| // Check if the sum item already exists | ||
| let existing_element = self.grove_get_raw_optional( | ||
| path.as_slice().into(), | ||
| key.as_slice(), | ||
| apply_type.to_direct_query_type(), | ||
| transaction, | ||
| drive_operations, | ||
| drive_version, | ||
| )?; | ||
|
|
||
| if let Some(Element::SumItem(existing_value, _)) = existing_element { | ||
| // Add to the existing sum item | ||
| let updated_value = existing_value | ||
| .checked_add(new_value) | ||
| .ok_or(ProtocolError::Overflow("overflow when adding to sum item"))?; | ||
| drive_operations.push( | ||
| LowLevelDriveOperation::insert_for_known_path_key_element( | ||
| path, | ||
| key, | ||
| Element::new_sum_item(updated_value), | ||
| ), | ||
| ); | ||
| } else if existing_element.is_some() { | ||
| return Err(Error::Drive(DriveError::CorruptedElementType( | ||
| "expected sum item element type", | ||
| ))); | ||
| } else { | ||
| // Insert as a new sum item | ||
| drive_operations.push( | ||
| LowLevelDriveOperation::insert_for_known_path_key_element( | ||
| path, | ||
| key, | ||
| Element::new_sum_item(new_value), | ||
| ), | ||
| ); | ||
| } | ||
| } else { | ||
| return Err(Error::Drive(DriveError::CorruptedCodeExecution( | ||
| "expected sum item element type", | ||
| ))); | ||
| } | ||
| Ok(()) | ||
| } | ||
| PathFixedSizeKeyRefElement((path, key, element)) => { | ||
| if let Element::SumItem(new_value, _) = element { | ||
| // Check if the sum item already exists | ||
| let existing_element = self.grove_get_raw_optional( | ||
| path.as_slice().into(), | ||
| key, | ||
| apply_type.to_direct_query_type(), | ||
| transaction, | ||
| drive_operations, | ||
| drive_version, | ||
| )?; | ||
|
|
||
| if let Some(Element::SumItem(existing_value, _)) = existing_element { | ||
| // Add to the existing sum item | ||
| let updated_value = existing_value | ||
| .checked_add(new_value) | ||
| .ok_or(ProtocolError::Overflow("overflow when adding to sum item"))?; | ||
| let path_items: Vec<Vec<u8>> = path.into_iter().map(Vec::from).collect(); | ||
| drive_operations.push( | ||
| LowLevelDriveOperation::insert_for_known_path_key_element( | ||
| path_items, | ||
| key.to_vec(), | ||
| Element::new_sum_item(updated_value), | ||
| ), | ||
| ); | ||
| } else if existing_element.is_some() { | ||
| return Err(Error::Drive(DriveError::CorruptedElementType( | ||
| "expected sum item element type", | ||
| ))); | ||
| } else { | ||
| // Insert as a new sum item | ||
| let path_items: Vec<Vec<u8>> = path.into_iter().map(Vec::from).collect(); | ||
| drive_operations.push( | ||
| LowLevelDriveOperation::insert_for_known_path_key_element( | ||
| path_items, | ||
| key.to_vec(), | ||
| Element::new_sum_item(new_value), | ||
| ), | ||
| ); | ||
| } | ||
| } else { | ||
| return Err(Error::Drive(DriveError::CorruptedCodeExecution( | ||
| "expected sum item element type", | ||
| ))); | ||
| } | ||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
@coderabbitai suggest the refactoring
| .get_integer(withdrawal::properties::AMOUNT)?; | ||
|
|
||
| // Check if adding this amount would exceed the current withdrawal limit. | ||
| if total_withdrawal_amount |
There was a problem hiding this comment.
| if total_withdrawal_amount | |
| let total_withdrawal_amount = | |
| total_withdrawal_amount.checked_add(amount).ok_or_else(|| { | |
| Error::Execution(ExecutionError::Overflow( | |
| "overflow in total withdrawal amount", | |
| )) | |
| })?; | |
| // If adding this withdrawal would exceed the limit, stop processing further. | |
| if total_withdrawal_amount > current_withdrawal_limit { | |
| break; | |
| } | |
| // Add this document to the list of documents to be processed. | |
| documents_to_process.push(document); | |
| block_info: &BlockInfo, | ||
| transaction: TransactionArg, | ||
| platform_version: &PlatformVersion, | ||
| ) -> Result<(), Error> { |
There was a problem hiding this comment.
v1 is without the core issue
| }; | ||
| use dpp::identity::{Identity, KeyType, Purpose, SecurityLevel}; | ||
| use dpp::state_transition::StateTransition; | ||
| use dpp::{dash_to_credits, dash_to_duffs}; |
| // Withdrawal documents with pooled status should exist. | ||
| let withdrawal_documents_pooled = outcome | ||
| .abci_app | ||
| .platform | ||
| .drive | ||
| .fetch_oldest_withdrawal_documents_by_status( | ||
| withdrawals_contract::WithdrawalStatus::POOLED.into(), | ||
| DEFAULT_QUERY_LIMIT, | ||
| None, | ||
| platform_version, | ||
| ) | ||
| .unwrap(); | ||
| assert!(withdrawal_documents_pooled.is_empty()); | ||
|
|
There was a problem hiding this comment.
@QuantumExplorer good point!
| #[derive(Clone, Debug, Default)] | ||
| pub struct DriveAbciWithdrawalConstants { | ||
| pub core_expiration_blocks: u32, | ||
| pub cleanup_expired_locks_of_withdrawal_amounts_limit: u16, | ||
| } | ||
|
|
There was a problem hiding this comment.
Verbosity here is better. Rust doc would be even better.
QuantumExplorer
changed the title
Issue being fixed or feature implemented
Withdrawals have a limit on Core, however they were not limited on Platform. We needed to insert such a limit.
What was done?
Withdrawals are now added to a daily amount queue which uses a sum tree to track locked amounts.
How Has This Been Tested?
Added a plethora of tests.
Breaking Changes
This is breaking, and will be activated in version 1.4.
Checklist:
For repository code-owners and collaborators only
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Documentation