revert: "fix(rpc-json): make DMNState.service and MasternodeStatus.service optional for Core v24"#586
Conversation
…service`…" This reverts commit b05bf70.
📝 WalkthroughWalkthroughThe pull request modifies deserialization behavior for three public structs in the RPC JSON library. The Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
QuantumExplorer
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: 2
🧹 Nitpick comments (1)
rpc-json/src/lib.rs (1)
3260-3262: Note: Regression tests for optionalservicehandling have been removed.The tests that validated deserialization when
serviceis missing are removed as part of this revert. If Core v24 compatibility is needed in the future, these tests would need to be re-added.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rpc-json/src/lib.rs` around lines 3260 - 3262, Regression tests that validated deserialization when the optional `service` field is missing were removed; re-add tests that exercise deserializing payloads lacking `service` for the affected types (e.g. MasternodeListDiff and any structures handled by deserialize_u32_opt / ExtendedQuorumListResult) so Core v24 compatibility is preserved. Add unit tests that construct JSON blobs without `service`, call the existing deserialization codepaths (the functions/impls that produce MasternodeListDiff, ExtendedQuorumListResult, MnSyncStatus or QuorumType instances) and assert that parsing succeeds and the resulting structs have the expected defaults/None for the missing service value. Ensure tests reference the same deserialization helpers (deserialize_u32_opt) used in production so they fail if optional-service handling regresses.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@rpc-json/src/lib.rs`:
- Around line 2369-2370: The service field was made required by changing its
type/serde handling; revert it to optional to avoid the breaking API change:
change the field declaration to use Option<SocketAddr> and update the serde_as
attribute to #[serde_as(as = "Option<DisplayFromStr>")] (or add
#[serde(default)] if preferred), i.e., update the symbol service to
Option<SocketAddr> and adjust any code that constructs or reads this struct
(parsing/unwrap sites) to handle None safely.
- Around line 2059-2060: Revert the breaking change by restoring service to an
optional field: change the type in the affected structs (DMNState, DMNStateDiff,
MasternodeStatus) from SocketAddr to Option<SocketAddr> and restore serde
handling with serde_as to handle Option (e.g., use serde_as(as =
"Option<DisplayFromStr>")); update any code paths that access service to handle
None safely; include in the commit message a clear rationale referencing Dash
Core v24 compatibility and the original PR (`#523` / dash#7181) so the public API
break is documented or explicitly justified if you intend to keep the change.
---
Nitpick comments:
In `@rpc-json/src/lib.rs`:
- Around line 3260-3262: Regression tests that validated deserialization when
the optional `service` field is missing were removed; re-add tests that exercise
deserializing payloads lacking `service` for the affected types (e.g.
MasternodeListDiff and any structures handled by deserialize_u32_opt /
ExtendedQuorumListResult) so Core v24 compatibility is preserved. Add unit tests
that construct JSON blobs without `service`, call the existing deserialization
codepaths (the functions/impls that produce MasternodeListDiff,
ExtendedQuorumListResult, MnSyncStatus or QuorumType instances) and assert that
parsing succeeds and the resulting structs have the expected defaults/None for
the missing service value. Ensure tests reference the same deserialization
helpers (deserialize_u32_opt) used in production so they fail if
optional-service handling regresses.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d8bd76c4-ea47-4df8-99ab-b0f5b6a4f98f
📒 Files selected for processing (1)
rpc-json/src/lib.rs
| #[serde_as(as = "DisplayFromStr")] | ||
| pub service: SocketAddr, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for any documentation or comments about Core v24 compatibility
rg -n -i "core.*v?24|v24|service.*optional" --type=md --type=rsRepository: dashpay/rust-dashcore
Length of output: 91
🏁 Script executed:
#!/bin/bash
# Search for Core v24 references with correct ripgrep syntax
rg -n -i "core.*v?24|v24|service.*optional" --type=markdown --type=rustRepository: dashpay/rust-dashcore
Length of output: 389
🏁 Script executed:
#!/bin/bash
# Also search without type restrictions to catch comments
rg -n -i "core.*v?24|v24"Repository: dashpay/rust-dashcore
Length of output: 389
🏁 Script executed:
#!/bin/bash
# Look at the git history to find PR `#523` or related commits
git log --oneline --all | grep -i "optional\|service\|v24\|fix.*rpc" | head -20Repository: dashpay/rust-dashcore
Length of output: 1388
🏁 Script executed:
#!/bin/bash
# Check the actual file to understand the changes around lines 2059-2060
cd rpc-json/src && head -n 2070 lib.rs | tail -n 30Repository: dashpay/rust-dashcore
Length of output: 1113
🏁 Script executed:
#!/bin/bash
# Get the full commit message for the revert
git log -1 --format="%B" 6c5d0f22Repository: dashpay/rust-dashcore
Length of output: 208
🏁 Script executed:
#!/bin/bash
# Check if there's a PR or issue that explains the revert
git log -1 --format="%b" 6c5d0f22 | grep -i "close\|fix\|ref\|#"Repository: dashpay/rust-dashcore
Length of output: 47
🏁 Script executed:
#!/bin/bash
# Check what the actual PR `#523` commit message said
git log -1 --format="%B" b05bf704Repository: dashpay/rust-dashcore
Length of output: 1298
🏁 Script executed:
#!/bin/bash
# Check if this is part of a PR that documents the rationale
# Look for any PR or issue references in recent commits
git log --oneline -20 | head -5Repository: dashpay/rust-dashcore
Length of output: 153
🏁 Script executed:
#!/bin/bash
# Check the broader PR context - look at what files changed in this revert
git show --stat 6c5d0f22 | head -40Repository: dashpay/rust-dashcore
Length of output: 2335
This revert removes Core v24 compatibility without justification in the commit message.
PR #523 explicitly made service optional to support Dash Core v24, which gates the field behind -deprecatedrpc=service. The commit message documented that Core PR dash#7181 changed the default behavior, requiring this library to handle the field being absent.
This revert makes service required again, breaking support for Core v24 nodes. The revert commit message provides no explanation for this change.
Before merging, provide:
- Confirmation that Core v24 compatibility is no longer required
- Documentation of why this breaking change is necessary
- A proper commit message explaining the rationale (currently missing)
This is a breaking public API change affecting DMNState, DMNStateDiff, and MasternodeStatus.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@rpc-json/src/lib.rs` around lines 2059 - 2060, Revert the breaking change by
restoring service to an optional field: change the type in the affected structs
(DMNState, DMNStateDiff, MasternodeStatus) from SocketAddr to Option<SocketAddr>
and restore serde handling with serde_as to handle Option (e.g., use serde_as(as
= "Option<DisplayFromStr>")); update any code paths that access service to
handle None safely; include in the commit message a clear rationale referencing
Dash Core v24 compatibility and the original PR (`#523` / dash#7181) so the public
API break is documented or explicitly justified if you intend to keep the
change.
| #[serde_as(as = "DisplayFromStr")] | ||
| pub service: SocketAddr, |
There was a problem hiding this comment.
Same breaking API change as DMNState.service.
This field also becomes required, with the same Core v24 compatibility implications noted above.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@rpc-json/src/lib.rs` around lines 2369 - 2370, The service field was made
required by changing its type/serde handling; revert it to optional to avoid the
breaking API change: change the field declaration to use Option<SocketAddr> and
update the serde_as attribute to #[serde_as(as = "Option<DisplayFromStr>")] (or
add #[serde(default)] if preferred), i.e., update the symbol service to
Option<SocketAddr> and adjust any code that constructs or reads this struct
(parsing/unwrap sites) to handle None safely.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## v0.42-dev #586 +/- ##
=============================================
- Coverage 67.90% 67.70% -0.21%
=============================================
Files 318 318
Lines 67125 67022 -103
=============================================
- Hits 45583 45376 -207
- Misses 21542 21646 +104
|
1 participant
Reverts #523
Summary by CodeRabbit