Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(sdk): added transfer transition to rs-sdk #2289

Merged
merged 4 commits into from
Oct 29, 2024
Merged
Show file tree
Hide file tree
Changes from all 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
@@ -1,7 +1,57 @@
mod v0;

pub use v0::*;

use crate::state_transition::identity_credit_transfer_transition::IdentityCreditTransferTransition;
#[cfg(feature = "state-transition-signing")]
use crate::{
identity::{signer::Signer, Identity, IdentityPublicKey},
prelude::{IdentityNonce, UserFeeIncrease},
state_transition::{
identity_credit_transfer_transition::v0::IdentityCreditTransferTransitionV0,
StateTransition,
},
ProtocolError,
};
#[cfg(feature = "state-transition-signing")]
use platform_value::Identifier;
#[cfg(feature = "state-transition-signing")]
use platform_version::version::{FeatureVersion, PlatformVersion};

impl IdentityCreditTransferTransitionMethodsV0 for IdentityCreditTransferTransition {}
impl IdentityCreditTransferTransitionMethodsV0 for IdentityCreditTransferTransition {
#[cfg(feature = "state-transition-signing")]
fn try_from_identity<S: Signer>(
identity: &Identity,
to_identity_with_identifier: Identifier,
amount: u64,
user_fee_increase: UserFeeIncrease,
signer: S,
signing_withdrawal_key_to_use: Option<&IdentityPublicKey>,
nonce: IdentityNonce,
platform_version: &PlatformVersion,
version: Option<FeatureVersion>,
) -> Result<StateTransition, ProtocolError> {
match version.unwrap_or(
platform_version
.dpp
.state_transition_conversion_versions
.identity_to_identity_transfer_transition,
) {
0 => Ok(IdentityCreditTransferTransitionV0::try_from_identity(
identity,
to_identity_with_identifier,
amount,
user_fee_increase,
signer,
signing_withdrawal_key_to_use,
nonce,
platform_version,
version,
)?),
version => Err(ProtocolError::UnknownVersionMismatch {
method: "IdentityCreditTransferTransition::try_from_identity".to_string(),
known_versions: vec![0],
received: version,
}),
Comment on lines +33 to +54
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

Fix variable shadowing of version to prevent type confusion

The variable version is being shadowed within the match expression, changing it from Option<FeatureVersion> to FeatureVersion (u16). This can lead to confusion and potential bugs, especially when passing version to try_from_identity, which expects an Option<FeatureVersion>. To avoid this, consider renaming the unwrapped version to version_value and updating the error handling accordingly.

Apply this diff to fix the issue:

33-        match version.unwrap_or(
34-            platform_version
35-                .dpp
36-                .state_transition_conversion_versions
37-                .identity_to_identity_transfer_transition,
38-        ) {
39-            0 => Ok(IdentityCreditTransferTransitionV0::try_from_identity(
40-                identity,
41-                to_identity_with_identifier,
42-                amount,
43-                user_fee_increase,
44-                signer,
45-                signing_withdrawal_key_to_use,
46-                nonce,
47-                platform_version,
48-                version,
49-            )?),
50-            version => Err(ProtocolError::UnknownVersionMismatch {
51-                method: "IdentityCreditTransferTransition::try_from_identity".to_string(),
52-                known_versions: vec![0],
53-                received: version,
54-            }),
55-        }
+        let version_value = version.unwrap_or(
+            platform_version
+                .dpp
+                .state_transition_conversion_versions
+                .identity_to_identity_transfer_transition,
+        );
+        match version_value {
+            0 => Ok(IdentityCreditTransferTransitionV0::try_from_identity(
+                identity,
+                to_identity_with_identifier,
+                amount,
+                user_fee_increase,
+                signer,
+                signing_withdrawal_key_to_use,
+                nonce,
+                platform_version,
+                version,
+            )?),
+            unknown_version => Err(ProtocolError::UnknownVersionMismatch {
+                method: "IdentityCreditTransferTransition::try_from_identity".to_string(),
+                known_versions: vec![0],
+                received: unknown_version,
+            }),
+        }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
match version.unwrap_or(
platform_version
.dpp
.state_transition_conversion_versions
.identity_to_identity_transfer_transition,
) {
0 => Ok(IdentityCreditTransferTransitionV0::try_from_identity(
identity,
to_identity_with_identifier,
amount,
user_fee_increase,
signer,
signing_withdrawal_key_to_use,
nonce,
platform_version,
version,
)?),
version => Err(ProtocolError::UnknownVersionMismatch {
method: "IdentityCreditTransferTransition::try_from_identity".to_string(),
known_versions: vec![0],
received: version,
}),
let version_value = version.unwrap_or(
platform_version
.dpp
.state_transition_conversion_versions
.identity_to_identity_transfer_transition,
);
match version_value {
0 => Ok(IdentityCreditTransferTransitionV0::try_from_identity(
identity,
to_identity_with_identifier,
amount,
user_fee_increase,
signer,
signing_withdrawal_key_to_use,
nonce,
platform_version,
version,
)?),
unknown_version => Err(ProtocolError::UnknownVersionMismatch {
method: "IdentityCreditTransferTransition::try_from_identity".to_string(),
known_versions: vec![0],
received: unknown_version,
}),
}

}
}
}
Original file line number Diff line number Diff line change
@@ -1,6 +1,30 @@
#[cfg(feature = "state-transition-signing")]
use crate::{
identity::{signer::Signer, Identity, IdentityPublicKey},
prelude::{IdentityNonce, UserFeeIncrease},
state_transition::StateTransition,
ProtocolError,
};
use platform_value::Identifier;

Check warning on line 8 in packages/rs-dpp/src/state_transition/state_transitions/identity/identity_credit_transfer_transition/methods/v0/mod.rs

View workflow job for this annotation

GitHub Actions / Rust packages (drive) / Linting

unused import: `platform_value::Identifier`

warning: unused import: `platform_value::Identifier` --> packages/rs-dpp/src/state_transition/state_transitions/identity/identity_credit_transfer_transition/methods/v0/mod.rs:8:5 | 8 | use platform_value::Identifier; | ^^^^^^^^^^^^^^^^^^^^^^^^^^ | = note: `#[warn(unused_imports)]` on by default

Check warning on line 8 in packages/rs-dpp/src/state_transition/state_transitions/identity/identity_credit_transfer_transition/methods/v0/mod.rs

View workflow job for this annotation

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

unused import: `platform_value::Identifier`

warning: unused import: `platform_value::Identifier` --> packages/rs-dpp/src/state_transition/state_transitions/identity/identity_credit_transfer_transition/methods/v0/mod.rs:8:5 | 8 | use platform_value::Identifier; | ^^^^^^^^^^^^^^^^^^^^^^^^^^ | = note: `#[warn(unused_imports)]` on by default
#[cfg(feature = "state-transition-signing")]
use platform_version::version::{FeatureVersion, PlatformVersion};

use crate::state_transition::StateTransitionType;

pub trait IdentityCreditTransferTransitionMethodsV0 {
#[cfg(feature = "state-transition-signing")]
fn try_from_identity<S: Signer>(
identity: &Identity,
to_identity_with_identifier: Identifier,
amount: u64,
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

Based on the searches performed, we haven't found any existing validation for the amount in the credit transfer transition. Since this involves financial operations (credit transfers between identities), proper validation is crucial for security and system integrity. Let me generate the final response:

Amount validation is required for secure credit transfers

The current implementation lacks essential validations for the credit transfer amount:

  • No minimum amount validation to prevent dust transfers
  • No maximum limit checks to prevent excessive transfers
  • No balance verification against the source identity's available credits

These validations are crucial for maintaining system integrity and preventing potential abuse of the credit transfer mechanism.

🔗 Analysis chain

Consider adding amount validation.

The amount parameter is defined as u64 without apparent validation. Consider whether there should be:

  • Minimum transfer amount validation
  • Maximum transfer limit checks
  • Balance verification against the source identity

Let's check if there are any existing amount validations:

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for amount validation patterns in the codebase
rg -A 5 "amount.*validate|validate.*amount" 

Length of output: 45


Script:

#!/bin/bash
# Let's search for any validation related to amount in the identity credit transfer
rg -A 10 "amount" packages/rs-dpp/src/state_transition/state_transitions/identity/identity_credit_transfer_transition/

# Also check for any validation traits or implementations
ast-grep --pattern 'impl Validate for IdentityCreditTransferTransition'

Length of output: 16013


Script:

#!/bin/bash
# Let's search for validation implementations in the state transition module
rg -A 10 "impl.*Validate.*IdentityCreditTransfer" 

# Also check for any balance or credit validation patterns
rg -A 10 "validate.*balance|balance.*check|credit.*check" packages/rs-dpp/src/state_transition/

Length of output: 149


Script:

#!/bin/bash
# Let's check for validation modules and state validation
rg -A 10 "validate" packages/rs-dpp/src/state_transition/state_transitions/identity/identity_credit_transfer_transition/

# Also search for any validation traits or state validation modules
fd validation packages/rs-dpp/src/state_transition/ --exec rg -l "IdentityCreditTransfer"

# Check for any balance or credit related validation in validation modules
fd validation packages/rs-dpp/src/state_transition/ --exec rg -A 5 "amount|balance|credit" {}

Length of output: 1354

user_fee_increase: UserFeeIncrease,
signer: S,
signing_withdrawal_key_to_use: Option<&IdentityPublicKey>,
nonce: IdentityNonce,
platform_version: &PlatformVersion,
version: Option<FeatureVersion>,
) -> Result<StateTransition, ProtocolError>;

/// Get State Transition Type
fn get_type() -> StateTransitionType {
StateTransitionType::IdentityCreditTransfer
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,80 @@
#[cfg(feature = "state-transition-signing")]
use crate::{
identity::{
accessors::IdentityGettersV0, signer::Signer, Identity, IdentityPublicKey, KeyType,
Purpose, SecurityLevel,
},
prelude::{IdentityNonce, UserFeeIncrease},
state_transition::StateTransition,
ProtocolError,
};
use platform_value::Identifier;

Check warning on line 11 in packages/rs-dpp/src/state_transition/state_transitions/identity/identity_credit_transfer_transition/v0/v0_methods.rs

View workflow job for this annotation

GitHub Actions / Rust packages (drive) / Linting

unused import: `platform_value::Identifier`

warning: unused import: `platform_value::Identifier` --> packages/rs-dpp/src/state_transition/state_transitions/identity/identity_credit_transfer_transition/v0/v0_methods.rs:11:5 | 11 | use platform_value::Identifier; | ^^^^^^^^^^^^^^^^^^^^^^^^^^

Check warning on line 11 in packages/rs-dpp/src/state_transition/state_transitions/identity/identity_credit_transfer_transition/v0/v0_methods.rs

View workflow job for this annotation

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

unused import: `platform_value::Identifier`

warning: unused import: `platform_value::Identifier` --> packages/rs-dpp/src/state_transition/state_transitions/identity/identity_credit_transfer_transition/v0/v0_methods.rs:11:5 | 11 | use platform_value::Identifier; | ^^^^^^^^^^^^^^^^^^^^^^^^^^

use crate::state_transition::identity_credit_transfer_transition::methods::IdentityCreditTransferTransitionMethodsV0;
use crate::state_transition::identity_credit_transfer_transition::v0::IdentityCreditTransferTransitionV0;
use crate::state_transition::GetDataContractSecurityLevelRequirementFn;

Check warning on line 15 in packages/rs-dpp/src/state_transition/state_transitions/identity/identity_credit_transfer_transition/v0/v0_methods.rs

View workflow job for this annotation

GitHub Actions / Rust packages (drive) / Linting

unused import: `crate::state_transition::GetDataContractSecurityLevelRequirementFn`

warning: unused import: `crate::state_transition::GetDataContractSecurityLevelRequirementFn` --> packages/rs-dpp/src/state_transition/state_transitions/identity/identity_credit_transfer_transition/v0/v0_methods.rs:15:5 | 15 | use crate::state_transition::GetDataContractSecurityLevelRequirementFn; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

Check warning on line 15 in packages/rs-dpp/src/state_transition/state_transitions/identity/identity_credit_transfer_transition/v0/v0_methods.rs

View workflow job for this annotation

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

unused import: `crate::state_transition::GetDataContractSecurityLevelRequirementFn`

warning: unused import: `crate::state_transition::GetDataContractSecurityLevelRequirementFn` --> packages/rs-dpp/src/state_transition/state_transitions/identity/identity_credit_transfer_transition/v0/v0_methods.rs:15:5 | 15 | use crate::state_transition::GetDataContractSecurityLevelRequirementFn; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
#[cfg(feature = "state-transition-signing")]
use platform_version::version::{FeatureVersion, PlatformVersion};

impl IdentityCreditTransferTransitionMethodsV0 for IdentityCreditTransferTransitionV0 {
#[cfg(feature = "state-transition-signing")]
fn try_from_identity<S: Signer>(
identity: &Identity,
to_identity_with_identifier: Identifier,
amount: u64,
user_fee_increase: UserFeeIncrease,
signer: S,
signing_withdrawal_key_to_use: Option<&IdentityPublicKey>,
nonce: IdentityNonce,
_platform_version: &PlatformVersion,
_version: Option<FeatureVersion>,
) -> Result<StateTransition, ProtocolError> {
let mut transition: StateTransition = IdentityCreditTransferTransitionV0 {
identity_id: identity.id(),
recipient_id: to_identity_with_identifier,
amount,
nonce,
user_fee_increase,
signature_public_key_id: 0,
signature: Default::default(),
}
.into();

let identity_public_key = match signing_withdrawal_key_to_use {
Some(key) => {
if signer.can_sign_with(key) {
key
} else {
return Err(
ProtocolError::DesiredKeyWithTypePurposeSecurityLevelMissing(
"specified transfer public key cannot be used for signing".to_string(),
),
);
}
}
None => {
let key = identity
.get_first_public_key_matching(
Purpose::TRANSFER,
SecurityLevel::full_range().into(),
KeyType::all_key_types().into(),
true,
)
.ok_or_else(|| {
ProtocolError::DesiredKeyWithTypePurposeSecurityLevelMissing(
"no transfer public key".to_string(),
)
})?;
key
}
};

transition.sign_external(
identity_public_key,
&signer,
None::<GetDataContractSecurityLevelRequirementFn>,
)?;

impl IdentityCreditTransferTransitionMethodsV0 for IdentityCreditTransferTransitionV0 {}
Ok(transition)
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ pub mod v2;
pub struct DPPStateTransitionConversionVersions {
pub identity_to_identity_create_transition: FeatureVersion,
pub identity_to_identity_top_up_transition: FeatureVersion,
pub identity_to_identity_transfer_transition: FeatureVersion,
pub identity_to_identity_withdrawal_transition: FeatureVersion,
pub identity_to_identity_create_transition_with_signer: FeatureVersion,
}
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ pub const STATE_TRANSITION_CONVERSION_VERSIONS_V1: DPPStateTransitionConversionV
DPPStateTransitionConversionVersions {
identity_to_identity_create_transition: 0,
identity_to_identity_top_up_transition: 0,
identity_to_identity_transfer_transition: 0,
identity_to_identity_withdrawal_transition: 0,
identity_to_identity_create_transition_with_signer: 0,
};
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ pub const STATE_TRANSITION_CONVERSION_VERSIONS_V2: DPPStateTransitionConversionV
DPPStateTransitionConversionVersions {
identity_to_identity_create_transition: 0,
identity_to_identity_top_up_transition: 0,
identity_to_identity_transfer_transition: 0,
identity_to_identity_withdrawal_transition: 1,
identity_to_identity_create_transition_with_signer: 0,
};
1 change: 1 addition & 0 deletions packages/rs-sdk/src/platform/transition.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ pub mod put_document;
pub mod put_identity;
pub mod put_settings;
pub mod top_up_identity;
pub mod transfer;
pub mod transfer_document;
mod txid;
pub mod update_price_of_document;
Expand Down
67 changes: 67 additions & 0 deletions packages/rs-sdk/src/platform/transition/transfer.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,67 @@
use dpp::identifier::Identifier;
use dpp::identity::accessors::IdentityGettersV0;

use crate::platform::transition::broadcast::BroadcastStateTransition;
use crate::platform::transition::put_settings::PutSettings;
use crate::{Error, Sdk};
use dpp::identity::signer::Signer;
use dpp::identity::{Identity, IdentityPublicKey};
use dpp::state_transition::identity_credit_transfer_transition::methods::IdentityCreditTransferTransitionMethodsV0;
use dpp::state_transition::identity_credit_transfer_transition::IdentityCreditTransferTransition;
use dpp::state_transition::proof_result::StateTransitionProofResult;

#[async_trait::async_trait]
pub trait TransferToIdentity {
/// Function to transfer credits from an identity to another identity. Returns the final
/// identity balance.
///
/// If signing_transfer_key_to_use is not set, we will try to use one in the signer that is
/// available for the transfer.
async fn transfer_credits<S: Signer + Send>(
&self,
sdk: &Sdk,
to_identity_id: Identifier,
amount: u64,
signing_transfer_key_to_use: Option<&IdentityPublicKey>,
signer: S,
settings: Option<PutSettings>,
) -> Result<u64, Error>;
}

#[async_trait::async_trait]
impl TransferToIdentity for Identity {
async fn transfer_credits<S: Signer + Send>(
&self,
sdk: &Sdk,
to_identity_id: Identifier,
amount: u64,
signing_transfer_key_to_use: Option<&IdentityPublicKey>,
signer: S,
settings: Option<PutSettings>,
) -> Result<u64, Error> {
let new_identity_nonce = sdk.get_identity_nonce(self.id(), true, settings).await?;
let user_fee_increase = settings.and_then(|settings| settings.user_fee_increase);
let state_transition = IdentityCreditTransferTransition::try_from_identity(
self,
to_identity_id,
amount,
user_fee_increase.unwrap_or_default(),
signer,
Comment on lines +43 to +49
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

Handle user_fee_increase more explicitly

Currently, user_fee_increase is extracted using and_then and defaults to zero if None. Consider handling the absence of user_fee_increase more explicitly to improve code readability.

Refactor the code as follows:

- let user_fee_increase = settings.and_then(|settings| settings.user_fee_increase);
- let state_transition = IdentityCreditTransferTransition::try_from_identity(
-     self,
-     to_identity_id,
-     amount,
-     user_fee_increase.unwrap_or_default(),
+ let user_fee_increase = settings
+     .and_then(|s| s.user_fee_increase)
+     .unwrap_or_default();
+ let state_transition = IdentityCreditTransferTransition::try_from_identity(
+     self,
+     to_identity_id,
+     amount,
+     user_fee_increase,
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
let user_fee_increase = settings.and_then(|settings| settings.user_fee_increase);
let state_transition = IdentityCreditTransferTransition::try_from_identity(
self,
to_identity_id,
amount,
user_fee_increase.unwrap_or_default(),
signer,
let user_fee_increase = settings
.and_then(|s| s.user_fee_increase)
.unwrap_or_default();
let state_transition = IdentityCreditTransferTransition::try_from_identity(
self,
to_identity_id,
amount,
user_fee_increase,
signer,

signing_transfer_key_to_use,
new_identity_nonce,
sdk.version(),
None,
)?;

let result = state_transition.broadcast_and_wait(sdk, None).await?;

match result {
StateTransitionProofResult::VerifiedPartialIdentity(identity) => {
identity.balance.ok_or(Error::DapiClientError(
"expected an identity balance after transfer".to_string(),
))
}
_ => Err(Error::DapiClientError("proved a non identity".to_string())),
}
Comment on lines +58 to +65
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

Improve error messages for clarity

The error messages could be made more descriptive to aid in debugging.

Apply this diff to enhance the error messages:

                 identity.balance.ok_or(Error::DapiClientError(
-                    "expected an identity balance after transfer".to_string(),
+                    "Expected an identity balance after transfer but found None.".to_string(),
                 ))
             }
-            _ => Err(Error::DapiClientError("proved a non identity".to_string())),
+            _ => Err(Error::DapiClientError("State transition proof result is not a VerifiedPartialIdentity.".to_string())),
         }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
match result {
StateTransitionProofResult::VerifiedPartialIdentity(identity) => {
identity.balance.ok_or(Error::DapiClientError(
"expected an identity balance after transfer".to_string(),
))
}
_ => Err(Error::DapiClientError("proved a non identity".to_string())),
}
match result {
StateTransitionProofResult::VerifiedPartialIdentity(identity) => {
identity.balance.ok_or(Error::DapiClientError(
"Expected an identity balance after transfer but found None.".to_string(),
))
}
_ => Err(Error::DapiClientError("State transition proof result is not a VerifiedPartialIdentity.".to_string())),
}

}
}
Loading