Repository navigation
test(contract): expect only the first schema violation of a document at protocol version 15 - #5614
Conversation
…at protocol version 15 From protocol version 15 (`validate_document` 2), documents are checked by the native document schema validator. It stops at the first violation it meets, in a fixed keyword order, and reports that single JsonSchemaError. The jsonschema crate, which protocol version 14 still uses, listed every violation. Fourteen DPNS, DashPay and withdrawals contract tests still expected two errors. Each of those tests now expects one error and asserts its keyword with its instance path or params. Where the value breaks several keywords, the reported keyword follows the native order: - DPNS names over 63 characters report `maxLength`, which is checked before `pattern`. The separate "should follow pattern" tests still cover the patterns. The parent domain name case now uses 64 characters instead of 191, so it exercises the 63-character limit. - An empty DashPay `avatarUrl` reports `minLength`, which is checked before `format`. - A non-array byte array reports one `type` error. The `type` keyword and the `byteArray` keyword each reported one before. - A missing avatar property reports one `required` error. Before, each dependentRequired entry that names it reported one. The avatar dependencies are symmetric, so a single error no longer shows which entry fired. New tests with one avatar property present alone fail if that property's entry is removed. - The withdrawals `type` cases now also assert the instance path. Test would have caught this in CI: 14 failing before this change on origin/v5.1-dev (dpns 3, dashpay 8, withdrawals 3). After it, dpns 31, dashpay 43 and withdrawals 22 pass. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review.
 |
| Layer / File(s) | Summary |
|---|---|
Avatar and account validation assertions packages/dashpay-contract/test/unit/schema.spec.js |
Avatar tests expect updated single-error results and check missing required properties for URL-only, hash-only, and fingerprint-only cases. Encrypted account label and auto-accept proof type tests also expect one error. |
DPNS domain-document validation
| Layer / File(s) | Summary |
|---|---|
Domain field length assertions packages/dpns-contract/test/unit/dpnsContract.spec.js |
Overlong label and normalized-label tests expect one maxLength error. The normalized parent-name test uses a 64-character value and expects one maxLength error. |
Withdrawals contract validation
| Layer / File(s) | Summary |
|---|---|
Property type error assertions packages/withdrawals-contract/test/unit/withdrawalsContract.spec.js |
The pooling, outputScript, and status tests expect one validation error and assert the corresponding instancePath. |
Priority:
Estimated code review effort: 2 (Simple) | ~8 minutes
Change: Other
Suggested reviewers: quantumexplorer
Merge Risk: 
aa0f6
-
No actionable merge-blocking risk was identified in these test-only updates.
Pre-merge checks: 
-
Check name Status Explanation Description Check Check skipped - CodeRabbit’s high-level summary is enabled. Title check The title states the main change: the tests for protocol version 15 now expect only the first schema violation of a document. The change touches DPNS, DashPay, and withdrawals contract tests, and the … Docstring Coverage No eligible functions found in the changed files. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentat… Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request. Full details: Title check
-
Explanation
The title states the main change: the tests for protocol version 15 now expect only the first schema violation of a document. The change touches DPNS, DashPay, and withdrawals contract tests, and the title's test(contract) scope matches a test-only change. The title is specific and accurate. It does not name the DashPay avatar dependency tests, which are a secondary addition, and that omission is acceptable.
Full details: Docstring Coverage
-
Explanation
No eligible functions found in the changed files. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 0 functions across 3 files.
-
Finishing Touches
-
📝 Generate docstrings
-
- Commit to this branch
- Create a new PR
🧪 Generate unit tests (beta)
-
- Commit to this branch
- Create a new PR
-
- 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.
Comment @coderabbitai help to get the list of available commands.
|
Waiting for bot review — thepastaclaw not yet. Wait for the missing reviews, or a writer can post |
github-actions
Bot
added
the
waiting-bots
|
✅ Final review complete — no blockers (commit aa0f616) · triage: low |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Phase 1 + Phase 2
The commit only updates contract-test expectations for protocol version 15's documented first-violation validation behavior and adds targeted avatar dependency coverage. The changed assertions match the schemas and native validator ordering, and the affected package tests, builds, and lint checks passed in CI; no in-scope issues were found.
Review provenance
Source: reviewer 1: glm-5.3-flash (agent: phase1-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
lowbygpt-6.1-sol(effort low) — This is a small, well-contained test-only change updating expected validation-error counts for an already established protocol behavior, with no production or consensus logic modified. - Phase 1 reviewers:
glm-5.3-flash— general (completed, effort high); agentphase1-reviewer - Phase 1 model:
glm-5.3-flash— zai quota: 5h 99% left, weekly 95% left; passed overgemini-3.8-flash-high(antigravity quota lookup failed) - Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort medium); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort medium); agentphase2-reviewer
|
Bots are done — your move: post |
github-actions
Bot
added
waiting-self-review
Issue being fixed or feature implemented
Since #5533, 14 JS contract tests fail on
v5.1-dev. Each fails the same way:expected [ Array(1) ] to have a length of 2 but got 1.@dashevo/dpns-contract: 3 tests (label, normalizedLabel and normalizedParentDomainName length).@dashevo/dashpay-contract: 8 tests (profile avatar properties, and contactRequestencryptedAccountLabelandautoAcceptProoftypes).@dashevo/withdrawals-contract: 3 tests (pooling,outputScriptandstatustypes).Root cause
The tests run at
dpp.protocolVersion, the latest version (15 onv5.1-dev). #5533 switched protocol version 15 tovalidate_document2 (CONTRACT_VERSIONS_V7). It checks a document with the nativeDocumentSchemaValidator, which stops at the first violation and returns oneJsonSchemaError(validate_document_properties_v2wrapsvalidator.validate(..) -> Option<JsonSchemaError>). The module documentation ofvalidation_schema/validate/v0says so:Protocol version 14 still uses
validate_document1, thejsonschemacrate, which collects every error.#5538 is about registration-time schema checks (
check_document_type_schema1). Its only edit on the document-validation path lets a plain$idcontain$. It doesn't change how a document is checked or how many errors it gets. Rebuilding wasm-dpp at each commit shows #5533 is the first failing commit (table under "How Has This Been Tested?").How CI missed it:
v5.1-devTestsrun was cancelled before any job started, superseded by feat(dpp)!: check document schemas natively at registration from protocol version 15 #5538's push, and PR runs skip the contract packages.v5.1-devpush that selected it since.v5.1-devpush since. Their failures showed up in the forward merge chore: merge v5.0-dev into v5.1-dev (5.0.1) #5608.eb3e1b8272's run is marked cancelled, but its dpns-contract job finished with 31 passing.What each failing case reports, from
document.validate()run on this branch's wasm-dpp build:jsonschemacrate)label/normalizedLabel/normalizedParentDomainNametoo longpattern,maxLengthmaxLengthavatarUrl: ''format,minLengthminLengthavatarUrl/avatarHash/avatarFingerprint(dependentRequired)required×2, one per dependentRequired entry that names the missing propertyrequiredavatarHash/avatarFingerprint/encryptedAccountLabel/autoAcceptProof='notAnArray'type×2, fromtype: arrayand from thebyteArraykeywordtypepooling/status='string'type,enumtypeoutputScript=1type×2, fromtype: arrayand frombyteArraytypeIntended, not a regression
JsonSchemaErrorfor a real violation, at the right instance path.byteArrayrepeatingtype, or a second dependentRequired entry naming the same missing property.pattern,formatorenum. The tests that target those keywords alone ("should follow pattern", "should be of type URL", "should be within enum range") still pass.validate_document2 (quoted above). feat(dpp)!: validate documents natively at protocol version 15 #5533's description says: "Onlyinfo, which the results hash does not cover, can differ: […] a document breaking two keywords […] can get another violation first".document.validate(14)on the same build).Consensus visibility
The length of the error list is not consensus-visible. Which error comes first is visible to clients, but it is not in the block results hash or in fees:
process_validation_result(v0 and v1) passes onlyerrors.first()/errors.remove(0)intoStateTransitionExecutionResult.execute_eventonly checksconsensus_errors.is_empty(), so fees don't depend on how many errors there are.ExecTxResult.codeis the consensus error's code: 10101 (JsonSchemaError) for every keyword.infocarries the serialized first error.check_txreadserrors.first(), but only for the mempool.Code,DataandGasUsedinto the results hash (deterministicExecTxResult).info, and so which keyword is reported, is not consensus-checked.maxLengthinstead ofpattern). That is client-visible ininfo, under the same error code, and feat(dpp)!: validate documents natively at protocol version 15 #5533 accepted it explicitly for the unreleased protocol version 15. PV14 behaviour is unchanged.What was done?
Test changes only; no code changes.
params.maxLength. ThenormalizedParentDomainNamecase ("should be less than 190 chars", 191 characters) now uses 64 characters and is named "should be less than 63 chars", matching the schema'smaxLengthof 63.avatarUrltest expectsminLength.requirederror.dependentRequiredmap is symmetric, so one reported error no longer shows which entry fired, and removing an entry would go unnoticed. Three new tests ("should require the other avatar properties when present alone") each set one avatar property alone; each fails if that property's entry is removed.typetests now also assert the instance path.How Has This Been Tested?
Red → green, locally, with
@dashevo/wasm-dppbuilt from this branch's base (yarn workspace @dashevo/wasm-dpp build, as CI's "Build JS packages" step does viayarn build):origin/v5.1-dev(cd00e8382d)@dashevo/dpns-contract@dashevo/dashpay-contract@dashevo/withdrawals-contractWhich commit started the failures, with the unchanged specs and wasm-dpp rebuilt at each commit:
eb3e1b8272(before #5533)2933198012(#5533)88c31c2bc1(#5538)Mutation check, with the schema JSON edited temporarily and then restored:
dependentRequiredentries fails exactly the matching new test and nothing else. Without the new tests, none of these removals was caught.labelmaxLengthto 64 fails "should be less than 63 chars", which reportspatterninstead.Other packages on the same validator, all passing, unchanged: moderation-charters (75), masternode-reward-shares (9), token-history (31), keyword-search (20), wallet-utils (10), app-connect (13), document-history (19).
@dashevo/wasm-dpp:test:nodegives 235 passing, 0 failing (533 pending are existing skips), andtest:typespasses.test:browserswas not run: there is no Chrome or Firefox on the test machine, and CI runs it nightly only.yarn workspace <pkg> lintpasses for the three changed packages.Breaking Changes
None. Test expectations only.
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
PR Hygiene ·
aa0f616/self-reviewedWhen every merge requirement is met, the
PR Hygienecheck passes. Reviewer limits do not block merging; other required GitHub checks and protections still apply.Summary by CodeRabbit