feat(dpp)!: payment addresses and payment key purposes (DIP-33) - #4272
feat(dpp)!: payment addresses and payment key purposes (DIP-33)#4272QuantumExplorer wants to merge 5 commits into
Conversation
… cache A read-only query thread fetches contracts from committed state with no transaction and populates the global cache from what it read. If such a thread reads a contract, is descheduled while block execution rewrites that contract, and performs its insert only after the block cache is promoted to the global cache, the unconditional insert clobbered the newer contract with the stale one. Block execution would then serialize documents against a different contract than a node whose cache was cold — the same consensus divergence the v13 DPNS refresh closes, reopened through a millisecond-scale scheduling window that exists on every contract rewrite, and that an attacker could widen at the predictable activation height by flooding document queries. Contract versions increase strictly monotonically (the data contract update transition enforces new == old + 1, and the v13 DPNS rewrite goes 1 -> 2), so an insert carrying a lower version than the cached entry is always a delayed writer racing a newer copy in, never fresh information. The insert now skips in that case, atomically per key via moka's compute API so the version comparison and the write have no window between them. Same-version inserts still overwrite, which the cache-hit fee-calculation path relies on. This closes the race for the ordinary contract-update path as well, where the batch finalization task evicts the superseded contract during block execution and the same delayed query insert could land after promotion. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Implements the transparent-rail (Core + Platform address) half of DIP-33: profile payment address fields, the two payment key purposes, and the client-side stealth one-time address derivation. DashPay contract v2 (protocol version 14): * profile gains optional corePaymentAddress (Base58 string) and platformPaymentAddress (21-byte DIP-18 storage form), positions 5-6. * Mirrors the DPNS-v2-at-PV13 mechanics: schema/v2, v2 loader, a new system_data_contract_versions v3 (dashpay: 2), and a transition_to_version_14 first-block migration that re-applies the DashPay contract. Reuses the SystemDataContract::ALL cache refresh, so the migration inherits the monotonic data-contract-cache guard that protects the DPNS v13 activation. Identity key purposes PAYMENT_SCAN (7) and PAYMENT_SPEND (8): * ECDSA_SECP256K1, non-signing, no contract bounds, at most one active key of each purpose per identity. Not searchable (no per-purpose key reference tree), found by fetching the identity's keys. * Gated at protocol version 14 via validate_identity_public_keys_structure v1 (in-transition rules) plus a validate_payment_key_uniqueness common state check wired into the identity update add path. Two new consensus errors: InvalidKeyPurposeKeyType (10535), TooManyPublicKeysOfPurpose (10536). Mirrored in wasm-dpp / wasm-dpp2. Stealth derivation: * platform-encryption::stealth — pure secp256k1 curve math (shared point, rail-domain-separated one-time tweak, one-time pub/secret keys) with DIP-33 known-answer test vectors. * platform-wallet stealth module — DIP-9 feature 33' key derivation and Core P2PKH / Platform one-time destination construction (payer, receiver, spender). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
📝 WalkthroughWalkthroughThe PR adds DashPay v2 schemas and protocol wiring, introduces DIP-33 payment purposes and stealth payment derivation, validates payment-key types and uniqueness, updates protocol v14 migration behavior, and makes contract-cache inserts monotonic by version. ChangesDashPay v2 contracts and payment-purpose contracts
DIP-33 stealth derivation
Payment-key validation
Protocol v14 and cache migration
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 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 |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## v4.2-dev #4272 +/- ##
============================================
- Coverage 87.53% 87.51% -0.02%
============================================
Files 2678 2682 +4
Lines 341047 341565 +518
============================================
+ Hits 298519 298919 +400
- Misses 42528 42646 +118
🚀 New features to boost your workflow:
|
QuantumExplorer
changed the title
|
⛔ Blockers found — Sonnet deferred (commit 6693d75) |
The PAYMENT_SCAN/PAYMENT_SPEND variants carry underscores, which trips clippy's non_camel_case_types lint (single-word all-caps variants do not). Matches the allow already on the rs-dpp Purpose enum. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ress key_wallet::Address has no pubkey_hash(); extract the P2PKH hash via script_pubkey().p2pkh_public_key_hash_bytes() as elsewhere in the crate. Only the test target used it, so lib check/clippy did not catch it. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…tests The pattern was enforced at consensus (verified: documents carrying a non-Base58 value were rejected with a JsonSchemaError), but it could not prove the checksum or network byte, so clients had to fully validate the address regardless. What it did do was make every random document generator using FillIfNotRequired emit schema-invalid dashpay profiles, breaking 16 unrelated document tests — a permanent tax on anyone generating a profile fixture. The byteArray platformPaymentAddress field broke nothing, which is the asymmetry that surfaced this. Consensus now constrains the length only; validating the address proper is explicitly a client responsibility (DIP-33 updated to match). Rebaselined for the larger PV14 dashpay contract, all latest-version only — historical protocol-version pins are untouched so chain replay stays bit-for-bit reproducible: * check_tx contract create/update processing fees * deterministic root hash after contract insertion * happy-path replace and delete fee baselines * expected profile document string representations, which now include the two new optional fields Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
CI fixes + one design change worth a lookThree CI rounds fixed. The third surfaced something I think is a genuine improvement, so flagging it explicitly rather than burying it in a commit. Dropped the Base58
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
packages/rs-platform-encryption/src/lib.rs (1)
13-13: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe intra-doc link points at a private module.
Line 26 declares
mod stealth;withoutpub. An intra-doc link to a private item does not resolve in generated documentation, and rustdoc emits aprivate_intra_doc_linkswarning. Either make the module public or drop the link.📝 Option A: make the module public
-mod stealth; +pub mod stealth;📝 Option B: keep the module private and remove the link
-//! - [`stealth`] — DIP-33 stealth one-time key derivation. +//! - `stealth` — DIP-33 stealth one-time key derivation (re-exported at the crate root).🤖 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-platform-encryption/src/lib.rs` at line 13, Resolve the broken intra-doc link to stealth in the crate-level documentation by either making the stealth module public at its declaration or removing the [`stealth`] entry from the documentation list; preserve the intended visibility of the module and ensure rustdoc no longer reports a private_intra_doc_links warning.packages/rs-platform-wallet/src/wallet/identity/crypto/stealth.rs (1)
161-172: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winEach stealth entry point builds its own
Secp256k1context. The root cause is that the module creates the context instead of accepting one.Secp256k1::new()allocates and initializes the full pre-computation table, which costs far more than the one or two curve operations that follow it.recognize_one_time_destinationruns once per candidate notification and per output index during wallet scanning, so this sits on a hot path. Add asecp: &Secp256k1<C>parameter to the three public functions, or use the crate-globalsecp256k1::SECP256K1.
packages/rs-platform-wallet/src/wallet/identity/crypto/stealth.rs#L161-L172: removelet secp = Secp256k1::new();fromderive_one_time_destinationand take the context from the caller.packages/rs-platform-wallet/src/wallet/identity/crypto/stealth.rs#L200-L200: removelet secp = Secp256k1::new();fromrecognize_one_time_destinationand take the context from the caller. This is the scanning path and benefits most.packages/rs-platform-wallet/src/wallet/identity/crypto/stealth.rs#L225-L225: removelet secp = Secp256k1::new();fromderive_one_time_secret_key. Onlystealth_shared_pointneeds the context;one_time_secret_keyperforms scalar addition and needs none.🤖 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-platform-wallet/src/wallet/identity/crypto/stealth.rs` around lines 161 - 172, The stealth entry points independently initialize expensive Secp256k1 contexts; update derive_one_time_destination, recognize_one_time_destination, and derive_one_time_secret_key in packages/rs-platform-wallet/src/wallet/identity/crypto/stealth.rs at lines 161-172, 200-200, and 225-225 to reuse a caller-provided context or the crate-global SECP256K1. Remove each Secp256k1::new() allocation, thread the context through the affected calls, and ensure derive_one_time_secret_key passes it only to stealth_shared_point while leaving one_time_secret_key context-free.packages/rs-platform-encryption/src/stealth.rs (1)
121-155: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winConsider making the modular reduction branch-free, or removing it.
ge_orderreturns on the first differing byte andreduce_mod_ordertakes a data-dependent branch. The input is SHA256 of the shared secret point, so the control flow depends on secret-derived data. The leak is weak because SHA256 is one-way, but a cryptographic primitive crate is the wrong place to accept a variable-time scalar path.Two options:
- Make the subtraction unconditional and select the result with a constant-time mask.
- Drop the reduction.
Scalar::from_be_bytesalready rejects values at or above the order. Treat that rejection like the existing zero case and require a fresh ephemeral key. The probability of either event is about 2^-128.Option 2 removes
reduce_mod_order,ge_order, andCURVE_ORDER_BEentirely. Confirm first that the DIP-33 specification defines the tweak as a reduction and not as a rejection, because the two differ observably on out-of-range digests.🤖 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-platform-encryption/src/stealth.rs` around lines 121 - 155, Verify whether DIP-33 requires reducing the SHA256-derived tweak modulo the curve order or rejecting out-of-range values. If rejection is permitted, remove reduce_mod_order, ge_order, and CURVE_ORDER_BE, and treat Scalar::from_be_bytes failure like the existing zero-tweak case by generating a fresh ephemeral key; otherwise replace the data-dependent comparison and subtraction in those helpers with constant-time unconditional subtraction and masked selection.packages/rs-drive-abci/src/execution/validation/state_transition/common/validate_payment_key_uniqueness/v0/mod.rs (1)
59-69: 🚀 Performance & Scalability | 🔵 TrivialUnbounded key fetch scales with total historical keys, not the current transition.
The
AllKeysfetch has nolimit. Total key count for an identity is unbounded over its lifetime, sincemax_public_keys_in_creationonly caps keys added per transition, not the total accumulated over many updates. Consider monitoring identities with unusually large key counts, or revisit pagination if this becomes a hot path under high key churn.🤖 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-drive-abci/src/execution/validation/state_transition/common/validate_payment_key_uniqueness/v0/mod.rs` around lines 59 - 69, Review the key lookup in the payment-key uniqueness validation flow and avoid relying on an unbounded AllKeys fetch for identities with large historical key counts. Update the logic around IdentityKeysRequest and fetch_identity_keys to use an appropriate bounded or paginated strategy while still checking all keys relevant to the current transition.
🤖 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-dpp/src/errors/consensus/basic/basic_error.rs`:
- Around line 604-613: Move InvalidKeyPurposeKeyTypeError and
TooManyPublicKeysOfPurposeError out of their current position in BasicError and
append both variants after the existing tail variant
TokenPricingScheduleEmptyError, preserving their transparent error annotations
and the order of all existing variants.
In `@packages/rs-platform-encryption/src/stealth.rs`:
- Around line 167-175: Verify whether the expected DIP-33 vector constants and
outputs used by the known-answer test were copied from the published DIP-33
specification. If so, cite the exact DIP-33 section in the comment near B_SCAN,
B_SPEND, and R_EPHEMERAL; otherwise, reword the interop claim to describe the
test as a locally generated regression lock and note a follow-up to add
published vectors.
In `@packages/wasm-dpp2/src/enums/keys/purpose.rs`:
- Around line 83-84: Update the PurposeLike numeric conversion near the
PurposeWasm mappings to validate that the input is finite, integral, and within
the supported purpose range before casting to u8. Reject fractional values and
non-negative overflow values instead of allowing truncation or wrapping, while
preserving valid mappings such as 7 to PAYMENT_SCAN and 8 to PAYMENT_SPEND.
---
Nitpick comments:
In
`@packages/rs-drive-abci/src/execution/validation/state_transition/common/validate_payment_key_uniqueness/v0/mod.rs`:
- Around line 59-69: Review the key lookup in the payment-key uniqueness
validation flow and avoid relying on an unbounded AllKeys fetch for identities
with large historical key counts. Update the logic around IdentityKeysRequest
and fetch_identity_keys to use an appropriate bounded or paginated strategy
while still checking all keys relevant to the current transition.
In `@packages/rs-platform-encryption/src/lib.rs`:
- Line 13: Resolve the broken intra-doc link to stealth in the crate-level
documentation by either making the stealth module public at its declaration or
removing the [`stealth`] entry from the documentation list; preserve the
intended visibility of the module and ensure rustdoc no longer reports a
private_intra_doc_links warning.
In `@packages/rs-platform-encryption/src/stealth.rs`:
- Around line 121-155: Verify whether DIP-33 requires reducing the
SHA256-derived tweak modulo the curve order or rejecting out-of-range values. If
rejection is permitted, remove reduce_mod_order, ge_order, and CURVE_ORDER_BE,
and treat Scalar::from_be_bytes failure like the existing zero-tweak case by
generating a fresh ephemeral key; otherwise replace the data-dependent
comparison and subtraction in those helpers with constant-time unconditional
subtraction and masked selection.
In `@packages/rs-platform-wallet/src/wallet/identity/crypto/stealth.rs`:
- Around line 161-172: The stealth entry points independently initialize
expensive Secp256k1 contexts; update derive_one_time_destination,
recognize_one_time_destination, and derive_one_time_secret_key in
packages/rs-platform-wallet/src/wallet/identity/crypto/stealth.rs at lines
161-172, 200-200, and 225-225 to reuse a caller-provided context or the
crate-global SECP256K1. Remove each Secp256k1::new() allocation, thread the
context through the affected calls, and ensure derive_one_time_secret_key passes
it only to stealth_shared_point while leaving one_time_secret_key context-free.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: c7e5d2fe-3388-47f1-b8fc-6fa3b5aab1c1
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (48)
packages/dashpay-contract/schema/v2/dashpay.schema.jsonpackages/dashpay-contract/src/lib.rspackages/dashpay-contract/src/v2/mod.rspackages/rs-dpp/src/errors/consensus/basic/basic_error.rspackages/rs-dpp/src/errors/consensus/basic/identity/invalid_key_purpose_key_type_error.rspackages/rs-dpp/src/errors/consensus/basic/identity/mod.rspackages/rs-dpp/src/errors/consensus/basic/identity/too_many_public_keys_of_purpose_error.rspackages/rs-dpp/src/errors/consensus/codes.rspackages/rs-dpp/src/identity/identity_public_key/purpose.rspackages/rs-dpp/src/state_transition/state_transitions/identity/public_key_in_creation/methods/validate_identity_public_keys_structure/mod.rspackages/rs-dpp/src/state_transition/state_transitions/identity/public_key_in_creation/methods/validate_identity_public_keys_structure/v1/mod.rspackages/rs-drive-abci/src/execution/check_tx/v0/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/common/mod.rspackages/rs-drive-abci/src/execution/validation/state_transition/common/validate_payment_key_uniqueness/mod.rspackages/rs-drive-abci/src/execution/validation/state_transition/common/validate_payment_key_uniqueness/v0/mod.rspackages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/batch/tests/document/deletion.rspackages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/batch/tests/document/replacement.rspackages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/identity_update/state/v0/mod.rspackages/rs-drive/src/cache/data_contract.rspackages/rs-drive/src/cache/system_contracts.rspackages/rs-drive/src/drive/contract/refresh_cache/mod.rspackages/rs-drive/src/drive/identity/estimation_costs/for_purpose_in_key_reference_tree/v0/mod.rspackages/rs-drive/tests/deterministic_root_hash.rspackages/rs-platform-encryption/Cargo.tomlpackages/rs-platform-encryption/src/error.rspackages/rs-platform-encryption/src/lib.rspackages/rs-platform-encryption/src/stealth.rspackages/rs-platform-version/src/version/dpp_versions/dpp_state_transition_method_versions/mod.rspackages/rs-platform-version/src/version/dpp_versions/dpp_state_transition_method_versions/v2.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/v1.rspackages/rs-platform-version/src/version/drive_abci_versions/drive_abci_validation_versions/v2.rspackages/rs-platform-version/src/version/drive_abci_versions/drive_abci_validation_versions/v3.rspackages/rs-platform-version/src/version/drive_abci_versions/drive_abci_validation_versions/v4.rspackages/rs-platform-version/src/version/drive_abci_versions/drive_abci_validation_versions/v5.rspackages/rs-platform-version/src/version/drive_abci_versions/drive_abci_validation_versions/v6.rspackages/rs-platform-version/src/version/drive_abci_versions/drive_abci_validation_versions/v7.rspackages/rs-platform-version/src/version/drive_abci_versions/drive_abci_validation_versions/v8.rspackages/rs-platform-version/src/version/drive_abci_versions/drive_abci_validation_versions/v9.rspackages/rs-platform-version/src/version/system_data_contract_versions/mod.rspackages/rs-platform-version/src/version/system_data_contract_versions/v3.rspackages/rs-platform-version/src/version/v14.rspackages/rs-platform-wallet/src/wallet/identity/crypto/mod.rspackages/rs-platform-wallet/src/wallet/identity/crypto/stealth.rspackages/wasm-dpp/src/errors/consensus/consensus_error.rspackages/wasm-dpp/src/identity/identity_public_key/purpose.rspackages/wasm-dpp2/src/enums/keys/purpose.rs
| #[error(transparent)] | ||
| InvalidKeyPurposeForContractBoundsError(InvalidKeyPurposeForContractBoundsError), | ||
|
|
||
| #[error(transparent)] | ||
| InvalidKeyPurposeKeyTypeError(InvalidKeyPurposeKeyTypeError), | ||
|
|
||
| #[error(transparent)] | ||
| TooManyPublicKeysOfPurposeError(TooManyPublicKeysOfPurposeError), | ||
|
|
||
| #[error(transparent)] |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🔴 Critical | 🏗️ Heavy lift
Move the two new variants to the tail of BasicError.
BasicError carries an explicit warning at line 694 that it is bincode-encoded positionally and that new variants must be appended at the tail, because inserting mid-enum shifts the wire discriminants of every following variant and mis-decodes previously-encoded errors.
InvalidKeyPurposeKeyTypeError and TooManyPublicKeysOfPurposeError are inserted right after InvalidKeyPurposeForContractBoundsError at line 605. That position is not the tail. Dozens of variants follow it, from StateTransitionNotActiveError through TokenPricingScheduleEmptyError. Every one of those variants now has a discriminant that is +2 relative to any prior encoding.
Move both new variants to the true tail, after TokenPricingScheduleEmptyError, to preserve the wire format for previously-encoded errors.
🐛 Proposed fix: relocate the new variants to the tail
#[error(transparent)]
InvalidKeyPurposeForContractBoundsError(InvalidKeyPurposeForContractBoundsError),
- #[error(transparent)]
- InvalidKeyPurposeKeyTypeError(InvalidKeyPurposeKeyTypeError),
-
- #[error(transparent)]
- TooManyPublicKeysOfPurposeError(TooManyPublicKeysOfPurposeError),
-
#[error(transparent)]
StateTransitionNotActiveError(StateTransitionNotActiveError), #[error(transparent)]
TokenPricingScheduleEmptyError(TokenPricingScheduleEmptyError),
+
+ #[error(transparent)]
+ InvalidKeyPurposeKeyTypeError(InvalidKeyPurposeKeyTypeError),
+
+ #[error(transparent)]
+ TooManyPublicKeysOfPurposeError(TooManyPublicKeysOfPurposeError),
}Run this to confirm the positional-encoding assumption and locate any round-trip tests that would catch this:
#!/bin/bash
# Description: Inspect BasicError's derive attributes and any bincode round-trip coverage.
set -euo pipefail
fd -e rs basic_error.rs --exec cat -n {}
# Check the derive list and any custom Encode/Decode impl.
rg -n -B3 -A3 'enum BasicError' packages/rs-dpp/src/errors/consensus/basic/basic_error.rs
# Look for existing tests that round-trip BasicError via bincode.
rg -nP '\bBasicError\b' --type=rust -g '*test*' -C3Also applies to: 694-706
🤖 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/errors/consensus/basic/basic_error.rs` around lines 604 -
613, Move InvalidKeyPurposeKeyTypeError and TooManyPublicKeysOfPurposeError out
of their current position in BasicError and append both variants after the
existing tail variant TokenPricingScheduleEmptyError, preserving their
transparent error annotations and the order of all existing variants.
| /// DIP-33 test vector inputs (test-only scalars). | ||
| const B_SCAN: &str = "61aafd85dbca17133515038343b05ed2019ca465d1ba93dcbffbf9534d2f436c"; | ||
| const B_SPEND: &str = "8f54f1cfa054739a1d4a87847bdf6a024caf78ff4b2c91aeb75cb843280957ff"; | ||
| const R_EPHEMERAL: &str = "37ca08877b0beea1b1e7655649274689e89944edae537ecad42764e59ac31e3e"; | ||
|
|
||
| /// Known-answer test pinning the full DIP-33 vector table: shared point, | ||
| /// tweaks, one-time public keys, and one-time secret keys for both rails. | ||
| /// A mismatch here means we broke interop with every other implementation | ||
| /// of the DIP, not just our own round-trip. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
State the provenance of the expected vector values.
The comment claims a mismatch breaks interop with every other DIP-33 implementation. That holds only if the expected hex strings at lines 215-231 were copied from the DIP-33 document. If they were generated by running this code, the test is a regression lock and cannot detect a specification mismatch.
Confirm the source. If the values come from the DIP, cite the section in the comment. If they do not, reword the comment and open a follow-up to add the published vectors.
🤖 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-platform-encryption/src/stealth.rs` around lines 167 - 175,
Verify whether the expected DIP-33 vector constants and outputs used by the
known-answer test were copied from the published DIP-33 specification. If so,
cite the exact DIP-33 section in the comment near B_SCAN, B_SPEND, and
R_EPHEMERAL; otherwise, reword the interop claim to describe the test as a
locally generated regression lock and note a follow-up to add published vectors.
| 7 => Ok(PurposeWasm::PAYMENT_SCAN), | ||
| 8 => Ok(PurposeWasm::PAYMENT_SPEND), |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
rg -n -C 5 \
'PurposeLike|as_f64\(\)|as u8|PAYMENT_SCAN|PAYMENT_SPEND|7\.5|8\.5' \
packages/wasm-dpp2Repository: dashpay/platform
Length of output: 38933
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
echo "== packages/wasm-dpp2/src/enums/keys/purpose.rs =="
wc -l packages/wasm-dpp2/src/enums/keys/purpose.rs
cat -n packages/wasm-dpp2/src/enums/keys/purpose.rs
echo
echo "== helper conversions around try_to_u8 =="
sed -n '588,632p' packages/wasm-dpp2/src/utils.rs | cat -n
echo
echo "== purpose-related tests =="
rg -n -C 4 'Purpose|purpose|PAYMENT_SCAN|PAYMENT_SPEND|7\.5|8\.5' packages/wasm-dpp2/tests packages/wasm-dpp2/src --glob '*.rs' || trueRepository: dashpay/platform
Length of output: 50372
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
echo "== Rust f64 to u16 cast behavior =="
cat > /tmp/cast_probe.rs <<'RS'
fn main() {
for value in [7.5, 8.5, -1.5, 208.5, 9.5, 0.5] {
println!("{} as u16 = {}", value, value as u16);
}
}
RS
rustc /tmp/cast_probe.rs -o /tmp/cast_probe && /tmp/cast_probe
echo
echo "== behavioral probe of Purpose numeric path =="
python3 - <<'PY'
def purpose_accept(num):
# Mirrors the numeric match after the direct u8 cast used in packages/wasm-dpp2/src/enums/keys/purpose.rs:75.
cast = num.astype('u1').view('u8')[0] if hasattr(num, 'astype') else int(num) % 256
try:
cast = int(num)
except TypeError:
cast = int(num)
return cast in (7, 8)
for value in [7.5, 8.5, -1.5, 208.5, 9.5, 0.5]:
# Python int() truncates toward zero; direct out-of-range u8 casts are ignored because source shows direct match.
print(value, "is accepted as PAYMENT_SCAN/SPEND" if int(value) in (7, 8) or int(value) % 256 in (7, 8) else "is rejected")
PYRepository: dashpay/platform
Length of output: 421
Reject invalid numeric purpose values before the cast.
PurposeLike accepts numeric values, but as_f64() followed by as u8 truncates fractional input. This makes 7.5 resolve to PAYMENT_SCAN, 8.5 resolve to PAYMENT_SPEND, and invalid non-negative overflow values wrap into accepted enum values such as 0 or 1. Validate finiteness, integrality, and the supported range before converting.
🤖 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/wasm-dpp2/src/enums/keys/purpose.rs` around lines 83 - 84, Update
the PurposeLike numeric conversion near the PurposeWasm mappings to validate
that the input is finite, integral, and within the supported purpose range
before casting to u8. Reject fractional values and non-negative overflow values
instead of allowing truncation or wrapping, while preserving valid mappings such
as 7 to PAYMENT_SCAN and 8 to PAYMENT_SPEND.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
The DIP-33 implementation is generally well-structured and version-dispatched, but three blocking issues remain: positional BasicError compatibility is broken, noncanonical 65-byte payment keys are accepted, and Swift rewrites the new key-purpose discriminants during persistence. Additional WASM and Kotlin boundary issues, retryable error flattening, and missing state-validation coverage should also be addressed.
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 (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 | 🟡 5 suggestion(s)
2 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/errors/consensus/basic/basic_error.rs`:
- [BLOCKING] packages/rs-dpp/src/errors/consensus/basic/basic_error.rs:607-611: Append the new BasicError variants to preserve wire discriminants
BasicError derives bincode Encode/Decode and PlatformSerialize/PlatformDeserialize, so its enum discriminants are positional. Inserting these variants before StateTransitionNotActiveError shifts every existing variant through TokenPricingScheduleEmptyError, causing previously encoded errors to decode as the wrong variant or fail. This directly violates the ordering invariant documented at the enum tail. Move both new variants after TokenPricingScheduleEmptyError without changing the order of any existing variant, and add a compatibility test that pins a pre-existing variant from after the current insertion point.
In `packages/rs-dpp/src/state_transition/state_transitions/identity/public_key_in_creation/methods/validate_identity_public_keys_structure/v1/mod.rs`:
- [BLOCKING] packages/rs-dpp/src/state_transition/state_transitions/identity/public_key_in_creation/methods/validate_identity_public_keys_structure/v1/mod.rs:148-161: Require compressed 33-byte encodings for payment keys
This check constrains payment keys to ECDSA_SECP256K1 but does not constrain their encoding. DPP's ECDSA public-key parser explicitly accepts both 33-byte compressed and 65-byte uncompressed SEC1 keys, and the generic added-key signature verifier passes either representation to PublicKey::from_slice. A signed transition can therefore register a 65-byte PAYMENT_SCAN or PAYMENT_SPEND key even though DIP-33 specifies compressed SEC1 points and this validator's own comment requires the full compressed key. Besides producing noncanonical persisted state, compressed and uncompressed encodings of the same point also evade the in-transition duplicate check, which compares raw bytes. Reject payment-purpose keys unless data().len() is exactly 33 and add a regression test using a valid 65-byte key.
In `packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift`:
- [BLOCKING] packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift:1798-1804: Preserve payment-key purposes across the Swift FFI boundary
Rust emits each DPP purpose as its raw u8 value, so the new PAYMENT_SCAN and PAYMENT_SPEND keys reach Swift as 7 and 8. Swift's KeyPurpose enum still ends at 6, and this fallback silently persists either new value as authentication. The cold-start restore path later sends that stored raw value back to Rust, changing the key's meaning, while ManagedIdentity.getPublicKeys() rejects the same values as unknown. Add paymentScan/paymentSpend cases with raw values 7 and 8, update exhaustive switches, and replace this semantic fallback with an unsupported-discriminant error rather than coercing an unknown purpose to authentication.
In `packages/wasm-dpp2/src/enums/keys/purpose.rs`:
- [SUGGESTION] packages/wasm-dpp2/src/enums/keys/purpose.rs:65-66: Accept the payment-purpose strings emitted by the WASM getter
The IdentityPublicKey purpose getter emits PAYMENT_SCAN and PAYMENT_SPEND. The setter lowercases those strings to payment_scan and payment_spend, but this parser accepts only paymentscan and paymentspend. Consequently, assigning key.purpose = key.purpose or copying a key through its exposed properties fails only for the new variants. Accept the underscore forms as aliases so getter values round-trip through constructors and setters.
- [SUGGESTION] packages/wasm-dpp2/src/enums/keys/purpose.rs:74-84: Reject fractional numeric payment-purpose values
PurposeLike's TypeScript union is not a runtime constraint. Casting an arbitrary JavaScript number directly to u8 truncates fractional values, so 7.5 is accepted as PAYMENT_SCAN and 8.5 as PAYMENT_SPEND. Validate that the number is finite, integral, and within the supported 0 through 8 range before casting. Rust's float-to-integer cast saturates rather than wraps, so positive overflow currently becomes 255 and is rejected; the newly introduced defect is specifically the acceptance of fractional values that truncate to 7 or 8.
In `packages/rs-drive-abci/src/execution/validation/state_transition/common/validate_payment_key_uniqueness/v0/mod.rs`:
- [SUGGESTION] packages/rs-drive-abci/src/execution/validation/state_transition/common/validate_payment_key_uniqueness/v0/mod.rs:37-84: Cover payment-key uniqueness against persisted identity state
This helper implements the consensus-critical against-state half of the one-active-key invariant, including the state-sensitive rotation exception, but rs-drive-abci contains no PAYMENT_SCAN or PAYMENT_SPEND tests. Add tests proving that an add-only update conflicts with an existing active key, disabling that exact key while adding a replacement succeeds, disabled historical keys do not block replacement, and scan/spend purposes remain independent. A pipeline-level test should also pin rejection before protocol version 14 and acceptance at version 14.
In `packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/identity/IdentityUpdates.kt`:
- [SUGGESTION] packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/identity/IdentityUpdates.kt:27-35: Expose the new DPP key purposes in Kotlin
The generic Kotlin identity registration and update codecs serialize KeyPurpose.ffiValue, but this mirror stops at OWNER=6. Kotlin callers therefore cannot construct PAYMENT_SCAN=7 or PAYMENT_SPEND=8 rows even though the Rust decoder now supports them. Add both enum cases and update the example application's exhaustive when expressions and purpose-selection logic so Android callers can use the protocol-version-14 feature.
In `packages/rs-platform-wallet/src/wallet/identity/crypto/stealth.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/identity/crypto/stealth.rs:171-229: Preserve retryable stealth crypto errors
The lower crypto layer exposes ZeroStealthTweak as a recoverable condition that requires the payer to choose a fresh ephemeral key, but every stealth operation converts CryptoError into InvalidIdentityData(String). Callers cannot distinguish the retry condition without parsing display text. Add a typed PlatformWalletError source variant for platform_encryption::CryptoError, or a dedicated zero-tweak variant, and propagate it with ?. Errors constructing or deriving the DIP-9 path should likewise use the existing KeyDerivation category rather than InvalidIdentityData.
| #[error(transparent)] | ||
| InvalidKeyPurposeKeyTypeError(InvalidKeyPurposeKeyTypeError), | ||
|
|
||
| #[error(transparent)] | ||
| TooManyPublicKeysOfPurposeError(TooManyPublicKeysOfPurposeError), |
There was a problem hiding this comment.
🔴 Blocking: Append the new BasicError variants to preserve wire discriminants
BasicError derives bincode Encode/Decode and PlatformSerialize/PlatformDeserialize, so its enum discriminants are positional. Inserting these variants before StateTransitionNotActiveError shifts every existing variant through TokenPricingScheduleEmptyError, causing previously encoded errors to decode as the wrong variant or fail. This directly violates the ordering invariant documented at the enum tail. Move both new variants after TokenPricingScheduleEmptyError without changing the order of any existing variant, and add a compatibility test that pins a pre-existing variant from after the current insertion point.
source: ['codex']
| if let Some(invalid_type_key) = identity_public_keys_with_witness.iter().find(|key| { | ||
| Purpose::payment_purposes().contains(&key.purpose()) | ||
| && key.key_type() != KeyType::ECDSA_SECP256K1 | ||
| }) { | ||
| return Ok(SimpleConsensusValidationResult::new_with_error( | ||
| BasicError::InvalidKeyPurposeKeyTypeError(InvalidKeyPurposeKeyTypeError::new( | ||
| invalid_type_key.id(), | ||
| invalid_type_key.purpose(), | ||
| invalid_type_key.key_type(), | ||
| vec![KeyType::ECDSA_SECP256K1], | ||
| )) | ||
| .into(), | ||
| )); | ||
| } |
There was a problem hiding this comment.
🔴 Blocking: Require compressed 33-byte encodings for payment keys
This check constrains payment keys to ECDSA_SECP256K1 but does not constrain their encoding. DPP's ECDSA public-key parser explicitly accepts both 33-byte compressed and 65-byte uncompressed SEC1 keys, and the generic added-key signature verifier passes either representation to PublicKey::from_slice. A signed transition can therefore register a 65-byte PAYMENT_SCAN or PAYMENT_SPEND key even though DIP-33 specifies compressed SEC1 points and this validator's own comment requires the full compressed key. Besides producing noncanonical persisted state, compressed and uncompressed encodings of the same point also evade the in-transition duplicate check, which compares raw bytes. Reject payment-purpose keys unless data().len() is exactly 33 and add a regression test using a valid 65-byte key.
source: ['codex']
| "paymentscan" => Ok(PurposeWasm::PAYMENT_SCAN), | ||
| "paymentspend" => Ok(PurposeWasm::PAYMENT_SPEND), |
There was a problem hiding this comment.
🟡 Suggestion: Accept the payment-purpose strings emitted by the WASM getter
The IdentityPublicKey purpose getter emits PAYMENT_SCAN and PAYMENT_SPEND. The setter lowercases those strings to payment_scan and payment_spend, but this parser accepts only paymentscan and paymentspend. Consequently, assigning key.purpose = key.purpose or copying a key through its exposed properties fails only for the new variants. Accept the underscore forms as aliases so getter values round-trip through constructors and setters.
| "paymentscan" => Ok(PurposeWasm::PAYMENT_SCAN), | |
| "paymentspend" => Ok(PurposeWasm::PAYMENT_SPEND), | |
| "paymentscan" | "payment_scan" => Ok(PurposeWasm::PAYMENT_SCAN), | |
| "paymentspend" | "payment_spend" => Ok(PurposeWasm::PAYMENT_SPEND), |
source: ['codex']
| pub(super) fn validate_payment_key_uniqueness_in_state_v0( | ||
| identity_id: Identifier, | ||
| public_keys_being_added: &[IdentityPublicKeyInCreation], | ||
| public_key_ids_to_disable: &[KeyID], | ||
| drive: &Drive, | ||
| _execution_context: &mut StateTransitionExecutionContext, | ||
| transaction: TransactionArg, | ||
| platform_version: &PlatformVersion, | ||
| ) -> Result<SimpleConsensusValidationResult, Error> { | ||
| let payment_purposes_being_added: Vec<Purpose> = Purpose::payment_purposes() | ||
| .into_iter() | ||
| .filter(|purpose| { | ||
| public_keys_being_added | ||
| .iter() | ||
| .any(|key| key.purpose() == *purpose) | ||
| }) | ||
| .collect(); | ||
|
|
||
| if payment_purposes_being_added.is_empty() { | ||
| return Ok(SimpleConsensusValidationResult::new()); | ||
| } | ||
|
|
||
| let identity_key_request = IdentityKeysRequest { | ||
| identity_id: identity_id.to_buffer(), | ||
| request_type: KeyRequestType::AllKeys, | ||
| limit: None, | ||
| offset: None, | ||
| }; | ||
| let existing_keys = drive.fetch_identity_keys::<KeyIDIdentityPublicKeyPairBTreeMap>( | ||
| identity_key_request, | ||
| transaction, | ||
| platform_version, | ||
| )?; | ||
|
|
||
| for purpose in payment_purposes_being_added { | ||
| let conflicting_active_key_exists = existing_keys.values().any(|key| { | ||
| key.purpose() == purpose | ||
| && key.disabled_at().is_none() | ||
| && !public_key_ids_to_disable.contains(&key.id()) | ||
| }); | ||
| if conflicting_active_key_exists { | ||
| return Ok(SimpleConsensusValidationResult::new_with_error( | ||
| TooManyPublicKeysOfPurposeError::new(purpose, 1).into(), | ||
| )); | ||
| } | ||
| } | ||
|
|
||
| Ok(SimpleConsensusValidationResult::new()) |
There was a problem hiding this comment.
🟡 Suggestion: Cover payment-key uniqueness against persisted identity state
This helper implements the consensus-critical against-state half of the one-active-key invariant, including the state-sensitive rotation exception, but rs-drive-abci contains no PAYMENT_SCAN or PAYMENT_SPEND tests. Add tests proving that an add-only update conflicts with an existing active key, disabling that exact key while adding a replacement succeeds, disabled historical keys do not block replacement, and scan/spend purposes remain independent. A pipeline-level test should also pin rejection before protocol version 14 and acceptance at version 14.
source: ['codex']
| 4 => Ok(PurposeWasm::SYSTEM), | ||
| 5 => Ok(PurposeWasm::VOTING), | ||
| 6 => Ok(PurposeWasm::OWNER), | ||
| 7 => Ok(PurposeWasm::PAYMENT_SCAN), | ||
| 8 => Ok(PurposeWasm::PAYMENT_SPEND), |
There was a problem hiding this comment.
🟡 Suggestion: Reject fractional numeric payment-purpose values
PurposeLike's TypeScript union is not a runtime constraint. Casting an arbitrary JavaScript number directly to u8 truncates fractional values, so 7.5 is accepted as PAYMENT_SCAN and 8.5 as PAYMENT_SPEND. Validate that the number is finite, integral, and within the supported 0 through 8 range before casting. Rust's float-to-integer cast saturates rather than wraps, so positive overflow currently becomes 255 and is rejected; the newly introduced defect is specifically the acceptance of fractional values that truncate to 7 or 8.
source: ['coderabbit']
| let shared = stealth_shared_point(&secp, ephemeral_secret, scan_public) | ||
| .map_err(|e| PlatformWalletError::InvalidIdentityData(e.to_string()))?; | ||
| let one_time_public = one_time_public_key( | ||
| &secp, | ||
| spend_public, | ||
| &shared, | ||
| &ephemeral_public, | ||
| rail.into(), | ||
| n, | ||
| ) | ||
| .map_err(|e| PlatformWalletError::InvalidIdentityData(e.to_string()))?; | ||
| Ok(( | ||
| destination_from_public_key(&one_time_public, rail, network), | ||
| ephemeral_public, | ||
| )) | ||
| } | ||
|
|
||
| /// Receiver / watch service: re-derive the one-time destination for output `n` | ||
| /// given the payer's published `R`, the recipient's scan secret, and the | ||
| /// recipient's spend public key. Compare the result against the referenced | ||
| /// on-chain output. Requires no spend secret. | ||
| pub fn recognize_one_time_destination( | ||
| scan_secret: &SecretKey, | ||
| spend_public: &PublicKey, | ||
| ephemeral_public: &PublicKey, | ||
| rail: PaymentRail, | ||
| n: u32, | ||
| network: Network, | ||
| ) -> Result<OneTimeDestination, PlatformWalletError> { | ||
| let secp = Secp256k1::new(); | ||
| let shared = stealth_shared_point(&secp, scan_secret, ephemeral_public) | ||
| .map_err(|e| PlatformWalletError::InvalidIdentityData(e.to_string()))?; | ||
| let one_time_public = one_time_public_key( | ||
| &secp, | ||
| spend_public, | ||
| &shared, | ||
| ephemeral_public, | ||
| rail.into(), | ||
| n, | ||
| ) | ||
| .map_err(|e| PlatformWalletError::InvalidIdentityData(e.to_string()))?; | ||
| Ok(destination_from_public_key(&one_time_public, rail, network)) | ||
| } | ||
|
|
||
| /// Spender: derive the one-time secret key controlling output `n`, given the | ||
| /// payer's published `R`, the recipient's scan secret, and the recipient's | ||
| /// spend secret. | ||
| pub fn derive_one_time_secret_key( | ||
| scan_secret: &SecretKey, | ||
| spend_secret: &SecretKey, | ||
| ephemeral_public: &PublicKey, | ||
| rail: PaymentRail, | ||
| n: u32, | ||
| ) -> Result<SecretKey, PlatformWalletError> { | ||
| let secp = Secp256k1::new(); | ||
| let shared = stealth_shared_point(&secp, scan_secret, ephemeral_public) | ||
| .map_err(|e| PlatformWalletError::InvalidIdentityData(e.to_string()))?; | ||
| one_time_secret_key(spend_secret, &shared, ephemeral_public, rail.into(), n) | ||
| .map_err(|e| PlatformWalletError::InvalidIdentityData(e.to_string())) |
There was a problem hiding this comment.
🟡 Suggestion: Preserve retryable stealth crypto errors
The lower crypto layer exposes ZeroStealthTweak as a recoverable condition that requires the payer to choose a fresh ephemeral key, but every stealth operation converts CryptoError into InvalidIdentityData(String). Callers cannot distinguish the retry condition without parsing display text. Add a typed PlatformWalletError source variant for platform_encryption::CryptoError, or a dedicated zero-tweak variant, and propagate it with ?. Errors constructing or deriving the DIP-9 path should likewise use the existing KeyDerivation category rather than InvalidIdentityData.
source: ['codex']
2 participants
Implements the transparent-rail (Core chain + Platform address) half of DIP-33. The shielded-pool tier and the
paymentNotificationprivate document type are deliberately out of scope here and will follow once the private-document-store infrastructure lands.What's in this PR
DashPay contract v2 (protocol version 14)
profilegains two optional fields:corePaymentAddress(Base58Check string, position 5) andplatformPaymentAddress(21-byte DIP-18 storage form, position 6). Both are static, deliberately public addresses (the "tips jar" tier).schema/v2, a thinv2loader, a newsystem_data_contract_versionsv3 (dashpay: 2),v14.rsrepointed, and atransition_to_version_14first-block migration that re-applies the DashPay contract.SystemDataContract::ALLcache refresh, so it inherits the monotonic data-contract-cache insert guard that protects the DPNS v13 activation against the stale-lower-version race — no new cache code, no reopened race at the PV14 activation height.Identity key purposes
PAYMENT_SCAN = 7/PAYMENT_SPEND = 8ECDSA_SECP256K1only, non-signing, no contract bounds, at most one active key of each purpose per identity.validate_identity_public_keys_structurev1 (in-transition: security level, ECDSA-only, at-most-one-of-each), plus avalidate_payment_key_uniquenesscommon state check wired into the identity-update add path (against-state half, runs even for add-only updates).InvalidKeyPurposeKeyTypeError(10535) andTooManyPublicKeysOfPurposeError(10536), mirrored inwasm-dpp/wasm-dpp2.Stealth one-time address derivation
platform-encryption::stealth— pure secp256k1 curve math (DH shared point, rail-domain-separated one-time tweakt_n, one-time public/secret keys) with DIP-33 known-answer test vectors.platform-walletstealth module — DIP-9 feature33'key derivation (m/9'/coin'/33'/account'/key_class'/index', scan/spend/notif-out classes) and one-time destination construction for both Core P2PKH and Platform addresses (payer, receiver/watch-only, spender).Testing
transition_to_version_14migration test.cargo check --workspaceclean;cargo clippyclean on the touched crates; targeted test suites green.Notes / follow-ups
!): new consensus-validated key purposes + system contract version, PV14-gated.dapi-grpc's hand-maintainedKeyPurposeproto enum is already out of sync with the Rust enum (missingSYSTEM/OWNER); left as-is since payment keys aren't queried by purpose. Worth a separate sync PR.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Platform Updates