feat: add the dash-sdk-contract declaration model, grammar and diagnostics - #4719
DCG-Claude wants to merge 8 commits into
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
|
📖 Book Preview built successfully. Download the preview from the workflow artifacts. Updated at 2026-09-16T03:02:43.555Z |
|
✅ Final review complete — no blockers (commit 84f400f) · triage: normal · Phase 2 only (queue backlog) |
|
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. |
…stics Add packages/rs-dash-sdk-contract (Cargo dash-sdk-contract, import dash_sdk_contract), the contract-author declaration model for DashVM: the attribute grammar as data, the typed declaration model and builders, the validator with typed append-only diagnostics, the sorted canonical manifest and the persistence semantics enums. The crate is no_std plus alloc without default features and depends on thiserror only. Refs #4680 Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Add the wasm32v1-none target to the toolchain, the package filters and the nextest package list for the new crate, a guest declaration cut step that checks the crate without std on wasm32v1-none and runs its tests without default features, and the check-features entry. Add the DashVM book section with the contract declarations chapter and the coding-conventions table row for author declarations. Refs #4680 Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Replace the closure-parameter dedupe with an Identified trait implemented per spec type, use validated collection summaries in the entry checks, and build grammar diagnostics with format. No behaviour change. Refs #4680 Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…ire types Group declarations by identity before judging them, so a repeated declaration of one origin is a duplicate and a disagreeing attribute and builder pair is a conflict whatever the declaration order. Run the bounded-value check on every interface function parameter and return under an interface parameter path. Refs #4680 Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…ve members Count, range count and range sum on collections and indexes are now optional so that average sugar promotes only what the author omitted and reports an explicit false as a conflicting option, matching the native parser. Reference paths resolve from the document root at every nesting level, permanent-document agreements are sorted and reject duplicate referring keys, and a restated collection compares fields, token costs, agreements, contested matches and ranked levels in canonical order so a reordering is never a conflict. Strings and byte arrays reject inverted length bounds, wire structs and contested matches reject duplicate keys, the cycle search states its stack invariant, and the canonical manifest exposes its tables through accessors only. Refs #4680 Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…py 1.98 The base bumped the workspace version to 4.2.0-dev.11 and the toolchain to 1.98.1, so the locked lock entry for dash-sdk-contract was stale and two token cost sorts tripped the unnecessary_sort_by lint on the newer clippy. Refs #4680 Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
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.
…diagnostics Collections gain a requires list of system properties (grammar key requires on persistent and singleton), carried sorted in the manifest, so a time-range index on a system timestamp can state the requirement the native parser demands instead of leaving the translator to invent it; a time range on an unrequired system timestamp is a diagnostic. Duplicate ranked levels are reported instead of silently deduplicated. Wire type diagnostics inside structs and lists point at the member through a new Member path segment. The book grammar table is now read from the chapter by an integration test and compared with ATTRIBUTES, and the in-crate table becomes an explicit grammar snapshot. Refs #4680 Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
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.
TimeRangeSpec gains an optional ttl_secs (grammar key ttl_secs under time_range) so the author's retention choice reaches the manifest; the native cap and grid rules stay native. Enum values are stored sorted in the manifest and compared sorted when merging a restatement, and typed collection diagnostics point at the key or element member. Refs #4680 Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
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.
2 participants
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(every native document type switch, 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 ranking,TimeRangeSpec,IndexOnlySpec),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}.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; a time range on an unrequired system timestamp isTimeRangeSourceNotRequired. 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, 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.^[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.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.
Automated reviewer consensus (Fable 5.1 implementer, GPT-6 Astra reviewer)
Reviewer consensus
Plan Review consensus
resolvedresolvedresolvedresolvedresolvedresolvedReview consensus
resolvedresolved