docs(rpc): introduce GetJsonHelp() to complement ToJson() for Dash-specific objects to aid generating RPC help text
#6872
Conversation
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
WalkthroughIntroduces a centralized RPCResult descriptor and GetRpcResult helper and declares Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📥 CommitsReviewing files that changed from the base of the PR and between 510fe5020a83ffa36dc989afd21ba9316f377475 and 56514c7. 📒 Files selected for processing (22)
💤 Files with no reviewable changes (3)
🚧 Files skipped from review as they are similar to previous changes (12)
🧰 Additional context used📓 Path-based instructions (2)src/**/*.{cpp,h,cc,cxx,hpp}📄 CodeRabbit inference engine (CLAUDE.md)
Files:
src/{masternode,evo}/**/*.{cpp,h,cc,cxx,hpp}📄 CodeRabbit inference engine (CLAUDE.md)
Files:
🧬 Code graph analysis (7)src/evo/mnhftx.h (1)
src/llmq/commitment.h (2)
src/core_io.h (2)
src/rpc/blockchain.cpp (1)
src/rpc/rawtransaction.cpp (1)
src/evo/core_write.cpp (4)
src/evo/simplifiedmns.h (2)
⏰ 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)
🔇 Additional comments (5)
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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between d2a6c34 and 64f5e15fea8aa49226c81122b37856f45724e2b9.
📒 Files selected for processing (19)
src/Makefile.am(1 hunks)src/core_io.h(3 hunks)src/evo/assetlocktx.h(3 hunks)src/evo/cbtx.h(2 hunks)src/evo/core_write.cpp(9 hunks)src/evo/mnhftx.h(3 hunks)src/evo/providertx.h(5 hunks)src/evo/simplifiedmns.h(2 hunks)src/evo/smldiff.h(3 hunks)src/llmq/commitment.h(4 hunks)src/llmq/core_write.cpp(1 hunks)src/llmq/signing.cpp(0 hunks)src/llmq/signing.h(2 hunks)src/llmq/snapshot.cpp(0 hunks)src/llmq/snapshot.h(3 hunks)src/rpc/blockchain.cpp(1 hunks)src/rpc/evo.cpp(1 hunks)src/rpc/quorums.cpp(3 hunks)src/rpc/rawtransaction.cpp(2 hunks)
💤 Files with no reviewable changes (2)
- src/llmq/signing.cpp
- src/llmq/snapshot.cpp
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-07-20T18:42:49.794Z
Learnt from: CR
PR: dashpay/dash#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-20T18:42:49.794Z
Learning: Applies to src/evo/specialtx.h : Special transactions use payload extensions defined in src/evo/specialtx.h
Applied to files:
src/evo/providertx.h
⏰ 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). (10)
- GitHub Check: linux64_multiprocess-build / Build source
- GitHub Check: mac-build / Build source
- GitHub Check: linux64_tsan-build / Build source
- GitHub Check: arm-linux-build / Build source
- GitHub Check: win64-build / Build source
- GitHub Check: linux64_nowallet-build / Build source
- GitHub Check: linux64_fuzz-build / Build source
- GitHub Check: linux64_ubsan-build / Build source
- GitHub Check: linux64_sqlite-build / Build source
- GitHub Check: linux64-build / Build source
🔇 Additional comments (12)
src/Makefile.am (1)
909-909: LGTM!The addition of
llmq/core_write.cppto the build sources is correct and aligns with the PR's objective of introducing GetJsonHelp infrastructure for llmq components.src/llmq/snapshot.h (2)
22-22: LGTM!The forward declaration of
RPCResultis properly placed and enables the GetJsonHelp method declarations below without requiring a full header include.
87-87: LGTM!The GetJsonHelp declarations are consistent across both
CQuorumSnapshotandCQuorumRotationInfoclasses, following the established pattern with proper[[nodiscard]]attributes.Also applies to: 204-204
src/evo/assetlocktx.h (1)
20-20: LGTM!The forward declaration and GetJsonHelp method declarations are properly placed and consistent with the pattern used throughout the PR for both
CAssetLockPayloadandCAssetUnlockPayloadclasses.Also applies to: 55-55, 113-113
src/evo/cbtx.h (1)
9-9: LGTM!The additions are consistent with the PR's pattern. The minor whitespace adjustments on lines 9, 11, and 15 don't affect functionality, and the forward declaration and GetJsonHelp method declaration are properly placed.
Also applies to: 11-11, 15-15, 22-22, 64-64
src/evo/simplifiedmns.h (2)
12-13: Verify the include reorganization.The PR adds
util/pointer.hand movesgsl/pointers.hto appear afterpubkey.h. This include reordering might be addressing header dependency issues or preparing for future changes.Please confirm this include reorganization is intentional and addresses a specific dependency requirement. If it resolves a compilation or ordering issue, consider adding a brief comment explaining why the order matters.
Also applies to: 17-18
23-23: LGTM!The forward declaration and GetJsonHelp method declaration follow the established pattern.
Also applies to: 102-102
src/rpc/evo.cpp (1)
1589-1589: LGTM!The change from empty
RPCResults{}toCSimplifiedMNListDiff::GetJsonHelp(/*key=*/"", /*optional=*/false)properly integrates the centralized JSON help schema for theprotx_diffRPC method, improving documentation consistency.src/evo/mnhftx.h (1)
29-29: LGTM!The forward declaration and GetJsonHelp declarations are properly added. The change to make
ToJson()non-inline (with implementation presumably moved to a .cpp file) is consistent with the PR's refactoring of JSON serialization infrastructure.Also applies to: 54-55, 85-86
src/llmq/commitment.h (2)
13-16: Include reorganization noted.The reordering of includes (moving BLS and LLMQ-specific headers together) appears to improve logical grouping, though the necessity isn't immediately clear from this diff.
If this include reorganization addresses compilation or ordering issues, it would be helpful to note that in the commit message or PR description.
32-32: LGTM!The forward declaration and GetJsonHelp method declarations follow the established pattern. The change to make
ToJson()non-inline inCFinalCommitmentis consistent with the refactoring approach seen in other files.Also applies to: 133-134, 163-163
src/rpc/rawtransaction.cpp (1)
175-184: Comprehensive payload docsNice to see the verbose schema now delegates to each payload’s
GetJsonHelp, so all DIP2/DIP29 extras stay documented in one place.
kwvg
force-pushed
the
evo_rpc_help
branch
4 times, most recently
from
ff98c19 to
ef2ccff
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between 64f5e15fea8aa49226c81122b37856f45724e2b9 and ef2ccffa5aca93da2a0d9f1c3e8fc982980055d4.
📒 Files selected for processing (22)
src/Makefile.am(1 hunks)src/core_io.h(3 hunks)src/evo/assetlocktx.h(3 hunks)src/evo/cbtx.h(2 hunks)src/evo/core_write.cpp(10 hunks)src/evo/deterministicmns.h(3 hunks)src/evo/dmnstate.cpp(0 hunks)src/evo/dmnstate.h(3 hunks)src/evo/mnhftx.h(3 hunks)src/evo/simplifiedmns.h(2 hunks)src/evo/smldiff.h(3 hunks)src/llmq/commitment.h(4 hunks)src/llmq/core_write.cpp(1 hunks)src/llmq/signing.cpp(0 hunks)src/llmq/signing.h(2 hunks)src/llmq/snapshot.cpp(0 hunks)src/llmq/snapshot.h(3 hunks)src/rpc/blockchain.cpp(1 hunks)src/rpc/evo.cpp(3 hunks)src/rpc/masternode.cpp(1 hunks)src/rpc/quorums.cpp(3 hunks)src/rpc/rawtransaction.cpp(2 hunks)
💤 Files with no reviewable changes (3)
- src/evo/dmnstate.cpp
- src/llmq/signing.cpp
- src/llmq/snapshot.cpp
✅ Files skipped from review due to trivial changes (1)
- src/rpc/masternode.cpp
🚧 Files skipped from review as they are similar to previous changes (4)
- src/llmq/snapshot.h
- src/rpc/blockchain.cpp
- src/Makefile.am
- src/llmq/core_write.cpp
⏰ 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). (10)
- GitHub Check: linux64_multiprocess-build / Build source
- GitHub Check: linux64_nowallet-build / Build source
- GitHub Check: win64-build / Build source
- GitHub Check: arm-linux-build / Build source
- GitHub Check: linux64_tsan-build / Build source
- GitHub Check: mac-build / Build source
- GitHub Check: linux64_sqlite-build / Build source
- GitHub Check: linux64_ubsan-build / Build source
- GitHub Check: linux64_fuzz-build / Build source
- GitHub Check: linux64-build / Build source
🔇 Additional comments (13)
src/evo/cbtx.h (1)
22-22: LGTM! Clean addition of GetJsonHelp pattern.The forward declaration of
RPCResultand the new static methodGetJsonHelpfollow the consistent pattern established across the PR for centralizing JSON schema generation.Also applies to: 64-64
src/evo/simplifiedmns.h (1)
12-18: LGTM! Consistent implementation of GetJsonHelp pattern.The changes properly introduce the JSON schema helper method while reorganizing includes for clearer dependencies. The
util/pointer.hinclude is needed for theutil::shared_ptr_equalusage at line 54.Also applies to: 23-23, 102-102
src/evo/assetlocktx.h (1)
20-20: LGTM! Consistent addition of GetJsonHelp to both payload classes.Both
CAssetLockPayloadandCAssetUnlockPayloadproperly receive theGetJsonHelpstatic method, maintaining consistency with the broader refactoring pattern.Also applies to: 55-55, 113-113
src/llmq/commitment.h (1)
13-16: LGTM! Well-structured refactoring for commitment classes.The changes properly introduce the
GetJsonHelppattern for bothCFinalCommitmentandCFinalCommitmentTxPayload. The include reorganization and conversion ofToJson()from inline to declaration-only align with the broader refactoring that moves implementations tocore_write.cppfiles.Also applies to: 32-39, 133-134, 163-164
src/evo/deterministicmns.h (1)
23-23: LGTM! Clean addition of GetJsonHelp to CDeterministicMN.The forward declarations are properly placed, and the new
GetJsonHelpmethod follows the established pattern. TheCProRegTxforward declaration is appropriate given its usage inCDeterministicMNStateat line 68.Also applies to: 38-38, 89-90
src/evo/mnhftx.h (1)
29-29: LGTM! Consistent GetJsonHelp pattern for MNHF transaction types.Both
MNHFTxandMNHFTxPayloadreceive theGetJsonHelpmethod, and the conversion ofToJson()to declaration-only aligns with moving the implementation to a separate compilation unit.Also applies to: 54-55, 85-85
src/evo/dmnstate.h (1)
23-30: LGTM! Comprehensive addition of GetJsonHelp to state classes.The forward declarations are well-organized, and both
CDeterministicMNStateandCDeterministicMNStateDiffproperly receive theGetJsonHelpmethod. The namespace reorganization improves clarity.Also applies to: 152-152, 246-246
src/llmq/signing.h (1)
33-34: LGTM! Clean addition with improved safety attribute.The
GetJsonHelpmethod follows the established pattern, and adding the[[nodiscard]]attribute toToJson()is a good practice to prevent accidental ignoring of the return value.Also applies to: 111-112
src/evo/core_write.cpp (5)
27-80: LGTM!The centralized RPC result descriptor system is well-designed. The
GetRpcResultfunction properly handles lookups and throws a descriptiveNonFatalCheckErrorfor invalid keys, which helps catch programming errors during development.
82-172: LGTM!The
GetJsonHelpandToJsonimplementations forCAssetLockPayload,CAssetUnlockPayload, andCCbTxare consistent and correctly describe their respective JSON schemas.
174-270: LGTM!The
CDeterministicMNStateimplementation correctly handles optional fields based on masternode type, and the schema properly marks these as optional. The separation ofToJsonimplementations across files is noted but doesn't affect correctness.
272-402: LGTM!The provider transaction types (
CProRegTx,CProUpRegTx,CProUpRevTx,CProUpServTx) have consistentGetJsonHelpandToJsonimplementations with proper handling of optional fields.
497-580: LGTM!The implementations for
CSimplifiedMNListEntry,MNHFTx, andMNHFTxPayloadare consistent between theirGetJsonHelpschemas andToJsonimplementations.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between ef2ccffa5aca93da2a0d9f1c3e8fc982980055d4 and 413c4eb9ae395aca5a0cbd20fdf4d7b12c836991.
📒 Files selected for processing (22)
src/Makefile.am(1 hunks)src/core_io.h(3 hunks)src/evo/assetlocktx.h(3 hunks)src/evo/cbtx.h(2 hunks)src/evo/core_write.cpp(10 hunks)src/evo/deterministicmns.h(3 hunks)src/evo/dmnstate.cpp(0 hunks)src/evo/dmnstate.h(3 hunks)src/evo/mnhftx.h(3 hunks)src/evo/simplifiedmns.h(2 hunks)src/evo/smldiff.h(3 hunks)src/llmq/commitment.h(4 hunks)src/llmq/core_write.cpp(1 hunks)src/llmq/signing.cpp(0 hunks)src/llmq/signing.h(2 hunks)src/llmq/snapshot.cpp(0 hunks)src/llmq/snapshot.h(3 hunks)src/rpc/blockchain.cpp(1 hunks)src/rpc/evo.cpp(3 hunks)src/rpc/masternode.cpp(1 hunks)src/rpc/quorums.cpp(3 hunks)src/rpc/rawtransaction.cpp(2 hunks)
💤 Files with no reviewable changes (3)
- src/llmq/snapshot.cpp
- src/llmq/signing.cpp
- src/evo/dmnstate.cpp
🚧 Files skipped from review as they are similar to previous changes (6)
- src/Makefile.am
- src/rpc/masternode.cpp
- src/evo/cbtx.h
- src/llmq/snapshot.h
- src/evo/smldiff.h
- src/rpc/quorums.cpp
🧰 Additional context used
📓 Path-based instructions (2)
src/**/*.{cpp,h,cc,cxx,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
src/**/*.{cpp,h,cc,cxx,hpp}: Dash Core C++ codebase must be written in C++20 and require at least Clang 16 or GCC 11.1
Dash uses unordered_lru_cache for efficient caching with LRU eviction
Files:
src/rpc/evo.cppsrc/evo/dmnstate.hsrc/evo/simplifiedmns.hsrc/llmq/signing.hsrc/evo/core_write.cppsrc/llmq/commitment.hsrc/llmq/core_write.cppsrc/rpc/blockchain.cppsrc/evo/deterministicmns.hsrc/evo/assetlocktx.hsrc/core_io.hsrc/rpc/rawtransaction.cppsrc/evo/mnhftx.h
src/{masternode,evo}/**/*.{cpp,h,cc,cxx,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
Masternode lists must use immutable data structures (Immer library) for thread safety
Files:
src/evo/dmnstate.hsrc/evo/simplifiedmns.hsrc/evo/core_write.cppsrc/evo/deterministicmns.hsrc/evo/assetlocktx.hsrc/evo/mnhftx.h
🧬 Code graph analysis (13)
src/rpc/evo.cpp (1)
src/evo/core_write.cpp (16)
GetJsonHelp(82-97)GetJsonHelp(82-82)GetJsonHelp(118-129)GetJsonHelp(118-118)GetJsonHelp(143-155)GetJsonHelp(143-143)GetJsonHelp(175-187)GetJsonHelp(175-175)GetJsonHelp(189-212)GetJsonHelp(189-189)GetJsonHelp(247-270)GetJsonHelp(247-247)GetJsonHelp(272-292)GetJsonHelp(272-272)GetJsonHelp(319-330)GetJsonHelp(319-319)
src/evo/dmnstate.h (3)
src/evo/deterministicmns.h (8)
nodiscard(85-113)nodiscard(259-267)nodiscard(269-282)nodiscard(420-423)nodiscard(425-432)nodiscard(450-468)nodiscard(470-488)nodiscard(490-505)src/evo/providertx.cpp (1)
nodiscard(18-28)src/evo/core_write.cpp (16)
GetJsonHelp(82-97)GetJsonHelp(82-82)GetJsonHelp(118-129)GetJsonHelp(118-118)GetJsonHelp(143-155)GetJsonHelp(143-143)GetJsonHelp(175-187)GetJsonHelp(175-175)GetJsonHelp(189-212)GetJsonHelp(189-189)GetJsonHelp(247-270)GetJsonHelp(247-247)GetJsonHelp(272-292)GetJsonHelp(272-272)GetJsonHelp(319-330)GetJsonHelp(319-319)
src/evo/simplifiedmns.h (1)
src/evo/core_write.cpp (16)
GetJsonHelp(82-97)GetJsonHelp(82-82)GetJsonHelp(118-129)GetJsonHelp(118-118)GetJsonHelp(143-155)GetJsonHelp(143-143)GetJsonHelp(175-187)GetJsonHelp(175-175)GetJsonHelp(189-212)GetJsonHelp(189-189)GetJsonHelp(247-270)GetJsonHelp(247-247)GetJsonHelp(272-292)GetJsonHelp(272-272)GetJsonHelp(319-330)GetJsonHelp(319-319)
src/llmq/signing.h (1)
src/evo/core_write.cpp (32)
GetJsonHelp(82-97)GetJsonHelp(82-82)GetJsonHelp(118-129)GetJsonHelp(118-118)GetJsonHelp(143-155)GetJsonHelp(143-143)GetJsonHelp(175-187)GetJsonHelp(175-175)GetJsonHelp(189-212)GetJsonHelp(189-189)GetJsonHelp(247-270)GetJsonHelp(247-247)GetJsonHelp(272-292)GetJsonHelp(272-272)GetJsonHelp(319-330)GetJsonHelp(319-319)ToJson(99-116)ToJson(99-99)ToJson(131-141)ToJson(131-131)ToJson(157-172)ToJson(157-157)ToJson(214-244)ToJson(214-214)ToJson(294-317)ToJson(294-294)ToJson(332-344)ToJson(332-332)ToJson(357-365)ToJson(357-357)ToJson(384-402)ToJson(384-384)
src/evo/core_write.cpp (5)
src/evo/netinfo.cpp (6)
ret(459-459)ToJson(242-257)ToJson(242-242)ToJson(448-468)ToJson(448-448)obj(40-40)src/evo/dmnstate.cpp (4)
ToJson(35-130)ToJson(35-35)obj(37-37)obj(106-106)src/evo/cbtx.h (1)
nVersion(41-68)src/evo/smldiff.h (1)
nVersion(56-91)src/evo/dmnstate.h (7)
nRegisteredHeight(42-285)nLastPaidHeight(43-43)nConsecutivePayments(44-44)nPoSePenalty(45-45)nPoSeRevivedHeight(46-284)nRevocationReason(47-154)platformNodeID(62-62)
src/llmq/commitment.h (1)
src/evo/core_write.cpp (32)
GetJsonHelp(82-97)GetJsonHelp(82-82)GetJsonHelp(118-129)GetJsonHelp(118-118)GetJsonHelp(143-155)GetJsonHelp(143-143)GetJsonHelp(175-187)GetJsonHelp(175-175)GetJsonHelp(189-212)GetJsonHelp(189-189)GetJsonHelp(247-270)GetJsonHelp(247-247)GetJsonHelp(272-292)GetJsonHelp(272-272)GetJsonHelp(319-330)GetJsonHelp(319-319)ToJson(99-116)ToJson(99-99)ToJson(131-141)ToJson(131-131)ToJson(157-172)ToJson(157-157)ToJson(214-244)ToJson(214-214)ToJson(294-317)ToJson(294-294)ToJson(332-344)ToJson(332-332)ToJson(357-365)ToJson(357-357)ToJson(384-402)ToJson(384-384)
src/llmq/core_write.cpp (2)
src/evo/core_write.cpp (10)
GetRpcResult(72-80)GetRpcResult(72-72)ret(112-112)ret(133-133)ret(159-159)ret(296-296)ret(334-334)ret(359-359)ret(386-386)ret(576-576)src/llmq/commitment.h (2)
quorumIndex(54-54)CountSigners(68-75)
src/rpc/blockchain.cpp (1)
src/evo/core_write.cpp (16)
GetJsonHelp(82-97)GetJsonHelp(82-82)GetJsonHelp(118-129)GetJsonHelp(118-118)GetJsonHelp(143-155)GetJsonHelp(143-143)GetJsonHelp(175-187)GetJsonHelp(175-175)GetJsonHelp(189-212)GetJsonHelp(189-189)GetJsonHelp(247-270)GetJsonHelp(247-247)GetJsonHelp(272-292)GetJsonHelp(272-272)GetJsonHelp(319-330)GetJsonHelp(319-319)
src/evo/deterministicmns.h (1)
src/evo/core_write.cpp (16)
GetJsonHelp(82-97)GetJsonHelp(82-82)GetJsonHelp(118-129)GetJsonHelp(118-118)GetJsonHelp(143-155)GetJsonHelp(143-143)GetJsonHelp(175-187)GetJsonHelp(175-175)GetJsonHelp(189-212)GetJsonHelp(189-189)GetJsonHelp(247-270)GetJsonHelp(247-247)GetJsonHelp(272-292)GetJsonHelp(272-272)GetJsonHelp(319-330)GetJsonHelp(319-319)
src/evo/assetlocktx.h (1)
src/evo/core_write.cpp (16)
GetJsonHelp(82-97)GetJsonHelp(82-82)GetJsonHelp(118-129)GetJsonHelp(118-118)GetJsonHelp(143-155)GetJsonHelp(143-143)GetJsonHelp(175-187)GetJsonHelp(175-175)GetJsonHelp(189-212)GetJsonHelp(189-189)GetJsonHelp(247-270)GetJsonHelp(247-247)GetJsonHelp(272-292)GetJsonHelp(272-272)GetJsonHelp(319-330)GetJsonHelp(319-319)
src/core_io.h (1)
src/evo/core_write.cpp (2)
GetRpcResult(72-80)GetRpcResult(72-72)
src/rpc/rawtransaction.cpp (1)
src/evo/core_write.cpp (16)
GetJsonHelp(82-97)GetJsonHelp(82-82)GetJsonHelp(118-129)GetJsonHelp(118-118)GetJsonHelp(143-155)GetJsonHelp(143-143)GetJsonHelp(175-187)GetJsonHelp(175-175)GetJsonHelp(189-212)GetJsonHelp(189-189)GetJsonHelp(247-270)GetJsonHelp(247-247)GetJsonHelp(272-292)GetJsonHelp(272-272)GetJsonHelp(319-330)GetJsonHelp(319-319)
src/evo/mnhftx.h (2)
src/evo/deterministicmns.h (8)
nodiscard(85-113)nodiscard(259-267)nodiscard(269-282)nodiscard(420-423)nodiscard(425-432)nodiscard(450-468)nodiscard(470-488)nodiscard(490-505)src/evo/core_write.cpp (32)
GetJsonHelp(82-97)GetJsonHelp(82-82)GetJsonHelp(118-129)GetJsonHelp(118-118)GetJsonHelp(143-155)GetJsonHelp(143-143)GetJsonHelp(175-187)GetJsonHelp(175-175)GetJsonHelp(189-212)GetJsonHelp(189-189)GetJsonHelp(247-270)GetJsonHelp(247-247)GetJsonHelp(272-292)GetJsonHelp(272-272)GetJsonHelp(319-330)GetJsonHelp(319-319)ToJson(99-116)ToJson(99-99)ToJson(131-141)ToJson(131-131)ToJson(157-172)ToJson(157-157)ToJson(214-244)ToJson(214-214)ToJson(294-317)ToJson(294-294)ToJson(332-344)ToJson(332-332)ToJson(357-365)ToJson(357-357)ToJson(384-402)ToJson(384-384)
⏰ 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). (10)
- GitHub Check: linux64_multiprocess-build / Build source
- GitHub Check: linux64_tsan-build / Build source
- GitHub Check: linux64_sqlite-build / Build source
- GitHub Check: linux64_ubsan-build / Build source
- GitHub Check: linux64_fuzz-build / Build source
- GitHub Check: linux64-build / Build source
- GitHub Check: mac-build / Build source
- GitHub Check: arm-linux-build / Build source
- GitHub Check: linux64_nowallet-build / Build source
- GitHub Check: win64-build / Build source
🔇 Additional comments (9)
src/evo/deterministicmns.h (1)
38-38: LGTM!The forward declaration of
RPCResultand the newGetJsonHelpmethod are consistent with the broader JSON schema standardization pattern applied across the codebase.Also applies to: 90-90
src/core_io.h (1)
61-62: LGTM!The centralized
GetRpcResultfunction declaration is well-placed and the comment indicating the implementation location (// evo/core_write.cpp) aids code navigation.src/evo/dmnstate.h (1)
152-152: LGTM!The
GetJsonHelpmethods added to bothCDeterministicMNStateandCDeterministicMNStateDifffollow the consistent signature pattern and are appropriately marked[[nodiscard]].Also applies to: 246-246
src/rpc/evo.cpp (2)
1589-1589: LGTM!The switch from
RPCResults{}toCSimplifiedMNListDiff::GetJsonHelp(/*key=*/"", /*optional=*/false)successfully integrates the standardized JSON schema approach for theprotx diffRPC.
1682-1682: Good tracking of future work.The TODO comments appropriately document future refactoring opportunities to integrate
GetJsonHelpformnandstateDiffobjects inprotx listdiff. This aligns with the PR's scoped approach.Also applies to: 1705-1705
src/evo/mnhftx.h (1)
54-55: LGTM!The additions of
GetJsonHelpmethods to bothMNHFTxandMNHFTxPayloadfollow the consistent pattern. Moving theToJsonimplementation out of the header is a good practice that reduces header bloat.Also applies to: 85-85
src/evo/simplifiedmns.h (1)
23-23: LGTM!The forward declaration and
GetJsonHelpmethod addition toCSimplifiedMNListEntryalign with the standardized JSON schema pattern across the codebase.Also applies to: 102-102
src/evo/assetlocktx.h (1)
55-55: LGTM!The
GetJsonHelpmethods added to bothCAssetLockPayloadandCAssetUnlockPayloadfollow the consistent pattern and enable standardized JSON schema generation for asset lock transactions.Also applies to: 113-113
src/llmq/signing.h (1)
111-112: LGTM!The addition of
GetJsonHelptoCRecoveredSigand markingToJsonas[[nodiscard]]are good changes. The[[nodiscard]]attribute helps catch potential bugs where the return value is accidentally ignored.
kwvg
force-pushed
the
evo_rpc_help
branch
2 times, most recently
from
44a196c to
6eb0c3c
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/rpc/quorums.cpp (1)
855-861: Remove stray semicolon and fix formattingThere is an extra standalone semicolon after context initialization; also CI reports clang-format differences. Remove the semicolon and run clang-format.
- const LLMQContext& llmq_ctx = EnsureLLMQContext(node); - ; + const LLMQContext& llmq_ctx = EnsureLLMQContext(node);src/rpc/evo.cpp (1)
1-1: Run clang-format on src/rpc/evo.cpp
CI failed due to style mismatches. Please applyclang-format -style=file -i src/rpc/evo.cppand commit the changes.
🧹 Nitpick comments (3)
src/evo/deterministicmns.h (1)
23-27: Remove redundant forward declaration
class CProRegTx;is redundant since <evo/providertx.h> is already included. Consider dropping it to avoid confusion.src/core_io.h (1)
61-63: Avoid layering inversion for GetRpcResult
Move the declaration of GetRpcResult out of src/core_io.h into an evo-specific header (e.g. src/evo/core_rpc.h) or relocate its implementation into core to maintain proper layer boundaries. Ensure all callers include the full RPCResult definition (from src/rpc/util.h) before using it by value.src/llmq/core_write.cpp (1)
157-174: Consider using STL algorithms for array construction.The current loop-based approach is clear and correct. However, you could use
std::transformorstd::ranges::copyto address the cppcheck warnings if desired.For example:
UniValue CQuorumSnapshot::ToJson() const { UniValue obj(UniValue::VOBJ); UniValue activeQ(UniValue::VARR); - for (const bool h : activeQuorumMembers) { - // cppcheck-suppress useStlAlgorithm - activeQ.push_back(h); - } + std::ranges::for_each(activeQuorumMembers, [&](bool h) { activeQ.push_back(h); }); obj.pushKV("activeQuorumMembers", activeQ); obj.pushKV("mnSkipListMode", mnSkipListMode); UniValue skipList(UniValue::VARR); - for (const auto& h : mnSkipList) { - // cppcheck-suppress useStlAlgorithm - skipList.push_back(h); - } + std::ranges::for_each(mnSkipList, [&](const auto& h) { skipList.push_back(h); }); obj.pushKV("mnSkipList", skipList); return obj; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between 6eb0c3c6c403c5a46ba71619c57bf72b632d1a7c and fc9b0e62a0725df0808a611748e05ede87fefca7.
📒 Files selected for processing (22)
src/Makefile.am(1 hunks)src/core_io.h(3 hunks)src/evo/assetlocktx.h(3 hunks)src/evo/cbtx.h(2 hunks)src/evo/core_write.cpp(10 hunks)src/evo/deterministicmns.h(3 hunks)src/evo/dmnstate.cpp(0 hunks)src/evo/dmnstate.h(3 hunks)src/evo/mnhftx.h(3 hunks)src/evo/simplifiedmns.h(2 hunks)src/evo/smldiff.h(3 hunks)src/llmq/commitment.h(4 hunks)src/llmq/core_write.cpp(1 hunks)src/llmq/signing.cpp(0 hunks)src/llmq/signing.h(2 hunks)src/llmq/snapshot.cpp(0 hunks)src/llmq/snapshot.h(3 hunks)src/rpc/blockchain.cpp(1 hunks)src/rpc/evo.cpp(3 hunks)src/rpc/masternode.cpp(1 hunks)src/rpc/quorums.cpp(3 hunks)src/rpc/rawtransaction.cpp(2 hunks)
💤 Files with no reviewable changes (3)
- src/llmq/snapshot.cpp
- src/llmq/signing.cpp
- src/evo/dmnstate.cpp
🚧 Files skipped from review as they are similar to previous changes (9)
- src/evo/mnhftx.h
- src/rpc/blockchain.cpp
- src/rpc/masternode.cpp
- src/llmq/snapshot.h
- src/Makefile.am
- src/evo/simplifiedmns.h
- src/evo/smldiff.h
- src/evo/cbtx.h
- src/evo/assetlocktx.h
🧰 Additional context used
📓 Path-based instructions (2)
src/**/*.{cpp,h,cc,cxx,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
src/**/*.{cpp,h,cc,cxx,hpp}: Dash Core C++ codebase must be written in C++20 and require at least Clang 16 or GCC 11.1
Dash uses unordered_lru_cache for efficient caching with LRU eviction
Files:
src/rpc/evo.cppsrc/rpc/rawtransaction.cppsrc/evo/deterministicmns.hsrc/rpc/quorums.cppsrc/llmq/signing.hsrc/core_io.hsrc/evo/core_write.cppsrc/llmq/core_write.cppsrc/evo/dmnstate.hsrc/llmq/commitment.h
src/{masternode,evo}/**/*.{cpp,h,cc,cxx,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
Masternode lists must use immutable data structures (Immer library) for thread safety
Files:
src/evo/deterministicmns.hsrc/evo/core_write.cppsrc/evo/dmnstate.h
🧬 Code graph analysis (6)
src/rpc/quorums.cpp (1)
src/llmq/core_write.cpp (10)
GetJsonHelp(18-35)GetJsonHelp(18-18)GetJsonHelp(55-63)GetJsonHelp(55-55)GetJsonHelp(74-99)GetJsonHelp(74-74)GetJsonHelp(143-155)GetJsonHelp(143-143)GetJsonHelp(176-187)GetJsonHelp(176-176)
src/llmq/signing.h (1)
src/llmq/core_write.cpp (20)
GetJsonHelp(18-35)GetJsonHelp(18-18)GetJsonHelp(55-63)GetJsonHelp(55-55)GetJsonHelp(74-99)GetJsonHelp(74-74)GetJsonHelp(143-155)GetJsonHelp(143-143)GetJsonHelp(176-187)GetJsonHelp(176-176)ToJson(37-53)ToJson(37-37)ToJson(65-72)ToJson(65-65)ToJson(101-141)ToJson(101-101)ToJson(157-174)ToJson(157-157)ToJson(189-199)ToJson(189-189)
src/core_io.h (1)
src/univalue/include/univalue.h (1)
UniValue(19-23)
src/llmq/core_write.cpp (3)
src/llmq/commitment.h (2)
quorumIndex(54-54)CountSigners(68-75)src/evo/assetlocktx.h (1)
quorumSig(84-84)src/evo/mnhftx.h (1)
sig(40-40)
src/evo/dmnstate.h (1)
src/evo/deterministicmns.h (8)
nodiscard(85-113)nodiscard(259-267)nodiscard(269-282)nodiscard(420-423)nodiscard(425-432)nodiscard(450-468)nodiscard(470-488)nodiscard(490-505)
src/llmq/commitment.h (1)
src/llmq/core_write.cpp (20)
GetJsonHelp(18-35)GetJsonHelp(18-18)GetJsonHelp(55-63)GetJsonHelp(55-55)GetJsonHelp(74-99)GetJsonHelp(74-74)GetJsonHelp(143-155)GetJsonHelp(143-143)GetJsonHelp(176-187)GetJsonHelp(176-176)ToJson(37-53)ToJson(37-37)ToJson(65-72)ToJson(65-65)ToJson(101-141)ToJson(101-101)ToJson(157-174)ToJson(157-157)ToJson(189-199)ToJson(189-189)
🪛 GitHub Actions: Clang Diff Format Check
src/rpc/evo.cpp
[error] 1-1: Clang format differences found. Run the clang-format-diff tool or 'clang-format' to fix formatting in this file.
src/rpc/quorums.cpp
[error] 1-1: Clang format differences found. Run the clang-format-diff tool or 'clang-format' to fix formatting in this file.
src/evo/core_write.cpp
[error] 1-1: Clang format differences found. Run the clang-format-diff tool or 'clang-format' to fix formatting in this file.
src/llmq/core_write.cpp
[error] 1-1: Clang format differences found. Run the clang-format-diff tool or 'clang-format' to fix formatting in this file.
⏰ 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). (10)
- GitHub Check: linux64_multiprocess-build / Build source
- GitHub Check: mac-build / Build source
- GitHub Check: linux64_ubsan-build / Build source
- GitHub Check: linux64_sqlite-build / Build source
- GitHub Check: linux64_fuzz-build / Build source
- GitHub Check: linux64-build / Build source
- GitHub Check: linux64_nowallet-build / Build source
- GitHub Check: arm-linux-build / Build source
- GitHub Check: win64-build / Build source
- GitHub Check: linux64_tsan-build / Build source
🔇 Additional comments (21)
src/llmq/commitment.h (1)
133-135: LGTM: Public JSON help and ToJson separationGetJsonHelp hooks and moving ToJson out-of-line align with the new schema approach and reduce header churn.
Also applies to: 163-164
src/evo/deterministicmns.h (1)
90-92: LGTM: Added GetJsonHelp for CDeterministicMNGood addition for consistent RPC help across DMN types.
src/rpc/evo.cpp (1)
1589-1589: LGTM: Result schema now uses GetJsonHelpSwitching protx diff to CSimplifiedMNListDiff::GetJsonHelp standardizes help text.
src/llmq/signing.h (1)
111-113: LGTM: JSON help hook and nodiscard ToJsonAdditions are consistent with the new RPC schema approach and improve API usage.
Also applies to: 33-36
src/rpc/quorums.cpp (3)
655-656: LGTM: Standardized result schema for getrecsigUsing CRecoveredSig::GetJsonHelp is consistent and avoids duplication.
851-852: LGTM: Standardized result schema for rotationinfoUsing CQuorumRotationInfo::GetJsonHelp keeps help coherent across RPCs.
1-1: Fix formatting in src/rpc/quorums.cpp Runclang-format -style=fileon this file to satisfy CI formatting checks.src/evo/dmnstate.h (1)
152-153: LGTM: JSON help for state and diffAdding GetJsonHelp to CDeterministicMNState and CDeterministicMNStateDiff supports consistent RPC docs.
Also applies to: 246-247
src/rpc/rawtransaction.cpp (1)
175-184: LGTM: Verbose schema now documents special TX payloads
Confirmed propagation of all payload keys from DecodeTxDoc to TxToUniv in core_write.cpp.src/llmq/core_write.cpp (4)
18-53: LGTM!The
GetJsonHelpandToJsonimplementations forCFinalCommitmentare correct. The version-dependent BLS formatting logic properly handles legacy and indexed quorum versions.
55-72: LGTM!The nested
CFinalCommitmentschema integration is handled correctly.
74-141: LGTM!The conditional field exposure based on
extraSharecorrectly matches the schema. Array construction forlastCommitmentPerIndex,quorumSnapshotList, andmnListDiffListis properly implemented.
176-199: LGTM!The
GetJsonHelpandToJsonimplementations forCRecoveredSigcorrectly handle BLS signature serialization and hash computation.src/evo/core_write.cpp (8)
27-71: LGTM!The centralized
RPCRESULT_MAPprovides comprehensive RPC result descriptors. Good documentation of deprecated fields (service,platformHTTPPort,platformP2PPort) that aids developers working with nightlies, aligning with the PR's motivation.
73-81: LGTM!The
GetRpcResulthelper correctly validates descriptor lookups and throwsNonFatalCheckErrorfor missing keys, preventing runtime issues from referencing undefined descriptors.
83-98: LGTM!Asset lock and unlock payload schemas are correctly defined with proper nesting and field types.
Also applies to: 119-130
144-173: LGTM!The
CCbTxschema correctly documents version-dependent fields, matching the conditional logic inToJsonforMERKLE_ROOT_QUORUMSandCLSIG_AND_BALANCEversions.
176-245: LGTM!The
CDeterministicMNandCDeterministicMNStateimplementations correctly handle nested schemas, conditional field exposure based onMnType::Evo, and safe destination extraction for optional address fields.
273-403: LGTM!Provider transaction schemas (
CProRegTx,CProUpRegTx,CProUpRevTx,CProUpServTx) are correctly defined. TheToJsonimplementations properly handle conditional field inclusion based on masternode type and destination extraction success.
405-433: LGTM!The
CSimplifiedMNListDiffschema is correctly defined. ThequorumsCLSigsfield properly documents the dynamic key structure using the"<sig_hex>"placeholder, matching the actual JSON output where each object uses BLS signature hex as the key. This aligns with the resolution of previous review feedback.Based on learnings
498-581: LGTM!The remaining schemas (
CSimplifiedMNListEntry,MNHFTx,MNHFTxPayload) are correctly implemented with proper conditional logic for extended mode serialization and type-based field exposure.
thephez
left a comment
thephez
left a comment
There was a problem hiding this comment.
Some comments/suggestions
Currently, the most reliable source of RPC help text is the published documentation. This is fine for releases as the whole set of changes undergo evaluation but a problem for nightlies or documenting behavior changes or deprecations in-source.
Header resorting in the previous commit revealed some headers that were transiently included were no longer available, this should resolve that.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/rpc/evo.cpp (1)
1683-1683: Replace TODOs with GetJsonHelp wiring when readyFollow-up: use CDeterministicMN::GetJsonHelp for mn and CDeterministicMNStateDiff::GetJsonHelp for stateDiff to keep docs in sync.
I can draft the small changes when you’re ready.
Also applies to: 1706-1706
src/rpc/quorums.cpp (1)
369-369: Follow-up: Convert comment to GetJsonHelp usageReplace this TODO by wiring CFinalCommitment::GetJsonHelp to document fqc.ToJson().
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between fc9b0e62a0725df0808a611748e05ede87fefca7 and 510fe5020a83ffa36dc989afd21ba9316f377475.
📒 Files selected for processing (23)
src/Makefile.am(1 hunks)src/core_io.h(3 hunks)src/evo/assetlocktx.h(3 hunks)src/evo/cbtx.h(2 hunks)src/evo/core_write.cpp(10 hunks)src/evo/deterministicmns.h(3 hunks)src/evo/dmnstate.cpp(0 hunks)src/evo/dmnstate.h(3 hunks)src/evo/mnhftx.h(3 hunks)src/evo/providertx.h(5 hunks)src/evo/simplifiedmns.h(2 hunks)src/evo/smldiff.h(3 hunks)src/llmq/commitment.h(4 hunks)src/llmq/core_write.cpp(1 hunks)src/llmq/signing.cpp(0 hunks)src/llmq/signing.h(2 hunks)src/llmq/snapshot.cpp(0 hunks)src/llmq/snapshot.h(3 hunks)src/rpc/blockchain.cpp(1 hunks)src/rpc/evo.cpp(3 hunks)src/rpc/masternode.cpp(1 hunks)src/rpc/quorums.cpp(3 hunks)src/rpc/rawtransaction.cpp(2 hunks)
💤 Files with no reviewable changes (3)
- src/evo/dmnstate.cpp
- src/llmq/snapshot.cpp
- src/llmq/signing.cpp
🚧 Files skipped from review as they are similar to previous changes (10)
- src/rpc/masternode.cpp
- src/evo/smldiff.h
- src/evo/deterministicmns.h
- src/rpc/blockchain.cpp
- src/evo/providertx.h
- src/evo/cbtx.h
- src/evo/assetlocktx.h
- src/Makefile.am
- src/evo/simplifiedmns.h
- src/evo/mnhftx.h
🧰 Additional context used
📓 Path-based instructions (2)
src/**/*.{cpp,h,cc,cxx,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
src/**/*.{cpp,h,cc,cxx,hpp}: Dash Core C++ codebase must be written in C++20 and require at least Clang 16 or GCC 11.1
Dash uses unordered_lru_cache for efficient caching with LRU eviction
Files:
src/core_io.hsrc/llmq/snapshot.hsrc/llmq/commitment.hsrc/evo/dmnstate.hsrc/llmq/core_write.cppsrc/rpc/evo.cppsrc/llmq/signing.hsrc/rpc/quorums.cppsrc/evo/core_write.cppsrc/rpc/rawtransaction.cpp
src/{masternode,evo}/**/*.{cpp,h,cc,cxx,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
Masternode lists must use immutable data structures (Immer library) for thread safety
Files:
src/evo/dmnstate.hsrc/evo/core_write.cpp
🧬 Code graph analysis (10)
src/core_io.h (2)
src/univalue/include/univalue.h (1)
UniValue(19-23)src/evo/core_write.cpp (2)
GetRpcResult(73-81)GetRpcResult(73-73)
src/llmq/snapshot.h (1)
src/evo/core_write.cpp (16)
GetJsonHelp(83-98)GetJsonHelp(83-83)GetJsonHelp(119-130)GetJsonHelp(119-119)GetJsonHelp(144-156)GetJsonHelp(144-144)GetJsonHelp(176-188)GetJsonHelp(176-176)GetJsonHelp(190-213)GetJsonHelp(190-190)GetJsonHelp(248-271)GetJsonHelp(248-248)GetJsonHelp(273-293)GetJsonHelp(273-273)GetJsonHelp(320-331)GetJsonHelp(320-320)
src/llmq/commitment.h (1)
src/evo/core_write.cpp (16)
GetJsonHelp(83-98)GetJsonHelp(83-83)GetJsonHelp(119-130)GetJsonHelp(119-119)GetJsonHelp(144-156)GetJsonHelp(144-144)GetJsonHelp(176-188)GetJsonHelp(176-176)GetJsonHelp(190-213)GetJsonHelp(190-190)GetJsonHelp(248-271)GetJsonHelp(248-248)GetJsonHelp(273-293)GetJsonHelp(273-273)GetJsonHelp(320-331)GetJsonHelp(320-320)
src/evo/dmnstate.h (3)
src/evo/deterministicmns.h (8)
nodiscard(85-113)nodiscard(259-267)nodiscard(269-282)nodiscard(420-423)nodiscard(425-432)nodiscard(450-468)nodiscard(470-488)nodiscard(490-505)src/evo/providertx.cpp (1)
nodiscard(18-28)src/evo/core_write.cpp (16)
GetJsonHelp(83-98)GetJsonHelp(83-83)GetJsonHelp(119-130)GetJsonHelp(119-119)GetJsonHelp(144-156)GetJsonHelp(144-144)GetJsonHelp(176-188)GetJsonHelp(176-176)GetJsonHelp(190-213)GetJsonHelp(190-190)GetJsonHelp(248-271)GetJsonHelp(248-248)GetJsonHelp(273-293)GetJsonHelp(273-273)GetJsonHelp(320-331)GetJsonHelp(320-320)
src/llmq/core_write.cpp (2)
src/evo/core_write.cpp (10)
GetRpcResult(73-81)GetRpcResult(73-73)ret(113-113)ret(134-134)ret(160-160)ret(297-297)ret(335-335)ret(360-360)ret(387-387)ret(577-577)src/llmq/commitment.h (2)
quorumIndex(54-54)CountSigners(68-75)
src/rpc/evo.cpp (1)
src/evo/core_write.cpp (16)
GetJsonHelp(83-98)GetJsonHelp(83-83)GetJsonHelp(119-130)GetJsonHelp(119-119)GetJsonHelp(144-156)GetJsonHelp(144-144)GetJsonHelp(176-188)GetJsonHelp(176-176)GetJsonHelp(190-213)GetJsonHelp(190-190)GetJsonHelp(248-271)GetJsonHelp(248-248)GetJsonHelp(273-293)GetJsonHelp(273-273)GetJsonHelp(320-331)GetJsonHelp(320-320)
src/llmq/signing.h (1)
src/evo/core_write.cpp (16)
GetJsonHelp(83-98)GetJsonHelp(83-83)GetJsonHelp(119-130)GetJsonHelp(119-119)GetJsonHelp(144-156)GetJsonHelp(144-144)GetJsonHelp(176-188)GetJsonHelp(176-176)GetJsonHelp(190-213)GetJsonHelp(190-190)GetJsonHelp(248-271)GetJsonHelp(248-248)GetJsonHelp(273-293)GetJsonHelp(273-273)GetJsonHelp(320-331)GetJsonHelp(320-320)
src/rpc/quorums.cpp (1)
src/evo/core_write.cpp (16)
GetJsonHelp(83-98)GetJsonHelp(83-83)GetJsonHelp(119-130)GetJsonHelp(119-119)GetJsonHelp(144-156)GetJsonHelp(144-144)GetJsonHelp(176-188)GetJsonHelp(176-176)GetJsonHelp(190-213)GetJsonHelp(190-190)GetJsonHelp(248-271)GetJsonHelp(248-248)GetJsonHelp(273-293)GetJsonHelp(273-273)GetJsonHelp(320-331)GetJsonHelp(320-320)
src/evo/core_write.cpp (7)
src/llmq/core_write.cpp (25)
ret(67-67)ret(191-191)GetJsonHelp(18-35)GetJsonHelp(18-18)GetJsonHelp(55-63)GetJsonHelp(55-55)GetJsonHelp(74-99)GetJsonHelp(74-74)GetJsonHelp(143-155)GetJsonHelp(143-143)GetJsonHelp(176-187)GetJsonHelp(176-176)ToJson(37-53)ToJson(37-37)ToJson(65-72)ToJson(65-65)ToJson(101-141)ToJson(101-101)ToJson(157-174)ToJson(157-157)ToJson(189-199)ToJson(189-189)obj(39-39)obj(103-103)obj(159-159)src/evo/netinfo.cpp (6)
ret(459-459)ToJson(242-257)ToJson(242-242)ToJson(448-468)ToJson(448-448)obj(40-40)src/evo/cbtx.h (1)
nVersion(41-68)src/evo/smldiff.h (1)
nVersion(56-91)src/evo/dmnstate.h (7)
nRegisteredHeight(42-285)nLastPaidHeight(43-43)nConsecutivePayments(44-44)nPoSePenalty(45-45)nPoSeRevivedHeight(46-284)nRevocationReason(47-154)platformNodeID(62-62)src/evo/assetlocktx.h (1)
quorumHash(83-83)src/evo/mnhftx.h (2)
quorumHash(39-39)sig(40-40)
src/rpc/rawtransaction.cpp (1)
src/evo/core_write.cpp (16)
GetJsonHelp(83-98)GetJsonHelp(83-83)GetJsonHelp(119-130)GetJsonHelp(119-119)GetJsonHelp(144-156)GetJsonHelp(144-144)GetJsonHelp(176-188)GetJsonHelp(176-176)GetJsonHelp(190-213)GetJsonHelp(190-190)GetJsonHelp(248-271)GetJsonHelp(248-248)GetJsonHelp(273-293)GetJsonHelp(273-273)GetJsonHelp(320-331)GetJsonHelp(320-320)
⏰ 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). (11)
- GitHub Check: mac-build / Build source
- GitHub Check: predict_conflicts
- GitHub Check: linux64_tsan-build / Build source
- GitHub Check: linux64_nowallet-build / Build source
- GitHub Check: arm-linux-build / Build source
- GitHub Check: linux64_sqlite-build / Build source
- GitHub Check: linux64-build / Build source
- GitHub Check: linux64_ubsan-build / Build source
- GitHub Check: linux64_fuzz-build / Build source
- GitHub Check: win64-build / Build source
- GitHub Check: Lint / Run linters
🔇 Additional comments (15)
src/llmq/signing.h (2)
33-34: Forward declaration looks goodForward-declaring RPCResult here avoids heavy includes and keeps header lean.
111-112: Definitions exist for CRecoveredSig JSON helpers Both CRecoveredSig::GetJsonHelp and CRecoveredSig::ToJson are implemented in src/llmq/core_write.cpp; no further action required.src/rpc/evo.cpp (1)
1590-1590: Good switch to schema helperUsing CSimplifiedMNListDiff::GetJsonHelp keeps help text centralized and consistent with ToJson().
src/evo/dmnstate.h (2)
23-27: Forward declarations are appropriateKeeps header light; no issues.
Also applies to: 29-30
152-152: Approve GetJsonHelp declarations Confirmed implementations in src/evo/core_write.cpp at lines 190 and 248.src/core_io.h (1)
61-63: Public GetRpcResult() API — implementation confirmedFound matching definition in
src/evo/core_write.cpp; signature aligns with the declaration insrc/core_io.h. No further action.src/rpc/quorums.cpp (2)
851-851: Verified CQuorumRotationInfo methods implemented CQuorumRotationInfo::GetJsonHelp and ToJson are defined in src/llmq/core_write.cpp.
655-655: CRecoveredSig JSON helpers implemented – CRecoveredSig::GetJsonHelp and ToJson are defined in src/llmq/core_write.cpp.src/rpc/rawtransaction.cpp (1)
175-184: Confirm payload help key consistencyAll referenced GetJsonHelp functions (proRegTx, proUpServTx, proUpRegTx, proUpRevTx, cbTx, qcTx, mnhfTx, assetLockTx, assetUnlockTx) are defined. Verify these JSON help keys match the corresponding field names used in ToJson and DecodeTxDoc.
src/llmq/snapshot.h (1)
22-23: Implementations present; forward declarations match definitionsRPCResult GetJsonHelp and UniValue ToJson are defined in src/llmq/core_write.cpp for both CQuorumSnapshot and CQuorumRotationInfo, matching the declarations in snapshot.h.
src/llmq/core_write.cpp (1)
1-199: LGTM! Well-structured JSON serialization for LLMQ types.The implementation is clean and follows a consistent pattern:
GetJsonHelp()provides RPC schema metadataToJson()produces matching JSON output- Conditional field exposure (e.g.,
extraShareat lines 110-112, 120-122) correctly aligns with optional markers in the schema (lines 82, 88)- Version-aware BLS serialization (lines 48-51) properly handles legacy vs. modern formats
The
cppcheck-suppress useStlAlgorithmcomments (lines 162, 169) are justified—UniValue's API requires explicitpush_back()calls rather than algorithm-based approaches.src/evo/core_write.cpp (4)
27-81: LGTM! Centralized RPC result map with robust error handling.The
RPCRESULT_MAPprovides a single source of truth for RPC field descriptors, andGetRpcResult()ensures missing keys fail fast with clear diagnostic messages viaNonFatalCheckError. The macro-based entry definition (line 28) keeps the map concise and consistent.Note: Past review comments confirm
ownerAddress(line 51) was added and is correctly referenced by downstream code.
83-293: LGTM! Comprehensive GetJsonHelp implementations for asset and masternode transactions.The implementations correctly:
- Use
GetRpcResult()for shared fields (e.g.,version,height, addresses)- Define inline descriptors for transaction-specific fields
- Mark optional fields consistently (e.g.,
payoutAddress, Platform fields)- Handle nested objects (e.g.,
scriptPubKeyat lines 92-95,commitmentat line 61)The conditional Platform field handling (lines 230-234 in
ToJson, lines 288-290 inGetJsonHelp) properly gates EVO-specific fields onnType == MnType::Evo.
405-545: LGTM! Masternode list diff schema correctly documents dynamic signature keys.The
quorumsCLSigsschema (lines 427-431) now accurately reflects the JSON structure where each array element is an object keyed by BLS signature hex (<sig_hex>) mapping to an array of quorum indices, matching the implementation at lines 484-494.Note: Past review comments confirm this schema mismatch was resolved.
The
CSimplifiedMNListEntryimplementation (lines 498-545) properly handles theextendedparameter to conditionally include payout addresses (lines 535-543).
547-581: LGTM! Hard fork signal transaction schemas are concise and correct.Both
MNHFTxandMNHFTxPayloadfollow the established pattern with nested object composition (signalat line 571) and proper field descriptions.
| #include <string> | ||
|
|
||
| namespace llmq { | ||
| RPCResult CFinalCommitment::GetJsonHelp(const std::string& key, bool optional) |
There was a problem hiding this comment.
nit: I don't really like RPC code be spread all over codebase in different subdirectories, src/llmq/core_write.cpp is not the best place to have it, but I have no better idea at the moment.
If methods such as CSimplifiedMNListEntry::ToJson is kinda RPC related, but generic - it's json, no direct mentions of RPC, but GetjsonHelp is RPCResult, not just a json.
There was a problem hiding this comment.
maybe src/rpc/llmq_utils.cpp?
Consider also changing signature of method to GetJsonHelp<CFinalCommitment>() maybe?
There was a problem hiding this comment.
Yeah, I don't like obviously RPC code living outside the rpc/ directory but our problem is twofold
-
We need the sources to be a part of
libbitcoin_common(the whole reason we've made{evo,llmq}/core_write.cpp) -
We need the help text to be right next to the definition so they don't go out of sync (but even this rule had to be broken as two
ToJson()definitions use txindex, which is not a part oflibbitcoin_commonand movingGetJsonInfo()to those source files creates very ugly circular dependencies)
What makes this acceptable is that RPCResult is a relatively uncomplicated structure that is mostly defined in the header.
|
I am working on series of backports, that validates RPCResult; I will have a PR in a couple of days, bitcoin#25237, bitcoin#25170, bitcoin#25161 and missing changes from bitcoin#20459 It will help to validate RPCResult in help is really matching with provided output. For example, wrong field names, types such as: will cause error like that: or like these: Consider waiting my PR. |
| #include <string> | ||
|
|
||
| namespace llmq { | ||
| RPCResult CFinalCommitment::GetJsonHelp(const std::string& key, bool optional) |
There was a problem hiding this comment.
maybe src/rpc/llmq_utils.cpp?
Consider also changing signature of method to GetJsonHelp<CFinalCommitment>() maybe?
| @@ -6,16 +6,20 @@ | |||
| #define BITCOIN_EVO_CBTX_H | |||
|
|
|||
| #include <bls/bls.h> | |||
|
|
|||
| #include <primitives/transaction.h> | ||
|
|
| #include <bls/bls.h> | ||
| #include <evo/dmn_types.h> | ||
| #include <evo/netinfo.h> | ||
| #include <evo/providertx.h> | ||
| #include <evo/simplifiedmns.h> | ||
|
|
| @@ -9,15 +9,19 @@ | |||
| #include <evo/dmn_types.h> | |||
| #include <evo/netinfo.h> | |||
| #include <evo/providertx.h> | |||
| #include <gsl/pointers.h> | |||
| #include <util/pointer.h> | |||
|
|
|||
There was a problem hiding this comment.
nit: no \n here needed; sort headers also
| struct CSpentIndexTxInfo; | ||
| struct RPCResult; | ||
|
|
Actually, let's get this one merged first; if any follow-up updates or fixes needed, I will make them. LGTM overall |
Keeping true to separation of concerns
We exclude the following:
- `evo/netinfo.{cpp,h}` (already a part of `libbitcoin_common`)
as it as an inheritance structure and the help text for it is
relatively uncomplicated.
- `CDeterministicMN::ToJson() `and `CDeterministicMNStateDiff::ToJson()`
as it relies on `g_txindex` that is part of `libbitcoin_node` so it
cannot be part of `evo/core_write.cpp`.
We are using it extensively and could do with some cleanup
…governance RPC logic to resolve pending definitions 562aeb7 rpc: add JSON help defs for `CGovernanceObject` fragments (Kittywhiskers Van Gogh) 57edf31 rpc: deduplicate routines used to generate governance RPC results (Kittywhiskers Van Gogh) 5866061 rpc: add `GetJsonHelp()` defs for `CGovernance{Manager,Object},Object` (Kittywhiskers Van Gogh) 0efaad3 move-only: consolidate governance `ToJson()` defs to `core_write.cpp` (Kittywhiskers Van Gogh) 6063d79 rpc: add help text for `masternode list` (Kittywhiskers Van Gogh) ae40575 rpc: add help text for `protx listdiff`, `masternode status` (Kittywhiskers Van Gogh) 291b8f6 rpc: add help text for `quorum dkgstatus` (Kittywhiskers Van Gogh) 56b95d4 rpc: add `GetJsonHelp()` defs for `CDKGDebug{,Session}Status` (Kittywhiskers Van Gogh) Pull request description: ## Motivation [dash#7062](#7062) introduces breaking changes. To convey the change in return object parameter's optional status, we need to be able to emit complete documentation to begin with. This pull request aims to fill up the remaining gaps in Dash-specific RPC documentation (which were thankfully annotated with TODOs courtesy of [dash#6886](#6886), thanks knst!). This is a continuation of the work started in [dash#6872](#6872). ## Additional Information * Dependency for #7062 * `quorum dkgstatus` requires additional work to generate its help text as the RPC does something unusual, it takes the object returned by CDKGDebugStatus ([source](https://github.com/dashpay/dash/blob/bac6bfadffe6fcf323e5e0208c8619e1e68401fe/src/rpc/quorums.cpp#L356C10-L356C13)) and _modifies_ it ([source](https://github.com/dashpay/dash/blob/bac6bfadffe6fcf323e5e0208c8619e1e68401fe/src/rpc/quorums.cpp#L425)) instead of treating it as a distinct value paired to a key. This necessitated defining a separate function to generate the help text for `quorum dkgstatus`, `quorum_dkgstatus_help()`, that has to mirror this mutating behavior due to the `const`-only structure of `RPCResult` ([source](https://github.com/dashpay/dash/blob/bac6bfadffe6fcf323e5e0208c8619e1e68401fe/src/rpc/util.h#L265-L270)). * This has also been done for `gobject get` though it is a product of deduplication done in this PR and not a decision made when the RPC was first introduced. * Two governance RPCs, `gobject {diff,list}` and `gobject get`, refer to the same value, local validity status, with different keys, `fBlockchainValidity` and `fLocalValidity` respectively. This required a workaround to maintain deduplication by allowing to overwrite the key name after fetching the description using the key in `GetRpcResult` by specifying `override_name`. ## Breaking Changes None expected. ## Checklist - [x] I have performed a self-review of my own code - [x] I have commented my code, particularly in hard-to-understand areas - [x] I have added or updated relevant unit/integration/functional/e2e tests **(note: N/A)** - [x] I have made corresponding changes to the documentation **(note: N/A)** - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_ ACKs for top commit: UdjinM6: utACK 562aeb7 Tree-SHA512: 5497ad7aff0c07900d9775fe9f52af0661bceb76f48ee709de63e742b2e767f9fdc968717001dccb63cbebf75249a43931704f352a08a8e4dc14e8b5a7b47c4b
5 participants
Motivation
When working on extended addresses, we deprecated
service,platformHTTPPortandplatformP2PPortand had the method of deprecation changed forservicedue to feedback. During that process, it was apparent that there wasn't a way to convey deprecation status outside of release notes as we didn't generate RPC help text for objects that feature the affected fields.Currently, the authoritative source of documentation is Dash Core developer documentation (source) and it has been used as a source for the description of various fields described in the PR but changes, reorganizations and deprecations are not reflected in real time when they are done in-source, they need to feature in a release in order for the corresponding documentation to reflect it.
This makes development on nightlies relatively painful and makes decisions like deprecations noted in release notes with little to show for it in-source. This pull request aims to take the first step to remedying that.
Additional Information
The description of a
GetJsonHelp()'sRPCResultis suppressed if thekeyis blank, this is because if the object is part of an array, the description should be set in the higher level entity.clang-formatsuggestions have been intentionally ignored in the interest of legibility, the extent of nesting and construction in this PR requires us to make an exception to standard linting practices. This is a general pain point in RPC code but is mentioned here for posterity.This pull request is not exhaustive, the scope of this pull request is to define the help text for objects and integrate them where convenient, if there are RPCs that have more complex structures, they will not benefit from this PR as no existing structure is present for
GetJsonHelp()to slot into. Work on this can be addressed in future pull requests.We have used
const std::string&instead ofstd::string_viewdue to limitations inRPCResultand the UniValue API. If these limitations have been resolved, we can switch over to it trivially. This will not help makeRPCRESULT_MAPconstexprasstd::maps cannot be standaloneconstexprobjects even in C++26 (source).Breaking Changes
None expected.
Checklist