fix(dpp): harden nested document-property position parsing

[QuantumExplorer]

Issue being fixed or feature implemented

Hardens parsing of position on nested document-schema properties. The recursive nested-property parser performed a position-based sort whose result was never used, and that sort could fail on certain malformed schema inputs reached during contract processing. This removes the dead sort so nested properties parse robustly.

Reported by @DanielDerefaka — thanks for the careful report.

What was done?

• Removed the unused position sort in try_from_schema/insert_values_nested. Its result was never consumed — nested properties are emitted in source-map order by the existing properties.iter() loop just below it — so removing it changes no accepted-contract behavior while eliminating the failure path. Added a code comment so it isn't reintroduced.
• Added regression tests covering malformed nested positions on both the full-validation and check-tx parse paths, plus a valid-position case confirming parsing behavior is unchanged.

How Has This Been Tested?

• cargo test -p dpp --features validation --lib try_from_schema — all 49 tests pass, including the 3 new regression tests and the existing nested-property tests (behavior unchanged).
• cargo clippy -p dpp --features validation and cargo fmt --all — clean.

Breaking Changes

None. The removed sort had no effect (its output was discarded), so accepted contracts parse identically; only a previously-failing edge case now parses cleanly.

Checklist:

• I have performed a self-review of my own code
• I have commented my code, particularly in hard-to-understand areas
• I have added or updated relevant unit/integration/functional/e2e tests
• I have added "!" to the title and described breaking changes in the corresponding section if my code contains any
• I have made corresponding changes to the documentation if needed

For repository code-owners and collaborators only

• I have assigned this pull request to a milestone

🤖 Generated with Claude Code

Summary by CodeRabbit

• Bug Fixes

  • Improved schema parsing robustness by removing a risky sorting step that could cause panics when encountering edge-case position values, preserving original property order.

• Tests

  • Added unit tests to ensure nested property position values are handled safely across validation modes and to prevent regressions.

[thepastaclaw]

✅ Review complete (commit efa1d73)

[coderabbitai]

Warning

Review limit reached

@QuantumExplorer, we couldn't start this review because you've reached your PR review rate limit.

More reviews will be available in 13 minutes and 2 seconds. Learn how PR review limits work.

Your organization has run out of usage credits. Purchase more credits in the billing tab to continue.

⌛ How to resolve this issue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available.

Please see our Fair Usage Limits Policy for further information.

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 8e3402ef-8962-4812-b5fa-5ca0fc102eb6

📥 Commits

Reviewing files that changed from the base of the PR and between b953432 and efa1d73.

📒 Files selected for processing (1)

• packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/data_contract_create/mod.rs

📝 Walkthrough

Walkthrough

Removed dead nested-property position-sorting (and its panicking .expect() reads) from schema parsing and added unit tests that exercise nested position values across validation modes to ensure no panics and correct rejection under full validation.

Changes

Schema Parsing Robustness

Layer / File(s)	Summary
Remove nested position-sorting and add nested-position tests packages/rs-dpp/src/data_contract/document_type/class_methods/try_from_schema/mod.rs, packages/rs-dpp/src/data_contract/document_type/class_methods/try_from_schema/v1/mod.rs	Removed the nested-property position-based sorting and .expect() reads from insert_values_nested, preserving iteration order. Added nested_property_position_handling tests that build nested schemas with zero-fraction floats, malformed numeric position values, out-of-range values, and valid integers, asserting parsing behavior under full and check_tx validation modes.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

A rabbit nudges code with nimble paws,

Removing traps and fixing flawed small laws,
Tests now guard nested positions' art,
Parsing steady, no more panic-start! 🐇

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately describes the main change: hardening nested document-property position parsing by removing unused and potentially panicking position-based sort logic.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch claude/fix-nested-position-halt

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[thepastaclaw]

Code Review

Targeted hardening PR that removes a dead sort_by in insert_values_nested whose .expect("expected a position") could panic on adversarial nested-schema input. The sorted vector was never consumed (the loop iterates properties.iter() directly), so accepted-contract ordering is unchanged. Tests cover both validation paths plus a positive baseline. Two minor test-strengthening suggestions; no blocking issues.

🟡 1 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/data_contract/document_type/class_methods/try_from_schema/v1/mod.rs`:
- [SUGGESTION] packages/rs-dpp/src/data_contract/document_type/class_methods/try_from_schema/v1/mod.rs:834-841: Malformed-position regression tests only assert non-panic, not the parse outcome
  `malformed_nested_positions_do_not_panic_check_tx` and `nested_float_position_does_not_panic_full_validation` discard results with `let _ = parse(...)`, so they only lock in the no-panic invariant. The current behavior on these inputs is `Err` (propagated from `get_integer::<u64>()?` or the meta-schema), but a future refactor that silently accepts `Value::Float(0.0)`, `Value::I64(-1)`, or `Value::U128(u64::MAX as u128 + 1)` — or returns a misleading error variant — would still pass. Mirror the style of `valid_nested_positions_still_parse` and assert `Err(_)` (ideally the specific `ProtocolError` / `DataContractError` variant) on each malformed case so the post-fix contract is pinned.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 70.73%. Comparing base (93b657d) to head (b953432).
⚠️ Report is 4 commits behind head on v3.1-dev.

❗ There is a different number of reports uploaded between BASE (93b657d) and HEAD (b953432). Click for more details.

HEAD has 1 upload less than BASE

Flag	BASE (93b657d)	HEAD (b953432)
rust	2	1

Additional details and impacted files

@@              Coverage Diff              @@
##           v3.1-dev    #3857       +/-   ##
=============================================
- Coverage     87.15%   70.73%   -16.43%     
=============================================
  Files          2641       20     -2621     
  Lines        327793     2788   -325005     
=============================================
- Hits         285701     1972   -283729     
+ Misses        42092      816    -41276     

Components	Coverage Δ
dpp	∅ <ø> (∅)
drive	∅ <ø> (∅)
drive-abci	∅ <ø> (∅)
sdk	∅ <ø> (∅)
dapi-client	∅ <ø> (∅)
platform-version	∅ <ø> (∅)
platform-value	∅ <ø> (∅)
platform-wallet	∅ <ø> (∅)
drive-proof-verifier	∅ <ø> (∅)

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[github-actions]

✅ DashSDKFFI.xcframework built for this PR.

• Workflow run: https://github.com/dashpay/platform/actions/runs/27392233744
• Artifacts: DashSDKFFI.xcframework (folder), DashSDKFFI.xcframework.zip, xc_checksum.txt

SwiftPM (host the zip at a stable URL, then use):

.binaryTarget(
  name: "DashSDKFFI",
  url: "https://your.cdn.example/DashSDKFFI.xcframework.zip",
  checksum: "402aa45da1e981df92e7b1f074d74dd3242ae721234e3f5b56af8cdff906fbce"
)

Xcode manual integration:

• Download 'DashSDKFFI.xcframework' artifact from the run link above.
• Drag it into your app target (Frameworks, Libraries & Embedded Content) and set Embed & Sign.
• If using the Swift wrapper package, point its binaryTarget to the xcframework location or add the package and place the xcframework at the expected path.

[thepastaclaw]

Code Review

Incremental review of b953432 over 43d6c01. Both prior findings are FIXED on the current head: the malformed-position regression tests now pin Ok/Err parse outcomes via assert_matches! (v1/mod.rs:826-886), and the comment at mod.rs:207-216 now documents the consensus-observable source-map IndexMap insertion-order invariant and explicitly warns against any nested-property sort. The genuinely new diff (test outcome assertions + invariant comment) directly resolves both carried-forward findings; no new in-scope issues identified.

[thepastaclaw]

Code Review

All six agents (Claude + Codex across general/security/rust-quality) converged on a clean review: the PR removes a dead position-based sort in insert_values_nested whose .expect() could panic deterministically on adversarial nested-property positions (floats, negatives, or values exceeding u64), eliminating a chain-halt vector while preserving source-map ordering already provided by properties.iter(). Test coverage is layered (DPP unit tests across both validation modes plus a drive-abci end-to-end regression through process_raw_state_transitions). Prior-finding revalidation list is empty as expected (the b953432 review was clean), so there are no carried-forward findings, and no new in-scope issues were identified in the latest delta.