Repository navigation
refactor(dpp): own the contract-bounds verdict for a document create - #5660
Claudius-Maginificent wants to merge 6 commits into
Conversation
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
|
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
|
|
✅ Final review complete — no blockers (commit 0ec3c44) · triage: critical |
thepastaclaw
left a comment
There was a problem hiding this comment.
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:
criticalbygpt-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); 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) - 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; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— platform-versioning (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort xhigh); 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-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.
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
left a comment
There was a problem hiding this comment.
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:
criticalbygpt-6.1-sol(effort low) — This is a large, intricate refactor that changes shipped consensus-critical validation invalidate_advanced_structure_from_state_v1and 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); 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) - 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; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— platform-versioning (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— platform-versioning (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort xhigh); 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-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.
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
DocumentCreatebut 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?
identity_public_key/contract_bounds,state-transitionsfeature):ContractBounds::check_document_consumption(document_type)yields oneBatchedTransitionBoundsCheckper type the create may consume, inconsumable_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 askscheck_document_create.ContractBounds::check_document_create(document_type)combinescheck_scopeon the created type with the consumption checks into the single verdict a client asks before it builds and signs.advanced_structurev1: the inlinematch boundsover consumed types becomescreate_consumption_in_bounds, which askscheck_document_consumptionand 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.SigningScope::DocumentCreate: callscheck_document_create, and the copied consumed-type loop is gone. Answers are the same, includingRequiresContractGroupMembershipfor a group-bound key: the explicit check accepts it and automatic selection skips it, exactly as before.In-place changes to shipped generations
batch/advanced_structure/v1(validate_advanced_structure_from_state_v1), consumed-type bounds checkDRIVE_ABCI_VALIDATION_VERSIONS_V10, shipped in v5.0.1), and PV15 (V11, unreleased) throughadvanced_structurev2, which runs v1 firstBTreeSetorder, 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 sameaction.contract_group_memberships(&contract.id())lookup, with the sameCorruptedCodeExecutionerror when that is missing. Nothing is billed, as before. The paidContractBoundedKeyOutOfBoundsErrorand its nonce-bump action are untouched.Evidence:
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 thev5.0.1tag). 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.commit_reveal_lookup_tests):should_refuse_a_reveal_consuming_outside_the_signing_keys_bounds(adomain-bound key consuming apreorder→ 20014, commitment kept) and the newshould_accept_a_reveal_consuming_inside_the_signing_keys_bounds(a whole-contract-bound key → accepted, commitment consumed). Both run throughprocess_raw_state_transitionson a chain at protocol version 14 and at the latest. The fixture gainednew_at(platform_version, …);new_withdefaults tolatest().How Has This Been Tested?
cargo test -p dpp --features all_features --lib contract_bounds: newdocument_create_tests::should_check_each_consumed_type_once_the_create_is_coveredandshould_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 newshould_leave_a_group_bound_create_key_to_platform.cargo clippy --all-targets -D warningson 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:
structure.rs, regeneratedgrovedb-structure.json, and checked the structure viewer link posted on this pull requestFor repository code-owners and collaborators only
🤖 Generated with Claude Code
https://claude.ai/code/session_01WunYYjpfLux1ByK7fGF7SG
🤖 Co-authored by Claudius the Magnificent AI Agent