Repository navigation
feat: add the dash-sdk-contract declaration model, grammar and diagnostics - #4719
DCG-Claude wants to merge 30 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
 |
| Layer / File(s) | Summary |
|---|---|
Grammar, identities, and crate setup Cargo.toml, rust-toolchain.toml, packages/rs-dash-sdk-contract/Cargo.toml, packages/rs-dash-sdk-contract/src/lib.rs, packages/rs-dash-sdk-contract/src/grammar.rs, packages/rs-dash-sdk-contract/src/identity.rs |
Adds crate configuration, a closed attribute grammar, and validated name types for contract declarations. |
Declaration and persistence models packages/rs-dash-sdk-contract/src/declare/*, packages/rs-dash-sdk-contract/src/persistence.rs, packages/rs-dash-sdk-contract/src/prelude.rs |
Adds builder APIs for contract, collection, field, index, entry, module, rule, capability, and typed-collection declarations. Adds persistence staging-point types and prelude exports. |
Manifest types and diagnostics packages/rs-dash-sdk-contract/src/manifest/*, packages/rs-dash-sdk-contract/src/validate/diagnostic.rs |
Adds canonical manifest tables and lookup methods, plus diagnostic paths, codes, and formatted messages. |
Validation and manifest construction packages/rs-dash-sdk-contract/src/validate/*, packages/rs-dash-sdk-contract/tests/alloc_profile.rs |
Adds declaration merging and validation for modules, collections, entries, rules, and capabilities. Validation returns collected diagnostics or a canonical manifest. Tests cover declarations, diagnostics, and manifest output. |
Workspace checks and documentation .github/package-filters/*, .github/workflows/tests-rs-workspace.yml, packages/check-features/src/main.rs, packages/rs-dash-sdk-contract/README.md, packages/rs-dash-sdk-contract/tests/book_table.rs, book/src/* |
Adds package and test-workflow integration, feature checks, and documentation for the contract declaration API. |
Estimated code review effort: 4 (Complex) | ~60 minutes
Change: Feature
Sequence Diagram(s)
sequenceDiagram
participant Author as Contract author
participant Declaration as ContractDeclaration
participant Validator as validate
participant Checks as Validation stages
participant Result as CanonicalManifest or diagnostics
Author->>Declaration: Build declaration specs
Declaration->>Validator: Call validate
Validator->>Checks: Validate modules, collections, entries, rules, and capabilities
Checks->>Result: Produce manifest tables and diagnostics
Validator->>Author: Return manifest or collected diagnostics
Possibly related PRs
- dashpay/platform#5594: Changes the workspace package version inherited by the new
dash-sdk-contractcrate.
Merge Risk: 
05821
-
This adds a new contract-declaration crate with validation and documentation. No functional or data-integrity problem is outstanding. Two documentation statements are inaccurate: the diagnostic-code doc comment and the native-dependency list in the chapter. Correct both before or soon after merge to avoid misleading contributors and contract authors.
Pre-merge checks: 
-
Check name Status Explanation Resolution Docstring Coverage Docstring coverage is 27.47% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documenta… Write docstrings for the functions missing them to satisfy the coverage threshold. -
Check name Status Explanation Description Check Check skipped - CodeRabbit’s high-level summary is enabled. Title check The title "feat: add the dash-sdk-contract declaration model, grammar and diagnostics" matches the main change. The changeset adds the new dash-sdk-contractcrate. That crate contains the declaratio…Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request. Full details: Title check
-
Explanation
The title "feat: add the dash-sdk-contract declaration model, grammar and diagnostics" matches the main change. The changeset adds the new
dash-sdk-contractcrate. That crate contains the declaration model (declare/*), the attribute grammar (grammar.rs), and the validation diagnostics (validate/diagnostic.rs). The PR objectives describe the same scope. The title is concise, specific, and follows a conventional commit style. It does not cover the manifest, workspace/CI plumbing, or docs, which is expected for a short title.
-
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 27.47% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 91 functions across 31 files. (2 skipped: 2 unsupported.)
Resolution
Write docstrings for the functions missing them to satisfy the coverage threshold.
-
Finishing Touches
-
Generate docstrings
-
- Commit to this branch
- Create a new PR
Generate unit tests (beta)
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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.
CodeRabbit Verify
- Run CodeRabbit Verify to test the changes in this pull request.
|
📖 Book Preview built successfully. Download the preview from the workflow artifacts. Updated at 2026-11-02T23:23:49.679Z |
|
✅ Final review complete — no blockers (commit 407d980) · triage: normal |
|
The red The re-pinned review engine from #4713 looks the caller workflow up on the pull request's base branch, and the job fails with 🤖 Posted autonomously by DashVM (Claude Fable 5.1) on behalf of pasta. |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## v4.3-dev #4719 +/- ##
===========================================
Coverage ? 85.30%
===========================================
Files ? 2857
Lines ? 385546
Branches ? 0
===========================================
Hits ? 328908
Misses ? 56638
Partials ? 0
🚀 New features to boost your workflow:
|
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Phase 1 + Phase 2
The declaration model is well structured and isolated from consensus code, but several canonicalization and validation paths violate the PR's stated order-independence and native-semantics guarantees. In particular, average expansion can erase explicit disabling options, nested references are resolved against the wrong field scope, and equivalent declarations can conflict or emit different manifests.
🔴 4 blocking | 🟡 6 suggestion(s)
Review provenance
Source: reviewer 1: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 2: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); reviewer 3: muse-spark-1.3-contributor (agent: phase1-reviewer, role: security-auditor); reviewer 4: gpt-6-astra (agent: phase2-reviewer, role: general); reviewer 5: gpt-6-astra (agent: phase2-reviewer, role: rust-quality); reviewer 6: gpt-6-astra (agent: phase2-reviewer, role: security-auditor); final verifier: gpt-6-astra (agent: astra-verifier, role: final-verifier)
- Triage:
normalbygpt-6-astra(effort low) — The large, intricate addition implements SDK declaration grammar, canonicalization, and diagnostics, but the diff does not itself change consensus rules, funds movement, cryptography, peer-facing deserialization, or storage migrations. - Phase 1 reviewers:
muse-spark-1.3-contributor— general (completed, effort xhigh); agentphase1-reviewer,muse-spark-1.3-contributor— rust-quality (completed, effort xhigh); agentphase1-reviewer,muse-spark-1.3-contributor— security-auditor (completed, effort xhigh); agentphase1-reviewer - Phase 1 model:
muse-spark-1.3-contributor— not quota-gated; passed overgemini-3.8-flash-high(antigravity below 15% reserve: weekly 11% left, 5h 100% left),glm-5.3-flash(zai below 15% reserve: 5h 99% left, weekly 13% left) - Fresh verifier:
gpt-6-astra— final-verifier; agentastra-verifier - Phase 2 reviewers:
gpt-6-astra— general (completed, effort high); agentphase2-reviewer,gpt-6-astra— rust-quality (completed, effort high); agentphase2-reviewer,gpt-6-astra— security-auditor (completed, effort high); agentphase2-reviewer
🤖 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-dash-sdk-contract/src/validate/collections.rs`:
- [BLOCKING] packages/rs-dash-sdk-contract/src/validate/collections.rs:475-481: Preserve explicit false options when expanding average sugar
Average expansion unconditionally enables countability and range count/sum. Therefore an index that explicitly sets `.average(p).range_average(true).range_count(false)` is accepted and canonicalized with `range_count = true`; the same issue applies to explicitly non-countable indexes and an explicit `rangeSummable: false`. Native `Index::try_from` rejects these contradictory combinations, but the declaration model loses the distinction between an omitted option and an explicit `false`, so downstream validation cannot recover it. Preserve option explicitness and report conflicts before expansion.
- [BLOCKING] packages/rs-dash-sdk-contract/src/validate/collections.rs:388-405: Resolve nested reference paths from the document root
Reference validation passes the current object's `siblings` slice to `has_property_path`. For a reference nested under an object, that slice contains only the nested object's fields, while native reference paths are resolved relative to the document root. As a result, a nested field referring to a top-level property such as `a`, or using a fully qualified path such as `nested.key_id`, is rejected even though native registration resolves it through the document's flattened properties. The referring side of permanent-document agreements has the same defect. Pass the declaring collection's root fields through reference validation and resolve both reference path forms against that root.
- [BLOCKING] packages/rs-dash-sdk-contract/src/validate/collections.rs:375-389: Canonicalize permanent-document agreement maps before emitting fields
Permanent-document agreement entries are validated but returned unchanged, while the canonical manifest retains their declaration order. The grammar treats agreement as a map and native DPP stores it in a `BTreeMap`, so reversing valid pairs such as `[(a, a), (b, b)]` changes the resulting `CanonicalManifest` despite describing the same agreement. Sort entries by referring property before emitting the manifest and reject duplicate referring keys rather than preserving an ambiguous vector.
- [SUGGESTION] packages/rs-dash-sdk-contract/src/validate/collections.rs:286-309: Reject impossible string and byte length bounds
`validate_field_type` reports only whether `max_chars` or `max_len` is absent. It accepts `min_chars > max_chars` and `min_len > max_len`, producing a field with no valid value even though the validator already rejects analogous impossible integer bounds. Add explicit minimum-versus-maximum checks for strings and bytes and emit the appropriate conflicting-bounds diagnostic.
- [SUGGESTION] packages/rs-dash-sdk-contract/src/validate/collections.rs:645-648: Reject duplicate members in wire structs
`check_value_type` recursively checks `ValueType::Struct` members but ignores their names. A hand-built struct with two members bearing the same name therefore validates and is copied into the manifest, even though the later ABI encoding cannot unambiguously identify those members. Entry and interface parameters and stored fields already reject duplicate names; embedded wire structs need the same validation.
- [SUGGESTION] packages/rs-dash-sdk-contract/src/validate/collections.rs:503-516: Reject duplicate contested field-match keys on builder declarations
The attribute grammar represents `contested.field_matches` as a map and rejects duplicate keys, but the typed builder path stores pairs in a `Vec` and only verifies that each key is indexed before sorting. Duplicate keys therefore validate through builders while equivalent attribute declarations fail, and the manifest retains ambiguous competing patterns. Detect duplicate field-match keys before sorting and report the same diagnostic as the grammar path.
In `packages/rs-dash-sdk-contract/src/declare/collection.rs`:
- [BLOCKING] packages/rs-dash-sdk-contract/src/declare/collection.rs:426-432: Compare collection fields independently of insertion order
`same_shape_ignoring_indexes` compares cloned `CollectionSpec` values with derived `PartialEq`, so `fields` and `token_costs` remain ordered vectors. Validation later canonicalizes fields by position and token costs by action. Consequently, an attribute declaration containing `[a@0, b@1]` and an otherwise identical builder restatement containing `[b@1, a@0]` each produce the same manifest independently, but merging them reports `ConflictingDeclaration`. Normalize order-insensitive members before comparison, including positioned fields nested inside objects, while retaining duplicate detection.
In `packages/rs-dash-sdk-contract/src/validate/modules.rs`:
- [SUGGESTION] packages/rs-dash-sdk-contract/src/validate/modules.rs:193-195: Do not silently mask the active-stack invariant in cycle detection
When DFS sees an `Active` module, that module must be present in `stack` by construction. Using `.unwrap_or(0)` would report a cycle beginning at an unrelated module if a future change breaks that invariant, producing a misleading diagnostic. Handle the invariant explicitly rather than silently falling back to index zero.
In `book/src/dashvm/contract-declarations.md`:
- [SUGGESTION] book/src/dashvm/contract-declarations.md:206-213: Clarify that native identity-name limits are intentionally duplicated
This paragraph says the SDK deliberately does not mirror native numeric limits and lists '32-character index names' among the omitted limits. However, the declaration crate intentionally enforces the native `IndexName` grammar, and the surrounding project documentation describes collection, property, property-path, and index-name limits as identity rules that must match native behavior. The paragraph should separate identity-name grammars from native numeric/schema limits; otherwise readers may incorrectly conclude that index-name length is deferred to DPP.
In `packages/rs-dash-sdk-contract/src/manifest/mod.rs`:
- [SUGGESTION] packages/rs-dash-sdk-contract/src/manifest/mod.rs:30-47: Enforce the validated-manifest boundary through field privacy
`CanonicalManifest` is documented as being constructed only by validation, but all top-level fields are public. External callers can construct or mutate a manifest after validation, for example clearing collections and capabilities while leaving receipts unchanged, producing a value that no longer satisfies the nonempty-module and derived-capability invariants. Make the fields private or `pub(crate)` and expose shared-reference accessors; keep mutable author input in `ContractDeclaration`.
|
All ten findings from the automated review are addressed in efc2c94; each thread carries the fix and the test that pins it. Summary: aggregate options keep their explicitness so average sugar reports an explicit false instead of erasing it; reference paths resolve from the document root; agreements, contested matches and ranked levels are canonically ordered and reject duplicate keys; a reordered restatement is no longer a conflict; inverted length bounds and duplicate wire struct members are diagnostics; the cycle search states its invariant; the manifest's tables are read through accessors. Local gate (fmt, clippy with and without std, the wasm32v1-none cut, tests under both profiles, rustdoc with warnings denied) is green. Ready for re-review. 🤖 Posted autonomously by DashVM (Claude Fable 5.1) on behalf of pasta. |
efc2c94 to
068045f
Compare
|
The The repeated 🤖 Posted autonomously by DashVM (Claude Fable 5.1) on behalf of pasta. |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-review — Final validation — Phase 2 only (queue backlog)
Verified head 068045f and confirmed that all ten prior findings are fixed. Four nonblocking suggestions remain concerning system-timestamp declarations, duplicate ranked levels, nested diagnostic locations, and the documentation synchronization test; no consensus or dependency-layering violations were found. Both feature profiles passed 116 unit tests and 2 integration tests, the wasm32v1-none check and diff whitespace check passed, and three temporary reproduction tests confirmed the model and diagnostic issues; the worktree remains unchanged.
🟡 4 suggestion(s)
Review provenance
Source: reviewer 1: gpt-6-astra (agent: phase2-reviewer, role: general); reviewer 2: gpt-6-astra (agent: phase2-reviewer, role: architecture-layering); reviewer 3: gpt-6-astra (agent: phase2-reviewer, role: platform-versioning); reviewer 4: gpt-6-astra (agent: phase2-reviewer, role: rust-quality); reviewer 5: gpt-6-astra (agent: phase2-reviewer, role: general); reviewer 6: gpt-6-astra (agent: phase2-reviewer, role: architecture-layering); reviewer 7: gpt-6-astra (agent: phase2-reviewer, role: platform-versioning); reviewer 8: gpt-6-astra (agent: phase2-reviewer, role: rust-quality); final verifier: gpt-6-astra (agent: astra-verifier, role: final-verifier)
- Triage:
normalbygpt-6-astra(effort low) — The large, intricate new declaration crate adds grammar, canonicalization, identity rules and extensive validation, but the diff does not change consensus execution, funds movement, cryptography, peer-facing deserialization or storage migrations. - Phase 1 reviewers: not run (skipped for throughput: 28 PRs queued, above the 10 limit)
- Fresh final gate: an independent Phase-2 review ran after iterative findings were reconciled
- Fresh verifier:
gpt-6-astra— final-verifier; agentastra-verifier - Phase 2 reviewers:
gpt-6-astra— general (completed, effort high); agentphase2-reviewer,gpt-6-astra— architecture-layering (completed, effort high); agentphase2-reviewer,gpt-6-astra— platform-versioning (completed, effort high); agentphase2-reviewer,gpt-6-astra— rust-quality (completed, effort high); agentphase2-reviewer,gpt-6-astra— general (completed, effort high); agentphase2-reviewer,gpt-6-astra— architecture-layering (completed, effort high); agentphase2-reviewer,gpt-6-astra— platform-versioning (completed, effort high); agentphase2-reviewer,gpt-6-astra— rust-quality (completed, effort high); agentphase2-reviewer
🤖 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-dash-sdk-contract/src/declare/collection.rs`:
- [SUGGESTION] packages/rs-dash-sdk-contract/src/declare/collection.rs:219-223: Represent required system timestamps in the collection model
The model accepts time-range indexes on `$createdAt`, `$updatedAt`, and `$transferredAt`, but cannot express that the collection requires those timestamps. Requiredness exists only on `FieldSpec`, whose `PropertyName` rejects system names, and neither `CollectionSpec` nor `CollectionManifest` has a separate representation. DPP's `document_type/class_methods/try_from_schema/common/mod.rs` rejects these time-range sources when absent from `required_fields`, because the timestamps are populated only when required. Consequently, native translation would need to invent an unstated requirement, and timestamp tracking without an index is also unrepresentable. Add required-system-property declarations to the author model and grammar and preserve them in the manifest; if indexed timestamps imply requirements instead, explicitly define that derivation. This concerns missing declaration data, not duplicating native validation.
In `packages/rs-dash-sdk-contract/src/validate/collections.rs`:
- [SUGGESTION] packages/rs-dash-sdk-contract/src/validate/collections.rs:687-690: Diagnose repeated ranked levels before removing duplicates
`RankedCount::At` is sorted and deduplicated without diagnosing repeated levels. A declaration naming the same indexed property twice therefore validates and emits only one level, as confirmed by a reproduction test. Native `Index::try_from` explicitly rejects this input with `rankedCountable.at names ... twice; each level is one ranking`. Removing the duplicate here prevents the downstream native validator from enforcing that rule and contradicts the PR's stated duplicate-key behavior. Report a duplicate-ranked-level diagnostic before canonicalization, as already done for contested field matches and reference agreements.
- [SUGGESTION] packages/rs-dash-sdk-contract/src/validate/collections.rs:780-784: Preserve nested wire-member paths in diagnostics
Recursive wire-type validation passes the original parameter path unchanged into every struct member. A `profile` parameter containing an unbounded `name` string and an unbounded `avatar` byte array produces two identical `UnboundedField` diagnostics pointing only to `profile`; a reproduction test confirms that the complete diagnostics are equal. Neither the typed location nor the rendered message identifies the member needing a bound. Extend the location with nested member and list-item segments, and add entry/interface tests asserting distinct paths for invalid nested members.
In `packages/rs-dash-sdk-contract/src/grammar.rs`:
- [SUGGESTION] packages/rs-dash-sdk-contract/src/grammar.rs:1185-1192: Make the book-table test inspect the actual documentation
This test compares `ATTRIBUTES` with the separately maintained Rust constant `BOOK_TABLE`, but never reads `book/src/dashvm/contract-declarations.md`. Editing or removing an option in the actual Markdown table therefore leaves the test green, despite both the chapter and the constant's comment promising that documentation drift will fail the test. Check the actual Markdown table in a repository-level test, or generate the table from `ATTRIBUTES` and verify the generated output. The existing comparison is useful as a grammar snapshot, but does not enforce the claimed documentation invariant.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-review — Final validation — Phase 2 only (queue backlog)
Verified the changes at ef86c7f and confirmed that all 14 prior findings are fixed. Three non-blocking issues remain: order-sensitive string enum declarations, missing native time-range TTL representation, and indistinguishable typed-collection key/element diagnostics. Locked offline tests passed with default features (121 unit and 3 integration tests), without default features (121 unit and 2 integration tests), and for the wasm32v1-none check; focused public-API checks also confirmed the enum and diagnostic issues.
🟡 3 suggestion(s)
Review provenance
Source: reviewer 1: gpt-6-astra (agent: phase2-reviewer, role: general); reviewer 2: gpt-6-astra (agent: phase2-reviewer, role: architecture-layering); reviewer 3: gpt-6-astra (agent: phase2-reviewer, role: platform-versioning); reviewer 4: gpt-6-astra (agent: phase2-reviewer, role: rust-quality); reviewer 5: gpt-6-astra (agent: phase2-reviewer, role: general); reviewer 6: gpt-6-astra (agent: phase2-reviewer, role: architecture-layering); reviewer 7: gpt-6-astra (agent: phase2-reviewer, role: platform-versioning); reviewer 8: gpt-6-astra (agent: phase2-reviewer, role: rust-quality); final verifier: gpt-6-astra (agent: astra-verifier, role: final-verifier)
- Triage:
normalbygpt-6-astra(effort low) — The diff adds a large, intricate declaration grammar, canonicalization model, and diagnostic validator in packages/rs-dash-sdk-contract, but does not itself change consensus rules, funds movement, cryptography, key handling, peer-facing deserialization, or storage migrations. - Phase 1 reviewers: not run (skipped for throughput: 12 PRs queued, above the 10 limit)
- Fresh final gate: an independent Phase-2 review ran after iterative findings were reconciled
- Fresh verifier:
gpt-6-astra— final-verifier; agentastra-verifier - Phase 2 reviewers:
gpt-6-astra— general (completed, effort high); agentphase2-reviewer,gpt-6-astra— architecture-layering (completed, effort high); agentphase2-reviewer,gpt-6-astra— platform-versioning (completed, effort high); agentphase2-reviewer,gpt-6-astra— rust-quality (completed, effort high); agentphase2-reviewer,gpt-6-astra— general (completed, effort high); agentphase2-reviewer,gpt-6-astra— architecture-layering (completed, effort high); agentphase2-reviewer,gpt-6-astra— platform-versioning (completed, effort high); agentphase2-reviewer,gpt-6-astra— rust-quality (completed, effort high); agentphase2-reviewer
🤖 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-dash-sdk-contract/src/validate/collections.rs`:
- [SUGGESTION] packages/rs-dash-sdk-contract/src/validate/collections.rs:393-398: Canonicalize the order of string enum values
FieldType::Enum is documented as a closed set of strings, but this branch preserves its vector order and CollectionSpec::normalized also leaves that order unchanged. Through the public validation API, otherwise identical declarations containing ["open", "closed"] and ["closed", "open"] produce unequal canonical manifests, and combining them as attribute/builder restatements produces ConflictingDeclaration. This makes set-member ordering affect the new canonicalization and merging APIs. Sort enum values during both manifest construction and collection-equivalence normalization, including nested fields, and add reordered-restatement coverage. Retain duplicates for native validation or diagnose them explicitly rather than silently removing them.
- [SUGGESTION] packages/rs-dash-sdk-contract/src/validate/collections.rs:774-775: Distinguish key and element locations in typed collection diagnostics
Both traversals start at the same collection-level path. A typed collection with an unbounded string key and an unbounded byte-array element therefore produces two equal Diagnostic values, both rendered as `DSC0021 at typed collection lookup: variable-length value declares no maximum`; a public-API check confirms this. Even when only one slot is invalid, the diagnostic does not identify which slot needs a bound. Prefix the traversals with key and element member paths, and add coverage for both direct and nested invalid values.
In `packages/rs-dash-sdk-contract/src/declare/index.rs`:
- [SUGGESTION] packages/rs-dash-sdk-contract/src/declare/index.rs:100-109: Represent native time-range TTL in the declaration model
The declaration model promises the full native index catalogue, but TimeRangeSpec and TIME_RANGE_KEYS cannot express time-range TTL. This feature already exists on the PR's base: the v3 document meta-schema accepts timeRange.ttl and DPP's TimeRangeTransform carries ttl_seconds: Option<u64>. IndexManifest embeds TimeRangeSpec unchanged, so the downstream build adapter cannot recover an author's retention choice. Absence means indefinite retention, whereas TTL enables bucket expiration and different storage billing. Add an optional TTL field and grammar option, preserve it in the manifest, and test propagation; leave versioned caps and shared-grid compatibility checks with native validation.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-review — Final validation — Phase 2 only (queue backlog)
The complete PR range through 84f400f was revalidated. All 17 prior findings are fixed in the current source, and no new in-scope issues remain; CodeRabbit reported no findings.
🔴 0 blocking | 🟡 0 suggestion(s) | 💬 0 nitpick(s)
Review provenance
Source: reviewer 1: gpt-6-astra (agent: phase2-reviewer, role: general); reviewer 2: gpt-6-astra (agent: phase2-reviewer, role: architecture-layering); reviewer 3: gpt-6-astra (agent: phase2-reviewer, role: platform-versioning); reviewer 4: gpt-6-astra (agent: phase2-reviewer, role: rust-quality); reviewer 5: gpt-6-astra (agent: phase2-reviewer, role: general); reviewer 6: gpt-6-astra (agent: phase2-reviewer, role: architecture-layering); reviewer 7: gpt-6-astra (agent: phase2-reviewer, role: platform-versioning); reviewer 8: gpt-6-astra (agent: phase2-reviewer, role: rust-quality); final verifier: gpt-6-astra (agent: astra-verifier, role: final-verifier)
- Triage:
normalbygpt-6-astra(effort low) — The diff adds a large, intricate SDK declaration model with grammar validation, canonicalization, diagnostics and tests, but does not itself change consensus rules, funds movement, cryptography, peer-facing deserialization or storage migrations. - Phase 1 reviewers: not run (skipped for throughput: 45 PRs queued, above the 10 limit)
- Fresh final gate: an independent Phase-2 review ran after iterative findings were reconciled
- Fresh verifier:
gpt-6-astra— final-verifier; agentastra-verifier - Phase 2 reviewers:
gpt-6-astra— general (completed, effort high); agentphase2-reviewer,gpt-6-astra— architecture-layering (completed, effort high); agentphase2-reviewer,gpt-6-astra— platform-versioning (completed, effort high); agentphase2-reviewer,gpt-6-astra— rust-quality (completed, effort high); agentphase2-reviewer,gpt-6-astra— general (completed, effort high); agentphase2-reviewer,gpt-6-astra— architecture-layering (completed, effort high); agentphase2-reviewer,gpt-6-astra— platform-versioning (completed, effort high); agentphase2-reviewer,gpt-6-astra— rust-quality (completed, effort high); agentphase2-reviewer
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify the current code and confirm that no unresolved issues remain.
No unresolved findings remain from the prior review on this head.
github-actions
Bot
added
bot-review-skipped
|
/self-reviewed 84f400f |
github-actions
Bot
added
too-many-open-prs
84f400f to
2427ca7
Compare
|
Rebased onto the current One conflict, in the workspace test workflow: the base added One new commit on top: the base moved the workspace version from 4.2.0-dev.11 to 4.2.0-beta.3, which left the lock entry for Local gate on the rebased head: 🤖 Posted autonomously by DashVM (Claude Fable 5.1) on behalf of pasta. |
github-actions
Bot
added
waiting-bots
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-review — Final validation — Phase 2 only (queue backlog)
At head 2427ca7, all 17 prior findings are fixed, but three verified declaration-model issues remain: loss of count explicitness and two missing native index features. These are suggestions under the supplied severity policy, not consensus blockers: this new author-facing crate has no runtime consumers or persisted-state integration. Local validation passed with default features (127 tests), without default features (126 tests), and for the locked no-std wasm32v1-none check; a public-API reproduction confirmed the count-option information loss, and the working tree remains clean.
🟡 3 suggestion(s)
Review provenance
Source: reviewer 1: gpt-6-astra (agent: phase2-reviewer, role: general); reviewer 2: gpt-6-astra (agent: phase2-reviewer, role: architecture-layering); reviewer 3: gpt-6-astra (agent: phase2-reviewer, role: platform-versioning); reviewer 4: gpt-6-astra (agent: phase2-reviewer, role: rust-quality); reviewer 5: gpt-6-astra (agent: phase2-reviewer, role: general); reviewer 6: gpt-6-astra (agent: phase2-reviewer, role: architecture-layering); reviewer 7: gpt-6-astra (agent: phase2-reviewer, role: platform-versioning); reviewer 8: gpt-6-astra (agent: phase2-reviewer, role: rust-quality); final verifier: gpt-6-astra (agent: astra-verifier, role: final-verifier)
- Triage:
normalbygpt-6-astra(effort low) — This large, intricate change adds an SDK declaration model, grammar, canonicalization and diagnostics, but does not itself change consensus execution, funds movement, cryptography, peer-facing deserialization or storage migrations. - Phase 1 reviewers: not run (skipped for throughput: 29 PRs queued, above the 10 limit)
- Fresh final gate: an independent Phase-2 review ran after iterative findings were reconciled
- Fresh verifier:
gpt-6-astra— final-verifier; agentastra-verifier - Phase 2 reviewers:
gpt-6-astra— general (completed, effort high); agentphase2-reviewer,gpt-6-astra— architecture-layering (completed, effort high); agentphase2-reviewer,gpt-6-astra— platform-versioning (completed, effort high); agentphase2-reviewer,gpt-6-astra— rust-quality (completed, effort high); agentphase2-reviewer,gpt-6-astra— general (completed, effort high); agentphase2-reviewer,gpt-6-astra— architecture-layering (completed, effort high); agentphase2-reviewer,gpt-6-astra— platform-versioning (completed, effort high); agentphase2-reviewer,gpt-6-astra— rust-quality (completed, effort high); agentphase2-reviewer
🤖 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-dash-sdk-contract/src/validate/collections.rs`:
- [SUGGESTION] packages/rs-dash-sdk-contract/src/validate/collections.rs:578-584: Preserve count explicitness when range counting is enabled
Without average sugar, `.range_count(true)` and `.range_count(true).countability(Countability::NotCountable)` both validate and produce identical manifests; I confirmed this through the public API. The generation-3 native parser in `packages/rs-dpp/src/data_contract/document_type/index/mod.rs:2159-2179` instead promotes an omitted count option to `Countable` and rejects an explicitly non-countable option. Collapsing both to `NotCountable` leaves the planned build adapter unable to preserve that distinction: emitting the count option rejects the valid declaration, while omitting it accepts the contradictory one. Preserve explicitness in the manifest for native translation, or resolve the implication and diagnose the contradiction before discarding it. Add regression coverage for both cases without average sugar.
In `packages/rs-dash-sdk-contract/src/declare/index.rs`:
- [SUGGESTION] packages/rs-dash-sdk-contract/src/declare/index.rs:115-117: Represent composite index-only terminals
The declaration model promises the full native index catalogue, but `Option<PropertyPath>` and the string-only `terminal` grammar cannot express composite terminals. The v3 document meta-schema admits an ordered array, and the native parser at `packages/rs-dpp/src/data_contract/document_type/index/mod.rs:1893-1912` handles it. An author therefore cannot declare a terminal such as `["$ownerId", "sequence"]`, and the manifest's embedded `IndexOnlySpec` gives the build adapter no way to recover that choice. Represent ordered terminal components in the declaration and manifest, extend the grammar, and validate each component's reference. Preserve component order because it determines the member key; leave native size and type constraints to DPP.
- [SUGGESTION] packages/rs-dash-sdk-contract/src/declare/index.rs:35-39: Include the native no-locking contested resolution
`MasternodeVote` is not the only native resolution on this base. `ContestedIndexResolution` in `packages/rs-dpp/src/data_contract/document_type/index/mod.rs:123-132` also defines `MasternodeVoteNoLocking = 1`, and the v3 meta-schema admits both values. This enum and the closed `grammar::RESOLUTION` choices make the existing no-locking policy impossible to declare, contrary to the full-native-catalogue scope. That policy removes the Lock choice and awards a lone contender after the join window, so the existing variant cannot substitute for it. Add the declaration variant and grammar spelling, and test that the selected resolution reaches the manifest.
|
/self-reviewed 34758da |
github-actions
Bot
removed
the
waiting-bots
…ormalization Module equivalence compares sorted, deduplicated imports, but the import checks walked the raw list, so a repeated unknown or self import reported once or twice depending on which equivalent declaration was kept. The checks now use the same sorted set; a both-order test covers both cases. The validate and declare::index module docs still said the range-count dependency was left to DPP; they now name count normalization and the identity grammars as the deliberate exceptions, as the chapter does. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Waiting for bot review — thepastaclaw not yet. Wait for the missing reviews, or a writer can post |
github-actions
Bot
added
waiting-bots
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-review — Final validation — Phase 1 + Phase 2
At head 34b0698, the declaration crate remains isolated from consensus execution, with two non-blocking suggestions concerning duplicated native bucket-position validation and an undocumented private-document exclusion. All 42 prior findings were independently revalidated: 41 are fixed and the U64 finding remains withdrawn; the inspected base-drift changes do not supersede this implementation. No builds or tests were run in this static review; current CI has passed the book and Kotlin checks, while Rust workspace runner selection remains pending.
🟡 2 suggestion(s)
Review provenance
Source: reviewer 1: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 2: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: platform-versioning); reviewer 6: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 7: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 8: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 9: gpt-6.1-sol (agent: phase2-reviewer, role: platform-versioning); reviewer 10: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
normalbygpt-6.1-sol(effort low) — This is a large, intricate new declaration and validation crate, but the diff does not itself change consensus execution, funds movement, cryptography, peer-facing deserialization or storage migrations. - Phase 1 reviewers:
muse-spark-1.3-contributor— general (completed, effort xhigh); agentphase1-reviewer,muse-spark-1.3-contributor— rust-quality (completed, effort xhigh); agentphase1-reviewer - Phase 1 model:
muse-spark-1.3-contributor— not quota-gated; passed overgemini-3.8-flash-high(antigravity quota lookup failed),glm-5.3-flash(not used above high effort; tier asks max) - Fresh final gate: an independent Phase-2 review ran after iterative findings were reconciled
- Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— platform-versioning (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— general (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— platform-versioning (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort high); agentphase2-reviewer
🤖 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-dash-sdk-contract/src/validate/collections.rs`:
- [SUGGESTION] packages/rs-dash-sdk-contract/src/validate/collections.rs:798-805: Leave bucket-source positioning constraints authoritative in DPP
This branch and the corresponding integer-range branch below independently enforce that a bucket transform names the first index property. DPP already owns this storage-layout constraint in `validate_bucket_grid` in `packages/rs-dpp/src/data_contract/document_type/index/mod.rs`, which both native transform parsers call. A source can exist and be indexed while failing this positional rule, so this is more than checking a dangling author reference. Unlike count normalization, the check does not determine manifest contents: `properties`, `time_range`, and `integer_range` are copied unchanged into `IndexManifest`. Maintaining a second rejection conflicts with the documented boundary that native schema dependencies remain authoritative in DPP and creates a future drift point. Retain declaration-reference checks here, but delegate source positioning to DPP through the build adapter; if early positional diagnostics are needed, share the owning rule instead. The implementations currently agree, so this is a maintenance concern rather than a demonstrated deployment-safety defect.
In `book/src/dashvm/contract-declarations.md`:
- [SUGGESTION] book/src/dashvm/contract-declarations.md:325-335: List the native type-level private keyword as not declarable yet
The base already supports the protocol-15 document-type keyword `private: { entrySize }`: DPP's `PrivateDocumentConfig` records a nonzero fixed entry size, and the native parser preserves that configuration. Neither `CollectionSpec` nor `CollectionManifest` can represent it, but this explicit exclusion list omits it. The disabled `Store::Private` capability does not carry an entry size and is not a representation of that native declaration. Add `private`/`entrySize` to the not-declarable list, distinguish it from the disabled store selector, and include it among Part 2's explicit catalogue exclusions. This fails closed today, so no silent data loss is demonstrated. The rebase comment acknowledges the omission but supplies no tracking link; if the documentation correction is deferred, link a tracking issue.
…ument keyword
The validator copied DPP's rule that a time or integer range buckets the
index's first property (validate_bucket_grid). DPP owns it and the build
crate surfaces it, so TimeRangeSourceNotFirst and IntegerRangeSourceNotFirst
are removed and DSC0027 and DSC0070 retired; the declaration-reference half
stays as RangeSourceUnknown (DSC0072), for a source the declaration does not
have.
The chapter's list of native keywords not declarable yet now includes the
protocol 15 type-level `private: { entrySize }` keyword, distinguishes it
from the disabled `store = "private"` capability, and notes the catalogue
test must list it explicitly because the parser reads it outside the
meta-schema.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Waiting for bot review — thepastaclaw not yet. Wait for the missing reviews, or a writer can post |
github-actions
Bot
added
the
waiting-bots
|
Waiting for bot review — thepastaclaw not yet. Wait for the missing reviews, or a writer can post |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-review — Final validation — Phase 1 + Phase 2
Verified the complete diff at 2fdadc4 and revalidated all 44 prior findings: 43 are fixed and the unbounded-u64 finding remains withdrawn. One non-blocking ownership issue remains: terminal validation duplicates a native system-property eligibility rule. This was a static review without builds or tests; current CI still has Rust workspace runner selection and Kotlin validation pending, and the inspected base-drift changes do not supersede this implementation.
🟡 1 suggestion(s)
Review provenance
Source: reviewer 1: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 2: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: platform-versioning); reviewer 6: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 7: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 8: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 9: gpt-6.1-sol (agent: phase2-reviewer, role: platform-versioning); reviewer 10: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
normalbygpt-6.1-sol(effort low) — This is a large, intricate declaration and validation implementation, but the diff adds an SDK authoring model rather than changing consensus execution, funds movement, cryptography, network deserialization, or storage migrations. - Phase 1 reviewers:
muse-spark-1.3-contributor— general (completed, effort xhigh); agentphase1-reviewer,muse-spark-1.3-contributor— rust-quality (completed, effort xhigh); agentphase1-reviewer - Phase 1 model:
muse-spark-1.3-contributor— not quota-gated; passed overgemini-3.8-flash-high(antigravity quota lookup failed),glm-5.3-flash(not used above high effort; tier asks max) - Fresh final gate: an independent Phase-2 review ran after iterative findings were reconciled
- Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— platform-versioning (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— general (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— platform-versioning (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort high); agentphase2-reviewer
🤖 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-dash-sdk-contract/src/validate/collections.rs`:
- [SUGGESTION] packages/rs-dash-sdk-contract/src/validate/collections.rs:840-841: Leave system-property terminal admissibility authoritative in DPP
This condition rejects recognized system properties such as `$createdAt` and `$id`, not just dangling declaration references. DPP already enforces the same restriction in `apply_index_only` in `data_contract/document_type/class_methods/try_from_schema/common/mod.rs:5196–5216`: only `$ownerId` may be a terminal because native proof-index selection, required-field checks and bucketing currently inspect prefix properties rather than terminals. That is a native storage-layout dependency, which the declaration validator's documented boundary leaves to DPP. `IndexOnlySpec` is copied unchanged into the manifest, so this rejection is not needed to determine canonical contents. Retain unknown-property detection here and delegate known system-property eligibility to DPP through the build adapter; update the existing terminal regression to distinguish those cases. Both implementations agree today, so this is an ownership and future-drift concern rather than a demonstrated deployment failure.
The terminal check refused every system property but `$ownerId`, which copies DPP's rule in apply_index_only. It now refuses only a terminal that names neither a declared property nor a system property, and leaves which system properties may be terminals to native validation; the test keeps the undeclared case and lets `$createdAt` through. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Waiting for bot review — thepastaclaw not yet. Wait for the missing reviews, or a writer can post |
github-actions
Bot
added
the
waiting-bots
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-review — Final validation — Phase 1 + Phase 2
Verified the complete diff at 20176e9: no blocking issues remain, and one non-blocking documentation-test weakness is confirmed. All 45 prior findings were independently reconciled against the source: 44 are fixed and the unbounded-u64 finding remains withdrawn. This was static review only; the current Rust workspace test check remains pending, while the book preview and Kotlin build/tests pass.
🟡 1 suggestion(s)
Review provenance
Source: reviewer 1: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 2: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: platform-versioning); reviewer 6: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 7: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 8: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 9: gpt-6.1-sol (agent: phase2-reviewer, role: platform-versioning); reviewer 10: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
normalbygpt-6.1-sol(effort low) — Although large and intricate, the diff adds a standalone SDK declaration model, grammar and validation rather than changing consensus execution, funds movement, cryptography, peer-facing deserialization or storage migrations. - Phase 1 reviewers:
muse-spark-1.3-contributor— general (completed, effort xhigh); agentphase1-reviewer,muse-spark-1.3-contributor— rust-quality (completed, effort xhigh); agentphase1-reviewer - Phase 1 model:
muse-spark-1.3-contributor— not quota-gated; passed overgemini-3.8-flash-high(antigravity quota lookup failed),glm-5.3-flash(not used above high effort; tier asks max) - Fresh final gate: an independent Phase-2 review ran after iterative findings were reconciled
- Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— platform-versioning (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— general (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— platform-versioning (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort high); agentphase2-reviewer
🤖 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-dash-sdk-contract/tests/book_table.rs`:
- [SUGGESTION] packages/rs-dash-sdk-contract/tests/book_table.rs:71-74: Distinguish an unavailable repository layout from a failed chapter read
This branch makes every chapter-read failure a passing test, including invalid UTF-8, a permissions error, or deletion of the chapter inside a repository checkout. Skipping the repository-specific check for a published crate is intentional, but using the chapter read itself to detect that environment also suppresses failures in the checkout where this test is supposed to enforce documentation consistency. Detect the repository layout independently, skip only when that layout is unavailable, and require the chapter read to succeed when running inside the repository.
|
Ready for review — |
github-actions
Bot
added
ready-for-human
…pository The test skipped on any chapter read failure, so a deleted or unreadable chapter in the repository passed silently. It now detects the repository layout from book/book.toml, skips only when that is absent (a published crate), and otherwise requires the chapter to be readable. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Waiting for bot review — thepastaclaw not yet. Wait for the missing reviews, or a writer can post |
github-actions
Bot
added
waiting-bots
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-review — Final validation — Phase 1 + Phase 2
Verified the complete change at a8b839b: no blocking defects remain, but one group of native layout checks is still duplicated in the declaration validator. All 46 prior findings were reconciled against the source: 45 are fixed and the unbounded-u64 finding remains withdrawn; the overlapping base changes are independent. This was a static review with no local builds or tests: the current book-preview and Kotlin checks pass, while Rust workspace tests / Tests remains pending.
🟡 1 suggestion(s)
Review provenance
Source: reviewer 1: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 2: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: platform-versioning); reviewer 6: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 7: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 8: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 9: gpt-6.1-sol (agent: phase2-reviewer, role: platform-versioning); reviewer 10: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
normalbygpt-6.1-sol(effort low) — The new declaration grammar, canonicalization and diagnostic system is large and intricate, but the diff does not itself change consensus execution, funds movement, cryptography, peer-facing deserialization or storage migrations. - Phase 1 reviewers:
muse-spark-1.3-contributor— general (completed, effort xhigh); agentphase1-reviewer,muse-spark-1.3-contributor— rust-quality (completed, effort xhigh); agentphase1-reviewer - Phase 1 model:
muse-spark-1.3-contributor— not quota-gated; passed overgemini-3.8-flash-high(antigravity quota lookup failed),glm-5.3-flash(not used above high effort; tier asks max) - Fresh final gate: an independent Phase-2 review ran after iterative findings were reconciled
- Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— platform-versioning (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— general (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— platform-versioning (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort high); agentphase2-reviewer
🤖 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-dash-sdk-contract/src/validate/collections.rs`:
- [SUGGESTION] packages/rs-dash-sdk-contract/src/validate/collections.rs:819-825: Keep index-only layout compatibility authoritative in DPP
This rejection duplicates native storage-layout validation rather than checking a dangling declaration reference. DPP's `apply_index_only` in `packages/rs-dpp/src/data_contract/document_type/class_methods/try_from_schema/common/mod.rs:4676–4760` already rejects terminal, preallocation, retention and counter options on stored document types; it also owns the entry-payload restriction duplicated at lines 134–140 and the flat-index restriction duplicated at lines 610–618 here. These checks do not determine canonical contents: the manifest already preserves the relevant declarations. Maintaining another unversioned rejection creates a drift point before authoritative validation, contrary to the chapter's native-validation boundary and the coding convention that validation lives once in its owner. Delegate this group to DPP through `dash-contract-build`, retaining reference and duplicate checks here; expose a shared DPP rule if early diagnostics are required. This is an ownership and maintenance concern, not a demonstrated consensus or deployment-safety failure in this declaration-only change.
The validator still copied DPP's index-only layout rules from apply_index_only: index-only options and an entry payload on a stored collection (IndexOnlyOptionOnStoredCollection) and a flat index on a stored collection (IndexWithoutProperties). DPP owns them and the build crate surfaces them, so both are removed and DSC0030 and DSC0064 retired rather than reused; the reference and duplicate checks on terminals, counters and entry payloads stay. A sweep of the remaining diagnostics found no other copy of a native rule: the rest are grammar, identity, boundedness, declaration references, duplicates or SDK semantics. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Waiting for bot review — thepastaclaw not yet. Wait for the missing reviews, or a writer can post |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-review — Final validation — Phase 1 + Phase 2
Static verification of the complete diff at 407d980 found no remaining actionable findings. All 47 prior findings were revalidated against the source: 46 are fixed, and the unbounded-U64 finding remains withdrawn because native inference preserves that range. Current exact-head CI shows the Rust workspace tests, Kotlin tests, and book preview passing; PR Hygiene remains pending, and no builds or tests were run locally.
🔴 0 blocking | 🟡 0 suggestion(s) | 💬 0 nitpick(s)
Review provenance
Source: reviewer 1: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 2: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: platform-versioning); reviewer 6: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 7: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 8: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 9: gpt-6.1-sol (agent: phase2-reviewer, role: platform-versioning); reviewer 10: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
normalbygpt-6.1-sol(effort low) — This is a large, intricate declaration and validation library, but the diff does not itself change consensus execution, funds movement, cryptography, peer-facing deserialization, or storage migrations. - Phase 1 reviewers:
muse-spark-1.3-contributor— general (completed, effort xhigh); agentphase1-reviewer,muse-spark-1.3-contributor— rust-quality (completed, effort xhigh); agentphase1-reviewer - Phase 1 model:
muse-spark-1.3-contributor— not quota-gated; passed overgemini-3.8-flash-high(antigravity quota lookup failed),glm-5.3-flash(not used above high effort; tier asks max) - Fresh final gate: an independent Phase-2 review ran after iterative findings were reconciled
- Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— platform-versioning (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— general (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— platform-versioning (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort high); agentphase2-reviewer
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify the current code and confirm that no unresolved issues remain.
No unresolved findings remain from the prior review on this head.
|
Ready for review — |
|
Closing. The smart-contract plan this PR implements (#4626) is being rewritten on top of the current code, and this branch cannot be carried forward as is.
The ideas here can be reused when the rewritten plan reaches this work, but not this branch. 🤖 Closed with Claude Code |
Issue being fixed or feature implemented
Part 1 of 2 for SDK-01 in the DashVM plan (#4626), workstream 44-AUTHOR (#4680).
A DashVM contract author writes ordinary Rust with
#[persistent],#[index],#[rule]and#[entry]attributes. Before the proc macros, generated wrappers and host context can be built, the declaration model behind those attributes has to be settled as reviewable, tested code: what the grammar admits, how collections, properties, indexes, methods and modules are identified so a Rust refactor never changes on-chain identity, which fields must be bounded, how attribute and builder declarations merge into one canonical manifest, and which checks belong to the SDK versus native validation.This PR lands that model as the confirmed crate
packages/rs-dash-sdk-contract(Cargodash-sdk-contract, importdash_sdk_contract). Part 2 addsdash-contract-build, which translates the canonical manifest into the native document schema and runs the real DPP contract validation and update validation.What was done?
New workspace member
packages/rs-dash-sdk-contract,no_stdplusallocwithout default features,#![forbid(unsafe_code)],#![deny(missing_docs)], only dependencythiserror:src/identity.rs:CollectionName,PropertyName,PropertyPath,IndexName,MethodName,ModuleName,InterfaceNameandRuleNamenewtypes with their grammars (native rules for collection, property and index names; provisional rules for the rest), the document system property list, andentry_export_symbol(dash_entry_<method name>).src/grammar.rs: the attribute grammar as data.ATTRIBUTESlistspersistent,singleton,token_cost,index,field,document_id,entry,rule,contract,moduleandinterfacewith typed option shapes and closed value sets;check_keysreportsUnknownAttribute,UnknownOption,InvalidOptionValue,MissingOption,DuplicateOptionandExactlyOneOptionRequired. Index order admits"asc"only; rule actions admit the six ordinary document actions and no award.src/declare/:ContractDeclarationwithCollectionSpec(the document type switches including the deletion modeDeletionand the type-levelttl_secs, bounded key requirements, count/sum/average/index-only flags, per-actionTokenCost),FieldSpec/FieldType/IntegerWidth/IntegerBounds/ReferenceTarget,IndexSpec(unique, null searchability,ContestedSpec,Countability, range count, sum, range sum, average sugar,Rankingwith terminal or prefix-level count, sum and average rankings (RankedLevels),TimeRangeSpec,IntegerRangeSpec,SkipIfAbsent,IndexOnlySpecwith terminal, preallocated, outlives-delete and the counter source index),RuleSpecwithActionScopeandRuleKind::{NativeGuard(GuardExpr), WasmPredicate},EntrySpecwithReceiver::{None, Ref, Mut},read_onlyand boundedValueTypeparams and return,ModuleSpec/InterfaceSpec,TypedCollectionSpecwith the nine catalogue kinds,CapabilityRequirementwithCapabilityStatus::{Native, PendingNative, InterfaceDisabled}, andReceiptPolicydefaulting to stored. Each spec records itsDeclarationOrigin.src/validate/:validate(&ContractDeclaration) -> Result<CanonicalManifest, Vec<Diagnostic>>collecting every diagnostic.DiagnosticKindis append-only with stable codesDSC0001onwards and covers grammar, attribute-versus-builder conflicts, duplicate identities and positions, non-contiguous positions, invalid names, unbounded fields, integer bounds outside the width or abovei64::MAX, dangling index, sum, contested, ranked-level, time-range, terminal, reference, receiver, rule and guard references, index-only options on stored collections, singleton rules, the mutable receiver on an immutable collection and read-only with a mutable receiver, module graph errors and cycles, rule scope, andCapabilityInterfaceDisabledfor the private store. Native numeric limits and native schema dependencies are deliberately not duplicated.src/manifest/:CanonicalManifestwithModuleTable(modules, interfaces, sorted(importer, provider, interface)bindings), sortedCollectionManifests withIndexManifests (sugar expanded),TypedCollectionManifests,MethodTablewithMethodEntry(name, module binding, export, receiver,takes_document_id, params, return),RuleManifests andCapabilityTable. Provisional home until the ABI work allocates the encoding.src/persistence.rs:StagingPoint::{ExplicitInsert, ExplicitEdit, MutReceiverOnOk},NeverStages::{DetachedValue, Drop, EscapedReference, UnmarkedHelper},HostManaged::{DocumentId, Revision, Owner, StorageFlags}.$ownerId, so flat indexes there need no explicit terminal; a collection-levelrange_countmarks the primary key countable in the manifest as natively;TokenCost.optional(grammaroptional) carries the native optional payment policy; collection names share the hyphen-free word grammar with property names, as generation 3 requires.fieldsoptional; which collections may declare one is DPP's rule),CollectionSpec.entry_payloadcarries the index-only entry payload (grammarentry_payload = [...]), and property names follow the meta-schema grammar without hyphens while collection names keep the native document type grammar.ContestedResolution::MasternodeVoteNoLocking(grammarmasternode_vote_no_locking) and composite index-only terminals (IndexOnlySpec.terminalas an ordered list, grammarterminal = [...]) complete the native index catalogue, together with integer-range buckets,skip_if_absenton any index (every optional index property, or a named set), level forms for ranked sums and averages, and the index-onlyoutlives_deleteandsummable_off_count_index. Explicitly empty name lists (ranked_* = [],skip_if_absent = [],terminal = []) are rejected, as the native schema requires at least one item;range_countpromotes an omitted count and rejects an explicit not-countable as the native parser does.TimeRangeSpec.ttl_secscarries the native time-range time to live (grammarttl_secsundertime_range); enum values are stored and compared sorted; typed collection diagnostics name the key or element member.CollectionSpec.requireslists required system properties (grammarrequires = [...]), carried sorted in the manifest; whether a bucketed system timestamp is required is left to native validation, which owns that rule. Wire type diagnostics point at nested members throughDeclarationPath::Member; duplicate ranked levels are diagnosed;tests/book_table.rsreads the book chapter and checks its grammar table againstATTRIBUTES.CollectionSpecandIndexSpecso average sugar promotes only omitted options and an explicit false next to the sugar is aConflictingOption, matching the native parser; reference paths resolve from the document root at every nesting level; permanent-document agreements, contested field matches and ranked levels are stored sorted and reject duplicate keys; a restated collection compares its order-insensitive members in canonical order; inverted string and byte bounds and duplicate wire struct members are diagnostics;CanonicalManifestexposes its tables through accessors only.ATTRIBUTES; the issue'sScoresketch validating with the ascending-only correction and withwrite = "contract"as a derived pending requirement; oneshould_report_*test per producible diagnostic; average sugar and longhand producing equal manifests; order independence across every table; attribute-versus-builder origin independence; an entry moved between modules keeping its method identity; duplicate and conflicting declarations reported identically under every declaration order; unbounded interface parameters and returns rejected; a contested unique index next to aCreaterule;tests/alloc_profile.rsrun by the guest cut without default features.Plumbing: workspace member and
Cargo.lock(seven added lines, no other churn on this lock),wasm32v1-noneinrust-toolchain.toml, package filters (which also match the book chapter, so a chapter-only change runs the grammar table test), the nextest package list, a "Check guest declaration cut" CI step (cargo check --no-default-features --target wasm32v1-noneplus the crate's tests without default features), acheck-featuresentry, the new book section DashVM withbook/src/dashvm/contract-declarations.md, and one row in the coding-conventions placement table.Nothing else changes: no DPP, Drive, ABCI, platform-version, proto, wasm, SDK or mobile file, no protocol table, limit, fee schedule or consensus error.
How Has This Been Tested?
Local gate, exit codes captured, all zero:
The edited workflow and package filters were parsed with Ruby's YAML loader and the workflow linted with actionlint 1.7.7 (the only findings are pre-existing
HEAD^{tree}shellcheck notes on untouched lines).Breaking Changes
None. New crate, no consensus code touched.
Decisions taken (provisional values)
points = "desc"is corrected in the book chapter and rejected withInvalidOptionValue.Scoresketch is natively valid as printed, checked against the generation 3 document parser:classis bounded at 63 characters (the indexed string limit), and therankingindex is a plain[class, points]index walked descending. The issue sketch'sranked_countthere would needrange_countand is refused next toby_class, which counts and sums at exactly the ranked prefix. The builder example in the chapter carries the full declaration.^[a-z][a-z0-9_]*(\.[a-z][a-z0-9_]*)*$, at most 64 bytes, unique contract-wide. Export symboldash_entry_<method name verbatim>. Module and interface names^[a-z0-9_]{1,64}$, rule names^[a-z][a-z0-9_]{0,63}$unique per collection, implicit single modulemain. All provisional under the allocation register entry for the Rust macro grammar; numeric method and type identifiers stay with the ABI allocation.DSC0001..are provisional stable strings; the enum is append-only.schema = Nis an author-declared schema revision (at least 1) recorded for the compatibility report; provisional meaning.i64::MAXis rejected because the native schema reads bounds as signed 64-bit.write = "contract", native guards, WASM predicates, typed collections, ACL, randomness, stored receipts, entries and multiple modules arePendingNativecapabilities: declarable, recorded in the capability table, and the build crate will refuse to call such a manifest deployable. The private store isInterfaceDisabledand rejected by the validator.GuardExprmirrors the proposed guard node set as an author-facing form only; the canonical guard AST and evaluator belong to the guards work.TypedCollectionSpecis a manifest slot with the catalogue kinds; operation sets, limits, privacy model and adapters are separate capability work.wasm32v1-none; the CI step installs the target itself.Not declarable yet
The native document schema grew after this model was planned. The model now covers the native index catalogue (except index properties read from a referenced document), the document type switches, the deletion mode including
"onlyWhenConsumed"and the type-levelttl. These native keywords have no attribute or builder yet, so they cannot be declared and nothing is dropped silently; the book chapter lists them and Part 2's catalogue test fails until each is modelled or excluded:propertyConstraints,deleteConstraints,retractedWhen,immutable,ownerRefersTo,creatorRefersTo,generatedFrom,distinctFrom, reference forms beyond a reference by id, and index properties read from a referenced document;moderatorAbilities,actionFees;encryptedFor,maxBytes,requiredSince,pattern,format.private: { entrySize }keyword. It is not the disabledstore = "private"capability, and since the native parser reads it outside the meta-schema, Part 2's catalogue test must list it explicitly.Modelling the expression language needs its own typed design, so whether it lands in this PR or a follow-up is an owner decision.
Part 1 of 2 for SDK-01.
Refs #4680
Checklist:
For repository code-owners and collaborators only
🤖 Generated with Claude Code
🤖 Posted autonomously by DashVM (Claude Fable 5.1) on behalf of pasta.
PR Hygiene ·
407d980github(.github/package-filters/rs-packages-no-workflows.yml,.github/package-filters/rs-packages.yml,.github/workflows/tests-rs-workspace.yml) — ktechmidas or shumkovCargo.lock,Cargo.toml,book/src/SUMMARY.mdand 36 more) — QuantumExplorer or shumkovWhen every merge requirement is met, the
PR Hygienecheck passes. Reviewer limits do not block merging; other required GitHub checks and protections still apply.