chore(dpp): remove orphaned JSON schemas and dead wasm-dpp tests#3470
chore(dpp): remove orphaned JSON schemas and dead wasm-dpp tests#3470
Conversation
… of 6 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
identityCreate.json and identityUpdate.json are not referenced anywhere in the codebase; validation is handled entirely by Rust code. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Validation for these state transitions moved entirely to Rust. identity.json is retained as it is still referenced in test fixtures. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Both test suites were describe.skip and fully covered by Rust tests in rs-drive-abci and rs-dpp. identity.json had no other references. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (15)
💤 Files with no reviewable changes (15)
📝 WalkthroughWalkthroughRemoved 15 JSON Schema definition files and 2 test suites totaling approximately 1,214 lines. The deleted schemas validated data contract, document, identity, and associated state transition objects across the rs-dpp package. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
|
🕓 Ready for review — 7 ahead in queue (commit 0c28504) |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
I verified the PR against the source at d7333f9e9599b7c4eb54be3e92ea825b082112a3. The change is deletion-only: it removes packages/rs-dpp/src/schema/identity/v0/identity.json plus two describe.skip wasm tests that were the only consumers of that file, while the active Rust schema implementation in packages/rs-dpp/src/schema/identity/v0/identity.rs remains intact. I did not find any runtime regressions, broken references, or consensus-critical issues in this diff.
Reviewed commit: d7333f9
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## v3.1-dev #3470 +/- ##
=========================================
Coverage 88.29% 88.29%
=========================================
Files 2474 2474
Lines 300927 300927
=========================================
Hits 265707 265707
Misses 35220 35220
🚀 New features to boost your workflow:
|
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
I re-reviewed this PR at 0d0a8cd4 against the checked-out tree. The change remains a pure deletion of 13 unused JSON schema files plus 2 skipped wasm-dpp tests, and the live schema loading path in packages/rs-dpp/src/schema/mod.rs still references only the retained document transition schemas. I did not find any correctness, security, consensus, or test-coverage regressions caused by these deletions.
Reviewed commit: 0d0a8cd
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
No agent findings or CodeRabbit findings were provided in the verifier prompt for PR #3470, so there was nothing to validate against detached HEAD cb57e99ef3cae0e4ef788abd8c8b1ffa846863e0. Based on the prompt contents alone, this review produces no verified issues.
Reviewed commit: cb57e99
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
The PR is described as a focused cleanup that removes orphaned JSON schemas and two skipped wasm-dpp tests, but the actual merge diff against v3.1-dev is a massive feature branch: 1018 files changed with 182k+ insertions across workflows, docs, rs-dpp, rs-dapi, data contracts, SDK crates, and many unrelated packages. That mismatch is the real review problem here — this is not reviewable or merge-safe as a narrowly scoped schema-cleanup PR.
Reviewed commit: 556d857
🔴 1 blocking
1 additional finding
🔴 blocking: This schema-cleanup PR is not actually a focused deletion — it merges a huge product branch into `v3.1-dev`
packages/rs-dpp/src/schema/identity/stateTransition/identityUpdate.json (line 1)
The PR description says this change simply removes 13 orphaned JSON schema files from packages/rs-dpp/src/schema/ and deletes two fully skipped wasm-dpp tests that only referenced identity.json. But the real merge diff against v3.1-dev is 1018 files with 182k+ insertions touching workflows, docs/book content, rs-dpp core types and validation, rs-dapi, rs-dapi-client, new/expanded data-contract packages, SDK/support crates, and many unrelated packages across the monorepo. That means approving/merging this PR would not just land the schema cleanup — it would also merge a very large body of unrelated platform changes that are not disclosed by the PR summary and cannot be meaningfully audited as part of a narrow chore review. Split this into (a) a true schema-cleanup PR containing only the orphaned JSON schema/test deletions, and (b) separate feature/integration PRs for the broader platform changes. As-is, the PR description materially understates what will actually merge.
🤖 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/schema/identity/stateTransition/identityUpdate.json`:
- [BLOCKING] line 1: This schema-cleanup PR is not actually a focused deletion — it merges a huge product branch into `v3.1-dev`
The PR description says this change simply removes 13 orphaned JSON schema files from `packages/rs-dpp/src/schema/` and deletes two fully skipped wasm-dpp tests that only referenced `identity.json`. But the real merge diff against `v3.1-dev` is 1018 files with 182k+ insertions touching workflows, docs/book content, rs-dpp core types and validation, rs-dapi, rs-dapi-client, new/expanded data-contract packages, SDK/support crates, and many unrelated packages across the monorepo. That means approving/merging this PR would not just land the schema cleanup — it would also merge a very large body of unrelated platform changes that are not disclosed by the PR summary and cannot be meaningfully audited as part of a narrow chore review. Split this into (a) a true schema-cleanup PR containing only the orphaned JSON schema/test deletions, and (b) separate feature/integration PRs for the broader platform changes. As-is, the PR description materially understates what will actually merge.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
The actual PR-shaped diff is the narrow cleanup described in the PR: relative to the merge base used by the review branch, it deletes 15 files and no production code paths. One suggestion remains: the surviving packages/rs-dpp/src/schema/ Rust module is still dead code, so the cleanup rationale is incomplete. The blocking finding from Codex does not apply because it compared the merge commit against the wrong side of the merge.
Reviewed commit: 556d857
🟡 1 suggestion(s)
1 additional finding
🟡 suggestion: The remaining `src/schema` module is still unreachable dead code
packages/rs-dpp/src/schema/mod.rs (lines 4-23)
packages/rs-dpp/src/lib.rs:28-77 never declares mod schema;, so this module is not compiled into the crate at all. That makes the retained include_str! loads here for documentTransition/base.json, create.json, replace.json, and documentsBatch.json unreachable, and the same dead module tree still contains packages/rs-dpp/src/schema/identity/v0/identity.rs:3-45, whose identity_json helper has no callers outside src/schema. This does not make the PR unsafe, but it means the reason given for keeping the remaining src/schema files is incorrect and the cleanup is incomplete.
🤖 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/schema/mod.rs`:
- [SUGGESTION] lines 4-23: The remaining `src/schema` module is still unreachable dead code
`packages/rs-dpp/src/lib.rs:28-77` never declares `mod schema;`, so this module is not compiled into the crate at all. That makes the retained `include_str!` loads here for `documentTransition/base.json`, `create.json`, `replace.json`, and `documentsBatch.json` unreachable, and the same dead module tree still contains `packages/rs-dpp/src/schema/identity/v0/identity.rs:3-45`, whose `identity_json` helper has no callers outside `src/schema`. This does not make the PR unsafe, but it means the reason given for keeping the remaining `src/schema` files is incorrect and the cleanup is incomplete.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
PR is a clean dead-code deletion of 13 orphaned JSON schema files and 2 fully-skipped wasm-dpp test files. No correctness issues. Both reviewers converge on one valid follow-up: an inline copy of the deleted identity.json survives as IDENTITY_JSON_STRING/identity_json() in schema/identity/v0/identity.rs with no real callers, preserving the same drift risk the PR set out to eliminate (it hard-codes publicKeys.maxItems=32 while live validators use max_public_keys_in_creation=6).
Reviewed commit: 8db4b77
🟡 1 suggestion(s)
1 additional finding
🟡 suggestion: Inline copy of deleted identity.json is orphaned and already drifts from live validators
packages/rs-dpp/src/schema/identity/v0/identity.rs (lines 1-47)
packages/rs-dpp/src/schema/identity/v0/identity.rs embeds the just-deleted identity.json verbatim as IDENTITY_JSON_STRING and exposes it via pub fn identity_json(), re-exported from packages/rs-dpp/src/schema/identity/v0/mod.rs:1 (pub use identity::identity_json;). A repo-wide grep on the current head finds no live callers — the only matches outside the definition site are a commented-out block in packages/wasm-dpp/src/identity/identity_facade.rs:122-126, an unrelated local variable in packages/wasm-drive-verify/src/utils/serialization.rs:28, and a test function name in packages/rs-dpp/src/voting/vote_choices/resource_vote_choice/mod.rs:60. More importantly, the embedded schema already disagrees with the active Rust validators: it hard-codes publicKeys.maxItems = 32, while validation now reads platform_version.dpp.state_transitions.identities.max_public_keys_in_creation, which is 6 in v1.rs, v2.rs, and v3.rs of packages/rs-platform-version/src/version/dpp_versions/dpp_state_transition_versions/. Leaving this helper in place preserves the exact schema-drift hazard the PR description cites as the reason for deleting the file-based schemas. To complete the stated goal, delete schema/identity/v0/identity.rs, the pub use identity::identity_json; line, and the mod identity; declaration in schema/identity/v0/mod.rs. Not a blocker — leaving it in just preserves dead code the PR otherwise removes.
🤖 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/schema/identity/v0/identity.rs`:
- [SUGGESTION] lines 1-47: Inline copy of deleted identity.json is orphaned and already drifts from live validators
`packages/rs-dpp/src/schema/identity/v0/identity.rs` embeds the just-deleted `identity.json` verbatim as `IDENTITY_JSON_STRING` and exposes it via `pub fn identity_json()`, re-exported from `packages/rs-dpp/src/schema/identity/v0/mod.rs:1` (`pub use identity::identity_json;`). A repo-wide grep on the current head finds no live callers — the only matches outside the definition site are a commented-out block in `packages/wasm-dpp/src/identity/identity_facade.rs:122-126`, an unrelated local variable in `packages/wasm-drive-verify/src/utils/serialization.rs:28`, and a test function name in `packages/rs-dpp/src/voting/vote_choices/resource_vote_choice/mod.rs:60`. More importantly, the embedded schema already disagrees with the active Rust validators: it hard-codes `publicKeys.maxItems = 32`, while validation now reads `platform_version.dpp.state_transitions.identities.max_public_keys_in_creation`, which is `6` in `v1.rs`, `v2.rs`, and `v3.rs` of `packages/rs-platform-version/src/version/dpp_versions/dpp_state_transition_versions/`. Leaving this helper in place preserves the exact schema-drift hazard the PR description cites as the reason for deleting the file-based schemas. To complete the stated goal, delete `schema/identity/v0/identity.rs`, the `pub use identity::identity_json;` line, and the `mod identity;` declaration in `schema/identity/v0/mod.rs`. Not a blocker — leaving it in just preserves dead code the PR otherwise removes.
2 participants
Issue being fixed or feature implemented
rs-dppcontained JSON schema files for identity, data contract, and document state transitions that were never loaded by any code — validation for these transitions moved entirely to native Rust. These orphaned files were misleading (e.g.identityUpdate.jsondeclaredaddPublicKeys.maxItems: 10while Rust enforces 6) and would inevitably drift further from the actual implementation. This was causing confusion when updating docs also.What was done?
packages/rs-dpp/src/schema/:identity.json,publicKey.json, allstateTransition/files including asset lock proofsdataContractCreate.json,dataContractUpdate.jsondocumentBase.jsonwasm-dppintegration test files that only existed to referenceidentity.json:validatePublicKeysState.spec.jsvalidateIdentityUpdateTransitionStateFactory.spec.jsThe four document transition schemas loaded via
include_str!inschema/mod.rs(base.json,create.json,replace.json,documentsBatch.json) are retained as they are actively used.How Has This Been Tested?
The deleted JS tests were
describe.skipand covered by existing Rust tests inrs-drive-abci:test_identity_update_duplicate_key_ids_to_disabletest_identity_update_wrong_revisiontest_identity_update_disabling_nonexistent_keytest_identity_update_too_many_keys_to_disableupdate_identities_tests.rsNo new tests added — this is pure deletion of dead code.
Breaking Changes
None.
Checklist:
For repository code-owners and collaborators only
Summary by CodeRabbit