Skip to content

refactor(dpp): own the contract-bounds verdict for a document create - #5660

Open
Claudius-Maginificent wants to merge 6 commits into
feat/platform-wallet-public-signing-key-selectionfrom
refactor/document-create-bounds-in-dpp
Open

Claudius-Maginificent wants to merge 6 commits into
feat/platform-wallet-public-signing-key-selectionfrom
refactor/document-create-bounds-in-dpp

Conversation

@Claudius-Maginificent

@Claudius-Maginificent Claudius-Maginificent commented Oct 29, 2026 •

Copy link
Copy Markdown
Collaborator

TL;DR

The rule for whether a contract-bound key may sign a document create (the created type plus every type the create consumes) now lives once, in rs-dpp. Drive's batch validation and the wallet's key selection both call it, instead of Drive keeping the rule inline and the wallet keeping a copy. This is an internal refactor with no user-observable change: every protocol version accepts and refuses exactly what it did before, with the same consensus error and the same fees.

Stacked on #5588.

Issue being fixed or feature implemented

In #5588 the wallet learned to skip a key whose contract bounds cover a DocumentCreate but not the types that create consumes (refersTo … consume). To do that it copied the check from Drive's batch structure validation, and review on #5588 asked for one DPP-owned verdict instead. We deferred that until Drive itself could use it. This PR delivers it: per the coding conventions, validation lives once, in its owner.

What was done?

  • rs-dpp (identity_public_key/contract_bounds, state-transitions feature):
    • ContractBounds::check_document_consumption(document_type) yields one BatchedTransitionBoundsCheck per type the create may consume, in consumable_document_type_names() order, given that the bounds already cover the create. A whole-contract bound allows them. A document-type bound allows only its own type. A contract-group bound asks for the contract's membership for that type, with the contract id taken from the document type. It does not compare the bounds' contract id, as the shipped rule did not, so any other caller asks check_document_create.
    • ContractBounds::check_document_create(document_type) combines check_scope on the created type with the consumption checks into the single verdict a client asks before it builds and signs.
  • drive-abci, batch advanced_structure v1: the inline match bounds over consumed types becomes create_consumption_in_bounds, which asks check_document_consumption and resolves a group bound against the memberships the transformer already resolved for the create's contract. The lookup, the internal error when that lookup is missing, and the no-extra-fee behaviour are all unchanged.
  • rs-platform-wallet, SigningScope::DocumentCreate: calls check_document_create, and the copied consumed-type loop is gone. Answers are the same, including RequiresContractGroupMembership for a group-bound key: the explicit check accepts it and automatic selection skips it, exactly as before.

In-place changes to shipped generations

Edited generation Selected by Why consensus cannot change
batch/advanced_structure/v1 (validate_advanced_structure_from_state_v1), consumed-type bounds check PV14 (DRIVE_ABCI_VALIDATION_VERSIONS_V10, shipped in v5.0.1), and PV15 (V11, unreleased) through advanced_structure v2, which runs v1 first Pure refactor with identical output. For each consumed type, in the same BTreeSet order, the dpp check maps every bound exactly as the old inline arms did: SingleContract → allowed, SingleContractDocumentType → the same name comparison, ContractGroup → the same {Contract, DocumentType(consumed)} membership test against the same action.contract_group_memberships(&contract.id()) lookup, with the same CorruptedCodeExecution error when that is missing. Nothing is billed, as before. The paid ContractBoundedKeyOutOfBoundsError and its nonce-bump action are untouched.

Evidence:

  • Oracle test advanced_structure::v1::tests::consumption_in_bounds_matches_the_rule_it_replaced. The old inline rule is copied verbatim from v5.0.1 (v1 at the base of this branch is byte-identical to the v5.0.1 tag). The test compares it with the new path on every document type of DPNS and of the commit-reveal reference-validation contract, parsed at PV14 and at the latest version. It covers whole-contract, document-type (own, consumed, absent, foreign-contract) and group bounds × no memberships resolved, none held, the whole contract, a token, or each type in the key's group or another. Results and errors match on every input.
  • End-to-end at PV14 and latest (commit_reveal_lookup_tests): should_refuse_a_reveal_consuming_outside_the_signing_keys_bounds (a domain-bound key consuming a preorder → 20014, commitment kept) and the new should_accept_a_reveal_consuming_inside_the_signing_keys_bounds (a whole-contract-bound key → accepted, commitment consumed). Both run through process_raw_state_transitions on a chain at protocol version 14 and at the latest. The fixture gained new_at(platform_version, …); new_with defaults to latest().

How Has This Been Tested?

  • cargo test -p dpp --features all_features --lib contract_bounds: new document_create_tests::should_check_each_consumed_type_once_the_create_is_covered and should_require_a_create_key_to_cover_its_type_and_every_type_it_consumes.
  • cargo test -p drive-abci --lib -- advanced_structure::v1 commit_reveal_lookup contract_group_bound_auth: oracle, PV14 + latest end-to-end, and the existing group-bound batch tests.
  • cargo test -p platform-wallet --lib key_selection: the existing consume test unchanged, plus new should_leave_a_group_bound_create_key_to_platform.
  • cargo clippy --all-targets -D warnings on dpp + drive-abci and on platform-wallet (--all-features). cargo check -p dpp --no-default-features --features state-transitions. cargo fmt.

Breaking Changes

None. Additive dpp API. No protocol, fee or version-table change.

Checklist:

  • I have performed a self-review of my own code
  • My Rust, FFI, WASM, Swift or Kotlin changes follow the coding conventions
  • I have commented my code, particularly in hard-to-understand areas
  • I have added or updated relevant unit/integration/functional/e2e tests
  • I have added "!" to the title and described breaking changes in the corresponding section if my code contains any
  • I have made corresponding changes to the documentation if needed
  • If I added or changed GroveDB structure, I described it in the area's structure.rs, regenerated grovedb-structure.json, and checked the structure viewer link posted on this pull request
  • If I added or changed a system contract's document schemas, I checked the contract viewer link posted on this pull request

For repository code-owners and collaborators only

  • I have assigned this pull request to a milestone

🤖 Generated with Claude Code

https://claude.ai/code/session_01WunYYjpfLux1ByK7fGF7SG

🤖 Co-authored by Claudius the Magnificent AI Agent

lklimek and others added 4 commits October 29, 2026 09:52
ContractBounds::check_document_consumption yields what each type a create
may consume (`consume`) needs once the bounds cover the create itself;
ContractBounds::check_document_create combines it with the create's own
type into one verdict a client can ask before it builds and signs.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01WunYYjpfLux1ByK7fGF7SG
Batch advanced_structure v1 (PV14, and PV15 through v2) answers the
consumed-type check of a contract-bound key through
ContractBounds::check_document_consumption instead of its inline match.
Edited in place: output-identical, pinned by an oracle test against the
rule as shipped in v5.0.1 and by the bound-key consume reveal run at
protocol version 14 and the latest, refused and accepted.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01WunYYjpfLux1ByK7fGF7SG
SigningScope::DocumentCreate asks ContractBounds::check_document_create
instead of copying Drive's consumed-type rule. Same answers, a group-bound
key included, which a new test pins.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01WunYYjpfLux1ByK7fGF7SG
…ing-key-selection' into refactor/document-create-bounds-in-dpp
@coderabbitai

coderabbitai Bot commented Oct 29, 2026 •

Copy link
Copy Markdown
Contributor

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

Run configuration

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
  • 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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@thepastaclaw

thepastaclaw commented Oct 29, 2026 •

Copy link
Copy Markdown
Collaborator

✅ Final review complete — no blockers (commit 0ec3c44) · triage: critical

@Claudius-Maginificent
Claudius-Maginificent marked this pull request as ready for review October 29, 2026 12:13

@thepastaclaw thepastaclaw left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Final validation — Phase 1 + Phase 2

The refactor correctly centralizes document-create bounds decisions for wallet selection and Drive validation, and the available CI checks are green. Two non-blocking issues remain: DPP signing preflight does not use the new consumed-document verdict, and the new public consumption API accepts a redundant contract ID that can produce incorrect results for inconsistent callers.

🟡 2 suggestion(s)

1 finding(s) not shown inline (the lines are not part of this PR's diff)

🟡 Suggestion: Apply the document-create verdict in DPP signing preflight
packages/rs-dpp/src/state_transition/mod.rs:1996-1998

verify_identity_key_bounds still validates a batch only through check_batched_transition. For a document create, that checks the created document type but cannot check the types listed in the create's consume references. Both DPP document-create constructors receive the full DocumentTypeRef and then call sign_external_with_options, so a key bound to the domain type can sign a domain create that consumes preorder; the platform later rejects it with ContractBoundedKeyOutOfBoundsError (20014), potentially after the transition has incurred fees. Thread the document type or its consumable-type information into the signing preflight and apply check_document_create, while retaining the existing group-membership behavior for checks that only consensus can resolve.

source: muse-spark-1.3-contributor (phase1-reviewer: general, rust-quality); gpt-6.1-sol (phase2-reviewer: general, architecture-layering, platform-versioning, rust-quality, security-auditor)

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: security-auditor); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)

  • Triage: critical by gpt-6.1-sol (effort low) — The diff changes consensus-critical contract-bound validation in rs-drive-abci's advanced_structure v1 and also changes signing-key selection, so it is both intricate and directly affects consensus rules and key handling.
  • Phase 1 reviewers: muse-spark-1.3-contributor — general (completed, effort xhigh); agent phase1-reviewer, muse-spark-1.3-contributor — rust-quality (completed, effort xhigh); agent phase1-reviewer
  • Phase 1 model: muse-spark-1.3-contributor — not quota-gated; passed over gemini-3.8-flash-high (antigravity quota lookup failed), glm-5.3-flash (not used above high effort; tier asks max)
  • Single stage: Phase 1 and Phase 2 reviewed this head side by side, with no blocker gate between them (triage tier)
  • Fresh verifier: gpt-6.1-sol — final-verifier; agent sol-verifier
  • Phase 2 reviewers: gpt-6.1-sol — general (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — architecture-layering (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — platform-versioning (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — rust-quality (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — security-auditor (completed, effort xhigh); agent phase2-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-dpp/src/state_transition/mod.rs`:
- [SUGGESTION] packages/rs-dpp/src/state_transition/mod.rs:1996-1998: Apply the document-create verdict in DPP signing preflight
  `verify_identity_key_bounds` still validates a batch only through `check_batched_transition`. For a document create, that checks the created document type but cannot check the types listed in the create's `consume` references. Both DPP document-create constructors receive the full `DocumentTypeRef` and then call `sign_external_with_options`, so a key bound to the `domain` type can sign a `domain` create that consumes `preorder`; the platform later rejects it with `ContractBoundedKeyOutOfBoundsError` (20014), potentially after the transition has incurred fees. Thread the document type or its consumable-type information into the signing preflight and apply `check_document_create`, while retaining the existing group-membership behavior for checks that only consensus can resolve.

In `packages/rs-dpp/src/identity/identity_public_key/contract_bounds/mod.rs`:
- [SUGGESTION] packages/rs-dpp/src/identity/identity_public_key/contract_bounds/mod.rs:291-315: Make the consumption verdict derive and validate its contract ID
  `DocumentTypeRef` already contains the data-contract ID, but `check_document_consumption` accepts a second `contract_id` source of truth. The `SingleContract` arm ignores that argument, the `SingleContractDocumentType` arm checks only the consumed name, and the `ContractGroup` arm reports the caller-supplied ID. A direct caller that supplies an inconsistent contract ID can therefore receive `Allowed` for a foreign contract or a membership requirement for the wrong contract. Current callers pass matching IDs and first verify the create itself, so this does not change current behavior, but the new public consensus-adjacent API should be safe to call independently. Remove the redundant argument, derive `document_type.data_contract_id()`, and compare it with the bound contract ID in the single-contract arms; update the callers and tests accordingly.

lklimek and others added 2 commits October 30, 2026 10:31
The contract_id parameter only ever repeated the document type's own
id: every contract path that builds or re-ids a document type passes
the contract's id. Read it from the document type, as
check_document_create does. Drive's create_consumption_in_bounds drops
the parameter too; it ignored that field of the group check.

The function still does not compare the bounds' contract id, since a
comparison would change the output of shipped generation v1 on
foreign-contract bounds. Its doc comment now says so and points other
callers to check_document_create.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01WunYYjpfLux1ByK7fGF7SG
Both sides added the same new_at fixture to the commit-reveal lookup
tests and differed only in two doc comments. Keeps #5588's wording; no
code differs from either side there.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01WunYYjpfLux1ByK7fGF7SG

@thepastaclaw thepastaclaw left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-review — Final validation — Phase 1 + Phase 2

The refactor correctly centralizes the document-create contract-bounds verdict for Drive and wallet callers without changing the shipped validation behavior. One non-blocking gap remains: canonical DPP document-create signing still performs only the per-member bounds check and can sign a transition that Platform later rejects. No code-check failures are reported; the compatible runner check is still queued.

🟡 1 suggestion(s)

1 finding(s) not shown inline (the lines are not part of this PR's diff)

🟡 Suggestion: Apply the document-create verdict in DPP signing preflight
packages/rs-dpp/src/state_transition/mod.rs:2039-2041

verify_identity_key_bounds still checks a batch only by applying check_batched_transition to each explicit member. For a document create, that verifies the created type but not the types listed in its consume metadata. The DPP document-create constructors receive the required DocumentTypeRef and then call this unchanged preflight, so a key bound to the created type can still sign a create that consumes a type outside its bounds; Drive later rejects it with ContractBoundedKeyOutOfBoundsError during paid advanced-structure validation and performs the nonce-bump path. The new check_document_create verdict should be threaded into this signing boundary, or the signing API should explicitly expose that this preflight is incomplete for document consumption; do not duplicate the consumption rule in an SDK or wallet adapter.

source: muse-spark-1.3-contributor (phase1-reviewer: general, rust-quality); gpt-6.1-sol (phase2-reviewer: general, architecture-layering, rust-quality)

Review provenance

Source: reviewer 1: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: platform-versioning); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 5: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 6: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); reviewer 7: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); reviewer 8: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 9: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 10: gpt-6.1-sol (agent: phase2-reviewer, role: platform-versioning); reviewer 11: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 12: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)

  • Triage: critical by gpt-6.1-sol (effort low) — This is a large, intricate refactor that changes shipped consensus-critical validation in validate_advanced_structure_from_state_v1 and contract-bound signing-key selection, both of which require proving identical consensus and authorization outcomes.
  • Phase 1 reviewers: muse-spark-1.3-contributor — general (completed, effort xhigh); agent phase1-reviewer, muse-spark-1.3-contributor — rust-quality (completed, effort xhigh); agent phase1-reviewer
  • Phase 1 model: muse-spark-1.3-contributor — not quota-gated; passed over gemini-3.8-flash-high (antigravity quota lookup failed), glm-5.3-flash (not used above high effort; tier asks max)
  • Single stage: Phase 1 and Phase 2 reviewed this head side by side, with no blocker gate between them (triage tier)
  • Fresh final gate: an independent Phase-2 review ran after iterative findings were reconciled
  • Fresh verifier: gpt-6.1-sol — final-verifier; agent sol-verifier
  • Phase 2 reviewers: gpt-6.1-sol — general (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — architecture-layering (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — platform-versioning (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — rust-quality (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — security-auditor (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — general (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — architecture-layering (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — platform-versioning (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — rust-quality (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — security-auditor (completed, effort xhigh); agent phase2-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-dpp/src/state_transition/mod.rs`:
- [SUGGESTION] packages/rs-dpp/src/state_transition/mod.rs:2039-2041: Apply the document-create verdict in DPP signing preflight
  `verify_identity_key_bounds` still checks a batch only by applying `check_batched_transition` to each explicit member. For a document create, that verifies the created type but not the types listed in its `consume` metadata. The DPP document-create constructors receive the required `DocumentTypeRef` and then call this unchanged preflight, so a key bound to the created type can still sign a create that consumes a type outside its bounds; Drive later rejects it with `ContractBoundedKeyOutOfBoundsError` during paid advanced-structure validation and performs the nonce-bump path. The new `check_document_create` verdict should be threaded into this signing boundary, or the signing API should explicitly expose that this preflight is incomplete for document consumption; do not duplicate the consumption rule in an SDK or wallet adapter.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants