From 178dfa4b14f3d73f78e6ba1aafa81e3010ef0ca8 Mon Sep 17 00:00:00 2001 From: Ivan Shumkov Date: Tue, 1 Oct 2024 16:37:40 +0300 Subject: [PATCH 1/4] chore(drive): remove duplicated withdrawal amount validation --- ...thdrawal_transition_output_script_error.rs | 1 + .../balance/v0/mod.rs | 103 +++++++- .../state/v0/mod.rs | 25 +- .../structure/v1/mod.rs | 229 +++++++++++++++++- 4 files changed, 328 insertions(+), 30 deletions(-) diff --git a/packages/rs-dpp/src/errors/consensus/basic/identity/invalid_identity_credit_withdrawal_transition_output_script_error.rs b/packages/rs-dpp/src/errors/consensus/basic/identity/invalid_identity_credit_withdrawal_transition_output_script_error.rs index 337cc1908c..9af57eb8be 100644 --- a/packages/rs-dpp/src/errors/consensus/basic/identity/invalid_identity_credit_withdrawal_transition_output_script_error.rs +++ b/packages/rs-dpp/src/errors/consensus/basic/identity/invalid_identity_credit_withdrawal_transition_output_script_error.rs @@ -29,6 +29,7 @@ impl InvalidIdentityCreditWithdrawalTransitionOutputScriptError { } pub fn output_script(&self) -> CoreScript { + // TODO: We shouldn't clone in getter 🤦 self.output_script.clone() } } diff --git a/packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/identity_credit_withdrawal/balance/v0/mod.rs b/packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/identity_credit_withdrawal/balance/v0/mod.rs index 9dc60ba353..0843ed7ec1 100644 --- a/packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/identity_credit_withdrawal/balance/v0/mod.rs +++ b/packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/identity_credit_withdrawal/balance/v0/mod.rs @@ -5,10 +5,6 @@ use dpp::state_transition::identity_credit_withdrawal_transition::accessors::Ide use dpp::state_transition::identity_credit_withdrawal_transition::IdentityCreditWithdrawalTransition; use crate::error::execution::ExecutionError; -use crate::execution::types::execution_operation::ValidationOperation; -use crate::execution::types::state_transition_execution_context::{ - StateTransitionExecutionContext, StateTransitionExecutionContextMethodsV0, -}; use dpp::validation::SimpleConsensusValidationResult; use dpp::version::PlatformVersion; @@ -34,7 +30,11 @@ impl IdentityCreditTransferTransitionBalanceValidationV0 for IdentityCreditWithd "expected to have a balance on identity for credit withdrawal transition", )))?; - if 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")))? { + let amount_and_fees = self.amount() + .checked_add(platform_version.fee_version.state_transition_min_fees.credit_withdrawal) + .ok_or_else(|| Error::Execution(ExecutionError::Overflow("overflow when adding amount and min_leftover_credits_before_processing in identity credit withdrawal")))?; + + if balance < amount_and_fees { return Ok(SimpleConsensusValidationResult::new_with_error( IdentityInsufficientBalanceError::new(self.identity_id(), balance, self.amount()) .into(), @@ -44,3 +44,96 @@ impl IdentityCreditTransferTransitionBalanceValidationV0 for IdentityCreditWithd Ok(SimpleConsensusValidationResult::new()) } } + +#[cfg(test)] +mod tests { + use super::*; + + use assert_matches::assert_matches; + use dpp::consensus::state::state_error::StateError; + use dpp::consensus::ConsensusError; + use dpp::prelude::Identifier; + use dpp::state_transition::identity_credit_withdrawal_transition::v0::IdentityCreditWithdrawalTransitionV0; + use platform_version::version::v1::PLATFORM_V1; + + mod validate_advanced_minimum_balance_pre_check_v0 { + use super::*; + + #[test] + fn should_return_invalid_result_if_balance_is_less_than_amount_and_fees() { + let balance = 100; + + let amount = 200; + + 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, + core_fee_per_byte: 0, + pooling: Default::default(), + output_script: Default::default(), + nonce: 0, + user_fee_increase: 0, + signature_public_key_id: 0, + signature: Default::default(), + }); + + let platform_version = &PLATFORM_V1; + + let result = transaction + .validate_advanced_minimum_balance_pre_check_v0(&identity, platform_version) + .expect("failed to validate minimum balance"); + + assert_matches!( + result.errors.as_slice(), + [ConsensusError::StateError( + StateError::IdentityInsufficientBalanceError(err) + )] if err.balance() == balance && err.required_balance() == amount && err.identity_id() == &identity.id + ); + } + + #[test] + fn should_return_valid_result() { + let balance = 200000000000; + + let amount = 100; + + 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: Default::default(), + amount, + core_fee_per_byte: 0, + pooling: Default::default(), + output_script: Default::default(), + nonce: 0, + user_fee_increase: 0, + signature_public_key_id: 0, + signature: Default::default(), + }); + + let platform_version = &PLATFORM_V1; + + let result = transaction + .validate_advanced_minimum_balance_pre_check_v0(&identity, platform_version) + .expect("failed to validate minimum balance"); + + assert_matches!(result.errors.as_slice(), []); + } + } +} diff --git a/packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/identity_credit_withdrawal/state/v0/mod.rs b/packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/identity_credit_withdrawal/state/v0/mod.rs index fefadaa8c7..39e2aea165 100644 --- a/packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/identity_credit_withdrawal/state/v0/mod.rs +++ b/packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/identity_credit_withdrawal/state/v0/mod.rs @@ -49,29 +49,6 @@ impl IdentityCreditWithdrawalStateTransitionStateValidationV0 tx: TransactionArg, platform_version: &PlatformVersion, ) -> Result, Error> { - let maybe_existing_identity_balance = platform.drive.fetch_identity_balance( - self.identity_id().to_buffer(), - tx, - platform_version, - )?; - - let Some(existing_identity_balance) = maybe_existing_identity_balance else { - return Ok(ConsensusValidationResult::new_with_error( - IdentityNotFoundError::new(self.identity_id()).into(), - )); - }; - - if existing_identity_balance < self.amount() { - return Ok(ConsensusValidationResult::new_with_error( - IdentityInsufficientBalanceError::new( - self.identity_id(), - existing_identity_balance, - self.amount(), - ) - .into(), - )); - } - self.transform_into_action_v0( platform, block_info, @@ -91,7 +68,7 @@ impl IdentityCreditWithdrawalStateTransitionStateValidationV0 ) -> Result, Error> { let consensus_validation_result = IdentityCreditWithdrawalTransitionAction::try_from_identity_credit_withdrawal( - &platform.drive, + platform.drive, tx, self, block_info, diff --git a/packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/identity_credit_withdrawal/structure/v1/mod.rs b/packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/identity_credit_withdrawal/structure/v1/mod.rs index 7f018630a8..258c98a191 100644 --- a/packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/identity_credit_withdrawal/structure/v1/mod.rs +++ b/packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/identity_credit_withdrawal/structure/v1/mod.rs @@ -42,7 +42,7 @@ impl IdentityCreditWithdrawalStateTransitionStructureValidationV1 )); } - // currently we do not support pooling, so we must validate that pooling is `Never` + // Now, we do not support pooling, so we must validate that pooling is `Never` if self.pooling() != Pooling::Never { result.add_error( @@ -78,3 +78,230 @@ impl IdentityCreditWithdrawalStateTransitionStructureValidationV1 Ok(result) } } + +#[cfg(test)] +mod tests { + use super::*; + + use assert_matches::assert_matches; + use dpp::consensus::basic::BasicError; + use dpp::dashcore::ScriptBuf; + use dpp::identity::core_script::CoreScript; + use dpp::state_transition::identity_credit_withdrawal_transition::v1::IdentityCreditWithdrawalTransitionV1; + use platform_version::version::v1::PLATFORM_V1; + + mod validate_basic_structure_v1 { + use super::*; + + #[test] + fn should_return_invalid_result_if_amount_too_low() { + let amount = 18000; + + let transition = + IdentityCreditWithdrawalTransition::V1(IdentityCreditWithdrawalTransitionV1 { + identity_id: Default::default(), + amount, + core_fee_per_byte: 1, + pooling: Default::default(), + output_script: None, + nonce: 0, + user_fee_increase: 0, + signature_public_key_id: 0, + signature: Default::default(), + }); + + let platform_version = &PLATFORM_V1; + + let result = transition + .validate_basic_structure_v1(platform_version) + .expect("failed to validate basic structure"); + + assert_matches!( + result.errors.as_slice(), + [ConsensusError::BasicError( + BasicError::InvalidIdentityCreditWithdrawalTransitionAmountError( + InvalidIdentityCreditWithdrawalTransitionAmountError { + amount: a, + min_amount: 190000, + max_amount: 50000000000000, + }, + ), + )] if *a == amount + ); + } + + #[test] + fn should_return_invalid_result_if_amount_too_high() { + let amount = 60000000000000; + + let transition = + IdentityCreditWithdrawalTransition::V1(IdentityCreditWithdrawalTransitionV1 { + identity_id: Default::default(), + amount, + core_fee_per_byte: 1, + pooling: Default::default(), + output_script: None, + nonce: 0, + user_fee_increase: 0, + signature_public_key_id: 0, + signature: Default::default(), + }); + + let platform_version = &PLATFORM_V1; + + let result = transition + .validate_basic_structure_v1(platform_version) + .expect("failed to validate basic structure"); + + assert_matches!( + result.errors.as_slice(), + [ConsensusError::BasicError( + BasicError::InvalidIdentityCreditWithdrawalTransitionAmountError( + InvalidIdentityCreditWithdrawalTransitionAmountError { + amount: a, + min_amount: 190000, + max_amount: 50000000000000, + }, + ), + )] if *a == amount + ); + } + + #[test] + fn should_return_invalid_result_if_pooling_not_never() { + let transition = + IdentityCreditWithdrawalTransition::V1(IdentityCreditWithdrawalTransitionV1 { + identity_id: Default::default(), + amount: 200000, + core_fee_per_byte: 1, + pooling: Pooling::Standard, + output_script: None, + nonce: 0, + user_fee_increase: 0, + signature_public_key_id: 0, + signature: Default::default(), + }); + + let platform_version = &PLATFORM_V1; + + let result = transition + .validate_basic_structure_v1(platform_version) + .expect("failed to validate basic structure"); + + assert_matches!( + result.errors.as_slice(), + [ConsensusError::BasicError( + BasicError::NotImplementedIdentityCreditWithdrawalTransitionPoolingError(err), + )] if err.pooling() == Pooling::Standard as u8 + ); + } + + #[test] + fn should_return_invalid_result_if_core_fee_not_fibonacci() { + let transition = + IdentityCreditWithdrawalTransition::V1(IdentityCreditWithdrawalTransitionV1 { + identity_id: Default::default(), + amount: 200000, + core_fee_per_byte: 0, + pooling: Pooling::Never, + output_script: None, + nonce: 0, + user_fee_increase: 0, + signature_public_key_id: 0, + signature: Default::default(), + }); + + let platform_version = &PLATFORM_V1; + + let result = transition + .validate_basic_structure_v1(platform_version) + .expect("failed to validate basic structure"); + + assert_matches!( + result.errors.as_slice(), + [ConsensusError::BasicError( + BasicError::InvalidIdentityCreditWithdrawalTransitionCoreFeeError(err) + )] if err.min_core_fee_per_byte() == 1 && err.core_fee_per_byte() == 0 + ); + } + + #[test] + fn should_return_invalid_result_if_output_script_is_not_p2pkh_or_p2sh() { + let output_script = CoreScript::new(ScriptBuf::new()); + + let transition = + IdentityCreditWithdrawalTransition::V1(IdentityCreditWithdrawalTransitionV1 { + identity_id: Default::default(), + amount: 200000, + core_fee_per_byte: 1, + pooling: Pooling::Never, + output_script: Some(output_script.clone()), + nonce: 0, + user_fee_increase: 0, + signature_public_key_id: 0, + signature: Default::default(), + }); + + let platform_version = &PLATFORM_V1; + + let result = transition + .validate_basic_structure_v1(platform_version) + .expect("failed to validate basic structure"); + + assert_matches!( + result.errors.as_slice(), + [ConsensusError::BasicError( + BasicError::InvalidIdentityCreditWithdrawalTransitionOutputScriptError(err) + )] if err.output_script() == output_script + ); + } + + #[test] + fn should_return_valid_result_if_output_script_is_p2pkh() { + let transition = + IdentityCreditWithdrawalTransition::V1(IdentityCreditWithdrawalTransitionV1 { + identity_id: Default::default(), + amount: 200000, + core_fee_per_byte: 1, + pooling: Pooling::Never, + output_script: None, + nonce: 0, + user_fee_increase: 0, + signature_public_key_id: 0, + signature: Default::default(), + }); + + let platform_version = &PLATFORM_V1; + + let result = transition + .validate_basic_structure_v1(platform_version) + .expect("failed to validate basic structure"); + + assert!(result.is_valid()); + } + + #[test] + fn should_return_valid_result_if_output_script_is_p2sh() { + let transition = + IdentityCreditWithdrawalTransition::V1(IdentityCreditWithdrawalTransitionV1 { + identity_id: Default::default(), + amount: 200000, + core_fee_per_byte: 1, + pooling: Pooling::Never, + output_script: None, + nonce: 0, + user_fee_increase: 0, + signature_public_key_id: 0, + signature: Default::default(), + }); + + let platform_version = &PLATFORM_V1; + + let result = transition + .validate_basic_structure_v1(platform_version) + .expect("failed to validate basic structure"); + + assert!(result.is_valid()); + } + } +} From 31d4b134b859cb00250bae872d75fcf3f36dad31 Mon Sep 17 00:00:00 2001 From: Ivan Shumkov Date: Tue, 1 Oct 2024 17:11:27 +0300 Subject: [PATCH 2/4] refactor: remove unused imports --- .../identity_credit_withdrawal/state/v0/mod.rs | 3 --- 1 file changed, 3 deletions(-) diff --git a/packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/identity_credit_withdrawal/state/v0/mod.rs b/packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/identity_credit_withdrawal/state/v0/mod.rs index 39e2aea165..87eff3dbaa 100644 --- a/packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/identity_credit_withdrawal/state/v0/mod.rs +++ b/packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/identity_credit_withdrawal/state/v0/mod.rs @@ -3,10 +3,7 @@ use crate::platform_types::platform::PlatformRef; use crate::rpc::core::CoreRPCLike; use dpp::block::block_info::BlockInfo; -use dpp::consensus::signature::IdentityNotFoundError; -use dpp::consensus::state::identity::IdentityInsufficientBalanceError; use dpp::prelude::ConsensusValidationResult; -use dpp::state_transition::identity_credit_withdrawal_transition::accessors::IdentityCreditWithdrawalTransitionAccessorsV0; use dpp::state_transition::identity_credit_withdrawal_transition::IdentityCreditWithdrawalTransition; use crate::execution::types::execution_operation::ValidationOperation; From 5ed5ea917337552e5200c3f1ebb4798d213aea47 Mon Sep 17 00:00:00 2001 From: Ivan Shumkov Date: Tue, 1 Oct 2024 17:22:17 +0300 Subject: [PATCH 3/4] test: verify p2sh and p2pkh outputs --- .../structure/v1/mod.rs | 32 ++++++++++++++++++- 1 file changed, 31 insertions(+), 1 deletion(-) diff --git a/packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/identity_credit_withdrawal/structure/v1/mod.rs b/packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/identity_credit_withdrawal/structure/v1/mod.rs index 258c98a191..afaabcbe88 100644 --- a/packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/identity_credit_withdrawal/structure/v1/mod.rs +++ b/packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/identity_credit_withdrawal/structure/v1/mod.rs @@ -89,9 +89,11 @@ mod tests { use dpp::identity::core_script::CoreScript; use dpp::state_transition::identity_credit_withdrawal_transition::v1::IdentityCreditWithdrawalTransitionV1; use platform_version::version::v1::PLATFORM_V1; + use rand::SeedableRng; mod validate_basic_structure_v1 { use super::*; + use rand::prelude::StdRng; #[test] fn should_return_invalid_result_if_amount_too_low() { @@ -258,13 +260,15 @@ mod tests { #[test] fn should_return_valid_result_if_output_script_is_p2pkh() { + let rng = &mut StdRng::from_entropy(); + let transition = IdentityCreditWithdrawalTransition::V1(IdentityCreditWithdrawalTransitionV1 { identity_id: Default::default(), amount: 200000, core_fee_per_byte: 1, pooling: Pooling::Never, - output_script: None, + output_script: Some(CoreScript::random_p2pkh(rng)), nonce: 0, user_fee_increase: 0, signature_public_key_id: 0, @@ -282,6 +286,32 @@ mod tests { #[test] fn should_return_valid_result_if_output_script_is_p2sh() { + let rng = &mut StdRng::from_entropy(); + + let transition = + IdentityCreditWithdrawalTransition::V1(IdentityCreditWithdrawalTransitionV1 { + identity_id: Default::default(), + amount: 200000, + core_fee_per_byte: 1, + pooling: Pooling::Never, + output_script: Some(CoreScript::random_p2sh(rng)), + nonce: 0, + user_fee_increase: 0, + signature_public_key_id: 0, + signature: Default::default(), + }); + + let platform_version = &PLATFORM_V1; + + let result = transition + .validate_basic_structure_v1(platform_version) + .expect("failed to validate basic structure"); + + assert!(result.is_valid()); + } + + #[test] + fn should_return_valid_result_without_output_script() { let transition = IdentityCreditWithdrawalTransition::V1(IdentityCreditWithdrawalTransitionV1 { identity_id: Default::default(), From 3624b46d22a423c73458411d604d77b95d7a8c1b Mon Sep 17 00:00:00 2001 From: Ivan Shumkov Date: Wed, 2 Oct 2024 13:15:09 +0300 Subject: [PATCH 4/4] revert: grammar fix --- .../identity_credit_withdrawal/structure/v1/mod.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/identity_credit_withdrawal/structure/v1/mod.rs b/packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/identity_credit_withdrawal/structure/v1/mod.rs index afaabcbe88..f3898d0b0d 100644 --- a/packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/identity_credit_withdrawal/structure/v1/mod.rs +++ b/packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/identity_credit_withdrawal/structure/v1/mod.rs @@ -42,7 +42,7 @@ impl IdentityCreditWithdrawalStateTransitionStructureValidationV1 )); } - // Now, we do not support pooling, so we must validate that pooling is `Never` + // currently we do not support pooling, so we must validate that pooling is `Never` if self.pooling() != Pooling::Never { result.add_error(