-
Notifications
You must be signed in to change notification settings - Fork 39
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
Changes from 2 commits
3bb6712
fe027f8
656e39c
1b42803
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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, | ||
}), | ||
} | ||
} | ||
} |
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 GitHub Actions / Rust packages (drive) / Lintingunused import: `platform_value::Identifier`
|
||
#[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, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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:
These validations are crucial for maintaining system integrity and preventing potential abuse of the credit transfer mechanism. 🔗 Analysis chainConsider adding amount validation. The
Let's check if there are any existing amount validations: 🏁 Scripts executedThe 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 | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,81 @@ | ||
#[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 GitHub Actions / Rust packages (drive) / Lintingunused import: `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 GitHub Actions / Rust packages (drive) / Lintingunused import: `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 withdrawal public key cannot be used for signing" | ||
.to_string(), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Enhance error messages for improved clarity The current error messages:
are generic and may not provide sufficient context during debugging. Consider including additional details such as the key ID, expected key purposes, or security levels to help developers quickly identify the issue. Also applies to: 66-67 |
||
), | ||
); | ||
} | ||
} | ||
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 | ||||
---|---|---|---|---|---|---|
|
@@ -9,6 +9,7 @@ pub mod put_document; | |||||
pub mod put_identity; | ||||||
pub mod put_settings; | ||||||
pub mod top_up_identity; | ||||||
mod transfer; | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Consider making the transfer module public. The module should be declared as
Apply this diff to fix the visibility: -mod transfer;
+pub mod transfer; 📝 Committable suggestion
Suggested change
|
||||||
pub mod transfer_document; | ||||||
mod txid; | ||||||
pub mod update_price_of_document; | ||||||
|
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_withdrawal_key_to_use is not set, we will try to use one in the signer that is | ||||||||||||||||||||||||||||||||||
/// available for the transfer | ||||||||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Inconsistent naming in the documentation comment The parameter is named Apply this diff to fix the inconsistency: /// If signing_transfer_key_to_use is not set, we will try to use one in the signer that is
-/// available for the transfer
+/// 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
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Handle Currently, 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
Suggested change
|
||||||||||||||||||||||||||||||||||
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
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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
Suggested change
|
||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix variable shadowing of
version
to prevent type confusionThe variable
version
is being shadowed within thematch
expression, changing it fromOption<FeatureVersion>
toFeatureVersion
(u16
). This can lead to confusion and potential bugs, especially when passingversion
totry_from_identity
, which expects anOption<FeatureVersion>
. To avoid this, consider renaming the unwrapped version toversion_value
and updating the error handling accordingly.Apply this diff to fix the issue:
📝 Committable suggestion