feat(sdk): change to wasm sdk for js-sdk #2714
Conversation
- Replace DPP with wasm-sdk for all platform methods - Add WasmPlatformAdapter for wasm-sdk integration - Update all identity, document, contract, and name methods - Add compatibility layer for platform test suite - Fix createAssetLockProof to handle undefined dpp - Document test suite issues and findings 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
WalkthroughThis change integrates the Changes
Sequence Diagram(s)sequenceDiagram
participant App
participant Platform
participant WasmAdapter
participant WasmSdk
App->>Platform: Call platform method (e.g., publish, get, update)
alt wasmSdk available and enabled
Platform->>WasmAdapter: Delegate call (convert params)
WasmAdapter->>WasmSdk: Call wasm-sdk method
WasmSdk-->>WasmAdapter: Return result
WasmAdapter-->>Platform: Convert and return result
Platform-->>App: Return result
else legacy fallback
Platform->>Platform: Use legacy DPP logic
Platform-->>App: Return result
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
♻️ Duplicate comments (2)
packages/js-dash-sdk/src/SDK/Client/Platform/methods/contracts/update.ts (2)
22-24: Same non-null assertion issue as in broadcast.ts.
65-65: Critical: Inconsistent return types between execution paths.Similar to broadcast.ts, this method returns different types:
- wasm-sdk path: result object from wasm-sdk
- legacy path: dataContractUpdateTransition
Also applies to: 101-101
🧹 Nitpick comments (23)
packages/js-dash-sdk/src/SDK/Client/Platform/methods/contracts/get.ts (1)
53-60: Extract contract caching logic to reduce duplication.The contract caching logic is duplicated between the wasm-sdk and legacy paths. Consider extracting this into a helper method.
+ private cacheContract(contractId: Identifier, contract: any) { + for (const appName of this.client.getApps().getNames()) { + const appDefinition = this.client.getApps().get(appName); + if (appDefinition.contractId.equals(contractId)) { + appDefinition.contract = contract; + } + } + }Then use
this.cacheContract(contractId, contract)in both paths.Also applies to: 90-97
packages/js-dash-sdk/src/SDK/Client/Platform/broadcastStateTransition.ts (1)
41-43: Extract hash computation to reduce duplication.The SHA-256 hash computation for the state transition is duplicated between the wasm-sdk and legacy paths.
+ const computeStateTransitionHash = (stateTransition: any): Buffer => { + return crypto.createHash('sha256') + .update(stateTransition.toBuffer()) + .digest(); + }; // In wasm-sdk path: - const hash = crypto.createHash('sha256') - .update(stateTransition.toBuffer()) - .digest(); + const hash = computeStateTransitionHash(stateTransition); // In legacy path: - const hash = crypto.createHash('sha256') - .update(stateTransition.toBuffer()) - .digest(); + const hash = computeStateTransitionHash(stateTransition);Also applies to: 97-99
packages/js-dash-sdk/src/SDK/Client/Platform/methods/names/search.ts (1)
14-36: Consider adding caching for consistency.Unlike the contracts
getmethod, this search method doesn't implement caching for wasm-sdk results. Consider whether caching would be beneficial for name searches.packages/js-dash-sdk/src/SDK/Client/Platform/methods/names/register.ts (1)
50-52: Simplify records serialization.The records object conversion is unnecessarily complex. Consider simplifying the JSON serialization.
- const recordsJson = JSON.stringify({ - identity: records.identity ? Identifier.from(records.identity).toString() : undefined, - }); + const recordsJson = JSON.stringify({ + ...records, + identity: records.identity ? Identifier.from(records.identity).toString() : records.identity, + });packages/js-dash-sdk/src/SDK/Client/Platform/methods/identities/get.ts (1)
17-41: Good wasm-sdk integration pattern with room for error handling improvements.The conditional delegation approach is well-structured and maintains backward compatibility. The caching strategy is a smart performance optimization.
However, consider these improvements:
Error handling robustness: Line 36 relies on string matching for error detection which could be fragile across different error formats.
Type safety: The
id.toString()call assumes thetoString()method exists but doesn't handle potential conversion failures.Consider enhancing error handling:
- } catch (e) { - if (e.message?.includes('not found') || e.message?.includes('does not exist')) { - return null; - } - throw e; - } + } catch (e) { + // Handle known not-found patterns and specific error types + if (e.message?.toLowerCase().includes('not found') || + e.message?.toLowerCase().includes('does not exist') || + e.code === 'IDENTITY_NOT_FOUND') { + return null; + } + throw e; + }CLAUDE.md (1)
132-149: Valuable documentation addition with minor style fixes needed.This documentation provides essential troubleshooting guidance for wasm-sdk integration testing.
Apply these style fixes:
-1. **Faucet Wallet Setup**: The faucet wallet needs funded UTXOs. Generate blocks to the faucet address: +1. **Faucet Wallet Setup**: The faucet wallet needs to be funded with UTXOs. Generate blocks to the faucet address:- - Bootstrap.js maps FAUCET_1_* vars to FAUCET_* based on worker ID + - Bootstrap.js maps FAUCET_1_* vars to FAUCET_* based on worker IDpackages/js-dash-sdk/src/SDK/Client/Platform/methods/names/resolveByRecord.ts (1)
12-43: Well-structured wasm-sdk delegation with thoughtful value conversion.The implementation correctly handles different record types and includes helpful debug logging.
Consider these improvements:
Value conversion safety: Lines 20-21 use generic
toString()which could fail for objects without this method.Hardcoded parameter: Line 30 hardcodes
parentDomainNameasundefined. Consider if this should be configurable.Enhance value conversion safety:
- } else if (typeof value === 'object' && value.toString) { - recordValue = value.toString(); + } else if (typeof value === 'object' && value && typeof value.toString === 'function') { + recordValue = value.toString(); + } else if (typeof value === 'object') { + recordValue = JSON.stringify(value);packages/js-dash-sdk/src/SDK/Client/Platform/methods/identities/topUp.ts (1)
41-64: Solid wasm-sdk delegation with adapter-based conversions.The use of adapter methods for format conversions maintains good separation of concerns.
Consider adding error handling for adapter conversions:
+ try { // Convert asset lock proof to hex format for wasm-sdk const assetLockProofHex = adapter.convertAssetLockProof(assetLockProof); // Convert private key to WIF format const assetLockPrivateKeyWIF = adapter.convertPrivateKeyToWIF(assetLockPrivateKey); + } catch (conversionError) { + this.logger.error(`[Identity#topUp] Conversion error: ${conversionError.message}`); + throw new Error(`Failed to convert parameters for wasm-sdk: ${conversionError.message}`); + }Also, consider validating the success response format:
- return result.success !== false; + return Boolean(result?.success);packages/platform-test-suite/lib/test/wasm-sdk-compat.js (1)
11-67: Excellent compatibility layer design with defensive buffer handling.The approach of checking for legacy DPP first and providing a shim otherwise is elegant. The buffer conversion logic is appropriately defensive.
Consider these minor improvements:
More specific error messages: Lines 49, 56, 63 could provide more context about what needs to be implemented.
Validation: Consider validating that required platform utility methods exist.
Enhance error messages with more context:
- throw new Error('createFromBuffer not yet implemented for wasm-sdk compatibility'); + throw new Error('Identity.createFromBuffer not yet implemented for wasm-sdk compatibility. ' + + 'This method needs to convert wasm-sdk identity responses to DPP format.');packages/js-dash-sdk/src/SDK/Client/Platform/methods/contracts/history.ts (2)
67-72: Consider more specific error detectionThe error handling checks for "not found" or "does not exist" in the message. Consider checking for specific error types or codes if the wasm-sdk provides them, as string matching on error messages can be fragile.
26-26: Improve return type definitionThe function returns
Promise<any>but actually returns a specific structure{ [key: number]: DataContract }. Consider defining a proper return type for better type safety.-): Promise<any> { +): Promise<{ [key: number]: DataContract } | null> {packages/js-dash-sdk/src/SDK/Client/Platform/methods/identities/update.ts (1)
70-83: Consider more explicit success checkThe success check
result.success !== falsewould return true for undefined or null values. Consider a more explicit check.- return result.success !== false; + return result.success === true;packages/js-dash-sdk/src/SDK/Client/Platform/methods/documents/broadcast.ts (3)
32-35: Consider avoiding the non-null assertion operator for better type safety.While the non-null assertion is safe here due to the check on line 33, consider restructuring to avoid using
!:- if (this.wasmSdk && this.getAdapter()) { - const adapter = this.getAdapter()!; + const adapter = this.getAdapter(); + if (this.wasmSdk && adapter) {
40-41: Add error handling for HD key retrieval.The
getIdentityHDKeyByIdcall could potentially fail if the identity doesn't exist in the wallet. Consider adding error handling:+ try { const { privateKey: documentPrivateKey } = account.identities .getIdentityHDKeyById(identity.getId().toString(), 1); + } catch (error) { + throw new Error(`Failed to retrieve document private key for identity ${identity.getId()}: ${error.message}`); + }
133-146: Consider extracting cache management logic to reduce duplication.The cache acknowledgment logic is duplicated between the wasm-sdk path and legacy path. Consider extracting it to a helper method:
+ // Helper method for cache management + const manageCacheForDocuments = (documents: { + create?: ExtendedDocument[], + delete?: ExtendedDocument[] + }) => { + if (documents.create) { + documents.create.forEach((document) => { + const documentLocator = `${document.getDataContractId().toString()}/${document.getType()}`; + this.fetcher.acknowledgeKey(documentLocator); + }); + } + + if (documents.delete) { + documents.delete.forEach((document) => { + const documentLocator = `${document.getDataContractId().toString()}/${document.getType()}`; + this.fetcher.forgetKey(documentLocator); + }); + } + }; // In wasm-sdk path: - // Handle document acknowledgment for cache - if (documents.create) { - documents.create.forEach((document) => { - const documentLocator = `${document.getDataContractId().toString()}/${document.getType()}`; - this.fetcher.acknowledgeKey(documentLocator); - }); - } - - if (documents.delete) { - documents.delete.forEach((document) => { - const documentLocator = `${document.getDataContractId().toString()}/${document.getType()}`; - this.fetcher.forgetKey(documentLocator); - }); - } + manageCacheForDocuments(documents); // In legacy path: - // Acknowledge documents identifiers to handle retry attempts to mitigate - // state transition propagation lag - if (documents.create) { - documents.create.forEach((document) => { - const documentLocator = `${document.getDataContractId().toString()}/${document.getType()}`; - this.fetcher.acknowledgeKey(documentLocator); - }); - } - - // Forget documents identifiers to not retry on them anymore - if (documents.delete) { - documents.delete.forEach((document) => { - const documentLocator = `${document.getDataContractId().toString()}/${document.getType()}`; - this.fetcher.forgetKey(documentLocator); - }); - } + manageCacheForDocuments(documents);Also applies to: 186-200
packages/js-dash-sdk/src/SDK/Client/Platform/methods/contracts/update.ts (1)
54-61: Extract duplicated app update logic.The app update logic is duplicated between wasm-sdk and legacy paths. Consider extracting to a helper:
+ // Helper to update apps with the new contract + const updateAppsWithContract = (updatedContract: any) => { + for (const appName of this.client.getApps().getNames()) { + const appDefinition = this.client.getApps().get(appName); + if (appDefinition.contractId.equals(updatedContract.getId()) && appDefinition.contract) { + appDefinition.contract = updatedContract; + } + } + }; // In wasm-sdk path: - // Update app with updated data contract if available - // eslint-disable-next-line - for (const appName of this.client.getApps().getNames()) { - const appDefinition = this.client.getApps().get(appName); - if (appDefinition.contractId.equals(updatedDataContract.getId()) && appDefinition.contract) { - appDefinition.contract = updatedDataContract; - } - } + updateAppsWithContract(updatedDataContract); // In legacy path: - // Update app with updated data contract if available - // eslint-disable-next-line - for (const appName of this.client.getApps().getNames()) { - const appDefinition = this.client.getApps().get(appName); - if (appDefinition.contractId.equals(updatedDataContract.getId()) && appDefinition.contract) { - appDefinition.contract = updatedDataContract; - } - } + updateAppsWithContract(updatedDataContract);Also applies to: 91-98
packages/js-dash-sdk/package.json (1)
134-136: Consider the implications of using a local file path for the wasm-sdk dependency.Using
"file:../wasm-sdk/pkg"has several implications:
- Requires wasm-sdk to be built locally before installing js-dash-sdk
- Won't work for external consumers installing from npm
- May cause issues in CI/CD environments
Consider:
- Publishing @dashevo/wasm-sdk to npm (even as a pre-release)
- Using a git URL with a specific commit/tag
- Documenting the build requirements clearly
packages/platform-test-suite/WASM_SDK_TEST_ISSUES.md (1)
83-83: Clarify which platform method files were modifiedThe documentation mentions "Various platform method files - Migrated to use wasm-sdk" but doesn't specify which files. Consider listing the specific files or file patterns for better traceability.
packages/js-dash-sdk/src/SDK/Client/Platform/adapters/WasmPlatformAdapter.ts (2)
114-118: Complete regtest network configurationThe TODO comment indicates that regtest/local networks need custom URL configuration but currently fall back to testnet, which could cause connection issues.
Would you like me to implement proper regtest network configuration that accepts custom DAPI URLs from the constructor?
205-208: Add error logging for DPP conversion failuresWhen DPP identity creation fails, the error is silently caught. Consider logging the error to help debug compatibility issues during migration.
} catch (e) { + console.debug('Failed to convert identity using DPP, falling back to raw object:', e); // Fallback to raw object if DPP creation fails return wasmIdentity; }packages/js-dash-sdk/src/SDK/Client/Platform/Platform.ts (2)
116-117: Document the deprecation timeline for legacy DPPThe comment indicates DPP will be removed once migration is complete, but there's no timeline or migration guide. Consider adding more specific deprecation information.
-// Legacy DPP - will be removed once migration is complete +// Legacy DPP - deprecated, will be removed in v4.0.0 +// Migration guide: https://docs.dash.org/sdk/migration/wasm-sdk dpp?: any;
266-271: Add error handling to dispose methodThe dispose method should handle potential errors during adapter disposal to ensure cleanup completes even if adapter disposal fails.
async dispose(): Promise<void> { if (this.adapter) { - await this.adapter.dispose(); + try { + await this.adapter.dispose(); + } catch (error) { + this.logger.error('Error disposing adapter:', error); + } } this.wasmSdk = undefined; }packages/js-dash-sdk/src/SDK/Client/Platform/types/wasm-sdk.d.ts (1)
14-17: Document the numeric enum valuesThe numeric values for type, purpose, and securityLevel should reference the actual enum names for better maintainability.
export interface IdentityPublicKey { id: number; - type: number; // 0 = ECDSA_SECP256K1, 1 = BLS12_381, 2 = ECDSA_HASH160 - purpose: number; // 0 = AUTHENTICATION, 1 = ENCRYPTION, 2 = DECRYPTION, 3 = TRANSFER - securityLevel: number; // 0 = MASTER, 1 = CRITICAL, 2 = HIGH, 3 = MEDIUM + type: number; // KeyType: 0 = ECDSA_SECP256K1, 1 = BLS12_381, 2 = ECDSA_HASH160 + purpose: number; // KeyPurpose: 0 = AUTHENTICATION, 1 = ENCRYPTION, 2 = DECRYPTION, 3 = TRANSFER + securityLevel: number; // SecurityLevel: 0 = MASTER, 1 = CRITICAL, 2 = HIGH, 3 = MEDIUM data: string; // Base64-encoded public key readOnly: boolean; }
| ["@dashevo/wasm-sdk", [\ | ||
| ["file:../wasm-sdk/pkg#../wasm-sdk/pkg::hash=944b57&locator=dash%40workspace%3Apackages%2Fjs-dash-sdk", {\ | ||
| "packageLocation": "./.yarn/cache/@dashevo-wasm-sdk-file-7f6fe61b82-1ccf5cd50c.zip/node_modules/@dashevo/wasm-sdk/",\ | ||
| "packageDependencies": [\ | ||
| ["@dashevo/wasm-sdk", "file:../wasm-sdk/pkg#../wasm-sdk/pkg::hash=944b57&locator=dash%40workspace%3Apackages%2Fjs-dash-sdk"]\ | ||
| ],\ | ||
| "linkType": "HARD"\ | ||
| }]\ |
There was a problem hiding this comment.
Avoid committing the auto-generated .pnp.cjs file
.pnp.cjs is regenerated every time yarn install runs and is highly environment-specific. Keeping it under version control:
• Bloats the diff and repository size
• Causes constant merge conflicts between branches
• Risks leaking absolute cache paths that break on other machines
Move .pnp.cjs (and the accompanying .pnp.loader.mjs) to .gitignore and drop it from the PR; rely on yarn install to recreate it in CI/dev environments.
🤖 Prompt for AI Agents
In the .pnp.cjs file around lines 3067 to 3074, the issue is that this
auto-generated file should not be committed to version control because it is
environment-specific and causes repository bloat, merge conflicts, and potential
path leaks. To fix this, remove .pnp.cjs and .pnp.loader.mjs from the PR, add
both files to the .gitignore file to prevent future commits, and rely on running
yarn install to regenerate them in development and CI environments.
| @@ -8572,6 +8581,7 @@ | |||
| ["@dashevo/masternode-reward-shares-contract", "workspace:packages/masternode-reward-shares-contract"],\ | |||
| ["@dashevo/wallet-lib", "workspace:packages/wallet-lib"],\ | |||
| ["@dashevo/wasm-dpp", "workspace:packages/wasm-dpp"],\ | |||
| ["@dashevo/wasm-sdk", "file:../wasm-sdk/pkg#../wasm-sdk/pkg::hash=944b57&locator=dash%40workspace%3Apackages%2Fjs-dash-sdk"],\ | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Workspace dependency should be declared in package.json, not patched via PnP state
The extra entry that injects @dashevo/wasm-sdk as a file:../wasm-sdk/pkg dependency belongs in packages/js-dash-sdk/package.json (e.g. "@dashevo/wasm-sdk": "workspace:*").
Editing/committing PnP runtime state is brittle—any subsequent yarn install will overwrite these lines. Please ensure the dependency is declared in the proper manifest and let Yarn regenerate .pnp.cjs automatically.
🤖 Prompt for AI Agents
In the .pnp.cjs file at line 8584, the dependency on @dashevo/wasm-sdk is
incorrectly patched directly in the PnP runtime state. Remove this manual patch
from .pnp.cjs and instead declare "@dashevo/wasm-sdk": "workspace:*" as a
dependency in the packages/js-dash-sdk/package.json file. Then run yarn install
to regenerate the .pnp.cjs file automatically with the correct workspace
dependency.
| // Node.js environment - use initSync with buffer | ||
| const fs = require('fs'); | ||
| const path = require('path'); | ||
| const wasmPath = path.join(__dirname, '..', '..', '..', '..', '..', '..', '..', '.yarn', 'cache', '@dashevo-wasm-sdk-file-7f6fe61b82-1ccf5cd50c.zip', 'node_modules', '@dashevo', 'wasm-sdk', 'wasm_sdk_bg.wasm'); |
There was a problem hiding this comment.
Avoid hardcoded WASM file path
The hardcoded path to the WASM file in the Yarn cache is brittle and will break if the package version or cache structure changes. This path construction is highly specific to the current Yarn PnP setup.
Consider using a more robust approach:
-const wasmPath = path.join(__dirname, '..', '..', '..', '..', '..', '..', '..', '.yarn', 'cache', '@dashevo-wasm-sdk-file-7f6fe61b82-1ccf5cd50c.zip', 'node_modules', '@dashevo', 'wasm-sdk', 'wasm_sdk_bg.wasm');
+// Use require.resolve to find the wasm file relative to the wasm-sdk package
+const wasmSdkPath = require.resolve('@dashevo/wasm-sdk/package.json');
+const wasmPath = path.join(path.dirname(wasmSdkPath), 'wasm_sdk_bg.wasm');📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const wasmPath = path.join(__dirname, '..', '..', '..', '..', '..', '..', '..', '.yarn', 'cache', '@dashevo-wasm-sdk-file-7f6fe61b82-1ccf5cd50c.zip', 'node_modules', '@dashevo', 'wasm-sdk', 'wasm_sdk_bg.wasm'); | |
| // Use require.resolve to find the wasm file relative to the wasm-sdk package | |
| const wasmSdkPath = require.resolve('@dashevo/wasm-sdk/package.json'); | |
| const wasmPath = path.join(path.dirname(wasmSdkPath), 'wasm_sdk_bg.wasm'); |
🤖 Prompt for AI Agents
In packages/js-dash-sdk/src/SDK/Client/Platform/adapters/WasmPlatformAdapter.ts
at line 68, the WASM file path is hardcoded with deep relative paths pointing to
the Yarn cache, which is fragile and prone to breakage with version or structure
changes. Replace this hardcoded path by dynamically resolving the WASM file
location using package resolution methods such as require.resolve or importing
the path from the package's metadata, ensuring the path adapts to different
environments and package versions.
| convertAssetLockProof(assetLock: any): string { | ||
| // Convert asset lock from js-dash-sdk format to wasm-sdk hex format | ||
| const proofObject = { | ||
| type: assetLock.type || 0, | ||
| transaction: assetLock.transaction.toString('hex'), | ||
| outputIndex: assetLock.outputIndex, | ||
| instantLock: assetLock.instantLock ? assetLock.instantLock.toString('hex') : undefined, | ||
| }; | ||
|
|
||
| return JSON.stringify(proofObject); | ||
| } |
There was a problem hiding this comment.
Inconsistent return format in convertAssetLockProof
The method comment mentions converting to "wasm-sdk hex format" but the implementation returns a JSON string. Either update the comment or the implementation to match the expected format.
🤖 Prompt for AI Agents
In packages/js-dash-sdk/src/SDK/Client/Platform/adapters/WasmPlatformAdapter.ts
around lines 135 to 145, the method convertAssetLockProof's comment states it
returns a wasm-sdk hex format, but it actually returns a JSON string. To fix
this, either update the comment to accurately describe that the method returns a
JSON string representation of the proof object or modify the implementation to
return the asset lock proof in the expected wasm-sdk hex format, ensuring
consistency between the comment and the code.
| const wasmAdapter = adapter as any; | ||
| return await wasmAdapter.wasmSdk.data_contract_fetch(sdk, contractIdString); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove unsafe type assertion.
The type assertion as any bypasses TypeScript's type safety. Consider properly typing the adapter or using a more specific interface.
- const wasmAdapter = adapter as any;
- return await wasmAdapter.wasmSdk.data_contract_fetch(sdk, contractIdString);
+ return await adapter.wasmSdk.data_contract_fetch(sdk, contractIdString);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const wasmAdapter = adapter as any; | |
| return await wasmAdapter.wasmSdk.data_contract_fetch(sdk, contractIdString); | |
| return await adapter.wasmSdk.data_contract_fetch(sdk, contractIdString); |
🤖 Prompt for AI Agents
In packages/js-dash-sdk/src/SDK/Client/Platform/methods/contracts/get.ts at
lines 42-43, remove the unsafe type assertion 'as any' on the adapter variable.
Instead, define or import a proper TypeScript interface that accurately
describes the adapter's structure, especially the wasmSdk property and its
data_contract_fetch method, and use that interface to type the adapter variable.
This will restore type safety and avoid bypassing TypeScript's checks.
| })); | ||
|
|
||
| const documentsJson = JSON.stringify(documentsData); | ||
| const putType = documents.replace?.includes(docs[0]) ? 'replace' : 'create'; |
There was a problem hiding this comment.
Critical: Incorrect put type determination for mixed document groups.
The current logic only checks if the first document in a group is a replacement, but a group could contain both create and replace operations for the same contract/type combination. This would cause all documents in the group to be treated with the same operation type.
Consider processing create and replace operations separately:
- const putType = documents.replace?.includes(docs[0]) ? 'replace' : 'create';
+ // Process create and replace operations separately
+ const createDocs = docs.filter(doc => documents.create?.includes(doc));
+ const replaceDocs = docs.filter(doc => documents.replace?.includes(doc));
+
+ if (createDocs.length > 0) {
+ // Process create operations
+ const result = await this.wasmSdk.documentsPut(
+ dataContractId,
+ documentType,
+ JSON.stringify(createDocs.map(doc => ({
+ id: doc.getId()?.toString(),
+ data: doc.getData(),
+ createdAt: doc.getCreatedAt(),
+ updatedAt: doc.getUpdatedAt(),
+ }))),
+ identityHex,
+ privateKeyWIF,
+ 'create'
+ );
+ results.push(result);
+ }
+
+ if (replaceDocs.length > 0) {
+ // Process replace operations
+ const result = await this.wasmSdk.documentsPut(
+ dataContractId,
+ documentType,
+ JSON.stringify(replaceDocs.map(doc => ({
+ id: doc.getId()?.toString(),
+ data: doc.getData(),
+ createdAt: doc.getCreatedAt(),
+ updatedAt: doc.getUpdatedAt(),
+ }))),
+ identityHex,
+ privateKeyWIF,
+ 'replace'
+ );
+ results.push(result);
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const putType = documents.replace?.includes(docs[0]) ? 'replace' : 'create'; | |
| - const putType = documents.replace?.includes(docs[0]) ? 'replace' : 'create'; | |
| + // Process create and replace operations separately | |
| + const createDocs = docs.filter(doc => documents.create?.includes(doc)); | |
| + const replaceDocs = docs.filter(doc => documents.replace?.includes(doc)); | |
| + | |
| + if (createDocs.length > 0) { | |
| + // Process create operations | |
| + const result = await this.wasmSdk.documentsPut( | |
| + dataContractId, | |
| + documentType, | |
| + JSON.stringify(createDocs.map(doc => ({ | |
| + id: doc.getId()?.toString(), | |
| + data: doc.getData(), | |
| + createdAt: doc.getCreatedAt(), | |
| + updatedAt: doc.getUpdatedAt(), | |
| + }))), | |
| + identityHex, | |
| + privateKeyWIF, | |
| + 'create' | |
| + ); | |
| + results.push(result); | |
| + } | |
| + | |
| + if (replaceDocs.length > 0) { | |
| + // Process replace operations | |
| + const result = await this.wasmSdk.documentsPut( | |
| + dataContractId, | |
| + documentType, | |
| + JSON.stringify(replaceDocs.map(doc => ({ | |
| + id: doc.getId()?.toString(), | |
| + data: doc.getData(), | |
| + createdAt: doc.getCreatedAt(), | |
| + updatedAt: doc.getUpdatedAt(), | |
| + }))), | |
| + identityHex, | |
| + privateKeyWIF, | |
| + 'replace' | |
| + ); | |
| + results.push(result); | |
| + } |
🤖 Prompt for AI Agents
In packages/js-dash-sdk/src/SDK/Client/Platform/methods/documents/broadcast.ts
at line 84, the current logic determines the putType based only on the first
document, which fails for mixed groups containing both create and replace
operations. To fix this, separate the documents into two groups: one for create
operations and one for replace operations, then process each group independently
to ensure correct handling of each operation type.
| }); | ||
|
|
||
| // Return the results | ||
| return results; |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Critical: Inconsistent return types between wasm-sdk and legacy paths.
The function returns different types:
- wasm-sdk path: array of results
- legacy path: documentsBatchTransition object
This inconsistency will break code that depends on the return value.
Also applies to: 208-208
🏁 Script executed:
#!/bin/bash
# Description: Find all usages of broadcast method to understand expected return type
# Search for broadcast method calls
rg -A 5 "\.broadcast\(" --glob "*.ts" --glob "*.js" | grep -v "test" | head -20
# Search for type definitions or interfaces that might define the expected return type
ast-grep --pattern 'broadcast($_): $_'Length of output: 1178
🏁 Script executed:
#!/bin/bash
set -e
# Display relevant portion around wasm-sdk return
sed -n '140,165p' packages/js-dash-sdk/src/SDK/Client/Platform/methods/documents/broadcast.ts
# Display relevant portion around legacy return
sed -n '200,225p' packages/js-dash-sdk/src/SDK/Client/Platform/methods/documents/broadcast.tsLength of output: 1118
Fix Required: Unify broadcast return type
The broadcast method in
packages/js-dash-sdk/src/SDK/Client/Platform/methods/documents/broadcast.ts
returns two different types depending on the code path:
- Lines 148–153 (wasm-sdk path):
// …returns an array of wasm-sdk results return results; // DppDocumentsBroadcastResponse[]
- Lines 221–225 (legacy DPP path):
// …returns a single DocumentsBatchTransition return documentsBatchTransition; // DocumentsBatchTransition
This mismatch will break callers expecting a consistent payload.
Recommendations:
- Update the method signature to reflect a union return type, e.g.
public async broadcast( documents: DocumentBatch, identity: Identity, ): Promise<DppDocumentsBroadcastResponse[] | DocumentsBatchTransition> { … }
- Or—preferably—normalize both branches to return the same shape (e.g. always an array), wrapping the legacy
documentsBatchTransitionin a one-item array. - Adjust any downstream type definitions and documentation to match the new return type.
Please apply these changes to ensure callers can rely on a single, consistent return type.
🤖 Prompt for AI Agents
In packages/js-dash-sdk/src/SDK/Client/Platform/methods/documents/broadcast.ts
at line 153 and around lines 221-225, the broadcast method returns inconsistent
types: an array of DppDocumentsBroadcastResponse in the wasm-sdk path and a
single DocumentsBatchTransition in the legacy DPP path. To fix this, update the
method to always return the same type by wrapping the legacy
DocumentsBatchTransition return value in a one-item array, ensuring both paths
return an array. Also, update the method signature to reflect this consistent
array return type and adjust any related type definitions and documentation
accordingly.
| // Convert startAt/startAfter | ||
| let startAt = opts.startAt; | ||
| let startAfter = opts.startAfter; | ||
|
|
||
| if (startAt instanceof ExtendedDocument) { | ||
| startAt = startAt.getId().toString(); | ||
| } else if (typeof startAt === 'string') { | ||
| startAt = Identifier.from(startAt).toString(); | ||
| } else if (startAt && typeof startAt === 'object') { | ||
| startAt = startAt.toString(); | ||
| } | ||
|
|
||
| if (startAfter instanceof ExtendedDocument) { | ||
| startAfter = startAfter.getId().toString(); | ||
| } else if (typeof startAfter === 'string') { | ||
| startAfter = Identifier.from(startAfter).toString(); | ||
| } else if (startAfter && typeof startAfter === 'object') { | ||
| startAfter = startAfter.toString(); | ||
| } | ||
|
|
||
| // Call wasm-sdk getDocuments | ||
| const result = await this.wasmSdk.getDocuments( | ||
| appDefinition.contractId.toString(), | ||
| fieldType, | ||
| whereClause, | ||
| orderByClause, | ||
| opts.limit, | ||
| startAt ? parseInt(startAt as string) : undefined, | ||
| startAfter ? parseInt(startAfter as string) : undefined | ||
| ); |
There was a problem hiding this comment.
Critical: Incorrect identifier to integer conversion
The code converts startAt and startAfter to identifier strings (lines 130-143) but then attempts to parse them as integers (lines 152-153). Identifier strings are not numeric values and parseInt will likely return NaN or incorrect values.
The wasm-sdk likely expects the identifier strings directly, not parsed as integers:
- startAt ? parseInt(startAt as string) : undefined,
- startAfter ? parseInt(startAfter as string) : undefined
+ startAt as string | undefined,
+ startAfter as string | undefined🤖 Prompt for AI Agents
In packages/js-dash-sdk/src/SDK/Client/Platform/methods/documents/get.ts around
lines 125 to 154, the code converts startAt and startAfter to identifier strings
but then incorrectly parses them as integers before passing to
wasmSdk.getDocuments. Remove the parseInt calls and pass startAt and startAfter
directly as strings to the getDocuments method, ensuring the identifiers are
used correctly without numeric conversion.
|
|
||
| this.logger.debug(`[Identity#creditTransfer] Transferred ${amount} credits from ${identity.getId().toString()} to ${recipientId.toString()}`); | ||
|
|
||
| return result.success !== false; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Simplify return value logic and add null check.
The double negative result.success !== false is confusing and doesn't handle undefined/null cases properly:
- return result.success !== false;
+ return result.success === true;Or if the API can return undefined:
- return result.success !== false;
+ return result.success ?? false;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| return result.success !== false; | |
| return result.success ?? false; |
🤖 Prompt for AI Agents
In
packages/js-dash-sdk/src/SDK/Client/Platform/methods/identities/creditTransfer.ts
at line 44, simplify the return statement by directly returning a boolean that
checks if result.success is truthy, and add a null or undefined check for result
to avoid runtime errors. Replace the current double negative logic with a
clearer expression that safely handles cases where result or result.success
might be undefined or null.
Issue being fixed or feature implemented
What was done?
How Has This Been Tested?
Breaking Changes
Checklist:
For repository code-owners and collaborators only
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Refactor
Chores