feat(sdk)!: pure DPNS and DashPay document builders shared with embedders - #4632
feat(sdk)!: pure DPNS and DashPay document builders shared with embedders#4632PastaPastaPasta wants to merge 2 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe change adds transport-free DashPay contact request and DPNS document builders. The SDK now uses these builders for document creation, validation, identifier generation, and submission. The builders are publicly re-exported through the platform SDK modules. ChangesDocument builder foundation
Priority: ➖ Normal Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant DashPaySDK
participant ContactRequestBuilder
participant DashpayContract
participant Platform
DashPaySDK->>ContactRequestBuilder: Build contact request parameters
ContactRequestBuilder->>DashpayContract: Resolve contactRequest schema
ContactRequestBuilder-->>DashPaySDK: Return validated document and entropy
DashPaySDK->>Platform: Submit the document with entropy
Platform-->>DashPaySDK: Return submission result
sequenceDiagram
participant DPNSSDK
participant DPNSBuilders
participant DPNSContract
participant Platform
DPNSSDK->>DPNSBuilders: Build preorder and domain documents
DPNSBuilders->>DPNSContract: Resolve preorder and domain schemas
DPNSBuilders-->>DPNSSDK: Return entropy-derived documents
DPNSSDK->>Platform: Register the documents
Platform-->>DPNSSDK: Return registration result
Merge Risk: ⚪ Minimal · up to The change centralizes DPNS and DashPay document construction while preserving the existing SDK document behavior. No actionable correctness, security, or availability risk remains identified for merge. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 79.17% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 24 functions across 7 files. (1 skipped: 1 unsupported.)
✨ Finishing Touches 💡 1📝 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 |
05927f9 to
1e8f252
Compare
0631c07 to
5279114
Compare
47b05e3 to
cfb93ac
Compare
|
✅ Final review complete — no blockers (commit 796a86b) · triage: normal |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## v4.2-dev #4632 +/- ##
============================================
- Coverage 87.86% 87.41% -0.45%
============================================
Files 2766 2797 +31
Lines 360981 366966 +5985
============================================
+ Hits 317162 320801 +3639
- Misses 43819 46165 +2346
🚀 New features to boost your workflow:
|
…ders dash-platform-queries gains build_dpns_preorder_document / build_dpns_domain_document / salted_domain_hash (dpns_usernames) and build_contact_request_document (new dashpay module): the document-assembly halves of dash-sdk's register_dpns_name and create_contact_request as pure functions that take caller-supplied entropy, salt and ciphertexts and touch no network or randomness. dash-sdk's networked flows now call them, so an embedder that assembles its own transitions (the Dash Core platform GUI) and the SDK share one implementation. This is a move, not a rewrite: the documents these builders produce are byte for byte what the inline SDK code produced. The builders lean on what the codebase already has rather than re-deriving it: normalization is dpp's consensus convert_to_homograph_safe_chars (the crate's ASCII-only copy, whose non-ASCII behaviour differed from the data trigger's, is replaced by a re-export; the two agree on every label the contract's ASCII-only pattern admits); the preorder commitment uses dpp::util::hash::hash_double; property names come from the dpns-contract / dashpay-contract constants; and the DashPay byte-array bounds (96 / 48-80 / 38-102) are read from the contract schema instead of being hard-coded to the same numbers. Deliberately not changed here, to keep this reviewable as a pure extraction: - No new label validation. An earlier draft rejected labels via is_valid_username before the preorder was paid for, but that helper is stricter than the DPNS contract (it also refuses consecutive hyphens, which the contract's pattern admits), so it would have refused names Platform accepts. Failing early on a bad label is worth doing on its own, against the contract's actual pattern; it is not this PR. - No entropy/document-id consistency check in dpp. dash-sdk keeps its existing private ensure_entropy_matches_document_id. Hoisting that into DocumentCreateTransitionV0::from_document so every caller inherits it is a good change, but it adds an error path to a shared crate and is separable. The autoAcceptProof bound is still checked in create_contact_request before the recipient lookup: the shared builder re-checks it against the schema, but that field is raw caller input and the lookup is a network round trip. The two checks on the SDK's own encryption output are dropped as dead code — the pre-existing COMPACT_XPUB_LEN guard forces the encrypted xpub to 96 bytes and fit_account_label bounds the encrypted label to 48-80 — and the builder covers both for embedders that do their own encryption. API shape (unreleased v4.2-dev): ContactRequestResult now carries the assembled document plus entropy instead of id/owner_id/properties; send_contact_request no longer hand-rebuilds a DocumentV0.
cfb93ac to
edb833b
Compare
The coverage phase of tests-rs-workspace.yml drives nextest from an explicit package allowlist, and dash-platform-queries was never added to it when the crate was split out of dash-sdk. The crate still reaches the report as a dependency of dash-sdk, so llvm-cov instruments its lines — but its own test binaries are never run, and every line it owns is recorded as a miss. Two consequences: the crate's unit tests have not executed in CI since the split, and any PR touching it is charged for uncovered lines that its tests do in fact cover, which no amount of added testing can fix from the PR side. Adding the package runs those tests and makes the reported coverage reflect them. It only adds hits, since the lines were already in the denominator.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Phase 1 + Phase 2
The extraction is faithful and preserves the existing SDK behavior, document IDs, entropy handling, normalization, and schema-derived byte bounds. The new public builders have a few quality issues: one test does not independently verify the trigger commitment, invalid caller data is mislabeled as SDK configuration, and one builder doc recommends a stricter pre-check than consensus requires.
🟡 2 suggestion(s) | 💬 3 nitpick(s)
Review provenance
Source: reviewer 1: glm-5.3-flash (agent: phase1-reviewer, role: general); reviewer 2: glm-5.3-flash (agent: phase1-reviewer, role: security-auditor); reviewer 3: gpt-6-astra (agent: phase2-reviewer, role: general); reviewer 4: gpt-6-astra (agent: phase2-reviewer, role: rust-quality); reviewer 5: gpt-6-astra (agent: phase2-reviewer, role: security-auditor); final verifier: gpt-6-astra (agent: astra-verifier, role: final-verifier)
- Triage:
normalbygpt-6-astra(effort low) — This is a substantial cross-package extraction affecting DPNS and DashPay document construction, encryption-related field handling, schema-derived bounds, and SDK flow integration, but it does not itself alter consensus rules, signing/key handling, funds movement, or other critical surfaces. - Phase 1 reviewers:
glm-5.3-flash— general (completed, effort max); agentphase1-reviewer,glm-5.3-flash— security-auditor (completed, effort max); agentphase1-reviewer - Phase 1 model:
glm-5.3-flash— zai quota: 5h 69% left, weekly 36% left; passed overgemini-3.8-flash-high(antigravity below 15% reserve: weekly 53% left, 5h 2% left) - Fresh verifier:
gpt-6-astra— final-verifier; agentastra-verifier - Phase 2 reviewers:
gpt-6-astra— general (completed, effort high); agentphase2-reviewer,gpt-6-astra— rust-quality (completed, effort high); agentphase2-reviewer,gpt-6-astra— security-auditor (completed, effort high); 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/dash-platform-queries/src/dashpay.rs`:
- [SUGGESTION] packages/dash-platform-queries/src/dashpay.rs:121-127: Caller-input size violations surface as Error::Config
`check_byte_field` reports invalid encrypted fields through `Error::Config`, whose documented meaning is that the SDK is misconfigured. These values are caller input (`encryptedPublicKey`, `encryptedAccountLabel`, or `autoAcceptProof`), so embedders cannot distinguish malformed document data from an integration/configuration failure. Add a dedicated invalid-field error variant and map it to the corresponding non-configuration SDK error.
- [NITPICK] packages/dash-platform-queries/src/dashpay.rs:118-122: Byte-bounds validation silently becomes a no-op when schema bounds are absent
`unwrap_or(0)` and `unwrap_or(u16::MAX)` make `check_byte_field` fail open if a property type reports no size bounds. The current DashPay contract makes that path unreachable, so this is not an active vulnerability, but the helper is intended to provide schema-driven validation for embedders. Return an error when either bound is unavailable so a future contract/type change cannot silently remove local validation.
In `packages/dash-platform-queries/src/dpns_usernames.rs`:
- [SUGGESTION] packages/dash-platform-queries/src/dpns_usernames.rs:140-143: Builder documentation recommends a pre-check that rejects valid contract labels
The documentation points callers to `is_valid_username` as a client-side pre-check, but that helper rejects consecutive hyphens while the contract pattern `^[a-zA-Z0-9][a-zA-Z0-9-]{0,61}[a-zA-Z0-9]$` accepts them. An embedder following this documentation would reject names that consensus accepts. Document the actual contract pattern, or explicitly warn that `is_valid_username` is stricter than consensus until it is corrected separately.
- [NITPICK] packages/dash-platform-queries/src/dpns_usernames.rs:251-256: Commitment test compares the builder against its own helper
The test claims to verify agreement with the DPNS trigger, but both the builder and assertion call `salted_domain_hash` with the same inputs. This verifies that the builder invokes the helper, not that the preimage framing matches the independently implemented trigger. Reconstruct the expected preimage from the domain document's stored salt and normalized label, or assert against a fixed independently computed digest so a coordinated helper/preimage regression cannot pass unnoticed.
- [NITPICK] packages/dash-platform-queries/src/dpns_usernames.rs:252-277: DPNS builder test does not pin all generated properties
The DPNS test checks only the normalized label, label, preorder salt, and document IDs. It does not assert `parentDomainName`, `normalizedParentDomainName`, `records`, or `subdomainRules`, even though these fields are part of the shared builder output and are consumed by the DPNS trigger. Add assertions for every generated property to catch dropped entries or incorrect `Value` variants.
Out-of-scope follow-up suggestions (3)
These are valid observations, but they are outside this PR's scope and should be handled in separate issues or author/maintainer-requested PRs rather than blocking this review.
- Align is_valid_username with the deployed DPNS contract —
is_valid_usernamerejects consecutive hyphens even though the DPNS contract pattern accepts them, and its documentation attributes that restriction to the pattern. This is pre-existing behavior outside the pure-builder extraction, but it can cause client-side false negatives on existing registration paths.- Follow-up: Track a separate change that aligns the helper and its documentation with the contract, then review all callers that use it as a hard gate.
- Move entropy/document-ID consistency validation into DPP — The entropy/document-ID consistency check remains in the SDK-specific path, so embedders assembling transitions directly after using these builders do not receive the same early consistency check. Consensus still rejects mismatched transitions, making this a hardening and user-feedback improvement rather than a correctness vulnerability in this PR.
- Follow-up: Implement the check in the shared document-create transition constructor in a separate change covering SDK, WASM, FFI, and embedder callers.
- Generic document-construction helper parked in the DPNS module — NOT_ACTIONABLE: The helper is crate-private, has only two current consumers, and its placement introduces no correctness, API, or dependency problem. Moving it would be organizational cleanup outside the feature's stated goal.
- Follow-up: Consider creating a separate issue or author/maintainer-requested PR for this.
| let min = property.property_type.min_size().unwrap_or(0) as usize; | ||
| let max = property.property_type.max_size().unwrap_or(u16::MAX) as usize; | ||
| if bytes.len() < min || bytes.len() > max { | ||
| return Err(Error::Config(format!( | ||
| "{field} must be {min}-{max} bytes, got {}", | ||
| bytes.len() | ||
| ))); |
There was a problem hiding this comment.
🟡 Suggestion: Caller-input size violations surface as Error::Config
check_byte_field reports invalid encrypted fields through Error::Config, whose documented meaning is that the SDK is misconfigured. These values are caller input (encryptedPublicKey, encryptedAccountLabel, or autoAcceptProof), so embedders cannot distinguish malformed document data from an integration/configuration failure. Add a dedicated invalid-field error variant and map it to the corresponding non-configuration SDK error.
source: glm-5.3-flash (phase1-reviewer: general, security-auditor)
| /// `normalizedLabel` is the consensus normalization of `label`. The label is | ||
| /// not validated here; the contract's label pattern is enforced by consensus | ||
| /// when the domain document is created (see [`is_valid_username`] for a | ||
| /// client-side pre-check). |
There was a problem hiding this comment.
🟡 Suggestion: Builder documentation recommends a pre-check that rejects valid contract labels
The documentation points callers to is_valid_username as a client-side pre-check, but that helper rejects consecutive hyphens while the contract pattern ^[a-zA-Z0-9][a-zA-Z0-9-]{0,61}[a-zA-Z0-9]$ accepts them. An embedder following this documentation would reject names that consensus accepts. Document the actual contract pattern, or explicitly warn that is_valid_username is stricter than consensus until it is corrected separately.
| /// `normalizedLabel` is the consensus normalization of `label`. The label is | |
| /// not validated here; the contract's label pattern is enforced by consensus | |
| /// when the domain document is created (see [`is_valid_username`] for a | |
| /// client-side pre-check). | |
| /// `normalizedLabel` is the consensus normalization of `label`. The label is | |
| /// not validated here; the contract's label pattern | |
| /// (`^[a-zA-Z0-9][a-zA-Z0-9-]{0,61}[a-zA-Z0-9]$`) is enforced by consensus | |
| /// when the domain document is created. |
source: glm-5.3-flash (phase1-reviewer: general, security-auditor)
| // The commitment in the preorder is the one the DPNS data trigger | ||
| // recomputes from the domain document's salt and normalized label. | ||
| assert_eq!( | ||
| preorder.get("saltedDomainHash"), | ||
| Some(&Value::Bytes32(salted_domain_hash("Alice", salt))) | ||
| ); |
There was a problem hiding this comment.
💬 Nitpick: Commitment test compares the builder against its own helper
The test claims to verify agreement with the DPNS trigger, but both the builder and assertion call salted_domain_hash with the same inputs. This verifies that the builder invokes the helper, not that the preimage framing matches the independently implemented trigger. Reconstruct the expected preimage from the domain document's stored salt and normalized label, or assert against a fixed independently computed digest so a coordinated helper/preimage regression cannot pass unnoticed.
source: glm-5.3-flash (phase1-reviewer: general, security-auditor)
| // recomputes from the domain document's salt and normalized label. | ||
| assert_eq!( | ||
| preorder.get("saltedDomainHash"), | ||
| Some(&Value::Bytes32(salted_domain_hash("Alice", salt))) | ||
| ); | ||
| assert_eq!( | ||
| domain.get(domain::properties::NORMALIZED_LABEL), | ||
| Some(&Value::Text("a11ce".to_string())) | ||
| ); | ||
| assert_eq!( | ||
| domain.get(domain::properties::LABEL), | ||
| Some(&Value::Text("Alice".to_string())) | ||
| ); | ||
| assert_eq!( | ||
| domain.get(domain::properties::PREORDER_SALT), | ||
| Some(&Value::Bytes32(salt)) | ||
| ); | ||
| assert_eq!( | ||
| preorder.id(), | ||
| Document::generate_document_id_v0( | ||
| &contract.id(), | ||
| &owner, | ||
| PREORDER_DOCUMENT_TYPE, | ||
| &entropy | ||
| ) | ||
| ); |
There was a problem hiding this comment.
💬 Nitpick: DPNS builder test does not pin all generated properties
The DPNS test checks only the normalized label, label, preorder salt, and document IDs. It does not assert parentDomainName, normalizedParentDomainName, records, or subdomainRules, even though these fields are part of the shared builder output and are consumed by the DPNS trigger. Add assertions for every generated property to catch dropped entries or incorrect Value variants.
source: glm-5.3-flash (phase1-reviewer: general, security-auditor)
| // Only `Array`/`VariableTypeArray` report no size, and dpp refuses those | ||
| // at contract creation ("only byte arrays are supported now"), so every | ||
| // property reachable here is sized and the defaults never apply. | ||
| let min = property.property_type.min_size().unwrap_or(0) as usize; | ||
| let max = property.property_type.max_size().unwrap_or(u16::MAX) as usize; |
There was a problem hiding this comment.
💬 Nitpick: Byte-bounds validation silently becomes a no-op when schema bounds are absent
unwrap_or(0) and unwrap_or(u16::MAX) make check_byte_field fail open if a property type reports no size bounds. The current DashPay contract makes that path unreachable, so this is not an active vulnerability, but the helper is intended to provide schema-driven validation for embedders. Return an error when either bound is unavailable so a future contract/type change cannot silently remove local validation.
| // Only `Array`/`VariableTypeArray` report no size, and dpp refuses those | |
| // at contract creation ("only byte arrays are supported now"), so every | |
| // property reachable here is sized and the defaults never apply. | |
| let min = property.property_type.min_size().unwrap_or(0) as usize; | |
| let max = property.property_type.max_size().unwrap_or(u16::MAX) as usize; | |
| let (min, max) = match ( | |
| property.property_type.min_size(), | |
| property.property_type.max_size(), | |
| ) { | |
| (Some(min), Some(max)) => (min as usize, max as usize), | |
| _ => { | |
| return Err(Error::Config(format!( | |
| "{field} declares no byte bounds to validate against" | |
| ))) | |
| } | |
| }; |
source: glm-5.3-flash (phase1-reviewer: general, security-auditor)
2 participants
Issue being fixed or feature implemented
Carries forward the pure-builder half of #4619, without that PR's request-driven
FromProof<GetDocumentsRequest>verifier, which the SDK-first C++ embedding (next PR in this series) no longer needs: the SDK retains the rich query it built and verifies against it, so no wire request is reconstructed from bytes.dash-sdk'sregister_dpns_nameandcreate_contact_requestassemble DPNS and DashPay documents inline. An embedder that signs with a wallet-held key (Dash Core's platform GUI) needs the same assembly as a pure function over caller-supplied entropy, salt and ciphertexts, and must not reimplement it in C++.What was done?
Pure document builders, in
dash-platform-queries:dpns_usernames::{build_dpns_preorder_document, build_dpns_domain_document, salted_domain_hash}anddashpay::build_contact_request_document, the assembly halves ofdash-sdk'sregister_dpns_name/create_contact_requestas pure functions.dash-sdk's networked flows now call them.convert_to_homograph_safe_chars(the crate's ASCII-only copy is replaced by a re-export); the preorder commitment usesdpp::util::hash::hash_double; property names come from thedpns-contract/dashpay-contractconstants; the DashPay byte bounds are read from the contract schema'sDocumentPropertyTypesizes instead of being hard-coded to the same numbers.This is a move, not a rewrite. The documents the builders produce are byte for byte what the inline SDK code produced. The normalization swap is the only substitution, and the two implementations agree on every label the contract's ASCII-only pattern admits — they differ only on non-ASCII input, which consensus rejects anyway. The re-export additionally makes the crate agree with the DPNS data trigger.
Deliberately not in this PR
Two behaviour changes were dropped to keep this reviewable as a pure extraction. Both are worth doing on their own:
is_valid_usernamebefore the preorder was paid for. That helper is stricter than the DPNS contract — it also refuses consecutive hyphens, which the contract's^[a-zA-Z0-9][a-zA-Z0-9-]{0,61}[a-zA-Z0-9]$pattern admits — so it would have refused names Platform accepts, on paths (wasm-sdk,platform-wallet) that previously had no local check at all. Failing early on a bad label is a real improvement, but it should check the contract's actual pattern, and it belongs in its own PR alongside a fix tois_valid_username's docstring, which currently claims the pattern forbids consecutive hyphens.dash-sdkkeeps its existing privateensure_entropy_matches_document_id. Hoisting it intoDocumentCreateTransitionV0::from_documentso every caller (SDK, wasm, FFI, embedders) inherits it is a good change — it cannot change any outcome, since it only refuses transitions Drive would reject after the nonce bump — but it adds an error path to a shared crate and is separable from this move.packages/rs-dppandput_document.rsare therefore byte-identical tov4.2-devin this PR.The
autoAcceptProofbound is still checked increate_contact_requestbefore the recipient lookup. The shared builder re-checks it against the schema, but that field is raw caller input and the lookup is a network round trip, so the early rejection is preserved. The two checks on the SDK's own encryption output are dropped as unreachable code — the pre-existingCOMPACT_XPUB_LENguard forces the encrypted xpub to 96 bytes, andfit_account_labelbounds the encrypted label to 48-80 — and the builder covers both for embedders doing their own encryption.How Has This Been Tested?
dash-platform-queries: 60 lib tests pass, including builder tests against the real DPNS/DashPay system contracts (preorder commitment matches the domain document's salt+label, id derivation, schema byte-bound enforcement).dash-sdk --lib: 187 pass.cargo checkclean forplatform-wallet,rs-sdk-ffi,strategy-tests,rs-scripts.cargo fmt --checkclean.sha256dhelper) and asserted document equality against the new builders, across several DPNS labels — includingalice--bob,-badandab, which the dropped gate used to reject — and a contact request with every optional field populated. All equal. Not committed; it exists only to prove the extraction.register_dpns_name/send_contact_request. The on-wire behaviour is byte-identical by construction and the unit tests pin the property maps.Breaking Changes
API shape on unreleased
v4.2-dev(not on crates.io):dash_sdk::platform::dashpay::ContactRequestResultnow carries the assembleddocumentplusentropyinstead ofid/owner_id/properties. No consumer outsiders-sdkuses it. No behavioural breaking changes.Checklist:
For repository code-owners and collaborators only
Summary by CodeRabbit
New Features
Improvements
Tests