feat(sdk)!: pure DPNS and DashPay document builders shared with embedders - #4632
Open
PastaPastaPasta wants to merge 2 commits into
Open
feat(sdk)!: pure DPNS and DashPay document builders shared with embedders#4632PastaPastaPasta wants to merge 2 commits into
PastaPastaPasta wants to merge 2 commits into
Conversation
Contributor
|
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 |
PastaPastaPasta
force-pushed
the
build/vendor-locked-single-source
branch
from
September 8, 2026 21:16
05927f9 to
1e8f252
Compare
PastaPastaPasta
force-pushed
the
feat/shared-dpns-dashpay-builders
branch
from
September 8, 2026 21:16
0631c07 to
5279114
Compare
Base automatically changed from
build/vendor-locked-single-source
to
v4.2-dev
September 8, 2026 21:57
PastaPastaPasta
force-pushed
the
feat/shared-dpns-dashpay-builders
branch
2 times, most recently
from
September 10, 2026 19:36
47b05e3 to
cfb93ac
Compare
PastaPastaPasta
marked this pull request as ready for review
September 10, 2026 19:37
PastaPastaPasta
requested review from
QuantumExplorer,
lklimek and
shumkov
as code owners
September 10, 2026 19:37
Collaborator
|
🕓 Queued for automated review — 4th in line, estimated start in ~35 min (commit 796a86b)
|
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.
PastaPastaPasta
force-pushed
the
feat/shared-dpns-dashpay-builders
branch
from
September 10, 2026 20:18
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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