fix casting issues in RPC CLIs, remove proof from book_offers
#6044
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #6044 +/- ##
=======================================
Coverage 78.6% 78.6%
=======================================
Files 818 818
Lines 68989 69012 +23
Branches 8248 8244 -4
=======================================
+ Hits 54199 54226 +27
+ Misses 14790 14786 -4
... and 2 files with indirect coverage changes 🚀 New features to boost your workflow:
|
mvadari
changed the title
proof from book_offers
|
|
||
| while (!bDone && iParams >= 2) | ||
| { | ||
| // VFALCO Why is Json::StaticString appearing on the right side? |
There was a problem hiding this comment.
As a flyby, does this comment still make sense? It's seems a little pointless
There was a problem hiding this comment.
It is still factually correct (jss is Json::StaticString). Not sure why it's a problem though.
src/xrpld/rpc/detail/RPCCall.cpp
Outdated
| std::int64_t uLedgerMin = jvParams[1u].asInt(); | ||
| std::int64_t uLedgerMax = jvParams[2u].asInt(); | ||
| std::int32_t ledgerMin, ledgerMax; | ||
| if (auto ledgerMinOpt = jvParseInt(jvParams[1u])) |
There was a problem hiding this comment.
Can be const
| if (auto ledgerMinOpt = jvParseInt(jvParams[1u])) | |
| if (auto const ledgerMinOpt = jvParseInt(jvParams[1u])) |
There was a problem hiding this comment.
Here too, shouldn't we parse the ledgerMin as uint?
There was a problem hiding this comment.
No, -1 is a valid value. https://xrpl.org/docs/references/http-websocket-apis/public-api-methods/account-methods/account_tx
A value of -1 instructs the server to use the most recent validated ledger version available.
There was a problem hiding this comment.
Got it, thanks! Might be worth adding a blurb in the comments though, explaining that.
src/xrpld/rpc/detail/RPCCall.cpp
Outdated
| std::int64_t uLedgerMin = jvParams[1u].asInt(); | ||
| std::int64_t uLedgerMax = jvParams[2u].asInt(); | ||
| std::int32_t ledgerMin, ledgerMax; | ||
| if (auto ledgerMinOpt = jvParseInt(jvParams[1u])) |
There was a problem hiding this comment.
Here too, shouldn't we parse the ledgerMin as uint?
src/xrpld/rpc/detail/RPCCall.cpp
Outdated
|
|
||
| #include <array> | ||
| #include <iostream> | ||
| #include <type_traits> |
There was a problem hiding this comment.
nit: (driveby) This include isn't used.
src/xrpld/rpc/detail/RPCCall.cpp
Outdated
| if (jvParams.size()) | ||
| jvRequest[jss::min_count] = jvParams[0u].asUInt(); | ||
| { | ||
| if (auto minCount = jvParseUInt(jvParams[0u])) |
There was a problem hiding this comment.
My bad, I missed this:
| if (auto minCount = jvParseUInt(jvParams[0u])) | |
| if (auto const minCount = jvParseUInt(jvParams[0u])) |
src/xrpld/rpc/detail/RPCCall.cpp
Outdated
| { | ||
| jvRequest[jss::ip] = ip; | ||
| jvRequest[jss::port] = jvParams[1u].asUInt(); | ||
| if (auto port = jvParseUInt(jvParams[1u])) |
There was a problem hiding this comment.
I missed this too:
| if (auto port = jvParseUInt(jvParams[1u])) | |
| if (auto const port = jvParseUInt(jvParams[1u])) |
src/xrpld/rpc/detail/RPCCall.cpp
Outdated
| if (input.find_first_not_of("0123456789") == std::string::npos) | ||
| jvRequest["can_delete"] = jvParams[0u].asUInt(); | ||
| { | ||
| if (auto seq = jvParseUInt(jvParams[0u])) |
There was a problem hiding this comment.
And I missed this too:
| if (auto seq = jvParseUInt(jvParams[0u])) | |
| if (auto const seq = jvParseUInt(jvParams[0u])) |
High Level Overview of Change
This PR fixes some casting issues that resulted in some bad errors in the RPC CLIs, namely:
account_txbook_offerscan_deleteconnectget_countstx_historyThis PR also removes the
proofparameter frombook_offers, which isn't documented anywhere and also doesn't do anything (there will be no change in behavior except in the CLI, since rippled just ignores other parameters).This also results in some serious simplification opportunities in the
RPCCall_testtests.Context of Change
Downstream of #6043
Type of Change
API Impact
Some bugs were fixed in the CLI API. The
proofparameter was also removed, which decreases the number of CLI params inbook_offers.There are no changes to HTTPS and WS API users.
Test Plan
CI passes. Tests were adjusted.