docs: add DIP-33 (DashPay payment addresses and payment notifications) - #188
Open
QuantumExplorer wants to merge 3 commits into
Open
docs: add DIP-33 (DashPay payment addresses and payment notifications)#188QuantumExplorer wants to merge 3 commits into
QuantumExplorer wants to merge 3 commits into
Conversation
Two-tier payment addressing for DashPay across the Core chain and Platform payment address (DIP-17/18) rails: * Public tier: optional corePaymentAddress / platformPaymentAddress fields on the profile document (static, deliberately linkable). * Notified tier: PAYMENT_SCAN / PAYMENT_SPEND identity key purposes, DIP-9 feature 33' derivation, stealth one-time address derivation shared by both rails, and encrypted payment notification documents with sender recovery and an anonymous-sender variant. Reserves the shielded-pool tier and a silent-payments-style tweak index as future extensions. Includes derivation test vectors. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
📝 WalkthroughWalkthroughDIP-0033 documents DashPay payment addresses and encrypted payment notifications across Core and Platform. It defines key derivation, notification formats, settlement ordering, client recovery, test vectors, and DIP-9 registration. Repository references and spell-check entries are also updated. ChangesDashPay payment notifications
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Sender
participant Platform
participant Recipient
participant Core
Sender->>Platform: Publish encrypted paymentNotification
Platform-->>Recipient: Expose finalized notification
Recipient->>Recipient: Decrypt and verify notification
Sender->>Core: Broadcast settlement
Core-->>Recipient: Provide settlement data
Recipient->>Recipient: Verify one-time destination
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 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 `@dip-0033.md`:
- Around line 194-201: Bind notifications to the exact settled amount and
transition: require the Core output amount to equal the signed amount, and add a
Platform transition locator plus proof binding the credited amount rather than
relying on aggregate balance/nonce state. Update notification deduplication and
restore logic to use this locator, including the corresponding flows near the
Platform verification and restore sections.
- Around line 127-139: Update the DIP-11 identity public-key schema and related
validation to accept purpose values 7 and 8 for PAYMENT_SCAN and PAYMENT_SPEND,
while preserving their required non-signing, no-contract-bounds, and
single-active-key constraints. Activate the updated schema and DIP-0033 under
the same protocol-version rule so validators accept these purposes only when
that version is enabled.
- Around line 288-297: Revise the Anonymous variant and Funding modes text in
dip-0033.md to distinguish inner-payload anonymity from envelope-level creator
attribution: identity-funded notifications must still be rendered as revealing
their document creator, while pool-funded anonymous notifications can omit payer
identity entirely. Update the client-rendering, abstract, and privacy statements
accordingly, and remove or qualify the claim that attribution always comes from
the inner signature.
- Around line 183-186: Update the scalar derivation and serialization rules
around t_n, esk, r, and ovk to make them normative: interpret each hash digest
as a big-endian integer, encode k_out as a fixed 32-byte scalar before
concatenation, and require regenerating eskSeed whenever esk == 0 or r == 0.
Apply the same clarification to the corresponding derivations in the referenced
sections so all implementations produce identical values.
- Around line 362-375: Update pay_notified to reject rail values other than 0
and 2 before deriving the stealth destination, so public profile rails 1 and 3
cannot enter this flow. Preserve the existing stealth derivation for supported
rails and require a separate public-payment path for public rails.
- Around line 139-162: The periodic notification scan must retain disabled
historical PAYMENT_SCAN keys and rescan each one once, not only the currently
active registered scan key. Update the periodic scan and key-rotation handling
to include disabled scan-key versions alongside the active key, while preserving
the existing restore behavior that replays the notification store from position
zero for every derived key.
- Around line 249-256: Update the signature verification requirements around
ECDSA_sign_recoverable so recipients resolve senderKeyIndex against the sender’s
authentication-capable key state at notification publication or finality, retain
proof of that historical key, and reject or anonymize notifications lacking
valid historical verification. Extend the signed preimage to include
senderIdentityId and senderKeyIndex alongside the existing fields, and require
both values during verification.
- Around line 207-211: Extend the paymentNotification document-type section to
define bounded read and admission semantics: specify how recipients are
partitioned or how private range reads are scoped so clients do not scan all
recipients, and define quotas or rate limits for immutable entries to bound
light-client trial-decryption work and append-only storage growth.
🪄 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: 945aef72-cdea-4a28-b096-92c1c2ffed18
📒 Files selected for processing (3)
README.mddip-0033.mdproject-words.txt
…g, rotation scanning * Define int_be / ser_scalar and zero-scalar regeneration normatively * Bind sig preimage to senderIdentityId + senderKeyIndex; verify against key state at notification creation (disabled-after counts as valid) * Settled value is authoritative; signed amount must match for verified attribution; rail 2 gains an optional stHash settlement locator * Anonymous variant must be pool-funded; clarify inner vs envelope attribution * Scan-key rotation: retain disabled keys, grace-period dual scanning * Document store admission economics and deliberate non-partitioning * Guard pay_notified pseudo-code to notified rails only * Couple the DIP-11 purpose registry update to the same activation rule Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Member
Author
|
Addressed the review in 0fdee78:
|
The character-class pattern could not prove the checksum or network byte, so clients had to fully validate the address regardless. Enforcing it on chain only bought a partial check while making every optional-field document generator produce schema-invalid profiles. Consensus now constrains the length only; full validation is explicitly a client responsibility. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@dip-0033.md`:
- Line 118: Update the Markdown wording in the corePaymentAddress description to
use the hyphenated “on-chain” form instead of “on chain,” without changing the
surrounding validation requirements.
- Line 261: Update the notification verification requirements around
senderIdentityId, senderKeyIndex, and sig so any payload with claimed sender
fields whose signature fails verification is discarded, rather than downgraded
to anonymous. Preserve the anonymous variant only when all three sender fields
are zero, and retain the existing authentication-capability and key-validity
checks for attributed notifications.
- Around line 247-248: Update the rail-2 deduplication rule to treat an all-zero
stHash as absent rather than as a matching identifier. Deduplicate
locator-bearing notifications using the derived address and outputCounter, and
use non-zero stHash only as an additional identifier; preserve separate entries
for unrelated or multiple outputs that lack a usable locator.
🪄 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: 8b990fb3-c6d5-440b-ae3a-d3112cae4cda
📒 Files selected for processing (1)
dip-0033.md
|
|
||
| Normative requirements: | ||
|
|
||
| * `corePaymentAddress` MUST be a valid Base58Check Core chain address for the network the contract is deployed on. Consensus constrains only the string length: a Base58 character-class pattern would still not prove the checksum or network byte, so validation of the address proper is a client responsibility and is not duplicated on chain. Clients MUST validate the checksum and network byte before paying, and MUST treat an undecodable value as no address rather than as an error in the profile. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Use on-chain in the Markdown text.
Replace on chain with on-chain to resolve the reported documentation lint warning.
🧰 Tools
🪛 LanguageTool
[grammar] ~118-~118: Use a hyphen to join words.
Context: ... responsibility and is not duplicated on chain. Clients MUST validate the checksu...
(QB_NEW_EN_HYPHEN)
🤖 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 `@dip-0033.md` at line 118, Update the Markdown wording in the
corePaymentAddress description to use the hyphenated “on-chain” form instead of
“on chain,” without changing the surrounding validation requirements.
Source: Linters/SAST tools
Comment on lines
+247
to
+248
| | `2` | Platform, one-time address | `R[33] \|\| outputCounter[4] \|\| amount[8] \|\| fundingKind[1] \|\| stHash[32]` — amount in credits; `fundingKind`: `0` = address funds transfer, `1` = unshielding. The derived address is the primary locator; `stHash` (all-zeros when absent) SHOULD name the crediting state transition to bind the exact settlement | | ||
| | `3` | Platform, public profile address | `platformAddress[21] \|\| amount[8] \|\| stHash[32]` — amount in credits; `stHash` identifies the crediting state transition | |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Exclude an absent stHash from deduplication.
Rail 2 uses an all-zero stHash when no locator exists. The deduplication rule at Line 318 treats equal stHash values as duplicates. That can merge unrelated rail-2 notifications without locators. It can also merge multiple outputs from one transition. Treat zero as absent and deduplicate by derived address plus outputCounter; use stHash only when non-zero.
🤖 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 `@dip-0033.md` around lines 247 - 248, Update the rail-2 deduplication rule to
treat an all-zero stHash as absent rather than as a matching identifier.
Deduplicate locator-bearing notifications using the derived address and
outputCounter, and use non-zero stHash only as an additional identifier;
preserve separate entries for unrelated or multiple outputs that lack a usable
locator.
|
|
||
| Including `senderIdentityId` and `senderKeyIndex` in the preimage binds the signature to the claimed identity: a signature cannot be re-attributed to another identity that happens to have registered the same public key. | ||
|
|
||
| Recipients MUST verify `sig` against the key at `senderKeyIndex` of `senderIdentityId` (fetched with proof). The key MUST be authentication-capable and MUST have been valid when the notification was created: either currently active, or disabled with a disabling time later than the entry's creation time (identity keys are disabled, never removed, so current identity state suffices for this check — sender key rotation does not retroactively invalidate attribution). Notifications whose signature does not verify MUST be treated as anonymous at best and SHOULD be discarded. |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Reject malformed attributed notifications instead of downgrading them to anonymous.
The anonymous variant requires all-zero senderIdentityId, senderKeyIndex, and sig. If a payload has claimed sender fields but its signature fails, clients MUST discard it. Otherwise, an attacker can attach an arbitrary memo to a real settlement and have clients display it as an anonymous payment.
🤖 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 `@dip-0033.md` at line 261, Update the notification verification requirements
around senderIdentityId, senderKeyIndex, and sig so any payload with claimed
sender fields whose signature fails verification is discarded, rather than
downgraded to anonymous. Preserve the anonymous variant only when all three
sender fields are zero, and retain the existing authentication-capability and
key-validity checks for attributed notifications.
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
1 participant
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.
Summary
Adds DIP 0033: DashPay Payment Addresses and Payment Notifications — two-tier payment addressing for DashPay across the Core chain and Platform payment address (DIP-17/DIP-18) rails.
Public tier — optional static address fields on the DashPay profile document:
corePaymentAddress(Base58Check string, position 5)platformPaymentAddress(21-byte DIP-18 storage form, position 6)Payments to these are publicly linkable to the profile by design (the "tips jar"). Extended public keys are deliberately rejected: a world-readable derivation source is exactly as linkable as one static address while adding gap-limit and index-coordination costs.
Notified tier — private payments without a prior contact relationship:
PAYMENT_SCAN(7) andPAYMENT_SPEND(8): non-signing, no contract bounds, at most one active each. Registry placement puts key changes behind master-key authorization and makes any identity payable without a DashPay profile.33'derivation:m/9'/coin'/33'/account'/key_class'/index'(scan / spend / notification out-key classes).Includes rationale for rejecting BIP-47-style notification transactions and BIP-352-style scanning (input public keys are absent from compact block filters, and the sought output script is uncomputable without per-transaction ECDH — so scanning degenerates to downloading every transaction forever), self-verified secp256k1 test vectors, and reservations for a shielded-pool tier and a future tweak-index scanning mode.
Also in this PR: README table row, a few dictionary words, and a fix for the one pre-existing markdownlint error on the README (
[here]link text) so CI passes on changed files.Note: DIP number 32 is intentionally skipped to avoid confusion with BIP-32, since both concern key derivation.
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation