fix(wasm-sdk): enable identity_update to add ECDSA_SECP256K1 and BLS12_381 keys - #2947
Conversation
📝 WalkthroughWalkthroughIntroduces helper parser functions for key type, purpose, and security level, along with a comprehensive Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
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 |
…2_381 keys Previously, identity_update failed when adding ECDSA_SECP256K1 or BLS12_381 keys with signature verification errors. The root cause was that these key types require self-signing (is_unique_key_type() == true), but identity_update used SingleKeySigner which only held the master key. Changes: - Switch from SingleKeySigner to SimpleSigner in identity_update - Add master key to signer for state transition signing - Extract parse_keys_for_identity_update helper function for key parsing - Accept privateKeyHex/privateKeyWif for signing key types (ECDSA_SECP256K1, BLS12_381) and derive public keys from private keys - Add new signing keys to SimpleSigner so they can sign for themselves - Maintain backward compatibility: ECDSA_HASH160 still accepts data field API changes for add_public_keys parameter: - ECDSA_SECP256K1: requires privateKeyHex or privateKeyWif (was: data) - BLS12_381: requires privateKeyHex (was: data) - ECDSA_HASH160: accepts privateKeyHex or data (backward compatible) - BIP13_SCRIPT_HASH, EDDSA_25519_HASH160: unchanged (data field) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
5debb12 to
c455ee6
Compare
![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/state_transitions/identity/mod.rs (2)
54-64: Consider error handling for invalid security level values.Unlike
parse_key_typeandparse_purposewhich error on unknown values,parse_security_levelsilently defaults toHIGHfor any unrecognized string. While the docstring documents this behavior, a typo like"HGIH"would silently be accepted asHIGH, potentially masking configuration errors.Consider returning a
Resultlike the other parsers, or at least logging a warning for non-Noneunrecognized values:🔎 Potential alternative approach
-fn parse_security_level(s: Option<&str>) -> SecurityLevel { +fn parse_security_level(s: Option<&str>) -> Result<SecurityLevel, WasmSdkError> { match s { - Some("MASTER") => SecurityLevel::MASTER, - Some("CRITICAL") => SecurityLevel::CRITICAL, - Some("HIGH") => SecurityLevel::HIGH, - Some("MEDIUM") => SecurityLevel::MEDIUM, - _ => SecurityLevel::HIGH, + Some("MASTER") => Ok(SecurityLevel::MASTER), + Some("CRITICAL") => Ok(SecurityLevel::CRITICAL), + Some("HIGH") => Ok(SecurityLevel::HIGH), + Some("MEDIUM") => Ok(SecurityLevel::MEDIUM), + None => Ok(SecurityLevel::HIGH), // Default when not specified + Some(s) => Err(WasmSdkError::invalid_argument(format!( + "Unknown security level: {}", + s + ))), } }
396-561: Consider reusingparse_keys_for_identity_updateto reduce duplication.The key parsing logic in
identity_create(lines 396-561) largely duplicates what's now inparse_keys_for_identity_update. While there are minor differences (e.g.,BTreeMapvsVec, different unsupported key type handling), consolidating this logic could improve maintainability.If the behaviors need to differ, consider extracting a lower-level helper that both functions can call with configuration flags.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/wasm-sdk/src/state_transitions/identity/mod.rs
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.rs: Rust code must passcargo clippy --workspacelinter checks
Rust code must be formatted usingcargo fmt --all
**/*.rs: Use 4-space indent for Rust files
Follow rustfmt defaults and keep code clippy-clean for Rust modules
Use snake_case for Rust module names
Use PascalCase for Rust type names
Use SCREAMING_SNAKE_CASE for Rust constants
Files:
packages/wasm-sdk/src/state_transitions/identity/mod.rs
🧠 Learnings (10)
📓 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.
📚 Learning: 2025-08-05T13:55:39.147Z
Learnt from: thephez
Repo: dashpay/platform PR: 2718
File: packages/wasm-sdk/index.html:0-0
Timestamp: 2025-08-05T13:55:39.147Z
Learning: The get_identity_keys_with_proof_info function in the Rust WASM bindings does not support the "search" key request type and lacks the searchPurposeMap parameter. When proof mode is enabled with keyRequestType === 'search', the implementation falls back to the non-proof version (get_identity_keys) to maintain functionality.
Applied to files:
packages/wasm-sdk/src/state_transitions/identity/mod.rs
📚 Learning: 2024-10-09T00:22:57.778Z
Learnt from: QuantumExplorer
Repo: dashpay/platform PR: 2215
File: packages/rs-drive-abci/src/execution/platform_events/core_based_updates/update_masternode_identities/create_owner_identity/v1/mod.rs:19-30
Timestamp: 2024-10-09T00:22:57.778Z
Learning: In the Rust file `packages/rs-drive-abci/src/execution/platform_events/core_based_updates/update_masternode_identities/create_owner_identity/v1/mod.rs`, within the `create_owner_identity_v1` function, the `add_public_keys` method of the `Identity` struct cannot fail and does not require explicit error handling.
Applied to files:
packages/wasm-sdk/src/state_transitions/identity/mod.rs
📚 Learning: 2025-07-28T20:00:08.502Z
Learnt from: QuantumExplorer
Repo: dashpay/platform PR: 2711
File: packages/wasm-sdk/AI_REFERENCE.md:771-783
Timestamp: 2025-07-28T20:00:08.502Z
Learning: In packages/wasm-sdk/AI_REFERENCE.md, the documentation correctly shows the actual SDK method signatures (including identityCreate and identityTopUp with their full parameter lists), which may differ from the UI inputs shown in fixed_definitions.json. The UI may collect fewer parameters from users while handling additional requirements internally.
Applied to files:
packages/wasm-sdk/src/state_transitions/identity/mod.rs
📚 Learning: 2024-11-20T20:43:41.185Z
Learnt from: QuantumExplorer
Repo: dashpay/platform PR: 2257
File: packages/rs-drive-abci/tests/strategy_tests/masternodes.rs:212-220
Timestamp: 2024-11-20T20:43:41.185Z
Learning: In `packages/rs-drive-abci/tests/strategy_tests/masternodes.rs`, the pattern of generating a `PrivateKey`, converting it to bytes, and reconstructing a `BlsPrivateKey` from those bytes is intentional. Avoid suggesting to simplify this code in future reviews.
Applied to files:
packages/wasm-sdk/src/state_transitions/identity/mod.rs
📚 Learning: 2025-11-25T13:10:38.019Z
Learnt from: CR
Repo: dashpay/platform PR: 0
File: packages/swift-sdk/SwiftExampleApp/CLAUDE.md:0-0
Timestamp: 2025-11-25T13:10:38.019Z
Learning: Applies to packages/swift-sdk/SwiftExampleApp/**/{KeychainManager,*Key,*Identity}.swift : Store private keys separately from identities using iOS Keychain for secure storage; private keys belong to public keys, not identities
Applied to files:
packages/wasm-sdk/src/state_transitions/identity/mod.rs
📚 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/wasm-sdk/src/state_transitions/identity/mod.rs
📚 Learning: 2024-10-21T01:03:42.458Z
Learnt from: QuantumExplorer
Repo: dashpay/platform PR: 2227
File: packages/rs-dpp/src/core_types/validator_set/v0/mod.rs:299-299
Timestamp: 2024-10-21T01:03:42.458Z
Learning: In the `test_serialize_deserialize_validator_set_v0` test within `packages/rs-dpp/src/core_types/validator_set/v0/mod.rs`, deterministic BLS keys cannot be easily used; therefore, using `BlsPublicKey::generate()` is acceptable.
Applied to files:
packages/wasm-sdk/src/state_transitions/identity/mod.rs
📚 Learning: 2024-11-20T16:05:40.200Z
Learnt from: QuantumExplorer
Repo: dashpay/platform PR: 2257
File: packages/rs-drive-abci/src/platform_types/signature_verification_quorum_set/v0/for_saving.rs:148-151
Timestamp: 2024-11-20T16:05:40.200Z
Learning: In `packages/rs-drive-abci/src/platform_types/signature_verification_quorum_set/v0/for_saving.rs`, when converting public keys from `QuorumForSavingV0` to `VerificationQuorum`, it's acceptable to use `.expect()` for public key conversion, as the conversion has been verified and panics are acceptable in this context.
Applied to files:
packages/wasm-sdk/src/state_transitions/identity/mod.rs
📚 Learning: 2024-10-30T11:19:59.163Z
Learnt from: lklimek
Repo: dashpay/platform PR: 2277
File: packages/rs-sdk/tests/fetch/config.rs:233-233
Timestamp: 2024-10-30T11:19:59.163Z
Learning: In the Rust SDK's `rs-sdk/tests` integration tests (e.g., in `packages/rs-sdk/tests/fetch/config.rs`), we cannot create objects during tests because there is no support for object creation in this context. Therefore, hardcoded values for test identities must be used.
Applied to files:
packages/wasm-sdk/src/state_transitions/identity/mod.rs
🧬 Code graph analysis (1)
packages/wasm-sdk/src/state_transitions/identity/mod.rs (3)
packages/wasm-sdk/src/error.rs (2)
invalid_argument(75-77)generic(71-73)packages/wasm-sdk/src/queries/identity.rs (3)
new(94-96)new(125-127)keys(107-113)packages/simple-signer/src/signer.rs (1)
private_keys(39-43)
🪛 Gitleaks (8.30.0)
packages/wasm-sdk/src/state_transitions/identity/mod.rs
[high] 1936-1936: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
⏰ 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) / Linting
- GitHub Check: Rust packages (wasm-sdk) / Tests
- GitHub Check: Build Docker images (Dashmate helper, dashmate-helper, dashmate-helper) / Build Dashmate helper image
- GitHub Check: Build Docker images (Drive, drive, drive-abci, SDK_TEST_DATA=true
) / 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 (3)
packages/wasm-sdk/src/state_transitions/identity/mod.rs (3)
89-288: LGTM!The
parse_keys_for_identity_updatefunction is well-structured with comprehensive key type handling. The approach of usingOption<[u8; 32]>to track whether keys need signing (matchingis_unique_key_type()semantics) is clean, and error messages are specific and helpful for debugging.
1248-1263: LGTM - Core fix for identity_update with non-HASH160 keys.The switch from
SingleKeySignertoSimpleSignerwith both the master key and new signing keys correctly addresses the root cause. NewECDSA_SECP256K1andBLS12_381keys can now self-sign as required by DPP validation (is_unique_key_type() == true).
1817-2092: Excellent test coverage for the new parser.The tests comprehensively cover the key parsing scenarios including:
- Both
privateKeyHexandprivateKeyWifformats for ECDSA_SECP256K1- WIF rejection for BLS12_381
- Signer population verification (signing keys added, non-signing keys not added)
- Error cases (missing keys, invalid formats, wrong lengths)
- Mixed key sets and edge cases
The static analysis hint on line 1936 is a false positive - the WIF
cNurmMT7bXe3S92JijPi2V5wSHvWzrk4vKJ7fgLPNo5Ajv2YAP3zis an intentional test key used to verify BLS12_381 correctly rejects WIF format.
2 participants
Issue being fixed or feature implemented
Previously, identity_update failed when adding non HASH_160 keys with signature verification errors. The root cause was that these key types require self-signing (is_unique_key_type() == true), but identity_update used SingleKeySigner which only held the master key.
What was done?
Changes:
API changes for add_public_keys parameter:
How Has This Been Tested?
Added 12 unit tests for the parse_keys_for_identity_update helper function:
Tests cover:
Tests verify both the returned keys and that the SimpleSigner is correctly populated (signing keys added, non-signing keys excluded).
Breaking Changes
None. The previous behavior for adding ECDSA_SECP256K1 and BLS12_381 keys via identity_update was non-functional (always failed signature verification). This fix makes these key types work by requiring private keys for signing.
Checklist:
For repository code-owners and collaborators only
Summary by CodeRabbit
New Features
Refactor
Tests
✏️ Tip: You can customize this high-level summary in your review settings.