feat(dpp): add client-side validation to remaining ST construction methods#3138
feat(dpp): add client-side validation to remaining ST construction methods#3138thepastaclaw wants to merge 6 commits intodashpay:v3.1-devfrom
Conversation
📝 WalkthroughWalkthroughAdded validation to two identity state transition constructors: credit-transfer now rejects self-transfers and transfers below 100,000 using new consensus error types; top-up now validates the asset lock proof against the platform version. Both functions rename the platform_version parameter from unused to active. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/rs-dpp/src/state_transition/state_transitions/identity/identity_topup_transition/v0/v0_methods.rs (2)
39-43: Prefer anif letpattern to eliminate the.unwrap().The
.unwrap()on line 41 is logically safe today because!validation_result.is_valid()implies a non-emptyerrorsvec — but it couples correctness toValidationResult::is_valid()'s semantics never diverging from a plainerrors.is_empty()check. Rewriting asif letremoves that fragility with zero overhead:♻️ Proposed refactor to eliminate `.unwrap()`
- if !validation_result.is_valid() { - let first_error = validation_result.errors.into_iter().next().unwrap(); - return Err(ProtocolError::ConsensusError(Box::new(first_error))); - } + if let Some(first_error) = validation_result.errors.into_iter().next() { + return Err(ProtocolError::ConsensusError(Box::new(first_error))); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-dpp/src/state_transition/state_transitions/identity/identity_topup_transition/v0/v0_methods.rs` around lines 39 - 43, Change the unwrap on validation_result.errors.into_iter().next() to an if-let to avoid panicking: after calling asset_lock_proof.validate_structure(platform_version) and observing !validation_result.is_valid(), match the first error with if let Some(first_error) = validation_result.errors.into_iter().next() { return Err(ProtocolError::ConsensusError(Box::new(first_error))); } else { unreachable!("validation_result.is_valid() == false must mean there is at least one error") }; this replaces the .unwrap() and references asset_lock_proof.validate_structure, validation_result, and ProtocolError::ConsensusError.
46-46:let _ = platform_version;is a valid but noisy workaround.This silences the "unused variable" warning when
#[cfg(feature = "validation")]is absent. It's correct, but an alternative that makes the intent more explicit without a spurious statement is to use a#[cfg_attr]allow on the parameter itself.♻️ Alternative: suppress at the parameter level
+ #[cfg_attr(not(feature = "validation"), allow(unused_variables))] fn try_from_identity( identity: &Identity, asset_lock_proof: AssetLockProof, asset_lock_proof_private_key: &[u8], user_fee_increase: UserFeeIncrease, platform_version: &PlatformVersion, _version: Option<FeatureVersion>, ) -> Result<StateTransition, ProtocolError> { #[cfg(feature = "validation")] { ... } - let _ = platform_version;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-dpp/src/state_transition/state_transitions/identity/identity_topup_transition/v0/v0_methods.rs` at line 46, Remove the noisy placeholder "let _ = platform_version;" and instead annotate the platform_version parameter so the compiler only ignores it when the validation feature is disabled; e.g., on the function in v0_methods.rs that receives platform_version, add #[cfg_attr(not(feature = "validation"), allow(unused_variables))] to the platform_version parameter declaration (or use allow(unused_variables) via cfg_attr on the parameter) and delete the let _ = ... statement.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@packages/rs-dpp/src/state_transition/state_transitions/identity/identity_topup_transition/v0/v0_methods.rs`:
- Around line 39-43: Change the unwrap on
validation_result.errors.into_iter().next() to an if-let to avoid panicking:
after calling asset_lock_proof.validate_structure(platform_version) and
observing !validation_result.is_valid(), match the first error with if let
Some(first_error) = validation_result.errors.into_iter().next() { return
Err(ProtocolError::ConsensusError(Box::new(first_error))); } else {
unreachable!("validation_result.is_valid() == false must mean there is at least
one error") }; this replaces the .unwrap() and references
asset_lock_proof.validate_structure, validation_result, and
ProtocolError::ConsensusError.
- Line 46: Remove the noisy placeholder "let _ = platform_version;" and instead
annotate the platform_version parameter so the compiler only ignores it when the
validation feature is disabled; e.g., on the function in v0_methods.rs that
receives platform_version, add #[cfg_attr(not(feature = "validation"),
allow(unused_variables))] to the platform_version parameter declaration (or use
allow(unused_variables) via cfg_attr on the parameter) and delete the let _ =
... statement.
…action Replace is_valid() check + unwrap() with if-let pattern to eliminate fragile coupling to ValidationResult internals. Addresses CodeRabbit nitpick on PR dashpay#3138.
|
Thanks for the review! Nitpick 1 (if-let instead of unwrap): Good catch — applied in f226e6f. The Nitpick 2 ( |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/rs-dpp/src/state_transition/state_transitions/identity/identity_topup_transition/v0/v0_methods.rs (1)
40-42:validation_result.errorsis a direct internal-field access — consider a consuming accessor.The commit message says the refactor avoids "fragile coupling to
ValidationResultinternals," yetvalidation_result.errors.into_iter().next()still reaches into the struct's field directly. The existingfirst_error()returnsOption<&E>(not owned), which is why it can't be used here — but the gap between intent and implementation is worth closing.♻️ Suggested approach: add a consuming
into_first_error()onValidationResultIn
packages/rs-dpp/src/validation/validation_result.rs:+ pub fn into_first_error(mut self) -> Option<E> { + if self.errors.is_empty() { None } else { Some(self.errors.remove(0)) } + }Then here:
- if let Some(first_error) = validation_result.errors.into_iter().next() { + if let Some(first_error) = validation_result.into_first_error() {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-dpp/src/state_transition/state_transitions/identity/identity_topup_transition/v0/v0_methods.rs` around lines 40 - 42, Introduce a consuming accessor on ValidationResult (e.g., into_first_error() -> Option<E>) that returns and consumes the first error instead of exposing the internal errors field; then update the v0_methods.rs call site (where validation_result is used) to call validation_result.into_first_error().map(|e| ProtocolError::ConsensusError(Box::new(e))).and_then(Err) or equivalently return Err(ProtocolError::ConsensusError(Box::new(e))) when Some, thereby removing direct access to validation_result.errors and using ValidationResult::into_first_error() instead.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@packages/rs-dpp/src/state_transition/state_transitions/identity/identity_topup_transition/v0/v0_methods.rs`:
- Around line 40-42: Introduce a consuming accessor on ValidationResult (e.g.,
into_first_error() -> Option<E>) that returns and consumes the first error
instead of exposing the internal errors field; then update the v0_methods.rs
call site (where validation_result is used) to call
validation_result.into_first_error().map(|e|
ProtocolError::ConsensusError(Box::new(e))).and_then(Err) or equivalently return
Err(ProtocolError::ConsensusError(Box::new(e))) when Some, thereby removing
direct access to validation_result.errors and using
ValidationResult::into_first_error() instead.
…action Replace is_valid() check + unwrap() with if-let pattern to eliminate fragile coupling to ValidationResult internals. Addresses CodeRabbit nitpick on PR dashpay#3138.
thepastaclaw
force-pushed
the
feat/validate-remaining-st-construction
branch
from
f226e6f to
df9ecbd
Compare
…tion construction
…action Replace is_valid() check + unwrap() with if-let pattern to eliminate fragile coupling to ValidationResult internals. Addresses CodeRabbit nitpick on PR dashpay#3138.
…stead of magic number
thepastaclaw
force-pushed
the
feat/validate-remaining-st-construction
branch
from
df9ecbd to
f6d73d1
Compare
2 participants
Issue Being Fixed or Feature Description
Adds client-side validation to the remaining state transition construction methods that lack it, following the pattern established in #3096.
Context: shumkov's review on #3096 asked for validation to be added consistently across all ST construction methods. This PR covers the identity credit transfer and identity top-up transitions.
What was done?
IdentityCreditTransferTransition (
try_from_identity)IdentityCreditTransferToSelfErrorifidentity_id == recipient_idInvalidIdentityCreditTransferAmountErrorifamount < 100_000creditsThese checks mirror the server-side
validate_basic_structure_v0inrs-drive-abci, catching invalid transfers before they're signed and broadcast.IdentityTopUpTransition (
try_from_identity)AssetLockProof::validate_structure()to validate the proof before constructing the transition#[cfg(feature = "validation")]sincevalidate_structurerequires that featureWhat's NOT covered (and why)
IdentityCreditWithdrawalTransitionUnsupportedFeatureError) — no meaningful checks to portMasternodeVoteTransitionDataContractCreate/UpdateTransitionNetworkparameter not available in the constructor; SDK-levelput_contract.rsalready callsensure_valid_state_transition_structureHow Has This Been Tested?
cargo check -p dpp --features "state-transition-signing"✅cargo check -p dpp --features "state-transition-signing,validation"✅cargo check -p dash-sdk✅cargo test -p dpp --features "state-transition-signing" -- identity_credit_transfer✅ (3 tests pass)Breaking Changes
None. Existing valid transitions continue to work. Invalid transitions that would have been rejected server-side are now caught earlier at construction time.
Checklist
Summary by CodeRabbit
New Features
Improvements
Validation
What was tested
cargo test -p dash-sdk --lib— client-side validation integrationcargo test -p dpp --lib— validation logic unit testsResults
Environment