refactor(drive): remove duplicated withdrawal amount validation

[shumkov]

Issue being fixed or feature implemented

• Withdrawal amount is validated two times.
• Missing test coverage for withdrawal ST validation

What was done?

• Remove addition amount validation in the validate state function

How Has This Been Tested?

Add test for balance and structure validation

Breaking Changes

None

Checklist:

• I have performed a self-review of my own code
• I have commented my code, particularly in hard-to-understand areas
• I have added or updated relevant unit/integration/functional/e2e tests
• I have added "!" to the title and described breaking changes in the corresponding section if my code contains any
• I have made corresponding changes to the documentation if needed

For repository code-owners and collaborators only

• I have assigned this pull request to a milestone

Summary by CodeRabbit

• New Features

  • Introduced a validation mechanism for identity credit withdrawal transitions, ensuring compliance with specified limits and criteria.
  • Added comprehensive unit tests for the new validation logic.

• Bug Fixes

  • Enhanced error handling for insufficient balance during identity credit withdrawals.

• Refactor

  • Streamlined balance validation logic by removing unnecessary checks and optimizing calculations.
  • Updated validation logic for output scripts and pooling methods.

• Tests

  • Added tests for various scenarios in the identity credit withdrawal process to ensure robustness.

[coderabbitai]

Walkthrough

The changes introduce modifications to the InvalidIdentityCreditWithdrawalTransitionOutputScriptError struct and various methods related to the IdentityCreditWithdrawalTransition. Key updates include refined balance validation logic, enhanced structure validations, and the addition of new test cases to ensure proper functionality. The changes streamline existing methods while implementing new validation mechanisms for withdrawal transitions, improving clarity and error handling in the codebase.

Changes

File Path	Change Summary
packages/rs-dpp/src/errors/consensus/basic/identity/.../invalid_identity_credit_withdrawal_transition_output_script_error.rs	Modified InvalidIdentityCreditWithdrawalTransitionOutputScriptError struct; added comment regarding cloning in the output_script getter method; no changes to method logic or From trait implementation.
packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/identity_credit_withdrawal/balance/v0/mod.rs	Implemented validate_advanced_minimum_balance_pre_check_v0 method; refined balance validation logic; added test module with two test cases to verify balance checks.
packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/identity_credit_withdrawal/state/v0/mod.rs	Modified validate_state_v0 method; removed identity balance checks; streamlined logic by directly calling transform_into_action_v0.
packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/identity_credit_withdrawal/structure/v1/mod.rs	Introduced validate_basic_structure_v1 method for IdentityCreditWithdrawalTransition; added validation for withdrawal amount limits, pooling method, core fee, and output script type; included comprehensive test module.

Possibly related PRs

• feat(platform)!: matched withdrawal fees to actual processing cost #2186: This PR involves changes to the IdentityCreditWithdrawalTransition struct, which is directly related to the changes made in the current PR regarding error handling in withdrawal transactions.
• chore(drive): logs and metrics for withdrawal daily limit #2192: This PR enhances the withdrawal processing logic by incorporating metrics and limits, which relates to the overall context of managing withdrawal transactions and errors, similar to the focus on error handling in the main PR.

Suggested reviewers

• QuantumExplorer

🐰 In fields so wide and bright,
A struct was changed, oh what a sight!
With balance checks and validations clear,
Our code hops forward, full of cheer!
Let errors fade, let logic reign,
In our code garden, we’ll dance in the rain! 🌼

---

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?

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

• Review comments: Directly reply to a review comment made by CodeRabbit. Example:
  • I pushed a fix in commit <commit_id>, please review it.
  • Generate unit testing code for this file.
  • Open a follow-up GitHub issue for this discussion.
• Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
  • @coderabbitai generate unit testing code for this file.
  • @coderabbitai modularize this function.
• PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
  • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
  • @coderabbitai read src/utils.ts and generate unit testing code.
  • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
  • @coderabbitai help me debug CodeRabbit configuration file.

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)

• @coderabbitai pause to pause the reviews on a PR.
• @coderabbitai resume to resume the paused reviews.
• @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
• @coderabbitai full review to do a full review from scratch and review all the files again.
• @coderabbitai summary to regenerate the summary of the PR.
• @coderabbitai resolve resolve all the CodeRabbit review comments.
• @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
• @coderabbitai help to get help.

Other keywords and placeholders

• Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
• Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
• Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

• You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
• Please see the configuration documentation for more information.
• If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

• Visit our Documentation for detailed information on how to use CodeRabbit.
• Join our Discord Community to get help, request features, and share feedback.
• Follow us on X/Twitter for updates and announcements.

[coderabbitai]

Actionable comments posted: 2

🧹 Outside diff range and nitpick comments (4)

packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/identity_credit_withdrawal/balance/v0/mod.rs (2)

33-37: Improved balance check logic with overflow protection.

The changes enhance the clarity and safety of the balance validation logic. The use of checked_add prevents potential overflow issues, which is a great improvement.

Consider making the error message more specific:

- "overflow when adding amount and min_leftover_credits_before_processing in identity credit withdrawal"
+ "overflow when calculating total required balance (amount + fees) for identity credit withdrawal"

---

48-139: Comprehensive test coverage with room for improvement.

The new test module provides good coverage for both valid and invalid scenarios. The use of assert_matches enhances the clarity of test assertions.

Consider adding a test case for the overflow scenario:

#[test]
fn should_return_error_on_overflow() {
    let balance = u64::MAX;
    let amount = u64::MAX;

    let identity = PartialIdentity {
        id: Identifier::random(),
        loaded_public_keys: Default::default(),
        balance: Some(balance),
        revision: None,
        not_found_public_keys: Default::default(),
    };

    let transaction =
        IdentityCreditWithdrawalTransition::V0(IdentityCreditWithdrawalTransitionV0 {
            identity_id: identity.id,
            amount,
            // ... other fields
        });

    let platform_version = &PLATFORM_V1;

    let result = transaction
        .validate_advanced_minimum_balance_pre_check_v0(&identity, platform_version);

    assert_matches!(
        result,
        Err(Error::Execution(ExecutionError::Overflow(_)))
    );
}

This test ensures that the overflow protection we've added is working as expected.

packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/identity_credit_withdrawal/structure/v1/mod.rs (2)

Line range hint 45-49: Ensure consistent handling of unsupported pooling types.

The validation correctly checks that self.pooling() is not Pooling::Never. However, after adding the error to the result, the function returns early with return Ok(result);. This means that subsequent validations (e.g., core fee per byte and output script validations) are skipped when pooling is not Never.

Consider allowing the function to continue validating other fields even if the pooling is incorrect, unless it's intentional to halt validation at this point.

---

Line range hint 53-60: Clarify the core fee per byte validation logic.

The validation checks if self.core_fee_per_byte() is a Fibonacci number, and adds an error if it's not. However, the use of the Fibonacci sequence for core fee validation may not be immediately clear to other developers or may require justification.

Consider adding a comment explaining why the Fibonacci sequence is being used for core_fee_per_byte, or if this is a temporary measure, ensure documentation is updated accordingly.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between 023ff85 and 178dfa4.

📒 Files selected for processing (4)

• packages/rs-dpp/src/errors/consensus/basic/identity/invalid_identity_credit_withdrawal_transition_output_script_error.rs (1 hunks)
• packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/identity_credit_withdrawal/balance/v0/mod.rs (2 hunks)
• packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/identity_credit_withdrawal/state/v0/mod.rs (1 hunks)
• packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/identity_credit_withdrawal/structure/v1/mod.rs (2 hunks)

🧰 Additional context used

📓 Learnings (1)

packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/identity_credit_withdrawal/state/v0/mod.rs (1)

Learnt from: shumkov
PR: dashpay/platform#2186
File: packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/identity_credit_withdrawal/mod.rs:48-54
Timestamp: 2024-09-30T12:11:35.148Z
Learning: In the identity credit withdrawal transition code, the field `platform_version.drive_abci.validation_and_processing.state_transitions.identity_credit_withdrawal_state_transition.transform_into_action` is not an `Option` type, so handling `None` cases is unnecessary.

🔇 Additional comments (4)

packages/rs-dpp/src/errors/consensus/basic/identity/invalid_identity_credit_withdrawal_transition_output_script_error.rs (1)

Line range hint 1-45: Maintain focus on PR objectives

While the added TODO comment raises a valid performance concern, it doesn't align with the PR's main objective of removing duplicated withdrawal amount validation. To maintain a focused and efficient review process:

1. Consider addressing the performance issue in a separate PR or issue.
2. Ensure that the changes in this PR directly contribute to removing the duplicated withdrawal amount validation, as stated in the PR objectives.

To confirm that this file is relevant to the PR's objectives, let's check for any references to withdrawal validation:

If the search doesn't yield relevant results, consider removing changes to this file from the current PR to maintain its focus.

packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/identity_credit_withdrawal/balance/v0/mod.rs (1)

Line range hint 1-139: Summary: Changes align with PR objectives and improve code quality.

The modifications in this file successfully address the PR's objective of removing duplicated withdrawal amount validation. The implementation of validate_advanced_minimum_balance_pre_check_v0 has been streamlined and made more robust with overflow protection. The added test module provides good coverage of the main scenarios.

The changes improve both the efficiency and safety of the code, aligning well with the stated goals of the pull request. The removal of redundant validation and the addition of comprehensive tests enhance the overall quality of the codebase.

packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/identity_credit_withdrawal/state/v0/mod.rs (1)

71-71: LGTM!

Passing platform.drive by value instead of by reference is appropriate if the drive field implements the necessary traits for ownership transfer. This change simplifies the call to try_from_identity_credit_withdrawal and should not introduce any issues.

packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/identity_credit_withdrawal/structure/v1/mod.rs (1)

230-257: Handle the case when output_script is empty.

In the test should_return_invalid_result_if_output_script_is_not_p2pkh_or_p2sh, the output_script is initialized with an empty ScriptBuf. An empty script might not be the only invalid case. There could be scripts that are non-empty but still not P2PKH or P2SH.

Consider extending the test to include such scenarios, ensuring that the validation catches all invalid script types.

Run the following script to search for other usages of CoreScript::new with potentially invalid scripts:

[coderabbitai]

Actionable comments posted: 0

🧹 Outside diff range and nitpick comments (1)

packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/identity_credit_withdrawal/structure/v1/mod.rs (1)

81-337: LGTM: Comprehensive test suite with a minor suggestion

The test suite for validate_basic_structure_v1 is well-structured and covers a wide range of scenarios:

1. Invalid amounts (too low and too high)
2. Invalid pooling
3. Invalid core fee
4. Invalid output script
5. Valid P2PKH output script
6. Valid P2SH output script
7. Valid transition without output script

The use of assert_matches! macro enhances readability and provides detailed error checking. Both positive and negative scenarios are covered, ensuring robust validation.

Consider adding a test case for the upper bound of the valid amount range to ensure the validation works correctly at the boundary condition.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between 31d4b13 and 3624b46.

📒 Files selected for processing (1)

• packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/identity_credit_withdrawal/structure/v1/mod.rs (1 hunks)

🔇 Additional comments (2)

packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/identity_credit_withdrawal/structure/v1/mod.rs (2)

Line range hint 28-79: LGTM: Comprehensive validation implementation

The validate_basic_structure_v1 method provides a thorough validation of the IdentityCreditWithdrawalTransition. It covers all necessary aspects:

1. Amount validation
2. Pooling validation
3. Core fee validation
4. Output script validation

The implementation is well-structured, uses appropriate error types, and follows best practices for error handling and result composition.

---

Line range hint 1-337: Summary: Changes align well with PR objectives

The modifications in this file successfully address the PR objectives:

1. The validate_basic_structure_v1 method provides a single, comprehensive validation for the withdrawal amount, eliminating any potential duplication.
2. The extensive test suite significantly improves the test coverage for withdrawal state validation.

These changes contribute to streamlining the validation process and enhancing the overall robustness of the codebase. The implementation is well-structured, and the tests are comprehensive, covering both positive and negative scenarios.