-
Notifications
You must be signed in to change notification settings - Fork 58
chore: dip-17/18 suggestions #172
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Add Prior Work sections referencing DIP-0009, BIP-0032, and BIP-0044 in DIP-17 and DIP-0017 in DIP-18. Convert path examples to table format, add key_class forward compatibility note, and fix minor formatting.
WalkthroughDocumentation updates adding DIP-0017 and DIP-0018 entries and refining both proposals: added Prior Work sections, converted list formatting, standardized references (cross-linking to DIP-9, DIP-17, DIP-18), and made derivation-path syntax explicit (hardened account' and key_class' with non-hardened index) plus related reference-implementation and test-vector wording edits. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10–15 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 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 review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (2)
dip-0017.md (2)
53-53: Simplify redundant phrase.Line 53 contains the phrase "hold value outside of any identity," which is slightly redundant. Simplifying to "hold value outside any identity" improves conciseness without losing meaning.
-They function as standalone payment keys that hold value outside of any identity. They use a single secp256k1 key pair and are not script-based. +They function as standalone payment keys that hold value outside any identity. They use a single secp256k1 key pair and are not script-based.
65-71: Consider rewording repetitive normative phrasing for style.Lines 65–71 contain multiple successive bullets beginning with "*
...MUST", which LanguageTool flags as repetitive sentence structure. While this format is intentional for normative clarity, minor rewording could improve readability without reducing normative weight. For example, combine closely related requirements or vary the phrasing slightly (e.g., "Thekey_class'MUST be hardened and SHOULD default to0'..." instead of repeating "*key_class'MUST..." patterns).This is a low-priority style improvement suitable for future polish and is not critical to clarity or correctness.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
README.md(1 hunks)dip-0017.md(5 hunks)dip-0018.md(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- dip-0018.md
🧰 Additional context used
🪛 GitHub Actions: .github/workflows/markdownlint.yml
README.md
[error] 5-5: MD059/descriptive-link-text Link text should be descriptive [Context: "[here]"]
🪛 LanguageTool
dip-0017.md
[style] ~53-~53: This phrase is redundant. Consider using “outside”.
Context: ...standalone payment keys that hold value outside of any identity. They use a single secp256...
(OUTSIDE_OF)
[style] ~67-~67: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...t with BIP-44 conventions. * feature' MUST be 17' (Platform payment feature). * ...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~68-~68: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...Platform payment feature). * account' MUST be hardened. 0' is the default accoun...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~89-~89: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...t of 20 is RECOMMENDED for discovery. * Wallets MAY support watch-only by exporting the...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~90-~90: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...account'/key_class'` (test networks). * Wallets MAY present multiple accounts following...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
🔇 Additional comments (9)
README.md (1)
36-37: DIP entries properly integrated into table.The new DIP-0017 and DIP-0018 entries are correctly sequenced, well-formatted, and follow existing table conventions. Link text is descriptive and metadata (layer, type, status, author) is accurate.
dip-0017.md (8)
35-39: Improved cross-linking and clarity in Abstract and Motivation.The additions of DIP-9, DIP-13, DIP-15, and DIP-18 cross-links contextualize the Platform payment specification within the broader DIP ecosystem and enhance clarity for readers.
41-47: Prior Work section well-researched.The new Prior Work section appropriately references foundational specifications (DIP-9, DIP-13, DIP-15, BIP-32, BIP-44), helping readers understand the lineage and design decisions behind the Platform payment derivation paths.
57-80: Derivation path definition is explicit and comprehensive.The canonical path specification, normative requirements, default account table, and watch-only xpub guidance are clear and provide actionable direction for wallet implementers. The hardened/unhardened structure is well-reasoned and properly explained.
86-93: Wallet and hardware wallet behavior guidance is thorough.Clear, specific requirements for derivation paths, address separation, address rotation, xpub export, multi-account support, and hardware wallet labeling will aid implementation consistency across wallets and hardware devices.
144-156: Security, Privacy, and Rationale sections are well-reformatted.The shift to bullet-driven presentation in the Security Considerations, Privacy Considerations, and Rationale sections improves scannability and aligns formatting with the normative requirements elsewhere in the DIP. The explicit xpub boundary clarity (line 145) and practical privacy guidance (lines 153–155) are valuable for implementers.
162-162: Cross-references to DIP-18 and DIP-17 are consistent.The Test Vectors and DIP-9 Registry Update sections correctly reference DIP-18 for address encoding and DIP-17 for feature index reservation. These cross-links will help readers navigate the related specifications.
Also applies to: 172-172, 176-176
159-168: Test vectors table is clear and complete.The test vectors provide three diverse paths (default, alternative key_class', and mainnet/testnet variants), supporting implementation validation. The hex representation and HASH160 columns are appropriate for verification and reference implementation guidance.
1-180: Overall structure and comprehensiveness are strong.This DIP is well-organized, technically sound, and provides clear guidance for wallet and hardware wallet implementers. The integration of cross-links, explicit path syntax, normative requirements, practical examples, and security/privacy considerations makes the specification implementable and reduces ambiguity across the Dash ecosystem.
Add new DIPs to the README, fix lint, and misc other cleanup.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.