chore(dapi): cleanup — drop unused deps, inline winston/fetch/promisify shims#3679
chore(dapi): cleanup — drop unused deps, inline winston/fetch/promisify shims#3679PastaPastaPasta wants to merge 3 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughRemoves Node-only dependencies and polyfills, inlines small helper shims, updates package metadata and Yarn PnP mappings (adds undici, adjusts node-fetch virtual ID), replaces Winston with a console-threshold logger, and adapts gRPC and transport code to avoid Node-only modules (util, https). ChangesReduce Node dependencies and enable browser compatibility
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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: "1a89265fcbf7dca1f2addd0833e6cf1fb73467cbd5fa7ad050bb94e5f1606a98"
)Xcode manual integration:
|
PastaPastaPasta
changed the title
PastaPastaPasta
force-pushed
the
claude/esm-1-cleanup
branch
from
9812cf7 to
2d952ac
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/js-dapi-client/lib/logger/index.js`:
- Around line 12-26: The prefixed logger currently passes the prefix as a
separate console argument (fmt), which breaks printf-style interpolation for
methods returned by getForId/build; change fmt so that when a prefix exists it
merges the prefix into the first argument if that first arg is a string (e.g.
transform (first, ...rest) => [prefix + ' ' + first, ...rest]) and otherwise
inserts the prefix before the first value (e.g. [prefix, first, ...rest]);
update the fmt helper used by logger.error/warn/info/verbose/debug/silly and the
getForId/build usage to use this new behavior so %s/%d formatting is preserved.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 56b6e1b7-0224-44e7-9c04-294c54bd62cf
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (11)
.pnp.cjspackages/dapi-grpc/clients/core/v0/web/CorePromiseClient.jspackages/dapi-grpc/clients/platform/v0/web/PlatformPromiseClient.jspackages/js-dapi-client/lib/dapiAddressProvider/ListDAPIAddressProvider.jspackages/js-dapi-client/lib/index.jspackages/js-dapi-client/lib/logger/index.jspackages/js-dapi-client/lib/test/bootstrap.jspackages/js-dapi-client/lib/test/karma/bootstrap.jspackages/js-dapi-client/lib/transport/JsonRpcTransport/requestJsonRpc.jspackages/js-dapi-client/package.jsonpackages/js-dapi-client/polyfills/fetch-polyfill.js
💤 Files with no reviewable changes (5)
- packages/js-dapi-client/lib/index.js
- packages/js-dapi-client/lib/test/bootstrap.js
- packages/js-dapi-client/lib/test/karma/bootstrap.js
- packages/js-dapi-client/lib/transport/JsonRpcTransport/requestJsonRpc.js
- packages/js-dapi-client/polyfills/fetch-polyfill.js
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
PR is a JS cleanup pass on @dashevo/dapi-client and dapi-grpc browser build. Three carried-over findings are still valid at 2d952ac: one blocking behavioral regression where the selfSigned parameter is accepted/propagated end-to-end but no longer has any effect (breaking regtest/local 127.0.0.1 self-signed flows that DAPIAddressProvider explicitly opts into), plus a stale test that no longer guards distinct behavior and a stale setimmediate devDependency that contradicts the PR description.
🔴 1 blocking | 🟡 1 suggestion(s) | 💬 1 nitpick(s)
3 additional finding(s)
blocking: `selfSigned` parameter is dead but still propagated, silently breaking regtest TLS
packages/js-dapi-client/lib/transport/JsonRpcTransport/requestJsonRpc.js (line 14)
requestJsonRpc still accepts selfSigned (line 14), copies it into requestInfo (line 28), and is called with address.isSelfSignedCertificateAllowed() by JsonRpcTransport.request(). The previous Node-only https.Agent({rejectUnauthorized: false}) branch is gone, so fetch(url, requestOptions) at line 49 ignores the flag entirely. This is not a theoretical issue: ListDAPIAddressProvider.getLiveAddress() rewrites local/regtest addresses to https://127.0.0.1 with allowSelfSignedCertificate = true. Under Node, JSON-RPC calls against the built-in local network config will now fail certificate validation where they previously succeeded. Because the API surface still advertises and emits selfSigned, this is a silent behavioral regression rather than an explicit breaking change. Either restore Node-side self-signed handling for the regtest path, or drop the parameter from the function signature, requestInfo, all callers (JsonRpcTransport, DAPIAddress, both address providers, and corresponding tests), and document the removed regtest support.
suggestion: Self-signed HTTPS test no longer exercises any distinct behavior
packages/js-dapi-client/test/unit/transport/JsonRpcTransport/requestJsonRpc.spec.js (line 76)
This test sets selfSigned = true, but with the https.Agent branch removed, its call path is identical to the plain HTTPS test on lines 52-74. Since globalThis.fetch is fully stubbed, the assertion only confirms the stub resolves to 'passed' — it can stay green while the production self-signed path is broken (which is exactly what is happening at this SHA). Either delete the test along with the selfSigned parameter, or rework it to assert on whatever mechanism actually controls TLS verification once that path is restored.
nitpick: `setimmediate` devDependency contradicts the cleanup description
packages/js-dapi-client/package.json (line 64)
The PR description/commit message states setimmediate was removed, but it is still listed as a devDependency here and is still required by lib/test/karma/bootstrap.js:1. The code and metadata are internally consistent (the require resolves), but the dependency-removal claim in the PR description is incorrect. Either fully drop the polyfill (require + devDependency) or update the description to reflect that it was retained for the karma browser bootstrap.
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
- [BLOCKING] In `packages/js-dapi-client/lib/transport/JsonRpcTransport/requestJsonRpc.js`:14-49: `selfSigned` parameter is dead but still propagated, silently breaking regtest TLS
`requestJsonRpc` still accepts `selfSigned` (line 14), copies it into `requestInfo` (line 28), and is called with `address.isSelfSignedCertificateAllowed()` by `JsonRpcTransport.request()`. The previous Node-only `https.Agent({rejectUnauthorized: false})` branch is gone, so `fetch(url, requestOptions)` at line 49 ignores the flag entirely. This is not a theoretical issue: `ListDAPIAddressProvider.getLiveAddress()` rewrites `local`/regtest addresses to `https://127.0.0.1` with `allowSelfSignedCertificate = true`. Under Node, JSON-RPC calls against the built-in local network config will now fail certificate validation where they previously succeeded. Because the API surface still advertises and emits `selfSigned`, this is a silent behavioral regression rather than an explicit breaking change. Either restore Node-side self-signed handling for the regtest path, or drop the parameter from the function signature, `requestInfo`, all callers (`JsonRpcTransport`, `DAPIAddress`, both address providers, and corresponding tests), and document the removed regtest support.
- [SUGGESTION] In `packages/js-dapi-client/test/unit/transport/JsonRpcTransport/requestJsonRpc.spec.js`:76-99: Self-signed HTTPS test no longer exercises any distinct behavior
This test sets `selfSigned = true`, but with the `https.Agent` branch removed, its call path is identical to the plain HTTPS test on lines 52-74. Since `globalThis.fetch` is fully stubbed, the assertion only confirms the stub resolves to `'passed'` — it can stay green while the production self-signed path is broken (which is exactly what is happening at this SHA). Either delete the test along with the `selfSigned` parameter, or rework it to assert on whatever mechanism actually controls TLS verification once that path is restored.
- [NITPICK] In `packages/js-dapi-client/package.json`:64-64: `setimmediate` devDependency contradicts the cleanup description
The PR description/commit message states `setimmediate` was removed, but it is still listed as a devDependency here and is still required by `lib/test/karma/bootstrap.js:1`. The code and metadata are internally consistent (the require resolves), but the dependency-removal claim in the PR description is incorrect. Either fully drop the polyfill (require + devDependency) or update the description to reflect that it was retained for the karma browser bootstrap.
Inline posting hit GitHub HTTP 422, so I posted the same verified findings as a top-level review body.
PastaPastaPasta
force-pushed
the
claude/esm-1-cleanup
branch
from
2d952ac to
f8432f9
Compare
PastaPastaPasta
force-pushed
the
claude/esm-1-cleanup
branch
from
f8432f9 to
d0185fb
Compare
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Cleanup PR drops unused deps and inlines small shims for @dashevo/dapi-client and the browser path of @dashevo/dapi-grpc. The three prior findings (self-signed HTTPS path, missing dispatcher test, stale setimmediate) are all resolved at f8432f9: an undici Agent dispatcher restores TLS-bypass behavior in Node, the spec asserts on it, and setimmediate is gone from karma bootstrap and package.json. No new correctness, security, or architectural issues verified.
PastaPastaPasta
force-pushed
the
claude/esm-1-cleanup
branch
from
d0185fb to
414a5c5
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/js-dapi-client/lib/transport/JsonRpcTransport/requestJsonRpc.js`:
- Around line 54-61: The code creates a new undici Agent per self-signed HTTPS
request (see the Agent allocation and requestOptions.dispatcher) but never
closes it, leaking sockets; fix by replacing per-request Agent creation with a
single shared module-level Agent (e.g., sharedUndiciAgent) that is instantiated
once when needed and assigned to requestOptions.dispatcher, or if you must
create per-request dispatchers ensure you call dispatcher.destroy() /
dispatcher.close() after the request completes; update the code paths that set
requestOptions.dispatcher and add cleanup (destroy/close) or the shared Agent
instantiation so sockets are not leaked.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 81b75deb-a89b-4428-be81-60dfa6c8d3a8
⛔ Files ignored due to path filters (2)
.yarn/cache/undici-npm-6.25.0-6002e70879-a475e45da3.zipis excluded by!**/.yarn/**,!**/*.zipyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (12)
.pnp.cjspackages/dapi-grpc/clients/core/v0/web/CorePromiseClient.jspackages/dapi-grpc/clients/platform/v0/web/PlatformPromiseClient.jspackages/js-dapi-client/lib/dapiAddressProvider/ListDAPIAddressProvider.jspackages/js-dapi-client/lib/index.jspackages/js-dapi-client/lib/logger/index.jspackages/js-dapi-client/lib/test/bootstrap.jspackages/js-dapi-client/lib/test/karma/bootstrap.jspackages/js-dapi-client/lib/transport/JsonRpcTransport/requestJsonRpc.jspackages/js-dapi-client/package.jsonpackages/js-dapi-client/polyfills/fetch-polyfill.jspackages/js-dapi-client/test/unit/transport/JsonRpcTransport/requestJsonRpc.spec.js
💤 Files with no reviewable changes (4)
- packages/js-dapi-client/lib/index.js
- packages/js-dapi-client/lib/test/karma/bootstrap.js
- packages/js-dapi-client/polyfills/fetch-polyfill.js
- packages/js-dapi-client/lib/test/bootstrap.js
✅ Files skipped from review due to trivial changes (2)
- packages/js-dapi-client/lib/dapiAddressProvider/ListDAPIAddressProvider.js
- .pnp.cjs
🚧 Files skipped from review as they are similar to previous changes (5)
- packages/dapi-grpc/clients/core/v0/web/CorePromiseClient.js
- packages/dapi-grpc/clients/platform/v0/web/PlatformPromiseClient.js
- packages/js-dapi-client/package.json
- packages/js-dapi-client/test/unit/transport/JsonRpcTransport/requestJsonRpc.spec.js
- packages/js-dapi-client/lib/logger/index.js
PastaPastaPasta
force-pushed
the
claude/esm-1-cleanup
branch
from
414a5c5 to
0cd7109
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').
PastaPastaPasta
force-pushed
the
claude/esm-1-cleanup
branch
from
0cd7109 to
c2b4925
Compare
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Verified at d0185fb: undici Agent dispatcher for self-signed HTTPS is in place at requestJsonRpc.js:54-62 with corresponding spec assertions (positive and negative cases), and setimmediate plus other dropped deps are absent from js-dapi-client / dapi-grpc. Both reviewers converged on zero findings and I confirmed nothing new in the cumulative diff.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Both Claude and Codex independently verified the cumulative changes at c2b4925 and found no outstanding issues. The two follow-up fixes since d0185fb (module-level cached undici Agent for self-signed TLS, and merging the logger prefix into the printf format string to preserve %s/%d/%o interpolation) are correct and address prior findings. No blocking, suggestion, or nitpick findings were verified in the current changeset.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
PR head d579034 adds a single test-only change: two undici dispatcher specs in requestJsonRpc.spec.js are wrapped in nodeOnlyIt so they skip under Karma/browsers, mirroring the Node-only guard around the dispatcher branch in requestJsonRpc.js:59-70. Node coverage of the self-signed HTTPS path is preserved, and the browser-side negative assertions remain unchanged. Prior carried-forward findings (dispatcher attachment, reuse assertion, removed setimmediate) remain resolved at this SHA, and no new issues were verified.
|
CI failure is unrelated to this PR |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
PR head b219e27 merges the updated v3.1-dev base and leaves this PR's effective diff as the DAPI JS/browser cleanup plus package-version bumps. Carried-forward prior findings from the immediately previous d579034 review: none; I reconciled the earlier self-signed HTTPS / dispatcher-test / setimmediate findings and they remain resolved at this head. The latest delta was inspected with git diff d579034b37db160a30d7424614809d352b0a596c..b219e2748ff75ee709d3f25ca4cd63719560447a, and no new actionable issues were verified.
3 participants
Summary
Non-breaking cleanup pass on
@dashevo/dapi-clientand the browser path of@dashevo/dapi-grpc. Package stays CJS, public API unchanged, no consumer changes required.This is PR 1 of 5 in a stacked series that incrementally makes
dapi-clientbrowser-native and ESM. Each PR is independently shippable and leaves the monorepo green.What changes
@dashevo/dapi-clientwinstonwith a ~30-line console-backed logger that preserves the same API (.error/.warn/.info/.verbose/.debug/.silly/.getForId).node-fetchand thefetch-polyfill(Node 18+ has globalfetch).setimmediate(require in karma bootstrap + devDep). Test helper inBlockHeadersReader.spec.jsalready has asetTimeoutfallback that handles browsers withoutsetImmediate.requestJsonRpcfromnode-fetch'shttps.Agentto undici'sAgentas thedispatcheroption on Node 18+ built-infetch. Behavior preserved for regtest/local; flag is still a no-op in browsers (TLS bypass is not programmatically possible there).undiciis added as a dependency (Node 18+ ships it internally; declaring it explicitly is required under Yarn PnP). The import is hidden from bundler static analysis viaeval('require')so undici is not pulled into browser bundles.lodash/sampleinListDAPIAddressProvider(one call site).engines.node >= 18.18.winston,node-fetch,lodash,bs58,node-inspect-extracted,setimmediate.@dashevo/dapi-grpcpromisifyshim inclients/core/v0/web/CorePromiseClient.jsandclients/platform/v0/web/PlatformPromiseClient.js, removing therequire('util')from the browser bundle path. Both files document the shim so a future codegen regen does not silently reintroducerequire('util').Test plan
yarn workspace @dashevo/dapi-client run test:unit— 316 passing (adds a test that asserts the undicidispatcheris set whenselfSigned=true, and another that asserts it is NOT set whenselfSigned=false)yarn workspace @dashevo/dapi-client run lint— 0 errors, 73 pre-existing warningsyarn workspace @dashevo/wallet-lib run test:unit— no regressionyarn workspace dash run test:unit— no regressionBreaking changes
engines.node >= 18.18(was unspecified; older Node lacks globalfetchand undici).Stack
Buffer→Uint8Arrayin dapi-client public surface (refactor(dapi)!: expose Uint8Array instead of Buffer in public API of dapi-client #3680)@dashevo/dapi-client→ ESM (refactor(dapi)!: convert dapi-client to ESM, drop webpack/karma #3681)@dashevo/wallet-lib→ ESM (refactor(wallet-lib)!: convert to ESM, consume ESM dapi-client #3682)@dashevo/js-dash-sdk+@dashevo/platform-test-suite→ ESM (refactor(sdk,test-suite)!: convert to ESM, consume ESM dapi-client and wallet-lib #3683)Summary by CodeRabbit
Breaking Changes
Bug Fixes
New Features
Chores
Tests