Unify JSON serialization format of transactions #4775

Bronek · 2023-10-18T16:16:06Z

High Level Overview of Change

Unify JSON format of transaction as per #4727 , intended for API version 2 release

Type of Change

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
Refactor (non-breaking change that only restructures code)
Tests (you added tests for code that already exists, or your new feature included in this PR)
Documentation update
Chore (no impact to binary, e.g. .gitignore, formatting, dropping support for older tooling)
Release

API Impact

Public API: New feature (new methods and/or new fields)
Public API: Breaking change (in general, breaking changes should only impact the next api_version)
libxrpl change (any change that may affect libxrpl or dependents of libxrpl)
Peer protocol change (must be backward compatible or bump the peer protocol version)

intelliot · 2023-10-24T20:49:17Z

@ximinez @ckeshava this is ready for review

Bronek · 2023-10-25T16:19:33Z

@ximinez @ckeshava this is ready for review

Please note I will appreciate if you verify that I have not missed (or indeed, missed) anything in #4727

Bronek · 2023-11-02T17:50:30Z

@ximinez I think I implemented all your remarks. Do you mind reviewing again ? Thank you !

We may consider turning this into a general-purpose template and using it elsewhere

ckeshava

hey bronek, I really like the latest set of changes on getJson, it really simplifies things. I've left a few minor comments.

I think the presence of jss::ledger_index inside tx_json still needs to be addressed.

src/ripple/rpc/handlers/AccountTx.cpp

src/ripple/protocol/STTx.h

src/ripple/app/misc/impl/Transaction.cpp

src/ripple/app/misc/Transaction.h

src/test/rpc/AccountTx_test.cpp

Bronek · 2023-11-02T21:27:12Z

I'd like to draw your attention to the jss::tx_json field. In apiVersion > 1, is it supposed to contain the jss::ledger_index and jss::date fields? The Recommendations suggest that these fields need to be removed.

no, want to keep it where it is (in addition to the new location) in API version 2; then remove in API version 3

ckeshava

thanks bronek, this PR looks good.

Per XRPLF/rippled#4775 (comment) this command only returns transactions from validated ledgers, so this clarifies that detail

ximinez

Partial review. Some of these comments may be outdated due to changes since I originally wrote them.

ximinez · 2023-10-26T20:09:44Z

src/ripple/app/ledger/impl/LedgerMaster.cpp

+    }
+    catch (SHAMapMissingNode const& mn)
+    {
+        auto stream = app_.journal("IsValidated").warn();  // TODO Better name ?


LedgerMaster has an m_journal member. May as well use that. In which case, you won't really need the stream local. Just fall back to the usual pattern of JLOG(m_journal.warn()).

src/ripple/app/ledger/impl/LedgerMaster.cpp

src/ripple/app/ledger/impl/LedgerToJson.cpp

ximinez · 2023-10-26T20:59:36Z

src/ripple/app/misc/NetworkOPs.cpp

+    // NOTE jvObj which is not a finished object for either API version. After
+    // it's populated, we need to finish it for a specific API version. This is
+    // done in a loop, near the end of this function.
+    std::string hash = {};


What about this idea? hash is getting built the same way, regardless of the code path (to_string(getTransactionID())). The old code path sticks it into the Json object; the new way sticks it into an optional. What if you undo the changes to STTx::getJson, then pull the hash value it builds out here?
It would look something like:

jvObj[jss::transaction] = transaction->getJson(JsonOptions::none); std::string const hash = jvObj[jss::transaction][jss::hash].asString(); jvObj[jss::transaction].removeMember(jss::hash);

Another advantage of initializing it here is that it can be made const.

src/ripple/app/ledger/impl/LedgerMaster.cpp

src/ripple/protocol/STTx.h

src/ripple/protocol/impl/STTx.cpp

src/ripple/app/misc/impl/Transaction.cpp

src/ripple/protocol/STBase.h

Fix #4727 Squashed commit of the following: commit 53e384f Author: Bronek Kozicki <[email protected]> Date: Fri Nov 3 13:37:29 2023 +0000 Minor improvements commit c1f2ea4 Merge: 942e209 056255e Author: Bronek Kozicki <[email protected]> Date: Thu Nov 2 21:43:11 2023 +0000 Merge branch 'develop' into feature/unify_transaction_json commit 942e209 Author: Bronek Kozicki <[email protected]> Date: Thu Nov 2 21:26:05 2023 +0000 Unconditionally set validated in account_tx output commit b5b1118 Author: Bronek Kozicki <[email protected]> Date: Thu Nov 2 21:14:37 2023 +0000 Remove obsolete comments commit 7770bc5 Author: Chenna Keshava <[email protected]> Date: Thu Nov 2 13:16:44 2023 -0700 simplify the extraction of transactionID from Transaction object commit 94c16d8 Author: Bronek Kozicki <[email protected]> Date: Thu Nov 2 20:26:09 2023 +0000 Replace class enum JsonOptions with struct We may consider turning this into a general-purpose template and using it elsewhere commit b2a8a2d Author: Bronek Kozicki <[email protected]> Date: Thu Nov 2 17:46:54 2023 +0000 Minor improvements commit 95b6055 Author: Bronek Kozicki <[email protected]> Date: Thu Nov 2 17:14:53 2023 +0000 Improve getJson for Transaction and STTx commit 17e6588 Author: Bronek Kozicki <[email protected]> Date: Thu Nov 2 12:06:30 2023 +0000 Fix typos commit 5973390 Author: Bronek Kozicki <[email protected]> Date: Wed Nov 1 15:55:02 2023 +0000 Fix validated and close_time_iso in account_tx commit cdb3384 Author: Bronek Kozicki <[email protected]> Date: Tue Oct 31 17:27:57 2023 +0000 Add ledger_hash, ledger_index to transaction_entry commit 0d8673c Author: Bronek Kozicki <[email protected]> Date: Tue Oct 31 17:12:53 2023 +0000 Update API-CHANGELOG.md commit 625c812 Merge: 54f1d60 3b624d8 Author: Bronek Kozicki <[email protected]> Date: Mon Oct 30 21:32:35 2023 +0000 Merge branch 'develop' into feature/unify_transaction_json commit 54f1d60 Author: Bronek Kozicki <[email protected]> Date: Mon Oct 30 21:19:40 2023 +0000 Set ledger_hash on closed ledger, even if not validated commit ef1276c Author: Bronek Kozicki <[email protected]> Date: Mon Oct 30 20:59:58 2023 +0000 Minor fixes commit c81bece Author: Bronek Kozicki <[email protected]> Date: Mon Oct 30 17:45:49 2023 +0000 Rename mInLedger to mLedgerIndex commit c477eb4 Author: Bronek Kozicki <[email protected]> Date: Mon Oct 30 17:35:33 2023 +0000 Improved comments commit e386093 Author: Chenna Keshava <[email protected]> Date: Thu Oct 26 17:51:41 2023 -0700 additional tests for Subscribe unit tests commit 9f1d544 Author: Bronek Kozicki <[email protected]> Date: Mon Oct 30 16:09:17 2023 +0000 Time formatting fix commit 4e92ee4 Author: Bronek Kozicki <[email protected]> Date: Thu Oct 26 14:04:40 2023 +0000 Store closeTime in LedgerFill commit 1115cef Author: Bronek Kozicki <[email protected]> Date: Thu Oct 26 13:22:26 2023 +0100 Move isValidated from RPCHelpers to LedgerMaster commit 2035bbc Author: Bronek Kozicki <[email protected]> Date: Tue Oct 24 19:15:50 2023 +0000 Set ledger_hash and ledger_index commit bdf90e5 Author: Bronek Kozicki <[email protected]> Date: Tue Oct 24 18:24:30 2023 +0000 Remove inLedger from API version 2 commit c2a3b52 Author: Bronek Kozicki <[email protected]> Date: Tue Oct 24 17:39:45 2023 +0000 Add unit test for tx commit 1a96800 Author: Bronek Kozicki <[email protected]> Date: Tue Oct 24 16:53:31 2023 +0000 Add unit test for transaction_entry commit 2b397c9 Author: Bronek Kozicki <[email protected]> Date: Tue Oct 24 15:45:41 2023 +0000 Add small APIv2 unit test for subscribe commit 4d517e2 Author: Bronek Kozicki <[email protected]> Date: Mon Oct 23 15:27:35 2023 +0000 Store close_time_iso in API v2 output commit 910f125 Author: Bronek Kozicki <[email protected]> Date: Wed Oct 18 17:24:42 2023 +0100 Output from tx commit 0fb0517 Author: Bronek Kozicki <[email protected]> Date: Wed Oct 18 17:24:26 2023 +0100 Output from transaction_entry commit e4ce8b1 Author: Bronek Kozicki <[email protected]> Date: Wed Oct 18 17:24:08 2023 +0100 Output from account_tx commit 92635aa Author: Bronek Kozicki <[email protected]> Date: Wed Oct 18 17:23:49 2023 +0100 Output from ledger commit 76eee5a Author: Bronek Kozicki <[email protected]> Date: Wed Oct 18 17:23:16 2023 +0100 Output from sign, submit etc. commit aa6de60 Author: Bronek Kozicki <[email protected]> Date: Mon Oct 23 18:20:21 2023 +0000 Output for subscriptions commit ee53c5d Author: Bronek Kozicki <[email protected]> Date: Wed Oct 25 18:17:10 2023 +0000 Formatting fix commit 65346dc Author: Bronek Kozicki <[email protected]> Date: Wed Oct 25 18:07:18 2023 +0000 Remove include <ranges>

ximinez

I have a handful of comments here, but they're all pretty minor.

src/ripple/app/misc/NetworkOPs.cpp

src/ripple/protocol/STBase.h

src/ripple/rpc/handlers/AccountTx.cpp

src/test/rpc/Subscribe_test.cpp

src/test/rpc/TransactionEntry_test.cpp

src/test/rpc/Transaction_test.cpp

arihantkothari · 2023-11-07T14:02:50Z

src/test/rpc/Subscribe_test.cpp

@@ -163,7 +164,7 @@ class Subscribe_test : public beast::unit_test::suite
    }

    void
-    testTransactions()
+    testTransactions_APIv1()


This PR is great and I'm approving it. But I think we should also add testcase("transactions API Version 1"). Small change :) I know that a bunch of tests don't have the testcase, and I'm not sure why... but if we have the facility, we should use it 😆

src/test/rpc/Transaction_test.cpp

* Remove include <ranges> * Formatting fix * Output for subscriptions * Output from sign, submit etc. * Output from ledger * Output from account_tx * Output from transaction_entry * Output from tx * Store close_time_iso in API v2 output * Add small APIv2 unit test for subscribe * Add unit test for transaction_entry * Add unit test for tx * Remove inLedger from API version 2 * Set ledger_hash and ledger_index * Move isValidated from RPCHelpers to LedgerMaster * Store closeTime in LedgerFill * Time formatting fix * additional tests for Subscribe unit tests * Improved comments * Rename mInLedger to mLedgerIndex * Minor fixes * Set ledger_hash on closed ledger, even if not validated * Update API-CHANGELOG.md * Add ledger_hash, ledger_index to transaction_entry * Fix validated and close_time_iso in account_tx * Fix typos * Improve getJson for Transaction and STTx * Minor improvements * Replace class enum JsonOptions with struct We may consider turning this into a general-purpose template and using it elsewhere * simplify the extraction of transactionID from Transaction object * Remove obsolete comments * Unconditionally set validated in account_tx output * Minor improvements * Minor fixes --------- Co-authored-by: Chenna Keshava <[email protected]>

Bronek force-pushed the feature/unify_transaction_json branch 5 times, most recently from 3118db5 to 3801519 Compare October 18, 2023 17:02

intelliot mentioned this pull request Oct 19, 2023

Remove tx_history and ledger_header in API version 2 #4759

Merged

7 tasks

intelliot added this to the 2.0 milestone Oct 19, 2023

Bronek force-pushed the feature/unify_transaction_json branch 5 times, most recently from e9881c1 to 83c1934 Compare October 20, 2023 17:18

intelliot added API Change API V2 Testing labels Oct 20, 2023

Bronek force-pushed the feature/unify_transaction_json branch 2 times, most recently from b8b2813 to 43237ff Compare October 23, 2023 18:20

intelliot added the Testable label Oct 23, 2023

Bronek force-pushed the feature/unify_transaction_json branch from 43237ff to 4c9c01e Compare October 24, 2023 10:52

Bronek mentioned this pull request Oct 24, 2023

Add a constant for ripple epoch offset #4563

Closed

Bronek force-pushed the feature/unify_transaction_json branch 2 times, most recently from be2893a to e750918 Compare October 24, 2023 15:45

Bronek changed the title ~~[DRAFT] Unify JSON serialization format of transactions~~ Unify JSON serialization format of transactions Oct 24, 2023

Bronek marked this pull request as ready for review October 24, 2023 19:16

intelliot requested review from ximinez, ckeshava and arihantkothari October 24, 2023 19:41

Bronek force-pushed the feature/unify_transaction_json branch from 0ffd642 to 7f6bf53 Compare October 25, 2023 14:55

Remove include <ranges>

65346dc

intelliot unassigned Bronek Nov 2, 2023

Replace class enum JsonOptions with struct

94c16d8

We may consider turning this into a general-purpose template and using it elsewhere

Bronek force-pushed the feature/unify_transaction_json branch from d5d6af0 to 94c16d8 Compare November 2, 2023 20:43

ckeshava suggested changes Nov 2, 2023

View reviewed changes

ckeshava and others added 3 commits November 2, 2023 21:10

simplify the extraction of transactionID from Transaction object

7770bc5

Remove obsolete comments

b5b1118

Unconditionally set validated in account_tx output

942e209

ckeshava approved these changes Nov 2, 2023

View reviewed changes

Merge branch 'develop' into feature/unify_transaction_json

c1f2ea4

intelliot assigned arihantkothari and unassigned ckeshava Nov 2, 2023

intelliot requested a review from mDuo13 November 2, 2023 22:47

mDuo13 added a commit to XRPLF/xrpl-dev-portal that referenced this pull request Nov 2, 2023

account_tx only returns validated transactions

c0d7cd7

Per XRPLF/rippled#4775 (comment) this command only returns transactions from validated ledgers, so this clarifies that detail

mDuo13 mentioned this pull request Nov 2, 2023

account_tx only returns validated transactions XRPLF/xrpl-dev-portal#2250

Merged

ximinez requested changes Nov 2, 2023

View reviewed changes

Minor improvements

53e384f

Bronek mentioned this pull request Nov 3, 2023

Refactor population of transaction JSON #4801

Open

intelliot mentioned this pull request Nov 3, 2023

Proposed 2.0.0-rc1 #4802

Closed

1 task

ximinez requested changes Nov 3, 2023

View reviewed changes

Minor fixes

d0060f8

ximinez approved these changes Nov 6, 2023

View reviewed changes

arihantkothari approved these changes Nov 7, 2023

View reviewed changes

Merge branch 'develop' into feature/unify_transaction_json

311952f

manojsdoshi merged commit 32ced49 into XRPLF:develop Nov 8, 2023
16 checks passed

This was referenced Nov 13, 2023

Regression: crash when rendering JSON binary transaction with API v.2 enabled #4811

Closed

Fix 2.0 regression in tx method with binary output #4812

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Unify JSON serialization format of transactions #4775

Unify JSON serialization format of transactions #4775

Bronek commented Oct 18, 2023 •

edited

Loading

intelliot commented Oct 24, 2023

Bronek commented Oct 25, 2023

Bronek commented Nov 2, 2023

ckeshava left a comment

Bronek commented Nov 2, 2023

ckeshava left a comment

ximinez left a comment

ximinez Oct 26, 2023

ximinez Oct 26, 2023

ximinez left a comment

arihantkothari Nov 7, 2023

Unify JSON serialization format of transactions #4775

Unify JSON serialization format of transactions #4775

Conversation

Bronek commented Oct 18, 2023 • edited Loading

High Level Overview of Change

Type of Change

API Impact

intelliot commented Oct 24, 2023

Bronek commented Oct 25, 2023

Bronek commented Nov 2, 2023

ckeshava left a comment

Choose a reason for hiding this comment

Bronek commented Nov 2, 2023

ckeshava left a comment

Choose a reason for hiding this comment

ximinez left a comment

Choose a reason for hiding this comment

ximinez Oct 26, 2023

Choose a reason for hiding this comment

ximinez Oct 26, 2023

Choose a reason for hiding this comment

ximinez left a comment

Choose a reason for hiding this comment

arihantkothari Nov 7, 2023

Choose a reason for hiding this comment

Bronek commented Oct 18, 2023 •

edited

Loading