-
Notifications
You must be signed in to change notification settings - Fork 1.6k
Fix: Remove non-canonical fields from tx_json in API v3
#6448
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
base: develop
Are you sure you want to change the base?
Changes from all commits
4a31ee1
9b0e87a
cf2835e
cbabee1
249fb12
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -760,6 +760,25 @@ class Transaction_test : public beast::unit_test::suite | |
| result[jss::result][jss::ledger_hash] == | ||
| "B41882E20F0EC6228417D28B9AE0F33833645D35F6799DFB782AC97FC4BB51" | ||
| "D2"); | ||
|
|
||
| auto const& tx_json = result[jss::result][jss::tx_json]; | ||
| if (apiVersion >= 3) | ||
| { | ||
| // In API v3, server-added lower-case fields must not appear | ||
| // inside tx_json; they are at the result level. | ||
| BEAST_EXPECT(!tx_json.isMember(jss::date)); | ||
| BEAST_EXPECT(!tx_json.isMember(jss::ledger_index)); | ||
| BEAST_EXPECT(!tx_json.isMember(jss::ctid)); | ||
| // date must be at result level in API v3 | ||
| BEAST_EXPECT(result[jss::result].isMember(jss::date)); | ||
| } | ||
| else | ||
| { | ||
| // In API v2, date and ledger_index are still included in | ||
| // tx_json for backwards compatibility. | ||
| BEAST_EXPECT(tx_json.isMember(jss::date)); | ||
| BEAST_EXPECT(tx_json.isMember(jss::ledger_index)); | ||
| } | ||
|
Comment on lines
764
to
781
|
||
| } | ||
|
|
||
| for (auto memberIt = expected.begin(); memberIt != expected.end(); memberIt++) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3,6 +3,7 @@ | |
| #include <xrpld/app/misc/DeliverMax.h> | ||
| #include <xrpld/app/misc/Transaction.h> | ||
| #include <xrpld/app/rdb/backend/SQLiteDatabase.h> | ||
| #include <xrpld/rpc/CTID.h> | ||
| #include <xrpld/rpc/Context.h> | ||
| #include <xrpld/rpc/DeliveredAmount.h> | ||
| #include <xrpld/rpc/MPTokenIssuanceID.h> | ||
|
|
@@ -11,6 +12,7 @@ | |
| #include <xrpld/rpc/detail/RPCLedgerHelpers.h> | ||
| #include <xrpld/rpc/detail/Tuning.h> | ||
|
|
||
| #include <xrpl/core/NetworkIDService.h> | ||
| #include <xrpl/json/json_value.h> | ||
| #include <xrpl/ledger/ReadView.h> | ||
| #include <xrpl/protocol/ErrorCodes.h> | ||
|
|
@@ -286,16 +288,31 @@ populateJsonResponse( | |
| auto const json_tx = (context.apiVersion > 1 ? jss::tx_json : jss::tx); | ||
| if (context.apiVersion > 1) | ||
| { | ||
| jvObj[json_tx] = txn->getJson( | ||
| JsonOptions::include_date | JsonOptions::disable_API_prior_V2, false); | ||
| auto const opts = context.apiVersion >= 3 | ||
| ? JsonOptions::disable_API_prior_V2 | JsonOptions::disable_API_prior_V3 | ||
| : JsonOptions::include_date | JsonOptions::disable_API_prior_V2; | ||
| jvObj[json_tx] = txn->getJson(opts, false); | ||
|
Comment on lines
291
to
294
|
||
| jvObj[jss::hash] = to_string(txn->getID()); | ||
| jvObj[jss::ledger_index] = txn->getLedger(); | ||
| jvObj[jss::ledger_hash] = | ||
| to_string(context.ledgerMaster.getHashBySeq(txn->getLedger())); | ||
|
|
||
| if (auto closeTime = | ||
| context.ledgerMaster.getCloseTimeBySeq(txn->getLedger())) | ||
| { | ||
| jvObj[jss::close_time_iso] = to_string_iso(*closeTime); | ||
| if (context.apiVersion >= 3) | ||
| jvObj[jss::date] = closeTime->time_since_epoch().count(); | ||
| } | ||
|
|
||
| if (context.apiVersion >= 3 && txnMeta) | ||
| { | ||
| uint32_t const lgrSeq = txn->getLedger(); | ||
| uint32_t const txnIdx = txnMeta->getIndex(); | ||
| uint32_t const netID = context.app.getNetworkIDService().getNetworkID(); | ||
| if (auto const ctid = RPC::encodeCTID(lgrSeq, txnIdx, netID)) | ||
| jvObj[jss::ctid] = *ctid; | ||
| } | ||
| } | ||
| else | ||
| jvObj[json_tx] = txn->getJson(JsonOptions::include_date); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -189,8 +189,14 @@ populateJsonResponse( | |
| auto const& sttx = result.txn->getSTransaction(); | ||
| if (context.apiVersion > 1) | ||
| { | ||
| constexpr auto optionsJson = | ||
| // In API v2, include_date and disable_API_prior_V2 are used to | ||
| // include date/ledger_index/ctid in tx_json. In API v3+, those | ||
| // fields are excluded from tx_json and are only at result level. | ||
| constexpr auto optionsV2 = | ||
| JsonOptions::include_date | JsonOptions::disable_API_prior_V2; | ||
| constexpr auto optionsV3 = | ||
| JsonOptions::disable_API_prior_V2 | JsonOptions::disable_API_prior_V3; | ||
| auto const optionsJson = context.apiVersion >= 3 ? optionsV3 : optionsV2; | ||
|
Comment on lines
+192
to
+199
|
||
| if (args.binary) | ||
| response[jss::tx_blob] = result.txn->getJson(optionsJson, true); | ||
| else | ||
|
|
@@ -210,7 +216,11 @@ populateJsonResponse( | |
| { | ||
| response[jss::ledger_index] = result.txn->getLedger(); | ||
| if (result.closeTime) | ||
| { | ||
| response[jss::close_time_iso] = to_string_iso(*result.closeTime); | ||
| if (context.apiVersion >= 3) | ||
| response[jss::date] = result.closeTime->time_since_epoch().count(); | ||
| } | ||
| } | ||
| } | ||
| else | ||
|
|
||
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.
For API v3 this test asserts
date/ledger_index/ctidare absent fromtx_json, but it doesn’t assert wheredate/ctidwent. If the goal is to move these server-added fields to the transaction object level (outsidetx_json), consider adding assertions thatpaymentincludesdate(andctidwhen available) for API v3 so the test fails if those fields are accidentally dropped.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.
Fixed in cf2835e. Added assertions
(payment.isMember(jss::date)) && (payment.isMember(jss::ctid))to verify these fields exist at the transaction object level (outsidetx_json) for API v3.