Skip to content
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

Unify JSON serialization format of transactions #4775

Merged
merged 37 commits into from
Nov 8, 2023
Merged
Show file tree
Hide file tree
Changes from 15 commits
Commits
Show all changes
37 commits
Select commit Hold shift + click to select a range
65346dc
Remove include <ranges>
Bronek Oct 25, 2023
ee53c5d
Formatting fix
Bronek Oct 25, 2023
aa6de60
Output for subscriptions
Bronek Oct 23, 2023
76eee5a
Output from sign, submit etc.
Bronek Oct 18, 2023
92635aa
Output from ledger
Bronek Oct 18, 2023
e4ce8b1
Output from account_tx
Bronek Oct 18, 2023
0fb0517
Output from transaction_entry
Bronek Oct 18, 2023
910f125
Output from tx
Bronek Oct 18, 2023
4d517e2
Store close_time_iso in API v2 output
Bronek Oct 23, 2023
2b397c9
Add small APIv2 unit test for subscribe
Bronek Oct 24, 2023
1a96800
Add unit test for transaction_entry
Bronek Oct 24, 2023
c2a3b52
Add unit test for tx
Bronek Oct 24, 2023
bdf90e5
Remove inLedger from API version 2
Bronek Oct 24, 2023
2035bbc
Set ledger_hash and ledger_index
Bronek Oct 24, 2023
1115cef
Move isValidated from RPCHelpers to LedgerMaster
Bronek Oct 26, 2023
4e92ee4
Store closeTime in LedgerFill
Bronek Oct 26, 2023
9f1d544
Time formatting fix
Bronek Oct 30, 2023
e386093
additional tests for Subscribe unit tests
ckeshava Oct 27, 2023
c477eb4
Improved comments
Bronek Oct 30, 2023
c81bece
Rename mInLedger to mLedgerIndex
Bronek Oct 30, 2023
ef1276c
Minor fixes
Bronek Oct 30, 2023
54f1d60
Set ledger_hash on closed ledger, even if not validated
Bronek Oct 30, 2023
625c812
Merge branch 'develop' into feature/unify_transaction_json
Bronek Oct 30, 2023
0d8673c
Update API-CHANGELOG.md
Bronek Oct 31, 2023
cdb3384
Add ledger_hash, ledger_index to transaction_entry
Bronek Oct 31, 2023
5973390
Fix validated and close_time_iso in account_tx
Bronek Nov 1, 2023
17e6588
Fix typos
Bronek Nov 2, 2023
95b6055
Improve getJson for Transaction and STTx
Bronek Nov 2, 2023
b2a8a2d
Minor improvements
Bronek Nov 2, 2023
94c16d8
Replace class enum JsonOptions with struct
Bronek Nov 2, 2023
7770bc5
simplify the extraction of transactionID from Transaction object
ckeshava Nov 2, 2023
b5b1118
Remove obsolete comments
Bronek Nov 2, 2023
942e209
Unconditionally set validated in account_tx output
Bronek Nov 2, 2023
c1f2ea4
Merge branch 'develop' into feature/unify_transaction_json
Bronek Nov 2, 2023
53e384f
Minor improvements
Bronek Nov 3, 2023
d0060f8
Minor fixes
Bronek Nov 6, 2023
311952f
Merge branch 'develop' into feature/unify_transaction_json
Bronek Nov 8, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions src/ripple/app/ledger/LedgerMaster.h
Original file line number Diff line number Diff line change
Expand Up @@ -215,6 +215,8 @@ class LedgerMaster : public AbstractFetchPackContainer
void
clearLedger(std::uint32_t seq);
bool
isValidated(ReadView const& ledger);
bool
getValidatedRange(std::uint32_t& minVal, std::uint32_t& maxVal);
bool
getFullValidatedRange(std::uint32_t& minVal, std::uint32_t& maxVal);
Expand Down
49 changes: 49 additions & 0 deletions src/ripple/app/ledger/impl/LedgerMaster.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -603,6 +603,55 @@ LedgerMaster::clearLedger(std::uint32_t seq)
mCompleteLedgers.erase(seq);
}

bool
LedgerMaster::isValidated(ReadView const& ledger)
{
if (app_.config().reporting())
return haveLedger(ledger.seq()); // TODO Is this correct ?
Bronek marked this conversation as resolved.
Show resolved Hide resolved

if (ledger.open())
return false;

if (ledger.info().validated)
return true;

auto const seq = ledger.info().seq;
try
{
// Use the skip list in the last validated ledger to see if ledger
// comes before the last validated ledger (and thus has been
// validated).
auto hash = walkHashBySeq(seq, InboundLedger::Reason::GENERIC);
ximinez marked this conversation as resolved.
Show resolved Hide resolved

if (!hash || ledger.info().hash != *hash)
{
// This ledger's hash is not the hash of the validated ledger
if (hash)
{
assert(hash->isNonZero());
uint256 valHash =
app_.getRelationalDatabase().getHashByIndex(seq);
if (valHash == ledger.info().hash)
{
// SQL database doesn't match ledger chain
clearLedger(seq);
}
}
return false;
}
}
catch (SHAMapMissingNode const& mn)
{
auto stream = app_.journal("IsValidated").warn(); // TODO Better name ?
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel app_.journal("LedgerMaster::IsValidated") is a descriptive name

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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()).

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not know how these names are rendered, but it appears we almost never use them with :: in the middle ...

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm, i see

JLOG(stream) << "Ledger #" << seq << ": " << mn.what();
return false;
}

// Mark ledger as validated to save time if we see it again.
ledger.info().validated = true;
return true;
}

// returns Ledgers we have all the nodes for
bool
LedgerMaster::getFullValidatedRange(
Expand Down
80 changes: 66 additions & 14 deletions src/ripple/app/ledger/impl/LedgerToJson.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -17,12 +17,14 @@
*/
//==============================================================================

#include <ripple/app/ledger/LedgerMaster.h>
#include <ripple/app/ledger/LedgerToJson.h>
#include <ripple/app/main/Application.h>
#include <ripple/app/misc/DeliverMax.h>
#include <ripple/app/misc/TxQ.h>
#include <ripple/basics/base_uint.h>
#include <ripple/core/Pg.h>
#include <ripple/protocol/jss.h>
#include <ripple/rpc/Context.h>
#include <ripple/rpc/DeliveredAmount.h>

Expand Down Expand Up @@ -83,6 +85,7 @@ fillJson(Object& json, bool closed, LedgerInfo const& info, bool bFull)
json[jss::close_time_human] = to_string(info.closeTime);
if (!getCloseAgree(info))
json[jss::close_time_estimated] = true;
json[jss::close_time_iso] = to_string_iso(info.closeTime);
}
}

Expand Down Expand Up @@ -118,24 +121,69 @@ fillJsonTx(
if (bBinary)
{
txJson[jss::tx_blob] = serializeHex(*txn);
if (fill.context->apiVersion > 1)
txJson[jss::hash] = to_string(txn->getTransactionID());

auto const json_meta =
(fill.context->apiVersion > 1 ? jss::meta_blob : jss::meta);
if (stMeta)
txJson[jss::meta] = serializeHex(*stMeta);
txJson[json_meta] = serializeHex(*stMeta);
}
else
{
copyFrom(txJson, txn->getJson(JsonOptions::none));
RPC::insertDeliverMax(txJson, txnType, fill.context->apiVersion);
if (stMeta)
if (fill.context->apiVersion > 1)
{
ximinez marked this conversation as resolved.
Show resolved Hide resolved
txJson[jss::metaData] = stMeta->getJson(JsonOptions::none);

// If applicable, insert delivered amount
if (txnType == ttPAYMENT || txnType == ttCHECK_CASH)
RPC::insertDeliveredAmount(
txJson[jss::metaData],
fill.ledger,
txn,
{txn->getTransactionID(), fill.ledger.seq(), *stMeta});
std::string hash;
copyFrom(
txJson[jss::tx_json],
txn->getJson(
JsonOptions::none, false, {std::optional(std::ref(hash))}));
txJson[jss::hash] = hash;
RPC::insertDeliverMax(
txJson[jss::tx_json], txnType, fill.context->apiVersion);

if (stMeta)
{
txJson[jss::meta] = stMeta->getJson(JsonOptions::none);

// If applicable, insert delivered amount
if (txnType == ttPAYMENT || txnType == ttCHECK_CASH)
RPC::insertDeliveredAmount(
txJson[jss::meta],
fill.ledger,
txn,
{txn->getTransactionID(), fill.ledger.seq(), *stMeta});
}

const bool validated =
fill.context->ledgerMaster.isValidated(fill.ledger);
txJson[jss::validated] = validated;
if (validated)
{
txJson[jss::ledger_index] = to_string(fill.ledger.seq());
txJson[jss::ledger_hash] = to_string(fill.ledger.info().hash);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As per Recommendations, ledger_hash is expected to be included in closed ledgers. The code includes this field in validated ledgers, not closed ledgers.

Should we update the recommendations to reflect the code?
image

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, I implemented your suggestion.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thank you for looking into it 👍

if (auto close_time =
fill.context->ledgerMaster.getCloseTimeBySeq(
fill.ledger.seq()))
txJson[jss::close_time_iso] = to_string_iso(*close_time);
intelliot marked this conversation as resolved.
Show resolved Hide resolved
}
}
else
{
copyFrom(txJson, txn->getJson(JsonOptions::none));
RPC::insertDeliverMax(txJson, txnType, fill.context->apiVersion);
if (stMeta)
{
txJson[jss::metaData] = stMeta->getJson(JsonOptions::none);

// If applicable, insert delivered amount
if (txnType == ttPAYMENT || txnType == ttCHECK_CASH)
RPC::insertDeliveredAmount(
txJson[jss::metaData],
fill.ledger,
txn,
{txn->getTransactionID(), fill.ledger.seq(), *stMeta});
}
}
}

Expand Down Expand Up @@ -254,7 +302,11 @@ fillJsonQueue(Object& json, LedgerFill const& fill)
if (tx.lastResult)
txJson["last_result"] = transToken(*tx.lastResult);

txJson[jss::tx] = fillJsonTx(fill, bBinary, bExpanded, tx.txn, nullptr);
auto&& temp = fillJsonTx(fill, bBinary, bExpanded, tx.txn, nullptr);
if (fill.context->apiVersion > 1)
copyFrom(txJson, temp);
else
copyFrom(txJson[jss::tx], temp);
}
}

Expand Down
26 changes: 22 additions & 4 deletions src/ripple/app/misc/NetworkOPs.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,7 @@
#include <ripple/protocol/BuildInfo.h>
#include <ripple/protocol/Feature.h>
#include <ripple/protocol/STParsedJSON.h>
#include <ripple/protocol/jss.h>
#include <ripple/resource/Fees.h>
#include <ripple/resource/ResourceManager.h>
#include <ripple/rpc/BookChanges.h>
Expand All @@ -74,6 +75,7 @@

#include <algorithm>
#include <mutex>
#include <optional>
#include <string>
#include <tuple>
#include <unordered_map>
Expand Down Expand Up @@ -3101,7 +3103,12 @@ NetworkOPsImp::transJson(
transResultInfo(result, sToken, sHuman);

jvObj[jss::type] = "transaction";
jvObj[jss::transaction] = transaction->getJson(JsonOptions::none);
// NOTE jvObj which is not a finished object for either API version. After
ximinez marked this conversation as resolved.
Show resolved Hide resolved
// 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 = {};
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel it would be more clear if we separate the serialization logic and data initialization in the getJson() function. hash can be directly initialized here as hash = transaction->getTransactionID(); (instead of passing an optional ref-wrapper)

This is not a strong opinion, an alternative design suggestion, that's all

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe. Trouble is that we do not have flexible design choice here, since Transaction::getJson is used in quite a few locations.

Copy link
Collaborator Author

@Bronek Bronek Nov 2, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, having seen your (good, but unfortunately breaking API version 1) idea to remove ledger_index from inside transaction JSON output, I will try to do that properly both for ledger_index and for transaction hash. Hopefully in this PR, but unsure as this might come with larger churn that I want here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok done! It wasn't so bad - see the latest commit 95b6055

jvObj[jss::transaction] =
transaction->getJson(JsonOptions::none, false, {std::ref(hash)});

if (meta)
{
Expand All @@ -3117,6 +3124,8 @@ NetworkOPsImp::transJson(
jvObj[jss::transaction][jss::date] =
ledger->info().closeTime.time_since_epoch().count();
jvObj[jss::validated] = true;
if (auto close_time = m_ledgerMaster.getCloseTimeBySeq(ledger->seq()))
jvObj[jss::close_time_iso] = to_string_iso(*close_time);

// WRITEME: Put the account next seq here
}
Expand Down Expand Up @@ -3165,11 +3174,20 @@ NetworkOPsImp::transJson(
assert(index < MultiApiJson::size);
if (index != lastIndex)
{
Json::Value& jvTx = multiObj.val[index];
RPC::insertDeliverMax(
multiObj.val[index][jss::transaction],
transaction->getTxnType(),
apiVersion);
jvTx[jss::transaction], transaction->getTxnType(), apiVersion);
lastIndex = index;

if (apiVersion > 1)
{
jvTx[jss::tx_json] = jvTx.removeMember(jss::transaction);
jvTx[jss::hash] = hash;
}
else
{
jvTx[jss::transaction][jss::hash] = hash;
}
}
}

Expand Down
9 changes: 7 additions & 2 deletions src/ripple/app/misc/Transaction.h
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@
#include <ripple/protocol/STTx.h>
#include <ripple/protocol/TER.h>
#include <ripple/protocol/TxMeta.h>
#include <boost/optional.hpp>

#include <optional>
#include <variant>

Expand Down Expand Up @@ -305,8 +305,13 @@ class Transaction : public std::enable_shared_from_this<Transaction>,
validatedLedger, fee, accountSeq, availableSeq);
}

/// Same as similar overload for STTx::getJson
Json::Value
getJson(JsonOptions options, bool binary = false) const;
getJson(
JsonOptions options,
bool binary = false,
bool showInLedger = false,
std::optional<std::reference_wrapper<std::string>> hash = {}) const;

// Information used to locate a transaction.
// Contains a nodestore hash and ledger sequence pair if the transaction was
Expand Down
12 changes: 9 additions & 3 deletions src/ripple/app/misc/impl/Transaction.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -165,13 +165,19 @@ Transaction::load(

// options 1 to include the date of the transaction
Json::Value
Transaction::getJson(JsonOptions options, bool binary) const
Transaction::getJson(
JsonOptions options,
bool binary,
bool showInLedger,
std::optional<std::reference_wrapper<std::string>> hash) const
{
Json::Value ret(mTransaction->getJson(JsonOptions::none, binary));
Json::Value ret(mTransaction->getJson(JsonOptions::none, binary, hash));

if (mInLedger)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since inLedger field is being deprecated, it would be nice to rename the mInLedger data member into mLedgerIndex or something like that. It would help with the readability of the code.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, will do that. In general I do not like mixing refactoring with functional changes, but here it is a clear readability improvement with very small added churn.

{
ret[jss::inLedger] = mInLedger; // Deprecated.
if (showInLedger)
ret[jss::inLedger] = mInLedger; // Deprecated.

ret[jss::ledger_index] = mInLedger;

if (options == JsonOptions::include_date)
Expand Down
33 changes: 31 additions & 2 deletions src/ripple/basics/chrono.h
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
#include <ripple/beast/clock/abstract_clock.h>
#include <ripple/beast/clock/basic_seconds_clock.h>
#include <ripple/beast/clock/manual_clock.h>

#include <chrono>
#include <cstdint>
#include <string>
Expand All @@ -43,8 +44,19 @@ using weeks = std::chrono::
/** Clock for measuring the network time.

The epoch is January 1, 2000
epoch_offset = days(10957); // 2000-01-01

epoch_offset
= date(2000-01-01) - date(1970-0-01)
= days(10957)
= seconds(946684800)
*/

constexpr static std::chrono::seconds epoch_offset =
date::sys_days{date::year{2000} / 1 / 1} -
date::sys_days{date::year{1970} / 1 / 1};

static_assert(epoch_offset.count() == 946684800);

class NetClock
{
public:
Expand All @@ -71,7 +83,24 @@ to_string(NetClock::time_point tp)
// 2000-01-01 00:00:00 UTC is 946684800s from 1970-01-01 00:00:00 UTC
using namespace std::chrono;
return to_string(
system_clock::time_point{tp.time_since_epoch() + 946684800s});
system_clock::time_point{tp.time_since_epoch() + epoch_offset});
}

template <class Duration>
std::string
to_string_iso(date::sys_time<Duration> tp)
{
using namespace std::chrono;
return date::format("%FT%H:%M:%OSZ", tp);
}

inline std::string
to_string_iso(NetClock::time_point tp)
{
// 2000-01-01 00:00:00 UTC is 946684800s from 1970-01-01 00:00:00 UTC
using namespace std::chrono;
return to_string_iso(
system_clock::time_point{tp.time_since_epoch() + epoch_offset});
}

/** A clock for measuring elapsed time.
Expand Down
3 changes: 2 additions & 1 deletion src/ripple/core/TimeKeeper.h
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@

#include <ripple/basics/chrono.h>
#include <ripple/beast/clock/abstract_clock.h>

#include <atomic>

namespace ripple {
Expand All @@ -37,7 +38,7 @@ class TimeKeeper : public beast::abstract_clock<NetClock>
adjust(std::chrono::system_clock::time_point when)
{
return time_point(std::chrono::duration_cast<duration>(
when.time_since_epoch() - days(10957)));
when.time_since_epoch() - epoch_offset));
}

public:
Expand Down
10 changes: 9 additions & 1 deletion src/ripple/protocol/STTx.h
Original file line number Diff line number Diff line change
Expand Up @@ -108,8 +108,16 @@ class STTx final : public STObject, public CountedObject<STTx>

Json::Value
getJson(JsonOptions options) const override;

/// If `hash` is set, will store hash inside the provided string. Otherwise
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it more accurate to say -- "If hash is set, will store transaction ID inside the provided string" ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks , will do that !

/// hash will be stored as nested jss::hash element inside the returned JSON
/// Additionally, if `hash` is set and `binary` is true, will not create
/// nested jss::tx for binary hex; instead will return it as JSON string
Json::Value
getJson(JsonOptions options, bool binary) const;
getJson(
JsonOptions options,
bool binary,
std::optional<std::reference_wrapper<std::string>> hash = {}) const;

void
sign(PublicKey const& publicKey, SecretKey const& secretKey);
Expand Down
Loading
Loading