-
Notifications
You must be signed in to change notification settings - Fork 45
fix(dashmate): rs-dapi not stopped when dashmate reset --platform -f is called #2824
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughA new factory function Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20–25 minutes
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 |
|
@coderabbitai review all |
|
@lklimek: I'll review all the changes in this PR. ✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Nitpick comments (5)
packages/dapi-grpc/clients/platform/v0/objective-c/Platform.pbobjc.h (3)
3365-3366: Nit: align “proof” wording with other messages for claritySuggest clarifying that the proof is optional and of the response data, not “the epoch” generically.
Apply this comment-only diff:
-/** Cryptographic proof of the finalized epoch */ +/** Cryptographic proof of the finalized epoch info, if requested */
3387-3389: Nit: avoid mid-sentence line break; add periodKeep the short sentence on one line and terminate with a period for readability.
Apply this comment-only diff:
-/** - * FinalizedEpochInfos holds a collection of finalized epoch information - * entries - **/ +/** + * FinalizedEpochInfos holds a collection of finalized epoch information entries. + **/
6442-6446: Clarify what setting contractInfo doesMake it explicit that this targets a specific contract token type and what fields are expected.
Apply this comment-only diff:
-/** - * This should be set if you wish to get back the last claim info as a - * specific type - **/ +/** + * Set to request last-claim info for a specific contract token type. + * Provide contractId and tokenContractPosition in contractInfo. + **/packages/dashmate/src/listr/tasks/startNodeTaskFactory.js (1)
94-96: Confirm behavior with stale/inactive DAPI containersUsing active profiles here is fine since you forcibly remove the inactive DAPI stack later. Please verify that on nodes with leftover deprecated DAPI containers running, the “Remove inactive DAPI stack” step succeeds on all platforms (Docker Desktop/macOS/Windows) without needing an additional pre-check.
Also applies to: 149-152
packages/dashmate/src/listr/tasks/stopNodeTaskFactory.js (1)
32-35: Stopping with includeAll profiles fixes orphaned rs-dapiThe includeAll flag ensures both DAPI stacks are covered during platform-only stop. Consider extracting a tiny helper inside the factory to compute profiles from ctx to avoid duplication.
+ const selectProfiles = (ctx, config) => + ctx.platformOnly ? getPlatformProfiles(config, { includeAll: true }) : []; @@ - const profiles = ctx.platformOnly - ? getPlatformProfiles(config, { includeAll: true }) - : []; + const profiles = selectProfiles(ctx, config); @@ - const profiles = ctx.platformOnly - ? getPlatformProfiles(config, { includeAll: true }) - : []; + const profiles = selectProfiles(ctx, config);Also applies to: 74-77
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
packages/dapi-grpc/clients/platform/v0/objective-c/Platform.pbobjc.h(3 hunks)packages/dashmate/src/config/getPlatformProfilesFactory.js(1 hunks)packages/dashmate/src/createDIContainer.js(2 hunks)packages/dashmate/src/listr/tasks/resetNodeTaskFactory.js(5 hunks)packages/dashmate/src/listr/tasks/startNodeTaskFactory.js(2 hunks)packages/dashmate/src/listr/tasks/stopNodeTaskFactory.js(3 hunks)
🔇 Additional comments (6)
packages/dapi-grpc/clients/platform/v0/objective-c/Platform.pbobjc.h (1)
1-2: Confirm changes are generated from .proto, not hand-editedThis file is generated (“DO NOT EDIT!”). Please confirm these doc updates were made in platform.proto comments and the Objective‑C stubs were re‑generated, to avoid drift across languages. If not, move the edits to the .proto and re-generate.
packages/dashmate/src/createDIContainer.js (1)
120-120: DI wiring for getPlatformProfiles looks correctImport and singleton registration align with existing patterns and InjectionMode.CLASSIC param names. Please add/extend a lightweight container-resolution test to assert container.getPlatformProfiles is callable and returns expected profiles for a sample config.
Also applies to: 173-174
packages/dashmate/src/listr/tasks/startNodeTaskFactory.js (1)
22-22: Adopted DI for getPlatformProfiles — good moveSignature and JSDoc updated consistently. This aligns start with stop/reset for profile sourcing.
Also applies to: 34-35
packages/dashmate/src/listr/tasks/resetNodeTaskFactory.js (1)
75-82: Profiles handling for platform-only reset looks correctUsing getPlatformProfiles(config, { includeAll: true }) ensures both deprecated DAPI and rs-dapi are targeted. Good coverage.
Also applies to: 107-123
packages/dashmate/src/listr/tasks/stopNodeTaskFactory.js (1)
10-11: Good switch to injected getPlatformProfilesFactory signature and docs updated consistently.
Also applies to: 17-18
packages/dashmate/src/config/getPlatformProfilesFactory.js (1)
1-32: PLATFORM_PROFILES verified in sync with docker-compose.ymlConfirmed: All three profiles declared in docker-compose.yml (
platform,platform-dapi-deprecated,platform-dapi-rs) match the PLATFORM_PROFILES constant exactly. Code is clean and composable.
shumkov
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
Issue being fixed or feature implemented
Commands like
dashmate stop --platform,dashmate reset --platform -fdon't stop dapi and/or rs-dapi, leading to orphaned containers.What was done?
Fixed code with Codex:
In https://github.com/dashpay/platform/blob/v2.2-dev/packages/dashmate/docker-compose.yml , you have profiles assigned to various services. To support deprecated DAPI (in case someone has trouble with RS-DAPI), rs-dapi has:
profiles:
- platform-dapi-rs
and JS dapi has:
profiles:
- platform-dapi-deprecated
This PR adjusts Dashmate to use these profiles.
How Has This Been Tested?
On local network, changing deprecated flag and restarting / resetting
Breaking Changes
None
Checklist:
For repository code-owners and collaborators only
Summary by CodeRabbit
Refactor
Documentation