refactor: move API functions from RPCHelpers.h to ApiVersion.h - #5889
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #5889 +/- ##
=======================================
Coverage 78.2% 78.2%
=======================================
Files 817 816 -1
Lines 68940 68949 +9
Branches 8353 8353
=======================================
+ Hits 53934 53943 +9
Misses 15006 15006
... and 3 files with indirect coverage changes 🚀 New features to boost your workflow:
|
| if (apiVersion == apiVersionIfUnspecified) | ||
| { | ||
| /** | ||
| * API version numbers used in API version 1 |
There was a problem hiding this comment.
I think this is not a doxy comment that will actually work (reach documentation). You probably better use a simple comment instead
| * 3) the version number is unspecified and | ||
| * APIVersionIfUnspecified is out of the supported range | ||
| * | ||
| * @param value a Json value that may or may not specifies |
There was a problem hiding this comment.
The parameter is currently called "jv" not "value"
|
|
||
| Json::Value const maxVersion( | ||
| betaEnabled ? RPC::apiBetaVersion : RPC::apiMaximumSupportedVersion); | ||
| Json::Value requestedVersion(RPC::apiVersionIfUnspecified); |
There was a problem hiding this comment.
I understand this is just moving code around but consider cleaning this up (e.g. adding some whitespace where applicable)
| { | ||
| requestedVersion = jv.get(jss::api_version, requestedVersion); | ||
| } | ||
| if (!(requestedVersion.isInt() || requestedVersion.isUInt()) || |
There was a problem hiding this comment.
nit: This is a horrible condition. Any improvement here would be nice 🔢
| XRPL_ASSERT( | ||
| apiVersion != apiInvalidVersion, | ||
| "ripple::RPC::setVersion : input is valid"); | ||
| auto&& object = addObject(parent, jss::version); |
There was a problem hiding this comment.
This auto&& is a bit confusing. According to addObject signature this would call the overload which returns by value if Object is not a Value. Is it some sort of shared pointer inside? I don't understand why setting fields on object below would make sense as object is technically a temporary here.
Also, Object is a terrible name for a template parameter - I did not even notice it at first and assumed that it's the Object type from the Json library. This definitely needs renaming at the very least but also try implementing some tests that use this function with Object (template type) be actual Object from Json library and see if it even does anything useful.
There was a problem hiding this comment.
I think adding tests etc for Object is out of scope of this PR but I'll look into a separate cleanup effort. Based on some preliminary digging I think there's a good chance we can get rid of that whole Object class and only use Json::Value everywhere.
There was a problem hiding this comment.
I did switch out the && for & in 7617f1b though
mvadari
added
the
Ready to merge
…RPLF#5889) This change moves two functions, `setVersion` and `getAPIVersionNumber`, from `RPCHelpers.h` to `ApiVersion.h`.
4 participants
High Level Overview of Change
This PR moves two functions,
setVersionandgetAPIVersionNumber, fromRPCHelpers.htoApiVersion.h. There are no functionality or code changes.Context of Change
Split out of #5684 to hopefully start making that PR a bit easier to review.
Type of Change
API Impact
N/A
Test Plan
CI passes
Future Tasks
Add another PR that creates
RPCLedgerHelpersand splits out that part of #5684, leaving only the actual functionality changes in #5684