feat(platform)!: matched withdrawal fees to actual processing cost #2186
Conversation
|
Warning Rate limit exceeded@QuantumExplorer has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 4 minutes and 41 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughThe changes introduce several new features and modifications to existing structures within the codebase. A new variant, Changes
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: 4
🧹 Outside diff range and nitpick comments (7)
packages/rs-platform-version/src/version/fee/processing/mod.rs (2)
13-13: LGTM. Consider adding documentation for the new field.The addition of
perform_network_threshold_signingaligns with the PR objective of updating the fee structure. This new field likely represents the cost associated with performing network threshold signing operations.Consider adding a doc comment to explain the purpose and unit of measurement for this new field. For example:
/// Cost of performing network threshold signing operations, measured in [unit]. pub perform_network_threshold_signing: u64,
30-32: LGTM. Consider updating the test values for clarity.The test function has been correctly updated to include the new
perform_network_threshold_signingfield. The equality check remains valid and will help catch any future changes to the struct.For improved readability and easier maintenance, consider updating the test values to follow a more obvious pattern. For example:
let version1 = FeeProcessingVersion { fetch_identity_balance_processing_cost: 1, fetch_identity_revision_processing_cost: 2, fetch_identity_balance_and_revision_processing_cost: 3, fetch_identity_cost_per_look_up_key_by_id: 4, fetch_single_identity_key_processing_cost: 5, perform_network_threshold_signing: 6, validate_key_structure: 7, fetch_prefunded_specialized_balance_processing_cost: 8, }; let version2 = FeeProcessingVersion { fetch_identity_balance_processing_cost: 1, fetch_identity_revision_processing_cost: 2, fetch_identity_balance_and_revision_processing_cost: 3, fetch_identity_cost_per_look_up_key_by_id: 4, fetch_single_identity_key_processing_cost: 5, perform_network_threshold_signing: 6, validate_key_structure: 7, fetch_prefunded_specialized_balance_processing_cost: 8, };This makes it easier to spot any discrepancies at a glance.
Also applies to: 41-43
packages/rs-platform-version/src/version/fee/mod.rs (1)
Line range hint
1-174: Overall assessment: Changes align with PR objectives, but consider broader implications.The modifications to the
FeeProcessingVersionstruct and the corresponding test updates are well-implemented and align with the PR objective of matching withdrawal fees to actual processing costs. However, consider the following recommendations:
- Document the rationale behind the specific cost changes, especially the increase in
fetch_prefunded_specialized_balance_processing_cost.- Assess the impact of these changes on overall fee calculations in the platform.
- Ensure that any dependent modules or services are updated to accommodate these changes.
- Consider adding more granular tests to verify the behavior of individual fee components.
To ensure a smooth transition and maintain system integrity:
- Review and update any documentation related to fee structures.
- Conduct thorough integration testing to verify that these changes don't introduce unexpected behavior in other parts of the system.
- Consider implementing a feature flag or gradual rollout strategy to monitor the impact of these fee changes in production.
packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/identity_credit_withdrawal/balance/v0/mod.rs (3)
Line range hint
15-22: Inconsistent naming: 'Transfer' vs 'Withdrawal' in trait nameThe trait is named
IdentityCreditTransferTransitionBalanceValidationV0, but it is implemented forIdentityCreditWithdrawalTransition. To maintain consistency and avoid confusion, consider renaming the trait toIdentityCreditWithdrawalTransitionBalanceValidationV0.
Line range hint
24-46: Add unit tests for the new validation methodIt's important to add unit tests for the
validate_advanced_minimum_balance_pre_check_v0method to ensure its correctness and prevent future regressions.Would you like me to help generate the unit tests or open a new GitHub issue to track this task?
Line range hint
37-37: Simplify the condition to improve readabilityThe condition within the
ifstatement is complex and can be simplified for better readability. Consider breaking down the expression into intermediate variables.Here's a suggestion:
let required_balance = self.amount() .checked_add(platform_version.fee_version.state_transition_min_fees.credit_withdrawal) .ok_or(Error::Execution(ExecutionError::Overflow( "overflow when adding amount and min_leftover_credits_before_processing in identity credit withdrawal", )))?; if balance < required_balance { return Ok(SimpleConsensusValidationResult::new_with_error( IdentityInsufficientBalanceError::new(self.identity_id(), balance, self.amount()).into(), )); }packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/identity_credit_withdrawal/mod.rs (1)
Line range hint
75-83: Updateknown_versionsto include all supported versionsIn the error handling of the
validate_basic_structuremethod, theknown_versionsvector currently includes only version0. Since version1is also handled in this method (as seen withSome(1)), theknown_versionsshould be updated to include both versions for accurate error reporting.Apply this diff to update the
known_versions:Some(version) => Err(Error::Execution(ExecutionError::UnknownVersionMismatch { method: "identity credit withdrawal transition: validate_basic_structure" .to_string(), - known_versions: vec![0], + known_versions: vec![0, 1], received: version, })), None => Err(Error::Execution(ExecutionError::VersionNotActive { method: "identity credit withdrawal transition: validate_basic_structure" .to_string(), - known_versions: vec![0], + known_versions: vec![0, 1], })),
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (8)
- packages/rs-drive-abci/src/execution/types/execution_operation/mod.rs (2 hunks)
- packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/identity_credit_withdrawal/balance/v0/mod.rs (1 hunks)
- packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/identity_credit_withdrawal/mod.rs (4 hunks)
- packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/identity_credit_withdrawal/state/v0/mod.rs (6 hunks)
- packages/rs-platform-version/src/version/fee/mod.rs (2 hunks)
- packages/rs-platform-version/src/version/fee/processing/mod.rs (3 hunks)
- packages/rs-platform-version/src/version/fee/processing/v1.rs (1 hunks)
- packages/rs-platform-version/src/version/fee/state_transition_min_fees/v1.rs (1 hunks)
🔇 Additional comments (16)
packages/rs-platform-version/src/version/fee/state_transition_min_fees/v1.rs (3)
5-5: Significant increase in credit withdrawal fee requires explanation and documentation.The
credit_withdrawalfee has been increased from 100000 to 400000000, which is a 4000x increase. While the comment indicates that credit withdrawals are more expensive than other operations, this substantial change warrants further explanation and documentation.Could you please provide more context on this change?
- What are the actual processing costs that justify this increase?
- How will this affect users of the platform?
- Are there any plans to communicate this change to users?
Additionally, consider updating the documentation to reflect this significant change and its rationale. This will help maintain transparency and avoid potential user confusion or dissatisfaction.
Given the magnitude of this change, consider implementing a more flexible fee structure that can be adjusted without code changes, such as:
- Storing fee values in a database or configuration file.
- Implementing a fee calculation algorithm that takes into account various factors (e.g., network load, transaction volume).
This would allow for easier future adjustments and potentially a more nuanced fee structure.
5-5: Update PR description with specific details about the fee change.The change aligns with the PR title "feat(platform)!: matched withdrawal fees to actual processing cost". However, the PR description lacks specific details about this significant fee increase.
Please update the PR description to include:
- The specific issue being addressed (e.g., discrepancy between previous fees and actual costs).
- Detailed explanation of the changes implemented (e.g., why 400000000 was chosen as the new fee).
- Any potential impact on users or the system.
- Testing procedures to ensure the new fee structure works as intended.
This information will help reviewers and future maintainers understand the rationale behind this significant change.
Line range hint
3-11: Consider reviewing the entire fee structure for consistency.While the credit withdrawal fee has been significantly increased, all other fees remain at 100000. This large discrepancy raises questions about the overall fee structure.
- Have the processing costs for other operations been reviewed as well?
- Is there a reason why only the credit withdrawal fee needed such a significant adjustment?
- Should we consider adjusting other fees to maintain a consistent ratio or relationship between different operation costs?
Consider conducting a comprehensive review of all fees to ensure they accurately reflect current processing costs and maintain a logical relationship to each other.
packages/rs-platform-version/src/version/fee/processing/mod.rs (1)
Line range hint
1-49: Consider broader implications and update documentation.The changes to
FeeProcessingVersionalign well with the PR objective of updating the fee structure. However, there are a few additional considerations:
- Ensure that all components using
FeeProcessingVersionare updated to handle the newperform_network_threshold_signingfield.- Update any relevant documentation or API references to reflect this change in the fee structure.
- Consider adding unit tests that specifically validate the behavior of the new
perform_network_threshold_signingfield in various scenarios.To verify the impact of these changes, you can run the following script:
This script will help identify areas of the codebase that might need attention due to the changes in
FeeProcessingVersion.packages/rs-platform-version/src/version/fee/mod.rs (2)
133-135: LGTM: Test case updated correctly.The
test_fee_version_equalitytest case has been properly updated to reflect the changes in theFeeProcessingVersionstruct. This is crucial for maintaining the integrity of theFeeVersionequality checks.Some points to consider:
- The test now includes the new
perform_network_threshold_signingfield.- The order of fields in the test matches the new order in the struct.
- The updated value for
fetch_prefunded_specialized_balance_processing_costis correctly reflected.This update helps ensure that any future changes to the
FeeVersionstruct will be caught by the test, preventing potential bugs related to fee calculations or comparisons.
75-77: Verify the updated fee structure and its implications.The changes to the
FeeProcessingVersionstruct align with the PR objective of matching withdrawal fees to actual processing costs. However, please consider the following:
- The addition of
perform_network_threshold_signingsuggests a new operation being accounted for in fee calculations. Ensure this is documented and its impact on overall fees is understood.- The
fetch_prefunded_specialized_balance_processing_costhas increased from 7 to 8. Verify if this increase is justified and its potential impact on users.- The reordering of fields might affect serialization/deserialization if not handled carefully.
To ensure these changes are consistently applied and don't introduce breaking changes:
packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/identity_credit_withdrawal/state/v0/mod.rs (8)
12-15: Added necessary imports for execution context handlingThe imports for
ValidationOperation,StateTransitionExecutionContext, andStateTransitionExecutionContextMethodsV0are correctly added to support the execution context functionality introduced in this update.
26-26: Updated method signature to include execution contextThe
validate_state_v0method in the trait now includesexecution_context: &mut StateTransitionExecutionContextas a parameter. This allows state validation to interact with the execution context, which is essential for tracking operations during the execution flow.
35-35: Updated method signature to include execution contextSimilarly, the
transform_into_action_v0method now acceptsexecution_context: &mut StateTransitionExecutionContext. This inclusion enables the method to modify the execution context based on the transformation outcome.
48-48: Implemented updatedvalidate_state_v0method with execution contextIn the implementation of
validate_state_v0forIdentityCreditWithdrawalTransition, the addedexecution_contextparameter is correctly propagated, ensuring that the execution context can be utilized within the method.
75-81: Correctly passing execution context totransform_into_action_v0The call to
self.transform_into_action_v0withinvalidate_state_v0now includes theexecution_contextparameter. This ensures that any modifications to the execution context withintransform_into_action_v0are reflected in the overall execution flow.
88-88: Implemented updatedtransform_into_action_v0method with execution contextThe method signature for
transform_into_action_v0now includesexecution_context: &mut StateTransitionExecutionContext, allowing it to add operations to the execution context as needed.
103-107: Adding operation to execution context upon successful validationWhen the
consensus_validation_resultis valid, the code correctly addsValidationOperation::PerformNetworkThresholdSigningto theexecution_context. This ensures that the network threshold signing operation is scheduled only when the validation passes.
26-26: Verify updates to method signatures across the codebaseThe method signatures for
validate_state_v0andtransform_into_action_v0now include theexecution_contextparameter. Please ensure that all implementations and invocations of these methods across the codebase have been updated to match the new signatures to prevent potential compilation errors.Run the following script to identify any usages of these methods that may need updating:
Also applies to: 35-35, 48-48, 88-88
packages/rs-drive-abci/src/execution/types/execution_operation/mod.rs (1)
70-70: Addition ofPerformNetworkThresholdSigningvariantThe new
PerformNetworkThresholdSigningvariant is appropriately added to theValidationOperationenum and follows the existing naming conventions.packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/identity_credit_withdrawal/mod.rs (1)
36-36: Ensureexecution_contextis utilized appropriatelyThe
execution_contextparameter has been added to the method signature. Please verify that it is being used within the method as intended and that any necessary modifications to the execution context are performed.
...rc/execution/validation/state_transition/state_transitions/identity_credit_withdrawal/mod.rs
Show resolved
Hide resolved
...rc/execution/validation/state_transition/state_transitions/identity_credit_withdrawal/mod.rs
Show resolved
Hide resolved
Issue being fixed or feature implemented
This PR addresses the need to better match withdrawal fees to the actual processing costs, ensuring that fees reflect the workload incurred by the platform when processing withdrawals. It solves discrepancies between the previously estimated fees and the actual cost incurred during the withdrawal process, leading to a more accurate and fair fee calculation.
What was done?
Added a fee of around 2.5 cents at a Dash price of 25$ for withdrawals.
How Has This Been Tested?
Needs testing on Testnet, not yet tested.
Breaking Changes
Since fees change it is is a breaking change that will take effect in v4 of the protocol.
Checklist:
For repository code-owners and collaborators only
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Refactor