feat(sdk): add validation/tests for registerName publicKeyId parameter #2832
Conversation
|
@coderabbitai review |
📝 WalkthroughWalkthroughAdds a new public Changes
Sequence DiagramsequenceDiagram
participant User as User Code
participant JsFacade as JS Facade
participant Wasm as WASM SDK
participant KeyStore as Identity Key Store / Signer
User->>JsFacade: registerName(label, identityId, publicKeyId, privateKeyWif, onPreorder?)
activate JsFacade
JsFacade->>JsFacade: Validate publicKeyId (defined, non-null, >= 0)
alt publicKeyId invalid
JsFacade-->>User: Throw validation error
else publicKeyId valid
JsFacade->>Wasm: dpnsRegisterName(label, identityId, publicKeyId, privateKeyWif, onPreorder)
activate Wasm
Wasm->>KeyStore: Get public key for (identityId, publicKeyId)
activate KeyStore
KeyStore-->>Wasm: publicKey (purpose, security_level, data)
Wasm->>Wasm: Check purpose == AUTHENTICATION
Wasm->>Wasm: Check security_level ∈ {CRITICAL, HIGH}
Wasm->>KeyStore: signer.can_sign_with(publicKey, privateKeyWif)
KeyStore-->>Wasm: can_sign (true/false)
alt any validation fails
Wasm-->>JsFacade: Return invalid-argument error (descriptive)
JsFacade-->>User: Propagate error
else all validations pass
Wasm->>Wasm: Store callback & register DPNS name
Wasm-->>JsFacade: Registration result
JsFacade-->>User: Registration result
end
deactivate KeyStore
deactivate Wasm
end
deactivate JsFacade
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 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 |
✅ Actions performedReview triggered.
|
thephez
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/wasm-sdk/src/dpns.rs (1)
82-141: Validation logic is correct and provides excellent error messages.The key validation properly enforces DPNS requirements (AUTHENTICATION purpose, CRITICAL/HIGH security levels) and provides helpful guidance by listing suitable alternative keys.
Consider these minor refinements for more idiomatic Rust:
Lines 107-118: Replace the if-else and string concatenation with a match expression and format! macro:
- let level_name = if k.security_level() == SecurityLevel::CRITICAL { - "CRITICAL" - } else { - "HIGH" - }; - Some( - String::from(" Key ") - + &key_id.to_string() - + ": " - + level_name - + " security level", - ) + let level_name = match k.security_level() { + SecurityLevel::CRITICAL => "CRITICAL", + SecurityLevel::HIGH => "HIGH", + _ => return None, + }; + Some(format!(" Key {}: {} security level", key_id, level_name))packages/js-evo-sdk/src/dpns/facade.ts (1)
60-81: Solid input validation with helpful error messages.The validation logic correctly handles the common error cases (undefined, null, non-number, negative) and provides clear guidance about which keys are suitable for DPNS registration. This addresses the PR objective of providing early validation before cryptographic operations.
For completeness, you could add validation for
NaNandInfinity, though these are unlikely in practice:- if (typeof publicKeyId !== 'number' || publicKeyId < 0) { + if (typeof publicKeyId !== 'number' || !Number.isFinite(publicKeyId) || publicKeyId < 0) { throw new Error(`publicKeyId must be a non-negative number, got: ${publicKeyId}`); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/js-evo-sdk/src/dpns/facade.ts(1 hunks)packages/js-evo-sdk/tests/unit/facades/dpns.spec.mjs(1 hunks)packages/wasm-sdk/src/dpns.rs(2 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
packages/**/tests/**
📄 CodeRabbit inference engine (AGENTS.md)
Place unit and integration tests alongside each package in packages//tests
Files:
packages/js-evo-sdk/tests/unit/facades/dpns.spec.mjs
**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.rs: Format Rust code with cargo fmt
Run Clippy linter for Rust code
Files:
packages/wasm-sdk/src/dpns.rs
packages/wasm-sdk/**
📄 CodeRabbit inference engine (CLAUDE.md)
Keep WASM SDK docs in sync (run generate_docs.py) when updating the WASM SDK
Files:
packages/wasm-sdk/src/dpns.rs
packages/**/**/*.{js,ts,jsx,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
packages/**/**/*.{js,ts,jsx,tsx}: Adhere to ESLint with Airbnb/TypeScript configs for JS/TS code
Use camelCase for JS/TS variables and functions
Use PascalCase for JS/TS classes
Prefer kebab-case filenames within JS packages
Files:
packages/js-evo-sdk/src/dpns/facade.ts
🔇 Additional comments (4)
packages/wasm-sdk/src/dpns.rs (2)
5-7: LGTM! Imports support the new validation logic.The imported traits and enums are necessary for the key validation functionality added below.
143-155: Excellent security validation!Verifying that the private key matches the public key via
signer.can_sign_withbefore proceeding prevents cryptographic failures and provides clear error messages early in the flow. This directly addresses the PR objective of catching errors before attempting to sign state transitions.packages/js-evo-sdk/tests/unit/facades/dpns.spec.mjs (1)
58-143: Excellent test coverage for publicKeyId validation!The test suite comprehensively covers all edge cases:
- Missing, undefined, and null values
- Invalid types (string)
- Invalid ranges (negative numbers)
- Valid positive case
The error message assertions verify that users receive helpful guidance, directly supporting the PR objective of improving error messages for DPNS registration.
packages/js-evo-sdk/src/dpns/facade.ts (1)
36-59: Outstanding documentation with clear security guidance!The JSDoc provides comprehensive guidance that will help developers avoid the cryptic error described in the PR objectives. The explicit warning about key security levels and the practical example make the API much more developer-friendly.
thephez
changed the title
thephez
changed the title
| async registerName(args: { label: string; identityId: string; publicKeyId: number; privateKeyWif: string; onPreorder?: Function }): Promise<any> { | ||
| const { label, identityId, publicKeyId, privateKeyWif, onPreorder } = args; | ||
|
|
||
| // Validate inputs |
There was a problem hiding this comment.
How this is possible? Typing is not allow it. Oh, when you use it form JS, not TypeScript?
There was a problem hiding this comment.
Yes, it was from JS. Just trying to make things easier for people like me that don't know what they're doing 🤪 You can close the PR if it is a bad approach.
|
@shumkov what should be done here? |
…election Add comprehensive validation before signing DPNS registration state transitions to catch common errors early and provide clear, actionable error messages to developers. Fixes issue where users accidentally use MASTER key (ID 0) which is not allowed for DPNS registration, resulting in cryptic error messages only after signing attempt.
thephez
force-pushed
the
feat/dpns-key-validation
branch
from
28f639a to
9e06d0d
Compare
|
Rebased on latest v3.0-dev. Defer to @shumkov as to whether or not it makes sense to include. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/js-evo-sdk/tests/unit/facades/dpns.spec.mjs (2)
103-130: Consider testing additional edge cases.The tests cover negative and non-number values, but could also test:
publicKeyId: 0(MASTER key, which per the PR description should be rejected)- Float values like
1.5- Special values like
NaNorInfinityThese would ensure the validation is comprehensive.
132-140: Consider verifying call arguments.The positive test confirms
dpnsRegisterNameis called once but doesn't verify the arguments passed. Adding argument verification would strengthen the test.🔎 Example enhancement
it('should accept valid publicKeyId', async () => { await client.dpns.registerName({ label: 'test', identityId: 'someId', publicKeyId: 1, privateKeyWif: 'someKey', }); expect(wasmSdk.dpnsRegisterName).to.be.calledOnce(); + expect(wasmSdk.dpnsRegisterName).to.be.calledWith( + 'test', + 'someId', + 1, + 'someKey', + null + ); });
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/js-evo-sdk/src/dpns/facade.tspackages/js-evo-sdk/tests/unit/facades/dpns.spec.mjs
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/js-evo-sdk/src/dpns/facade.ts
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: QuantumExplorer
Repo: dashpay/platform PR: 2673
File: packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/identity_update/mod.rs:1164-1197
Timestamp: 2025-06-18T03:44:14.385Z
Learning: QuantumExplorer determined that a CodeRabbit suggestion about fixing signable_bytes calculation in identity update tests with contract-bound keys was incorrect - the code flow is working as intended without the suggested changes.
Learnt from: QuantumExplorer
Repo: dashpay/platform PR: 2711
File: packages/wasm-sdk/docs.html:2359-2383
Timestamp: 2025-07-28T20:00:24.323Z
Learning: In packages/wasm-sdk/docs.html, QuantumExplorer confirmed that placeholder private keys in documentation examples are acceptable as they are not real keys, though field name accuracy for the SDK API should still be maintained.
📚 Learning: 2025-11-25T13:10:47.943Z
Learnt from: CR
Repo: dashpay/platform PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-25T13:10:47.943Z
Learning: Applies to **/tests/**/*.{js,jsx,ts,tsx,rs} : Name tests descriptively, starting with 'should …'
Applied to files:
packages/js-evo-sdk/tests/unit/facades/dpns.spec.mjs
📚 Learning: 2025-07-28T20:00:24.323Z
Learnt from: QuantumExplorer
Repo: dashpay/platform PR: 2711
File: packages/wasm-sdk/docs.html:2359-2383
Timestamp: 2025-07-28T20:00:24.323Z
Learning: In packages/wasm-sdk/docs.html, QuantumExplorer confirmed that placeholder private keys in documentation examples are acceptable as they are not real keys, though field name accuracy for the SDK API should still be maintained.
Applied to files:
packages/js-evo-sdk/tests/unit/facades/dpns.spec.mjs
🧬 Code graph analysis (1)
packages/js-evo-sdk/tests/unit/facades/dpns.spec.mjs (6)
packages/wasm-sdk/tests/functional/identities.spec.mjs (1)
client(9-9)packages/js-evo-sdk/tests/unit/facades/group.spec.mjs (2)
client(6-6)wasmSdk(5-5)packages/js-evo-sdk/tests/unit/facades/identities.spec.mjs (5)
client(6-6)wasmSdk(5-5)wasmSdk(168-168)wasmSdk(185-185)wasmSdk(201-201)packages/js-evo-sdk/tests/unit/facades/protocol.spec.mjs (2)
client(6-6)wasmSdk(5-5)packages/wasm-sdk/tests/functional/dpns.spec.mjs (1)
client(8-8)packages/js-evo-sdk/tests/unit/facades/tokens.spec.mjs (2)
client(6-6)wasmSdk(5-5)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: Rust packages (wasm-sdk) / Unused dependencies
- GitHub Check: Rust packages (wasm-sdk) / Linting
- GitHub Check: Rust packages (wasm-sdk) / Tests
- GitHub Check: Build Docker images (RS-DAPI, rs-dapi, rs-dapi) / Build RS-DAPI image
- GitHub Check: Build Docker images (Drive, drive, drive-abci) / Build Drive image
- GitHub Check: Build JS packages / Build JS
- GitHub Check: Rust crates security audit
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/js-evo-sdk/tests/unit/facades/dpns.spec.mjs (1)
58-142: Comprehensive test coverage for publicKeyId validation.The test suite thoroughly covers the JS-layer validation requirements:
- Presence checks (omitted, undefined, null)
- Type validation (non-number)
- Range validation (negative)
- Happy path (valid publicKeyId)
All test names follow the "should..." convention, error assertions are specific and helpful, and the structure is consistent throughout.
💡 Optional: Consider edge case tests
While the current coverage is solid, you might optionally add tests for:
publicKeyId: 0to document that MASTER keys pass JS validation (and are rejected later by WASM with a better error)- Non-integer numbers like
1.5to verify behavior with floats- Special values like
NaNorInfinityThese are not critical since the current tests adequately cover the intended validation logic.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/js-evo-sdk/tests/unit/facades/dpns.spec.mjs
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: QuantumExplorer
Repo: dashpay/platform PR: 2673
File: packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/identity_update/mod.rs:1164-1197
Timestamp: 2025-06-18T03:44:14.385Z
Learning: QuantumExplorer determined that a CodeRabbit suggestion about fixing signable_bytes calculation in identity update tests with contract-bound keys was incorrect - the code flow is working as intended without the suggested changes.
Learnt from: QuantumExplorer
Repo: dashpay/platform PR: 2711
File: packages/wasm-sdk/docs.html:2359-2383
Timestamp: 2025-07-28T20:00:24.323Z
Learning: In packages/wasm-sdk/docs.html, QuantumExplorer confirmed that placeholder private keys in documentation examples are acceptable as they are not real keys, though field name accuracy for the SDK API should still be maintained.
📚 Learning: 2024-12-03T05:47:30.524Z
Learnt from: shumkov
Repo: dashpay/platform PR: 2365
File: packages/dashmate/src/listr/tasks/ssl/zerossl/obtainZeroSSLCertificateTaskFactory.js:206-213
Timestamp: 2024-12-03T05:47:30.524Z
Learning: In `obtainZeroSSLCertificateTaskFactory.js`, when handling ZeroSSL API errors during domain verification, the error detail structure is consistent when an error occurs, so additional null checks on `e.details[ctx.externalIp]` may not be necessary.
Applied to files:
packages/js-evo-sdk/tests/unit/facades/dpns.spec.mjs
📚 Learning: 2025-06-18T03:44:14.385Z
Learnt from: QuantumExplorer
Repo: dashpay/platform PR: 2673
File: packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/identity_update/mod.rs:1164-1197
Timestamp: 2025-06-18T03:44:14.385Z
Learning: QuantumExplorer determined that a CodeRabbit suggestion about fixing signable_bytes calculation in identity update tests with contract-bound keys was incorrect - the code flow is working as intended without the suggested changes.
Applied to files:
packages/js-evo-sdk/tests/unit/facades/dpns.spec.mjs
📚 Learning: 2025-07-28T20:00:24.323Z
Learnt from: QuantumExplorer
Repo: dashpay/platform PR: 2711
File: packages/wasm-sdk/docs.html:2359-2383
Timestamp: 2025-07-28T20:00:24.323Z
Learning: In packages/wasm-sdk/docs.html, QuantumExplorer confirmed that placeholder private keys in documentation examples are acceptable as they are not real keys, though field name accuracy for the SDK API should still be maintained.
Applied to files:
packages/js-evo-sdk/tests/unit/facades/dpns.spec.mjs
📚 Learning: 2025-11-25T13:10:47.943Z
Learnt from: CR
Repo: dashpay/platform PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-25T13:10:47.943Z
Learning: Applies to **/tests/**/*.{js,jsx,ts,tsx,rs} : Name tests descriptively, starting with 'should …'
Applied to files:
packages/js-evo-sdk/tests/unit/facades/dpns.spec.mjs
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: Rust packages (wasm-sdk) / Unused dependencies
- GitHub Check: Rust packages (wasm-sdk) / Linting
- GitHub Check: Rust packages (wasm-sdk) / Tests
- GitHub Check: Build Docker images (Drive, drive, drive-abci) / Build Drive image
- GitHub Check: Build Docker images (RS-DAPI, rs-dapi, rs-dapi) / Build RS-DAPI image
- GitHub Check: Build JS packages / Build JS
- GitHub Check: Rust crates security audit
🔇 Additional comments (1)
packages/js-evo-sdk/tests/unit/facades/dpns.spec.mjs (1)
89-102: Past review feedback addressed.The null
publicKeyIdtest now includes both error message assertions ('publicKeyId is required' and 'CRITICAL or HIGH security level'), making it consistent with the omitted and undefined test cases. This resolves the previous review concern.
|
Test failure is unrelated to this PR. Also present on v3.0-dev (e.g. https://github.com/dashpay/platform/actions/runs/20488069314/job/58874775920). |
Issue being fixed or feature implemented
Developers using the DPNS registration API were encountering a cryptic error message "Invalid public key security level MASTER" when the identity key parameter was not passed. This error only appeared after attempting to sign the state transition, making it difficult to understand what went wrong and how to fix it.
What was done?
Added comprehensive early validation and helpful error messages for DPNS name registration:
packages/wasm-sdk/src/dpns.rs): Validate key purpose is AUTHENTICATION, security level is CRITICAL or HIGH (not MASTER), and private key matches the specified public key before attempting cryptographic operationspackages/js-evo-sdk/src/dpns/facade.ts): Add input validation forpublicKeyIdparameter with clear error messages and comprehensive JSDoc documentationpackages/js-evo-sdk/tests/unit/facades/dpns.spec.mjs): Add 6 test cases covering validation of omitted, undefined, null, negative, non-number, and validpublicKeyIdvaluesError messages now explain which requirement failed, list all suitable keys available in the identity, and provide specific guidance (e.g., "Use key 1 (CRITICAL) or 2 (HIGH), NOT 0 (MASTER)").
How Has This Been Tested?
Breaking Changes
None. This is fully backwards compatible - it only adds validation and improves error messages.
Checklist:
For repository code-owners and collaborators only
Summary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.