Skip to content
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ use bincode::{Decode, Encode};
#[derive(
Error, Debug, Clone, PartialEq, Eq, Encode, Decode, PlatformSerialize, PlatformDeserialize,
)]
#[error("Credit withdrawal amount {amount} must be greater or equal to {min_amount}")]
#[error("Credit withdrawal amount {amount} must be greater or equal to {min_amount} and less than {max_amount}")]
#[platform_serialize(unversioned)]
pub struct InvalidIdentityCreditWithdrawalTransitionAmountError {
/*
Expand All @@ -20,11 +20,16 @@ pub struct InvalidIdentityCreditWithdrawalTransitionAmountError {
*/
pub amount: u64,
pub min_amount: u64,
pub max_amount: u64,
}

impl InvalidIdentityCreditWithdrawalTransitionAmountError {
pub fn new(amount: u64, min_amount: u64) -> Self {
Self { amount, min_amount }
pub fn new(amount: u64, min_amount: u64, max_amount: u64) -> Self {
Self {
amount,
min_amount,
max_amount,
}
}

pub fn amount(&self) -> u64 {
Expand All @@ -34,6 +39,10 @@ impl InvalidIdentityCreditWithdrawalTransitionAmountError {
pub fn min_amount(&self) -> u64 {
self.min_amount
}

pub fn max_amount(&self) -> u64 {
self.max_amount
}
}

impl From<InvalidIdentityCreditWithdrawalTransitionAmountError> for ConsensusError {
Expand Down
10 changes: 10 additions & 0 deletions packages/rs-dpp/src/identity/core_script.rs
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,16 @@ impl CoreScript {
Self::from_bytes(bytes)
}

pub fn new_p2sh(script_hash: [u8; 20]) -> Self {
let mut bytes = vec![
opcodes::all::OP_HASH160.to_u8(),
opcodes::all::OP_PUSHBYTES_20.to_u8(),
];
bytes.extend_from_slice(&script_hash);
bytes.push(opcodes::all::OP_EQUAL.to_u8());
Self::from_bytes(bytes)
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Consider refactoring to reduce code duplication.

The new_p2sh method is similar to the existing random_p2sh method. To reduce duplication and improve maintainability, consider refactoring these methods to share common logic.

Here's a suggested refactoring:

impl CoreScript {
    // ... (other methods)

    fn create_p2sh_script(script_hash: [u8; 20]) -> Vec<u8> {
        let mut bytes = vec![
            opcodes::all::OP_HASH160.to_u8(),
            opcodes::all::OP_PUSHBYTES_20.to_u8(),
        ];
        bytes.extend_from_slice(&script_hash);
        bytes.push(opcodes::all::OP_EQUAL.to_u8());
        bytes
    }

    pub fn new_p2sh(script_hash: [u8; 20]) -> Self {
        Self::from_bytes(Self::create_p2sh_script(script_hash))
    }

    pub fn random_p2sh(rng: &mut StdRng) -> Self {
        Self::from_bytes(Self::create_p2sh_script(rng.gen()))
    }
}

This refactoring extracts the common logic into a private create_p2sh_script method, which is then used by both new_p2sh and random_p2sh.


pub fn random_p2pkh(rng: &mut StdRng) -> Self {
Self::new_p2pkh(rng.gen::<[u8; 20]>())
}
Expand Down
18 changes: 18 additions & 0 deletions packages/rs-dpp/src/withdrawal/daily_withdrawal_limit/mod.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
use crate::fee::Credits;
use crate::withdrawal::daily_withdrawal_limit::v0::daily_withdrawal_limit_v0;
use crate::ProtocolError;
use platform_version::version::PlatformVersion;

mod v0;

pub fn daily_withdrawal_limit(
total_credits_in_platform: Credits,
platform_version: &PlatformVersion,
) -> Result<Credits, ProtocolError> {
match platform_version.dpp.methods.daily_withdrawal_limit {
0 => Ok(daily_withdrawal_limit_v0(total_credits_in_platform)),
v => Err(ProtocolError::UnknownVersionError(format!(
"Unknown daily_withdrawal_limit version {v}"
))),
}
}
31 changes: 31 additions & 0 deletions packages/rs-dpp/src/withdrawal/daily_withdrawal_limit/v0/mod.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
use crate::fee::Credits;
use crate::ProtocolError;

Check warning on line 2 in packages/rs-dpp/src/withdrawal/daily_withdrawal_limit/v0/mod.rs

View workflow job for this annotation

GitHub Actions / Rust packages (dash-sdk) / Linting

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

Check warning on line 2 in packages/rs-dpp/src/withdrawal/daily_withdrawal_limit/v0/mod.rs

View workflow job for this annotation

GitHub Actions / Rust packages (dpp) / Linting

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

Check warning on line 2 in packages/rs-dpp/src/withdrawal/daily_withdrawal_limit/v0/mod.rs

View workflow job for this annotation

GitHub Actions / Rust packages (drive) / Linting

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

Check warning on line 2 in packages/rs-dpp/src/withdrawal/daily_withdrawal_limit/v0/mod.rs

View workflow job for this annotation

GitHub Actions / Rust packages (wasm-dpp) / Linting

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

Check warning on line 2 in packages/rs-dpp/src/withdrawal/daily_withdrawal_limit/v0/mod.rs

View workflow job for this annotation

GitHub Actions / Rust packages (drive-abci) / Linting

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

/// Calculates the daily withdrawal limit based on the total credits available in the platform.
///
/// The function enforces the following rules:
///
/// 1. If the total credits are 1000 or more:
/// - The withdrawal limit is set to 10% of the total credits.
/// 2. If the total credits are between 100 and 999:
/// - The withdrawal limit is capped at 100 credits.
/// 3. If the total credits are less than 100:
/// - The withdrawal limit is the total available credits, as no more than the available amount can be withdrawn.
///
/// # Parameters
///
/// * `total_credits_in_platform`: The total amount of credits available in the platform.
///
/// # Returns
///
/// * `Credits`: The calculated daily withdrawal limit based on the available credits.
///
pub fn daily_withdrawal_limit_v0(total_credits_in_platform: Credits) -> Credits {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it credits in dash?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean it's credits

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, fixed.

if total_credits_in_platform >= 1000 {
total_credits_in_platform / 10
} else if total_credits_in_platform >= 100 {
100
} else {
total_credits_in_platform
}
}
Copy link
Contributor

@coderabbitai coderabbitai bot Sep 28, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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:

  1. Input validation: Consider handling invalid input, such as negative credit values.
  2. Unit tests: Add unit tests to verify the function's behavior for different input scenarios.
  3. 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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Credits is a u64. I don't think a division by 10 could ever overflow.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

3 changes: 3 additions & 0 deletions packages/rs-dpp/src/withdrawal/mod.rs
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
pub mod daily_withdrawal_limit;
#[cfg(feature = "system_contracts")]
mod document_try_into_asset_unlock_base_transaction_info;

Expand All @@ -19,4 +20,6 @@ pub enum Pooling {
pub type WithdrawalTransactionIndex = u64;

/// Simple type alias for withdrawal transaction with it's index

pub type WithdrawalTransactionIndexAndBytes = (WithdrawalTransactionIndex, Vec<u8>);
pub type WithdrawalTransactionIndexAndBytesAndAmounts = (WithdrawalTransactionIndex, Vec<u8>, u64);
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ use crate::platform_types::platform_state::PlatformState;
use crate::platform_types::validator_set::ValidatorSetExt;
use dpp::version::PlatformVersion;
use std::sync::Arc;
use tenderdash_abci::proto::abci::{RequestInitChain, ResponseInitChain, ValidatorSetUpdate};
use tenderdash_abci::proto::abci::{RequestInitChain, ResponseInitChain};
use tenderdash_abci::proto::google::protobuf::Timestamp;
use tenderdash_abci::proto::serializers::timestamp::FromMilis;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -91,8 +91,20 @@ Your software version: {}, latest supported protocol version: {}."#,
);
};

// Set current protocol version to the block platform state
block_platform_state.set_current_protocol_version_in_consensus(next_protocol_version);
let old_protocol_version = block_platform_state.current_protocol_version_in_consensus();

if old_protocol_version != next_protocol_version {
// Set current protocol version to the block platform state
block_platform_state
.set_current_protocol_version_in_consensus(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(
&epoch_info,
transaction,
old_protocol_version,
next_platform_version,
)?;
}

next_platform_version
} else {
Expand Down
Original file line number Diff line number Diff line change
@@ -1,2 +1,3 @@
mod check_for_desired_protocol_upgrade;
mod perform_events_on_first_block_of_protocol_change;
mod upgrade_protocol_version;
Original file line number Diff line number Diff line change
@@ -0,0 +1,71 @@
mod v0;

use crate::error::execution::ExecutionError;
use crate::error::Error;
use crate::platform_types::epoch_info::EpochInfo;
use crate::platform_types::platform::Platform;
use dpp::version::PlatformVersion;
use dpp::version::ProtocolVersion;
use drive::grovedb::Transaction;

impl<C> Platform<C> {
/// Executes specific events that need to be performed on the first block of a protocol change.
///
/// # Parameters
///
/// - `&self`: A reference to the current instance of the struct implementing this function.
/// - `block_info`: Information about the current block, encapsulated in a `BlockInfo` struct.
/// - `epoch_info`: Information about the current epoch, encapsulated in an `EpochInfo` struct.
/// - `last_committed_platform_state`: The state of the platform as it was after the last committed block, before the protocol change.
/// - `block_platform_state`: A mutable reference to the platform state that will be modified for the current block.
/// - `transaction`: The transaction object representing the current transaction context.
/// - `platform_version`: The version of the platform being executed, encapsulated in a `PlatformVersion` struct.
///
/// # Returns
///
/// - `Result<(), Error>`: Returns `Ok(())` if the events are successfully executed.
/// Returns an `Error` if there is a version mismatch or another execution issue.
///
/// # Errors
///
/// - Returns an `Error::Execution(ExecutionError::UnknownVersionMismatch)` if an unknown version is received
/// that is not supported by the current implementation.
///
/// # Versioning
///
/// This function uses the `platform_version` parameter to determine which version-specific implementation
/// of the protocol change events should be executed:
///
/// - If the version is `0`, it calls the `perform_events_on_first_block_of_protocol_change_v0` function,
/// which contains the logic for version `0`.
/// - If no version is specified (`None`), the function does nothing and returns `Ok(())`.
/// - If a different version is specified, it returns an error indicating an unknown version mismatch.
///
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Inconsistent documentation: parameters listed do not match function signature

The documentation comments list parameters block_info, last_committed_platform_state, and block_platform_state, which are not present in the actual function signature. Please update the documentation to accurately reflect the function's parameters.

pub fn perform_events_on_first_block_of_protocol_change(
&self,
epoch_info: &EpochInfo,
transaction: &Transaction,
previous_protocol_version: ProtocolVersion,
platform_version: &PlatformVersion,
) -> Result<(), Error> {
match platform_version
.drive_abci
.methods
.protocol_upgrade
.perform_events_on_first_block_of_protocol_change
{
Some(0) => self.perform_events_on_first_block_of_protocol_change_v0(
epoch_info,
transaction,
previous_protocol_version,
platform_version,
),
None => return Ok(()),
Some(version) => Err(Error::Execution(ExecutionError::UnknownVersionMismatch {
method: "perform_events_on_first_block_of_protocol_change".to_string(),
known_versions: vec![0],
received: version,
})),
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
use crate::error::Error;
use crate::platform_types::epoch_info::EpochInfo;
use crate::platform_types::platform::Platform;
use dpp::version::PlatformVersion;
use dpp::version::ProtocolVersion;
use drive::drive::identity::withdrawals::paths::{
get_withdrawal_root_path, WITHDRAWAL_TRANSACTIONS_SUM_AMOUNT_TREE_KEY,
};
use drive::drive::RootTree;
use drive::grovedb::{Element, Transaction};

impl<C> Platform<C> {
/// checks for a network upgrade and resets activation window
/// this should only be called on epoch change
pub(super) fn perform_events_on_first_block_of_protocol_change_v0(
&self,
_epoch_info: &EpochInfo,
transaction: &Transaction,
previous_protocol_version: ProtocolVersion,
platform_version: &PlatformVersion,
) -> Result<(), Error> {
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(())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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.

  1. 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 the grove_insert_if_not_exists call in a match statement or using .map_err() to add context to any errors.

  2. Version Check: The condition previous_protocol_version < 4 && platform_version.protocol_version >= 4 assumes a specific upgrade path. Verify if this is the intended logic, especially considering future protocol versions.

  3. 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

}
}
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ use dashcore_rpc::Error as CoreRPCError;
use dpp::dashcore::bls_sig_utils::BLSSignature;
use dpp::dashcore::transaction::special_transaction::TransactionPayload::AssetUnlockPayloadType;
use dpp::dashcore::{consensus, Transaction, Txid};
use std::collections::{BTreeMap, HashMap};
use std::collections::BTreeMap;

use std::fs::{self, File};
use std::io::Write;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,12 @@ use crate::platform_types::platform::Platform;
use crate::rpc::core::CoreRPCLike;
use dpp::block::block_info::BlockInfo;
use dpp::document::Document;
use dpp::fee::Credits;
use dpp::version::PlatformVersion;
use dpp::withdrawal::{WithdrawalTransactionIndex, WithdrawalTransactionIndexAndBytes};
use dpp::withdrawal::{
WithdrawalTransactionIndex, WithdrawalTransactionIndexAndBytes,
WithdrawalTransactionIndexAndBytesAndAmounts,
};

mod v0;

Expand Down Expand Up @@ -35,7 +39,7 @@ where
start_index: WithdrawalTransactionIndex,
block_info: &BlockInfo,
platform_version: &PlatformVersion,
) -> Result<Vec<WithdrawalTransactionIndexAndBytes>, Error> {
) -> Result<(Vec<WithdrawalTransactionIndexAndBytes>, Credits), Error> {
match platform_version
.drive_abci
.methods
Expand Down
Loading
Loading