Skip to content

Commit

Permalink
[FOLD] A collection of suggested changes from review (#24)
Browse files Browse the repository at this point in the history 
Co-authored-by: Scott Schurr <scott@ripple.com>
  • Loading branch information
@dangell7 @scottschurr
dangell7 and scottschurr authored Oct 27, 2023
1 parent 198f006 commit 51d78a1
Show file tree
Hide file tree
Showing 7 changed files with 62 additions and 108 deletions.
2 changes: 1 addition & 1 deletion src/ripple/app/ledger/TransactionMaster.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,7 @@ class TransactionMaster
uint256 const& hash,
std::uint32_t ledger,
std::optional<uint32_t> tseq,
std::optional<uint16_t> netID);
std::optional<uint32_t> netID);

void
canonicalize(std::shared_ptr<Transaction>* pTransaction);
Expand Down
2 changes: 1 addition & 1 deletion src/ripple/app/ledger/impl/TransactionMaster.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ TransactionMaster::inLedger(
uint256 const& hash,
std::uint32_t ledger,
std::optional<uint32_t> tseq,
std::optional<uint16_t> netID)
std::optional<uint32_t> netID)
{
auto txn = mCache.fetch(hash);

Expand Down
12 changes: 4 additions & 8 deletions src/ripple/app/misc/NetworkOPs.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3107,14 +3107,10 @@ NetworkOPsImp::transJson(
if (transaction.isFieldPresent(sfNetworkID))
netID = transaction.getFieldU32(sfNetworkID);

if (txnSeq <= 0xFFFFU && netID < 0xFFFFU &&
ledger->info().seq < 0xFFFFFFFUL)
{
if (std::optional<std::string> ctid =
RPC::encodeCTID(ledger->info().seq, txnSeq, netID);
ctid)
jvObj[jss::ctid] = *ctid;
}
if (std::optional<std::string> ctid =
RPC::encodeCTID(ledger->info().seq, txnSeq, netID);
ctid)
jvObj[jss::ctid] = *ctid;
}

if (validated)
Expand Down
4 changes: 2 additions & 2 deletions src/ripple/app/misc/Transaction.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -131,7 +131,7 @@ class Transaction : public std::enable_shared_from_this<Transaction>,
TransStatus status,
std::uint32_t ledgerSeq,
std::optional<uint32_t> transactionSeq = std::nullopt,
std::optional<uint16_t> networkID = std::nullopt);
std::optional<uint32_t> networkID = std::nullopt);

void
setStatus(TransStatus status)
Expand Down Expand Up @@ -392,7 +392,7 @@ class Transaction : public std::enable_shared_from_this<Transaction>,

LedgerIndex mInLedger = 0;
std::optional<uint32_t> mTxnSeq;
std::optional<uint16_t> mNetworkID;
std::optional<uint32_t> mNetworkID;

TransStatus mStatus = INVALID;
TER mResult = temUNCERTAIN;
Expand Down
5 changes: 2 additions & 3 deletions src/ripple/app/misc/impl/Transaction.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ Transaction::setStatus(
TransStatus ts,
std::uint32_t lseq,
std::optional<std::uint32_t> tseq,
std::optional<std::uint16_t> netID)
std::optional<std::uint32_t> netID)
{
mStatus = ts;
mInLedger = lseq;
Expand Down Expand Up @@ -196,8 +196,7 @@ Transaction::getJson(JsonOptions options, bool binary) const
if (mTransaction->isFieldPresent(sfNetworkID))
netID = mTransaction->getFieldU32(sfNetworkID);

if (mTxnSeq && netID && *mTxnSeq <= 0xFFFFU && *netID < 0xFFFFU &&
mInLedger < 0xFFFFFFFUL)
if (mTxnSeq && netID)
{
std::optional<std::string> ctid =
RPC::encodeCTID(mInLedger, *mTxnSeq, *netID);
Expand Down
20 changes: 13 additions & 7 deletions src/ripple/rpc/CTID.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -30,18 +30,24 @@ namespace ripple {

namespace RPC {

// CTID stands for Compact Transaction ID.
//
// The CTID comes from XLS-15d: Concise Transaction Identifier #34
//
// https://github.com/XRPLF/XRPL-Standards/discussions/34
//
// The Compact Transaction ID provides a way to identify a transaction
// that includes which network the transaction was submitted to.

inline std::optional<std::string>
encodeCTID(
uint32_t ledger_seq,
uint16_t txn_index,
uint16_t network_id) noexcept
encodeCTID(uint32_t ledgerSeq, uint32_t txnIndex, uint32_t networkID) noexcept
{
if (ledger_seq > 0x0FFF'FFFF)
if (ledgerSeq > 0x0FFF'FFFF || txnIndex > 0xFFFF || networkID > 0xFFFF)
return {};

uint64_t ctidValue =
((0xC000'0000ULL + static_cast<uint64_t>(ledger_seq)) << 32) +
(static_cast<uint64_t>(txn_index) << 16) + network_id;
((0xC000'0000ULL + static_cast<uint64_t>(ledgerSeq)) << 32) +
(static_cast<uint64_t>(txnIndex) << 16) + networkID;

std::stringstream buffer;
buffer << std::hex << std::uppercase << std::setfill('0') << std::setw(16)
Expand Down
125 changes: 39 additions & 86 deletions src/test/rpc/Transaction_test.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -564,20 +564,10 @@ class Transaction_test : public beast::unit_test::suite
BEAST_EXPECT(!RPC::encodeCTID(0x1000'0000UL, 0xFFFFU, 0xFFFFU));

// Test case 3: txn_index greater than 0xFFFF
// this test case is impossible in c++ due to the type, left in for
// completeness
auto const expected3 = std::optional<std::string>("CFFFFFFF0000FFFF");
BEAST_EXPECT(
RPC::encodeCTID(0x0FFF'FFFF, (uint16_t)0x10000, 0xFFFF) ==
expected3);
BEAST_EXPECT(!RPC::encodeCTID(0x0FFF'FFFF, 0x1'0000, 0xFFFF));

// Test case 4: network_id greater than 0xFFFF
// this test case is impossible in c++ due to the type, left in for
// completeness
auto const expected4 = std::optional<std::string>("CFFFFFFFFFFF0000");
BEAST_EXPECT(
RPC::encodeCTID(0x0FFF'FFFFUL, 0xFFFFU, (uint16_t)0x1000'0U) ==
expected4);
BEAST_EXPECT(!RPC::encodeCTID(0x0FFF'FFFFUL, 0xFFFFU, 0x1'0000U));

// Test case 5: Valid input values
auto const expected51 =
Expand Down Expand Up @@ -645,10 +635,11 @@ class Transaction_test : public beast::unit_test::suite

using namespace test::jtx;

// test that the ctid AND the hash are in the response
// Use a Compact Transaction Identifier to request a transaction.
for (uint32_t netID : {11111, 65535, 65536})
{
Env env{*this, makeNetworkConfig(11111)};
uint32_t netID = env.app().config().NETWORK_ID;
Env env{*this, makeNetworkConfig(netID)};
BEAST_EXPECT(netID == env.app().config().NETWORK_ID);

auto const alice = Account("alice");
auto const bob = Account("bob");
Expand All @@ -658,39 +649,57 @@ class Transaction_test : public beast::unit_test::suite
env(pay(alice, bob, XRP(10)));
env.close();

auto const ctid = *RPC::encodeCTID(startLegSeq, 0, netID);
auto const ctid = RPC::encodeCTID(startLegSeq, 0, netID);
if (netID > 0xFFFF)
{
// Compact transaction IDs do not support a network ID > 0xFFFF.
BEAST_EXPECT(ctid == std::nullopt);
continue;
}

Json::Value jsonTx;
jsonTx[jss::binary] = false;
jsonTx[jss::ctid] = ctid;
jsonTx[jss::ctid] = *ctid;
jsonTx[jss::id] = 1;
auto const jrr =
env.rpc("json", "tx", to_string(jsonTx))[jss::result];
BEAST_EXPECT(jrr[jss::ctid] == ctid);
BEAST_EXPECT(jrr[jss::hash]);
BEAST_EXPECT(jrr.isMember(jss::hash));
}

// test that if the network is 65535 the ctid is not in the response
// Using a hash to request the transaction, test the network ID
// boundary where the CTID is (not) in the response.
for (uint32_t netID : {2, 1024, 65535, 65536})
{
Env env{*this, makeNetworkConfig(65535)};
uint32_t netID = env.app().config().NETWORK_ID;
Env env{*this, makeNetworkConfig(netID)};
BEAST_EXPECT(netID == env.app().config().NETWORK_ID);

auto const alice = Account("alice");
auto const bob = Account("bob");

auto const startLegSeq = env.current()->info().seq;
env.fund(XRP(10000), alice, bob);
env(pay(alice, bob, XRP(10)));
env.close();

auto const ctid = *RPC::encodeCTID(startLegSeq, 0, netID);
Json::Value jsonTx;
jsonTx[jss::binary] = false;
jsonTx[jss::ctid] = ctid;
jsonTx[jss::id] = 1;
auto const ledgerSeq = env.current()->info().seq;

env(noop(alice), ter(tesSUCCESS));
env.close();

Json::Value params;
params[jss::id] = 1;
auto const hash = env.tx()->getJson(JsonOptions::none)[jss::hash];
params[jss::transaction] = hash;
auto const jrr =
env.rpc("json", "tx", to_string(jsonTx))[jss::result];
BEAST_EXPECT(!jrr[jss::ctid]);
BEAST_EXPECT(jrr[jss::hash]);
env.rpc("json", "tx", to_string(params))[jss::result];
BEAST_EXPECT(jrr[jss::hash] == hash);

BEAST_EXPECT(jrr.isMember(jss::ctid) == (netID <= 0xFFFF));
if (jrr.isMember(jss::ctid))
{
auto const ctid = RPC::encodeCTID(ledgerSeq, 0, netID);
BEAST_EXPECT(jrr[jss::ctid] == *ctid);
}
}

// test the wrong network ID was submitted
Expand Down Expand Up @@ -720,62 +729,6 @@ class Transaction_test : public beast::unit_test::suite
"Wrong network. You should submit this request to a node "
"running on NetworkID: 21338");
}

// test the ctid is returned when network id < 1024
{
Env env{*this, makeNetworkConfig(2)};
uint32_t netID = env.app().config().NETWORK_ID;

auto const alice = Account("alice");
auto const bob = Account("bob");

env.fund(XRP(10000), alice, bob);
env(pay(alice, bob, XRP(10)));
env.close();

auto const ledgerSeq = env.current()->info().seq;

env(noop(alice), ter(tesSUCCESS));
env.close();

Json::Value params;
params[jss::id] = 1;
auto const hash = env.tx()->getJson(JsonOptions::none)[jss::hash];
params[jss::transaction] = hash;
auto const jrr =
env.rpc("json", "tx", to_string(params))[jss::result];
auto const ctid = *RPC::encodeCTID(ledgerSeq, 0, netID);
BEAST_EXPECT(jrr[jss::ctid] == ctid);
BEAST_EXPECT(jrr[jss::hash] == hash);
}

// test the ctid is returned when network id == 1024
{
Env env{*this, makeNetworkConfig(1024)};
uint32_t netID = env.app().config().NETWORK_ID;

auto const alice = Account("alice");
auto const bob = Account("bob");

env.fund(XRP(10000), alice, bob);
env(pay(alice, bob, XRP(10)));
env.close();

auto const ledgerSeq = env.current()->info().seq;

env(noop(alice), ter(tesSUCCESS));
env.close();

Json::Value params;
params[jss::id] = 1;
auto const hash = env.tx()->getJson(JsonOptions::none)[jss::hash];
params[jss::transaction] = hash;
auto const jrr =
env.rpc("json", "tx", to_string(params))[jss::result];
auto const ctid = *RPC::encodeCTID(ledgerSeq, 0, netID);
BEAST_EXPECT(jrr[jss::ctid] == ctid);
BEAST_EXPECT(jrr[jss::hash] == hash);
}
}

public:
Expand Down

0 comments on commit 51d78a1

Please sign in to comment.