-
Notifications
You must be signed in to change notification settings - Fork 1.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Proposed 2.0.0-rc4 #4841
Proposed 2.0.0-rc4 #4841
Conversation
For api_version 2, always return ledger_index as integer in JSON output. api_version 1 retains prior behavior.
Show `DeliverMax` instead of `Amount` in output from `submit`, `submit_multisigned`, `sign`, and `sign_for`. Fix XRPLF#4829
// NOTE Use MultiApiJson to publish two slightly different JSON objects | ||
// for consumers supporting different API versions | ||
MultiApiJson multiObj{jvObj}; | ||
visit<RPC::apiMinimumSupportedVersion, RPC::apiMaximumValidVersion>( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR is designed to allow for trivial switching between API version 3 and version 2, which is why it looks like this. If this is indeed released for API version 2 then I have another PR lined up to clean this up, so we simply modify one version of Json::Value
rather than visit
here - i.e. simplify accordingly
@@ -159,7 +167,10 @@ fillJsonTx( | |||
txJson[jss::validated] = validated; | |||
if (validated) | |||
{ | |||
txJson[jss::ledger_index] = to_string(fill.ledger.seq()); | |||
auto const seq = fill.ledger.seq(); | |||
txJson[jss::ledger_index] = (fill.context->apiVersion > 1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR is designed to allow for trivial switching between API version 3 and version 2, which is why it looks like this. If this is indeed released for API version 2 then I have another PR lined up to remove redundant conditional expression here and instead unconditionally set ledger_index
to a number (since this is inside else if (fill.context->apiVersion > 1)
block starting at line 140)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Includes the two API fixes that we've been discussing.
Due to the lack of "2 weeks" of test time, @manojsdoshi has the right to reject this release candidate in favor of 2.0.0-rc3. However, these changes only impact the RPC API and are considered to be well-reviewed and low-risk bug fixes.