fix(dpp): derive deterministic names for unnamed document type indexes - #4280
fix(dpp): derive deterministic names for unnamed document type indexes#4280QuantumExplorer wants to merge 1 commit into
Conversation
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>
📝 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 |
|
✅ Final review complete — no blockers (commit 422597f) |
![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
| // If the index didn't have a name, derive one deterministically from | ||
| // its properties and their directions. Every document meta-schema | ||
| // (v0/v1/v2) requires `name`, so an unnamed index can only reach this | ||
| // point when schema validation is skipped (check_tx, legacy fixtures, | ||
| // client-side parses of contracts that could never register); a random | ||
| // name here would make two parses of the same contract disagree on the | ||
| // index name and on the iteration order of the name-keyed indices map. | ||
| // Two unnamed indexes over identical properties and directions derive | ||
| // the same name and collapse to one entry in that map, which is the | ||
| // right outcome for a duplicate index declaration. | ||
| let name = name.unwrap_or_else(|| { | ||
| if index_properties.is_empty() { | ||
| "index".to_string() | ||
| } else { | ||
| index_properties | ||
| .iter() | ||
| .map(|property| { | ||
| format!( | ||
| "{}_{}", | ||
| property.name, | ||
| if property.ascending { "asc" } else { "desc" } | ||
| ) | ||
| }) | ||
| .collect::<Vec<_>>() | ||
| .join("_") | ||
| } | ||
| }); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
rg -n -C 8 --glob '*.rs' \
'IndexProperty|property.*pattern|pattern.*property|DuplicateIndexNameError|index_names' \
packages/rs-dpp/src || trueRepository: dashpay/platform
Length of output: 50373
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
echo "== index name derivation context =="
sed -n '950,1022p' packages/rs-dpp/src/data_contract/document_type/index/mod.rs
echo
echo "== IndexProperty platform parser =="
sed -n '1025,1060p' packages/rs-dpp/src/data_contract/document_type/index/mod.rs
echo
echo "== duplicate index name definitions/usages =="
rg -n -C 4 --glob '*.rs' 'duplicate .*index|DuplicateIndex|index_names|index\.name|index->name|dedupe|same name|distinct indexes' packages/rs-dpp/src packages/rs-dpp/tests .github packages/rs-dpp -S || trueRepository: dashpay/platform
Length of output: 50373
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
echo "== check contract schema names =="
rg -n -C 4 '"name"|field.*pattern|pattern.*field|properties.*field|index.*property|indexProperty' packages/rs-dpp/src -g '*.rs' -g '*.json' -g '*.jsonc' -g '*.js' -g '*.ts' | head -n 300
echo
echo "== deterministic name collision probe =="
python3 - <<'PY'
def name(props):
if not props:
return "index"
return "_".join(f"{name_}_{dir_}" for name_, dir_ in props)
cases = [
[({"a": "asc"},), ({"a"} {"b": "asc"},)],
[({"ownerId_a": "asc"},), ({"ownerId": "asc", "a": "asc"},)],
]
propses = cases[0]
print(name(propses[0]), name(propses[1]))
print(name.propses[0])
PYRepository: dashpay/platform
Length of output: 27286
🌐 Web query:
Dash Platform Data Contracts field names pattern underscore property names
💡 Result:
In Dash Platform, property names (field names) within data contracts are allowed to include underscores () [1][2][3]. The official Dash Platform documentation specifies that property names must adhere to the following constraints [1][2]: Characters allowed: Alphanumeric characters (A-Z, a-z, 0-9), hyphens (-), and underscores () [1][2]. Length: Minimum length of 1 character and maximum length of 64 characters [1][2][3]. While underscores are permitted as part of the naming convention, developers should ensure their schema definitions remain consistent with these rules [1][2]. Note that historical restrictions on property name length and allowed characters existed prior to Dash Platform v0.23, but these have since been updated to the current flexible standard [4].
Citations:
- 1: https://docs.dash.org/projects/platform/en/stable/docs/reference/data-contracts.html
- 2: https://docs.dash.org/projects/platform/en/latest/docs/protocol-ref/data-contract-document.html
- 3: https://docs.dash.org/projects/platform/en/1.0.0/docs/reference/data-contracts.html
- 4: https://dash-user-docs.readthedocs.io/projects/platform/en/0.24.0/docs/reference/data-contracts.html
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
python3 - <<'PY'
def derived_name(index_properties):
if not index_properties:
return "index"
return "_".join(f"{name}_{dir_}" for name, dir_ in index_properties)
print(derived_name([("a", True), ("b", True)]))
print(derived_name([("a_asc_b", True)]))
print(derived_name([("ownerId_a", True)]))
print(derived_name([("ownerId", True), ("a", True)]))
PYRepository: dashpay/platform
Length of output: 216
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
echo "== generated derived name in current source with asc/desc =="
sed -n '993,1007p' packages/rs-dpp/src/data_contract/document_type/index/mod.rs
echo
echo "== exact collision probe matching current source =="
python3 - <<'PY'
def derived_name(index_properties):
if not index_properties:
return "index"
return "_".join(f"{name}_{dir_}" for name, dir_ in index_properties)
print(derived_name([("a", True), ("b", True)]))
print(derived_name([("a_asc_b", True)]))
print(derived_name([("ownerId_a", True)]))
print(derived_name([("ownerId", True), ("a", True)]))
PYRepository: dashpay/platform
Length of output: 885
Avoid using _ as a joiner in derived index names.
Dash property names may contain _, but the current format joins derived names with _. Since IndexProperty::from_platform_value does not reject _ in property names, distinct unknown names like ownerId_a ascending and ownerId/a ascending both derive the same index name. Use an injective separator/encoding and add a regression test for property names containing the derived-name separator.
🤖 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
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.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## v4.2-dev #4280 +/- ##
=========================================
Coverage 87.54% 87.54%
=========================================
Files 2679 2679
Lines 341312 341342 +30
=========================================
+ Hits 298799 298827 +28
- Misses 42513 42515 +2
🚀 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.
| // right outcome for a duplicate index declaration. | ||
| let name = name.unwrap_or_else(|| { | ||
| if index_properties.is_empty() { | ||
| "index".to_string() | ||
| } else { | ||
| index_properties | ||
| .iter() | ||
| .map(|property| { | ||
| format!( | ||
| "{}_{}", | ||
| property.name, | ||
| if property.ascending { "asc" } else { "desc" } | ||
| ) | ||
| }) | ||
| .collect::<Vec<_>>() | ||
| .join("_") | ||
| } | ||
| }); |
There was a problem hiding this comment.
🟡 Suggestion: 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.
| // right outcome for a duplicate index declaration. | |
| let name = name.unwrap_or_else(|| { | |
| if index_properties.is_empty() { | |
| "index".to_string() | |
| } else { | |
| index_properties | |
| .iter() | |
| .map(|property| { | |
| format!( | |
| "{}_{}", | |
| property.name, | |
| if property.ascending { "asc" } else { "desc" } | |
| ) | |
| }) | |
| .collect::<Vec<_>>() | |
| .join("_") | |
| } | |
| }); | |
| let name = name.unwrap_or_else(|| { | |
| if index_properties.is_empty() { | |
| "index".to_string() | |
| } else { | |
| // Length-prefix each property name so the joined string stays | |
| // injective even when a property name itself contains '_', | |
| // 'asc', or 'desc' -- this fallback runs exactly where the | |
| // meta-schema's property-name pattern isn't enforced. | |
| index_properties | |
| .iter() | |
| .map(|property| { | |
| format!( | |
| "{}:{}_{}", | |
| property.name.len(), | |
| property.name, | |
| if property.ascending { "asc" } else { "desc" } | |
| ) | |
| }) | |
| .collect::<Vec<_>>() | |
| .join("_") | |
| } | |
| }); |
source: ['claude', 'codex']
| @@ -1556,8 +1580,30 @@ mod tests { | |||
| )])]), | |||
| )]; | |||
| let index = Index::try_from(index_map.as_slice()).unwrap(); | |||
| assert!(!index.name.is_empty()); | |||
| assert_eq!(index.name.len(), 24); // Alphanumeric.sample_string with len 24 | |||
| assert_eq!(index.name, "fieldA_asc"); | |||
|
|
|||
| // Parsing the same definition again must produce the same name | |||
| let again = Index::try_from(index_map.as_slice()).unwrap(); | |||
| assert_eq!(again.name, index.name); | |||
| } | |||
|
|
|||
| #[test] | |||
| fn test_index_try_from_without_name_multi_property_directions() { | |||
| let index_map: Vec<(Value, Value)> = vec![( | |||
| Value::Text("properties".to_string()), | |||
| Value::Array(vec![ | |||
| Value::Map(vec![( | |||
| Value::Text("ownerId".to_string()), | |||
| Value::Text("asc".to_string()), | |||
| )]), | |||
| Value::Map(vec![( | |||
| Value::Text("createdAt".to_string()), | |||
| Value::Text("desc".to_string()), | |||
| )]), | |||
| ]), | |||
| )]; | |||
| let index = Index::try_from(index_map.as_slice()).unwrap(); | |||
There was a problem hiding this comment.
💬 Nitpick: No test covers the empty-properties fallback ("index") or the name-collision edge case
The two new tests cover the happy-path single- and multi-property derivations and correctly assert re-parse stability, but two edge cases from this same change are untested: (1) the index_properties.is_empty() branch that falls back to the literal "index" — reachable when properties is [] or absent, since nothing before this point requires a non-empty property list; and (2) the collision case from the sibling finding, where a single property literally named e.g. a_asc_b (desc) derives the same name as two properties a:asc, b:desc. Adding regression tests for both would close this coverage gap and make the collapsing behavior an explicit, verifiable contract rather than an implicit side effect of the join scheme.
source: ['claude']
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