refactor(wallet-lib)!: convert to ESM, consume ESM dapi-client#3677
Closed
PastaPastaPasta wants to merge 4 commits into
Closed
refactor(wallet-lib)!: convert to ESM, consume ESM dapi-client#3677PastaPastaPasta wants to merge 4 commits into
PastaPastaPasta wants to merge 4 commits into
Conversation
…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).
Converts @dashevo/dapi-client to pure ESM: type: module, exports map with ./lib/* wildcard, all CJS require/module.exports replaced with import/export, .js extensions on relative imports. Engine bumped to >=18.18. Drops webpack.config.js, karma.conf.js, and lib/test/karma/. Removes the @babel/core, babel-loader, browser-polyfill, karma, and webpack devDeps. Consumers must use a modern bundler (Vite/esbuild/webpack 5) which handles ESM natively. Moves 'events' npm package from devDependencies to dependencies — used by DAPIClient, ReconnectableStream, and BlockHeadersProvider for EventEmitter, and resolves correctly in both Node and browser bundlers. BREAKING: CJS consumers (require) no longer work. Downstream consumers wallet-lib (PR 4) and js-dash-sdk (PR 5) are temporarily broken between this PR merging and PRs 4/5 merging — they must land as a sequence. Dashmate is already ESM and continues to work. Test results: dapi-client 315/315. wallet-lib + js-dash-sdk fail as expected (fixed by PRs 4 + 5).
Converts @dashevo/wallet-lib to pure ESM so it can consume the ESM dapi-client from PR 3. Adds 'type: module' to package.json. All src/, fixtures/, and tests/ files converted from CJS require/module.exports to ESM import/export with .js extensions on relative imports. Deletes webpack.config.js, karma/, src/test/karma/. Removes browser-polyfill devDeps (buffer, crypto-browserify, stream-browserify, etc.) and webpack/karma. Tests run via Mocha in Node 18+; browser builds are out of scope. Engines: >=18.18. CJS-named-import fixes: lodash, crypto-js, @dashevo/dashcore-lib, @dashevo/wasm-dpp, @dashevo/grpc-common all use default-import + destructure pattern because Node ESM cannot statically enumerate named exports of CJS modules. Surfaces three previously-silent CJS bugs that ESM strict mode catches: missing UnknownStrategy export from errors/index.js, missing named exports for coinSelection strategies, and a 'type=' implicit-global in DerivableKeyChain.spec.js. Defensive (loadDpp.default ?? loadDpp)() unwrap in IdentitySyncWorker.onStart for the same wasm-dpp NodeNext interop reason as PR 3's bootstrap. Test results: wallet-lib 377/377 passing. dapi-client unchanged (315/315). js-dash-sdk still broken (PR 5 fixes).
Contributor
|
Important Review skippedToo many files! This PR contains 288 files, which is 138 over the limit of 150. To get a review, narrow the scope: ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (12)
📒 Files selected for processing (288)
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 |
4 tasks
Member
Author
|
Closing — reopening as upstream→upstream so CI runs against secrets/runners. New PR link will be added shortly. |
Member
Author
|
Reopened as #3682 (upstream→upstream so CI runs against repo secrets/runners). |
thepastaclaw
suggested changes
May 19, 2026
Collaborator
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
This PR leaves @dashevo/wallet-lib in an incompatible intermediate state. The wasm-dpp worker report does not hold up against the package's actual CommonJS interop pattern, but the new ESM wallet-lib packaging does break current workspace consumers through unresolved extensionless deep imports and CommonJS callers that now receive ESM namespace objects instead of callable defaults.
Reviewed commit: f6bb12e8 (f6bb12e88900be8a1fddcc67e08de540b9b9c3db)
🔴 1 blocking
GitHub refused the PR diff API for this large PR (>300 files), so I’m posting the verified finding in the top-level review body instead of inline comments.
Findings
1. [blocking] packages/wallet-lib/package.json:5
Switching wallet-lib to ESM breaks existing deep-import consumers in this repo
Declaring @dashevo/wallet-lib as "type": "module" while exposing raw ./src/* entrypoints leaves existing workspace consumers on the old import shape. packages/platform-test-suite/lib/test/createClientWithFundedWallet.js:3 and packages/bench-suite/lib/client/createClientWithFundedWallet.js:5 both still do const fundWallet = require('@dashevo/wallet-lib/src/utils/fundWallet');. With the new package exports map, that extensionless subpath maps to ./src/utils/fundWallet exactly rather than ./src/utils/fundWallet.js; once corrected to .js, it is still an ESM file and require() cannot load it as the old callable CommonJS export. packages/js-dash-sdk/src/SDK/Client/Client.ts:3 has the same unresolved extensionless deep import for @dashevo/wallet-lib/src/transport/DAPIClientTransport/DAPIClientTransport. So this PR cannot be merged by itself without breaking monorepo consumers; migrate the workspace consumers in this PR, or keep the compatibility surface until the follow-up PR lands.
Inline posting could not be used because GitHub’s PR diff endpoint rejected the oversized diff; this review intentionally covers the assigned SHA.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Converts
@dashevo/wallet-libto pure ESM so it canimportthe ESMdapi-clientfrom PR 3. Drops webpack/karma and the browser-polyfill devDeps.This is PR 4 of 5 in the stacked series. Depends on #3674, #3675, and #3676.
What changes
wallet-lib
package.json:"type": "module",engines.node >= 18.18. Drops scriptsbuild:web,test:browsers*,prepublishOnly. Removes devDeps: allkarma-*,webpack, browser polyfills (buffer,crypto-browserify,https-browserify,os-browserify,path-browserify,process,stream-browserify,stream-http,string_decoder,url,util,assert,browserify-zlib,events,setimmediate). Removes depswinston(replaced with console-backed logger),node-inspect-extracted.src/,fixtures/, andtests/converted from CJS to ESM with.jsextensions on relative imports.src/logger/index.jsrewritten as a console-backed logger (matching the dapi-client pattern from PR 1).@dashevo/dapi-clientupdated to include.jsextensions (e.g.'@dashevo/dapi-client/lib/transport/ReconnectableStream.js').webpack.config.js,karma/,src/test/karma/..mocharc.ymlusesrequire:for the ESM bootstrap (works for this package's bootstrap shape under Mocha 11 + Node 18+).CJS-named-import conversions
Node ESM cannot statically enumerate named exports of CJS modules. So imports like
import { sortBy } from 'lodash';are replaced withimport lodash from 'lodash'; const { sortBy } = lodash;across all uses oflodash,crypto-js,@dashevo/dashcore-lib,@dashevo/wasm-dpp,@dashevo/grpc-common.Previously-silent CJS bugs that ESM strict mode catches
src/errors/index.js: addedUnknownStrategyto the named-exports list (the class file existed but was never re-exported).src/utils/coinSelections/strategies/index.js: added named exports forsimpleAscendingAccumulatorandsimpleDescendingAccumulator(previously only available via the defaultSTRATEGIESobject, butAccount.jswas importing them by name).src/types/DerivableKeyChain/DerivableKeyChain.spec.js: fixedtype='HDPublicKey'syntax error (an implicit-global assignment that worked in CJS sloppy mode but fails in ESM strict mode).Defensive wasm-dpp unwrap
src/plugins/Workers/IdentitySyncWorker.jsuses(loadDpp.default ?? loadDpp)()because wasm-dpp is CJS and the NodeNext default import sometimes resolves to the namespace object instead of the callable.Test plan
yarn workspace @dashevo/wallet-lib run test:unit— 377 passing, 2 pending, 0 failing ✅yarn workspace @dashevo/dapi-client run test:unit— 315 passing (no regression from PR 3)yarn workspace dash run test:unit— TS compile still fails on dapi-client deep imports (PR 5 fixes)Breaking changes
require('@dashevo/wallet-lib')breaks..jsextensions.bs58from deps if unused (verified via grep).Stack