Skip to content

Conversation

@thephez
Copy link
Collaborator

@thephez thephez commented Oct 29, 2025

Issue being fixed or feature implemented

The evo-sdk's documents.create() method required manual entropy generation, causing poor developer experience with cryptic error messages ("Cannot read properties of undefined (reading 'length')") and forcing developers to write boilerplate code for every document creation.

What was done?

This PR adds automatic entropy generation to the documents.create() method in js-evo-sdk, making the API simpler and more consistent with other SDK methods like dpns.registerName().

Key changes:

  • Made entropyHex parameter optional in documents.create()
  • Added generateEntropy() utility function supporting both Node.js and browser environments
  • Auto-generates cryptographically secure 32-byte entropy when not provided
  • Maintains full backward compatibility - existing code with manual entropy still works
  • Added comprehensive unit tests for entropy generation

How Has This Been Tested?

  • Added new unit tests in tests/unit/facades/documents.spec.mjs to verify auto-generation behavior
  • Added new test file tests/unit/util.spec.mjs with comprehensive tests for generateEntropy()
  • All existing tests pass (105 tests passing)
  • Built package successfully with yarn build

Breaking Changes

None - this change is fully backward compatible. The entropyHex parameter is now optional but still accepts values when provided.

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

    • Entropy for document creation is now optional and will be auto-generated when omitted.
    • Added a cross-environment entropy generation utility that produces secure 32-byte entropy (works in Node and browsers, with fallback behavior).
  • Tests

    • Added tests validating entropy generation format, uniqueness, byte-length, and distribution properties.
    • Updated document creation tests to verify auto-generation and that explicit entropy is forwarded unchanged.

Copy link
Collaborator Author

@thephez thephez left a comment

Choose a reason for hiding this comment

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

@coderabbitai review

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Oct 29, 2025

Walkthrough

create() in DocumentsFacade now accepts an optional entropyHex and auto-generates a 64‑char hex entropy when omitted. A new generateEntropy() util was added to produce 32 bytes (64 hex chars) of cryptographically secure entropy. Tests updated/added to cover forwarding and generation behavior.

Changes

Cohort / File(s) Summary
Entropy Utility
packages/js-evo-sdk/src/util.ts
Added generateEntropy(): string producing 32 bytes (64 hex chars) of cryptographically secure entropy with Node/Web Crypto detection and an error when no secure source is available.
Documents Facade
packages/js-evo-sdk/src/documents/facade.ts
Made entropyHex optional in create() signature, imported generateEntropy, compute entropyHex = args.entropyHex ?? generateEntropy() before calling documentCreate, and pass it through.
Facade Tests
packages/js-evo-sdk/tests/unit/facades/documents.spec.mjs
Updated test to assert provided entropy is forwarded; added test verifying auto-generation when entropyHex is omitted and that a 64‑char hex string is forwarded.
Utility Tests
packages/js-evo-sdk/tests/unit/util.spec.mjs
New tests for generateEntropy() (64-char length, hex format, distinct outputs, decodes to 32 bytes, sampling checks) and tests for asJsonString() behavior.

Sequence Diagram(s)

sequenceDiagram
    participant Caller
    participant DocumentsFacade
    participant generateEntropy as EntropyUtil
    participant wasmSdk as DocumentCreate

    Caller->>DocumentsFacade: create(args {..., entropyHex?})
    alt entropyHex provided
        DocumentsFacade->>wasmSdk: documentCreate({... entropyHex ...})
    else entropyHex missing
        DocumentsFacade->>EntropyUtil: generateEntropy()
        EntropyUtil-->>DocumentsFacade: entropyHex (64‑char hex)
        DocumentsFacade->>wasmSdk: documentCreate({... entropyHex ...})
    end
    wasmSdk-->>Caller: Promise result
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20–30 minutes

  • Review focus:
    • Correctness and environment coverage of generateEntropy() (Node vs browser detection and error path).
    • Ensure entropyHex format and length meet documentCreate expectations.
    • Tests that assert randomness/distribution may be flaky — validate sampling approach and thresholds.

"I nibble at bytes in moonlit code,
Sixty‑four hops of hex in my tote,
A fresh seed sprung from rabbit's paw,
Documents waltz with entropy raw. 🐇"

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The PR title "feat(sdk): add automatic entropy generation for document creation" accurately reflects the primary change in this changeset. The PR introduces a new generateEntropy() utility function and modifies documents.create() to auto-generate entropy when the entropyHex parameter is omitted, which is precisely what the title communicates. The title is concise, uses standard conventional commit formatting, avoids vague terminology, and provides sufficient specificity that a developer reviewing the project history would understand the core purpose of the change.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch wasm-sdk-entropy-util

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

Comment @coderabbitai help to get the list of available commands and usage tips.

@thephez
Copy link
Collaborator Author

thephez commented Oct 29, 2025

@coderabbitai full review

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Oct 29, 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.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Oct 29, 2025

✅ Actions performed

Full review triggered.

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/js-evo-sdk/src/util.ts (1)

16-19: Consider removing likely dead code.

In Node.js, globalThis.crypto provides the Web Crypto API, which includes getRandomValues but not randomBytes. The randomBytes method is specific to the Node.js crypto module (accessed via import or require), not globalThis.crypto.

This branch will likely never execute, though the function still works correctly via the getRandomValues path below (lines 22-26).

Consider removing this branch or replacing it with a proper Node.js crypto module import if Node.js-specific optimization is desired:

-  // Node.js environment
-  if (typeof globalThis !== 'undefined' && globalThis.crypto && 'randomBytes' in globalThis.crypto) {
-    // @ts-ignore - Node.js crypto.randomBytes exists but may not be in types
-    return globalThis.crypto.randomBytes(32).toString('hex');
-  }
-
packages/js-evo-sdk/tests/unit/util.spec.mjs (1)

51-59: Consider using slice() instead of deprecated substr().

Line 56 uses substr(), which is deprecated. While it still works, prefer slice() or substring() for better future compatibility.

Apply this diff:

-        bytes.push(parseInt(entropy.substr(i, 2), 16));
+        bytes.push(parseInt(entropy.slice(i, i + 2), 16));
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between f49390f and d852462.

📒 Files selected for processing (4)
  • packages/js-evo-sdk/src/documents/facade.ts (2 hunks)
  • packages/js-evo-sdk/src/util.ts (1 hunks)
  • packages/js-evo-sdk/tests/unit/facades/documents.spec.mjs (2 hunks)
  • packages/js-evo-sdk/tests/unit/util.spec.mjs (1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
packages/**/tests/**

📄 CodeRabbit inference engine (AGENTS.md)

Place unit and integration tests alongside each package in packages//tests

Files:

  • packages/js-evo-sdk/tests/unit/facades/documents.spec.mjs
  • packages/js-evo-sdk/tests/unit/util.spec.mjs
packages/**/**/*.{js,ts,jsx,tsx}

📄 CodeRabbit inference engine (AGENTS.md)

packages/**/**/*.{js,ts,jsx,tsx}: Adhere to ESLint with Airbnb/TypeScript configs for JS/TS code
Use camelCase for JS/TS variables and functions
Use PascalCase for JS/TS classes
Prefer kebab-case filenames within JS packages

Files:

  • packages/js-evo-sdk/src/documents/facade.ts
  • packages/js-evo-sdk/src/util.ts
🧠 Learnings (7)
📓 Common learnings
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.
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.
📚 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/js-evo-sdk/tests/unit/facades/documents.spec.mjs
📚 Learning: 2025-09-22T05:59:31.140Z
Learnt from: shumkov
PR: dashpay/platform#2783
File: packages/js-evo-sdk/tests/functional/dpns.spec.mjs:2-2
Timestamp: 2025-09-22T05:59:31.140Z
Learning: The dist/evo-sdk.module.js file in packages/js-evo-sdk is a generated build artifact created during the build process, not a source file that should exist in the repository.

Applied to files:

  • packages/js-evo-sdk/tests/unit/util.spec.mjs
📚 Learning: 2025-09-12T13:18:08.661Z
Learnt from: CR
PR: dashpay/platform#0
File: AGENTS.md:0-0
Timestamp: 2025-09-12T13:18:08.661Z
Learning: Applies to packages/**/tests/**/*.{js,ts,jsx,tsx,rs} : Name tests descriptively, starting with “should …”

Applied to files:

  • packages/js-evo-sdk/tests/unit/util.spec.mjs
📚 Learning: 2024-10-18T15:37:21.329Z
Learnt from: shumkov
PR: dashpay/platform#2249
File: packages/dashmate/test/unit/status/scopes/platform.spec.js:466-467
Timestamp: 2024-10-18T15:37:21.329Z
Learning: In test files for the dashmate project, such as `packages/dashmate/test/unit/status/scopes/platform.spec.js`, prefer to keep mocks explicitly in tests rather than consolidating them, to increase readability.

Applied to files:

  • packages/js-evo-sdk/tests/unit/util.spec.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/js-evo-sdk/src/documents/facade.ts
📚 Learning: 2024-10-24T04:58:02.843Z
Learnt from: shumkov
PR: dashpay/platform#2270
File: packages/dapi/lib/grpcServer/handlers/platform/broadcastStateTransitionHandlerFactory.js:19-19
Timestamp: 2024-10-24T04:58:02.843Z
Learning: For operations involving Tenderdash where no client library exists yet, it's acceptable to use the Node.js built-in `crypto` module.

Applied to files:

  • packages/js-evo-sdk/src/util.ts
🔇 Additional comments (9)
packages/js-evo-sdk/tests/unit/facades/documents.spec.mjs (2)

56-67: LGTM! Good test coverage for explicit entropy.

The test clearly verifies that when entropyHex is explicitly provided, it's forwarded unchanged to the underlying documentCreate call. The updated test name improves clarity.


69-95: LGTM! Comprehensive test for auto-generation.

The test thoroughly verifies the auto-generation behavior by:

  • Confirming documentCreate is called with all expected parameters
  • Validating the generated entropy matches the expected format (64 hex characters)
  • Ensuring other parameters are forwarded correctly
packages/js-evo-sdk/src/documents/facade.ts (2)

1-1: LGTM! Correct import addition.

The import of generateEntropy is necessary for the new auto-generation feature.


70-90: LGTM! Clean implementation of optional entropy.

The changes correctly implement the auto-generation feature:

  • Making entropyHex optional maintains backward compatibility
  • Using the nullish coalescing operator (??) ensures entropy is auto-generated only when not provided
  • The implementation is concise and clear
packages/js-evo-sdk/src/util.ts (2)

7-13: LGTM! Clear documentation.

The JSDoc clearly describes the function's purpose, return value, and error conditions.


22-36: LGTM! Core implementation is correct.

The getRandomValues path (lines 22-26) works correctly in both modern Node.js and browsers. The hex conversion properly pads each byte to two characters.

The window.crypto fallback (lines 29-33) is redundant in modern environments since globalThis.crypto and window.crypto are equivalent in browsers, but keeping it improves compatibility with older environments at minimal cost.

packages/js-evo-sdk/tests/unit/util.spec.mjs (3)

4-26: LGTM! Comprehensive test coverage for asJsonString.

The tests cover all relevant cases: null, undefined, strings, objects, and arrays.


28-50: LGTM! Good coverage of basic entropy properties.

The tests effectively verify:

  • Correct output length (64 characters)
  • Valid hexadecimal format
  • Non-deterministic generation (different values each time)

61-76: LGTM! Good statistical validation.

The distribution test effectively verifies that the entropy generation produces a good variety of hex digits across multiple samples, helping ensure the randomness quality.

@thephez thephez marked this pull request as ready for review October 29, 2025 19:50
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

🧹 Nitpick comments (3)
packages/js-evo-sdk/tests/unit/util.spec.mjs (1)

61-75: Consider raising the uniqueness threshold for better cryptographic quality validation.

The threshold of 10 unique hex digits (62.5% coverage) might be too permissive for validating cryptographic randomness. With 640 characters (10 samples × 64 chars), you would typically expect to see all 16 hex digits with high probability.

Consider raising the threshold to ensure better validation:

-    // We should see most of the 16 possible hex digits (0-9, a-f)
-    // With 640 characters (10 * 64), we expect to see all 16
-    expect(uniqueChars).to.be.at.least(10);
+    // We should see most of the 16 possible hex digits (0-9, a-f)
+    // With 640 characters (10 * 64), we expect to see all or nearly all 16
+    expect(uniqueChars).to.be.at.least(14);
packages/js-evo-sdk/src/util.ts (2)

28-33: Remove redundant fallback code.

Lines 28-33 duplicate the logic from lines 22-26. In modern JavaScript environments where globalThis is not available, window also won't be available. Since globalThis is available in all modern browsers and Node.js versions that support the Web Crypto API, this fallback is unnecessary.

Apply this diff to remove the redundant code:

   // Browser environment or Node.js with Web Crypto API
   if (typeof globalThis !== 'undefined' && globalThis.crypto && globalThis.crypto.getRandomValues) {
     const buffer = new Uint8Array(32);
     globalThis.crypto.getRandomValues(buffer);
     return Array.from(buffer).map(b => b.toString(16).padStart(2, '0')).join('');
   }

-  // Fallback for older environments
-  if (typeof window !== 'undefined' && window.crypto && window.crypto.getRandomValues) {
-    const buffer = new Uint8Array(32);
-    window.crypto.getRandomValues(buffer);
-    return Array.from(buffer).map(b => b.toString(16).padStart(2, '0')).join('');
-  }
-
   throw new Error('No secure random source available. This environment does not support crypto.randomBytes or crypto.getRandomValues.');

17-17: Consider addressing TypeScript type issues properly instead of using @ts-ignore.

The @ts-ignore comment suppresses type checking, which can mask legitimate type issues. Consider using proper type assertions or checking if the types can be improved.

If the concern is that randomBytes might not be in the type definitions for globalThis.crypto, you could use a type guard or type assertion:

// Node.js environment
if (typeof globalThis !== 'undefined' && globalThis.crypto && 'randomBytes' in globalThis.crypto) {
  const crypto = globalThis.crypto as any;
  if (typeof crypto.randomBytes === 'function') {
    return crypto.randomBytes(32).toString('hex');
  }
}

However, given that this branch is unreachable (see previous comment), this may be moot if the branch is removed.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between f49390f and d852462.

📒 Files selected for processing (4)
  • packages/js-evo-sdk/src/documents/facade.ts (2 hunks)
  • packages/js-evo-sdk/src/util.ts (1 hunks)
  • packages/js-evo-sdk/tests/unit/facades/documents.spec.mjs (2 hunks)
  • packages/js-evo-sdk/tests/unit/util.spec.mjs (1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
packages/**/tests/**

📄 CodeRabbit inference engine (AGENTS.md)

Place unit and integration tests alongside each package in packages//tests

Files:

  • packages/js-evo-sdk/tests/unit/facades/documents.spec.mjs
  • packages/js-evo-sdk/tests/unit/util.spec.mjs
packages/**/**/*.{js,ts,jsx,tsx}

📄 CodeRabbit inference engine (AGENTS.md)

packages/**/**/*.{js,ts,jsx,tsx}: Adhere to ESLint with Airbnb/TypeScript configs for JS/TS code
Use camelCase for JS/TS variables and functions
Use PascalCase for JS/TS classes
Prefer kebab-case filenames within JS packages

Files:

  • packages/js-evo-sdk/src/util.ts
  • packages/js-evo-sdk/src/documents/facade.ts
🧠 Learnings (6)
📓 Common learnings
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.
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.
📚 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/js-evo-sdk/tests/unit/facades/documents.spec.mjs
📚 Learning: 2024-10-24T04:58:02.843Z
Learnt from: shumkov
PR: dashpay/platform#2270
File: packages/dapi/lib/grpcServer/handlers/platform/broadcastStateTransitionHandlerFactory.js:19-19
Timestamp: 2024-10-24T04:58:02.843Z
Learning: For operations involving Tenderdash where no client library exists yet, it's acceptable to use the Node.js built-in `crypto` module.

Applied to files:

  • packages/js-evo-sdk/src/util.ts
📚 Learning: 2025-09-22T05:59:31.140Z
Learnt from: shumkov
PR: dashpay/platform#2783
File: packages/js-evo-sdk/tests/functional/dpns.spec.mjs:2-2
Timestamp: 2025-09-22T05:59:31.140Z
Learning: The dist/evo-sdk.module.js file in packages/js-evo-sdk is a generated build artifact created during the build process, not a source file that should exist in the repository.

Applied to files:

  • packages/js-evo-sdk/tests/unit/util.spec.mjs
📚 Learning: 2025-09-12T13:18:08.661Z
Learnt from: CR
PR: dashpay/platform#0
File: AGENTS.md:0-0
Timestamp: 2025-09-12T13:18:08.661Z
Learning: Applies to packages/**/tests/**/*.{js,ts,jsx,tsx,rs} : Name tests descriptively, starting with “should …”

Applied to files:

  • packages/js-evo-sdk/tests/unit/util.spec.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/js-evo-sdk/src/documents/facade.ts
🔇 Additional comments (7)
packages/js-evo-sdk/tests/unit/util.spec.mjs (1)

3-26: LGTM! Comprehensive test coverage for asJsonString.

The tests cover all expected edge cases (null, undefined, string passthrough, object/array serialization) with correct assertions.

packages/js-evo-sdk/tests/unit/facades/documents.spec.mjs (2)

56-67: LGTM! Clear test for explicit entropy forwarding.

The test correctly verifies that when entropyHex is explicitly provided, it's forwarded unchanged to the underlying WASM SDK call.


69-95: LGTM! Comprehensive test for auto-generation behavior.

The test thoroughly validates that:

  1. Entropy is auto-generated when not provided
  2. All other parameters are forwarded correctly
  3. Generated entropy matches the expected 64-character hex format
packages/js-evo-sdk/src/util.ts (1)

14-19: Node.js randomBytes branch is unreachable.

In Node.js environments, globalThis.crypto provides the Web Crypto API (which includes getRandomValues), but randomBytes is a separate method available only through import crypto from 'crypto' or require('crypto'). The check 'randomBytes' in globalThis.crypto will always fail in Node.js, so this branch will never execute.

The current implementation will still work correctly because it falls through to the getRandomValues branch (lines 22-26), which works in both Node.js (v15+) and browsers.

If Node.js-specific randomBytes optimization is intended, consider importing the crypto module explicitly:

export function generateEntropy(): string {
  // Try Node.js crypto module first (if available)
  try {
    // Dynamic import for Node.js compatibility
    const nodeCrypto = await import('crypto');
    if (nodeCrypto && nodeCrypto.randomBytes) {
      return nodeCrypto.randomBytes(32).toString('hex');
    }
  } catch {
    // Fall through to Web Crypto API
  }

  // Browser/Node.js Web Crypto API
  if (typeof globalThis !== 'undefined' && globalThis.crypto?.getRandomValues) {
    const buffer = new Uint8Array(32);
    globalThis.crypto.getRandomValues(buffer);
    return Array.from(buffer).map(b => b.toString(16).padStart(2, '0')).join('');
  }

  throw new Error('No secure random source available.');
}

However, the current implementation using getRandomValues works correctly in both environments, so this optimization may not be necessary.

packages/js-evo-sdk/src/documents/facade.ts (3)

1-1: LGTM! Import correctly updated.

The generateEntropy import is properly added to support the new auto-generation feature.


70-77: LGTM! Backward-compatible API enhancement.

Making entropyHex optional is a clean improvement that:

  • Maintains backward compatibility (existing code with explicit entropy still works)
  • Reduces boilerplate for common use cases
  • Provides clear documentation via the inline comment

78-90: LGTM! Clean implementation of auto-generation logic.

The implementation correctly:

  1. Handles entropyHex separately from other destructured parameters
  2. Uses nullish coalescing (??) to generate entropy only when not provided
  3. Passes the resolved entropy value to the underlying WASM SDK call

The logic is clear, concise, and maintains the expected behavior for both explicit and auto-generated entropy.

shumkov
shumkov previously approved these changes Oct 30, 2025
@lklimek
Copy link
Contributor

lklimek commented Oct 31, 2025

Please rebase to 2.2-dev

- Make entropyHex parameter optional in documents.create()
- Add generateEntropy() utility function that works in Node.js and browsers
- Auto-generate entropy when not provided
- Add comprehensive tests for entropy generation and auto-generation behavior
@thephez thephez changed the base branch from master to v2.2-dev October 31, 2025 17:18
@thephez thephez dismissed shumkov’s stale review October 31, 2025 17:18

The base branch was changed.

@thephez thephez force-pushed the wasm-sdk-entropy-util branch from 3d69a24 to cbb42ea Compare October 31, 2025 17:20
@thephez thephez added this to the v2.2.0 milestone Oct 31, 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

🧹 Nitpick comments (2)
packages/js-evo-sdk/src/documents/facade.ts (1)

75-80: Consider validating user-provided entropyHex format.

The implementation correctly auto-generates entropy when not provided and maintains backward compatibility. However, when users do provide entropyHex, there's no validation to ensure it's a valid 64-character hexadecimal string. Invalid input will be passed directly to documentCreate, which may result in unclear error messages downstream.

Consider adding validation when entropyHex is provided:

  async create(args: {
    contractId: string;
    type: string;
    ownerId: string;
    data: unknown;
    entropyHex?: string; // Now optional - will auto-generate if not provided
    privateKeyWif: string;
  }): Promise<any> {
    const { contractId, type, ownerId, data, privateKeyWif } = args;
    // Auto-generate entropy if not provided
    const entropyHex = args.entropyHex ?? generateEntropy();
+   // Validate entropy format if provided by user
+   if (args.entropyHex && !/^[0-9a-fA-F]{64}$/.test(entropyHex)) {
+     throw new Error('entropyHex must be a 64-character hexadecimal string');
+   }
    const w = await this.sdk.getWasmSdkConnected();
    return w.documentCreate(
packages/js-evo-sdk/src/util.ts (1)

14-36: Remove dead code branches from generateEntropy() to simplify the implementation.

The first branch (lines 16-19) checking 'randomBytes' in globalThis.crypto is dead code and will never execute. In Node.js 15+, globalThis.crypto is the Web Crypto API, which does not have a randomBytes method. This method exists only in the Node.js crypto module.

The second branch (lines 22-26) using getRandomValues already handles both Node.js 15+ and browser environments correctly, so the dead code is unnecessary. The third branch (lines 29-33) with the window.crypto fallback is also redundant since globalThis is universally supported in modern environments (Node.js 12+ and all modern browsers).

Recommended simplification:

  • Remove lines 16-19 (dead randomBytes check)
  • Remove lines 29-33 (redundant window.crypto fallback)
  • Keep only the globalThis.crypto.getRandomValues branch, which is sufficient for all supported environments
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between d852462 and cbb42ea.

📒 Files selected for processing (4)
  • packages/js-evo-sdk/src/documents/facade.ts (2 hunks)
  • packages/js-evo-sdk/src/util.ts (1 hunks)
  • packages/js-evo-sdk/tests/unit/facades/documents.spec.mjs (2 hunks)
  • packages/js-evo-sdk/tests/unit/util.spec.mjs (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
  • packages/js-evo-sdk/tests/unit/facades/documents.spec.mjs
  • packages/js-evo-sdk/tests/unit/util.spec.mjs
🧰 Additional context used
📓 Path-based instructions (1)
packages/**/**/*.{js,ts,jsx,tsx}

📄 CodeRabbit inference engine (AGENTS.md)

packages/**/**/*.{js,ts,jsx,tsx}: Adhere to ESLint with Airbnb/TypeScript configs for JS/TS code
Use camelCase for JS/TS variables and functions
Use PascalCase for JS/TS classes
Prefer kebab-case filenames within JS packages

Files:

  • packages/js-evo-sdk/src/util.ts
  • packages/js-evo-sdk/src/documents/facade.ts
🧠 Learnings (2)
📚 Learning: 2024-10-24T04:58:02.843Z
Learnt from: shumkov
Repo: dashpay/platform PR: 2270
File: packages/dapi/lib/grpcServer/handlers/platform/broadcastStateTransitionHandlerFactory.js:19-19
Timestamp: 2024-10-24T04:58:02.843Z
Learning: For operations involving Tenderdash where no client library exists yet, it's acceptable to use the Node.js built-in `crypto` module.

Applied to files:

  • packages/js-evo-sdk/src/util.ts
📚 Learning: 2025-07-28T20:00:08.502Z
Learnt from: QuantumExplorer
Repo: dashpay/platform PR: 2711
File: packages/wasm-sdk/AI_REFERENCE.md:771-783
Timestamp: 2025-07-28T20:00:08.502Z
Learning: In packages/wasm-sdk/AI_REFERENCE.md, the documentation correctly shows the actual SDK method signatures (including identityCreate and identityTopUp with their full parameter lists), which may differ from the UI inputs shown in fixed_definitions.json. The UI may collect fewer parameters from users while handling additional requirements internally.

Applied to files:

  • packages/js-evo-sdk/src/documents/facade.ts
🧬 Code graph analysis (2)
packages/js-evo-sdk/src/util.ts (1)
packages/wasm-sdk/tests/unit/derivation.spec.mjs (1)
  • b (140-140)
packages/js-evo-sdk/src/documents/facade.ts (1)
packages/js-evo-sdk/src/util.ts (1)
  • generateEntropy (14-36)
⏰ 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). (3)
  • GitHub Check: Build Docker images (Drive, drive, drive-abci) / Build Drive image
  • GitHub Check: Build JS packages / Build JS
  • GitHub Check: Rust crates security audit
🔇 Additional comments (1)
packages/js-evo-sdk/src/documents/facade.ts (1)

1-1: LGTM: Import added correctly.

The generateEntropy import is properly added alongside the existing asJsonString import.

@thephez thephez requested a review from shumkov November 3, 2025 14:46
@thephez
Copy link
Collaborator Author

thephez commented Nov 3, 2025

@lklimek rebased and passing tests

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.

4 participants