feat(platform)!: document history system contract with per-doctype opt-in#4171
Conversation
|
🕓 Ready for review — next in queue (commit 0753175) |
|
Warning Review limit reached
Next review available in: 26 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (21)
📝 WalkthroughWalkthroughAdds the DocumentHistory system contract, per-event retention settings, protocol 13 version wiring, DPNS v2 support, Drive history operations, and integration tests for transfer, purchase, pricing, migration, and rejection behavior. ChangesDocument-history contract package
System contract registration and availability
History configuration and document model
DPNS and protocol 13 support
History operation execution
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant DocumentTransition
participant DocumentEvent
participant Drive
participant DocumentHistoryContract
DocumentTransition->>DocumentEvent: create event payload
DocumentEvent->>DocumentHistoryContract: resolve event document type
DocumentEvent-->>Drive: build historical document
Drive->>DocumentHistoryContract: insert history document
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
packages/rs-dpp/src/document/document_event.rs (1)
53-59: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRedundant
Ok(...?)— clippy::needless_question_mark.
document_type_for_namealready returns the exactResult<DocumentTypeRef, ProtocolError>this function returns, so wrapping it inOk(...?)triggers clippy'sneedless_question_marklint.As per coding guidelines,
packages/**/*.rsshould "follow rustfmt defaults, keep code clippy-clean."♻️ Proposed fix
pub fn associated_document_type<'a>( &self, document_history_contract: &'a DataContract, ) -> Result<DocumentTypeRef<'a>, ProtocolError> { - Ok(document_history_contract - .document_type_for_name(self.associated_document_type_name())?) + document_history_contract.document_type_for_name(self.associated_document_type_name()) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/rs-dpp/src/document/document_event.rs` around lines 53 - 59, Update associated_document_type to return document_history_contract.document_type_for_name(self.associated_document_type_name()) directly, removing the redundant Ok wrapper and question-mark operator while preserving the existing Result<DocumentTypeRef, ProtocolError> behavior.Source: Coding guidelines
packages/document-history-contract/test/unit/documentHistoryContract.spec.js (1)
1-1: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePrefer kebab-case filenames.
As per coding guidelines, JavaScript packages should prefer kebab-case filenames. Consider renaming this file to
document-history-contract.spec.js.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/document-history-contract/test/unit/documentHistoryContract.spec.js` at line 1, Rename the test file from its camelCase filename to the kebab-case name document-history-contract.spec.js, updating any references to the existing filename as needed.Source: Coding guidelines
packages/document-history-contract/lib/systemIds.js (1)
1-1: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePrefer kebab-case filenames.
As per coding guidelines, JavaScript packages should prefer kebab-case filenames. Consider renaming this file to
system-ids.js.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/document-history-contract/lib/systemIds.js` at line 1, Rename the JavaScript file from systemIds.js to system-ids.js, and update all imports, requires, and package references to use the new kebab-case filename while preserving the module’s existing exports.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@packages/rs-drive/src/state_transition_action/action_convert_to_operations/batch/document/document_update_price_transition.rs`:
- Around line 112-117: Update the price extraction in the document update
transition to propagate missing or undecodable PRICE values instead of
converting them to a default zero. Replace the ok/flatten/default chain with the
surrounding error-return convention, ensuring the pricing-history record is not
persisted unless a valid Credits value is present.
---
Nitpick comments:
In `@packages/document-history-contract/lib/systemIds.js`:
- Line 1: Rename the JavaScript file from systemIds.js to system-ids.js, and
update all imports, requires, and package references to use the new kebab-case
filename while preserving the module’s existing exports.
In
`@packages/document-history-contract/test/unit/documentHistoryContract.spec.js`:
- Line 1: Rename the test file from its camelCase filename to the kebab-case
name document-history-contract.spec.js, updating any references to the existing
filename as needed.
In `@packages/rs-dpp/src/document/document_event.rs`:
- Around line 53-59: Update associated_document_type to return
document_history_contract.document_type_for_name(self.associated_document_type_name())
directly, removing the redundant Ok wrapper and question-mark operator while
preserving the existing Result<DocumentTypeRef, ProtocolError> behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c11fa015-59bf-4ad6-99a8-56c66e1d2b93
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.lockyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (68)
Cargo.tomlpackage.jsonpackages/data-contracts/Cargo.tomlpackages/data-contracts/src/error.rspackages/data-contracts/src/lib.rspackages/document-history-contract/.mocharc.ymlpackages/document-history-contract/Cargo.tomlpackages/document-history-contract/LICENSEpackages/document-history-contract/README.mdpackages/document-history-contract/eslint.config.mjspackages/document-history-contract/lib/systemIds.jspackages/document-history-contract/package.jsonpackages/document-history-contract/schema/v1/document-history-contract-documents.jsonpackages/document-history-contract/src/error.rspackages/document-history-contract/src/lib.rspackages/document-history-contract/src/v1/mod.rspackages/document-history-contract/test/bootstrap.jspackages/document-history-contract/test/unit/documentHistoryContract.spec.jspackages/dpns-contract/schema/v2/dpns-contract-documents.jsonpackages/dpns-contract/src/lib.rspackages/dpns-contract/src/v1/mod.rspackages/dpns-contract/src/v2/mod.rspackages/rs-dpp/Cargo.tomlpackages/rs-dpp/schema/meta_schemas/document/v1/document-meta.jsonpackages/rs-dpp/src/data_contract/document_type/accessors/mod.rspackages/rs-dpp/src/data_contract/document_type/accessors/v0/mod.rspackages/rs-dpp/src/data_contract/document_type/class_methods/try_from_schema/v0/mod.rspackages/rs-dpp/src/data_contract/document_type/class_methods/try_from_schema/v1/mod.rspackages/rs-dpp/src/data_contract/document_type/methods/validate_update/v0/mod.rspackages/rs-dpp/src/data_contract/document_type/mod.rspackages/rs-dpp/src/data_contract/document_type/v0/accessors.rspackages/rs-dpp/src/data_contract/document_type/v0/mod.rspackages/rs-dpp/src/data_contract/document_type/v0/random_document_type.rspackages/rs-dpp/src/data_contract/document_type/v1/accessors.rspackages/rs-dpp/src/data_contract/document_type/v1/mod.rspackages/rs-dpp/src/data_contract/document_type/v2/accessors.rspackages/rs-dpp/src/data_contract/document_type/v2/mod.rspackages/rs-dpp/src/document/document_event.rspackages/rs-dpp/src/document/mod.rspackages/rs-dpp/src/system_data_contracts.rspackages/rs-drive-abci/src/execution/platform_events/initialization/create_genesis_state/v1/mod.rspackages/rs-drive-abci/src/execution/platform_events/protocol_upgrade/perform_events_on_first_block_of_protocol_change/v0/mod.rspackages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/batch/data_triggers/bindings/list/mod.rspackages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/batch/data_triggers/bindings/list/v1/mod.rspackages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/batch/tests/document/dpns.rspackages/rs-drive/src/cache/system_contracts.rspackages/rs-drive/src/drive/document/insert/add_history_operations/mod.rspackages/rs-drive/src/drive/document/insert/add_history_operations/v0/mod.rspackages/rs-drive/src/drive/document/insert/mod.rspackages/rs-drive/src/state_transition_action/action_convert_to_operations/batch/document/document_purchase_transition.rspackages/rs-drive/src/state_transition_action/action_convert_to_operations/batch/document/document_transfer_transition.rspackages/rs-drive/src/state_transition_action/action_convert_to_operations/batch/document/document_update_price_transition.rspackages/rs-drive/src/state_transition_action/action_convert_to_operations/batch/document/mod.rspackages/rs-drive/src/util/batch/drive_op_batch/document.rspackages/rs-platform-version/src/version/drive_abci_versions/drive_abci_validation_versions/mod.rspackages/rs-platform-version/src/version/drive_abci_versions/drive_abci_validation_versions/v9.rspackages/rs-platform-version/src/version/drive_versions/drive_document_method_versions/mod.rspackages/rs-platform-version/src/version/drive_versions/drive_document_method_versions/v1.rspackages/rs-platform-version/src/version/drive_versions/drive_document_method_versions/v2.rspackages/rs-platform-version/src/version/drive_versions/drive_document_method_versions/v3.rspackages/rs-platform-version/src/version/drive_versions/drive_state_transition_method_versions/mod.rspackages/rs-platform-version/src/version/drive_versions/drive_state_transition_method_versions/v3.rspackages/rs-platform-version/src/version/drive_versions/mod.rspackages/rs-platform-version/src/version/drive_versions/v8.rspackages/rs-platform-version/src/version/system_data_contract_versions/mod.rspackages/rs-platform-version/src/version/system_data_contract_versions/v1.rspackages/rs-platform-version/src/version/system_data_contract_versions/v2.rspackages/rs-platform-version/src/version/v13.rs
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
The document-history implementation follows the existing system-contract architecture, but three PV13 changes alter consensus-visible behavior under protocol version 12: schema validation accepts the new flags, shared genesis inserts the contract, and the system-contract cache exposes it before activation. The C/Swift trusted context provider also lacks the feature plumbing needed to resolve the new contract through contract-ID-based document APIs.
Validated blockers were found in the Codex precheck. Sonnet is deferred until a fresh Codex revalidation clears the blocker gate.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (failed),gpt-5.6-sol— security-auditor (failed),gpt-5.6-sol— rust-quality (failed),gpt-5.6-sol— ffi-engineer (failed),gpt-5.6-sol— general (failed),gpt-5.6-sol— security-auditor (failed),gpt-5.6-sol— rust-quality (unparseable_transport_capture),gpt-5.6-sol— ffi-engineer (unparseable_transport_capture),gpt-5.6-sol— ffi-engineer (failed),gpt-5.6-sol— general (completed),gpt-5.6-sol— security-auditor (completed),gpt-5.6-sol— rust-quality (completed),gpt-5.6-sol— ffi-engineer (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 3 blocking | 🟡 1 suggestion(s)
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-dpp/schema/meta_schemas/document/v1/document-meta.json`:
- [BLOCKING] packages/rs-dpp/schema/meta_schemas/document/v1/document-meta.json:497-507: Gate the document-history schema fields to protocol version 13
The three new properties are added directly to document meta-schema v1. Both PLATFORM_V12 and PLATFORM_V13 select CONTRACT_VERSIONS_V4, whose document_type_schema version is 1, so upgraded validators accept these properties while validating a protocol-version-12 data-contract create. The base revision rejects them because the same v1 schema has additionalProperties=false. During pre-activation deployment, the two binaries can therefore disagree on contract validity in process_proposal. Add a new document meta-schema version selected only by PLATFORM_V13, or introduce equivalent protocol-version gating that leaves PV12 validation unchanged.
In `packages/rs-drive-abci/src/execution/platform_events/initialization/create_genesis_state/v1/mod.rs`:
- [BLOCKING] packages/rs-drive-abci/src/execution/platform_events/initialization/create_genesis_state/v1/mod.rs:59-62: Preserve pre-v13 genesis state
create_genesis_state_v1 is shared by protocol versions 12 and 13, but this entry registers DocumentHistory unconditionally. A network initialized or historically replayed at PV12 now stores state that the base revision did not, changing its deterministic application hash before the advertised PV13 activation. The upgrade test also starts from a PV12 genesis that already contains the contract, so it does not prove that transition_to_version_13 performs the insertion. Gate registration on protocol_version >= 13 or dispatch PV13 genesis through a new method version, and assert that the contract is absent before the upgrade transition.
In `packages/rs-drive/src/cache/system_contracts.rs`:
- [BLOCKING] packages/rs-drive/src/cache/system_contracts.rs:212-213: Keep the cached contract inactive until protocol version 13
load_genesis_system_contracts preloads DocumentHistory with active_since_protocol_version set to 13, but find_by_id ignores that field and returns the contract at every protocol version. On an existing PV12 chain where the v13 migration has not inserted the contract into GroveDB, an upgraded node treats this ID as a free, present system contract while the base binary performs a billed lookup and reports it absent. validate_identity_public_key_contract_bounds_v1 uses this path for identity key contract bounds, so the nodes can produce different validation errors and operation costs for the same PV12 transition. Make cache lookup protocol-version-aware and exclude contracts whose activation version is greater than the active platform version.
In `packages/data-contracts/src/lib.rs`:
- [SUGGESTION] packages/data-contracts/src/lib.rs:48: Register DocumentHistory with the C/Swift trusted context provider
The new SystemDataContract::DocumentHistory variant is not propagated through the rs-sdk and rs-sdk-trusted-context-provider feature lists, and TrustedHttpContextProvider::get_data_contract has no branch that loads it. rs-sdk-ffi enables the existing system-contract features and dash_sdk_create_trusted constructs this provider without a fallback, so dash_sdk_document_fetch_by_contract_id returns "Contract ... not found in trusted context" for the document-history ID. Add a document-history-contract feature through rs-sdk, rs-sdk-trusted-context-provider, and rs-sdk-ffi, include it in all-system-contracts, and load SystemDataContract::DocumentHistory alongside TokenHistory and KeywordSearch.
| "keepsTransferHistory": { | ||
| "type": "boolean", | ||
| "description": "True if transfers of these documents are recorded in the document history system contract, default is false" | ||
| }, | ||
| "keepsPurchaseHistory": { | ||
| "type": "boolean", | ||
| "description": "True if purchases of these documents are recorded in the document history system contract, default is false" | ||
| }, | ||
| "keepsPricingHistory": { | ||
| "type": "boolean", | ||
| "description": "True if price updates on these documents are recorded in the document history system contract, default is false" |
There was a problem hiding this comment.
🔴 Blocking: Gate the document-history schema fields to protocol version 13
The three new properties are added directly to document meta-schema v1. Both PLATFORM_V12 and PLATFORM_V13 select CONTRACT_VERSIONS_V4, whose document_type_schema version is 1, so upgraded validators accept these properties while validating a protocol-version-12 data-contract create. The base revision rejects them because the same v1 schema has additionalProperties=false. During pre-activation deployment, the two binaries can therefore disagree on contract validity in process_proposal. Add a new document meta-schema version selected only by PLATFORM_V13, or introduce equivalent protocol-version gating that leaves PV12 validation unchanged.
source: ['codex']
There was a problem hiding this comment.
Fixed in 31b4f83 — the three keywords moved to a new document meta-schema v2, selected only by PV13 via CONTRACT_VERSIONS_V5 (document_type_schema: 2). Meta-schema v1 is reverted to its base-revision content, so PV12 contract validation is unchanged and both binaries agree in process_proposal pre-activation.
There was a problem hiding this comment.
Resolved in this update — Gate the document-history schema fields to protocol version 13 no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
| ( | ||
| SystemDataContract::DocumentHistory, | ||
| system_data_contracts.load_document_history(), | ||
| ), |
There was a problem hiding this comment.
🔴 Blocking: Preserve pre-v13 genesis state
create_genesis_state_v1 is shared by protocol versions 12 and 13, but this entry registers DocumentHistory unconditionally. A network initialized or historically replayed at PV12 now stores state that the base revision did not, changing its deterministic application hash before the advertised PV13 activation. The upgrade test also starts from a PV12 genesis that already contains the contract, so it does not prove that transition_to_version_13 performs the insertion. Gate registration on protocol_version >= 13 or dispatch PV13 genesis through a new method version, and assert that the contract is absent before the upgrade transition.
source: ['codex']
There was a problem hiding this comment.
Fixed in 31b4f83 — genesis registers DocumentHistory only when protocol_version >= 13, so PV12 genesis/replay app hashes match the base revision. The upgrade test now asserts the contract is absent from the PV12 genesis before running transition_to_version_13, proving the transition performs the insertion. The DPNS PV12-rejection tests likewise now assert the contract does not exist at all pre-activation.
There was a problem hiding this comment.
Resolved in this update — Preserve pre-v13 genesis state no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
| } else if id == SystemDataContract::DocumentHistory.id() { | ||
| &self.document_history |
There was a problem hiding this comment.
🔴 Blocking: Keep the cached contract inactive until protocol version 13
load_genesis_system_contracts preloads DocumentHistory with active_since_protocol_version set to 13, but find_by_id ignores that field and returns the contract at every protocol version. On an existing PV12 chain where the v13 migration has not inserted the contract into GroveDB, an upgraded node treats this ID as a free, present system contract while the base binary performs a billed lookup and reports it absent. validate_identity_public_key_contract_bounds_v1 uses this path for identity key contract bounds, so the nodes can produce different validation errors and operation costs for the same PV12 transition. Make cache lookup protocol-version-aware and exclude contracts whose activation version is greater than the active platform version.
source: ['codex']
There was a problem hiding this comment.
Fixed in 31b4f83 — find_by_id now takes the current protocol version and returns None for contracts whose active_since_protocol_version is greater, so pre-activation lookups fall through to the billed grovedb fetch and report absent exactly like a non-upgraded node (same validation errors, same operation costs). This also retroactively hardens the TokenHistory/KeywordSearch entries (active since 9), with no behavior change at any released protocol version since the caller (get_system_or_user_contract_with_fee) postdates their activation.
There was a problem hiding this comment.
Resolved in this update — Keep the cached contract inactive until protocol version 13 no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
| WalletUtils = 5, | ||
| TokenHistory = 6, | ||
| KeywordSearch = 7, | ||
| DocumentHistory = 8, |
There was a problem hiding this comment.
🟡 Suggestion: Register DocumentHistory with the C/Swift trusted context provider
The new SystemDataContract::DocumentHistory variant is not propagated through the rs-sdk and rs-sdk-trusted-context-provider feature lists, and TrustedHttpContextProvider::get_data_contract has no branch that loads it. rs-sdk-ffi enables the existing system-contract features and dash_sdk_create_trusted constructs this provider without a fallback, so dash_sdk_document_fetch_by_contract_id returns "Contract ... not found in trusted context" for the document-history ID. Add a document-history-contract feature through rs-sdk, rs-sdk-trusted-context-provider, and rs-sdk-ffi, include it in all-system-contracts, and load SystemDataContract::DocumentHistory alongside TokenHistory and KeywordSearch.
source: ['codex']
There was a problem hiding this comment.
Fixed in 31b4f83 — added a document-history-contract feature through rs-sdk, rs-sdk-trusted-context-provider (included in its all-system-contracts umbrella and enabled by rs-sdk-ffi's provider dependency), and a DocumentHistory branch in TrustedHttpContextProvider::get_data_contract alongside TokenHistory and KeywordSearch.
There was a problem hiding this comment.
Resolved in this update — Register DocumentHistory with the C/Swift trusted context provider no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
…t-in Adds a document-history system contract recording transfer, purchase and price-update events for document types that subscribe via three new doctype configuration flags (keepsTransferHistory, keepsPurchaseHistory, keepsPricingHistory), mirroring the token history architecture. DPNS v2 subscribes the domain document type, so username transfers, sales and listings become queryable with proofs as of protocol version 13. - new document-history-contract package: transfer/purchase/priceUpdate doctypes, creation-restricted (protocol writes only), immutable and permanent, indexed by document, contract, buyer, seller and price - DocumentEvent in dpp builds deterministic history documents (id derived from source document, actor and nonce) - transfer/purchase/update-price ops conversions emit a DocumentHistory op when the doctype subscribed; actor pays for the record - transition_to_version_13 inserts the new contract and re-stores DPNS v2 on upgrade; genesis registers both for new networks - documentTypeName is a data field, not an index member: doctype names may be 64 chars while indexed strings cap at 63 - new flags are immutable on contract update, mirroring documentsKeepHistory - also fixes dpns_contract load fns gating on the withdrawals version field instead of dpns Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…version 13 Addresses PR review blockers — three ways an upgraded binary diverged from the base binary under protocol version 12 before activation: - document meta-schema v2 (CONTRACT_VERSIONS_V5, PV13 only) now carries the keeps*History keywords; meta-schema v1 is reverted so PV12 contract validation is byte-identical to the base revision - genesis registers the document history contract only at protocol version >= 13, preserving pre-13 genesis app hashes; the upgrade test now proves transition_to_version_13 performs the insertion by asserting the contract is absent beforehand - the system-contract cache find_by_id is activation-aware: contracts whose active_since_protocol_version exceeds the current protocol version fall through to the billed grovedb lookup and report absent, matching non-upgraded nodes Also per review: the pricing-history record now propagates a $price decode error instead of silently recording a free listing, and the DocumentHistory contract is plumbed through rs-sdk, rs-sdk-trusted-context-provider and rs-sdk-ffi so contract-ID document APIs resolve it in trusted mode. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
31b4f83 to
dada3b9
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/rs-sdk-trusted-context-provider/src/provider.rs (1)
654-662: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick winAdd
document-history-contractto the outercfgblock.The outer
#[cfg(any(...))]macro guards the execution block for all system contracts. Sincefeature = "document-history-contract"is missing from this list, if a downstream crate compiles the provider with only this specific contract feature enabled (and withoutall-system-contractsor other contract features), the entire block will be skipped andDocumentHistorywill not be resolved.🐛 Proposed fix
#[cfg(any( feature = "dpns-contract", feature = "dashpay-contract", feature = "withdrawals-contract", feature = "wallet-utils-contract", feature = "token-history-contract", feature = "keywords-contract", + feature = "document-history-contract", feature = "all-system-contracts" ))]🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/rs-sdk-trusted-context-provider/src/provider.rs` around lines 654 - 662, Add feature = "document-history-contract" to the outer #[cfg(any(...))] guard surrounding the system-contract execution block, preserving the existing feature entries. Ensure DocumentHistory resolution remains compiled when only this feature is enabled.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@packages/rs-sdk-trusted-context-provider/src/provider.rs`:
- Around line 654-662: Add feature = "document-history-contract" to the outer
#[cfg(any(...))] guard surrounding the system-contract execution block,
preserving the existing feature entries. Ensure DocumentHistory resolution
remains compiled when only this feature is enabled.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 05613113-2e50-41c2-bd46-664717e369c2
📒 Files selected for processing (17)
packages/rs-dpp/schema/meta_schemas/document/v2/document-meta.jsonpackages/rs-dpp/src/data_contract/document_type/class_methods/try_from_schema/v0/mod.rspackages/rs-dpp/src/data_contract/document_type/class_methods/try_from_schema/v1/mod.rspackages/rs-dpp/src/validation/meta_validators/mod.rspackages/rs-drive-abci/src/execution/platform_events/initialization/create_genesis_state/v1/mod.rspackages/rs-drive-abci/src/execution/platform_events/protocol_upgrade/perform_events_on_first_block_of_protocol_change/v0/mod.rspackages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/batch/tests/document/dpns.rspackages/rs-drive/src/cache/system_contracts.rspackages/rs-drive/src/drive/contract/get_fetch/get_system_or_user_contract_with_fee/v0/mod.rspackages/rs-drive/src/state_transition_action/action_convert_to_operations/batch/document/document_update_price_transition.rspackages/rs-platform-version/src/version/dpp_versions/dpp_contract_versions/mod.rspackages/rs-platform-version/src/version/dpp_versions/dpp_contract_versions/v5.rspackages/rs-platform-version/src/version/v13.rspackages/rs-sdk-ffi/Cargo.tomlpackages/rs-sdk-trusted-context-provider/Cargo.tomlpackages/rs-sdk-trusted-context-provider/src/provider.rspackages/rs-sdk/Cargo.toml
🚧 Files skipped from review as they are similar to previous changes (5)
- packages/rs-drive-abci/src/execution/platform_events/initialization/create_genesis_state/v1/mod.rs
- packages/rs-platform-version/src/version/v13.rs
- packages/rs-drive-abci/src/execution/platform_events/protocol_upgrade/perform_events_on_first_block_of_protocol_change/v0/mod.rs
- packages/rs-drive/src/state_transition_action/action_convert_to_operations/batch/document/document_update_price_transition.rs
- packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/batch/tests/document/dpns.rs
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## v4.1-dev #4171 +/- ##
============================================
+ Coverage 87.45% 87.47% +0.02%
============================================
Files 2648 2655 +7
Lines 334328 334968 +640
============================================
+ Hits 292373 293018 +645
+ Misses 41955 41950 -5
🚀 New features to boost your workflow:
|
…sis state The PV13 genesis now registers the document history contract and stores the larger DPNS v2 schema, which changes contracts-subtree node sizes and therefore the byte-billed contract reads. Two version-pinned baselines shift by the same 48,640 credits: the token burn group-action confirmer processing fee and the direct-purchase buyer balance. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
@coderabbitai review |
✅ Action performedReview finished.
|
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
Four previously reported activation and FFI-plumbing defects are fixed at the exact head, and the latest delta only updates internally consistent PV13 fee baselines. A consensus-blocking regression remains because historical schema parsers now interpret PV13-only history flags during PV1–PV11 validation and replay. The standalone trusted-context DocumentHistory feature also compiles while its lookup block is excluded.
Validated blockers were found in the Codex precheck. Sonnet is deferred until a fresh Codex revalidation clears the blocker gate.
Review provenance
- Codex reviewers:
gpt-5.6-sol— unknown role (failed),gpt-5.6-sol— unknown role (failed),gpt-5.6-sol— unknown role (failed),gpt-5.6-sol— unknown role (failed),gpt-5.6-sol— general (failed),gpt-5.6-sol— rust-quality (failed),gpt-5.6-sol— general (completed),gpt-5.6-sol— security-auditor (completed),gpt-5.6-sol— rust-quality (completed),gpt-5.6-sol— ffi-engineer (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 1 blocking | 🟡 1 suggestion(s)
1 additional finding(s) omitted (not in diff).
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-dpp/src/data_contract/document_type/class_methods/try_from_schema/v0/mod.rs`:
- [BLOCKING] packages/rs-dpp/src/data_contract/document_type/class_methods/try_from_schema/v0/mod.rs:180-199: Keep PV13 history fields out of historical schema parsers
The historical v0 parser now reads `keepsTransferHistory`, `keepsPurchaseHistory`, and `keepsPricingHistory` unconditionally, and the v1 parser does the same at lines 199–218. PLATFORM_V1 through PLATFORM_V11 select these parsers with document meta-schema v0, which permits unknown top-level properties; the base implementation therefore accepted and ignored these names. The new code rejects non-boolean values and lets boolean values affect the typed document configuration and immutable-update checks, changing historical consensus validation and potentially causing replay or mixed-binary disagreement. Parse these fields only when `document_type_schema >= 2`, leaving them false for earlier schema versions.
In `packages/rs-sdk-trusted-context-provider/src/provider.rs`:
- [SUGGESTION] packages/rs-sdk-trusted-context-provider/src/provider.rs:654-662: Include DocumentHistory in the outer feature gate
The manifest exposes `document-history-contract` as an independent feature, and the inner dispatch branch handles it, but both this enclosing cfg and the matching import cfg at lines 13–21 omit the feature. Consequently, `cargo check -p rs-sdk-trusted-context-provider --no-default-features --features document-history-contract` succeeds while compiling out the entire system-contract lookup block, so DocumentHistory resolution falls through to the fallback provider or returns `None`. Add the feature to both outer cfg lists and cover isolated feature resolution. The shipped FFI configuration is unaffected because its other enabled contract features activate the enclosing block.
| // Are transfers of documents of this type recorded in the document | ||
| // history system contract? | ||
| let documents_keep_transfer_history: bool = | ||
| Value::inner_optional_bool_value(schema_map, KEEPS_TRANSFER_HISTORY) | ||
| .map_err(consensus_or_protocol_value_error)? | ||
| .unwrap_or_default(); | ||
|
|
||
| // Are purchases of documents of this type recorded in the document | ||
| // history system contract? | ||
| let documents_keep_purchase_history: bool = | ||
| Value::inner_optional_bool_value(schema_map, KEEPS_PURCHASE_HISTORY) | ||
| .map_err(consensus_or_protocol_value_error)? | ||
| .unwrap_or_default(); | ||
|
|
||
| // Are price updates on documents of this type recorded in the | ||
| // document history system contract? | ||
| let documents_keep_pricing_history: bool = | ||
| Value::inner_optional_bool_value(schema_map, KEEPS_PRICING_HISTORY) | ||
| .map_err(consensus_or_protocol_value_error)? | ||
| .unwrap_or_default(); |
There was a problem hiding this comment.
🔴 Blocking: Keep PV13 history fields out of historical schema parsers
The historical v0 parser now reads keepsTransferHistory, keepsPurchaseHistory, and keepsPricingHistory unconditionally, and the v1 parser does the same at lines 199–218. PLATFORM_V1 through PLATFORM_V11 select these parsers with document meta-schema v0, which permits unknown top-level properties; the base implementation therefore accepted and ignored these names. The new code rejects non-boolean values and lets boolean values affect the typed document configuration and immutable-update checks, changing historical consensus validation and potentially causing replay or mixed-binary disagreement. Parse these fields only when document_type_schema >= 2, leaving them false for earlier schema versions.
source: ['codex']
There was a problem hiding this comment.
Fixed in 8650436 — the v0 and v1 parsers now read the three flags only when document_type_schema >= 2, so PV1–PV11 (and PV12) parsing is byte-identical to the base implementation: unknown keys under these names stay accepted-and-ignored, non-boolean values can't fail replay, and boolean values can't retroactively flip the typed config or immutable-update checks. Pinned by three regression tests (non-boolean ignored at PV11, boolean ignored at PV11, boolean parsed at PV13).
Historical protocol versions used document meta-schema v0, which accepts and ignores unknown top-level keys: a pre-v12 contract carrying e.g. a non-boolean value under keepsTransferHistory validated fine on the base implementation. Parsing the flags unconditionally would reject such contracts on replay (and let boolean values flip the typed config and immutable-update checks retroactively). The parsers now read the flags only when document_type_schema >= 2, pinned by regression tests at protocol versions 11 and 13. Also adds the document-history-contract feature to the trusted context provider's outer cfg gates so an isolated-feature build keeps the system contract lookup block compiled in. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Round-2 suggestion (trusted-context-provider outer cfg gate) also fixed in 8650436: document-history-contract added to both the import cfg and the enclosing lookup-block cfg, verified with an isolated |
✅ Action performedReview finished.
|
…story The history doctypes now carry aggregation trees, turning the contract into a provable marketplace-stats source: - purchase: doctype-averageable on price (O(1) provable all-time sale count, volume, and average), with byContract and byDocument indices range-averageable over $createdAt — provable sale count, volume and average price between any two dates, per contract or per document - priceUpdate: doctype-countable, byContract/byDocument range-averageable over $createdAt — provable average asking price between dates (listing-event-weighted, documented on the doctype) - transfer: doctype-countable, byContract range-countable — provable transfer counts per contract and time window price drops its minimum bound so it infers as i64 (grovedb's sum aggregator is i64; values are protocol-written consensus-validated credit amounts). The genesis cache loads the contract pinned to its activation version, since earlier document meta-schemas do not recognize the index aggregation keywords. Tests assert the provable count, sum, and (count, sum) average pairs over the DPNS sale and transfer flows, including a wide date window, an empty window, and the between-dates average asking price query. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Added in c94eb2e: the history doctypes now carry provable aggregation trees — |
The $comment was carried over verbatim from v1 and described v1's activation (protocol v12, CONTRACT_VERSIONS_V4, frozen at the 3.1 release, changes go to v2+). v2 activates with protocol v13 via CONTRACT_VERSIONS_V5, adds the keeps*History document type flags, freezes when the release carrying protocol v13 ships, and future changes must go to v3+. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
QuantumExplorer
left a comment
QuantumExplorer
left a comment
There was a problem hiding this comment.
AI PR Reviewed
Three more v4.1-dev breakages that only surface on full (dispatch or version-change) CI runs: - Docker image builds fail at cargo-chef prepare: #4171 made document-history-contract a dependency of data-contracts but the Dockerfile never copies the new crate. Add it everywhere token-history-contract appears. This also broke Test Suite and Dashmate E2E, which need the images. - keyword-search-contract unit tests were copy-pasted from wallet-utils-contract and still tested its txMetadata document type, which this contract does not define; every case threw. Rewrite the suite against the real schema (contractKeywords, shortDescription, fullDescription). Two behaviors surfaced while doing so are asserted as such: wrong-length identifier fields throw at document creation (identifier conversion precedes schema validation), and fullDescription's schema maxLength (10000) sits above the system per-field cap (5120), so overlength strings yield a field-size error, not a maxLength error. - document-history-contract asserted a minimum on purchase.price, but the schema deliberately leaves the integer unbounded so the property stays sum-aggregatable; consensus validates actual amounts. Assert the type check and the intentional absence of a schema minimum instead. These suites were never exercised in CI before #4203 added the missing package filters, and the contract-test jobs only run on full pipelines. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
2 participants
Issue being fixed or feature implemented
PR #4145 enabled DPNS username transfers and sales at protocol version 13, but they leave no queryable trace: the domain document is updated in place, overwriting the previous owner, the sale price, and when it happened. More generally, no document type on Platform has any transfer/sale/listing history — even though tokens already solved this exact problem with the TokenHistory system contract and per-token
keepsHistoryrules.This PR ports that architecture to documents: a document-history system contract that any document type can subscribe to, with DPNS
domainas the first subscriber.What was done?
New
document-historysystem contract (packages/document-history-contract,SystemDataContract::DocumentHistory = 8) with three document types —transfer,purchase(records seller and the explicit price paid) andpriceUpdate. All arecreationRestrictionMode: 2(only the protocol can write them), immutable, and non-deletable. Indices support per-document provenance (dataContractId, documentId, $createdAt), per-contract feeds, per-identity history (buyer/seller/sender/recipient) and price analytics.documentTypeNameis recorded as a data field but deliberately not indexed: document type names may be 64 chars while indexed strings cap at 63.Per-doctype subscription flags in DPP —
keepsTransferHistory,keepsPurchaseHistory,keepsPricingHistory— mirroringTokenKeepsHistoryRules. Parsed from the document type schema (default false), whitelisted in the v1 document meta-schema only (they are v13-introduced, so the v11→v12 migration allowlist is untouched, same asdocumentsCountable), and immutable on contract update likedocumentsKeepHistory.History writes in ops conversion — the v1 transfer/purchase arms and a new v1 update-price arm check the source doctype's flags and emit a
DocumentOperationType::DocumentHistoryop. At apply time drive builds the history document via the newDocumentEvent(rs-dpp), with a deterministic ID derived from the source document, acting identity, and identity contract nonce — every validator produces identical history documents. The acting party (sender / buyer / seller respectively) owns and pays for the record.DPNS v2 subscribes
domainto all three flags (SYSTEM_DATA_CONTRACT_VERSIONS_V2, consumed byPLATFORM_V13), so as of PV13: username transfers record sender→recipient, listings record the asking price, and sales record buyer, seller, and price paid — all queryable through the standard document query path with proofs.Upgrade and genesis —
transition_to_version_13inserts the document-history contract and re-stores DPNS v2 on the first block after upgrade; genesis registers both for new networks. No migration of existing documents is required — the history contract starts empty and the DPNS change is contract-definition-only.Also fixes a latent copy-paste bug:
dpns_contract::load_definitions/load_documents_schemasgated on the withdrawals version field instead of dpns.How Has This Been Tested?
New drive-abci execution tests (all in
batch/tests/document/dpns.rsunless noted):test_dpns_username_transfernow also asserts atransferhistory document exists, owned by the sender, pointing at the recipient, timestamped.test_dpns_username_salenow also asserts thepriceUpdaterecord (seller, 0.1 DASH asking price) and thepurchaserecord (buyer-owned, seller identity, explicit price paid) — and that notransferrecord exists for a sale.test_document_transfer_without_history_subscription_records_nothing— a transferable doctype without the flags (crypto card game) records nothing at PV13, proving flag-gating rather than version-gating.test_document_history_contract_direct_writes_rejected— user document creation on the history contract fails withDocumentCreationNotAllowedError.test_transition_to_version_13_inserts_document_history_contract_and_updates_dpns(protocol upgrade tests) — a PV12-born chain upgrading to 13 gains the history contract and a stored DPNS v2 whose domain doctype reports all three flags.Full runs: all 3789 dpp tests, all 102 drive-abci document batch tests (includes NFT update-price/purchase flows with pinned fee constants — the new v1 update-price arm is behavior-identical for unsubscribed doctypes), all 14 protocol-upgrade tests,
cargo check --workspace --all-targets, clippy clean on all touched crates,cargo check -p drive --no-default-features --features verify.Breaking Changes
Consensus-breaking at protocol version 13 activation (extends the PV13 changes from #4145): nodes without this change would neither register the document-history contract nor write history documents, diverging in app hash from nodes that do. No behavior change for protocol versions ≤ 12. The seller-balance economics pinned in #4145's tests are unchanged — the buyer pays for the purchase history record.
Checklist:
For repository code-owners and collaborators only
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation
Tests