feat(dashmate): add protocol version to the status command #2255
Conversation
shumkov
changed the title
…into feat/dashmate/protocol-version
WalkthroughThe changes in this pull request enhance the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
packages/dashmate/src/commands/status/platform.js (1)
107-108: LGTM: Protocol version properties correctly assigned with a minor suggestionThe assignment of
protocolVersionanddesiredProtocolVersionto theplainobject is well-implemented and consistent with the existing code structure. The use of the logical OR operator for fallback to 'n/a' is a concise approach.For consistency with other assignments in this block, consider using the
||operator instead of the ternary operator used elsewhere. For example:plain['Protocol Version'] = protocolVersion || 'n/a'; plain['Desired Protocol Version'] = desiredProtocolVersion || 'n/a';This change would make the code more uniform with the surrounding assignments.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- packages/dashmate/src/commands/status/platform.js (3 hunks)
- packages/dashmate/src/status/scopes/platform.js (5 hunks)
- packages/dashmate/test/unit/status/scopes/platform.spec.js (16 hunks)
🧰 Additional context used
📓 Learnings (1)
packages/dashmate/test/unit/status/scopes/platform.spec.js (2)
Learnt from: shumkov PR: dashpay/platform#2249 File: packages/dashmate/test/unit/status/scopes/platform.spec.js:466-467 Timestamp: 2024-10-18T15:37:21.329Z Learning: In test files for the dashmate project, such as `packages/dashmate/test/unit/status/scopes/platform.spec.js`, prefer to keep mocks explicitly in tests rather than consolidating them, to increase readability.Learnt from: shumkov PR: dashpay/platform#2249 File: packages/dashmate/test/unit/status/scopes/platform.spec.js:77-78 Timestamp: 2024-10-18T15:37:36.387Z Learning: In `packages/dashmate/test/unit/status/scopes/platform.spec.js`, prefer to keep mocks explicitly in tests to increase readability, rather than refactoring them into shared `beforeEach` blocks or helper functions.
🔇 Additional comments (15)
packages/dashmate/src/commands/status/platform.js (3)
50-51: LGTM: New protocol version properties added correctlyThe addition of 'Protocol Version' and 'Desired Protocol Version' properties to the
plainobject is well-implemented. These new fields are consistent with the existing structure and are appropriately initialized with 'n/a'.
97-98: LGTM: Protocol version properties correctly extractedThe extraction of
protocolVersionanddesiredProtocolVersionfrom thetenderdashobject is well-implemented. It's correctly placed within the existing check fortenderdash.version, ensuring these properties are only accessed whentenderdashdata is available.
50-51: Summary: Protocol version information successfully added to status commandThe changes effectively implement the addition of protocol version information to the
PlatformStatusCommandoutput. The new properties are correctly initialized, extracted from thetenderdashobject, and assigned to theplainobject for display.These additions enhance the status command by providing users with visibility into both the current and desired protocol versions, as requested. The implementation is consistent with the existing code structure and follows good practices.
To ensure that these changes are properly integrated and don't introduce any regressions, please run the following verification script:
This script will help ensure that the changes are correctly implemented and that any necessary test updates have been made.
Also applies to: 97-98, 107-108
packages/dashmate/test/unit/status/scopes/platform.spec.js (8)
83-87: LGTM: Addition of protocol_version objectThe new
protocol_versionobject inmockStatuscorrectly reflects the structure of the API response, including p2p, block, and app versions. This addition aligns well with the PR objective of including protocol version information in the status command.
102-110: LGTM: Addition of mockAbciInfo objectThe new
mockAbciInfoobject provides appropriate mock data for testing the ABCI info retrieval, including version, app_version, last_block_height, and last_block_app_hash. This addition supports the testing of the new protocol version functionality.
126-127: LGTM: Addition of protocol version properties in expectedScopeThe new
protocolVersionanddesiredProtocolVersionproperties in thetenderdashobject ofexpectedScopecorrectly reflect the expected output of the status command. The values (3 and 4 respectively) match the mock data provided, ensuring proper testing of the new functionality.
150-152: LGTM: Addition of third mockFetch call for ABCI infoThe modification to
mockFetchto include a third call that resolves withmockAbciInfois appropriate. This change correctly mocks the new API call for retrieving ABCI info, following the established pattern for other API calls in the test.
176-180: LGTM: Consistent addition of protocol_version objectThe addition of the
protocol_versionobject in the "platform syncing" test case maintains consistency with the previous test case. This ensures that protocol version information is available and tested even when the platform is in a syncing state.
195-203: LGTM: Consistent addition of mockAbciInfo objectThe addition of the
mockAbciInfoobject in the "platform syncing" test case is consistent with the previous test case. This ensures that ABCI info is available and tested even when the platform is in a syncing state, maintaining thorough test coverage.
220-221: LGTM: Consistent addition of protocol version properties across test casesThe
protocolVersionanddesiredProtocolVersionproperties have been consistently added to theexpectedScopeobject across various test scenarios. The values are appropriately set (specific values for success cases, null for error cases), reflecting the expected behavior in different situations. This consistency ensures comprehensive test coverage for the new protocol version functionality.Also applies to: 277-278, 332-333, 391-392, 491-492, 549-550
Line range hint
1-571: Overall assessment: Well-implemented and thoroughly tested changesThe modifications to this test file comprehensively cover the addition of protocol version information to the platform status command. The changes are consistent across various test scenarios, including both success and error cases. The new mock objects and expected outputs are well-structured and align perfectly with the PR objective.
Key points:
- Consistent addition of
protocol_versiontomockStatus- Introduction of
mockAbciInfofor ABCI information- Consistent updates to
expectedScopewithprotocolVersionanddesiredProtocolVersion- Appropriate handling of error cases with null values
The changes follow existing patterns in the test file, maintaining code consistency and readability. Overall, these modifications provide robust test coverage for the new protocol version functionality.
packages/dashmate/src/status/scopes/platform.js (4)
55-56: Initialization ofprotocolVersionanddesiredProtocolVersionAdding
protocolVersionanddesiredProtocolVersionto theinfoobject with initialnullvalues maintains consistency with the existing properties.
258-259: Addition ofprotocolVersionanddesiredProtocolVersionto scopeIncluding
protocolVersionanddesiredProtocolVersionin thescope.tenderdashobject enhances the status output by providing protocol version details.
273-273: Includeversionproperty in Drive informationAdding the
versionproperty to thedriveobject enriches the output with Drive service version details, aiding in diagnostics and monitoring.
122-125: Ensure robust JSON parsing ofabci_inforesponseWhen parsing the
tenderdashAbciInfoResponseobject, handle potential JSON parsing errors that may occur if the response is not in the expected format.To verify, you can run the following script:
Replace
<tenderdashHost>and<port>with the appropriate variables from your configuration.
Issue being fixed or feature implemented
Users want to see the current and desired protocol versions
What was done?
dashmate status platformcommand.How Has This Been Tested?
Running the command locally
Breaking Changes
None
Checklist:
For repository code-owners and collaborators only
Summary by CodeRabbit
New Features
Protocol VersionandDesired Protocol Versionto platform status details.Bug Fixes
Tests
getPlatformScopefunction to include new properties and scenarios.