refactor(dapi)!: expose Uint8Array instead of Buffer in public API of dapi-client#3680
refactor(dapi)!: expose Uint8Array instead of Buffer in public API of dapi-client#3680PastaPastaPasta wants to merge 2 commits into
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
✅ DashSDKFFI.xcframework built for this PR.
SwiftPM (host the zip at a stable URL, then use): .binaryTarget(
name: "DashSDKFFI",
url: "https://your.cdn.example/DashSDKFFI.xcframework.zip",
checksum: "bfa0ce328feabc7b4f6d66a72c6cdc47229bcc9ec155eaa86e41747b372d302b"
)Xcode manual integration:
|
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Two genuine blocking regressions in this Buffer→Uint8Array refactor: (1) the new hexToBytes helper silently substitutes 0x00 for invalid hex pairs, where Buffer.from(x, 'hex') truncates — affecting public request builders that wire user-supplied hex straight into protobuf setters, and (2) GetIdentityByPublicKeyHashResponse now returns Uint8Array, which the downstream IdentitySyncWorker explicitly rejects via Buffer.isBuffer() before calling Identity.fromBuffer. Several smaller suggestions (missing direct unit tests for bytes.js, a test mock that violates the dashcore-lib Buffer rule the PR otherwise honors, and minor cleanup) are also valid.
Reviewed commit: 0b8c68e
🔴 2 blocking | 🟡 3 suggestion(s) | 💬 1 nitpick(s)
6 additional finding(s) could not be mapped inline and are included below.
1. blocking: `hexToBytes` silently fabricates zero bytes for invalid hex input (packages/js-dapi-client/lib/utils/bytes.js:8)
parseInt('zz', 16) returns NaN, which coerces to 0 when written into a Uint8Array. So hexToBytes('zz') returns Uint8Array([0]) and hexToBytes('abxx') returns Uint8Array([0xab, 0x00]). The previous Buffer.from(hex, 'hex') calls this replaces instead truncated at the first non-hex character (Buffer.from('zz', 'hex') is empty; Buffer.from('abxx', 'hex') is <ab>).
This helper is now wired into the public request surface: getProtocolVersionUpgradeVoteStatusFactory.js:35 pipes startProTxHash straight through hexToBytes into setStartProTxHash, and the same pattern is used by subscribeToBlockHeadersWithChainLocksFactory and subscribeToTransactionsWithProofsFactory. A typo in a caller's hex string that previously failed cleanly (or sent partial bytes) now silently becomes a zero-padded request that the server will dispatch against an unrelated key. Reject non-hex characters explicitly.
2. blocking: Returning `Uint8Array` breaks the wallet-lib → wasm-dpp identity sync path (packages/js-dapi-client/lib/methods/platform/getIdentityByPublicKeyHash/GetIdentityByPublicKeyHashResponse.js:32)
createFromProto now returns new Uint8Array(identity) instead of a Node Buffer. That value crosses the package boundary into packages/wallet-lib/src/transport/DAPIClientTransport/methods/getIdentityByPublicKeyHash.js, which forwards it unchanged. packages/wallet-lib/src/plugins/Workers/IdentitySyncWorker.js:98 then runs if (!Buffer.isBuffer(identityBuffer)) throw new Error(...) before calling Identity.fromBuffer(...). A plain Uint8Array fails that guard, so identity sync now throws on every successful lookup.
The PR notes claim downstream consumers don't need updates, but this is a runtime regression in the in-repo consumer. Either return a Buffer here (the public API surface that downstream code depends on), or update IdentitySyncWorker to accept Uint8Array and convert at the wasm-dpp boundary.
3. suggestion: New `bytes.js` utility has no direct unit tests (packages/js-dapi-client/lib/utils/bytes.js:1)
This module is foundational — ~10 files in dapi-client depend on it (response classes, fixtures, createGrpcTransportError, mock fixtures). Its correctness is currently only verified transitively. A direct spec covering at minimum the edge cases below would have caught the hexToBytes regression flagged above:
hexToByteson invalid hex characters, odd-length input, and empty stringbase64ToByteson URL-safe alphabet (-,_) whichBuffer.from(x, 'base64')acceptsbytesToHex/bytesToBase64on empty inputconcatBytes([])and on single-element arraysbytesEqualon different-length inputs
Add test/unit/utils/bytes.spec.js so these behaviors are pinned down before the planned ESM migration touches this file.
4. suggestion: Test mock passes `Uint8Array` fields into `BlockHeader` — same boundary the PR otherwise honors (packages/js-dapi-client/lib/test/mocks/mockHeadersChain.js:40)
The PR explicitly preserves new BlockHeader(Buffer.from(header)) at BlockHeadersReader.js:125,241 because @dashevo/dashcore-lib's BlockHeader relies on Buffer-only methods (BufferReader.readInt32LE, Buffer.concat, toString('hex')). This mock now hands the same constructor a plain Uint8Array prevHash and merkleRoot. Even if the current test paths don't exercise the serialization helpers, the boundary rule the PR is explicit about elsewhere is violated here — any future test that hashes or serializes one of these mock headers will silently produce wrong bytes (e.g. Uint8Array.toString('hex') returns '[object Uint8Array]') or throw from Buffer.concat. Wrap with Buffer.from(...) like the production code does.
5. suggestion: `bytesEqual` is non-constant-time and exported for future use (packages/js-dapi-client/lib/utils/bytes.js:48)
bytesEqual exits at the first differing byte, leaking the position of the first mismatch via timing. It is currently exported but has no callers in this PR. Sitting next to hexToBytes/base64ToBytes it will be the natural choice for any future caller comparing quorum hashes, signatures, MACs, or dash-serialized-consensus-error-bin digests. Rewrite as constant-time (XOR accumulator over the full length), or drop the export until a non-secret use case lands, or rename to bytesEqualUnsafe so the timing semantics are obvious at call sites.
6. nitpick: Triplicated `typeof === 'string' ? base64ToBytes : new Uint8Array` could be a helper (packages/js-dapi-client/lib/transport/GrpcTransport/createGrpcTransportError.js:63)
The same dual-format normalization (typeof x === 'string' ? base64ToBytes(x) : new Uint8Array(x)) and the explanatory grpc-js-vs-grpc-web comment are repeated three times in this file (drive-error-data-bin, stack-bin, dash-serialized-consensus-error-bin). A tiny helper co-located in utils/bytes.js (e.g. metadataBinToBytes) would DRY it up and make the eventual removal of grpc-web support (per the existing TODO at parseMetadata.js) a one-line change.
Inline posting hit GitHub HTTP 422, so I posted the same verified findings as a top-level review body.
0b8c68e to
0eb8b2c
Compare
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Two blocking bugs confirmed at HEAD 0eb8b2c: hexToBytes silently inserts 0x00 for any non-hex character (parseInt → NaN → 0 in Uint8Array), now reachable from three public request factories; and GetIdentityByPublicKeyHashResponse returns Uint8Array which IdentitySyncWorker.js:98 explicitly rejects via Buffer.isBuffer() before calling Identity.fromBuffer. Four non-blocking findings (missing unit tests for bytes.js, mock passing Uint8Array into Buffer-only BlockHeader, non-constant-time bytesEqual export, triplicated metadata-bin normalization) are all corroborated by direct file reads.
🔴 2 blocking | 🟡 3 suggestion(s) | 💬 1 nitpick(s)
6 additional finding(s) omitted (not in diff).
Findings not attached inline (unchanged lines / prior carried-forward findings)
-
**[blocking] packages/js-dapi-client/lib/utils/bytes.js —
hexToBytessilently fabricates zero bytes for invalid hex inputparseInt(hex.substr(i*2, 2), 16)returnsNaNfor any non-hex character, andNaNcoerces to0when assigned into aUint8Array. SohexToBytes('zz')returnsUint8Array([0])andhexToBytes('abxx')returnsUint8Array([0xab, 0x00]). TheBuffer.from(hex, 'hex')calls this replaces truncated at the first non-hex character (Buffer.from('zz', 'hex')is empty;Buffer.from('abxx', 'hex')is<ab>) — different behavior, but at least non-fabricating.
This helper is wired straight through the public request surface: getProtocolVersionUpgradeVoteStatusFactory.js pipes startProTxHash through hexToBytes into setStartProTxHash, and the same pattern is used in subscribeToBlockHeadersWithChainLocksFactory and subscribeToTransactionsWithProofsFactory. A typo in a caller's hex string that previously failed cleanly (or sent partial bytes) now silently becomes a zero-padded request the server will execute against an unrelated key. Add an explicit Number.isNaN check per byte (or a /^[0-9a-fA-F]*$/ test up front) and throw on invalid input.
-
**[blocking] packages/js-dapi-client/lib/methods/platform/getIdentityByPublicKeyHash/GetIdentityByPublicKeyHashResponse.js — Returning
Uint8Arraybreaks the wallet-lib → wasm-dpp identity sync pathcreateFromProtonow constructs the response withnew Uint8Array(identity)instead of a NodeBuffer. That value crosses package boundaries viapackages/wallet-lib/src/transport/DAPIClientTransport/methods/getIdentityByPublicKeyHash.js, which returnsresponse.getIdentity()unchanged.packages/wallet-lib/src/plugins/Workers/IdentitySyncWorker.js:98then guards withif (!Buffer.isBuffer(identityBuffer)) throw new Error(...)before callingIdentity.fromBuffer(...). A plainUint8ArrayfailsBuffer.isBuffer, so identity sync now throws on every successful lookup.
The PR notes claim downstream consumers don't need updates, but this is a runtime regression in the in-repo consumer. Either return a Buffer from this public API (matching the documented Promise<Buffer> contract in the wallet-lib transport method's JSDoc), or update IdentitySyncWorker to accept Uint8Array and convert at the wasm-dpp boundary. Identical patterns likely exist for other Response classes that flipped from Buffer to Uint8Array — audit the full set.
-
**[suggestion] packages/js-dapi-client/lib/utils/bytes.js — New
bytes.jsutility has no direct unit testsThis module is foundational — response classes, fixtures,
createGrpcTransportError, mock fixtures, and three public request factories depend on it. Its correctness is currently only verified transitively. A direct spec covering at minimum these edge cases would have caught thehexToBytesregression above: -
hexToByteson invalid hex characters, odd-length input, and empty string -
base64ToByteson URL-safe alphabet (-,_) whichBuffer.from(x, 'base64')accepted -
bytesToHex/bytesToBase64on empty input -
concatBytes([])and on single-element arrays -
bytesEqualon different-length inputs and identical inputs
Add test/unit/utils/bytes.spec.js so these behaviors are pinned down before the planned ESM migration touches this file.
-
**[suggestion] packages/js-dapi-client/lib/test/mocks/mockHeadersChain.js — Test mock passes
Uint8ArrayintoBlockHeader— same boundary the PR otherwise honorsThe PR explicitly preserves
new BlockHeader(Buffer.from(header))atBlockHeadersReader.js:125,241because@dashevo/dashcore-lib'sBlockHeaderrelies on Buffer-only methods (BufferReader.readInt32LE,Buffer.concat,toString('hex')). This mock now hands the same constructor a plainUint8ArrayprevHash(fromhexToBytes(...).reverse()) and a plainUint8Array(32)merkleRoot. Even if current test paths don't exercise serialization helpers, the boundary rule the PR is explicit about elsewhere is violated here — any future test that hashes or serializes one of these mock headers will silently produce wrong bytes (e.g.Uint8Array.prototype.toString('hex')ignores the radix arg and returns a comma-joined decimal string) or throw fromBuffer.concat. Wrap the two byte fields withBuffer.from(...)like the production code does. -
**[suggestion] packages/js-dapi-client/lib/utils/bytes.js —
bytesEqualis non-constant-time and exported for future usebytesEqualshort-circuits at the first differing byte, leaking the position of the first mismatch via timing. It is currently exported but has no callers in this PR. Sitting next tohexToBytes/base64ToBytesit will be the natural choice for any future caller comparing quorum hashes, signatures, MACs, ordash-serialized-consensus-error-bindigests. Either rewrite as constant-time (XOR accumulator over the full common length, then compare lengths last), drop the export until a non-secret use case lands, or rename tobytesEqualUnsafeso the timing semantics are obvious at call sites. -
**[nitpick] packages/js-dapi-client/lib/transport/GrpcTransport/createGrpcTransportError.js — Triplicated
typeof === 'string' ? base64ToBytes : new Uint8Arraycould be a helperThe same dual-format normalization (
typeof x === 'string' ? base64ToBytes(x) : new Uint8Array(x)) and the explanatory grpc-js-vs-grpc-web comment are repeated three times in this file:drive-error-data-binat lines 63–69,stack-binat lines 78–84,dash-serialized-consensus-error-binat lines 128–136. A tiny helper co-located inutils/bytes.js(e.g.metadataBinToBytes) would DRY it up and make the eventual removal of grpc-web support (per the existing TODO inparseMetadata.js) a one-line change.
Inline posting failed; posted the verified findings in the top-level review body.
0eb8b2c to
29cf55f
Compare
34a5c3e to
0698a39
Compare
…ton/fetch/promisify shims
Non-breaking cleanup pass; package stays CJS, no public API changes, no consumer changes required.
dapi-client: replace winston with a minimal console-backed logger that preserves the same API (.error/.warn/.info/.verbose/.debug/.silly/.getForId). Drop node-fetch and the lib/test/bootstrap setimmediate shim — Node 18+ has both globally. Drop the https.Agent self-signed-TLS branch from requestJsonRpc (was Node-only; consumers wanting this must configure NODE_TLS_REJECT_UNAUTHORIZED at the app layer). Inline lodash/sample in ListDAPIAddressProvider. Add engines.node >=18.18. Remove dependencies: winston, node-fetch, lodash, bs58 (unused), node-inspect-extracted (unused). Remove devDeps: setimmediate.
dapi-grpc: inline the promisify shim in core/v0/web/CorePromiseClient.js and platform/v0/web/PlatformPromiseClient.js so the browser bundle no longer requires Node's util module. Both files document the shim so a future codegen regen does not silently reintroduce require('util').
… API
Adds lib/utils/bytes.js helper (hexToBytes/bytesToHex/base64ToBytes/bytesToBase64/concatBytes/bytesEqual) and converts all Buffer.* call sites in dapi-client lib/ to Uint8Array, with corresponding test updates. Package stays CJS.
Production exceptions where Buffer is retained: BlockHeadersReader passes Buffer to dashcore-lib's BlockHeader (its BufferReader needs .readInt32LE), and GetIdentitiesContractKeysResponse passes Buffer to wasm-dpp's Identifier.from (it explicitly requires Node Buffer).
createGrpcTransportError now handles both raw bytes (grpc-js path) and base64 strings (grpc-web path) for drive-error-data-bin, stack-bin, and dash-serialized-consensus-error-bin metadata fields, restoring the dual-format behavior that Buffer.from(x, 'base64') used to provide implicitly.
Test updates: spec files that construct expected protobuf requests now wrap .toBuffer() with new Uint8Array(...) to match production's normalization (sinon calledOnceWithExactly distinguishes Buffer from plain Uint8Array).
Breaking change for direct consumers: response object byte fields are now Uint8Array. Callers that do response.field.toString('hex') will fail — use bytesToHex(response.field) from lib/utils/bytes instead. Buffer.isBuffer(response.field) now returns false; use response.field instanceof Uint8Array.
Test results: dapi-client 315/315, wallet-lib 377/377, js-dash-sdk 60/60 — downstream consumers continue passing without modification (they exercise dapi-client mostly via mocks).
0698a39 to
cbdf276
Compare
Summary
Replaces
BufferwithUint8Arrayacross@dashevo/dapi-client's public API. Adds a smalllib/utils/bytes.jshelper for hex/base64/concat operations. Package stays CJS — that conversion happens in PR 3.This is PR 2 of 5 in the stacked series. Depends on #3679.
Why
Removes
Bufferfrom the dapi-client surface so consumers don't need aBufferpolyfill in browser bundles.Bufferis Node-specific;Uint8Arrayworks everywhere.What changes
dapi-client lib/
lib/utils/bytes.js:hexToBytes,bytesToHex,base64ToBytes,bytesToBase64,concatBytes,bytesEqual(CJS exports).Buffer.*call sites converted per the translation table in the commit message.Bufferis retained:BlockHeadersReader→new BlockHeader(Buffer.from(header))because dashcore-lib'sBufferReaderneeds.readInt32LE.GetIdentitiesContractKeysResponse→Identifier.from(Buffer.from(entry.getIdentityId()))because wasm-dpp'sIdentifierexplicitly requires NodeBuffer.createGrpcTransportError
Restored dual-format handling that
Buffer.from(x, 'base64')provided implicitly:typeof x === 'string' ? base64ToBytes(x) : new Uint8Array(x)for the three metadata-bin fields (grpc-js sends raw bytes; grpc-web sends base64 strings).Tests
Spec files that construct expected protobuf requests now wrap
.toBuffer()withnew Uint8Array(...)to match production's normalization. Sinon'scalledOnceWithExactlyis strict about Buffer-vs-Uint8Array distinction.Test plan
yarn workspace @dashevo/dapi-client run test:unit— 315 passingyarn workspace @dashevo/wallet-lib run test:unit— 377 passing (no regression)yarn workspace dash run test:unit— 60 passing (no regression)Downstream consumers (wallet-lib, js-dash-sdk) exercise dapi-client mostly through mocks, so they didn't need updates here.
Breaking changes
Public response objects now expose
Uint8Arrayinstead ofBuffer. Consumer code patterns that need updating:response.field.toString('hex')bytesToHex(response.field)response.field.toString('base64')bytesToBase64(response.field)Buffer.isBuffer(response.field)response.field instanceof Uint8Arrayresponse.field.slice(a, b)response.field.subarray(a, b)In Node,
Buffer extends Uint8Array, so anything that just reads bytes (passes tocrypto, writes to a stream, length checks, indexed access) continues to work identically.Stack