feat(sdk): add client-side validation to state transition construction methods#3096
Open
thepastaclaw wants to merge 20 commits into
Open
Conversation
Contributor
|
Caution Review failedAn error occurred during the review process. Please try again later. ✨ 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}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/rs-dpp/src/state_transition/state_transitions/identity/identity_create_transition/v0/v0_methods.rs (1)
45-45:_platform_versionis now used — consider removing the underscore prefix.The
_prefix conventionally signals an intentionally-unused binding. Since this parameter is now actively consumed byvalidate_identity_public_keys_structure(line 64), the prefix is misleading. This applies to all three files in the PR (identity_create_transition,identity_update_transition,identity_create_from_addresses_transition).That said, this is a pre-existing naming choice inherited from the trait signature, so feel free to defer if changing it would cascade across the trait definition.
🤖 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_create_transition/v0/v0_methods.rs` at line 45, The parameter named `_platform_version` is now used by validate_identity_public_keys_structure, so remove the misleading underscore by renaming `_platform_version` to `platform_version` in the function signature in v0_methods.rs (and analogously in the other two files: identity_update_transition and identity_create_from_addresses_transition), and update all usages inside the function (including the call to validate_identity_public_keys_structure) to use the new `platform_version` identifier; if the underscore comes from a trait signature you can instead change the local binding to `platform_version` (keeping the trait name) to avoid cascading trait edits.
🤖 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_create_transition/v0/v0_methods.rs`:
- Line 45: The parameter named `_platform_version` is now used by
validate_identity_public_keys_structure, so remove the misleading underscore by
renaming `_platform_version` to `platform_version` in the function signature in
v0_methods.rs (and analogously in the other two files:
identity_update_transition and identity_create_from_addresses_transition), and
update all usages inside the function (including the call to
validate_identity_public_keys_structure) to use the new `platform_version`
identifier; if the underscore comes from a trait signature you can instead
change the local binding to `platform_version` (keeping the trait name) to avoid
cascading trait edits.
Member
|
This is a test that you'll actually act on my review comment. Please just comment with "got it" :D |
Collaborator
Author
|
got it :D |
shumkov
requested changes
Feb 18, 2026
| _platform_version: &PlatformVersion, | ||
| platform_version: &PlatformVersion, | ||
| _version: Option<FeatureVersion>, | ||
| ) -> Result<StateTransition, ProtocolError> { |
Collaborator
There was a problem hiding this comment.
@QuantumExplorer @lklimek do you think, guys, is it the right place to validate data in SDK when the user creates it? I just wondering if we should call the validation method in SDK methods instead + have an option to skip validation.
Contributor
There was a problem hiding this comment.
As a rule of thumb, I would say user should not be able to create an object that is invalid, unless he really tries to and knows what he's doing.
I don't see the use case for non-validated identity update transition, but if you do and you think we need it, it should be separate constructor IMO.
See https://rust-lang.github.io/api-guidelines/dependability.html#dynamic-enforcement-with-opt-out
The convention is to mark these opt-out functions with a suffix like _unchecked or by placing them in a raw submodule.
The unchecked functions can be used judiciously in cases where (1) performance dictates avoiding checks and (2) the client is otherwise confident that the inputs are valid.
Collaborator
Author
There was a problem hiding this comment.
Agreed — validation by default is the right call, and the current implementation does exactly that. No use case for an unchecked path right now, but if one comes up I'll follow the _unchecked convention from the Rust API guidelines. Thanks for the reference!
Collaborator
There was a problem hiding this comment.
Ok, sounds good! I'm fine with validated only version for now. @thepastaclaw please create PRs with validation for other state transitions so we have consistent behaviour for SDK.
Collaborator
Author
|
Re: @shumkov's question about validation placement: Good question. I put validation here (in the DPP method that constructs the transition) because this is the earliest point where we know all the keys and can catch the error — before any signing or serialization happens. The alternative of validating in SDK methods would work too, but would mean the raw DPP construction method silently accepts invalid key combinations that the platform will reject anyway. Happy to move it to the SDK layer with a skip-validation option if that is the preferred pattern. Deferring to @QuantumExplorer and @lklimek on the right approach. |
shumkov
previously approved these changes
Feb 18, 2026
Collaborator
Author
|
Re: @shumkov's request for consistent validation: Will do! I'll create follow-up PRs adding the same client-side validation to the other state transitions for consistency across the SDK. Thanks for the review! |
thepastaclaw
added a commit
to thepastaclaw/platform
that referenced
this pull request
Feb 18, 2026
…ansitions Add client-side structure validation to 6 state transition SDK construction methods, following the pattern established in PR dashpay#3096. This ensures invalid transitions are caught early on the client side before being submitted. State transitions updated: - AddressCreditWithdrawalTransition - AddressFundingFromAssetLockTransition - AddressFundsTransferTransition - IdentityCreateFromAddressesTransition - IdentityCreditTransferToAddressesTransition - IdentityTopUpFromAddressesTransition Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
thepastaclaw
added a commit
to thepastaclaw/platform
that referenced
this pull request
Feb 18, 2026
…ansitions Add client-side structure validation to 6 state transition SDK construction methods, following the pattern established in PR dashpay#3096. This ensures invalid transitions are caught early on the client side before being submitted. State transitions updated: - AddressCreditWithdrawalTransition - AddressFundingFromAssetLockTransition - AddressFundsTransferTransition - IdentityCreateFromAddressesTransition - IdentityCreditTransferToAddressesTransition - IdentityTopUpFromAddressesTransition Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
thepastaclaw
changed the title
Contributor
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
thepastaclaw
added a commit
to thepastaclaw/platform
that referenced
this pull request
Feb 20, 2026
…ansitions Add client-side structure validation to 6 state transition SDK construction methods, following the pattern established in PR dashpay#3096. This ensures invalid transitions are caught early on the client side before being submitted. State transitions updated: - AddressCreditWithdrawalTransition - AddressFundingFromAssetLockTransition - AddressFundsTransferTransition - IdentityCreateFromAddressesTransition - IdentityCreditTransferToAddressesTransition - IdentityTopUpFromAddressesTransition Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
thepastaclaw
force-pushed
the
fix/validate-transfer-key-security-level-client-side
branch
from
ef57031 to
44c71dd
Compare
thepastaclaw
added a commit
to thepastaclaw/platform
that referenced
this pull request
Feb 20, 2026
…oken transitions Add structural validation to all document and token SDK transition builders, matching the pattern from PR dashpay#3096 (identity/address transitions). Calls validate_base_structure() on BatchTransition after construction but before broadcast, catching invalid transitions early. Applied to: - Document transitions: create, delete, replace, purchase, set_price, transfer - Token builders: burn, claim, config_update, destroy, purchase, emergency_action, freeze, mint, set_price, transfer, unfreeze - Enabled dpp 'validation' feature for dash-sdk crate
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/rs-dpp/src/state_transition/state_transitions/address_funds/address_funds_transfer_transition/signing_tests.rs (2)
829-837:⚠️ Potential issue | 🟡 MinorTampered output value could coincide with the fee-reduced stored value.
After
ReduceOutput(0)is applied during construction the stored output is1_000_000 − fee. The tampering sets it to950_000. If the platform fee happens to equal exactly50_000credits, the two values are identical, the signable bytes are unchanged, verification succeeds, andassert!(result.is_err())would fail. Consider choosing a tampered value that is guaranteed to differ (e.g.,500_000u64or any value far from the original1_000_000), or read the actual stored output and modify it by a fixed delta.🤖 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/address_funds/address_funds_transfer_transition/signing_tests.rs` around lines 829 - 837, The test uses a hardcoded tampered value that may equal the stored output after ReduceOutput(0); update the tamper logic in signing_tests.rs so the modified output is guaranteed different: either set a clearly different constant (e.g., 500_000u64) when calling transition.outputs.insert(...) or fetch the stored value for the output (from transition.outputs.get(&output) or equivalent) and change it by a fixed non-zero delta (e.g., -1 or +12345) before reinserting; keep references to the existing ReduceOutput(0) behavior and ensure verify_transition_signatures(&transition) is expected to return Err.
1012-1013:⚠️ Potential issue | 🟡 MinorMissing
else { panic!() }guards in edge-caseif letblocks.If the witness at index 0 is unexpectedly not
P2sh, bothtest_1_of_1_multisigandtest_high_threshold_multisigsilently skip thesignatures.len()assertion and pass vacuously — hiding a type-mismatch. Other P2SH tests (e.g.,test_single_p2sh_2_of_3_multisig_input_signing) correctly include anelse { panic!("Expected P2SH witness") }branch.🔧 Proposed fix for both tests
if let AddressWitness::P2sh { signatures, .. } = &transition.input_witnesses[0] { assert_eq!(signatures.len(), 1); +} else { + panic!("Expected P2SH witness"); }if let AddressWitness::P2sh { signatures, .. } = &transition.input_witnesses[0] { assert_eq!(signatures.len(), 5); +} else { + panic!("Expected P2SH witness"); }Also applies to: 1051-1053
🤖 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/address_funds/address_funds_transfer_transition/signing_tests.rs` around lines 1012 - 1013, Both tests use an if let AddressWitness::P2sh { signatures, .. } = &transition.input_witnesses[0] pattern but lack an else panic branch, letting a non-P2sh witness silently skip the assertion; update the two tests (test_1_of_1_multisig and test_high_threshold_multisig) to add an else { panic!("Expected P2SH witness") } guard after the if let so the test fails loudly on a mismatched witness type, referencing the same AddressWitness::P2sh destructuring and transition.input_witnesses[0] access used now; apply the same change for the analogous block around lines 1051-1053.
🧹 Nitpick comments (2)
packages/rs-dpp/src/state_transition/state_transitions/address_funds/address_funds_transfer_transition/signing_tests.rs (1)
287-292: Extract the repeated V0 transition unwrapping into a test helper.The nested
matchthat destructuresStateTransition::AddressFundsTransfer(…::V0(v0))appears ~15 times. A small private helper eliminates the boilerplate and makes every test body easier to scan.♻️ Suggested helper
fn unwrap_transfer_v0(st: StateTransition) -> AddressFundsTransferTransitionV0 { match st { StateTransition::AddressFundsTransfer( crate::state_transition::address_funds_transfer_transition::AddressFundsTransferTransition::V0(v0), ) => v0, _ => panic!("Expected AddressFundsTransfer V0 transition"), } }Then every call site becomes:
- let transition = match state_transition { - StateTransition::AddressFundsTransfer(t) => match t { - crate::state_transition::address_funds_transfer_transition::AddressFundsTransferTransition::V0(v0) => v0, - }, - _ => panic!("Expected AddressFundsTransfer transition"), - }; + let transition = unwrap_transfer_v0(state_transition);🤖 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/address_funds/address_funds_transfer_transition/signing_tests.rs` around lines 287 - 292, The test suite repeats a nested match to extract StateTransition::AddressFundsTransfer(...::V0(v0)) about 15 times; add a small private helper fn unwrap_transfer_v0(st: StateTransition) -> AddressFundsTransferTransitionV0 that matches StateTransition::AddressFundsTransfer(crate::state_transition::address_funds_transfer_transition::AddressFundsTransferTransition::V0(v0)) => v0 and panics otherwise, then replace each repeated match in signing_tests.rs with a call to unwrap_transfer_v0(state_transition) to remove boilerplate and simplify test bodies.packages/rs-dpp/src/state_transition/state_transitions/identity/identity_create_from_addresses_transition/v0/v0_methods.rs (1)
110-116: Consider extracting the repeated validation-to-error pattern into a helper.The same 5-line block (
validate_structure→is_valid→errors.into_iter().next().unwrap()→ConsensusError) is duplicated across ~7 call sites in this PR. A small helper onValidationResult(or a free function) would reduce boilerplate and ensure consistency.Example helper
Something like (in
validation_result.rsor a utility module):impl<E: Into<ConsensusError>> ValidationResult<E> { pub fn into_result(self) -> Result<(), ProtocolError> { if self.is_valid() { Ok(()) } else { let first_error = self.errors.into_iter().next().unwrap(); Err(ProtocolError::ConsensusError(Box::new(first_error.into()))) } } }Then each call site simplifies to:
- let validation_result = - identity_create_from_addresses_transition.validate_structure(platform_version); - if !validation_result.is_valid() { - let first_error = validation_result.errors.into_iter().next().unwrap(); - return Err(ProtocolError::ConsensusError(Box::new(first_error))); - } + identity_create_from_addresses_transition + .validate_structure(platform_version) + .into_result()?;🤖 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_create_from_addresses_transition/v0/v0_methods.rs` around lines 110 - 116, The repeated pattern of calling validate_structure(...), checking validation_result.is_valid(), extracting the first error via validation_result.errors.into_iter().next().unwrap(), and wrapping it in ProtocolError::ConsensusError should be extracted into a helper to remove boilerplate; add a method (e.g., impl ValidationResult<E> { pub fn into_result(self) -> Result<(), ProtocolError> }) or a free utility that returns Ok(()) when is_valid() and returns Err(ProtocolError::ConsensusError(Box::new(first_error.into()))) otherwise, then replace the repeated blocks in functions like identity_create_from_addresses_transition.validate_structure(...) call sites with a single call to validation_result.into_result() (or the free helper) to ensure consistent behavior and concise code.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@packages/rs-dpp/src/state_transition/state_transitions/address_funds/address_funds_transfer_transition/signing_tests.rs`:
- Line 270: Update the stale inline comment next to the inputs.insert call that
currently reads "nonce: 1, credits: 1000" to reflect the actual value passed
(1_000_000); locate the inputs.insert(input_address.clone(), (1u32,
1_000_000u64)) line and change the comment to "nonce: 1, credits: 1_000_000" (or
remove the comment if redundant).
---
Outside diff comments:
In
`@packages/rs-dpp/src/state_transition/state_transitions/address_funds/address_funds_transfer_transition/signing_tests.rs`:
- Around line 829-837: The test uses a hardcoded tampered value that may equal
the stored output after ReduceOutput(0); update the tamper logic in
signing_tests.rs so the modified output is guaranteed different: either set a
clearly different constant (e.g., 500_000u64) when calling
transition.outputs.insert(...) or fetch the stored value for the output (from
transition.outputs.get(&output) or equivalent) and change it by a fixed non-zero
delta (e.g., -1 or +12345) before reinserting; keep references to the existing
ReduceOutput(0) behavior and ensure verify_transition_signatures(&transition) is
expected to return Err.
- Around line 1012-1013: Both tests use an if let AddressWitness::P2sh {
signatures, .. } = &transition.input_witnesses[0] pattern but lack an else panic
branch, letting a non-P2sh witness silently skip the assertion; update the two
tests (test_1_of_1_multisig and test_high_threshold_multisig) to add an else {
panic!("Expected P2SH witness") } guard after the if let so the test fails
loudly on a mismatched witness type, referencing the same AddressWitness::P2sh
destructuring and transition.input_witnesses[0] access used now; apply the same
change for the analogous block around lines 1051-1053.
---
Nitpick comments:
In
`@packages/rs-dpp/src/state_transition/state_transitions/address_funds/address_funds_transfer_transition/signing_tests.rs`:
- Around line 287-292: The test suite repeats a nested match to extract
StateTransition::AddressFundsTransfer(...::V0(v0)) about 15 times; add a small
private helper fn unwrap_transfer_v0(st: StateTransition) ->
AddressFundsTransferTransitionV0 that matches
StateTransition::AddressFundsTransfer(crate::state_transition::address_funds_transfer_transition::AddressFundsTransferTransition::V0(v0))
=> v0 and panics otherwise, then replace each repeated match in signing_tests.rs
with a call to unwrap_transfer_v0(state_transition) to remove boilerplate and
simplify test bodies.
In
`@packages/rs-dpp/src/state_transition/state_transitions/identity/identity_create_from_addresses_transition/v0/v0_methods.rs`:
- Around line 110-116: The repeated pattern of calling validate_structure(...),
checking validation_result.is_valid(), extracting the first error via
validation_result.errors.into_iter().next().unwrap(), and wrapping it in
ProtocolError::ConsensusError should be extracted into a helper to remove
boilerplate; add a method (e.g., impl ValidationResult<E> { pub fn
into_result(self) -> Result<(), ProtocolError> }) or a free utility that returns
Ok(()) when is_valid() and returns
Err(ProtocolError::ConsensusError(Box::new(first_error.into()))) otherwise, then
replace the repeated blocks in functions like
identity_create_from_addresses_transition.validate_structure(...) call sites
with a single call to validation_result.into_result() (or the free helper) to
ensure consistent behavior and concise code.
| // Build inputs and outputs | ||
| let mut inputs = BTreeMap::new(); | ||
| inputs.insert(input_address.clone(), (1u32, 1000u64)); // nonce: 1, credits: 1000 | ||
| inputs.insert(input_address.clone(), (1u32, 1_000_000u64)); // nonce: 1, credits: 1000 |
Contributor
There was a problem hiding this comment.
Stale comment: credits: 1000 doesn't match the updated value 1_000_000.
The inline comment was not updated when the credit value was scaled up.
🔧 Proposed fix
- inputs.insert(input_address.clone(), (1u32, 1_000_000u64)); // nonce: 1, credits: 1000
+ inputs.insert(input_address.clone(), (1u32, 1_000_000u64)); // nonce: 1, credits: 1_000_000📝 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
| inputs.insert(input_address.clone(), (1u32, 1_000_000u64)); // nonce: 1, credits: 1000 | |
| inputs.insert(input_address.clone(), (1u32, 1_000_000u64)); // nonce: 1, credits: 1_000_000 |
🤖 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/address_funds/address_funds_transfer_transition/signing_tests.rs`
at line 270, Update the stale inline comment next to the inputs.insert call that
currently reads "nonce: 1, credits: 1000" to reflect the actual value passed
(1_000_000); locate the inputs.insert(input_address.clone(), (1u32,
1_000_000u64)) line and change the comment to "nonce: 1, credits: 1_000_000" (or
remove the comment if redundant).
5 tasks
thepastaclaw
added a commit
to thepastaclaw/platform
that referenced
this pull request
Feb 21, 2026
…oken transitions Add structural validation to all document and token SDK transition builders, matching the pattern from PR dashpay#3096 (identity/address transitions). Calls validate_base_structure() on BatchTransition after construction but before broadcast, catching invalid transitions early. Applied to: - Document transitions: create, delete, replace, purchase, set_price, transfer - Token builders: burn, claim, config_update, destroy, purchase, emergency_action, freeze, mint, set_price, transfer, unfreeze - Enabled dpp 'validation' feature for dash-sdk crate
thepastaclaw
added a commit
to thepastaclaw/platform
that referenced
this pull request
Feb 21, 2026
…ansitions Add client-side structure validation to 6 state transition SDK construction methods, following the pattern established in PR dashpay#3096. This ensures invalid transitions are caught early on the client side before being submitted. State transitions updated: - AddressCreditWithdrawalTransition - AddressFundingFromAssetLockTransition - AddressFundsTransferTransition - IdentityCreateFromAddressesTransition - IdentityCreditTransferToAddressesTransition - IdentityTopUpFromAddressesTransition Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
thepastaclaw
force-pushed
the
fix/validate-transfer-key-security-level-client-side
branch
from
67ef15e to
42a8d45
Compare
thepastaclaw
added a commit
to thepastaclaw/platform
that referenced
this pull request
Feb 21, 2026
…oken transitions Add structural validation to all document and token SDK transition builders, matching the pattern from PR dashpay#3096 (identity/address transitions). Calls validate_base_structure() on BatchTransition after construction but before broadcast, catching invalid transitions early. Applied to: - Document transitions: create, delete, replace, purchase, set_price, transfer - Token builders: burn, claim, config_update, destroy, purchase, emergency_action, freeze, mint, set_price, transfer, unfreeze - Enabled dpp 'validation' feature for dash-sdk crate
thepastaclaw
added a commit
to thepastaclaw/platform
that referenced
this pull request
Feb 21, 2026
…ansitions Add client-side structure validation to 6 state transition SDK construction methods, following the pattern established in PR dashpay#3096. This ensures invalid transitions are caught early on the client side before being submitted. State transitions updated: - AddressCreditWithdrawalTransition - AddressFundingFromAssetLockTransition - AddressFundsTransferTransition - IdentityCreateFromAddressesTransition - IdentityCreditTransferToAddressesTransition - IdentityTopUpFromAddressesTransition Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…UpdateTransition Add client-side validation of public key purpose/security level compatibility in try_from_identity_with_signer() before the state transition is signed and broadcast. Previously, adding a TRANSFER key with a security level other than CRITICAL would only be rejected by the network after broadcasting. Now the validation from validate_identity_public_keys_structure() is called during transition construction, giving immediate feedback (e.g. 'Transfer keys must use CRITICAL security level') without wasting a network round-trip. This catches issues like trying to create a transfer key with HIGH or MEDIUM security level, which Platform requires to be CRITICAL.
Extend the same validate_identity_public_keys_structure() check to IdentityCreateTransition and IdentityCreateFromAddressesTransition. The previous commit only covered IdentityUpdateTransition (adding keys), but the same issue affects identity creation — e.g. creating an identity with a TRANSFER key at non-CRITICAL security level would only be rejected by the network, with no client-side feedback.
…ameter Addresses review comment: variable was previously unused but is now passed to validate_identity_public_keys_structure().
The _platform_version parameters in identity_create_transition and identity_create_from_addresses_transition are now actively used by validate_identity_public_keys_structure, so remove the underscore prefix that conventionally signals unused bindings.
…ansitions Add client-side structure validation to 6 state transition SDK construction methods, following the pattern established in PR dashpay#3096. This ensures invalid transitions are caught early on the client side before being submitted. State transitions updated: - AddressCreditWithdrawalTransition - AddressFundingFromAssetLockTransition - AddressFundsTransferTransition - IdentityCreateFromAddressesTransition - IdentityCreditTransferToAddressesTransition - IdentityTopUpFromAddressesTransition Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…cture Update signing_tests to use valid amounts (>= min thresholds), balanced input/output sums, and non-empty fee strategies. Update drive-abci structure_validation tests to use raw transition construction (bypassing client-side validation) since they intentionally test server-side rejection of invalid structures.
- Add take_random_amounts_with_range_and_min_per_input to enforce min_input_amount per individual input (prevents InputBelowMinimumError) - Update all address transition constructors to use min_per_input from platform_version.dpp.state_transitions.address_funds.min_input_amount - Cap output_count in transfers so each output >= min_output_amount - Add remainder distribution to first output to prevent InputOutputBalanceMismatchError from integer division - Relax hardcoded tree structure assertions in checkpoint tests (elements count and chunk_depths) to range checks since the deterministic output changes with the new amount generation
thepastaclaw
force-pushed
the
fix/validate-transfer-key-security-level-client-side
branch
from
a414f24 to
37c5f3e
Compare
Collaborator
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
thepastaclaw
commented
Jun 17, 2026
thepastaclaw
left a comment
thepastaclaw
left a comment
Collaborator
Author
There was a problem hiding this comment.
Code Review
Two convergent blocking findings: (1) the new shared basic_structure_rules_v1_for_transition helper uses the constant MIN_WITHDRAWAL_AMOUNT (190,000) instead of platform_version.system_limits.min_withdrawal_amount (1,000,000 at v12), producing a consensus split on v12; (2) the WASM SDK's WasmSdkError::protocol_with_consensus_errors exposes the structured array under different field names in the empty vs populated branches. Several non-blocking quality concerns flag uneven application of the new client-side validation in core_key_wallet-gated external-signer constructors.
🔴 2 blocking | 🟡 3 suggestion(s) | 💬 3 nitpick(s)
3 additional finding(s) omitted (not in diff).
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-dpp/src/state_transition/state_transitions/identity/identity_credit_withdrawal_transition/mod.rs`:
- [BLOCKING] packages/rs-dpp/src/state_transition/state_transitions/identity/identity_credit_withdrawal_transition/mod.rs:138-146: Consensus regression on v12: shared withdrawal helper uses constant MIN_WITHDRAWAL_AMOUNT instead of versioned system_limits.min_withdrawal_amount
`basic_structure_rules_v1_for_transition` compares the withdrawal `amount` against the local constant `MIN_WITHDRAWAL_AMOUNT` (190,000) and surfaces the same value as the `min_amount` in `InvalidIdentityCreditWithdrawalTransitionAmountError`. Before this PR, drive-abci's `IdentityCreditWithdrawalStateTransitionStructureValidationV1::validate_basic_structure_v1` used the versioned `platform_version.system_limits.min_withdrawal_amount`. `SYSTEM_LIMITS_V2` (used by `PLATFORM_V12`, the `LATEST_PLATFORM_VERSION`) raises this floor to 1,000,000 credits — see `packages/rs-platform-version/src/version/system_limits/v2.rs:16`. After this PR, v12 nodes will accept identity credit withdrawals with `amount ∈ [190_000, 999_999]` that pre-PR v12 nodes (and the published v12 spec) reject. Both V0 and V1 transition variants route through this helper, and the existing drive-abci unit tests only exercise `PLATFORM_V1` (where v1 and v2 limits coincide at 190_000), so they don't catch the regression. The doc comment on `MIN_WITHDRAWAL_AMOUNT` itself acknowledges the consensus floor is read from the *versioned* field. Use `platform_version.system_limits.min_withdrawal_amount` for both the comparison and the error payload, and add a v12 regression test.
In `packages/wasm-sdk/src/error.rs`:
- [BLOCKING] packages/wasm-sdk/src/error.rs:271-280: WASM error `details` uses `consensusErrors` in empty branch but `errors` in populated branch
`WasmSdkError::protocol_with_consensus_errors` is the user-facing JS contract for the new structured consensus errors. The empty-list branch (lines 264–289) sets the structured array on `details.consensusErrors`, while the populated branch (lines 327–331) sets it on `details.errors`. A JS caller cannot read a single, consistent field for the structured list — whichever name they pick is `undefined` for the other case. Aligning on `errors` (matching the `Error.errors` shape already used in `packages/wasm-dpp/src/errors/protocol_error.rs::consensus_errors_to_js_error`) is the consistent fix; the populated branch is correct.
In `packages/rs-dpp/src/state_transition/state_transitions/identity/identity_create_transition/v0/v0_methods.rs`:
- [SUGGESTION] packages/rs-dpp/src/state_transition/state_transitions/identity/identity_create_transition/v0/v0_methods.rs:226-229: Bypasses `consensus_errors_as_protocol_error`, silently drops additional consensus errors
Every other validation-failure site in this file (lines 88, 106, 139) funnels `SimpleConsensusValidationResult` through the new `consensus_errors_as_protocol_error` helper, which preserves the entire error list via `ProtocolError::ConsensusErrors`. This single site in `try_from_identity_with_signers` hand-rolls `validation_result.errors.into_iter().next().unwrap()`, which drops any additional consensus errors `validate_identity_public_keys_structure` surfaced and reintroduces an unguarded `unwrap()` in a constructor where the helper makes it panic-free. The helper is already imported on line 39.
- [SUGGESTION] packages/rs-dpp/src/state_transition/state_transitions/identity/identity_create_transition/v0/v0_methods.rs:197-269: `try_from_identity_with_signers` skips asset-lock-proof and PoP validations its sibling now performs
The primary entry point `try_from_identity_with_signer_and_private_key` (lines 56–188) now does three pre/post-signing validations: `validate_identity_public_keys_structure`, `asset_lock_proof.validate_structure(platform_version)`, and a post-signing proof-of-possession verification loop mirroring the server-side `identity_create` signatures validator. The `core_key_wallet`-gated sibling `try_from_identity_with_signers` only does the first. This is the external-signer path used by Swift / hardware-wallet consumers and is the exact "silently construct, network rejects" failure mode this PR is trying to eliminate. Mirror the missing two checks here (using the same `consensus_errors_as_protocol_error` helper) or factor the shared pre/post-signing checks into a helper both signers call.
In `packages/rs-dpp/src/state_transition/state_transitions/address_funds/address_funding_from_asset_lock_transition/v0/v0_methods.rs`:
- [SUGGESTION] packages/rs-dpp/src/state_transition/state_transitions/address_funds/address_funding_from_asset_lock_transition/v0/v0_methods.rs:175-230: `try_from_asset_lock_with_signers` skips every new client-side check added by the PR
The raw-private-key sibling `try_from_asset_lock_with_signer_and_private_key` (lines 42–173) was extended in this PR with `address_funds_constructor_dispatch_error`, `validate_structure_without_input_witnesses`, `asset_lock_proof.validate_structure`, `verify_address_witnesses`, and a post-signing `validate_input_witnesses_count` — the exact pattern the PR rolls out. The `core_key_wallet`-gated variant directly below received none of those checks: it still names the version argument `_platform_version`, goes straight to building signable bytes, gathers witnesses, and returns a signed transition. This leaves the external-signer path (Swift / HSM consumers) on the same pre-PR "construct → broadcast → network rejection" behaviour the PR is trying to fix. Mirror the validation calls here or extract a shared pre-/post-signing helper.
Collaborator
Author
|
@shumkov @QuantumExplorer moved this out of draft. The visible current state is review/merge judgment rather than an open implementation blocker: CodeRabbit completed, PR title/semantic checks pass, old Shumkov review state is dismissed, and this consolidates the client-side transition validation work from the earlier review thread. Ready for Platform review. |
Member
|
@thepastaclaw -- let's rebase this PR, also ensure that your latest automated code review findings are handled |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## v4.1-dev #3096 +/- ##
=============================================
- Coverage 87.16% 52.54% -34.63%
=============================================
Files 2627 11 -2616
Lines 321557 1707 -319850
=============================================
- Hits 280299 897 -279402
+ Misses 41258 810 -40448
🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
SDK construction methods for state transitions don't validate the transition structure before returning. Invalid transitions silently construct and broadcast, only to be rejected by the network with confusing errors.
For example:
IdentityUpdateTransition— rejected on broadcast with no clear indication whyFix
Add client-side validation calls during transition construction, before signing and broadcasting. This reuses existing validation logic from
rs-dpp(which Platform already uses server-side inrs-drive-abci).Changes in two parts:
1. Public key security level validation (originally reported by @thephez):
IdentityUpdateTransition::try_from_identity_with_signer()— validates key purpose/security level compatibilityIdentityCreateTransition::try_from_identity_with_signer()— same validationIdentityCreateFromAddressesTransition::try_from_inputs_with_signer()— same validationWhat gets validated:
2. Full
validate_structure()on remaining state transitions (per shumkov's review):AddressCreditWithdrawalTransitiontry_from_inputs_with_signerAddressFundingFromAssetLockTransitiontry_from_asset_lock_with_signerAddressFundsTransferTransitiontry_from_inputs_with_signerIdentityCreateFromAddressesTransitiontry_from_inputs_with_signerIdentityCreditTransferToAddressesTransitiontry_from_identityIdentityTopUpFromAddressesTransitiontry_from_inputs_with_signerAll use the same pattern:
Validation is placed after the transition is fully constructed (witnesses set, signatures applied) so
validate_structure()sees the complete state.Context
/cc @QuantumExplorer
Summary by CodeRabbit
Bug Fixes
Tests
Validation
Build & Compilation
All modified Rust packages compile successfully:
rs-dpp— check each feature (13m21s ✅), formatting ✅, linting ✅rs-drive-abci— check each feature (5m27s ✅), formatting ✅, linting ✅Tests
All relevant test suites pass:
rs-dpptests ✅ (2m52s) — includes updated tests for new validation paths inaddress_funds_transfer_transition/signing_tests.rsrs-drive-abcitests ✅ (11m40s) — includes updated/expanded tests across all affected state transitions:address_credit_withdrawal/tests.rs(+140/-43 lines)address_funding_from_asset_lock/tests.rs(+12/-4)address_funds_transfer/tests.rs(+92/-42)identity_create_from_addresses/tests.rs(+59/-30)identity_credit_transfer_to_addresses/tests.rs(+77/-9)identity_top_up_from_addresses/tests.rs(+11/-1)rs-drivetests ✅ (7m48s)dash-sdktests ✅ (3m42s)strategy.rs+59/-10,address_tests.rs+24/-24)Additional CI
Unrelated Failures
Two CI jobs fail but are not related to this PR: