fix(dpp): derive deterministic names for unnamed document type indexes - #4280
Conversation
|
Warning Review limit reached
Next review available in: 22 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughUnnamed document indexes now receive deterministic names from indexed properties and sort directions. Empty indexes use ChangesIndex naming
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
🔍 Review in progress — actively reviewing now (commit b22d10a) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/rs-dpp/src/data_contract/document_type/index/mod.rs (1)
1583-1606: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the empty-properties fallback.
The updated tests cover non-empty property lists, but not the branch at Lines 994-996. Add a case with
properties: []and assert thatindex.nameis"index".Suggested regression test
+ #[test] + fn test_index_try_from_empty_properties_uses_index_name() { + let index_map: Vec<(Value, Value)> = vec![( + Value::Text("properties".to_string()), + Value::Array(vec![]), + )]; + let index = Index::try_from(index_map.as_slice()).unwrap(); + assert_eq!(index.name, "index"); + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/rs-dpp/src/data_contract/document_type/index/mod.rs` around lines 1583 - 1606, Add a regression test alongside test_index_try_from_without_name_multi_property_directions that parses an Index definition with properties set to an empty array and no explicit name, then assert the resulting index.name is "index" to cover the empty-properties fallback.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/rs-dpp/src/data_contract/document_type/index/mod.rs`:
- Around line 983-1009: The fallback naming logic in the unnamed-index branch of
the surrounding index conversion function is not injective because it joins
property names and directions with underscores. Replace this derivation with an
injective separator or encoding that cannot create collisions from property
names, preserve deterministic ordering and duplicate-index collapsing, and add a
regression test covering property names containing the chosen separator.
---
Nitpick comments:
In `@packages/rs-dpp/src/data_contract/document_type/index/mod.rs`:
- Around line 1583-1606: Add a regression test alongside
test_index_try_from_without_name_multi_property_directions that parses an Index
definition with properties set to an empty array and no explicit name, then
assert the resulting index.name is "index" to cover the empty-properties
fallback.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 2830a095-4040-4486-8138-67e0cdbe9fa2
📒 Files selected for processing (1)
packages/rs-dpp/src/data_contract/document_type/index/mod.rs
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## v4.2-dev #4280 +/- ##
==========================================
Coverage 87.60% 87.61%
==========================================
Files 2703 2704 +1
Lines 344990 345206 +216
==========================================
+ Hits 302242 302442 +200
- Misses 42748 42764 +16
🚀 New features to boost your workflow:
|
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Codex + Sonnet
This PR correctly replaces a random 24-char index-name fallback with a deterministic derivation, fixing run-to-run/node-to-node reproducibility for unnamed indexes on the check_tx/client/fixture-only parse path (verified: every meta-schema version requires name, so the fallback never fires on the validated/consensus path). The one real gap, independently identified and converged upon by all Claude and Codex reviewer lanes, is that the {prop}_{asc|desc} join reuses _ as both the intra-token and inter-property separator, so two structurally different unnamed index declarations (e.g. a property literally named a_asc_b with direction desc vs. two properties a:asc, b:desc) derive the identical name string and silently collapse into one BTreeMap entry — verified against the meta-schema's ^[a-zA-Z0-9-_]{1,64}$ property-name pattern (underscores are legal) and the parse_indices collect-into-BTreeMap call site. This contradicts the PR's own stated invariant that collapsing only happens for true duplicate declarations, though it remains bounded to the non-consensus-visible fallback path.
Source: reviewers codex/general=gpt-5.6-sol(completed); codex/security-auditor=gpt-5.6-sol(completed); codex/rust-quality=gpt-5.6-sol(completed); claude/general=claude-sonnet-5(completed); claude/security-auditor=claude-sonnet-5(completed); claude/rust-quality=claude-sonnet-5(failed); claude/rust-quality=claude-sonnet-5(completed); verifier=claude/final-verifier=claude-sonnet-5(completed); coordinator=openclaw-agent/cliproxy/gpt-5.6-sol(orchestration-only).
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— security-auditor (completed),gpt-5.6-sol— rust-quality (completed) - Verifier:
claude-sonnet-5— final-verifier - Sonnet reviewers:
claude-sonnet-5— general (completed),claude-sonnet-5— security-auditor (completed),claude-sonnet-5— rust-quality (failed),claude-sonnet-5— rust-quality (completed)
🟡 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/index/mod.rs`:
- [SUGGESTION] packages/rs-dpp/src/data_contract/document_type/index/mod.rs:992-1009: Derived index name is not injective — property-set collisions silently drop indexes from the BTreeMap
The derivation joins each property's `{name}_{asc|desc}` token with `_`, using the same character as both the intra-token separator and the inter-property joiner. The document meta-schema's `propertyNames` pattern (`^[a-zA-Z0-9-_]{1,64}$`, present in v0/v1/v2 `document-meta.json`) explicitly permits `_` in property names, and that constraint isn't enforced on the unvalidated code path this fallback targets, so the scheme is not collision-free between *different* index definitions.
Concrete collision: a compound index over properties `a` (asc) and `b` (desc) derives `"a_asc_b_desc"`. A single-property index over a property literally named `a_asc_b` with direction `desc` also derives `"a_asc_b_desc"` (verified against the code — a single property just contributes one `{name}_{dir}` token with no extra join needed). These are two semantically distinct index declarations — different property sets, different tree shapes — that end up as the same key.
`parse_indices` (packages/rs-dpp/src/data_contract/document_type/class_methods/try_from_schema/common/mod.rs:885, feeding the `.collect::<BTreeMap<String, Index>>()` at line 728) collects `(index.name.clone(), index)` pairs directly, so a name collision silently overwrites one entry instead of erroring — this contradicts the PR's own commentary that collapsing here 'is the right outcome for a duplicate index declaration,' since the collision case above is not a duplicate declaration at all.
Impact is bounded: this fallback is unreachable for any contract that has passed meta-schema validation (a name is mandatory there), so it can't affect consensus, storage, or proof correctness. It can, however, cause check_tx-time or SDK/client-side parsing of not-yet-validated contract JSON to silently lose one of two distinct index declarations that a client believes are both present.
Parsing an index declared without a name assigned a random 24-char
alphanumeric name, so two parses of the same contract disagreed on
in-memory index names and on the iteration order of the name-keyed
indices map. Unnamed indexes can only reach this code when schema
validation is skipped (check_tx, legacy fixtures, client-side parses):
every document meta-schema version (v0/v1/v2) requires `name`, and
block execution validates against the meta-schema before index parsing,
so nothing consensus- or storage-visible ever saw a generated name.
Derive the name from the index properties and directions instead
({prop}_{asc|desc} joined by _), making parses reproducible run-to-run
and node-to-node. Identical unnamed duplicate declarations now collapse
to one entry, which is the correct outcome for a duplicate index.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
422597f to
b22d10a
Compare
The workspace test job invokes `cargo llvm-cov nextest` but only
installed cargo-llvm-cov, relying on self-hosted runners to have
nextest preinstalled. A runner without it fails the job before any
test runs ("error: no such command: nextest"). Install it with the
same idempotent pattern used for cargo-llvm-cov.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Note: the "Rust workspace tests / Tests" failure on b22d10a was infrastructure, not a test failure — the job landed on a self-hosted runner without 🤖 Addressed by Claude Code |
2 participants
Issue being fixed or feature implemented
Index::try_fromassigned a random 24-character alphanumeric name to any index declared without one, so two parses of the same contract produced different in-memory index names and a different iteration order of the name-keyedBTreeMap<String, Index>. This was found by a differential-equivalence harness where two runs of the same code on the same fixture corpus produced different transcripts (dozens of test fixture contracts, e.g. the family and dashpay corpora, declare unnamed indexes).This is not consensus- or storage-visible: every document meta-schema version (v0/v1/v2) requires index
name, block execution (ValidationMode::Validator) validates against the meta-schema before index parsing, and stored contracts persist the raw schemaValue(never the parsedIndex, which has no bincode derives). The random name was reachable only where validation is skipped — check_tx, client-side/SDK parses, and test fixtures — but there it broke parse reproducibility run-to-run and node-to-node.What was done?
packages/rs-dpp/src/data_contract/document_type/index/mod.rs: replaced the random fallback name with a deterministic derivation from the index definition — properties joined as{property}_{asc|desc}(e.g.ownerId_asc_createdAt_desc), with"index"as the fallback for an empty property list. Removed the now-unusedrand::distributions::{Alphanumeric, DistString}import.How Has This Been Tested?
test_index_try_from_without_name_generates_randomwithtest_index_try_from_without_name_derives_deterministic_name(asserts the derived name and that a second parse yields the same name) and addedtest_index_try_from_without_name_multi_property_directions(multi-property asc/desc derivation).cargo test -p dpp --lib— 3809 passed.cargo test -p drive— full suite passed (heavy consumer of the unnamed-index family/dashpay fixtures).cargo clippy -p dpp --all-features --testsclean (one pre-existing warning infactory/v0/mod.rs),cargo fmt --allapplied.Breaking Changes
None.
Checklist:
For repository code-owners and collaborators only
🤖 Generated with Claude Code
Summary by CodeRabbit