refactor(dashmate): remove deprecated javascript dapi #2827
Conversation
|
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (1)
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including You can disable this status message by setting the WalkthroughConsolidates the legacy DAPI JavaScript stack into a single Rust implementation (rs-dapi): removes dapi_api and dapi_core_streams services and build files, migrates dapi.api settings into platform.dapi.rsDapi, and updates compose, schema, docs, tests, profiles, CI/workflows, and migrations. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant User as User / Config
participant Migrator as Migration (2.2.0-dev.1)
participant Defaults as Base Config
participant Schema as JSON Schema
participant Compose as Env & Compose Generator
participant Runtime as rs-dapi Service
User->>Migrator: provide config with legacy platform.dapi.api + upstreams
Migrator->>Defaults: ensure platform.dapi.rsDapi exists (clone defaults if missing)
Migrator->>Migrator: if dapi.api exists -> copy waitForStResultTimeout -> delete dapi.api
Migrator->>Migrator: normalize upstreams -> derive rsDapi.maxRequests from legacy upstreams
Migrator-->>User: return migrated config (rsDapi present, legacy removed)
User->>Schema: validate config (requires rsDapi)
Schema-->>User: validation result
User->>Compose: generate envs/profiles (no legacy build files)
Compose->>Runtime: start rs_dapi (JSON-RPC 3009, gRPC 3010, Metrics/Health 9091)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
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 |
lklimek
changed the title
![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
🧹 Nitpick comments (1)
packages/dashmate/src/listr/tasks/startNodeTaskFactory.js (1)
125-138: Robust legacy DAPI cleanup before start — good approachPer‑service rm with tolerant error handling is solid. Consider renaming the list to reflect intent (e.g., cleanupServiceNames) since rs_dapi is included.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
packages/dashmate/templates/platform/gateway/envoy.yaml.dotis excluded by!**/*.dot
📒 Files selected for processing (19)
packages/dashmate/configs/defaults/getBaseConfigFactory.js(2 hunks)packages/dashmate/configs/getConfigFileMigrationsFactory.js(22 hunks)packages/dashmate/docker-compose.build.dapi_api.yml(0 hunks)packages/dashmate/docker-compose.build.dapi_core_streams.yml(0 hunks)packages/dashmate/docker-compose.yml(2 hunks)packages/dashmate/docs/config/dapi.md(2 hunks)packages/dashmate/docs/config/gateway.md(1 hunks)packages/dashmate/docs/services/gateway.md(4 hunks)packages/dashmate/docs/services/platform.md(1 hunks)packages/dashmate/docs/update.md(3 hunks)packages/dashmate/src/config/configJsonSchema.js(2 hunks)packages/dashmate/src/config/generateEnvsFactory.js(0 hunks)packages/dashmate/src/config/getConfigProfilesFactory.js(0 hunks)packages/dashmate/src/listr/tasks/startNodeTaskFactory.js(1 hunks)packages/dashmate/src/util/isServiceBuildRequired.js(1 hunks)packages/dashmate/test/e2e/localNetwork.spec.js(0 hunks)packages/dashmate/test/e2e/testnetEvonode.spec.js(0 hunks)packages/dashmate/test/e2e/testnetFullnode.spec.js(0 hunks)scripts/configure_dashmate.sh(0 hunks)
💤 Files with no reviewable changes (8)
- packages/dashmate/src/config/getConfigProfilesFactory.js
- packages/dashmate/test/e2e/testnetFullnode.spec.js
- packages/dashmate/test/e2e/testnetEvonode.spec.js
- packages/dashmate/src/config/generateEnvsFactory.js
- packages/dashmate/docker-compose.build.dapi_core_streams.yml
- scripts/configure_dashmate.sh
- packages/dashmate/docker-compose.build.dapi_api.yml
- packages/dashmate/test/e2e/localNetwork.spec.js
🧰 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/dashmate/src/util/isServiceBuildRequired.jspackages/dashmate/src/listr/tasks/startNodeTaskFactory.jspackages/dashmate/configs/defaults/getBaseConfigFactory.jspackages/dashmate/src/config/configJsonSchema.jspackages/dashmate/configs/getConfigFileMigrationsFactory.js
🧠 Learnings (2)
📓 Common learnings
Learnt from: lklimek
PR: dashpay/platform#2716
File: packages/dashmate/src/test/constants/services.js:4-4
Timestamp: 2025-10-15T14:45:30.856Z
Learning: In the dashmate codebase (packages/dashmate), during the DAPI Rust migration (rs-dapi), the old service keys `dapi_api` and `dapi_core_streams` are intentionally kept in `generateEnvsFactory.js` for backward compatibility even though the test constants in `src/test/constants/services.js` have been updated to use `rs_dapi`. These deprecated keys will be removed in a future PR after the transition is complete.
Learnt from: shumkov
PR: dashpay/platform#2270
File: packages/dapi/lib/grpcServer/handlers/platform/broadcastStateTransitionHandlerFactory.js:69-125
Timestamp: 2024-10-24T05:07:35.892Z
Learning: Future development plans involve rewriting DAPI into Rust, as the rest of the project has already migrated.
📚 Learning: 2025-10-15T14:45:30.856Z
Learnt from: lklimek
PR: dashpay/platform#2716
File: packages/dashmate/src/test/constants/services.js:4-4
Timestamp: 2025-10-15T14:45:30.856Z
Learning: In the dashmate codebase (packages/dashmate), during the DAPI Rust migration (rs-dapi), the old service keys `dapi_api` and `dapi_core_streams` are intentionally kept in `generateEnvsFactory.js` for backward compatibility even though the test constants in `src/test/constants/services.js` have been updated to use `rs_dapi`. These deprecated keys will be removed in a future PR after the transition is complete.
Applied to files:
packages/dashmate/src/util/isServiceBuildRequired.jspackages/dashmate/src/listr/tasks/startNodeTaskFactory.jspackages/dashmate/docs/services/gateway.mdpackages/dashmate/docs/services/platform.mdpackages/dashmate/configs/defaults/getBaseConfigFactory.jspackages/dashmate/src/config/configJsonSchema.jspackages/dashmate/docker-compose.yml
⏰ 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). (6)
- GitHub Check: Build Docker images (RS-DAPI, rs-dapi, rs-dapi) / Build RS-DAPI image
- GitHub Check: Build Docker images (Dashmate helper, dashmate-helper, dashmate-helper) / Build Dashmate helper image
- GitHub Check: Build Docker images (DAPI, dapi, dapi) / Build DAPI image
- 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 (16)
packages/dashmate/configs/defaults/getBaseConfigFactory.js (1)
163-165: Gateway upstreams: rsDapi switch LGTMReplacing dapiApi/dapiCoreStreams with rsDapi is correct and consistent with the schema.
packages/dashmate/src/config/configJsonSchema.js (3)
530-533: Upstreams schema updates for rsDapi look correctrsDapi added and required list updated appropriately.
Also applies to: 538-539
874-881: rsDapi waitForStResultTimeout: clarify and keep consistent with runtime configMillis integer definition is fine. Ensure the configured value matches gateway.listener waitForStResultTimeout to avoid racey timeouts.
884-885: Top-level dapi required set to ['rsDapi'] — LGTMThis reflects the removal of JS DAPI paths.
packages/dashmate/docs/update.md (2)
32-33: Docs: rs‑dapi rename LGTMTable correctly reflects rs‑dapi.
43-44: Docs: JSON sample updated to rs_dapi — LGTMSample aligns with service naming.
packages/dashmate/src/util/isServiceBuildRequired.js (1)
11-15: rsDapi build flag correctly implemented — verification completeThe rsDapi build check at
platform.dapi.rsDapi.docker.build.enabledis properly integrated. Verification confirms oldplatform.dapi.apireferences exist only in migration logic (for backward compatibility) and legacy service name tracking, not in active build checks. The optional refactor to cacheplatform.enableis a minor stylistic improvement; the code is correct as-is.packages/dashmate/docs/config/dapi.md (1)
1-42: LGTM! Documentation clearly reflects the rs-dapi migration.The documentation has been successfully updated to focus on the Rust implementation of DAPI, with comprehensive coverage of Docker, health monitoring, metrics, logging, and timeout configuration.
packages/dashmate/docs/config/gateway.md (1)
42-42: LGTM! Gateway upstream configuration updated correctly.The consolidation of
dapiApianddapiCoreStreamsupstreams into a singlersDapiupstream aligns with the architectural change to use rs-dapi as the sole DAPI implementation.packages/dashmate/docs/services/gateway.md (2)
27-76: LGTM! Gateway service documentation updated consistently.All references to the deprecated DAPI services have been properly updated to reflect the rs-dapi architecture. The architecture diagram, cluster definitions, and route mappings are all consistent with the migration.
128-129: LGTM! Configuration options updated correctly.The circuit breaker configuration options now reference the consolidated rs-dapi services instead of the separate DAPI API and Core Streams services.
packages/dashmate/docs/services/platform.md (1)
116-142: LGTM! Platform services documentation accurately reflects rs-dapi consolidation.The documentation has been properly updated to show rs-dapi as the unified DAPI implementation, consolidating the responsibilities and ports from the previously separate DAPI API and Core Streams services. The port mappings (3009 for JSON-RPC, 3010 for gRPC/gRPC-Web, 9091 for Metrics & Health) are consistent with the docker-compose configuration.
packages/dashmate/configs/getConfigFileMigrationsFactory.js (3)
73-75: LGTM! Guarded migrations handle backward compatibility correctly.The conditional checks throughout the migration history appropriately guard against cases where
platform.dapi.apimay not exist in the configuration. This defensive pattern ensures smooth migrations for configurations at various stages of the DAPI transition.Based on learnings
Also applies to: 128-130, 189-191, 276-278, 453-455, 518-520, 835-837, 885-888, 908-910, 965-967, 1008-1010, 1019-1021, 1042-1044, 1065-1067, 1075-1077, 1095-1097, 1107-1109, 1119-1121, 1131-1133, 1155-1157, 1285-1288, 1312-1314, 1325-1327
1216-1279: LGTM! Migration logic correctly handles the DAPI removal.The
2.2.0-dev.1migration properly handles the removal of deprecated DAPI configuration:
- Removes the
deprecatedflag- Ensures
rsDapiconfiguration exists- Migrates
waitForStResultTimeoutfrom the oldapiconfig torsDapi- Migrates gateway upstreams, intelligently using
Math.max()to preserve the higher circuit breaker limit from eitherdapiApiordapiCoreStreams- Cleans up by deleting the old configuration objects
The logic handles edge cases appropriately with existence checks and fallbacks to defaults.
1216-1216: Verify migration ordering between versions.The
2.2.0-dev.1migration appears before2.1.0-pr.2716.1in the code. While this may be intentional (if they're on different branches), please verify that:
- The migration system correctly determines which migrations to run based on the current version
- Users upgrading from
2.1.0-pr.2716.1to2.2.0-dev.1won't skip the2.2.0-dev.1migration- The ordering doesn't cause issues for configurations that have already been migrated by
2.1.0-pr.2716.1If the ordering is correct and the migration system handles this properly, this can be safely ignored.
Also applies to: 1280-1280
packages/dashmate/docker-compose.yml (1)
130-169: LGTM! rs_dapi service correctly replaces deprecated DAPI services.The rs_dapi service definition properly consolidates the functionality of the removed dapi_api and dapi_core_streams services. Key observations:
- Deploy replicas and environment variables follow the correct naming conventions
- Timeout configuration uses
PLATFORM_DAPI_RS_DAPI_WAIT_FOR_ST_RESULT_TIMEOUT, matching the migration logic- Port exposure (3009 for JSON-RPC, 3010 for gRPC) aligns with documentation
- Profile changed from
platform-dapi-rstoplatform, consolidating service startupBased on learnings
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
Dockerfile(1 hunks)packages/dashmate/configs/getConfigFileMigrationsFactory.js(22 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
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/dashmate/configs/getConfigFileMigrationsFactory.js
{Dockerfile,docker-compose*.yml}
📄 CodeRabbit inference engine (AGENTS.md)
Keep Docker configuration files at the repository root (e.g., Dockerfile, docker-compose*.yml)
Files:
Dockerfile
🧠 Learnings (1)
📓 Common learnings
Learnt from: lklimek
PR: dashpay/platform#2716
File: packages/dashmate/src/test/constants/services.js:4-4
Timestamp: 2025-10-15T14:45:30.856Z
Learning: In the dashmate codebase (packages/dashmate), during the DAPI Rust migration (rs-dapi), the old service keys `dapi_api` and `dapi_core_streams` are intentionally kept in `generateEnvsFactory.js` for backward compatibility even though the test constants in `src/test/constants/services.js` have been updated to use `rs_dapi`. These deprecated keys will be removed in a future PR after the transition is complete.
Learnt from: shumkov
PR: dashpay/platform#2270
File: packages/dapi/lib/grpcServer/handlers/platform/broadcastStateTransitionHandlerFactory.js:69-125
Timestamp: 2024-10-24T05:07:35.892Z
Learning: Future development plans involve rewriting DAPI into Rust, as the rest of the project has already migrated.
⏰ 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). (5)
- GitHub Check: Build Docker images (Dashmate helper, dashmate-helper, dashmate-helper) / Build Dashmate helper image
- GitHub Check: Build Docker images (RS-DAPI, rs-dapi, rs-dapi) / Build RS-DAPI image
- 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 (2)
packages/dashmate/configs/getConfigFileMigrationsFactory.js (1)
73-75: Good defensive gating of legacy dapi.api edits.The added optional-chaining guards avoid touching non-existent configs and reduce migration brittleness. LGTM.
Also applies to: 128-130, 189-191, 276-278, 453-455, 835-837, 885-888, 909-910, 965-967, 1009-1010, 1020-1021, 1043-1044, 1065-1067, 1076-1077, 1096-1097, 1108-1109, 1120-1121, 1132-1133, 1156-1157, 1221-1224, 1249-1250, 1262-1263
Dockerfile (1)
13-13: No changes required to Dockerfile line 13 comment—it accurately reflects the final images.Verified all build stages in Dockerfile: the comment correctly lists drive-abci, dashmate-helper, test-suite, and rs-dapi as the final images. The Dockerfile itself contains no stale JS DAPI image references (no
dashpay/dapi:*builds).The legacy DAPI references found during scanning (dapi_api, dapi_core_streams, platform.dapi.api.*) exist in configuration handling within
packages/dashmate/, not in build stages. These appear to be intentional backward-compatibility and migration code (the configuration layer deletes these legacy paths during transitions to rs-dapi), not stale artifacts requiring cleanup in this Dockerfile PR.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
.github/actions/docker/action.yaml(1 hunks).github/actions/local-network/action.yaml(1 hunks).github/workflows/release-docker-image.yml(1 hunks).github/workflows/release.yml(0 hunks).github/workflows/tests-dashmate.yml(0 hunks).github/workflows/tests.yml(0 hunks)packages/dashmate/src/config/getConfigProfilesFactory.js(0 hunks)packages/dashmate/src/listr/tasks/startNodeTaskFactory.js(1 hunks)
💤 Files with no reviewable changes (4)
- packages/dashmate/src/config/getConfigProfilesFactory.js
- .github/workflows/tests.yml
- .github/workflows/tests-dashmate.yml
- .github/workflows/release.yml
🧰 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/dashmate/src/listr/tasks/startNodeTaskFactory.js
🧠 Learnings (2)
📓 Common learnings
Learnt from: lklimek
PR: dashpay/platform#2716
File: packages/dashmate/src/test/constants/services.js:4-4
Timestamp: 2025-10-15T14:45:30.856Z
Learning: In the dashmate codebase (packages/dashmate), during the DAPI Rust migration (rs-dapi), the old service keys `dapi_api` and `dapi_core_streams` are intentionally kept in `generateEnvsFactory.js` for backward compatibility even though the test constants in `src/test/constants/services.js` have been updated to use `rs_dapi`. These deprecated keys will be removed in a future PR after the transition is complete.
📚 Learning: 2025-10-15T14:45:30.856Z
Learnt from: lklimek
PR: dashpay/platform#2716
File: packages/dashmate/src/test/constants/services.js:4-4
Timestamp: 2025-10-15T14:45:30.856Z
Learning: In the dashmate codebase (packages/dashmate), during the DAPI Rust migration (rs-dapi), the old service keys `dapi_api` and `dapi_core_streams` are intentionally kept in `generateEnvsFactory.js` for backward compatibility even though the test constants in `src/test/constants/services.js` have been updated to use `rs_dapi`. These deprecated keys will be removed in a future PR after the transition is complete.
Applied to files:
packages/dashmate/src/listr/tasks/startNodeTaskFactory.js
🔇 Additional comments (3)
.github/actions/docker/action.yaml (1)
24-24: Documentation update is accurate and consistent.The target description now correctly reflects that
rs-dapiis the DAPI implementation, replacing the deprecateddapioption..github/workflows/release-docker-image.yml (1)
10-10: Documentation examples correctly reflect rs-dapi naming.Both input descriptions now use
rs-dapiin their examples, which is accurate and consistent with the action.yaml updates and broader DAPI removal.Also applies to: 18-18
.github/actions/local-network/action.yaml (1)
85-86: Sed pattern correctly targets rs-dapi image replacement.The regex has been updated from matching
(drive|dapi)to(drive|rs-dapi), which is correct. The pattern accurately matches and replaces docker image references in the dashmate config.Please verify that the Dockerfile contains the
rs-dapibuild target referenced in this sed command and other workflows.
lklimek
force-pushed
the
refactor/remove-js-dapi
branch
from
312ff57 to
6c43fb6
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (26)
Cargo.toml(1 hunks)package.json(1 hunks)packages/bench-suite/package.json(1 hunks)packages/dapi-grpc/package.json(1 hunks)packages/dapi/package.json(1 hunks)packages/dash-spv/package.json(1 hunks)packages/dashmate/configs/getConfigFileMigrationsFactory.js(23 hunks)packages/dashmate/package.json(1 hunks)packages/dashmate/src/config/getConfigProfilesFactory.js(1 hunks)packages/dashpay-contract/package.json(1 hunks)packages/dpns-contract/package.json(1 hunks)packages/feature-flags-contract/package.json(1 hunks)packages/js-dapi-client/package.json(1 hunks)packages/js-dash-sdk/package.json(1 hunks)packages/js-evo-sdk/package.json(1 hunks)packages/js-grpc-common/package.json(1 hunks)packages/keyword-search-contract/package.json(1 hunks)packages/masternode-reward-shares-contract/package.json(1 hunks)packages/platform-test-suite/package.json(1 hunks)packages/token-history-contract/package.json(1 hunks)packages/wallet-lib/package.json(1 hunks)packages/wallet-utils-contract/package.json(1 hunks)packages/wasm-dpp/package.json(1 hunks)packages/wasm-drive-verify/package.json(1 hunks)packages/wasm-sdk/package.json(1 hunks)packages/withdrawals-contract/package.json(1 hunks)
✅ Files skipped from review due to trivial changes (9)
- packages/js-dash-sdk/package.json
- packages/dash-spv/package.json
- packages/wallet-lib/package.json
- packages/masternode-reward-shares-contract/package.json
- packages/token-history-contract/package.json
- packages/dashmate/package.json
- packages/bench-suite/package.json
- packages/keyword-search-contract/package.json
- packages/withdrawals-contract/package.json
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/dashmate/src/config/getConfigProfilesFactory.js
🧰 Additional context used
📓 Path-based instructions (3)
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/dashmate/configs/getConfigFileMigrationsFactory.js
packages/platform-test-suite/**
📄 CodeRabbit inference engine (AGENTS.md)
packages/platform-test-suite/**: Keep end-to-end tests and helpers in packages/platform-test-suite
Keep all E2E tests exclusively in packages/platform-test-suite
Files:
packages/platform-test-suite/package.json
packages/wasm-sdk/**
📄 CodeRabbit inference engine (CLAUDE.md)
Keep WASM SDK docs in sync (run generate_docs.py) when updating the WASM SDK
Files:
packages/wasm-sdk/package.json
🧠 Learnings (2)
📓 Common learnings
Learnt from: lklimek
PR: dashpay/platform#2716
File: packages/dashmate/src/test/constants/services.js:4-4
Timestamp: 2025-10-15T14:45:30.856Z
Learning: In the dashmate codebase (packages/dashmate), during the DAPI Rust migration (rs-dapi), the old service keys `dapi_api` and `dapi_core_streams` are intentionally kept in `generateEnvsFactory.js` for backward compatibility even though the test constants in `src/test/constants/services.js` have been updated to use `rs_dapi`. These deprecated keys will be removed in a future PR after the transition is complete.
Learnt from: shumkov
PR: dashpay/platform#2270
File: packages/dapi/lib/grpcServer/handlers/platform/broadcastStateTransitionHandlerFactory.js:69-125
Timestamp: 2024-10-24T05:07:35.892Z
Learning: Future development plans involve rewriting DAPI into Rust, as the rest of the project has already migrated.
📚 Learning: 2025-10-15T14:45:30.856Z
Learnt from: lklimek
PR: dashpay/platform#2716
File: packages/dashmate/src/test/constants/services.js:4-4
Timestamp: 2025-10-15T14:45:30.856Z
Learning: In the dashmate codebase (packages/dashmate), during the DAPI Rust migration (rs-dapi), the old service keys `dapi_api` and `dapi_core_streams` are intentionally kept in `generateEnvsFactory.js` for backward compatibility even though the test constants in `src/test/constants/services.js` have been updated to use `rs_dapi`. These deprecated keys will be removed in a future PR after the transition is complete.
Applied to files:
packages/dashmate/configs/getConfigFileMigrationsFactory.js
⏰ 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). (14)
- GitHub Check: Rust packages (masternode-reward-shares-contract) / Linting
- GitHub Check: Rust packages (drive) / Tests
- GitHub Check: Rust packages (token-history-contract) / Formatting
- GitHub Check: Rust packages (token-history-contract) / Unused dependencies
- GitHub Check: Rust packages (token-history-contract) / Tests
- GitHub Check: Rust packages (wasm-dpp) / Unused dependencies
- GitHub Check: Rust packages (wasm-dpp) / Linting
- GitHub Check: Rust packages (wasm-dpp) / Tests
- GitHub Check: Rust packages (dpp) / Tests
- GitHub Check: Build Docker images (Dashmate helper, dashmate-helper, dashmate-helper) / Build Dashmate helper image
- GitHub Check: Build Docker images (RS-DAPI, rs-dapi, rs-dapi) / Build RS-DAPI image
- 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 (19)
packages/js-evo-sdk/package.json (1)
3-3: Version bump is consistent with repository-wide 2.2.0-dev.1 release cycle.The version update aligns with the broader package versioning scheme for this development release.
package.json (1)
3-3: Root package version bump aligns with 2.2.0-dev.1 release cycle.The update is consistent across the monorepo. All workspace references remain intact as expected for this refactoring phase.
packages/dapi/package.json (1)
4-4: Version bump is consistent with development release cycle.The package version is updated to 2.2.0-dev.1 alongside the broader refactoring toward rs-dapi.
packages/wallet-utils-contract/package.json (1)
3-3: Version bump aligns with 2.2.0-dev.1 release scheme.No functional changes; version update is consistent across the repository.
packages/js-grpc-common/package.json (1)
3-3: Version bump is consistent with the 2.2.0-dev.1 release cycle.Dependencies and configuration remain unchanged; this is a straightforward version update.
packages/dpns-contract/package.json (1)
3-3: Version bump aligns with 2.2.0-dev.1 development release.Straightforward version update; no functional changes to this package.
packages/wasm-sdk/package.json (1)
3-3: Version bump is consistent with 2.2.0-dev.1 release cycle; verify documentation was regenerated.Per the coding guidelines, WASM SDK docs should be kept in sync by running
generate_docs.pywhen updating the WASM SDK. Please confirm that this step was completed.packages/feature-flags-contract/package.json (1)
3-3: Version bump is consistent with 2.2.0-dev.1 development release.Straightforward version update with no functional changes to the package.
packages/js-dapi-client/package.json (1)
3-3: Version bump is consistent with release cycle.The version is correctly updated to 2.2.0-dev.1, aligning with the coordinated release across the workspace and the transition to the rs-dapi implementation.
packages/dashpay-contract/package.json (1)
3-3: Version bump is consistent with coordinated release.Updated to 2.2.0-dev.1 as part of the workspace release cycle.
packages/wasm-drive-verify/package.json (1)
6-6: Version bump is consistent and appropriate.Updated to 2.2.0-dev.1 in line with workspace release cycle.
packages/wasm-dpp/package.json (1)
3-3: Version bump is consistent with workspace release.Updated to 2.2.0-dev.1; all dependencies remain at stable versions.
Cargo.toml (1)
47-47: Workspace version bump is consistent with coordinated release.Updated to 2.2.0-dev.1; Rust version requirement and resolver remain unchanged.
packages/dapi-grpc/package.json (1)
3-3: Version bump is consistent with coordinated release.Updated to 2.2.0-dev.1; protocol dependencies remain stable.
packages/platform-test-suite/package.json (1)
4-4: Version bump is consistent with coordinated release.Updated to 2.2.0-dev.1; E2E test suite dependencies remain stable and appropriately scoped per coding guidelines.
packages/dashmate/configs/getConfigFileMigrationsFactory.js (4)
73-75: LGTM: Guards properly protect against missing deprecated DAPI configuration.The added guards correctly check both the existence of
platform.dapi.apiand the presence of the corresponding key in the base config before attempting updates. This defensive pattern prevents migration errors when processing configurations that have already had the JavaScript DAPI removed.Also applies to: 128-130, 189-191, 276-278, 453-455, 518-520, 835-837, 885-888, 908-910, 965-967, 1008-1010, 1019-1021, 1042-1044, 1065-1067, 1075-1077, 1095-1097, 1107-1109, 1119-1121, 1131-1133, 1155-1157, 1221-1224, 1248-1250, 1261-1263
1340-1353: LGTM: Timeout migration properly preserves user settings.The migration correctly propagates
waitForStResultTimeoutfrom the deprecateddapi.apitorsDapi, checking that it's a number and only migrating if the target is undefined. The fallback to defaults at lines 1351-1353 ensures all configs have the required setting.
1359-1376: LGTM: Upstream migration correctly derives maxRequests from deprecated services.The logic properly derives
rsDapi.maxRequestsby taking the maximum of the deprecateddapiApianddapiCoreStreamsvalues, filtering nulls and falling back to defaults when no valid candidates exist. This preserves user-configured rate limits during the migration.
1303-1316: Port migration values are correct.The migration logic checks for base defaults (9091 for metrics, 29998 for ZMQ) and adjusts them to network-specific values that match the current config factories:
- Local: metrics 9091→29091, ZMQ 29998→49998
- Testnet: metrics 9091→19091, ZMQ 29998→39998
These align with the actual defaults in
getLocalConfigFactory.js,getTestnetConfigFactory.js, andgetBaseConfigFactory.js. The migration correctly transitions from uniform base defaults to network-differentiated configuration.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
packages/dashmate/configs/getConfigFileMigrationsFactory.js (1)
1321-1394: 2.2.0-dev.1 migration: upstreams cleanup moved before early return — resolved.Upstreams normalization/removal now runs regardless of
platform.dapipresence, addressing the earlier concern about staledapiApi/dapiCoreStreams. Nice.Also, make
rsDapi.maxRequestsderivation tolerant of numeric strings:- if (!upstreams.rsDapi) { - const { dapiApi, dapiCoreStreams } = upstreams; - const dapiApiMax = dapiApi?.maxRequests; - const dapiCoreStreamsMax = dapiCoreStreams?.maxRequests; - - const candidates = [ - typeof dapiApiMax === 'number' ? dapiApiMax : null, - typeof dapiCoreStreamsMax === 'number' ? dapiCoreStreamsMax : null, - ].filter((value) => value !== null); + if (!upstreams.rsDapi) { + const { dapiApi, dapiCoreStreams } = upstreams; + const toNum = (v) => { + const n = Number(v); + return Number.isFinite(n) ? n : null; + }; + const candidates = [toNum(dapiApi?.maxRequests), toNum(dapiCoreStreams?.maxRequests)] + .filter((v) => v !== null);This preserves existing behavior while accepting
"1000"as well as1000.
🧹 Nitpick comments (4)
packages/dashmate/docs/update.md (3)
32-35: Rename clarity: call out JS DAPI removal in this page.The table rename to “rs-dapi” is good. Add a short note in “Notes on Compatibility” stating that the legacy JavaScript DAPI was removed as of 2.2.x so users don’t look for “DAPI API” anymore. Example: “Starting with 2.2.x, the legacy JavaScript DAPI was removed; rs-dapi replaces it.”
Also applies to: 111-111
42-43: Fix markdownlint MD014: remove shell prompt from command-only block.Keep the JSON output in a separate block; drop the leading “$” to satisfy MD014.
Apply this diff:
-```bash -$ dashmate update --format=json --config local_1 | jq -``` +```bash +dashmate update --format=json --config local_1 | jq +```(From static analysis: MD014 at line 42.)
45-83: Optional: align image tags across examples to reduce confusion.The table shows 0.24 tags while the JSON sample mixes “:local”, “:22”, and “1.30.2-impr.1”. Consider using either consistent “:local” tags or a cohesive version set in both examples. This avoids implying cross-version combos.
packages/dashmate/configs/getConfigFileMigrationsFactory.js (1)
1269-1320: 2.2.0-dev.0 rsDapi metrics/ZMQ normalization: looks good; minor nit on clone.Logic for port re-mapping by network is clear and idempotent. The
else { options.core.zmq = lodash.cloneDeep(options.core.zmq); }deep clone seems redundant; unless you’re breaking shared refs, you can drop it to avoid work.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/dashmate/configs/getConfigFileMigrationsFactory.js(23 hunks)packages/dashmate/docs/update.md(3 hunks)packages/dashmate/src/listr/tasks/startNodeTaskFactory.js(1 hunks)
🧰 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/dashmate/configs/getConfigFileMigrationsFactory.jspackages/dashmate/src/listr/tasks/startNodeTaskFactory.js
🧠 Learnings (2)
📓 Common learnings
Learnt from: lklimek
PR: dashpay/platform#2716
File: packages/dashmate/src/test/constants/services.js:4-4
Timestamp: 2025-10-15T14:45:30.856Z
Learning: In the dashmate codebase (packages/dashmate), during the DAPI Rust migration (rs-dapi), the old service keys `dapi_api` and `dapi_core_streams` are intentionally kept in `generateEnvsFactory.js` for backward compatibility even though the test constants in `src/test/constants/services.js` have been updated to use `rs_dapi`. These deprecated keys will be removed in a future PR after the transition is complete.
Learnt from: shumkov
PR: dashpay/platform#2270
File: packages/dapi/lib/grpcServer/handlers/platform/broadcastStateTransitionHandlerFactory.js:69-125
Timestamp: 2024-10-24T05:07:35.892Z
Learning: Future development plans involve rewriting DAPI into Rust, as the rest of the project has already migrated.
📚 Learning: 2025-10-15T14:45:30.856Z
Learnt from: lklimek
PR: dashpay/platform#2716
File: packages/dashmate/src/test/constants/services.js:4-4
Timestamp: 2025-10-15T14:45:30.856Z
Learning: In the dashmate codebase (packages/dashmate), during the DAPI Rust migration (rs-dapi), the old service keys `dapi_api` and `dapi_core_streams` are intentionally kept in `generateEnvsFactory.js` for backward compatibility even though the test constants in `src/test/constants/services.js` have been updated to use `rs_dapi`. These deprecated keys will be removed in a future PR after the transition is complete.
Applied to files:
packages/dashmate/configs/getConfigFileMigrationsFactory.jspackages/dashmate/src/listr/tasks/startNodeTaskFactory.js
🪛 markdownlint-cli2 (0.18.1)
packages/dashmate/docs/update.md
42-42: Dollar signs used before commands without showing output
(MD014, commands-show-output)
⏰ 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). (20)
- GitHub Check: Rust packages (masternode-reward-shares-contract) / Unused dependencies
- GitHub Check: Rust packages (masternode-reward-shares-contract) / Tests
- GitHub Check: Rust packages (dashpay-contract) / Linting
- GitHub Check: Rust packages (rs-dapi-client) / Tests
- GitHub Check: Rust packages (drive-abci) / Tests
- GitHub Check: Rust packages (wasm-dpp) / Formatting
- GitHub Check: Rust packages (wasm-dpp) / Linting
- GitHub Check: Rust packages (wasm-dpp) / Unused dependencies
- GitHub Check: Rust packages (wasm-dpp) / Tests
- GitHub Check: Rust packages (wallet-utils-contract) / Unused dependencies
- GitHub Check: Rust packages (wallet-utils-contract) / Formatting
- GitHub Check: Rust packages (wallet-utils-contract) / Linting
- GitHub Check: Rust packages (rs-sdk-ffi) / Tests
- GitHub Check: Rust packages (token-history-contract) / Unused dependencies
- GitHub Check: Rust packages (token-history-contract) / Tests
- GitHub Check: Rust packages (token-history-contract) / Formatting
- GitHub Check: Build Docker images (Dashmate helper, dashmate-helper, dashmate-helper) / Build Dashmate helper image
- 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 (4)
packages/dashmate/src/listr/tasks/startNodeTaskFactory.js (2)
116-116: Improved task title clarity.The title change from "Remove inactive DAPI stack" to "Remove legacy DAPI stack" more accurately describes the operation and aligns with the PR objective.
119-132: Previous critical issue resolved - implementation looks good.The
legacyServiceNamesarray now correctly contains only the deprecated JavaScript DAPI services (dapi_apianddapi_core_streams) and excludesrs_dapi, which is the new Rust implementation. The per-service removal loop with error handling appropriately handles cases where legacy services may not exist.Based on learnings.
packages/dashmate/configs/getConfigFileMigrationsFactory.js (2)
73-75: Guarded dapi.api updates verified—no issues found.The script confirms all 23 writes to
platform.dapi.api.*are properly guarded. Read-from-config patterns (lines 73–75, 128–130, 189–191, 276–278, 453–455, 518–520, 1221–1224) usebase.has()ordefaultConfig.has()checks; direct-assignment patterns (lines 835–837, 885–887, 909–910, 965–967, 1009–1010, 1020–1021, 1043–1044, 1066–1067, 1076–1077, 1096–1097, 1108–1109, 1120–1121, 1132–1133, 1156–1157, 1249–1250, 1262–1263) use optional chaining. All writes are protected from runtime errors.
1330-1355: Config migration correctly handles old service keys; cleanup is intentional and verified.The deprecated keys
dapi_api/dapi_core_streamsare intentionally retained ingenerateEnvsFactory.jsduring the transition for backward compatibility (as noted in learnings). The old keys are explicitly removed via the cleanup task instartNodeTaskFactory.jswhen the platform starts, and user configs are migrated torsDapiby the migration code you reviewed. Compose files and env generation confirmed clean of old references. The transition strategy is complete and correct.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Issue being fixed or feature implemented
In Platform v2.1.0, new version of DAPI written in Rust was introduced. Previous version of DAPI in Javascript was deprecated.
What was done?
Removed deprecated JS Dapi.
How Has This Been Tested?
Breaking Changes
Checklist:
For repository code-owners and collaborators only
Summary by CodeRabbit
New Features
Refactor
Documentation
Tests / Chores
Chores