Skip to content

Conversation

thephez
Copy link
Collaborator

@thephez thephez commented Sep 2, 2025

Issue being fixed or feature implemented

Fix for the duplicate signature issue in WASM SDK identity creation.

What was done?

Replaces SingleKeySigner with SimpleSigner, enabling individual signing for each public key. The implementation now accepts privateKeyHex/privateKeyWif for each key and ensures ECDSA_SECP256K1 and BLS12_381 keys receive unique signatures while ECDSA_HASH160 keys appropriately have empty signatures. Comprehensive test coverage validates all key type scenarios and confirms the fix prevents signature verification errors during identity state transitions.

Also, commented out the state transition test that causes a panic and was preventing state transition tests from actually running in CI.

How Has This Been Tested?

Via the newly added tests. Also put raw transition output into DET transaction deserializer to check for correct format.

Breaking Changes

The publicKeys parameter now requires additional fields for certain key types

Checklist:

  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have added or updated relevant unit/integration/functional/e2e tests
  • I have added "!" to the title and described breaking changes in the corresponding section if my code contains any
  • I have made corresponding changes to the documentation if needed

For repository code-owners and collaborators only

  • I have assigned this pull request to a milestone

Summary by CodeRabbit

  • New Features

    • Added keyType selection for Identity Create, applied across all generated keys.
    • Live preview shows chosen key type and per-key signature requirements.
    • SDK now exports generate_key_pair for programmatic key generation.
  • Documentation

    • Updated Identity Create examples and docs to use keyType and document per-type signing requirements.
    • Clarified asset lock proof wording and publicKeys structure (including private key fields).
  • Tests

    • Added identity_create tests for SECP256K1-only, mixed, and HASH160-only scenarios.
    • Removed a previously panicking test.

- Replace SingleKeySigner with SimpleSigner for identity creation
- Enable individual signing for each public key in identity
- Add support for privateKeyHex/privateKeyWif for each key
- Add comprehensive test coverage for all key type scenarios
- Fix duplicate signature issue in identity state transitions

This ensures each ECDSA_SECP256K1 and BLS12_381 key gets its own unique signature during identity creation, preventing signature verification errors.
@thephez thephez added this to the v2.1 milestone Sep 2, 2025
Copy link
Contributor

coderabbitai bot commented Sep 2, 2025

Walkthrough

Refactors identity creation to accumulate per-key private keys into a SimpleSigner (enabled via the state-transitions feature), updates keyType handling and requirements across SDK, tests, docs, API definitions, and UI, and exports generate_key_pair for test use.

Changes

Cohort / File(s) Summary
Build config
packages/wasm-sdk/Cargo.toml
Enables features = ["state-transitions"] for the simple-signer dependency.
Identity state transition logic
packages/wasm-sdk/src/state_transitions/identity/mod.rs
Reworks identity creation to parse keyType, derive/require per-key private data, accumulate signable keys into a per-key SimpleSigner::default(), skip HASH160 from signing, remove single-key asset_lock_proof signer, and route final signing through the aggregated SimpleSigner. Adds explicit errors for missing/unsupported key data.
Tests and SDK export
packages/wasm-sdk/test/state-transitions.test.mjs, packages/wasm-sdk/pkg/wasm_sdk.js
Adds and imports generate_key_pair export; introduces three identity_create tests (SECP256K1-only, mixed, HASH160-only) using generated keys and mock asset lock proof; removes a previously panicking test.
API definitions and docs
packages/wasm-sdk/api-definitions.json, packages/wasm-sdk/AI_REFERENCE.md, packages/wasm-sdk/docs.html
Adds keyType input to identityCreate, updates publicKeys descriptions to list per-type signing requirements, renames typekeyType, converts numeric enums to string enums, and updates examples to include privateKeyWif/privateKeyHex usage and HASH160 notes.
UI integration
packages/wasm-sdk/index.html
Adds keyType selection to identity creation form, wires handlers to update preview and populate generated publicKeys with the selected key type, and shows signature-requirement notes in preview.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  participant UI as UI (index.html)
  participant SDK as wasm-sdk (identityCreate)
  participant SS as SimpleSigner (per-key)
  participant Plat as Platform

  UI->>SDK: identityCreate(assetLockProof, assetLockPrivateKey, publicKeys)
  SDK->>SS: SimpleSigner::default()
  loop for each publicKey in publicKeys
    SDK->>SDK: parse keyType, purpose, data/privateKeyHex/privateKeyWif
    alt keyType == ECDSA_SECP256K1 or BLS12_381
      SDK->>SS: add(publicKeyBytes, private_key_bytes)
    else keyType == ECDSA_HASH160
      SDK->>SDK: use/derive hash (no signer entry)
    else Unsupported
      SDK-->>UI: error (unsupported key type)
    end
  end
  SDK->>Plat: submit state transition (signed via SS)
  alt Platform validation OK
    Plat-->>SDK: ack
    SDK-->>UI: success
  else validation/signature failure
    Plat-->>SDK: error
    SDK-->>UI: error (detailed)
  end
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Possibly related PRs

Suggested reviewers

  • QuantumExplorer
  • pauldelucia
  • ktechmidas

Poem

A rabbit signs with careful paws,
Each key obeys its proper laws.
SECP hops, HASH stays still,
BLS hums on the windy hill.
With SimpleSigner’s tidy art, the keys all play their part. 🐇✨

✨ Finishing Touches
  • 📝 Generate Docstrings
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch wasm-sdk-fix-id-create-ecdsa-signing

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.

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.

Support

Need help? Create a ticket on our support page for assistance with any issues or questions.

CodeRabbit Commands (Invoked using PR/Issue comments)

Type @coderabbitai help to get the list of available commands.

Other keywords and placeholders

  • Add @coderabbitai ignore or @coderabbit ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Status, Documentation and Community

  • Visit our Status Page to check the current availability of CodeRabbit.
  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@thephez
Copy link
Collaborator Author

thephez commented Sep 2, 2025

@coderabbitai review

Copy link
Contributor

coderabbitai bot commented Sep 2, 2025

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

Copy link
Contributor

@coderabbitai coderabbitai bot left a 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

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)
packages/wasm-sdk/src/state_transitions/identity/mod.rs (2)

44-49: Do not log sensitive material — redact public_keys and asset lock proof content.

public_keys contains privateKeyHex/privateKeyWif per key; logging it leaks secrets to the console. Also avoid dumping the asset lock proof body.

Apply:

-        web_sys::console::log_1(&JsValue::from_str(&format!("identityCreate called with:")));
-        web_sys::console::log_1(&JsValue::from_str(&format!("  asset_lock_proof (length {}): {}", asset_lock_proof.len(), if asset_lock_proof.len() > 100 { format!("{}...", &asset_lock_proof[..100]) } else { asset_lock_proof.clone() })));
-        web_sys::console::log_1(&JsValue::from_str(&format!("  asset_lock_proof_private_key: [REDACTED] (length: {})", asset_lock_proof_private_key.len())));
-        web_sys::console::log_1(&JsValue::from_str(&format!("  public_keys: {}", public_keys)));
+        web_sys::console::log_1(&JsValue::from_str("identityCreate called"));
+        web_sys::console::log_1(&JsValue::from_str(&format!("  asset_lock_proof length: {}", asset_lock_proof.len())));
+        web_sys::console::log_1(&JsValue::from_str(&format!("  asset_lock_proof_private_key: [REDACTED] (length: {})", asset_lock_proof_private_key.len())));
+        // public_keys intentionally not logged (contains private key material)

And after parsing keys_array (see Lines 80-82), you can safely log count:

         let keys_array = keys_data.as_array()
             .ok_or_else(|| JsValue::from_str("public_keys must be a JSON array"))?;
+
+        web_sys::console::log_1(&JsValue::from_str(&format!("  public_keys count: {}", keys_array.len())));

106-116: Purpose parsing misses 'OWNER' (used elsewhere in this file).

identity_credit_withdrawal accepts OWNER keys, but identityCreate can’t create them from JSON (“Unknown purpose: OWNER”). Add it here.

Apply:

             let purpose = match purpose_str {
                 "AUTHENTICATION" => Purpose::AUTHENTICATION,
                 "ENCRYPTION" => Purpose::ENCRYPTION,
                 "DECRYPTION" => Purpose::DECRYPTION,
                 "TRANSFER" => Purpose::TRANSFER,
+                "OWNER" => Purpose::OWNER,
                 "SYSTEM" => Purpose::SYSTEM,
                 "VOTING" => Purpose::VOTING,
                 _ => return Err(JsValue::from_str(&format!("Unknown purpose: {}", purpose_str)))
             };
🧹 Nitpick comments (7)
packages/wasm-sdk/Cargo.toml (1)

72-74: CI warning: wasm-pack ignores package.metadata.wasm-pack.wasm-opt.

The pipeline warns this key is unknown and ignored. Either remove it or update to the currently supported config to avoid noisy CI.

If not needed, delete the section; otherwise, adjust to the current wasm-pack schema.

packages/wasm-sdk/src/state_transitions/identity/mod.rs (2)

51-66: Clarify accepted asset_lock_proof format (hex vs JSON) and parse accordingly.

The current path treats “hex” as hex-encoded JSON; if callers pass raw Core tx hex (per the docstring), this will fail on UTF‑8/JSON. Align docs and parsing with the actual expected wire format.

Options:

  • If JSON is the only supported input, update the doc comment to say “JSON string” and drop the hex branch.
  • If raw tx hex must be supported, construct the appropriate AssetLockProof variant from the tx bytes rather than trying to parse JSON from bytes.
    Would you like me to propose a concrete parser once the intended format is confirmed?

244-266: Apply the same logging hygiene to identityTopUp.

Top-up logs private key length (good) but still logs full asset_lock_proof elsewhere by length—OK. Ensure you never log secrets here either.

packages/wasm-sdk/test/state-transitions.test.mjs (4)

23-25: Avoid double-importing the WASM module.

You can drop the separate named import and use wasmSdk.generate_key_pair to prevent dual module bindings.

Apply:

-import init, { generate_key_pair } from '../pkg/wasm_sdk.js';
-import * as wasmSdk from '../pkg/wasm_sdk.js';
+import init, * as wasmSdk from '../pkg/wasm_sdk.js';

And replace generate_key_pair(...) with wasmSdk.generate_key_pair(...).


85-138: SECP256K1-only identity_create test — good negative coverage; de-dup error checks.

The structure is fine; consider factoring the repeated “not signature failed verification” assertion into a helper to reduce repetition.

Example helper:

+function expectNotSigVerificationError(error) {
+  const msg = error?.message || String(error);
+  if (msg.includes('Should fail')) throw error;
+  if (msg.includes('signature failed verification')) {
+    throw new Error('SIGNATURE VERIFICATION ERROR - SimpleSigner may not be working correctly');
+  }
+}
...
-    } catch (error) {
-        const errorMessage = error.message || error.toString() || 'Unknown error';
-        if (errorMessage.includes('Should fail')) {
-            throw error;
-        }
-        // Check that it's NOT a signature verification error
-        if (errorMessage.includes('signature failed verification')) {
-            throw new Error('SIGNATURE VERIFICATION ERROR - SimpleSigner may not be working correctly');
-        }
-        console.log('   Expected error with mock data (not signature error)');
-    }
+    } catch (error) {
+        expectNotSigVerificationError(error);
+        console.log('   Expected error with mock data (not signature error)');
+    }

140-193: Mixed types test — OK; same suggestion to factor error handling helper.


313-323: Commented-out panic test — prefer an explicit skip toggle.

Given the custom runner, a guard flag (e.g., env var) around the block is clearer than commented code and keeps history intact.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

💡 Knowledge Base configuration:

  • MCP integration is disabled by default for public repositories
  • Jira integration is disabled by default for public repositories
  • Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between 23125f8 and 4bfbec7.

📒 Files selected for processing (3)
  • packages/wasm-sdk/Cargo.toml (1 hunks)
  • packages/wasm-sdk/src/state_transitions/identity/mod.rs (5 hunks)
  • packages/wasm-sdk/test/state-transitions.test.mjs (3 hunks)
🧰 Additional context used
🧠 Learnings (15)
📓 Common learnings
Learnt from: QuantumExplorer
PR: dashpay/platform#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-07-23T08:31:42.268Z
Learnt from: CR
PR: dashpay/platform#0
File: packages/wasm-sdk/CLAUDE.md:0-0
Timestamp: 2025-07-23T08:31:42.268Z
Learning: Applies to packages/wasm-sdk/src/**/*.rs : Token functions are methods on WasmSdk, not standalone functions; avoid importing them as standalone.

Applied to files:

  • packages/wasm-sdk/Cargo.toml
  • packages/wasm-sdk/test/state-transitions.test.mjs
📚 Learning: 2025-07-23T08:31:42.268Z
Learnt from: CR
PR: dashpay/platform#0
File: packages/wasm-sdk/CLAUDE.md:0-0
Timestamp: 2025-07-23T08:31:42.268Z
Learning: Applies to packages/wasm-sdk/src/**/*.rs : When implementing WASM SDK functionality, always refer to AI_REFERENCE.md first for accurate method signatures and examples.

Applied to files:

  • packages/wasm-sdk/Cargo.toml
📚 Learning: 2025-07-23T08:31:42.268Z
Learnt from: CR
PR: dashpay/platform#0
File: packages/wasm-sdk/CLAUDE.md:0-0
Timestamp: 2025-07-23T08:31:42.268Z
Learning: Applies to packages/wasm-sdk/**/index.html : When adding new queries or state transitions, update the definitions in index.html.

Applied to files:

  • packages/wasm-sdk/Cargo.toml
  • packages/wasm-sdk/test/state-transitions.test.mjs
📚 Learning: 2025-07-23T08:31:42.268Z
Learnt from: CR
PR: dashpay/platform#0
File: packages/wasm-sdk/CLAUDE.md:0-0
Timestamp: 2025-07-23T08:31:42.268Z
Learning: Applies to packages/wasm-sdk/src/**/*.rs : For WASM builds, fix 'time not implemented on this platform' errors by using js_sys::Date::now().

Applied to files:

  • packages/wasm-sdk/Cargo.toml
📚 Learning: 2025-09-02T13:30:17.696Z
Learnt from: thephez
PR: dashpay/platform#2739
File: packages/wasm-sdk/test/ui-automation/tests/state-transitions.spec.js:1-171
Timestamp: 2025-09-02T13:30:17.696Z
Learning: In packages/wasm-sdk/index.html, state transition definitions are loaded dynamically from api-definitions.json rather than being hardcoded in the HTML file. The UI loads transition categories, types, inputs, and labels from this JSON configuration file.

Applied to files:

  • packages/wasm-sdk/Cargo.toml
📚 Learning: 2025-09-02T13:30:17.696Z
Learnt from: thephez
PR: dashpay/platform#2739
File: packages/wasm-sdk/test/ui-automation/tests/state-transitions.spec.js:1-171
Timestamp: 2025-09-02T13:30:17.696Z
Learning: In packages/wasm-sdk/index.html, state transition definitions are loaded dynamically from api-definitions.json via the loadApiDefinitions() function that fetches './api-definitions.json'. The UI doesn't have hardcoded transition definitions - instead it populates categories, types, inputs, and labels from this JSON configuration file at runtime.

Applied to files:

  • packages/wasm-sdk/Cargo.toml
📚 Learning: 2025-03-11T09:39:23.071Z
Learnt from: shumkov
PR: dashpay/platform#2489
File: packages/rs-dpp/Cargo.toml:32-32
Timestamp: 2025-03-11T09:39:23.071Z
Learning: In the Dash Platform project, dependencies are currently managed using Git repository references with tags (repo+tag format in Cargo.toml) rather than published crates, as the team is not currently publishing crates to crates.io.

Applied to files:

  • packages/wasm-sdk/Cargo.toml
📚 Learning: 2025-07-28T20:00:24.323Z
Learnt from: QuantumExplorer
PR: dashpay/platform#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/wasm-sdk/test/state-transitions.test.mjs
📚 Learning: 2025-07-28T20:00:08.502Z
Learnt from: QuantumExplorer
PR: dashpay/platform#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/test/state-transitions.test.mjs
  • packages/wasm-sdk/src/state_transitions/identity/mod.rs
📚 Learning: 2025-07-23T08:31:05.082Z
Learnt from: CR
PR: dashpay/platform#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-23T08:31:05.082Z
Learning: Applies to packages/wasm-sdk/index.html : Test the WASM SDK using the web interface at 'index.html' in 'packages/wasm-sdk'

Applied to files:

  • packages/wasm-sdk/test/state-transitions.test.mjs
📚 Learning: 2025-07-23T08:31:05.082Z
Learnt from: CR
PR: dashpay/platform#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-23T08:31:05.082Z
Learning: Applies to packages/wasm-sdk/build.sh : Build the WASM SDK by running './build.sh' in 'packages/wasm-sdk'

Applied to files:

  • packages/wasm-sdk/test/state-transitions.test.mjs
📚 Learning: 2025-07-23T08:31:05.082Z
Learnt from: CR
PR: dashpay/platform#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-23T08:31:05.082Z
Learning: Use WASM bindings to connect Rust and JavaScript code for cross-language integration

Applied to files:

  • packages/wasm-sdk/test/state-transitions.test.mjs
📚 Learning: 2024-10-09T00:22:57.778Z
Learnt from: QuantumExplorer
PR: dashpay/platform#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: 2024-10-03T11:51:06.980Z
Learnt from: shumkov
PR: dashpay/platform#2201
File: packages/rs-platform-version/src/version/v2.rs:1186-1188
Timestamp: 2024-10-03T11:51:06.980Z
Learning: In the `IdentityTransitionVersions` structure within `packages/rs-platform-version/src/version/v2.rs`, the field `credit_withdrawal` does not need the `identity_` prefix since it is already encompassed within identity state transitions.

Applied to files:

  • packages/wasm-sdk/src/state_transitions/identity/mod.rs
🪛 GitHub Actions: Build and Test WASM SDK
packages/wasm-sdk/Cargo.toml

[warning] 1-1: wasm-pack: Unknown key 'package.metadata.wasm-pack.wasm-opt' in Cargo.toml; it will be ignored.

🔇 Additional comments (7)
packages/wasm-sdk/Cargo.toml (1)

28-28: Confirm simple-signer feature and WASM compatibility

  • state-transitions is declared in packages/simple-signer/Cargo.toml (lines 9–12) and brings in the expected DPP crates.
  • The crate unconditionally uses std (e.g. std::collections::BTreeMap, std::fmt in packages/simple-signer/src/signer.rs), so ensure your WASM target is std-enabled or links libstd.
  • add_key returns () (unit), not Result; adjust identityCreate’s error handling if needed.
packages/wasm-sdk/src/state_transitions/identity/mod.rs (5)

83-86: Good: deterministic key ordering and per-key signer.

BTreeMap + sequential ids yields deterministic ordering; initializing SimpleSigner here matches the per-key signing goal.


126-147: Validate per-key secret format early; error messages are good.

Length check at 32 bytes is correct for secp256k1 private keys; WIF parse path looks fine.


149-153: Derivation path is correct; ensure network is used where needed.

Using key_type.public_key_data_from_private_key_data(..., self.network()) is the right hook for HASH160 and others.


167-169: Confirm add_key is infallible; handle errors if not.

If SimpleSigner::add_key can fail (dup ids, unsupported types), bubble it up; otherwise this is fine.

If it returns Result, prefer:

signer.add_key(public_key.clone(), private_key_bytes)
    .map_err(|e| JsValue::from_str(&format!("Failed to add key to signer: {}", e)))?;

183-189: Right signer passed to put_to_platform...

Using the accumulated SimpleSigner fixes duplicate/incorrect signatures across key types.

packages/wasm-sdk/test/state-transitions.test.mjs (1)

195-248: HASH160-only keys test — OK; confirm intent.

Since AUTHENTICATION with HASH160 is typically not usable for signing, expecting failure here is reasonable. Just confirm this is intentional so the test’s purpose is clear.

thephez and others added 3 commits September 3, 2025 09:57
- Add keyType selector to identity creation interface with HASH160/SECP256K1 options
- Update api-definitions.json with new keyType input field and clearer descriptions
- Modify index.html to dynamically use selected key type for all generated keys
- Add real-time preview updates showing signature requirements per key type
- Enable testing of both traditional HASH160 behavior and SECP256K1 SimpleSigner fix

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <noreply@anthropic.com>
@thephez thephez changed the title fix(wasm-sdk): handle identity create transition signing for all types of keys fix(wasm-sdk)!: handle identity create transition signing for all types of keys Sep 3, 2025
@thephez
Copy link
Collaborator Author

thephez commented Sep 3, 2025

@coderabbitai review

Copy link
Contributor

coderabbitai bot commented Sep 3, 2025

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 6

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (3)
packages/wasm-sdk/index.html (2)

3078-3090: Await key derivations to avoid passing undefined privateKeyHex to identityCreate

derive_key_from_seed_with_path is async elsewhere in this file; here it’s used without await, so masterKey/authKey/transferKey are Promises. This yields undefined privateKeyHex and will cause identityCreate to reject each key (“Either privateKeyHex or privateKeyWif must be provided”). Also remove logs that would leak private key material.

Apply:

-            const masterKey = derive_key_from_seed_with_path(seedPhrase, undefined, masterKeyPath, currentNetwork);
-            console.log('Master key object:', masterKey);
-            console.log('Master key fields:', Object.keys(masterKey || {}));
+            const masterKey = await derive_key_from_seed_with_path(seedPhrase, undefined, masterKeyPath, currentNetwork);
...
-            const authKey = derive_key_from_seed_with_path(seedPhrase, undefined, authKeyPath, currentNetwork);
+            const authKey = await derive_key_from_seed_with_path(seedPhrase, undefined, authKeyPath, currentNetwork);
...
-            const transferKey = derive_key_from_seed_with_path(seedPhrase, undefined, transferKeyPath, currentNetwork);
+            const transferKey = await derive_key_from_seed_with_path(seedPhrase, undefined, transferKeyPath, currentNetwork);

4299-4351: Make updateKeyPreview async and include BLS in “Individual signatures required” notice

  • Same await issue as above; preview shows “undefined” without awaiting key derivations.
  • Per PR goals, both ECDSA_SECP256K1 and BLS12_381 require per-key signatures; the notice currently mentions only ECDSA_SECP256K1.

Apply:

-          const updateKeyPreview = () => {
+          const updateKeyPreview = async () => {
...
-                const masterKey = derive_key_from_seed_with_path(seedPhrase, undefined, masterKeyPath, currentNetwork);
+                const masterKey = await derive_key_from_seed_with_path(seedPhrase, undefined, masterKeyPath, currentNetwork);
...
-                const authKey = derive_key_from_seed_with_path(seedPhrase, undefined, authKeyPath, currentNetwork);
+                const authKey = await derive_key_from_seed_with_path(seedPhrase, undefined, authKeyPath, currentNetwork);
...
-                const transferKey = derive_key_from_seed_with_path(seedPhrase, undefined, transferKeyPath, currentNetwork);
+                const transferKey = await derive_key_from_seed_with_path(seedPhrase, undefined, transferKeyPath, currentNetwork);
...
-                const signatureInfo = selectedKeyType === 'ECDSA_SECP256K1' 
-                  ? ' (Individual signatures required)' 
-                  : ' (No individual signatures required)';
+                const signatureInfo =
+                  (selectedKeyType === 'ECDSA_SECP256K1' || selectedKeyType === 'BLS12_381')
+                    ? ' (Individual signatures required)'
+                    : ' (No individual signatures required)';
packages/wasm-sdk/src/state_transitions/identity/mod.rs (1)

52-57: Sanitize logs: public_keys contains privateKeyHex and should not be logged

Leaking per-key private material to the console is a security risk. Redact or remove.

Apply:

-        web_sys::console::log_1(&JsValue::from_str(&format!("  public_keys: {}", public_keys)));
+        web_sys::console::log_1(&JsValue::from_str("  public_keys: [REDACTED]"));
🧹 Nitpick comments (6)
packages/wasm-sdk/AI_REFERENCE.md (2)

774-784: Make the example note the WIF alternative for ECDSA keys.

Keep JSON as-is but clarify in the comment that ECDSA may use privateKeyHex or privateKeyWif; BLS uses privateKeyHex.

-    privateKeyHex: "d1a9...f5a6b7" // Private key for this public key
+    privateKeyHex: "d1a9...f5a6b7" // Private key for this public key (BLS: privateKeyHex only; ECDSA: privateKeyHex or privateKeyWif)

814-814: Nit: Use a consistent placeholder WIF across examples.

The WIF here differs slightly from the one on Line 772. Align them to prevent copy/paste confusion.

-const assetLockProofPrivateKey = "XFfpaSbZq52HPy3WWve1dXsZMiU1bQn8vQd34HNXkSZThevBWRn1";
+const assetLockProofPrivateKey = "XFfpaSbZq52HPy3WWwe1dXsZMiU1bQn8vQd34HNXkSZThevBWRn1";
packages/wasm-sdk/api-definitions.json (2)

1237-1254: Optional: Gate keyType selector by mode for cleaner UX.

Show Key Type only when keySelectionMode is "default" (or whichever mode uses it), to avoid redundancy in advanced flows.

             {
               "name": "keyType",
               "type": "select",
               "label": "Key Type",
               "required": true,
               "value": "ECDSA_HASH160",
+              "dependsOn": { "field": "keySelectionMode", "value": "default" },
               "options": [

1285-1285: Align example comment with accepted formats.

Mirror the above clarification inside the sdk_example to prevent mismatched guidance between UI text and example.

-    privateKeyHex: "d1a9...f5a6b7" // Private key for this public key
+    privateKeyHex: "d1a9...f5a6b7" // Private key for this public key (BLS: privateKeyHex only; ECDSA: privateKeyHex or privateKeyWif)
packages/wasm-sdk/docs.html (1)

1993-1999: Clarify non-signing key guidance

Good call switching example to ECDSA_HASH160. Strengthen the note to explicitly forbid privateKeyHex/privateKeyWif for this type.

Apply:

-    // Note: ECDSA_HASH160 keys don't need privateKeyHex as they don't get individual signatures
+    // Note: ECDSA_HASH160 keys do not get individual signatures; do not include privateKeyHex/privateKeyWif
packages/wasm-sdk/src/state_transitions/identity/mod.rs (1)

134-161: Optional: allow ECDSA_HASH160 keys without private keys (accept precomputed data and skip signer)

If you want to match “ECDSA_HASH160 receive empty signatures,” permit missing privateKey for that type by accepting data (base64) and not adding it to the signer. Keeps current behavior for signable types.

-            // Get private key - required for signing each key
-            let private_key_bytes = if let Some(private_key_hex) = key_data["privateKeyHex"].as_str() {
+            // Resolve key material
+            // For ECDSA_SECP256K1 and BLS12_381: require private key (hex or WIF) to sign.
+            // For ECDSA_HASH160: prefer private key, otherwise accept precomputed `data` (base64) and skip signer.
+            let (private_key_bytes_opt, public_key_data) = if let Some(private_key_hex) = key_data["privateKeyHex"].as_str() {
                 // Decode private key from hex
-                let bytes = hex::decode(private_key_hex)
+                let bytes = hex::decode(private_key_hex)
                     .map_err(|e| JsValue::from_str(&format!("Invalid private key hex: {}", e)))?;
-                
-                if bytes.len() != 32 {
-                    return Err(JsValue::from_str(&format!("Private key must be 32 bytes, got {}", bytes.len())));
-                }
-                
-                let mut private_key_array = [0u8; 32];
-                private_key_array.copy_from_slice(&bytes);
-                private_key_array
+                if bytes.len() != 32 {
+                    return Err(JsValue::from_str(&format!("Private key must be 32 bytes, got {}", bytes.len())));
+                }
+                let mut private_key_array = [0u8; 32];
+                private_key_array.copy_from_slice(&bytes);
+                let pub_data = key_type.public_key_data_from_private_key_data(&private_key_array, self.network())
+                    .map_err(|e| JsValue::from_str(&format!("Failed to derive public key data: {}", e)))?;
+                (Some(private_key_array), pub_data)
             } else if let Some(private_key_wif) = key_data["privateKeyWif"].as_str() {
-                // Parse WIF format private key
-                let private_key = PrivateKey::from_wif(private_key_wif)
-                    .map_err(|e| JsValue::from_str(&format!("Invalid WIF private key: {}", e)))?;
-                private_key.inner.secret_bytes()
+                let private_key = PrivateKey::from_wif(private_key_wif)
+                    .map_err(|e| JsValue::from_str(&format!("Invalid WIF private key: {}", e)))?;
+                let pk = private_key.inner.secret_bytes();
+                let pub_data = key_type.public_key_data_from_private_key_data(&pk, self.network())
+                    .map_err(|e| JsValue::from_str(&format!("Failed to derive public key data: {}", e)))?;
+                (Some(pk), pub_data)
+            } else if matches!(key_type, KeyType::ECDSA_HASH160) {
+                // Accept base64 `data` for hash160 keys and skip signer
+                let data_b64 = key_data["data"].as_str()
+                    .ok_or_else(|| JsValue::from_str("ECDSA_HASH160 key requires either privateKeyHex/privateKeyWif or base64 `data`"))?;
+                let pub_data = dash_sdk::dpp::dashcore::base64::decode(data_b64)
+                    .map_err(|e| JsValue::from_str(&format!("Invalid base64 key data: {}", e)))?;
+                (None, pub_data)
             } else {
-                return Err(JsValue::from_str("Either privateKeyHex or privateKeyWif must be provided for each key"));
+                return Err(JsValue::from_str("Either privateKeyHex/privateKeyWif must be provided for each key (ECDSA_HASH160 may use base64 `data`)"));
             };
-            
-            // Derive public key data from private key
-            let public_key_data = key_type.public_key_data_from_private_key_data(
-                &private_key_bytes,
-                self.network()
-            ).map_err(|e| JsValue::from_str(&format!("Failed to derive public key data: {}", e)))?;
+

And only add signable keys to the signer:

-            // Add the public key and its private key to the signer
-            signer.add_key(public_key.clone(), private_key_bytes);
+            // Add to signer only for keys that require signatures and have private material
+            if private_key_bytes_opt.is_some() && matches!(key_type, KeyType::ECDSA_SECP256K1 | KeyType::BLS12_381) {
+                signer.add_key(public_key.clone(), private_key_bytes_opt.unwrap());
+            }

Also applies to: 175-177

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

💡 Knowledge Base configuration:

  • MCP integration is disabled by default for public repositories
  • Jira integration is disabled by default for public repositories
  • Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between 4bfbec7 and 315d1bf.

📒 Files selected for processing (6)
  • packages/wasm-sdk/AI_REFERENCE.md (1 hunks)
  • packages/wasm-sdk/api-definitions.json (2 hunks)
  • packages/wasm-sdk/docs.html (2 hunks)
  • packages/wasm-sdk/docs_manifest.json (1 hunks)
  • packages/wasm-sdk/index.html (5 hunks)
  • packages/wasm-sdk/src/state_transitions/identity/mod.rs (6 hunks)
✅ Files skipped from review due to trivial changes (1)
  • packages/wasm-sdk/docs_manifest.json
🧰 Additional context used
🧠 Learnings (6)
📓 Common learnings
Learnt from: QuantumExplorer
PR: dashpay/platform#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-07-28T20:00:08.502Z
Learnt from: QuantumExplorer
PR: dashpay/platform#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/index.html
  • packages/wasm-sdk/AI_REFERENCE.md
  • packages/wasm-sdk/docs.html
  • packages/wasm-sdk/api-definitions.json
  • packages/wasm-sdk/src/state_transitions/identity/mod.rs
📚 Learning: 2025-07-28T20:00:24.323Z
Learnt from: QuantumExplorer
PR: dashpay/platform#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/wasm-sdk/index.html
  • packages/wasm-sdk/AI_REFERENCE.md
  • packages/wasm-sdk/docs.html
  • packages/wasm-sdk/api-definitions.json
📚 Learning: 2025-07-23T08:31:42.268Z
Learnt from: CR
PR: dashpay/platform#0
File: packages/wasm-sdk/CLAUDE.md:0-0
Timestamp: 2025-07-23T08:31:42.268Z
Learning: Applies to packages/wasm-sdk/src/**/*.rs : When implementing WASM SDK functionality, always refer to AI_REFERENCE.md first for accurate method signatures and examples.

Applied to files:

  • packages/wasm-sdk/AI_REFERENCE.md
  • packages/wasm-sdk/docs.html
📚 Learning: 2024-10-09T00:22:57.778Z
Learnt from: QuantumExplorer
PR: dashpay/platform#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: 2024-11-20T20:43:41.185Z
Learnt from: QuantumExplorer
PR: dashpay/platform#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
🪛 Gitleaks (8.27.2)
packages/wasm-sdk/docs.html

1989-1989: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.

(generic-api-key)

🔇 Additional comments (8)
packages/wasm-sdk/AI_REFERENCE.md (2)

787-793: Good: HASH160 key is documented as non-signing.

The switch to type: 2 and the explanatory note accurately reflect that ECDSA_HASH160 keys do not carry per-key signatures in identity create.


766-784: Include per-key privateKeyWif in publicKeys documentation
Update packages/wasm-sdk/AI_REFERENCE.md (around lines 766–784) to state that each ECDSA_SECP256K1 and BLS12_381 key may specify either privateKeyHex or privateKeyWif, matching the implementation in src/state_transitions/identity/mod.rs.

⛔ Skipped due to learnings
Learnt from: QuantumExplorer
PR: dashpay/platform#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.
Learnt from: QuantumExplorer
PR: dashpay/platform#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.
Learnt from: QuantumExplorer
PR: dashpay/platform#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.
Learnt from: CR
PR: dashpay/platform#0
File: packages/wasm-sdk/CLAUDE.md:0-0
Timestamp: 2025-07-23T08:31:42.268Z
Learning: Applies to packages/wasm-sdk/src/**/*.rs : When implementing WASM SDK functionality, always refer to AI_REFERENCE.md first for accurate method signatures and examples.
packages/wasm-sdk/api-definitions.json (1)

1237-1254: Per-key private key format phrasing is consistent. Descriptions and examples in AI_REFERENCE.md and api-definitions.json match exactly.

packages/wasm-sdk/index.html (1)

4366-4375: Wire the new async preview correctly

Listeners are fine; no changes required beyond making updateKeyPreview async. LGTM once the async change lands.

packages/wasm-sdk/src/state_transitions/identity/mod.rs (4)

12-12: Importing SimpleSigner alongside SingleKeySigner is appropriate

This enables per-key signing for identity create while preserving SingleKeySigner for other transitions. LGTM.


91-98: Using SimpleSigner for per-key signing aligns with PR objective

Creating a single signer instance to collect keys before broadcasting is correct.


191-193: Comment-only lines: good context

These comments clarify SimpleSigner usage. LGTM.


538-541: No action needed: Purpose::OWNER is valid for withdrawals
Purpose::OWNER is defined in the Purpose enum and intentionally allowed alongside TRANSFER for identity credit withdrawals—this aligns with rs-dpp’s v1 purpose_requirement (which returns both TRANSFER and OWNER) and is already referenced in the file’s fallback clause.

Update code comments / docs for consistency
Incorrect field name was used in an example
@thephez thephez marked this pull request as ready for review September 3, 2025 15:43
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)
packages/wasm-sdk/src/state_transitions/identity/mod.rs (2)

53-58: Do not log private key material (PII/secret leakage)

Full public_keys JSON is printed to console, exposing privateKeyHex/Wif. Sanitize or remove.

-        // Debug log all parameters
-        web_sys::console::log_1(&JsValue::from_str(&format!("identityCreate called with:")));
-        web_sys::console::log_1(&JsValue::from_str(&format!("  asset_lock_proof (length {}): {}", asset_lock_proof.len(), if asset_lock_proof.len() > 100 { format!("{}...", &asset_lock_proof[..100]) } else { asset_lock_proof.clone() })));
-        web_sys::console::log_1(&JsValue::from_str(&format!("  asset_lock_proof_private_key: [REDACTED] (length: {})", asset_lock_proof_private_key.len())));
-        web_sys::console::log_1(&JsValue::from_str(&format!("  public_keys: {}", public_keys)));
+        // Debug log with redaction
+        web_sys::console::log_1(&JsValue::from_str("identityCreate called"));
+        web_sys::console::log_1(&JsValue::from_str(&format!(
+            "  asset_lock_proof (length {}): {}",
+            asset_lock_proof.len(),
+            if asset_lock_proof.len() > 100 { format!("{}...", &asset_lock_proof[..100]) } else { "[short]".to_string() }
+        )));
+        web_sys::console::log_1(&JsValue::from_str(&format!(
+            "  asset_lock_proof_private_key: [REDACTED] (length: {})",
+            asset_lock_proof_private_key.len()
+        )));
+        let sanitized = serde_json::from_str::<serde_json::Value>(&public_keys).ok().map(|mut v| {
+            if let Some(arr) = v.as_array_mut() {
+                for e in arr {
+                    if let Some(obj) = e.as_object_mut() {
+                        obj.remove("privateKeyHex");
+                        obj.remove("privateKeyWif");
+                    }
+                }
+            }
+            v
+        }).map(|v| v.to_string()).unwrap_or_else(|| "[invalid json]".into());
+        web_sys::console::log_1(&JsValue::from_str(&format!("  public_keys (sanitized): {}", sanitized)));

98-134: Accept both string and numeric enums for keyType/purpose/securityLevel

Docs/UI examples use numeric values; current code only accepts strings and will reject valid inputs.

-            let key_type_str = key_data["keyType"].as_str()
-                .ok_or_else(|| JsValue::from_str("keyType is required"))?;
+            let key_type = match &key_data["keyType"] {
+                serde_json::Value::String(s) => match s.as_str() {
+                    "ECDSA_SECP256K1" => KeyType::ECDSA_SECP256K1,
+                    "BLS12_381" => KeyType::BLS12_381,
+                    "ECDSA_HASH160" => KeyType::ECDSA_HASH160,
+                    "BIP13_SCRIPT_HASH" => KeyType::BIP13_SCRIPT_HASH,
+                    "EDDSA_25519_HASH160" => KeyType::EDDSA_25519_HASH160,
+                    _ => return Err(JsValue::from_str(&format!("Unknown key type: {}", s))),
+                },
+                serde_json::Value::Number(n) => match n.as_u64().unwrap_or(u64::MAX) {
+                    0 => KeyType::ECDSA_SECP256K1,
+                    1 => KeyType::BLS12_381,
+                    2 => KeyType::ECDSA_HASH160,
+                    3 => KeyType::BIP13_SCRIPT_HASH,
+                    4 => KeyType::EDDSA_25519_HASH160,
+                    _ => return Err(JsValue::from_str(&format!("Unknown key type id: {}", n))),
+                },
+                _ => return Err(JsValue::from_str("keyType is required")),
+            };
-            let purpose_str = key_data["purpose"].as_str()
-                .ok_or_else(|| JsValue::from_str("purpose is required"))?;
+            let purpose = match &key_data["purpose"] {
+                serde_json::Value::String(s) => match s.as_str() {
+                    "AUTHENTICATION" => Purpose::AUTHENTICATION,
+                    "ENCRYPTION" => Purpose::ENCRYPTION,
+                    "DECRYPTION" => Purpose::DECRYPTION,
+                    "TRANSFER" => Purpose::TRANSFER,
+                    "SYSTEM" => Purpose::SYSTEM,
+                    "VOTING" => Purpose::VOTING,
+                    _ => return Err(JsValue::from_str(&format!("Unknown purpose: {}", s))),
+                },
+                serde_json::Value::Number(n) => match n.as_u64().unwrap_or(u64::MAX) {
+                    0 => Purpose::AUTHENTICATION,
+                    1 => Purpose::ENCRYPTION,
+                    2 => Purpose::DECRYPTION,
+                    3 => Purpose::TRANSFER,
+                    5 => Purpose::VOTING,
+                    _ => return Err(JsValue::from_str(&format!("Unknown purpose id: {}", n))),
+                },
+                _ => return Err(JsValue::from_str("purpose is required")),
+            };
-            let security_level_str = key_data["securityLevel"].as_str()
-                .unwrap_or("HIGH");
-            let security_level = match security_level_str {
-                "MASTER" => SecurityLevel::MASTER,
-                "CRITICAL" => SecurityLevel::CRITICAL,
-                "HIGH" => SecurityLevel::HIGH,
-                "MEDIUM" => SecurityLevel::MEDIUM,
-                _ => SecurityLevel::HIGH
-            };
+            let security_level = match &key_data["securityLevel"] {
+                serde_json::Value::String(s) => match s.as_str() {
+                    "MASTER" => SecurityLevel::MASTER,
+                    "CRITICAL" => SecurityLevel::CRITICAL,
+                    "HIGH" => SecurityLevel::HIGH,
+                    "MEDIUM" => SecurityLevel::MEDIUM,
+                    _ => SecurityLevel::HIGH,
+                },
+                serde_json::Value::Number(n) => match n.as_u64().unwrap_or(2) {
+                    0 => SecurityLevel::MASTER,
+                    1 => SecurityLevel::CRITICAL,
+                    2 => SecurityLevel::HIGH,
+                    3 => SecurityLevel::MEDIUM,
+                    _ => SecurityLevel::HIGH,
+                },
+                _ => SecurityLevel::HIGH,
+            };
♻️ Duplicate comments (2)
packages/wasm-sdk/docs.html (2)

1989-1999: Secret-scanner noise acknowledged

Realistic-looking placeholders trip gitleaks. Per maintainer preference, keep them; consider adding an inline “not real” comment.


1970-1970: Auto-generated: update description in api-definitions.json

This text comes from api-definitions.json. Apply the description fix there (see suggested diff in api-definitions.json).

🧹 Nitpick comments (3)
packages/wasm-sdk/api-definitions.json (1)

1237-1254: Show Key Type only when Advanced is selected

Avoid confusing default users. Gate the Key Type selector on keySelectionMode=advanced.

             {
               "name": "keyType",
               "type": "select",
               "label": "Key Type",
               "required": true,
               "value": "ECDSA_HASH160",
+              "dependsOn": { "field": "keySelectionMode", "value": "advanced" },
               "options": [
packages/wasm-sdk/src/state_transitions/identity/mod.rs (2)

30-40: Doc comment OK; add accepted formats note for HASH160

State that HASH160 accepts privateKeyHex or privateKeyWif (still produces empty signatures).

-    ///   - ECDSA_HASH160 keys: Used to derive the public key hash (produces empty signatures)
+    ///   - ECDSA_HASH160 keys: Used to derive the public key hash (produces empty signatures). Accepts privateKeyHex or privateKeyWif.

176-178: Don’t add HASH160 keys to signer

HASH160 signatures must be empty; avoid adding them to the signer to prevent accidental signing.

-            // Add the public key and its private key to the signer
-            signer.add_key(public_key.clone(), private_key_bytes);
+            // Add only signing key types to the signer
+            if !matches!(key_type, KeyType::ECDSA_HASH160) {
+                signer.add_key(public_key.clone(), private_key_bytes);
+            }

Please confirm SimpleSigner already ignores non-signing key types; if so, this is optional.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

💡 Knowledge Base configuration:

  • MCP integration is disabled by default for public repositories
  • Jira integration is disabled by default for public repositories
  • Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between 315d1bf and 1292741.

📒 Files selected for processing (5)
  • packages/wasm-sdk/AI_REFERENCE.md (1 hunks)
  • packages/wasm-sdk/api-definitions.json (2 hunks)
  • packages/wasm-sdk/docs.html (2 hunks)
  • packages/wasm-sdk/docs_manifest.json (1 hunks)
  • packages/wasm-sdk/src/state_transitions/identity/mod.rs (6 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
  • packages/wasm-sdk/docs_manifest.json
  • packages/wasm-sdk/AI_REFERENCE.md
🧰 Additional context used
📓 Path-based instructions (1)
packages/wasm-sdk/src/**/*.rs

📄 CodeRabbit inference engine (packages/wasm-sdk/CLAUDE.md)

packages/wasm-sdk/src/**/*.rs: When implementing WASM SDK functionality, always refer to AI_REFERENCE.md first for accurate method signatures and examples.
For WASM builds, fix 'time not implemented on this platform' errors by using js_sys::Date::now().
Token functions are methods on WasmSdk, not standalone functions; avoid importing them as standalone.
The WASM SDK now fully supports where and orderBy clauses for document queries; use the specified JSON array formats and supported operators.

Files:

  • packages/wasm-sdk/src/state_transitions/identity/mod.rs
🧠 Learnings (11)
📓 Common learnings
Learnt from: QuantumExplorer
PR: dashpay/platform#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: 2024-10-09T00:22:57.778Z
Learnt from: QuantumExplorer
PR: dashpay/platform#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: 2024-11-20T20:43:41.185Z
Learnt from: QuantumExplorer
PR: dashpay/platform#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
  • packages/wasm-sdk/docs.html
📚 Learning: 2025-07-28T20:00:08.502Z
Learnt from: QuantumExplorer
PR: dashpay/platform#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
  • packages/wasm-sdk/api-definitions.json
  • packages/wasm-sdk/docs.html
📚 Learning: 2025-07-28T20:00:24.323Z
Learnt from: QuantumExplorer
PR: dashpay/platform#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/wasm-sdk/src/state_transitions/identity/mod.rs
  • packages/wasm-sdk/api-definitions.json
  • packages/wasm-sdk/docs.html
📚 Learning: 2024-11-20T16:05:40.200Z
Learnt from: QuantumExplorer
PR: dashpay/platform#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: 2025-09-03T15:44:33.860Z
Learnt from: thephez
PR: dashpay/platform#2754
File: packages/wasm-sdk/docs.html:0-0
Timestamp: 2025-09-03T15:44:33.860Z
Learning: In packages/wasm-sdk/docs.html, thephez prefers to keep realistic-looking private key placeholders in documentation examples rather than using obvious fake placeholders, despite secret scanner warnings.

Applied to files:

  • packages/wasm-sdk/src/state_transitions/identity/mod.rs
  • packages/wasm-sdk/api-definitions.json
  • packages/wasm-sdk/docs.html
📚 Learning: 2025-09-03T14:42:29.907Z
Learnt from: thephez
PR: dashpay/platform#2754
File: packages/wasm-sdk/docs.html:1970-1971
Timestamp: 2025-09-03T14:42:29.907Z
Learning: In packages/wasm-sdk/, the docs.html file is auto-generated from api-definitions.json. Any documentation fixes should be made in api-definitions.json rather than directly in docs.html, as manual changes to docs.html would be overwritten during regeneration.

Applied to files:

  • packages/wasm-sdk/docs.html
📚 Learning: 2025-09-03T14:41:16.119Z
Learnt from: thephez
PR: dashpay/platform#2754
File: packages/wasm-sdk/AI_REFERENCE.md:766-766
Timestamp: 2025-09-03T14:41:16.119Z
Learning: In packages/wasm-sdk/, the AI_REFERENCE.md file is auto-generated from api-definitions.json. Any documentation fixes should be made in api-definitions.json rather than directly in AI_REFERENCE.md, as manual changes to AI_REFERENCE.md would be overwritten during regeneration.

Applied to files:

  • packages/wasm-sdk/docs.html
📚 Learning: 2025-07-23T08:31:42.268Z
Learnt from: CR
PR: dashpay/platform#0
File: packages/wasm-sdk/CLAUDE.md:0-0
Timestamp: 2025-07-23T08:31:42.268Z
Learning: Applies to packages/wasm-sdk/**/index.html : The WASM SDK now fully supports where and orderBy clauses for document queries; use the specified JSON array formats and supported operators.

Applied to files:

  • packages/wasm-sdk/docs.html
📚 Learning: 2025-06-18T03:44:14.385Z
Learnt from: QuantumExplorer
PR: dashpay/platform#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/docs.html
🧬 Code graph analysis (1)
packages/wasm-sdk/src/state_transitions/identity/mod.rs (1)
packages/simple-signer/src/single_key_signer.rs (2)
  • new (18-22)
  • private_key (59-61)
🪛 Gitleaks (8.27.2)
packages/wasm-sdk/docs.html

1989-1989: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.

(generic-api-key)


1998-1998: 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). (2)
  • GitHub Check: Build JS packages / Build JS
  • GitHub Check: build-wasm-sdk
🔇 Additional comments (2)
packages/wasm-sdk/src/state_transitions/identity/mod.rs (2)

197-207: Good: switch to SimpleSigner for per-key signatures

Using a single SimpleSigner with per-key material is aligned with DPP validation.


533-562: Ignore Purpose::OWNER concern Purpose::OWNER is a valid variant in the Purpose enum (defined in rs-dpp/src/identity/identity_public_key/purpose.rs) and is intentionally used across SDK and DPP code for owner‐key workflows, including withdrawal logic.

Likely an incorrect or invalid review comment.

…ASH160 keys

Allow ECDSA_HASH160 keys to be provided either as:
- privateKeyHex: derive the 20-byte hash from the private key
 - data: direct base64-encoded 20-byte public key hash

This maintains backward compatibility while addressing the review feedback that ECDSA_HASH160 keys don't require private keys for cryptographic operations.
QuantumExplorer
QuantumExplorer previously approved these changes Sep 3, 2025
Copy link
Contributor

@coderabbitai coderabbitai bot left a 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

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (4)
packages/wasm-sdk/src/state_transitions/identity/mod.rs (4)

52-57: Stop logging secrets: redact public_keys input

public_keys likely contains privateKeyHex/privateKeyWif. Printing it to the console is a hard secret leak.

Apply:

-        web_sys::console::log_1(&JsValue::from_str(&format!("  public_keys: {}", public_keys)));
+        // Do not log raw public_keys (may contain private material)
+        web_sys::console::log_1(&JsValue::from_str(&format!(
+            "  public_keys: [REDACTED] ({} bytes)",
+            public_keys.len()
+        )));

312-328: Same asset_lock_proof parsing bug in identity_top_up

Duplicate of earlier issue: tx-hex is treated as UTF‑8 JSON after hex decode.

Apply the same simplification to JSON-only parsing (or add proper tx-hex handling) as proposed for identity_create.

-        let asset_lock_proof: AssetLockProof = if asset_lock_proof.chars().all(|c| c.is_ascii_hexdigit()) {
-            ...
-        } else {
-            serde_json::from_str(&asset_lock_proof)
-                .map_err(|e| JsValue::from_str(&format!("Invalid asset lock proof JSON: {}", e)))?
-        };
+        let asset_lock_proof: AssetLockProof = serde_json::from_str(&asset_lock_proof)
+            .map_err(|e| JsValue::from_str(&format!("Invalid asset lock proof JSON: {}", e)))?;

709-724: Master key detection uses HASH160; should match ECDSA_SECP256K1 bytes

For identityUpdate, the master authentication key is ECDSA_SECP256K1. Comparing HASH160 to identity key data is incorrect; compare the raw compressed pubkey bytes to ECDSA_SECP256K1 key data.

Apply:

-        // Create public key hash using hash160
-        let public_key_hash160 = {
-            use dash_sdk::dpp::dashcore::hashes::{Hash, hash160};
-            hash160::Hash::hash(&public_key_bytes[..]).to_byte_array().to_vec()
-        };
-
-        // Find matching master key
-        let master_key = identity.public_keys().iter()
-            .find(|(_, key)| {
-                key.purpose() == Purpose::AUTHENTICATION &&
-                key.security_level() == SecurityLevel::MASTER &&
-                key.key_type() == KeyType::ECDSA_HASH160 &&
-                key.data().as_slice() == public_key_hash160.as_slice()
-            })
-            .map(|(id, _)| *id)
-            .ok_or_else(|| JsValue::from_str("Provided private key does not match any master key"))?;
+        // Find matching master ECDSA_SECP256K1 auth key by raw compressed pubkey bytes
+        let master_key = identity.public_keys().iter()
+            .find(|(_, key)| {
+                key.purpose() == Purpose::AUTHENTICATION &&
+                key.security_level() == SecurityLevel::MASTER &&
+                key.key_type() == KeyType::ECDSA_SECP256K1 &&
+                key.data().as_slice() == public_key_bytes.as_slice()
+            })
+            .map(|(id, _)| *id)
+            .ok_or_else(|| JsValue::from_str("Provided private key does not match any master ECDSA_SECP256K1 authentication key"))?;

1045-1047: Signer creation fails when hex private key is supplied

You accept hex or WIF above, but still pass the original string to SingleKeySigner (expects WIF). Use the parsed PrivateKey to construct the signer so both paths work.

Apply:

-        let signer = SingleKeySigner::from_string(&voting_key_wif, self.network())
+        let signer = SingleKeySigner::from_string(&private_key.to_wif(), self.network())
             .map_err(|e| JsValue::from_str(&e))?;
♻️ Duplicate comments (1)
packages/wasm-sdk/src/state_transitions/identity/mod.rs (1)

184-205: Reject WIF for BLS12_381 keys

WIF applies to ECDSA_SECP256K1 only. Current code accepts WIF for BLS and will produce invalid key material.

Apply:

-                    } else if let Some(private_key_wif) = key_data["privateKeyWif"].as_str() {
+                    } else if let Some(private_key_wif) = key_data["privateKeyWif"].as_str() {
+                        if key_type == KeyType::BLS12_381 {
+                            return Err(JsValue::from_str("BLS12_381 keys require privateKeyHex; WIF is not supported"));
+                        }
                         // Parse WIF format private key
                         let private_key = PrivateKey::from_wif(private_key_wif)
                             .map_err(|e| JsValue::from_str(&format!("Invalid WIF private key: {}", e)))?;
                         private_key.inner.secret_bytes()
🧹 Nitpick comments (1)
packages/wasm-sdk/src/state_transitions/identity/mod.rs (1)

740-782: Validate data length per key type in add_public_keys

Add fast sanity checks to catch malformed key data before building the transition (33/48/20 byte expectations).

Apply after base64 decode:

                     let key_data = dash_sdk::dpp::dashcore::base64::decode(data_str)
                         .map_err(|e| JsValue::from_str(&format!("Invalid base64 key data: {}", e)))?;
+                    // Length checks per key type
+                    match key_type {
+                        KeyType::ECDSA_SECP256K1 if key_data.len() != 33 =>
+                            return Err(JsValue::from_str("ECDSA_SECP256K1 key data must be 33 bytes (compressed pubkey)")),
+                        KeyType::BLS12_381 if key_data.len() != 48 =>
+                            return Err(JsValue::from_str("BLS12_381 key data must be 48 bytes")),
+                        KeyType::ECDSA_HASH160 | KeyType::BIP13_SCRIPT_HASH | KeyType::EDDSA_25519_HASH160
+                            if key_data.len() != 20 =>
+                            return Err(JsValue::from_str(&format!("{} key data must be 20 bytes", key_type_str))),
+                        _ => {}
+                    }
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

💡 Knowledge Base configuration:

  • MCP integration is disabled by default for public repositories
  • Jira integration is disabled by default for public repositories
  • Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between 1292741 and 2136b00.

📒 Files selected for processing (6)
  • packages/wasm-sdk/AI_REFERENCE.md (1 hunks)
  • packages/wasm-sdk/api-definitions.json (2 hunks)
  • packages/wasm-sdk/docs.html (2 hunks)
  • packages/wasm-sdk/docs_manifest.json (1 hunks)
  • packages/wasm-sdk/index.html (5 hunks)
  • packages/wasm-sdk/src/state_transitions/identity/mod.rs (6 hunks)
✅ Files skipped from review due to trivial changes (1)
  • packages/wasm-sdk/docs_manifest.json
🚧 Files skipped from review as they are similar to previous changes (4)
  • packages/wasm-sdk/AI_REFERENCE.md
  • packages/wasm-sdk/docs.html
  • packages/wasm-sdk/index.html
  • packages/wasm-sdk/api-definitions.json
🧰 Additional context used
📓 Path-based instructions (1)
packages/wasm-sdk/src/**/*.rs

📄 CodeRabbit inference engine (packages/wasm-sdk/CLAUDE.md)

packages/wasm-sdk/src/**/*.rs: When implementing WASM SDK functionality, always refer to AI_REFERENCE.md first for accurate method signatures and examples.
For WASM builds, fix 'time not implemented on this platform' errors by using js_sys::Date::now().
Token functions are methods on WasmSdk, not standalone functions; avoid importing them as standalone.
The WASM SDK now fully supports where and orderBy clauses for document queries; use the specified JSON array formats and supported operators.

Files:

  • packages/wasm-sdk/src/state_transitions/identity/mod.rs
🧠 Learnings (8)
📓 Common learnings
Learnt from: QuantumExplorer
PR: dashpay/platform#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-07-28T20:00:08.502Z
Learnt from: QuantumExplorer
PR: dashpay/platform#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
PR: dashpay/platform#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-07-28T20:00:24.323Z
Learnt from: QuantumExplorer
PR: dashpay/platform#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/wasm-sdk/src/state_transitions/identity/mod.rs
📚 Learning: 2025-09-03T15:44:33.860Z
Learnt from: thephez
PR: dashpay/platform#2754
File: packages/wasm-sdk/docs.html:0-0
Timestamp: 2025-09-03T15:44:33.860Z
Learning: In packages/wasm-sdk/docs.html, thephez prefers to keep realistic-looking private key placeholders in documentation examples rather than using obvious fake placeholders, despite secret scanner warnings.

Applied to files:

  • packages/wasm-sdk/src/state_transitions/identity/mod.rs
📚 Learning: 2024-11-20T16:05:40.200Z
Learnt from: QuantumExplorer
PR: dashpay/platform#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: 2025-06-18T03:44:14.385Z
Learnt from: QuantumExplorer
PR: dashpay/platform#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
PR: dashpay/platform#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
⏰ 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). (2)
  • GitHub Check: Build JS packages / Build JS
  • GitHub Check: build-wasm-sdk
🔇 Additional comments (3)
packages/wasm-sdk/src/state_transitions/identity/mod.rs (3)

243-249: Identity ID generation: verify derivation is not ignored

Using Identifier::random() may diverge from DPP’s asset-lock–derived identity ID. If PutIdentity overwrites/derives the ID internally, fine; otherwise this will cause rejection.

Please confirm PutIdentity derives the ID from asset_lock_proof and ignores the provided id; if not, compute the derived ID here instead of random.


30-38: Align docs with AI_REFERENCE.md and UI

Doc says “transaction hex” for asset_lock_proof and details per-key requirements. Ensure this matches AI_REFERENCE.md examples and the UI/form inputs to avoid user confusion.

Do you want me to cross-check AI_REFERENCE.md and propose doc updates in-place?


58-74: Clarify and handle asset_lock_proof formats
The current hex branch decodes any hex string into UTF-8 JSON, which will error on raw tx-hex inputs. Either restrict inputs to JSON-only (removing the hex path) or implement a proper tx-hex→AssetLockProof parser (packages/wasm-sdk/src/state_transitions/identity/mod.rs:58–74). Confirm the supported format in AI_REFERENCE.md and update code/comments to match.

thephez and others added 2 commits September 3, 2025 15:22
…ld names

- Change keyType/purpose/securityLevel from numeric to string enums
- Fix privateKeyHex to privateKeyWif for ECDSA keys
- Update placeholder values to be more realistic
- Regenerate documentation from updated api-definitions.json

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <noreply@anthropic.com>
- Separate ECDSA_SECP256K1 and BLS12_381 key handling logic
- BLS12_381 keys now only accept privateKeyHex format
- Return clear error when WIF is provided for BLS12_381 keys
- Maintain ECDSA support for both privateKeyHex and privateKeyWif
- Add key-type specific error messages for better debugging

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <noreply@anthropic.com>
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
packages/wasm-sdk/src/state_transitions/identity/mod.rs (1)

52-57: Redact per-key private material from logs.

public_keys may contain privateKeyHex/privateKeyWif. Logging it leaks secrets to browser console.

Apply:

-        web_sys::console::log_1(&JsValue::from_str(&format!("  public_keys: {}", public_keys)));
+        let sanitized_public_keys = match serde_json::from_str::<serde_json::Value>(&public_keys) {
+            Ok(serde_json::Value::Array(mut arr)) => {
+                for v in &mut arr {
+                    if let serde_json::Value::Object(map) = v {
+                        if map.contains_key("privateKeyHex") { map.insert("privateKeyHex".into(), serde_json::Value::String("[REDACTED]".into())); }
+                        if map.contains_key("privateKeyWif") { map.insert("privateKeyWif".into(), serde_json::Value::String("[REDACTED]".into())); }
+                    }
+                }
+                serde_json::to_string(&arr).unwrap_or("[unserializable]".into())
+            },
+            _ => "[unparseable public_keys]".into(),
+        };
+        web_sys::console::log_1(&JsValue::from_str(&format!("  public_keys: {}", sanitized_public_keys)));
♻️ Duplicate comments (3)
packages/wasm-sdk/AI_REFERENCE.md (2)

766-767: Keep HASH160/signing text consistent with api-definitions; regenerate.

Mirror the clarified wording from api-definitions (HASH160 signatures always empty; privateKeyHex only derives the hash).

Note: This file is auto-generated; apply fixes in api-definitions.json and regenerate.


774-793: Fix HASH160 example data to valid base64; align with api-definitions.

Replace the non-base64 placeholder with a valid 20-byte base64 string.

Apply in api-definitions.json so it propagates here:

-    data: "ripemd160hash20bytes1234", // Base64-encoded 20-byte RIPEMD160 hash
+    data: "AAAAAAAAAAAAAAAAAAAAAAAAAAA=", // Base64-encoded 20-byte RIPEMD160 hash
packages/wasm-sdk/docs.html (1)

1980-1999: Tighten example comments; mark placeholders and base64 requirement

Keep realistic-looking placeholders as preferred, but make the format expectations explicit to avoid copy/paste confusion and false bug reports.

   {
     id: 0,
     keyType: "ECDSA_SECP256K1",
     purpose: "AUTHENTICATION",
     securityLevel: "MASTER",
     data: "A5GzYHPIolbHkFrp5l+s9IvF2lWMuuuSu3oWZB8vWHNJ", // Base64-encoded public key
-    readOnly: false,
-    privateKeyWif: "XBrZJKcW4ajWVNAU6yP87WQog6CjFnpbqyAKgNTZRqmhYvPgMNV2"
+    readOnly: false,
+    // Use either privateKeyHex OR privateKeyWif (not both). Example only.
+    privateKeyWif: "XBrZJKcW4ajWVNAU6yP87WQog6CjFnpbqyAKgNTZRqmhYvPgMNV2" // example placeholder
   },
   {
     id: 1,
     keyType: "ECDSA_HASH160",
     purpose: "AUTHENTICATION",
     securityLevel: "HIGH",
-    data: "ripemd160hash20bytes1234", // Base64-encoded 20-byte RIPEMD160 hash
+    data: "ripemd160hash20bytes1234", // Placeholder: must be base64-encoded 20-byte RIPEMD160 hash
     readOnly: false,
     // ECDSA_HASH160 keys produce empty signatures (not required/used for signing)
   }
🧹 Nitpick comments (6)
packages/wasm-sdk/api-definitions.json (2)

1237-1254: Expose BLS key type in UI (optional).

Since identityCreate supports BLS12_381 in publicKeys, consider adding it to the Key Type selector for parity and easier testing.

Apply:

             "options": [
               {
                 "value": "ECDSA_HASH160",
                 "label": "ECDSA_HASH160 (Dash Evo Tool default)"
               },
               {
                 "value": "ECDSA_SECP256K1",
                 "label": "ECDSA_SECP256K1 (Dash mobile wallet default)"
+              },
+              {
+                "value": "BLS12_381",
+                "label": "BLS12_381"
               }
             ],

1282-1282: Tighten HASH160 wording; clarify signature behavior.

Avoid implying privateKeyHex “produces empty signatures.” Signatures are empty for HASH160 regardless; the private key, if provided, is only for deriving the hash.

Apply:

-              "description": "JSON array of public keys. Key requirements: ECDSA_SECP256K1 requires privateKeyHex or privateKeyWif for signing, BLS12_381 requires privateKeyHex for signing, ECDSA_HASH160 requires either the data field (base64-encoded 20-byte public key hash) or privateKeyHex (produces empty signatures)."
+              "description": "JSON array of public keys. ECDSA_SECP256K1 accepts privateKeyHex or privateKeyWif for signing; BLS12_381 requires privateKeyHex for signing; ECDSA_HASH160 accepts either data (base64-encoded 20-byte public key hash) or privateKeyHex (used only to derive the hash). HASH160 signatures are always empty."
packages/wasm-sdk/src/state_transitions/identity/mod.rs (3)

135-183: HASH160: validate conflicting inputs.

If both privateKeyHex and data are provided and mismatch, fail fast to avoid silent divergence.

Apply:

-                KeyType::ECDSA_HASH160 => {
+                KeyType::ECDSA_HASH160 => {
                     // Derive HASH160 data from the private key if provided
-                    if let Some(private_key_hex) = key_data["privateKeyHex"].as_str() {
+                    let from_hex = key_data["privateKeyHex"].as_str();
+                    let from_data = key_data["data"].as_str();
+                    if let Some(private_key_hex) = from_hex {
                         ...
-                        (derived_data, [0u8; 32])
-                    } else if let Some(data_str) = key_data["data"].as_str() {
+                        if let Some(data_str) = from_data {
+                            let provided = dash_sdk::dpp::dashcore::base64::decode(data_str)
+                                .map_err(|e| JsValue::from_str(&format!("Invalid base64 key data: {}", e)))?;
+                            if provided != derived_data {
+                                return Err(JsValue::from_str("ECDSA_HASH160: provided data does not match hash derived from privateKeyHex"));
+                            }
+                        }
+                        (derived_data, [0u8; 32])
+                    } else if let Some(data_str) = from_data {
                         ...

215-244: BLS guard looks good; consider symmetric check for unexpected 'data'.

If a user supplies data along with privateKeyHex for BLS, optionally verify it matches derived bytes.


251-261: Honor readOnly if provided (optional).

You hardcode read_only: false, ignoring input. If UI allows readOnly, consider respecting it.

Apply:

-                read_only: false,
+                read_only: key_data.get("readOnly").and_then(|v| v.as_bool()).unwrap_or(false),
packages/wasm-sdk/docs.html (1)

1970-1970: Clarify ECDSA_HASH160 description in api-definitions.json
Modify the publicKeys input in packages/wasm-sdk/api-definitions.json (around line 1278) as follows, then regenerate docs.html and AI_REFERENCE.md:

- "description": "JSON array of public keys. Key requirements: ECDSA_SECP256K1 requires privateKeyHex or privateKeyWif for signing, BLS12_381 requires privateKeyHex for signing, ECDSA_HASH160 requires either the data field (base64-encoded 20-byte public key hash) or privateKeyHex (produces empty signatures)."
+ "description": "JSON array of public keys. For signing keys: ECDSA_SECP256K1 accepts <code>privateKeyHex</code> or <code>privateKeyWif</code>; BLS12_381 accepts <code>privateKeyHex</code>. For non-signing keys (<code>ECDSA_HASH160</code>), provide the <code>data</code> field (base64-encoded 20-byte public key hash) and omit private keys (these produce empty signatures)."
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

💡 Knowledge Base configuration:

  • MCP integration is disabled by default for public repositories
  • Jira integration is disabled by default for public repositories
  • Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between 2136b00 and 8a213b9.

📒 Files selected for processing (5)
  • packages/wasm-sdk/AI_REFERENCE.md (1 hunks)
  • packages/wasm-sdk/api-definitions.json (2 hunks)
  • packages/wasm-sdk/docs.html (2 hunks)
  • packages/wasm-sdk/docs_manifest.json (1 hunks)
  • packages/wasm-sdk/src/state_transitions/identity/mod.rs (6 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • packages/wasm-sdk/docs_manifest.json
🧰 Additional context used
📓 Path-based instructions (1)
packages/wasm-sdk/src/**/*.rs

📄 CodeRabbit inference engine (packages/wasm-sdk/CLAUDE.md)

packages/wasm-sdk/src/**/*.rs: When implementing WASM SDK functionality, always refer to AI_REFERENCE.md first for accurate method signatures and examples.
For WASM builds, fix 'time not implemented on this platform' errors by using js_sys::Date::now().
Token functions are methods on WasmSdk, not standalone functions; avoid importing them as standalone.
The WASM SDK now fully supports where and orderBy clauses for document queries; use the specified JSON array formats and supported operators.

Files:

  • packages/wasm-sdk/src/state_transitions/identity/mod.rs
🧠 Learnings (12)
📓 Common learnings
Learnt from: QuantumExplorer
PR: dashpay/platform#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-07-28T20:00:08.502Z
Learnt from: QuantumExplorer
PR: dashpay/platform#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/AI_REFERENCE.md
  • packages/wasm-sdk/src/state_transitions/identity/mod.rs
  • packages/wasm-sdk/docs.html
  • packages/wasm-sdk/api-definitions.json
📚 Learning: 2025-07-23T08:31:42.268Z
Learnt from: CR
PR: dashpay/platform#0
File: packages/wasm-sdk/CLAUDE.md:0-0
Timestamp: 2025-07-23T08:31:42.268Z
Learning: Applies to packages/wasm-sdk/src/**/*.rs : When implementing WASM SDK functionality, always refer to AI_REFERENCE.md first for accurate method signatures and examples.

Applied to files:

  • packages/wasm-sdk/AI_REFERENCE.md
  • packages/wasm-sdk/src/state_transitions/identity/mod.rs
📚 Learning: 2025-07-28T20:00:24.323Z
Learnt from: QuantumExplorer
PR: dashpay/platform#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/wasm-sdk/AI_REFERENCE.md
  • packages/wasm-sdk/src/state_transitions/identity/mod.rs
  • packages/wasm-sdk/docs.html
  • packages/wasm-sdk/api-definitions.json
📚 Learning: 2025-09-03T14:41:16.119Z
Learnt from: thephez
PR: dashpay/platform#2754
File: packages/wasm-sdk/AI_REFERENCE.md:766-766
Timestamp: 2025-09-03T14:41:16.119Z
Learning: In packages/wasm-sdk/, the AI_REFERENCE.md file is auto-generated from api-definitions.json. Any documentation fixes should be made in api-definitions.json rather than directly in AI_REFERENCE.md, as manual changes to AI_REFERENCE.md would be overwritten during regeneration.

Applied to files:

  • packages/wasm-sdk/AI_REFERENCE.md
  • packages/wasm-sdk/docs.html
📚 Learning: 2025-06-18T03:44:14.385Z
Learnt from: QuantumExplorer
PR: dashpay/platform#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/AI_REFERENCE.md
  • packages/wasm-sdk/src/state_transitions/identity/mod.rs
  • packages/wasm-sdk/docs.html
📚 Learning: 2024-11-20T20:43:41.185Z
Learnt from: QuantumExplorer
PR: dashpay/platform#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/AI_REFERENCE.md
  • packages/wasm-sdk/src/state_transitions/identity/mod.rs
  • packages/wasm-sdk/docs.html
  • packages/wasm-sdk/api-definitions.json
📚 Learning: 2025-09-03T15:44:33.860Z
Learnt from: thephez
PR: dashpay/platform#2754
File: packages/wasm-sdk/docs.html:0-0
Timestamp: 2025-09-03T15:44:33.860Z
Learning: In packages/wasm-sdk/docs.html, thephez prefers to keep realistic-looking private key placeholders in documentation examples rather than using obvious fake placeholders, despite secret scanner warnings.

Applied to files:

  • packages/wasm-sdk/AI_REFERENCE.md
  • packages/wasm-sdk/src/state_transitions/identity/mod.rs
  • packages/wasm-sdk/docs.html
  • packages/wasm-sdk/api-definitions.json
📚 Learning: 2025-09-03T14:42:29.907Z
Learnt from: thephez
PR: dashpay/platform#2754
File: packages/wasm-sdk/docs.html:1970-1971
Timestamp: 2025-09-03T14:42:29.907Z
Learning: In packages/wasm-sdk/, the docs.html file is auto-generated from api-definitions.json. Any documentation fixes should be made in api-definitions.json rather than directly in docs.html, as manual changes to docs.html would be overwritten during regeneration.

Applied to files:

  • packages/wasm-sdk/AI_REFERENCE.md
  • packages/wasm-sdk/docs.html
📚 Learning: 2024-10-09T00:22:57.778Z
Learnt from: QuantumExplorer
PR: dashpay/platform#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: 2024-11-20T16:05:40.200Z
Learnt from: QuantumExplorer
PR: dashpay/platform#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-21T01:03:42.458Z
Learnt from: QuantumExplorer
PR: dashpay/platform#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
🧬 Code graph analysis (1)
packages/wasm-sdk/src/state_transitions/identity/mod.rs (1)
packages/simple-signer/src/single_key_signer.rs (2)
  • new (18-22)
  • private_key (59-61)
🪛 Gitleaks (8.27.2)
packages/wasm-sdk/docs.html

1989-1989: 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). (2)
  • GitHub Check: Build JS packages / Build JS
  • GitHub Check: build-wasm-sdk
🔇 Additional comments (3)
packages/wasm-sdk/src/state_transitions/identity/mod.rs (3)

12-12: Good switch to SimpleSigner for identity create.

Using SimpleSigner enables per-key signatures and resolves duplicate-signature issues.


263-266: Per-key signer inclusion is correct.

Skipping HASH160 in signer prevents bogus signatures; matches DPP expectations.


275-279: Confirm identity ID handling; avoid random if used.

Using Identifier::random() could conflict if the ID is included in the transition. If PutIdentity derives the ID from the asset lock proof (expected), this is harmless; otherwise it’s wrong.

Would you confirm PutIdentity ignores the supplied identity.id and derives it from the asset lock proof? If not, we should set the correct derived ID here (or leave a placeholder that the builder will overwrite).

@thephez
Copy link
Collaborator Author

thephez commented Sep 3, 2025

@QuantumExplorer finished addressing coderabbit comments also, so this should be ready for re-review / merge if it looks good to you

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
packages/wasm-sdk/index.html (1)

3078-3086: Remove console logging of key material.

console.log('Master key object:', masterKey) and related logs risk leaking private key data into the console. Remove these.

-            console.log('Master key object:', masterKey);
-            console.log('Master key fields:', Object.keys(masterKey || {}));
♻️ Duplicate comments (1)
packages/wasm-sdk/AI_REFERENCE.md (1)

766-766: Fix ECDSA_HASH160 doc: no private key should be provided (empty per-key signature).

Docs still suggest supplying privateKeyHex for ECDSA_HASH160. This is misleading—HASH160 keys don’t have per-key signatures and should not include a private key. Please update the source (api-definitions.json) so this line regenerates correctly.

Apply this wording change in the source (regenerates into AI_REFERENCE.md as below):

-  - JSON array of public keys. Key requirements: ECDSA_SECP256K1 requires privateKeyHex or privateKeyWif for signing, BLS12_381 requires privateKeyHex for signing, ECDSA_HASH160 requires either the data field (base64-encoded 20-byte public key hash) or privateKeyHex (produces empty signatures).
+  - JSON array of public keys. Key requirements: ECDSA_SECP256K1 accepts privateKeyHex or privateKeyWif for per-key signing; BLS12_381 requires privateKeyHex for per-key signing; ECDSA_HASH160 must not include a private key (no per-key signature) and requires only the data field (20‑byte RIPEMD160 public key hash, base64‑encoded).
🧹 Nitpick comments (1)
packages/wasm-sdk/AI_REFERENCE.md (1)

787-793: Clarify placeholder vs. encoding for HASH160 data comment.

Given the chosen placeholder ripemd160hash20bytes1234, avoid calling it “Base64-encoded” here to prevent confusion. Update the source (api-definitions.json) so the generated comment reflects it’s a placeholder.

-    data: "ripemd160hash20bytes1234", // Base64-encoded 20-byte RIPEMD160 hash
+    data: "ripemd160hash20bytes1234", // Placeholder for 20-byte RIPEMD160 public key hash
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

💡 Knowledge Base configuration:

  • MCP integration is disabled by default for public repositories
  • Jira integration is disabled by default for public repositories
  • Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between 889ebde and dda6025.

📒 Files selected for processing (4)
  • packages/wasm-sdk/AI_REFERENCE.md (1 hunks)
  • packages/wasm-sdk/api-definitions.json (2 hunks)
  • packages/wasm-sdk/docs.html (2 hunks)
  • packages/wasm-sdk/index.html (5 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
  • packages/wasm-sdk/docs.html
  • packages/wasm-sdk/api-definitions.json
🧰 Additional context used
📓 Path-based instructions (2)
packages/wasm-sdk/index.html

📄 CodeRabbit inference engine (CLAUDE.md)

Test the WASM SDK using the web interface at 'index.html' in 'packages/wasm-sdk'

Files:

  • packages/wasm-sdk/index.html
packages/wasm-sdk/**/index.html

📄 CodeRabbit inference engine (packages/wasm-sdk/CLAUDE.md)

packages/wasm-sdk/**/index.html: When adding new queries or state transitions, update the definitions in index.html.
The WASM SDK now fully supports where and orderBy clauses for document queries; use the specified JSON array formats and supported operators.

Files:

  • packages/wasm-sdk/index.html
🧠 Learnings (10)
📓 Common learnings
Learnt from: QuantumExplorer
PR: dashpay/platform#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-07-28T20:00:08.502Z
Learnt from: QuantumExplorer
PR: dashpay/platform#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/AI_REFERENCE.md
  • packages/wasm-sdk/index.html
📚 Learning: 2025-07-28T20:00:24.323Z
Learnt from: QuantumExplorer
PR: dashpay/platform#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/wasm-sdk/AI_REFERENCE.md
  • packages/wasm-sdk/index.html
📚 Learning: 2025-07-23T08:31:42.268Z
Learnt from: CR
PR: dashpay/platform#0
File: packages/wasm-sdk/CLAUDE.md:0-0
Timestamp: 2025-07-23T08:31:42.268Z
Learning: Applies to packages/wasm-sdk/src/**/*.rs : When implementing WASM SDK functionality, always refer to AI_REFERENCE.md first for accurate method signatures and examples.

Applied to files:

  • packages/wasm-sdk/AI_REFERENCE.md
📚 Learning: 2025-09-03T19:33:21.688Z
Learnt from: thephez
PR: dashpay/platform#2754
File: packages/wasm-sdk/api-definitions.json:1285-1285
Timestamp: 2025-09-03T19:33:21.688Z
Learning: In packages/wasm-sdk/api-definitions.json, thephez prefers to keep the existing "ripemd160hash20bytes1234" placeholder for ECDSA_HASH160 data field in documentation examples rather than using a valid base64-encoded format, maintaining consistency with the previous documentation approach.

Applied to files:

  • packages/wasm-sdk/AI_REFERENCE.md
  • packages/wasm-sdk/index.html
📚 Learning: 2025-09-03T14:41:16.196Z
Learnt from: thephez
PR: dashpay/platform#2754
File: packages/wasm-sdk/AI_REFERENCE.md:766-766
Timestamp: 2025-09-03T14:41:16.196Z
Learning: In packages/wasm-sdk/, the AI_REFERENCE.md file is auto-generated from api-definitions.json. Any documentation fixes should be made in api-definitions.json rather than directly in AI_REFERENCE.md, as manual changes to AI_REFERENCE.md would be overwritten during regeneration.

Applied to files:

  • packages/wasm-sdk/AI_REFERENCE.md
📚 Learning: 2025-06-18T03:44:14.385Z
Learnt from: QuantumExplorer
PR: dashpay/platform#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/AI_REFERENCE.md
📚 Learning: 2024-11-20T20:43:41.185Z
Learnt from: QuantumExplorer
PR: dashpay/platform#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/AI_REFERENCE.md
📚 Learning: 2025-09-03T15:44:33.889Z
Learnt from: thephez
PR: dashpay/platform#2754
File: packages/wasm-sdk/docs.html:0-0
Timestamp: 2025-09-03T15:44:33.889Z
Learning: In packages/wasm-sdk/docs.html, thephez prefers to keep realistic-looking private key placeholders in documentation examples rather than using obvious fake placeholders, despite secret scanner warnings.

Applied to files:

  • packages/wasm-sdk/AI_REFERENCE.md
  • packages/wasm-sdk/index.html
📚 Learning: 2025-09-03T14:42:29.958Z
Learnt from: thephez
PR: dashpay/platform#2754
File: packages/wasm-sdk/docs.html:1970-1971
Timestamp: 2025-09-03T14:42:29.958Z
Learning: In packages/wasm-sdk/, the docs.html file is auto-generated from api-definitions.json. Any documentation fixes should be made in api-definitions.json rather than directly in docs.html, as manual changes to docs.html would be overwritten during regeneration.

Applied to files:

  • packages/wasm-sdk/AI_REFERENCE.md
⏰ 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). (6)
  • GitHub Check: Build Docker images (Dashmate helper, dashmate-helper, dashmate-helper) / Build Dashmate helper image
  • GitHub Check: Build Docker images (DAPI, dapi, dapi) / Build DAPI image
  • GitHub Check: Rust crates security audit
  • GitHub Check: Build JS packages / Build JS
  • GitHub Check: Determine changed packages
  • GitHub Check: build-wasm-sdk
🔇 Additional comments (5)
packages/wasm-sdk/AI_REFERENCE.md (1)

774-784: LGTM: ECDSA_SECP256K1 example with WIF is correct and matches accepted formats.

packages/wasm-sdk/index.html (4)

3065-3065: Default keyType wiring looks good; confirm API definition provides the select.

Using values.keyType || 'ECDSA_HASH160' is fine. Please verify packages/wasm-sdk/api-definitions.json exposes a keyType input with allowed values that match SDK enum strings exactly (e.g., ECDSA_HASH160, ECDSA_SECP256K1, BLS12_381).


3093-3119: Selected keyType applied to all 3 keys: verify BLS private key derivation path.

You derive all private keys via derive_key_from_seed_with_path(...) and then set keyType: selectedKeyType. If the user selects BLS12_381, are these private keys BLS-derived (and expected by the SDK), or are we passing secp256k1-derived scalars as BLS private keys? If the latter, either gate the UI to ECDSA-only for “default” mode or implement BLS derivation before enabling BLS here.


4298-4298: Good: live preview tracks selected keyType.

This keeps the preview consistent with the actual create payload.


4364-4370: LGTM: preview updates on keyType changes.

The listener keeps the preview accurate as users switch key types.

Copy link
Contributor

@coderabbitai coderabbitai bot left a 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)
packages/wasm-sdk/test/state-transitions.test.mjs (2)

23-24: Consolidate WASM imports to a single namespace import.

Avoid double-loading/caching the same module and keep call-sites consistent by importing the default and namespace in one statement.

Apply this diff here:

-import init, { generate_key_pair } from '../pkg/wasm_sdk.js';
-import * as wasmSdk from '../pkg/wasm_sdk.js';
+import init, * as wasmSdk from '../pkg/wasm_sdk.js';

Then call the generator via the namespace (example outside this hunk):

const assetLockKey = wasmSdk.generate_key_pair("testnet");

195-249: Make HASH160-only case align with “empty signatures” semantics; remove privateKeyHex and consider a valid purpose.

Keeps the test focused on the signer behavior vs. schema validation noise.

Apply this diff to the three HASH160 entries:

-            {
-                keyType: "ECDSA_HASH160",
-                purpose: "AUTHENTICATION",
-                securityLevel: "MASTER", 
-                privateKeyHex: hash160Key1.private_key_hex
-            },
+            {
+                keyType: "ECDSA_HASH160",
+                purpose: "AUTHENTICATION",
+                securityLevel: "MASTER"
+            },
-            {
-                keyType: "ECDSA_HASH160",
-                purpose: "AUTHENTICATION",
-                securityLevel: "CRITICAL",
-                privateKeyHex: hash160Key2.private_key_hex
-            },
+            {
+                keyType: "ECDSA_HASH160",
+                purpose: "AUTHENTICATION",
+                securityLevel: "CRITICAL"
+            },
-            {
-                keyType: "ECDSA_HASH160",
-                purpose: "TRANSFER",
-                securityLevel: "HIGH",
-                privateKeyHex: hash160Key3.private_key_hex
-            }
+            {
+                keyType: "ECDSA_HASH160",
+                purpose: "AUTHENTICATION",
+                securityLevel: "HIGH"
+            }

And drop the unused assignment:

-        const result = await sdk.identityCreate(
+        await sdk.identityCreate(
             mockAssetLockProof,
             assetLockKey.private_key_wif,
             publicKeys
         );

If a BLS test vector generator is available, adding a SECP256K1+BLS mix would directly exercise the “unique signatures per-key-type” claim. I can help wire that in.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

💡 Knowledge Base configuration:

  • MCP integration is disabled by default for public repositories
  • Jira integration is disabled by default for public repositories
  • Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between dda6025 and 442a403.

📒 Files selected for processing (1)
  • packages/wasm-sdk/test/state-transitions.test.mjs (2 hunks)
🧰 Additional context used
🧠 Learnings (7)
📓 Common learnings
Learnt from: QuantumExplorer
PR: dashpay/platform#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-07-28T20:00:24.323Z
Learnt from: QuantumExplorer
PR: dashpay/platform#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/wasm-sdk/test/state-transitions.test.mjs
📚 Learning: 2025-07-28T20:00:08.502Z
Learnt from: QuantumExplorer
PR: dashpay/platform#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/test/state-transitions.test.mjs
📚 Learning: 2025-07-23T08:31:05.082Z
Learnt from: CR
PR: dashpay/platform#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-23T08:31:05.082Z
Learning: Applies to packages/wasm-sdk/index.html : Test the WASM SDK using the web interface at 'index.html' in 'packages/wasm-sdk'

Applied to files:

  • packages/wasm-sdk/test/state-transitions.test.mjs
📚 Learning: 2025-07-23T08:31:42.268Z
Learnt from: CR
PR: dashpay/platform#0
File: packages/wasm-sdk/CLAUDE.md:0-0
Timestamp: 2025-07-23T08:31:42.268Z
Learning: Applies to packages/wasm-sdk/src/**/*.rs : Token functions are methods on WasmSdk, not standalone functions; avoid importing them as standalone.

Applied to files:

  • packages/wasm-sdk/test/state-transitions.test.mjs
📚 Learning: 2025-07-23T08:31:05.082Z
Learnt from: CR
PR: dashpay/platform#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-23T08:31:05.082Z
Learning: Applies to packages/wasm-sdk/build.sh : Build the WASM SDK by running './build.sh' in 'packages/wasm-sdk'

Applied to files:

  • packages/wasm-sdk/test/state-transitions.test.mjs
📚 Learning: 2025-07-23T08:31:05.082Z
Learnt from: CR
PR: dashpay/platform#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-23T08:31:05.082Z
Learning: Use WASM bindings to connect Rust and JavaScript code for cross-language integration

Applied to files:

  • packages/wasm-sdk/test/state-transitions.test.mjs
🧬 Code graph analysis (1)
packages/wasm-sdk/test/state-transitions.test.mjs (2)
packages/wasm-sdk/test/dashpay-contact-keys.test.mjs (1)
  • test (48-57)
packages/wasm-sdk/src/wallet/key_generation.rs (1)
  • generate_key_pair (29-66)
🪛 Gitleaks (8.27.2)
packages/wasm-sdk/test/state-transitions.test.mjs

105-105: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.

(generic-api-key)


111-111: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.

(generic-api-key)


117-117: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.

(generic-api-key)


160-160: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.

(generic-api-key)


166-166: 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). (2)
  • GitHub Check: Build JS packages / Build JS
  • GitHub Check: build-wasm-sdk
🔇 Additional comments (3)
packages/wasm-sdk/test/state-transitions.test.mjs (3)

100-119: Gitleaks “Generic API Key” false positives on privateKeyHex usage in tests.

These values are generated at runtime and never committed. Consider allowlisting this test path/pattern to avoid noise.

Suggested .gitleaks.toml addition (tune as needed):

[[rules]]
id = "ignore-wasm-sdk-privateKeyHex-tests"
description = "Ignore privateKeyHex in wasm-sdk tests (runtime values)"
regex = '''privateKeyHex\s*:\s*[a-zA-Z_][\w\.]*'''
path = '''packages/wasm-sdk/test/'''
allowlist = { paths = ["packages/wasm-sdk/test/"] }

85-139: Tighten the test: drop the unused result and keep the await. The string literals for keyType/purpose/securityLevel are valid and mapped in the wasm binding.

-        const result = await sdk.identityCreate(
+        await sdk.identityCreate(
             mockAssetLockProof,
             assetLockKey.private_key_wif,
             publicKeys
         );

140-194: Verify sdk.identityCreate key handling

Inspect JS wrapper to confirm it omits signatures for ECDSA_HASH160 when no privateKeyHex is provided and supports "TRANSFER" purpose.

@QuantumExplorer QuantumExplorer merged commit 8b681ad into v2.1-dev Sep 7, 2025
19 of 20 checks passed
@QuantumExplorer QuantumExplorer deleted the wasm-sdk-fix-id-create-ecdsa-signing branch September 7, 2025 18:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants