DIP: Dash Platform Application Key Exchange and State Transition Signing#181
DIP: Dash Platform Application Key Exchange and State Transition Signing#181PastaPastaPasta wants to merge 1 commit intodashpay:masterfrom
Conversation
…ansition Signing Defines the dash-key: and dash-st: URI protocols for wallet-based QR code login to Dash Platform applications. Covers ECDH key exchange, AES-256-GCM encryption, BIP32+HKDF key derivation, the loginKeyResponse contract schema, and first-time key registration flow.
📝 WalkthroughWalkthroughIntroduces a new DIP document specifying two complementary URI-based protocols (dash-key and dash-st) for secure wallet-app interaction, including protocol architecture, URI formats, cryptographic flows, data contract schemas, security considerations, and validation requirements. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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: 1
🧹 Nitpick comments (2)
dip-pasta-yappr.md (2)
262-265: Consider clarifying concurrent request handling.The keyIndex logic states that if no document exists,
keyIndexis0. If a wallet processes multiple key exchange requests for the same identity and contract concurrently (before the first document is created), both could usekeyIndex0, potentially creating a race condition. Consider adding guidance on how wallets should handle this scenario (e.g., serialize requests per identity+contract or retry on conflict).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dip-pasta-yappr.md` around lines 262 - 265, Clarify concurrent-request behavior for keyIndex/loginKeyResponse: explain that when no loginKeyResponse exists wallets must handle races by serializing requests per identity+contract or using a create-if-not-exists atomic operation and retry-on-conflict; reference the keyIndex and loginKeyResponse documents and state a recommended strategy (e.g., queue/serialize per identity+contract, use optimistic concurrency/version checks on loginKeyResponse, or retry on conflict) so two concurrent requests cannot both assume keyIndex = 0 and create duplicate keys.
391-401: Consider exponential backoff for revision conflict retries.The document lifecycle specifies retrying up to 3 times with a fixed 500ms delay on revision conflicts. Consider recommending exponential backoff (e.g., 500ms, 1000ms, 2000ms) to reduce contention under high load while maintaining the same maximum retry count.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dip-pasta-yappr.md` around lines 391 - 401, The retry logic for handling revision conflicts when updating a loginKeyResponse document should use exponential backoff instead of a fixed 500ms delay: update the retry loop that currently retries up to 3 times with a fixed 500ms pause to instead wait 500ms, then 1000ms, then 2000ms between attempts (keeping the same max retry count), and continue to re-query the loginKeyResponse document on each retry and preserve the existing update/create flow and revision bump behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@dip-pasta-yappr.md`:
- Line 36: The Table of Contents link fragment "#supported-encodings" is broken
because the actual header is "## Encoding"; update either the TOC entry or the
heading so they match—e.g., change the TOC item "* [Supported
Encodings](`#supported-encodings`)" to "* [Encoding](`#encoding`)" or rename the
header "## Encoding" to "## Supported Encodings" so the fragment and header text
are consistent.
---
Nitpick comments:
In `@dip-pasta-yappr.md`:
- Around line 262-265: Clarify concurrent-request behavior for
keyIndex/loginKeyResponse: explain that when no loginKeyResponse exists wallets
must handle races by serializing requests per identity+contract or using a
create-if-not-exists atomic operation and retry-on-conflict; reference the
keyIndex and loginKeyResponse documents and state a recommended strategy (e.g.,
queue/serialize per identity+contract, use optimistic concurrency/version checks
on loginKeyResponse, or retry on conflict) so two concurrent requests cannot
both assume keyIndex = 0 and create duplicate keys.
- Around line 391-401: The retry logic for handling revision conflicts when
updating a loginKeyResponse document should use exponential backoff instead of a
fixed 500ms delay: update the retry loop that currently retries up to 3 times
with a fixed 500ms pause to instead wait 500ms, then 1000ms, then 2000ms between
attempts (keeping the same max retry count), and continue to re-query the
loginKeyResponse document on each retry and preserve the existing update/create
flow and revision bump behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6edfd7c4-83bf-45c9-b579-c785373b1631
📒 Files selected for processing (1)
dip-pasta-yappr.md
| 1. [State Transition Signing Protocol](#state-transition-signing-protocol) | ||
| * [dash-st: URI Format](#dash-st-uri-format) | ||
| * [Query Parameters](#query-parameters) | ||
| * [Supported Encodings](#supported-encodings) |
There was a problem hiding this comment.
Fix the broken link fragment in the Table of Contents.
The ToC entry references #supported-encodings but the actual section heading at line 434 is ## Encoding (singular). This causes a broken link and is flagged by markdownlint (MD051).
🔗 Proposed fix
- * [Supported Encodings](`#supported-encodings`)
+ * [Encoding](`#encoding`)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| * [Supported Encodings](#supported-encodings) | |
| * [Encoding](`#encoding`) |
🧰 Tools
🪛 GitHub Actions: .github/workflows/markdownlint.yml
[error] 36-36: markdownlint: MD051 link-fragments should be valid for link Supported Encodings in dip-pasta-yappr.md.
🪛 GitHub Check: lint
[failure] 36-36: Link fragments should be valid
dip-pasta-yappr.md:36:7 MD051/link-fragments Link fragments should be valid [Context: "Supported Encodings"] https://github.com/DavidAnson/markdownlint/blob/v0.38.0/doc/md051.md
🪛 markdownlint-cli2 (0.21.0)
[warning] 36-36: Link fragments should be valid
(MD051, link-fragments)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@dip-pasta-yappr.md` at line 36, The Table of Contents link fragment
"#supported-encodings" is broken because the actual header is "## Encoding";
update either the TOC entry or the heading so they match—e.g., change the TOC
item "* [Supported Encodings](`#supported-encodings`)" to "*
[Encoding](`#encoding`)" or rename the header "## Encoding" to "## Supported
Encodings" so the fragment and header text are consistent.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9482c77c3f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| | Path Level | Value | Description | | ||
| | ---------- | ----- | ----------- | | ||
| | 9' | Feature purpose (per [DIP-0009](https://github.com/dashpay/dips/blob/master/dip-0009.md)) | | | ||
| | coin_type' | `5` (mainnet) or `1` (testnet/devnet/regtest) | Per [BIP-0044](https://github.com/bitcoin/bips/blob/master/bip-0044.mediawiki) | |
There was a problem hiding this comment.
Using the same coin_type' = 1 for testnet/devnet/regtest means the derivation inputs are identical across those networks unless contractId or identity differs, so the same wallet can produce the same login key material in multiple non-mainnet environments; this weakens the stated network isolation and creates cross-environment key reuse risk if one environment is less trusted. To preserve isolation, include an explicit network discriminator in HKDF input (or use distinct derivation branches per non-mainnet network).
Useful? React with 👍 / 👎.
| | `d`, `devnet` | Devnet | | ||
| | `r`, `regtest` | Regtest (local) | | ||
|
|
||
| ## Binary Payload Layout |
There was a problem hiding this comment.
Seems like this is a subsection? If so, I'd also move it above Network identifiers to match the order from the component table.
| ## Binary Payload Layout | |
| ### Binary Payload Layout |
| | Path Level | Value | Description | | ||
| | ---------- | ----- | ----------- | | ||
| | 9' | Feature purpose (per [DIP-0009](https://github.com/dashpay/dips/blob/master/dip-0009.md)) | | | ||
| | coin_type' | `5` (mainnet) or `1` (testnet/devnet/regtest) | Per [BIP-0044](https://github.com/bitcoin/bips/blob/master/bip-0044.mediawiki) | | ||
| | 21' | Key exchange feature index | | | ||
| | account' | Wallet account index for the selected identity (default `0`) | | |
There was a problem hiding this comment.
This table is weird. Seems unnecessary to have the Description column that only contains values for a single row. You could collapse that into the Value column and drop the extra column or smth
| | ---------- | ----- | ----------- | | ||
| | 9' | Feature purpose (per [DIP-0009](https://github.com/dashpay/dips/blob/master/dip-0009.md)) | | | ||
| | coin_type' | `5` (mainnet) or `1` (testnet/devnet/regtest) | Per [BIP-0044](https://github.com/bitcoin/bips/blob/master/bip-0044.mediawiki) | | ||
| | 21' | Key exchange feature index | | |
| | --------- | ----------- | | ||
| | `dash-st:` | URI scheme prefix | | ||
| | `<encoded_transition>` | Encoded unsigned state transition bytes | | ||
| | `n` | Network identifier (required; same values as `dash-key:`) | |
| | --------- | -------- | ----------- | | ||
| | `n` | Yes | Network identifier (`m`, `t`, `d`, or `r`) | | ||
| | `v` | Yes | Protocol version; must be `1` | | ||
| | `id` | No | Base58-encoded identity identifier hint (32 bytes); wallet should pre-select this identity | |
There was a problem hiding this comment.
Extraneous word?
| | `id` | No | Base58-encoded identity identifier hint (32 bytes); wallet should pre-select this identity | | |
| | `id` | No | Base58-encoded identity identifier (32 bytes); wallet should pre-select this identity | |
| | Security Level | HIGH | | ||
| | Key Type | ECDSA_HASH160 | | ||
| | Data | Hash160 of the authentication public key (20 bytes) | | ||
| | Read Only | false | |
There was a problem hiding this comment.
Is this really something that needs to be specified?
| | contractId | Identifier | 32 | The target application's data contract identifier | | ||
| | appEphemeralPubKeyHash | Bytes | 20 | Hash160 of the application's ephemeral public key | | ||
| | walletEphemeralPubKey | Bytes | 33 | Wallet's compressed secp256k1 ephemeral public key | | ||
| | encryptedPayload | Bytes | 1–4096 | Encrypted login key (nonce ‖ ciphertext ‖ tag; typically 60 bytes) | |
There was a problem hiding this comment.
Why such a large max if 60 is typical?
2 participants
Summary
dash-key:URI protocol for wallet-based QR code login to Dash Platform applications using ECDH key exchange and AES-256-GCM encryptiondash-st:URI protocol for requesting wallet signature and broadcast of unsigned state transitionsm/9'/coin_type'/21'/account'), HKDF key derivation, ECDH shared secret, and per-contract/per-identity key isolationloginKeyResponsePlatform contract schema, indices, and polling mechanismIdentityUpdateTransitionwith ECDSA_HASH160 keysTest plan
🤖 Generated with Claude Code
Summary by CodeRabbit