feat(sdk): add client-side validation to state transition construction methods#3096
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughAdds client-side identity public-key structure validation and runtime state-transition structure validation across multiple transition types; consistently renames Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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)
Tip 💬 Introducing Slack Agent: Turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 👉 Get your free trial and get 200 agent minutes per Slack user (a $50 value). 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 (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.
|
This is a test that you'll actually act on my review comment. Please just comment with "got it" :D |
|
got it :D |
| _platform_version: &PlatformVersion, | ||
| platform_version: &PlatformVersion, | ||
| _version: Option<FeatureVersion>, | ||
| ) -> Result<StateTransition, ProtocolError> { |
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.
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.
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!
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.
|
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. |
|
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! |
…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>
…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
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
…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
…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
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 |
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.
| 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).
…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
…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
…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
…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
95a3e20 to
465a55c
Compare
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Well-executed PR adding client-side validation to state transition constructors. No blocking issues found. The sign-then-validate ordering in address-based transitions is correct (validate_structure checks witness counts). Main suggestions: replace logically-safe-but-convention-violating unwrap() calls with if-let, and consider returning all validation errors rather than just the first.
Reviewed commit: 465a55c
🟡 3 suggestion(s) | 💬 1 nitpick(s)
🤖 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/address_funds/address_credit_withdrawal_transition/v0/v0_methods.rs`:
- [SUGGESTION] lines 74-77: Replace unwrap() with if-let across 9 validation sites
The pattern `validation_result.errors.into_iter().next().unwrap()` appears 9 times across production code gated behind `state-transition-signing`. The unwrap is logically safe — `!is_valid()` guarantees `errors` is non-empty since `is_valid()` returns `self.errors.is_empty()`. However, it violates the project convention of no `unwrap()` in production code, and the panic-free alternative is trivial.
Affected files (all in `packages/rs-dpp/src/state_transition/state_transitions/`):
- `address_funds/address_credit_withdrawal_transition/v0/v0_methods.rs`
- `address_funds/address_funding_from_asset_lock_transition/v0/v0_methods.rs`
- `address_funds/address_funds_transfer_transition/v0/v0_methods.rs`
- `identity/identity_create_from_addresses_transition/v0/v0_methods.rs` (×2)
- `identity/identity_create_transition/v0/v0_methods.rs`
- `identity/identity_credit_transfer_to_addresses_transition/v0/v0_methods.rs`
- `identity/identity_topup_from_addresses_transition/v0/v0_methods.rs`
- `identity/identity_update_transition/v0/v0_methods.rs`
- [SUGGESTION] lines 74-77: Only the first validation error is returned; remaining errors are discarded
All 9 validation sites extract only the first error from `ValidationResult` and discard the rest. In a client-side SDK context this forces users into a fix-one-resubmit cycle. For example, `validate_identity_public_keys_structure` specifically collects ALL invalid keys via `filter_map`, but only the first issue is surfaced. Consider whether `ProtocolError` could carry a `Vec<ConsensusError>` or a multi-error variant. This is a deliberate design choice (consistent across all call sites), so flagging for consideration rather than as a bug.
In `packages/rs-dpp/src/state_transition/state_transitions/identity/identity_create_transition/v0/v0_methods.rs`:
- [SUGGESTION] line 73: IdentityCreateTransition skips asset lock proof structure validation
The server-side basic_structure validation for identity_create explicitly calls `asset_lock_proof.validate_structure(platform_version)` before proceeding. This client-side constructor validates public key structure but not the asset lock proof, so malformed proofs still get signed and sent to the network.
However, fixing this is not straightforward: `AssetLockProof::validate_structure()` is gated behind `#[cfg(feature = "validation")]`, which is NOT included in the `dash-sdk-features` feature set that the SDK uses. Adding the call would require also adding `validation` to `dash-sdk-features` or re-gating the method. Worth addressing but requires a feature-flag change.
| if !validation_result.is_valid() { | ||
| let first_error = validation_result.errors.into_iter().next().unwrap(); | ||
| return Err(ProtocolError::ConsensusError(Box::new(first_error))); | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: Replace unwrap() with if-let across 9 validation sites
The pattern validation_result.errors.into_iter().next().unwrap() appears 9 times across production code gated behind state-transition-signing. The unwrap is logically safe — !is_valid() guarantees errors is non-empty since is_valid() returns self.errors.is_empty(). However, it violates the project convention of no unwrap() in production code, and the panic-free alternative is trivial.
Affected files (all in packages/rs-dpp/src/state_transition/state_transitions/):
address_funds/address_credit_withdrawal_transition/v0/v0_methods.rsaddress_funds/address_funding_from_asset_lock_transition/v0/v0_methods.rsaddress_funds/address_funds_transfer_transition/v0/v0_methods.rsidentity/identity_create_from_addresses_transition/v0/v0_methods.rs(×2)identity/identity_create_transition/v0/v0_methods.rsidentity/identity_credit_transfer_to_addresses_transition/v0/v0_methods.rsidentity/identity_topup_from_addresses_transition/v0/v0_methods.rsidentity/identity_update_transition/v0/v0_methods.rs
| 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))); | |
| } |
source: ['claude']
🤖 Fix this 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/address_funds/address_credit_withdrawal_transition/v0/v0_methods.rs`:
- [SUGGESTION] lines 74-77: Replace unwrap() with if-let across 9 validation sites
The pattern `validation_result.errors.into_iter().next().unwrap()` appears 9 times across production code gated behind `state-transition-signing`. The unwrap is logically safe — `!is_valid()` guarantees `errors` is non-empty since `is_valid()` returns `self.errors.is_empty()`. However, it violates the project convention of no `unwrap()` in production code, and the panic-free alternative is trivial.
Affected files (all in `packages/rs-dpp/src/state_transition/state_transitions/`):
- `address_funds/address_credit_withdrawal_transition/v0/v0_methods.rs`
- `address_funds/address_funding_from_asset_lock_transition/v0/v0_methods.rs`
- `address_funds/address_funds_transfer_transition/v0/v0_methods.rs`
- `identity/identity_create_from_addresses_transition/v0/v0_methods.rs` (×2)
- `identity/identity_create_transition/v0/v0_methods.rs`
- `identity/identity_credit_transfer_to_addresses_transition/v0/v0_methods.rs`
- `identity/identity_topup_from_addresses_transition/v0/v0_methods.rs`
- `identity/identity_update_transition/v0/v0_methods.rs`
| if !validation_result.is_valid() { | ||
| let first_error = validation_result.errors.into_iter().next().unwrap(); | ||
| return Err(ProtocolError::ConsensusError(Box::new(first_error))); | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: Only the first validation error is returned; remaining errors are discarded
All 9 validation sites extract only the first error from ValidationResult and discard the rest. In a client-side SDK context this forces users into a fix-one-resubmit cycle. For example, validate_identity_public_keys_structure specifically collects ALL invalid keys via filter_map, but only the first issue is surfaced. Consider whether ProtocolError could carry a Vec<ConsensusError> or a multi-error variant. This is a deliberate design choice (consistent across all call sites), so flagging for consideration rather than as a bug.
source: ['claude']
🤖 Fix this 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/address_funds/address_credit_withdrawal_transition/v0/v0_methods.rs`:
- [SUGGESTION] lines 74-77: Only the first validation error is returned; remaining errors are discarded
All 9 validation sites extract only the first error from `ValidationResult` and discard the rest. In a client-side SDK context this forces users into a fix-one-resubmit cycle. For example, `validate_identity_public_keys_structure` specifically collects ALL invalid keys via `filter_map`, but only the first issue is surfaced. Consider whether `ProtocolError` could carry a `Vec<ConsensusError>` or a multi-error variant. This is a deliberate design choice (consistent across all call sites), so flagging for consideration rather than as a bug.
|
|
||
| identity_create_transition.set_public_keys(public_keys); | ||
|
|
||
| identity_create_transition.set_asset_lock_proof(asset_lock_proof)?; |
There was a problem hiding this comment.
🟡 Suggestion: IdentityCreateTransition skips asset lock proof structure validation
The server-side basic_structure validation for identity_create explicitly calls asset_lock_proof.validate_structure(platform_version) before proceeding. This client-side constructor validates public key structure but not the asset lock proof, so malformed proofs still get signed and sent to the network.
However, fixing this is not straightforward: AssetLockProof::validate_structure() is gated behind #[cfg(feature = "validation")], which is NOT included in the dash-sdk-features feature set that the SDK uses. Adding the call would require also adding validation to dash-sdk-features or re-gating the method. Worth addressing but requires a feature-flag change.
source: ['codex', 'claude']
🤖 Fix this 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_create_transition/v0/v0_methods.rs`:
- [SUGGESTION] line 73: IdentityCreateTransition skips asset lock proof structure validation
The server-side basic_structure validation for identity_create explicitly calls `asset_lock_proof.validate_structure(platform_version)` before proceeding. This client-side constructor validates public key structure but not the asset lock proof, so malformed proofs still get signed and sent to the network.
However, fixing this is not straightforward: `AssetLockProof::validate_structure()` is gated behind `#[cfg(feature = "validation")]`, which is NOT included in the `dash-sdk-features` feature set that the SDK uses. Adding the call would require also adding `validation` to `dash-sdk-features` or re-gating the method. Worth addressing but requires a feature-flag change.
| assert!( | ||
| trunk_result.elements.len() >= 16, | ||
| "trunk query should return at least 16 elements, got {}", | ||
| trunk_result.elements.len() | ||
| ); | ||
| assert_eq!( | ||
| trunk_result.leaf_keys.len(), | ||
| 0, | ||
| "trunk query should return 0 leaf keys" | ||
| ); | ||
| assert_eq!( | ||
| trunk_result.chunk_depths, | ||
| vec![6], | ||
| "trunk query should have chunk_depths [6]" | ||
| assert!( | ||
| trunk_result.chunk_depths.len() == 1 && trunk_result.chunk_depths[0] >= 5, | ||
| "trunk query chunk_depths should have 1 element >= 5, got {:?}", | ||
| trunk_result.chunk_depths | ||
| ); |
There was a problem hiding this comment.
💬 Nitpick: Trunk query assertions relaxed from exact values to wide ranges
Assertions changed from exact equality (== 32 elements, chunk_depths == vec![6]) to broad ranges (>= 16, >= 5). This was likely needed because the higher minimum input/output amounts changed the data distribution in the GroveDB tree. Since strategy tests use StdRng with fixed seeds, results should be deterministic for a given code version. Consider computing the actual new expected values and using tighter bounds.
source: ['claude']
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Follow-up exact-SHA review: the constructor validation changes look fine, but the new strategy helper still allows a final sub-minimum input after the requested minimum total has already been reached.
Reviewed commit: 465a55c
🔴 1 blocking
🤖 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/strategy-tests/src/addresses_with_balance.rs`:
- [BLOCKING] lines 431-449: The new min-per-input helper can still generate a final input below the enforced minimum
`take_random_amounts_with_range_and_min_per_input()` is supposed to enforce a minimum amount for every picked input, but once `taken_total >= range_min`, `remaining_to_min` becomes `0` and `step_min` collapses to `min(min_per_input, remaining_max)`. If only a small tail remains under `global_max` (for example `remaining_max < min_per_input`), the helper will still call `take_random_amount_with_bounds(step_min, step_max, ...)` with that smaller bound and can pick one more address for less than `min_per_input`. The function comment says this helper exists specifically because client-side validation now rejects inputs below `min_input_amount`, so this last-pick path defeats its own contract and can still generate invalid transitions or flaky tests.
| let remaining_to_min = range_min.saturating_sub(taken_total); | ||
|
|
||
| // Per-step min: | ||
| // - at least 1 | ||
| // - at least min_per_input (enforces per-input validation minimums) | ||
| // - at least enough so we can eventually reach range_min | ||
| // - but not more than remaining_max | ||
| let step_min = remaining_to_min.max(1).min(remaining_max); | ||
| let step_min = remaining_to_min.max(min_per_input).min(remaining_max); |
There was a problem hiding this comment.
🔴 Blocking: The new min-per-input helper can still generate a final input below the enforced minimum
take_random_amounts_with_range_and_min_per_input() is supposed to enforce a minimum amount for every picked input, but once taken_total >= range_min, remaining_to_min becomes 0 and step_min collapses to min(min_per_input, remaining_max). If only a small tail remains under global_max (for example remaining_max < min_per_input), the helper will still call take_random_amount_with_bounds(step_min, step_max, ...) with that smaller bound and can pick one more address for less than min_per_input. The function comment says this helper exists specifically because client-side validation now rejects inputs below min_input_amount, so this last-pick path defeats its own contract and can still generate invalid transitions or flaky tests.
source: ['codex']
🤖 Fix this 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/strategy-tests/src/addresses_with_balance.rs`:
- [BLOCKING] lines 431-449: The new min-per-input helper can still generate a final input below the enforced minimum
`take_random_amounts_with_range_and_min_per_input()` is supposed to enforce a minimum amount for every picked input, but once `taken_total >= range_min`, `remaining_to_min` becomes `0` and `step_min` collapses to `min(min_per_input, remaining_max)`. If only a small tail remains under `global_max` (for example `remaining_max < min_per_input`), the helper will still call `take_random_amount_with_bounds(step_min, step_max, ...)` with that smaller bound and can pick one more address for less than `min_per_input`. The function comment says this helper exists specifically because client-side validation now rejects inputs below `min_input_amount`, so this last-pick path defeats its own contract and can still generate invalid transitions or flaky tests.
…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
…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
465a55c to
a72e0e7
Compare
|
✅ Review complete (commit a2d190f) |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
PR adds client-side validate_structure() calls to seven state-transition constructors and key-security-level validation to identity create/update transitions. The implementation is consistent, but one blocking issue exists: the shared packages/strategy-tests generator was not updated alongside the rs-drive-abci copy and will panic non-deterministically under the new validation. Several quality and test-coverage suggestions also apply.
Reviewed commit: a72e0e7
🔴 1 blocking | 🟡 4 suggestion(s) | 💬 2 nitpick(s)
1 additional finding
🔴 blocking: `strategy-tests` still uses unconstrained selector and will panic under new client-side validation
packages/strategy-tests/src/lib.rs (lines 1481-1497)
The PR updated packages/rs-drive-abci/tests/strategy_tests/strategy.rs to call take_random_amounts_with_range_and_min_per_input(...) (passing the platform-version min_input_amount) before constructing address-funded transitions, but the shared generator in packages/strategy-tests/src/lib.rs still uses take_random_amounts_with_range(...) at lines 1482, 1983, 2219, and 2518. That helper now hardcodes min_per_input = 1 (see addresses_with_balance.rs:384-390), so the resulting inputs can fall below the per-input minimum enforced by the new validate_structure() checks. Each call site then .expect(...)s successful construction (e.g., line 1497 for IdentityTopUpFromAddressesTransitionV0, plus the analogous IdentityCreateFromAddresses, AddressFundsTransfer, and AddressCreditWithdrawal paths), so any sub-minimum input/output draw will abort the entire strategy run with InputBelowMinimumError/OutputBelowMinimumError instead of skipping the candidate. The PR fixed one copy of the test generator but left external callers of the strategy-tests package on the old invariants and exposed to non-deterministic panics.
🤖 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/strategy-tests/src/lib.rs`:
- [BLOCKING] lines 1481-1497: `strategy-tests` still uses unconstrained selector and will panic under new client-side validation
The PR updated `packages/rs-drive-abci/tests/strategy_tests/strategy.rs` to call `take_random_amounts_with_range_and_min_per_input(...)` (passing the platform-version `min_input_amount`) before constructing address-funded transitions, but the shared generator in `packages/strategy-tests/src/lib.rs` still uses `take_random_amounts_with_range(...)` at lines 1482, 1983, 2219, and 2518. That helper now hardcodes `min_per_input = 1` (see `addresses_with_balance.rs:384-390`), so the resulting inputs can fall below the per-input minimum enforced by the new `validate_structure()` checks. Each call site then `.expect(...)`s successful construction (e.g., line 1497 for `IdentityTopUpFromAddressesTransitionV0`, plus the analogous `IdentityCreateFromAddresses`, `AddressFundsTransfer`, and `AddressCreditWithdrawal` paths), so any sub-minimum input/output draw will abort the entire strategy run with `InputBelowMinimumError`/`OutputBelowMinimumError` instead of skipping the candidate. The PR fixed one copy of the test generator but left external callers of the `strategy-tests` package on the old invariants and exposed to non-deterministic panics.
In `packages/rs-dpp/src/state_transition/state_transitions/address_funds/address_credit_withdrawal_transition/v0/v0_methods.rs`:
- [SUGGESTION] lines 66-78: Structure validation runs after async signing — invalid transitions still incur N signer round-trips
`validate_structure()` is called only after the loop that awaits `signer.sign_create_witness(...)` for every input. None of the structural checks (max inputs, fee strategy, output script, input/output amounts) actually depend on the witnesses; only the witness-count check does, and it is tautologically satisfied here because the loop produces exactly `inputs.len()` witnesses. The same pattern exists in `address_funds_transfer_transition/v0/v0_methods.rs`, `address_funding_from_asset_lock_transition/v0/v0_methods.rs`, `identity_create_from_addresses_transition/v0/v0_methods.rs`, and `identity_topup_from_addresses_transition/v0/v0_methods.rs`. When `Signer` is backed by a hardware wallet, secure enclave, or remote KMS — the SDK's primary use case — every input requires a user prompt or network round-trip. The whole point of this PR is to fail fast client-side before broadcast; failing fast *after* N HSM confirmations defeats much of the benefit. Add a pre-signing structure check (skipping only the witness-count assertion) in addition to the existing post-signing call.
- [SUGGESTION] lines 73-78: `errors.into_iter().next().unwrap()` pattern duplicated across 7+ sites and discards all but the first error
The same six-line block — call `validate_structure`/`validate_identity_public_keys_structure`, check `is_valid()`, then `validation_result.errors.into_iter().next().unwrap()` and wrap in `ProtocolError::ConsensusError(Box::new(...))` — appears in seven places across this PR (`address_credit_withdrawal`, `address_funding_from_asset_lock`, `address_funds_transfer`, `identity_create_from_addresses` (×2), `identity_credit_transfer_to_addresses`, `identity_topup_from_addresses`, `identity_update`). Two issues: (1) The `unwrap()` is technically safe today because `ValidationResult::is_valid()` is defined as `errors.is_empty()`, but it relies on a non-local invariant; a future refactor that lets a result be "invalid with no errors" would silently introduce a panic. (2) Discarding every error after the first is user-hostile — a transition with two structural problems requires two round-trips to discover both. A small `pub(crate) fn first_error_as_protocol_error(result: SimpleConsensusValidationResult) -> Option<ProtocolError>` (or a variant returning all errors) in `state_transition/mod.rs` would deduplicate the boilerplate and remove the unwrap.
In `packages/rs-drive-abci/tests/strategy_tests/test_cases/address_tests.rs`:
- [SUGGESTION] lines 882-898: BLAST trunk query assertions relaxed from exact match to inequalities
Previously these tests asserted `trunk_result.elements.len() == 32` and `trunk_result.chunk_depths == vec![6]` exactly. The PR relaxes them to `>= 16` and `chunk_depths.len() == 1 && chunk_depths[0] >= 5`. The same relaxation appears at lines 1080-1098 and 2251-2269. The change is needed because the new client-side validation causes the strategy to generate fewer/larger inputs and a different set of persisted addresses, but accepting a wide range silently makes it possible for real BLAST sync regressions (e.g., a tree-shape change that halves expected element count) to pass unnoticed. Pin the strategy seed/parameters to produce a deterministic count under the new validation rules and assert that exact value instead.
In `packages/rs-dpp/src/state_transition/state_transitions/identity/identity_update_transition/v0/v0_methods.rs`:
- [SUGGESTION] lines 55-73: No rs-dpp unit test for the new client-side key validation in IdentityUpdateTransition
The PR description highlights the TRANSFER+HIGH security-level case as the original motivating bug, but there is no rs-dpp unit test asserting that `IdentityUpdateTransitionV0::try_from_identity_with_signer` returns a `ProtocolError::ConsensusError(InvalidIdentityPublicKeySecurityLevelError)` when an invalid combination is passed. Coverage relies on rs-drive-abci integration tests and the existing `validate_identity_public_keys_structure_v0` unit tests (which test the helper, not the new SDK call site). A small async test that constructs an `Identity` with a TRANSFER/HIGH key and asserts the error variant would lock in the user-visible contract this PR is establishing.
| } | ||
| address_credit_withdrawal_transition.input_witnesses = input_witnesses; | ||
|
|
||
| // Validate the fully-constructed transition structure | ||
| let validation_result = | ||
| address_credit_withdrawal_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))); | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: Structure validation runs after async signing — invalid transitions still incur N signer round-trips
validate_structure() is called only after the loop that awaits signer.sign_create_witness(...) for every input. None of the structural checks (max inputs, fee strategy, output script, input/output amounts) actually depend on the witnesses; only the witness-count check does, and it is tautologically satisfied here because the loop produces exactly inputs.len() witnesses. The same pattern exists in address_funds_transfer_transition/v0/v0_methods.rs, address_funding_from_asset_lock_transition/v0/v0_methods.rs, identity_create_from_addresses_transition/v0/v0_methods.rs, and identity_topup_from_addresses_transition/v0/v0_methods.rs. When Signer is backed by a hardware wallet, secure enclave, or remote KMS — the SDK's primary use case — every input requires a user prompt or network round-trip. The whole point of this PR is to fail fast client-side before broadcast; failing fast after N HSM confirmations defeats much of the benefit. Add a pre-signing structure check (skipping only the witness-count assertion) in addition to the existing post-signing call.
source: ['claude']
🤖 Fix this 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/address_funds/address_credit_withdrawal_transition/v0/v0_methods.rs`:
- [SUGGESTION] lines 66-78: Structure validation runs after async signing — invalid transitions still incur N signer round-trips
`validate_structure()` is called only after the loop that awaits `signer.sign_create_witness(...)` for every input. None of the structural checks (max inputs, fee strategy, output script, input/output amounts) actually depend on the witnesses; only the witness-count check does, and it is tautologically satisfied here because the loop produces exactly `inputs.len()` witnesses. The same pattern exists in `address_funds_transfer_transition/v0/v0_methods.rs`, `address_funding_from_asset_lock_transition/v0/v0_methods.rs`, `identity_create_from_addresses_transition/v0/v0_methods.rs`, and `identity_topup_from_addresses_transition/v0/v0_methods.rs`. When `Signer` is backed by a hardware wallet, secure enclave, or remote KMS — the SDK's primary use case — every input requires a user prompt or network round-trip. The whole point of this PR is to fail fast client-side before broadcast; failing fast *after* N HSM confirmations defeats much of the benefit. Add a pre-signing structure check (skipping only the witness-count assertion) in addition to the existing post-signing call.
| let validation_result = | ||
| address_credit_withdrawal_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))); | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: errors.into_iter().next().unwrap() pattern duplicated across 7+ sites and discards all but the first error
The same six-line block — call validate_structure/validate_identity_public_keys_structure, check is_valid(), then validation_result.errors.into_iter().next().unwrap() and wrap in ProtocolError::ConsensusError(Box::new(...)) — appears in seven places across this PR (address_credit_withdrawal, address_funding_from_asset_lock, address_funds_transfer, identity_create_from_addresses (×2), identity_credit_transfer_to_addresses, identity_topup_from_addresses, identity_update). Two issues: (1) The unwrap() is technically safe today because ValidationResult::is_valid() is defined as errors.is_empty(), but it relies on a non-local invariant; a future refactor that lets a result be "invalid with no errors" would silently introduce a panic. (2) Discarding every error after the first is user-hostile — a transition with two structural problems requires two round-trips to discover both. A small pub(crate) fn first_error_as_protocol_error(result: SimpleConsensusValidationResult) -> Option<ProtocolError> (or a variant returning all errors) in state_transition/mod.rs would deduplicate the boilerplate and remove the unwrap.
source: ['claude']
🤖 Fix this 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/address_funds/address_credit_withdrawal_transition/v0/v0_methods.rs`:
- [SUGGESTION] lines 73-78: `errors.into_iter().next().unwrap()` pattern duplicated across 7+ sites and discards all but the first error
The same six-line block — call `validate_structure`/`validate_identity_public_keys_structure`, check `is_valid()`, then `validation_result.errors.into_iter().next().unwrap()` and wrap in `ProtocolError::ConsensusError(Box::new(...))` — appears in seven places across this PR (`address_credit_withdrawal`, `address_funding_from_asset_lock`, `address_funds_transfer`, `identity_create_from_addresses` (×2), `identity_credit_transfer_to_addresses`, `identity_topup_from_addresses`, `identity_update`). Two issues: (1) The `unwrap()` is technically safe today because `ValidationResult::is_valid()` is defined as `errors.is_empty()`, but it relies on a non-local invariant; a future refactor that lets a result be "invalid with no errors" would silently introduce a panic. (2) Discarding every error after the first is user-hostile — a transition with two structural problems requires two round-trips to discover both. A small `pub(crate) fn first_error_as_protocol_error(result: SimpleConsensusValidationResult) -> Option<ProtocolError>` (or a variant returning all errors) in `state_transition/mod.rs` would deduplicate the boilerplate and remove the unwrap.
| assert_eq!(root_hash.len(), 32, "root hash should be 32 bytes"); | ||
|
|
||
| // Verify trunk query results | ||
| assert_eq!( | ||
| trunk_result.elements.len(), | ||
| 32, | ||
| "trunk query should return 32 elements" | ||
| assert!( | ||
| trunk_result.elements.len() >= 16, | ||
| "trunk query should return at least 16 elements, got {}", | ||
| trunk_result.elements.len() | ||
| ); | ||
| assert_eq!( | ||
| trunk_result.leaf_keys.len(), | ||
| 0, | ||
| "trunk query should return 0 leaf keys" | ||
| ); | ||
| assert_eq!( | ||
| trunk_result.chunk_depths, | ||
| vec![6], | ||
| "trunk query should have chunk_depths [6]" | ||
| assert!( | ||
| trunk_result.chunk_depths.len() == 1 && trunk_result.chunk_depths[0] >= 5, | ||
| "trunk query chunk_depths should have 1 element >= 5, got {:?}", | ||
| trunk_result.chunk_depths |
There was a problem hiding this comment.
🟡 Suggestion: BLAST trunk query assertions relaxed from exact match to inequalities
Previously these tests asserted trunk_result.elements.len() == 32 and trunk_result.chunk_depths == vec![6] exactly. The PR relaxes them to >= 16 and chunk_depths.len() == 1 && chunk_depths[0] >= 5. The same relaxation appears at lines 1080-1098 and 2251-2269. The change is needed because the new client-side validation causes the strategy to generate fewer/larger inputs and a different set of persisted addresses, but accepting a wide range silently makes it possible for real BLAST sync regressions (e.g., a tree-shape change that halves expected element count) to pass unnoticed. Pin the strategy seed/parameters to produce a deterministic count under the new validation rules and assert that exact value instead.
source: ['claude']
🤖 Fix this 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-drive-abci/tests/strategy_tests/test_cases/address_tests.rs`:
- [SUGGESTION] lines 882-898: BLAST trunk query assertions relaxed from exact match to inequalities
Previously these tests asserted `trunk_result.elements.len() == 32` and `trunk_result.chunk_depths == vec![6]` exactly. The PR relaxes them to `>= 16` and `chunk_depths.len() == 1 && chunk_depths[0] >= 5`. The same relaxation appears at lines 1080-1098 and 2251-2269. The change is needed because the new client-side validation causes the strategy to generate fewer/larger inputs and a different set of persisted addresses, but accepting a wide range silently makes it possible for real BLAST sync regressions (e.g., a tree-shape change that halves expected element count) to pass unnoticed. Pin the strategy seed/parameters to produce a deterministic count under the new validation rules and assert that exact value instead.
| input_witnesses_count: usize, | ||
| ) -> StateTransition { | ||
| let witnesses: Vec<AddressWitness> = (0..input_witnesses_count) | ||
| .map(|_| create_dummy_witness()) | ||
| .collect(); | ||
| AddressFundsTransferTransition::V0(AddressFundsTransferTransitionV0 { | ||
| let mut transition = AddressFundsTransferTransitionV0 { | ||
| inputs, | ||
| outputs, | ||
| fee_strategy, | ||
| user_fee_increase: 0, | ||
| input_witnesses: witnesses, | ||
| }) | ||
| .into() | ||
| input_witnesses: vec![], | ||
| }; | ||
|
|
||
| let signable_bytes = StateTransition::AddressFundsTransfer( | ||
| AddressFundsTransferTransition::V0(transition.clone()), | ||
| ) | ||
| .signable_bytes() | ||
| .expect("should create signable bytes"); | ||
|
|
||
| // Recover deterministic test keys (seeded as [i; 32]) and sign any matching inputs. | ||
| let mut deterministic_signer = TestAddressSigner::new(); | ||
| for i in 0u8..=255u8 { | ||
| deterministic_signer.add_p2pkh([i; 32]); | ||
| } | ||
|
|
||
| let input_addresses: Vec<PlatformAddress> = transition.inputs.keys().cloned().collect(); | ||
| let witnesses: Vec<AddressWitness> = (0..input_witnesses_count) | ||
| .map(|idx| { | ||
| input_addresses | ||
| .get(idx) | ||
| .and_then(|address| { | ||
| if deterministic_signer.can_sign_with(address) { | ||
| run_immediate( | ||
| deterministic_signer.sign_create_witness(address, &signable_bytes), | ||
| ) | ||
| .ok() | ||
| } else { | ||
| None |
There was a problem hiding this comment.
💬 Nitpick: Brittle 0..=255 brute-force key recovery in create_raw_transition_with_dummy_witnesses
The new helper rebuilds a TestAddressSigner and seeds it with every byte 0..=255 (deterministic_signer.add_p2pkh([i; 32])) so it can later look up matching keys for arbitrary input addresses. This is 256 fresh key derivations per call, a brittle coupling to the test-key seeding convention used elsewhere, and silently falls back to create_dummy_witness for any address whose seed isn't in 0..=255. The fall-through path means a test that intends to hit "witness count mismatch" can instead silently produce a partly-valid signed transition if the address happens to match a derived key. Take the signer (or the seeds used to construct inputs) as an explicit parameter so callers opt in to which addresses get real witnesses.
source: ['claude']
| fn run_immediate<F: Future>(future: F) -> F::Output { | ||
| struct NoopWake; | ||
|
|
||
| impl Wake for NoopWake { | ||
| fn wake(self: Arc<Self>) {} | ||
| } | ||
|
|
||
| let waker = Waker::from(Arc::new(NoopWake)); | ||
| let mut future = std::pin::pin!(future); | ||
| match future.as_mut().poll(&mut Context::from_waker(&waker)) { | ||
| Poll::Ready(output) => output, | ||
| Poll::Pending => panic!("expected signer future to complete immediately"), | ||
| } | ||
| } |
There was a problem hiding this comment.
💬 Nitpick: run_immediate test helper duplicated verbatim across two test modules
The hand-rolled futures executor run_immediate (with its NoopWake stub) is copy-pasted between address_credit_withdrawal/tests.rs:46 and address_funds_transfer/tests.rs:40. Both copies are identical — same NoopWake struct, same Pending => panic!("expected signer future to complete immediately"). The TestAddressSigner::sign_create_witness future is genuinely synchronous, so this is fine, but the duplication invites drift if any caller's signer becomes truly async. Lift it into a shared test-helper module (e.g., the existing test_helpers module already imported in both files) and import from both sites.
source: ['claude']
| let add_public_keys_in_creation: Vec<IdentityPublicKeyInCreation> = add_public_keys | ||
| .iter() | ||
| .map(|public_key| public_key.into()) | ||
| .collect(); | ||
|
|
||
| // Validate public key structure (purpose/security level compatibility) | ||
| // before broadcasting, so invalid combinations are caught client-side | ||
| // rather than being rejected by the network. | ||
| let validation_result = | ||
| IdentityPublicKeyInCreation::validate_identity_public_keys_structure( | ||
| &add_public_keys_in_creation, | ||
| false, // not in create_identity context | ||
| platform_version, | ||
| )?; | ||
| if !validation_result.is_valid() { | ||
| // Return the first validation error as a ProtocolError | ||
| let first_error = validation_result.errors.into_iter().next().unwrap(); | ||
| return Err(ProtocolError::ConsensusError(Box::new(first_error))); | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: No rs-dpp unit test for the new client-side key validation in IdentityUpdateTransition
The PR description highlights the TRANSFER+HIGH security-level case as the original motivating bug, but there is no rs-dpp unit test asserting that IdentityUpdateTransitionV0::try_from_identity_with_signer returns a ProtocolError::ConsensusError(InvalidIdentityPublicKeySecurityLevelError) when an invalid combination is passed. Coverage relies on rs-drive-abci integration tests and the existing validate_identity_public_keys_structure_v0 unit tests (which test the helper, not the new SDK call site). A small async test that constructs an Identity with a TRANSFER/HIGH key and asserts the error variant would lock in the user-visible contract this PR is establishing.
source: ['claude']
🤖 Fix this 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_update_transition/v0/v0_methods.rs`:
- [SUGGESTION] lines 55-73: No rs-dpp unit test for the new client-side key validation in IdentityUpdateTransition
The PR description highlights the TRANSFER+HIGH security-level case as the original motivating bug, but there is no rs-dpp unit test asserting that `IdentityUpdateTransitionV0::try_from_identity_with_signer` returns a `ProtocolError::ConsensusError(InvalidIdentityPublicKeySecurityLevelError)` when an invalid combination is passed. Coverage relies on rs-drive-abci integration tests and the existing `validate_identity_public_keys_structure_v0` unit tests (which test the helper, not the new SDK call site). A small async test that constructs an `Identity` with a TRANSFER/HIGH key and asserts the error variant would lock in the user-visible contract this PR is establishing.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
PR adds client-side structural validation to SDK state-transition constructors using validate_structure() with placeholder witnesses. The change is purely additive, doesn't touch signed payloads or consensus logic, and security/correctness review is clean. Findings center on quality concerns: a redundant post-signing validation call, fragile coupling via placeholder witnesses, and incomplete direct test coverage of the new constructor-level checks.
Reviewed commit: d2b0f17
🟡 3 suggestion(s) | 💬 1 nitpick(s)
🤖 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/address_funds/address_funds_transfer_transition/v0/v0_methods.rs`:
- [SUGGESTION] lines 75-79: Post-signing validate_structure() is redundant after a successful pre-signing call
After the pre-signing block succeeds with placeholder witnesses (count = inputs.len()), the signing loop pushes exactly one witness per `inputs.keys()` entry, so the witness count is guaranteed to match again. `validate_structure` for these transitions only inspects `input_witnesses.len()` (verified in `state_transition_validation.rs`), never witness contents. No other validated field is mutated between the two calls. The post-signing call therefore re-runs HashSet allocation, fee_strategy iteration, overflow checks, and balance checks without any way to surface a new failure mode. This duplication is repeated across all five address-based transitions: address_funds_transfer_transition (line 76), address_credit_withdrawal_transition (~88), address_funding_from_asset_lock_transition (~84), identity_topup_from_addresses_transition (~78), and identity_create_from_addresses_transition (~135). Drop the second call, or — better — split the witness-count check out of validate_structure() into a dedicated function so the pre-signing path validates everything except the count, and the post-signing path validates only the count. That removes the placeholder-witness shuffle entirely.
In `packages/rs-dpp/src/address_funds/witness.rs`:
- [SUGGESTION] lines 40-51: Placeholder-witness pattern couples constructors to undocumented validate_structure internals
Five constructors now call `AddressWitness::placeholder()` solely to satisfy the witness-count invariant inside `validate_structure()`, then clear the field. This works only because no validator currently inspects witness contents. The invariant is not enforced by the type system or test suite — if any future change adds even mild per-witness checks (signature length per variant, P2SH script-type checks, multisig structure validation), every client constructor using this pattern will silently start rejecting otherwise-valid transitions because the empty P2pkh signature will fail the new check. The server-side validator would still accept those transitions, so the divergence would only appear client-side. Two durable alternatives: (a) split `validate_structure()` into `validate_structure_pre_witnesses()` (no witness checks) and a tiny `validate_witness_count()` helper, and have constructors call only the first pre-signing — eliminating the need for `AddressWitness::placeholder()`; or (b) at minimum, document the witness-count-only contract on `AddressWitness::placeholder()` and on each `validate_structure` impl. Option (a) also lets the helper be removed, which is desirable since it produces semantically-invalid witnesses with no domain purpose.
In `packages/rs-dpp/src/state_transition/state_transitions/identity/identity_update_transition/v0/mod.rs`:
- [SUGGESTION] lines 257-372: Only one of three new constructor-level public-key validation paths has a direct regression test
This PR adds the same client-side `validate_identity_public_keys_structure()` gate to three constructors (IdentityUpdateTransitionV0, IdentityCreateTransitionV0, IdentityCreateFromAddressesTransitionV0), but the new direct constructor-level rejection test exists only for `IdentityUpdateTransitionV0::try_from_identity_with_signer()`, and only covers the TRANSFER+HIGH rule. The other rule classes the validator enforces (ENCRYPTION/DECRYPTION must be MEDIUM, no duplicate key IDs/data, max key count) and the other two constructors have no equivalent direct test, so a future refactor could remove or reorder the pre-signing check in IdentityCreate* without failing CI — the server-side validator would still reject the transition later, defeating the purpose of fail-fast client validation. Add at least one direct constructor-level test per constructor (with an unreachable signer asserting the consensus error returns before any signing), and ideally a parametrized test covering each rule category.
| // Validate the fully-constructed transition structure | ||
| let validation_result = address_funds_transition.validate_structure(platform_version); | ||
| if let Some(error) = first_consensus_error_as_protocol_error(validation_result) { | ||
| return Err(error); | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: Post-signing validate_structure() is redundant after a successful pre-signing call
After the pre-signing block succeeds with placeholder witnesses (count = inputs.len()), the signing loop pushes exactly one witness per inputs.keys() entry, so the witness count is guaranteed to match again. validate_structure for these transitions only inspects input_witnesses.len() (verified in state_transition_validation.rs), never witness contents. No other validated field is mutated between the two calls. The post-signing call therefore re-runs HashSet allocation, fee_strategy iteration, overflow checks, and balance checks without any way to surface a new failure mode. This duplication is repeated across all five address-based transitions: address_funds_transfer_transition (line 76), address_credit_withdrawal_transition (~88), address_funding_from_asset_lock_transition (~84), identity_topup_from_addresses_transition (~78), and identity_create_from_addresses_transition (~135). Drop the second call, or — better — split the witness-count check out of validate_structure() into a dedicated function so the pre-signing path validates everything except the count, and the post-signing path validates only the count. That removes the placeholder-witness shuffle entirely.
source: ['claude']
🤖 Fix this 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/address_funds/address_funds_transfer_transition/v0/v0_methods.rs`:
- [SUGGESTION] lines 75-79: Post-signing validate_structure() is redundant after a successful pre-signing call
After the pre-signing block succeeds with placeholder witnesses (count = inputs.len()), the signing loop pushes exactly one witness per `inputs.keys()` entry, so the witness count is guaranteed to match again. `validate_structure` for these transitions only inspects `input_witnesses.len()` (verified in `state_transition_validation.rs`), never witness contents. No other validated field is mutated between the two calls. The post-signing call therefore re-runs HashSet allocation, fee_strategy iteration, overflow checks, and balance checks without any way to surface a new failure mode. This duplication is repeated across all five address-based transitions: address_funds_transfer_transition (line 76), address_credit_withdrawal_transition (~88), address_funding_from_asset_lock_transition (~84), identity_topup_from_addresses_transition (~78), and identity_create_from_addresses_transition (~135). Drop the second call, or — better — split the witness-count check out of validate_structure() into a dedicated function so the pre-signing path validates everything except the count, and the post-signing path validates only the count. That removes the placeholder-witness shuffle entirely.
| impl AddressWitness { | ||
| /// Returns a placeholder `P2pkh` witness with an empty signature. | ||
| /// | ||
| /// This is intended for pre-signing structural validation, where the | ||
| /// witness-count invariant must be satisfied even though real signatures | ||
| /// have not yet been computed. Placeholder witnesses MUST be replaced with | ||
| /// real witnesses before the transition is broadcast or accepted. | ||
| pub(crate) fn placeholder() -> Self { | ||
| AddressWitness::P2pkh { | ||
| signature: BinaryData::new(Vec::new()), | ||
| } | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: Placeholder-witness pattern couples constructors to undocumented validate_structure internals
Five constructors now call AddressWitness::placeholder() solely to satisfy the witness-count invariant inside validate_structure(), then clear the field. This works only because no validator currently inspects witness contents. The invariant is not enforced by the type system or test suite — if any future change adds even mild per-witness checks (signature length per variant, P2SH script-type checks, multisig structure validation), every client constructor using this pattern will silently start rejecting otherwise-valid transitions because the empty P2pkh signature will fail the new check. The server-side validator would still accept those transitions, so the divergence would only appear client-side. Two durable alternatives: (a) split validate_structure() into validate_structure_pre_witnesses() (no witness checks) and a tiny validate_witness_count() helper, and have constructors call only the first pre-signing — eliminating the need for AddressWitness::placeholder(); or (b) at minimum, document the witness-count-only contract on AddressWitness::placeholder() and on each validate_structure impl. Option (a) also lets the helper be removed, which is desirable since it produces semantically-invalid witnesses with no domain purpose.
source: ['claude', 'codex']
🤖 Fix this 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/address_funds/witness.rs`:
- [SUGGESTION] lines 40-51: Placeholder-witness pattern couples constructors to undocumented validate_structure internals
Five constructors now call `AddressWitness::placeholder()` solely to satisfy the witness-count invariant inside `validate_structure()`, then clear the field. This works only because no validator currently inspects witness contents. The invariant is not enforced by the type system or test suite — if any future change adds even mild per-witness checks (signature length per variant, P2SH script-type checks, multisig structure validation), every client constructor using this pattern will silently start rejecting otherwise-valid transitions because the empty P2pkh signature will fail the new check. The server-side validator would still accept those transitions, so the divergence would only appear client-side. Two durable alternatives: (a) split `validate_structure()` into `validate_structure_pre_witnesses()` (no witness checks) and a tiny `validate_witness_count()` helper, and have constructors call only the first pre-signing — eliminating the need for `AddressWitness::placeholder()`; or (b) at minimum, document the witness-count-only contract on `AddressWitness::placeholder()` and on each `validate_structure` impl. Option (a) also lets the helper be removed, which is desirable since it produces semantically-invalid witnesses with no domain purpose.
| /// Verifies that `try_from_identity_with_signer` rejects an invalid TRANSFER+HIGH | ||
| /// added key client-side via the structural public-key validation, returning | ||
| /// `ProtocolError::ConsensusError(InvalidIdentityPublicKeySecurityLevelError)` | ||
| /// before any signing work is attempted. | ||
| #[cfg(feature = "state-transition-signing")] | ||
| #[tokio::test] | ||
| async fn try_from_identity_with_signer_rejects_transfer_high_added_key() { | ||
| use crate::address_funds::AddressWitness; | ||
| use crate::consensus::basic::BasicError; | ||
| use crate::consensus::ConsensusError; | ||
| use crate::identity::identity_public_key::v0::IdentityPublicKeyV0; | ||
| use crate::identity::signer::Signer; | ||
| use crate::identity::v0::IdentityV0; | ||
| use crate::identity::{Identity, IdentityPublicKey, KeyType, Purpose, SecurityLevel}; | ||
| use crate::state_transition::identity_update_transition::methods::IdentityUpdateTransitionMethodsV0; | ||
| use crate::version::PlatformVersion; | ||
| use crate::ProtocolError; | ||
| use async_trait::async_trait; | ||
| use std::collections::BTreeMap; | ||
|
|
||
| /// A signer that should never be invoked: pre-signing validation must fail | ||
| /// before this signer is asked to sign anything. | ||
| #[derive(Debug)] | ||
| struct UnreachableSigner; | ||
|
|
||
| #[async_trait] | ||
| impl Signer<IdentityPublicKey> for UnreachableSigner { | ||
| async fn sign( | ||
| &self, | ||
| _key: &IdentityPublicKey, | ||
| _data: &[u8], | ||
| ) -> Result<BinaryData, ProtocolError> { | ||
| panic!("UnreachableSigner::sign must not be called when pre-signing validation rejects the transition"); | ||
| } | ||
|
|
||
| async fn sign_create_witness( | ||
| &self, | ||
| _key: &IdentityPublicKey, | ||
| _data: &[u8], | ||
| ) -> Result<AddressWitness, ProtocolError> { | ||
| panic!("UnreachableSigner::sign_create_witness must not be called when pre-signing validation rejects the transition"); | ||
| } | ||
|
|
||
| fn can_sign_with(&self, _key: &IdentityPublicKey) -> bool { | ||
| false | ||
| } | ||
| } | ||
|
|
||
| let platform_version = PlatformVersion::latest(); | ||
|
|
||
| // Master key on the existing identity (not used here, but the constructor expects | ||
| // an identity to read id/revision from). | ||
| let master_key: IdentityPublicKey = IdentityPublicKeyV0 { | ||
| id: 0, | ||
| purpose: Purpose::AUTHENTICATION, | ||
| security_level: SecurityLevel::MASTER, | ||
| contract_bounds: None, | ||
| key_type: KeyType::ECDSA_SECP256K1, | ||
| read_only: false, | ||
| data: BinaryData::new(vec![0u8; 33]), | ||
| disabled_at: None, | ||
| } | ||
| .into(); | ||
|
|
||
| let identity: Identity = IdentityV0 { | ||
| id: Identifier::default(), | ||
| public_keys: BTreeMap::from([(0, master_key)]), | ||
| balance: 0, | ||
| revision: 0, | ||
| } | ||
| .into(); | ||
|
|
||
| // Invalid combination: TRANSFER purpose only allows CRITICAL security level. | ||
| let invalid_transfer_high_key: IdentityPublicKey = IdentityPublicKeyV0 { | ||
| id: 1, | ||
| purpose: Purpose::TRANSFER, | ||
| security_level: SecurityLevel::HIGH, | ||
| contract_bounds: None, | ||
| key_type: KeyType::ECDSA_SECP256K1, | ||
| read_only: false, | ||
| data: BinaryData::new(vec![1u8; 33]), | ||
| disabled_at: None, | ||
| } | ||
| .into(); | ||
|
|
||
| let result = IdentityUpdateTransitionV0::try_from_identity_with_signer( | ||
| &identity, | ||
| &0, | ||
| vec![invalid_transfer_high_key], | ||
| vec![], | ||
| 1, | ||
| 0, | ||
| &UnreachableSigner, | ||
| platform_version, | ||
| None, | ||
| ) | ||
| .await; | ||
|
|
||
| match result { | ||
| Err(ProtocolError::ConsensusError(boxed)) => match *boxed { | ||
| ConsensusError::BasicError( | ||
| BasicError::InvalidIdentityPublicKeySecurityLevelError(err), | ||
| ) => { | ||
| assert_eq!(err.purpose(), Purpose::TRANSFER); | ||
| assert_eq!(err.security_level(), SecurityLevel::HIGH); | ||
| } | ||
| other => panic!( | ||
| "expected InvalidIdentityPublicKeySecurityLevelError, got {:?}", | ||
| other | ||
| ), | ||
| }, | ||
| other => panic!( | ||
| "expected ConsensusError(InvalidIdentityPublicKeySecurityLevelError), got {:?}", | ||
| other | ||
| ), | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: Only one of three new constructor-level public-key validation paths has a direct regression test
This PR adds the same client-side validate_identity_public_keys_structure() gate to three constructors (IdentityUpdateTransitionV0, IdentityCreateTransitionV0, IdentityCreateFromAddressesTransitionV0), but the new direct constructor-level rejection test exists only for IdentityUpdateTransitionV0::try_from_identity_with_signer(), and only covers the TRANSFER+HIGH rule. The other rule classes the validator enforces (ENCRYPTION/DECRYPTION must be MEDIUM, no duplicate key IDs/data, max key count) and the other two constructors have no equivalent direct test, so a future refactor could remove or reorder the pre-signing check in IdentityCreate* without failing CI — the server-side validator would still reject the transition later, defeating the purpose of fail-fast client validation. Add at least one direct constructor-level test per constructor (with an unreachable signer asserting the consensus error returns before any signing), and ideally a parametrized test covering each rule category.
source: ['claude', 'codex']
🤖 Fix this 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_update_transition/v0/mod.rs`:
- [SUGGESTION] lines 257-372: Only one of three new constructor-level public-key validation paths has a direct regression test
This PR adds the same client-side `validate_identity_public_keys_structure()` gate to three constructors (IdentityUpdateTransitionV0, IdentityCreateTransitionV0, IdentityCreateFromAddressesTransitionV0), but the new direct constructor-level rejection test exists only for `IdentityUpdateTransitionV0::try_from_identity_with_signer()`, and only covers the TRANSFER+HIGH rule. The other rule classes the validator enforces (ENCRYPTION/DECRYPTION must be MEDIUM, no duplicate key IDs/data, max key count) and the other two constructors have no equivalent direct test, so a future refactor could remove or reorder the pre-signing check in IdentityCreate* without failing CI — the server-side validator would still reject the transition later, defeating the purpose of fail-fast client validation. Add at least one direct constructor-level test per constructor (with an unreachable signer asserting the consensus error returns before any signing), and ideally a parametrized test covering each rule category.
| pub fn first_consensus_error_as_protocol_error( | ||
| result: SimpleConsensusValidationResult, | ||
| ) -> Option<ProtocolError> { | ||
| result | ||
| .errors | ||
| .into_iter() | ||
| .next() | ||
| .map(|error| ProtocolError::ConsensusError(Box::new(error))) | ||
| } |
There was a problem hiding this comment.
💬 Nitpick: first_consensus_error_as_protocol_error silently drops trailing errors
first_consensus_error_as_protocol_error returns result.errors.into_iter().next(). Today every validate_structure impl in this crate is early-return style and produces at most one error, so the practical impact is nil. Since this PR introduces a named, reusable helper that hides this truncation behind a friendly name, document it explicitly in the rustdoc (e.g. "any errors after the first are discarded") so a future validator that accumulates errors doesn't silently lose diagnostics through this helper.
source: ['claude']
thepastaclaw
force-pushed
the
fix/validate-transfer-key-security-level-client-side
branch
from
d2b0f17 to
300c3cb
Compare
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Follow-up commit cleanly addresses prior blocking concerns: validate_structure split into pre- and post-signing halves, placeholder witnesses removed, unwrap() panic surface replaced with first_consensus_error_as_protocol_error helper, BLAST trunk assertions tightened, min_input_amount threaded through all four shared call sites, and rs-dpp constructor unit tests added. Remaining issues are confined to test/strategy generators that the new client-side validation can now make panic on certain configurations, plus minor test-helper duplications. No production/consensus/security blockers.
Reviewed commit: 300c3cb
🟡 3 suggestion(s) | 💬 3 nitpick(s)
2 additional findings
🟡 suggestion: Shared AddressTransfer generator does not cap output_count by min_output_amount
packages/strategy-tests/src/lib.rs (lines 2261-2263)
The in-tree drive-abci copy at packages/rs-drive-abci/tests/strategy_tests/strategy.rs:2466-2468 was updated to cap output_count by (total_input / min_per_output).max(1) so each output meets platform_version.dpp.state_transitions.address_funds.min_output_amount. The published strategy-tests crate at this site still does let output_count = rng.gen_range(output_count_range.clone()).max(1) as usize; with no such cap. After this PR AddressFundsTransferTransition::try_from_inputs_with_signer enforces min_output_amount per output, so any caller whose amount_range lower bound sits between min_input_amount (100_000) and output_count_range.start * min_output_amount (e.g. amount_range = 200_000..=400_000, output_count_range = 1..=4) will draw a total_input whose even split falls below 500_000, and the .expect("expected to create address funds transfer transition") at lines 2521-2531 panics. The companion fix only landed on the in-tree drive-abci path; mirror it here for parity with strategy.rs:2466-2468.
🟡 suggestion: create_identity_credit_transfer_to_addresses_transition can build sub-minimum outputs that now panic in the validated constructor
packages/strategy-tests/src/transitions.rs (lines 958-980)
This helper still treats output_count.max(1) as the only constraint and computes amount_per_output = total_amount / output_count without checking against min_output_amount. After this PR, IdentityCreditTransferToAddressesTransition::try_from_identity runs validate_structure() and rejects any recipient amount below platform_version.dpp.state_transitions.address_funds.min_output_amount with OutputBelowMinimumError. Because the helper immediately .expect("expected to create transfer to addresses transition") at lines 969-980, the failure is a panic in shared test infrastructure. The random identity-transfer path in packages/rs-drive-abci/tests/strategy_tests/strategy.rs:1531-1548 feeds arbitrary amount_range and output_count_range into this helper, so the new fail-fast validation has exposed a real logic hole. Either return None and skip the candidate when total_amount / output_count < min_output_amount, or cap output_count by (total_amount / min_output_amount).max(1) and document the floor.
🤖 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/strategy-tests/src/lib.rs`:
- [SUGGESTION] lines 2261-2263: Shared AddressTransfer generator does not cap output_count by min_output_amount
The in-tree drive-abci copy at `packages/rs-drive-abci/tests/strategy_tests/strategy.rs:2466-2468` was updated to cap `output_count` by `(total_input / min_per_output).max(1)` so each output meets `platform_version.dpp.state_transitions.address_funds.min_output_amount`. The published `strategy-tests` crate at this site still does `let output_count = rng.gen_range(output_count_range.clone()).max(1) as usize;` with no such cap. After this PR `AddressFundsTransferTransition::try_from_inputs_with_signer` enforces `min_output_amount` per output, so any caller whose `amount_range` lower bound sits between `min_input_amount` (100_000) and `output_count_range.start * min_output_amount` (e.g. `amount_range = 200_000..=400_000`, `output_count_range = 1..=4`) will draw a `total_input` whose even split falls below 500_000, and the `.expect("expected to create address funds transfer transition")` at lines 2521-2531 panics. The companion fix only landed on the in-tree drive-abci path; mirror it here for parity with `strategy.rs:2466-2468`.
In `packages/strategy-tests/src/transitions.rs`:
- [SUGGESTION] lines 958-980: create_identity_credit_transfer_to_addresses_transition can build sub-minimum outputs that now panic in the validated constructor
This helper still treats `output_count.max(1)` as the only constraint and computes `amount_per_output = total_amount / output_count` without checking against `min_output_amount`. After this PR, `IdentityCreditTransferToAddressesTransition::try_from_identity` runs `validate_structure()` and rejects any recipient amount below `platform_version.dpp.state_transitions.address_funds.min_output_amount` with `OutputBelowMinimumError`. Because the helper immediately `.expect("expected to create transfer to addresses transition")` at lines 969-980, the failure is a panic in shared test infrastructure. The random identity-transfer path in `packages/rs-drive-abci/tests/strategy_tests/strategy.rs:1531-1548` feeds arbitrary `amount_range` and `output_count_range` into this helper, so the new fail-fast validation has exposed a real logic hole. Either return `None` and skip the candidate when `total_amount / output_count < min_output_amount`, or cap `output_count` by `(total_amount / min_output_amount).max(1)` and document the floor.
In `packages/rs-drive-abci/tests/strategy_tests/strategy.rs`:
- [SUGGESTION] lines 2466-2468: max_outputs_by_amount.max(1) still manufactures an unsatisfiable single-output transfer when total_input < min_output_amount
`min_per_input` (100_000) is below `min_per_output` (500_000), so `take_random_amounts_with_range_and_min_per_input` can return inputs whose total falls in `[min_per_input, min_per_output)`. In that band `(total_input / min_per_output) = 0` and `.max(1)` clamps to 1, yielding `output_count = 1` with `amount_per_output = total_input < 500_000`. `validate_structure_without_input_witnesses` then fails with `OutputBelowMinimumError` and the `.expect(...)` at lines 2548-2557 panics. Current address tests use `dash_to_credits!(5)` so they don't trip this in CI, but any future test whose `amount_range.start()` is below `min_output_amount` will hit it deterministically. Either skip the candidate by returning `None` when `total_input < min_per_output`, or pass `min_per_output` (not `min_per_input`) as the per-input floor for AddressTransfer call sites.
| let max_outputs_by_amount = (total_input / min_per_output).max(1) as usize; | ||
| let output_count = | ||
| (rng.gen_range(output_count_range.clone()).max(1) as usize).min(max_outputs_by_amount); |
There was a problem hiding this comment.
🟡 Suggestion: max_outputs_by_amount.max(1) still manufactures an unsatisfiable single-output transfer when total_input < min_output_amount
min_per_input (100_000) is below min_per_output (500_000), so take_random_amounts_with_range_and_min_per_input can return inputs whose total falls in [min_per_input, min_per_output). In that band (total_input / min_per_output) = 0 and .max(1) clamps to 1, yielding output_count = 1 with amount_per_output = total_input < 500_000. validate_structure_without_input_witnesses then fails with OutputBelowMinimumError and the .expect(...) at lines 2548-2557 panics. Current address tests use dash_to_credits!(5) so they don't trip this in CI, but any future test whose amount_range.start() is below min_output_amount will hit it deterministically. Either skip the candidate by returning None when total_input < min_per_output, or pass min_per_output (not min_per_input) as the per-input floor for AddressTransfer call sites.
source: ['claude-general', 'codex-rust-quality']
🤖 Fix this 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-drive-abci/tests/strategy_tests/strategy.rs`:
- [SUGGESTION] lines 2466-2468: max_outputs_by_amount.max(1) still manufactures an unsatisfiable single-output transfer when total_input < min_output_amount
`min_per_input` (100_000) is below `min_per_output` (500_000), so `take_random_amounts_with_range_and_min_per_input` can return inputs whose total falls in `[min_per_input, min_per_output)`. In that band `(total_input / min_per_output) = 0` and `.max(1)` clamps to 1, yielding `output_count = 1` with `amount_per_output = total_input < 500_000`. `validate_structure_without_input_witnesses` then fails with `OutputBelowMinimumError` and the `.expect(...)` at lines 2548-2557 panics. Current address tests use `dash_to_credits!(5)` so they don't trip this in CI, but any future test whose `amount_range.start()` is below `min_output_amount` will hit it deterministically. Either skip the candidate by returning `None` when `total_input < min_per_output`, or pass `min_per_output` (not `min_per_input`) as the per-input floor for AddressTransfer call sites.
| fn run_immediate<F: Future>(future: F) -> F::Output { | ||
| struct NoopWake; | ||
|
|
||
| impl Wake for NoopWake { | ||
| fn wake(self: Arc<Self>) {} | ||
| } | ||
|
|
||
| let waker = Waker::from(Arc::new(NoopWake)); | ||
| let mut future = std::pin::pin!(future); | ||
| match future.as_mut().poll(&mut Context::from_waker(&waker)) { | ||
| Poll::Ready(output) => output, | ||
| Poll::Pending => panic!("expected signer future to complete immediately"), | ||
| } | ||
| } |
There was a problem hiding this comment.
💬 Nitpick: run_immediate / NoopWake helper is copy-pasted across two test modules
The hand-rolled futures executor run_immediate (with its NoopWake impl and Pending => panic!("expected signer future to complete immediately") arm) is identical at address_credit_withdrawal/tests.rs:46-58 and address_funds_transfer/tests.rs:40-53. The TestAddressSigner::sign_create_witness future is genuinely synchronous today so this works, but the duplication invites silent drift if either signer becomes truly async. Lift it into the existing test_helpers module and import from both sites.
source: ['claude-general', 'claude-rust-quality']
| fee_strategy: AddressFundsFeeStrategy, | ||
| input_witnesses_count: usize, | ||
| ) -> StateTransition { | ||
| let witnesses: Vec<AddressWitness> = (0..input_witnesses_count) | ||
| .map(|_| create_dummy_witness()) | ||
| .collect(); | ||
| AddressFundsTransferTransition::V0(AddressFundsTransferTransitionV0 { | ||
| let mut transition = AddressFundsTransferTransitionV0 { | ||
| inputs, | ||
| outputs, | ||
| fee_strategy, | ||
| user_fee_increase: 0, | ||
| input_witnesses: witnesses, | ||
| }) | ||
| .into() | ||
| input_witnesses: vec![], | ||
| }; | ||
|
|
||
| let signable_bytes = StateTransition::AddressFundsTransfer( | ||
| AddressFundsTransferTransition::V0(transition.clone()), | ||
| ) | ||
| .signable_bytes() | ||
| .expect("should create signable bytes"); | ||
|
|
||
| // Recover deterministic test keys (seeded as [i; 32]) and sign any matching inputs. | ||
| let mut deterministic_signer = TestAddressSigner::new(); | ||
| for i in 0u8..=255u8 { | ||
| deterministic_signer.add_p2pkh([i; 32]); | ||
| } | ||
|
|
||
| let input_addresses: Vec<PlatformAddress> = transition.inputs.keys().cloned().collect(); | ||
| let witnesses: Vec<AddressWitness> = (0..input_witnesses_count) | ||
| .map(|idx| { | ||
| input_addresses | ||
| .get(idx) | ||
| .and_then(|address| { | ||
| if deterministic_signer.can_sign_with(address) { | ||
| run_immediate( | ||
| deterministic_signer.sign_create_witness(address, &signable_bytes), | ||
| ) | ||
| .ok() | ||
| } else { | ||
| None | ||
| } | ||
| }) | ||
| .unwrap_or_else(create_dummy_witness) | ||
| }) | ||
| .collect(); | ||
|
|
||
| transition.input_witnesses = witnesses; | ||
| AddressFundsTransferTransition::V0(transition).into() | ||
| } |
There was a problem hiding this comment.
💬 Nitpick: create_raw_transition_with_dummy_witnesses brute-forces 256 P2PKH seeds and silently falls back to dummy witnesses
The helper rebuilds a fresh TestAddressSigner and seeds it with every byte 0..=255 (deterministic_signer.add_p2pkh([i; 32])) on each call so that arbitrary input addresses can be re-signed after the fact, then silently falls back to create_dummy_witness when no seed matches. A test that intends to exercise an explicit invalid scenario (witness-count mismatch, mis-signed witness, etc.) can instead silently produce a partly-valid signed transition if the caller's address happens to derive from one of the 256 seeds — masking the test's intent. Take the signer (or the seeds used to construct inputs) as an explicit parameter so callers opt in to which addresses get real witnesses, and remove the silent fallback.
source: ['claude-general', 'claude-rust-quality']
| let signable_bytes: Vec<u8> = StateTransition::from( | ||
| IdentityCreditTransferToAddressesTransition::V0(transition_v0.clone()), | ||
| ) | ||
| .signable_bytes() | ||
| .expect("should get signable bytes"); | ||
| let transfer_key = identity | ||
| .public_keys() | ||
| .get(&1) | ||
| .expect("transfer key should exist"); | ||
| transition_v0.signature = signer | ||
| .sign(transfer_key, &signable_bytes) | ||
| .await | ||
| .expect("should sign"); | ||
|
|
||
| let transition = StateTransition::from( | ||
| IdentityCreditTransferToAddressesTransition::V0(transition_v0), | ||
| ); | ||
| let transition_bytes = transition.serialize_to_bytes().expect("should serialize"); |
There was a problem hiding this comment.
💬 Nitpick: Three near-identical manual transition_v0 + signer.sign + StateTransition::from blocks added to bypass new client-side validation
Three places (around lines 4977, 5144, 5295) replace a single create_signed_transition(...) call with ~22 lines that build IdentityCreditTransferToAddressesTransitionV0 by hand, compute signable_bytes, look up key id 1, call signer.sign, and wrap as StateTransition. The duplication exists because try_from_identity now invokes validate_structure() and these tests deliberately construct invalid scenarios. Lift the manual path into a single helper (e.g. create_signed_transition_skipping_client_validation) so the three sites stay synchronized and the intent ("we want to bypass client-side validation here") is documented at one location.
source: ['claude-rust-quality']
thepastaclaw
force-pushed
the
fix/validate-transfer-key-security-level-client-side
branch
from
300c3cb to
70fe34d
Compare
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Follow-up commit 70fe34d cleanly resolves prior blocking concerns. Validation now splits pre-signing structure checks from post-signing witness-count checks across all five address transitions, the unwrap() panic surface is gone, min_input_amount is threaded through every shared call site, BLAST trunk assertions are pinned to exact counts, and rs-dpp now has direct UnreachableSigner unit tests for IdentityUpdate, IdentityCreate, and IdentityCreateFromAddresses. No production or consensus blockers remain. Remaining items are test-infrastructure quality issues confined to the strategy-tests crate and rs-drive-abci test modules.
Reviewed commit: 70fe34d
🟡 2 suggestion(s) | 💬 3 nitpick(s)
2 additional findings
🟡 suggestion: take_random_amounts_with_range_and_min_per_input can still pick a sub-min input when taken_total < range_min
packages/strategy-tests/src/addresses_with_balance.rs (lines 440-470)
The early-break at line 440 only fires once taken_total >= range_min. When taken_total < range_min and remaining_max < min_per_input (reachable when global_max - taken_total < min_per_input, e.g. range = 200_000..=220_000, min_per_input = 100_000, after a first pick of 150_000), step_min = remaining_to_min.max(min_per_input).min(remaining_max) clamps down to remaining_max, which is below min_per_input. step_max == step_min, so the guard at line 457 doesn't fire and take_random_amount_with_bounds(remaining_max, remaining_max, ...) returns an address with taken < min_per_input. The constructor's validate_structure() then rejects the input with InputBelowMinimumError, panicking at the .expect("expected to create address funds transfer transition") call sites in lib.rs and strategy.rs. Current CI seeds keep amount_range six orders of magnitude above min_per_input so this is not reachable today, but it defeats the helper's documented contract. Tighten the break to fire whenever remaining_max < min_per_input regardless of taken_total, or compute step_min = remaining_to_min.max(min_per_input) and break (returning None) when step_min > step_max.
🟡 suggestion: take_random_amounts_with_range_and_min_per_input mutates staged balances/nonces on its failure path and over-counts available funds
packages/strategy-tests/src/addresses_with_balance.rs (lines 407-492)
Two related issues in this helper, which now powers the new address-funded strategy paths in lib.rs (lines ~1486, 2013, 2258, 2587) where callers treat None as "no transition emitted" and simply break.
-
Failure path is not rollback-safe. Each successful
take_random_amount_with_bounds(...)immediately stages a reduced balance and incremented nonce. When a later iteration fails (maybe == Noneat 463-470) or the loop exits withtaken_total < range_min(lines 485-490), the function returnsNonewithout restoring earlier staged mutations. The in-source comment at 486-488 explicitly acknowledges this. Reachable example:min_per_input = 100_000, balances{150_000, 80_000}, range200_000..=300_000— first pick stages 150k, second is impossible, helper returnsNonewhile leaving the first address consumed. Subsequent strategy steps then operate on silently corrupted balances and nonces. -
Availability check counts funds the picker can't actually use. Lines 410-419 sum balances from both
addresses_in_block_with_new_balanceand the non-overridden subset ofaddresses_with_balance, buttake_random_amount_with_bounds(lines 260-268) only selects from committed addresses not already present in the staged map, to avoid same-block nonce conflicts. The function can therefore enter the loop believingrange_minis reachable when the only remaining funds sit in already-staged addresses, then fail mid-loop with the corruption above.
Either snapshot/rollback the staged map across the loop, or compute total_available from only the picker-eligible set so failure is detected up front.
🤖 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/strategy-tests/src/addresses_with_balance.rs`:
- [SUGGESTION] lines 440-470: take_random_amounts_with_range_and_min_per_input can still pick a sub-min input when taken_total < range_min
The early-break at line 440 only fires once `taken_total >= range_min`. When `taken_total < range_min` and `remaining_max < min_per_input` (reachable when `global_max - taken_total < min_per_input`, e.g. `range = 200_000..=220_000`, `min_per_input = 100_000`, after a first pick of 150_000), `step_min = remaining_to_min.max(min_per_input).min(remaining_max)` clamps down to `remaining_max`, which is below `min_per_input`. `step_max == step_min`, so the guard at line 457 doesn't fire and `take_random_amount_with_bounds(remaining_max, remaining_max, ...)` returns an address with `taken < min_per_input`. The constructor's `validate_structure()` then rejects the input with `InputBelowMinimumError`, panicking at the `.expect("expected to create address funds transfer transition")` call sites in `lib.rs` and `strategy.rs`. Current CI seeds keep `amount_range` six orders of magnitude above `min_per_input` so this is not reachable today, but it defeats the helper's documented contract. Tighten the break to fire whenever `remaining_max < min_per_input` regardless of `taken_total`, or compute `step_min = remaining_to_min.max(min_per_input)` and break (returning `None`) when `step_min > step_max`.
- [SUGGESTION] lines 407-492: take_random_amounts_with_range_and_min_per_input mutates staged balances/nonces on its failure path and over-counts available funds
Two related issues in this helper, which now powers the new address-funded strategy paths in `lib.rs` (lines ~1486, 2013, 2258, 2587) where callers treat `None` as "no transition emitted" and simply `break`.
1. **Failure path is not rollback-safe.** Each successful `take_random_amount_with_bounds(...)` immediately stages a reduced balance and incremented nonce. When a later iteration fails (`maybe == None` at 463-470) or the loop exits with `taken_total < range_min` (lines 485-490), the function returns `None` without restoring earlier staged mutations. The in-source comment at 486-488 explicitly acknowledges this. Reachable example: `min_per_input = 100_000`, balances `{150_000, 80_000}`, range `200_000..=300_000` — first pick stages 150k, second is impossible, helper returns `None` while leaving the first address consumed. Subsequent strategy steps then operate on silently corrupted balances and nonces.
2. **Availability check counts funds the picker can't actually use.** Lines 410-419 sum balances from both `addresses_in_block_with_new_balance` and the non-overridden subset of `addresses_with_balance`, but `take_random_amount_with_bounds` (lines 260-268) only selects from committed addresses *not already present in the staged map*, to avoid same-block nonce conflicts. The function can therefore enter the loop believing `range_min` is reachable when the only remaining funds sit in already-staged addresses, then fail mid-loop with the corruption above.
Either snapshot/rollback the staged map across the loop, or compute `total_available` from only the picker-eligible set so failure is detected up front.
| fn run_immediate<F: Future>(future: F) -> F::Output { | ||
| struct NoopWake; | ||
|
|
||
| impl Wake for NoopWake { | ||
| fn wake(self: Arc<Self>) {} | ||
| } | ||
|
|
||
| let waker = Waker::from(Arc::new(NoopWake)); | ||
| let mut future = std::pin::pin!(future); | ||
| match future.as_mut().poll(&mut Context::from_waker(&waker)) { | ||
| Poll::Ready(output) => output, | ||
| Poll::Pending => panic!("expected signer future to complete immediately"), | ||
| } | ||
| } |
There was a problem hiding this comment.
💬 Nitpick: run_immediate / NoopWake helper is duplicated verbatim across two test modules
The hand-rolled futures executor (fn run_immediate plus struct NoopWake and the Pending => panic!("expected signer future to complete immediately") arm) is identical to the copy at address_credit_withdrawal/tests.rs:46-58. Both copies depend on TestAddressSigner::sign_create_witness being synchronous; if a signer ever becomes truly async, only one copy may be updated and the other will silently panic. Both files already import from the shared test_helpers module — lift it there to single-source the synchronous-signer assumption.
source: ['claude-general', 'claude-rust-quality']
| fee_strategy: AddressFundsFeeStrategy, | ||
| input_witnesses_count: usize, | ||
| ) -> StateTransition { | ||
| let witnesses: Vec<AddressWitness> = (0..input_witnesses_count) | ||
| .map(|_| create_dummy_witness()) | ||
| .collect(); | ||
| AddressFundsTransferTransition::V0(AddressFundsTransferTransitionV0 { | ||
| let mut transition = AddressFundsTransferTransitionV0 { | ||
| inputs, | ||
| outputs, | ||
| fee_strategy, | ||
| user_fee_increase: 0, | ||
| input_witnesses: witnesses, | ||
| }) | ||
| .into() | ||
| input_witnesses: vec![], | ||
| }; | ||
|
|
||
| let signable_bytes = StateTransition::AddressFundsTransfer( | ||
| AddressFundsTransferTransition::V0(transition.clone()), | ||
| ) | ||
| .signable_bytes() | ||
| .expect("should create signable bytes"); | ||
|
|
||
| // Recover deterministic test keys (seeded as [i; 32]) and sign any matching inputs. | ||
| let mut deterministic_signer = TestAddressSigner::new(); | ||
| for i in 0u8..=255u8 { | ||
| deterministic_signer.add_p2pkh([i; 32]); | ||
| } | ||
|
|
||
| let input_addresses: Vec<PlatformAddress> = transition.inputs.keys().cloned().collect(); | ||
| let witnesses: Vec<AddressWitness> = (0..input_witnesses_count) | ||
| .map(|idx| { | ||
| input_addresses | ||
| .get(idx) | ||
| .and_then(|address| { | ||
| if deterministic_signer.can_sign_with(address) { | ||
| run_immediate( | ||
| deterministic_signer.sign_create_witness(address, &signable_bytes), | ||
| ) | ||
| .ok() | ||
| } else { | ||
| None | ||
| } | ||
| }) | ||
| .unwrap_or_else(create_dummy_witness) | ||
| }) | ||
| .collect(); | ||
|
|
||
| transition.input_witnesses = witnesses; | ||
| AddressFundsTransferTransition::V0(transition).into() | ||
| } |
There was a problem hiding this comment.
💬 Nitpick: create_raw_transition_with_dummy_witnesses brute-forces 256 P2PKH seeds and silently falls back to dummy witnesses
The helper rebuilds a fresh TestAddressSigner and seeds it with every [i; 32] for i in 0..=255 (lines 136-139), then silently falls back to create_dummy_witness() when can_sign_with returns false (line 156). A test intended to exercise an explicit invalid scenario (witness-count mismatch, bad witness, etc.) can instead silently produce a partly-valid signed transition if the caller's input address happens to derive from one of the 256 seeds — masking the test's intent and weakening negative-path coverage. The fallback is also undocumented, so a future maintainer has no signal that some witnesses may be real signatures and others stubs. Take the signer (or seed list) as an explicit parameter so callers opt in to which inputs get real witnesses, and panic/return an error rather than silently substituting when a requested input cannot be signed.
source: ['claude-general', 'claude-rust-quality', 'codex-rust-quality']
| @@ -5109,9 +5133,33 @@ mod tests { | |||
| let total_outputs: u64 = recipient_addresses.values().sum(); | |||
|
|
|||
| // Create the transition once - we'll reuse it for both runs | |||
| let transition = | |||
| create_signed_transition(&identity, &signer, recipient_addresses.clone(), 1).await; | |||
| let transition_bytes = transition.serialize_to_bytes().expect("should serialize"); | |||
| let mut transition_v0 = IdentityCreditTransferToAddressesTransitionV0 { | |||
| identity_id: identity.id(), | |||
| recipient_addresses: recipient_addresses.clone(), | |||
| nonce: 1, | |||
| user_fee_increase: 0, | |||
| signature_public_key_id: 1, | |||
| signature: BinaryData::new(vec![]), | |||
| }; | |||
| let signable_bytes = StateTransition::from( | |||
| IdentityCreditTransferToAddressesTransition::V0(transition_v0.clone()), | |||
| ) | |||
| .signable_bytes() | |||
| .expect("should get signable bytes"); | |||
| let transfer_key = identity | |||
| .public_keys() | |||
| .get(&1) | |||
| .expect("transfer key should exist"); | |||
| transition_v0.signature = signer | |||
| .sign(transfer_key, &signable_bytes) | |||
| .await | |||
| .expect("should sign"); | |||
|
|
|||
| let transition_bytes = StateTransition::from( | |||
| IdentityCreditTransferToAddressesTransition::V0(transition_v0), | |||
| ) | |||
| .serialize_to_bytes() | |||
| .expect("should serialize"); | |||
|
|
|||
| // First run in a transaction to measure actual fee (then rollback) | |||
| let platform_state = platform.state.load(); | |||
| @@ -5236,9 +5284,32 @@ mod tests { | |||
| recipient_addresses.insert(PlatformAddress::P2pkh(hash), min_output); | |||
| } | |||
|
|
|||
| let transition = | |||
| create_signed_transition(&identity, &signer, recipient_addresses.clone(), 1).await; | |||
| let transition_bytes = transition.serialize_to_bytes().expect("should serialize"); | |||
| let mut transition_v0 = IdentityCreditTransferToAddressesTransitionV0 { | |||
| identity_id: identity.id(), | |||
| recipient_addresses: recipient_addresses.clone(), | |||
| nonce: 1, | |||
| user_fee_increase: 0, | |||
| signature_public_key_id: 1, | |||
| signature: BinaryData::new(vec![]), | |||
| }; | |||
| let signable_bytes = StateTransition::from( | |||
There was a problem hiding this comment.
💬 Nitpick: Three near-identical manual transition_v0 + signer.sign blocks duplicated to bypass new client-side validation
Three sites near lines 4969, 5136, and 5287 each replace what was previously a create_signed_transition(...) call with ~22 lines that build IdentityCreditTransferToAddressesTransitionV0 by hand, compute signable_bytes, look up key id 1, call signer.sign, and wrap as StateTransition. The duplication exists because try_from_identity now invokes validate_structure() and these tests deliberately construct invalid scenarios the new client-side gate would reject. Lift the manual path into a single helper (e.g. create_signed_transition_skipping_client_validation) so the three sites stay synchronized when key-id conventions or signing logic evolve, and the intent ("this test bypasses client-side validation") is documented in one place.
source: ['claude-general', 'claude-rust-quality']
thepastaclaw
force-pushed
the
fix/validate-transfer-key-security-level-client-side
branch
from
70fe34d to
9bd72f1
Compare
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
PR's core SDK validation changes are clean: structural validators are properly split pre/post-signing, error propagation is typed, and prior strategy-helper concerns around input minimums are resolved. Two remaining suggestion-level gaps in the strategy generators still synthesize optional outputs without honoring the now-enforced min_output_amount, which can convert what used to be a skipped candidate into a hard .expect() panic. The remaining items are test-infrastructure nitpicks (helper duplication, brute-forced signer seeds, duplicated manual transition assembly).
Reviewed commit: 9bd72f1
🟡 2 suggestion(s) | 💬 3 nitpick(s)
2 additional findings
🟡 suggestion: IdentityCreateFromAddresses strategy still generates sub-minimum optional outputs that the validated constructor now rejects
packages/strategy-tests/src/lib.rs (lines 2136-2145)
This helper does rng.gen_range(output_range.clone()) for the optional output and then calls .expect("expected to create identity from addresses transition") on IdentityCreateFromAddressesTransitionV0::try_from_inputs_with_signer() (line 2158). After this PR, that constructor pre-validates structure via validate_structure_without_input_witnesses() and rejects any output below platform_version.dpp.state_transitions.address_funds.min_output_amount (identity_create_from_addresses_transition/v0/state_transition_validation.rs:184-191) with OutputBelowMinimumError. A caller configuring OperationType::IdentityCreateFromAddresses(..., Some(1..=400_000), ...) will now panic the strategy run instead of skipping the invalid candidate. The earlier input-selection fixes handled min_input_amount but this optional-output path still has the same problem for min_output_amount. Skip the candidate when the sampled amount falls below the minimum (mirrors how the input/transfer paths now handle their minimums).
💡 Suggested change
let min_output_amount = platform_version
.dpp
.state_transitions
.address_funds
.min_output_amount;
let output = if let Some(output_range) = maybe_output_amount.as_ref() {
let output_amount = rng.gen_range(output_range.clone());
if output_amount < min_output_amount {
continue;
}
let output_address = signer.add_random_address_key(rng);
let actual_output_amount = output_amount.saturating_sub(output_fee_deduction);
current_addresses_with_balance
.register_new_address(output_address, actual_output_amount);
Some((output_address, output_amount))
} else {
None
};
🟡 suggestion: AddressWithdrawal strategy can still panic on optional change outputs below min_output_amount
packages/strategy-tests/src/lib.rs (lines 2671-2683)
The withdrawal generator samples output_amount from maybe_output_range, stages the new address, and .expect("expected to create address credit withdrawal transition") on AddressCreditWithdrawalTransition::try_from_inputs_with_signer() (line 2705). That constructor now runs validate_structure_without_input_witnesses() first, and the validator rejects any optional output below min_output_amount (address_credit_withdrawal_transition/v0/state_transition_validation.rs:155-166) with OutputBelowMinimumError. Any scenario configured with a smaller change-output range therefore aborts the strategy run instead of skipping the draw. This is the same fail-fast regression the PR already fixed for inputs and multi-output transfers — apply the same skip-on-sub-minimum guard here.
💡 Suggested change
let min_output_amount = platform_version
.dpp
.state_transitions
.address_funds
.min_output_amount;
let output = if let Some(output_amount_range) = maybe_output_range {
let output_amount = rng.gen_range(output_amount_range.clone());
if output_amount < min_output_amount {
continue;
}
let output_address = signer.add_random_address_key(rng);
let actual_output_amount = output_amount.saturating_sub(output_fee_deduction);
current_addresses_with_balance
.addresses_in_block_with_new_balance
.insert(output_address, (0, actual_output_amount));
Some((output_address, output_amount))
} else {
None
};
🤖 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/strategy-tests/src/lib.rs`:
- [SUGGESTION] lines 2136-2145: IdentityCreateFromAddresses strategy still generates sub-minimum optional outputs that the validated constructor now rejects
This helper does `rng.gen_range(output_range.clone())` for the optional output and then calls `.expect("expected to create identity from addresses transition")` on `IdentityCreateFromAddressesTransitionV0::try_from_inputs_with_signer()` (line 2158). After this PR, that constructor pre-validates structure via `validate_structure_without_input_witnesses()` and rejects any output below `platform_version.dpp.state_transitions.address_funds.min_output_amount` (`identity_create_from_addresses_transition/v0/state_transition_validation.rs:184-191`) with `OutputBelowMinimumError`. A caller configuring `OperationType::IdentityCreateFromAddresses(..., Some(1..=400_000), ...)` will now panic the strategy run instead of skipping the invalid candidate. The earlier input-selection fixes handled `min_input_amount` but this optional-output path still has the same problem for `min_output_amount`. Skip the candidate when the sampled amount falls below the minimum (mirrors how the input/transfer paths now handle their minimums).
- [SUGGESTION] lines 2671-2683: AddressWithdrawal strategy can still panic on optional change outputs below min_output_amount
The withdrawal generator samples `output_amount` from `maybe_output_range`, stages the new address, and `.expect("expected to create address credit withdrawal transition")` on `AddressCreditWithdrawalTransition::try_from_inputs_with_signer()` (line 2705). That constructor now runs `validate_structure_without_input_witnesses()` first, and the validator rejects any optional output below `min_output_amount` (`address_credit_withdrawal_transition/v0/state_transition_validation.rs:155-166`) with `OutputBelowMinimumError`. Any scenario configured with a smaller change-output range therefore aborts the strategy run instead of skipping the draw. This is the same fail-fast regression the PR already fixed for inputs and multi-output transfers — apply the same skip-on-sub-minimum guard here.
| fn run_immediate<F: Future>(future: F) -> F::Output { | ||
| struct NoopWake; | ||
|
|
||
| impl Wake for NoopWake { | ||
| fn wake(self: Arc<Self>) {} | ||
| } | ||
|
|
||
| let waker = Waker::from(Arc::new(NoopWake)); | ||
| let mut future = std::pin::pin!(future); | ||
| match future.as_mut().poll(&mut Context::from_waker(&waker)) { | ||
| Poll::Ready(output) => output, | ||
| Poll::Pending => panic!("expected signer future to complete immediately"), | ||
| } | ||
| } |
There was a problem hiding this comment.
💬 Nitpick: run_immediate / NoopWake helper duplicated verbatim across two test modules
The hand-rolled futures executor run_immediate (with its NoopWake impl and Pending => panic!("expected signer future to complete immediately") arm) is identical to the copy at address_credit_withdrawal/tests.rs:46-59. Both copies depend on TestAddressSigner::sign_create_witness being synchronous; if the signer ever becomes truly async, only one site may be updated and the other will silently panic. Both files already import from the shared test_helpers module — lift the helper there so the synchronous-signer assumption lives in one place.
source: ['claude-general', 'claude-rust-quality']
| fee_strategy: AddressFundsFeeStrategy, | ||
| input_witnesses_count: usize, | ||
| ) -> StateTransition { | ||
| let witnesses: Vec<AddressWitness> = (0..input_witnesses_count) | ||
| .map(|_| create_dummy_witness()) | ||
| .collect(); | ||
| AddressFundsTransferTransition::V0(AddressFundsTransferTransitionV0 { | ||
| let mut transition = AddressFundsTransferTransitionV0 { | ||
| inputs, | ||
| outputs, | ||
| fee_strategy, | ||
| user_fee_increase: 0, | ||
| input_witnesses: witnesses, | ||
| }) | ||
| .into() | ||
| input_witnesses: vec![], | ||
| }; | ||
|
|
||
| let signable_bytes = StateTransition::AddressFundsTransfer( | ||
| AddressFundsTransferTransition::V0(transition.clone()), | ||
| ) | ||
| .signable_bytes() | ||
| .expect("should create signable bytes"); | ||
|
|
||
| // Recover deterministic test keys (seeded as [i; 32]) and sign any matching inputs. | ||
| let mut deterministic_signer = TestAddressSigner::new(); | ||
| for i in 0u8..=255u8 { | ||
| deterministic_signer.add_p2pkh([i; 32]); | ||
| } | ||
|
|
||
| let input_addresses: Vec<PlatformAddress> = transition.inputs.keys().cloned().collect(); | ||
| let witnesses: Vec<AddressWitness> = (0..input_witnesses_count) | ||
| .map(|idx| { | ||
| input_addresses | ||
| .get(idx) | ||
| .and_then(|address| { | ||
| if deterministic_signer.can_sign_with(address) { | ||
| run_immediate( | ||
| deterministic_signer.sign_create_witness(address, &signable_bytes), | ||
| ) | ||
| .ok() | ||
| } else { | ||
| None | ||
| } | ||
| }) | ||
| .unwrap_or_else(create_dummy_witness) | ||
| }) | ||
| .collect(); | ||
|
|
||
| transition.input_witnesses = witnesses; | ||
| AddressFundsTransferTransition::V0(transition).into() | ||
| } |
There was a problem hiding this comment.
💬 Nitpick: create_raw_transition_with_dummy_witnesses brute-forces 256 P2PKH seeds and silently falls back to dummy witnesses
The helper rebuilds a fresh TestAddressSigner and seeds it with every byte 0..=255 (deterministic_signer.add_p2pkh([i; 32]), lines 137-139) on each call so arbitrary input addresses can be re-signed after the fact, then silently falls back to create_dummy_witness when no seed matches (line 156). A test that intends to exercise an explicit invalid scenario (witness-count mismatch, mis-signed witness, etc.) can instead silently produce a partly-valid signed transition if the caller's input address derives from one of the 256 seeds — masking the test's intent and weakening negative-path coverage. The fallback is also undocumented. Take the signer (or the seeds used to construct inputs) as an explicit parameter so callers opt in to which addresses get real witnesses, and panic/return rather than silently substituting when an input cannot be signed.
source: ['claude-general', 'claude-rust-quality']
| let mut transition_v0 = IdentityCreditTransferToAddressesTransitionV0 { | ||
| identity_id: identity.id(), | ||
| recipient_addresses: recipient_addresses.clone(), | ||
| nonce: 1, | ||
| user_fee_increase: 0, | ||
| signature_public_key_id: 1, | ||
| signature: BinaryData::new(vec![]), | ||
| }; | ||
| let signable_bytes: Vec<u8> = StateTransition::from( | ||
| IdentityCreditTransferToAddressesTransition::V0(transition_v0.clone()), | ||
| ) | ||
| .signable_bytes() | ||
| .expect("should get signable bytes"); | ||
| let transfer_key = identity | ||
| .public_keys() | ||
| .get(&1) | ||
| .expect("transfer key should exist"); | ||
| transition_v0.signature = signer | ||
| .sign(transfer_key, &signable_bytes) | ||
| .await | ||
| .expect("should sign"); | ||
|
|
||
| let transition = StateTransition::from( | ||
| IdentityCreditTransferToAddressesTransition::V0(transition_v0), | ||
| ); | ||
| let transition_bytes = transition.serialize_to_bytes().expect("should serialize"); |
There was a problem hiding this comment.
💬 Nitpick: Three near-identical manual transition_v0 + signer.sign blocks duplicated to bypass new client-side validation
Three sites (around lines 4969, 5136, and 5287) each replace what was previously a create_signed_transition(...) call with ~22 lines that build IdentityCreditTransferToAddressesTransitionV0 by hand, compute signable_bytes, look up key id 1, call signer.sign, and wrap as StateTransition. The duplication exists because try_from_identity now invokes validate_structure() and at least the 129-output case at line 5287 deliberately constructs an invalid scenario the new client-side gate would reject. Lift the manual path into a single helper (e.g. create_signed_transition_skipping_client_validation) so the three sites stay synchronized when key-id conventions or signing logic evolve, and the intent ("this test bypasses client-side validation") is documented in one place.
source: ['claude-general', 'claude-rust-quality']
thepastaclaw
force-pushed
the
fix/validate-transfer-key-security-level-client-side
branch
from
9bd72f1 to
b0a0ded
Compare
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Validated all eight findings against the worktree at b0a0ded. No production, consensus, or security blockers remain at this head — prior validation/min-amount/unwrap concerns are resolved. The remaining items are test-infrastructure suggestions/nitpicks plus one rs-dpp visibility nit and three real strategy-generator inconsistencies (one shared bootstrap path and one in-tree asset-lock helper still stage gross funded amounts; one transfer-to-addresses branch silently ignores caller-supplied outputs).
Reviewed commit: b0a0ded
🟡 4 suggestion(s) | 💬 4 nitpick(s)
3 additional findings
🟡 suggestion: Bootstrap address funding stages the gross asset-lock amount instead of the net post-fee balance
packages/strategy-tests/src/lib.rs (lines 754-783)
The start-address bootstrap path registers funded_amount into current_addresses_with_balance (line 756) before constructing the funding transition with vec![AddressFundsFeeStrategyStep::ReduceOutput(0)]. With one output and ReduceOutput(0), the on-chain credit equals funded_amount - output_fee, so each bootstrap-funded address starts the simulation with phantom credits equal to the fee. The sibling AddressFundingFromCoreAssetLock branch in the same file already computes actual_funded_amount = funded_amount.saturating_sub(estimated_fee) at lines 2238-2240 and registers the net amount, which confirms the bootstrap path is now inconsistent with the corrected behavior. Additionally the Err(e) => tracing::error!(...) arm at lines 776-781 only logs — if the constructor (which now runs validate_structure_without_input_witnesses) or the signer fails, the staged address remains in current_addresses_with_balance despite no funding transition being emitted. Subtract the estimated output fee before staging, and unstage the address (or skip registration until success) on the error path.
🟡 suggestion: In-tree asset-lock funding generator still over-credits new addresses by the output fee
packages/rs-drive-abci/tests/strategy_tests/strategy.rs (lines 2663-2687)
create_address_funding_from_core_asset_lock_transition calls register_new_address_keep_only_highest(address, funded_amount, ...) and then constructs the transition with vec![AddressFundsFeeStrategyStep::ReduceOutput(0)]. create_asset_lock_proof_with_amount returns the gross asset-lock output value, so the cached balance is larger than the address's actual post-fee credit after the funding transition executes. Unlike the AddressFundingFromCoreAssetLock branch in packages/strategy-tests/src/lib.rs:2238-2240 (which subtracts address_funds_transfer_output_cost before staging), this in-tree copy never deducts the fee. Compounding the issue, on try_from_asset_lock_with_signer failure the .ok()? at line 2685 drops the transition while leaving the staged address behind, letting follow-on transitions spend credits the funding transition never created. Mirror the strategy-tests helper: subtract the estimated fee before register_new_address_keep_only_highest, and on failure remove the staged address before returning None.
💬 nitpick: create_identity_credit_transfer_to_addresses_transition_with_outputs panics on caller-supplied outputs below min_output_amount
packages/strategy-tests/src/transitions.rs (lines 1024-1046)
Unlike its sibling create_identity_credit_transfer_to_addresses_transition (lines 951-1004) which caps output_count by (total_amount / min_output_amount).max(1) and returns None for sub-minimum totals, this _with_outputs variant accepts a caller-provided recipient_addresses: BTreeMap<PlatformAddress, u64> and .expect(...)s the constructor result. After this PR, IdentityCreditTransferToAddressesTransition::try_from_identity runs validate_structure() and rejects any recipient amount below platform_version.dpp.state_transitions.address_funds.min_output_amount with OutputBelowMinimumError. Today the only caller (packages/rs-drive-abci/tests/strategy_tests/strategy.rs:1512-1518) feeds in transfer_info.outputs from a hardcoded test config, so existing CI never trips this. A future test fixture or external strategy-tests consumer that passes a smaller change/dust output would convert what looks like a benign helper into a strategy-run panic. Either pre-validate against min_output_amount and return Result/Option like the random variant, or replace the .expect(...) with explicit error propagation.
🤖 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/strategy-tests/src/lib.rs`:
- [SUGGESTION] lines 1879-1900: IdentityTransferToAddresses(Some(transfer_info)) silently ignores the caller's explicit output map
This branch is documented as handling "specific sender and outputs are provided", but it forwards only `transfer_info.amount` and `transfer_info.outputs.len()` to `create_identity_credit_transfer_to_addresses_transition`, which then generates fresh random recipient addresses and splits the amount evenly. The caller's hand-picked recipient addresses and per-output amounts are dropped entirely. Strategies that depend on deterministic destinations or uneven per-output amounts will silently exercise a different scenario, and any subsequent operation keyed to the configured addresses will not see the expected balances. The in-tree `rs-drive-abci/tests/strategy_tests/strategy.rs:1502-1518` already routes this case through `create_identity_credit_transfer_to_addresses_transition_with_outputs(..., transfer_info.outputs.clone(), ...)`, so the published `strategy-tests` crate is now out of sync with the in-tree expectation. Switch this branch to call the `_with_outputs` variant and pass `transfer_info.outputs.clone()` so caller-specified destinations are honored.
- [SUGGESTION] lines 754-783: Bootstrap address funding stages the gross asset-lock amount instead of the net post-fee balance
The start-address bootstrap path registers `funded_amount` into `current_addresses_with_balance` (line 756) before constructing the funding transition with `vec![AddressFundsFeeStrategyStep::ReduceOutput(0)]`. With one output and `ReduceOutput(0)`, the on-chain credit equals `funded_amount - output_fee`, so each bootstrap-funded address starts the simulation with phantom credits equal to the fee. The sibling `AddressFundingFromCoreAssetLock` branch in the same file already computes `actual_funded_amount = funded_amount.saturating_sub(estimated_fee)` at lines 2238-2240 and registers the net amount, which confirms the bootstrap path is now inconsistent with the corrected behavior. Additionally the `Err(e) => tracing::error!(...)` arm at lines 776-781 only logs — if the constructor (which now runs `validate_structure_without_input_witnesses`) or the signer fails, the staged address remains in `current_addresses_with_balance` despite no funding transition being emitted. Subtract the estimated output fee before staging, and unstage the address (or skip registration until success) on the error path.
In `packages/rs-drive-abci/tests/strategy_tests/strategy.rs`:
- [SUGGESTION] lines 2663-2687: In-tree asset-lock funding generator still over-credits new addresses by the output fee
`create_address_funding_from_core_asset_lock_transition` calls `register_new_address_keep_only_highest(address, funded_amount, ...)` and then constructs the transition with `vec![AddressFundsFeeStrategyStep::ReduceOutput(0)]`. `create_asset_lock_proof_with_amount` returns the gross asset-lock output value, so the cached balance is larger than the address's actual post-fee credit after the funding transition executes. Unlike the `AddressFundingFromCoreAssetLock` branch in `packages/strategy-tests/src/lib.rs:2238-2240` (which subtracts `address_funds_transfer_output_cost` before staging), this in-tree copy never deducts the fee. Compounding the issue, on `try_from_asset_lock_with_signer` failure the `.ok()?` at line 2685 drops the transition while leaving the staged address behind, letting follow-on transitions spend credits the funding transition never created. Mirror the strategy-tests helper: subtract the estimated fee before `register_new_address_keep_only_highest`, and on failure remove the staged address before returning `None`.
In `packages/rs-dpp/src/state_transition/traits/state_transition_structure_validation.rs`:
- [SUGGESTION] lines 19-27: first_consensus_error_as_protocol_error unnecessarily widens rs-dpp's public API
All sixteen call sites of this helper live inside `rs-dpp` (verified across the seven address-funded/identity constructors). Declaring it `pub` inside `state_transition::traits` (re-exported from `state_transition`) commits to it as part of the published crate API for what is just a local error-shape policy ("keep only the first consensus error"). Restrict to `pub(crate)` so future shape changes — e.g. preserving multi-error context — are not a semver-breaking change for downstream crates.
| transfer_info.outputs.len(), | ||
| rng, | ||
| platform_version, | ||
| ).await; | ||
| operations.push(state_transition); | ||
| ).await | ||
| { | ||
| operations.push(state_transition); | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: IdentityTransferToAddresses(Some(transfer_info)) silently ignores the caller's explicit output map
This branch is documented as handling "specific sender and outputs are provided", but it forwards only transfer_info.amount and transfer_info.outputs.len() to create_identity_credit_transfer_to_addresses_transition, which then generates fresh random recipient addresses and splits the amount evenly. The caller's hand-picked recipient addresses and per-output amounts are dropped entirely. Strategies that depend on deterministic destinations or uneven per-output amounts will silently exercise a different scenario, and any subsequent operation keyed to the configured addresses will not see the expected balances. The in-tree rs-drive-abci/tests/strategy_tests/strategy.rs:1502-1518 already routes this case through create_identity_credit_transfer_to_addresses_transition_with_outputs(..., transfer_info.outputs.clone(), ...), so the published strategy-tests crate is now out of sync with the in-tree expectation. Switch this branch to call the _with_outputs variant and pass transfer_info.outputs.clone() so caller-specified destinations are honored.
source: ['codex-general']
🤖 Fix this 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/strategy-tests/src/lib.rs`:
- [SUGGESTION] lines 1879-1900: IdentityTransferToAddresses(Some(transfer_info)) silently ignores the caller's explicit output map
This branch is documented as handling "specific sender and outputs are provided", but it forwards only `transfer_info.amount` and `transfer_info.outputs.len()` to `create_identity_credit_transfer_to_addresses_transition`, which then generates fresh random recipient addresses and splits the amount evenly. The caller's hand-picked recipient addresses and per-output amounts are dropped entirely. Strategies that depend on deterministic destinations or uneven per-output amounts will silently exercise a different scenario, and any subsequent operation keyed to the configured addresses will not see the expected balances. The in-tree `rs-drive-abci/tests/strategy_tests/strategy.rs:1502-1518` already routes this case through `create_identity_credit_transfer_to_addresses_transition_with_outputs(..., transfer_info.outputs.clone(), ...)`, so the published `strategy-tests` crate is now out of sync with the in-tree expectation. Switch this branch to call the `_with_outputs` variant and pass `transfer_info.outputs.clone()` so caller-specified destinations are honored.
| pub fn first_consensus_error_as_protocol_error( | ||
| result: SimpleConsensusValidationResult, | ||
| ) -> Option<ProtocolError> { | ||
| result | ||
| .errors | ||
| .into_iter() | ||
| .next() | ||
| .map(|error| ProtocolError::ConsensusError(Box::new(error))) | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: first_consensus_error_as_protocol_error unnecessarily widens rs-dpp's public API
All sixteen call sites of this helper live inside rs-dpp (verified across the seven address-funded/identity constructors). Declaring it pub inside state_transition::traits (re-exported from state_transition) commits to it as part of the published crate API for what is just a local error-shape policy ("keep only the first consensus error"). Restrict to pub(crate) so future shape changes — e.g. preserving multi-error context — are not a semver-breaking change for downstream crates.
💡 Suggested change
| pub fn first_consensus_error_as_protocol_error( | |
| result: SimpleConsensusValidationResult, | |
| ) -> Option<ProtocolError> { | |
| result | |
| .errors | |
| .into_iter() | |
| .next() | |
| .map(|error| ProtocolError::ConsensusError(Box::new(error))) | |
| } | |
| pub(crate) fn first_consensus_error_as_protocol_error( | |
| result: SimpleConsensusValidationResult, | |
| ) -> Option<ProtocolError> { | |
| result | |
| .errors | |
| .into_iter() | |
| .next() | |
| .map(|error| ProtocolError::ConsensusError(Box::new(error))) | |
| } |
source: ['codex-rust-quality']
🤖 Fix this 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/traits/state_transition_structure_validation.rs`:
- [SUGGESTION] lines 19-27: first_consensus_error_as_protocol_error unnecessarily widens rs-dpp's public API
All sixteen call sites of this helper live inside `rs-dpp` (verified across the seven address-funded/identity constructors). Declaring it `pub` inside `state_transition::traits` (re-exported from `state_transition`) commits to it as part of the published crate API for what is just a local error-shape policy ("keep only the first consensus error"). Restrict to `pub(crate)` so future shape changes — e.g. preserving multi-error context — are not a semver-breaking change for downstream crates.
| fee_strategy: AddressFundsFeeStrategy, | ||
| input_witnesses_count: usize, | ||
| ) -> StateTransition { | ||
| let witnesses: Vec<AddressWitness> = (0..input_witnesses_count) | ||
| .map(|_| create_dummy_witness()) | ||
| .collect(); | ||
| AddressFundsTransferTransition::V0(AddressFundsTransferTransitionV0 { | ||
| let mut transition = AddressFundsTransferTransitionV0 { | ||
| inputs, | ||
| outputs, | ||
| fee_strategy, | ||
| user_fee_increase: 0, | ||
| input_witnesses: witnesses, | ||
| }) | ||
| .into() | ||
| input_witnesses: vec![], | ||
| }; | ||
|
|
||
| let signable_bytes = StateTransition::AddressFundsTransfer( | ||
| AddressFundsTransferTransition::V0(transition.clone()), | ||
| ) | ||
| .signable_bytes() | ||
| .expect("should create signable bytes"); | ||
|
|
||
| // Recover deterministic test keys (seeded as [i; 32]) and sign any matching inputs. | ||
| let mut deterministic_signer = TestAddressSigner::new(); | ||
| for i in 0u8..=255u8 { | ||
| deterministic_signer.add_p2pkh([i; 32]); | ||
| } | ||
|
|
||
| let input_addresses: Vec<PlatformAddress> = transition.inputs.keys().cloned().collect(); | ||
| let witnesses: Vec<AddressWitness> = (0..input_witnesses_count) | ||
| .map(|idx| { | ||
| input_addresses | ||
| .get(idx) | ||
| .and_then(|address| { | ||
| if deterministic_signer.can_sign_with(address) { | ||
| run_immediate( | ||
| deterministic_signer.sign_create_witness(address, &signable_bytes), | ||
| ) | ||
| .ok() | ||
| } else { | ||
| None | ||
| } | ||
| }) | ||
| .unwrap_or_else(create_dummy_witness) | ||
| }) | ||
| .collect(); | ||
|
|
||
| transition.input_witnesses = witnesses; | ||
| AddressFundsTransferTransition::V0(transition).into() | ||
| } |
There was a problem hiding this comment.
💬 Nitpick: create_raw_transition_with_dummy_witnesses silently substitutes real signatures for some inputs
The helper rebuilds a fresh TestAddressSigner and seeds it with every byte 0..=255 via deterministic_signer.add_p2pkh([i; 32]) (lines 137-139) on each call, then falls back to create_dummy_witness() (line 156) when no derived key matches. The fallback is undocumented and the seed coverage is implicit, so a negative-path test (witness-count mismatch, mis-signed witness, etc.) can silently produce a partly-valid signed transition if any input address happens to derive from one of those 256 seeds — masking the test's intent and weakening coverage. Take the signer or seed list as an explicit parameter so callers opt in to which addresses get real witnesses, and panic/return rather than silently substituting when an input cannot be signed.
source: ['claude-general', 'claude-rust-quality', 'codex-rust-quality']
| fn run_immediate<F: Future>(future: F) -> F::Output { | ||
| struct NoopWake; | ||
|
|
||
| impl Wake for NoopWake { | ||
| fn wake(self: Arc<Self>) {} | ||
| } | ||
|
|
||
| let waker = Waker::from(Arc::new(NoopWake)); | ||
| let mut future = std::pin::pin!(future); | ||
| match future.as_mut().poll(&mut Context::from_waker(&waker)) { | ||
| Poll::Ready(output) => output, | ||
| Poll::Pending => panic!("expected signer future to complete immediately"), | ||
| } | ||
| } |
There was a problem hiding this comment.
💬 Nitpick: run_immediate / NoopWake helper duplicated verbatim across two test modules
The hand-rolled futures executor run_immediate (with its NoopWake: Wake impl and Pending => panic!("expected signer future to complete immediately") arm) is identical to the copy at packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/address_credit_withdrawal/tests.rs:46-58. Both copies bake in the assumption that TestAddressSigner::sign_create_witness is synchronous; if the signer ever becomes truly async, only one copy is likely to be updated and the other will silently panic. Both files already import from the shared test_helpers module — lift the helper there so the synchronous-signer assumption lives in one place.
source: ['claude-general', 'claude-rust-quality']
| let mut transition_v0 = IdentityCreditTransferToAddressesTransitionV0 { | ||
| identity_id: identity.id(), | ||
| recipient_addresses: recipient_addresses.clone(), | ||
| nonce: 1, | ||
| user_fee_increase: 0, | ||
| signature_public_key_id: 1, | ||
| signature: BinaryData::new(vec![]), | ||
| }; | ||
| let signable_bytes: Vec<u8> = StateTransition::from( | ||
| IdentityCreditTransferToAddressesTransition::V0(transition_v0.clone()), | ||
| ) | ||
| .signable_bytes() | ||
| .expect("should get signable bytes"); | ||
| let transfer_key = identity | ||
| .public_keys() | ||
| .get(&1) | ||
| .expect("transfer key should exist"); | ||
| transition_v0.signature = signer | ||
| .sign(transfer_key, &signable_bytes) | ||
| .await | ||
| .expect("should sign"); | ||
|
|
||
| let transition = StateTransition::from( | ||
| IdentityCreditTransferToAddressesTransition::V0(transition_v0), | ||
| ); | ||
| let transition_bytes = transition.serialize_to_bytes().expect("should serialize"); |
There was a problem hiding this comment.
💬 Nitpick: Three near-identical manual transition_v0 + signer.sign blocks duplicated to bypass new client-side validation
Three sites at lines 4969-4994, 5136-5156, and 5287+ each replace what was previously a create_signed_transition(...) call with ~22 lines that build IdentityCreditTransferToAddressesTransitionV0 by hand, compute signable_bytes, look up key id 1, call signer.sign, and wrap as StateTransition. The duplication exists because try_from_identity now invokes validate_structure() and these tests deliberately construct invalid scenarios the new client-side gate would reject (for example the 129-output case at line 5287). Lift the manual path into a single helper (e.g. create_signed_transition_skipping_client_validation) so the three sites stay synchronized when key-id conventions or signing logic evolve, and the intent ("this test bypasses client-side validation") is documented in one place.
source: ['claude-general', 'claude-rust-quality']
thepastaclaw
force-pushed
the
fix/validate-transfer-key-security-level-client-side
branch
from
b0a0ded to
a2d190f
Compare
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Re-validated all eight prior findings against head a2d190f. The four production/architecture concerns from the prior review (strategy-tests ignoring caller-supplied outputs, bootstrap and in-tree asset-lock helpers over-crediting on constructor failure, and the public visibility of first_consensus_error_as_protocol_error) are resolved at this head. Four test-infrastructure nitpicks remain — both agents converge on the same set, so confidence is boosted. No blockers; this PR is a defense-in-depth client-side validation lift with no consensus, security, or proof-system surface touched.
Reviewed commit: a2d190f
💬 4 nitpick(s)
1 additional finding
💬 nitpick: `create_identity_credit_transfer_to_addresses_transition_with_outputs` panics on caller-supplied outputs below `min_output_amount`
packages/strategy-tests/src/transitions.rs (lines 1024-1046)
Unlike its sibling create_identity_credit_transfer_to_addresses_transition (lines 951-1004), which caps output_count by (total_amount / min_output_amount).max(1) and returns None for sub-minimum totals, this _with_outputs variant accepts a caller-provided recipient_addresses: BTreeMap<PlatformAddress, u64> and .expect(...)s the constructor result. After this PR, IdentityCreditTransferToAddressesTransition::try_from_identity runs validate_structure() and rejects any recipient amount below platform_version.dpp.state_transitions.address_funds.min_output_amount with OutputBelowMinimumError, plus >128 outputs and other shape errors. Today the only caller (packages/rs-drive-abci/tests/strategy_tests/strategy.rs ~1907-1918) forwards transfer_info.outputs.clone() from a hardcoded test config, so existing CI never trips this. Because strategy-tests is a published reusable crate, a future fixture or external consumer that passes a smaller change/dust output would convert what looks like a benign helper into a strategy-run panic. Either pre-validate against min_output_amount/output-count limits and return Result/Option like the random variant, or replace the .expect(...) with explicit error propagation.
| fn run_immediate<F: Future>(future: F) -> F::Output { | ||
| struct NoopWake; | ||
|
|
||
| impl Wake for NoopWake { | ||
| fn wake(self: Arc<Self>) {} | ||
| } | ||
|
|
||
| let waker = Waker::from(Arc::new(NoopWake)); | ||
| let mut future = std::pin::pin!(future); | ||
| match future.as_mut().poll(&mut Context::from_waker(&waker)) { | ||
| Poll::Ready(output) => output, | ||
| Poll::Pending => panic!("expected signer future to complete immediately"), | ||
| } | ||
| } |
There was a problem hiding this comment.
💬 Nitpick: run_immediate / NoopWake executor duplicated verbatim across two test modules
The hand-rolled executor here is identical to the copy at packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/address_credit_withdrawal/tests.rs:46-59. Both copies bake in the same hidden invariant — TestAddressSigner::sign_create_witness must complete synchronously, otherwise the Pending => panic!("expected signer future to complete immediately") arm fires. If the signer ever becomes truly async, only one copy is likely to be updated and the other will silently panic in CI. Both files already import from the shared test_helpers module, so lift the helper there to keep the synchronous-signer assumption in one place.
source: ['claude', 'codex']
| fee_strategy: AddressFundsFeeStrategy, | ||
| input_witnesses_count: usize, | ||
| ) -> StateTransition { | ||
| let witnesses: Vec<AddressWitness> = (0..input_witnesses_count) | ||
| .map(|_| create_dummy_witness()) | ||
| .collect(); | ||
| AddressFundsTransferTransition::V0(AddressFundsTransferTransitionV0 { | ||
| let mut transition = AddressFundsTransferTransitionV0 { | ||
| inputs, | ||
| outputs, | ||
| fee_strategy, | ||
| user_fee_increase: 0, | ||
| input_witnesses: witnesses, | ||
| }) | ||
| .into() | ||
| input_witnesses: vec![], | ||
| }; | ||
|
|
||
| let signable_bytes = StateTransition::AddressFundsTransfer( | ||
| AddressFundsTransferTransition::V0(transition.clone()), | ||
| ) | ||
| .signable_bytes() | ||
| .expect("should create signable bytes"); | ||
|
|
||
| // Recover deterministic test keys (seeded as [i; 32]) and sign any matching inputs. | ||
| let mut deterministic_signer = TestAddressSigner::new(); | ||
| for i in 0u8..=255u8 { | ||
| deterministic_signer.add_p2pkh([i; 32]); | ||
| } | ||
|
|
||
| let input_addresses: Vec<PlatformAddress> = transition.inputs.keys().cloned().collect(); | ||
| let witnesses: Vec<AddressWitness> = (0..input_witnesses_count) | ||
| .map(|idx| { | ||
| input_addresses | ||
| .get(idx) | ||
| .and_then(|address| { | ||
| if deterministic_signer.can_sign_with(address) { | ||
| run_immediate( | ||
| deterministic_signer.sign_create_witness(address, &signable_bytes), | ||
| ) | ||
| .ok() | ||
| } else { | ||
| None | ||
| } | ||
| }) | ||
| .unwrap_or_else(create_dummy_witness) | ||
| }) | ||
| .collect(); | ||
|
|
||
| transition.input_witnesses = witnesses; | ||
| AddressFundsTransferTransition::V0(transition).into() | ||
| } |
There was a problem hiding this comment.
💬 Nitpick: create_raw_transition_with_dummy_witnesses silently substitutes real signatures for some inputs
The helper is documented as producing dummy witnesses, but on each call it rebuilds a fresh TestAddressSigner and seeds it with every byte 0..=255 via deterministic_signer.add_p2pkh([i; 32]) (lines 137-139), then signs any input whose address matches one of those 256 seeds and only falls back to create_dummy_witness() (line 156) when no derived key matches. The seed coverage is implicit and the substitution is undocumented, so a negative-path test (witness-count mismatch, malformed-witness behavior) can silently produce a partly-valid signed transition if any input address happens to derive from one of those seeds — masking the test's intent. Take the signer or seed list as an explicit parameter so callers opt in to which addresses get real witnesses, and panic or return when an input cannot be signed instead of silently substituting.
source: ['claude', 'codex']
| let mut transition_v0 = IdentityCreditTransferToAddressesTransitionV0 { | ||
| identity_id: identity.id(), | ||
| recipient_addresses: recipient_addresses.clone(), | ||
| nonce: 1, | ||
| user_fee_increase: 0, | ||
| signature_public_key_id: 1, | ||
| signature: BinaryData::new(vec![]), | ||
| }; | ||
| let signable_bytes: Vec<u8> = StateTransition::from( | ||
| IdentityCreditTransferToAddressesTransition::V0(transition_v0.clone()), | ||
| ) | ||
| .signable_bytes() | ||
| .expect("should get signable bytes"); | ||
| let transfer_key = identity | ||
| .public_keys() | ||
| .get(&1) | ||
| .expect("transfer key should exist"); | ||
| transition_v0.signature = signer | ||
| .sign(transfer_key, &signable_bytes) | ||
| .await | ||
| .expect("should sign"); | ||
|
|
||
| let transition = StateTransition::from( | ||
| IdentityCreditTransferToAddressesTransition::V0(transition_v0), | ||
| ); | ||
| let transition_bytes = transition.serialize_to_bytes().expect("should serialize"); |
There was a problem hiding this comment.
💬 Nitpick: Manual transition_v0 + sign blocks duplicated three times to bypass new client-side validation
Three sites at lines 4969-4994, 5136-5162, and 5287-5312 each replace what was previously a create_signed_transition(...) call with ~22 lines that build IdentityCreditTransferToAddressesTransitionV0 by hand, compute signable_bytes, look up key id 1, call signer.sign, and serialize as StateTransition. The duplication exists because try_from_identity now invokes validate_structure() and these tests deliberately construct invalid scenarios (for example the 129-output case at line 5287) the new client-side gate would reject. The duplication is fragile — any future change to the signing convention, key selection, or transition shape has to land in three places, and the bypass intent is implicit. Lift the manual path into a helper such as create_signed_transition_skipping_client_validation(...) so the three sites stay synchronized and the intent is documented in one place.
source: ['claude', 'codex']
4 participants
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: