chore(drive): logs and metrics for withdrawal daily limit #2192
Conversation
WalkthroughThe changes in this pull request enhance the functionality and clarity of withdrawal transaction handling within the codebase. Key updates include the introduction of a new struct for withdrawal limit information, the enhancement of withdrawal processing methods to incorporate metrics, and adjustments to existing method signatures to reflect these changes. Overall, the modifications provide more detailed insights into withdrawal limits while refining the control flow for processing withdrawal transactions. Changes
Possibly related PRs
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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 (
|
...src/execution/platform_events/withdrawals/pool_withdrawals_into_transactions_queue/v1/mod.rs
Outdated
Show resolved
Hide resolved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (9)
packages/rs-drive/src/drive/identity/withdrawals/transaction/queue/dequeue_untied_withdrawal_transactions/mod.rs (1)
12-13: Documentation update looks good, but consider adding more details.The updated documentation clearly describes the purpose of the method. However, to improve clarity and align with the PR objectives, consider adding information about logging and metrics.
Consider expanding the documentation to include:
- Mention of any logging that occurs during the dequeuing process.
- Information about metrics that are updated, if any.
- The significance of the
limitparameter in relation to withdrawal limits.Example:
/// Dequeue specified amount of untied withdrawal transactions /// and move them to broadcasted queue. /// This operation logs the withdrawal limit in action and updates /// Prometheus metrics for total withdrawal amount and available limit. /// The `limit` parameter controls the maximum number of transactions to dequeue.packages/rs-drive/src/drive/identity/withdrawals/calculate_current_withdrawal_limit/v0/mod.rs (2)
45-45: Approve signature change, but update function documentation.The change in return type from
Result<Credits, Error>toResult<WithdrawalLimitInfo, Error>enhances the function's output by providing more detailed information about the withdrawal limit. This aligns well with the PR objective of improving visibility into the withdrawal logic.However, the function's documentation (comments above the function) needs to be updated to reflect this change in the return type and explain the new
WithdrawalLimitInfostructure.Could you please update the function's documentation to accurately describe the new return type and its contents?
84-87: Approve return statement change, suggest minor improvement for consistency.The new return statement using
WithdrawalLimitInfoprovides more comprehensive information about the withdrawal limit, including both the daily maximum and the amount already withdrawn. This change aligns well with the PR objective of adding metrics for withdrawal limits.For improved consistency and clarity, consider updating the field name in the return statement:
Ok(WithdrawalLimitInfo { daily_maximum, - withdrawals_amount: withdrawal_amount_in_last_day, + withdrawal_amount_in_last_day, })This change makes the field name consistent with the variable name used in the function body, enhancing readability and maintaining a clear connection between the calculated value and the returned field.
packages/rs-drive-abci/src/metrics.rs (2)
32-35: LGTM: New gauge metric constants added correctly.The addition of
GAUGE_CREDIT_WITHDRAWAL_LIMIT_AVAILABLEandGAUGE_CREDIT_WITHDRAWAL_LIMIT_TOTALis consistent with the PR objectives. The naming and visibility are appropriate.Consider adding "in credits" to the end of both comments for consistency with the constant names:
- /// Withdrawal daily limit available credits + /// Withdrawal daily limit available in credits - /// Total withdrawal daily limit in credits + /// Total withdrawal daily limit available in credits
217-227: LGTM: New gauge metrics correctly registered.The new gauge metrics for credit withdrawal limits are properly registered using the
describe_gauge!macro. The implementation is consistent with other metrics in the function.For consistency with other metric descriptions in this function, consider adding the unit of measurement to the descriptions:
describe_gauge!( GAUGE_CREDIT_WITHDRAWAL_LIMIT_AVAILABLE, - "Available withdrawal limit for last 24 hours in credits" + metrics::Unit::Count, + "Available withdrawal limit for last 24 hours in credits" ); describe_gauge!( GAUGE_CREDIT_WITHDRAWAL_LIMIT_TOTAL, - "Total withdrawal limit for last 24 hours in credits" + metrics::Unit::Count, + "Total withdrawal limit for last 24 hours in credits" );packages/rs-drive/src/drive/identity/withdrawals/calculate_current_withdrawal_limit/mod.rs (2)
18-23: Consider adding unit tests foravailablemethod.Adding unit tests for the
availablemethod inWithdrawalLimitInfowould ensure it correctly calculates the available credits under various scenarios, such as when thewithdrawals_amountexceeds thedaily_maximum.
Line range hint
24-51: Update method documentation to reflect the new return type.The documentation for
calculate_current_withdrawal_limitstill referencesOk(Credits)as the return type. Please update it to reflect the new return typeOk(WithdrawalLimitInfo)and adjust the description accordingly.Apply this diff to correct the documentation:
/// # Returns /// -/// * `Ok(Credits)`: The calculated current withdrawal limit, representing the maximum amount that can still be withdrawn in the current 24-hour window. +/// * `Ok(WithdrawalLimitInfo)`: The calculated withdrawal limit information, including the daily maximum and the amount already withdrawn. /// * `Err(Error)`: Returns an error if the version specified in `platform_version` is not supported or if there is an issue in the version-specific calculation.packages/rs-drive-abci/src/execution/platform_events/withdrawals/pool_withdrawals_into_transactions_queue/v1/mod.rs (1)
84-87: Improve clarity of warning message.Consider rephrasing the warning message to enhance clarity and readability.
You might consider changing it to:
tracing::warn!( - "Pooling is limited due to daily withdrawals limit. {} credits left", + "Pooling is limited due to the daily withdrawal limit. {} credits remaining", current_withdrawal_limit );packages/rs-drive-abci/src/execution/engine/run_block_proposal/v0/mod.rs (1)
340-340: Add the missing article in the commentThe comment on line 340 is missing an article and should read "to update the daily withdrawal limit."
Suggested fix:
-// to update daily withdrawal limit +// to update the daily withdrawal limit
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
- packages/rs-drive-abci/src/execution/engine/run_block_proposal/v0/mod.rs (3 hunks)
- packages/rs-drive-abci/src/execution/platform_events/withdrawals/pool_withdrawals_into_transactions_queue/v0/mod.rs (1 hunks)
- packages/rs-drive-abci/src/execution/platform_events/withdrawals/pool_withdrawals_into_transactions_queue/v1/mod.rs (5 hunks)
- packages/rs-drive-abci/src/metrics.rs (3 hunks)
- packages/rs-drive/src/drive/identity/withdrawals/calculate_current_withdrawal_limit/mod.rs (2 hunks)
- packages/rs-drive/src/drive/identity/withdrawals/calculate_current_withdrawal_limit/v0/mod.rs (3 hunks)
- packages/rs-drive/src/drive/identity/withdrawals/transaction/queue/dequeue_untied_withdrawal_transactions/mod.rs (1 hunks)
🧰 Additional context used
📓 Learnings (2)
packages/rs-drive-abci/src/execution/platform_events/withdrawals/pool_withdrawals_into_transactions_queue/v0/mod.rs (1)
Learnt from: QuantumExplorer PR: dashpay/platform#2182 File: packages/rs-drive-abci/src/execution/platform_events/withdrawals/pool_withdrawals_into_transactions_queue/v0/mod.rs:71-73 Timestamp: 2024-09-28T22:25:00.684Z Learning: When retrieving the `AMOUNT` property in withdrawal documents, it is guaranteed to be present and valid, so additional error handling when calling `get_integer` is not necessary.packages/rs-drive-abci/src/execution/platform_events/withdrawals/pool_withdrawals_into_transactions_queue/v1/mod.rs (1)
Learnt from: QuantumExplorer PR: dashpay/platform#2182 File: packages/rs-drive-abci/src/execution/platform_events/withdrawals/pool_withdrawals_into_transactions_queue/v0/mod.rs:71-73 Timestamp: 2024-09-28T22:25:00.684Z Learning: When retrieving the `AMOUNT` property in withdrawal documents, it is guaranteed to be present and valid, so additional error handling when calling `get_integer` is not necessary.
🔇 Additional comments (12)
packages/rs-drive/src/drive/identity/withdrawals/calculate_current_withdrawal_limit/v0/mod.rs (3)
2-2: LGTM: New import aligns with the function's updated return type.The addition of the
WithdrawalLimitInfoimport is consistent with the changes made to the function's return type. This import ensures that the new structure is available for use within the module.
Line range hint
1-87: Verify implementation of logging for withdrawal limits.The changes successfully enhance the metrics for withdrawal limits by introducing the
WithdrawalLimitInfostructure, which aligns with one of the PR objectives. However, the PR summary also mentions adding logging for the withdrawal limit, which is not evident in this function.Could you please confirm if logging for withdrawal limits has been implemented elsewhere? If not, consider adding appropriate logging statements to this function to fully meet the PR objectives. For example:
log::info!("Withdrawal limit calculated: daily_maximum={}, withdrawal_amount_in_last_day={}", daily_maximum, withdrawal_amount_in_last_day);This logging statement could be added just before the return statement to provide visibility into the calculated withdrawal limits.
Line range hint
1-87: Summary of review for calculate_current_withdrawal_limit_v0The changes in this file enhance the withdrawal limit calculation by introducing the
WithdrawalLimitInfostructure, which provides more detailed metrics about withdrawal limits. This aligns well with the PR objectives of improving visibility into the withdrawal logic.Key points:
- The function signature and return type have been updated appropriately.
- The new return statement provides more comprehensive information about withdrawal limits.
- The changes partially fulfill the PR objectives by enhancing metrics for withdrawal limits.
Suggestions for improvement:
- Update the function's documentation to reflect the new return type and explain the
WithdrawalLimitInfostructure.- Consider adding logging statements to fully meet the PR objectives of adding logging for withdrawal limits.
- Make the field name in the return statement consistent with the variable name used in the function body for improved clarity.
Overall, the changes are well-implemented and contribute positively to the codebase's functionality and clarity.
packages/rs-drive-abci/src/execution/platform_events/withdrawals/pool_withdrawals_into_transactions_queue/v0/mod.rs (4)
22-23: Improved comment clarity on quorum selection.The updated comment provides valuable insight into why the first quorum is used. This explanation enhances code maintainability by clearly stating the relationship between the platform and Core, which is crucial for understanding this implementation.
27-28: Improved consistency in warning message.The updated warning message now uses "do not pool withdrawals" instead of "not making withdrawals". This change:
- Aligns better with the method's name and purpose.
- Provides more precise information about the action being skipped.
- Maintains consistency in terminology throughout the code.
Good job on improving clarity without altering functionality.
33-36: Consistent terminology in debug message.The debug message has been updated to maintain consistency with the earlier warning message:
- "do not pool withdrawals" is now used instead of "not making withdrawals".
- This change reinforces the precise nature of the skipped action.
- The slight formatting adjustment (moving the position to a new line) enhances readability.
These modifications contribute to a more cohesive and clear codebase.
22-36: Overall improvement in code clarity and consistency.The changes in this file focus on enhancing the clarity of comments and log messages related to the withdrawal pooling process. Key points:
- Improved explanation of quorum selection logic, providing crucial context for future maintenance.
- Consistent use of terminology across warning and debug messages ("do not pool withdrawals").
- No functional changes to the code logic, maintaining the original behavior.
These updates align well with the PR objectives of enhancing visibility for withdrawal logic. While no new logging or metrics have been added in this specific file, the improved messages contribute to a better understanding of the withdrawal process.
The changes are approved as they improve code maintainability without introducing any security issues or performance impacts.
packages/rs-drive-abci/src/metrics.rs (2)
9-9: LGTM: Import statement updated correctly.The addition of
describe_gaugeto the import statement is appropriate and necessary for the new gauge metrics being introduced in this file.
Line range hint
1-338: Overall changes align well with PR objectives.The additions to this file successfully implement new gauge metrics for credit withdrawal limits, which aligns with the PR's goal of enhancing visibility into the withdrawal logic. The changes are well-integrated into the existing metrics system and maintain consistency with the file's structure and coding style.
To ensure that these new metrics are being utilized correctly, it would be beneficial to verify their usage in the withdrawal logic implementation. Could you provide the relevant code where these metrics are updated?
✅ Verification successful
Metrics Integration Verified
The new gauge metrics for credit withdrawal limits are properly defined and utilized within the withdrawal logic implementation. The integration aligns with the PR's objectives of enhancing visibility into the withdrawal logic without altering existing functionalities.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for usage of the new gauge metrics in the codebase rg "GAUGE_CREDIT_WITHDRAWAL_LIMIT_(AVAILABLE|TOTAL)" --type rustLength of output: 1130
packages/rs-drive/src/drive/identity/withdrawals/calculate_current_withdrawal_limit/mod.rs (1)
52-52:⚠️ Potential issuePotential breaking change: Method return type modified.
The return type of
calculate_current_withdrawal_limithas changed fromResult<Credits, Error>toResult<WithdrawalLimitInfo, Error>. This alteration may impact existing code that relies on this method. Ensure that all callers are updated to handle the new return type.Run the following script to locate all usages of
calculate_current_withdrawal_limit:✅ Verification successful
Return type modification verified successfully.
All call sites have been updated to handle the new
WithdrawalLimitInforeturn type appropriately, ensuring no existing code is impacted.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find all call sites of `calculate_current_withdrawal_limit` and display their context. rg --type rust -A 3 -B 3 'calculate_current_withdrawal_limit\('Length of output: 2194
Script:
#!/bin/bash # Description: Retrieve the lines following each call to `calculate_current_withdrawal_limit` to inspect handling of the new return type. rg --type rust 'calculate_current_withdrawal_limit\(' -A 5Length of output: 1807
packages/rs-drive-abci/src/execution/engine/run_block_proposal/v0/mod.rs (2)
356-356: Verify thatBlockExecutionContextV0includesepoch_infofieldThe
epoch_infofield is added to the initialization ofBlockExecutionContextV0. Please ensure that theBlockExecutionContextV0struct has been updated to include theepoch_infofield.To automate this verification, you can run:
#!/bin/bash # Description: Check if `BlockExecutionContextV0` struct includes `epoch_info` field. # Use `ast-grep` to find the struct definition with `epoch_info` ast-grep --lang rust --pattern ' struct BlockExecutionContextV0 { $_block_state_info, epoch_info: $_, $$$ } '
334-334: Verify function signature matches updated parametersThe function
pool_withdrawals_into_transactions_queueis now called with an additional parameterlast_committed_platform_state. Please ensure that the function's definition has been updated to accept this new parameter.To automate this verification, you can run:
✅ Verification successful
Function signature verified
The
pool_withdrawals_into_transactions_queuefunction inpackages/rs-drive-abci/src/execution/platform_events/withdrawals/pool_withdrawals_into_transactions_queue/mod.rsincludes the updated parameterlast_committed_platform_state: &PlatformState.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if `pool_withdrawals_into_transactions_queue` accepts `last_committed_platform_state` as a parameter. # Use `ast-grep` to find the function definition including `last_committed_platform_state` ast-grep --lang rust --pattern ' fn pool_withdrawals_into_transactions_queue( &self, $_block_info, last_committed_platform_state: $_, $_transaction, $_platform_version ) $_body 'Length of output: 291
Script:
#!/bin/bash # Description: Check if `pool_withdrawals_into_transactions_queue` accepts `last_committed_platform_state` as a parameter. # Use `ast-grep` to find the function definition including `last_committed_platform_state` ast-grep --lang rust --pattern 'fn pool_withdrawals_into_transactions_queue(&self, $_block_info, last_committed_platform_state: $_, $_transaction, $_platform_version) $_body'Length of output: 521
Script:
#!/bin/bash # Description: Verify if `pool_withdrawals_into_transactions_queue` includes `last_committed_platform_state` as a parameter with a flexible pattern. # Use `ast-grep` with a less restrictive pattern to match the function definition ast-grep --lang rust --pattern 'fn pool_withdrawals_into_transactions_queue(&self, $$$, last_committed_platform_state: $_, $$$) $_'Length of output: 478
Script:
#!/bin/bash # Description: Search for the `pool_withdrawals_into_transactions_queue` function and verify the presence of `last_committed_platform_state` parameter. # Use `rg` to find the function definition rg 'fn\s+pool_withdrawals_into_transactions_queue\s*\(' --glob '*.rs' -A 5Length of output: 1011
packages/rs-drive/src/drive/identity/withdrawals/calculate_current_withdrawal_limit/mod.rs
Outdated
Show resolved
Hide resolved
...src/execution/platform_events/withdrawals/pool_withdrawals_into_transactions_queue/v1/mod.rs
Outdated
Show resolved
Hide resolved
packages/rs-drive-abci/src/execution/engine/run_block_proposal/v0/mod.rs
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
packages/rs-drive-abci/src/execution/platform_events/withdrawals/pool_withdrawals_into_transactions_queue/v1/mod.rs (2)
53-61: LGTM: Withdrawal limit calculation and metrics.The added code for calculating the current withdrawal limit and storing metrics using Prometheus gauges aligns well with the PR objectives. This will enhance the visibility of the withdrawal logic as intended.
Consider adding a debug log statement to output the calculated withdrawal limit for easier troubleshooting:
tracing::debug!("Current withdrawal limit calculated: {}", current_withdrawal_limit);
82-87: LGTM: Enhanced withdrawal processing logic.The modifications to stop processing when the total withdrawal amount exceeds the current limit and the addition of a debug log are good improvements. These changes align well with the PR objectives of enhancing the withdrawal logic.
Consider using string interpolation for better readability in the debug log:
tracing::debug!( "Pooling is limited due to daily withdrawals limit. {current_withdrawal_limit} credits left" );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- packages/rs-drive-abci/src/execution/platform_events/withdrawals/pool_withdrawals_into_transactions_queue/v1/mod.rs (5 hunks)
- packages/rs-drive/src/drive/identity/withdrawals/calculate_current_withdrawal_limit/mod.rs (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/rs-drive/src/drive/identity/withdrawals/calculate_current_withdrawal_limit/mod.rs
🧰 Additional context used
📓 Learnings (1)
packages/rs-drive-abci/src/execution/platform_events/withdrawals/pool_withdrawals_into_transactions_queue/v1/mod.rs (1)
Learnt from: QuantumExplorer PR: dashpay/platform#2182 File: packages/rs-drive-abci/src/execution/platform_events/withdrawals/pool_withdrawals_into_transactions_queue/v0/mod.rs:71-73 Timestamp: 2024-09-28T22:25:00.684Z Learning: When retrieving the `AMOUNT` property in withdrawal documents, it is guaranteed to be present and valid, so additional error handling when calling `get_integer` is not necessary.
🔇 Additional comments (3)
packages/rs-drive-abci/src/execution/platform_events/withdrawals/pool_withdrawals_into_transactions_queue/v1/mod.rs (3)
2-2: LGTM: New imports for metrics.The added imports for metrics are appropriate for the new functionality being introduced in this file.
Also applies to: 13-15
35-38: Acknowledged: TODO for future optimization.The TODO comment suggests a potential optimization for querying documents. As discussed in a previous review, an issue has been created to track this improvement (Issue #2197). This ensures the optimization will be addressed in a future PR.
Line range hint
26-180: Overall assessment: Good implementation of withdrawal limit logging and metrics.The changes in this PR successfully implement logging and metrics for withdrawal limits, aligning well with the stated objectives. Key improvements include:
- Addition of Prometheus metrics for available and total withdrawal limits.
- Enhanced logic to stop processing when the total withdrawal amount exceeds the current limit.
- Debug logging to indicate when pooling is limited due to the daily withdrawal limit.
These changes will significantly improve the visibility of the withdrawal process and facilitate testing of withdrawal limits. The implementation is correct and doesn't introduce breaking changes.
One suggestion for future improvement:
- Consider implementing the optimization mentioned in the TODO comment (querying documents less than the current limit) in a future PR to further enhance efficiency.
Issue being fixed or feature implemented
To test withdrawal limits we need some additional visibility on the withdrawal logic
What was done?
How Has This Been Tested?
None
Breaking Changes
None
Checklist:
For repository code-owners and collaborators only
Summary by CodeRabbit
New Features
WithdrawalLimitInfostruct to provide detailed withdrawal limit information.Bug Fixes
Documentation